| Message ID | 1518697262-3001-9-git-send-email-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 15, 2018 at 05:51:01PM +0530, Nautiyal, Ankit K wrote: > From: "Sharma, Shashank" <shashank.sharma@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@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> (V2) > Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com> (V4) > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Jim Bride <jim.bride@linux.intel.com> > Cc: Jose Abreu <Jose.Abreu@synopsys.com> > Cc: Ankit Nautiyal <ankit.k.nautiyal@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. > } > 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? > + > + 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. > + 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
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@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@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> (V2) >> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com> (V4) >> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Jim Bride <jim.bride@linux.intel.com> >> Cc: Jose Abreu <Jose.Abreu@synopsys.com> >> Cc: Ankit Nautiyal <ankit.k.nautiyal@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? > >> } >> 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
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@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@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> (V2) > >> Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com> (V4) > >> > >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > >> Cc: Jim Bride <jim.bride@linux.intel.com> > >> Cc: Jose Abreu <Jose.Abreu@synopsys.com> > >> Cc: Ankit Nautiyal <ankit.k.nautiyal@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 >
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); } 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; + + 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 */ + 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;