| Message ID | 20240307001554.162153-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] drm/i915/display: convert inner wakeref get towards get_if_in_use | expand |
On Thu, Mar 07, 2024 at 09:46:22AM -0500, Rodrigo Vivi wrote: > On Thu, Mar 07, 2024 at 02:30:46AM +0200, Ville Syrjälä wrote: > > On Wed, Mar 06, 2024 at 07:15:45PM -0500, Rodrigo Vivi wrote: > > > This patch brings no functional change. Since at this point of > > > the code we are already asserting a wakeref was held, it means > > > that we are with runtime_pm 'in_use' and in practical terms we > > > are only bumping the pm_runtime usage counter and moving on. > > > > > > However, xe driver has a lockdep annotation that warned us that > > > if a sync resume was actually called at this point, we could have > > > a deadlock because we are inside the power_domains->lock locked > > > area and the resume would call the irq_reset, which would also > > > try to get the power_domains->lock. > > > > > > For this reason, let's convert this call to a safer option and > > > calm lockdep on. > > > > > > Cc: Matthew Auld <matthew.auld@intel.com> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display_power.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > > index 6fd4fa52253a..4c5168a5bbf4 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > > @@ -646,7 +646,7 @@ release_async_put_domains(struct i915_power_domains *power_domains, > > > * power well disabling. > > > */ > > > assert_rpm_raw_wakeref_held(rpm); > > > - wakeref = intel_runtime_pm_get(rpm); > > > + wakeref = intel_runtime_pm_get_if_in_use(rpm); > > > > On first glance that sequence looks like complete nonsense, and > > thus likely to be cleaned up by someone later. > > indeed. as many other things around i915's rpm infra. > > > > > To me _noresume() would seem like the more sensible thing to use > > here. > > well, same effect actually. we would use the _noresume if we > put it without checking if the usage counter was bumped. > However, since our put takes the 'wakeref' into consideration > anyway, let's use this one that is more straight forward for > our current code. > > > And even that might still warrant a comment to explain > > why that one is used specifically. > > In general we grab this inner references when we want to ensure > that we have full control of the situation, i.e. ensuring that the > other reference which we are relying are not dropped while we still > have some operation to do. It is safe to have and cheap, so that's okay. > > > > > I'm also confused by the wakeref vs. wakelock stuff in the runtime pm > > code. Is that there just because not all places track the wakerefs? > > Do we still have those left? > > yeap, those are very nasty and not documented. But looking a bit of > the history and the documentation about our get vs get_raw, it looks > like wakelock only exists so gem/gt side could ensure that gem/gt > side itself is holding the reference, and not relying on some reference > that was actually taken by display. The difference between a wakeref (aka wakelock) and a raw-wakeref is that the former is required for accessing the HW, which is asserted when reading/writing a register. A raw-wakeref is not enough for this and is only taken to prevent runtime suspending, for instance held after dropping a display power reference, until the power well is actually disabled in a delayed manner. During this time any register access is considered invalid. Both wakerefs and raw-wakerefs are tracked. > One thing that crossed my mind many times already is to simply entirely > remove the runtime_pm from display and do like other drivers simply > checking for crtc connection at runtime_idle. > > But then there are places where current display code uses the rpm > in use to take different code paths, and also all the possible impact > with the dc states transitions and other cases that I always gave up > on the thought very quickly. > > But you are right, we will have to comeback and clean things up > one way or another. > > But I wish we can have at least this small change in first so I don't > get blocked by xe's lockdep annotation and I also don't have to > workaround the annotation itself. > > > > > > > > > for_each_power_domain(domain, mask) { > > > /* Clear before put, so put's sanity check is happy. */ > > > -- > > > 2.43.2 > > > > -- > > Ville Syrjälä > > Intel
On Thu, Mar 07, 2024 at 10:14:12PM +0200, Imre Deak wrote: > On Thu, Mar 07, 2024 at 09:46:22AM -0500, Rodrigo Vivi wrote: > > On Thu, Mar 07, 2024 at 02:30:46AM +0200, Ville Syrjälä wrote: > > > On Wed, Mar 06, 2024 at 07:15:45PM -0500, Rodrigo Vivi wrote: > > > > This patch brings no functional change. Since at this point of > > > > the code we are already asserting a wakeref was held, it means > > > > that we are with runtime_pm 'in_use' and in practical terms we > > > > are only bumping the pm_runtime usage counter and moving on. > > > > > > > > However, xe driver has a lockdep annotation that warned us that > > > > if a sync resume was actually called at this point, we could have > > > > a deadlock because we are inside the power_domains->lock locked > > > > area and the resume would call the irq_reset, which would also > > > > try to get the power_domains->lock. > > > > > > > > For this reason, let's convert this call to a safer option and > > > > calm lockdep on. > > > > > > > > Cc: Matthew Auld <matthew.auld@intel.com> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_display_power.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > > > index 6fd4fa52253a..4c5168a5bbf4 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > > > @@ -646,7 +646,7 @@ release_async_put_domains(struct i915_power_domains *power_domains, > > > > * power well disabling. > > > > */ > > > > assert_rpm_raw_wakeref_held(rpm); > > > > - wakeref = intel_runtime_pm_get(rpm); > > > > + wakeref = intel_runtime_pm_get_if_in_use(rpm); > > > > > > On first glance that sequence looks like complete nonsense, and > > > thus likely to be cleaned up by someone later. > > > > indeed. as many other things around i915's rpm infra. > > > > > > > > To me _noresume() would seem like the more sensible thing to use > > > here. > > > > well, same effect actually. we would use the _noresume if we > > put it without checking if the usage counter was bumped. > > However, since our put takes the 'wakeref' into consideration > > anyway, let's use this one that is more straight forward for > > our current code. > > > > > And even that might still warrant a comment to explain > > > why that one is used specifically. > > > > In general we grab this inner references when we want to ensure > > that we have full control of the situation, i.e. ensuring that the > > other reference which we are relying are not dropped while we still > > have some operation to do. It is safe to have and cheap, so that's okay. > > > > > > > > I'm also confused by the wakeref vs. wakelock stuff in the runtime pm > > > code. Is that there just because not all places track the wakerefs? > > > Do we still have those left? > > > > yeap, those are very nasty and not documented. But looking a bit of > > the history and the documentation about our get vs get_raw, it looks > > like wakelock only exists so gem/gt side could ensure that gem/gt > > side itself is holding the reference, and not relying on some reference > > that was actually taken by display. > > The difference between a wakeref (aka wakelock) and a raw-wakeref is > that the former is required for accessing the HW, which is asserted when > reading/writing a register. A raw-wakeref is not enough for this and is > only taken to prevent runtime suspending, for instance held after > dropping a display power reference, until the power well is actually > disabled in a delayed manner. During this time any register access is > considered invalid. ah okay, so it is not just about the GT, but also about MMIO accesses. So the ones in display looks better now. Thanks for this correction. > > Both wakerefs and raw-wakerefs are tracked. Indeed. And also it is worth to say that this patch doesn't introduce any change on that. both intel_runtime_pm_get() and intel_runtime_pm_get_if_in_use() calls intel_runtime_pm_acquire(rpm, true); return track_intel_runtime_pm_wakeref(rpm); so, can we move forward with this change or do you guys see any blocker? Thanks a lot, Rodrigo. > > > One thing that crossed my mind many times already is to simply entirely > > remove the runtime_pm from display and do like other drivers simply > > checking for crtc connection at runtime_idle. > > > > But then there are places where current display code uses the rpm > > in use to take different code paths, and also all the possible impact > > with the dc states transitions and other cases that I always gave up > > on the thought very quickly. > > > > But you are right, we will have to comeback and clean things up > > one way or another. > > > > But I wish we can have at least this small change in first so I don't > > get blocked by xe's lockdep annotation and I also don't have to > > workaround the annotation itself. > > > > > > > > > > > > > for_each_power_domain(domain, mask) { > > > > /* Clear before put, so put's sanity check is happy. */ > > > > -- > > > > 2.43.2 > > > > > > -- > > > Ville Syrjälä > > > Intel
On Fri, Mar 08, 2024 at 10:19:58AM -0500, Rodrigo Vivi wrote: > [...] > > > > The difference between a wakeref (aka wakelock) and a raw-wakeref is > > that the former is required for accessing the HW, which is asserted when > > reading/writing a register. A raw-wakeref is not enough for this and is > > only taken to prevent runtime suspending, for instance held after > > dropping a display power reference, until the power well is actually > > disabled in a delayed manner. During this time any register access is > > considered invalid. > > ah okay, so it is not just about the GT, but also about MMIO accesses. > So the ones in display looks better now. Thanks for this correction. > > > > > Both wakerefs and raw-wakerefs are tracked. > > Indeed. And also it is worth to say that this patch doesn't introduce > any change on that. > > both > intel_runtime_pm_get() > and > intel_runtime_pm_get_if_in_use() > > calls > intel_runtime_pm_acquire(rpm, true); > return track_intel_runtime_pm_wakeref(rpm); > > so, can we move forward with this change or do you guys see any blocker? I also think intel_runtime_pm_get_noresume() would be more logical here, as it's already known that rpm->usecount is non-zero, intel_runtime_pm_get_if_in_use() also works though. Either way: Acked-by: Imre Deak <imre.deak@intel.com> > Thanks a lot, > Rodrigo. > > > > > > One thing that crossed my mind many times already is to simply entirely > > > remove the runtime_pm from display and do like other drivers simply > > > checking for crtc connection at runtime_idle. > > > > > > But then there are places where current display code uses the rpm > > > in use to take different code paths, and also all the possible impact > > > with the dc states transitions and other cases that I always gave up > > > on the thought very quickly. > > > > > > But you are right, we will have to comeback and clean things up > > > one way or another. > > > > > > But I wish we can have at least this small change in first so I don't > > > get blocked by xe's lockdep annotation and I also don't have to > > > workaround the annotation itself. > > > > > > > > > > > > > > > > > for_each_power_domain(domain, mask) { > > > > > /* Clear before put, so put's sanity check is happy. */ > > > > > -- > > > > > 2.43.2 > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel
On Mon, Mar 11, 2024 at 05:06:32PM +0200, Imre Deak wrote: > On Fri, Mar 08, 2024 at 10:19:58AM -0500, Rodrigo Vivi wrote: > > [...] > > > > > > The difference between a wakeref (aka wakelock) and a raw-wakeref is > > > that the former is required for accessing the HW, which is asserted when > > > reading/writing a register. A raw-wakeref is not enough for this and is > > > only taken to prevent runtime suspending, for instance held after > > > dropping a display power reference, until the power well is actually > > > disabled in a delayed manner. During this time any register access is > > > considered invalid. > > > > ah okay, so it is not just about the GT, but also about MMIO accesses. > > So the ones in display looks better now. Thanks for this correction. > > > > > > > > Both wakerefs and raw-wakerefs are tracked. > > > > Indeed. And also it is worth to say that this patch doesn't introduce > > any change on that. > > > > both > > intel_runtime_pm_get() > > and > > intel_runtime_pm_get_if_in_use() > > > > calls > > intel_runtime_pm_acquire(rpm, true); > > return track_intel_runtime_pm_wakeref(rpm); > > > > so, can we move forward with this change or do you guys see any blocker? > > I also think intel_runtime_pm_get_noresume() would be more logical here, > as it's already known that rpm->usecount is non-zero, > intel_runtime_pm_get_if_in_use() also works though. Either way: Well, I can also go with the noresume version since my plan is to merge this through drm-xe-next anyway along with the rest of this series. However I will need to move this to the top of the series, because xe's noresume is introduced later. And introduce the xe compat layer version of the intel_runtime_pm_get_noresume() A stand alone version of this patch with the noresume would break drm-tip build: ../drivers/gpu/drm/i915/display/intel_display_power.c: In function ‘release_async_put_domains’: ../drivers/gpu/drm/i915/display/intel_display_power.c:649:19: error: implicit declaration of function ‘intel_runtime_pm_get_noresume’; did you mean ‘intel_runtime_pm_get_if_in_use’? [-Werror=implicit-function-declaration] 649 | wakeref = intel_runtime_pm_get_noresume(rpm); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | intel_runtime_pm_get_if_in_use make[3]: *** [../drivers/gpu/drm/xe/Makefile:185: drivers/gpu/drm/xe/i915-display/intel_display_power.o] Error 1 > > Acked-by: Imre Deak <imre.deak@intel.com> Thank you. > > > Thanks a lot, > > Rodrigo. > > > > > > > > > One thing that crossed my mind many times already is to simply entirely > > > > remove the runtime_pm from display and do like other drivers simply > > > > checking for crtc connection at runtime_idle. > > > > > > > > But then there are places where current display code uses the rpm > > > > in use to take different code paths, and also all the possible impact > > > > with the dc states transitions and other cases that I always gave up > > > > on the thought very quickly. > > > > > > > > But you are right, we will have to comeback and clean things up > > > > one way or another. > > > > > > > > But I wish we can have at least this small change in first so I don't > > > > get blocked by xe's lockdep annotation and I also don't have to > > > > workaround the annotation itself. > > > > > > > > > > > > > > > > > > > > > for_each_power_domain(domain, mask) { > > > > > > /* Clear before put, so put's sanity check is happy. */ > > > > > > -- > > > > > > 2.43.2 > > > > > > > > > > -- > > > > > Ville Syrjälä > > > > > Intel
On Thu, Mar 07, 2024 at 09:46:22AM -0500, Rodrigo Vivi wrote: > On Thu, Mar 07, 2024 at 02:30:46AM +0200, Ville Syrjälä wrote: > > On Wed, Mar 06, 2024 at 07:15:45PM -0500, Rodrigo Vivi wrote: > > > This patch brings no functional change. Since at this point of > > > the code we are already asserting a wakeref was held, it means > > > that we are with runtime_pm 'in_use' and in practical terms we > > > are only bumping the pm_runtime usage counter and moving on. > > > > > > However, xe driver has a lockdep annotation that warned us that > > > if a sync resume was actually called at this point, we could have > > > a deadlock because we are inside the power_domains->lock locked > > > area and the resume would call the irq_reset, which would also > > > try to get the power_domains->lock. > > > > > > For this reason, let's convert this call to a safer option and > > > calm lockdep on. > > > > > > Cc: Matthew Auld <matthew.auld@intel.com> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display_power.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c > > > index 6fd4fa52253a..4c5168a5bbf4 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c > > > @@ -646,7 +646,7 @@ release_async_put_domains(struct i915_power_domains *power_domains, > > > * power well disabling. > > > */ > > > assert_rpm_raw_wakeref_held(rpm); > > > - wakeref = intel_runtime_pm_get(rpm); > > > + wakeref = intel_runtime_pm_get_if_in_use(rpm); > > > > On first glance that sequence looks like complete nonsense, and > > thus likely to be cleaned up by someone later. > > indeed. as many other things around i915's rpm infra. > > > > > To me _noresume() would seem like the more sensible thing to use > > here. > > well, same effect actually. we would use the _noresume if we > put it without checking if the usage counter was bumped. > However, since our put takes the 'wakeref' into consideration > anyway, let's use this one that is more straight forward for > our current code. > > > And even that might still warrant a comment to explain > > why that one is used specifically. > > In general we grab this inner references when we want to ensure > that we have full control of the situation, i.e. ensuring that the > other reference which we are relying are not dropped while we still > have some operation to do. It is safe to have and cheap, so that's okay. > > > > > I'm also confused by the wakeref vs. wakelock stuff in the runtime pm > > code. Is that there just because not all places track the wakerefs? > > Do we still have those left? > > yeap, those are very nasty and not documented. But looking a bit of > the history and the documentation about our get vs get_raw, it looks > like wakelock only exists so gem/gt side could ensure that gem/gt > side itself is holding the reference, and not relying on some reference > that was actually taken by display. > > One thing that crossed my mind many times already is to simply entirely > remove the runtime_pm from display and do like other drivers simply > checking for crtc connection at runtime_idle. > > But then there are places where current display code uses the rpm > in use to take different code paths, and also all the possible impact > with the dc states transitions and other cases that I always gave up > on the thought very quickly. Yeah. IMO that weird "are we there yet?" approach to runtime pm can only really work for trivial devices (eg. maybe input devices). For any device with any kind of real complexity you have a lot of entrypoints where you need to access the hardware and thus need to make sure it's awake. There's no way to the idle callback can check all of it without making a huge mess of everything.
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c index 6fd4fa52253a..4c5168a5bbf4 100644 --- a/drivers/gpu/drm/i915/display/intel_display_power.c +++ b/drivers/gpu/drm/i915/display/intel_display_power.c @@ -646,7 +646,7 @@ release_async_put_domains(struct i915_power_domains *power_domains, * power well disabling. */ assert_rpm_raw_wakeref_held(rpm); - wakeref = intel_runtime_pm_get(rpm); + wakeref = intel_runtime_pm_get_if_in_use(rpm); for_each_power_domain(domain, mask) { /* Clear before put, so put's sanity check is happy. */
This patch brings no functional change. Since at this point of the code we are already asserting a wakeref was held, it means that we are with runtime_pm 'in_use' and in practical terms we are only bumping the pm_runtime usage counter and moving on. However, xe driver has a lockdep annotation that warned us that if a sync resume was actually called at this point, we could have a deadlock because we are inside the power_domains->lock locked area and the resume would call the irq_reset, which would also try to get the power_domains->lock. For this reason, let's convert this call to a safer option and calm lockdep on. Cc: Matthew Auld <matthew.auld@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/display/intel_display_power.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)