Re: [PATCH v2 31/34] drm/amd/display: set stream gamut remap matrix to MPC for DCN301
On 2023-08-28 04:20, Pekka Paalanen wrote: > On Fri, 25 Aug 2023 13:37:08 -0100 > Melissa Wen wrote: > >> On 08/22, Pekka Paalanen wrote: >>> On Thu, 10 Aug 2023 15:03:11 -0100 >>> Melissa Wen wrote: >>> dc->caps.color.mpc.gamut_remap says there is a post-blending color block for gamut remap matrix for DCN3 HW family and newer versions. However, those drivers still follow DCN10 programming that remap stream gamut_remap_matrix to DPP (pre-blending). >>> >>> That's ok only as long as CRTC degamma is pass-through. Blending itself >>> is a linear operation, so it doesn't matter if a matrix is applied to >>> the blending result or to all blending inputs. But you cannot move a >>> matrix operation to the other side of a non-linear operation, and you >>> cannot move a non-linear operation across blending. >> >> Oh, I'm not moving it, what I'm doing here is the opposite and fixing >> it. This patch puts each pre- and post-blending CTM in their right >> place, since we have the HW caps for it on DCN3+... Or are you just >> pointing out the implementation mistake on old driver versions? > > It's just the old mistake. > > I hope no-one complains, forcing you to revert this fix as a regression. > I'm worried this will break other OSes since its in DC and shared. I'll check with Kruno when he's back from vacation. But most likely this will be problematic. Worst case we can add a new "program_gamut_remap_actually_post_blending" (with a better name) function to HWSS, expose it in DC, and make sure amdgpu_dm never calls the old "program_gamut_remap". I hope nobody relies on the current (IMO broken) behavior on Linux. Harry > > Thanks, > pq > > To enable pre-blending and post-blending gamut_remap matrix supports at the same time, set stream gamut_remap to MPC and plane gamut_remap to DPP for DCN301 that support both. It was tested using IGT KMS color tests for DRM CRTC CTM property and it preserves test results. Signed-off-by: Melissa Wen --- .../drm/amd/display/dc/dcn30/dcn30_hwseq.c| 37 +++ .../drm/amd/display/dc/dcn30/dcn30_hwseq.h| 3 ++ .../drm/amd/display/dc/dcn301/dcn301_init.c | 2 +- 3 files changed, 41 insertions(+), 1 deletion(-)
Re: [PATCH v2 31/34] drm/amd/display: set stream gamut remap matrix to MPC for DCN301
On Fri, 25 Aug 2023 13:37:08 -0100 Melissa Wen wrote: > On 08/22, Pekka Paalanen wrote: > > On Thu, 10 Aug 2023 15:03:11 -0100 > > Melissa Wen wrote: > > > > > dc->caps.color.mpc.gamut_remap says there is a post-blending color block > > > for gamut remap matrix for DCN3 HW family and newer versions. However, > > > those drivers still follow DCN10 programming that remap stream > > > gamut_remap_matrix to DPP (pre-blending). > > > > That's ok only as long as CRTC degamma is pass-through. Blending itself > > is a linear operation, so it doesn't matter if a matrix is applied to > > the blending result or to all blending inputs. But you cannot move a > > matrix operation to the other side of a non-linear operation, and you > > cannot move a non-linear operation across blending. > > Oh, I'm not moving it, what I'm doing here is the opposite and fixing > it. This patch puts each pre- and post-blending CTM in their right > place, since we have the HW caps for it on DCN3+... Or are you just > pointing out the implementation mistake on old driver versions? It's just the old mistake. I hope no-one complains, forcing you to revert this fix as a regression. Thanks, pq > > > To enable pre-blending and post-blending gamut_remap matrix supports at > > > the same time, set stream gamut_remap to MPC and plane gamut_remap to > > > DPP for DCN301 that support both. > > > > > > It was tested using IGT KMS color tests for DRM CRTC CTM property and it > > > preserves test results. > > > > > > Signed-off-by: Melissa Wen > > > --- > > > .../drm/amd/display/dc/dcn30/dcn30_hwseq.c| 37 +++ > > > .../drm/amd/display/dc/dcn30/dcn30_hwseq.h| 3 ++ > > > .../drm/amd/display/dc/dcn301/dcn301_init.c | 2 +- > > > 3 files changed, 41 insertions(+), 1 deletion(-) pgp_Lo7dCqd2A.pgp Description: OpenPGP digital signature
Re: [PATCH v2 31/34] drm/amd/display: set stream gamut remap matrix to MPC for DCN301
On 08/22, Pekka Paalanen wrote:
> On Thu, 10 Aug 2023 15:03:11 -0100
> Melissa Wen wrote:
>
> > dc->caps.color.mpc.gamut_remap says there is a post-blending color block
> > for gamut remap matrix for DCN3 HW family and newer versions. However,
> > those drivers still follow DCN10 programming that remap stream
> > gamut_remap_matrix to DPP (pre-blending).
>
> That's ok only as long as CRTC degamma is pass-through. Blending itself
> is a linear operation, so it doesn't matter if a matrix is applied to
> the blending result or to all blending inputs. But you cannot move a
> matrix operation to the other side of a non-linear operation, and you
> cannot move a non-linear operation across blending.
Oh, I'm not moving it, what I'm doing here is the opposite and fixing
it. This patch puts each pre- and post-blending CTM in their right
place, since we have the HW caps for it on DCN3+... Or are you just
pointing out the implementation mistake on old driver versions?
>
>
> Thanks,
> pq
>
> > To enable pre-blending and post-blending gamut_remap matrix supports at
> > the same time, set stream gamut_remap to MPC and plane gamut_remap to
> > DPP for DCN301 that support both.
> >
> > It was tested using IGT KMS color tests for DRM CRTC CTM property and it
> > preserves test results.
> >
> > Signed-off-by: Melissa Wen
> > ---
> > .../drm/amd/display/dc/dcn30/dcn30_hwseq.c| 37 +++
> > .../drm/amd/display/dc/dcn30/dcn30_hwseq.h| 3 ++
> > .../drm/amd/display/dc/dcn301/dcn301_init.c | 2 +-
> > 3 files changed, 41 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
> > b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
> > index 4cd4ae07d73d..4fb4e9ec03f1 100644
> > --- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
> > +++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
> > @@ -186,6 +186,43 @@ bool dcn30_set_input_transfer_func(struct dc *dc,
> > return result;
> > }
> >
> > +void dcn30_program_gamut_remap(struct pipe_ctx *pipe_ctx)
> > +{
> > + int i = 0;
> > + struct dpp_grph_csc_adjustment dpp_adjust;
> > + struct mpc_grph_gamut_adjustment mpc_adjust;
> > + int mpcc_id = pipe_ctx->plane_res.hubp->inst;
> > + struct mpc *mpc = pipe_ctx->stream_res.opp->ctx->dc->res_pool->mpc;
> > +
> > + memset(_adjust, 0, sizeof(dpp_adjust));
> > + dpp_adjust.gamut_adjust_type = GRAPHICS_GAMUT_ADJUST_TYPE_BYPASS;
> > +
> > + if (pipe_ctx->plane_state &&
> > + pipe_ctx->plane_state->gamut_remap_matrix.enable_remap == true) {
> > + dpp_adjust.gamut_adjust_type = GRAPHICS_GAMUT_ADJUST_TYPE_SW;
> > + for (i = 0; i < CSC_TEMPERATURE_MATRIX_SIZE; i++)
> > + dpp_adjust.temperature_matrix[i] =
> > +
> > pipe_ctx->plane_state->gamut_remap_matrix.matrix[i];
> > + }
> > +
> > +
> > pipe_ctx->plane_res.dpp->funcs->dpp_set_gamut_remap(pipe_ctx->plane_res.dpp,
> > + _adjust);
> > +
> > + memset(_adjust, 0, sizeof(mpc_adjust));
> > + mpc_adjust.gamut_adjust_type = GRAPHICS_GAMUT_ADJUST_TYPE_BYPASS;
> > +
> > + if (pipe_ctx->top_pipe == NULL) {
> > + if (pipe_ctx->stream->gamut_remap_matrix.enable_remap == true) {
> > + mpc_adjust.gamut_adjust_type =
> > GRAPHICS_GAMUT_ADJUST_TYPE_SW;
> > + for (i = 0; i < CSC_TEMPERATURE_MATRIX_SIZE; i++)
> > + mpc_adjust.temperature_matrix[i] =
> > +
> > pipe_ctx->stream->gamut_remap_matrix.matrix[i];
> > + }
> > + }
> > +
> > + mpc->funcs->set_gamut_remap(mpc, mpcc_id, _adjust);
> > +}
> > +
> > bool dcn30_set_output_transfer_func(struct dc *dc,
> > struct pipe_ctx *pipe_ctx,
> > const struct dc_stream_state *stream)
> > diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.h
> > b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.h
> > index a24a8e33a3d2..cb34ca932a5f 100644
> > --- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.h
> > +++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.h
> > @@ -58,6 +58,9 @@ bool dcn30_set_blend_lut(struct pipe_ctx *pipe_ctx,
> > bool dcn30_set_input_transfer_func(struct dc *dc,
> > struct pipe_ctx *pipe_ctx,
> > const struct dc_plane_state *plane_state);
> > +
> > +void dcn30_program_gamut_remap(struct pipe_ctx *pipe_ctx);
> > +
> > bool dcn30_set_output_transfer_func(struct dc *dc,
> > struct pipe_ctx *pipe_ctx,
> > const struct dc_stream_state *stream);
> > diff --git a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_init.c
> > b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_init.c
> > index 257df8660b4c..81fd50ee97c3 100644
> > --- a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_init.c
> > +++
Re: [PATCH v2 31/34] drm/amd/display: set stream gamut remap matrix to MPC for DCN301
On Thu, 10 Aug 2023 15:03:11 -0100
Melissa Wen wrote:
> dc->caps.color.mpc.gamut_remap says there is a post-blending color block
> for gamut remap matrix for DCN3 HW family and newer versions. However,
> those drivers still follow DCN10 programming that remap stream
> gamut_remap_matrix to DPP (pre-blending).
That's ok only as long as CRTC degamma is pass-through. Blending itself
is a linear operation, so it doesn't matter if a matrix is applied to
the blending result or to all blending inputs. But you cannot move a
matrix operation to the other side of a non-linear operation, and you
cannot move a non-linear operation across blending.
Thanks,
pq
> To enable pre-blending and post-blending gamut_remap matrix supports at
> the same time, set stream gamut_remap to MPC and plane gamut_remap to
> DPP for DCN301 that support both.
>
> It was tested using IGT KMS color tests for DRM CRTC CTM property and it
> preserves test results.
>
> Signed-off-by: Melissa Wen
> ---
> .../drm/amd/display/dc/dcn30/dcn30_hwseq.c| 37 +++
> .../drm/amd/display/dc/dcn30/dcn30_hwseq.h| 3 ++
> .../drm/amd/display/dc/dcn301/dcn301_init.c | 2 +-
> 3 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
> b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
> index 4cd4ae07d73d..4fb4e9ec03f1 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
> @@ -186,6 +186,43 @@ bool dcn30_set_input_transfer_func(struct dc *dc,
> return result;
> }
>
> +void dcn30_program_gamut_remap(struct pipe_ctx *pipe_ctx)
> +{
> + int i = 0;
> + struct dpp_grph_csc_adjustment dpp_adjust;
> + struct mpc_grph_gamut_adjustment mpc_adjust;
> + int mpcc_id = pipe_ctx->plane_res.hubp->inst;
> + struct mpc *mpc = pipe_ctx->stream_res.opp->ctx->dc->res_pool->mpc;
> +
> + memset(_adjust, 0, sizeof(dpp_adjust));
> + dpp_adjust.gamut_adjust_type = GRAPHICS_GAMUT_ADJUST_TYPE_BYPASS;
> +
> + if (pipe_ctx->plane_state &&
> + pipe_ctx->plane_state->gamut_remap_matrix.enable_remap == true) {
> + dpp_adjust.gamut_adjust_type = GRAPHICS_GAMUT_ADJUST_TYPE_SW;
> + for (i = 0; i < CSC_TEMPERATURE_MATRIX_SIZE; i++)
> + dpp_adjust.temperature_matrix[i] =
> +
> pipe_ctx->plane_state->gamut_remap_matrix.matrix[i];
> + }
> +
> +
> pipe_ctx->plane_res.dpp->funcs->dpp_set_gamut_remap(pipe_ctx->plane_res.dpp,
> + _adjust);
> +
> + memset(_adjust, 0, sizeof(mpc_adjust));
> + mpc_adjust.gamut_adjust_type = GRAPHICS_GAMUT_ADJUST_TYPE_BYPASS;
> +
> + if (pipe_ctx->top_pipe == NULL) {
> + if (pipe_ctx->stream->gamut_remap_matrix.enable_remap == true) {
> + mpc_adjust.gamut_adjust_type =
> GRAPHICS_GAMUT_ADJUST_TYPE_SW;
> + for (i = 0; i < CSC_TEMPERATURE_MATRIX_SIZE; i++)
> + mpc_adjust.temperature_matrix[i] =
> +
> pipe_ctx->stream->gamut_remap_matrix.matrix[i];
> + }
> + }
> +
> + mpc->funcs->set_gamut_remap(mpc, mpcc_id, _adjust);
> +}
> +
> bool dcn30_set_output_transfer_func(struct dc *dc,
> struct pipe_ctx *pipe_ctx,
> const struct dc_stream_state *stream)
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.h
> b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.h
> index a24a8e33a3d2..cb34ca932a5f 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.h
> +++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.h
> @@ -58,6 +58,9 @@ bool dcn30_set_blend_lut(struct pipe_ctx *pipe_ctx,
> bool dcn30_set_input_transfer_func(struct dc *dc,
> struct pipe_ctx *pipe_ctx,
> const struct dc_plane_state *plane_state);
> +
> +void dcn30_program_gamut_remap(struct pipe_ctx *pipe_ctx);
> +
> bool dcn30_set_output_transfer_func(struct dc *dc,
> struct pipe_ctx *pipe_ctx,
> const struct dc_stream_state *stream);
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_init.c
> b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_init.c
> index 257df8660b4c..81fd50ee97c3 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_init.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_init.c
> @@ -33,7 +33,7 @@
> #include "dcn301_init.h"
>
> static const struct hw_sequencer_funcs dcn301_funcs = {
> - .program_gamut_remap = dcn10_program_gamut_remap,
> + .program_gamut_remap = dcn30_program_gamut_remap,
> .init_hw = dcn10_init_hw,
> .power_down_on_boot = dcn10_power_down_on_boot,
> .apply_ctx_to_hw = dce110_apply_ctx_to_hw,
[PATCH v2 31/34] drm/amd/display: set stream gamut remap matrix to MPC for DCN301
dc->caps.color.mpc.gamut_remap says there is a post-blending color block
for gamut remap matrix for DCN3 HW family and newer versions. However,
those drivers still follow DCN10 programming that remap stream
gamut_remap_matrix to DPP (pre-blending).
To enable pre-blending and post-blending gamut_remap matrix supports at
the same time, set stream gamut_remap to MPC and plane gamut_remap to
DPP for DCN301 that support both.
It was tested using IGT KMS color tests for DRM CRTC CTM property and it
preserves test results.
Signed-off-by: Melissa Wen
---
.../drm/amd/display/dc/dcn30/dcn30_hwseq.c| 37 +++
.../drm/amd/display/dc/dcn30/dcn30_hwseq.h| 3 ++
.../drm/amd/display/dc/dcn301/dcn301_init.c | 2 +-
3 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
index 4cd4ae07d73d..4fb4e9ec03f1 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.c
@@ -186,6 +186,43 @@ bool dcn30_set_input_transfer_func(struct dc *dc,
return result;
}
+void dcn30_program_gamut_remap(struct pipe_ctx *pipe_ctx)
+{
+ int i = 0;
+ struct dpp_grph_csc_adjustment dpp_adjust;
+ struct mpc_grph_gamut_adjustment mpc_adjust;
+ int mpcc_id = pipe_ctx->plane_res.hubp->inst;
+ struct mpc *mpc = pipe_ctx->stream_res.opp->ctx->dc->res_pool->mpc;
+
+ memset(_adjust, 0, sizeof(dpp_adjust));
+ dpp_adjust.gamut_adjust_type = GRAPHICS_GAMUT_ADJUST_TYPE_BYPASS;
+
+ if (pipe_ctx->plane_state &&
+ pipe_ctx->plane_state->gamut_remap_matrix.enable_remap == true) {
+ dpp_adjust.gamut_adjust_type = GRAPHICS_GAMUT_ADJUST_TYPE_SW;
+ for (i = 0; i < CSC_TEMPERATURE_MATRIX_SIZE; i++)
+ dpp_adjust.temperature_matrix[i] =
+
pipe_ctx->plane_state->gamut_remap_matrix.matrix[i];
+ }
+
+
pipe_ctx->plane_res.dpp->funcs->dpp_set_gamut_remap(pipe_ctx->plane_res.dpp,
+ _adjust);
+
+ memset(_adjust, 0, sizeof(mpc_adjust));
+ mpc_adjust.gamut_adjust_type = GRAPHICS_GAMUT_ADJUST_TYPE_BYPASS;
+
+ if (pipe_ctx->top_pipe == NULL) {
+ if (pipe_ctx->stream->gamut_remap_matrix.enable_remap == true) {
+ mpc_adjust.gamut_adjust_type =
GRAPHICS_GAMUT_ADJUST_TYPE_SW;
+ for (i = 0; i < CSC_TEMPERATURE_MATRIX_SIZE; i++)
+ mpc_adjust.temperature_matrix[i] =
+
pipe_ctx->stream->gamut_remap_matrix.matrix[i];
+ }
+ }
+
+ mpc->funcs->set_gamut_remap(mpc, mpcc_id, _adjust);
+}
+
bool dcn30_set_output_transfer_func(struct dc *dc,
struct pipe_ctx *pipe_ctx,
const struct dc_stream_state *stream)
diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.h
b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.h
index a24a8e33a3d2..cb34ca932a5f 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.h
+++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_hwseq.h
@@ -58,6 +58,9 @@ bool dcn30_set_blend_lut(struct pipe_ctx *pipe_ctx,
bool dcn30_set_input_transfer_func(struct dc *dc,
struct pipe_ctx *pipe_ctx,
const struct dc_plane_state *plane_state);
+
+void dcn30_program_gamut_remap(struct pipe_ctx *pipe_ctx);
+
bool dcn30_set_output_transfer_func(struct dc *dc,
struct pipe_ctx *pipe_ctx,
const struct dc_stream_state *stream);
diff --git a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_init.c
b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_init.c
index 257df8660b4c..81fd50ee97c3 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_init.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_init.c
@@ -33,7 +33,7 @@
#include "dcn301_init.h"
static const struct hw_sequencer_funcs dcn301_funcs = {
- .program_gamut_remap = dcn10_program_gamut_remap,
+ .program_gamut_remap = dcn30_program_gamut_remap,
.init_hw = dcn10_init_hw,
.power_down_on_boot = dcn10_power_down_on_boot,
.apply_ctx_to_hw = dce110_apply_ctx_to_hw,
--
2.40.1
