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@xxxxxxxxx> > >> > >> 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@xxxxxxxxx> > >> Signed-off-by: Lin, Jia <lin.a.jia@xxxxxxxxx> > >> Signed-off-by: Akashdeep Sharma <akashdeep.sharma@xxxxxxxxx> > >> Reviewed-by: Jim Bride <jim.bride@xxxxxxxxxxxxxxx> (V2) > >> Reviewed-by: Jose Abreu <Jose.Abreu@xxxxxxxxxxxx> (V4) > >> > >> Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> > >> Cc: Jim Bride <jim.bride@xxxxxxxxxxxxxxx> > >> Cc: Jose Abreu <Jose.Abreu@xxxxxxxxxxxx> > >> Cc: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx> > >> > >> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
