On Fri, 2023-11-17 at 13:09 +0200, Ville Syrjälä wrote:
> On Fri, Nov 17, 2023 at 12:02:27PM +0200, Jouni Högander wrote:
> > Currently we are enabling selective fetch for all planes that are
> > visible.
> > This is suboptimal as we might be fetching for memory for planes
> > that are
> > not part of selective update.
> >
> > Fix this by adding proper handling for disabling plane selective
> > fetch:
> > If plane previously part of selective update is now not part of
> > update:
> > Add it into updated planes and let the plane configuration to
> > disable
> > selective fetch for it.
> >
> > v2:
> > - Add setting sel_fetch_area->y1/y2 to -1
> > - Remove setting again local sel_fetch_area variable
> >
> > Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_cursor.c | 22 +++++++++++----
> > ----
> > drivers/gpu/drm/i915/display/intel_psr.c | 13 ++++++++++-
> > .../drm/i915/display/skl_universal_plane.c | 8 +++++--
> > 3 files changed, 31 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c
> > b/drivers/gpu/drm/i915/display/intel_cursor.c
> > index c089dd6f9781..299d22708fa4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > @@ -485,22 +485,22 @@ static int i9xx_check_cursor(struct
> > intel_crtc_state *crtc_state,
> > return 0;
> > }
> >
> > -static void i9xx_cursor_update_sel_fetch_arm(struct intel_plane
> > *plane,
> > - const struct
> > intel_crtc_state *crtc_state,
> > - const struct
> > intel_plane_state *plane_state)
> > +static void i9xx_cursor_disable_sel_fetch_arm(struct intel_plane
> > *plane,
> > + const struct
> > intel_crtc_state *crtc_state)
> > {
> > - struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > + struct drm_i915_private *dev_priv = to_i915(plane-
> > >base.dev);
> > enum pipe pipe = plane->pipe;
> >
> > if (!crtc_state->enable_psr2_sel_fetch)
> > return;
> >
> > - intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane-
> > >id),
> > - plane_state->ctl);
> > +
> > + intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe,
> > plane->id), 0);
> > }
> >
> > -static void i9xx_cursor_disable_sel_fetch_arm(struct intel_plane
> > *plane,
> > - const struct
> > intel_crtc_state *crtc_state)
> > +static void i9xx_cursor_update_sel_fetch_arm(struct intel_plane
> > *plane,
> > + const struct
> > intel_crtc_state *crtc_state,
> > + const struct
> > intel_plane_state *plane_state)
> > {
> > struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > enum pipe pipe = plane->pipe;
> > @@ -508,7 +508,11 @@ static void
> > i9xx_cursor_disable_sel_fetch_arm(struct intel_plane *plane,
> > if (!crtc_state->enable_psr2_sel_fetch)
> > return;
> >
> > - intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane-
> > >id), 0);
> > + if (drm_rect_height(&plane_state->psr2_sel_fetch_area) > 0)
>
> drm_rect_visible() is less magic.
Our hw the area is always just full lines -> choose to use
drm_recth_height. Drm_rect_visible should work as well. We just need to
ensure x1 and x2 are set accordingly if we do the change.
>
> > + intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe,
> > plane->id),
> > + plane_state->ctl);
>
> Does this even have anything besides the enable bit?
Bspec says:
"If the update region (translated to pipe source coordinates) overlaps
this cursor, then the selective fetch cursor mode select should be the
same as the cursor control mode select (SEL_FETCH_CUR_CTL[ Cursor Mode
Select ] = CUR_CTL[ Cursor Mode Select ]. Otherwise, disable
(SEL_FETCH_CUR_CTL[ Cursor Mode Select ] = 0)."
and
"Program the other fields in SEL_FETCH_CUR_CTL to match CUR_CTL."
>
> > + else
> > + i9xx_cursor_disable_sel_fetch_arm(plane,
> > crtc_state);
> > }
> >
> > /* TODO: split into noarm+arm pair */
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 87eb1535ba98..239365c666e2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -2173,8 +2173,19 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> > continue;
> >
> > inter = pipe_clip;
> > - if (!drm_rect_intersect(&inter, &new_plane_state-
> > >uapi.dst))
> > + sel_fetch_area = &new_plane_state-
> > >psr2_sel_fetch_area;
> > + if (!drm_rect_intersect(&inter, &new_plane_state-
> > >uapi.dst)) {
> > + sel_fetch_area->y1 = -1;
> > + sel_fetch_area->y2 = -1;
> > + /*
> > + * if plane sel fetch was previously
> > enabled ->
> > + * disable it
> > + */
> > + if (drm_rect_height(&old_plane_state-
> > >psr2_sel_fetch_area) > 0)
> > + crtc_state->update_planes |=
> > BIT(plane->id);
> > +
> > continue;
> > + }
>
> I tried to look at this code, but it just looks entirely confused
> about things.
>
> I had a quick stab at rewriting it all:
> https://github.com/vsyrjala/linux.git sel_fetch_redo_2
> but I don't have a machine to test it, so can't guarantee that it's
> 100%
> correct.
I will take a look and give a try...
BR,
Jouni Högander
>
> >
> > if
> > (!psr2_sel_fetch_plane_state_supported(new_plane_state)) {
> > full_update = true;
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 99d33ac5ceee..a969bb835baf 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -1336,8 +1336,12 @@ static void
> > icl_plane_update_sel_fetch_arm(struct intel_plane *plane,
> > if (!crtc_state->enable_psr2_sel_fetch)
> > return;
> >
> > - intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane-
> > >id),
> > - PLANE_SEL_FETCH_CTL_ENABLE);
> > +
> > + if (drm_rect_height(&plane_state->psr2_sel_fetch_area) > 0)
> > + intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe,
> > plane->id),
> > + PLANE_SEL_FETCH_CTL_ENABLE);
> > + else
> > + icl_plane_disable_sel_fetch_arm(plane, crtc_state);
> > }
> >
> > static void
> > --
> > 2.34.1
>
