Re: [PATCH 01/10] drm/i915/display: convert inner wakeref get towards get_if_in_use

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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