Re: [PATCH v4 2/4] drm: Add aspect ratio parsing in DRM layer

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

On Mon, Oct 17, 2016 at 08:21:21PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 10/17/2016 7:36 PM, Ville Syrjälä wrote:
> > On Mon, Oct 17, 2016 at 07:10:42PM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 10/17/2016 6:01 PM, Ville Syrjälä wrote:
> >>> On Mon, Oct 17, 2016 at 05:34:38PM +0530, Shashank Sharma wrote:
> >>>> Current DRM layer functions don't parse aspect ratio information
> >>>> while converting a user mode->kernel mode or vice versa. This
> >>>> causes modeset to pick mode with wrong aspect ratio, eventually
> >>>> causing failures in HDMI compliance test cases, due to wrong VIC.
> >>>>
> >>>> This patch adds aspect ratio information in DRM's mode conversion
> >>>> and mode comparision functions, to make sure kernel picks mode
> >>>> with right aspect ratio (as per the VIC).
> >>>>
> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>>> Signed-off-by: Lin, Jia <lin.a.jia@intel.com>
> >>>> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@intel.com>
> >>>> Reviewed-by: Jim Bride <jim.bride@linux.intel.com>
> >>>> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com>
> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> >>>>
> >>>> V2: Addressed review comments from Sean:
> >>>> - Fix spellings/typo
> >>>> - No need to handle aspect ratio none
> >>>> - Add a break, for default case too
> >>>> V3: Rebase
> >>>> V4: Added r-b from Jose
> >>>> ---
> >>>>    drivers/gpu/drm/drm_modes.c | 31 +++++++++++++++++++++++++++++++
> >>>>    1 file changed, 31 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> >>>> index 53f07ac..fde927a 100644
> >>>> --- a/drivers/gpu/drm/drm_modes.c
> >>>> +++ b/drivers/gpu/drm/drm_modes.c
> >>>> @@ -997,6 +997,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1,
> >>>>    	    mode1->vsync_end == mode2->vsync_end &&
> >>>>    	    mode1->vtotal == mode2->vtotal &&
> >>>>    	    mode1->vscan == mode2->vscan &&
> >>>> +	    mode1->picture_aspect_ratio == mode2->picture_aspect_ratio &&
> >>>>    	    (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) ==
> >>>>    	     (mode2->flags & ~DRM_MODE_FLAG_3D_MASK))
> >>>>    		return true;
> >>>> @@ -1499,6 +1500,21 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out,
> >>>>    	out->vrefresh = in->vrefresh;
> >>>>    	out->flags = in->flags;
> >>>>    	out->type = in->type;
> >>>> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
> >>>> +
> >>>> +	switch (in->picture_aspect_ratio) {
> >>>> +	case HDMI_PICTURE_ASPECT_4_3:
> >>>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_4_3;
> >>>> +		break;
> >>>> +	case HDMI_PICTURE_ASPECT_16_9:
> >>>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_16_9;
> >>>> +		break;
> >>>> +	case HDMI_PICTURE_ASPECT_RESERVED:
> >>>> +	default:
> >>>> +		out->flags |= DRM_MODE_FLAG_PIC_AR_NONE;
> >>>> +		break;
> >>>> +	}
> >>>> +
> >>>>    	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
> >>>>    	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
> >>>>    }
> >>>> @@ -1544,6 +1560,21 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
> >>>>    	strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN);
> >>>>    	out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
> >>>>    
> >>>> +	/* Clearing picture aspect ratio bits from out flags */
> >>>> +	out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
> >>>> +
> >>>> +	switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
> >>>> +	case DRM_MODE_FLAG_PIC_AR_4_3:
> >>>> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3;
> >>>> +		break;
> >>>> +	case DRM_MODE_FLAG_PIC_AR_16_9:
> >>>> +		out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9;
> >>>> +		break;
> >>>> +	default:
> >>>> +		out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >>>> +		break;
> >>>> +	}
> >>> Hmm. So now we have the mode aspect ratio infromation duplicated
> >>> in two different places. Not sure that won't lead to some confusion.
> >> In drm layer, this is the only place. Actually till now, DRM layer dint
> >> have the support for aspect ratio at all. This was causing
> >> HDMI compliance tests like 7-27 fail, which expect a particular unique
> >> VIC in AVI infoframe on modeset.
> >>
> >> I have given some details about this in cover-letter.
> >>> Although we do want the user to be able to override via the property I
> >>> suppose, so we'd have to change that (+ the inforframe code) to
> >>> look at the mode flags instead if we would eliminate
> >>> 'picture_aspect_ratio'.
> >> I had a small discussion on this with Daniel, and we discussed that it
> >> doesnt make sense to override just the aspect ratio if the monitor
> >> doesn't even support that aspect ratio.
> >> And currently the was how this property is implemented is, we just
> >> override the aspect ratio without any validity check.
> >>
> >> Now as we have all the supported aspect ratio added properly in the mode
> >> info itself, we need not to have this property at all, So Daniel
> >> suggested me to remove that property too.
> >>> And that brings me to the other point. At least i915 will simply
> >>> override the mode's aspect ration with the property. So this looks like
> >>> a big no-op for now on i915.
> >> Yes, This is a bug in I915. When I published the first version of this
> >> series, I had one patch, which was overriding the value only when the
> >> property is set.
> >> This should be the right case. And then Daniel suggested to remove the
> >> property all together (and it makes sense as we have proper aspect
> >> ratios in mode information
> >> itself) So I kept that patch separate, to be merged separately.
> > Yeah, removing the property entirely seems like an OKish solution since
> > userspace can just stuff that information into the mode flags instead.
> > The only slight concern is that someone's setup might depend on the
> > property, and now we're removing it.
> Thanks, will send the patch for it soon, adding you in the mail chain.
> >>>    It's looking like we might need a new
> >>> propetty value to differentiate between "auto" and "none" for real.
> >> Now we have the exact aspect ratio given by monitor EDID, so IMHO it
> >> would be better if we can just have real aspect.
> > Well, "real" isn't quite the right term. It's still a user specified
> > thing, which means the user can ask for somehting the display didn't
> > even advertise. Which is fine as such, however since the AVI infoframe
> > lacks the capability to transmit these new aspect ratios we have a bit
> > of problem on our hands if the user asks for something we can't even
> > tell the display to do. I guess we would just need to return an error
> > to the user in that case.
> The current implementation which we have in andoroid, is hardware 
> composer shows the whole mode in the list like:
> - 1920x1080@60 16:9    /* From CEA block of EDID */
> - 1920x1080@60 0:0        /* From detailed descriptor block of EDID */
> - 1280x720@60 4:3          /* From CEA block of EDID */
> -etc
> These mode is the connector->probed_modes which we populated while 
> parsing the EDID.
> Now, when user selects the mode, its a complete set of resolution + 
> refresh rate + aspect ratio, so user can only pick what
> we are showing to it. In this case, there is no option which we cant 
> support.
> 
> Do you think that there would be some other model of use case, where we 
> can mix something among these?

You can't assume the user just picks the mode off the list. They can
stuff whatever random garbage into the mode if they wish.

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