| Message ID | 20191003071549.31272-1-jkorsnes@cisco.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drivers: video: hdmi: log ext colorimetry applicability | expand |
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
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 >
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
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 >
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 >
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));
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(-)