Re: [PATCH v4 17/17] drm/amd/display: Add handling for new "Broadcast RGB" property

Am 22.06.21 um 09:29 schrieb Pekka Paalanen:

On Fri, 18 Jun 2021 11:11:16 +0200 Werner Sembach wse@tuxedocomputers.com wrote:

...

This commit implements the "Broadcast RGB" drm property for the AMD GPU driver.

Signed-off-by: Werner Sembach wse@tuxedocomputers.com

.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 22 ++++++++++++++----- .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 4 ++++ 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 9ffd2f9d3d75..c5dbf948a47a 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5252,7 +5252,8 @@ get_aspect_ratio(const struct drm_display_mode *mode_in) }

static enum dc_color_space -get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing) +get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,

•        enum drm_mode_color_range preferred_color_range)
  

{ enum dc_color_space color_space = COLOR_SPACE_SRGB;

@@ -5267,13 +5268,17 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing) * respectively */ if (dc_crtc_timing->pix_clk_100hz > 270300) {

• 	if (dc_crtc_timing->flags.Y_ONLY)
  

• 	if (dc_crtc_timing->flags.Y_ONLY
  

• 			|| preferred_color_range ==
  

• 				DRM_MODE_COLOR_RANGE_LIMITED_16_235)
  	color_space =
  		COLOR_SPACE_YCBCR709_LIMITED;
  else
  	color_space = COLOR_SPACE_YCBCR709;
  

Hi,

does this mean that amdgpu would be using a property named "Broadcast RGB" to control the range of YCbCr too?

Yes, because I avoided creating a new property, but I'm not really happy with it either.

Possibility 1: Use "Broadcast RGB" for Y'CbCr too and clarify in documentation     - still confusing name     - limited does not mean something a little bit different for Y'CbCr and not strictly 16-235: https://www.kernel.org/doc/html/v5.12/userspace-api/media/v4l/colorspaces-de... , but name of option is given by preexisting property

Possibility 2: Deprecate "Broadcast RGB" and a a more neutral sounding "preferred color range", with the more neutral sounding "limited" option instead of "Limited 16:235"     - What's the relation between the 2? pq mentioned on the amdgpu gitlab that there is a posibility for userspace to have only the new or the old one shown     - Alternatively ignore "Broadcast RGB" when "preferred color range" is set and have them coexist?

That is surprising. If this is truly wanted, then the documentation of "Broadcast RGB" must say that it applies to YCbCr too.

Does amdgpu do the same as intel wrt. to the question about whose responsibility it is to make the pixels at the connector to match the set range?

I guess the kernel driver does the conversion, but i have to check for both.

For Intel I did not change the behavior of Boradcast RGB, but i think it's not clearly specified in the docs where the conversion happens.

Thanks, pq

...

} else {

• 	if (dc_crtc_timing->flags.Y_ONLY)
  

• 	if (dc_crtc_timing->flags.Y_ONLY
  

• 			|| preferred_color_range ==
  

• 				DRM_MODE_COLOR_RANGE_LIMITED_16_235)
  	color_space =
  		COLOR_SPACE_YCBCR601_LIMITED;
  else
  

@@ -5283,7 +5288,10 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing) } break; case PIXEL_ENCODING_RGB:

• color_space = COLOR_SPACE_SRGB;
  

• if (preferred_color_range == DRM_MODE_COLOR_RANGE_LIMITED_16_235)
  

• 	color_space = COLOR_SPACE_SRGB_LIMITED;
  

• else
  

• 	color_space = COLOR_SPACE_SRGB;
  

  break;

  default:

@@ -5429,7 +5437,10 @@ static void fill_stream_properties_from_drm_display_mode(

timing_out->aspect_ratio = get_aspect_ratio(mode_in);

• stream->output_color_space = get_output_color_space(timing_out);

• stream->output_color_space = get_output_color_space(timing_out,

• 					    connector_state ?
  

• 					    connector_state->preferred_color_range :
  

• 					    DRM_MODE_COLOR_RANGE_UNSET);
  
  

  stream->out_transfer_func->type = TF_TYPE_PREDEFINED; stream->out_transfer_func->tf = TRANSFER_FUNCTION_SRGB;

@@ -7780,6 +7791,7 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, drm_connector_attach_active_bpc_property(&aconnector->base, 8, 16); drm_connector_attach_preferred_color_format_property(&aconnector->base); drm_connector_attach_active_color_format_property(&aconnector->base);

• drm_connector_attach_preferred_color_range_property(&aconnector->base);
  

  drm_connector_attach_active_color_range_property(&aconnector->base); }

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c index 2563788ba95a..80e1389fd0ec 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c @@ -421,6 +421,10 @@ dm_dp_add_mst_connector(struct drm_dp_mst_topology_mgr *mgr, if (connector->active_color_format_property) drm_connector_attach_active_color_format_property(&aconnector->base);

• connector->preferred_color_range_property = master->base.preferred_color_range_property;
• if (connector->preferred_color_range_property)

• drm_connector_attach_preferred_color_range_property(&aconnector->base);
  

• connector->active_color_range_property = master->base.active_color_range_property; if (connector->active_color_range_property) drm_connector_attach_active_color_range_property(&aconnector->base);

---

amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx