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

["Sharma, Shashank" <shashank.sharma@intel.com>]

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@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 ?

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

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