diff mbox series

[1/2] drm/i915/overlay: Fix active retire callback alignment

Message ID 20210429083530.849546-1-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/i915/overlay: Fix active retire callback alignment | expand

Commit Message

Tvrtko Ursulin April 29, 2021, 8:35 a.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

__i915_active_call annotation is required on the retire callback to ensure
correct function alignment.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Fixes: a21ce8ad12d2 ("drm/i915/overlay: Switch to using i915_active tracking")
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/display/intel_overlay.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ville Syrjälä April 29, 2021, 4:31 p.m. UTC | #1 
On Thu, Apr 29, 2021 at 09:35:29AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> __i915_active_call annotation is required on the retire callback to ensure
> correct function alignment.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Fixes: a21ce8ad12d2 ("drm/i915/overlay: Switch to using i915_active tracking")
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_overlay.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
> index fffbde4256db..428819ba18dd 100644
> --- a/drivers/gpu/drm/i915/display/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
> @@ -383,7 +383,7 @@ static void intel_overlay_off_tail(struct intel_overlay *overlay)
>  		i830_overlay_clock_gating(dev_priv, true);
>  }
>  
> -static void
> +__i915_active_call static void

Am I blind or are we just packing flag bits into a pointer, passing
that to a function, and then immediately unpack the bits again in
said function? Why not just pass the flags explicitly?

Looks like you missed auto_retire()?

>  intel_overlay_last_flip_retire(struct i915_active *active)
>  {
>  	struct intel_overlay *overlay =
> -- 
> 2.30.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Ville Syrjälä April 29, 2021, 5:31 p.m. UTC | #2 
On Thu, Apr 29, 2021 at 07:31:43PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 29, 2021 at 09:35:29AM +0100, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > __i915_active_call annotation is required on the retire callback to ensure
> > correct function alignment.
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Fixes: a21ce8ad12d2 ("drm/i915/overlay: Switch to using i915_active tracking")
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Matthew Auld <matthew.auld@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_overlay.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
> > index fffbde4256db..428819ba18dd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_overlay.c
> > +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
> > @@ -383,7 +383,7 @@ static void intel_overlay_off_tail(struct intel_overlay *overlay)
> >  		i830_overlay_clock_gating(dev_priv, true);
> >  }
> >  
> > -static void
> > +__i915_active_call static void
> 
> Am I blind or are we just packing flag bits into a pointer, passing
> that to a function, and then immediately unpack the bits again in
> said function? Why not just pass the flags explicitly?
> 
> Looks like you missed auto_retire()?

Ah, just saw the other patch from Stéphane.

For the series:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> >  intel_overlay_last_flip_retire(struct i915_active *active)
> >  {
> >  	struct intel_overlay *overlay =
> > -- 
> > 2.30.2
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Tvrtko Ursulin April 29, 2021, 5:34 p.m. UTC | #3 
On 29/04/2021 17:31, Ville Syrjälä wrote:
> On Thu, Apr 29, 2021 at 09:35:29AM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> __i915_active_call annotation is required on the retire callback to ensure
>> correct function alignment.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Fixes: a21ce8ad12d2 ("drm/i915/overlay: Switch to using i915_active tracking")
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Matthew Auld <matthew.auld@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_overlay.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
>> index fffbde4256db..428819ba18dd 100644
>> --- a/drivers/gpu/drm/i915/display/intel_overlay.c
>> +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
>> @@ -383,7 +383,7 @@ static void intel_overlay_off_tail(struct intel_overlay *overlay)
>>   		i830_overlay_clock_gating(dev_priv, true);
>>   }
>>   
>> -static void
>> +__i915_active_call static void
> 
> Am I blind or are we just packing flag bits into a pointer, passing
> that to a function, and then immediately unpack the bits again in
> said function? Why not just pass the flags explicitly?
> 
> Looks like you missed auto_retire()?

Yeah, both points already either fixed or under consideration: 
https://patchwork.freedesktop.org/patch/431473/?series=89623&rev=1

I left the splitting up vfunc vs flags for later.

Regards,

Tvrtko

>>   intel_overlay_last_flip_retire(struct i915_active *active)
>>   {
>>   	struct intel_overlay *overlay =
>> -- 
>> 2.30.2
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Tvrtko Ursulin April 30, 2021, 11:14 a.m. UTC | #4 
On 29/04/2021 18:31, Ville Syrjälä wrote:
> On Thu, Apr 29, 2021 at 07:31:43PM +0300, Ville Syrjälä wrote:
>> On Thu, Apr 29, 2021 at 09:35:29AM +0100, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> __i915_active_call annotation is required on the retire callback to ensure
>>> correct function alignment.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Fixes: a21ce8ad12d2 ("drm/i915/overlay: Switch to using i915_active tracking")
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/display/intel_overlay.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
>>> index fffbde4256db..428819ba18dd 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_overlay.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
>>> @@ -383,7 +383,7 @@ static void intel_overlay_off_tail(struct intel_overlay *overlay)
>>>   		i830_overlay_clock_gating(dev_priv, true);
>>>   }
>>>   
>>> -static void
>>> +__i915_active_call static void
>>
>> Am I blind or are we just packing flag bits into a pointer, passing
>> that to a function, and then immediately unpack the bits again in
>> said function? Why not just pass the flags explicitly?
>>
>> Looks like you missed auto_retire()?
> 
> Ah, just saw the other patch from Stéphane.
> 
> For the series:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks Ville, pushed.

Regards,

Tvrtko
Daniel Vetter April 30, 2021, 11:52 a.m. UTC | #5 
On Thu, Apr 29, 2021 at 06:34:51PM +0100, Tvrtko Ursulin wrote:
> 
> On 29/04/2021 17:31, Ville Syrjälä wrote:
> > On Thu, Apr 29, 2021 at 09:35:29AM +0100, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > 
> > > __i915_active_call annotation is required on the retire callback to ensure
> > > correct function alignment.
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Fixes: a21ce8ad12d2 ("drm/i915/overlay: Switch to using i915_active tracking")
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Matthew Auld <matthew.auld@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/display/intel_overlay.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
> > > index fffbde4256db..428819ba18dd 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_overlay.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
> > > @@ -383,7 +383,7 @@ static void intel_overlay_off_tail(struct intel_overlay *overlay)
> > >   		i830_overlay_clock_gating(dev_priv, true);
> > >   }
> > > -static void
> > > +__i915_active_call static void
> > 
> > Am I blind or are we just packing flag bits into a pointer, passing
> > that to a function, and then immediately unpack the bits again in
> > said function? Why not just pass the flags explicitly?
> > 
> > Looks like you missed auto_retire()?
> 
> Yeah, both points already either fixed or under consideration:
> https://patchwork.freedesktop.org/patch/431473/?series=89623&rev=1
> 
> I left the splitting up vfunc vs flags for later.

Yeah pls remove this pointer packing asap.

This is yet another case of pointless complications and fragility in the
code base for not reason at all, and it needs to go.

I'll file a jira and assign to Matt Auld, since he reviewed this
originally. I'll ping you in case you want to take it over.

Thanks, Daniel

> 
> Regards,
> 
> Tvrtko
> 
> > >   intel_overlay_last_flip_retire(struct i915_active *active)
> > >   {
> > >   	struct intel_overlay *overlay =
> > > -- 
> > > 2.30.2
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
index fffbde4256db..428819ba18dd 100644
--- a/drivers/gpu/drm/i915/display/intel_overlay.c
+++ b/drivers/gpu/drm/i915/display/intel_overlay.c
@@ -383,7 +383,7 @@  static void intel_overlay_off_tail(struct intel_overlay *overlay)
 		i830_overlay_clock_gating(dev_priv, true);
 }
 
-static void
+__i915_active_call static void
 intel_overlay_last_flip_retire(struct i915_active *active)
 {
 	struct intel_overlay *overlay =