Re: [RFC v2 3/5] drm: Add HDMI infoframe helpers

[Lars-Peter Clausen <lars@metafoo.de>]

On 12/06/2012 08:28 AM, Thierry Reding wrote:
> On Wed, Dec 05, 2012 at 06:51:20PM +0100, Lars-Peter Clausen wrote:
>> On 12/05/2012 05:45 PM, Thierry Reding wrote:
>>> Add a generic helper to fill in an HDMI AVI infoframe with data
>>> extracted from a DRM display mode.
>>
>> That's a very nice patch series, comes in pretty handy. Thanks :)
>>
>> I've just one comment.
>>
>>> [...]
>>> +static inline enum hdmi_picture_aspect
>>> +drm_display_mode_get_aspect(const struct drm_display_mode *mode)
>>> +{
>>> +	enum hdmi_picture_aspect aspect = HDMI_PICTURE_ASPECT_NONE;
>>> +
>>> +	if ((mode->hdisplay * 9) / 16 == mode->vdisplay)
>>> +		aspect = HDMI_PICTURE_ASPECT_16_9;
>>> +	else if ((mode->hdisplay * 3) / 4 == mode->vdisplay)
>>> +		aspect = HDMI_PICTURE_ASPECT_4_3;
>>> +
>>> +	return aspect;
>>> +}
>>> +
>>> +/**
>>> + * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe with
>>> + *                                              data from a DRM display mode
>>> + * @frame: HDMI AVI infoframe
>>> + * @mode: DRM display mode
>>> + *
>>> + * Returns 0 on success or a negative error code on failure.
>>> + */
>>> +int
>>> +drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
>>> +					 const struct drm_display_mode *mode)
>>> +{
>> [...]
>>> +
>>> +	frame->picture_aspect = drm_display_mode_get_aspect(mode);
>>
>> As far as I know the picture aspect ratio is supposed to be the aspect ratio
>> of the displayed content not the pixel aspect ratio. The receiver already
>> knows the pixel aspect ratio since it knows both the pixel width and the
>> height. The picture aspect ratio is something that could be set by a
>> userspace application running in fullscreen, e.g. a video player.
> 
> Isn't that what the active aspect ratio is supposed to do? Unfortunately
> neither the HDMI specification nor CEA-861-D seem to be very specific
> about this. What I did in the above was basically just refactoring from
> what the Tegra driver does. None of the other drivers are filling in any
> useful values here. Most equipment seems to work just fine if you pass a
> zeroed out AVI infoframe. =)
> 

The active aspect ratio is for when you have black borders on the sides of your
images. The picture aspect field is for when you for example have non square
pixels, which probably does not happen so often with PC like device.

Hm, btw. I think some HDMI VICs are defined to have non-square pixels like for
example 576p I wonder if we should skip these when we select the VIC. Otherwise
the video data may look stretched when displayed on the HDMI sink.

> Given that we don't have a way to pass any kind of information to the
> kernel about this, the only options we have are to either use this or go
> with HDMI_PICTURE_ASPECT_NONE. I suppose that the latter will work fine
> too.
> 

Agreed. HDMI_PICTURE_ASPECT_NONE should hopefully work.