Re: [PATCH 06/10] drm/edid: Fix cea mode aspect ratio handling

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 24, 2017 at 02:26:09PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 11/17/2017 6:19 PM, Ville Syrjälä wrote:
> > On Fri, Nov 17, 2017 at 05:50:11PM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 11/17/2017 5:05 PM, Ville Syrjälä wrote:
> >>> On Fri, Nov 17, 2017 at 08:49:49AM +0530, Sharma, Shashank wrote:
> >>>> Regards
> >>>>
> >>>> Shashank
> >>>>
> >>>>
> >>>> On 11/16/2017 9:53 PM, Ville Syrjälä wrote:
> >>>>> On Thu, Nov 16, 2017 at 08:21:44PM +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>
> >>>>>>>
> >>>>>>> commit 6dffd431e229 ("drm: Add aspect ratio parsing in DRM layer")
> >>>>>>> cause us to not send out any VICs in the AVI infoframes. That commit
> >>>>>>> was since reverted, but if and when we add aspect ratio handing back
> >>>>>>> we need to be more careful.
> >>>>>>>
> >>>>>>> Let's handle this by considering the aspect ratio as a requirement
> >>>>>>> for cea mode matching only if the passed in mode actually has a
> >>>>>>> non-zero aspect ratio field. This will keep userspace that doesn't
> >>>>>>> provide an aspect ratio working as before by matching it to the
> >>>>>>> first otherwise equal cea mode. And once userspace starts to
> >>>>>>> provide the aspect ratio it will be considerd a hard requirement
> >>>>>>> for the match.
> >>>>>>>
> >>>>>>> Also change the hdmi mode matching to use drm_mode_match() for
> >>>>>>> consistency, but we don't match on aspect ratio there since the
> >>>>>>> spec doesn't list a specific aspect ratio for those modes.
> >>>>>>>
> >>>>>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
> >>>>>>> Cc: "Lin, Jia" <lin.a.jia@intel.com>
> >>>>>>> Cc: Akashdeep Sharma <akashdeep.sharma@intel.com>
> >>>>>>> Cc: Jim Bride <jim.bride@linux.intel.com>
> >>>>>>> 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 | 18 ++++++++++++++----
> >>>>>>>      1 file changed, 14 insertions(+), 4 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >>>>>>> index 7220b8f9a7e8..00aa98f3e55d 100644
> >>>>>>> --- a/drivers/gpu/drm/drm_edid.c
> >>>>>>> +++ b/drivers/gpu/drm/drm_edid.c
> >>>>>>> @@ -2903,11 +2903,15 @@ cea_mode_alternate_timings(u8 vic, struct drm_display_mode *mode)
> >>>>>>>      static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_match,
> >>>>>>>      					     unsigned int clock_tolerance)
> >>>>>>>      {
> >>>>>>> +	unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | DRM_MODE_MATCH_FLAGS;
> >>>>>>>      	u8 vic;
> >>>>>>>      
> >>>>>>>      	if (!to_match->clock)
> >>>>>>>      		return 0;
> >>>>>>>      
> >>>>>>> +	if (to_match->picture_aspect_ratio)
> >>>>>>> +		match_flags |= DRM_MODE_MATCH_ASPECT_RATIO;
> >>>>>> This doesn't look right. This means we are expecting a CEA mode without
> >>>>>> a pic aspect ratio field,
> >>>>>> which is invalid.
> >>>>> No, it's perfectly valid. It's what we currently get from userspace.
> >>>> Yep, but that's due to missing Aspect ratio handling in the DRM layer.
> >>>> If that's fixed, as per the list of CEA modes,
> >>>> each CEA VIC contains an aspect ratio, which is a part of its unique
> >>>> identity.
> >>>>
> >>>> I guess once we have the aspect ratio handling in DRM layer, it
> >>>> would/should look like this:
> >>>> - EDID gives you all supported modes, including CEA modes with Aspect ratio
> >>>> - Userspcae gets the mode information, with aspect ratio (for CEA modes)
> >>>> If ( Userspace picks one of the CEA modes)
> >>>>        - sends a modeset
> >>>>        - we find a matching CEA VIC, found one from modedb
> >>>>        - we load this VIC = nonzero information in AVI IF VIC field,
> >>>> else
> >>>>        - sends a modeset
> >>>>        - we could not find a matching CEA VIC, as aspect ratio is 0
> >>>>        - we make VIC field in AVI IF as 0
> >>> No. That would break current userspace.
> >> I guess I forgot to make it clear, that userspace will set the cap, only
> >> then we will provide aspect ratio information.
> >> So this should not break userspace, isn't it ?
> >>>> This is important, as HDMI compliance test 7-27 inspects if the VIC
> >>>> field in the AVI IF is accurate.
> >>> Complicance is secondary to not breaking things that work. Also I find
> >>> it hard to see what purpose there is in having a complicance test that
> >>> sets a CEA modes w/o aspect ratio and then expects the infoframe to have
> >>> VIC 0.
> >> Again, typically this is how these analyzers force modeset:
> >> - They send EDID with only one mode, which is the test mode, with aspect
> >> ratio.
> >> - They expect that VIC to be present in AVI IF
> >>>> - Shashank
> >>>>>> Ankit is going to publish the aspect ratio patch
> >>>>>> series again, with proper DRM cap and flags check. Would it be
> >>>>>> ok if we have a look that handling first ?
> >>>>> This patch will be needed by that work. Otherwise we're going to stop
> >>>>> sending a VIC for CEA modes with current userspace.
> >>>> I guess we should force userspaces to start bothering about aspect ratio
> >>>> field, right now we
> >>>> are doing this for Wayland based compositors, may be we should extend it
> >>>> to X based too.
> >>> Yes, I've been saying that someone should look into extending the randr
> >>> protocol with the necessary bits. But that still doesn't allow us to
> >>> change the current behaviour as old userspace would anyway linger around
> >>> for a long time.
> >> I think cap will cove this part
> > The cap is irrelevant to this discussion. It will not be set by old
> > userspace, hence it won't change anything for old userspace. Modes
> > coming from old userspace will still have aspect_ratio=0.
> Agree, and in that case, why bother about aspect ratio flags here in 
> this series ? Probably let this be taken care in aspect ratio series.

If someone wants to suck these patches into the aspect ratio series that
fine by me.

I had them here because they were on the same branch where I had the
fix for the aspect ratio in the infoframe. And I was too lazy to
remove them from the series, figuring they're semi-related anyway.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel