* [PATCH 1/2] drm/amd/display: Hook up 'content type' property for HDMI @ 2023-01-17 21:26 Joshua Ashton 2023-01-17 21:26 ` [PATCH 2/2] drm/amd/display: Remove unused display_content_support Joshua Ashton 2023-01-19 18:14 ` [PATCH 1/2] drm/amd/display: Hook up 'content type' property for HDMI Melissa Wen 0 siblings, 2 replies; 8+ messages in thread From: Joshua Ashton @ 2023-01-17 21:26 UTC (permalink / raw) To: amd-gfx; +Cc: Joshua Ashton Implements the 'content type' property for HDMI connectors. Verified by checking the avi infoframe on a connected TV. This also simplifies a lot of the code in that area as well, there were a lot of temp variables doing very little and unnecessary logic that was quite confusing. It is not necessary to check for support in the EDID before sending a 'content type' value in the avi infoframe also. Signed-off-by: Joshua Ashton <joshua@froggi.es> --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 24 +++++++ .../gpu/drm/amd/display/dc/core/dc_resource.c | 69 ++++++------------- drivers/gpu/drm/amd/display/dc/dc_stream.h | 1 + 3 files changed, 46 insertions(+), 48 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 9547037857b6..999965fe3de9 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5216,6 +5216,24 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing) return color_space; } +static enum display_content_type +get_output_content_type(const struct drm_connector_state *connector_state) +{ + switch (connector_state->content_type) { + default: + case DRM_MODE_CONTENT_TYPE_NO_DATA: + return DISPLAY_CONTENT_TYPE_NO_DATA; + case DRM_MODE_CONTENT_TYPE_GRAPHICS: + return DISPLAY_CONTENT_TYPE_GRAPHICS; + case DRM_MODE_CONTENT_TYPE_PHOTO: + return DISPLAY_CONTENT_TYPE_PHOTO; + case DRM_MODE_CONTENT_TYPE_CINEMA: + return DISPLAY_CONTENT_TYPE_CINEMA; + case DRM_MODE_CONTENT_TYPE_GAME: + return DISPLAY_CONTENT_TYPE_GAME; + } +} + static bool adjust_colour_depth_from_display_info( struct dc_crtc_timing *timing_out, const struct drm_display_info *info) @@ -5349,6 +5367,7 @@ static void fill_stream_properties_from_drm_display_mode( } stream->output_color_space = get_output_color_space(timing_out); + stream->content_type = get_output_content_type(connector_state); } static void fill_audio_info(struct audio_info *audio_info, @@ -7123,6 +7142,11 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, adev->mode_info.abm_level_property, 0); } + if (connector_type == DRM_MODE_CONNECTOR_HDMIA) { + /* Content Type is currently only implemented for HDMI. */ + drm_connector_attach_content_type_property(&aconnector->base); + } + if (connector_type == DRM_MODE_CONNECTOR_HDMIA || connector_type == DRM_MODE_CONNECTOR_DisplayPort || connector_type == DRM_MODE_CONNECTOR_eDP) { diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c index a5b5f8592c1b..39ceccdb6586 100644 --- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c +++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c @@ -2944,14 +2944,9 @@ static void set_avi_info_frame( uint32_t pixel_encoding = 0; enum scanning_type scan_type = SCANNING_TYPE_NODATA; enum dc_aspect_ratio aspect = ASPECT_RATIO_NO_DATA; - bool itc = false; - uint8_t itc_value = 0; - uint8_t cn0_cn1 = 0; - unsigned int cn0_cn1_value = 0; uint8_t *check_sum = NULL; uint8_t byte_index = 0; union hdmi_info_packet hdmi_info; - union display_content_support support = {0}; unsigned int vic = pipe_ctx->stream->timing.vic; unsigned int rid = pipe_ctx->stream->timing.rid; unsigned int fr_ind = pipe_ctx->stream->timing.fr_index; @@ -3055,49 +3050,27 @@ static void set_avi_info_frame( /* Active Format Aspect ratio - same as Picture Aspect Ratio. */ hdmi_info.bits.R0_R3 = ACTIVE_FORMAT_ASPECT_RATIO_SAME_AS_PICTURE; - /* TODO: un-hardcode cn0_cn1 and itc */ - - cn0_cn1 = 0; - cn0_cn1_value = 0; - - itc = true; - itc_value = 1; - - support = stream->content_support; - - if (itc) { - if (!support.bits.valid_content_type) { - cn0_cn1_value = 0; - } else { - if (cn0_cn1 == DISPLAY_CONTENT_TYPE_GRAPHICS) { - if (support.bits.graphics_content == 1) { - cn0_cn1_value = 0; - } - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_PHOTO) { - if (support.bits.photo_content == 1) { - cn0_cn1_value = 1; - } else { - cn0_cn1_value = 0; - itc_value = 0; - } - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_CINEMA) { - if (support.bits.cinema_content == 1) { - cn0_cn1_value = 2; - } else { - cn0_cn1_value = 0; - itc_value = 0; - } - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_GAME) { - if (support.bits.game_content == 1) { - cn0_cn1_value = 3; - } else { - cn0_cn1_value = 0; - itc_value = 0; - } - } - } - hdmi_info.bits.CN0_CN1 = cn0_cn1_value; - hdmi_info.bits.ITC = itc_value; + switch (stream->content_type) { + case DISPLAY_CONTENT_TYPE_NO_DATA: + hdmi_info.bits.CN0_CN1 = 0; + hdmi_info.bits.ITC = 0; + break; + case DISPLAY_CONTENT_TYPE_GRAPHICS: + hdmi_info.bits.CN0_CN1 = 0; + hdmi_info.bits.ITC = 1; + break; + case DISPLAY_CONTENT_TYPE_PHOTO: + hdmi_info.bits.CN0_CN1 = 1; + hdmi_info.bits.ITC = 1; + break; + case DISPLAY_CONTENT_TYPE_CINEMA: + hdmi_info.bits.CN0_CN1 = 2; + hdmi_info.bits.ITC = 1; + break; + case DISPLAY_CONTENT_TYPE_GAME: + hdmi_info.bits.CN0_CN1 = 3; + hdmi_info.bits.ITC = 1; + break; } if (stream->qs_bit == 1) { diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h index ef33d7d8a2bf..51dc30706e43 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h @@ -205,6 +205,7 @@ struct dc_stream_state { struct dc_csc_transform csc_color_matrix; enum dc_color_space output_color_space; + enum display_content_type content_type; enum dc_dither_option dither_option; enum view_3d_format view_format; -- 2.39.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] drm/amd/display: Remove unused display_content_support 2023-01-17 21:26 [PATCH 1/2] drm/amd/display: Hook up 'content type' property for HDMI Joshua Ashton @ 2023-01-17 21:26 ` Joshua Ashton 2023-01-19 18:21 ` Melissa Wen 2023-01-19 18:14 ` [PATCH 1/2] drm/amd/display: Hook up 'content type' property for HDMI Melissa Wen 1 sibling, 1 reply; 8+ messages in thread From: Joshua Ashton @ 2023-01-17 21:26 UTC (permalink / raw) To: amd-gfx; +Cc: Joshua Ashton This was never filled in and thus never truly used. Checking the EDID for content_type support is not required for sending the avi infoframe packet. Signed-off-by: Joshua Ashton <joshua@froggi.es> --- drivers/gpu/drm/amd/display/dc/dc_stream.h | 1 - drivers/gpu/drm/amd/display/dc/dc_types.h | 14 -------------- 2 files changed, 15 deletions(-) diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h index 51dc30706e43..a499c0952ea0 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h @@ -182,7 +182,6 @@ struct dc_stream_state { */ struct link_encoder *link_enc; struct dc_panel_patch sink_patches; - union display_content_support content_support; struct dc_crtc_timing timing; struct dc_crtc_timing_adjust adjust; struct dc_info_packet vrr_infopacket; diff --git a/drivers/gpu/drm/amd/display/dc/dc_types.h b/drivers/gpu/drm/amd/display/dc/dc_types.h index c73a655bd687..862af36027e7 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_types.h +++ b/drivers/gpu/drm/amd/display/dc/dc_types.h @@ -175,18 +175,6 @@ struct dc_edid { #define AUDIO_INFO_DISPLAY_NAME_SIZE_IN_CHARS 20 -union display_content_support { - unsigned int raw; - struct { - unsigned int valid_content_type :1; - unsigned int game_content :1; - unsigned int cinema_content :1; - unsigned int photo_content :1; - unsigned int graphics_content :1; - unsigned int reserved :27; - } bits; -}; - struct dc_panel_patch { unsigned int dppowerup_delay; unsigned int extra_t12_ms; @@ -219,8 +207,6 @@ struct dc_edid_caps { uint32_t audio_latency; uint32_t video_latency; - union display_content_support content_support; - uint8_t qs_bit; uint8_t qy_bit; -- 2.39.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] drm/amd/display: Remove unused display_content_support 2023-01-17 21:26 ` [PATCH 2/2] drm/amd/display: Remove unused display_content_support Joshua Ashton @ 2023-01-19 18:21 ` Melissa Wen 0 siblings, 0 replies; 8+ messages in thread From: Melissa Wen @ 2023-01-19 18:21 UTC (permalink / raw) To: Joshua Ashton; +Cc: sunpeng.li, Rodrigo.Siqueira, harry.wentland, amd-gfx [-- Attachment #1: Type: text/plain, Size: 2279 bytes --] On 01/17, Joshua Ashton wrote: > This was never filled in and thus never truly used. > > Checking the EDID for content_type support is not required for sending > the avi infoframe packet. > > Signed-off-by: Joshua Ashton <joshua@froggi.es> > --- > drivers/gpu/drm/amd/display/dc/dc_stream.h | 1 - > drivers/gpu/drm/amd/display/dc/dc_types.h | 14 -------------- > 2 files changed, 15 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h > index 51dc30706e43..a499c0952ea0 100644 > --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h > +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h > @@ -182,7 +182,6 @@ struct dc_stream_state { > */ > struct link_encoder *link_enc; > struct dc_panel_patch sink_patches; > - union display_content_support content_support; > struct dc_crtc_timing timing; > struct dc_crtc_timing_adjust adjust; > struct dc_info_packet vrr_infopacket; > diff --git a/drivers/gpu/drm/amd/display/dc/dc_types.h b/drivers/gpu/drm/amd/display/dc/dc_types.h > index c73a655bd687..862af36027e7 100644 > --- a/drivers/gpu/drm/amd/display/dc/dc_types.h > +++ b/drivers/gpu/drm/amd/display/dc/dc_types.h > @@ -175,18 +175,6 @@ struct dc_edid { > > #define AUDIO_INFO_DISPLAY_NAME_SIZE_IN_CHARS 20 > > -union display_content_support { > - unsigned int raw; > - struct { > - unsigned int valid_content_type :1; > - unsigned int game_content :1; > - unsigned int cinema_content :1; > - unsigned int photo_content :1; > - unsigned int graphics_content :1; > - unsigned int reserved :27; > - } bits; > -}; > - > struct dc_panel_patch { > unsigned int dppowerup_delay; > unsigned int extra_t12_ms; > @@ -219,8 +207,6 @@ struct dc_edid_caps { > uint32_t audio_latency; > uint32_t video_latency; > > - union display_content_support content_support; > - LGTM. It depends on the previous patch, but if the first patch are fine, they are both: Reviewed-by: Melissa Wen <mwen@igalia.com> CC'ing AMD DC folks again since I don't know if it's in use by other platforms. But it'd be nice to get rid of this or have a comment/explanation, if not. > uint8_t qs_bit; > uint8_t qy_bit; > > -- > 2.39.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/amd/display: Hook up 'content type' property for HDMI 2023-01-17 21:26 [PATCH 1/2] drm/amd/display: Hook up 'content type' property for HDMI Joshua Ashton 2023-01-17 21:26 ` [PATCH 2/2] drm/amd/display: Remove unused display_content_support Joshua Ashton @ 2023-01-19 18:14 ` Melissa Wen 2023-01-19 19:29 ` Harry Wentland 2023-01-19 22:20 ` Joshua Ashton 1 sibling, 2 replies; 8+ messages in thread From: Melissa Wen @ 2023-01-19 18:14 UTC (permalink / raw) To: Joshua Ashton; +Cc: sunpeng.li, Rodrigo.Siqueira, harry.wentland, amd-gfx [-- Attachment #1: Type: text/plain, Size: 6629 bytes --] On 01/17, Joshua Ashton wrote: > Implements the 'content type' property for HDMI connectors. > Verified by checking the avi infoframe on a connected TV. > > This also simplifies a lot of the code in that area as well, there were > a lot of temp variables doing very little and unnecessary logic > that was quite confusing. > > It is not necessary to check for support in the EDID before sending a > 'content type' value in the avi infoframe also. > > Signed-off-by: Joshua Ashton <joshua@froggi.es> > --- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 24 +++++++ > .../gpu/drm/amd/display/dc/core/dc_resource.c | 69 ++++++------------- > drivers/gpu/drm/amd/display/dc/dc_stream.h | 1 + > 3 files changed, 46 insertions(+), 48 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index 9547037857b6..999965fe3de9 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -5216,6 +5216,24 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing) > return color_space; > } > > +static enum display_content_type > +get_output_content_type(const struct drm_connector_state *connector_state) > +{ > + switch (connector_state->content_type) { > + default: > + case DRM_MODE_CONTENT_TYPE_NO_DATA: > + return DISPLAY_CONTENT_TYPE_NO_DATA; > + case DRM_MODE_CONTENT_TYPE_GRAPHICS: > + return DISPLAY_CONTENT_TYPE_GRAPHICS; > + case DRM_MODE_CONTENT_TYPE_PHOTO: > + return DISPLAY_CONTENT_TYPE_PHOTO; > + case DRM_MODE_CONTENT_TYPE_CINEMA: > + return DISPLAY_CONTENT_TYPE_CINEMA; > + case DRM_MODE_CONTENT_TYPE_GAME: > + return DISPLAY_CONTENT_TYPE_GAME; > + } > +} > + > static bool adjust_colour_depth_from_display_info( > struct dc_crtc_timing *timing_out, > const struct drm_display_info *info) > @@ -5349,6 +5367,7 @@ static void fill_stream_properties_from_drm_display_mode( > } > > stream->output_color_space = get_output_color_space(timing_out); > + stream->content_type = get_output_content_type(connector_state); > } > > static void fill_audio_info(struct audio_info *audio_info, > @@ -7123,6 +7142,11 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, > adev->mode_info.abm_level_property, 0); > } > > + if (connector_type == DRM_MODE_CONNECTOR_HDMIA) { > + /* Content Type is currently only implemented for HDMI. */ > + drm_connector_attach_content_type_property(&aconnector->base); > + } > + > if (connector_type == DRM_MODE_CONNECTOR_HDMIA || > connector_type == DRM_MODE_CONNECTOR_DisplayPort || > connector_type == DRM_MODE_CONNECTOR_eDP) { > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c > index a5b5f8592c1b..39ceccdb6586 100644 > --- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c > +++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c > @@ -2944,14 +2944,9 @@ static void set_avi_info_frame( > uint32_t pixel_encoding = 0; > enum scanning_type scan_type = SCANNING_TYPE_NODATA; > enum dc_aspect_ratio aspect = ASPECT_RATIO_NO_DATA; > - bool itc = false; > - uint8_t itc_value = 0; > - uint8_t cn0_cn1 = 0; > - unsigned int cn0_cn1_value = 0; > uint8_t *check_sum = NULL; > uint8_t byte_index = 0; > union hdmi_info_packet hdmi_info; > - union display_content_support support = {0}; > unsigned int vic = pipe_ctx->stream->timing.vic; > unsigned int rid = pipe_ctx->stream->timing.rid; > unsigned int fr_ind = pipe_ctx->stream->timing.fr_index; > @@ -3055,49 +3050,27 @@ static void set_avi_info_frame( > /* Active Format Aspect ratio - same as Picture Aspect Ratio. */ > hdmi_info.bits.R0_R3 = ACTIVE_FORMAT_ASPECT_RATIO_SAME_AS_PICTURE; > > - /* TODO: un-hardcode cn0_cn1 and itc */ > - > - cn0_cn1 = 0; > - cn0_cn1_value = 0; > - > - itc = true; > - itc_value = 1; > - > - support = stream->content_support; > - > - if (itc) { > - if (!support.bits.valid_content_type) { > - cn0_cn1_value = 0; > - } else { > - if (cn0_cn1 == DISPLAY_CONTENT_TYPE_GRAPHICS) { > - if (support.bits.graphics_content == 1) { > - cn0_cn1_value = 0; > - } > - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_PHOTO) { > - if (support.bits.photo_content == 1) { > - cn0_cn1_value = 1; > - } else { > - cn0_cn1_value = 0; > - itc_value = 0; > - } > - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_CINEMA) { > - if (support.bits.cinema_content == 1) { > - cn0_cn1_value = 2; > - } else { > - cn0_cn1_value = 0; > - itc_value = 0; > - } > - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_GAME) { > - if (support.bits.game_content == 1) { > - cn0_cn1_value = 3; > - } else { > - cn0_cn1_value = 0; > - itc_value = 0; > - } > - } > - } > - hdmi_info.bits.CN0_CN1 = cn0_cn1_value; > - hdmi_info.bits.ITC = itc_value; > + switch (stream->content_type) { > + case DISPLAY_CONTENT_TYPE_NO_DATA: > + hdmi_info.bits.CN0_CN1 = 0; > + hdmi_info.bits.ITC = 0; Hmm.. why is ITC value equal zero here ^, instead of the same hardcoded `itc_value = 1`? Does it come from a DRM default value? Other than that, changes seem fine to me and it's nice to see the code wired to the DRM and actually used. CC'ing other AMD DC folks since I don't know if these changes affect other platforms. Can you guys verify it? > + break; > + case DISPLAY_CONTENT_TYPE_GRAPHICS: > + hdmi_info.bits.CN0_CN1 = 0; > + hdmi_info.bits.ITC = 1; > + break; > + case DISPLAY_CONTENT_TYPE_PHOTO: > + hdmi_info.bits.CN0_CN1 = 1; > + hdmi_info.bits.ITC = 1; > + break; > + case DISPLAY_CONTENT_TYPE_CINEMA: > + hdmi_info.bits.CN0_CN1 = 2; > + hdmi_info.bits.ITC = 1; > + break; > + case DISPLAY_CONTENT_TYPE_GAME: > + hdmi_info.bits.CN0_CN1 = 3; > + hdmi_info.bits.ITC = 1; > + break; > } > > if (stream->qs_bit == 1) { > diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h > index ef33d7d8a2bf..51dc30706e43 100644 > --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h > +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h > @@ -205,6 +205,7 @@ struct dc_stream_state { > struct dc_csc_transform csc_color_matrix; > > enum dc_color_space output_color_space; > + enum display_content_type content_type; > enum dc_dither_option dither_option; > > enum view_3d_format view_format; > -- > 2.39.0 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/amd/display: Hook up 'content type' property for HDMI 2023-01-19 18:14 ` [PATCH 1/2] drm/amd/display: Hook up 'content type' property for HDMI Melissa Wen @ 2023-01-19 19:29 ` Harry Wentland 2023-01-19 22:20 ` Joshua Ashton 1 sibling, 0 replies; 8+ messages in thread From: Harry Wentland @ 2023-01-19 19:29 UTC (permalink / raw) To: Melissa Wen, Joshua Ashton; +Cc: sunpeng.li, Rodrigo.Siqueira, amd-gfx On 1/19/23 13:14, Melissa Wen wrote: > On 01/17, Joshua Ashton wrote: >> Implements the 'content type' property for HDMI connectors. >> Verified by checking the avi infoframe on a connected TV. >> >> This also simplifies a lot of the code in that area as well, there were >> a lot of temp variables doing very little and unnecessary logic >> that was quite confusing. >> >> It is not necessary to check for support in the EDID before sending a >> 'content type' value in the avi infoframe also. >> >> Signed-off-by: Joshua Ashton <joshua@froggi.es> >> --- >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 24 +++++++ >> .../gpu/drm/amd/display/dc/core/dc_resource.c | 69 ++++++------------- >> drivers/gpu/drm/amd/display/dc/dc_stream.h | 1 + >> 3 files changed, 46 insertions(+), 48 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> index 9547037857b6..999965fe3de9 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> @@ -5216,6 +5216,24 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing) >> return color_space; >> } >> >> +static enum display_content_type >> +get_output_content_type(const struct drm_connector_state *connector_state) >> +{ >> + switch (connector_state->content_type) { >> + default: >> + case DRM_MODE_CONTENT_TYPE_NO_DATA: >> + return DISPLAY_CONTENT_TYPE_NO_DATA; >> + case DRM_MODE_CONTENT_TYPE_GRAPHICS: >> + return DISPLAY_CONTENT_TYPE_GRAPHICS; >> + case DRM_MODE_CONTENT_TYPE_PHOTO: >> + return DISPLAY_CONTENT_TYPE_PHOTO; >> + case DRM_MODE_CONTENT_TYPE_CINEMA: >> + return DISPLAY_CONTENT_TYPE_CINEMA; >> + case DRM_MODE_CONTENT_TYPE_GAME: >> + return DISPLAY_CONTENT_TYPE_GAME; >> + } >> +} >> + >> static bool adjust_colour_depth_from_display_info( >> struct dc_crtc_timing *timing_out, >> const struct drm_display_info *info) >> @@ -5349,6 +5367,7 @@ static void fill_stream_properties_from_drm_display_mode( >> } >> >> stream->output_color_space = get_output_color_space(timing_out); >> + stream->content_type = get_output_content_type(connector_state); >> } >> >> static void fill_audio_info(struct audio_info *audio_info, >> @@ -7123,6 +7142,11 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, >> adev->mode_info.abm_level_property, 0); >> } >> >> + if (connector_type == DRM_MODE_CONNECTOR_HDMIA) { >> + /* Content Type is currently only implemented for HDMI. */ >> + drm_connector_attach_content_type_property(&aconnector->base); >> + } >> + >> if (connector_type == DRM_MODE_CONNECTOR_HDMIA || >> connector_type == DRM_MODE_CONNECTOR_DisplayPort || >> connector_type == DRM_MODE_CONNECTOR_eDP) { >> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c >> index a5b5f8592c1b..39ceccdb6586 100644 >> --- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c >> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c >> @@ -2944,14 +2944,9 @@ static void set_avi_info_frame( >> uint32_t pixel_encoding = 0; >> enum scanning_type scan_type = SCANNING_TYPE_NODATA; >> enum dc_aspect_ratio aspect = ASPECT_RATIO_NO_DATA; >> - bool itc = false; >> - uint8_t itc_value = 0; >> - uint8_t cn0_cn1 = 0; >> - unsigned int cn0_cn1_value = 0; >> uint8_t *check_sum = NULL; >> uint8_t byte_index = 0; >> union hdmi_info_packet hdmi_info; >> - union display_content_support support = {0}; >> unsigned int vic = pipe_ctx->stream->timing.vic; >> unsigned int rid = pipe_ctx->stream->timing.rid; >> unsigned int fr_ind = pipe_ctx->stream->timing.fr_index; >> @@ -3055,49 +3050,27 @@ static void set_avi_info_frame( >> /* Active Format Aspect ratio - same as Picture Aspect Ratio. */ >> hdmi_info.bits.R0_R3 = ACTIVE_FORMAT_ASPECT_RATIO_SAME_AS_PICTURE; >> >> - /* TODO: un-hardcode cn0_cn1 and itc */ >> - >> - cn0_cn1 = 0; >> - cn0_cn1_value = 0; >> - >> - itc = true; >> - itc_value = 1; >> - >> - support = stream->content_support; >> - >> - if (itc) { >> - if (!support.bits.valid_content_type) { >> - cn0_cn1_value = 0; >> - } else { >> - if (cn0_cn1 == DISPLAY_CONTENT_TYPE_GRAPHICS) { >> - if (support.bits.graphics_content == 1) { >> - cn0_cn1_value = 0; >> - } >> - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_PHOTO) { >> - if (support.bits.photo_content == 1) { >> - cn0_cn1_value = 1; >> - } else { >> - cn0_cn1_value = 0; >> - itc_value = 0; >> - } >> - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_CINEMA) { >> - if (support.bits.cinema_content == 1) { >> - cn0_cn1_value = 2; >> - } else { >> - cn0_cn1_value = 0; >> - itc_value = 0; >> - } >> - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_GAME) { >> - if (support.bits.game_content == 1) { >> - cn0_cn1_value = 3; >> - } else { >> - cn0_cn1_value = 0; >> - itc_value = 0; >> - } >> - } >> - } >> - hdmi_info.bits.CN0_CN1 = cn0_cn1_value; >> - hdmi_info.bits.ITC = itc_value; >> + switch (stream->content_type) { >> + case DISPLAY_CONTENT_TYPE_NO_DATA: >> + hdmi_info.bits.CN0_CN1 = 0; >> + hdmi_info.bits.ITC = 0; > Hmm.. why is ITC value equal zero here ^, instead of the same hardcoded > `itc_value = 1`? Does it come from a DRM default value? > > Other than that, changes seem fine to me and it's nice to see the code > wired to the DRM and actually used. > > CC'ing other AMD DC folks since I don't know if these changes affect > other platforms. Can you guys verify it? > It's on my radar. Harry >> + break; >> + case DISPLAY_CONTENT_TYPE_GRAPHICS: >> + hdmi_info.bits.CN0_CN1 = 0; >> + hdmi_info.bits.ITC = 1; >> + break; >> + case DISPLAY_CONTENT_TYPE_PHOTO: >> + hdmi_info.bits.CN0_CN1 = 1; >> + hdmi_info.bits.ITC = 1; >> + break; >> + case DISPLAY_CONTENT_TYPE_CINEMA: >> + hdmi_info.bits.CN0_CN1 = 2; >> + hdmi_info.bits.ITC = 1; >> + break; >> + case DISPLAY_CONTENT_TYPE_GAME: >> + hdmi_info.bits.CN0_CN1 = 3; >> + hdmi_info.bits.ITC = 1; >> + break; >> } >> >> if (stream->qs_bit == 1) { >> diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h >> index ef33d7d8a2bf..51dc30706e43 100644 >> --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h >> +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h >> @@ -205,6 +205,7 @@ struct dc_stream_state { >> struct dc_csc_transform csc_color_matrix; >> >> enum dc_color_space output_color_space; >> + enum display_content_type content_type; >> enum dc_dither_option dither_option; >> >> enum view_3d_format view_format; >> -- >> 2.39.0 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/amd/display: Hook up 'content type' property for HDMI 2023-01-19 18:14 ` [PATCH 1/2] drm/amd/display: Hook up 'content type' property for HDMI Melissa Wen 2023-01-19 19:29 ` Harry Wentland @ 2023-01-19 22:20 ` Joshua Ashton 2023-01-24 0:24 ` Harry Wentland 1 sibling, 1 reply; 8+ messages in thread From: Joshua Ashton @ 2023-01-19 22:20 UTC (permalink / raw) To: Melissa Wen; +Cc: sunpeng.li, Rodrigo.Siqueira, harry.wentland, amd-gfx On 1/19/23 18:14, Melissa Wen wrote: > On 01/17, Joshua Ashton wrote: >> Implements the 'content type' property for HDMI connectors. >> Verified by checking the avi infoframe on a connected TV. >> >> This also simplifies a lot of the code in that area as well, there were >> a lot of temp variables doing very little and unnecessary logic >> that was quite confusing. >> >> It is not necessary to check for support in the EDID before sending a >> 'content type' value in the avi infoframe also. >> >> Signed-off-by: Joshua Ashton <joshua@froggi.es> >> --- >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 24 +++++++ >> .../gpu/drm/amd/display/dc/core/dc_resource.c | 69 ++++++------------- >> drivers/gpu/drm/amd/display/dc/dc_stream.h | 1 + >> 3 files changed, 46 insertions(+), 48 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> index 9547037857b6..999965fe3de9 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> @@ -5216,6 +5216,24 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing) >> return color_space; >> } >> >> +static enum display_content_type >> +get_output_content_type(const struct drm_connector_state *connector_state) >> +{ >> + switch (connector_state->content_type) { >> + default: >> + case DRM_MODE_CONTENT_TYPE_NO_DATA: >> + return DISPLAY_CONTENT_TYPE_NO_DATA; >> + case DRM_MODE_CONTENT_TYPE_GRAPHICS: >> + return DISPLAY_CONTENT_TYPE_GRAPHICS; >> + case DRM_MODE_CONTENT_TYPE_PHOTO: >> + return DISPLAY_CONTENT_TYPE_PHOTO; >> + case DRM_MODE_CONTENT_TYPE_CINEMA: >> + return DISPLAY_CONTENT_TYPE_CINEMA; >> + case DRM_MODE_CONTENT_TYPE_GAME: >> + return DISPLAY_CONTENT_TYPE_GAME; >> + } >> +} >> + >> static bool adjust_colour_depth_from_display_info( >> struct dc_crtc_timing *timing_out, >> const struct drm_display_info *info) >> @@ -5349,6 +5367,7 @@ static void fill_stream_properties_from_drm_display_mode( >> } >> >> stream->output_color_space = get_output_color_space(timing_out); >> + stream->content_type = get_output_content_type(connector_state); >> } >> >> static void fill_audio_info(struct audio_info *audio_info, >> @@ -7123,6 +7142,11 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, >> adev->mode_info.abm_level_property, 0); >> } >> >> + if (connector_type == DRM_MODE_CONNECTOR_HDMIA) { >> + /* Content Type is currently only implemented for HDMI. */ >> + drm_connector_attach_content_type_property(&aconnector->base); >> + } >> + >> if (connector_type == DRM_MODE_CONNECTOR_HDMIA || >> connector_type == DRM_MODE_CONNECTOR_DisplayPort || >> connector_type == DRM_MODE_CONNECTOR_eDP) { >> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c >> index a5b5f8592c1b..39ceccdb6586 100644 >> --- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c >> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c >> @@ -2944,14 +2944,9 @@ static void set_avi_info_frame( >> uint32_t pixel_encoding = 0; >> enum scanning_type scan_type = SCANNING_TYPE_NODATA; >> enum dc_aspect_ratio aspect = ASPECT_RATIO_NO_DATA; >> - bool itc = false; >> - uint8_t itc_value = 0; >> - uint8_t cn0_cn1 = 0; >> - unsigned int cn0_cn1_value = 0; >> uint8_t *check_sum = NULL; >> uint8_t byte_index = 0; >> union hdmi_info_packet hdmi_info; >> - union display_content_support support = {0}; >> unsigned int vic = pipe_ctx->stream->timing.vic; >> unsigned int rid = pipe_ctx->stream->timing.rid; >> unsigned int fr_ind = pipe_ctx->stream->timing.fr_index; >> @@ -3055,49 +3050,27 @@ static void set_avi_info_frame( >> /* Active Format Aspect ratio - same as Picture Aspect Ratio. */ >> hdmi_info.bits.R0_R3 = ACTIVE_FORMAT_ASPECT_RATIO_SAME_AS_PICTURE; >> >> - /* TODO: un-hardcode cn0_cn1 and itc */ >> - >> - cn0_cn1 = 0; >> - cn0_cn1_value = 0; >> - >> - itc = true; >> - itc_value = 1; >> - >> - support = stream->content_support; >> - >> - if (itc) { >> - if (!support.bits.valid_content_type) { >> - cn0_cn1_value = 0; >> - } else { >> - if (cn0_cn1 == DISPLAY_CONTENT_TYPE_GRAPHICS) { >> - if (support.bits.graphics_content == 1) { >> - cn0_cn1_value = 0; >> - } >> - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_PHOTO) { >> - if (support.bits.photo_content == 1) { >> - cn0_cn1_value = 1; >> - } else { >> - cn0_cn1_value = 0; >> - itc_value = 0; >> - } >> - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_CINEMA) { >> - if (support.bits.cinema_content == 1) { >> - cn0_cn1_value = 2; >> - } else { >> - cn0_cn1_value = 0; >> - itc_value = 0; >> - } >> - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_GAME) { >> - if (support.bits.game_content == 1) { >> - cn0_cn1_value = 3; >> - } else { >> - cn0_cn1_value = 0; >> - itc_value = 0; >> - } >> - } >> - } >> - hdmi_info.bits.CN0_CN1 = cn0_cn1_value; >> - hdmi_info.bits.ITC = itc_value; >> + switch (stream->content_type) { >> + case DISPLAY_CONTENT_TYPE_NO_DATA: >> + hdmi_info.bits.CN0_CN1 = 0; >> + hdmi_info.bits.ITC = 0; > Hmm.. why is ITC value equal zero here ^, instead of the same hardcoded > `itc_value = 1`? Does it come from a DRM default value? > > Other than that, changes seem fine to me and it's nice to see the code > wired to the DRM and actually used. > > CC'ing other AMD DC folks since I don't know if these changes affect > other platforms. Can you guys verify it? This is the same logic as before, itc_value is defaulted to 1, yes, but only assigned to hdmi_info.bits.ITC in the path with valid_content_type which was always false as that struct was never filled in. So previously, ITC = 0 CN0_CN1 = 0 always. The logic added here also matches the logic used by other DRM drivers using the common `drm_hdmi_avi_infoframe_content_type` logic such as Intel. It might be nice to clean up the code so we can take advantage of the common helper here at some point, but currently AMDGPU uses its own `avi_info_frame` structure instead of the common one in DRM. The structures should match (in theory! I did not look) so it might be possible to just cast `avi_info_frame*` to `hdmi_avi_infoframe*` and use the helper Let me know what is preferred. Thanks! - Joshie 🐸✨ > >> + break; >> + case DISPLAY_CONTENT_TYPE_GRAPHICS: >> + hdmi_info.bits.CN0_CN1 = 0; >> + hdmi_info.bits.ITC = 1; >> + break; >> + case DISPLAY_CONTENT_TYPE_PHOTO: >> + hdmi_info.bits.CN0_CN1 = 1; >> + hdmi_info.bits.ITC = 1; >> + break; >> + case DISPLAY_CONTENT_TYPE_CINEMA: >> + hdmi_info.bits.CN0_CN1 = 2; >> + hdmi_info.bits.ITC = 1; >> + break; >> + case DISPLAY_CONTENT_TYPE_GAME: >> + hdmi_info.bits.CN0_CN1 = 3; >> + hdmi_info.bits.ITC = 1; >> + break; >> } >> >> if (stream->qs_bit == 1) { >> diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h >> index ef33d7d8a2bf..51dc30706e43 100644 >> --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h >> +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h >> @@ -205,6 +205,7 @@ struct dc_stream_state { >> struct dc_csc_transform csc_color_matrix; >> >> enum dc_color_space output_color_space; >> + enum display_content_type content_type; >> enum dc_dither_option dither_option; >> >> enum view_3d_format view_format; >> -- >> 2.39.0 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/amd/display: Hook up 'content type' property for HDMI 2023-01-19 22:20 ` Joshua Ashton @ 2023-01-24 0:24 ` Harry Wentland 2023-02-17 23:55 ` Joshua Ashton 0 siblings, 1 reply; 8+ messages in thread From: Harry Wentland @ 2023-01-24 0:24 UTC (permalink / raw) To: Joshua Ashton, Melissa Wen; +Cc: sunpeng.li, Rodrigo.Siqueira, amd-gfx On 1/19/23 17:20, Joshua Ashton wrote: > > > On 1/19/23 18:14, Melissa Wen wrote: >> On 01/17, Joshua Ashton wrote: >>> Implements the 'content type' property for HDMI connectors. >>> Verified by checking the avi infoframe on a connected TV. >>> >>> This also simplifies a lot of the code in that area as well, there were >>> a lot of temp variables doing very little and unnecessary logic >>> that was quite confusing. >>> >>> It is not necessary to check for support in the EDID before sending a >>> 'content type' value in the avi infoframe also. >>> >>> Signed-off-by: Joshua Ashton <joshua@froggi.es> >>> --- >>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 24 +++++++ >>> .../gpu/drm/amd/display/dc/core/dc_resource.c | 69 ++++++------------- >>> drivers/gpu/drm/amd/display/dc/dc_stream.h | 1 + >>> 3 files changed, 46 insertions(+), 48 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>> index 9547037857b6..999965fe3de9 100644 >>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>> @@ -5216,6 +5216,24 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing) >>> return color_space; >>> } >>> +static enum display_content_type >>> +get_output_content_type(const struct drm_connector_state *connector_state) >>> +{ >>> + switch (connector_state->content_type) { >>> + default: >>> + case DRM_MODE_CONTENT_TYPE_NO_DATA: >>> + return DISPLAY_CONTENT_TYPE_NO_DATA; >>> + case DRM_MODE_CONTENT_TYPE_GRAPHICS: >>> + return DISPLAY_CONTENT_TYPE_GRAPHICS; >>> + case DRM_MODE_CONTENT_TYPE_PHOTO: >>> + return DISPLAY_CONTENT_TYPE_PHOTO; >>> + case DRM_MODE_CONTENT_TYPE_CINEMA: >>> + return DISPLAY_CONTENT_TYPE_CINEMA; >>> + case DRM_MODE_CONTENT_TYPE_GAME: >>> + return DISPLAY_CONTENT_TYPE_GAME; >>> + } >>> +} >>> + >>> static bool adjust_colour_depth_from_display_info( >>> struct dc_crtc_timing *timing_out, >>> const struct drm_display_info *info) >>> @@ -5349,6 +5367,7 @@ static void fill_stream_properties_from_drm_display_mode( >>> } >>> stream->output_color_space = get_output_color_space(timing_out); >>> + stream->content_type = get_output_content_type(connector_state); >>> } >>> static void fill_audio_info(struct audio_info *audio_info, >>> @@ -7123,6 +7142,11 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, >>> adev->mode_info.abm_level_property, 0); >>> } >>> + if (connector_type == DRM_MODE_CONNECTOR_HDMIA) { >>> + /* Content Type is currently only implemented for HDMI. */ >>> + drm_connector_attach_content_type_property(&aconnector->base); >>> + } >>> + >>> if (connector_type == DRM_MODE_CONNECTOR_HDMIA || >>> connector_type == DRM_MODE_CONNECTOR_DisplayPort || >>> connector_type == DRM_MODE_CONNECTOR_eDP) { >>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c >>> index a5b5f8592c1b..39ceccdb6586 100644 >>> --- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c >>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c >>> @@ -2944,14 +2944,9 @@ static void set_avi_info_frame( >>> uint32_t pixel_encoding = 0; >>> enum scanning_type scan_type = SCANNING_TYPE_NODATA; >>> enum dc_aspect_ratio aspect = ASPECT_RATIO_NO_DATA; >>> - bool itc = false; >>> - uint8_t itc_value = 0; >>> - uint8_t cn0_cn1 = 0; >>> - unsigned int cn0_cn1_value = 0; >>> uint8_t *check_sum = NULL; >>> uint8_t byte_index = 0; >>> union hdmi_info_packet hdmi_info; >>> - union display_content_support support = {0}; >>> unsigned int vic = pipe_ctx->stream->timing.vic; >>> unsigned int rid = pipe_ctx->stream->timing.rid; >>> unsigned int fr_ind = pipe_ctx->stream->timing.fr_index; >>> @@ -3055,49 +3050,27 @@ static void set_avi_info_frame( >>> /* Active Format Aspect ratio - same as Picture Aspect Ratio. */ >>> hdmi_info.bits.R0_R3 = ACTIVE_FORMAT_ASPECT_RATIO_SAME_AS_PICTURE; >>> - /* TODO: un-hardcode cn0_cn1 and itc */ >>> - >>> - cn0_cn1 = 0; >>> - cn0_cn1_value = 0; >>> - >>> - itc = true; >>> - itc_value = 1; >>> - >>> - support = stream->content_support; >>> - >>> - if (itc) { >>> - if (!support.bits.valid_content_type) { >>> - cn0_cn1_value = 0; >>> - } else { >>> - if (cn0_cn1 == DISPLAY_CONTENT_TYPE_GRAPHICS) { >>> - if (support.bits.graphics_content == 1) { >>> - cn0_cn1_value = 0; >>> - } >>> - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_PHOTO) { >>> - if (support.bits.photo_content == 1) { >>> - cn0_cn1_value = 1; >>> - } else { >>> - cn0_cn1_value = 0; >>> - itc_value = 0; >>> - } >>> - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_CINEMA) { >>> - if (support.bits.cinema_content == 1) { >>> - cn0_cn1_value = 2; >>> - } else { >>> - cn0_cn1_value = 0; >>> - itc_value = 0; >>> - } >>> - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_GAME) { >>> - if (support.bits.game_content == 1) { >>> - cn0_cn1_value = 3; >>> - } else { >>> - cn0_cn1_value = 0; >>> - itc_value = 0; >>> - } >>> - } >>> - } >>> - hdmi_info.bits.CN0_CN1 = cn0_cn1_value; >>> - hdmi_info.bits.ITC = itc_value; >>> + switch (stream->content_type) { >>> + case DISPLAY_CONTENT_TYPE_NO_DATA: >>> + hdmi_info.bits.CN0_CN1 = 0; >>> + hdmi_info.bits.ITC = 0; >> Hmm.. why is ITC value equal zero here ^, instead of the same hardcoded >> `itc_value = 1`? Does it come from a DRM default value? >> >> Other than that, changes seem fine to me and it's nice to see the code >> wired to the DRM and actually used. >> >> CC'ing other AMD DC folks since I don't know if these changes affect >> other platforms. Can you guys verify it? > > This is the same logic as before, itc_value is defaulted to 1, yes, but only assigned to hdmi_info.bits.ITC in the path with valid_content_type which was always false as that struct was never filled in. > So previously, ITC = 0 CN0_CN1 = 0 always. > > The logic added here also matches the logic used by other DRM drivers using the common `drm_hdmi_avi_infoframe_content_type` logic such as Intel. > > It might be nice to clean up the code so we can take advantage of the common helper here at some point, but currently AMDGPU uses its own `avi_info_frame` structure instead of the common one in DRM. > The structures should match (in theory! I did not look) so it might be possible to just cast `avi_info_frame*` to `hdmi_avi_infoframe*` and use the helper > > Let me know what is preferred. > This series looks good to me and is Reviewed-by: Harry Wentland <harry.wentland@amd.com> I have not had a chance to test it. Do you have any way to verify that the receiver gets the correct content type? I also don't see any IGT tests that exercise these. It might be good to have those. Even if we can't verify correct functionality from IGT we can at least ensure setting this property doesn't lead to bad results in the drivers. Would you like me to merge it through amd-staging-drm-next? Harry > Thanks! > - Joshie 🐸✨ > >> >>> + break; >>> + case DISPLAY_CONTENT_TYPE_GRAPHICS: >>> + hdmi_info.bits.CN0_CN1 = 0; >>> + hdmi_info.bits.ITC = 1; >>> + break; >>> + case DISPLAY_CONTENT_TYPE_PHOTO: >>> + hdmi_info.bits.CN0_CN1 = 1; >>> + hdmi_info.bits.ITC = 1; >>> + break; >>> + case DISPLAY_CONTENT_TYPE_CINEMA: >>> + hdmi_info.bits.CN0_CN1 = 2; >>> + hdmi_info.bits.ITC = 1; >>> + break; >>> + case DISPLAY_CONTENT_TYPE_GAME: >>> + hdmi_info.bits.CN0_CN1 = 3; >>> + hdmi_info.bits.ITC = 1; >>> + break; >>> } >>> if (stream->qs_bit == 1) { >>> diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h >>> index ef33d7d8a2bf..51dc30706e43 100644 >>> --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h >>> +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h >>> @@ -205,6 +205,7 @@ struct dc_stream_state { >>> struct dc_csc_transform csc_color_matrix; >>> enum dc_color_space output_color_space; >>> + enum display_content_type content_type; >>> enum dc_dither_option dither_option; >>> enum view_3d_format view_format; >>> -- >>> 2.39.0 >>> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] drm/amd/display: Hook up 'content type' property for HDMI 2023-01-24 0:24 ` Harry Wentland @ 2023-02-17 23:55 ` Joshua Ashton 0 siblings, 0 replies; 8+ messages in thread From: Joshua Ashton @ 2023-02-17 23:55 UTC (permalink / raw) To: Harry Wentland, Melissa Wen; +Cc: sunpeng.li, Rodrigo.Siqueira, amd-gfx On 1/24/23 00:24, Harry Wentland wrote: > > > On 1/19/23 17:20, Joshua Ashton wrote: >> >> >> On 1/19/23 18:14, Melissa Wen wrote: >>> On 01/17, Joshua Ashton wrote: >>>> Implements the 'content type' property for HDMI connectors. >>>> Verified by checking the avi infoframe on a connected TV. >>>> >>>> This also simplifies a lot of the code in that area as well, there were >>>> a lot of temp variables doing very little and unnecessary logic >>>> that was quite confusing. >>>> >>>> It is not necessary to check for support in the EDID before sending a >>>> 'content type' value in the avi infoframe also. >>>> >>>> Signed-off-by: Joshua Ashton <joshua@froggi.es> >>>> --- >>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 24 +++++++ >>>> .../gpu/drm/amd/display/dc/core/dc_resource.c | 69 ++++++------------- >>>> drivers/gpu/drm/amd/display/dc/dc_stream.h | 1 + >>>> 3 files changed, 46 insertions(+), 48 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>> index 9547037857b6..999965fe3de9 100644 >>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>>> @@ -5216,6 +5216,24 @@ get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing) >>>> return color_space; >>>> } >>>> +static enum display_content_type >>>> +get_output_content_type(const struct drm_connector_state *connector_state) >>>> +{ >>>> + switch (connector_state->content_type) { >>>> + default: >>>> + case DRM_MODE_CONTENT_TYPE_NO_DATA: >>>> + return DISPLAY_CONTENT_TYPE_NO_DATA; >>>> + case DRM_MODE_CONTENT_TYPE_GRAPHICS: >>>> + return DISPLAY_CONTENT_TYPE_GRAPHICS; >>>> + case DRM_MODE_CONTENT_TYPE_PHOTO: >>>> + return DISPLAY_CONTENT_TYPE_PHOTO; >>>> + case DRM_MODE_CONTENT_TYPE_CINEMA: >>>> + return DISPLAY_CONTENT_TYPE_CINEMA; >>>> + case DRM_MODE_CONTENT_TYPE_GAME: >>>> + return DISPLAY_CONTENT_TYPE_GAME; >>>> + } >>>> +} >>>> + >>>> static bool adjust_colour_depth_from_display_info( >>>> struct dc_crtc_timing *timing_out, >>>> const struct drm_display_info *info) >>>> @@ -5349,6 +5367,7 @@ static void fill_stream_properties_from_drm_display_mode( >>>> } >>>> stream->output_color_space = get_output_color_space(timing_out); >>>> + stream->content_type = get_output_content_type(connector_state); >>>> } >>>> static void fill_audio_info(struct audio_info *audio_info, >>>> @@ -7123,6 +7142,11 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm, >>>> adev->mode_info.abm_level_property, 0); >>>> } >>>> + if (connector_type == DRM_MODE_CONNECTOR_HDMIA) { >>>> + /* Content Type is currently only implemented for HDMI. */ >>>> + drm_connector_attach_content_type_property(&aconnector->base); >>>> + } >>>> + >>>> if (connector_type == DRM_MODE_CONNECTOR_HDMIA || >>>> connector_type == DRM_MODE_CONNECTOR_DisplayPort || >>>> connector_type == DRM_MODE_CONNECTOR_eDP) { >>>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c >>>> index a5b5f8592c1b..39ceccdb6586 100644 >>>> --- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c >>>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c >>>> @@ -2944,14 +2944,9 @@ static void set_avi_info_frame( >>>> uint32_t pixel_encoding = 0; >>>> enum scanning_type scan_type = SCANNING_TYPE_NODATA; >>>> enum dc_aspect_ratio aspect = ASPECT_RATIO_NO_DATA; >>>> - bool itc = false; >>>> - uint8_t itc_value = 0; >>>> - uint8_t cn0_cn1 = 0; >>>> - unsigned int cn0_cn1_value = 0; >>>> uint8_t *check_sum = NULL; >>>> uint8_t byte_index = 0; >>>> union hdmi_info_packet hdmi_info; >>>> - union display_content_support support = {0}; >>>> unsigned int vic = pipe_ctx->stream->timing.vic; >>>> unsigned int rid = pipe_ctx->stream->timing.rid; >>>> unsigned int fr_ind = pipe_ctx->stream->timing.fr_index; >>>> @@ -3055,49 +3050,27 @@ static void set_avi_info_frame( >>>> /* Active Format Aspect ratio - same as Picture Aspect Ratio. */ >>>> hdmi_info.bits.R0_R3 = ACTIVE_FORMAT_ASPECT_RATIO_SAME_AS_PICTURE; >>>> - /* TODO: un-hardcode cn0_cn1 and itc */ >>>> - >>>> - cn0_cn1 = 0; >>>> - cn0_cn1_value = 0; >>>> - >>>> - itc = true; >>>> - itc_value = 1; >>>> - >>>> - support = stream->content_support; >>>> - >>>> - if (itc) { >>>> - if (!support.bits.valid_content_type) { >>>> - cn0_cn1_value = 0; >>>> - } else { >>>> - if (cn0_cn1 == DISPLAY_CONTENT_TYPE_GRAPHICS) { >>>> - if (support.bits.graphics_content == 1) { >>>> - cn0_cn1_value = 0; >>>> - } >>>> - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_PHOTO) { >>>> - if (support.bits.photo_content == 1) { >>>> - cn0_cn1_value = 1; >>>> - } else { >>>> - cn0_cn1_value = 0; >>>> - itc_value = 0; >>>> - } >>>> - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_CINEMA) { >>>> - if (support.bits.cinema_content == 1) { >>>> - cn0_cn1_value = 2; >>>> - } else { >>>> - cn0_cn1_value = 0; >>>> - itc_value = 0; >>>> - } >>>> - } else if (cn0_cn1 == DISPLAY_CONTENT_TYPE_GAME) { >>>> - if (support.bits.game_content == 1) { >>>> - cn0_cn1_value = 3; >>>> - } else { >>>> - cn0_cn1_value = 0; >>>> - itc_value = 0; >>>> - } >>>> - } >>>> - } >>>> - hdmi_info.bits.CN0_CN1 = cn0_cn1_value; >>>> - hdmi_info.bits.ITC = itc_value; >>>> + switch (stream->content_type) { >>>> + case DISPLAY_CONTENT_TYPE_NO_DATA: >>>> + hdmi_info.bits.CN0_CN1 = 0; >>>> + hdmi_info.bits.ITC = 0; >>> Hmm.. why is ITC value equal zero here ^, instead of the same hardcoded >>> `itc_value = 1`? Does it come from a DRM default value? >>> >>> Other than that, changes seem fine to me and it's nice to see the code >>> wired to the DRM and actually used. >>> >>> CC'ing other AMD DC folks since I don't know if these changes affect >>> other platforms. Can you guys verify it? >> >> This is the same logic as before, itc_value is defaulted to 1, yes, but only assigned to hdmi_info.bits.ITC in the path with valid_content_type which was always false as that struct was never filled in. >> So previously, ITC = 0 CN0_CN1 = 0 always. >> >> The logic added here also matches the logic used by other DRM drivers using the common `drm_hdmi_avi_infoframe_content_type` logic such as Intel. >> >> It might be nice to clean up the code so we can take advantage of the common helper here at some point, but currently AMDGPU uses its own `avi_info_frame` structure instead of the common one in DRM. >> The structures should match (in theory! I did not look) so it might be possible to just cast `avi_info_frame*` to `hdmi_avi_infoframe*` and use the helper >> >> Let me know what is preferred. >> > > This series looks good to me and is > Reviewed-by: Harry Wentland <harry.wentland@amd.com> > > I have not had a chance to test it. Do you have any way to verify > that the receiver gets the correct content type? It was tested on an LG OLED which has a debug overlay that shows the infoframe over the screen. > > I also don't see any IGT tests that exercise these. It might be > good to have those. Even if we can't verify correct functionality > from IGT we can at least ensure setting this property doesn't > lead to bad results in the drivers. > > Would you like me to merge it through amd-staging-drm-next? Yes please. - Joshie 🐸✨ > > Harry > >> Thanks! >> - Joshie 🐸✨ >> >>> >>>> + break; >>>> + case DISPLAY_CONTENT_TYPE_GRAPHICS: >>>> + hdmi_info.bits.CN0_CN1 = 0; >>>> + hdmi_info.bits.ITC = 1; >>>> + break; >>>> + case DISPLAY_CONTENT_TYPE_PHOTO: >>>> + hdmi_info.bits.CN0_CN1 = 1; >>>> + hdmi_info.bits.ITC = 1; >>>> + break; >>>> + case DISPLAY_CONTENT_TYPE_CINEMA: >>>> + hdmi_info.bits.CN0_CN1 = 2; >>>> + hdmi_info.bits.ITC = 1; >>>> + break; >>>> + case DISPLAY_CONTENT_TYPE_GAME: >>>> + hdmi_info.bits.CN0_CN1 = 3; >>>> + hdmi_info.bits.ITC = 1; >>>> + break; >>>> } >>>> if (stream->qs_bit == 1) { >>>> diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h >>>> index ef33d7d8a2bf..51dc30706e43 100644 >>>> --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h >>>> +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h >>>> @@ -205,6 +205,7 @@ struct dc_stream_state { >>>> struct dc_csc_transform csc_color_matrix; >>>> enum dc_color_space output_color_space; >>>> + enum display_content_type content_type; >>>> enum dc_dither_option dither_option; >>>> enum view_3d_format view_format; >>>> -- >>>> 2.39.0 >>>> > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-02-17 23:55 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-01-17 21:26 [PATCH 1/2] drm/amd/display: Hook up 'content type' property for HDMI Joshua Ashton 2023-01-17 21:26 ` [PATCH 2/2] drm/amd/display: Remove unused display_content_support Joshua Ashton 2023-01-19 18:21 ` Melissa Wen 2023-01-19 18:14 ` [PATCH 1/2] drm/amd/display: Hook up 'content type' property for HDMI Melissa Wen 2023-01-19 19:29 ` Harry Wentland 2023-01-19 22:20 ` Joshua Ashton 2023-01-24 0:24 ` Harry Wentland 2023-02-17 23:55 ` Joshua Ashton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox