Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm

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

On Thu, Jan 26, 2023 at 08:36:42AM -0800, Lucas De Marchi wrote:
> On Thu, Jan 26, 2023 at 06:05:32PM +0200, Jani Nikula wrote:
> > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
> > > On Thu, 2023-01-26 at 14:11 +0200, Luca Coelho wrote:
> > > > On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote:
> > > > > On Thu, 26 Jan 2023, Luca Coelho <luca@coelho.fi> wrote:
> > > > > > On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
> > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > index 7d4a15a283a0..63b79c611932 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > > > @@ -1559,7 +1559,26 @@ void intel_psr2_disable_plane_sel_fetch(struct intel_plane *plane,
> > > > > > > >  	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > > > > > > > +void intel_psr2_program_plane_sel_fetch_arm(struct intel_plane *plane,
> > > > > > > > +					const struct intel_crtc_state *crtc_state,
> > > > > > > > +					const struct intel_plane_state *plane_state,
> > > > > > > > +					int color_plane)
> > > > > > > > +{
> > > > > > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > > >
> > > > > > Should you use i915 instead of dev_priv? I've heard and read elsewhere
> > > > > > that this is generally a desired change.  Much easier to use always the
> > > > > > same local name for this kind of thing.  Though this file is already
> > > > > > interspersed with both versions...
> > > > >
> > > > > Basically the only reason to use dev_priv for new code is to deal with
> > > > > some register macros that still have implicit dev_priv in
> > > > > them. Otherwise, i915 should be used, and when convenient, dev_priv
> > > > > should be converted to i915 while touching the code anyway (in a
> > > > > separate patch, but while you're there).
> > > > 
> > > > Thanks for the clarification! In this case we're not using any of the
> > > > macros, AFAICT, so I guess it's better to go with i915 already? And I
> > > > think it should even be in this same patch, since it's a new function
> > > > anyway.
> > > > 
> > > > 
> > > > > The implicit dev_priv dependencies in the register macros are a bit
> > > > > annoying to fix, and it's been going slow. In retrospect maybe the right
> > > > > thing would have been to just sed the parameter to all of them
> > > > > everywhere and be done with it for good. Not too late now, I guess, and
> > > > > I'd take the patches in a heartbeat if someone were to step up and do
> > > > > it.
> > > > 
> > > > I see that there is a boatload of register macros using it... I won't
> > > > promise, but I think it would be a good exercise for a n00b like me to
> > > > make this patch, though I already foresee another boatload of conflicts
> > > > with the internal trees and everything...
> > > 
> > > There were actually 10 boatloads of places to change:
> > > 
> > >  187 files changed, 12104 insertions(+), 12104 deletions(-)
> > > 
> > > ...but it _does_ compile. 😄
> > > 
> > > Do you think this is fine? Lots of shuffle, but if you think it's okay,
> > > I can send the patch out now.
> > 
> > Heh, I said I'd take patchES, not everything together! ;)
> > 
> > Rodrigo, Tvrtko, Joonas, thoughts?
> 
> If it's a sed or something that can be automated, I think it could be
> ok as single patch as long as we find the right time to generate it,
> when the trees are in sync.
> 
> I do remember doing a sed s/dev_priv/i915/ (or it was with a cocci
> script, don't remember) a few years ago, and I'm
> glad we are giving up the slow conversion and just ripping the
> bandaid.

Well, I honestly was always in favor of this approach if possible
to automate and all... But I do have 2 big concerns here:

1. If we do this we will never ever remove the implicit dependency

2. there will be so many more failures on automagic stable backports.
We will need to scrutinize all the failures and track if the developers
are really following up on the backports. We are already bad at it btw.

Jani, you mentioned offline you were afraid of some implications on
xe if we don't do the sed, what would that be?

Thanks,
Rodrigo.

> 
> Lucas De Marchi