diff mbox series

[v2,19/34] drm/amd/display: decouple steps for mapping CRTC degamma to DC plane

Message ID 20230810160314.48225-20-mwen@igalia.com
State New
Headers
Series drm/amd/display: add AMD driver-specific properties for color mgmt |

Commit Message

Melissa Wen Aug. 10, 2023, 4:02 p.m. UTC 
  The next patch adds pre-blending degamma to AMD color mgmt pipeline, but
pre-blending degamma caps (DPP) is currently in use to provide DRM CRTC
atomic degamma or implict degamma on legacy gamma. Detach degamma usage
regarging CRTC color properties to manage plane and CRTC color
correction combinations.

Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 59 +++++++++++++------
 1 file changed, 41 insertions(+), 18 deletions(-)

Comments

Melissa Wen Aug. 28, 2023, 1:56 p.m. UTC | #1 
On 08/28, Pekka Paalanen wrote:
> On Mon, 28 Aug 2023 09:45:44 +0100
> Joshua Ashton <joshua@froggi.es> wrote:
> 
> > Degamma has always been on the plane on AMD. CRTC DEGAMMA_LUT has actually
> > just been applying it to every plane pre-blend.
> 
> I've never seen that documented anywhere.
> 
> It has seemed obvious, that since we have KMS objects for planes and
> CRTCs, stuff on the CRTC does not do plane stuff before blending. That
> also has not been documented in the past, but it seemed the most
> logical choice.
> 
> Even today
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#color-management-properties
> make no mention of whether they apply before or after blending.

It's mentioned in the next section:
https://dri.freedesktop.org/docs/drm/gpu/amdgpu/display/display-manager.html#dc-color-capabilities-between-dcn-generations
In hindsight, maybe it isn't the best place...

> 
> > Degamma makes no sense after blending anyway.
> 
> If the goal is to allow blending in optical or other space, you are
> correct. However, APIs do not need to make sense to exist, like most of
> the options of "Colorspace" connector property.
> 
> I have always thought the CRTC DEGAMMA only exists to allow the CRTC
> CTM to work in linear or other space.
> 
> I have at times been puzzled by what the DEGAMMA and CTM are actually
> good for.
> 
> > The entire point is for it to happen before blending to blend in linear
> > space. Otherwise DEGAMMA_LUT and REGAMMA_LUT are the exact same thing...
> 
> The CRTC CTM is between CRTC DEGAMMA and CRTC GAMMA, meaning they are
> not interchangeable.
> 
> I have literally believed that DRM KMS UAPI simply does not support
> blending in optical space, unless your framebuffers are in optical
> which no-one does, until the color management properties are added to
> KMS planes. This never even seemed weird, because non-linear blending
> is so common.
> 
> So I have been misunderstanding the CRTC DEGAMMA property forever. Am I
> the only one? Do all drivers agree today at what point does CRTC
> DEGAMMA apply, before blending on all planes or after blending?
> 

I'd like to know current userspace cases on Linux of this CRTC DEGAMMA
LUT.

> Does anyone know of any doc about that?

From what I retrieved about the introduction of CRTC color props[1], it
seems the main concern at that point was getting a linear space for
CTM[2] and CRTC degamma property seems to have followed intel
requirements, but didn't find anything about the blending space.

AFAIU, we have just interpreted that all CRTC color properties for DRM
interface are after blending[3]. Can this be seen in another way?

[1] https://patchwork.freedesktop.org/series/2720/
[2] https://codereview.chromium.org/1182063002
[3] https://dri.freedesktop.org/docs/drm/_images/dcn3_cm_drm_current.svg

> 
> If drivers do not agree on the behaviour of a KMS property, then that
> property is useless for generic userspace.
> 
> 
> Thanks,
> pq
> 
> 
> > On Tuesday, 22 August 2023, Pekka Paalanen <pekka.paalanen@collabora.com>
> > wrote:
> > > On Thu, 10 Aug 2023 15:02:59 -0100
> > > Melissa Wen <mwen@igalia.com> wrote:
> > >  
> > >> The next patch adds pre-blending degamma to AMD color mgmt pipeline, but
> > >> pre-blending degamma caps (DPP) is currently in use to provide DRM CRTC
> > >> atomic degamma or implict degamma on legacy gamma. Detach degamma usage
> > >> regarging CRTC color properties to manage plane and CRTC color
> > >> correction combinations.
> > >>
> > >> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> > >> Signed-off-by: Melissa Wen <mwen@igalia.com>
> > >> ---
> > >>  .../amd/display/amdgpu_dm/amdgpu_dm_color.c   | 59 +++++++++++++------
> > >>  1 file changed, 41 insertions(+), 18 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c  
> > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> > >> index 68e9f2c62f2e..74eb02655d96 100644
> > >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> > >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
> > >> @@ -764,20 +764,9 @@ int amdgpu_dm_update_crtc_color_mgmt(struct  
> > dm_crtc_state *crtc)
> > >>       return 0;
> > >>  }
> > >>
> > >> -/**
> > >> - * amdgpu_dm_update_plane_color_mgmt: Maps DRM color management to DC  
> > plane.
> > >> - * @crtc: amdgpu_dm crtc state
> > >> - * @dc_plane_state: target DC surface
> > >> - *
> > >> - * Update the underlying dc_stream_state's input transfer function  
> > (ITF) in
> > >> - * preparation for hardware commit. The transfer function used depends  
> > on
> > >> - * the preparation done on the stream for color management.
> > >> - *
> > >> - * Returns:
> > >> - * 0 on success. -ENOMEM if mem allocation fails.
> > >> - */
> > >> -int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
> > >> -                                   struct dc_plane_state  
> > *dc_plane_state)
> > >> +static int
> > >> +map_crtc_degamma_to_dc_plane(struct dm_crtc_state *crtc,
> > >> +                          struct dc_plane_state *dc_plane_state)
> > >>  {
> > >>       const struct drm_color_lut *degamma_lut;
> > >>       enum dc_transfer_func_predefined tf = TRANSFER_FUNCTION_SRGB;
> > >> @@ -800,8 +789,7 @@ int amdgpu_dm_update_plane_color_mgmt(struct  
> > dm_crtc_state *crtc,
> > >>                                                &degamma_size);
> > >>               ASSERT(degamma_size == MAX_COLOR_LUT_ENTRIES);
> > >>
> > >> -             dc_plane_state->in_transfer_func->type =
> > >> -                     TF_TYPE_DISTRIBUTED_POINTS;
> > >> +             dc_plane_state->in_transfer_func->type =  
> > TF_TYPE_DISTRIBUTED_POINTS;
> > >>
> > >>               /*
> > >>                * This case isn't fully correct, but also fairly
> > >> @@ -837,7 +825,7 @@ int amdgpu_dm_update_plane_color_mgmt(struct  
> > dm_crtc_state *crtc,
> > >>                                  degamma_lut, degamma_size);
> > >>               if (r)
> > >>                       return r;
> > >> -     } else if (crtc->cm_is_degamma_srgb) {
> > >> +     } else {
> > >>               /*
> > >>                * For legacy gamma support we need the regamma input
> > >>                * in linear space. Assume that the input is sRGB.
> > >> @@ -847,8 +835,43 @@ int amdgpu_dm_update_plane_color_mgmt(struct  
> > dm_crtc_state *crtc,
> > >>
> > >>               if (tf != TRANSFER_FUNCTION_SRGB &&
> > >>                   !mod_color_calculate_degamma_params(NULL,
> > >> -                         dc_plane_state->in_transfer_func, NULL, false))
> > >> +  
> >  dc_plane_state->in_transfer_func,
> > >> +                                                     NULL, false))
> > >>                       return -ENOMEM;
> > >> +     }
> > >> +
> > >> +     return 0;
> > >> +}
> > >> +
> > >> +/**
> > >> + * amdgpu_dm_update_plane_color_mgmt: Maps DRM color management to DC  
> > plane.
> > >> + * @crtc: amdgpu_dm crtc state
> > >> + * @dc_plane_state: target DC surface
> > >> + *
> > >> + * Update the underlying dc_stream_state's input transfer function  
> > (ITF) in
> > >> + * preparation for hardware commit. The transfer function used depends  
> > on
> > >> + * the preparation done on the stream for color management.
> > >> + *
> > >> + * Returns:
> > >> + * 0 on success. -ENOMEM if mem allocation fails.
> > >> + */
> > >> +int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
> > >> +                                   struct dc_plane_state  
> > *dc_plane_state)
> > >> +{
> > >> +     bool has_crtc_cm_degamma;
> > >> +     int ret;
> > >> +
> > >> +     has_crtc_cm_degamma = (crtc->cm_has_degamma ||  
> > crtc->cm_is_degamma_srgb);
> > >> +     if (has_crtc_cm_degamma){
> > >> +             /* AMD HW doesn't have post-blending degamma caps. When DRM
> > >> +              * CRTC atomic degamma is set, we maps it to DPP degamma  
> > block
> > >> +              * (pre-blending) or, on legacy gamma, we use DPP degamma  
> > to
> > >> +              * linearize (implicit degamma) from sRGB/BT709 according  
> > to
> > >> +              * the input space.  
> > >
> > > Uhh, you can't just move degamma before blending if KMS userspace
> > > wants it after blending. That would be incorrect behaviour. If you
> > > can't implement it correctly, reject it.
> > >
> > > I hope that magical unexpected linearization is not done with atomic,
> > > either.
> > >
> > > Or maybe this is all a lost cause, and only the new color-op pipeline
> > > UAPI will actually work across drivers.
> > >
> > >
> > > Thanks,
> > > pq
> > >  
> > >> +              */
> > >> +             ret = map_crtc_degamma_to_dc_plane(crtc, dc_plane_state);
> > >> +             if (ret)
> > >> +                     return ret;
> > >>       } else {
> > >>               /* ...Otherwise we can just bypass the DGM block. */
> > >>               dc_plane_state->in_transfer_func->type = TF_TYPE_BYPASS;  
> > >
> > >  
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
index 68e9f2c62f2e..74eb02655d96 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c
@@ -764,20 +764,9 @@  int amdgpu_dm_update_crtc_color_mgmt(struct dm_crtc_state *crtc)
 	return 0;
 }
 
-/**
- * amdgpu_dm_update_plane_color_mgmt: Maps DRM color management to DC plane.
- * @crtc: amdgpu_dm crtc state
- * @dc_plane_state: target DC surface
- *
- * Update the underlying dc_stream_state's input transfer function (ITF) in
- * preparation for hardware commit. The transfer function used depends on
- * the preparation done on the stream for color management.
- *
- * Returns:
- * 0 on success. -ENOMEM if mem allocation fails.
- */
-int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
-				      struct dc_plane_state *dc_plane_state)
+static int
+map_crtc_degamma_to_dc_plane(struct dm_crtc_state *crtc,
+			     struct dc_plane_state *dc_plane_state)
 {
 	const struct drm_color_lut *degamma_lut;
 	enum dc_transfer_func_predefined tf = TRANSFER_FUNCTION_SRGB;
@@ -800,8 +789,7 @@  int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
 						 &degamma_size);
 		ASSERT(degamma_size == MAX_COLOR_LUT_ENTRIES);
 
-		dc_plane_state->in_transfer_func->type =
-			TF_TYPE_DISTRIBUTED_POINTS;
+		dc_plane_state->in_transfer_func->type = TF_TYPE_DISTRIBUTED_POINTS;
 
 		/*
 		 * This case isn't fully correct, but also fairly
@@ -837,7 +825,7 @@  int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
 				   degamma_lut, degamma_size);
 		if (r)
 			return r;
-	} else if (crtc->cm_is_degamma_srgb) {
+	} else {
 		/*
 		 * For legacy gamma support we need the regamma input
 		 * in linear space. Assume that the input is sRGB.
@@ -847,8 +835,43 @@  int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
 
 		if (tf != TRANSFER_FUNCTION_SRGB &&
 		    !mod_color_calculate_degamma_params(NULL,
-			    dc_plane_state->in_transfer_func, NULL, false))
+							dc_plane_state->in_transfer_func,
+							NULL, false))
 			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+/**
+ * amdgpu_dm_update_plane_color_mgmt: Maps DRM color management to DC plane.
+ * @crtc: amdgpu_dm crtc state
+ * @dc_plane_state: target DC surface
+ *
+ * Update the underlying dc_stream_state's input transfer function (ITF) in
+ * preparation for hardware commit. The transfer function used depends on
+ * the preparation done on the stream for color management.
+ *
+ * Returns:
+ * 0 on success. -ENOMEM if mem allocation fails.
+ */
+int amdgpu_dm_update_plane_color_mgmt(struct dm_crtc_state *crtc,
+				      struct dc_plane_state *dc_plane_state)
+{
+	bool has_crtc_cm_degamma;
+	int ret;
+
+	has_crtc_cm_degamma = (crtc->cm_has_degamma || crtc->cm_is_degamma_srgb);
+	if (has_crtc_cm_degamma){
+		/* AMD HW doesn't have post-blending degamma caps. When DRM
+		 * CRTC atomic degamma is set, we maps it to DPP degamma block
+		 * (pre-blending) or, on legacy gamma, we use DPP degamma to
+		 * linearize (implicit degamma) from sRGB/BT709 according to
+		 * the input space.
+		 */
+		ret = map_crtc_degamma_to_dc_plane(crtc, dc_plane_state);
+		if (ret)
+			return ret;
 	} else {
 		/* ...Otherwise we can just bypass the DGM block. */
 		dc_plane_state->in_transfer_func->type = TF_TYPE_BYPASS;