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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@intel.com>
> >> ---
> >>   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.

> 
> - 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@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel