| Message ID | 20241107182921.102193-18-gustavo.sousa@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/dmc_wl: Fixes and enablement for Xe3_LPD | expand |
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/ > 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? > Let's implement that via a new field "initialized". Not that, since we > expect __intel_dmc_wl_supported() to be used for most non-static DMC > wakelock functions, let's add a drm_WARN_ONCE() there for when it is > called prior to initialization. s/not that/note that/ > 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. -- Cheers, Luca.
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". > > >> 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 wakelock. Ugh... I don't like the latter very much... But, with those provided, I believe we should be safe. Thoughts? > > >> Let's implement that via a new field "initialized". Not that, since we >> expect __intel_dmc_wl_supported() to be used for most non-static DMC >> wakelock functions, let's add a drm_WARN_ONCE() there for when it is >> called prior to initialization. > > >s/not that/note that/ Thanks! > > >> 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 :-) -- Gustavo Sousa
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". > >> >> >>> 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 :-) 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; -- Gustavo Sousa > wakelock. > >Ugh... I don't like the latter very much... But, with those provided, I >believe we should be safe. > >Thoughts? > >> >> >>> Let's implement that via a new field "initialized". Not that, since we >>> expect __intel_dmc_wl_supported() to be used for most non-static DMC >>> wakelock functions, let's add a drm_WARN_ONCE() there for when it is >>> called prior to initialization. >> >> >>s/not that/note that/ > >Thanks! > >> >> >>> 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 :-) > >-- >Gustavo Sousa
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". > >> >> >>> 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 > wakelock. Well, a hacky way to mitigate this is by checking the DISPLAY_VER() >= 20 before taking the spin lock, since that info is queried in probe_gmdid_display(), which happens at the "no-mmio" phase of driver initialization. By the way, that makes me think: is it too bad to do the same kind of early MMIO via pci_iomap_range() for ICL_DFSM_DMC_DISABLE? We could avoid this whole thing, since we would already have the correct value for HAS_DMC() when i915/xe MMIO functions are called. -- Gustavo Sousa > >Ugh... I don't like the latter very much... But, with those provided, I >believe we should be safe. > >Thoughts? > >> >> >>> Let's implement that via a new field "initialized". Not that, since we >>> expect __intel_dmc_wl_supported() to be used for most non-static DMC >>> wakelock functions, let's add a drm_WARN_ONCE() there for when it is >>> called prior to initialization. >> >> >>s/not that/note that/ > >Thanks! > >> >> >>> 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 :-) > >-- >Gustavo Sousa
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.
On Thu, 2024-11-07 at 17:47 -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) > > > > > > 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 > > wakelock. > > Well, a hacky way to mitigate this is by checking the DISPLAY_VER() >= > 20 before taking the spin lock, since that info is queried in > probe_gmdid_display(), which happens at the "no-mmio" phase of driver > initialization. > > By the way, that makes me think: is it too bad to do the same kind of > early MMIO via pci_iomap_range() for ICL_DFSM_DMC_DISABLE? We could > avoid this whole thing, since we would already have the correct value > for HAS_DMC() when i915/xe MMIO functions are called. I'm not sure it's worth it, but if you feel this would be better, go ahead and show us the code. :) -- Cheers, Luca.
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.
diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c index c164ac6e1ada..aae5ea0c72ff 100644 --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c @@ -264,6 +264,11 @@ static bool intel_dmc_wl_check_range(i915_reg_t reg, u32 dc_state) static bool __intel_dmc_wl_supported(struct intel_display *display) { + struct intel_dmc_wl *wl = &display->wl; + + if (drm_WARN_ON(display->drm, !wl->initialized)) + return false; + return display->params.enable_dmc_wl && intel_dmc_has_payload(display); } @@ -282,6 +287,8 @@ void intel_dmc_wl_init(struct intel_display *display) intel_dmc_wl_sanitize_param(display); + wl->initialized = true; + if (!display->params.enable_dmc_wl) return; @@ -378,6 +385,9 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg) struct intel_dmc_wl *wl = &display->wl; unsigned long flags; + if (!wl->initialized) + return; + if (!__intel_dmc_wl_supported(display)) return; @@ -410,6 +420,9 @@ void intel_dmc_wl_put(struct intel_display *display, i915_reg_t reg) struct intel_dmc_wl *wl = &display->wl; unsigned long flags; + if (!wl->initialized) + return; + if (!__intel_dmc_wl_supported(display)) return; diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.h b/drivers/gpu/drm/i915/display/intel_dmc_wl.h index 147eeb4d8432..06c8b61d7e87 100644 --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.h +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.h @@ -15,6 +15,17 @@ struct intel_display; struct intel_dmc_wl { + /* + * 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. + * + * So we need the initialized field to ensure that we turn the + * get/put routines into a no-op until we have initialized. + */ + bool initialized; spinlock_t lock; /* protects enabled, taken, dc_state and refcount */ bool enabled; bool taken;
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. 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. Let's implement that via a new field "initialized". Not that, since we expect __intel_dmc_wl_supported() to be used for most non-static DMC wakelock functions, let's add a drm_WARN_ONCE() there for when it is called prior to initialization. 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. Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> --- drivers/gpu/drm/i915/display/intel_dmc_wl.c | 13 +++++++++++++ drivers/gpu/drm/i915/display/intel_dmc_wl.h | 11 +++++++++++ 2 files changed, 24 insertions(+)