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

["Ville Syrjälä" <ville.syrjala@linux.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.
> 
> 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.

-- 
Ville Syrjälä
Intel