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

[Rodrigo Vivi]

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 at intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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.

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