On Fri, 2023-01-27 at 16:37 +0200, Jani Nikula wrote: > On Fri, 27 Jan 2023, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote: > > On 26/01/2023 16:05, 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? > > > > IMO if the elimination of implicit dev_priv is not included then I am > > not sure the churn is worth the effort. > > > > I think one trap is that it is easy to assume solving those conflicts is > > easy because there is a script, somewhere, whatever, but one needs to be > > careful with assuming a random person hitting a merge conflict will > > realize there is a script, know where to find it, and know how to use it > > against a state where conflict markers are sitting in their local tree. > > That's a lot of assumed knowledge which my experience tells me is not > > universally there. > > > > Having said all that, I looked at the occurrence histogram for the > > proposed churn and gut feel says conflicts wouldn't even be that bad > > since they seem heavily localized in a handful of files plus the display > > subdir. > > > > Plus it is upstream.. so we are allowed not to care too much about > > backporting woes. I would still hope implicit dev_priv, albeit > > orthogonal, would be coming somewhat together with the rename. For that > > warm fuzzy feeling that the churn was really really worth it. > > I was mostly talking about the implicit dev_priv removal. It's somewhat > easy, because you can always assume dev_priv is around when the macros > in question are used. > > The above is a dependency to any renames. I don't think the renames are > as important as removing the implicit dev_priv, and the renames are > easier to handle piecemeal, say a file at a time or something. I'm trying to write a semantic patch to convert this stuff. But coccinelle is problematic when it comes to macros, so it turned out not to be as trivial as I though. Now that I've been looking at the code more, so I see the issue with the implicit dev_priv in some of the macros. But I think that is really trivial to solve. It shouldn't be an issue to add a parameter to those macros. It will probably need some manual work, but I'm on it and hopefully will be able to send some patches as RFC. -- Cheers, Luca.