[PATCH 1/2] drm/amd/display: Hook up 'content type' property for HDMI
Joshua Ashton
joshua at froggi.es
Fri Feb 17 23:55:24 UTC 2023
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 at 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 at 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
>>>>
>
>
More information about the amd-gfx
mailing list