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.
+		 */
+		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;
-- 
2.40.1

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;

On 08/22, Pekka Paalanen 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.

I agree that crtc degamma is an optional property and should be not
exposed if not available.  I did something in this line for DCE that has
no degamma block[1].  Then, AMD DDX driver stopped to advertise atomic
API for DCE, that was not correct too[2].

But I see it as a lost cause that will only be fixed in a new generic
color API. I don't think we should change it using the current DRM CRTC
API with driver-specific props.

[1] https://lore.kernel.org/amd-gfx/20221103184500.14450-1-mwen@igalia.com/
[2] https://gitlab.freedesktop.org/xorg/driver/xf86-video-amdgpu/-/issues/67
> 
> 
> 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;
> 

On Fri, 25 Aug 2023 13:29:44 -0100
Melissa Wen <mwen@igalia.com> wrote:

> On 08/22, Pekka Paalanen 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.  
> 
> I agree that crtc degamma is an optional property and should be not
> exposed if not available.  I did something in this line for DCE that has
> no degamma block[1].  Then, AMD DDX driver stopped to advertise atomic
> API for DCE, that was not correct too[2].

Did AMD go through all the trouble of making their Xorg DDX use KMS
atomic, even after the kernel took it away from X due to modesetting
DDX screwing it up? I'm surprised, what did that achieve?

I saw that between 5.15 and 6.1 amdgpu stopped advertising CRTC DEGAMMA
on my card, which seems like the right thing to do if there is no
hardware for it.

> But I see it as a lost cause that will only be fixed in a new generic
> color API. I don't think we should change it using the current DRM CRTC
> API with driver-specific props.
> 
> [1] https://lore.kernel.org/amd-gfx/20221103184500.14450-1-mwen@igalia.com/
> [2] https://gitlab.freedesktop.org/xorg/driver/xf86-video-amdgpu/-/issues/67

Oh well.

Is the old CRTC GAMMA property still "safe" to use in that it is
definitely always after blending?


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;  
> >   

On 2023-08-28 04:17, Pekka Paalanen wrote:
> On Fri, 25 Aug 2023 13:29:44 -0100
> Melissa Wen <mwen@igalia.com> wrote:
> 
>> On 08/22, Pekka Paalanen 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.  
>>
>> I agree that crtc degamma is an optional property and should be not
>> exposed if not available.  I did something in this line for DCE that has
>> no degamma block[1].  Then, AMD DDX driver stopped to advertise atomic
>> API for DCE, that was not correct too[2].
> 
> Did AMD go through all the trouble of making their Xorg DDX use KMS
> atomic, even after the kernel took it away from X due to modesetting
> DDX screwing it up? I'm surprised, what did that achieve?
> 
> I saw that between 5.15 and 6.1 amdgpu stopped advertising CRTC DEGAMMA
> on my card, which seems like the right thing to do if there is no
> hardware for it.
> 
>> But I see it as a lost cause that will only be fixed in a new generic
>> color API. I don't think we should change it using the current DRM CRTC
>> API with driver-specific props.
>>
>> [1] https://lore.kernel.org/amd-gfx/20221103184500.14450-1-mwen@igalia.com/
>> [2] https://gitlab.freedesktop.org/xorg/driver/xf86-video-amdgpu/-/issues/67
> 
> Oh well.
> 
> Is the old CRTC GAMMA property still "safe" to use in that it is
> definitely always after blending?
> 

CRTC GAMMA is always post-blending. DEGAMMA and CTM are not always
post-blending and need to be fixed (or removed).

Harry

> 
> 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;  
>>>   
>

On 8/28/23 10:17, Pekka Paalanen wrote:
> On Fri, 25 Aug 2023 13:29:44 -0100
> Melissa Wen <mwen@igalia.com> wrote:
> 
>> On 08/22, Pekka Paalanen 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.  
>>
>> I agree that crtc degamma is an optional property and should be not
>> exposed if not available.  I did something in this line for DCE that has
>> no degamma block[1].  Then, AMD DDX driver stopped to advertise atomic
>> API for DCE, that was not correct too[2].
> 
> Did AMD go through all the trouble of making their Xorg DDX use KMS
> atomic, even after the kernel took it away from X due to modesetting
> DDX screwing it up?

No, I think Melissa meant the KMS properties for advanced colour transforms, which xf86-video-amdgpu uses, not with atomic KMS though.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer