[PATCH] drm/i915: Stop using flush_scheduled_work on driver remove

[PATCH] drm/i915: Stop using flush_scheduled_work on driver remove

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Kernel is trying to eliminate callers of flush_scheduled_work so lets
try to accommodate.

We currently call it from intel_modeset_driver_remove_noirq on the driver
remove path but the comment next to it does not tell me what exact work it
wants to flush.

I can spot three (or four) works using the system_wq:

  ..hotplug.reenable_work
  ..hotplug.hotplug_work
  ..psr.dc3co_work
  ..crtc->drrs.work

So if I replace it with intel_hpd_cancel_work() that appears would handle
the first two. What about the other two?

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
I am clueless about the display paths and only send this because Jani
convinced me to send a patch to kick off the discussion. No expectations
whatsoever this is correct or complete.
---
 drivers/gpu/drm/i915/display/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 2d0018ae34b1..0eb72530a003 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -8980,7 +8980,7 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915)
 	intel_unregister_dsm_handler();
 
 	/* flush any delayed tasks or pending work */
-	flush_scheduled_work();
+	intel_hpd_cancel_work(i915);
 
 	intel_hdcp_component_fini(i915);
 
-- 
2.34.1

Re: [PATCH] drm/i915: Stop using flush_scheduled_work on driver remove

[Ville Syrjälä]

On Fri, Sep 23, 2022 at 03:29:34PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Kernel is trying to eliminate callers of flush_scheduled_work so lets
> try to accommodate.
> 
> We currently call it from intel_modeset_driver_remove_noirq on the driver
> remove path but the comment next to it does not tell me what exact work it
> wants to flush.
> 
> I can spot three (or four) works using the system_wq:
> 
>   ..hotplug.reenable_work
>   ..hotplug.hotplug_work

Looks like we at least try to shoot those down via
intel_irq_uninstall()
 ->intel_hpd_cancel_work()
  ->cancel_delayed_work_sync()

But I'm not sure how broken the hpd disable path is here.
I know hpd cancel vs. irq disable has some known ordering
issues during suspend at least, some of which I think may
have gotten fixed recently. But hpd cancel is still a bit
of a mess in general.

Here we at least do cancel all the hpd works after irqs
have been disabled, so I don't think any further flushing
should help with whatever races we have left in there.

>   ..psr.dc3co_work

I think the whole dc3co thing should be disabled atm,
so nothing should ever schedule this. We should
probably garbage collect the whole thing...

>   ..crtc->drrs.work

That one should have been killed in
intel_display_driver_unregister()
 ->drm_atomic_helper_shutdown()
  ->...
   ->intel_drrs_deactivate()
    ->cancel_delayed_work_sync()

> So if I replace it with intel_hpd_cancel_work() that appears would handle
> the first two. What about the other two?

Other stuff that comes to mind is the pps vdd_off work.
But looks like that should get taken down in the
encoder->destroy() hook at the latest (via
intel_mode_config_cleanup()).

psr.work at least has a cancel_work_sync() in intel_psr_disable(),
so should hopefully get killed the same way as drrs.

opregion.asle_work seems to get cancelled from the unregister path.

The ones that look broken to me are dmc.work and fbc underrun_work.

> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> I am clueless about the display paths and only send this because Jani
> convinced me to send a patch to kick off the discussion. No expectations
> whatsoever this is correct or complete.
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 2d0018ae34b1..0eb72530a003 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -8980,7 +8980,7 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915)
>  	intel_unregister_dsm_handler();
>  
>  	/* flush any delayed tasks or pending work */
> -	flush_scheduled_work();
> +	intel_hpd_cancel_work(i915);
>  
>  	intel_hdcp_component_fini(i915);
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/i915: Stop using flush_scheduled_work on driver remove

[Tvrtko Ursulin]

On 23/09/2022 17:16, Ville Syrjälä wrote:
> On Fri, Sep 23, 2022 at 03:29:34PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Kernel is trying to eliminate callers of flush_scheduled_work so lets
>> try to accommodate.
>>
>> We currently call it from intel_modeset_driver_remove_noirq on the driver
>> remove path but the comment next to it does not tell me what exact work it
>> wants to flush.
>>
>> I can spot three (or four) works using the system_wq:
>>
>>    ..hotplug.reenable_work
>>    ..hotplug.hotplug_work
> 
> Looks like we at least try to shoot those down via
> intel_irq_uninstall()
>   ->intel_hpd_cancel_work()
>    ->cancel_delayed_work_sync()
> 
> But I'm not sure how broken the hpd disable path is here.
> I know hpd cancel vs. irq disable has some known ordering
> issues during suspend at least, some of which I think may
> have gotten fixed recently. But hpd cancel is still a bit
> of a mess in general.
> 
> Here we at least do cancel all the hpd works after irqs
> have been disabled, so I don't think any further flushing
> should help with whatever races we have left in there.
> 
>>    ..psr.dc3co_work
> 
> I think the whole dc3co thing should be disabled atm,
> so nothing should ever schedule this. We should
> probably garbage collect the whole thing...
> 
>>    ..crtc->drrs.work
> 
> That one should have been killed in
> intel_display_driver_unregister()
>   ->drm_atomic_helper_shutdown()
>    ->...
>     ->intel_drrs_deactivate()
>      ->cancel_delayed_work_sync()
> 
>> So if I replace it with intel_hpd_cancel_work() that appears would handle
>> the first two. What about the other two?
> 
> Other stuff that comes to mind is the pps vdd_off work.
> But looks like that should get taken down in the
> encoder->destroy() hook at the latest (via
> intel_mode_config_cleanup()).
> 
> psr.work at least has a cancel_work_sync() in intel_psr_disable(),
> so should hopefully get killed the same way as drrs.
> 
> opregion.asle_work seems to get cancelled from the unregister path.
> 
> The ones that look broken to me are dmc.work and fbc underrun_work.

Right, so I missed some and things are a bit more complicated. Okay to 
leave this with you, even if on a backlog?

Regards,

Tvrtko

> 
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> ---
>> I am clueless about the display paths and only send this because Jani
>> convinced me to send a patch to kick off the discussion. No expectations
>> whatsoever this is correct or complete.
>> ---
>>   drivers/gpu/drm/i915/display/intel_display.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 2d0018ae34b1..0eb72530a003 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -8980,7 +8980,7 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915)
>>   	intel_unregister_dsm_handler();
>>   
>>   	/* flush any delayed tasks or pending work */
>> -	flush_scheduled_work();
>> +	intel_hpd_cancel_work(i915);
>>   
>>   	intel_hdcp_component_fini(i915);
>>   
>> -- 
>> 2.34.1
>