[PATCH v5 8/9] drm: Add aspect ratio parsing in DRM layer

Mon Feb 26 15:57:18 UTC 2018

On Mon, Feb 26, 2018 at 09:18:57PM +0530, Nautiyal, Ankit K wrote:
> 
> 
> On 2/23/2018 8:24 PM, Ville Syrjälä wrote:
> > On Thu, Feb 15, 2018 at 05:51:01PM +0530, Nautiyal, Ankit K wrote:
> >> From: "Sharma, Shashank" <shashank.sharma at intel.com>
> >>
> >> 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).
> >>
> >> Background:
> >> This patch was once reviewed and merged, and later reverted due to
> >> lack of DRM cap protection. This is a re-spin of this patch, this
> >> time with DRM cap protection, to avoid aspect ratio information, when
> >> the client doesn't request for it.
> >>
> >> Review link: https://pw-emeril.freedesktop.org/patch/104068/
> >> Background discussion: https://patchwork.kernel.org/patch/9379057/
> >>
> >> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
> >> Signed-off-by: Lin, Jia <lin.a.jia at intel.com>
> >> Signed-off-by: Akashdeep Sharma <akashdeep.sharma at intel.com>
> >> Reviewed-by: Jim Bride <jim.bride at linux.intel.com> (V2)
> >> Reviewed-by: Jose Abreu <Jose.Abreu at synopsys.com> (V4)
> >>
> >> Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> >> Cc: Jim Bride <jim.bride at linux.intel.com>
> >> Cc: Jose Abreu <Jose.Abreu at synopsys.com>
> >> Cc: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
> >>
> >> V3: modified the aspect-ratio check in drm_mode_equal as per new flags
> >>      provided by Ville. https://patchwork.freedesktop.org/patch/188043/
> >> V4: rebase
> >> V5: rebase
> >> ---
> >>   drivers/gpu/drm/drm_modes.c | 33 ++++++++++++++++++++++++++++++++-
> >>   1 file changed, 32 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> >> index ca4c5cc..25140b9 100644
> >> --- a/drivers/gpu/drm/drm_modes.c
> >> +++ b/drivers/gpu/drm/drm_modes.c
> >> @@ -1050,7 +1050,8 @@ bool drm_mode_equal(const struct drm_display_mode *mode1,
> >>   			      DRM_MODE_MATCH_TIMINGS |
> >>   			      DRM_MODE_MATCH_CLOCK |
> >>   			      DRM_MODE_MATCH_FLAGS |
> >> -			      DRM_MODE_MATCH_3D_FLAGS);
> >> +			      DRM_MODE_MATCH_3D_FLAGS|
> >> +			      DRM_MODE_MATCH_ASPECT_RATIO);
> > Hmm. I wonder if all the users are expecting this. Based on a cursory
> > examination drm_fb_helper might want to ignore the aspect ratio since
> > it's just looking to clone the same mode on multiple outputs. The other
> > cases don't look to me like they should suffer badly from this.
> 
> Agreed.
> Need to add a function drm_mode_equal_no_aspect_ratio() in drm_modes.c 
> which will compare
> every field, other than the aspect-ratio.
> Next, in the drm_target_cloned( ), where same modes are to be cloned on 
> multiple outputs, instead
> of drm_mode_equal( ), drm_mode_equal_no_aspect_ratio( ) must be called.
> 
> Is this approach correct?

Please no more drm_mode_equal_no_whatever(). We should nuke the current
ones. If we think the no_whatever pattern is useful we should probably
add something like drm_mode_match_except() which takes the set of flags
we're going to ignore. Not 100% convinced it's a good pattern to follow
though. Just stating explicitly what we want to match seems generally
better to me.

> 
> >
> >>   }
> >>   EXPORT_SYMBOL(drm_mode_equal);
> >>   
> >> @@ -1649,6 +1650,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;
> > This looks redundant. The internal mode should not have the aspect ratio
> > flags set. Or are we changing that?
> 
> Yes right, the aspect-ratio flags are never set in the internal-mode flags.
> This line can be dropped.
> 
> >
> >> +
> >> +	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;
> >>   }
> >> @@ -1693,6 +1709,21 @@ int drm_mode_convert_umode(struct drm_device *dev,
> >>   	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 */
> > What the code is doing is obvious so this comment is redundant.
> > The non-obvious part (ie. the "why?") is what the comment
> > should contain.
> 
> Thanks for pointing that out, will take care of this in future patches.
> 
> Regards,
> Ankit
> 
> >
> >> +	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;
> >> +	}
> >> +
> >>   	out->status = drm_mode_validate_driver(dev, out);
> >>   	if (out->status != MODE_OK)
> >>   		goto out;
> >> -- 
> >> 2.7.4
> 

-- 
Ville Syrjälä
Intel OTC