[PATCH 0/3] MST BPC fixes for nouveau

[PATCH 0/3] MST BPC fixes for nouveau

[Lyude Paul]

Realized when I moved nouveau over to using the atomic DP MST VCPI
helpers that I forgot to ensure that we clamp the BPC to 8 to make us
less likely to run out of bandwidth on a topology when enabling multiple
displays that support >8 BPC - something we want to do until we have
support for dynamically selecting the bpc based on the topology's
available bandwidth, since userspace isn't really using HDR yet anyway.
This matches the behavior that i915 has, along with the behavior of
amdgpu and should fix some people's displays not turning on.

Lyude Paul (3):
  drm/nouveau/kms/nv50-: Call outp_atomic_check_view() before handling
    PBN
  drm/nouveau/kms/nv50-: Store the bpc we're using in nv50_head_atom
  drm/nouveau/kms/nv50-: Limit MST BPC to 8

 drivers/gpu/drm/nouveau/dispnv50/atom.h |   1 +
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 102 ++++++++++++++----------
 drivers/gpu/drm/nouveau/dispnv50/head.c |   5 +-
 3 files changed, 64 insertions(+), 44 deletions(-)

-- 
2.21.0

[PATCH 1/3] drm/nouveau/kms/nv50-: Call outp_atomic_check_view() before handling PBN

[Lyude Paul]

Since nv50_outp_atomic_check_view() can set crtc_state->mode_changed, we
probably should be calling it before handling any PBN changes. Just a
precaution.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes: 232c9eec417a ("drm/nouveau: Use atomic VCPI helpers for MST")
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: Sean Paul <seanpaul@chromium.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: <stable@vger.kernel.org> # v5.1+
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 44 ++++++++++++++-----------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 549486f1d937..6327aaf37c08 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -770,32 +770,36 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
 	struct nv50_mstm *mstm = mstc->mstm;
 	struct nv50_head_atom *asyh = nv50_head_atom(crtc_state);
 	int slots;
+	int ret;
 
-	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;
+	ret = nv50_outp_atomic_check_view(encoder, crtc_state, conn_state,
+					  mstc->native);
+	if (ret)
+		return ret;
 
-			asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp);
-		}
+	if (!crtc_state->mode_changed && !crtc_state->connectors_changed)
+		return 0;
 
-		slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr,
-						      mstc->port,
-						      asyh->dp.pbn);
-		if (slots < 0)
-			return 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) {
+		const int bpp = connector->display_info.bpc * 3;
+		const int clock = crtc_state->adjusted_mode.clock;
 
-		asyh->dp.tu = slots;
+		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp);
 	}
 
-	return nv50_outp_atomic_check_view(encoder, crtc_state, conn_state,
-					   mstc->native);
+	slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, mstc->port,
+					      asyh->dp.pbn);
+	if (slots < 0)
+		return slots;
+
+	asyh->dp.tu = slots;
+
+	return 0;
 }
 
 static void
-- 
2.21.0

Re: [Nouveau] [PATCH 1/3] drm/nouveau/kms/nv50-: Call outp_atomic_check_view() before handling PBN

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3291 bytes --]

On Fri, Nov 15, 2019 at 04:07:18PM -0500, Lyude Paul wrote:
> Since nv50_outp_atomic_check_view() can set crtc_state->mode_changed, we
> probably should be calling it before handling any PBN changes. Just a
> precaution.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Fixes: 232c9eec417a ("drm/nouveau: Use atomic VCPI helpers for MST")
> 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: Sean Paul <seanpaul@chromium.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: <stable@vger.kernel.org> # v5.1+
> ---
>  drivers/gpu/drm/nouveau/dispnv50/disp.c | 44 ++++++++++++++-----------
>  1 file changed, 24 insertions(+), 20 deletions(-)

Looks reasonable:

Reviewed-by: Thierry Reding <treding@nvidia.com>

> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 549486f1d937..6327aaf37c08 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -770,32 +770,36 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
>  	struct nv50_mstm *mstm = mstc->mstm;
>  	struct nv50_head_atom *asyh = nv50_head_atom(crtc_state);
>  	int slots;
> +	int ret;
>  
> -	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;
> +	ret = nv50_outp_atomic_check_view(encoder, crtc_state, conn_state,
> +					  mstc->native);
> +	if (ret)
> +		return ret;
>  
> -			asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp);
> -		}
> +	if (!crtc_state->mode_changed && !crtc_state->connectors_changed)
> +		return 0;
>  
> -		slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr,
> -						      mstc->port,
> -						      asyh->dp.pbn);
> -		if (slots < 0)
> -			return 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) {
> +		const int bpp = connector->display_info.bpc * 3;
> +		const int clock = crtc_state->adjusted_mode.clock;
>  
> -		asyh->dp.tu = slots;
> +		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, bpp);
>  	}
>  
> -	return nv50_outp_atomic_check_view(encoder, crtc_state, conn_state,
> -					   mstc->native);
> +	slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, mstc->port,
> +					      asyh->dp.pbn);
> +	if (slots < 0)
> +		return slots;
> +
> +	asyh->dp.tu = slots;
> +
> +	return 0;
>  }
>  
>  static void
> -- 
> 2.21.0
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/3] drm/nouveau/kms/nv50-: Store the bpc we're using in nv50_head_atom

[Lyude Paul]

In order to be able to use bpc values that are different from what the
connector reports, we want to be able to store the bpc value we decide
on using for an atomic state in nv50_head_atom and refer to that instead
of simply using the value that the connector reports throughout the
whole atomic check phase and commit phase. This will let us (eventually)
implement the max bpc connector property, and will also be needed for
limiting the bpc we use on MST displays to 8 in the next commit.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes: 232c9eec417a ("drm/nouveau: Use atomic VCPI helpers for MST")
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: Sean Paul <seanpaul@chromium.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: <stable@vger.kernel.org> # v5.1+
---
 drivers/gpu/drm/nouveau/dispnv50/atom.h |  1 +
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 57 ++++++++++++++-----------
 drivers/gpu/drm/nouveau/dispnv50/head.c |  5 +--
 3 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/atom.h b/drivers/gpu/drm/nouveau/dispnv50/atom.h
index 43df86c38f58..24f7700768da 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/atom.h
+++ b/drivers/gpu/drm/nouveau/dispnv50/atom.h
@@ -114,6 +114,7 @@ struct nv50_head_atom {
 		u8 nhsync:1;
 		u8 nvsync:1;
 		u8 depth:4;
+		u8 bpc;
 	} or;
 
 	/* Currently only used for MST */
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 6327aaf37c08..93665aecce57 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -353,10 +353,20 @@ nv50_outp_atomic_check(struct drm_encoder *encoder,
 		       struct drm_crtc_state *crtc_state,
 		       struct drm_connector_state *conn_state)
 {
-	struct nouveau_connector *nv_connector =
-		nouveau_connector(conn_state->connector);
-	return nv50_outp_atomic_check_view(encoder, crtc_state, conn_state,
-					   nv_connector->native_mode);
+	struct drm_connector *connector = conn_state->connector;
+	struct nouveau_connector *nv_connector = nouveau_connector(connector);
+	struct nv50_head_atom *asyh = nv50_head_atom(crtc_state);
+	int ret;
+
+	ret = nv50_outp_atomic_check_view(encoder, crtc_state, conn_state,
+					  nv_connector->native_mode);
+	if (ret)
+		return ret;
+
+	if (crtc_state->mode_changed || crtc_state->connectors_changed)
+		asyh->or.bpc = connector->display_info.bpc;
+
+	return 0;
 }
 
 /******************************************************************************
@@ -786,10 +796,10 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
 	 * 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);
+		asyh->or.bpc = connector->display_info.bpc;
+		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3);
 	}
 
 	slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, mstc->port,
@@ -802,6 +812,17 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
 	return 0;
 }
 
+static u8
+nv50_dp_bpc_to_depth(unsigned int bpc)
+{
+	switch (bpc) {
+	case  6: return 0x2;
+	case  8: return 0x5;
+	case 10: /* fall-through */
+	default: return 0x6;
+	}
+}
+
 static void
 nv50_msto_enable(struct drm_encoder *encoder)
 {
@@ -812,7 +833,7 @@ nv50_msto_enable(struct drm_encoder *encoder)
 	struct nv50_mstm *mstm = NULL;
 	struct drm_connector *connector;
 	struct drm_connector_list_iter conn_iter;
-	u8 proto, depth;
+	u8 proto;
 	bool r;
 
 	drm_connector_list_iter_begin(encoder->dev, &conn_iter);
@@ -841,14 +862,8 @@ nv50_msto_enable(struct drm_encoder *encoder)
 	else
 		proto = 0x9;
 
-	switch (mstc->connector.display_info.bpc) {
-	case  6: depth = 0x2; break;
-	case  8: depth = 0x5; break;
-	case 10:
-	default: depth = 0x6; break;
-	}
-
-	mstm->outp->update(mstm->outp, head->base.index, armh, proto, depth);
+	mstm->outp->update(mstm->outp, head->base.index, armh, proto,
+			   nv50_dp_bpc_to_depth(armh->or.bpc));
 
 	msto->head = head;
 	msto->mstc = mstc;
@@ -1502,20 +1517,14 @@ nv50_sor_enable(struct drm_encoder *encoder)
 					lvds.lvds.script |= 0x0200;
 			}
 
-			if (nv_connector->base.display_info.bpc == 8)
+			if (asyh->or.bpc == 8)
 				lvds.lvds.script |= 0x0200;
 		}
 
 		nvif_mthd(&disp->disp->object, 0, &lvds, sizeof(lvds));
 		break;
 	case DCB_OUTPUT_DP:
-		if (nv_connector->base.display_info.bpc == 6)
-			depth = 0x2;
-		else
-		if (nv_connector->base.display_info.bpc == 8)
-			depth = 0x5;
-		else
-			depth = 0x6;
+		depth = nv50_dp_bpc_to_depth(asyh->or.bpc);
 
 		if (nv_encoder->link & 1)
 			proto = 0x8;
@@ -1666,7 +1675,7 @@ nv50_pior_enable(struct drm_encoder *encoder)
 	nv50_outp_acquire(nv_encoder);
 
 	nv_connector = nouveau_encoder_connector_get(nv_encoder);
-	switch (nv_connector->base.display_info.bpc) {
+	switch (asyh->or.bpc) {
 	case 10: asyh->or.depth = 0x6; break;
 	case  8: asyh->or.depth = 0x5; break;
 	case  6: asyh->or.depth = 0x2; break;
diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c b/drivers/gpu/drm/nouveau/dispnv50/head.c
index 71c23bf1fe25..c9692df2b76c 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/head.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/head.c
@@ -81,18 +81,17 @@ nv50_head_atomic_check_dither(struct nv50_head_atom *armh,
 			      struct nv50_head_atom *asyh,
 			      struct nouveau_conn_atom *asyc)
 {
-	struct drm_connector *connector = asyc->state.connector;
 	u32 mode = 0x00;
 
 	if (asyc->dither.mode == DITHERING_MODE_AUTO) {
-		if (asyh->base.depth > connector->display_info.bpc * 3)
+		if (asyh->base.depth > asyh->or.bpc * 3)
 			mode = DITHERING_MODE_DYNAMIC2X2;
 	} else {
 		mode = asyc->dither.mode;
 	}
 
 	if (asyc->dither.depth == DITHERING_DEPTH_AUTO) {
-		if (connector->display_info.bpc >= 8)
+		if (asyh->or.bpc >= 8)
 			mode |= DITHERING_DEPTH_8BPC;
 	} else {
 		mode |= asyc->dither.depth;
-- 
2.21.0

Re: [Nouveau] [PATCH 2/3] drm/nouveau/kms/nv50-: Store the bpc we're using in nv50_head_atom

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 7826 bytes --]

On Fri, Nov 15, 2019 at 04:07:19PM -0500, Lyude Paul wrote:
> In order to be able to use bpc values that are different from what the
> connector reports, we want to be able to store the bpc value we decide
> on using for an atomic state in nv50_head_atom and refer to that instead
> of simply using the value that the connector reports throughout the
> whole atomic check phase and commit phase. This will let us (eventually)
> implement the max bpc connector property, and will also be needed for
> limiting the bpc we use on MST displays to 8 in the next commit.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Fixes: 232c9eec417a ("drm/nouveau: Use atomic VCPI helpers for MST")
> 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: Sean Paul <seanpaul@chromium.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: <stable@vger.kernel.org> # v5.1+
> ---
>  drivers/gpu/drm/nouveau/dispnv50/atom.h |  1 +
>  drivers/gpu/drm/nouveau/dispnv50/disp.c | 57 ++++++++++++++-----------
>  drivers/gpu/drm/nouveau/dispnv50/head.c |  5 +--
>  3 files changed, 36 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/atom.h b/drivers/gpu/drm/nouveau/dispnv50/atom.h
> index 43df86c38f58..24f7700768da 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/atom.h
> +++ b/drivers/gpu/drm/nouveau/dispnv50/atom.h
> @@ -114,6 +114,7 @@ struct nv50_head_atom {
>  		u8 nhsync:1;
>  		u8 nvsync:1;
>  		u8 depth:4;
> +		u8 bpc;
>  	} or;
>  
>  	/* Currently only used for MST */
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 6327aaf37c08..93665aecce57 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -353,10 +353,20 @@ nv50_outp_atomic_check(struct drm_encoder *encoder,
>  		       struct drm_crtc_state *crtc_state,
>  		       struct drm_connector_state *conn_state)
>  {
> -	struct nouveau_connector *nv_connector =
> -		nouveau_connector(conn_state->connector);
> -	return nv50_outp_atomic_check_view(encoder, crtc_state, conn_state,
> -					   nv_connector->native_mode);
> +	struct drm_connector *connector = conn_state->connector;
> +	struct nouveau_connector *nv_connector = nouveau_connector(connector);
> +	struct nv50_head_atom *asyh = nv50_head_atom(crtc_state);
> +	int ret;
> +
> +	ret = nv50_outp_atomic_check_view(encoder, crtc_state, conn_state,
> +					  nv_connector->native_mode);
> +	if (ret)
> +		return ret;
> +
> +	if (crtc_state->mode_changed || crtc_state->connectors_changed)
> +		asyh->or.bpc = connector->display_info.bpc;
> +
> +	return 0;
>  }
>  
>  /******************************************************************************
> @@ -786,10 +796,10 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
>  	 * 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);
> +		asyh->or.bpc = connector->display_info.bpc;
> +		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3);
>  	}
>  
>  	slots = drm_dp_atomic_find_vcpi_slots(state, &mstm->mgr, mstc->port,
> @@ -802,6 +812,17 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
>  	return 0;
>  }
>  
> +static u8
> +nv50_dp_bpc_to_depth(unsigned int bpc)
> +{
> +	switch (bpc) {
> +	case  6: return 0x2;
> +	case  8: return 0x5;
> +	case 10: /* fall-through */
> +	default: return 0x6;
> +	}

This is obviously just refactored from the code below, so this is
probably fine for now. But what about BPC > 10? The OR here seems to be
very similar to what's used on Tegra where the same values are used in
the SOR_STATE1 register, see:

	drivers/gpu/drm/tegra/sor.h

There are additional values for 12 and 16 BPC (see the definitions for
SOR_STATE_ASY_PIXELDEPTH_BPP_*). With the above anything higher than 10
BPC will be treated the same and likely lead to wrong results. So I
think either a WARN for the "default" case or additional cases for the
other values would be good to have.

Like I said, if this even is problematic (and given that userspace does
not really support > 8 BPC yet, it probably isn't) it's a preexisting
problem, so can be done in a different patch.

Other than that this looks good, so:

Reviewed-by: Thierry Reding <treding@nvidia.com>

> +}
> +
>  static void
>  nv50_msto_enable(struct drm_encoder *encoder)
>  {
> @@ -812,7 +833,7 @@ nv50_msto_enable(struct drm_encoder *encoder)
>  	struct nv50_mstm *mstm = NULL;
>  	struct drm_connector *connector;
>  	struct drm_connector_list_iter conn_iter;
> -	u8 proto, depth;
> +	u8 proto;
>  	bool r;
>  
>  	drm_connector_list_iter_begin(encoder->dev, &conn_iter);
> @@ -841,14 +862,8 @@ nv50_msto_enable(struct drm_encoder *encoder)
>  	else
>  		proto = 0x9;
>  
> -	switch (mstc->connector.display_info.bpc) {
> -	case  6: depth = 0x2; break;
> -	case  8: depth = 0x5; break;
> -	case 10:
> -	default: depth = 0x6; break;
> -	}
> -
> -	mstm->outp->update(mstm->outp, head->base.index, armh, proto, depth);
> +	mstm->outp->update(mstm->outp, head->base.index, armh, proto,
> +			   nv50_dp_bpc_to_depth(armh->or.bpc));
>  
>  	msto->head = head;
>  	msto->mstc = mstc;
> @@ -1502,20 +1517,14 @@ nv50_sor_enable(struct drm_encoder *encoder)
>  					lvds.lvds.script |= 0x0200;
>  			}
>  
> -			if (nv_connector->base.display_info.bpc == 8)
> +			if (asyh->or.bpc == 8)
>  				lvds.lvds.script |= 0x0200;
>  		}
>  
>  		nvif_mthd(&disp->disp->object, 0, &lvds, sizeof(lvds));
>  		break;
>  	case DCB_OUTPUT_DP:
> -		if (nv_connector->base.display_info.bpc == 6)
> -			depth = 0x2;
> -		else
> -		if (nv_connector->base.display_info.bpc == 8)
> -			depth = 0x5;
> -		else
> -			depth = 0x6;
> +		depth = nv50_dp_bpc_to_depth(asyh->or.bpc);
>  
>  		if (nv_encoder->link & 1)
>  			proto = 0x8;
> @@ -1666,7 +1675,7 @@ nv50_pior_enable(struct drm_encoder *encoder)
>  	nv50_outp_acquire(nv_encoder);
>  
>  	nv_connector = nouveau_encoder_connector_get(nv_encoder);
> -	switch (nv_connector->base.display_info.bpc) {
> +	switch (asyh->or.bpc) {
>  	case 10: asyh->or.depth = 0x6; break;
>  	case  8: asyh->or.depth = 0x5; break;
>  	case  6: asyh->or.depth = 0x2; break;
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/head.c b/drivers/gpu/drm/nouveau/dispnv50/head.c
> index 71c23bf1fe25..c9692df2b76c 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/head.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/head.c
> @@ -81,18 +81,17 @@ nv50_head_atomic_check_dither(struct nv50_head_atom *armh,
>  			      struct nv50_head_atom *asyh,
>  			      struct nouveau_conn_atom *asyc)
>  {
> -	struct drm_connector *connector = asyc->state.connector;
>  	u32 mode = 0x00;
>  
>  	if (asyc->dither.mode == DITHERING_MODE_AUTO) {
> -		if (asyh->base.depth > connector->display_info.bpc * 3)
> +		if (asyh->base.depth > asyh->or.bpc * 3)
>  			mode = DITHERING_MODE_DYNAMIC2X2;
>  	} else {
>  		mode = asyc->dither.mode;
>  	}
>  
>  	if (asyc->dither.depth == DITHERING_DEPTH_AUTO) {
> -		if (connector->display_info.bpc >= 8)
> +		if (asyh->or.bpc >= 8)
>  			mode |= DITHERING_DEPTH_8BPC;
>  	} else {
>  		mode |= asyc->dither.depth;
> -- 
> 2.21.0
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/3] drm/nouveau/kms/nv50-: Limit MST BPC to 8

[Lyude Paul]

Noticed this while working on some unrelated CRC stuff. Currently,
userspace has very little support for BPCs higher than 8. While this
doesn't matter for most things, on MST topologies we need to be careful
about ensuring that we do our best to make any given display
configuration fit within the bandwidth restraints of the topology, since
otherwise less people's monitor configurations will work.

Allowing for BPC settings higher than 8 dramatically increases the
required bandwidth for displays in most configurations, and consequently
makes it a lot less likely that said display configurations will pass
the atomic check.

In the future we want to fix this correctly by making it so that we
adjust the bpp for each display in a topology to be as high as possible,
while making sure to lower the bpp of each display in the event that we
run out of bandwidth and need to rerun our atomic check. But for now,
follow the behavior that both i915 and amdgpu are sticking to.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Fixes: 232c9eec417a ("drm/nouveau: Use atomic VCPI helpers for MST")
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: Sam Ravnborg <sam@ravnborg.org>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: <stable@vger.kernel.org> # v5.1+
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 93665aecce57..9ac47fe519f8 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -798,7 +798,14 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
 	if (!state->duplicated) {
 		const int clock = crtc_state->adjusted_mode.clock;
 
-		asyh->or.bpc = connector->display_info.bpc;
+		/*
+		 * XXX: Since we don't use HDR in userspace quite yet, limit
+		 * the bpc to 8 to save bandwidth on the topology. In the
+		 * future, we'll want to properly fix this by dynamically
+		 * selecting the highest possible bpc that would fit in the
+		 * topology
+		 */
+		asyh->or.bpc = min(connector->display_info.bpc, 8U);
 		asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3);
 	}
 
-- 
2.21.0

Re: [Nouveau] [PATCH 3/3] drm/nouveau/kms/nv50-: Limit MST BPC to 8

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]

On Fri, Nov 15, 2019 at 04:07:20PM -0500, Lyude Paul wrote:
> Noticed this while working on some unrelated CRC stuff. Currently,
> userspace has very little support for BPCs higher than 8. While this
> doesn't matter for most things, on MST topologies we need to be careful
> about ensuring that we do our best to make any given display
> configuration fit within the bandwidth restraints of the topology, since
> otherwise less people's monitor configurations will work.
> 
> Allowing for BPC settings higher than 8 dramatically increases the
> required bandwidth for displays in most configurations, and consequently
> makes it a lot less likely that said display configurations will pass
> the atomic check.
> 
> In the future we want to fix this correctly by making it so that we
> adjust the bpp for each display in a topology to be as high as possible,
> while making sure to lower the bpp of each display in the event that we
> run out of bandwidth and need to rerun our atomic check. But for now,
> follow the behavior that both i915 and amdgpu are sticking to.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Fixes: 232c9eec417a ("drm/nouveau: Use atomic VCPI helpers for MST")
> 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: Sam Ravnborg <sam@ravnborg.org>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: <stable@vger.kernel.org> # v5.1+
> ---
>  drivers/gpu/drm/nouveau/dispnv50/disp.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]