From: Thierry Reding <thierry.reding@gmail.com> To: Lyude Paul <lyude@redhat.com> Cc: nouveau@lists.freedesktop.org, Sam Ravnborg <sam@ravnborg.org>, David Airlie <airlied@linux.ie>, dri-devel@lists.freedesktop.org, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, linux-kernel@vger.kernel.org, stable@vger.kernel.org, Jerry Zuo <Jerry.Zuo@amd.com>, Sean Paul <seanpaul@chromium.org>, Ben Skeggs <bskeggs@redhat.com>, Daniel Vetter <daniel@ffwll.ch>, Peteris Rudzusiks <peteris.rudzusiks@gmail.com>, David Airlie <airlied@redhat.com>, Harry Wentland <harry.wentland@amd.com>, Juston Li <juston.li@intel.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com> Subject: Re: [Nouveau] [PATCH 2/3] drm/nouveau/kms/nv50-: Store the bpc we're using in nv50_head_atom Date: Mon, 18 Nov 2019 14:35:07 +0100 [thread overview] Message-ID: <20191118133507.GD2246533@ulmo> (raw) In-Reply-To: <20191115210728.3467-3-lyude@redhat.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 --]
WARNING: multiple messages have this Message-ID (diff)
From: Thierry Reding <thierry.reding@gmail.com> To: Lyude Paul <lyude@redhat.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>, David Airlie <airlied@linux.ie>, nouveau@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Jerry Zuo <Jerry.Zuo@amd.com>, Sean Paul <seanpaul@chromium.org>, stable@vger.kernel.org, Peteris Rudzusiks <peteris.rudzusiks@gmail.com>, David Airlie <airlied@redhat.com>, Sam Ravnborg <sam@ravnborg.org>, Juston Li <juston.li@intel.com>, Ben Skeggs <bskeggs@redhat.com> Subject: Re: [Nouveau] [PATCH 2/3] drm/nouveau/kms/nv50-: Store the bpc we're using in nv50_head_atom Date: Mon, 18 Nov 2019 14:35:07 +0100 [thread overview] Message-ID: <20191118133507.GD2246533@ulmo> (raw) Message-ID: <20191118133507._A2fTBIP65dKGv7DkEQR4LiudhmIb33xMhXCTTQesBI@z> (raw) In-Reply-To: <20191115210728.3467-3-lyude@redhat.com> [-- Attachment #1.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 #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-11-18 13:35 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-15 21:07 [PATCH 0/3] MST BPC fixes for nouveau Lyude Paul 2019-11-15 21:07 ` Lyude Paul 2019-11-15 21:07 ` [PATCH 1/3] drm/nouveau/kms/nv50-: Call outp_atomic_check_view() before handling PBN Lyude Paul 2019-11-15 21:07 ` Lyude Paul 2019-11-18 13:41 ` [Nouveau] " Thierry Reding 2019-11-18 13:41 ` Thierry Reding 2019-11-15 21:07 ` [PATCH 2/3] drm/nouveau/kms/nv50-: Store the bpc we're using in nv50_head_atom Lyude Paul 2019-11-15 21:07 ` Lyude Paul 2019-11-18 13:35 ` Thierry Reding [this message] 2019-11-18 13:35 ` [Nouveau] " Thierry Reding 2019-11-15 21:07 ` [PATCH 3/3] drm/nouveau/kms/nv50-: Limit MST BPC to 8 Lyude Paul 2019-11-15 21:07 ` Lyude Paul 2019-11-18 13:36 ` [Nouveau] " Thierry Reding 2019-11-18 13:36 ` Thierry Reding
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191118133507.GD2246533@ulmo \ --to=thierry.reding@gmail.com \ --cc=Jerry.Zuo@amd.com \ --cc=airlied@linux.ie \ --cc=airlied@redhat.com \ --cc=bskeggs@redhat.com \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=harry.wentland@amd.com \ --cc=juston.li@intel.com \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-kernel@vger.kernel.org \ --cc=lyude@redhat.com \ --cc=maarten.lankhorst@linux.intel.com \ --cc=nouveau@lists.freedesktop.org \ --cc=peteris.rudzusiks@gmail.com \ --cc=sam@ravnborg.org \ --cc=seanpaul@chromium.org \ --cc=stable@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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).