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

[Luca Coelho]

On Thu, 2023-01-26 at 08:36 -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 at 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 at 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.

I first started doing it with coccinelle, but it just required too many
rules to justify it.  My idea was to do it bit by bit with coccinelle
in separate patches, but it didn't look great.

A simple sed seems to have done the job very nicely and, since that's
the case, I do agree that doing it in one patch is the way to go.  I
don't see the reason to artificially break it down into several patches
and manually handle all the interdependencies.

--
Cheers,
Luca.