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

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

On Mon, Mar 11, 2024 at 05:06:32PM +0200, Imre Deak wrote:
> 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:

Well, I can also go with the noresume version since my plan is to merge
this through drm-xe-next anyway along with the rest of this series.

However I will need to move this to the top of the series,
because xe's noresume is introduced later. And introduce
the xe compat layer version of the intel_runtime_pm_get_noresume()

A stand alone version of this patch with the noresume would break
drm-tip build:

../drivers/gpu/drm/i915/display/intel_display_power.c: In function ‘release_async_put_domains’:
../drivers/gpu/drm/i915/display/intel_display_power.c:649:19: error: implicit declaration of function ‘intel_runtime_pm_get_noresume’; did you mean ‘intel_runtime_pm_get_if_in_use’? [-Werror=implicit-function-declaration]
  649 |         wakeref = intel_runtime_pm_get_noresume(rpm);
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                   intel_runtime_pm_get_if_in_use
make[3]: *** [../drivers/gpu/drm/xe/Makefile:185: drivers/gpu/drm/xe/i915-display/intel_display_power.o] Error 1

> 
> Acked-by: Imre Deak <imre.deak@intel.com>

Thank you.

> 
> > 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