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

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

[Rodrigo Vivi]

The goal is to have this function ready for Xe to use
directly. So, let's use the available macro.

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

[Intel-xe] ✗ CI.Patch_applied: failure for drm/i915: Abstract display info away during probe

[Patchwork]

== Series Details ==

Series: drm/i915: Abstract display info away during probe
URL   : https://patchwork.freedesktop.org/series/124473/
State : failure

== Summary ==

=== Applying kernel patches on branch 'drm-xe-next' with base: ===
Base commit: 22fcf2404 drm/xe: timeout needs to be a signed value
=== git am output follows ===
error: patch failed: drivers/gpu/drm/i915/display/intel_display_device.c:926
error: drivers/gpu/drm/i915/display/intel_display_device.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch
Applying: drm/i915: Abstract display info away during probe
Patch failed at 0001 drm/i915: Abstract display info away during probe
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

[Jani Nikula]

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, and I'm not sure why
this wouldn't work as-is.

But *shrug*.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

for merging to i915. (xe should come as a backport with cherry-pick -x.)

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

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

[Rodrigo Vivi]

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.

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

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

[Jani Nikula]

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;

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

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

[Rodrigo Vivi]

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

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

[Jani Nikula]

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