[PATCH] drivers: video: hdmi: log ext colorimetry applicability

[PATCH] drivers: video: hdmi: log ext colorimetry applicability

[Johan Korsnes]

When logging the AVI InfoFrame, clearly indicate whether or not the
extended colorimetry attribute is active. This is only the case when
the AVI InfoFrame colorimetry attribute is set to extended. [0]

[0] CTA-861-G section 6.4 page 57

Signed-off-by: Johan Korsnes <jkorsnes@cisco.com>
---
 drivers/video/hdmi.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index f29db728ff29..a709e38a53ca 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -682,8 +682,14 @@ 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",
+
+	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);
+
 	hdmi_log("    quantization range: %s\n",
 			hdmi_quantization_range_get_name(frame->quantization_range));
 	hdmi_log("    nups: %s\n", hdmi_nups_get_name(frame->nups));
-- 
2.20.1

Re: [PATCH] drivers: video: hdmi: log ext colorimetry applicability

[Ville Syrjälä]

On Thu, Oct 03, 2019 at 09:15:49AM +0200, Johan Korsnes wrote:
> When logging the AVI InfoFrame, clearly indicate whether or not the
> extended colorimetry attribute is active. This is only the case when
> the AVI InfoFrame colorimetry attribute is set to extended. [0]
> 
> [0] CTA-861-G section 6.4 page 57
> 
> Signed-off-by: Johan Korsnes <jkorsnes@cisco.com>
> ---
>  drivers/video/hdmi.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index f29db728ff29..a709e38a53ca 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -682,8 +682,14 @@ 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",
> +
> +	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);

Yeah, seems fine. Might make the logs a bit less confusing at least.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

PS. would be nice it someone were to extend this code to deal with the
ACE bits too. Do you have plans/interest in doing that?

> +
>  	hdmi_log("    quantization range: %s\n",
>  			hdmi_quantization_range_get_name(frame->quantization_range));
>  	hdmi_log("    nups: %s\n", hdmi_nups_get_name(frame->nups));
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel

Re: [PATCH] drivers: video: hdmi: log ext colorimetry applicability

[Johan Korsnes (jkorsnes)]

On 03/10/2019 15.44, Ville Syrjälä wrote:
> On Thu, Oct 03, 2019 at 09:15:49AM +0200, Johan Korsnes wrote:
>> When logging the AVI InfoFrame, clearly indicate whether or not the
>> extended colorimetry attribute is active. This is only the case when
>> the AVI InfoFrame colorimetry attribute is set to extended. [0]
>>
>> [0] CTA-861-G section 6.4 page 57
>>
>> Signed-off-by: Johan Korsnes <jkorsnes@cisco.com>
>> ---
>>  drivers/video/hdmi.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
>> index f29db728ff29..a709e38a53ca 100644
>> --- a/drivers/video/hdmi.c
>> +++ b/drivers/video/hdmi.c
>> @@ -682,8 +682,14 @@ 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",
>> +
>> +	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);
> 
> Yeah, seems fine. Might make the logs a bit less confusing at least.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> PS. would be nice it someone were to extend this code to deal with the
> ACE bits too. Do you have plans/interest in doing that?

I was actually going to deal with the ACE bits as part of this patch,
but noticed that things seem to be hard coded for AVI InfoFrame v2:

int hdmi_avi_infoframe_init(struct hdmi_avi_infoframe *frame) {
    memset(frame, 0, sizeof(*frame));
 
    frame->type = HDMI_INFOFRAME_TYPE_AVI;
    frame->version = 2;
    frame->length = HDMI_AVI_INFOFRAME_SIZE;

    return 0;
}

I have no plans to fix this, for now, unfortunately.

> 
>> +
>>  	hdmi_log("    quantization range: %s\n",
>>  			hdmi_quantization_range_get_name(frame->quantization_range));
>>  	hdmi_log("    nups: %s\n", hdmi_nups_get_name(frame->nups));
>> -- 
>> 2.20.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH] drivers: video: hdmi: log ext colorimetry applicability

[Hans Verkuil]

On 10/3/19 9:15 AM, Johan Korsnes wrote:
> When logging the AVI InfoFrame, clearly indicate whether or not the
> extended colorimetry attribute is active. This is only the case when
> the AVI InfoFrame colorimetry attribute is set to extended. [0]
> 
> [0] CTA-861-G section 6.4 page 57
> 
> Signed-off-by: Johan Korsnes <jkorsnes@cisco.com>
> ---
>  drivers/video/hdmi.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index f29db728ff29..a709e38a53ca 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -682,8 +682,14 @@ 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",
> +
> +	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);
> +
>  	hdmi_log("    quantization range: %s\n",
>  			hdmi_quantization_range_get_name(frame->quantization_range));
>  	hdmi_log("    nups: %s\n", hdmi_nups_get_name(frame->nups));
> 

I just realized that there are more fields like that:

content_type is only valid if itc == true

quantization_range is only valid if colorspace is RGB

ycc_quantization_range is only valid if colorspace is YCC

Can you make a v2 where these fields are handled in the same way?
That would really help reduce the confusion when logging the
AVI InfoFrame.

Regards,

	Hans

Re: [PATCH] drivers: video: hdmi: log ext colorimetry applicability

[Hans Verkuil]

On 10/3/19 3:44 PM, Ville Syrjälä wrote:
> On Thu, Oct 03, 2019 at 09:15:49AM +0200, Johan Korsnes wrote:
>> When logging the AVI InfoFrame, clearly indicate whether or not the
>> extended colorimetry attribute is active. This is only the case when
>> the AVI InfoFrame colorimetry attribute is set to extended. [0]
>>
>> [0] CTA-861-G section 6.4 page 57
>>
>> Signed-off-by: Johan Korsnes <jkorsnes@cisco.com>
>> ---
>>  drivers/video/hdmi.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
>> index f29db728ff29..a709e38a53ca 100644
>> --- a/drivers/video/hdmi.c
>> +++ b/drivers/video/hdmi.c
>> @@ -682,8 +682,14 @@ 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",
>> +
>> +	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);
> 
> Yeah, seems fine. Might make the logs a bit less confusing at least.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> PS. would be nice it someone were to extend this code to deal with the
> ACE bits too. Do you have plans/interest in doing that?

If we tackle this, then it would be part of a larger effort in updating
this code. There are more fields missing in other InfoFrames as well.

So yes, we have interest in this, but no, there are no plans :-)

Regards,

	Hans

> 
>> +
>>  	hdmi_log("    quantization range: %s\n",
>>  			hdmi_quantization_range_get_name(frame->quantization_range));
>>  	hdmi_log("    nups: %s\n", hdmi_nups_get_name(frame->nups));
>> -- 
>> 2.20.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH] drivers: video: hdmi: log ext colorimetry applicability

[Johan Korsnes (jkorsnes)]

On 03/10/2019 16.59, Hans Verkuil wrote:
> On 10/3/19 9:15 AM, Johan Korsnes wrote:
>> When logging the AVI InfoFrame, clearly indicate whether or not the
>> extended colorimetry attribute is active. This is only the case when
>> the AVI InfoFrame colorimetry attribute is set to extended. [0]
>>
>> [0] CTA-861-G section 6.4 page 57
>>
>> Signed-off-by: Johan Korsnes <jkorsnes@cisco.com>
>> ---
>>  drivers/video/hdmi.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
>> index f29db728ff29..a709e38a53ca 100644
>> --- a/drivers/video/hdmi.c
>> +++ b/drivers/video/hdmi.c
>> @@ -682,8 +682,14 @@ 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",
>> +
>> +	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);
>> +
>>  	hdmi_log("    quantization range: %s\n",
>>  			hdmi_quantization_range_get_name(frame->quantization_range));
>>  	hdmi_log("    nups: %s\n", hdmi_nups_get_name(frame->nups));
>>
> 
> I just realized that there are more fields like that:
> 
> content_type is only valid if itc == true
> 
> quantization_range is only valid if colorspace is RGB
> 
> ycc_quantization_range is only valid if colorspace is YCC
> 
> Can you make a v2 where these fields are handled in the same way?
> That would really help reduce the confusion when logging the
> AVI InfoFrame.

I will make a v2 handling these cases as well; thanks for pointing it
out.

Best regards,
Johan

> 
> Regards,
> 
> 	Hans
> 

[PATCH] drivers: video: hdmi: log ext colorimetry applicability

[Johan Korsnes]

When logging the AVI InfoFrame, clearly indicate whether or not the
extended colorimetry attribute is active. This is only the case when
the AVI InfoFrame colorimetry attribute is set to extended. [0]

[0] CTA-861-G section 6.4 page 57

Signed-off-by: Johan Korsnes <jkorsnes@cisco.com>
---
 drivers/video/hdmi.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index f29db728ff29..a709e38a53ca 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -682,8 +682,14 @@ 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",
+
+	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);
+
 	hdmi_log("    quantization range: %s\n",
 			hdmi_quantization_range_get_name(frame->quantization_range));
 	hdmi_log("    nups: %s\n", hdmi_nups_get_name(frame->nups));
-- 
2.20.1