linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] drm/nouveau: Only recalculate PBN/VCPI on mode/connector changes
@ 2019-08-09  0:53 Lyude Paul
  2019-08-13  4:39 ` Ben Skeggs
  0 siblings, 1 reply; 2+ messages in thread
From: Lyude Paul @ 2019-08-09  0:53 UTC (permalink / raw)
  To: nouveau
  Cc: William Lewis, Bohdan Milar, Ben Skeggs, Daniel Vetter,
	David Airlie, Jerry Zuo, Harry Wentland, Juston Li,
	Laurent Pinchart, Karol Herbst, Ilia Mirkin, stable,
	David Airlie, Daniel Vetter, dri-devel, linux-kernel

I -thought- I had fixed this entirely, but it looks like that I didn't
test this thoroughly enough as we apparently still make one big mistake
with nv50_msto_atomic_check() - we don't handle the following scenario:

* CRTC #1 has n VCPI allocated to it, is attached to connector DP-4
  which is attached to encoder #1. enabled=y active=n
* CRTC #1 is changed from DP-4 to DP-5, causing:
  * DP-4 crtc=#1→NULL (VCPI n→0)
  * DP-5 crtc=NULL→#1
  * CRTC #1 steals encoder #1 back from DP-4 and gives it to DP-5
  * CRTC #1 maintains the same mode as before, just with a different
    connector
* mode_changed=n connectors_changed=y
  (we _SHOULD_ do VCPI 0→n here, but don't)

Once the above scenario is repeated once, we'll attempt freeing VCPI
from the connector that we didn't allocate due to the connectors
changing, but the mode staying the same. Sigh.

Since nv50_msto_atomic_check() has broken a few times now, let's rethink
things a bit to be more careful: limit both VCPI/PBN allocations to
mode_changed || connectors_changed, since neither VCPI or PBN should
ever need to change outside of routing and mode changes.

Changes since v1:
* Fix accidental reversal of clock and bpp arguments in
  drm_dp_calc_pbn_mode() - William Lewis

Signed-off-by: Lyude Paul <lyude@redhat.com>
Reported-by: Bohdan Milar <bmilar@redhat.com>
Tested-by: Bohdan Milar <bmilar@redhat.com>
Fixes: 232c9eec417a ("drm/nouveau: Use atomic VCPI helpers for MST")
References: 412e85b60531 ("drm/nouveau: Only release VCPI slots on mode changes")
Cc: Lyude Paul <lyude@redhat.com>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: David Airlie <airlied@redhat.com>
Cc: Jerry Zuo <Jerry.Zuo@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Juston Li <juston.li@intel.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Karol Herbst <karolherbst@gmail.com>
Cc: Ilia Mirkin <imirkin@alum.mit.edu>
Cc: <stable@vger.kernel.org> # v5.1+
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 126703816794..5c36c75232e6 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -771,16 +771,20 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
 	struct nv50_head_atom *asyh = nv50_head_atom(crtc_state);
 	int slots;
 
-	/* When restoring duplicated states, we need to make sure that the
-	 * bw remains the same and avoid recalculating it, as the connector's
-	 * bpc may have changed after the state was duplicated
-	 */
-	if (!state->duplicated)
-		asyh->dp.pbn =
-			drm_dp_calc_pbn_mode(crtc_state->adjusted_mode.clock,
-					     connector->display_info.bpc * 3);
+	if (crtc_state->mode_changed || crtc_state->connectors_changed) {
+		/*
+		 * When restoring duplicated states, we need to make sure that
+		 * the bw remains the same and avoid recalculating it, as the
+		 * connector's bpc may have changed after the state was
+		 * duplicated
+		 */
+		if (!state->duplicated) {
+			const int bpp = connector->display_info.bpc * 3;
+			const int clock = crtc_state->adjusted_mode.clock;
+
+			asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp);
+		}
 
-	if (crtc_state->mode_changed) {
 		slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr,
 						      mstc->port,
 						      asyh->dp.pbn);
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 2+ messages in thread

* Re: [PATCH v2] drm/nouveau: Only recalculate PBN/VCPI on mode/connector changes
  2019-08-09  0:53 [PATCH v2] drm/nouveau: Only recalculate PBN/VCPI on mode/connector changes Lyude Paul
@ 2019-08-13  4:39 ` Ben Skeggs
  0 siblings, 0 replies; 2+ messages in thread
From: Ben Skeggs @ 2019-08-13  4:39 UTC (permalink / raw)
  To: Lyude Paul
  Cc: ML nouveau, Bohdan Milar, linux-kernel, David Airlie,
	Daniel Vetter, ML dri-devel, William Lewis, stable, Karol Herbst,
	Jerry Zuo, Ben Skeggs, David Airlie, Juston Li, Laurent Pinchart

On Fri, 9 Aug 2019 at 10:53, Lyude Paul <lyude@redhat.com> wrote:
>
> I -thought- I had fixed this entirely, but it looks like that I didn't
> test this thoroughly enough as we apparently still make one big mistake
> with nv50_msto_atomic_check() - we don't handle the following scenario:
>
> * CRTC #1 has n VCPI allocated to it, is attached to connector DP-4
>   which is attached to encoder #1. enabled=y active=n
> * CRTC #1 is changed from DP-4 to DP-5, causing:
>   * DP-4 crtc=#1→NULL (VCPI n→0)
>   * DP-5 crtc=NULL→#1
>   * CRTC #1 steals encoder #1 back from DP-4 and gives it to DP-5
>   * CRTC #1 maintains the same mode as before, just with a different
>     connector
> * mode_changed=n connectors_changed=y
>   (we _SHOULD_ do VCPI 0→n here, but don't)
>
> Once the above scenario is repeated once, we'll attempt freeing VCPI
> from the connector that we didn't allocate due to the connectors
> changing, but the mode staying the same. Sigh.
>
> Since nv50_msto_atomic_check() has broken a few times now, let's rethink
> things a bit to be more careful: limit both VCPI/PBN allocations to
> mode_changed || connectors_changed, since neither VCPI or PBN should
> ever need to change outside of routing and mode changes.
>
> Changes since v1:
> * Fix accidental reversal of clock and bpp arguments in
>   drm_dp_calc_pbn_mode() - William Lewis
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Reported-by: Bohdan Milar <bmilar@redhat.com>
> Tested-by: Bohdan Milar <bmilar@redhat.com>
> Fixes: 232c9eec417a ("drm/nouveau: Use atomic VCPI helpers for MST")
> References: 412e85b60531 ("drm/nouveau: Only release VCPI slots on mode changes")
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: David Airlie <airlied@redhat.com>
> Cc: Jerry Zuo <Jerry.Zuo@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Juston Li <juston.li@intel.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Karol Herbst <karolherbst@gmail.com>
> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> Cc: <stable@vger.kernel.org> # v5.1+
Acked-by: Ben Skeggs <bskeggs@redhat.com>

> ---
>  drivers/gpu/drm/nouveau/dispnv50/disp.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 126703816794..5c36c75232e6 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -771,16 +771,20 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
>         struct nv50_head_atom *asyh = nv50_head_atom(crtc_state);
>         int slots;
>
> -       /* When restoring duplicated states, we need to make sure that the
> -        * bw remains the same and avoid recalculating it, as the connector's
> -        * bpc may have changed after the state was duplicated
> -        */
> -       if (!state->duplicated)
> -               asyh->dp.pbn =
> -                       drm_dp_calc_pbn_mode(crtc_state->adjusted_mode.clock,
> -                                            connector->display_info.bpc * 3);
> +       if (crtc_state->mode_changed || crtc_state->connectors_changed) {
> +               /*
> +                * When restoring duplicated states, we need to make sure that
> +                * the bw remains the same and avoid recalculating it, as the
> +                * connector's bpc may have changed after the state was
> +                * duplicated
> +                */
> +               if (!state->duplicated) {
> +                       const int bpp = connector->display_info.bpc * 3;
> +                       const int clock = crtc_state->adjusted_mode.clock;
> +
> +                       asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp);
> +               }
>
> -       if (crtc_state->mode_changed) {
>                 slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr,
>                                                       mstc->port,
>                                                       asyh->dp.pbn);
> --
> 2.21.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2019-08-13  4:39 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-09  0:53 [PATCH v2] drm/nouveau: Only recalculate PBN/VCPI on mode/connector changes Lyude Paul
2019-08-13  4:39 ` Ben Skeggs

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).