On Thu, 2023-01-26 at 11:12 -0800, Lucas De Marchi wrote:
> 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@xxxxxxxxx> 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@xxxxxxxxx> 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 don't understand enough to say what these dependencies are, but I
don't see how renaming the symbol, while keeping the type, makes any
difference...
> 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.
Orthogonal, I agree.
> > 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.
The change is really simple. If backporting, or any other kind of
patch shuffling happens, it will be very easy to spot and correct.
The only annoying thing in doing this is in git-blame cases, IMHO,
because it's annoying to track back patches just because the line was
changed from dev_priv to i915.
> > Jani, you mentioned offline you were afraid of some implications on
> > xe if we don't do the sed, what would that be?
One big sed (I like the rip off the bandaid metaphor!) is the easiest
way to do this, I think. Very easy to automate.
Anyway, maybe I should just send the big sed patch, so we can continue
eventual discussions there. We already hijacked Jouni's patch and took
it to a completely unrelated planet. 😉
--
Cheers,
Luca.
