Re: [PATCH v2] video: hdmi: indicate applicability in avi infoframe log

["Johan Korsnes (jkorsnes)" <jkorsnes@cisco.com>]

On 12/11/19 5:08 PM, Ville Syrjälä wrote:
> On Wed, Dec 11, 2019 at 10:48:42AM +0100, Johan Korsnes wrote:
>> When logging the AVI InfoFrame, clearly indicate whether or not
>> attributes are active/"in effect". The specification is given in
>> CTA-861-G Section 6.4: Format of Version 2, 3 & 4 AVI InfoFrames.
>>
>> Attribute         Bytes    Requirement
>> Ext. Colorimetry  EC0..EC2 Colorimetry (C0,C1) set to Extended.
>> IT Contents Type  CN0,CN1  IT Content (ITC) set to True.
>> RGB Quant. Range  Q0,Q1    Color Space (Y0..Y2) set to RGB.
>> YCC Quant. Range  YQ0,YQ1  Color space (Y0..Y2) set to YCbCr.
>>
>> Example log output with patch applied:
>> HDMI infoframe: Auxiliary Video Information (AVI), version 2, length 13
>>     colorspace: RGB
>>     scan mode: No Data
>>     colorimetry: ITU709
>>     picture aspect: 16:9
>>     active aspect: Same as Picture
>>     itc: IT Content
>>     extended colorimetry: N/A (0x0)
>>     quantization range: Full
>>     nups: Unknown Non-uniform Scaling
>>     video code: 16
>>     ycc quantization range: N/A (0x0)
>>     hdmi content type: Graphics
>>     pixel repeat: 0
>>     bar top 0, bottom 0, left 0, right 0
>>
>> Signed-off-by: Johan Korsnes <jkorsnes@cisco.com>
>> Cc: Hans Verkuil <hansverk@cisco.com>
>> Cc: Martin Bugge <marbugge@cisco.com>
>>
>> ---
>> v1 -> v2:
>>  * Indicate applicability not only for ext. colorimetry
>> ---
>>  drivers/video/hdmi.c | 40 ++++++++++++++++++++++++++++++++--------
>>  1 file changed, 32 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
>> index 9c82e2a0a411..491a77b28a9b 100644
>> --- a/drivers/video/hdmi.c
>> +++ b/drivers/video/hdmi.c
>> @@ -1209,16 +1209,40 @@ static void hdmi_avi_infoframe_log(const char *level,
>>  	hdmi_log("    active aspect: %s\n",
>>  			hdmi_active_aspect_get_name(frame->active_aspect));
>>  	hdmi_log("    itc: %s\n", frame->itc ? "IT Content" : "No Data");
>> -	hdmi_log("    extended colorimetry: %s\n",
>> -			hdmi_extended_colorimetry_get_name(frame->extended_colorimetry));
>> -	hdmi_log("    quantization range: %s\n",
>> -			hdmi_quantization_range_get_name(frame->quantization_range));
>> +
>> +	if (frame->colorimetry == HDMI_COLORIMETRY_EXTENDED)
>> +		hdmi_log("    extended colorimetry: %s\n",
>> +			 hdmi_extended_colorimetry_get_name(frame->extended_colorimetry));
>> +	else
>> +		hdmi_log("    extended colorimetry: N/A (0x%x)\n",
>> +			 frame->extended_colorimetry);
>> +
>> +	if (frame->colorspace == HDMI_COLORSPACE_RGB)
>> +		hdmi_log("    quantization range: %s\n",
>> +			 hdmi_quantization_range_get_name(frame->quantization_range));
>> +	else
>> +		hdmi_log("    quantization range: N/A (0x%x)\n",
>> +			 frame->quantization_range);
>> +
>>  	hdmi_log("    nups: %s\n", hdmi_nups_get_name(frame->nups));
>>  	hdmi_log("    video code: %u\n", frame->video_code);
>> -	hdmi_log("    ycc quantization range: %s\n",
>> -			hdmi_ycc_quantization_range_get_name(frame->ycc_quantization_range));
>> -	hdmi_log("    hdmi content type: %s\n",
>> -			hdmi_content_type_get_name(frame->content_type));
>> +
>> +	if (frame->colorspace == HDMI_COLORSPACE_YUV422 ||
>> +	    frame->colorspace == HDMI_COLORSPACE_YUV444 ||
>> +	    frame->colorspace == HDMI_COLORSPACE_YUV420)
> 
> ocd nit: order 444||422||420 or 420||422||444
> 

Should I send a v3 with your Reviewed-by tag added after fixing this?

>> +		hdmi_log("    ycc quantization range: %s\n",
>> +			 hdmi_ycc_quantization_range_get_name(frame->ycc_quantization_range));
>> +	else
>> +		hdmi_log("    ycc quantization range: N/A (0x%x)\n",
>> +			 frame->ycc_quantization_range);
> 
> CTA-861-G does recommend that we set YQ to match Q when trasmitting
> RGB. So not sure "N/A" is entirely accurate here. However we also
> found out that following that recommendation did break some crappy
> sinks which get confused when they see RGB + YQ!=0. So now we follow
> that recommendation only for HDMI 2.0+ sinks. Anyways, as long as the
> raw value is present I guess we can stil spot such cases from the logs.
> 

Good observation. I don't have any strong opinion about this, so unless
you have any objections I won't change this in v3.

Johan

> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
>> +
>> +	if (frame->itc)
>> +		hdmi_log("    hdmi content type: %s\n",
>> +			 hdmi_content_type_get_name(frame->content_type));
>> +	else
>> +		hdmi_log("    hdmi content type: N/A (0x%x)\n",
>> +			 frame->content_type);
>> +
>>  	hdmi_log("    pixel repeat: %u\n", frame->pixel_repeat);
>>  	hdmi_log("    bar top %u, bottom %u, left %u, right %u\n",
>>  			frame->top_bar, frame->bottom_bar,
>> -- 
>> 2.23.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel