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

[Imre Deak <imre.deak@intel.com>]

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