[Intel-gfx] [PATCH 07/10] drm/edid: Don't send bogus aspect ratios in AVI infoframes

[Sharma, Shashank]

Regards

Shashank


On 11/16/2017 9:56 PM, Ville Syrjälä wrote:
> On Thu, Nov 16, 2017 at 08:31:36PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 11/13/2017 10:34 PM, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>
>>> If the user mode would specify an aspect ratio other than 4:3 or 16:9
>>> we now silently ignore it. Maybe a better apporoach is to return an
>>> error? Let's try that.
>>>
>>> Also we must be careful that we don't try to send illegal picture
>>> aspect in the infoframe as it's only capable of signalling none,
>>> 4:3, and 16:9. Currently we're sending these bogus infoframes
>>> whenever the cea mode specifies some other aspect ratio.
>>>
>>> Cc: Shashank Sharma <shashank.sharma at intel.com>
>>> Cc: Sean Paul <seanpaul at chromium.org>
>>> Cc: Jose Abreu <Jose.Abreu at synopsys.com>
>>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>>> Cc: Emil Velikov <emil.l.velikov at gmail.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/drm_edid.c | 23 +++++++++++++++++------
>>>    1 file changed, 17 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>> index 00aa98f3e55d..bafb3ee4ea97 100644
>>> --- a/drivers/gpu/drm/drm_edid.c
>>> +++ b/drivers/gpu/drm/drm_edid.c
>>> @@ -4786,6 +4786,7 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
>>>    					 const struct drm_display_mode *mode,
>>>    					 bool is_hdmi2_sink)
>>>    {
>>> +	enum hdmi_picture_aspect picture_aspect;
>>>    	int err;
>>>    
>>>    	if (!frame || !mode)
>>> @@ -4828,13 +4829,23 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
>>>    	 * Populate picture aspect ratio from either
>>>    	 * user input (if specified) or from the CEA mode list.
>>>    	 */
>>> -	if (mode->picture_aspect_ratio == HDMI_PICTURE_ASPECT_4_3 ||
>>> -		mode->picture_aspect_ratio == HDMI_PICTURE_ASPECT_16_9)
>>> -		frame->picture_aspect = mode->picture_aspect_ratio;
>>> -	else if (frame->video_code > 0)
>>> -		frame->picture_aspect = drm_get_cea_aspect_ratio(
>>> -						frame->video_code);
>>> +	picture_aspect = mode->picture_aspect_ratio;
>>> +	if (picture_aspect == HDMI_PICTURE_ASPECT_NONE)
>>> +		picture_aspect = drm_get_cea_aspect_ratio(frame->video_code);
>> This is slightly going in the loop.
>> - During the modeset the driver cant specify the aspect ratio
>> information, as DRM layer lacks this support.
>> - So we fill the VIC field, by comparing the mode with the
>> DRM_CEA_MODES[] list. This will pick the first mode
>>     available in the list (regardless of its aspect ratio), and fill the
>> VIC, as we don't consider aspect ratio while comparing timings.
>> - Again, now while sending the aspect ratio, we are picking up the VIC,
>> which may not be correct.
>>
>> So if we have 720x480(4:3) and 720x480(16:9) in the list, as 4:3 is
>> first in list, we will always pick 4:3 aspect ratio.
> Yes. The user didn't care about the aspect ratio (or rather couldn't
> specify one) so we just pick one. Which is exactly what we've been
> doing ever since we started sending the VIC in the infoframe.
Correct, and we are hoping that this should be better (if not fixed) 
with the aspect ratio support
patches + DRM cap. If the userspace doesn't set the cap, then anyways 
there is no aspect ratio
field available, and VIC would be always 0, as this becomes a Non CEA mode.

Or do you think it would be a better idea to send some VIC instead of No 
VIC, when userspace doesn't
set the DRM cap for aspect ratio ?

- Shashank
>> - Shashank
>>>    
>>> +	/*
>>> +	 * The infoframe can't convey anything but none, 4:3
>>> +	 * and 16:9, so if the user has asked for anything else
>>> +	 * we can only satisfy it by specifying the right VIC.
>>> +	 */
>>> +	if (picture_aspect > HDMI_PICTURE_ASPECT_16_9) {
>>> +		if (picture_aspect !=
>>> +		    drm_get_cea_aspect_ratio(frame->video_code))
>>> +			return -EINVAL;
>>> +		picture_aspect = HDMI_PICTURE_ASPECT_NONE;
>>> +	}
>>> +
>>> +	frame->picture_aspect = picture_aspect;
>>>    	frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
>>>    	frame->scan_mode = HDMI_SCAN_MODE_UNDERSCAN;
>>>