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

[Thierry Reding <thierry.reding@gmail.com>]

[-- 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 --]