Re: [PATCH 08/20] drm: set output colorspace in AVI infoframe

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Regards

Shashank


On 7/13/2017 6:05 PM, Ville Syrjälä wrote:
On Thu, Jul 13, 2017 at 10:37:53AM +0530, Sharma, Shashank wrote:
Regards

Shashank


On 7/12/2017 10:47 PM, Ville Syrjälä wrote:
On Mon, Jul 10, 2017 at 04:48:36PM +0530, Shashank Sharma wrote:
A source must set output colorspace information in AVI
infoframes, so that the sink can decode upcoming frames
accordingly.

This patch adds a function to add the output colorspace
information in the AVI infoframes.

V2: Rebase
V3: Rebase
V4: Rebase
V5: Rebase
V6: Made patch independent of HDMI output type.

Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
---
   drivers/gpu/drm/drm_edid.c | 29 +++++++++++++++++++++++++++++
   include/drm/drm_edid.h     |  5 +++++
   2 files changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 944a28f..cede86e 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4796,6 +4796,35 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
   EXPORT_SYMBOL(drm_hdmi_avi_infoframe_from_display_mode);
/**
+ * drm_hdmi_avi_infoframe_set_colorspace - fill an HDMI AVI infoframe with
+ * colorspace data of the output type
+ *
+ * @frame: HDMI AVI infoframe
+ * @mode: DRM display mode
+ * @hdmi_output: HDMI output colorspace
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int
+drm_hdmi_avi_infoframe_set_colorspace(struct hdmi_avi_infoframe *frame,
+				      const struct drm_display_mode *mode,
+				      enum hdmi_colorspace colorspace)
+{
+	if (colorspace > HDMI_COLORSPACE_YUV420 ||
+		colorspace < HDMI_COLORSPACE_RGB) {
+		DRM_ERROR("Invalid color space type\n");
+		return -EINVAL;
+	}
Seems overly defensive. I'd say that if someone insists on writing
buggy code just let them do it.
:) yep can be done, you know, its a new implementation, don't want to
create unnecessary noise so being
a bit defensive :)
+
+	frame->colorspace = colorspace;
+	if (colorspace == HDMI_COLORSPACE_YUV420)
+		frame->pixel_repeat = 0;
Most VICs don't allow pixel repeat in 444/etc. either, and we don't
protect against that. So this looks like pretty pointless check in
this form.

So IMO just drop this entire patch and just assign frame->colorspace in
the driver.
Actually YCBCR420 section of spec specifically calls out for not
allowing repetition, also, when I tested this on a
HDMI 2.0 analyzer, if was giving a AVI IF failure on pixel_repeat not 0,
so IMHO it would be a good idea to keep
this and get the tests passing.
That's just papering over bugs elsewhere. If we can't use pixel repeat
with a specific mode, then we should have rejected that mode much earlier.
I dint get this point, HDMI 2.0 spec section 7.1 says "when YCBCR420 encoding is active, pixel repetition is not allowed" and pixel repetition field should be set to = 0, in AVI IF. This seems to be like - if you are displaying YCBCR420, set PR=0 regardless of
mode, isn't it ?

- Shashank
- Shashank
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_hdmi_avi_infoframe_set_colorspace);
+
+/**
    * drm_hdmi_avi_infoframe_quant_range() - fill the HDMI AVI infoframe
    *                                        quantization range information
    * @frame: HDMI AVI infoframe
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index aa58146..b79e0cb 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -332,6 +332,7 @@ struct cea_sad {
   struct drm_encoder;
   struct drm_connector;
   struct drm_display_mode;
+enum drm_hdmi_output_type;
void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid);
   int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads);
@@ -354,6 +355,10 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
   					 const struct drm_display_mode *mode,
   					 bool is_hdmi2_sink);
   int
+drm_hdmi_avi_infoframe_set_colorspace(struct hdmi_avi_infoframe *frame,
+					 const struct drm_display_mode *mode,
+					 enum hdmi_colorspace colorspace);
+int
   drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
   					    const struct drm_display_mode *mode);
   void
--
2.7.4

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

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux