Re: [PATCH v2] video: hdmi: indicate applicability in avi infoframe log
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 BytesRequirement
>> 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,Q1Color 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
>> Cc: Hans Verkuil
>> Cc: Martin Bugge
>>
>> ---
>> 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ä
>
>> +
>> +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
>>
>> __
Re: [PATCH v2] video: hdmi: indicate applicability in avi infoframe log
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 BytesRequirement
> 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,Q1Color 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
> Cc: Hans Verkuil
> Cc: Martin Bugge
>
> ---
> 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
> + 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.
Reviewed-by: Ville Syrjälä
> +
> + 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
--
Ville Syrjälä
Intel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/l
