| Message ID | 20230125104439.1662832-1-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/psr: Split sel fetch plane configuration into arm and noarm | expand |
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ä <ville.syrjala@linux.intel.com> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Cc: Mika Kahola <mika.kahola@intel.com> > > Cc: Vinod Govindapillai <vinod.govindapillai@intel.com> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > --- 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 <luciano.coelho@intel.com> -- Cheers, Luca.
On Thu, 26 Jan 2023, Luca Coelho <luca@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). 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.
On Thu, 2023-01-26 at 14:00 +0200, Jani Nikula wrote: > On Thu, 26 Jan 2023, Luca Coelho <luca@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... -- Cheers, Luca.
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ä <ville.syrjala@linux.intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > Cc: Mika Kahola <mika.kahola@intel.com> > Cc: Vinod Govindapillai <vinod.govindapillai@intel.com> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > --- > 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 <vinod.govindapillai@intel.com> > +{ > + 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 = &plane_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(&plane_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 2ac3a46cccc5..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 *intel_dp); > int intel_psr2_sel_fetch_update(struct intel_atomic_state *state, > struct intel_crtc *crtc); > void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state *crtc_state); > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane, > +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); > +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); > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c > b/drivers/gpu/drm/i915/display/skl_universal_plane.c > index 9b172a1e90de..5a309a3e2812 100644 > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > @@ -1260,7 +1260,7 @@ icl_plane_update_noarm(struct intel_plane *plane, > if (plane_state->force_black) > icl_plane_csc_load_black(plane); > > - intel_psr2_program_plane_sel_fetch(plane, crtc_state, plane_state, color_plane); > + intel_psr2_program_plane_sel_fetch_noarm(plane, crtc_state, plane_state, color_plane); > } > > static void > @@ -1287,6 +1287,8 @@ icl_plane_update_arm(struct intel_plane *plane, > if (plane_state->scaler_id >= 0) > skl_program_plane_scaler(plane, crtc_state, plane_state); > > + intel_psr2_program_plane_sel_fetch_arm(plane, crtc_state, plane_state, color_plane); > + > /* > * The control register self-arms if the plane was previously > * disabled. Try to make the plane enable atomic by writing
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@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.
On Thu, 26 Jan 2023, Luca Coelho <luca@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 <luca@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.
On Thu, Jan 26, 2023 at 06:05:32PM +0200, Jani Nikula wrote: >On Thu, 26 Jan 2023, Luca Coelho <luca@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 <luca@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.
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@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 <luca@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.
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@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 <luca@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.
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 <luca@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 <luca@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.
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@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 <luca@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.
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ä <ville.syrjala@linux.intel.com> > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > Cc: Vinod Govindapillai <vinod.govindapillai@intel.com> > > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > --- > > 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 <luciano.coelho@intel.com> > > -- > Cheers, > Luca. BR, Jouni Högander
On Thu, 2023-01-26 at 13:01 +0000, 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ä <ville.syrjala@linux.intel.com> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Cc: Mika Kahola <mika.kahola@intel.com> > > Cc: Vinod Govindapillai <vinod.govindapillai@intel.com> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > --- > > 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 <vinod.govindapillai@intel.com> > > > +{ > > + 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 = &plane_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(&plane_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 2ac3a46cccc5..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 > > *intel_dp); > > int intel_psr2_sel_fetch_update(struct intel_atomic_state *state, > > struct intel_crtc *crtc); > > void intel_psr2_program_trans_man_trk_ctl(const struct > > intel_crtc_state *crtc_state); > > -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane, > > +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); > > +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); > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c > > b/drivers/gpu/drm/i915/display/skl_universal_plane.c > > index 9b172a1e90de..5a309a3e2812 100644 > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > > @@ -1260,7 +1260,7 @@ icl_plane_update_noarm(struct intel_plane > > *plane, > > if (plane_state->force_black) > > icl_plane_csc_load_black(plane); > > > > - intel_psr2_program_plane_sel_fetch(plane, crtc_state, > > plane_state, color_plane); > > + intel_psr2_program_plane_sel_fetch_noarm(plane, crtc_state, > > plane_state, color_plane); > > } > > > > static void > > @@ -1287,6 +1287,8 @@ icl_plane_update_arm(struct intel_plane > > *plane, > > if (plane_state->scaler_id >= 0) > > skl_program_plane_scaler(plane, crtc_state, > > plane_state); > > > > + intel_psr2_program_plane_sel_fetch_arm(plane, crtc_state, > > plane_state, color_plane); > > + > > /* > > * The control register self-arms if the plane was > > previously > > * disabled. Try to make the plane enable atomic by writing > BR, Jouni Högander
On 26/01/2023 16:05, Jani Nikula wrote: > On Thu, 26 Jan 2023, Luca Coelho <luca@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 <luca@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.
On Fri, 27 Jan 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > On 26/01/2023 16:05, Jani Nikula wrote: >> On Thu, 26 Jan 2023, Luca Coelho <luca@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 <luca@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.
On Fri, 2023-01-27 at 16:37 +0200, Jani Nikula wrote: > On Fri, 27 Jan 2023, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > On 26/01/2023 16:05, Jani Nikula wrote: > > > On Thu, 26 Jan 2023, Luca Coelho <luca@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 <luca@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.
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.ursulin@linux.intel.com> wrote: >> > On 26/01/2023 16:05, Jani Nikula wrote: >> > > On Thu, 26 Jan 2023, Luca Coelho <luca@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 <luca@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.
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.ursulin@linux.intel.com> wrote: > > > > On 26/01/2023 16:05, Jani Nikula wrote: > > > > > On Thu, 26 Jan 2023, Luca Coelho <luca@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 <luca@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.
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) +{ + 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 = &plane_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(&plane_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 2ac3a46cccc5..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 *intel_dp); int intel_psr2_sel_fetch_update(struct intel_atomic_state *state, struct intel_crtc *crtc); void intel_psr2_program_trans_man_trk_ctl(const struct intel_crtc_state *crtc_state); -void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane, +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); +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); diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c index 9b172a1e90de..5a309a3e2812 100644 --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c @@ -1260,7 +1260,7 @@ icl_plane_update_noarm(struct intel_plane *plane, if (plane_state->force_black) icl_plane_csc_load_black(plane); - intel_psr2_program_plane_sel_fetch(plane, crtc_state, plane_state, color_plane); + intel_psr2_program_plane_sel_fetch_noarm(plane, crtc_state, plane_state, color_plane); } static void @@ -1287,6 +1287,8 @@ icl_plane_update_arm(struct intel_plane *plane, if (plane_state->scaler_id >= 0) skl_program_plane_scaler(plane, crtc_state, plane_state); + intel_psr2_program_plane_sel_fetch_arm(plane, crtc_state, plane_state, color_plane); + /* * The control register self-arms if the plane was previously * disabled. Try to make the plane enable atomic by writing
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ä <ville.syrjala@linux.intel.com> Cc: José Roberto de Souza <jose.souza@intel.com> Cc: Mika Kahola <mika.kahola@intel.com> Cc: Vinod Govindapillai <vinod.govindapillai@intel.com> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> Signed-off-by: Jouni Högander <jouni.hogander@intel.com> --- 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(-)