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

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

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