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

[Lucas De Marchi]

On Thu, Jan 26, 2023 at 01:34:40PM -0500, Rodrigo Vivi wrote:
>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 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.
>
>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

why? it's pretty easy to see what are the macros with implicity
dependencies, regardless if that implicit dep is called dev_priv or
i915.  Fixing the implicit dependency is the nasty part as it will
touch a lot of places with hard-to-automate-patches.

I still think these are orthogonal issues and we shouldn't block the
dev_priv->i915 rename due to that.

Anyway, I will take a look on what an automated removal of the implicit
dependency would look like.

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

an stable backport that fails to build due to that but that has a script
to run to fix things up, isn't that bad.

Lucas De Marchi

>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