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

From: "Sharma, Shashank" <shashank.sharma@intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Jose Abreu <Jose.Abreu@synopsys.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	intel-gfx@lists.freedesktop.org,
	Emil Velikov <emil.l.velikov@gmail.com>,
	dri-devel@lists.freedesktop.org,
	Nautiyal Ankit <ankit.k.nautiyal@intel.com>
Subject: Re: [PATCH 07/10] drm/edid: Don't send bogus aspect ratios in AVI infoframes
Date: Fri, 24 Nov 2017 14:25:02 +0530	[thread overview]
Message-ID: <17e894d1-2be2-549c-4b71-2062af1cccf6@intel.com> (raw)
In-Reply-To: <20171117113857.GD10981@intel.com>

Regards

Shashank


On 11/17/2017 5:08 PM, Ville Syrjälä wrote:
> On Fri, Nov 17, 2017 at 08:53:54AM +0530, Sharma, Shashank wrote:
>> 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@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@intel.com>
>>>>> Cc: Sean Paul <seanpaul@chromium.org>
>>>>> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@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 ?
> Yes. That's the current behaviour.
>
> IIRC some crappy amplifiers etc. with HDMI passthrough don't even work
> correctly unless VIC is specified. Hence we do want to send it whenever
> possible.
Ok, in this case this make sense, and then I kindof agree with what this 
patch is trying to do.
Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
>> - 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;
>>>>>     

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