Re: [Intel-gfx] [PATCH] drm/i915: Abstract display info away during probe

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Oct 02, 2023 at 07:58:30PM +0300, Jani Nikula wrote:
> On Mon, 02 Oct 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > On Mon, Oct 02, 2023 at 10:41:14AM +0300, Jani Nikula wrote:
> >> On Fri, 29 Sep 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> >> > The goal is to have this function ready for Xe to use
> >> > directly. So, let's use the available macro.
> >> 
> >> Seesm wrong to use DISPLAY_INFO() as an lvalue
> >
> > to be really honestly I don't like that either.
> > I barely like macros, specially used like this.
> >
> >> and I'm not sure why
> >> this wouldn't work as-is.
> >
> > I should probably had collected some logs and added to the
> > commit message. But the thing was that without this assignment,
> > (xe)->info.display was NULL and the memcpy below was exploding
> > with NULL dereference.
> 
> Aww crap. That's because both DISPLAY_INFO() and DISPLAY_RUNTIME_INFO()
> in xe are completely bogus.
> 
> They should be
> 
> #define DISPLAY_INFO(i915)	((i915)->display.info.__device_info)
> #define DISPLAY_RUNTIME_INFO(i915)	(&(i915)->display.info.__runtime_info)
> 
> instead of
> 
> #define DISPLAY_INFO(xe)		((xe)->info.display)
> #define DISPLAY_RUNTIME_INFO(xe)	(&(xe)->info.display_runtime)
> 
> and these should be removed from struct xe_device info member:
> 
> 		const struct intel_display_device_info *display;
> 		struct intel_display_runtime_info display_runtime;

but in this case we would need the macros in Xe to resolve the access
to these items anyway right?!

or how should we handle cases like  'if (xe->info.display_runtime.pipe_mask)' ?



> 
> BR,
> Jani.
> 
> 
> >
> >> 
> >> But *shrug*.
> >> 
> >> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> >
> > thanks, pushed as is.
> >
> >> 
> >> for merging to i915. (xe should come as a backport with cherry-pick -x.)
> >
> > and sent the proper backported cherry-pick to intel-xe ml.
> >
> >> 
> >> BR,
> >> Jani
> >> 
> >> 
> >> >
> >> > Cc: Jani Nikula <jani.nikula@intel.com>
> >> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/display/intel_display_device.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.c b/drivers/gpu/drm/i915/display/intel_display_device.c
> >> > index a6a18eae7ae8..ce55b968e658 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_display_device.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.c
> >> > @@ -926,7 +926,7 @@ void intel_display_device_probe(struct drm_i915_private *i915)
> >> >  	else
> >> >  		info = probe_display(i915);
> >> >  
> >> > -	i915->display.info.__device_info = info;
> >> > +	DISPLAY_INFO(i915) = info;
> >> >  
> >> >  	memcpy(DISPLAY_RUNTIME_INFO(i915),
> >> >  	       &DISPLAY_INFO(i915)->__runtime_defaults,
> >> 
> >> -- 
> >> Jani Nikula, Intel
> 
> -- 
> Jani Nikula, Intel