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

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 02 Oct 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> 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)' ?

Hrmh, we should *not* have code doing direct dereference chases like
that to begin with. :(

I sent a series addressing this. But discovered a bunch of weirdness
around the concepts of "have display" and "display enabled" in xe that
I'm not sure what to do with. It took years to crystallize those
concepts in i915, and xe confuses them again. :(


BR,
Jani.


>
>
>
>> 
>> 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

-- 
Jani Nikula, Intel