[PATCH v3 17/18] drm/i915/dmc_wl: Do nothing until initialized
Gustavo Sousa
gustavo.sousa at intel.com
Fri Nov 8 13:10:50 UTC 2024
Quoting Luca Coelho (2024-11-08 06:57:11-03:00)
>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.
Okay. Let's go with this solution then.
So, to unblock this series, I decided to send v4 without this and the
other patches related to using HAS_DMC() in HAS_DMC_WAKELOCK(). I'll
send those in a new series.
--
Gustavo Sousa
>
>
>> > > > 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.
More information about the Intel-gfx
mailing list