Re: [RFC PATCH 07/40] drm/amd/display: add CRTC gamma TF to driver-private props

[Melissa Wen]

On 05/08, Harry Wentland wrote:
> 
> 
> On 4/23/23 10:10, Melissa Wen wrote:
> > From: Joshua Ashton <joshua@xxxxxxxxx>
> > 
> > Add predefined transfer function property to DRM CRTC gamma to convert
> > to wire encoding with or without gamma LUT.
> > 
> 
> Are all these new CRTC properties used by gamescope? I would be reluctant
> to merge them if they're currently not needed.

The regamma TF yes. The shaper and 3D LUT not yet.

I'll double check with Joshie and drop from the series what we don't
have a short-term perspective of usage.

> 
> > Co-developed-by: Melissa Wen <mwen@xxxxxxxxxx>
> > Signed-off-by: Melissa Wen <mwen@xxxxxxxxxx>
> > Signed-off-by: Joshua Ashton <joshua@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   | 22 ++++++++++++++++++
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h      |  4 ++++
> >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 23 +++++++++++++++++++
> >  .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    | 13 +++++++++++
> >  4 files changed, 62 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > index 2abe5fe87c10..1913903cab88 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> > @@ -1248,6 +1248,19 @@ amdgpu_display_user_framebuffer_create(struct drm_device *dev,
> >  }
> >  
> >  #ifdef CONFIG_STEAM_DECK
> > +static const struct drm_prop_enum_list drm_transfer_function_enum_list[] = {
> > +	{ DRM_TRANSFER_FUNCTION_DEFAULT, "Default" },
> > +	{ DRM_TRANSFER_FUNCTION_SRGB, "sRGB" },
> > +	{ DRM_TRANSFER_FUNCTION_BT709, "BT.709" },
> > +	{ DRM_TRANSFER_FUNCTION_PQ, "PQ (Perceptual Quantizer)" },
> > +	{ DRM_TRANSFER_FUNCTION_LINEAR, "Linear" },
> > +	{ DRM_TRANSFER_FUNCTION_UNITY, "Unity" },
> > +	{ DRM_TRANSFER_FUNCTION_HLG, "HLG (Hybrid Log Gamma)" },
> > +	{ DRM_TRANSFER_FUNCTION_GAMMA22, "Gamma 2.2" },
> > +	{ DRM_TRANSFER_FUNCTION_GAMMA24, "Gamma 2.4" },
> > +	{ DRM_TRANSFER_FUNCTION_GAMMA26, "Gamma 2.6" },
> > +};
> > +
> 
> Would it be better to prefix things with AMD_/amd_ to avoid confusion? On the other
> hand, these will likely just move into DRM core once we get the generic color uAPI.
> 
> Harry
> 
> >  static int
> >  amdgpu_display_create_color_properties(struct amdgpu_device *adev)
> >  {
> > @@ -1281,6 +1294,15 @@ amdgpu_display_create_color_properties(struct amdgpu_device *adev)
> >  		return -ENOMEM;
> >  	adev->mode_info.lut3d_size_property = prop;
> >  
> > +	prop = drm_property_create_enum(adev_to_drm(adev),
> > +					DRM_MODE_PROP_ENUM,
> > +					"GAMMA_TF",
> > +					drm_transfer_function_enum_list,
> > +					ARRAY_SIZE(drm_transfer_function_enum_list));
> > +	if (!prop)
> > +		return -ENOMEM;
> > +	adev->mode_info.gamma_tf_property = prop;
> > +
> >  	return 0;
> >  }
> >  #endif
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> > index 205fa4f5bea7..76337e18c728 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> > @@ -368,6 +368,10 @@ struct amdgpu_mode_info {
> >  	 * LUT as supported by the driver (read-only).
> >  	 */
> >  	struct drm_property *lut3d_size_property;
> > +	/**
> > +	 * @gamma_tf_property: Transfer function for CRTC regamma.
> > +	 */
> > +	struct drm_property *gamma_tf_property;
> >  #endif
> >  };
> >  
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > index 09c3e1858b56..1e90a2dd445e 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> > @@ -699,6 +699,23 @@ static inline void amdgpu_dm_set_mst_status(uint8_t *status,
> >  
> >  extern const struct amdgpu_ip_block_version dm_ip_block;
> >  
> > +#ifdef CONFIG_STEAM_DECK
> > +enum drm_transfer_function {
> > +	DRM_TRANSFER_FUNCTION_DEFAULT,
> > +
> > +	DRM_TRANSFER_FUNCTION_SRGB,
> > +	DRM_TRANSFER_FUNCTION_BT709,
> > +	DRM_TRANSFER_FUNCTION_PQ,
> > +	DRM_TRANSFER_FUNCTION_LINEAR,
> > +	DRM_TRANSFER_FUNCTION_UNITY,
> > +	DRM_TRANSFER_FUNCTION_HLG,
> > +	DRM_TRANSFER_FUNCTION_GAMMA22,
> > +	DRM_TRANSFER_FUNCTION_GAMMA24,
> > +	DRM_TRANSFER_FUNCTION_GAMMA26,
> > +	DRM_TRANSFER_FUNCTION_MAX,
> > +};
> > +#endif
> > +
> >  struct dm_plane_state {
> >  	struct drm_plane_state base;
> >  	struct dc_plane_state *dc_state;
> > @@ -751,6 +768,12 @@ struct dm_crtc_state {
> >  	 * &struct drm_color_lut.
> >  	 */
> >  	struct drm_property_blob *lut3d;
> > +        /**
> > +	 * @gamma_tf:
> > +	 *
> > +	 * Pre-defined transfer function for converting internal FB -> wire encoding.
> > +	 */
> > +	enum drm_transfer_function gamma_tf;
> >  #endif
> >  };
> >  
> > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
> > index 0e1280228e6e..79324fbab1f1 100644
> > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
> > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
> > @@ -272,6 +272,7 @@ static struct drm_crtc_state *dm_crtc_duplicate_state(struct drm_crtc *crtc)
> >  #ifdef CONFIG_STEAM_DECK
> >  	state->shaper_lut = cur->shaper_lut;
> >  	state->lut3d = cur->lut3d;
> > +	state->gamma_tf = cur->gamma_tf;
> >  
> >  	if (state->shaper_lut)
> >  		drm_property_blob_get(state->shaper_lut);
> > @@ -336,6 +337,11 @@ dm_crtc_additional_color_mgmt(struct drm_crtc *crtc)
> >  					   adev->mode_info.lut3d_size_property,
> >  					   MAX_COLOR_3DLUT_ENTRIES);
> >  	}
> > +
> > +	if(adev->dm.dc->caps.color.mpc.ogam_ram)
> > +		drm_object_attach_property(&crtc->base,
> > +					   adev->mode_info.gamma_tf_property,
> > +					   DRM_TRANSFER_FUNCTION_DEFAULT);
> >  }
> >  
> >  static int
> > @@ -398,6 +404,11 @@ amdgpu_dm_atomic_crtc_set_property(struct drm_crtc *crtc,
> >  					&replaced);
> >  		acrtc_state->base.color_mgmt_changed |= replaced;
> >  		return ret;
> > +	} else if (property == adev->mode_info.gamma_tf_property) {
> > +		if (acrtc_state->gamma_tf != val) {
> > +			acrtc_state->gamma_tf = val;
> > +			acrtc_state->base.color_mgmt_changed |= 1;
> > +		}
> >  	} else {
> >  		drm_dbg_atomic(crtc->dev,
> >  			       "[CRTC:%d:%s] unknown property [PROP:%d:%s]]\n",
> > @@ -424,6 +435,8 @@ amdgpu_dm_atomic_crtc_get_property(struct drm_crtc *crtc,
> >  	else if (property == adev->mode_info.lut3d_property)
> >  		*val = (acrtc_state->lut3d) ?
> >  			acrtc_state->lut3d->base.id : 0;
> > +	else if (property == adev->mode_info.gamma_tf_property)
> > +		*val = acrtc_state->gamma_tf;
> >  	else
> >  		return -EINVAL;
> >  
> 
>

Attachment: signature.asc
Description: PGP signature