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.
> + intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane->id),
> + plane_state->ctl);
Does this even have anything besides the enable bit?
> + 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.
>
> 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
--
Ville Syrjälä
Intel
