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

[Ville Syrjälä]

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

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.

> 
> - 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;
> >>>    

-- 
Ville Syrjälä
Intel OTC