Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
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
> > > wrote:
> > > > On 26/01/2023 16:05, Jani Nikula wrote:
> > > > > On Thu, 26 Jan 2023, Luca Coelho 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 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
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
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 wrote:
> On 26/01/2023 16:05, Jani Nikula wrote:
> > On Thu, 26 Jan 2023, Luca Coelho 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 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
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
On Fri, 2023-01-27 at 16:37 +0200, Jani Nikula wrote:
> On Fri, 27 Jan 2023, Tvrtko Ursulin wrote:
> > On 26/01/2023 16:05, Jani Nikula wrote:
> > > On Thu, 26 Jan 2023, Luca Coelho 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 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
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
On Fri, 27 Jan 2023, Tvrtko Ursulin wrote:
> On 26/01/2023 16:05, Jani Nikula wrote:
>> On Thu, 26 Jan 2023, Luca Coelho 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 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.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
On 26/01/2023 16:05, Jani Nikula wrote:
On Thu, 26 Jan 2023, Luca Coelho 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 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
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
On Thu, 2023-01-26 at 13:01 +, Govindapillai, Vinod wrote:
> On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
> > SEL_FETCH_CTL registers are armed immediately when plane is
> > disabled.
> > SEL_FETCH_* instances of plane configuration are used when doing
> > selective update and normal plane register instances for full
> > updates.
> > Currently all SEL_FETCH_* registers are written as a part of noarm
> > plane configuration. If noarm and arm plane configuration are not
> > happening within same vblank we may end up having plane as a part
> > of
> > selective update before it's PLANE_SURF register is written.
> >
> > Fix this by splitting plane selective fetch configuration into arm
> > and
> > noarm versions and call them accordingly. Write SEL_FETCH_CTL in
> > arm
> > version.
> >
> > Cc: Ville Syrjälä
> > Cc: José Roberto de Souza
> > Cc: Mika Kahola
> > Cc: Vinod Govindapillai
> > Cc: Stanislav Lisovskiy
> > Signed-off-by: Jouni Högander
> > ---
> > drivers/gpu/drm/i915/display/intel_cursor.c | 2 +-
> > drivers/gpu/drm/i915/display/intel_psr.c | 29 ++-
> >
> > drivers/gpu/drm/i915/display/intel_psr.h | 6 +++-
> > .../drm/i915/display/skl_universal_plane.c | 4 ++-
> > 4 files changed, 30 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> > b/drivers/gpu/drm/i915/display/intel_cursor.c
> > index d190fa0d393b..50232cec48e0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > @@ -532,7 +532,7 @@ static void i9xx_cursor_update_arm(struct
> > intel_plane *plane,
> > skl_write_cursor_wm(plane, crtc_state);
> >
> > if (plane_state)
> > - intel_psr2_program_plane_sel_fetch(plane,
> > crtc_state, plane_state, 0);
> > + intel_psr2_program_plane_sel_fetch_arm(plane,
> > crtc_state, plane_state, 0);
>
> > else
> > intel_psr2_disable_plane_sel_fetch(plane,
> > crtc_state);
> >
> > 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)
> Looks like color_plane is redundant here.
>
> Otherwise, looks good to me.
Thank you Vinod for checking my patch. There is a new version
addressing your comment.
>
> Reviewed-by: Vinod Govindapillai
>
> > +{
> > + struct drm_i915_private *dev_priv = to_i915(plane-
> > >base.dev);
> > + enum pipe pipe = plane->pipe;
> > +
> > + if (!crtc_state->enable_psr2_sel_fetch)
> > + return;
> > +
> > + if (plane->id == PLANE_CURSOR)
> > + intel_de_write_fw(dev_priv,
> > PLANE_SEL_FETCH_CTL(pipe, plane->id),
> > + plane_state->ctl);
> > + else
> > + intel_de_write_fw(dev_priv,
> > PLANE_SEL_FETCH_CTL(pipe, plane->id),
> > + PLANE_SEL_FETCH_CTL_ENABLE);
> > +}
> > +
> > +void intel_psr2_program_plane_sel_fetch_noarm(struct intel_plane
> > *plane,
> > const struct
> > intel_crtc_state *crtc_state,
> > const struct
> > intel_plane_state *plane_state,
> > int color_plane)
> > @@ -1573,11 +1592,8 @@ void
> > intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > if (!crtc_state->enable_psr2_sel_fetch)
> > return;
> >
> > - if (plane->id == PLANE_CURSOR) {
> > - intel_de_write_fw(dev_priv,
> > PLANE_SEL_FETCH_CTL(pipe, plane->id),
> > - plane_state->ctl);
> > + if (plane->id == PLANE_CURSOR)
> > return;
> > - }
> >
> > clip = _state->psr2_sel_fetch_area;
> >
> > @@ -1605,9 +1621,6 @@ void
> > intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > val = (drm_rect_height(clip) - 1) << 16;
> > val |= (drm_rect_width(_state->uapi.src) >> 16) - 1;
> > intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe,
> > plane->id), val);
> > -
> > - intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe,
> > plane->id),
> > -
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
On Thu, 2023-01-26 at 13:29 +0200, Luca Coelho wrote:
> On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
> > > SEL_FETCH_CTL registers are armed immediately when plane is
> > > disabled.
> > > SEL_FETCH_* instances of plane configuration are used when doing
> > > selective update and normal plane register instances for full
> > > updates.
> > > Currently all SEL_FETCH_* registers are written as a part of
> > > noarm
> > > plane configuration. If noarm and arm plane configuration are not
> > > happening within same vblank we may end up having plane as a part
> > > of
> > > selective update before it's PLANE_SURF register is written.
> > >
> > > Fix this by splitting plane selective fetch configuration into
> > > arm and
> > > noarm versions and call them accordingly. Write SEL_FETCH_CTL in
> > > arm
> > > version.
> > >
> > > Cc: Ville Syrjälä
> > > Cc: José Roberto de Souza
> > > Cc: Mika Kahola
> > > Cc: Vinod Govindapillai
> > > Cc: Stanislav Lisovskiy
> > > Signed-off-by: Jouni Högander
> > > ---
>
> Looks fine to me. A couple of nitpicks.
>
>
> > > drivers/gpu/drm/i915/display/intel_cursor.c | 2 +-
> > > drivers/gpu/drm/i915/display/intel_psr.c | 29
> > > ++-
> > > drivers/gpu/drm/i915/display/intel_psr.h | 6 +++-
> > > .../drm/i915/display/skl_universal_plane.c | 4 ++-
> > > 4 files changed, 30 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> > > b/drivers/gpu/drm/i915/display/intel_cursor.c
> > > index d190fa0d393b..50232cec48e0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > > @@ -532,7 +532,7 @@ static void i9xx_cursor_update_arm(struct
> > > intel_plane *plane,
> > > skl_write_cursor_wm(plane, crtc_state);
> > >
> > > if (plane_state)
> > > - intel_psr2_program_plane_sel_fetch(plane,
> > > crtc_state, plane_state, 0);
> > > + intel_psr2_program_plane_sel_fetch_arm(plane,
> > > crtc_state, plane_state, 0);
>
> This goes well over 80 chars. Even though it's accepted to go over
> that nowadays, I think it's still preferred to keep it shorter and
> this
> line is easily breakable.
>
>
> > > else
> > > intel_psr2_disable_plane_sel_fetch(plane,
> > > crtc_state);
> > >
> > > 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...
>
> Regardless of these nitpicks (change them if you want):
Thank you Luca for checking my patch. Sent new version addressing your
comments.
> Reviewed-by: Luca Coelho
>
> --
> Cheers,
> Luca.
BR,
Jouni Högander
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
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 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 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
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
On Thu, 2023-01-26 at 08:36 -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 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 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.
I first started doing it with coccinelle, but it just required too many
rules to justify it. My idea was to do it bit by bit with coccinelle
in separate patches, but it didn't look great.
A simple sed seems to have done the job very nicely and, since that's
the case, I do agree that doing it in one patch is the way to go. I
don't see the reason to artificially break it down into several patches
and manually handle all the interdependencies.
--
Cheers,
Luca.
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
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 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 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
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
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 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 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
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.
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
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
On Thu, Jan 26, 2023 at 06:05:32PM +0200, Jani Nikula wrote:
On Thu, 26 Jan 2023, Luca Coelho 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 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.
Lucas De Marchi
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
On Thu, 26 Jan 2023, Luca Coelho 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 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?
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
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 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.
--
Cheers,
Luca.
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
> SEL_FETCH_CTL registers are armed immediately when plane is disabled.
> SEL_FETCH_* instances of plane configuration are used when doing
> selective update and normal plane register instances for full updates.
> Currently all SEL_FETCH_* registers are written as a part of noarm
> plane configuration. If noarm and arm plane configuration are not
> happening within same vblank we may end up having plane as a part of
> selective update before it's PLANE_SURF register is written.
>
> Fix this by splitting plane selective fetch configuration into arm and
> noarm versions and call them accordingly. Write SEL_FETCH_CTL in arm
> version.
>
> Cc: Ville Syrjälä
> Cc: José Roberto de Souza
> Cc: Mika Kahola
> Cc: Vinod Govindapillai
> Cc: Stanislav Lisovskiy
> Signed-off-by: Jouni Högander
> ---
> drivers/gpu/drm/i915/display/intel_cursor.c | 2 +-
> drivers/gpu/drm/i915/display/intel_psr.c | 29 ++-
> drivers/gpu/drm/i915/display/intel_psr.h | 6 +++-
> .../drm/i915/display/skl_universal_plane.c | 4 ++-
> 4 files changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> b/drivers/gpu/drm/i915/display/intel_cursor.c
> index d190fa0d393b..50232cec48e0 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -532,7 +532,7 @@ static void i9xx_cursor_update_arm(struct intel_plane
> *plane,
> skl_write_cursor_wm(plane, crtc_state);
>
> if (plane_state)
> - intel_psr2_program_plane_sel_fetch(plane, crtc_state,
> plane_state, 0);
> + intel_psr2_program_plane_sel_fetch_arm(plane, crtc_state,
> plane_state, 0);
> else
> intel_psr2_disable_plane_sel_fetch(plane, crtc_state);
>
> 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)
Looks like color_plane is redundant here.
Otherwise, looks good to me.
Reviewed-by: Vinod Govindapillai
> +{
> + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> + enum pipe pipe = plane->pipe;
> +
> + if (!crtc_state->enable_psr2_sel_fetch)
> + return;
> +
> + if (plane->id == PLANE_CURSOR)
> + intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe,
> plane->id),
> + plane_state->ctl);
> + else
> + intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe,
> plane->id),
> + PLANE_SEL_FETCH_CTL_ENABLE);
> +}
> +
> +void intel_psr2_program_plane_sel_fetch_noarm(struct intel_plane *plane,
> const struct intel_crtc_state
> *crtc_state,
> const struct intel_plane_state
> *plane_state,
> int color_plane)
> @@ -1573,11 +1592,8 @@ void intel_psr2_program_plane_sel_fetch(struct
> intel_plane *plane,
> if (!crtc_state->enable_psr2_sel_fetch)
> return;
>
> - if (plane->id == PLANE_CURSOR) {
> - intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe,
> plane->id),
> - plane_state->ctl);
> + if (plane->id == PLANE_CURSOR)
> return;
> - }
>
> clip = _state->psr2_sel_fetch_area;
>
> @@ -1605,9 +1621,6 @@ void intel_psr2_program_plane_sel_fetch(struct
> intel_plane *plane,
> val = (drm_rect_height(clip) - 1) << 16;
> val |= (drm_rect_width(_state->uapi.src) >> 16) - 1;
> intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_SIZE(pipe, plane->id),
> val);
> -
> - intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id),
> - PLANE_SEL_FETCH_CTL_ENABLE);
> }
>
> void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state
> *crtc_state)
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> b/drivers/gpu/drm/i915/display/intel_psr.h
> index 2ac3a465..49cd5beacf98 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.h
> +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> @@ -46,7 +46,11 @@ bool intel_psr_enabled(struct intel_dp
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote:
> On Thu, 26 Jan 2023, Luca Coelho 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...
--
Cheers,
Luca.
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
On Thu, 26 Jan 2023, Luca Coelho 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).
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.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH] drm/i915/psr: Split sel fetch plane configuration into arm and noarm
On Wed, 2023-01-25 at 12:44 +0200, Jouni Högander wrote:
> > SEL_FETCH_CTL registers are armed immediately when plane is disabled.
> > SEL_FETCH_* instances of plane configuration are used when doing
> > selective update and normal plane register instances for full updates.
> > Currently all SEL_FETCH_* registers are written as a part of noarm
> > plane configuration. If noarm and arm plane configuration are not
> > happening within same vblank we may end up having plane as a part of
> > selective update before it's PLANE_SURF register is written.
> >
> > Fix this by splitting plane selective fetch configuration into arm and
> > noarm versions and call them accordingly. Write SEL_FETCH_CTL in arm
> > version.
> >
> > Cc: Ville Syrjälä
> > Cc: José Roberto de Souza
> > Cc: Mika Kahola
> > Cc: Vinod Govindapillai
> > Cc: Stanislav Lisovskiy
> > Signed-off-by: Jouni Högander
> > ---
Looks fine to me. A couple of nitpicks.
> > drivers/gpu/drm/i915/display/intel_cursor.c | 2 +-
> > drivers/gpu/drm/i915/display/intel_psr.c | 29 ++-
> > drivers/gpu/drm/i915/display/intel_psr.h | 6 +++-
> > .../drm/i915/display/skl_universal_plane.c| 4 ++-
> > 4 files changed, 30 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> > b/drivers/gpu/drm/i915/display/intel_cursor.c
> > index d190fa0d393b..50232cec48e0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > @@ -532,7 +532,7 @@ static void i9xx_cursor_update_arm(struct intel_plane
> > *plane,
> > skl_write_cursor_wm(plane, crtc_state);
> >
> > if (plane_state)
> > - intel_psr2_program_plane_sel_fetch(plane, crtc_state,
> > plane_state, 0);
> > + intel_psr2_program_plane_sel_fetch_arm(plane, crtc_state,
> > plane_state, 0);
This goes well over 80 chars. Even though it's accepted to go over
that nowadays, I think it's still preferred to keep it shorter and this
line is easily breakable.
> > else
> > intel_psr2_disable_plane_sel_fetch(plane, crtc_state);
> >
> > 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...
Regardless of these nitpicks (change them if you want):
Reviewed-by: Luca Coelho
--
Cheers,
Luca.
