On Fri, 2023-01-27 at 11:33 -0800, Lucas De Marchi wrote:
> On Fri, Jan 27, 2023 at 07:12:29PM +0200, Luca Coelho wrote:
> > On Fri, 2023-01-27 at 16:37 +0200, Jani Nikula wrote:
> > > On Fri, 27 Jan 2023, Tvrtko Ursulin <tvrtko.ursu...@linux.intel.com>
> > > wrote:
> > > > On 26/01/2023 16:05, Jani Nikula wrote:
> > > > > On Thu, 26 Jan 2023, Luca Coelho <l...@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 <l...@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?
> > > >
> > > > 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.
>
> I think that the definition in the header is easier to do manually and
> let coccinelle change only the users. I started this and it seems to be
> going the right direction:
>
> 2 prerequisite commits:
> https://git.kernel.org/pub/scm/linux/kernel/git/demarchi/linux.git/log/?h=tip-drm-intel-dev-priv
>
> $ cat /tmp/a.cocci
> virtual patch
>
> @@
> expression e;
> @@
> - DPLL(e)
> + DPLL(dev_priv, e)
>
> @@
> expression e;
> @@
> - DPLL_MD(e)
> + DPLL_MD(dev_priv, e)
>
> @@
> expression e1, e2;
> @@
> - PALETTE(e1, e2)
> + PALETTE(dev_priv, e1, e2)
>
> ... simply continuing with the same pattern for the other macros
> I *think* would produce a good result. I slightly tested it with
> `make coccicheck MODE=patch COCCI=/tmp/a.cocci M=./drivers/gpu/drm/i915`
Yes, this probably works, but it is more "seddy" than semantic. My
goal was to find rules that will change all these occurrences without
having to specify each different patch individually. ;)
> Then if we change the macro in i915_reg.h we could remove all the
> implicit deps. Wether we should pass dev_priv or mmio_base I think will
> vary from macro to macro. The rename s/dev_priv/i915/ being the cherry
> on top.
Yes, the changes are a bit independent, but we'll have to do one on top
of the other, of course.
--
Cheers,
Luca.
