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.
Regards,
Tvrtko
