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@xxxxxxxxx>
>> ---
>> .../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
>>
