[PATCH] drm/amdgpu add "pixel_encoding"

[PATCH] drm/amdgpu add "pixel_encoding"

[Matias N. Goldberg]

[-- Attachment #1: Type: text/plain, Size: 21422 bytes --]

Hi!
This is my first patch ever to DRM/amdgpu (technically I didn't write the patch, I just kept rebasing it over 4 years, and did lots of testing).
I'm following the advise from:https://gitlab.freedesktop.org/drm/amd/-/issues/476#note_2628536
Why this patch? Because it fix a practical problem with certain monitors that auto-select ycbcr444 when they should be set to "rgb" (this issue also happens on Windows) when plugging an HDMI monitor to a DisplayPort via passive adapter.

IMPORTANT remarks about the public API:
1. This API adds "pixel encoding" which has options "auto" (Default), "rgb", "ycbcr444", and "ycbcr420"
2. Intel has "RGB Broadcast". Its options are "Full" and "Limited 16:235". There is a lot of overlap here. On Windows AMD Control Panel offers the option "Pixel Encoding" and its options are: - RGB (full) - RGB (limited) - ycbcr444 - ycbcr420 - ycbcr422
Which means that it may be worth reserving more keywords for this patch.I don't know why ycbcr444/ycbcr420 do not offer full/limited variants on Windows, but I suspect it's because the driver can automatically tell whether to use Full vs Limited for ycbcr variants..
3. amdgpu already has "Colorspace" option. However this is a different (although related) option. But there is quite the overlap:
- Full/Limited options are sometimes included as part of the colorspace, such is the case of COLOR_SPACE_2020_RGB_FULLRANGE vs COLOR_SPACE_2020_RGB_LIMITEDRANGE, but there is no "LIMITED"/"FULL" variants for COLOR_SPACE_2020_YCBCR. Yet there are for YCBCR601 and YCBCR709. This may be a spec thing (I don’t know).- AFAIK amdgpu’s Limited/Full variants are not controllable from the public user-space API.- Certain options in "Colorspace" only makes sense in specific pixel encoding. For example the option "opRGB" currently glitches when amdgpu is using ycbcr encoding (you can tell the monitor is interpreting the RGB signal as YUV, thus white becomes purple and black becomes green).- The kernel has DRM_MODE_COLORIMETRY_BT2020_RGB and DRM_MODE_COLORIMETRY_BT2020_YCC. AMDGPU kernel currently forces COLOR_SPACE_2020_RGB_FULLRANGE or COLOR_SPACE_2020_YCBCR based on the current pixel encoding (e.g. if using RGB encodings and colorspace == DRM_MODE_COLORIMETRY_BT2020_YCC, it will be changed for COLOR_SPACE_2020_RGB_FULLRANGE with currently no choice for Limited range).- amdgpu seems to be able to automatically distinguish between Full and Limited for ycbcr  formats via the presence of flags.Y_ONLY bit. There is no such auto-detection for RGB.

The way I see it, there are 3 configurations that can be arranged together:
- Pixel Encoding (RGB vs ycbcr444 vs ycbcr420).- Colorspace (YCBCR709 vs BT.2020 etc).- Full vs Limited.
However not all options are compatible with everything.
I suspect doing the same as what AMD does on Windows (offer RGB/Full + RGB/Limited + all the ycbcr variants for "pixel encoding") is the best choice (and leave the "Colorspace" option as is, for very advanced manipulation).
4. Xaver Hugl expressed his concerns with the current patch that having an option called "auto" with no way of querying what is the current auto-selected encoding is sub-optimal.

CheersMatias


From 6806baac51f1ac2028c49bcab216c19f26a7e92b Mon Sep 17 00:00:00 2001From: "Matias N. Goldberg" <dark_sylinc@yahoo.com.ar>Date: Sun, 6 Oct 2024 21:04:11 -0300Subject: [PATCH] [amdgpu] Add "pixel_encoding" to switch between RGB & YUV color modes
Usage:xrandr --output DisplayPort-1 --set "pixel encoding" rgb
Supported options are: "auto" (Default), "rgb", "ycbcr444", and"ycbcr420"
This patch allows users to switch between pixel encodings, which isspecially important when auto gets it wrong (probably because ofmonitor's manufacturer mistake) and needs user intervention.
Original patch by Yassine ImounachenRebased by Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
Full discussion:https://gitlab.freedesktop.org/drm/amd/-/issues/476#note_2628536--- drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   |  35 +++ drivers/gpu/drm/amd/amdgpu/amdgpu_display.h   |   3 + drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h      |   2 + .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 215 +++++++++++++++++- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 + drivers/gpu/drm/amd/display/dc/core/dc.c      |   8 + drivers/gpu/drm/amd/display/dc/dc_stream.h    |   2 + drivers/gpu/drm/drm_modes.c                   |  29 +++ include/drm/drm_connector.h                   |   7 + 9 files changed, 290 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.cindex b119d27271c1..9d201f368b6e 100644--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c@@ -1362,6 +1362,33 @@ static const struct drm_prop_enum_list amdgpu_dither_enum_list[] = {  { AMDGPU_FMT_DITHER_ENABLE, "on" }, }; +static const struct drm_prop_enum_list amdgpu_user_pixenc_list[] = {+ { 0, "auto" },+ { DRM_COLOR_FORMAT_RGB444, "rgb" },+ { DRM_COLOR_FORMAT_YCBCR444, "ycbcr444" },+ { DRM_COLOR_FORMAT_YCBCR420, "ycbcr420" },+};++bool amdgpu_user_pixenc_from_name(+ unsigned int *user_pixenc,+ const char *pixenc_name)+{+ bool found = false;++ if (pixenc_name && (*pixenc_name != '\0')) {+ const int sz = ARRAY_SIZE(amdgpu_user_pixenc_list);+ int i;++ for (i = 0; !found && i < sz; ++i) {+ if (strcmp(pixenc_name, amdgpu_user_pixenc_list[i].name) == 0) {+ *user_pixenc = amdgpu_user_pixenc_list[i].type;+ found = true;+ }+ }+ }+ return found;+}+ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev) {  int sz;@@ -1408,6 +1435,14 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)   "dither",   amdgpu_dither_enum_list, sz); + sz = ARRAY_SIZE(amdgpu_user_pixenc_list);+ adev->mode_info.pixel_encoding_property =+ drm_property_create_enum(adev_to_drm(adev), 0,+ "pixel encoding",+ amdgpu_user_pixenc_list, sz);+ if (!adev->mode_info.pixel_encoding_property)+ return -ENOMEM;+  return 0; } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.hindex 9d19940f73c8..ee1ad49fa123 100644--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h@@ -49,4 +49,7 @@ amdgpu_lookup_format_info(u32 format, uint64_t modifier); int amdgpu_display_suspend_helper(struct amdgpu_device *adev); int amdgpu_display_resume_helper(struct amdgpu_device *adev); +bool amdgpu_user_pixenc_from_name(unsigned int *user_pixenc,+   const char *pixenc_name);+ #endifdiff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.hindex 5e3faefc5510..b984e66a5d75 100644--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h@@ -326,6 +326,8 @@ struct amdgpu_mode_info {  struct drm_property *audio_property;  /* FMT dithering */  struct drm_property *dither_property;+ /* User HDMI pixel encoding override */+ struct drm_property *pixel_encoding_property;  /* hardcoded DFP edid from BIOS */  const struct drm_edid *bios_hardcoded_edid; diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.cindex bbfc47f6595f..2c03e0733178 100644--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c@@ -6069,6 +6069,115 @@ static bool adjust_colour_depth_from_display_info(  return false; } +/* convert an pixel encoding property value to a dc_pixel_encoding */+static bool drm_prop_to_dc_pixel_encoding(+ enum dc_pixel_encoding *dc_pixenc,+ unsigned int propval)+{+ bool ret = false;++ switch (propval) {+ case 0:+ *dc_pixenc = PIXEL_ENCODING_UNDEFINED;+ ret = true;+ break;+ case DRM_COLOR_FORMAT_RGB444:+ *dc_pixenc = PIXEL_ENCODING_RGB;+ ret = true;+ break;+ case DRM_COLOR_FORMAT_YCBCR444:+ *dc_pixenc = PIXEL_ENCODING_YCBCR444;+ ret = true;+ break;+ case DRM_COLOR_FORMAT_YCBCR420:+ *dc_pixenc = PIXEL_ENCODING_YCBCR420;+ ret = true;+ break;+ default:+ break;+ }+ return ret;+}++/* convert an dc_pixel_encoding to a pixel encoding property value */+static unsigned int dc_pixel_encoding_to_drm_prop(+ enum dc_pixel_encoding pixel_encoding)+{+ unsigned int propval = 0;++ switch (pixel_encoding) {+ case PIXEL_ENCODING_RGB:+ propval = DRM_COLOR_FORMAT_RGB444;+ break;+ case PIXEL_ENCODING_YCBCR444:+ propval = DRM_COLOR_FORMAT_YCBCR444;+ break;+ case PIXEL_ENCODING_YCBCR420:+ propval = DRM_COLOR_FORMAT_YCBCR420;+ break;+ default:+ break;+ }+ return propval;+}++/*+ * Tries to read 'pixel_encoding' from the pixel_encoding DRM property on+ * 'state'. Returns true if a supported, acceptable, non-undefined value is+ * found; false otherwise. Only modifies 'pixel_encoding' if returning true.+ */+bool get_connector_state_pixel_encoding(+ enum dc_pixel_encoding *pixel_encoding,+ const struct drm_connector_state *state,+ const struct drm_display_info *info,+ const struct drm_display_mode *mode_in)+{+ bool ret = false;+ struct dm_connector_state *dm_state;++ dm_state = to_dm_connector_state(state);+ if (!dm_state)+ return false;++ /* check encoding is supported */+ switch (dm_state->pixel_encoding) {+ case PIXEL_ENCODING_RGB:+ ret = (info->color_formats & DRM_COLOR_FORMAT_RGB444);+ break;+ case PIXEL_ENCODING_YCBCR444:+ ret = (info->color_formats & DRM_COLOR_FORMAT_YCBCR444);+ break;+ case PIXEL_ENCODING_YCBCR420:+ ret = drm_mode_is_420(info, mode_in);+ break;+ default:+ break;+ }++ if (ret)+ *pixel_encoding = dm_state->pixel_encoding;++ return ret;+}++/*+ * Writes 'pixel_encoding' to the pixel_encoding DRM property on 'state', if+ * the enum value is valid and supported; otherwise writes+ * PIXEL_ENCODING_UNDEFINED which corresponds to the "auto" property state.+ */+void set_connector_state_pixel_encoding(+ const struct drm_connector_state *state,+ enum dc_pixel_encoding pixel_encoding)+{+ struct dm_connector_state *dm_state;++ dm_state = to_dm_connector_state(state);+ if (!dm_state)+ return;++ dm_state->pixel_encoding = pixel_encoding;+}+ static void fill_stream_properties_from_drm_display_mode(  struct dc_stream_state *stream,  const struct drm_display_mode *mode_in,@@ -6093,19 +6202,23 @@ static void fill_stream_properties_from_drm_display_mode(  timing_out->h_border_right = 0;  timing_out->v_border_top = 0;  timing_out->v_border_bottom = 0;- /* TODO: un-hardcode */- if (drm_mode_is_420_only(info, mode_in)++ if (!get_connector_state_pixel_encoding(&timing_out->pixel_encoding,+ connector_state, info, mode_in)) {+ /* auto-select a pixel encoding */+ if (drm_mode_is_420_only(info, mode_in)  && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)- timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;- else if (drm_mode_is_420_also(info, mode_in)+ timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;+ else if (drm_mode_is_420_also(info, mode_in)  && aconnector  && aconnector->force_yuv420_output)- timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;- else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)- && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)- timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;- else- timing_out->pixel_encoding = PIXEL_ENCODING_RGB;+ timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;+ else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)+ && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)+ timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;+ else+ timing_out->pixel_encoding = PIXEL_ENCODING_RGB;+ }   timing_out->timing_3d_format = TIMING_3D_FORMAT_NONE;  timing_out->display_color_depth = convert_color_depth_from_display_info(@@ -6169,6 +6282,9 @@ static void fill_stream_properties_from_drm_display_mode(  }  } + /* write back final choice of pixel encoding */+ set_connector_state_pixel_encoding(connector_state, timing_out->pixel_encoding);+  stream->output_color_space = get_output_color_space(timing_out, connector_state);  stream->content_type = get_output_content_type(connector_state); }@@ -6875,6 +6991,9 @@ int amdgpu_dm_connector_atomic_set_property(struct drm_connector *connector,  } else if (property == adev->mode_info.underscan_property) {  dm_new_state->underscan_enable = val;  ret = 0;+ } else if (property == adev->mode_info.pixel_encoding_property) {+ if (drm_prop_to_dc_pixel_encoding(&dm_new_state->pixel_encoding, val))+ ret = 0;  }   return ret;@@ -6917,6 +7036,9 @@ int amdgpu_dm_connector_atomic_get_property(struct drm_connector *connector,  } else if (property == adev->mode_info.underscan_property) {  *val = dm_state->underscan_enable;  ret = 0;+ } else if (property == adev->mode_info.pixel_encoding_property) {+ *val = dc_pixel_encoding_to_drm_prop(dm_state->pixel_encoding);+ ret = 0;  }   return ret;@@ -7088,6 +7210,20 @@ void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)  state->abm_level = amdgpu_dm_abm_level;  } + switch (connector->cmdline_mode.pixel_encoding) {+ case DRM_COLOR_FORMAT_RGB444:+ state->pixel_encoding = PIXEL_ENCODING_RGB;+ break;+ case DRM_COLOR_FORMAT_YCBCR444:+ state->pixel_encoding = PIXEL_ENCODING_YCBCR444;+ break;+ case DRM_COLOR_FORMAT_YCBCR420:+ state->pixel_encoding = PIXEL_ENCODING_YCBCR420;+ break;+ default:+ break;+ }+  __drm_atomic_helper_connector_reset(connector, &state->base);  } }@@ -7114,6 +7250,7 @@ amdgpu_dm_connector_atomic_duplicate_state(struct drm_connector *connector)  new_state->underscan_vborder = state->underscan_vborder;  new_state->vcpi_slots = state->vcpi_slots;  new_state->pbn = state->pbn;+ new_state->pixel_encoding = state->pixel_encoding;  return &new_state->base; } @@ -8175,6 +8312,12 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,   if (adev->dm.hdcp_workqueue)  drm_connector_attach_content_protection_property(&aconnector->base, true);++ if (adev->mode_info.pixel_encoding_property) {+ drm_object_attach_property(&aconnector->base.base,+ adev->mode_info.pixel_encoding_property, 0);+ DRM_DEBUG_DRIVER("amdgpu: attached pixel encoding drm property");+ }  } } @@ -9377,6 +9520,38 @@ static void amdgpu_dm_commit_audio(struct drm_device *dev,  } } +static void update_stream_for_pixel_encoding(+ struct dc_stream_update *stream_update,+ struct drm_connector *connector,+ struct dm_crtc_state *dm_old_crtc_state,+ struct dm_crtc_state *dm_new_crtc_state,+ struct dm_connector_state *dm_new_con_state)+{+ struct amdgpu_dm_connector *aconnector =+ to_amdgpu_dm_connector(connector);+ struct dc_stream_state *new_stream = NULL;++ if (aconnector)+ new_stream = create_validate_stream_for_sink(+ aconnector,+ &dm_new_crtc_state->base.mode,+ dm_new_con_state,+ dm_old_crtc_state->stream);+ if (new_stream) {+ dm_new_crtc_state->stream->timing =+ new_stream->timing;+ stream_update->timing_for_pixel_encoding =+ &dm_new_crtc_state->stream->timing;++ dm_new_crtc_state->stream->output_color_space =+ new_stream->output_color_space;+ stream_update->output_color_space =+ &dm_new_crtc_state->stream->output_color_space;++ dc_stream_release(new_stream);+ }+}+ /*  * amdgpu_dm_crtc_copy_transient_flags - copy mirrored flags from DRM to DC  * @crtc_state: the DRM CRTC state@@ -9850,7 +10025,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)  struct dc_stream_update stream_update;  struct dc_info_packet hdr_packet;  struct dc_stream_status *status = NULL;- bool abm_changed, hdr_changed, scaling_changed;+ bool abm_changed, hdr_changed, scaling_changed, pixenc_changed;   memset(&stream_update, 0, sizeof(stream_update)); @@ -9875,7 +10050,10 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)  hdr_changed =  !drm_connector_atomic_hdr_metadata_equal(old_con_state, new_con_state); - if (!scaling_changed && !abm_changed && !hdr_changed)+ pixenc_changed = dm_new_con_state->pixel_encoding !=+  dm_old_con_state->pixel_encoding;++ if (!scaling_changed && !abm_changed && !hdr_changed && !pixenc_changed)  continue;   stream_update.stream = dm_new_crtc_state->stream;@@ -9898,6 +10076,13 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)  stream_update.hdr_static_metadata = &hdr_packet;  } + if (pixenc_changed) {+ update_stream_for_pixel_encoding(&stream_update,+ connector,+ dm_old_crtc_state, dm_new_crtc_state,+ dm_new_con_state);+ }+  status = dc_stream_get_status(dm_new_crtc_state->stream);   if (WARN_ON(!status))@@ -11403,6 +11588,12 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,  if (dm_old_con_state->abm_level != dm_new_con_state->abm_level ||      dm_old_con_state->scaling != dm_new_con_state->scaling)  new_crtc_state->connectors_changed = true;++ if (dm_old_con_state->pixel_encoding !=+     dm_new_con_state->pixel_encoding) {+ new_crtc_state->connectors_changed = true;+ new_crtc_state->mode_changed = true;+ }  }   if (dc_resource_is_dsc_encoding_supported(dc)) {diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.hindex 25e95775c45c..132944272c18 100644--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h@@ -918,6 +918,7 @@ struct dm_connector_state {  uint8_t abm_level;  int vcpi_slots;  uint64_t pbn;+ enum dc_pixel_encoding pixel_encoding; };  #define to_dm_connector_state(x)\diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.cindex 5a12fc75f97f..4ac004536707 100644--- a/drivers/gpu/drm/amd/display/dc/core/dc.c+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c@@ -2765,6 +2765,11 @@ static enum surface_update_type check_update_surfaces_for_stream(  if (stream_update->output_csc_transform || stream_update->output_color_space)  su_flags->bits.out_csc = 1; + if (stream_update->timing_for_pixel_encoding) {+ su_flags->bits.pixel_encoding = 1;+ elevate_update_type(&overall_type, UPDATE_TYPE_FULL);+ }+  /* Output transfer function changes do not require bandwidth recalculation,   * so don't trigger a full update   */@@ -3112,6 +3117,8 @@ static void copy_stream_update_to_stream(struct dc *dc,  stream->scaler_sharpener_update = *update->scaler_sharpener_update;  if (update->sharpening_required)  stream->sharpening_required = *update->sharpening_required;+ if (update->timing_for_pixel_encoding)+ stream->timing = *update->timing_for_pixel_encoding; }  static void backup_planes_and_stream_state(@@ -3361,6 +3368,7 @@ static void commit_planes_do_stream_update(struct dc *dc,  stream_update->vsc_infopacket ||  stream_update->vsp_infopacket ||  stream_update->hfvsif_infopacket ||+ stream_update->timing_for_pixel_encoding ||  stream_update->adaptive_sync_infopacket ||  stream_update->vtem_infopacket) {  resource_build_info_frame(pipe_ctx);diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.hindex 413970588a26..de7f02f330d8 100644--- a/drivers/gpu/drm/amd/display/dc/dc_stream.h+++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h@@ -144,6 +144,7 @@ union stream_update_flags {  uint32_t fams_changed : 1;  uint32_t scaler_sharpener : 1;  uint32_t sharpening_required : 1;+ uint32_t pixel_encoding:1;  } bits;   uint32_t raw;@@ -350,6 +351,7 @@ struct dc_stream_update {  struct dc_mst_stream_bw_update *mst_bw_update;  struct dc_transfer_func *func_shaper;  struct dc_3dlut *lut3d_func;+ struct dc_crtc_timing *timing_for_pixel_encoding;   struct test_pattern *pending_test_pattern;  struct dc_crtc_timing_adjust *crtc_timing_adjust;diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.cindex 1a0890083aee..b5ce8f93b672 100644--- a/drivers/gpu/drm/drm_modes.c+++ b/drivers/gpu/drm/drm_modes.c@@ -2161,6 +2161,32 @@ static int drm_mode_parse_tv_mode(const char *delim,  return 0; } +static int drm_mode_parse_pixel_encoding(const char *delim,+  struct drm_cmdline_mode *mode)+{+ const char *value;++ if (*delim != '=')+ return -EINVAL;++ value = delim + 1;+ delim = strchr(value, ',');+ if (!delim)+ delim = value + strlen(value);++ if (!strncmp(value, "auto", delim - value))+ mode->pixel_encoding = 0;+ else if (!strncmp(value, "rgb", delim - value))+ mode->pixel_encoding = DRM_COLOR_FORMAT_RGB444;+ else if (!strncmp(value, "ycbcr444", delim - value))+ mode->pixel_encoding = DRM_COLOR_FORMAT_YCBCR444;+ else if (!strncmp(value, "ycbcr420", delim - value))+ mode->pixel_encoding = DRM_COLOR_FORMAT_YCBCR420;+ else+ return -EINVAL;++ return 0;+} static int drm_mode_parse_cmdline_options(const char *str,    bool freestanding,    const struct drm_connector *connector,@@ -2233,6 +2259,9 @@ static int drm_mode_parse_cmdline_options(const char *str,  } else if (!strncmp(option, "tv_mode", delim - option)) {  if (drm_mode_parse_tv_mode(delim, mode))  return -EINVAL;+ } else if (!strncmp(option, "pixel_encoding", delim - option)) {+ if (drm_mode_parse_pixel_encoding(delim, mode))+ return -EINVAL;  } else {  return -EINVAL;  }diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.hindex c754651044d4..89c0f42873df 100644--- a/include/drm/drm_connector.h+++ b/include/drm/drm_connector.h@@ -1666,6 +1666,13 @@ struct drm_cmdline_mode {   * Did the mode have a preferred TV mode?   */  bool tv_mode_specified;++ /**+  * @pixel_encoding:+  *+  * Initial pixel encoding.+  */+ unsigned int pixel_encoding; };  /*-- 2.43.0

[-- Attachment #2: Type: text/html, Size: 35090 bytes --]

Fw: [PATCH] drm/amdgpu add "pixel_encoding"

[Matias N. Goldberg]

SENDING AGAIN because it was sent with HTML formatting, which screwed up the email.


Hi!

This is my first patch ever to DRM/amdgpu (technically I didn't write the patch, I just kept rebasing it over 4 years, and did lots of testing).

I'm following the advise from:
https://gitlab.freedesktop.org/drm/amd/-/issues/476#note_2628536

Why this patch? Because it fix a practical problem with certain monitors that auto-select ycbcr444 when they should be set to "rgb" (this issue also happens on Windows) when plugging an HDMI monitor to a DisplayPort via passive adapter.


IMPORTANT remarks about the public API:

1. This API adds "pixel encoding" which has options "auto" (Default), "rgb", "ycbcr444", and "ycbcr420"

2. Intel has "RGB Broadcast". Its options are "Full" and "Limited 16:235". There is a lot of overlap here. On Windows AMD Control Panel offers the option "Pixel Encoding" and its options are:
 - RGB (full)
 - RGB (limited)
 - ycbcr444
 - ycbcr420
 - ycbcr422

Which means that it may be worth reserving more keywords for this patch.
I don't know why ycbcr444/ycbcr420 do not offer full/limited variants on Windows, but I suspect it's because the driver can automatically tell whether to use Full vs Limited for ycbcr variants..

3. amdgpu already has "Colorspace" option. However this is a different (although related) option. But there is quite the overlap:

- Full/Limited options are sometimes included as part of the colorspace, such is the case of COLOR_SPACE_2020_RGB_FULLRANGE vs COLOR_SPACE_2020_RGB_LIMITEDRANGE, but there is no "LIMITED"/"FULL" variants for COLOR_SPACE_2020_YCBCR. Yet there are for YCBCR601 and YCBCR709. This may be a spec thing (I don’t know).

- AFAIK amdgpu’s Limited/Full variants are not controllable from the public user-space API.

- Certain options in "Colorspace" only makes sense in specific pixel encoding. For example the option "opRGB" currently glitches when amdgpu is using ycbcr encoding (you can tell the monitor is interpreting the RGB signal as YUV, thus white becomes purple and black becomes green).

- The kernel has DRM_MODE_COLORIMETRY_BT2020_RGB and DRM_MODE_COLORIMETRY_BT2020_YCC. AMDGPU kernel currently forces COLOR_SPACE_2020_RGB_FULLRANGE or COLOR_SPACE_2020_YCBCR based on the current pixel encoding (e.g. if using RGB encodings and colorspace == DRM_MODE_COLORIMETRY_BT2020_YCC, it will be changed for COLOR_SPACE_2020_RGB_FULLRANGE with currently no choice for Limited range).
- amdgpu seems to be able to automatically distinguish between Full and Limited for ycbcr  formats via the presence of flags.Y_ONLY bit. There is no such auto-detection for RGB.


The way I see it, there are 3 configurations that can be arranged together:

- Pixel Encoding (RGB vs ycbcr444 vs ycbcr420).
- Colorspace (YCBCR709 vs BT.2020 etc).
- Full vs Limited.

However not all options are compatible with everything.

I suspect doing the same as what AMD does on Windows (offer RGB/Full + RGB/Limited + all the ycbcr variants for "pixel encoding") is the best choice (and leave the "Colorspace" option as is, for very advanced manipulation).

4. Xaver Hugl expressed his concerns with the current patch that having an option called "auto" with no way of querying what is the current auto-selected encoding is sub-optimal.


Cheers
Matias



From 6806baac51f1ac2028c49bcab216c19f26a7e92b Mon Sep 17 00:00:00 2001
From: "Matias N. Goldberg" <dark_sylinc@yahoo.com.ar>
Date: Sun, 6 Oct 2024 21:04:11 -0300
Subject: [PATCH] [amdgpu] Add "pixel_encoding" to switch between RGB & YUV
 color modes

Usage:
xrandr --output DisplayPort-1 --set "pixel encoding" rgb

Supported options are: "auto" (Default), "rgb", "ycbcr444", and
"ycbcr420"

This patch allows users to switch between pixel encodings, which is
specially important when auto gets it wrong (probably because of
monitor's manufacturer mistake) and needs user intervention.

Original patch by Yassine Imounachen
Rebased by Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

Full discussion:
https://gitlab.freedesktop.org/drm/amd/-/issues/476#note_2628536
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   |  35 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.h   |   3 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h      |   2 +
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 215 +++++++++++++++++-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 +
 drivers/gpu/drm/amd/display/dc/core/dc.c      |   8 +
 drivers/gpu/drm/amd/display/dc/dc_stream.h    |   2 +
 drivers/gpu/drm/drm_modes.c                   |  29 +++
 include/drm/drm_connector.h                   |   7 +
 9 files changed, 290 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index b119d27271c1..9d201f368b6e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -1362,6 +1362,33 @@ static const struct drm_prop_enum_list amdgpu_dither_enum_list[] = {
  { AMDGPU_FMT_DITHER_ENABLE, "on" },
 };
 
+static const struct drm_prop_enum_list amdgpu_user_pixenc_list[] = {
+ { 0, "auto" },
+ { DRM_COLOR_FORMAT_RGB444, "rgb" },
+ { DRM_COLOR_FORMAT_YCBCR444, "ycbcr444" },
+ { DRM_COLOR_FORMAT_YCBCR420, "ycbcr420" },
+};
+
+bool amdgpu_user_pixenc_from_name(
+ unsigned int *user_pixenc,
+ const char *pixenc_name)
+{
+ bool found = false;
+
+ if (pixenc_name && (*pixenc_name != '\0')) {
+ const int sz = ARRAY_SIZE(amdgpu_user_pixenc_list);
+ int i;
+
+ for (i = 0; !found && i < sz; ++i) {
+ if (strcmp(pixenc_name, amdgpu_user_pixenc_list[i].name) == 0) {
+ *user_pixenc = amdgpu_user_pixenc_list[i].type;
+ found = true;
+ }
+ }
+ }
+ return found;
+}
+
 int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
 {
  int sz;
@@ -1408,6 +1435,14 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
   "dither",
   amdgpu_dither_enum_list, sz);
 
+ sz = ARRAY_SIZE(amdgpu_user_pixenc_list);
+ adev->mode_info.pixel_encoding_property =
+ drm_property_create_enum(adev_to_drm(adev), 0,
+ "pixel encoding",
+ amdgpu_user_pixenc_list, sz);
+ if (!adev->mode_info.pixel_encoding_property)
+ return -ENOMEM;
+
  return 0;
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
index 9d19940f73c8..ee1ad49fa123 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
@@ -49,4 +49,7 @@ amdgpu_lookup_format_info(u32 format, uint64_t modifier);
 int amdgpu_display_suspend_helper(struct amdgpu_device *adev);
 int amdgpu_display_resume_helper(struct amdgpu_device *adev);
 
+bool amdgpu_user_pixenc_from_name(unsigned int *user_pixenc,
+   const char *pixenc_name);
+
 #endif
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index 5e3faefc5510..b984e66a5d75 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -326,6 +326,8 @@ struct amdgpu_mode_info {
  struct drm_property *audio_property;
  /* FMT dithering */
  struct drm_property *dither_property;
+ /* User HDMI pixel encoding override */
+ struct drm_property *pixel_encoding_property;
  /* hardcoded DFP edid from BIOS */
  const struct drm_edid *bios_hardcoded_edid;
 
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 bbfc47f6595f..2c03e0733178 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6069,6 +6069,115 @@ static bool adjust_colour_depth_from_display_info(
  return false;
 }
 
+/* convert an pixel encoding property value to a dc_pixel_encoding */
+static bool drm_prop_to_dc_pixel_encoding(
+ enum dc_pixel_encoding *dc_pixenc,
+ unsigned int propval)
+{
+ bool ret = false;
+
+ switch (propval) {
+ case 0:
+ *dc_pixenc = PIXEL_ENCODING_UNDEFINED;
+ ret = true;
+ break;
+ case DRM_COLOR_FORMAT_RGB444:
+ *dc_pixenc = PIXEL_ENCODING_RGB;
+ ret = true;
+ break;
+ case DRM_COLOR_FORMAT_YCBCR444:
+ *dc_pixenc = PIXEL_ENCODING_YCBCR444;
+ ret = true;
+ break;
+ case DRM_COLOR_FORMAT_YCBCR420:
+ *dc_pixenc = PIXEL_ENCODING_YCBCR420;
+ ret = true;
+ break;
+ default:
+ break;
+ }
+ return ret;
+}
+
+/* convert an dc_pixel_encoding to a pixel encoding property value */
+static unsigned int dc_pixel_encoding_to_drm_prop(
+ enum dc_pixel_encoding pixel_encoding)
+{
+ unsigned int propval = 0;
+
+ switch (pixel_encoding) {
+ case PIXEL_ENCODING_RGB:
+ propval = DRM_COLOR_FORMAT_RGB444;
+ break;
+ case PIXEL_ENCODING_YCBCR444:
+ propval = DRM_COLOR_FORMAT_YCBCR444;
+ break;
+ case PIXEL_ENCODING_YCBCR420:
+ propval = DRM_COLOR_FORMAT_YCBCR420;
+ break;
+ default:
+ break;
+ }
+ return propval;
+}
+
+/*
+ * Tries to read 'pixel_encoding' from the pixel_encoding DRM property on
+ * 'state'. Returns true if a supported, acceptable, non-undefined value is
+ * found; false otherwise. Only modifies 'pixel_encoding' if returning true.
+ */
+bool get_connector_state_pixel_encoding(
+ enum dc_pixel_encoding *pixel_encoding,
+ const struct drm_connector_state *state,
+ const struct drm_display_info *info,
+ const struct drm_display_mode *mode_in)
+{
+ bool ret = false;
+ struct dm_connector_state *dm_state;
+
+ dm_state = to_dm_connector_state(state);
+ if (!dm_state)
+ return false;
+
+ /* check encoding is supported */
+ switch (dm_state->pixel_encoding) {
+ case PIXEL_ENCODING_RGB:
+ ret = (info->color_formats & DRM_COLOR_FORMAT_RGB444);
+ break;
+ case PIXEL_ENCODING_YCBCR444:
+ ret = (info->color_formats & DRM_COLOR_FORMAT_YCBCR444);
+ break;
+ case PIXEL_ENCODING_YCBCR420:
+ ret = drm_mode_is_420(info, mode_in);
+ break;
+ default:
+ break;
+ }
+
+ if (ret)
+ *pixel_encoding = dm_state->pixel_encoding;
+
+ return ret;
+}
+
+/*
+ * Writes 'pixel_encoding' to the pixel_encoding DRM property on 'state', if
+ * the enum value is valid and supported; otherwise writes
+ * PIXEL_ENCODING_UNDEFINED which corresponds to the "auto" property state.
+ */
+void set_connector_state_pixel_encoding(
+ const struct drm_connector_state *state,
+ enum dc_pixel_encoding pixel_encoding)
+{
+ struct dm_connector_state *dm_state;
+
+ dm_state = to_dm_connector_state(state);
+ if (!dm_state)
+ return;
+
+ dm_state->pixel_encoding = pixel_encoding;
+}
+
 static void fill_stream_properties_from_drm_display_mode(
  struct dc_stream_state *stream,
  const struct drm_display_mode *mode_in,
@@ -6093,19 +6202,23 @@ static void fill_stream_properties_from_drm_display_mode(
  timing_out->h_border_right = 0;
  timing_out->v_border_top = 0;
  timing_out->v_border_bottom = 0;
- /* TODO: un-hardcode */
- if (drm_mode_is_420_only(info, mode_in)
+
+ if (!get_connector_state_pixel_encoding(&timing_out->pixel_encoding,
+ connector_state, info, mode_in)) {
+ /* auto-select a pixel encoding */
+ if (drm_mode_is_420_only(info, mode_in)
  && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
- timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
- else if (drm_mode_is_420_also(info, mode_in)
+ timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
+ else if (drm_mode_is_420_also(info, mode_in)
  && aconnector
  && aconnector->force_yuv420_output)
- timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
- else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
- && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
- timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
- else
- timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
+ timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
+ else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
+ && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
+ timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
+ else
+ timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
+ }
 
  timing_out->timing_3d_format = TIMING_3D_FORMAT_NONE;
  timing_out->display_color_depth = convert_color_depth_from_display_info(
@@ -6169,6 +6282,9 @@ static void fill_stream_properties_from_drm_display_mode(
  }
  }
 
+ /* write back final choice of pixel encoding */
+ set_connector_state_pixel_encoding(connector_state, timing_out->pixel_encoding);
+
  stream->output_color_space = get_output_color_space(timing_out, connector_state);
  stream->content_type = get_output_content_type(connector_state);
 }
@@ -6875,6 +6991,9 @@ int amdgpu_dm_connector_atomic_set_property(struct drm_connector *connector,
  } else if (property == adev->mode_info.underscan_property) {
  dm_new_state->underscan_enable = val;
  ret = 0;
+ } else if (property == adev->mode_info.pixel_encoding_property) {
+ if (drm_prop_to_dc_pixel_encoding(&dm_new_state->pixel_encoding, val))
+ ret = 0;
  }
 
  return ret;
@@ -6917,6 +7036,9 @@ int amdgpu_dm_connector_atomic_get_property(struct drm_connector *connector,
  } else if (property == adev->mode_info.underscan_property) {
  *val = dm_state->underscan_enable;
  ret = 0;
+ } else if (property == adev->mode_info.pixel_encoding_property) {
+ *val = dc_pixel_encoding_to_drm_prop(dm_state->pixel_encoding);
+ ret = 0;
  }
 
  return ret;
@@ -7088,6 +7210,20 @@ void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
  state->abm_level = amdgpu_dm_abm_level;
  }
 
+ switch (connector->cmdline_mode.pixel_encoding) {
+ case DRM_COLOR_FORMAT_RGB444:
+ state->pixel_encoding = PIXEL_ENCODING_RGB;
+ break;
+ case DRM_COLOR_FORMAT_YCBCR444:
+ state->pixel_encoding = PIXEL_ENCODING_YCBCR444;
+ break;
+ case DRM_COLOR_FORMAT_YCBCR420:
+ state->pixel_encoding = PIXEL_ENCODING_YCBCR420;
+ break;
+ default:
+ break;
+ }
+
  __drm_atomic_helper_connector_reset(connector, &state->base);
  }
 }
@@ -7114,6 +7250,7 @@ amdgpu_dm_connector_atomic_duplicate_state(struct drm_connector *connector)
  new_state->underscan_vborder = state->underscan_vborder;
  new_state->vcpi_slots = state->vcpi_slots;
  new_state->pbn = state->pbn;
+ new_state->pixel_encoding = state->pixel_encoding;
  return &new_state->base;
 }
 
@@ -8175,6 +8312,12 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 
  if (adev->dm.hdcp_workqueue)
  drm_connector_attach_content_protection_property(&aconnector->base, true);
+
+ if (adev->mode_info.pixel_encoding_property) {
+ drm_object_attach_property(&aconnector->base.base,
+ adev->mode_info.pixel_encoding_property, 0);
+ DRM_DEBUG_DRIVER("amdgpu: attached pixel encoding drm property");
+ }
  }
 }
 
@@ -9377,6 +9520,38 @@ static void amdgpu_dm_commit_audio(struct drm_device *dev,
  }
 }
 
+static void update_stream_for_pixel_encoding(
+ struct dc_stream_update *stream_update,
+ struct drm_connector *connector,
+ struct dm_crtc_state *dm_old_crtc_state,
+ struct dm_crtc_state *dm_new_crtc_state,
+ struct dm_connector_state *dm_new_con_state)
+{
+ struct amdgpu_dm_connector *aconnector =
+ to_amdgpu_dm_connector(connector);
+ struct dc_stream_state *new_stream = NULL;
+
+ if (aconnector)
+ new_stream = create_validate_stream_for_sink(
+ aconnector,
+ &dm_new_crtc_state->base.mode,
+ dm_new_con_state,
+ dm_old_crtc_state->stream);
+ if (new_stream) {
+ dm_new_crtc_state->stream->timing =
+ new_stream->timing;
+ stream_update->timing_for_pixel_encoding =
+ &dm_new_crtc_state->stream->timing;
+
+ dm_new_crtc_state->stream->output_color_space =
+ new_stream->output_color_space;
+ stream_update->output_color_space =
+ &dm_new_crtc_state->stream->output_color_space;
+
+ dc_stream_release(new_stream);
+ }
+}
+
 /*
  * amdgpu_dm_crtc_copy_transient_flags - copy mirrored flags from DRM to DC
  * @crtc_state: the DRM CRTC state
@@ -9850,7 +10025,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
  struct dc_stream_update stream_update;
  struct dc_info_packet hdr_packet;
  struct dc_stream_status *status = NULL;
- bool abm_changed, hdr_changed, scaling_changed;
+ bool abm_changed, hdr_changed, scaling_changed, pixenc_changed;
 
  memset(&stream_update, 0, sizeof(stream_update));
 
@@ -9875,7 +10050,10 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
  hdr_changed =
  !drm_connector_atomic_hdr_metadata_equal(old_con_state, new_con_state);
 
- if (!scaling_changed && !abm_changed && !hdr_changed)
+ pixenc_changed = dm_new_con_state->pixel_encoding !=
+  dm_old_con_state->pixel_encoding;
+
+ if (!scaling_changed && !abm_changed && !hdr_changed && !pixenc_changed)
  continue;
 
  stream_update.stream = dm_new_crtc_state->stream;
@@ -9898,6 +10076,13 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
  stream_update.hdr_static_metadata = &hdr_packet;
  }
 
+ if (pixenc_changed) {
+ update_stream_for_pixel_encoding(&stream_update,
+ connector,
+ dm_old_crtc_state, dm_new_crtc_state,
+ dm_new_con_state);
+ }
+
  status = dc_stream_get_status(dm_new_crtc_state->stream);
 
  if (WARN_ON(!status))
@@ -11403,6 +11588,12 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
  if (dm_old_con_state->abm_level != dm_new_con_state->abm_level ||
      dm_old_con_state->scaling != dm_new_con_state->scaling)
  new_crtc_state->connectors_changed = true;
+
+ if (dm_old_con_state->pixel_encoding !=
+     dm_new_con_state->pixel_encoding) {
+ new_crtc_state->connectors_changed = true;
+ new_crtc_state->mode_changed = true;
+ }
  }
 
  if (dc_resource_is_dsc_encoding_supported(dc)) {
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 25e95775c45c..132944272c18 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -918,6 +918,7 @@ struct dm_connector_state {
  uint8_t abm_level;
  int vcpi_slots;
  uint64_t pbn;
+ enum dc_pixel_encoding pixel_encoding;
 };
 
 #define to_dm_connector_state(x)\
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 5a12fc75f97f..4ac004536707 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -2765,6 +2765,11 @@ static enum surface_update_type check_update_surfaces_for_stream(
  if (stream_update->output_csc_transform || stream_update->output_color_space)
  su_flags->bits.out_csc = 1;
 
+ if (stream_update->timing_for_pixel_encoding) {
+ su_flags->bits.pixel_encoding = 1;
+ elevate_update_type(&overall_type, UPDATE_TYPE_FULL);
+ }
+
  /* Output transfer function changes do not require bandwidth recalculation,
   * so don't trigger a full update
   */
@@ -3112,6 +3117,8 @@ static void copy_stream_update_to_stream(struct dc *dc,
  stream->scaler_sharpener_update = *update->scaler_sharpener_update;
  if (update->sharpening_required)
  stream->sharpening_required = *update->sharpening_required;
+ if (update->timing_for_pixel_encoding)
+ stream->timing = *update->timing_for_pixel_encoding;
 }
 
 static void backup_planes_and_stream_state(
@@ -3361,6 +3368,7 @@ static void commit_planes_do_stream_update(struct dc *dc,
  stream_update->vsc_infopacket ||
  stream_update->vsp_infopacket ||
  stream_update->hfvsif_infopacket ||
+ stream_update->timing_for_pixel_encoding ||
  stream_update->adaptive_sync_infopacket ||
  stream_update->vtem_infopacket) {
  resource_build_info_frame(pipe_ctx);
diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h
index 413970588a26..de7f02f330d8 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_stream.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h
@@ -144,6 +144,7 @@ union stream_update_flags {
  uint32_t fams_changed : 1;
  uint32_t scaler_sharpener : 1;
  uint32_t sharpening_required : 1;
+ uint32_t pixel_encoding:1;
  } bits;
 
  uint32_t raw;
@@ -350,6 +351,7 @@ struct dc_stream_update {
  struct dc_mst_stream_bw_update *mst_bw_update;
  struct dc_transfer_func *func_shaper;
  struct dc_3dlut *lut3d_func;
+ struct dc_crtc_timing *timing_for_pixel_encoding;
 
  struct test_pattern *pending_test_pattern;
  struct dc_crtc_timing_adjust *crtc_timing_adjust;
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 1a0890083aee..b5ce8f93b672 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -2161,6 +2161,32 @@ static int drm_mode_parse_tv_mode(const char *delim,
  return 0;
 }
 
+static int drm_mode_parse_pixel_encoding(const char *delim,
+  struct drm_cmdline_mode *mode)
+{
+ const char *value;
+
+ if (*delim != '=')
+ return -EINVAL;
+
+ value = delim + 1;
+ delim = strchr(value, ',');
+ if (!delim)
+ delim = value + strlen(value);
+
+ if (!strncmp(value, "auto", delim - value))
+ mode->pixel_encoding = 0;
+ else if (!strncmp(value, "rgb", delim - value))
+ mode->pixel_encoding = DRM_COLOR_FORMAT_RGB444;
+ else if (!strncmp(value, "ycbcr444", delim - value))
+ mode->pixel_encoding = DRM_COLOR_FORMAT_YCBCR444;
+ else if (!strncmp(value, "ycbcr420", delim - value))
+ mode->pixel_encoding = DRM_COLOR_FORMAT_YCBCR420;
+ else
+ return -EINVAL;
+
+ return 0;
+}
 static int drm_mode_parse_cmdline_options(const char *str,
    bool freestanding,
    const struct drm_connector *connector,
@@ -2233,6 +2259,9 @@ static int drm_mode_parse_cmdline_options(const char *str,
  } else if (!strncmp(option, "tv_mode", delim - option)) {
  if (drm_mode_parse_tv_mode(delim, mode))
  return -EINVAL;
+ } else if (!strncmp(option, "pixel_encoding", delim - option)) {
+ if (drm_mode_parse_pixel_encoding(delim, mode))
+ return -EINVAL;
  } else {
  return -EINVAL;
  }
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index c754651044d4..89c0f42873df 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1666,6 +1666,13 @@ struct drm_cmdline_mode {
   * Did the mode have a preferred TV mode?
   */
  bool tv_mode_specified;
+
+ /**
+  * @pixel_encoding:
+  *
+  * Initial pixel encoding.
+  */
+ unsigned int pixel_encoding;
 };
 
 /*
-- 
2.43.0

Re: [PATCH] drm/amdgpu add "pixel_encoding"

[Matias N. Goldberg]

I just realized there is another (probably much better) possibility, which is to go the DXGI_FORMAT route:

Offer an enum with all possibilities e.g offer:

- BT2020_YCC_AUTO
- BT2020_YCC_LIMITED
- BT2020_YCC_FULL
- BT2020_RGB_LIMITED
- BT2020_RGB_FULL
- etc.

The list would grow huge. This offloads the problem to the UI, to be handled from user space:

UI could offer a list with the 100 or so options, and have a search editbox to filter the wanted ones (e.g. user types "2020 RGB" and only the 2 last options I listed above are shown).

The UI could ask the user "Pixel Encoding" and "Full vs Limited" to make it user friendly (since that's what 99% of users want), and have an advance option to pick the exact color space.

This means that Intel's option "RGB Broadcast" and amdgpu's "Colorspace" would be deprecated in favor of a new option that sets all 3 settings (color encoding + space + limited/full) at the same time.


Cheers
Matias


El lunes, 28 de octubre de 2024, 12:18:33 p. m. ART, Matias N. Goldberg <dark_sylinc@yahoo.com.ar> escribió: 





SENDING AGAIN because it was sent with HTML formatting, which screwed up the email.


Hi!

This is my first patch ever to DRM/amdgpu (technically I didn't write the patch, I just kept rebasing it over 4 years, and did lots of testing).

I'm following the advise from:
https://gitlab.freedesktop.org/drm/amd/-/issues/476#note_2628536

Why this patch? Because it fix a practical problem with certain monitors that auto-select ycbcr444 when they should be set to "rgb" (this issue also happens on Windows) when plugging an HDMI monitor to a DisplayPort via passive adapter.


IMPORTANT remarks about the public API:

1. This API adds "pixel encoding" which has options "auto" (Default), "rgb", "ycbcr444", and "ycbcr420"

2. Intel has "RGB Broadcast". Its options are "Full" and "Limited 16:235". There is a lot of overlap here. On Windows AMD Control Panel offers the option "Pixel Encoding" and its options are:
 - RGB (full)
 - RGB (limited)
 - ycbcr444
 - ycbcr420
 - ycbcr422

Which means that it may be worth reserving more keywords for this patch.
I don't know why ycbcr444/ycbcr420 do not offer full/limited variants on Windows, but I suspect it's because the driver can automatically tell whether to use Full vs Limited for ycbcr variants..

3. amdgpu already has "Colorspace" option. However this is a different (although related) option. But there is quite the overlap:

- Full/Limited options are sometimes included as part of the colorspace, such is the case of COLOR_SPACE_2020_RGB_FULLRANGE vs COLOR_SPACE_2020_RGB_LIMITEDRANGE, but there is no "LIMITED"/"FULL" variants for COLOR_SPACE_2020_YCBCR. Yet there are for YCBCR601 and YCBCR709. This may be a spec thing (I don’t know).

- AFAIK amdgpu’s Limited/Full variants are not controllable from the public user-space API.

- Certain options in "Colorspace" only makes sense in specific pixel encoding. For example the option "opRGB" currently glitches when amdgpu is using ycbcr encoding (you can tell the monitor is interpreting the RGB signal as YUV, thus white becomes purple and black becomes green).

- The kernel has DRM_MODE_COLORIMETRY_BT2020_RGB and DRM_MODE_COLORIMETRY_BT2020_YCC. AMDGPU kernel currently forces COLOR_SPACE_2020_RGB_FULLRANGE or COLOR_SPACE_2020_YCBCR based on the current pixel encoding (e.g. if using RGB encodings and colorspace == DRM_MODE_COLORIMETRY_BT2020_YCC, it will be changed for COLOR_SPACE_2020_RGB_FULLRANGE with currently no choice for Limited range).
- amdgpu seems to be able to automatically distinguish between Full and Limited for ycbcr  formats via the presence of flags.Y_ONLY bit. There is no such auto-detection for RGB.


The way I see it, there are 3 configurations that can be arranged together:

- Pixel Encoding (RGB vs ycbcr444 vs ycbcr420).
- Colorspace (YCBCR709 vs BT.2020 etc).
- Full vs Limited.

However not all options are compatible with everything.

I suspect doing the same as what AMD does on Windows (offer RGB/Full + RGB/Limited + all the ycbcr variants for "pixel encoding") is the best choice (and leave the "Colorspace" option as is, for very advanced manipulation).

4. Xaver Hugl expressed his concerns with the current patch that having an option called "auto" with no way of querying what is the current auto-selected encoding is sub-optimal.


Cheers
Matias



From 6806baac51f1ac2028c49bcab216c19f26a7e92b Mon Sep 17 00:00:00 2001
From: "Matias N. Goldberg" <dark_sylinc@yahoo.com.ar>
Date: Sun, 6 Oct 2024 21:04:11 -0300
Subject: [PATCH] [amdgpu] Add "pixel_encoding" to switch between RGB & YUV
 color modes

Usage:
xrandr --output DisplayPort-1 --set "pixel encoding" rgb

Supported options are: "auto" (Default), "rgb", "ycbcr444", and
"ycbcr420"

This patch allows users to switch between pixel encodings, which is
specially important when auto gets it wrong (probably because of
monitor's manufacturer mistake) and needs user intervention.

Original patch by Yassine Imounachen
Rebased by Matias N. Goldberg <dark_sylinc@yahoo.com.ar>

Full discussion:
https://gitlab.freedesktop.org/drm/amd/-/issues/476#note_2628536
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   |  35 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.h   |   3 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h      |   2 +
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 215 +++++++++++++++++-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 +
 drivers/gpu/drm/amd/display/dc/core/dc.c      |   8 +
 drivers/gpu/drm/amd/display/dc/dc_stream.h    |   2 +
 drivers/gpu/drm/drm_modes.c                   |  29 +++
 include/drm/drm_connector.h                   |   7 +
 9 files changed, 290 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index b119d27271c1..9d201f368b6e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -1362,6 +1362,33 @@ static const struct drm_prop_enum_list amdgpu_dither_enum_list[] = {
  { AMDGPU_FMT_DITHER_ENABLE, "on" },
 };
 
+static const struct drm_prop_enum_list amdgpu_user_pixenc_list[] = {
+ { 0, "auto" },
+ { DRM_COLOR_FORMAT_RGB444, "rgb" },
+ { DRM_COLOR_FORMAT_YCBCR444, "ycbcr444" },
+ { DRM_COLOR_FORMAT_YCBCR420, "ycbcr420" },
+};
+
+bool amdgpu_user_pixenc_from_name(
+ unsigned int *user_pixenc,
+ const char *pixenc_name)
+{
+ bool found = false;
+
+ if (pixenc_name && (*pixenc_name != '\0')) {
+ const int sz = ARRAY_SIZE(amdgpu_user_pixenc_list);
+ int i;
+
+ for (i = 0; !found && i < sz; ++i) {
+ if (strcmp(pixenc_name, amdgpu_user_pixenc_list[i].name) == 0) {
+ *user_pixenc = amdgpu_user_pixenc_list[i].type;
+ found = true;
+ }
+ }
+ }
+ return found;
+}
+
 int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
 {
  int sz;
@@ -1408,6 +1435,14 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
   "dither",
   amdgpu_dither_enum_list, sz);
 
+ sz = ARRAY_SIZE(amdgpu_user_pixenc_list);
+ adev->mode_info.pixel_encoding_property =
+ drm_property_create_enum(adev_to_drm(adev), 0,
+ "pixel encoding",
+ amdgpu_user_pixenc_list, sz);
+ if (!adev->mode_info.pixel_encoding_property)
+ return -ENOMEM;
+
  return 0;
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
index 9d19940f73c8..ee1ad49fa123 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
@@ -49,4 +49,7 @@ amdgpu_lookup_format_info(u32 format, uint64_t modifier);
 int amdgpu_display_suspend_helper(struct amdgpu_device *adev);
 int amdgpu_display_resume_helper(struct amdgpu_device *adev);
 
+bool amdgpu_user_pixenc_from_name(unsigned int *user_pixenc,
+   const char *pixenc_name);
+
 #endif
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index 5e3faefc5510..b984e66a5d75 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -326,6 +326,8 @@ struct amdgpu_mode_info {
  struct drm_property *audio_property;
  /* FMT dithering */
  struct drm_property *dither_property;
+ /* User HDMI pixel encoding override */
+ struct drm_property *pixel_encoding_property;
  /* hardcoded DFP edid from BIOS */
  const struct drm_edid *bios_hardcoded_edid;
 
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 bbfc47f6595f..2c03e0733178 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6069,6 +6069,115 @@ static bool adjust_colour_depth_from_display_info(
  return false;
 }
 
+/* convert an pixel encoding property value to a dc_pixel_encoding */
+static bool drm_prop_to_dc_pixel_encoding(
+ enum dc_pixel_encoding *dc_pixenc,
+ unsigned int propval)
+{
+ bool ret = false;
+
+ switch (propval) {
+ case 0:
+ *dc_pixenc = PIXEL_ENCODING_UNDEFINED;
+ ret = true;
+ break;
+ case DRM_COLOR_FORMAT_RGB444:
+ *dc_pixenc = PIXEL_ENCODING_RGB;
+ ret = true;
+ break;
+ case DRM_COLOR_FORMAT_YCBCR444:
+ *dc_pixenc = PIXEL_ENCODING_YCBCR444;
+ ret = true;
+ break;
+ case DRM_COLOR_FORMAT_YCBCR420:
+ *dc_pixenc = PIXEL_ENCODING_YCBCR420;
+ ret = true;
+ break;
+ default:
+ break;
+ }
+ return ret;
+}
+
+/* convert an dc_pixel_encoding to a pixel encoding property value */
+static unsigned int dc_pixel_encoding_to_drm_prop(
+ enum dc_pixel_encoding pixel_encoding)
+{
+ unsigned int propval = 0;
+
+ switch (pixel_encoding) {
+ case PIXEL_ENCODING_RGB:
+ propval = DRM_COLOR_FORMAT_RGB444;
+ break;
+ case PIXEL_ENCODING_YCBCR444:
+ propval = DRM_COLOR_FORMAT_YCBCR444;
+ break;
+ case PIXEL_ENCODING_YCBCR420:
+ propval = DRM_COLOR_FORMAT_YCBCR420;
+ break;
+ default:
+ break;
+ }
+ return propval;
+}
+
+/*
+ * Tries to read 'pixel_encoding' from the pixel_encoding DRM property on
+ * 'state'. Returns true if a supported, acceptable, non-undefined value is
+ * found; false otherwise. Only modifies 'pixel_encoding' if returning true.
+ */
+bool get_connector_state_pixel_encoding(
+ enum dc_pixel_encoding *pixel_encoding,
+ const struct drm_connector_state *state,
+ const struct drm_display_info *info,
+ const struct drm_display_mode *mode_in)
+{
+ bool ret = false;
+ struct dm_connector_state *dm_state;
+
+ dm_state = to_dm_connector_state(state);
+ if (!dm_state)
+ return false;
+
+ /* check encoding is supported */
+ switch (dm_state->pixel_encoding) {
+ case PIXEL_ENCODING_RGB:
+ ret = (info->color_formats & DRM_COLOR_FORMAT_RGB444);
+ break;
+ case PIXEL_ENCODING_YCBCR444:
+ ret = (info->color_formats & DRM_COLOR_FORMAT_YCBCR444);
+ break;
+ case PIXEL_ENCODING_YCBCR420:
+ ret = drm_mode_is_420(info, mode_in);
+ break;
+ default:
+ break;
+ }
+
+ if (ret)
+ *pixel_encoding = dm_state->pixel_encoding;
+
+ return ret;
+}
+
+/*
+ * Writes 'pixel_encoding' to the pixel_encoding DRM property on 'state', if
+ * the enum value is valid and supported; otherwise writes
+ * PIXEL_ENCODING_UNDEFINED which corresponds to the "auto" property state.
+ */
+void set_connector_state_pixel_encoding(
+ const struct drm_connector_state *state,
+ enum dc_pixel_encoding pixel_encoding)
+{
+ struct dm_connector_state *dm_state;
+
+ dm_state = to_dm_connector_state(state);
+ if (!dm_state)
+ return;
+
+ dm_state->pixel_encoding = pixel_encoding;
+}
+
 static void fill_stream_properties_from_drm_display_mode(
  struct dc_stream_state *stream,
  const struct drm_display_mode *mode_in,
@@ -6093,19 +6202,23 @@ static void fill_stream_properties_from_drm_display_mode(
  timing_out->h_border_right = 0;
  timing_out->v_border_top = 0;
  timing_out->v_border_bottom = 0;
- /* TODO: un-hardcode */
- if (drm_mode_is_420_only(info, mode_in)
+
+ if (!get_connector_state_pixel_encoding(&timing_out->pixel_encoding,
+ connector_state, info, mode_in)) {
+ /* auto-select a pixel encoding */
+ if (drm_mode_is_420_only(info, mode_in)
  && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
- timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
- else if (drm_mode_is_420_also(info, mode_in)
+ timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
+ else if (drm_mode_is_420_also(info, mode_in)
  && aconnector
  && aconnector->force_yuv420_output)
- timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
- else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
- && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
- timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
- else
- timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
+ timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
+ else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
+ && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
+ timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
+ else
+ timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
+ }
 
  timing_out->timing_3d_format = TIMING_3D_FORMAT_NONE;
  timing_out->display_color_depth = convert_color_depth_from_display_info(
@@ -6169,6 +6282,9 @@ static void fill_stream_properties_from_drm_display_mode(
  }
  }
 
+ /* write back final choice of pixel encoding */
+ set_connector_state_pixel_encoding(connector_state, timing_out->pixel_encoding);
+
  stream->output_color_space = get_output_color_space(timing_out, connector_state);
  stream->content_type = get_output_content_type(connector_state);
 }
@@ -6875,6 +6991,9 @@ int amdgpu_dm_connector_atomic_set_property(struct drm_connector *connector,
  } else if (property == adev->mode_info.underscan_property) {
  dm_new_state->underscan_enable = val;
  ret = 0;
+ } else if (property == adev->mode_info.pixel_encoding_property) {
+ if (drm_prop_to_dc_pixel_encoding(&dm_new_state->pixel_encoding, val))
+ ret = 0;
  }
 
  return ret;
@@ -6917,6 +7036,9 @@ int amdgpu_dm_connector_atomic_get_property(struct drm_connector *connector,
  } else if (property == adev->mode_info.underscan_property) {
  *val = dm_state->underscan_enable;
  ret = 0;
+ } else if (property == adev->mode_info.pixel_encoding_property) {
+ *val = dc_pixel_encoding_to_drm_prop(dm_state->pixel_encoding);
+ ret = 0;
  }
 
  return ret;
@@ -7088,6 +7210,20 @@ void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
  state->abm_level = amdgpu_dm_abm_level;
  }
 
+ switch (connector->cmdline_mode.pixel_encoding) {
+ case DRM_COLOR_FORMAT_RGB444:
+ state->pixel_encoding = PIXEL_ENCODING_RGB;
+ break;
+ case DRM_COLOR_FORMAT_YCBCR444:
+ state->pixel_encoding = PIXEL_ENCODING_YCBCR444;
+ break;
+ case DRM_COLOR_FORMAT_YCBCR420:
+ state->pixel_encoding = PIXEL_ENCODING_YCBCR420;
+ break;
+ default:
+ break;
+ }
+
  __drm_atomic_helper_connector_reset(connector, &state->base);
  }
 }
@@ -7114,6 +7250,7 @@ amdgpu_dm_connector_atomic_duplicate_state(struct drm_connector *connector)
  new_state->underscan_vborder = state->underscan_vborder;
  new_state->vcpi_slots = state->vcpi_slots;
  new_state->pbn = state->pbn;
+ new_state->pixel_encoding = state->pixel_encoding;
  return &new_state->base;
 }
 
@@ -8175,6 +8312,12 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 
  if (adev->dm.hdcp_workqueue)
  drm_connector_attach_content_protection_property(&aconnector->base, true);
+
+ if (adev->mode_info.pixel_encoding_property) {
+ drm_object_attach_property(&aconnector->base.base,
+ adev->mode_info.pixel_encoding_property, 0);
+ DRM_DEBUG_DRIVER("amdgpu: attached pixel encoding drm property");
+ }
  }
 }
 
@@ -9377,6 +9520,38 @@ static void amdgpu_dm_commit_audio(struct drm_device *dev,
  }
 }
 
+static void update_stream_for_pixel_encoding(
+ struct dc_stream_update *stream_update,
+ struct drm_connector *connector,
+ struct dm_crtc_state *dm_old_crtc_state,
+ struct dm_crtc_state *dm_new_crtc_state,
+ struct dm_connector_state *dm_new_con_state)
+{
+ struct amdgpu_dm_connector *aconnector =
+ to_amdgpu_dm_connector(connector);
+ struct dc_stream_state *new_stream = NULL;
+
+ if (aconnector)
+ new_stream = create_validate_stream_for_sink(
+ aconnector,
+ &dm_new_crtc_state->base.mode,
+ dm_new_con_state,
+ dm_old_crtc_state->stream);
+ if (new_stream) {
+ dm_new_crtc_state->stream->timing =
+ new_stream->timing;
+ stream_update->timing_for_pixel_encoding =
+ &dm_new_crtc_state->stream->timing;
+
+ dm_new_crtc_state->stream->output_color_space =
+ new_stream->output_color_space;
+ stream_update->output_color_space =
+ &dm_new_crtc_state->stream->output_color_space;
+
+ dc_stream_release(new_stream);
+ }
+}
+
 /*
  * amdgpu_dm_crtc_copy_transient_flags - copy mirrored flags from DRM to DC
  * @crtc_state: the DRM CRTC state
@@ -9850,7 +10025,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
  struct dc_stream_update stream_update;
  struct dc_info_packet hdr_packet;
  struct dc_stream_status *status = NULL;
- bool abm_changed, hdr_changed, scaling_changed;
+ bool abm_changed, hdr_changed, scaling_changed, pixenc_changed;
 
  memset(&stream_update, 0, sizeof(stream_update));
 
@@ -9875,7 +10050,10 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
  hdr_changed =
  !drm_connector_atomic_hdr_metadata_equal(old_con_state, new_con_state);
 
- if (!scaling_changed && !abm_changed && !hdr_changed)
+ pixenc_changed = dm_new_con_state->pixel_encoding !=
+  dm_old_con_state->pixel_encoding;
+
+ if (!scaling_changed && !abm_changed && !hdr_changed && !pixenc_changed)
  continue;
 
  stream_update.stream = dm_new_crtc_state->stream;
@@ -9898,6 +10076,13 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
  stream_update.hdr_static_metadata = &hdr_packet;
  }
 
+ if (pixenc_changed) {
+ update_stream_for_pixel_encoding(&stream_update,
+ connector,
+ dm_old_crtc_state, dm_new_crtc_state,
+ dm_new_con_state);
+ }
+
  status = dc_stream_get_status(dm_new_crtc_state->stream);
 
  if (WARN_ON(!status))
@@ -11403,6 +11588,12 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
  if (dm_old_con_state->abm_level != dm_new_con_state->abm_level ||
      dm_old_con_state->scaling != dm_new_con_state->scaling)
  new_crtc_state->connectors_changed = true;
+
+ if (dm_old_con_state->pixel_encoding !=
+     dm_new_con_state->pixel_encoding) {
+ new_crtc_state->connectors_changed = true;
+ new_crtc_state->mode_changed = true;
+ }
  }
 
  if (dc_resource_is_dsc_encoding_supported(dc)) {
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 25e95775c45c..132944272c18 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -918,6 +918,7 @@ struct dm_connector_state {
  uint8_t abm_level;
  int vcpi_slots;
  uint64_t pbn;
+ enum dc_pixel_encoding pixel_encoding;
 };
 
 #define to_dm_connector_state(x)\
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 5a12fc75f97f..4ac004536707 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -2765,6 +2765,11 @@ static enum surface_update_type check_update_surfaces_for_stream(
  if (stream_update->output_csc_transform || stream_update->output_color_space)
  su_flags->bits.out_csc = 1;
 
+ if (stream_update->timing_for_pixel_encoding) {
+ su_flags->bits.pixel_encoding = 1;
+ elevate_update_type(&overall_type, UPDATE_TYPE_FULL);
+ }
+
  /* Output transfer function changes do not require bandwidth recalculation,
   * so don't trigger a full update
   */
@@ -3112,6 +3117,8 @@ static void copy_stream_update_to_stream(struct dc *dc,
  stream->scaler_sharpener_update = *update->scaler_sharpener_update;
  if (update->sharpening_required)
  stream->sharpening_required = *update->sharpening_required;
+ if (update->timing_for_pixel_encoding)
+ stream->timing = *update->timing_for_pixel_encoding;
 }
 
 static void backup_planes_and_stream_state(
@@ -3361,6 +3368,7 @@ static void commit_planes_do_stream_update(struct dc *dc,
  stream_update->vsc_infopacket ||
  stream_update->vsp_infopacket ||
  stream_update->hfvsif_infopacket ||
+ stream_update->timing_for_pixel_encoding ||
  stream_update->adaptive_sync_infopacket ||
  stream_update->vtem_infopacket) {
  resource_build_info_frame(pipe_ctx);
diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h
index 413970588a26..de7f02f330d8 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_stream.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h
@@ -144,6 +144,7 @@ union stream_update_flags {
  uint32_t fams_changed : 1;
  uint32_t scaler_sharpener : 1;
  uint32_t sharpening_required : 1;
+ uint32_t pixel_encoding:1;
  } bits;
 
  uint32_t raw;
@@ -350,6 +351,7 @@ struct dc_stream_update {
  struct dc_mst_stream_bw_update *mst_bw_update;
  struct dc_transfer_func *func_shaper;
  struct dc_3dlut *lut3d_func;
+ struct dc_crtc_timing *timing_for_pixel_encoding;
 
  struct test_pattern *pending_test_pattern;
  struct dc_crtc_timing_adjust *crtc_timing_adjust;
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 1a0890083aee..b5ce8f93b672 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -2161,6 +2161,32 @@ static int drm_mode_parse_tv_mode(const char *delim,
  return 0;
 }
 
+static int drm_mode_parse_pixel_encoding(const char *delim,
+  struct drm_cmdline_mode *mode)
+{
+ const char *value;
+
+ if (*delim != '=')
+ return -EINVAL;
+
+ value = delim + 1;
+ delim = strchr(value, ',');
+ if (!delim)
+ delim = value + strlen(value);
+
+ if (!strncmp(value, "auto", delim - value))
+ mode->pixel_encoding = 0;
+ else if (!strncmp(value, "rgb", delim - value))
+ mode->pixel_encoding = DRM_COLOR_FORMAT_RGB444;
+ else if (!strncmp(value, "ycbcr444", delim - value))
+ mode->pixel_encoding = DRM_COLOR_FORMAT_YCBCR444;
+ else if (!strncmp(value, "ycbcr420", delim - value))
+ mode->pixel_encoding = DRM_COLOR_FORMAT_YCBCR420;
+ else
+ return -EINVAL;
+
+ return 0;
+}
 static int drm_mode_parse_cmdline_options(const char *str,
    bool freestanding,
    const struct drm_connector *connector,
@@ -2233,6 +2259,9 @@ static int drm_mode_parse_cmdline_options(const char *str,
  } else if (!strncmp(option, "tv_mode", delim - option)) {
  if (drm_mode_parse_tv_mode(delim, mode))
  return -EINVAL;
+ } else if (!strncmp(option, "pixel_encoding", delim - option)) {
+ if (drm_mode_parse_pixel_encoding(delim, mode))
+ return -EINVAL;
  } else {
  return -EINVAL;
  }
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index c754651044d4..89c0f42873df 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1666,6 +1666,13 @@ struct drm_cmdline_mode {
   * Did the mode have a preferred TV mode?
   */
  bool tv_mode_specified;
+
+ /**
+  * @pixel_encoding:
+  *
+  * Initial pixel encoding.
+  */
+ unsigned int pixel_encoding;
 };
 
 /*
-- 
2.43.0

Re: Fw: [PATCH] drm/amdgpu add "pixel_encoding"

[Xaver Hugl]

Am Mo., 28. Okt. 2024 um 16:18 Uhr schrieb Matias N. Goldberg
<dark_sylinc@yahoo.com.ar>:
>
> SENDING AGAIN because it was sent with HTML formatting, which screwed up the email.
>
>
> Hi!
>
> This is my first patch ever to DRM/amdgpu (technically I didn't write the patch, I just kept rebasing it over 4 years, and did lots of testing).
>
> I'm following the advise from:
> https://gitlab.freedesktop.org/drm/amd/-/issues/476#note_2628536
>
> Why this patch? Because it fix a practical problem with certain monitors that auto-select ycbcr444 when they should be set to "rgb" (this issue also happens on Windows) when plugging an HDMI monitor to a DisplayPort via passive adapter.
>
>
> IMPORTANT remarks about the public API:
>
> 1. This API adds "pixel encoding" which has options "auto" (Default), "rgb", "ycbcr444", and "ycbcr420"
Is ycbcr420 not possible with amdgpu?

> 2. Intel has "RGB Broadcast". Its options are "Full" and "Limited 16:235". There is a lot of overlap here. On Windows AMD Control Panel offers the option "Pixel Encoding" and its options are:
>  - RGB (full)
>  - RGB (limited)
>  - ycbcr444
>  - ycbcr420
>  - ycbcr422
>
> Which means that it may be worth reserving more keywords for this patch.
You don't need to reserve anything. Additional enum values can be
added at any time and don't need to have unique names vs. other enums.

> I don't know why ycbcr444/ycbcr420 do not offer full/limited variants on Windows, but I suspect it's because the driver can automatically tell whether to use Full vs Limited for ycbcr variants..
>
> 3. amdgpu already has "Colorspace" option. However this is a different (although related) option. But there is quite the overlap:
>
> - Full/Limited options are sometimes included as part of the colorspace, such is the case of COLOR_SPACE_2020_RGB_FULLRANGE vs COLOR_SPACE_2020_RGB_LIMITEDRANGE, but there is no "LIMITED"/"FULL" variants for COLOR_SPACE_2020_YCBCR. Yet there are for YCBCR601 and YCBCR709. This may be a spec thing (I don’t know).
>
> - AFAIK amdgpu’s Limited/Full variants are not controllable from the public user-space API.
>
> - Certain options in "Colorspace" only makes sense in specific pixel encoding. For example the option "opRGB" currently glitches when amdgpu is using ycbcr encoding (you can tell the monitor is interpreting the RGB signal as YUV, thus white becomes purple and black becomes green).
opRGB is not supported by any screen I've ever looked at, so it
doesn't surprise me that you'd get glitches.

> - The kernel has DRM_MODE_COLORIMETRY_BT2020_RGB and DRM_MODE_COLORIMETRY_BT2020_YCC. AMDGPU kernel currently forces COLOR_SPACE_2020_RGB_FULLRANGE or COLOR_SPACE_2020_YCBCR based on the current pixel encoding (e.g. if using RGB encodings and colorspace == DRM_MODE_COLORIMETRY_BT2020_YCC, it will be changed for COLOR_SPACE_2020_RGB_FULLRANGE with currently no choice for Limited range).
> - amdgpu seems to be able to automatically distinguish between Full and Limited for ycbcr  formats via the presence of flags.Y_ONLY bit. There is no such auto-detection for RGB.
>
>
> The way I see it, there are 3 configurations that can be arranged together:
>
> - Pixel Encoding (RGB vs ycbcr444 vs ycbcr420).
> - Colorspace (YCBCR709 vs BT.2020 etc).
> - Full vs Limited.
>
> However not all options are compatible with everything.
That's not a problem, incompatible options can just be rejected in atomic tests.

> I suspect doing the same as what AMD does on Windows (offer RGB/Full + RGB/Limited + all the ycbcr variants for "pixel encoding") is the best choice (and leave the "Colorspace" option as is, for very advanced manipulation).
>
> 4. Xaver Hugl expressed his concerns with the current patch that having an option called "auto" with no way of querying what is the current auto-selected encoding is sub-optimal.
>
>
> Cheers
> Matias
>
>
>
> From 6806baac51f1ac2028c49bcab216c19f26a7e92b Mon Sep 17 00:00:00 2001
> From: "Matias N. Goldberg" <dark_sylinc@yahoo.com.ar>
> Date: Sun, 6 Oct 2024 21:04:11 -0300
> Subject: [PATCH] [amdgpu] Add "pixel_encoding" to switch between RGB & YUV
>  color modes
>
> Usage:
> xrandr --output DisplayPort-1 --set "pixel encoding" rgb
>
> Supported options are: "auto" (Default), "rgb", "ycbcr444", and
> "ycbcr420"
>
> This patch allows users to switch between pixel encodings, which is
> specially important when auto gets it wrong (probably because of
> monitor's manufacturer mistake) and needs user intervention.
>
> Original patch by Yassine Imounachen
> Rebased by Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
>
> Full discussion:
> https://gitlab.freedesktop.org/drm/amd/-/issues/476#note_2628536
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   |  35 +++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.h   |   3 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h      |   2 +
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 215 +++++++++++++++++-
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   1 +
>  drivers/gpu/drm/amd/display/dc/core/dc.c      |   8 +
>  drivers/gpu/drm/amd/display/dc/dc_stream.h    |   2 +
>  drivers/gpu/drm/drm_modes.c                   |  29 +++
>  include/drm/drm_connector.h                   |   7 +
>  9 files changed, 290 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index b119d27271c1..9d201f368b6e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -1362,6 +1362,33 @@ static const struct drm_prop_enum_list amdgpu_dither_enum_list[] = {
>   { AMDGPU_FMT_DITHER_ENABLE, "on" },
>  };
>
> +static const struct drm_prop_enum_list amdgpu_user_pixenc_list[] = {
> + { 0, "auto" },
> + { DRM_COLOR_FORMAT_RGB444, "rgb" },
> + { DRM_COLOR_FORMAT_YCBCR444, "ycbcr444" },
> + { DRM_COLOR_FORMAT_YCBCR420, "ycbcr420" },
> +};
> +
> +bool amdgpu_user_pixenc_from_name(
> + unsigned int *user_pixenc,
> + const char *pixenc_name)
> +{
> + bool found = false;
> +
> + if (pixenc_name && (*pixenc_name != '\0')) {
> + const int sz = ARRAY_SIZE(amdgpu_user_pixenc_list);
> + int i;
> +
> + for (i = 0; !found && i < sz; ++i) {
> + if (strcmp(pixenc_name, amdgpu_user_pixenc_list[i].name) == 0) {
> + *user_pixenc = amdgpu_user_pixenc_list[i].type;
> + found = true;
> + }
> + }
> + }
> + return found;
> +}
> +
>  int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
>  {
>   int sz;
> @@ -1408,6 +1435,14 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
>    "dither",
>    amdgpu_dither_enum_list, sz);
>
> + sz = ARRAY_SIZE(amdgpu_user_pixenc_list);
> + adev->mode_info.pixel_encoding_property =
> + drm_property_create_enum(adev_to_drm(adev), 0,
> + "pixel encoding",
> + amdgpu_user_pixenc_list, sz);
> + if (!adev->mode_info.pixel_encoding_property)
> + return -ENOMEM;
> +
>   return 0;
>  }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> index 9d19940f73c8..ee1ad49fa123 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> @@ -49,4 +49,7 @@ amdgpu_lookup_format_info(u32 format, uint64_t modifier);
>  int amdgpu_display_suspend_helper(struct amdgpu_device *adev);
>  int amdgpu_display_resume_helper(struct amdgpu_device *adev);
>
> +bool amdgpu_user_pixenc_from_name(unsigned int *user_pixenc,
> +   const char *pixenc_name);
> +
>  #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index 5e3faefc5510..b984e66a5d75 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -326,6 +326,8 @@ struct amdgpu_mode_info {
>   struct drm_property *audio_property;
>   /* FMT dithering */
>   struct drm_property *dither_property;
> + /* User HDMI pixel encoding override */
> + struct drm_property *pixel_encoding_property;
>   /* hardcoded DFP edid from BIOS */
>   const struct drm_edid *bios_hardcoded_edid;
>
> 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 bbfc47f6595f..2c03e0733178 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6069,6 +6069,115 @@ static bool adjust_colour_depth_from_display_info(
>   return false;
>  }
>
> +/* convert an pixel encoding property value to a dc_pixel_encoding */
> +static bool drm_prop_to_dc_pixel_encoding(
> + enum dc_pixel_encoding *dc_pixenc,
> + unsigned int propval)
> +{
> + bool ret = false;
> +
> + switch (propval) {
> + case 0:
> + *dc_pixenc = PIXEL_ENCODING_UNDEFINED;
> + ret = true;
> + break;
> + case DRM_COLOR_FORMAT_RGB444:
> + *dc_pixenc = PIXEL_ENCODING_RGB;
> + ret = true;
> + break;
> + case DRM_COLOR_FORMAT_YCBCR444:
> + *dc_pixenc = PIXEL_ENCODING_YCBCR444;
> + ret = true;
> + break;
> + case DRM_COLOR_FORMAT_YCBCR420:
> + *dc_pixenc = PIXEL_ENCODING_YCBCR420;
> + ret = true;
> + break;
> + default:
> + break;
> + }
> + return ret;
> +}
> +
> +/* convert an dc_pixel_encoding to a pixel encoding property value */
> +static unsigned int dc_pixel_encoding_to_drm_prop(
> + enum dc_pixel_encoding pixel_encoding)
> +{
> + unsigned int propval = 0;
> +
> + switch (pixel_encoding) {
> + case PIXEL_ENCODING_RGB:
> + propval = DRM_COLOR_FORMAT_RGB444;
> + break;
> + case PIXEL_ENCODING_YCBCR444:
> + propval = DRM_COLOR_FORMAT_YCBCR444;
> + break;
> + case PIXEL_ENCODING_YCBCR420:
> + propval = DRM_COLOR_FORMAT_YCBCR420;
> + break;
> + default:
> + break;
> + }
> + return propval;
> +}
> +
> +/*
> + * Tries to read 'pixel_encoding' from the pixel_encoding DRM property on
> + * 'state'. Returns true if a supported, acceptable, non-undefined value is
> + * found; false otherwise. Only modifies 'pixel_encoding' if returning true.
> + */
> +bool get_connector_state_pixel_encoding(
> + enum dc_pixel_encoding *pixel_encoding,
> + const struct drm_connector_state *state,
> + const struct drm_display_info *info,
> + const struct drm_display_mode *mode_in)
> +{
> + bool ret = false;
> + struct dm_connector_state *dm_state;
> +
> + dm_state = to_dm_connector_state(state);
> + if (!dm_state)
> + return false;
> +
> + /* check encoding is supported */
> + switch (dm_state->pixel_encoding) {
> + case PIXEL_ENCODING_RGB:
> + ret = (info->color_formats & DRM_COLOR_FORMAT_RGB444);
> + break;
> + case PIXEL_ENCODING_YCBCR444:
> + ret = (info->color_formats & DRM_COLOR_FORMAT_YCBCR444);
> + break;
> + case PIXEL_ENCODING_YCBCR420:
> + ret = drm_mode_is_420(info, mode_in);
> + break;
> + default:
> + break;
> + }
> +
> + if (ret)
> + *pixel_encoding = dm_state->pixel_encoding;
> +
> + return ret;
> +}
> +
> +/*
> + * Writes 'pixel_encoding' to the pixel_encoding DRM property on 'state', if
> + * the enum value is valid and supported; otherwise writes
> + * PIXEL_ENCODING_UNDEFINED which corresponds to the "auto" property state.
> + */
> +void set_connector_state_pixel_encoding(
> + const struct drm_connector_state *state,
> + enum dc_pixel_encoding pixel_encoding)
> +{
> + struct dm_connector_state *dm_state;
> +
> + dm_state = to_dm_connector_state(state);
> + if (!dm_state)
> + return;
> +
> + dm_state->pixel_encoding = pixel_encoding;
> +}
> +
>  static void fill_stream_properties_from_drm_display_mode(
>   struct dc_stream_state *stream,
>   const struct drm_display_mode *mode_in,
> @@ -6093,19 +6202,23 @@ static void fill_stream_properties_from_drm_display_mode(
>   timing_out->h_border_right = 0;
>   timing_out->v_border_top = 0;
>   timing_out->v_border_bottom = 0;
> - /* TODO: un-hardcode */
> - if (drm_mode_is_420_only(info, mode_in)
> +
> + if (!get_connector_state_pixel_encoding(&timing_out->pixel_encoding,
> + connector_state, info, mode_in)) {
> + /* auto-select a pixel encoding */
> + if (drm_mode_is_420_only(info, mode_in)
>   && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
> - timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
> - else if (drm_mode_is_420_also(info, mode_in)
> + timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
> + else if (drm_mode_is_420_also(info, mode_in)
>   && aconnector
>   && aconnector->force_yuv420_output)
> - timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
> - else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
> - && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
> - timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
> - else
> - timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
> + timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
> + else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
> + && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
> + timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
> + else
> + timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
> + }
>
>   timing_out->timing_3d_format = TIMING_3D_FORMAT_NONE;
>   timing_out->display_color_depth = convert_color_depth_from_display_info(
> @@ -6169,6 +6282,9 @@ static void fill_stream_properties_from_drm_display_mode(
>   }
>   }
>
> + /* write back final choice of pixel encoding */
> + set_connector_state_pixel_encoding(connector_state, timing_out->pixel_encoding);
> +
>   stream->output_color_space = get_output_color_space(timing_out, connector_state);
>   stream->content_type = get_output_content_type(connector_state);
>  }
> @@ -6875,6 +6991,9 @@ int amdgpu_dm_connector_atomic_set_property(struct drm_connector *connector,
>   } else if (property == adev->mode_info.underscan_property) {
>   dm_new_state->underscan_enable = val;
>   ret = 0;
> + } else if (property == adev->mode_info.pixel_encoding_property) {
> + if (drm_prop_to_dc_pixel_encoding(&dm_new_state->pixel_encoding, val))
> + ret = 0;
>   }
>
>   return ret;
> @@ -6917,6 +7036,9 @@ int amdgpu_dm_connector_atomic_get_property(struct drm_connector *connector,
>   } else if (property == adev->mode_info.underscan_property) {
>   *val = dm_state->underscan_enable;
>   ret = 0;
> + } else if (property == adev->mode_info.pixel_encoding_property) {
> + *val = dc_pixel_encoding_to_drm_prop(dm_state->pixel_encoding);
> + ret = 0;
>   }
>
>   return ret;
> @@ -7088,6 +7210,20 @@ void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
>   state->abm_level = amdgpu_dm_abm_level;
>   }
>
> + switch (connector->cmdline_mode.pixel_encoding) {
> + case DRM_COLOR_FORMAT_RGB444:
> + state->pixel_encoding = PIXEL_ENCODING_RGB;
> + break;
> + case DRM_COLOR_FORMAT_YCBCR444:
> + state->pixel_encoding = PIXEL_ENCODING_YCBCR444;
> + break;
> + case DRM_COLOR_FORMAT_YCBCR420:
> + state->pixel_encoding = PIXEL_ENCODING_YCBCR420;
> + break;
> + default:
> + break;
> + }
> +
>   __drm_atomic_helper_connector_reset(connector, &state->base);
>   }
>  }
> @@ -7114,6 +7250,7 @@ amdgpu_dm_connector_atomic_duplicate_state(struct drm_connector *connector)
>   new_state->underscan_vborder = state->underscan_vborder;
>   new_state->vcpi_slots = state->vcpi_slots;
>   new_state->pbn = state->pbn;
> + new_state->pixel_encoding = state->pixel_encoding;
>   return &new_state->base;
>  }
>
> @@ -8175,6 +8312,12 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
>
>   if (adev->dm.hdcp_workqueue)
>   drm_connector_attach_content_protection_property(&aconnector->base, true);
> +
> + if (adev->mode_info.pixel_encoding_property) {
> + drm_object_attach_property(&aconnector->base.base,
> + adev->mode_info.pixel_encoding_property, 0);
> + DRM_DEBUG_DRIVER("amdgpu: attached pixel encoding drm property");
> + }
>   }
>  }
>
> @@ -9377,6 +9520,38 @@ static void amdgpu_dm_commit_audio(struct drm_device *dev,
>   }
>  }
>
> +static void update_stream_for_pixel_encoding(
> + struct dc_stream_update *stream_update,
> + struct drm_connector *connector,
> + struct dm_crtc_state *dm_old_crtc_state,
> + struct dm_crtc_state *dm_new_crtc_state,
> + struct dm_connector_state *dm_new_con_state)
> +{
> + struct amdgpu_dm_connector *aconnector =
> + to_amdgpu_dm_connector(connector);
> + struct dc_stream_state *new_stream = NULL;
> +
> + if (aconnector)
> + new_stream = create_validate_stream_for_sink(
> + aconnector,
> + &dm_new_crtc_state->base.mode,
> + dm_new_con_state,
> + dm_old_crtc_state->stream);
> + if (new_stream) {
> + dm_new_crtc_state->stream->timing =
> + new_stream->timing;
> + stream_update->timing_for_pixel_encoding =
> + &dm_new_crtc_state->stream->timing;
> +
> + dm_new_crtc_state->stream->output_color_space =
> + new_stream->output_color_space;
> + stream_update->output_color_space =
> + &dm_new_crtc_state->stream->output_color_space;
> +
> + dc_stream_release(new_stream);
> + }
> +}
> +
>  /*
>   * amdgpu_dm_crtc_copy_transient_flags - copy mirrored flags from DRM to DC
>   * @crtc_state: the DRM CRTC state
> @@ -9850,7 +10025,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>   struct dc_stream_update stream_update;
>   struct dc_info_packet hdr_packet;
>   struct dc_stream_status *status = NULL;
> - bool abm_changed, hdr_changed, scaling_changed;
> + bool abm_changed, hdr_changed, scaling_changed, pixenc_changed;
>
>   memset(&stream_update, 0, sizeof(stream_update));
>
> @@ -9875,7 +10050,10 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>   hdr_changed =
>   !drm_connector_atomic_hdr_metadata_equal(old_con_state, new_con_state);
>
> - if (!scaling_changed && !abm_changed && !hdr_changed)
> + pixenc_changed = dm_new_con_state->pixel_encoding !=
> +  dm_old_con_state->pixel_encoding;
> +
> + if (!scaling_changed && !abm_changed && !hdr_changed && !pixenc_changed)
>   continue;
>
>   stream_update.stream = dm_new_crtc_state->stream;
> @@ -9898,6 +10076,13 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>   stream_update.hdr_static_metadata = &hdr_packet;
>   }
>
> + if (pixenc_changed) {
> + update_stream_for_pixel_encoding(&stream_update,
> + connector,
> + dm_old_crtc_state, dm_new_crtc_state,
> + dm_new_con_state);
> + }
> +
>   status = dc_stream_get_status(dm_new_crtc_state->stream);
>
>   if (WARN_ON(!status))
> @@ -11403,6 +11588,12 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   if (dm_old_con_state->abm_level != dm_new_con_state->abm_level ||
>       dm_old_con_state->scaling != dm_new_con_state->scaling)
>   new_crtc_state->connectors_changed = true;
> +
> + if (dm_old_con_state->pixel_encoding !=
> +     dm_new_con_state->pixel_encoding) {
> + new_crtc_state->connectors_changed = true;
> + new_crtc_state->mode_changed = true;
> + }
>   }
>
>   if (dc_resource_is_dsc_encoding_supported(dc)) {
> 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 25e95775c45c..132944272c18 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -918,6 +918,7 @@ struct dm_connector_state {
>   uint8_t abm_level;
>   int vcpi_slots;
>   uint64_t pbn;
> + enum dc_pixel_encoding pixel_encoding;
>  };
>
>  #define to_dm_connector_state(x)\
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
> index 5a12fc75f97f..4ac004536707 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> @@ -2765,6 +2765,11 @@ static enum surface_update_type check_update_surfaces_for_stream(
>   if (stream_update->output_csc_transform || stream_update->output_color_space)
>   su_flags->bits.out_csc = 1;
>
> + if (stream_update->timing_for_pixel_encoding) {
> + su_flags->bits.pixel_encoding = 1;
> + elevate_update_type(&overall_type, UPDATE_TYPE_FULL);
> + }
> +
>   /* Output transfer function changes do not require bandwidth recalculation,
>    * so don't trigger a full update
>    */
> @@ -3112,6 +3117,8 @@ static void copy_stream_update_to_stream(struct dc *dc,
>   stream->scaler_sharpener_update = *update->scaler_sharpener_update;
>   if (update->sharpening_required)
>   stream->sharpening_required = *update->sharpening_required;
> + if (update->timing_for_pixel_encoding)
> + stream->timing = *update->timing_for_pixel_encoding;
>  }
>
>  static void backup_planes_and_stream_state(
> @@ -3361,6 +3368,7 @@ static void commit_planes_do_stream_update(struct dc *dc,
>   stream_update->vsc_infopacket ||
>   stream_update->vsp_infopacket ||
>   stream_update->hfvsif_infopacket ||
> + stream_update->timing_for_pixel_encoding ||
>   stream_update->adaptive_sync_infopacket ||
>   stream_update->vtem_infopacket) {
>   resource_build_info_frame(pipe_ctx);
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h
> index 413970588a26..de7f02f330d8 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h
> @@ -144,6 +144,7 @@ union stream_update_flags {
>   uint32_t fams_changed : 1;
>   uint32_t scaler_sharpener : 1;
>   uint32_t sharpening_required : 1;
> + uint32_t pixel_encoding:1;
>   } bits;
>
>   uint32_t raw;
> @@ -350,6 +351,7 @@ struct dc_stream_update {
>   struct dc_mst_stream_bw_update *mst_bw_update;
>   struct dc_transfer_func *func_shaper;
>   struct dc_3dlut *lut3d_func;
> + struct dc_crtc_timing *timing_for_pixel_encoding;
>
>   struct test_pattern *pending_test_pattern;
>   struct dc_crtc_timing_adjust *crtc_timing_adjust;
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 1a0890083aee..b5ce8f93b672 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2161,6 +2161,32 @@ static int drm_mode_parse_tv_mode(const char *delim,
>   return 0;
>  }
>
> +static int drm_mode_parse_pixel_encoding(const char *delim,
> +  struct drm_cmdline_mode *mode)
> +{
> + const char *value;
> +
> + if (*delim != '=')
> + return -EINVAL;
> +
> + value = delim + 1;
> + delim = strchr(value, ',');
> + if (!delim)
> + delim = value + strlen(value);
> +
> + if (!strncmp(value, "auto", delim - value))
> + mode->pixel_encoding = 0;
> + else if (!strncmp(value, "rgb", delim - value))
> + mode->pixel_encoding = DRM_COLOR_FORMAT_RGB444;
> + else if (!strncmp(value, "ycbcr444", delim - value))
> + mode->pixel_encoding = DRM_COLOR_FORMAT_YCBCR444;
> + else if (!strncmp(value, "ycbcr420", delim - value))
> + mode->pixel_encoding = DRM_COLOR_FORMAT_YCBCR420;
> + else
> + return -EINVAL;
> +
> + return 0;
> +}
>  static int drm_mode_parse_cmdline_options(const char *str,
>     bool freestanding,
>     const struct drm_connector *connector,
> @@ -2233,6 +2259,9 @@ static int drm_mode_parse_cmdline_options(const char *str,
>   } else if (!strncmp(option, "tv_mode", delim - option)) {
>   if (drm_mode_parse_tv_mode(delim, mode))
>   return -EINVAL;
> + } else if (!strncmp(option, "pixel_encoding", delim - option)) {
> + if (drm_mode_parse_pixel_encoding(delim, mode))
> + return -EINVAL;
>   } else {
>   return -EINVAL;
>   }
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index c754651044d4..89c0f42873df 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1666,6 +1666,13 @@ struct drm_cmdline_mode {
>    * Did the mode have a preferred TV mode?
>    */
>   bool tv_mode_specified;
> +
> + /**
> +  * @pixel_encoding:
> +  *
> +  * Initial pixel encoding.
> +  */
> + unsigned int pixel_encoding;
>  };
>
>  /*
> --
> 2.43.0

Re: [PATCH] drm/amdgpu add "pixel_encoding"

[Matias N. Goldberg]

> That's not a problem, incompatible options can just be rejected in atomic tests.

I was thinking from a user perspective. It'd be easier for user-space config apps to present only the valid options, rather than offering all possible combinations only to reject them after the user selected an invalid config (or perform hundreds of atomic tests every time we need to show a config dialog).


> opRGB is not supported by any screen I've ever looked at, so it doesn't surprise me that you'd get glitches.

I don't know if it was just a fallback, but my screen *does* work when using opRGB with rgb encoding (it glitches when using yuv encoding).



Cheers
Matias



El lunes, 4 de noviembre de 2024, 11:49:28 a. m. ART, Xaver Hugl <xaver.hugl@kde.org> escribió: 





Am Mo., 28. Okt. 2024 um 16:18 Uhr schrieb Matias N. Goldberg
<dark_sylinc@yahoo.com.ar>:
>
> SENDING AGAIN because it was sent with HTML formatting, which screwed up the email.
>
>
> Hi!
>
> This is my first patch ever to DRM/amdgpu (technically I didn't write the patch, I just kept rebasing it over 4 years, and did lots of testing).
>
> I'm following the advise from:
> https://gitlab.freedesktop.org/drm/amd/-/issues/476#note_2628536
>
> Why this patch? Because it fix a practical problem with certain monitors that auto-select ycbcr444 when they should be set to "rgb" (this issue also happens on Windows) when plugging an HDMI monitor to a DisplayPort via passive adapter.
>
>
> IMPORTANT remarks about the public API:
>
> 1. This API adds "pixel encoding" which has options "auto" (Default), "rgb", "ycbcr444", and "ycbcr420"
Is ycbcr420 not possible with amdgpu?

> 2. Intel has "RGB Broadcast". Its options are "Full" and "Limited 16:235". There is a lot of overlap here. On Windows AMD Control Panel offers the option "Pixel Encoding" and its options are:
>  - RGB (full)
>  - RGB (limited)
>  - ycbcr444
>  - ycbcr420
>  - ycbcr422
>
> Which means that it may be worth reserving more keywords for this patch.
You don't need to reserve anything. Additional enum values can be
added at any time and don't need to have unique names vs. other enums.

> I don't know why ycbcr444/ycbcr420 do not offer full/limited variants on Windows, but I suspect it's because the driver can automatically tell whether to use Full vs Limited for ycbcr variants..
>
> 3. amdgpu already has "Colorspace" option. However this is a different (although related) option. But there is quite the overlap:
>
> - Full/Limited options are sometimes included as part of the colorspace, such is the case of COLOR_SPACE_2020_RGB_FULLRANGE vs COLOR_SPACE_2020_RGB_LIMITEDRANGE, but there is no "LIMITED"/"FULL" variants for COLOR_SPACE_2020_YCBCR. Yet there are for YCBCR601 and YCBCR709. This may be a spec thing (I don’t know).
>
> - AFAIK amdgpu’s Limited/Full variants are not controllable from the public user-space API.
>
> - Certain options in "Colorspace" only makes sense in specific pixel encoding. For example the option "opRGB" currently glitches when amdgpu is using ycbcr encoding (you can tell the monitor is interpreting the RGB signal as YUV, thus white becomes purple and black becomes green).
opRGB is not supported by any screen I've ever looked at, so it
doesn't surprise me that you'd get glitches.

> - The kernel has DRM_MODE_COLORIMETRY_BT2020_RGB and DRM_MODE_COLORIMETRY_BT2020_YCC. AMDGPU kernel currently forces COLOR_SPACE_2020_RGB_FULLRANGE or COLOR_SPACE_2020_YCBCR based on the current pixel encoding (e.g. if using RGB encodings and colorspace == DRM_MODE_COLORIMETRY_BT2020_YCC, it will be changed for COLOR_SPACE_2020_RGB_FULLRANGE with currently no choice for Limited range).
> - amdgpu seems to be able to automatically distinguish between Full and Limited for ycbcr  formats via the presence of flags.Y_ONLY bit. There is no such auto-detection for RGB.
>
>
> The way I see it, there are 3 configurations that can be arranged together:
>
> - Pixel Encoding (RGB vs ycbcr444 vs ycbcr420).
> - Colorspace (YCBCR709 vs BT.2020 etc).
> - Full vs Limited.
>
> However not all options are compatible with everything.
That's not a problem, incompatible options can just be rejected in atomic tests.

> I suspect doing the same as what AMD does on Windows (offer RGB/Full + RGB/Limited + all the ycbcr variants for "pixel encoding") is the best choice (and leave the "Colorspace" option as is, for very advanced manipulation).
>
> 4. Xaver Hugl expressed his concerns with the current patch that having an option called "auto" with no way of querying what is the current auto-selected encoding is sub-optimal.
>
>
> Cheers
> Matias
>
>
>
> From 6806baac51f1ac2028c49bcab216c19f26a7e92b Mon Sep 17 00:00:00 2001
> From: "Matias N. Goldberg" <dark_sylinc@yahoo.com.ar>
> Date: Sun, 6 Oct 2024 21:04:11 -0300
> Subject: [PATCH] [amdgpu] Add "pixel_encoding" to switch between RGB & YUV
>  color modes
>
> Usage:
> xrandr --output DisplayPort-1 --set "pixel encoding" rgb
>
> Supported options are: "auto" (Default), "rgb", "ycbcr444", and
> "ycbcr420"
>
> This patch allows users to switch between pixel encodings, which is
> specially important when auto gets it wrong (probably because of
> monitor's manufacturer mistake) and needs user intervention.
>
> Original patch by Yassine Imounachen
> Rebased by Matias N. Goldberg <dark_sylinc@yahoo.com.ar>
>
> Full discussion:
> https://gitlab.freedesktop.org/drm/amd/-/issues/476#note_2628536
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c  |  35 +++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.h  |  3 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h      |  2 +
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 215 +++++++++++++++++-
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  1 +
>  drivers/gpu/drm/amd/display/dc/core/dc.c      |  8 +
>  drivers/gpu/drm/amd/display/dc/dc_stream.h    |  2 +
>  drivers/gpu/drm/drm_modes.c                  |  29 +++
>  include/drm/drm_connector.h                  |  7 +
>  9 files changed, 290 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index b119d27271c1..9d201f368b6e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -1362,6 +1362,33 @@ static const struct drm_prop_enum_list amdgpu_dither_enum_list[] = {
>  { AMDGPU_FMT_DITHER_ENABLE, "on" },
>  };
>
> +static const struct drm_prop_enum_list amdgpu_user_pixenc_list[] = {
> + { 0, "auto" },
> + { DRM_COLOR_FORMAT_RGB444, "rgb" },
> + { DRM_COLOR_FORMAT_YCBCR444, "ycbcr444" },
> + { DRM_COLOR_FORMAT_YCBCR420, "ycbcr420" },
> +};
> +
> +bool amdgpu_user_pixenc_from_name(
> + unsigned int *user_pixenc,
> + const char *pixenc_name)
> +{
> + bool found = false;
> +
> + if (pixenc_name && (*pixenc_name != '\0')) {
> + const int sz = ARRAY_SIZE(amdgpu_user_pixenc_list);
> + int i;
> +
> + for (i = 0; !found && i < sz; ++i) {
> + if (strcmp(pixenc_name, amdgpu_user_pixenc_list[i].name) == 0) {
> + *user_pixenc = amdgpu_user_pixenc_list[i].type;
> + found = true;
> + }
> + }
> + }
> + return found;
> +}
> +
>  int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
>  {
>  int sz;
> @@ -1408,6 +1435,14 @@ int amdgpu_display_modeset_create_props(struct amdgpu_device *adev)
>    "dither",
>    amdgpu_dither_enum_list, sz);
>
> + sz = ARRAY_SIZE(amdgpu_user_pixenc_list);
> + adev->mode_info.pixel_encoding_property =
> + drm_property_create_enum(adev_to_drm(adev), 0,
> + "pixel encoding",
> + amdgpu_user_pixenc_list, sz);
> + if (!adev->mode_info.pixel_encoding_property)
> + return -ENOMEM;
> +
>  return 0;
>  }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> index 9d19940f73c8..ee1ad49fa123 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.h
> @@ -49,4 +49,7 @@ amdgpu_lookup_format_info(u32 format, uint64_t modifier);
>  int amdgpu_display_suspend_helper(struct amdgpu_device *adev);
>  int amdgpu_display_resume_helper(struct amdgpu_device *adev);
>
> +bool amdgpu_user_pixenc_from_name(unsigned int *user_pixenc,
> +  const char *pixenc_name);
> +
>  #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index 5e3faefc5510..b984e66a5d75 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -326,6 +326,8 @@ struct amdgpu_mode_info {
>  struct drm_property *audio_property;
>  /* FMT dithering */
>  struct drm_property *dither_property;
> + /* User HDMI pixel encoding override */
> + struct drm_property *pixel_encoding_property;
>  /* hardcoded DFP edid from BIOS */
>  const struct drm_edid *bios_hardcoded_edid;
>
> 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 bbfc47f6595f..2c03e0733178 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6069,6 +6069,115 @@ static bool adjust_colour_depth_from_display_info(
>  return false;
>  }
>
> +/* convert an pixel encoding property value to a dc_pixel_encoding */
> +static bool drm_prop_to_dc_pixel_encoding(
> + enum dc_pixel_encoding *dc_pixenc,
> + unsigned int propval)
> +{
> + bool ret = false;
> +
> + switch (propval) {
> + case 0:
> + *dc_pixenc = PIXEL_ENCODING_UNDEFINED;
> + ret = true;
> + break;
> + case DRM_COLOR_FORMAT_RGB444:
> + *dc_pixenc = PIXEL_ENCODING_RGB;
> + ret = true;
> + break;
> + case DRM_COLOR_FORMAT_YCBCR444:
> + *dc_pixenc = PIXEL_ENCODING_YCBCR444;
> + ret = true;
> + break;
> + case DRM_COLOR_FORMAT_YCBCR420:
> + *dc_pixenc = PIXEL_ENCODING_YCBCR420;
> + ret = true;
> + break;
> + default:
> + break;
> + }
> + return ret;
> +}
> +
> +/* convert an dc_pixel_encoding to a pixel encoding property value */
> +static unsigned int dc_pixel_encoding_to_drm_prop(
> + enum dc_pixel_encoding pixel_encoding)
> +{
> + unsigned int propval = 0;
> +
> + switch (pixel_encoding) {
> + case PIXEL_ENCODING_RGB:
> + propval = DRM_COLOR_FORMAT_RGB444;
> + break;
> + case PIXEL_ENCODING_YCBCR444:
> + propval = DRM_COLOR_FORMAT_YCBCR444;
> + break;
> + case PIXEL_ENCODING_YCBCR420:
> + propval = DRM_COLOR_FORMAT_YCBCR420;
> + break;
> + default:
> + break;
> + }
> + return propval;
> +}
> +
> +/*
> + * Tries to read 'pixel_encoding' from the pixel_encoding DRM property on
> + * 'state'. Returns true if a supported, acceptable, non-undefined value is
> + * found; false otherwise. Only modifies 'pixel_encoding' if returning true.
> + */
> +bool get_connector_state_pixel_encoding(
> + enum dc_pixel_encoding *pixel_encoding,
> + const struct drm_connector_state *state,
> + const struct drm_display_info *info,
> + const struct drm_display_mode *mode_in)
> +{
> + bool ret = false;
> + struct dm_connector_state *dm_state;
> +
> + dm_state = to_dm_connector_state(state);
> + if (!dm_state)
> + return false;
> +
> + /* check encoding is supported */
> + switch (dm_state->pixel_encoding) {
> + case PIXEL_ENCODING_RGB:
> + ret = (info->color_formats & DRM_COLOR_FORMAT_RGB444);
> + break;
> + case PIXEL_ENCODING_YCBCR444:
> + ret = (info->color_formats & DRM_COLOR_FORMAT_YCBCR444);
> + break;
> + case PIXEL_ENCODING_YCBCR420:
> + ret = drm_mode_is_420(info, mode_in);
> + break;
> + default:
> + break;
> + }
> +
> + if (ret)
> + *pixel_encoding = dm_state->pixel_encoding;
> +
> + return ret;
> +}
> +
> +/*
> + * Writes 'pixel_encoding' to the pixel_encoding DRM property on 'state', if
> + * the enum value is valid and supported; otherwise writes
> + * PIXEL_ENCODING_UNDEFINED which corresponds to the "auto" property state.
> + */
> +void set_connector_state_pixel_encoding(
> + const struct drm_connector_state *state,
> + enum dc_pixel_encoding pixel_encoding)
> +{
> + struct dm_connector_state *dm_state;
> +
> + dm_state = to_dm_connector_state(state);
> + if (!dm_state)
> + return;
> +
> + dm_state->pixel_encoding = pixel_encoding;
> +}
> +
>  static void fill_stream_properties_from_drm_display_mode(
>  struct dc_stream_state *stream,
>  const struct drm_display_mode *mode_in,
> @@ -6093,19 +6202,23 @@ static void fill_stream_properties_from_drm_display_mode(
>  timing_out->h_border_right = 0;
>  timing_out->v_border_top = 0;
>  timing_out->v_border_bottom = 0;
> - /* TODO: un-hardcode */
> - if (drm_mode_is_420_only(info, mode_in)
> +
> + if (!get_connector_state_pixel_encoding(&timing_out->pixel_encoding,
> + connector_state, info, mode_in)) {
> + /* auto-select a pixel encoding */
> + if (drm_mode_is_420_only(info, mode_in)
>  && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
> - timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
> - else if (drm_mode_is_420_also(info, mode_in)
> + timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
> + else if (drm_mode_is_420_also(info, mode_in)
>  && aconnector
>  && aconnector->force_yuv420_output)
> - timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
> - else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
> - && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
> - timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
> - else
> - timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
> + timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR420;
> + else if ((connector->display_info.color_formats & DRM_COLOR_FORMAT_YCBCR444)
> + && stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
> + timing_out->pixel_encoding = PIXEL_ENCODING_YCBCR444;
> + else
> + timing_out->pixel_encoding = PIXEL_ENCODING_RGB;
> + }
>
>  timing_out->timing_3d_format = TIMING_3D_FORMAT_NONE;
>  timing_out->display_color_depth = convert_color_depth_from_display_info(
> @@ -6169,6 +6282,9 @@ static void fill_stream_properties_from_drm_display_mode(
>  }
>  }
>
> + /* write back final choice of pixel encoding */
> + set_connector_state_pixel_encoding(connector_state, timing_out->pixel_encoding);
> +
>  stream->output_color_space = get_output_color_space(timing_out, connector_state);
>  stream->content_type = get_output_content_type(connector_state);
>  }
> @@ -6875,6 +6991,9 @@ int amdgpu_dm_connector_atomic_set_property(struct drm_connector *connector,
>  } else if (property == adev->mode_info.underscan_property) {
>  dm_new_state->underscan_enable = val;
>  ret = 0;
> + } else if (property == adev->mode_info.pixel_encoding_property) {
> + if (drm_prop_to_dc_pixel_encoding(&dm_new_state->pixel_encoding, val))
> + ret = 0;
>  }
>
>  return ret;
> @@ -6917,6 +7036,9 @@ int amdgpu_dm_connector_atomic_get_property(struct drm_connector *connector,
>  } else if (property == adev->mode_info.underscan_property) {
>  *val = dm_state->underscan_enable;
>  ret = 0;
> + } else if (property == adev->mode_info.pixel_encoding_property) {
> + *val = dc_pixel_encoding_to_drm_prop(dm_state->pixel_encoding);
> + ret = 0;
>  }
>
>  return ret;
> @@ -7088,6 +7210,20 @@ void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
>  state->abm_level = amdgpu_dm_abm_level;
>  }
>
> + switch (connector->cmdline_mode.pixel_encoding) {
> + case DRM_COLOR_FORMAT_RGB444:
> + state->pixel_encoding = PIXEL_ENCODING_RGB;
> + break;
> + case DRM_COLOR_FORMAT_YCBCR444:
> + state->pixel_encoding = PIXEL_ENCODING_YCBCR444;
> + break;
> + case DRM_COLOR_FORMAT_YCBCR420:
> + state->pixel_encoding = PIXEL_ENCODING_YCBCR420;
> + break;
> + default:
> + break;
> + }
> +
>  __drm_atomic_helper_connector_reset(connector, &state->base);
>  }
>  }
> @@ -7114,6 +7250,7 @@ amdgpu_dm_connector_atomic_duplicate_state(struct drm_connector *connector)
>  new_state->underscan_vborder = state->underscan_vborder;
>  new_state->vcpi_slots = state->vcpi_slots;
>  new_state->pbn = state->pbn;
> + new_state->pixel_encoding = state->pixel_encoding;
>  return &new_state->base;
>  }
>
> @@ -8175,6 +8312,12 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
>
>  if (adev->dm.hdcp_workqueue)
>  drm_connector_attach_content_protection_property(&aconnector->base, true);
> +
> + if (adev->mode_info.pixel_encoding_property) {
> + drm_object_attach_property(&aconnector->base.base,
> + adev->mode_info.pixel_encoding_property, 0);
> + DRM_DEBUG_DRIVER("amdgpu: attached pixel encoding drm property");
> + }
>  }
>  }
>
> @@ -9377,6 +9520,38 @@ static void amdgpu_dm_commit_audio(struct drm_device *dev,
>  }
>  }
>
> +static void update_stream_for_pixel_encoding(
> + struct dc_stream_update *stream_update,
> + struct drm_connector *connector,
> + struct dm_crtc_state *dm_old_crtc_state,
> + struct dm_crtc_state *dm_new_crtc_state,
> + struct dm_connector_state *dm_new_con_state)
> +{
> + struct amdgpu_dm_connector *aconnector =
> + to_amdgpu_dm_connector(connector);
> + struct dc_stream_state *new_stream = NULL;
> +
> + if (aconnector)
> + new_stream = create_validate_stream_for_sink(
> + aconnector,
> + &dm_new_crtc_state->base.mode,
> + dm_new_con_state,
> + dm_old_crtc_state->stream);
> + if (new_stream) {
> + dm_new_crtc_state->stream->timing =
> + new_stream->timing;
> + stream_update->timing_for_pixel_encoding =
> + &dm_new_crtc_state->stream->timing;
> +
> + dm_new_crtc_state->stream->output_color_space =
> + new_stream->output_color_space;
> + stream_update->output_color_space =
> + &dm_new_crtc_state->stream->output_color_space;
> +
> + dc_stream_release(new_stream);
> + }
> +}
> +
>  /*
>  * amdgpu_dm_crtc_copy_transient_flags - copy mirrored flags from DRM to DC
>  * @crtc_state: the DRM CRTC state
> @@ -9850,7 +10025,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>  struct dc_stream_update stream_update;
>  struct dc_info_packet hdr_packet;
>  struct dc_stream_status *status = NULL;
> - bool abm_changed, hdr_changed, scaling_changed;
> + bool abm_changed, hdr_changed, scaling_changed, pixenc_changed;
>
>  memset(&stream_update, 0, sizeof(stream_update));
>
> @@ -9875,7 +10050,10 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>  hdr_changed =
>  !drm_connector_atomic_hdr_metadata_equal(old_con_state, new_con_state);
>
> - if (!scaling_changed && !abm_changed && !hdr_changed)
> + pixenc_changed = dm_new_con_state->pixel_encoding !=
> +  dm_old_con_state->pixel_encoding;
> +
> + if (!scaling_changed && !abm_changed && !hdr_changed && !pixenc_changed)
>  continue;
>
>  stream_update.stream = dm_new_crtc_state->stream;
> @@ -9898,6 +10076,13 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>  stream_update.hdr_static_metadata = &hdr_packet;
>  }
>
> + if (pixenc_changed) {
> + update_stream_for_pixel_encoding(&stream_update,
> + connector,
> + dm_old_crtc_state, dm_new_crtc_state,
> + dm_new_con_state);
> + }
> +
>  status = dc_stream_get_status(dm_new_crtc_state->stream);
>
>  if (WARN_ON(!status))
> @@ -11403,6 +11588,12 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>  if (dm_old_con_state->abm_level != dm_new_con_state->abm_level ||
>      dm_old_con_state->scaling != dm_new_con_state->scaling)
>  new_crtc_state->connectors_changed = true;
> +
> + if (dm_old_con_state->pixel_encoding !=
> +    dm_new_con_state->pixel_encoding) {
> + new_crtc_state->connectors_changed = true;
> + new_crtc_state->mode_changed = true;
> + }
>  }
>
>  if (dc_resource_is_dsc_encoding_supported(dc)) {
> 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 25e95775c45c..132944272c18 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -918,6 +918,7 @@ struct dm_connector_state {
>  uint8_t abm_level;
>  int vcpi_slots;
>  uint64_t pbn;
> + enum dc_pixel_encoding pixel_encoding;
>  };
>
>  #define to_dm_connector_state(x)\
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
> index 5a12fc75f97f..4ac004536707 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> @@ -2765,6 +2765,11 @@ static enum surface_update_type check_update_surfaces_for_stream(
>  if (stream_update->output_csc_transform || stream_update->output_color_space)
>  su_flags->bits.out_csc = 1;
>
> + if (stream_update->timing_for_pixel_encoding) {
> + su_flags->bits.pixel_encoding = 1;
> + elevate_update_type(&overall_type, UPDATE_TYPE_FULL);
> + }
> +
>  /* Output transfer function changes do not require bandwidth recalculation,
>    * so don't trigger a full update
>    */
> @@ -3112,6 +3117,8 @@ static void copy_stream_update_to_stream(struct dc *dc,
>  stream->scaler_sharpener_update = *update->scaler_sharpener_update;
>  if (update->sharpening_required)
>  stream->sharpening_required = *update->sharpening_required;
> + if (update->timing_for_pixel_encoding)
> + stream->timing = *update->timing_for_pixel_encoding;
>  }
>
>  static void backup_planes_and_stream_state(
> @@ -3361,6 +3368,7 @@ static void commit_planes_do_stream_update(struct dc *dc,
>  stream_update->vsc_infopacket ||
>  stream_update->vsp_infopacket ||
>  stream_update->hfvsif_infopacket ||
> + stream_update->timing_for_pixel_encoding ||
>  stream_update->adaptive_sync_infopacket ||
>  stream_update->vtem_infopacket) {
>  resource_build_info_frame(pipe_ctx);
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h
> index 413970588a26..de7f02f330d8 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h
> @@ -144,6 +144,7 @@ union stream_update_flags {
>  uint32_t fams_changed : 1;
>  uint32_t scaler_sharpener : 1;
>  uint32_t sharpening_required : 1;
> + uint32_t pixel_encoding:1;
>  } bits;
>
>  uint32_t raw;
> @@ -350,6 +351,7 @@ struct dc_stream_update {
>  struct dc_mst_stream_bw_update *mst_bw_update;
>  struct dc_transfer_func *func_shaper;
>  struct dc_3dlut *lut3d_func;
> + struct dc_crtc_timing *timing_for_pixel_encoding;
>
>  struct test_pattern *pending_test_pattern;
>  struct dc_crtc_timing_adjust *crtc_timing_adjust;
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 1a0890083aee..b5ce8f93b672 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2161,6 +2161,32 @@ static int drm_mode_parse_tv_mode(const char *delim,
>  return 0;
>  }
>
> +static int drm_mode_parse_pixel_encoding(const char *delim,
> +  struct drm_cmdline_mode *mode)
> +{
> + const char *value;
> +
> + if (*delim != '=')
> + return -EINVAL;
> +
> + value = delim + 1;
> + delim = strchr(value, ',');
> + if (!delim)
> + delim = value + strlen(value);
> +
> + if (!strncmp(value, "auto", delim - value))
> + mode->pixel_encoding = 0;
> + else if (!strncmp(value, "rgb", delim - value))
> + mode->pixel_encoding = DRM_COLOR_FORMAT_RGB444;
> + else if (!strncmp(value, "ycbcr444", delim - value))
> + mode->pixel_encoding = DRM_COLOR_FORMAT_YCBCR444;
> + else if (!strncmp(value, "ycbcr420", delim - value))
> + mode->pixel_encoding = DRM_COLOR_FORMAT_YCBCR420;
> + else
> + return -EINVAL;
> +
> + return 0;
> +}
>  static int drm_mode_parse_cmdline_options(const char *str,
>    bool freestanding,
>    const struct drm_connector *connector,
> @@ -2233,6 +2259,9 @@ static int drm_mode_parse_cmdline_options(const char *str,
>  } else if (!strncmp(option, "tv_mode", delim - option)) {
>  if (drm_mode_parse_tv_mode(delim, mode))
>  return -EINVAL;
> + } else if (!strncmp(option, "pixel_encoding", delim - option)) {
> + if (drm_mode_parse_pixel_encoding(delim, mode))
> + return -EINVAL;
>  } else {
>  return -EINVAL;
>  }
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index c754651044d4..89c0f42873df 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1666,6 +1666,13 @@ struct drm_cmdline_mode {
>    * Did the mode have a preferred TV mode?
>    */
>  bool tv_mode_specified;
> +
> + /**
> +  * @pixel_encoding:
> +  *
> +  * Initial pixel encoding.
> +  */
> + unsigned int pixel_encoding;
>  };
>
>  /*
> --
> 2.43.0

Re: [PATCH] drm/amdgpu add "pixel_encoding"

[Xaver Hugl]

Am Di., 5. Nov. 2024 um 16:16 Uhr schrieb Matias N. Goldberg
<dark_sylinc@yahoo.com.ar>:
>
> > That's not a problem, incompatible options can just be rejected in atomic tests.
>
> I was thinking from a user perspective. It'd be easier for user-space config apps to present only the valid options, rather than offering all possible combinations only to reject them after the user selected an invalid config (or perform hundreds of atomic tests every time we need to show a config dialog).
No, it's way easier to deal with 3 separate properties and have the
very simple logic (for which no atomic tests have to be made) in the
UI to prevent the user from setting obviously invalid combinations.
Having to split up an enum with tens or hundreds of values into
options the user can understand and that we can properly use is a lot
more effort.

> > opRGB is not supported by any screen I've ever looked at, so it doesn't surprise me that you'd get glitches.
>
> I don't know if it was just a fallback, but my screen *does* work when using opRGB with rgb encoding (it glitches when using yuv encoding).
That's most likely not because of the value of "opRGB", but just what
it does with all unsupported colorspace values.