drm/nouveau: Only recalculate PBN/VCPI on mode/connector changes

Submitted by Lyude Paul on Aug. 8, 2019, 6:04 p.m.

Details

Message ID 20190808180439.19118-1-lyude@redhat.com
State New
Headers show
Series "drm/nouveau: Only recalculate PBN/VCPI on mode/connector changes" ( rev: 1 ) in Nouveau

Not browsing as part of any series.

Commit Message

Lyude Paul Aug. 8, 2019, 6:04 p.m.
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.

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(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 126703816794..5d23ab8e4917 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(bpp, clock);
+		}
 
-	if (crtc_state->mode_changed) {
 		slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr,
 						      mstc->port,
 						      asyh->dp.pbn);

Comments

On 8/8/19 1:04 PM, Lyude Paul 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.

>

> 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..5d23ab8e4917 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);

drm_dp_calc_pbn_mode(clock, bpp)
> +	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(bpp, clock);

drm_dp_calc_pbn_mode(bpp, clock)
> +		}

>   

> -	if (crtc_state->mode_changed) {

>   		slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr,

>   						      mstc->port,

>   						      asyh->dp.pbn);

Which is correct?
On Thu, 2019-08-08 at 18:17 +0000, William Lewis wrote:
> On 8/8/19 1:04 PM, Lyude Paul 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.
> > 
> > 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..5d23ab8e4917 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);
> drm_dp_calc_pbn_mode(clock, bpp)
> > +	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(bpp, clock);
> drm_dp_calc_pbn_mode(bpp, clock)
> > +		}
> >   
> > -	if (crtc_state->mode_changed) {
> >   		slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr,
> >   						      mstc->port,
> >   						      asyh->dp.pbn);
> Which is correct?

!?!?!?!?!?!?
I am actually seriously confused as to how this code did not break when I tested
it, as I certainly reversed the two arguments here by accident.
Looks like we need to add more error checking for drm_dp_calc_pbn_mode.
Thank you for catching this, I'll respin this and send out a new version in a
second

> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
On Thu, 2019-08-08 at 20:46 -0400, Lyude Paul wrote:
> On Thu, 2019-08-08 at 18:17 +0000, William Lewis wrote:
> > On 8/8/19 1:04 PM, Lyude Paul 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.
> > > 
> > > 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..5d23ab8e4917 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);
> > drm_dp_calc_pbn_mode(clock, bpp)
> > > +	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(bpp, clock);
> > drm_dp_calc_pbn_mode(bpp, clock)
> > > +		}
> > >   
> > > -	if (crtc_state->mode_changed) {
> > >   		slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr,
> > >   						      mstc->port,
> > >   						      asyh->dp.pbn);
> > Which is correct?
> 
> !?!?!?!?!?!?
> I am actually seriously confused as to how this code did not break when I
> tested
> it, as I certainly reversed the two arguments here by accident.
> Looks like we need to add more error checking for drm_dp_calc_pbn_mode.
> Thank you for catching this, I'll respin this and send out a new version in a
> second

FWIW, I've sent out a respin with this fixed already, but I see how this didn't
manage to break anything now. The only time bpp and clock are used in
drm_dp_calc_pbn_mode() is:

kbps = clock * bpp;

So I guess that explains it :)

> 
> > _______________________________________________
> > Nouveau mailing list
> > Nouveau@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/nouveau