[Intel-gfx] [PATCH 08/20] drm: set output colorspace in AVI infoframe

[Ville Syrjälä]

On Thu, Jul 13, 2017 at 06:19:01PM +0530, Sharma, Shashank wrote:
> 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 at 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.
> 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 ?

If the mode requires pixel repeat to meet the minimum clock requirement,
then we can't just not do pixel repeat. That would violate the spec in
other ways, and IIRC we couldn't even generate a low enough clock for
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 at lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC