[PATCH v3 17/18] drm/i915/dmc_wl: Do nothing until initialized

[Luca Coelho]

On Thu, 2024-11-07 at 17:22 -0300, Gustavo Sousa wrote:
> Quoting Gustavo Sousa (2024-11-07 17:14:36-03:00)
> > Quoting Luca Coelho (2024-11-07 16:23:06-03:00)
> > > On Thu, 2024-11-07 at 15:27 -0300, Gustavo Sousa wrote:
> > > > There is a bit of a chicken and egg situation where we depend on runtime
> > > > info to know that DMC and wakelock are supported by the hardware, and
> > > > such information is grabbed via display MMIO functions, which in turns
> > > > call intel_dmc_wl_get() and intel_dmc_wl_put() as part of their regular
> > > > flow.
> > > 
> > > s/which in turns call/which in turn calls/
> > 
> > Thanks!
> > 
> > I'll do
> > 
> >  s/which in turns call/which in turn call/
> > 
> > as the subject for "call" is "display MMIO functions".

Right, I didn't pay much attention and conjugated it with
"information".


> > > > Since we do not expect DC states (and consequently the wakelock
> > > > mechanism) to be enabled until DMC and DMC wakelock software structures
> > > > are initialized, a simple and safe solution to this is to turn
> > > > intel_dmc_wl_get() and intel_dmc_wl_put() into no-op until we have
> > > > properly initialized.
> > > 
> > > 
> > > About "safe" here... Can we be sure this will be race-free?
> > 
> > The initialization is done only once, during driver load. The wakelock
> > will be enabled only at a later moment. So, we are good in that regard.
> > 
> > However, now that you mentioned, yeah, we should also consider that that
> > we do concurrent work during initialization (e.g. loading the DMC).
> > Based on that, we will need to protect "initialized", which means:
> > 
> > - initializing the lock early together with the other ones;
> > - always going for the lock, even for hardware that does not support the
> 
> Oh, to be clear: I meant the spin lock here :-)

Yeah, I got that. :)


> Something along the lines of:
> 
>     diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
>     index 4257cc380475..e6d4f6328c33 100644
>     --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
>     +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
>     @@ -186,6 +186,7 @@ void intel_display_driver_early_probe(struct drm_i915_private *i915)
>      		return;
>      
>      	spin_lock_init(&i915->display.fb_tracking.lock);
>     +	spin_lock_init(&i915->display.wl.lock);
>      	mutex_init(&i915->display.backlight.lock);
>      	mutex_init(&i915->display.audio.mutex);
>      	mutex_init(&i915->display.wm.wm_mutex);
>     diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>     index e43077453a99..bf8d3b04336d 100644
>     --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>     +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c
>     @@ -307,11 +307,11 @@ void intel_dmc_wl_enable(struct intel_display *display, u32 dc_state)
>      	struct intel_dmc_wl *wl = &display->wl;
>      	unsigned long flags;
>      
>     -	if (!__intel_dmc_wl_supported(display))
>     -		return;
>     -
>      	spin_lock_irqsave(&wl->lock, flags);
>      
>     +	if (!__intel_dmc_wl_supported(display))
>     +		goto out_unlock;
>     +
>      	wl->dc_state = dc_state;
>      
>      	if (drm_WARN_ON(display->drm, wl->enabled))
>     @@ -353,13 +353,13 @@ void intel_dmc_wl_disable(struct intel_display *display)
>      	struct intel_dmc_wl *wl = &display->wl;
>      	unsigned long flags;
>      
>     +	spin_lock_irqsave(&wl->lock, flags);
>     +
>      	if (!__intel_dmc_wl_supported(display))
>     -		return;
>     +		goto out_unlock;
>      
>      	flush_delayed_work(&wl->work);
>      
>     -	spin_lock_irqsave(&wl->lock, flags);
>     -
>      	if (drm_WARN_ON(display->drm, !wl->enabled))
>      		goto out_unlock;
>      
>     @@ -389,13 +389,13 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg)
>      	struct intel_dmc_wl *wl = &display->wl;
>      	unsigned long flags;
>      
>     +	spin_lock_irqsave(&wl->lock, flags);
>     +
>      	if (!wl->initialized)
>     -		return;
>     +		goto out_unlock;
>      
>      	if (!__intel_dmc_wl_supported(display))
>     -		return;
>     -
>     -	spin_lock_irqsave(&wl->lock, flags);
>     +		goto out_unlock;
>      
>      	if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg, wl->dc_state))
>      		goto out_unlock;
>     @@ -424,13 +424,13 @@ void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg)
>      	struct intel_dmc_wl *wl = &display->wl;
>      	unsigned long flags;
>      
>     +	spin_lock_irqsave(&wl->lock, flags);
>     +
>      	if (!wl->initialized)
>     -		return;
>     +		goto out_unlock;
>      
>      	if (!__intel_dmc_wl_supported(display))
>     -		return;
>     -
>     -	spin_lock_irqsave(&wl->lock, flags);
>     +		goto out_unlock;
>      
>      	if (i915_mmio_reg_valid(reg) && !intel_dmc_wl_check_range(reg, wl->dc_state))
>      		goto out_unlock;

I think this is the simplest solution.


> >  wakelock.
> > 
> > Ugh... I don't like the latter very much... But, with those provided, I
> > believe we should be safe.
> > 
> > Thoughts?

I don't think it's a huge problem to initialize the spinlock even for
HW that doesn't use it.  Yeah, it's a bit of wasteful in terms of
resources, but I think it's worth it because of the reduced complexity
of the implementation.


> > > > The only exception of functions that can be called before initialization
> > > > are intel_dmc_wl_get() and intel_dmc_wl_put(), so we bail before
> > > > calling __intel_dmc_wl_supported() if not initialized.
> > > > 
> > > > An alternative solution would be to revise MMIO-related stuff in the
> > > > whole driver initialization sequence, but that would possibly come with
> > > > the cost of some added ordering dependencies and complexity to the
> > > > source code.
> > > 
> > > I think this can be kept out of the commit message.  It's not very
> > > clear what you mean and it sounds much more complex than the solution
> > > you implemented.  Unless race can really be an issue here, but then the
> > > whole commit message should be changed to an eventual more complex
> > > solution.
> > 
> > I meant that we would need to revise the initialization code and find
> > the correct place to put the DMC Wakelock software initialization call.
> > That might also come with changes in some places where we do probe the
> > hardware for info:
> > 
> >  - We need our initialization to happen before
> >    intel_display_device_info_runtime_init(), because we want to check
> >    HAS_DMC().
> > 
> >  - Currently, __intel_display_device_info_runtime_init() is using
> >    intel_re_read(), which in turn uses
> >    intel_dmc_wl_get()/intel_dmc_wl_put().
> > 
> >  - The alternative solution to using the "initialized" flag would be to
> >    make sure that function does not use the MMIO functions that take
> >    the DMC wakelock path.
> > 
> >  - However, __intel_display_device_info_runtime_init() is not necessary
> >    the only function that would need to be changed, but rather
> >    basically everything that does MMIO before the initialization!
> > 
> > I hope it is clearer now :-)

Definitely clearer, but I still think it may not be worth it.  The
spinlock solution is very simple and clean, IMHO.  We already have the
spinlock for other stuff, so it aligns well with the existing code.

--
Cheers,
Luca.