| Message ID | 20231117100227.609261-3-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement sel_fetch disable for planes | expand |
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@intel.com> > --- > 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
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@intel.com> > > --- > > 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 >
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@intel.com> > --- > 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) Some kind of weird reordering happening here making the diff have spurious changes. Apart from that this seems fine Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > { > - 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) > + intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane->id), > + plane_state->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; > + } > > 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
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) + intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane->id), + plane_state->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; + } 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
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@intel.com> --- 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(-)