[Intel-gfx] [PATCH 1/2] drm/i915/overlay: Fix active retire callback alignment

[Intel-gfx] [PATCH 1/2] drm/i915/overlay: Fix active retire callback alignment

[Tvrtko Ursulin]

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
 intel_overlay_last_flip_retire(struct i915_active *active)
 {
 	struct intel_overlay *overlay =
-- 
2.30.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/2] drm/i915/selftests: Fix active retire callback alignment

[Tvrtko Ursulin]

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>
---
 drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c | 2 +-
 drivers/gpu/drm/i915/selftests/i915_active.c        | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
index b2c369317bf1..fcde223e26ff 100644
--- a/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
+++ b/drivers/gpu/drm/i915/gt/selftest_engine_heartbeat.c
@@ -63,7 +63,7 @@ static void pulse_put(struct pulse *p)
 	kref_put(&p->kref, pulse_free);
 }
 
-static void pulse_retire(struct i915_active *active)
+__i915_active_call static void pulse_retire(struct i915_active *active)
 {
 	pulse_put(container_of(active, struct pulse, active));
 }
diff --git a/drivers/gpu/drm/i915/selftests/i915_active.c b/drivers/gpu/drm/i915/selftests/i915_active.c
index 4002c984c2e0..1aa52b5cc488 100644
--- a/drivers/gpu/drm/i915/selftests/i915_active.c
+++ b/drivers/gpu/drm/i915/selftests/i915_active.c
@@ -51,7 +51,7 @@ static int __live_active(struct i915_active *base)
 	return 0;
 }
 
-static void __live_retire(struct i915_active *base)
+__i915_active_call static void __live_retire(struct i915_active *base)
 {
 	struct live_active *active = container_of(base, typeof(*active), base);
 
-- 
2.30.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915/overlay: Fix active retire callback alignment

[Patchwork]

[-- Attachment #1.1: Type: text/plain, Size: 3831 bytes --]

== Series Details ==

Series: series starting with [1/2] drm/i915/overlay: Fix active retire callback alignment
URL   : https://patchwork.freedesktop.org/series/89637/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_10027 -> Patchwork_20029
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_20029 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_20029, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20029/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_20029:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-tgl-y:           [PASS][1] -> [DMESG-FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10027/fi-tgl-y/igt@i915_selftest@live@gt_heartbeat.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20029/fi-tgl-y/igt@i915_selftest@live@gt_heartbeat.html

  
Known issues
------------

  Here are the changes found in Patchwork_20029 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@runner@aborted:
    - fi-kbl-r:           NOTRUN -> [FAIL][3] ([i915#1569] / [i915#192] / [i915#193] / [i915#194])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20029/fi-kbl-r/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-tgl-u2:          [FAIL][4] ([i915#1888]) -> [PASS][5]
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10027/fi-tgl-u2/igt@gem_exec_suspend@basic-s3.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20029/fi-tgl-u2/igt@gem_exec_suspend@basic-s3.html

  * igt@kms_frontbuffer_tracking@basic:
    - {fi-rkl-11500t}:    [SKIP][6] ([i915#1849] / [i915#3180]) -> [PASS][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10027/fi-rkl-11500t/igt@kms_frontbuffer_tracking@basic.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20029/fi-rkl-11500t/igt@kms_frontbuffer_tracking@basic.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1569]: https://gitlab.freedesktop.org/drm/intel/issues/1569
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#1888]: https://gitlab.freedesktop.org/drm/intel/issues/1888
  [i915#192]: https://gitlab.freedesktop.org/drm/intel/issues/192
  [i915#193]: https://gitlab.freedesktop.org/drm/intel/issues/193
  [i915#194]: https://gitlab.freedesktop.org/drm/intel/issues/194
  [i915#3180]: https://gitlab.freedesktop.org/drm/intel/issues/3180
  [i915#3303]: https://gitlab.freedesktop.org/drm/intel/issues/3303


Participating hosts (44 -> 40)
------------------------------

  Missing    (4): fi-ilk-m540 fi-bsw-cyan fi-bdw-samus fi-hsw-4200u 


Build changes
-------------

  * Linux: CI_DRM_10027 -> Patchwork_20029

  CI-20190529: 20190529
  CI_DRM_10027: 1748cb1e8bdf543570b86f39487b171ad4c1f896 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6077: 126a3f6fc0e97786e2819085efc84e741093aed5 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_20029: 0e99aab4b650ef69d4d0fcb312057f926580db4b @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

0e99aab4b650 drm/i915/selftests: Fix active retire callback alignment
dd8771b757f6 drm/i915/overlay: Fix active retire callback alignment

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20029/index.html

[-- Attachment #1.2: Type: text/html, Size: 4466 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/overlay: Fix active retire callback alignment

[Ville Syrjälä]

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ä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/overlay: Fix active retire callback alignment

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/overlay: Fix active retire callback alignment

[Tvrtko Ursulin]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/overlay: Fix active retire callback alignment

[Tvrtko Ursulin]

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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/overlay: Fix active retire callback alignment

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx