* [PATCH v3 0/8] Aspect ratio support in DRM layer @ 2018-01-12 6:21 Nautiyal, Ankit K 2018-01-12 6:21 ` [PATCH v3 1/8] drm/modes: Introduce drm_mode_match() Nautiyal, Ankit K ` (7 more replies) 0 siblings, 8 replies; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-01-12 6:21 UTC (permalink / raw) To: dri-devel; +Cc: Ankit Nautiyal From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> This patch series is a re-attempt to enable aspect ratio support in DRM layer. Currently the aspect ratio information gets lost in translation during a user->kernel mode or vice versa. The old patch series (https://pw-emeril.freedesktop.org/series/10850/) had 4 patches, out of which 2 patches were reverted due to lack of drm client protection while loading the aspect information. This patch series also includes 3 patches from Ville Syrjälä's series for 'Info-frame cleanup and fixes' https://patchwork.freedesktop.org/series/33730/ which fixes the mode matching mechanism via flags. This patch series, adds a DRM client option for aspect ratio, and loads aspect ratio flags, only when the client sets this cap. Also, to test this patch series there is a userspace implementation by Ankit Nautiyal in Wayland/weston layer: https://patchwork.freedesktop.org/patch/188125/ (Which is already ACK'ed by wayland community.) This, helps us in passing HDMI compliance test cases like 7-27, where the test equipment applies a CEA mode, and expects the exact VIC in the AVI infoframes. Ankit Nautiyal (3): drm: Add DRM client cap for aspect-ratio drm: Handle aspect ratio info in atomic and legacy modeset paths drm: Expose modes with aspect ratio, only if requested Sharma, Shashank (2): drm: Add aspect ratio parsing in DRM layer drm: Add and handle new aspect ratios in DRM layer Ville Syrjälä (3): drm/modes: Introduce drm_mode_match() drm/edid: Use drm_mode_match_no_clocks_no_stereo() for consistentcy drm/edid: Fix cea mode aspect ratio handling drivers/gpu/drm/drm_atomic.c | 61 +++++++++++++- drivers/gpu/drm/drm_connector.c | 31 ++++++- drivers/gpu/drm/drm_crtc.c | 19 +++++ drivers/gpu/drm/drm_edid.c | 18 +++- drivers/gpu/drm/drm_ioctl.c | 5 ++ drivers/gpu/drm/drm_modes.c | 177 +++++++++++++++++++++++++++++++++------- include/drm/drm_atomic.h | 2 + include/drm/drm_file.h | 7 ++ include/drm/drm_modes.h | 9 ++ include/uapi/drm/drm.h | 7 ++ include/uapi/drm/drm_mode.h | 6 ++ 11 files changed, 300 insertions(+), 42 deletions(-) -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 1/8] drm/modes: Introduce drm_mode_match() 2018-01-12 6:21 [PATCH v3 0/8] Aspect ratio support in DRM layer Nautiyal, Ankit K @ 2018-01-12 6:21 ` Nautiyal, Ankit K 2018-01-17 8:41 ` Sharma, Shashank 2018-01-12 6:21 ` [PATCH v3 2/8] drm/edid: Use drm_mode_match_no_clocks_no_stereo() for consistentcy Nautiyal, Ankit K ` (6 subsequent siblings) 7 siblings, 1 reply; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-01-12 6:21 UTC (permalink / raw) To: dri-devel From: Ville Syrjälä <ville.syrjala@linux.intel.com> Make mode matching less confusing by allowing the caller to specify which parts of the modes should match via some flags. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/drm_modes.c | 134 ++++++++++++++++++++++++++++++++++---------- include/drm/drm_modes.h | 9 +++ 2 files changed, 112 insertions(+), 31 deletions(-) diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 4a3f68a..8128dbb 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -940,17 +940,68 @@ struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev, } EXPORT_SYMBOL(drm_mode_duplicate); +static bool drm_mode_match_timings(const struct drm_display_mode *mode1, + const struct drm_display_mode *mode2) +{ + return mode1->hdisplay == mode2->hdisplay && + mode1->hsync_start == mode2->hsync_start && + mode1->hsync_end == mode2->hsync_end && + mode1->htotal == mode2->htotal && + mode1->hskew == mode2->hskew && + mode1->vdisplay == mode2->vdisplay && + mode1->vsync_start == mode2->vsync_start && + mode1->vsync_end == mode2->vsync_end && + mode1->vtotal == mode2->vtotal && + mode1->vscan == mode2->vscan; +} + +static bool drm_mode_match_clock(const struct drm_display_mode *mode1, + const struct drm_display_mode *mode2) +{ + /* + * do clock check convert to PICOS + * so fb modes get matched the same + */ + if (mode1->clock && mode2->clock) + return KHZ2PICOS(mode1->clock) == KHZ2PICOS(mode2->clock); + else + return mode1->clock == mode2->clock; +} + +static bool drm_mode_match_flags(const struct drm_display_mode *mode1, + const struct drm_display_mode *mode2) +{ + return (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) == + (mode2->flags & ~DRM_MODE_FLAG_3D_MASK); +} + +static bool drm_mode_match_3d_flags(const struct drm_display_mode *mode1, + const struct drm_display_mode *mode2) +{ + return (mode1->flags & DRM_MODE_FLAG_3D_MASK) == + (mode2->flags & DRM_MODE_FLAG_3D_MASK); +} + +static bool drm_mode_match_aspect_ratio(const struct drm_display_mode *mode1, + const struct drm_display_mode *mode2) +{ + return mode1->picture_aspect_ratio == mode2->picture_aspect_ratio; +} + /** - * drm_mode_equal - test modes for equality + * drm_mode_match - test modes for (partial) equality * @mode1: first mode * @mode2: second mode + * @match_flags: which parts need to match (DRM_MODE_MATCH_*) * * Check to see if @mode1 and @mode2 are equivalent. * * Returns: - * True if the modes are equal, false otherwise. + * True if the modes are (partially) equal, false otherwise. */ -bool drm_mode_equal(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2) +bool drm_mode_match(const struct drm_display_mode *mode1, + const struct drm_display_mode *mode2, + unsigned int match_flags) { if (!mode1 && !mode2) return true; @@ -958,15 +1009,48 @@ bool drm_mode_equal(const struct drm_display_mode *mode1, const struct drm_displ if (!mode1 || !mode2) return false; - /* do clock check convert to PICOS so fb modes get matched - * the same */ - if (mode1->clock && mode2->clock) { - if (KHZ2PICOS(mode1->clock) != KHZ2PICOS(mode2->clock)) - return false; - } else if (mode1->clock != mode2->clock) + if (match_flags & DRM_MODE_MATCH_TIMINGS && + !drm_mode_match_timings(mode1, mode2)) + return false; + + if (match_flags & DRM_MODE_MATCH_CLOCK && + !drm_mode_match_clock(mode1, mode2)) + return false; + + if (match_flags & DRM_MODE_MATCH_FLAGS && + !drm_mode_match_flags(mode1, mode2)) + return false; + + if (match_flags & DRM_MODE_MATCH_3D_FLAGS && + !drm_mode_match_3d_flags(mode1, mode2)) return false; - return drm_mode_equal_no_clocks(mode1, mode2); + if (match_flags & DRM_MODE_MATCH_ASPECT_RATIO && + !drm_mode_match_aspect_ratio(mode1, mode2)) + return false; + + return true; +} +EXPORT_SYMBOL(drm_mode_match); + +/** + * drm_mode_equal - test modes for equality + * @mode1: first mode + * @mode2: second mode + * + * Check to see if @mode1 and @mode2 are equivalent. + * + * Returns: + * True if the modes are equal, false otherwise. + */ +bool drm_mode_equal(const struct drm_display_mode *mode1, + const struct drm_display_mode *mode2) +{ + return drm_mode_match(mode1, mode2, + DRM_MODE_MATCH_TIMINGS | + DRM_MODE_MATCH_CLOCK | + DRM_MODE_MATCH_FLAGS | + DRM_MODE_MATCH_3D_FLAGS); } EXPORT_SYMBOL(drm_mode_equal); @@ -981,13 +1065,13 @@ EXPORT_SYMBOL(drm_mode_equal); * Returns: * True if the modes are equal, false otherwise. */ -bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2) +bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, + const struct drm_display_mode *mode2) { - if ((mode1->flags & DRM_MODE_FLAG_3D_MASK) != - (mode2->flags & DRM_MODE_FLAG_3D_MASK)) - return false; - - return drm_mode_equal_no_clocks_no_stereo(mode1, mode2); + return drm_mode_match(mode1, mode2, + DRM_MODE_MATCH_TIMINGS | + DRM_MODE_MATCH_FLAGS | + DRM_MODE_MATCH_3D_FLAGS); } EXPORT_SYMBOL(drm_mode_equal_no_clocks); @@ -1005,21 +1089,9 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks); bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2) { - if (mode1->hdisplay == mode2->hdisplay && - mode1->hsync_start == mode2->hsync_start && - mode1->hsync_end == mode2->hsync_end && - mode1->htotal == mode2->htotal && - mode1->hskew == mode2->hskew && - mode1->vdisplay == mode2->vdisplay && - mode1->vsync_start == mode2->vsync_start && - mode1->vsync_end == mode2->vsync_end && - mode1->vtotal == mode2->vtotal && - mode1->vscan == mode2->vscan && - (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) == - (mode2->flags & ~DRM_MODE_FLAG_3D_MASK)) - return true; - - return false; + return drm_mode_match(mode1, mode2, + DRM_MODE_MATCH_TIMINGS | + DRM_MODE_MATCH_FLAGS); } EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo); diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index 9f3421c..839eb9c 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -150,6 +150,12 @@ enum drm_mode_status { #define DRM_MODE_FLAG_3D_MAX DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF +#define DRM_MODE_MATCH_TIMINGS (1 << 0) +#define DRM_MODE_MATCH_CLOCK (1 << 1) +#define DRM_MODE_MATCH_FLAGS (1 << 2) +#define DRM_MODE_MATCH_3D_FLAGS (1 << 3) +#define DRM_MODE_MATCH_ASPECT_RATIO (1 << 4) + /** * struct drm_display_mode - DRM kernel-internal display mode structure * @hdisplay: horizontal display size @@ -493,6 +499,9 @@ void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode *src); struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev, const struct drm_display_mode *mode); +bool drm_mode_match(const struct drm_display_mode *mode1, + const struct drm_display_mode *mode2, + unsigned int match_flags); bool drm_mode_equal(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2); bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v3 1/8] drm/modes: Introduce drm_mode_match() 2018-01-12 6:21 ` [PATCH v3 1/8] drm/modes: Introduce drm_mode_match() Nautiyal, Ankit K @ 2018-01-17 8:41 ` Sharma, Shashank 2018-01-17 15:21 ` Ville Syrjälä 0 siblings, 1 reply; 35+ messages in thread From: Sharma, Shashank @ 2018-01-17 8:41 UTC (permalink / raw) To: Nautiyal, Ankit K, dri-devel On 1/12/2018 11:51 AM, Nautiyal, Ankit K wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Make mode matching less confusing by allowing the caller to specify > which parts of the modes should match via some flags. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_modes.c | 134 ++++++++++++++++++++++++++++++++++---------- > include/drm/drm_modes.h | 9 +++ > 2 files changed, 112 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > index 4a3f68a..8128dbb 100644 > --- a/drivers/gpu/drm/drm_modes.c > +++ b/drivers/gpu/drm/drm_modes.c > @@ -940,17 +940,68 @@ struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev, > } > EXPORT_SYMBOL(drm_mode_duplicate); > > +static bool drm_mode_match_timings(const struct drm_display_mode *mode1, > + const struct drm_display_mode *mode2) Shouldn't the second parameter be aligned to next char after the opening bracket in above line (instead of being align to the bracket) ? I have this same comment for most of the functions in this patch. > +{ > + return mode1->hdisplay == mode2->hdisplay && > + mode1->hsync_start == mode2->hsync_start && > + mode1->hsync_end == mode2->hsync_end && > + mode1->htotal == mode2->htotal && > + mode1->hskew == mode2->hskew && > + mode1->vdisplay == mode2->vdisplay && > + mode1->vsync_start == mode2->vsync_start && > + mode1->vsync_end == mode2->vsync_end && > + mode1->vtotal == mode2->vtotal && > + mode1->vscan == mode2->vscan; > +} > + > +static bool drm_mode_match_clock(const struct drm_display_mode *mode1, > + const struct drm_display_mode *mode2) > +{ > + /* > + * do clock check convert to PICOS > + * so fb modes get matched the same > + */ > + if (mode1->clock && mode2->clock) > + return KHZ2PICOS(mode1->clock) == KHZ2PICOS(mode2->clock); > + else > + return mode1->clock == mode2->clock; > +} > + > +static bool drm_mode_match_flags(const struct drm_display_mode *mode1, Should we call it drm_mode_match_flag_no_stereo ? > + const struct drm_display_mode *mode2) > +{ > + return (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) == > + (mode2->flags & ~DRM_MODE_FLAG_3D_MASK); > +} > + > +static bool drm_mode_match_3d_flags(const struct drm_display_mode *mode1, > + const struct drm_display_mode *mode2) > +{ > + return (mode1->flags & DRM_MODE_FLAG_3D_MASK) == > + (mode2->flags & DRM_MODE_FLAG_3D_MASK); > +} > + > +static bool drm_mode_match_aspect_ratio(const struct drm_display_mode *mode1, > + const struct drm_display_mode *mode2) > +{ > + return mode1->picture_aspect_ratio == mode2->picture_aspect_ratio; > +} > + > /** > - * drm_mode_equal - test modes for equality > + * drm_mode_match - test modes for (partial) equality > * @mode1: first mode > * @mode2: second mode > + * @match_flags: which parts need to match (DRM_MODE_MATCH_*) > * > * Check to see if @mode1 and @mode2 are equivalent. > * > * Returns: > - * True if the modes are equal, false otherwise. > + * True if the modes are (partially) equal, false otherwise. > */ > -bool drm_mode_equal(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2) > +bool drm_mode_match(const struct drm_display_mode *mode1, > + const struct drm_display_mode *mode2, > + unsigned int match_flags) > { > if (!mode1 && !mode2) > return true; > @@ -958,15 +1009,48 @@ bool drm_mode_equal(const struct drm_display_mode *mode1, const struct drm_displ > if (!mode1 || !mode2) > return false; > > - /* do clock check convert to PICOS so fb modes get matched > - * the same */ > - if (mode1->clock && mode2->clock) { > - if (KHZ2PICOS(mode1->clock) != KHZ2PICOS(mode2->clock)) > - return false; > - } else if (mode1->clock != mode2->clock) > + if (match_flags & DRM_MODE_MATCH_TIMINGS && > + !drm_mode_match_timings(mode1, mode2)) > + return false; > + > + if (match_flags & DRM_MODE_MATCH_CLOCK && > + !drm_mode_match_clock(mode1, mode2)) > + return false; > + > + if (match_flags & DRM_MODE_MATCH_FLAGS && > + !drm_mode_match_flags(mode1, mode2)) > + return false; > + > + if (match_flags & DRM_MODE_MATCH_3D_FLAGS && > + !drm_mode_match_3d_flags(mode1, mode2)) > return false; > > - return drm_mode_equal_no_clocks(mode1, mode2); > + if (match_flags & DRM_MODE_MATCH_ASPECT_RATIO && > + !drm_mode_match_aspect_ratio(mode1, mode2)) > + return false; > + > + return true; > +} > +EXPORT_SYMBOL(drm_mode_match); > + > +/** > + * drm_mode_equal - test modes for equality > + * @mode1: first mode > + * @mode2: second mode > + * > + * Check to see if @mode1 and @mode2 are equivalent. > + * > + * Returns: > + * True if the modes are equal, false otherwise. > + */ > +bool drm_mode_equal(const struct drm_display_mode *mode1, > + const struct drm_display_mode *mode2) > +{ > + return drm_mode_match(mode1, mode2, > + DRM_MODE_MATCH_TIMINGS | > + DRM_MODE_MATCH_CLOCK | > + DRM_MODE_MATCH_FLAGS | > + DRM_MODE_MATCH_3D_FLAGS); > } > EXPORT_SYMBOL(drm_mode_equal); > > @@ -981,13 +1065,13 @@ EXPORT_SYMBOL(drm_mode_equal); > * Returns: > * True if the modes are equal, false otherwise. > */ > -bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2) > +bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, > + const struct drm_display_mode *mode2) > { > - if ((mode1->flags & DRM_MODE_FLAG_3D_MASK) != > - (mode2->flags & DRM_MODE_FLAG_3D_MASK)) > - return false; > - > - return drm_mode_equal_no_clocks_no_stereo(mode1, mode2); > + return drm_mode_match(mode1, mode2, > + DRM_MODE_MATCH_TIMINGS | > + DRM_MODE_MATCH_FLAGS | > + DRM_MODE_MATCH_3D_FLAGS); > } > EXPORT_SYMBOL(drm_mode_equal_no_clocks); > > @@ -1005,21 +1089,9 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks); > bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1, > const struct drm_display_mode *mode2) > { > - if (mode1->hdisplay == mode2->hdisplay && > - mode1->hsync_start == mode2->hsync_start && > - mode1->hsync_end == mode2->hsync_end && > - mode1->htotal == mode2->htotal && > - mode1->hskew == mode2->hskew && > - mode1->vdisplay == mode2->vdisplay && > - mode1->vsync_start == mode2->vsync_start && > - mode1->vsync_end == mode2->vsync_end && > - mode1->vtotal == mode2->vtotal && > - mode1->vscan == mode2->vscan && > - (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) == > - (mode2->flags & ~DRM_MODE_FLAG_3D_MASK)) > - return true; > - > - return false; > + return drm_mode_match(mode1, mode2, > + DRM_MODE_MATCH_TIMINGS | > + DRM_MODE_MATCH_FLAGS); > } > EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo); > > diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h > index 9f3421c..839eb9c 100644 > --- a/include/drm/drm_modes.h > +++ b/include/drm/drm_modes.h > @@ -150,6 +150,12 @@ enum drm_mode_status { > > #define DRM_MODE_FLAG_3D_MAX DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF > > +#define DRM_MODE_MATCH_TIMINGS (1 << 0) > +#define DRM_MODE_MATCH_CLOCK (1 << 1) > +#define DRM_MODE_MATCH_FLAGS (1 << 2) > +#define DRM_MODE_MATCH_3D_FLAGS (1 << 3) > +#define DRM_MODE_MATCH_ASPECT_RATIO (1 << 4) > + > /** > * struct drm_display_mode - DRM kernel-internal display mode structure > * @hdisplay: horizontal display size > @@ -493,6 +499,9 @@ void drm_mode_copy(struct drm_display_mode *dst, > const struct drm_display_mode *src); > struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev, > const struct drm_display_mode *mode); > +bool drm_mode_match(const struct drm_display_mode *mode1, > + const struct drm_display_mode *mode2, Same here for the alignment > + unsigned int match_flags); > bool drm_mode_equal(const struct drm_display_mode *mode1, > const struct drm_display_mode *mode2); > bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, With the minor comments above, please feel free to use Reviewed-by: Shashank Sharma <shashank.sharma@intel.com> _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 1/8] drm/modes: Introduce drm_mode_match() 2018-01-17 8:41 ` Sharma, Shashank @ 2018-01-17 15:21 ` Ville Syrjälä 2018-01-18 6:10 ` Sharma, Shashank 0 siblings, 1 reply; 35+ messages in thread From: Ville Syrjälä @ 2018-01-17 15:21 UTC (permalink / raw) To: Sharma, Shashank; +Cc: Nautiyal, Ankit K, dri-devel On Wed, Jan 17, 2018 at 02:11:55PM +0530, Sharma, Shashank wrote: > > > On 1/12/2018 11:51 AM, Nautiyal, Ankit K wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Make mode matching less confusing by allowing the caller to specify > > which parts of the modes should match via some flags. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/drm_modes.c | 134 ++++++++++++++++++++++++++++++++++---------- > > include/drm/drm_modes.h | 9 +++ > > 2 files changed, 112 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > > index 4a3f68a..8128dbb 100644 > > --- a/drivers/gpu/drm/drm_modes.c > > +++ b/drivers/gpu/drm/drm_modes.c > > @@ -940,17 +940,68 @@ struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev, > > } > > EXPORT_SYMBOL(drm_mode_duplicate); > > > > +static bool drm_mode_match_timings(const struct drm_display_mode *mode1, > > + const struct drm_display_mode *mode2) > Shouldn't the second parameter be aligned to next char after the opening > bracket in above line (instead of being align to the bracket) ? It is. The extra junk at the start of the line is causing the alignment to look wrong when it's not because the tab stops are still aligned to the start of the line. This is one reason why I think that using tabs for alignment isn't a particularly great idea. > I have this same comment for most of the functions in this patch. > > +{ > > + return mode1->hdisplay == mode2->hdisplay && > > + mode1->hsync_start == mode2->hsync_start && > > + mode1->hsync_end == mode2->hsync_end && > > + mode1->htotal == mode2->htotal && > > + mode1->hskew == mode2->hskew && > > + mode1->vdisplay == mode2->vdisplay && > > + mode1->vsync_start == mode2->vsync_start && > > + mode1->vsync_end == mode2->vsync_end && > > + mode1->vtotal == mode2->vtotal && > > + mode1->vscan == mode2->vscan; > > +} > > + > > +static bool drm_mode_match_clock(const struct drm_display_mode *mode1, > > + const struct drm_display_mode *mode2) > > +{ > > + /* > > + * do clock check convert to PICOS > > + * so fb modes get matched the same > > + */ > > + if (mode1->clock && mode2->clock) > > + return KHZ2PICOS(mode1->clock) == KHZ2PICOS(mode2->clock); > > + else > > + return mode1->clock == mode2->clock; > > +} > > + > > +static bool drm_mode_match_flags(const struct drm_display_mode *mode1, > Should we call it drm_mode_match_flag_no_stereo ? It's not really "no stereo", rather it's "check all the flags not being checked by something more specific". > > + const struct drm_display_mode *mode2) > > +{ > > + return (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) == > > + (mode2->flags & ~DRM_MODE_FLAG_3D_MASK); > > +} > > + > > +static bool drm_mode_match_3d_flags(const struct drm_display_mode *mode1, > > + const struct drm_display_mode *mode2) > > +{ > > + return (mode1->flags & DRM_MODE_FLAG_3D_MASK) == > > + (mode2->flags & DRM_MODE_FLAG_3D_MASK); > > +} > > + > > +static bool drm_mode_match_aspect_ratio(const struct drm_display_mode *mode1, > > + const struct drm_display_mode *mode2) > > +{ > > + return mode1->picture_aspect_ratio == mode2->picture_aspect_ratio; > > +} > > + > > /** > > - * drm_mode_equal - test modes for equality > > + * drm_mode_match - test modes for (partial) equality > > * @mode1: first mode > > * @mode2: second mode > > + * @match_flags: which parts need to match (DRM_MODE_MATCH_*) > > * > > * Check to see if @mode1 and @mode2 are equivalent. > > * > > * Returns: > > - * True if the modes are equal, false otherwise. > > + * True if the modes are (partially) equal, false otherwise. > > */ > > -bool drm_mode_equal(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2) > > +bool drm_mode_match(const struct drm_display_mode *mode1, > > + const struct drm_display_mode *mode2, > > + unsigned int match_flags) > > { > > if (!mode1 && !mode2) > > return true; > > @@ -958,15 +1009,48 @@ bool drm_mode_equal(const struct drm_display_mode *mode1, const struct drm_displ > > if (!mode1 || !mode2) > > return false; > > > > - /* do clock check convert to PICOS so fb modes get matched > > - * the same */ > > - if (mode1->clock && mode2->clock) { > > - if (KHZ2PICOS(mode1->clock) != KHZ2PICOS(mode2->clock)) > > - return false; > > - } else if (mode1->clock != mode2->clock) > > + if (match_flags & DRM_MODE_MATCH_TIMINGS && > > + !drm_mode_match_timings(mode1, mode2)) > > + return false; > > + > > + if (match_flags & DRM_MODE_MATCH_CLOCK && > > + !drm_mode_match_clock(mode1, mode2)) > > + return false; > > + > > + if (match_flags & DRM_MODE_MATCH_FLAGS && > > + !drm_mode_match_flags(mode1, mode2)) > > + return false; > > + > > + if (match_flags & DRM_MODE_MATCH_3D_FLAGS && > > + !drm_mode_match_3d_flags(mode1, mode2)) > > return false; > > > > - return drm_mode_equal_no_clocks(mode1, mode2); > > + if (match_flags & DRM_MODE_MATCH_ASPECT_RATIO && > > + !drm_mode_match_aspect_ratio(mode1, mode2)) > > + return false; > > + > > + return true; > > +} > > +EXPORT_SYMBOL(drm_mode_match); > > + > > +/** > > + * drm_mode_equal - test modes for equality > > + * @mode1: first mode > > + * @mode2: second mode > > + * > > + * Check to see if @mode1 and @mode2 are equivalent. > > + * > > + * Returns: > > + * True if the modes are equal, false otherwise. > > + */ > > +bool drm_mode_equal(const struct drm_display_mode *mode1, > > + const struct drm_display_mode *mode2) > > +{ > > + return drm_mode_match(mode1, mode2, > > + DRM_MODE_MATCH_TIMINGS | > > + DRM_MODE_MATCH_CLOCK | > > + DRM_MODE_MATCH_FLAGS | > > + DRM_MODE_MATCH_3D_FLAGS); > > } > > EXPORT_SYMBOL(drm_mode_equal); > > > > @@ -981,13 +1065,13 @@ EXPORT_SYMBOL(drm_mode_equal); > > * Returns: > > * True if the modes are equal, false otherwise. > > */ > > -bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2) > > +bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, > > + const struct drm_display_mode *mode2) > > { > > - if ((mode1->flags & DRM_MODE_FLAG_3D_MASK) != > > - (mode2->flags & DRM_MODE_FLAG_3D_MASK)) > > - return false; > > - > > - return drm_mode_equal_no_clocks_no_stereo(mode1, mode2); > > + return drm_mode_match(mode1, mode2, > > + DRM_MODE_MATCH_TIMINGS | > > + DRM_MODE_MATCH_FLAGS | > > + DRM_MODE_MATCH_3D_FLAGS); > > } > > EXPORT_SYMBOL(drm_mode_equal_no_clocks); > > > > @@ -1005,21 +1089,9 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks); > > bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1, > > const struct drm_display_mode *mode2) > > { > > - if (mode1->hdisplay == mode2->hdisplay && > > - mode1->hsync_start == mode2->hsync_start && > > - mode1->hsync_end == mode2->hsync_end && > > - mode1->htotal == mode2->htotal && > > - mode1->hskew == mode2->hskew && > > - mode1->vdisplay == mode2->vdisplay && > > - mode1->vsync_start == mode2->vsync_start && > > - mode1->vsync_end == mode2->vsync_end && > > - mode1->vtotal == mode2->vtotal && > > - mode1->vscan == mode2->vscan && > > - (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) == > > - (mode2->flags & ~DRM_MODE_FLAG_3D_MASK)) > > - return true; > > - > > - return false; > > + return drm_mode_match(mode1, mode2, > > + DRM_MODE_MATCH_TIMINGS | > > + DRM_MODE_MATCH_FLAGS); > > } > > EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo); > > > > diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h > > index 9f3421c..839eb9c 100644 > > --- a/include/drm/drm_modes.h > > +++ b/include/drm/drm_modes.h > > @@ -150,6 +150,12 @@ enum drm_mode_status { > > > > #define DRM_MODE_FLAG_3D_MAX DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF > > > > +#define DRM_MODE_MATCH_TIMINGS (1 << 0) > > +#define DRM_MODE_MATCH_CLOCK (1 << 1) > > +#define DRM_MODE_MATCH_FLAGS (1 << 2) > > +#define DRM_MODE_MATCH_3D_FLAGS (1 << 3) > > +#define DRM_MODE_MATCH_ASPECT_RATIO (1 << 4) > > + > > /** > > * struct drm_display_mode - DRM kernel-internal display mode structure > > * @hdisplay: horizontal display size > > @@ -493,6 +499,9 @@ void drm_mode_copy(struct drm_display_mode *dst, > > const struct drm_display_mode *src); > > struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev, > > const struct drm_display_mode *mode); > > +bool drm_mode_match(const struct drm_display_mode *mode1, > > + const struct drm_display_mode *mode2, > Same here for the alignment > > + unsigned int match_flags); > > bool drm_mode_equal(const struct drm_display_mode *mode1, > > const struct drm_display_mode *mode2); > > bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, > With the minor comments above, please feel free to use > Reviewed-by: Shashank Sharma <shashank.sharma@intel.com> -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 1/8] drm/modes: Introduce drm_mode_match() 2018-01-17 15:21 ` Ville Syrjälä @ 2018-01-18 6:10 ` Sharma, Shashank 0 siblings, 0 replies; 35+ messages in thread From: Sharma, Shashank @ 2018-01-18 6:10 UTC (permalink / raw) To: Ville Syrjälä; +Cc: Nautiyal, Ankit K, dri-devel Regards Shashank On 1/17/2018 8:51 PM, Ville Syrjälä wrote: > On Wed, Jan 17, 2018 at 02:11:55PM +0530, Sharma, Shashank wrote: >> >> On 1/12/2018 11:51 AM, Nautiyal, Ankit K wrote: >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> >>> Make mode matching less confusing by allowing the caller to specify >>> which parts of the modes should match via some flags. >>> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> --- >>> drivers/gpu/drm/drm_modes.c | 134 ++++++++++++++++++++++++++++++++++---------- >>> include/drm/drm_modes.h | 9 +++ >>> 2 files changed, 112 insertions(+), 31 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c >>> index 4a3f68a..8128dbb 100644 >>> --- a/drivers/gpu/drm/drm_modes.c >>> +++ b/drivers/gpu/drm/drm_modes.c >>> @@ -940,17 +940,68 @@ struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev, >>> } >>> EXPORT_SYMBOL(drm_mode_duplicate); >>> >>> +static bool drm_mode_match_timings(const struct drm_display_mode *mode1, >>> + const struct drm_display_mode *mode2) >> Shouldn't the second parameter be aligned to next char after the opening >> bracket in above line (instead of being align to the bracket) ? > It is. The extra junk at the start of the line is causing the alignment > to look wrong when it's not because the tab stops are still aligned to > the start of the line. This is one reason why I think that using tabs > for alignment isn't a particularly great idea. > >> I have this same comment for most of the functions in this patch. >>> +{ >>> + return mode1->hdisplay == mode2->hdisplay && >>> + mode1->hsync_start == mode2->hsync_start && >>> + mode1->hsync_end == mode2->hsync_end && >>> + mode1->htotal == mode2->htotal && >>> + mode1->hskew == mode2->hskew && >>> + mode1->vdisplay == mode2->vdisplay && >>> + mode1->vsync_start == mode2->vsync_start && >>> + mode1->vsync_end == mode2->vsync_end && >>> + mode1->vtotal == mode2->vtotal && >>> + mode1->vscan == mode2->vscan; >>> +} >>> + >>> +static bool drm_mode_match_clock(const struct drm_display_mode *mode1, >>> + const struct drm_display_mode *mode2) >>> +{ >>> + /* >>> + * do clock check convert to PICOS >>> + * so fb modes get matched the same >>> + */ >>> + if (mode1->clock && mode2->clock) >>> + return KHZ2PICOS(mode1->clock) == KHZ2PICOS(mode2->clock); >>> + else >>> + return mode1->clock == mode2->clock; >>> +} >>> + >>> +static bool drm_mode_match_flags(const struct drm_display_mode *mode1, >> Should we call it drm_mode_match_flag_no_stereo ? > It's not really "no stereo", rather it's "check all the flags not being > checked by something more specific". Yeah, anyways no big deal. With or without, feel free to use my r-b - Shashank >>> + const struct drm_display_mode *mode2) >>> +{ >>> + return (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) == >>> + (mode2->flags & ~DRM_MODE_FLAG_3D_MASK); >>> +} >>> + >>> +static bool drm_mode_match_3d_flags(const struct drm_display_mode *mode1, >>> + const struct drm_display_mode *mode2) >>> +{ >>> + return (mode1->flags & DRM_MODE_FLAG_3D_MASK) == >>> + (mode2->flags & DRM_MODE_FLAG_3D_MASK); >>> +} >>> + >>> +static bool drm_mode_match_aspect_ratio(const struct drm_display_mode *mode1, >>> + const struct drm_display_mode *mode2) >>> +{ >>> + return mode1->picture_aspect_ratio == mode2->picture_aspect_ratio; >>> +} >>> + >>> /** >>> - * drm_mode_equal - test modes for equality >>> + * drm_mode_match - test modes for (partial) equality >>> * @mode1: first mode >>> * @mode2: second mode >>> + * @match_flags: which parts need to match (DRM_MODE_MATCH_*) >>> * >>> * Check to see if @mode1 and @mode2 are equivalent. >>> * >>> * Returns: >>> - * True if the modes are equal, false otherwise. >>> + * True if the modes are (partially) equal, false otherwise. >>> */ >>> -bool drm_mode_equal(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2) >>> +bool drm_mode_match(const struct drm_display_mode *mode1, >>> + const struct drm_display_mode *mode2, >>> + unsigned int match_flags) >>> { >>> if (!mode1 && !mode2) >>> return true; >>> @@ -958,15 +1009,48 @@ bool drm_mode_equal(const struct drm_display_mode *mode1, const struct drm_displ >>> if (!mode1 || !mode2) >>> return false; >>> >>> - /* do clock check convert to PICOS so fb modes get matched >>> - * the same */ >>> - if (mode1->clock && mode2->clock) { >>> - if (KHZ2PICOS(mode1->clock) != KHZ2PICOS(mode2->clock)) >>> - return false; >>> - } else if (mode1->clock != mode2->clock) >>> + if (match_flags & DRM_MODE_MATCH_TIMINGS && >>> + !drm_mode_match_timings(mode1, mode2)) >>> + return false; >>> + >>> + if (match_flags & DRM_MODE_MATCH_CLOCK && >>> + !drm_mode_match_clock(mode1, mode2)) >>> + return false; >>> + >>> + if (match_flags & DRM_MODE_MATCH_FLAGS && >>> + !drm_mode_match_flags(mode1, mode2)) >>> + return false; >>> + >>> + if (match_flags & DRM_MODE_MATCH_3D_FLAGS && >>> + !drm_mode_match_3d_flags(mode1, mode2)) >>> return false; >>> >>> - return drm_mode_equal_no_clocks(mode1, mode2); >>> + if (match_flags & DRM_MODE_MATCH_ASPECT_RATIO && >>> + !drm_mode_match_aspect_ratio(mode1, mode2)) >>> + return false; >>> + >>> + return true; >>> +} >>> +EXPORT_SYMBOL(drm_mode_match); >>> + >>> +/** >>> + * drm_mode_equal - test modes for equality >>> + * @mode1: first mode >>> + * @mode2: second mode >>> + * >>> + * Check to see if @mode1 and @mode2 are equivalent. >>> + * >>> + * Returns: >>> + * True if the modes are equal, false otherwise. >>> + */ >>> +bool drm_mode_equal(const struct drm_display_mode *mode1, >>> + const struct drm_display_mode *mode2) >>> +{ >>> + return drm_mode_match(mode1, mode2, >>> + DRM_MODE_MATCH_TIMINGS | >>> + DRM_MODE_MATCH_CLOCK | >>> + DRM_MODE_MATCH_FLAGS | >>> + DRM_MODE_MATCH_3D_FLAGS); >>> } >>> EXPORT_SYMBOL(drm_mode_equal); >>> >>> @@ -981,13 +1065,13 @@ EXPORT_SYMBOL(drm_mode_equal); >>> * Returns: >>> * True if the modes are equal, false otherwise. >>> */ >>> -bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, const struct drm_display_mode *mode2) >>> +bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, >>> + const struct drm_display_mode *mode2) >>> { >>> - if ((mode1->flags & DRM_MODE_FLAG_3D_MASK) != >>> - (mode2->flags & DRM_MODE_FLAG_3D_MASK)) >>> - return false; >>> - >>> - return drm_mode_equal_no_clocks_no_stereo(mode1, mode2); >>> + return drm_mode_match(mode1, mode2, >>> + DRM_MODE_MATCH_TIMINGS | >>> + DRM_MODE_MATCH_FLAGS | >>> + DRM_MODE_MATCH_3D_FLAGS); >>> } >>> EXPORT_SYMBOL(drm_mode_equal_no_clocks); >>> >>> @@ -1005,21 +1089,9 @@ EXPORT_SYMBOL(drm_mode_equal_no_clocks); >>> bool drm_mode_equal_no_clocks_no_stereo(const struct drm_display_mode *mode1, >>> const struct drm_display_mode *mode2) >>> { >>> - if (mode1->hdisplay == mode2->hdisplay && >>> - mode1->hsync_start == mode2->hsync_start && >>> - mode1->hsync_end == mode2->hsync_end && >>> - mode1->htotal == mode2->htotal && >>> - mode1->hskew == mode2->hskew && >>> - mode1->vdisplay == mode2->vdisplay && >>> - mode1->vsync_start == mode2->vsync_start && >>> - mode1->vsync_end == mode2->vsync_end && >>> - mode1->vtotal == mode2->vtotal && >>> - mode1->vscan == mode2->vscan && >>> - (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) == >>> - (mode2->flags & ~DRM_MODE_FLAG_3D_MASK)) >>> - return true; >>> - >>> - return false; >>> + return drm_mode_match(mode1, mode2, >>> + DRM_MODE_MATCH_TIMINGS | >>> + DRM_MODE_MATCH_FLAGS); >>> } >>> EXPORT_SYMBOL(drm_mode_equal_no_clocks_no_stereo); >>> >>> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h >>> index 9f3421c..839eb9c 100644 >>> --- a/include/drm/drm_modes.h >>> +++ b/include/drm/drm_modes.h >>> @@ -150,6 +150,12 @@ enum drm_mode_status { >>> >>> #define DRM_MODE_FLAG_3D_MAX DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF >>> >>> +#define DRM_MODE_MATCH_TIMINGS (1 << 0) >>> +#define DRM_MODE_MATCH_CLOCK (1 << 1) >>> +#define DRM_MODE_MATCH_FLAGS (1 << 2) >>> +#define DRM_MODE_MATCH_3D_FLAGS (1 << 3) >>> +#define DRM_MODE_MATCH_ASPECT_RATIO (1 << 4) >>> + >>> /** >>> * struct drm_display_mode - DRM kernel-internal display mode structure >>> * @hdisplay: horizontal display size >>> @@ -493,6 +499,9 @@ void drm_mode_copy(struct drm_display_mode *dst, >>> const struct drm_display_mode *src); >>> struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev, >>> const struct drm_display_mode *mode); >>> +bool drm_mode_match(const struct drm_display_mode *mode1, >>> + const struct drm_display_mode *mode2, >> Same here for the alignment >>> + unsigned int match_flags); >>> bool drm_mode_equal(const struct drm_display_mode *mode1, >>> const struct drm_display_mode *mode2); >>> bool drm_mode_equal_no_clocks(const struct drm_display_mode *mode1, >> With the minor comments above, please feel free to use >> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com> _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 2/8] drm/edid: Use drm_mode_match_no_clocks_no_stereo() for consistentcy 2018-01-12 6:21 [PATCH v3 0/8] Aspect ratio support in DRM layer Nautiyal, Ankit K 2018-01-12 6:21 ` [PATCH v3 1/8] drm/modes: Introduce drm_mode_match() Nautiyal, Ankit K @ 2018-01-12 6:21 ` Nautiyal, Ankit K 2018-01-17 8:53 ` Sharma, Shashank 2018-01-12 6:21 ` [PATCH v3 3/8] drm/edid: Fix cea mode aspect ratio handling Nautiyal, Ankit K ` (5 subsequent siblings) 7 siblings, 1 reply; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-01-12 6:21 UTC (permalink / raw) To: dri-devel From: Ville Syrjälä <ville.syrjala@linux.intel.com> Use drm_mode_equal_no_clocks_no_stereo() in drm_match_hdmi_mode_clock_tolerance() for consistency as we also use it in drm_match_hdmi_mode() and the cea mode matching functions. This doesn't actually change anything since the input mode comes from detailed timings and we match it against edid_4k_modes[] which. So none of those modes can have stereo flags set. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/drm_edid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index ddd5379..1818a71 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3025,7 +3025,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_ abs(to_match->clock - clock2) > clock_tolerance) continue; - if (drm_mode_equal_no_clocks(to_match, hdmi_mode)) + if (drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode)) return vic; } -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v3 2/8] drm/edid: Use drm_mode_match_no_clocks_no_stereo() for consistentcy 2018-01-12 6:21 ` [PATCH v3 2/8] drm/edid: Use drm_mode_match_no_clocks_no_stereo() for consistentcy Nautiyal, Ankit K @ 2018-01-17 8:53 ` Sharma, Shashank 0 siblings, 0 replies; 35+ messages in thread From: Sharma, Shashank @ 2018-01-17 8:53 UTC (permalink / raw) To: Nautiyal, Ankit K, dri-devel Regards Shashank On 1/12/2018 11:51 AM, Nautiyal, Ankit K wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Use drm_mode_equal_no_clocks_no_stereo() in > drm_match_hdmi_mode_clock_tolerance() for consistency as we > also use it in drm_match_hdmi_mode() and the cea mode matching > functions. > > This doesn't actually change anything since the input mode > comes from detailed timings and we match it against > edid_4k_modes[] which. So none of those modes can have stereo > flags set. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_edid.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index ddd5379..1818a71 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -3025,7 +3025,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_ > abs(to_match->clock - clock2) > clock_tolerance) > continue; > > - if (drm_mode_equal_no_clocks(to_match, hdmi_mode)) > + if (drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode)) > return vic; > } Reviewed-by: Shashank Sharma <shashank.sharma@intel.com> > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 3/8] drm/edid: Fix cea mode aspect ratio handling 2018-01-12 6:21 [PATCH v3 0/8] Aspect ratio support in DRM layer Nautiyal, Ankit K 2018-01-12 6:21 ` [PATCH v3 1/8] drm/modes: Introduce drm_mode_match() Nautiyal, Ankit K 2018-01-12 6:21 ` [PATCH v3 2/8] drm/edid: Use drm_mode_match_no_clocks_no_stereo() for consistentcy Nautiyal, Ankit K @ 2018-01-12 6:21 ` Nautiyal, Ankit K 2018-01-17 9:05 ` Sharma, Shashank 2018-01-12 6:21 ` [PATCH v3 4/8] drm: Add DRM client cap for aspect-ratio Nautiyal, Ankit K ` (4 subsequent siblings) 7 siblings, 1 reply; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-01-12 6:21 UTC (permalink / raw) To: dri-devel Cc: Jose Abreu, Lin, Jia, Akashdeep Sharma, Emil Velikov, Jim Bride, Daniel Vetter 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 1818a71..3d57c41 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2908,11 +2908,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; + for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) { struct drm_display_mode cea_mode = edid_cea_modes[vic]; unsigned int clock1, clock2; @@ -2926,7 +2930,7 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m continue; do { - if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode)) + if (drm_mode_match(to_match, &cea_mode, match_flags)) return vic; } while (cea_mode_alternate_timings(vic, &cea_mode)); } @@ -2943,11 +2947,15 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m */ u8 drm_match_cea_mode(const struct drm_display_mode *to_match) { + 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; + for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) { struct drm_display_mode cea_mode = edid_cea_modes[vic]; unsigned int clock1, clock2; @@ -2961,7 +2969,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match) continue; do { - if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode)) + if (drm_mode_match(to_match, &cea_mode, match_flags)) return vic; } while (cea_mode_alternate_timings(vic, &cea_mode)); } @@ -3008,6 +3016,7 @@ hdmi_mode_alternate_clock(const struct drm_display_mode *hdmi_mode) static u8 drm_match_hdmi_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) @@ -3025,7 +3034,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_ abs(to_match->clock - clock2) > clock_tolerance) continue; - if (drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode)) + if (drm_mode_match(to_match, hdmi_mode, match_flags)) return vic; } @@ -3042,6 +3051,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_ */ static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match) { + unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | DRM_MODE_MATCH_FLAGS; u8 vic; if (!to_match->clock) @@ -3057,7 +3067,7 @@ static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match) if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) || KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) && - drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode)) + drm_mode_match(to_match, hdmi_mode, match_flags)) return vic; } return 0; -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v3 3/8] drm/edid: Fix cea mode aspect ratio handling 2018-01-12 6:21 ` [PATCH v3 3/8] drm/edid: Fix cea mode aspect ratio handling Nautiyal, Ankit K @ 2018-01-17 9:05 ` Sharma, Shashank 2018-01-17 15:29 ` Ville Syrjälä 0 siblings, 1 reply; 35+ messages in thread From: Sharma, Shashank @ 2018-01-17 9:05 UTC (permalink / raw) To: Nautiyal, Ankit K, dri-devel Cc: Jose Abreu, Lin, Jia, Akashdeep Sharma, Emil Velikov, Daniel Vetter, Jim Bride Regards Shashank On 1/12/2018 11:51 AM, Nautiyal, Ankit K 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 1818a71..3d57c41 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -2908,11 +2908,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; > + > for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) { > struct drm_display_mode cea_mode = edid_cea_modes[vic]; > unsigned int clock1, clock2; > @@ -2926,7 +2930,7 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m > continue; > > do { > - if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode)) > + if (drm_mode_match(to_match, &cea_mode, match_flags)) > return vic; > } while (cea_mode_alternate_timings(vic, &cea_mode)); > } > @@ -2943,11 +2947,15 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m > */ > u8 drm_match_cea_mode(const struct drm_display_mode *to_match) > { > + 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; > + How about stereo flags ? CEA modes can be 3d and in that case we might want to add DRM_MODE_MATCH_3D_FLAGS here - Shashank > for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) { > struct drm_display_mode cea_mode = edid_cea_modes[vic]; > unsigned int clock1, clock2; > @@ -2961,7 +2969,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match) > continue; > > do { > - if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode)) > + if (drm_mode_match(to_match, &cea_mode, match_flags)) > return vic; > } while (cea_mode_alternate_timings(vic, &cea_mode)); > } > @@ -3008,6 +3016,7 @@ hdmi_mode_alternate_clock(const struct drm_display_mode *hdmi_mode) > static u8 drm_match_hdmi_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) > @@ -3025,7 +3034,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_ > abs(to_match->clock - clock2) > clock_tolerance) > continue; > > - if (drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode)) > + if (drm_mode_match(to_match, hdmi_mode, match_flags)) > return vic; > } > > @@ -3042,6 +3051,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_ > */ > static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match) > { > + unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | DRM_MODE_MATCH_FLAGS; > u8 vic; > > if (!to_match->clock) > @@ -3057,7 +3067,7 @@ static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match) > > if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) || > KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) && > - drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode)) > + drm_mode_match(to_match, hdmi_mode, match_flags)) > return vic; > } > return 0; _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 3/8] drm/edid: Fix cea mode aspect ratio handling 2018-01-17 9:05 ` Sharma, Shashank @ 2018-01-17 15:29 ` Ville Syrjälä 2018-01-18 6:14 ` Sharma, Shashank 0 siblings, 1 reply; 35+ messages in thread From: Ville Syrjälä @ 2018-01-17 15:29 UTC (permalink / raw) To: Sharma, Shashank Cc: Jose Abreu, Lin, Jia, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Nautiyal, Ankit K, Jim Bride On Wed, Jan 17, 2018 at 02:35:40PM +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 1/12/2018 11:51 AM, Nautiyal, Ankit K 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 1818a71..3d57c41 100644 > > --- a/drivers/gpu/drm/drm_edid.c > > +++ b/drivers/gpu/drm/drm_edid.c > > @@ -2908,11 +2908,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; > > + > > for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) { > > struct drm_display_mode cea_mode = edid_cea_modes[vic]; > > unsigned int clock1, clock2; > > @@ -2926,7 +2930,7 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m > > continue; > > > > do { > > - if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode)) > > + if (drm_mode_match(to_match, &cea_mode, match_flags)) > > return vic; > > } while (cea_mode_alternate_timings(vic, &cea_mode)); > > } > > @@ -2943,11 +2947,15 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m > > */ > > u8 drm_match_cea_mode(const struct drm_display_mode *to_match) > > { > > + 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; > > + > How about stereo flags ? CEA modes can be 3d and in that case we might > want to add DRM_MODE_MATCH_3D_FLAGS here There are no separate VICs for stereo vs. no stereo. So ignoring the stereo flags here seems like the correct thing to do. > > - Shashank > > for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) { > > struct drm_display_mode cea_mode = edid_cea_modes[vic]; > > unsigned int clock1, clock2; > > @@ -2961,7 +2969,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match) > > continue; > > > > do { > > - if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode)) > > + if (drm_mode_match(to_match, &cea_mode, match_flags)) > > return vic; > > } while (cea_mode_alternate_timings(vic, &cea_mode)); > > } > > @@ -3008,6 +3016,7 @@ hdmi_mode_alternate_clock(const struct drm_display_mode *hdmi_mode) > > static u8 drm_match_hdmi_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) > > @@ -3025,7 +3034,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_ > > abs(to_match->clock - clock2) > clock_tolerance) > > continue; > > > > - if (drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode)) > > + if (drm_mode_match(to_match, hdmi_mode, match_flags)) > > return vic; > > } > > > > @@ -3042,6 +3051,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_ > > */ > > static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match) > > { > > + unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | DRM_MODE_MATCH_FLAGS; > > u8 vic; > > > > if (!to_match->clock) > > @@ -3057,7 +3067,7 @@ static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match) > > > > if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) || > > KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) && > > - drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode)) > > + drm_mode_match(to_match, hdmi_mode, match_flags)) > > return vic; > > } > > return 0; -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 3/8] drm/edid: Fix cea mode aspect ratio handling 2018-01-17 15:29 ` Ville Syrjälä @ 2018-01-18 6:14 ` Sharma, Shashank 0 siblings, 0 replies; 35+ messages in thread From: Sharma, Shashank @ 2018-01-18 6:14 UTC (permalink / raw) To: Ville Syrjälä Cc: Jose Abreu, Lin, Jia, Akashdeep Sharma, Emil Velikov, dri-devel, Daniel Vetter, Nautiyal, Ankit K, Jim Bride Regards Shashank On 1/17/2018 8:59 PM, Ville Syrjälä wrote: > On Wed, Jan 17, 2018 at 02:35:40PM +0530, Sharma, Shashank wrote: >> Regards >> >> Shashank >> >> >> On 1/12/2018 11:51 AM, Nautiyal, Ankit K 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 1818a71..3d57c41 100644 >>> --- a/drivers/gpu/drm/drm_edid.c >>> +++ b/drivers/gpu/drm/drm_edid.c >>> @@ -2908,11 +2908,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; >>> + >>> for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) { >>> struct drm_display_mode cea_mode = edid_cea_modes[vic]; >>> unsigned int clock1, clock2; >>> @@ -2926,7 +2930,7 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m >>> continue; >>> >>> do { >>> - if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode)) >>> + if (drm_mode_match(to_match, &cea_mode, match_flags)) >>> return vic; >>> } while (cea_mode_alternate_timings(vic, &cea_mode)); >>> } >>> @@ -2943,11 +2947,15 @@ static u8 drm_match_cea_mode_clock_tolerance(const struct drm_display_mode *to_m >>> */ >>> u8 drm_match_cea_mode(const struct drm_display_mode *to_match) >>> { >>> + 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; >>> + >> How about stereo flags ? CEA modes can be 3d and in that case we might >> want to add DRM_MODE_MATCH_3D_FLAGS here > There are no separate VICs for stereo vs. no stereo. So ignoring the > stereo flags here seems like the correct thing to do. Agree, Reviewed-by: Shashank Sharma <shashank.sharma@intel.com> >> - Shashank >>> for (vic = 1; vic < ARRAY_SIZE(edid_cea_modes); vic++) { >>> struct drm_display_mode cea_mode = edid_cea_modes[vic]; >>> unsigned int clock1, clock2; >>> @@ -2961,7 +2969,7 @@ u8 drm_match_cea_mode(const struct drm_display_mode *to_match) >>> continue; >>> >>> do { >>> - if (drm_mode_equal_no_clocks_no_stereo(to_match, &cea_mode)) >>> + if (drm_mode_match(to_match, &cea_mode, match_flags)) >>> return vic; >>> } while (cea_mode_alternate_timings(vic, &cea_mode)); >>> } >>> @@ -3008,6 +3016,7 @@ hdmi_mode_alternate_clock(const struct drm_display_mode *hdmi_mode) >>> static u8 drm_match_hdmi_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) >>> @@ -3025,7 +3034,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_ >>> abs(to_match->clock - clock2) > clock_tolerance) >>> continue; >>> >>> - if (drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode)) >>> + if (drm_mode_match(to_match, hdmi_mode, match_flags)) >>> return vic; >>> } >>> >>> @@ -3042,6 +3051,7 @@ static u8 drm_match_hdmi_mode_clock_tolerance(const struct drm_display_mode *to_ >>> */ >>> static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match) >>> { >>> + unsigned int match_flags = DRM_MODE_MATCH_TIMINGS | DRM_MODE_MATCH_FLAGS; >>> u8 vic; >>> >>> if (!to_match->clock) >>> @@ -3057,7 +3067,7 @@ static u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match) >>> >>> if ((KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock1) || >>> KHZ2PICOS(to_match->clock) == KHZ2PICOS(clock2)) && >>> - drm_mode_equal_no_clocks_no_stereo(to_match, hdmi_mode)) >>> + drm_mode_match(to_match, hdmi_mode, match_flags)) >>> return vic; >>> } >>> return 0; _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 4/8] drm: Add DRM client cap for aspect-ratio 2018-01-12 6:21 [PATCH v3 0/8] Aspect ratio support in DRM layer Nautiyal, Ankit K ` (2 preceding siblings ...) 2018-01-12 6:21 ` [PATCH v3 3/8] drm/edid: Fix cea mode aspect ratio handling Nautiyal, Ankit K @ 2018-01-12 6:21 ` Nautiyal, Ankit K 2018-01-17 9:11 ` Sharma, Shashank 2018-01-18 10:16 ` Maarten Lankhorst 2018-01-12 6:21 ` [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths Nautiyal, Ankit K ` (3 subsequent siblings) 7 siblings, 2 replies; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-01-12 6:21 UTC (permalink / raw) To: dri-devel; +Cc: Ankit Nautiyal From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> A new drm client cap is required to enable user-space to advertise if it supports modes with aspect-ratio. Based on this cap value, the kernel will take a call on exposing the aspect ratio information in modes or not. This patch adds the client cap for aspect-ratio. Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Shashank Sharma <shashank.sharma@intel.com> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> V3: rebase --- drivers/gpu/drm/drm_ioctl.c | 5 +++++ include/drm/drm_file.h | 7 +++++++ include/uapi/drm/drm.h | 7 +++++++ 3 files changed, 19 insertions(+) diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 4aafe48..e092550 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -325,6 +325,11 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv) file_priv->atomic = req->value; file_priv->universal_planes = req->value; break; + case DRM_CLIENT_CAP_ASPECT_RATIO: + if (req->value > 1) + return -EINVAL; + file_priv->aspect_ratio_allowed = req->value; + break; default: return -EINVAL; } diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index 0e0c868..6e0e435 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -182,6 +182,13 @@ struct drm_file { unsigned atomic:1; /** + * @aspect_ratio_allowed: + * + * True if client has asked to expose the aspect-ratio flags with mode. + */ + unsigned aspect_ratio_allowed:1; + + /** * @is_master: * * This client is the creator of @master. Protected by struct diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 6fdff59..fe5f531 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -680,6 +680,13 @@ struct drm_get_cap { */ #define DRM_CLIENT_CAP_ATOMIC 3 +/** + * DRM_CLIENT_CAP_ASPECT_RATIO + * + * If set to 1, the DRM core will expose aspect ratio flags to userspace. + */ +#define DRM_CLIENT_CAP_ASPECT_RATIO 4 + /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ struct drm_set_client_cap { __u64 capability; -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v3 4/8] drm: Add DRM client cap for aspect-ratio 2018-01-12 6:21 ` [PATCH v3 4/8] drm: Add DRM client cap for aspect-ratio Nautiyal, Ankit K @ 2018-01-17 9:11 ` Sharma, Shashank 2018-01-18 10:16 ` Maarten Lankhorst 1 sibling, 0 replies; 35+ messages in thread From: Sharma, Shashank @ 2018-01-17 9:11 UTC (permalink / raw) To: Nautiyal, Ankit K, dri-devel Regards Shashank On 1/12/2018 11:51 AM, Nautiyal, Ankit K wrote: > From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > A new drm client cap is required to enable user-space to advertise > if it supports modes with aspect-ratio. Based on this cap value, the > kernel will take a call on exposing the aspect ratio information in > modes or not. > > This patch adds the client cap for aspect-ratio. > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > V3: rebase > --- > drivers/gpu/drm/drm_ioctl.c | 5 +++++ > include/drm/drm_file.h | 7 +++++++ > include/uapi/drm/drm.h | 7 +++++++ > 3 files changed, 19 insertions(+) > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 4aafe48..e092550 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -325,6 +325,11 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv) > file_priv->atomic = req->value; > file_priv->universal_planes = req->value; > break; > + case DRM_CLIENT_CAP_ASPECT_RATIO: > + if (req->value > 1) > + return -EINVAL; > + file_priv->aspect_ratio_allowed = req->value; > + break; > default: > return -EINVAL; > } > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > index 0e0c868..6e0e435 100644 > --- a/include/drm/drm_file.h > +++ b/include/drm/drm_file.h > @@ -182,6 +182,13 @@ struct drm_file { > unsigned atomic:1; > > /** > + * @aspect_ratio_allowed: > + * > + * True if client has asked to expose the aspect-ratio flags with mode. Minor reword, may be "True if client can handle picture aspect ratios, and has requested to pass this information along with the mode" > + */ > + unsigned aspect_ratio_allowed:1; > + > + /** > * @is_master: > * > * This client is the creator of @master. Protected by struct > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index 6fdff59..fe5f531 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -680,6 +680,13 @@ struct drm_get_cap { > */ > #define DRM_CLIENT_CAP_ATOMIC 3 > > +/** > + * DRM_CLIENT_CAP_ASPECT_RATIO > + * > + * If set to 1, the DRM core will expose aspect ratio flags to userspace. > + */ Again "If set to 1, the DRM core will provide aspect ratio information in modes" > +#define DRM_CLIENT_CAP_ASPECT_RATIO 4 > + > /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ > struct drm_set_client_cap { > __u64 capability; With these optional review comments, please feel free to use Reviewed-by: Shashank Sharma <shashank.sharma@intel.com> _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 4/8] drm: Add DRM client cap for aspect-ratio 2018-01-12 6:21 ` [PATCH v3 4/8] drm: Add DRM client cap for aspect-ratio Nautiyal, Ankit K 2018-01-17 9:11 ` Sharma, Shashank @ 2018-01-18 10:16 ` Maarten Lankhorst 2018-01-18 15:41 ` ankit.k.nautiyal 2018-01-22 4:34 ` [PATCH v4 " Nautiyal, Ankit K 1 sibling, 2 replies; 35+ messages in thread From: Maarten Lankhorst @ 2018-01-18 10:16 UTC (permalink / raw) To: Nautiyal, Ankit K, dri-devel Op 12-01-18 om 07:21 schreef Nautiyal, Ankit K: > From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > A new drm client cap is required to enable user-space to advertise > if it supports modes with aspect-ratio. Based on this cap value, the > kernel will take a call on exposing the aspect ratio information in > modes or not. > > This patch adds the client cap for aspect-ratio. > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> What's missing from the commit message is why this needs to be a flag that userspace needs to enable, instead of something that only counts as an informative query. I gathered from danvet that blindly exposing things breaks existing userspace, so could that information be added to the commit? Also missing IGT tests, would be nice we at least get some verification things work as expected. ~Maarten _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 4/8] drm: Add DRM client cap for aspect-ratio 2018-01-18 10:16 ` Maarten Lankhorst @ 2018-01-18 15:41 ` ankit.k.nautiyal 2018-01-18 16:04 ` Ville Syrjälä 2018-01-22 4:34 ` [PATCH v4 " Nautiyal, Ankit K 1 sibling, 1 reply; 35+ messages in thread From: ankit.k.nautiyal @ 2018-01-18 15:41 UTC (permalink / raw) To: Maarten Lankhorst, dri-devel [-- Attachment #1.1: Type: text/plain, Size: 1597 bytes --] Hi Marteen, Thanks for the review comments. Please find my comments in-line. On Thursday 18 January 2018 03:46 PM, Maarten Lankhorst wrote: > Op 12-01-18 om 07:21 schreef Nautiyal, Ankit K: >> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >> >> A new drm client cap is required to enable user-space to advertise >> if it supports modes with aspect-ratio. Based on this cap value, the >> kernel will take a call on exposing the aspect ratio information in >> modes or not. >> >> This patch adds the client cap for aspect-ratio. >> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Shashank Sharma <shashank.sharma@intel.com> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > What's missing from the commit message is why this needs to be a flag that userspace needs to enable, > instead of something that only counts as an informative query. > > I gathered from danvet that blindly exposing things breaks existing userspace, so could that information be added to the commit? Agreed. This information indeed brings out the real reason for the patch. Will add the details in the commit message in the next patch-set. > > Also missing IGT tests, would be nice we at least get some verification things work as expected. Currently this is being tested with the corresponding userspace changes in weston compositor, but a simple IGT on the same lines of kms_3d can be prepared. Will work on that and include the reference in this series. I hope in the meanwhile, the other patches of the series would be continued to be reviewed. > > ~Maarten Thanks & regards, Ankit [-- Attachment #1.2: Type: text/html, Size: 2926 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 4/8] drm: Add DRM client cap for aspect-ratio 2018-01-18 15:41 ` ankit.k.nautiyal @ 2018-01-18 16:04 ` Ville Syrjälä 2018-01-19 5:44 ` Nautiyal, Ankit K 0 siblings, 1 reply; 35+ messages in thread From: Ville Syrjälä @ 2018-01-18 16:04 UTC (permalink / raw) To: ankit.k.nautiyal; +Cc: dri-devel On Thu, Jan 18, 2018 at 09:11:19PM +0530, ankit.k.nautiyal@intel.com wrote: > Hi Marteen, > > Thanks for the review comments. Please find my comments in-line. > > > On Thursday 18 January 2018 03:46 PM, Maarten Lankhorst wrote: > > Op 12-01-18 om 07:21 schreef Nautiyal, Ankit K: > >> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > >> > >> A new drm client cap is required to enable user-space to advertise > >> if it supports modes with aspect-ratio. Based on this cap value, the > >> kernel will take a call on exposing the aspect ratio information in > >> modes or not. > >> > >> This patch adds the client cap for aspect-ratio. > >> > >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > >> Cc: Shashank Sharma <shashank.sharma@intel.com> > >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > What's missing from the commit message is why this needs to be a flag that userspace needs to enable, > > instead of something that only counts as an informative query. > > > > I gathered from danvet that blindly exposing things breaks existing userspace, so could that information be added to the commit? > > Agreed. This information indeed brings out the real reason for the patch. > Will add the details in the commit message in the next patch-set. > > > > > Also missing IGT tests, would be nice we at least get some verification things work as expected. > > Currently this is being tested with the corresponding userspace changes > in weston compositor, but a simple IGT on the same lines of kms_3d > can be prepared. A separate test to check that the different aspect ratio modes are correctly enumerated, and filtered out when the cap is not set seems like a good idea. As for actaully testing different aspect ratio modes, maybe just intergrate that into testdisplay? -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 4/8] drm: Add DRM client cap for aspect-ratio 2018-01-18 16:04 ` Ville Syrjälä @ 2018-01-19 5:44 ` Nautiyal, Ankit K 0 siblings, 0 replies; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-01-19 5:44 UTC (permalink / raw) To: Ville Syrjälä; +Cc: dri-devel [-- Attachment #1.1: Type: text/plain, Size: 2102 bytes --] On 1/18/2018 9:34 PM, Ville Syrjälä wrote: > On Thu, Jan 18, 2018 at 09:11:19PM +0530, ankit.k.nautiyal@intel.com wrote: >> Hi Marteen, >> >> Thanks for the review comments. Please find my comments in-line. >> >> >> On Thursday 18 January 2018 03:46 PM, Maarten Lankhorst wrote: >>> Op 12-01-18 om 07:21 schreef Nautiyal, Ankit K: >>>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>>> >>>> A new drm client cap is required to enable user-space to advertise >>>> if it supports modes with aspect-ratio. Based on this cap value, the >>>> kernel will take a call on exposing the aspect ratio information in >>>> modes or not. >>>> >>>> This patch adds the client cap for aspect-ratio. >>>> >>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >>>> Cc: Shashank Sharma <shashank.sharma@intel.com> >>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>> What's missing from the commit message is why this needs to be a flag that userspace needs to enable, >>> instead of something that only counts as an informative query. >>> >>> I gathered from danvet that blindly exposing things breaks existing userspace, so could that information be added to the commit? >> Agreed. This information indeed brings out the real reason for the patch. >> Will add the details in the commit message in the next patch-set. >> >>> Also missing IGT tests, would be nice we at least get some verification things work as expected. >> Currently this is being tested with the corresponding userspace changes >> in weston compositor, but a simple IGT on the same lines of kms_3d >> can be prepared. > A separate test to check that the different aspect ratio modes are > correctly enumerated, and filtered out when the cap is not set seems > like a good idea. > > As for actaully testing different aspect ratio modes, maybe just > intergrate that into testdisplay? Alright. Will modify the testdisplay to have a "test_aspect_ratio_modes", which will just set the aspect ratio modes from the connector modelist, to verify the basic functionality. kms_aspect_ratio to do detailed testing will be taken later. [-- Attachment #1.2: Type: text/html, Size: 3412 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v4 4/8] drm: Add DRM client cap for aspect-ratio 2018-01-18 10:16 ` Maarten Lankhorst 2018-01-18 15:41 ` ankit.k.nautiyal @ 2018-01-22 4:34 ` Nautiyal, Ankit K 1 sibling, 0 replies; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-01-22 4:34 UTC (permalink / raw) To: dri-devel; +Cc: Ankit Nautiyal From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> To enable aspect-ratio support in DRM, blindly exposing the aspect-ratio information along with mode, can break things in existing user-spaces which have no intention or support to use this aspect-ratio information. To avoid this, a new drm client cap is required to enable a user-space to advertise if it supports modes with aspect-ratio. Based on this cap value, the kernel will take a call on exposing the aspect-ratio information in modes or not. This patch adds the client cap for aspect-ratio. Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Shashank Sharma <shashank.sharma@intel.com> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> V3: rebase v4: As suggested by Marteen Lankhorst modified the commit message explaining the need to use the DRM cap for aspect-ratio. Also, tweaked the comment lines in the code for better understanding and clarity, as recommended by Shashank Sharma. Reviewed-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/drm_ioctl.c | 5 +++++ include/drm/drm_file.h | 8 ++++++++ include/uapi/drm/drm.h | 7 +++++++ 3 files changed, 20 insertions(+) diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index b1e96fb..cbcf2a2 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -325,6 +325,11 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv) file_priv->atomic = req->value; file_priv->universal_planes = req->value; break; + case DRM_CLIENT_CAP_ASPECT_RATIO: + if (req->value > 1) + return -EINVAL; + file_priv->aspect_ratio_allowed = req->value; + break; default: return -EINVAL; } diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index 0e0c868..c025301 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -182,6 +182,14 @@ struct drm_file { unsigned atomic:1; /** + * @aspect_ratio_allowed: + * + * True, if client can handle picture aspect ratios, and has requested + * to pass this information along with the mode. + */ + unsigned aspect_ratio_allowed:1; + + /** * @is_master: * * This client is the creator of @master. Protected by struct diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 6fdff59..82907d4 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -680,6 +680,13 @@ struct drm_get_cap { */ #define DRM_CLIENT_CAP_ATOMIC 3 +/** + * DRM_CLIENT_CAP_ASPECT_RATIO + * + * If set to 1, the DRM core will provide aspect ratio information in modes. + */ +#define DRM_CLIENT_CAP_ASPECT_RATIO 4 + /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */ struct drm_set_client_cap { __u64 capability; -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths 2018-01-12 6:21 [PATCH v3 0/8] Aspect ratio support in DRM layer Nautiyal, Ankit K ` (3 preceding siblings ...) 2018-01-12 6:21 ` [PATCH v3 4/8] drm: Add DRM client cap for aspect-ratio Nautiyal, Ankit K @ 2018-01-12 6:21 ` Nautiyal, Ankit K 2018-01-29 18:53 ` Ville Syrjälä 2018-01-12 6:21 ` [PATCH v3 6/8] drm: Expose modes with aspect ratio, only if requested Nautiyal, Ankit K ` (2 subsequent siblings) 7 siblings, 1 reply; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-01-12 6:21 UTC (permalink / raw) To: dri-devel; +Cc: Ankit Nautiyal From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> If the user mode does not support aspect-ratio, and requests for a modeset, then the flag bits representing aspect ratio in the given user-mode must be rejected. Similarly, while preparing a user-mode from kernel mode, the aspect-ratio info must not be given, if aspect-ratio is not supported by the user. This patch: 1. adds a new bit field aspect_ratio_allowed in drm_atomic_state, which is set only if the aspect-ratio is supported by the user. 2. discards the aspect-ratio info from the user mode while preparing kernel mode structure, during modeset, if the user does not support aspect ratio. 3. avoids setting the aspect-ratio info in user-mode, while converting user-mode from the kernel-mode. Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> V3: Addressed review comments from Ville: -Do not corrupt the current crtc state by updating aspect ratio on the fly. --- drivers/gpu/drm/drm_atomic.c | 61 +++++++++++++++++++++++++++++++++++++++++--- drivers/gpu/drm/drm_crtc.c | 19 ++++++++++++++ include/drm/drm_atomic.h | 2 ++ 3 files changed, 79 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 69ff763..39313e2 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -316,6 +316,35 @@ static s32 __user *get_out_fence_for_crtc(struct drm_atomic_state *state, return fence_ptr; } +/** + * drm_atomic_allow_aspect_ratio_for_kmode + * @state: the crtc state + * @mode: kernel-internal mode, which is used to create a duplicate mode, + * with updated picture aspect ratio. + * + * Allow the aspect ratio info in the kernel mode only if aspect ratio is + * supported. + * + * RETURNS: + * kernel-internal mode with updated picture aspect ratio value. + */ + +struct drm_display_mode* +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state *state, + const struct drm_display_mode *mode) +{ + struct drm_atomic_state *atomic_state = state->state; + struct drm_display_mode *ar_kmode; + + ar_kmode = drm_mode_duplicate(state->crtc->dev, mode); + /* + * If aspect ratio is not supported, set the picture aspect ratio as + * NONE. + */ + if (atomic_state && !atomic_state->allow_aspect_ratio) + ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; + return ar_kmode; +} /** * drm_atomic_set_mode_for_crtc - set mode for CRTC @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, state->mode_blob = NULL; if (mode) { - drm_mode_convert_to_umode(&umode, mode); + struct drm_display_mode *ar_mode; + + ar_mode = drm_atomic_allow_aspect_ratio_for_kmode(state, mode); + drm_mode_convert_to_umode(&umode, ar_mode); state->mode_blob = drm_property_create_blob(state->crtc->dev, sizeof(umode), @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, if (IS_ERR(state->mode_blob)) return PTR_ERR(state->mode_blob); - drm_mode_copy(&state->mode, mode); + drm_mode_copy(&state->mode, ar_mode); state->enable = true; DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n", - mode->name, state); + ar_mode->name, state); + drm_mode_destroy(state->crtc->dev, ar_mode); } else { memset(&state->mode, 0, sizeof(state->mode)); state->enable = false; @@ -436,6 +469,25 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, } /** + * drm_atomic_allow_aspect_ratio_for_umode + * @state: the crtc state + * @umode: userspace mode, whose aspect ratio flag bits are to be updated. + * + * Allow the aspect ratio info in the userspace mode only if aspect ratio is + * supported. + */ +void +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state *state, + struct drm_mode_modeinfo *umode) +{ + struct drm_atomic_state *atomic_state = state->state; + + /* Reset the aspect ratio flags if aspect ratio is not supported */ + if (atomic_state && !atomic_state->allow_aspect_ratio) + umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); +} + +/** * drm_atomic_crtc_set_property - set property on CRTC * @crtc: the drm CRTC to set a property on * @state: the state object to update with the new property value @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, else if (property == config->prop_mode_id) { struct drm_property_blob *mode = drm_property_lookup_blob(dev, val); + drm_atomic_allow_aspect_ratio_for_umode(state, + (struct drm_mode_modeinfo *) + mode->data); ret = drm_atomic_set_mode_prop_for_crtc(state, mode); drm_property_blob_put(mode); return ret; diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index f0556e6..a2d34fa 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev, drm_modeset_lock(&crtc->mutex, NULL); if (crtc->state) { if (crtc->state->enable) { + /* + * If the aspect-ratio is not required by the, + * userspace, do not set the aspect-ratio flags. + */ + if (!file_priv->aspect_ratio_allowed) + crtc->state->mode.picture_aspect_ratio = + HDMI_PICTURE_ASPECT_NONE; drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); crtc_resp->mode_valid = 1; @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device *dev, crtc_resp->x = crtc->x; crtc_resp->y = crtc->y; if (crtc->enabled) { + /* + * If the aspect-ratio is not required by the, + * userspace, do not set the aspect-ratio flags. + */ + if (!file_priv->aspect_ratio_allowed) + crtc->mode.picture_aspect_ratio = + HDMI_PICTURE_ASPECT_NONE; drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode); crtc_resp->mode_valid = 1; @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, goto out; } + /* If the user does not ask for aspect ratio, reset the aspect + * ratio bits in the usermode. + */ + if (!file_priv->aspect_ratio_allowed) + crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); ret = drm_mode_convert_umode(mode, &crtc_req->mode); if (ret) { DRM_DEBUG_KMS("Invalid mode\n"); diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 1c27526..130dad9 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -237,6 +237,7 @@ struct __drm_private_objs_state { * @ref: count of all references to this state (will not be freed until zero) * @dev: parent DRM device * @allow_modeset: allow full modeset + * @allow_aspect_ratio: allow the aspect ratio info to be passes to user * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics * @async_update: hint for asynchronous plane update * @planes: pointer to array of structures with per-plane data @@ -256,6 +257,7 @@ struct drm_atomic_state { struct drm_device *dev; bool allow_modeset : 1; + bool allow_aspect_ratio : 1; bool legacy_cursor_update : 1; bool async_update : 1; struct __drm_planes_state *planes; -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths 2018-01-12 6:21 ` [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths Nautiyal, Ankit K @ 2018-01-29 18:53 ` Ville Syrjälä 2018-01-31 6:34 ` Nautiyal, Ankit K 0 siblings, 1 reply; 35+ messages in thread From: Ville Syrjälä @ 2018-01-29 18:53 UTC (permalink / raw) To: Nautiyal, Ankit K; +Cc: dri-devel On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote: > From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > If the user mode does not support aspect-ratio, and requests for > a modeset, then the flag bits representing aspect ratio in the > given user-mode must be rejected. > Similarly, while preparing a user-mode from kernel mode, the > aspect-ratio info must not be given, if aspect-ratio is not > supported by the user. > > This patch: > 1. adds a new bit field aspect_ratio_allowed in drm_atomic_state, > which is set only if the aspect-ratio is supported by the user. > 2. discards the aspect-ratio info from the user mode while > preparing kernel mode structure, during modeset, if the > user does not support aspect ratio. > 3. avoids setting the aspect-ratio info in user-mode, while > converting user-mode from the kernel-mode. > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > V3: Addressed review comments from Ville: > -Do not corrupt the current crtc state by updating aspect ratio > on the fly. > --- > drivers/gpu/drm/drm_atomic.c | 61 +++++++++++++++++++++++++++++++++++++++++--- > drivers/gpu/drm/drm_crtc.c | 19 ++++++++++++++ > include/drm/drm_atomic.h | 2 ++ > 3 files changed, 79 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 69ff763..39313e2 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -316,6 +316,35 @@ static s32 __user *get_out_fence_for_crtc(struct drm_atomic_state *state, > > return fence_ptr; > } > +/** > + * drm_atomic_allow_aspect_ratio_for_kmode > + * @state: the crtc state > + * @mode: kernel-internal mode, which is used to create a duplicate mode, > + * with updated picture aspect ratio. > + * > + * Allow the aspect ratio info in the kernel mode only if aspect ratio is > + * supported. > + * > + * RETURNS: > + * kernel-internal mode with updated picture aspect ratio value. > + */ > + > +struct drm_display_mode* > +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state *state, > + const struct drm_display_mode *mode) > +{ > + struct drm_atomic_state *atomic_state = state->state; > + struct drm_display_mode *ar_kmode; > + > + ar_kmode = drm_mode_duplicate(state->crtc->dev, mode); > + /* > + * If aspect ratio is not supported, set the picture aspect ratio as > + * NONE. > + */ > + if (atomic_state && !atomic_state->allow_aspect_ratio) > + ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; > + return ar_kmode; > +} > > /** > * drm_atomic_set_mode_for_crtc - set mode for CRTC > @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, > state->mode_blob = NULL; > > if (mode) { > - drm_mode_convert_to_umode(&umode, mode); > + struct drm_display_mode *ar_mode; > + > + ar_mode = drm_atomic_allow_aspect_ratio_for_kmode(state, mode); > + drm_mode_convert_to_umode(&umode, ar_mode); This still looks sketchy. I believe there are just two places where we need to filter out the aspect ratio flags from the umode, namely the getblob and getcrtc ioctls. And for checking the user input I think we should probably just stick that into drm_mode_convert_umode(). Looks like we never call that from anywhere but the atomic/setprop and setcrtc paths with a non-NULL argument. I *think* those three places should be sufficient to cover everything. > state->mode_blob = > drm_property_create_blob(state->crtc->dev, > sizeof(umode), > @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, > if (IS_ERR(state->mode_blob)) > return PTR_ERR(state->mode_blob); > > - drm_mode_copy(&state->mode, mode); > + drm_mode_copy(&state->mode, ar_mode); > state->enable = true; > DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n", > - mode->name, state); > + ar_mode->name, state); > + drm_mode_destroy(state->crtc->dev, ar_mode); > } else { > memset(&state->mode, 0, sizeof(state->mode)); > state->enable = false; > @@ -436,6 +469,25 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, > } > > /** > + * drm_atomic_allow_aspect_ratio_for_umode > + * @state: the crtc state > + * @umode: userspace mode, whose aspect ratio flag bits are to be updated. > + * > + * Allow the aspect ratio info in the userspace mode only if aspect ratio is > + * supported. > + */ > +void > +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state *state, > + struct drm_mode_modeinfo *umode) > +{ > + struct drm_atomic_state *atomic_state = state->state; > + > + /* Reset the aspect ratio flags if aspect ratio is not supported */ > + if (atomic_state && !atomic_state->allow_aspect_ratio) > + umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); > +} > + > +/** > * drm_atomic_crtc_set_property - set property on CRTC > * @crtc: the drm CRTC to set a property on > * @state: the state object to update with the new property value > @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, > else if (property == config->prop_mode_id) { > struct drm_property_blob *mode = > drm_property_lookup_blob(dev, val); > + drm_atomic_allow_aspect_ratio_for_umode(state, > + (struct drm_mode_modeinfo *) > + mode->data); > ret = drm_atomic_set_mode_prop_for_crtc(state, mode); > drm_property_blob_put(mode); > return ret; > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index f0556e6..a2d34fa 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev, > drm_modeset_lock(&crtc->mutex, NULL); > if (crtc->state) { > if (crtc->state->enable) { > + /* > + * If the aspect-ratio is not required by the, > + * userspace, do not set the aspect-ratio flags. > + */ > + if (!file_priv->aspect_ratio_allowed) > + crtc->state->mode.picture_aspect_ratio = > + HDMI_PICTURE_ASPECT_NONE; These would still clobber the current crtc state. So definitely don't want to do this. > drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); > crtc_resp->mode_valid = 1; > > @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device *dev, > crtc_resp->x = crtc->x; > crtc_resp->y = crtc->y; > if (crtc->enabled) { > + /* > + * If the aspect-ratio is not required by the, > + * userspace, do not set the aspect-ratio flags. > + */ > + if (!file_priv->aspect_ratio_allowed) > + crtc->mode.picture_aspect_ratio = > + HDMI_PICTURE_ASPECT_NONE; > drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode); > crtc_resp->mode_valid = 1; > > @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, > goto out; > } > > + /* If the user does not ask for aspect ratio, reset the aspect > + * ratio bits in the usermode. > + */ > + if (!file_priv->aspect_ratio_allowed) > + crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); > ret = drm_mode_convert_umode(mode, &crtc_req->mode); > if (ret) { > DRM_DEBUG_KMS("Invalid mode\n"); > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index 1c27526..130dad9 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -237,6 +237,7 @@ struct __drm_private_objs_state { > * @ref: count of all references to this state (will not be freed until zero) > * @dev: parent DRM device > * @allow_modeset: allow full modeset > + * @allow_aspect_ratio: allow the aspect ratio info to be passes to user > * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics > * @async_update: hint for asynchronous plane update > * @planes: pointer to array of structures with per-plane data > @@ -256,6 +257,7 @@ struct drm_atomic_state { > > struct drm_device *dev; > bool allow_modeset : 1; > + bool allow_aspect_ratio : 1; > bool legacy_cursor_update : 1; > bool async_update : 1; > struct __drm_planes_state *planes; > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths 2018-01-29 18:53 ` Ville Syrjälä @ 2018-01-31 6:34 ` Nautiyal, Ankit K 2018-01-31 13:09 ` Ville Syrjälä 0 siblings, 1 reply; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-01-31 6:34 UTC (permalink / raw) To: Ville Syrjälä; +Cc: dri-devel [-- Attachment #1.1: Type: text/plain, Size: 9834 bytes --] On 1/30/2018 12:23 AM, Ville Syrjälä wrote: > On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote: >> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >> >> If the user mode does not support aspect-ratio, and requests for >> a modeset, then the flag bits representing aspect ratio in the >> given user-mode must be rejected. >> Similarly, while preparing a user-mode from kernel mode, the >> aspect-ratio info must not be given, if aspect-ratio is not >> supported by the user. >> >> This patch: >> 1. adds a new bit field aspect_ratio_allowed in drm_atomic_state, >> which is set only if the aspect-ratio is supported by the user. >> 2. discards the aspect-ratio info from the user mode while >> preparing kernel mode structure, during modeset, if the >> user does not support aspect ratio. >> 3. avoids setting the aspect-ratio info in user-mode, while >> converting user-mode from the kernel-mode. >> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >> >> V3: Addressed review comments from Ville: >> -Do not corrupt the current crtc state by updating aspect ratio >> on the fly. >> --- >> drivers/gpu/drm/drm_atomic.c | 61 +++++++++++++++++++++++++++++++++++++++++--- >> drivers/gpu/drm/drm_crtc.c | 19 ++++++++++++++ >> include/drm/drm_atomic.h | 2 ++ >> 3 files changed, 79 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> index 69ff763..39313e2 100644 >> --- a/drivers/gpu/drm/drm_atomic.c >> +++ b/drivers/gpu/drm/drm_atomic.c >> @@ -316,6 +316,35 @@ static s32 __user *get_out_fence_for_crtc(struct drm_atomic_state *state, >> >> return fence_ptr; >> } >> +/** >> + * drm_atomic_allow_aspect_ratio_for_kmode >> + * @state: the crtc state >> + * @mode: kernel-internal mode, which is used to create a duplicate mode, >> + * with updated picture aspect ratio. >> + * >> + * Allow the aspect ratio info in the kernel mode only if aspect ratio is >> + * supported. >> + * >> + * RETURNS: >> + * kernel-internal mode with updated picture aspect ratio value. >> + */ >> + >> +struct drm_display_mode* >> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state *state, >> + const struct drm_display_mode *mode) >> +{ >> + struct drm_atomic_state *atomic_state = state->state; >> + struct drm_display_mode *ar_kmode; >> + >> + ar_kmode = drm_mode_duplicate(state->crtc->dev, mode); >> + /* >> + * If aspect ratio is not supported, set the picture aspect ratio as >> + * NONE. >> + */ >> + if (atomic_state && !atomic_state->allow_aspect_ratio) >> + ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; >> + return ar_kmode; >> +} >> >> /** >> * drm_atomic_set_mode_for_crtc - set mode for CRTC >> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, >> state->mode_blob = NULL; >> >> if (mode) { >> - drm_mode_convert_to_umode(&umode, mode); >> + struct drm_display_mode *ar_mode; >> + >> + ar_mode = drm_atomic_allow_aspect_ratio_for_kmode(state, mode); >> + drm_mode_convert_to_umode(&umode, ar_mode); > This still looks sketchy. > > I believe there are just two places where we need to filter out the > aspect ratio flags from the umode, namely the getblob and getcrtc > ioctls. AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob, etc. apart from the mode blob. Is there any way to check from blob id (or by any other means), if the data is for the mode, or edid, or gamma etc. If we can check that, I can make sure that aspect-ratio flags are taken care of, if the blob is for mode, and the aspect ratio is not supported. > > And for checking the user input I think we should probably just > stick that into drm_mode_convert_umode(). Looks like we never call > that from anywhere but the atomic/setprop and setcrtc paths with > a non-NULL argument. > > I *think* those three places should be sufficient to cover everything. Agreed. Infact I tried to do that first, but the only problem we have here, is that, the file_priv which has the aspect ratio capability information is not supplied to the drm_mode_convert_umode() or the functions calling, drm_mode_conver_umode(). At best we have drm_crtc_state. I have added an aspect ratio allowed flag in drm_crtc_state->state so that its available in the functions calling drm_mode_convert_umode. I am open to suggestion to avoid adding a new flag in drm_atomic_state, if there is a better way to let the drm_mode_convert_umode( ) know that user supports aspect ratio capability. > >> state->mode_blob = >> drm_property_create_blob(state->crtc->dev, >> sizeof(umode), >> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, >> if (IS_ERR(state->mode_blob)) >> return PTR_ERR(state->mode_blob); >> >> - drm_mode_copy(&state->mode, mode); >> + drm_mode_copy(&state->mode, ar_mode); >> state->enable = true; >> DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n", >> - mode->name, state); >> + ar_mode->name, state); >> + drm_mode_destroy(state->crtc->dev, ar_mode); >> } else { >> memset(&state->mode, 0, sizeof(state->mode)); >> state->enable = false; >> @@ -436,6 +469,25 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, >> } >> >> /** >> + * drm_atomic_allow_aspect_ratio_for_umode >> + * @state: the crtc state >> + * @umode: userspace mode, whose aspect ratio flag bits are to be updated. >> + * >> + * Allow the aspect ratio info in the userspace mode only if aspect ratio is >> + * supported. >> + */ >> +void >> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state *state, >> + struct drm_mode_modeinfo *umode) >> +{ >> + struct drm_atomic_state *atomic_state = state->state; >> + >> + /* Reset the aspect ratio flags if aspect ratio is not supported */ >> + if (atomic_state && !atomic_state->allow_aspect_ratio) >> + umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); >> +} >> + >> +/** >> * drm_atomic_crtc_set_property - set property on CRTC >> * @crtc: the drm CRTC to set a property on >> * @state: the state object to update with the new property value >> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, >> else if (property == config->prop_mode_id) { >> struct drm_property_blob *mode = >> drm_property_lookup_blob(dev, val); >> + drm_atomic_allow_aspect_ratio_for_umode(state, >> + (struct drm_mode_modeinfo *) >> + mode->data); >> ret = drm_atomic_set_mode_prop_for_crtc(state, mode); >> drm_property_blob_put(mode); >> return ret; >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> index f0556e6..a2d34fa 100644 >> --- a/drivers/gpu/drm/drm_crtc.c >> +++ b/drivers/gpu/drm/drm_crtc.c >> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev, >> drm_modeset_lock(&crtc->mutex, NULL); >> if (crtc->state) { >> if (crtc->state->enable) { >> + /* >> + * If the aspect-ratio is not required by the, >> + * userspace, do not set the aspect-ratio flags. >> + */ >> + if (!file_priv->aspect_ratio_allowed) >> + crtc->state->mode.picture_aspect_ratio = >> + HDMI_PICTURE_ASPECT_NONE; > These would still clobber the current crtc state. So definitely don't > want to do this. Alright, so alternative way of doing this will be: Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel mode structure, but set the aspect ratio flags in the usermode to none, after calling the drm_mode_convert_to_umode(). Will take care of this in the next patchset. > >> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); >> crtc_resp->mode_valid = 1; >> >> @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device *dev, >> crtc_resp->x = crtc->x; >> crtc_resp->y = crtc->y; >> if (crtc->enabled) { >> + /* >> + * If the aspect-ratio is not required by the, >> + * userspace, do not set the aspect-ratio flags. >> + */ >> + if (!file_priv->aspect_ratio_allowed) >> + crtc->mode.picture_aspect_ratio = >> + HDMI_PICTURE_ASPECT_NONE; >> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode); >> crtc_resp->mode_valid = 1; >> >> @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, >> goto out; >> } >> >> + /* If the user does not ask for aspect ratio, reset the aspect >> + * ratio bits in the usermode. >> + */ >> + if (!file_priv->aspect_ratio_allowed) >> + crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); >> ret = drm_mode_convert_umode(mode, &crtc_req->mode); >> if (ret) { >> DRM_DEBUG_KMS("Invalid mode\n"); >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h >> index 1c27526..130dad9 100644 >> --- a/include/drm/drm_atomic.h >> +++ b/include/drm/drm_atomic.h >> @@ -237,6 +237,7 @@ struct __drm_private_objs_state { >> * @ref: count of all references to this state (will not be freed until zero) >> * @dev: parent DRM device >> * @allow_modeset: allow full modeset >> + * @allow_aspect_ratio: allow the aspect ratio info to be passes to user >> * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics >> * @async_update: hint for asynchronous plane update >> * @planes: pointer to array of structures with per-plane data >> @@ -256,6 +257,7 @@ struct drm_atomic_state { >> >> struct drm_device *dev; >> bool allow_modeset : 1; >> + bool allow_aspect_ratio : 1; >> bool legacy_cursor_update : 1; >> bool async_update : 1; >> struct __drm_planes_state *planes; >> -- >> 2.7.4 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel [-- Attachment #1.2: Type: text/html, Size: 11155 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths 2018-01-31 6:34 ` Nautiyal, Ankit K @ 2018-01-31 13:09 ` Ville Syrjälä 2018-02-01 11:05 ` Nautiyal, Ankit K 0 siblings, 1 reply; 35+ messages in thread From: Ville Syrjälä @ 2018-01-31 13:09 UTC (permalink / raw) To: Nautiyal, Ankit K; +Cc: dri-devel On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote: > > > On 1/30/2018 12:23 AM, Ville Syrjälä wrote: > > On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote: > >> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > >> > >> If the user mode does not support aspect-ratio, and requests for > >> a modeset, then the flag bits representing aspect ratio in the > >> given user-mode must be rejected. > >> Similarly, while preparing a user-mode from kernel mode, the > >> aspect-ratio info must not be given, if aspect-ratio is not > >> supported by the user. > >> > >> This patch: > >> 1. adds a new bit field aspect_ratio_allowed in drm_atomic_state, > >> which is set only if the aspect-ratio is supported by the user. > >> 2. discards the aspect-ratio info from the user mode while > >> preparing kernel mode structure, during modeset, if the > >> user does not support aspect ratio. > >> 3. avoids setting the aspect-ratio info in user-mode, while > >> converting user-mode from the kernel-mode. > >> > >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > >> > >> V3: Addressed review comments from Ville: > >> -Do not corrupt the current crtc state by updating aspect ratio > >> on the fly. > >> --- > >> drivers/gpu/drm/drm_atomic.c | 61 +++++++++++++++++++++++++++++++++++++++++--- > >> drivers/gpu/drm/drm_crtc.c | 19 ++++++++++++++ > >> include/drm/drm_atomic.h | 2 ++ > >> 3 files changed, 79 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > >> index 69ff763..39313e2 100644 > >> --- a/drivers/gpu/drm/drm_atomic.c > >> +++ b/drivers/gpu/drm/drm_atomic.c > >> @@ -316,6 +316,35 @@ static s32 __user *get_out_fence_for_crtc(struct drm_atomic_state *state, > >> > >> return fence_ptr; > >> } > >> +/** > >> + * drm_atomic_allow_aspect_ratio_for_kmode > >> + * @state: the crtc state > >> + * @mode: kernel-internal mode, which is used to create a duplicate mode, > >> + * with updated picture aspect ratio. > >> + * > >> + * Allow the aspect ratio info in the kernel mode only if aspect ratio is > >> + * supported. > >> + * > >> + * RETURNS: > >> + * kernel-internal mode with updated picture aspect ratio value. > >> + */ > >> + > >> +struct drm_display_mode* > >> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state *state, > >> + const struct drm_display_mode *mode) > >> +{ > >> + struct drm_atomic_state *atomic_state = state->state; > >> + struct drm_display_mode *ar_kmode; > >> + > >> + ar_kmode = drm_mode_duplicate(state->crtc->dev, mode); > >> + /* > >> + * If aspect ratio is not supported, set the picture aspect ratio as > >> + * NONE. > >> + */ > >> + if (atomic_state && !atomic_state->allow_aspect_ratio) > >> + ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; > >> + return ar_kmode; > >> +} > >> > >> /** > >> * drm_atomic_set_mode_for_crtc - set mode for CRTC > >> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, > >> state->mode_blob = NULL; > >> > >> if (mode) { > >> - drm_mode_convert_to_umode(&umode, mode); > >> + struct drm_display_mode *ar_mode; > >> + > >> + ar_mode = drm_atomic_allow_aspect_ratio_for_kmode(state, mode); > >> + drm_mode_convert_to_umode(&umode, ar_mode); > > This still looks sketchy. > > > > I believe there are just two places where we need to filter out the > > aspect ratio flags from the umode, namely the getblob and getcrtc > > ioctls. > > AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob, > etc. apart from the mode blob. > Is there any way to check from blob id (or by any other means), > if the data is for the mode, or edid, or gamma etc. I think we'd have to somehow mark the mode blobs as special. Until we have more need for cleaning up blobs before returning them to userspace I think a simple flag should do. If we come up with more uses like this then it might make sense to introduce some kind of optional filter function for blobs. > > If we can check that, I can make sure that aspect-ratio flags are taken > care of, if the blob is for mode, > and the aspect ratio is not supported. > > > > > And for checking the user input I think we should probably just > > stick that into drm_mode_convert_umode(). Looks like we never call > > that from anywhere but the atomic/setprop and setcrtc paths with > > a non-NULL argument. > > > > I *think* those three places should be sufficient to cover everything. > Agreed. Infact I tried to do that first, but the only problem we have > here, is that, the file_priv > which has the aspect ratio capability information is not supplied to the > drm_mode_convert_umode() > or the functions calling, drm_mode_conver_umode(). At best we have > drm_crtc_state. Simply passing the file_priv down would seem like the most obvious solution. > > I have added an aspect ratio allowed flag in drm_crtc_state->state so > that its available in the > functions calling drm_mode_convert_umode. > > I am open to suggestion to avoid adding a new flag in drm_atomic_state, > if there is a better > way to let the drm_mode_convert_umode( ) know that user supports aspect > ratio capability. > > > >> state->mode_blob = > >> drm_property_create_blob(state->crtc->dev, > >> sizeof(umode), > >> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, > >> if (IS_ERR(state->mode_blob)) > >> return PTR_ERR(state->mode_blob); > >> > >> - drm_mode_copy(&state->mode, mode); > >> + drm_mode_copy(&state->mode, ar_mode); > >> state->enable = true; > >> DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n", > >> - mode->name, state); > >> + ar_mode->name, state); > >> + drm_mode_destroy(state->crtc->dev, ar_mode); > >> } else { > >> memset(&state->mode, 0, sizeof(state->mode)); > >> state->enable = false; > >> @@ -436,6 +469,25 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, > >> } > >> > >> /** > >> + * drm_atomic_allow_aspect_ratio_for_umode > >> + * @state: the crtc state > >> + * @umode: userspace mode, whose aspect ratio flag bits are to be updated. > >> + * > >> + * Allow the aspect ratio info in the userspace mode only if aspect ratio is > >> + * supported. > >> + */ > >> +void > >> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state *state, > >> + struct drm_mode_modeinfo *umode) > >> +{ > >> + struct drm_atomic_state *atomic_state = state->state; > >> + > >> + /* Reset the aspect ratio flags if aspect ratio is not supported */ > >> + if (atomic_state && !atomic_state->allow_aspect_ratio) > >> + umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); > >> +} > >> + > >> +/** > >> * drm_atomic_crtc_set_property - set property on CRTC > >> * @crtc: the drm CRTC to set a property on > >> * @state: the state object to update with the new property value > >> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, > >> else if (property == config->prop_mode_id) { > >> struct drm_property_blob *mode = > >> drm_property_lookup_blob(dev, val); > >> + drm_atomic_allow_aspect_ratio_for_umode(state, > >> + (struct drm_mode_modeinfo *) > >> + mode->data); > >> ret = drm_atomic_set_mode_prop_for_crtc(state, mode); > >> drm_property_blob_put(mode); > >> return ret; > >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > >> index f0556e6..a2d34fa 100644 > >> --- a/drivers/gpu/drm/drm_crtc.c > >> +++ b/drivers/gpu/drm/drm_crtc.c > >> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev, > >> drm_modeset_lock(&crtc->mutex, NULL); > >> if (crtc->state) { > >> if (crtc->state->enable) { > >> + /* > >> + * If the aspect-ratio is not required by the, > >> + * userspace, do not set the aspect-ratio flags. > >> + */ > >> + if (!file_priv->aspect_ratio_allowed) > >> + crtc->state->mode.picture_aspect_ratio = > >> + HDMI_PICTURE_ASPECT_NONE; > > These would still clobber the current crtc state. So definitely don't > > want to do this. > Alright, so alternative way of doing this will be: > Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel > mode structure, > but set the aspect ratio flags in the usermode to none, after calling the > drm_mode_convert_to_umode(). > > Will take care of this in the next patchset. > > > > >> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); > >> crtc_resp->mode_valid = 1; > >> > >> @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device *dev, > >> crtc_resp->x = crtc->x; > >> crtc_resp->y = crtc->y; > >> if (crtc->enabled) { > >> + /* > >> + * If the aspect-ratio is not required by the, > >> + * userspace, do not set the aspect-ratio flags. > >> + */ > >> + if (!file_priv->aspect_ratio_allowed) > >> + crtc->mode.picture_aspect_ratio = > >> + HDMI_PICTURE_ASPECT_NONE; > >> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode); > >> crtc_resp->mode_valid = 1; > >> > >> @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, > >> goto out; > >> } > >> > >> + /* If the user does not ask for aspect ratio, reset the aspect > >> + * ratio bits in the usermode. > >> + */ > >> + if (!file_priv->aspect_ratio_allowed) > >> + crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); > >> ret = drm_mode_convert_umode(mode, &crtc_req->mode); > >> if (ret) { > >> DRM_DEBUG_KMS("Invalid mode\n"); > >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > >> index 1c27526..130dad9 100644 > >> --- a/include/drm/drm_atomic.h > >> +++ b/include/drm/drm_atomic.h > >> @@ -237,6 +237,7 @@ struct __drm_private_objs_state { > >> * @ref: count of all references to this state (will not be freed until zero) > >> * @dev: parent DRM device > >> * @allow_modeset: allow full modeset > >> + * @allow_aspect_ratio: allow the aspect ratio info to be passes to user > >> * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics > >> * @async_update: hint for asynchronous plane update > >> * @planes: pointer to array of structures with per-plane data > >> @@ -256,6 +257,7 @@ struct drm_atomic_state { > >> > >> struct drm_device *dev; > >> bool allow_modeset : 1; > >> + bool allow_aspect_ratio : 1; > >> bool legacy_cursor_update : 1; > >> bool async_update : 1; > >> struct __drm_planes_state *planes; > >> -- > >> 2.7.4 > >> > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths 2018-01-31 13:09 ` Ville Syrjälä @ 2018-02-01 11:05 ` Nautiyal, Ankit K 2018-02-01 12:54 ` Ville Syrjälä 0 siblings, 1 reply; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-02-01 11:05 UTC (permalink / raw) To: Ville Syrjälä; +Cc: dri-devel [-- Attachment #1.1: Type: text/plain, Size: 11780 bytes --] Hi Ville, Appreciate your time and the suggestions. Please find my response inline: On 1/31/2018 6:39 PM, Ville Syrjälä wrote: > On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote: >> >> On 1/30/2018 12:23 AM, Ville Syrjälä wrote: >>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote: >>>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>>> >>>> If the user mode does not support aspect-ratio, and requests for >>>> a modeset, then the flag bits representing aspect ratio in the >>>> given user-mode must be rejected. >>>> Similarly, while preparing a user-mode from kernel mode, the >>>> aspect-ratio info must not be given, if aspect-ratio is not >>>> supported by the user. >>>> >>>> This patch: >>>> 1. adds a new bit field aspect_ratio_allowed in drm_atomic_state, >>>> which is set only if the aspect-ratio is supported by the user. >>>> 2. discards the aspect-ratio info from the user mode while >>>> preparing kernel mode structure, during modeset, if the >>>> user does not support aspect ratio. >>>> 3. avoids setting the aspect-ratio info in user-mode, while >>>> converting user-mode from the kernel-mode. >>>> >>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>>> >>>> V3: Addressed review comments from Ville: >>>> -Do not corrupt the current crtc state by updating aspect ratio >>>> on the fly. >>>> --- >>>> drivers/gpu/drm/drm_atomic.c | 61 +++++++++++++++++++++++++++++++++++++++++--- >>>> drivers/gpu/drm/drm_crtc.c | 19 ++++++++++++++ >>>> include/drm/drm_atomic.h | 2 ++ >>>> 3 files changed, 79 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >>>> index 69ff763..39313e2 100644 >>>> --- a/drivers/gpu/drm/drm_atomic.c >>>> +++ b/drivers/gpu/drm/drm_atomic.c >>>> @@ -316,6 +316,35 @@ static s32 __user *get_out_fence_for_crtc(struct drm_atomic_state *state, >>>> >>>> return fence_ptr; >>>> } >>>> +/** >>>> + * drm_atomic_allow_aspect_ratio_for_kmode >>>> + * @state: the crtc state >>>> + * @mode: kernel-internal mode, which is used to create a duplicate mode, >>>> + * with updated picture aspect ratio. >>>> + * >>>> + * Allow the aspect ratio info in the kernel mode only if aspect ratio is >>>> + * supported. >>>> + * >>>> + * RETURNS: >>>> + * kernel-internal mode with updated picture aspect ratio value. >>>> + */ >>>> + >>>> +struct drm_display_mode* >>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state *state, >>>> + const struct drm_display_mode *mode) >>>> +{ >>>> + struct drm_atomic_state *atomic_state = state->state; >>>> + struct drm_display_mode *ar_kmode; >>>> + >>>> + ar_kmode = drm_mode_duplicate(state->crtc->dev, mode); >>>> + /* >>>> + * If aspect ratio is not supported, set the picture aspect ratio as >>>> + * NONE. >>>> + */ >>>> + if (atomic_state && !atomic_state->allow_aspect_ratio) >>>> + ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; >>>> + return ar_kmode; >>>> +} >>>> >>>> /** >>>> * drm_atomic_set_mode_for_crtc - set mode for CRTC >>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, >>>> state->mode_blob = NULL; >>>> >>>> if (mode) { >>>> - drm_mode_convert_to_umode(&umode, mode); >>>> + struct drm_display_mode *ar_mode; >>>> + >>>> + ar_mode = drm_atomic_allow_aspect_ratio_for_kmode(state, mode); >>>> + drm_mode_convert_to_umode(&umode, ar_mode); >>> This still looks sketchy. >>> >>> I believe there are just two places where we need to filter out the >>> aspect ratio flags from the umode, namely the getblob and getcrtc >>> ioctls. >> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob, >> etc. apart from the mode blob. >> Is there any way to check from blob id (or by any other means), >> if the data is for the mode, or edid, or gamma etc. > I think we'd have to somehow mark the mode blobs as special. Until we > have more need for cleaning up blobs before returning them to userspace > I think a simple flag should do. If we come up with more uses like this > then it might make sense to introduce some kind of optional filter > function for blobs. If my understanding is correct, 1. To be able to do this, we need to change in uapi. 2. Whenever the userspace creates a blob for mode, the new flag/class for blob type needs to be set. Do you think that it's worth the effort? >> If we can check that, I can make sure that aspect-ratio flags are taken >> care of, if the blob is for mode, >> and the aspect ratio is not supported. >> >>> And for checking the user input I think we should probably just >>> stick that into drm_mode_convert_umode(). Looks like we never call >>> that from anywhere but the atomic/setprop and setcrtc paths with >>> a non-NULL argument. >>> >>> I *think* those three places should be sufficient to cover everything. >> Agreed. Infact I tried to do that first, but the only problem we have >> here, is that, the file_priv >> which has the aspect ratio capability information is not supplied to the >> drm_mode_convert_umode() >> or the functions calling, drm_mode_conver_umode(). At best we have >> drm_crtc_state. > Simply passing the file_priv down would seem like the most obvious > solution. This means we have to make change in various drivers and we have to pull the file_priv parameter three levels down. Should we think of a better way for doing this? >> I have added an aspect ratio allowed flag in drm_crtc_state->state so >> that its available in the >> functions calling drm_mode_convert_umode. >> >> I am open to suggestion to avoid adding a new flag in drm_atomic_state, >> if there is a better >> way to let the drm_mode_convert_umode( ) know that user supports aspect >> ratio capability. >>>> state->mode_blob = >>>> drm_property_create_blob(state->crtc->dev, >>>> sizeof(umode), >>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, >>>> if (IS_ERR(state->mode_blob)) >>>> return PTR_ERR(state->mode_blob); >>>> >>>> - drm_mode_copy(&state->mode, mode); >>>> + drm_mode_copy(&state->mode, ar_mode); >>>> state->enable = true; >>>> DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n", >>>> - mode->name, state); >>>> + ar_mode->name, state); >>>> + drm_mode_destroy(state->crtc->dev, ar_mode); >>>> } else { >>>> memset(&state->mode, 0, sizeof(state->mode)); >>>> state->enable = false; >>>> @@ -436,6 +469,25 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, >>>> } >>>> >>>> /** >>>> + * drm_atomic_allow_aspect_ratio_for_umode >>>> + * @state: the crtc state >>>> + * @umode: userspace mode, whose aspect ratio flag bits are to be updated. >>>> + * >>>> + * Allow the aspect ratio info in the userspace mode only if aspect ratio is >>>> + * supported. >>>> + */ >>>> +void >>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state *state, >>>> + struct drm_mode_modeinfo *umode) >>>> +{ >>>> + struct drm_atomic_state *atomic_state = state->state; >>>> + >>>> + /* Reset the aspect ratio flags if aspect ratio is not supported */ >>>> + if (atomic_state && !atomic_state->allow_aspect_ratio) >>>> + umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); >>>> +} >>>> + >>>> +/** >>>> * drm_atomic_crtc_set_property - set property on CRTC >>>> * @crtc: the drm CRTC to set a property on >>>> * @state: the state object to update with the new property value >>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, >>>> else if (property == config->prop_mode_id) { >>>> struct drm_property_blob *mode = >>>> drm_property_lookup_blob(dev, val); >>>> + drm_atomic_allow_aspect_ratio_for_umode(state, >>>> + (struct drm_mode_modeinfo *) >>>> + mode->data); >>>> ret = drm_atomic_set_mode_prop_for_crtc(state, mode); >>>> drm_property_blob_put(mode); >>>> return ret; >>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >>>> index f0556e6..a2d34fa 100644 >>>> --- a/drivers/gpu/drm/drm_crtc.c >>>> +++ b/drivers/gpu/drm/drm_crtc.c >>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev, >>>> drm_modeset_lock(&crtc->mutex, NULL); >>>> if (crtc->state) { >>>> if (crtc->state->enable) { >>>> + /* >>>> + * If the aspect-ratio is not required by the, >>>> + * userspace, do not set the aspect-ratio flags. >>>> + */ >>>> + if (!file_priv->aspect_ratio_allowed) >>>> + crtc->state->mode.picture_aspect_ratio = >>>> + HDMI_PICTURE_ASPECT_NONE; >>> These would still clobber the current crtc state. So definitely don't >>> want to do this. >> Alright, so alternative way of doing this will be: >> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel >> mode structure, >> but set the aspect ratio flags in the usermode to none, after calling the >> drm_mode_convert_to_umode(). >> >> Will take care of this in the next patchset. I gave a thought about it again. As you have mentioned earlier that get_crtc is one of the place where the user mode aspect ratio flags must be filtered out, is this what you have in mind. I hope I did not misunderstand. Changing the usermode aspect ratio flag bits, without change in the picture_aspect_ratio kernel mode will not result in both structures out of sync? >> >>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); >>>> crtc_resp->mode_valid = 1; >>>> >>>> @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device *dev, >>>> crtc_resp->x = crtc->x; >>>> crtc_resp->y = crtc->y; >>>> if (crtc->enabled) { >>>> + /* >>>> + * If the aspect-ratio is not required by the, >>>> + * userspace, do not set the aspect-ratio flags. >>>> + */ >>>> + if (!file_priv->aspect_ratio_allowed) >>>> + crtc->mode.picture_aspect_ratio = >>>> + HDMI_PICTURE_ASPECT_NONE; >>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode); >>>> crtc_resp->mode_valid = 1; >>>> >>>> @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, >>>> goto out; >>>> } >>>> >>>> + /* If the user does not ask for aspect ratio, reset the aspect >>>> + * ratio bits in the usermode. >>>> + */ >>>> + if (!file_priv->aspect_ratio_allowed) >>>> + crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); >>>> ret = drm_mode_convert_umode(mode, &crtc_req->mode); >>>> if (ret) { >>>> DRM_DEBUG_KMS("Invalid mode\n"); >>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h >>>> index 1c27526..130dad9 100644 >>>> --- a/include/drm/drm_atomic.h >>>> +++ b/include/drm/drm_atomic.h >>>> @@ -237,6 +237,7 @@ struct __drm_private_objs_state { >>>> * @ref: count of all references to this state (will not be freed until zero) >>>> * @dev: parent DRM device >>>> * @allow_modeset: allow full modeset >>>> + * @allow_aspect_ratio: allow the aspect ratio info to be passes to user >>>> * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics >>>> * @async_update: hint for asynchronous plane update >>>> * @planes: pointer to array of structures with per-plane data >>>> @@ -256,6 +257,7 @@ struct drm_atomic_state { >>>> >>>> struct drm_device *dev; >>>> bool allow_modeset : 1; >>>> + bool allow_aspect_ratio : 1; >>>> bool legacy_cursor_update : 1; >>>> bool async_update : 1; >>>> struct __drm_planes_state *planes; >>>> -- >>>> 2.7.4 >>>> >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel Regards, Ankit [-- Attachment #1.2: Type: text/html, Size: 13497 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths 2018-02-01 11:05 ` Nautiyal, Ankit K @ 2018-02-01 12:54 ` Ville Syrjälä 2018-02-08 4:29 ` Nautiyal, Ankit K 0 siblings, 1 reply; 35+ messages in thread From: Ville Syrjälä @ 2018-02-01 12:54 UTC (permalink / raw) To: Nautiyal, Ankit K; +Cc: dri-devel On Thu, Feb 01, 2018 at 04:35:22PM +0530, Nautiyal, Ankit K wrote: > Hi Ville, > > Appreciate your time and the suggestions. > Please find my response inline: > > On 1/31/2018 6:39 PM, Ville Syrjälä wrote: > > On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote: > >> > >> On 1/30/2018 12:23 AM, Ville Syrjälä wrote: > >>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote: > >>>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > >>>> > >>>> If the user mode does not support aspect-ratio, and requests for > >>>> a modeset, then the flag bits representing aspect ratio in the > >>>> given user-mode must be rejected. > >>>> Similarly, while preparing a user-mode from kernel mode, the > >>>> aspect-ratio info must not be given, if aspect-ratio is not > >>>> supported by the user. > >>>> > >>>> This patch: > >>>> 1. adds a new bit field aspect_ratio_allowed in drm_atomic_state, > >>>> which is set only if the aspect-ratio is supported by the user. > >>>> 2. discards the aspect-ratio info from the user mode while > >>>> preparing kernel mode structure, during modeset, if the > >>>> user does not support aspect ratio. > >>>> 3. avoids setting the aspect-ratio info in user-mode, while > >>>> converting user-mode from the kernel-mode. > >>>> > >>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > >>>> > >>>> V3: Addressed review comments from Ville: > >>>> -Do not corrupt the current crtc state by updating aspect ratio > >>>> on the fly. > >>>> --- > >>>> drivers/gpu/drm/drm_atomic.c | 61 +++++++++++++++++++++++++++++++++++++++++--- > >>>> drivers/gpu/drm/drm_crtc.c | 19 ++++++++++++++ > >>>> include/drm/drm_atomic.h | 2 ++ > >>>> 3 files changed, 79 insertions(+), 3 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > >>>> index 69ff763..39313e2 100644 > >>>> --- a/drivers/gpu/drm/drm_atomic.c > >>>> +++ b/drivers/gpu/drm/drm_atomic.c > >>>> @@ -316,6 +316,35 @@ static s32 __user *get_out_fence_for_crtc(struct drm_atomic_state *state, > >>>> > >>>> return fence_ptr; > >>>> } > >>>> +/** > >>>> + * drm_atomic_allow_aspect_ratio_for_kmode > >>>> + * @state: the crtc state > >>>> + * @mode: kernel-internal mode, which is used to create a duplicate mode, > >>>> + * with updated picture aspect ratio. > >>>> + * > >>>> + * Allow the aspect ratio info in the kernel mode only if aspect ratio is > >>>> + * supported. > >>>> + * > >>>> + * RETURNS: > >>>> + * kernel-internal mode with updated picture aspect ratio value. > >>>> + */ > >>>> + > >>>> +struct drm_display_mode* > >>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state *state, > >>>> + const struct drm_display_mode *mode) > >>>> +{ > >>>> + struct drm_atomic_state *atomic_state = state->state; > >>>> + struct drm_display_mode *ar_kmode; > >>>> + > >>>> + ar_kmode = drm_mode_duplicate(state->crtc->dev, mode); > >>>> + /* > >>>> + * If aspect ratio is not supported, set the picture aspect ratio as > >>>> + * NONE. > >>>> + */ > >>>> + if (atomic_state && !atomic_state->allow_aspect_ratio) > >>>> + ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; > >>>> + return ar_kmode; > >>>> +} > >>>> > >>>> /** > >>>> * drm_atomic_set_mode_for_crtc - set mode for CRTC > >>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, > >>>> state->mode_blob = NULL; > >>>> > >>>> if (mode) { > >>>> - drm_mode_convert_to_umode(&umode, mode); > >>>> + struct drm_display_mode *ar_mode; > >>>> + > >>>> + ar_mode = drm_atomic_allow_aspect_ratio_for_kmode(state, mode); > >>>> + drm_mode_convert_to_umode(&umode, ar_mode); > >>> This still looks sketchy. > >>> > >>> I believe there are just two places where we need to filter out the > >>> aspect ratio flags from the umode, namely the getblob and getcrtc > >>> ioctls. > >> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob, > >> etc. apart from the mode blob. > >> Is there any way to check from blob id (or by any other means), > >> if the data is for the mode, or edid, or gamma etc. > > I think we'd have to somehow mark the mode blobs as special. Until we > > have more need for cleaning up blobs before returning them to userspace > > I think a simple flag should do. If we come up with more uses like this > > then it might make sense to introduce some kind of optional filter > > function for blobs. > If my understanding is correct, > 1. To be able to do this, we need to change in uapi. We should be fine with an internal flag. Obviously it won't be set for blobs freshly created by userspace, but that's fine because we do no other error checking for them either. The error checking will happen when the user tries to actually use the blob. > 2. Whenever the userspace creates a blob for mode, the new flag/class > for blob type needs to be set. > > Do you think that it's worth the effort? > >> If we can check that, I can make sure that aspect-ratio flags are taken > >> care of, if the blob is for mode, > >> and the aspect ratio is not supported. > >> > >>> And for checking the user input I think we should probably just > >>> stick that into drm_mode_convert_umode(). Looks like we never call > >>> that from anywhere but the atomic/setprop and setcrtc paths with > >>> a non-NULL argument. > >>> > >>> I *think* those three places should be sufficient to cover everything. > >> Agreed. Infact I tried to do that first, but the only problem we have > >> here, is that, the file_priv > >> which has the aspect ratio capability information is not supplied to the > >> drm_mode_convert_umode() > >> or the functions calling, drm_mode_conver_umode(). At best we have > >> drm_crtc_state. > > Simply passing the file_priv down would seem like the most obvious > > solution. > > This means we have to make change in various drivers and we have to pull > the file_priv > parameter three levels down. Should we think of a better way for doing this? Coccinelle is pretty good at modifying function parameters. > > >> I have added an aspect ratio allowed flag in drm_crtc_state->state so > >> that its available in the > >> functions calling drm_mode_convert_umode. > >> > >> I am open to suggestion to avoid adding a new flag in drm_atomic_state, > >> if there is a better > >> way to let the drm_mode_convert_umode( ) know that user supports aspect > >> ratio capability. > >>>> state->mode_blob = > >>>> drm_property_create_blob(state->crtc->dev, > >>>> sizeof(umode), > >>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, > >>>> if (IS_ERR(state->mode_blob)) > >>>> return PTR_ERR(state->mode_blob); > >>>> > >>>> - drm_mode_copy(&state->mode, mode); > >>>> + drm_mode_copy(&state->mode, ar_mode); > >>>> state->enable = true; > >>>> DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n", > >>>> - mode->name, state); > >>>> + ar_mode->name, state); > >>>> + drm_mode_destroy(state->crtc->dev, ar_mode); > >>>> } else { > >>>> memset(&state->mode, 0, sizeof(state->mode)); > >>>> state->enable = false; > >>>> @@ -436,6 +469,25 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, > >>>> } > >>>> > >>>> /** > >>>> + * drm_atomic_allow_aspect_ratio_for_umode > >>>> + * @state: the crtc state > >>>> + * @umode: userspace mode, whose aspect ratio flag bits are to be updated. > >>>> + * > >>>> + * Allow the aspect ratio info in the userspace mode only if aspect ratio is > >>>> + * supported. > >>>> + */ > >>>> +void > >>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state *state, > >>>> + struct drm_mode_modeinfo *umode) > >>>> +{ > >>>> + struct drm_atomic_state *atomic_state = state->state; > >>>> + > >>>> + /* Reset the aspect ratio flags if aspect ratio is not supported */ > >>>> + if (atomic_state && !atomic_state->allow_aspect_ratio) > >>>> + umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); > >>>> +} > >>>> + > >>>> +/** > >>>> * drm_atomic_crtc_set_property - set property on CRTC > >>>> * @crtc: the drm CRTC to set a property on > >>>> * @state: the state object to update with the new property value > >>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, > >>>> else if (property == config->prop_mode_id) { > >>>> struct drm_property_blob *mode = > >>>> drm_property_lookup_blob(dev, val); > >>>> + drm_atomic_allow_aspect_ratio_for_umode(state, > >>>> + (struct drm_mode_modeinfo *) > >>>> + mode->data); > >>>> ret = drm_atomic_set_mode_prop_for_crtc(state, mode); > >>>> drm_property_blob_put(mode); > >>>> return ret; > >>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > >>>> index f0556e6..a2d34fa 100644 > >>>> --- a/drivers/gpu/drm/drm_crtc.c > >>>> +++ b/drivers/gpu/drm/drm_crtc.c > >>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev, > >>>> drm_modeset_lock(&crtc->mutex, NULL); > >>>> if (crtc->state) { > >>>> if (crtc->state->enable) { > >>>> + /* > >>>> + * If the aspect-ratio is not required by the, > >>>> + * userspace, do not set the aspect-ratio flags. > >>>> + */ > >>>> + if (!file_priv->aspect_ratio_allowed) > >>>> + crtc->state->mode.picture_aspect_ratio = > >>>> + HDMI_PICTURE_ASPECT_NONE; > >>> These would still clobber the current crtc state. So definitely don't > >>> want to do this. > >> Alright, so alternative way of doing this will be: > >> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel > >> mode structure, > >> but set the aspect ratio flags in the usermode to none, after calling the > >> drm_mode_convert_to_umode(). > >> > >> Will take care of this in the next patchset. > > I gave a thought about it again. As you have mentioned earlier that > get_crtc is one of the > place where the user mode aspect ratio flags must be filtered out, is > this what you have > in mind. I hope I did not misunderstand. > > Changing the usermode aspect ratio flag bits, without change in the > picture_aspect_ratio > kernel mode will not result in both structures out of sync? I don't think that should really matter. I suppose we might get one extra modeset when the user feeds that mode back via setcrtc/atomic, but meh. And actually the aspect ratio should only affect the infoframe, so we should be able to eventually eliminate that extra modeset and just update the infoframe instead. > > >> > >>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); > >>>> crtc_resp->mode_valid = 1; > >>>> > >>>> @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device *dev, > >>>> crtc_resp->x = crtc->x; > >>>> crtc_resp->y = crtc->y; > >>>> if (crtc->enabled) { > >>>> + /* > >>>> + * If the aspect-ratio is not required by the, > >>>> + * userspace, do not set the aspect-ratio flags. > >>>> + */ > >>>> + if (!file_priv->aspect_ratio_allowed) > >>>> + crtc->mode.picture_aspect_ratio = > >>>> + HDMI_PICTURE_ASPECT_NONE; > >>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode); > >>>> crtc_resp->mode_valid = 1; > >>>> > >>>> @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, > >>>> goto out; > >>>> } > >>>> > >>>> + /* If the user does not ask for aspect ratio, reset the aspect > >>>> + * ratio bits in the usermode. > >>>> + */ > >>>> + if (!file_priv->aspect_ratio_allowed) > >>>> + crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); > >>>> ret = drm_mode_convert_umode(mode, &crtc_req->mode); > >>>> if (ret) { > >>>> DRM_DEBUG_KMS("Invalid mode\n"); > >>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > >>>> index 1c27526..130dad9 100644 > >>>> --- a/include/drm/drm_atomic.h > >>>> +++ b/include/drm/drm_atomic.h > >>>> @@ -237,6 +237,7 @@ struct __drm_private_objs_state { > >>>> * @ref: count of all references to this state (will not be freed until zero) > >>>> * @dev: parent DRM device > >>>> * @allow_modeset: allow full modeset > >>>> + * @allow_aspect_ratio: allow the aspect ratio info to be passes to user > >>>> * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics > >>>> * @async_update: hint for asynchronous plane update > >>>> * @planes: pointer to array of structures with per-plane data > >>>> @@ -256,6 +257,7 @@ struct drm_atomic_state { > >>>> > >>>> struct drm_device *dev; > >>>> bool allow_modeset : 1; > >>>> + bool allow_aspect_ratio : 1; > >>>> bool legacy_cursor_update : 1; > >>>> bool async_update : 1; > >>>> struct __drm_planes_state *planes; > >>>> -- > >>>> 2.7.4 > >>>> > >>>> _______________________________________________ > >>>> dri-devel mailing list > >>>> dri-devel@lists.freedesktop.org > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > Regards, > Ankit -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths 2018-02-01 12:54 ` Ville Syrjälä @ 2018-02-08 4:29 ` Nautiyal, Ankit K 2018-02-13 4:51 ` Nautiyal, Ankit K 0 siblings, 1 reply; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-02-08 4:29 UTC (permalink / raw) To: Ville Syrjälä; +Cc: dri-devel Hi Ville, I still have some queries regarding the handling of aspect ratio flags in getblob ioctl. Please find below my responses inline. On 2/1/2018 6:24 PM, Ville Syrjälä wrote: > On Thu, Feb 01, 2018 at 04:35:22PM +0530, Nautiyal, Ankit K wrote: >> Hi Ville, >> >> Appreciate your time and the suggestions. >> Please find my response inline: >> >> On 1/31/2018 6:39 PM, Ville Syrjälä wrote: >>> On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote: >>>> On 1/30/2018 12:23 AM, Ville Syrjälä wrote: >>>>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote: >>>>>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>>>>> >>>>>> If the user mode does not support aspect-ratio, and requests for >>>>>> a modeset, then the flag bits representing aspect ratio in the >>>>>> given user-mode must be rejected. >>>>>> Similarly, while preparing a user-mode from kernel mode, the >>>>>> aspect-ratio info must not be given, if aspect-ratio is not >>>>>> supported by the user. >>>>>> >>>>>> This patch: >>>>>> 1. adds a new bit field aspect_ratio_allowed in drm_atomic_state, >>>>>> which is set only if the aspect-ratio is supported by the user. >>>>>> 2. discards the aspect-ratio info from the user mode while >>>>>> preparing kernel mode structure, during modeset, if the >>>>>> user does not support aspect ratio. >>>>>> 3. avoids setting the aspect-ratio info in user-mode, while >>>>>> converting user-mode from the kernel-mode. >>>>>> >>>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>>>>> >>>>>> V3: Addressed review comments from Ville: >>>>>> -Do not corrupt the current crtc state by updating aspect ratio >>>>>> on the fly. >>>>>> --- >>>>>> drivers/gpu/drm/drm_atomic.c | 61 +++++++++++++++++++++++++++++++++++++++++--- >>>>>> drivers/gpu/drm/drm_crtc.c | 19 ++++++++++++++ >>>>>> include/drm/drm_atomic.h | 2 ++ >>>>>> 3 files changed, 79 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >>>>>> index 69ff763..39313e2 100644 >>>>>> --- a/drivers/gpu/drm/drm_atomic.c >>>>>> +++ b/drivers/gpu/drm/drm_atomic.c >>>>>> @@ -316,6 +316,35 @@ static s32 __user *get_out_fence_for_crtc(struct drm_atomic_state *state, >>>>>> >>>>>> return fence_ptr; >>>>>> } >>>>>> +/** >>>>>> + * drm_atomic_allow_aspect_ratio_for_kmode >>>>>> + * @state: the crtc state >>>>>> + * @mode: kernel-internal mode, which is used to create a duplicate mode, >>>>>> + * with updated picture aspect ratio. >>>>>> + * >>>>>> + * Allow the aspect ratio info in the kernel mode only if aspect ratio is >>>>>> + * supported. >>>>>> + * >>>>>> + * RETURNS: >>>>>> + * kernel-internal mode with updated picture aspect ratio value. >>>>>> + */ >>>>>> + >>>>>> +struct drm_display_mode* >>>>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state *state, >>>>>> + const struct drm_display_mode *mode) >>>>>> +{ >>>>>> + struct drm_atomic_state *atomic_state = state->state; >>>>>> + struct drm_display_mode *ar_kmode; >>>>>> + >>>>>> + ar_kmode = drm_mode_duplicate(state->crtc->dev, mode); >>>>>> + /* >>>>>> + * If aspect ratio is not supported, set the picture aspect ratio as >>>>>> + * NONE. >>>>>> + */ >>>>>> + if (atomic_state && !atomic_state->allow_aspect_ratio) >>>>>> + ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; >>>>>> + return ar_kmode; >>>>>> +} >>>>>> >>>>>> /** >>>>>> * drm_atomic_set_mode_for_crtc - set mode for CRTC >>>>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, >>>>>> state->mode_blob = NULL; >>>>>> >>>>>> if (mode) { >>>>>> - drm_mode_convert_to_umode(&umode, mode); >>>>>> + struct drm_display_mode *ar_mode; >>>>>> + >>>>>> + ar_mode = drm_atomic_allow_aspect_ratio_for_kmode(state, mode); >>>>>> + drm_mode_convert_to_umode(&umode, ar_mode); >>>>> This still looks sketchy. >>>>> >>>>> I believe there are just two places where we need to filter out the >>>>> aspect ratio flags from the umode, namely the getblob and getcrtc >>>>> ioctls. >>>> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob, >>>> etc. apart from the mode blob. >>>> Is there any way to check from blob id (or by any other means), >>>> if the data is for the mode, or edid, or gamma etc. >>> I think we'd have to somehow mark the mode blobs as special. Until we >>> have more need for cleaning up blobs before returning them to userspace >>> I think a simple flag should do. If we come up with more uses like this >>> then it might make sense to introduce some kind of optional filter >>> function for blobs. >> If my understanding is correct, >> 1. To be able to do this, we need to change in uapi. > We should be fine with an internal flag. Obviously it won't be set > for blobs freshly created by userspace, but that's fine because we > do no other error checking for them either. The error checking will > happen when the user tries to actually use the blob. I am not sure why getblob ioctl should even come into picture. (Perhaps I am missing something). As per my understanding: If a userspace needs to set a mode, it will just create a blob with drm_mode_mode_info, and get the blob-id. This blob-id would be supplied to drm_mode_atomic_ioctl as a crtc property mode_id. At the kernel side, the crtc property mode_id is set by looking-up for mode blob from blob id. I am doing the change in the user mode aspect-ratio flags at this junction. As far as getblob ioctl is concerned, if the user does call getblob ioctl for getting mode blob from the blob id, it will receive the same mode blob, with same aspect ratio info which the user had created using create blob ioctl. If it does want to use the blob, it has to come through the drm_mode_atomic_ioctl way, where aspect ratio flags are handled. >> 2. Whenever the userspace creates a blob for mode, the new flag/class >> for blob type needs to be set. >> >> Do you think that it's worth the effort? >>>> If we can check that, I can make sure that aspect-ratio flags are taken >>>> care of, if the blob is for mode, >>>> and the aspect ratio is not supported. >>>> >>>>> And for checking the user input I think we should probably just >>>>> stick that into drm_mode_convert_umode(). Looks like we never call >>>>> that from anywhere but the atomic/setprop and setcrtc paths with >>>>> a non-NULL argument. >>>>> >>>>> I *think* those three places should be sufficient to cover everything. >>>> Agreed. Infact I tried to do that first, but the only problem we have >>>> here, is that, the file_priv >>>> which has the aspect ratio capability information is not supplied to the >>>> drm_mode_convert_umode() >>>> or the functions calling, drm_mode_conver_umode(). At best we have >>>> drm_crtc_state. >>> Simply passing the file_priv down would seem like the most obvious >>> solution. >> This means we have to make change in various drivers and we have to pull >> the file_priv >> parameter three levels down. Should we think of a better way for doing this? > Coccinelle is pretty good at modifying function parameters. Thanks for pointing out Coccinelle, will defintely try that out. But my question here is whether we really want to drag the file_priv from drm_mode_atomic_ioctl --> drm_mode_atomic_set_property --> drm_atomic_crtc_set_property --> drm_mode_set_mode_prop_for_crtc--> drm_mode_convert_umode. Instead, I tried to add a new field (allow_aspect_ratio) in drm_crtc_state which is already passed from the drm_mode_atomic_ioctl all the way to drm_mode_convert_umode. -Regards Ankit > >>>> I have added an aspect ratio allowed flag in drm_crtc_state->state so >>>> that its available in the >>>> functions calling drm_mode_convert_umode. >>>> >>>> I am open to suggestion to avoid adding a new flag in drm_atomic_state, >>>> if there is a better >>>> way to let the drm_mode_convert_umode( ) know that user supports aspect >>>> ratio capability. >>>>>> state->mode_blob = >>>>>> drm_property_create_blob(state->crtc->dev, >>>>>> sizeof(umode), >>>>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, >>>>>> if (IS_ERR(state->mode_blob)) >>>>>> return PTR_ERR(state->mode_blob); >>>>>> >>>>>> - drm_mode_copy(&state->mode, mode); >>>>>> + drm_mode_copy(&state->mode, ar_mode); >>>>>> state->enable = true; >>>>>> DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n", >>>>>> - mode->name, state); >>>>>> + ar_mode->name, state); >>>>>> + drm_mode_destroy(state->crtc->dev, ar_mode); >>>>>> } else { >>>>>> memset(&state->mode, 0, sizeof(state->mode)); >>>>>> state->enable = false; >>>>>> @@ -436,6 +469,25 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, >>>>>> } >>>>>> >>>>>> /** >>>>>> + * drm_atomic_allow_aspect_ratio_for_umode >>>>>> + * @state: the crtc state >>>>>> + * @umode: userspace mode, whose aspect ratio flag bits are to be updated. >>>>>> + * >>>>>> + * Allow the aspect ratio info in the userspace mode only if aspect ratio is >>>>>> + * supported. >>>>>> + */ >>>>>> +void >>>>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state *state, >>>>>> + struct drm_mode_modeinfo *umode) >>>>>> +{ >>>>>> + struct drm_atomic_state *atomic_state = state->state; >>>>>> + >>>>>> + /* Reset the aspect ratio flags if aspect ratio is not supported */ >>>>>> + if (atomic_state && !atomic_state->allow_aspect_ratio) >>>>>> + umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); >>>>>> +} >>>>>> + >>>>>> +/** >>>>>> * drm_atomic_crtc_set_property - set property on CRTC >>>>>> * @crtc: the drm CRTC to set a property on >>>>>> * @state: the state object to update with the new property value >>>>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, >>>>>> else if (property == config->prop_mode_id) { >>>>>> struct drm_property_blob *mode = >>>>>> drm_property_lookup_blob(dev, val); >>>>>> + drm_atomic_allow_aspect_ratio_for_umode(state, >>>>>> + (struct drm_mode_modeinfo *) >>>>>> + mode->data); >>>>>> ret = drm_atomic_set_mode_prop_for_crtc(state, mode); >>>>>> drm_property_blob_put(mode); >>>>>> return ret; >>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >>>>>> index f0556e6..a2d34fa 100644 >>>>>> --- a/drivers/gpu/drm/drm_crtc.c >>>>>> +++ b/drivers/gpu/drm/drm_crtc.c >>>>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev, >>>>>> drm_modeset_lock(&crtc->mutex, NULL); >>>>>> if (crtc->state) { >>>>>> if (crtc->state->enable) { >>>>>> + /* >>>>>> + * If the aspect-ratio is not required by the, >>>>>> + * userspace, do not set the aspect-ratio flags. >>>>>> + */ >>>>>> + if (!file_priv->aspect_ratio_allowed) >>>>>> + crtc->state->mode.picture_aspect_ratio = >>>>>> + HDMI_PICTURE_ASPECT_NONE; >>>>> These would still clobber the current crtc state. So definitely don't >>>>> want to do this. >>>> Alright, so alternative way of doing this will be: >>>> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel >>>> mode structure, >>>> but set the aspect ratio flags in the usermode to none, after calling the >>>> drm_mode_convert_to_umode(). >>>> >>>> Will take care of this in the next patchset. >> I gave a thought about it again. As you have mentioned earlier that >> get_crtc is one of the >> place where the user mode aspect ratio flags must be filtered out, is >> this what you have >> in mind. I hope I did not misunderstand. >> >> Changing the usermode aspect ratio flag bits, without change in the >> picture_aspect_ratio >> kernel mode will not result in both structures out of sync? > I don't think that should really matter. I suppose we might get one > extra modeset when the user feeds that mode back via setcrtc/atomic, but > meh. And actually the aspect ratio should only affect the infoframe, so > we should be able to eventually eliminate that extra modeset and just > update the infoframe instead. > >>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); >>>>>> crtc_resp->mode_valid = 1; >>>>>> >>>>>> @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device *dev, >>>>>> crtc_resp->x = crtc->x; >>>>>> crtc_resp->y = crtc->y; >>>>>> if (crtc->enabled) { >>>>>> + /* >>>>>> + * If the aspect-ratio is not required by the, >>>>>> + * userspace, do not set the aspect-ratio flags. >>>>>> + */ >>>>>> + if (!file_priv->aspect_ratio_allowed) >>>>>> + crtc->mode.picture_aspect_ratio = >>>>>> + HDMI_PICTURE_ASPECT_NONE; >>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode); >>>>>> crtc_resp->mode_valid = 1; >>>>>> >>>>>> @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, >>>>>> goto out; >>>>>> } >>>>>> >>>>>> + /* If the user does not ask for aspect ratio, reset the aspect >>>>>> + * ratio bits in the usermode. >>>>>> + */ >>>>>> + if (!file_priv->aspect_ratio_allowed) >>>>>> + crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); >>>>>> ret = drm_mode_convert_umode(mode, &crtc_req->mode); >>>>>> if (ret) { >>>>>> DRM_DEBUG_KMS("Invalid mode\n"); >>>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h >>>>>> index 1c27526..130dad9 100644 >>>>>> --- a/include/drm/drm_atomic.h >>>>>> +++ b/include/drm/drm_atomic.h >>>>>> @@ -237,6 +237,7 @@ struct __drm_private_objs_state { >>>>>> * @ref: count of all references to this state (will not be freed until zero) >>>>>> * @dev: parent DRM device >>>>>> * @allow_modeset: allow full modeset >>>>>> + * @allow_aspect_ratio: allow the aspect ratio info to be passes to user >>>>>> * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics >>>>>> * @async_update: hint for asynchronous plane update >>>>>> * @planes: pointer to array of structures with per-plane data >>>>>> @@ -256,6 +257,7 @@ struct drm_atomic_state { >>>>>> >>>>>> struct drm_device *dev; >>>>>> bool allow_modeset : 1; >>>>>> + bool allow_aspect_ratio : 1; >>>>>> bool legacy_cursor_update : 1; >>>>>> bool async_update : 1; >>>>>> struct __drm_planes_state *planes; >>>>>> -- >>>>>> 2.7.4 >>>>>> >>>>>> _______________________________________________ >>>>>> dri-devel mailing list >>>>>> dri-devel@lists.freedesktop.org >>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> Regards, >> Ankit _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths 2018-02-08 4:29 ` Nautiyal, Ankit K @ 2018-02-13 4:51 ` Nautiyal, Ankit K 2018-02-13 13:18 ` Ville Syrjälä 0 siblings, 1 reply; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-02-13 4:51 UTC (permalink / raw) To: Ville Syrjälä; +Cc: dri-devel [-- Attachment #1.1: Type: text/plain, Size: 17606 bytes --] Hi Ville, As per our last discussion, following points were discussed: 1. To suppress the aspect-ratio info from getblob ioctl to a user that does not support it: i. A new flag must be added to drm_blob_property to mark if the blob has mode data. ii. This flag must be set when the user tries do an atomic modeset. iii. In the getblob ioctl, based on the above flag, it can be determined that if the blob has mode data, and the aspect ratio info can be suppressed in getblob ioctl, if user does not support it. 2. Instead of adding aspect ratio capabilities in drm_atomic_state, pass on the file_priv which already has the required information to the function drm_mode_convert_umode. It will require addition of an new argument file_priv to several functions, but that is right thing to do, as file_priv is the correct structure for the capability information. 3. Changing the usermode aspect ratio flag bits, without change in the picture_aspect_ratio would not matter, and does not need to be handled in this patch. I just have one query here. We have agreed to modify drm_mode_convert_umode, to filter out the aspect-ratio info if user has not set aspect-ratio capability, but do we need a similar change the drm_mode_convert_to_umode? If we do, I am not sure if it would be possible to have the file_priv to /drm_atomic_set_mode_for_crtc/, which calls drm_mode_convert_to_umode. The function /drm_atomic_set_mode_for_crtc/ is used to set mode, originating by kernel, and make a blob from the kernel mode, which it saves in crtc_state. This function /: drm_atomic_set_mode_for_crtc, /is called by several helper functions and driver functions, and passing file_priv from all these functions does not seem to be plausible. Any suggestions on how to handle this situation? Regards, Ankit On 2/8/2018 9:59 AM, Nautiyal, Ankit K wrote: > Hi Ville, > > I still have some queries regarding the handling of aspect ratio flags > in getblob ioctl. > > Please find below my responses inline. > > > On 2/1/2018 6:24 PM, Ville Syrjälä wrote: >> On Thu, Feb 01, 2018 at 04:35:22PM +0530, Nautiyal, Ankit K wrote: >>> Hi Ville, >>> >>> Appreciate your time and the suggestions. >>> Please find my response inline: >>> >>> On 1/31/2018 6:39 PM, Ville Syrjälä wrote: >>>> On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote: >>>>> On 1/30/2018 12:23 AM, Ville Syrjälä wrote: >>>>>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote: >>>>>>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>>>>>> >>>>>>> If the user mode does not support aspect-ratio, and requests for >>>>>>> a modeset, then the flag bits representing aspect ratio in the >>>>>>> given user-mode must be rejected. >>>>>>> Similarly, while preparing a user-mode from kernel mode, the >>>>>>> aspect-ratio info must not be given, if aspect-ratio is not >>>>>>> supported by the user. >>>>>>> >>>>>>> This patch: >>>>>>> 1. adds a new bit field aspect_ratio_allowed in drm_atomic_state, >>>>>>> which is set only if the aspect-ratio is supported by the user. >>>>>>> 2. discards the aspect-ratio info from the user mode while >>>>>>> preparing kernel mode structure, during modeset, if the >>>>>>> user does not support aspect ratio. >>>>>>> 3. avoids setting the aspect-ratio info in user-mode, while >>>>>>> converting user-mode from the kernel-mode. >>>>>>> >>>>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>>>>>> >>>>>>> V3: Addressed review comments from Ville: >>>>>>> -Do not corrupt the current crtc state by updating aspect ratio >>>>>>> on the fly. >>>>>>> --- >>>>>>> drivers/gpu/drm/drm_atomic.c | 61 >>>>>>> +++++++++++++++++++++++++++++++++++++++++--- >>>>>>> drivers/gpu/drm/drm_crtc.c | 19 ++++++++++++++ >>>>>>> include/drm/drm_atomic.h | 2 ++ >>>>>>> 3 files changed, 79 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c >>>>>>> b/drivers/gpu/drm/drm_atomic.c >>>>>>> index 69ff763..39313e2 100644 >>>>>>> --- a/drivers/gpu/drm/drm_atomic.c >>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c >>>>>>> @@ -316,6 +316,35 @@ static s32 __user >>>>>>> *get_out_fence_for_crtc(struct drm_atomic_state *state, >>>>>>> return fence_ptr; >>>>>>> } >>>>>>> +/** >>>>>>> + * drm_atomic_allow_aspect_ratio_for_kmode >>>>>>> + * @state: the crtc state >>>>>>> + * @mode: kernel-internal mode, which is used to create a >>>>>>> duplicate mode, >>>>>>> + * with updated picture aspect ratio. >>>>>>> + * >>>>>>> + * Allow the aspect ratio info in the kernel mode only if >>>>>>> aspect ratio is >>>>>>> + * supported. >>>>>>> + * >>>>>>> + * RETURNS: >>>>>>> + * kernel-internal mode with updated picture aspect ratio value. >>>>>>> + */ >>>>>>> + >>>>>>> +struct drm_display_mode* >>>>>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state >>>>>>> *state, >>>>>>> + const struct drm_display_mode *mode) >>>>>>> +{ >>>>>>> + struct drm_atomic_state *atomic_state = state->state; >>>>>>> + struct drm_display_mode *ar_kmode; >>>>>>> + >>>>>>> + ar_kmode = drm_mode_duplicate(state->crtc->dev, mode); >>>>>>> + /* >>>>>>> + * If aspect ratio is not supported, set the picture aspect >>>>>>> ratio as >>>>>>> + * NONE. >>>>>>> + */ >>>>>>> + if (atomic_state && !atomic_state->allow_aspect_ratio) >>>>>>> + ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; >>>>>>> + return ar_kmode; >>>>>>> +} >>>>>>> /** >>>>>>> * drm_atomic_set_mode_for_crtc - set mode for CRTC >>>>>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct >>>>>>> drm_crtc_state *state, >>>>>>> state->mode_blob = NULL; >>>>>>> if (mode) { >>>>>>> - drm_mode_convert_to_umode(&umode, mode); >>>>>>> + struct drm_display_mode *ar_mode; >>>>>>> + >>>>>>> + ar_mode = >>>>>>> drm_atomic_allow_aspect_ratio_for_kmode(state, mode); >>>>>>> + drm_mode_convert_to_umode(&umode, ar_mode); >>>>>> This still looks sketchy. >>>>>> >>>>>> I believe there are just two places where we need to filter out the >>>>>> aspect ratio flags from the umode, namely the getblob and getcrtc >>>>>> ioctls. >>>>> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob, >>>>> etc. apart from the mode blob. >>>>> Is there any way to check from blob id (or by any other means), >>>>> if the data is for the mode, or edid, or gamma etc. >>>> I think we'd have to somehow mark the mode blobs as special. Until we >>>> have more need for cleaning up blobs before returning them to >>>> userspace >>>> I think a simple flag should do. If we come up with more uses like >>>> this >>>> then it might make sense to introduce some kind of optional filter >>>> function for blobs. >>> If my understanding is correct, >>> 1. To be able to do this, we need to change in uapi. >> We should be fine with an internal flag. Obviously it won't be set >> for blobs freshly created by userspace, but that's fine because we >> do no other error checking for them either. The error checking will >> happen when the user tries to actually use the blob. > > I am not sure why getblob ioctl should even come into picture. > (Perhaps I am missing something). > As per my understanding: > If a userspace needs to set a mode, it will just create a blob with > drm_mode_mode_info, > and get the blob-id. This blob-id would be supplied to > drm_mode_atomic_ioctl as a crtc > property mode_id. > At the kernel side, the crtc property mode_id is set by looking-up > for mode blob from blob id. > I am doing the change in the user mode aspect-ratio flags at this > junction. > > As far as getblob ioctl is concerned, if the user does call getblob > ioctl for getting mode blob from > the blob id, it will receive the same mode blob, with same aspect > ratio info which the user had > created using create blob ioctl. If it does want to use the blob, it > has to come through the > drm_mode_atomic_ioctl way, where aspect ratio flags are handled. > >>> 2. Whenever the userspace creates a blob for mode, the new flag/class >>> for blob type needs to be set. >>> >>> Do you think that it's worth the effort? >>>>> If we can check that, I can make sure that aspect-ratio flags are >>>>> taken >>>>> care of, if the blob is for mode, >>>>> and the aspect ratio is not supported. >>>>> >>>>>> And for checking the user input I think we should probably just >>>>>> stick that into drm_mode_convert_umode(). Looks like we never call >>>>>> that from anywhere but the atomic/setprop and setcrtc paths with >>>>>> a non-NULL argument. >>>>>> >>>>>> I *think* those three places should be sufficient to cover >>>>>> everything. >>>>> Agreed. Infact I tried to do that first, but the only problem we have >>>>> here, is that, the file_priv >>>>> which has the aspect ratio capability information is not supplied >>>>> to the >>>>> drm_mode_convert_umode() >>>>> or the functions calling, drm_mode_conver_umode(). At best we have >>>>> drm_crtc_state. >>>> Simply passing the file_priv down would seem like the most obvious >>>> solution. >>> This means we have to make change in various drivers and we have to >>> pull >>> the file_priv >>> parameter three levels down. Should we think of a better way for >>> doing this? >> Coccinelle is pretty good at modifying function parameters. > > Thanks for pointing out Coccinelle, will defintely try that out. > But my question here is whether we really want to drag the file_priv > from drm_mode_atomic_ioctl --> drm_mode_atomic_set_property --> > drm_atomic_crtc_set_property --> drm_mode_set_mode_prop_for_crtc--> > drm_mode_convert_umode. > > Instead, I tried to add a new field (allow_aspect_ratio) in > drm_crtc_state which is > already passed from the drm_mode_atomic_ioctl all the way to > drm_mode_convert_umode. > > > -Regards > Ankit >> >>>>> I have added an aspect ratio allowed flag in >>>>> drm_crtc_state->state so >>>>> that its available in the >>>>> functions calling drm_mode_convert_umode. >>>>> >>>>> I am open to suggestion to avoid adding a new flag in >>>>> drm_atomic_state, >>>>> if there is a better >>>>> way to let the drm_mode_convert_umode( ) know that user supports >>>>> aspect >>>>> ratio capability. >>>>>>> state->mode_blob = >>>>>>> drm_property_create_blob(state->crtc->dev, >>>>>>> sizeof(umode), >>>>>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct >>>>>>> drm_crtc_state *state, >>>>>>> if (IS_ERR(state->mode_blob)) >>>>>>> return PTR_ERR(state->mode_blob); >>>>>>> - drm_mode_copy(&state->mode, mode); >>>>>>> + drm_mode_copy(&state->mode, ar_mode); >>>>>>> state->enable = true; >>>>>>> DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n", >>>>>>> - mode->name, state); >>>>>>> + ar_mode->name, state); >>>>>>> + drm_mode_destroy(state->crtc->dev, ar_mode); >>>>>>> } else { >>>>>>> memset(&state->mode, 0, sizeof(state->mode)); >>>>>>> state->enable = false; >>>>>>> @@ -436,6 +469,25 @@ >>>>>>> drm_atomic_replace_property_blob_from_id(struct drm_device *dev, >>>>>>> } >>>>>>> /** >>>>>>> + * drm_atomic_allow_aspect_ratio_for_umode >>>>>>> + * @state: the crtc state >>>>>>> + * @umode: userspace mode, whose aspect ratio flag bits are to >>>>>>> be updated. >>>>>>> + * >>>>>>> + * Allow the aspect ratio info in the userspace mode only if >>>>>>> aspect ratio is >>>>>>> + * supported. >>>>>>> + */ >>>>>>> +void >>>>>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state >>>>>>> *state, >>>>>>> + struct drm_mode_modeinfo *umode) >>>>>>> +{ >>>>>>> + struct drm_atomic_state *atomic_state = state->state; >>>>>>> + >>>>>>> + /* Reset the aspect ratio flags if aspect ratio is not >>>>>>> supported */ >>>>>>> + if (atomic_state && !atomic_state->allow_aspect_ratio) >>>>>>> + umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); >>>>>>> +} >>>>>>> + >>>>>>> +/** >>>>>>> * drm_atomic_crtc_set_property - set property on CRTC >>>>>>> * @crtc: the drm CRTC to set a property on >>>>>>> * @state: the state object to update with the new property >>>>>>> value >>>>>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct >>>>>>> drm_crtc *crtc, >>>>>>> else if (property == config->prop_mode_id) { >>>>>>> struct drm_property_blob *mode = >>>>>>> drm_property_lookup_blob(dev, val); >>>>>>> + drm_atomic_allow_aspect_ratio_for_umode(state, >>>>>>> + (struct drm_mode_modeinfo *) >>>>>>> + mode->data); >>>>>>> ret = drm_atomic_set_mode_prop_for_crtc(state, mode); >>>>>>> drm_property_blob_put(mode); >>>>>>> return ret; >>>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c >>>>>>> b/drivers/gpu/drm/drm_crtc.c >>>>>>> index f0556e6..a2d34fa 100644 >>>>>>> --- a/drivers/gpu/drm/drm_crtc.c >>>>>>> +++ b/drivers/gpu/drm/drm_crtc.c >>>>>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev, >>>>>>> drm_modeset_lock(&crtc->mutex, NULL); >>>>>>> if (crtc->state) { >>>>>>> if (crtc->state->enable) { >>>>>>> + /* >>>>>>> + * If the aspect-ratio is not required by the, >>>>>>> + * userspace, do not set the aspect-ratio flags. >>>>>>> + */ >>>>>>> + if (!file_priv->aspect_ratio_allowed) >>>>>>> + crtc->state->mode.picture_aspect_ratio = >>>>>>> + HDMI_PICTURE_ASPECT_NONE; >>>>>> These would still clobber the current crtc state. So definitely >>>>>> don't >>>>>> want to do this. >>>>> Alright, so alternative way of doing this will be: >>>>> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel >>>>> mode structure, >>>>> but set the aspect ratio flags in the usermode to none, after >>>>> calling the >>>>> drm_mode_convert_to_umode(). >>>>> >>>>> Will take care of this in the next patchset. >>> I gave a thought about it again. As you have mentioned earlier that >>> get_crtc is one of the >>> place where the user mode aspect ratio flags must be filtered out, is >>> this what you have >>> in mind. I hope I did not misunderstand. >>> >>> Changing the usermode aspect ratio flag bits, without change in the >>> picture_aspect_ratio >>> kernel mode will not result in both structures out of sync? >> I don't think that should really matter. I suppose we might get one >> extra modeset when the user feeds that mode back via setcrtc/atomic, but >> meh. And actually the aspect ratio should only affect the infoframe, so >> we should be able to eventually eliminate that extra modeset and just >> update the infoframe instead. >> >>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); >>>>>>> crtc_resp->mode_valid = 1; >>>>>>> @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device >>>>>>> *dev, >>>>>>> crtc_resp->x = crtc->x; >>>>>>> crtc_resp->y = crtc->y; >>>>>>> if (crtc->enabled) { >>>>>>> + /* >>>>>>> + * If the aspect-ratio is not required by the, >>>>>>> + * userspace, do not set the aspect-ratio flags. >>>>>>> + */ >>>>>>> + if (!file_priv->aspect_ratio_allowed) >>>>>>> + crtc->mode.picture_aspect_ratio = >>>>>>> + HDMI_PICTURE_ASPECT_NONE; >>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode); >>>>>>> crtc_resp->mode_valid = 1; >>>>>>> @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device >>>>>>> *dev, void *data, >>>>>>> goto out; >>>>>>> } >>>>>>> + /* If the user does not ask for aspect ratio, reset >>>>>>> the aspect >>>>>>> + * ratio bits in the usermode. >>>>>>> + */ >>>>>>> + if (!file_priv->aspect_ratio_allowed) >>>>>>> + crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); >>>>>>> ret = drm_mode_convert_umode(mode, &crtc_req->mode); >>>>>>> if (ret) { >>>>>>> DRM_DEBUG_KMS("Invalid mode\n"); >>>>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h >>>>>>> index 1c27526..130dad9 100644 >>>>>>> --- a/include/drm/drm_atomic.h >>>>>>> +++ b/include/drm/drm_atomic.h >>>>>>> @@ -237,6 +237,7 @@ struct __drm_private_objs_state { >>>>>>> * @ref: count of all references to this state (will not be >>>>>>> freed until zero) >>>>>>> * @dev: parent DRM device >>>>>>> * @allow_modeset: allow full modeset >>>>>>> + * @allow_aspect_ratio: allow the aspect ratio info to be >>>>>>> passes to user >>>>>>> * @legacy_cursor_update: hint to enforce legacy cursor >>>>>>> IOCTL semantics >>>>>>> * @async_update: hint for asynchronous plane update >>>>>>> * @planes: pointer to array of structures with per-plane data >>>>>>> @@ -256,6 +257,7 @@ struct drm_atomic_state { >>>>>>> struct drm_device *dev; >>>>>>> bool allow_modeset : 1; >>>>>>> + bool allow_aspect_ratio : 1; >>>>>>> bool legacy_cursor_update : 1; >>>>>>> bool async_update : 1; >>>>>>> struct __drm_planes_state *planes; >>>>>>> -- >>>>>>> 2.7.4 >>>>>>> >>>>>>> _______________________________________________ >>>>>>> dri-devel mailing list >>>>>>> dri-devel@lists.freedesktop.org >>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>> Regards, >>> Ankit > [-- Attachment #1.2: Type: text/html, Size: 27103 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths 2018-02-13 4:51 ` Nautiyal, Ankit K @ 2018-02-13 13:18 ` Ville Syrjälä 2018-02-13 16:23 ` Nautiyal, Ankit K 0 siblings, 1 reply; 35+ messages in thread From: Ville Syrjälä @ 2018-02-13 13:18 UTC (permalink / raw) To: Nautiyal, Ankit K; +Cc: dri-devel On Tue, Feb 13, 2018 at 10:21:15AM +0530, Nautiyal, Ankit K wrote: > Hi Ville, > > As per our last discussion, following points were discussed: > > 1. To suppress the aspect-ratio info from getblob ioctl to a user that > does not support it: > > i. A new flag must be added to drm_blob_property to mark if the > blob has mode data. > > ii. This flag must be set when the user tries do an atomic modeset. > > iii. In the getblob ioctl, based on the above flag, it can be > determined that if the blob > > has mode data, and the aspect ratio info can be suppressed in > getblob ioctl, if user does not support it. > > 2. Instead of adding aspect ratio capabilities in drm_atomic_state, pass > on the file_priv which already has > > the required information to the function drm_mode_convert_umode. > > It will require addition of an new argument file_priv to several > functions, but that is right thing to do, > > as file_priv is the correct structure for the capability information. > > 3. Changing the usermode aspect ratio flag bits, without change in the > picture_aspect_ratio would not matter, and does not need to be handled > in this patch. > > > I just have one query here. We have agreed to modify > drm_mode_convert_umode, to filter out the aspect-ratio > > info if user has not set aspect-ratio capability, but do we need a > similar change the drm_mode_convert_to_umode? I think you maybe had those backwards. drm_mode_convert_umode(), or more likely its relevant callers setcrtc and setproperty, need to reject the mode if the client cap is not set. drm_mode_convert_to_umode(), or rather its callers getblob and getcrtc, need to filter out the flags. > > If we do, I am not sure if it would be possible to have the file_priv to > /drm_atomic_set_mode_for_crtc/, which calls > > drm_mode_convert_to_umode. The function /drm_atomic_set_mode_for_crtc/ > is used to set mode, originating by kernel, > > and make a blob from the kernel mode, which it saves in crtc_state. > > This function /: drm_atomic_set_mode_for_crtc, /is called by several > helper functions and driver functions, and passing > > file_priv from all these functions does not seem to be plausible. I don't think we need to plumb it quite that deep. Doing the check in drm_atomic_crtc_set_property() should be sufficient. > > Any suggestions on how to handle this situation? > > > Regards, > > Ankit > > > On 2/8/2018 9:59 AM, Nautiyal, Ankit K wrote: > > Hi Ville, > > > > I still have some queries regarding the handling of aspect ratio flags > > in getblob ioctl. > > > > Please find below my responses inline. > > > > > > On 2/1/2018 6:24 PM, Ville Syrjälä wrote: > >> On Thu, Feb 01, 2018 at 04:35:22PM +0530, Nautiyal, Ankit K wrote: > >>> Hi Ville, > >>> > >>> Appreciate your time and the suggestions. > >>> Please find my response inline: > >>> > >>> On 1/31/2018 6:39 PM, Ville Syrjälä wrote: > >>>> On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote: > >>>>> On 1/30/2018 12:23 AM, Ville Syrjälä wrote: > >>>>>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote: > >>>>>>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > >>>>>>> > >>>>>>> If the user mode does not support aspect-ratio, and requests for > >>>>>>> a modeset, then the flag bits representing aspect ratio in the > >>>>>>> given user-mode must be rejected. > >>>>>>> Similarly, while preparing a user-mode from kernel mode, the > >>>>>>> aspect-ratio info must not be given, if aspect-ratio is not > >>>>>>> supported by the user. > >>>>>>> > >>>>>>> This patch: > >>>>>>> 1. adds a new bit field aspect_ratio_allowed in drm_atomic_state, > >>>>>>> which is set only if the aspect-ratio is supported by the user. > >>>>>>> 2. discards the aspect-ratio info from the user mode while > >>>>>>> preparing kernel mode structure, during modeset, if the > >>>>>>> user does not support aspect ratio. > >>>>>>> 3. avoids setting the aspect-ratio info in user-mode, while > >>>>>>> converting user-mode from the kernel-mode. > >>>>>>> > >>>>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > >>>>>>> > >>>>>>> V3: Addressed review comments from Ville: > >>>>>>> -Do not corrupt the current crtc state by updating aspect ratio > >>>>>>> on the fly. > >>>>>>> --- > >>>>>>> drivers/gpu/drm/drm_atomic.c | 61 > >>>>>>> +++++++++++++++++++++++++++++++++++++++++--- > >>>>>>> drivers/gpu/drm/drm_crtc.c | 19 ++++++++++++++ > >>>>>>> include/drm/drm_atomic.h | 2 ++ > >>>>>>> 3 files changed, 79 insertions(+), 3 deletions(-) > >>>>>>> > >>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c > >>>>>>> b/drivers/gpu/drm/drm_atomic.c > >>>>>>> index 69ff763..39313e2 100644 > >>>>>>> --- a/drivers/gpu/drm/drm_atomic.c > >>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c > >>>>>>> @@ -316,6 +316,35 @@ static s32 __user > >>>>>>> *get_out_fence_for_crtc(struct drm_atomic_state *state, > >>>>>>> return fence_ptr; > >>>>>>> } > >>>>>>> +/** > >>>>>>> + * drm_atomic_allow_aspect_ratio_for_kmode > >>>>>>> + * @state: the crtc state > >>>>>>> + * @mode: kernel-internal mode, which is used to create a > >>>>>>> duplicate mode, > >>>>>>> + * with updated picture aspect ratio. > >>>>>>> + * > >>>>>>> + * Allow the aspect ratio info in the kernel mode only if > >>>>>>> aspect ratio is > >>>>>>> + * supported. > >>>>>>> + * > >>>>>>> + * RETURNS: > >>>>>>> + * kernel-internal mode with updated picture aspect ratio value. > >>>>>>> + */ > >>>>>>> + > >>>>>>> +struct drm_display_mode* > >>>>>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state > >>>>>>> *state, > >>>>>>> + const struct drm_display_mode *mode) > >>>>>>> +{ > >>>>>>> + struct drm_atomic_state *atomic_state = state->state; > >>>>>>> + struct drm_display_mode *ar_kmode; > >>>>>>> + > >>>>>>> + ar_kmode = drm_mode_duplicate(state->crtc->dev, mode); > >>>>>>> + /* > >>>>>>> + * If aspect ratio is not supported, set the picture aspect > >>>>>>> ratio as > >>>>>>> + * NONE. > >>>>>>> + */ > >>>>>>> + if (atomic_state && !atomic_state->allow_aspect_ratio) > >>>>>>> + ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; > >>>>>>> + return ar_kmode; > >>>>>>> +} > >>>>>>> /** > >>>>>>> * drm_atomic_set_mode_for_crtc - set mode for CRTC > >>>>>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct > >>>>>>> drm_crtc_state *state, > >>>>>>> state->mode_blob = NULL; > >>>>>>> if (mode) { > >>>>>>> - drm_mode_convert_to_umode(&umode, mode); > >>>>>>> + struct drm_display_mode *ar_mode; > >>>>>>> + > >>>>>>> + ar_mode = > >>>>>>> drm_atomic_allow_aspect_ratio_for_kmode(state, mode); > >>>>>>> + drm_mode_convert_to_umode(&umode, ar_mode); > >>>>>> This still looks sketchy. > >>>>>> > >>>>>> I believe there are just two places where we need to filter out the > >>>>>> aspect ratio flags from the umode, namely the getblob and getcrtc > >>>>>> ioctls. > >>>>> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob, > >>>>> etc. apart from the mode blob. > >>>>> Is there any way to check from blob id (or by any other means), > >>>>> if the data is for the mode, or edid, or gamma etc. > >>>> I think we'd have to somehow mark the mode blobs as special. Until we > >>>> have more need for cleaning up blobs before returning them to > >>>> userspace > >>>> I think a simple flag should do. If we come up with more uses like > >>>> this > >>>> then it might make sense to introduce some kind of optional filter > >>>> function for blobs. > >>> If my understanding is correct, > >>> 1. To be able to do this, we need to change in uapi. > >> We should be fine with an internal flag. Obviously it won't be set > >> for blobs freshly created by userspace, but that's fine because we > >> do no other error checking for them either. The error checking will > >> happen when the user tries to actually use the blob. > > > > I am not sure why getblob ioctl should even come into picture. > > (Perhaps I am missing something). > > As per my understanding: > > If a userspace needs to set a mode, it will just create a blob with > > drm_mode_mode_info, > > and get the blob-id. This blob-id would be supplied to > > drm_mode_atomic_ioctl as a crtc > > property mode_id. > > At the kernel side, the crtc property mode_id is set by looking-up > > for mode blob from blob id. > > I am doing the change in the user mode aspect-ratio flags at this > > junction. > > > > As far as getblob ioctl is concerned, if the user does call getblob > > ioctl for getting mode blob from > > the blob id, it will receive the same mode blob, with same aspect > > ratio info which the user had > > created using create blob ioctl. If it does want to use the blob, it > > has to come through the > > drm_mode_atomic_ioctl way, where aspect ratio flags are handled. > > > >>> 2. Whenever the userspace creates a blob for mode, the new flag/class > >>> for blob type needs to be set. > >>> > >>> Do you think that it's worth the effort? > >>>>> If we can check that, I can make sure that aspect-ratio flags are > >>>>> taken > >>>>> care of, if the blob is for mode, > >>>>> and the aspect ratio is not supported. > >>>>> > >>>>>> And for checking the user input I think we should probably just > >>>>>> stick that into drm_mode_convert_umode(). Looks like we never call > >>>>>> that from anywhere but the atomic/setprop and setcrtc paths with > >>>>>> a non-NULL argument. > >>>>>> > >>>>>> I *think* those three places should be sufficient to cover > >>>>>> everything. > >>>>> Agreed. Infact I tried to do that first, but the only problem we have > >>>>> here, is that, the file_priv > >>>>> which has the aspect ratio capability information is not supplied > >>>>> to the > >>>>> drm_mode_convert_umode() > >>>>> or the functions calling, drm_mode_conver_umode(). At best we have > >>>>> drm_crtc_state. > >>>> Simply passing the file_priv down would seem like the most obvious > >>>> solution. > >>> This means we have to make change in various drivers and we have to > >>> pull > >>> the file_priv > >>> parameter three levels down. Should we think of a better way for > >>> doing this? > >> Coccinelle is pretty good at modifying function parameters. > > > > Thanks for pointing out Coccinelle, will defintely try that out. > > But my question here is whether we really want to drag the file_priv > > from drm_mode_atomic_ioctl --> drm_mode_atomic_set_property --> > > drm_atomic_crtc_set_property --> drm_mode_set_mode_prop_for_crtc--> > > drm_mode_convert_umode. > > > > Instead, I tried to add a new field (allow_aspect_ratio) in > > drm_crtc_state which is > > already passed from the drm_mode_atomic_ioctl all the way to > > drm_mode_convert_umode. > > > > > > -Regards > > Ankit > >> > >>>>> I have added an aspect ratio allowed flag in > >>>>> drm_crtc_state->state so > >>>>> that its available in the > >>>>> functions calling drm_mode_convert_umode. > >>>>> > >>>>> I am open to suggestion to avoid adding a new flag in > >>>>> drm_atomic_state, > >>>>> if there is a better > >>>>> way to let the drm_mode_convert_umode( ) know that user supports > >>>>> aspect > >>>>> ratio capability. > >>>>>>> state->mode_blob = > >>>>>>> drm_property_create_blob(state->crtc->dev, > >>>>>>> sizeof(umode), > >>>>>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct > >>>>>>> drm_crtc_state *state, > >>>>>>> if (IS_ERR(state->mode_blob)) > >>>>>>> return PTR_ERR(state->mode_blob); > >>>>>>> - drm_mode_copy(&state->mode, mode); > >>>>>>> + drm_mode_copy(&state->mode, ar_mode); > >>>>>>> state->enable = true; > >>>>>>> DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n", > >>>>>>> - mode->name, state); > >>>>>>> + ar_mode->name, state); > >>>>>>> + drm_mode_destroy(state->crtc->dev, ar_mode); > >>>>>>> } else { > >>>>>>> memset(&state->mode, 0, sizeof(state->mode)); > >>>>>>> state->enable = false; > >>>>>>> @@ -436,6 +469,25 @@ > >>>>>>> drm_atomic_replace_property_blob_from_id(struct drm_device *dev, > >>>>>>> } > >>>>>>> /** > >>>>>>> + * drm_atomic_allow_aspect_ratio_for_umode > >>>>>>> + * @state: the crtc state > >>>>>>> + * @umode: userspace mode, whose aspect ratio flag bits are to > >>>>>>> be updated. > >>>>>>> + * > >>>>>>> + * Allow the aspect ratio info in the userspace mode only if > >>>>>>> aspect ratio is > >>>>>>> + * supported. > >>>>>>> + */ > >>>>>>> +void > >>>>>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state > >>>>>>> *state, > >>>>>>> + struct drm_mode_modeinfo *umode) > >>>>>>> +{ > >>>>>>> + struct drm_atomic_state *atomic_state = state->state; > >>>>>>> + > >>>>>>> + /* Reset the aspect ratio flags if aspect ratio is not > >>>>>>> supported */ > >>>>>>> + if (atomic_state && !atomic_state->allow_aspect_ratio) > >>>>>>> + umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); > >>>>>>> +} > >>>>>>> + > >>>>>>> +/** > >>>>>>> * drm_atomic_crtc_set_property - set property on CRTC > >>>>>>> * @crtc: the drm CRTC to set a property on > >>>>>>> * @state: the state object to update with the new property > >>>>>>> value > >>>>>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct > >>>>>>> drm_crtc *crtc, > >>>>>>> else if (property == config->prop_mode_id) { > >>>>>>> struct drm_property_blob *mode = > >>>>>>> drm_property_lookup_blob(dev, val); > >>>>>>> + drm_atomic_allow_aspect_ratio_for_umode(state, > >>>>>>> + (struct drm_mode_modeinfo *) > >>>>>>> + mode->data); > >>>>>>> ret = drm_atomic_set_mode_prop_for_crtc(state, mode); > >>>>>>> drm_property_blob_put(mode); > >>>>>>> return ret; > >>>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c > >>>>>>> b/drivers/gpu/drm/drm_crtc.c > >>>>>>> index f0556e6..a2d34fa 100644 > >>>>>>> --- a/drivers/gpu/drm/drm_crtc.c > >>>>>>> +++ b/drivers/gpu/drm/drm_crtc.c > >>>>>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev, > >>>>>>> drm_modeset_lock(&crtc->mutex, NULL); > >>>>>>> if (crtc->state) { > >>>>>>> if (crtc->state->enable) { > >>>>>>> + /* > >>>>>>> + * If the aspect-ratio is not required by the, > >>>>>>> + * userspace, do not set the aspect-ratio flags. > >>>>>>> + */ > >>>>>>> + if (!file_priv->aspect_ratio_allowed) > >>>>>>> + crtc->state->mode.picture_aspect_ratio = > >>>>>>> + HDMI_PICTURE_ASPECT_NONE; > >>>>>> These would still clobber the current crtc state. So definitely > >>>>>> don't > >>>>>> want to do this. > >>>>> Alright, so alternative way of doing this will be: > >>>>> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel > >>>>> mode structure, > >>>>> but set the aspect ratio flags in the usermode to none, after > >>>>> calling the > >>>>> drm_mode_convert_to_umode(). > >>>>> > >>>>> Will take care of this in the next patchset. > >>> I gave a thought about it again. As you have mentioned earlier that > >>> get_crtc is one of the > >>> place where the user mode aspect ratio flags must be filtered out, is > >>> this what you have > >>> in mind. I hope I did not misunderstand. > >>> > >>> Changing the usermode aspect ratio flag bits, without change in the > >>> picture_aspect_ratio > >>> kernel mode will not result in both structures out of sync? > >> I don't think that should really matter. I suppose we might get one > >> extra modeset when the user feeds that mode back via setcrtc/atomic, but > >> meh. And actually the aspect ratio should only affect the infoframe, so > >> we should be able to eventually eliminate that extra modeset and just > >> update the infoframe instead. > >> > >>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); > >>>>>>> crtc_resp->mode_valid = 1; > >>>>>>> @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device > >>>>>>> *dev, > >>>>>>> crtc_resp->x = crtc->x; > >>>>>>> crtc_resp->y = crtc->y; > >>>>>>> if (crtc->enabled) { > >>>>>>> + /* > >>>>>>> + * If the aspect-ratio is not required by the, > >>>>>>> + * userspace, do not set the aspect-ratio flags. > >>>>>>> + */ > >>>>>>> + if (!file_priv->aspect_ratio_allowed) > >>>>>>> + crtc->mode.picture_aspect_ratio = > >>>>>>> + HDMI_PICTURE_ASPECT_NONE; > >>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode); > >>>>>>> crtc_resp->mode_valid = 1; > >>>>>>> @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device > >>>>>>> *dev, void *data, > >>>>>>> goto out; > >>>>>>> } > >>>>>>> + /* If the user does not ask for aspect ratio, reset > >>>>>>> the aspect > >>>>>>> + * ratio bits in the usermode. > >>>>>>> + */ > >>>>>>> + if (!file_priv->aspect_ratio_allowed) > >>>>>>> + crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); > >>>>>>> ret = drm_mode_convert_umode(mode, &crtc_req->mode); > >>>>>>> if (ret) { > >>>>>>> DRM_DEBUG_KMS("Invalid mode\n"); > >>>>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > >>>>>>> index 1c27526..130dad9 100644 > >>>>>>> --- a/include/drm/drm_atomic.h > >>>>>>> +++ b/include/drm/drm_atomic.h > >>>>>>> @@ -237,6 +237,7 @@ struct __drm_private_objs_state { > >>>>>>> * @ref: count of all references to this state (will not be > >>>>>>> freed until zero) > >>>>>>> * @dev: parent DRM device > >>>>>>> * @allow_modeset: allow full modeset > >>>>>>> + * @allow_aspect_ratio: allow the aspect ratio info to be > >>>>>>> passes to user > >>>>>>> * @legacy_cursor_update: hint to enforce legacy cursor > >>>>>>> IOCTL semantics > >>>>>>> * @async_update: hint for asynchronous plane update > >>>>>>> * @planes: pointer to array of structures with per-plane data > >>>>>>> @@ -256,6 +257,7 @@ struct drm_atomic_state { > >>>>>>> struct drm_device *dev; > >>>>>>> bool allow_modeset : 1; > >>>>>>> + bool allow_aspect_ratio : 1; > >>>>>>> bool legacy_cursor_update : 1; > >>>>>>> bool async_update : 1; > >>>>>>> struct __drm_planes_state *planes; > >>>>>>> -- > >>>>>>> 2.7.4 > >>>>>>> > >>>>>>> _______________________________________________ > >>>>>>> dri-devel mailing list > >>>>>>> dri-devel@lists.freedesktop.org > >>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > >>> Regards, > >>> Ankit > > > -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths 2018-02-13 13:18 ` Ville Syrjälä @ 2018-02-13 16:23 ` Nautiyal, Ankit K 2018-02-13 16:45 ` Ville Syrjälä 0 siblings, 1 reply; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-02-13 16:23 UTC (permalink / raw) To: Ville Syrjälä; +Cc: dri-devel [-- Attachment #1.1: Type: text/plain, Size: 20043 bytes --] Hi Ville, Thanks yet again to look into this. I am still skeptical about rejecting the mode, if aspect ratio cap is not set. Perhaps I am not aware with the userspace expectations. Please find my response inline: On 2/13/2018 6:48 PM, Ville Syrjälä wrote: > On Tue, Feb 13, 2018 at 10:21:15AM +0530, Nautiyal, Ankit K wrote: >> Hi Ville, >> >> As per our last discussion, following points were discussed: >> >> 1. To suppress the aspect-ratio info from getblob ioctl to a user that >> does not support it: >> >> i. A new flag must be added to drm_blob_property to mark if the >> blob has mode data. >> >> ii. This flag must be set when the user tries do an atomic modeset. >> >> iii. In the getblob ioctl, based on the above flag, it can be >> determined that if the blob >> >> has mode data, and the aspect ratio info can be suppressed in >> getblob ioctl, if user does not support it. >> >> 2. Instead of adding aspect ratio capabilities in drm_atomic_state, pass >> on the file_priv which already has >> >> the required information to the function drm_mode_convert_umode. >> >> It will require addition of an new argument file_priv to several >> functions, but that is right thing to do, >> >> as file_priv is the correct structure for the capability information. >> >> 3. Changing the usermode aspect ratio flag bits, without change in the >> picture_aspect_ratio would not matter, and does not need to be handled >> in this patch. >> >> >> I just have one query here. We have agreed to modify >> drm_mode_convert_umode, to filter out the aspect-ratio >> >> info if user has not set aspect-ratio capability, but do we need a >> similar change the drm_mode_convert_to_umode? > I think you maybe had those backwards. > > drm_mode_convert_umode(), or more likely its relevant callers setcrtc > and setproperty, need to reject the mode if the client cap is not set. I guess, rejecting the mode, altogether can break the existing user-spaces. Current user-spaces, oblivious of the aspect-ratio capability in drm, will not set the aspect-ratio capability. Some of them, might have some garbage value in the aspect ratio bits of the user mode. If we reject such modes, the user-spaces that were earlier working will break. (But if we are sure, that the aspect ratio flag bits will all be reset, then it makes sense to reject the mode.) Instead of rejecting such modes, if we just only ignore the aspect ratio bits in such cases, and carry on with the modeset for the given user mode, the behaviour would be intact, just as prior to the aspect ratio changes. > drm_mode_convert_to_umode(), or rather its callers getblob and getcrtc, > need to filter out the flags. Yes right. I'll be filtering out the flags in the getblob and getcrtc functions. Thanks & Regards, Ankit >> If we do, I am not sure if it would be possible to have the file_priv to >> /drm_atomic_set_mode_for_crtc/, which calls >> >> drm_mode_convert_to_umode. The function /drm_atomic_set_mode_for_crtc/ >> is used to set mode, originating by kernel, >> >> and make a blob from the kernel mode, which it saves in crtc_state. >> >> This function /: drm_atomic_set_mode_for_crtc, /is called by several >> helper functions and driver functions, and passing >> >> file_priv from all these functions does not seem to be plausible. > I don't think we need to plumb it quite that deep. Doing the > check in drm_atomic_crtc_set_property() should be sufficient. > >> Any suggestions on how to handle this situation? >> >> >> Regards, >> >> Ankit >> >> >> On 2/8/2018 9:59 AM, Nautiyal, Ankit K wrote: >>> Hi Ville, >>> >>> I still have some queries regarding the handling of aspect ratio flags >>> in getblob ioctl. >>> >>> Please find below my responses inline. >>> >>> >>> On 2/1/2018 6:24 PM, Ville Syrjälä wrote: >>>> On Thu, Feb 01, 2018 at 04:35:22PM +0530, Nautiyal, Ankit K wrote: >>>>> Hi Ville, >>>>> >>>>> Appreciate your time and the suggestions. >>>>> Please find my response inline: >>>>> >>>>> On 1/31/2018 6:39 PM, Ville Syrjälä wrote: >>>>>> On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote: >>>>>>> On 1/30/2018 12:23 AM, Ville Syrjälä wrote: >>>>>>>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote: >>>>>>>>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>>>>>>>> >>>>>>>>> If the user mode does not support aspect-ratio, and requests for >>>>>>>>> a modeset, then the flag bits representing aspect ratio in the >>>>>>>>> given user-mode must be rejected. >>>>>>>>> Similarly, while preparing a user-mode from kernel mode, the >>>>>>>>> aspect-ratio info must not be given, if aspect-ratio is not >>>>>>>>> supported by the user. >>>>>>>>> >>>>>>>>> This patch: >>>>>>>>> 1. adds a new bit field aspect_ratio_allowed in drm_atomic_state, >>>>>>>>> which is set only if the aspect-ratio is supported by the user. >>>>>>>>> 2. discards the aspect-ratio info from the user mode while >>>>>>>>> preparing kernel mode structure, during modeset, if the >>>>>>>>> user does not support aspect ratio. >>>>>>>>> 3. avoids setting the aspect-ratio info in user-mode, while >>>>>>>>> converting user-mode from the kernel-mode. >>>>>>>>> >>>>>>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>>>>>>>> >>>>>>>>> V3: Addressed review comments from Ville: >>>>>>>>> -Do not corrupt the current crtc state by updating aspect ratio >>>>>>>>> on the fly. >>>>>>>>> --- >>>>>>>>> drivers/gpu/drm/drm_atomic.c | 61 >>>>>>>>> +++++++++++++++++++++++++++++++++++++++++--- >>>>>>>>> drivers/gpu/drm/drm_crtc.c | 19 ++++++++++++++ >>>>>>>>> include/drm/drm_atomic.h | 2 ++ >>>>>>>>> 3 files changed, 79 insertions(+), 3 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c >>>>>>>>> b/drivers/gpu/drm/drm_atomic.c >>>>>>>>> index 69ff763..39313e2 100644 >>>>>>>>> --- a/drivers/gpu/drm/drm_atomic.c >>>>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c >>>>>>>>> @@ -316,6 +316,35 @@ static s32 __user >>>>>>>>> *get_out_fence_for_crtc(struct drm_atomic_state *state, >>>>>>>>> return fence_ptr; >>>>>>>>> } >>>>>>>>> +/** >>>>>>>>> + * drm_atomic_allow_aspect_ratio_for_kmode >>>>>>>>> + * @state: the crtc state >>>>>>>>> + * @mode: kernel-internal mode, which is used to create a >>>>>>>>> duplicate mode, >>>>>>>>> + * with updated picture aspect ratio. >>>>>>>>> + * >>>>>>>>> + * Allow the aspect ratio info in the kernel mode only if >>>>>>>>> aspect ratio is >>>>>>>>> + * supported. >>>>>>>>> + * >>>>>>>>> + * RETURNS: >>>>>>>>> + * kernel-internal mode with updated picture aspect ratio value. >>>>>>>>> + */ >>>>>>>>> + >>>>>>>>> +struct drm_display_mode* >>>>>>>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state >>>>>>>>> *state, >>>>>>>>> + const struct drm_display_mode *mode) >>>>>>>>> +{ >>>>>>>>> + struct drm_atomic_state *atomic_state = state->state; >>>>>>>>> + struct drm_display_mode *ar_kmode; >>>>>>>>> + >>>>>>>>> + ar_kmode = drm_mode_duplicate(state->crtc->dev, mode); >>>>>>>>> + /* >>>>>>>>> + * If aspect ratio is not supported, set the picture aspect >>>>>>>>> ratio as >>>>>>>>> + * NONE. >>>>>>>>> + */ >>>>>>>>> + if (atomic_state && !atomic_state->allow_aspect_ratio) >>>>>>>>> + ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; >>>>>>>>> + return ar_kmode; >>>>>>>>> +} >>>>>>>>> /** >>>>>>>>> * drm_atomic_set_mode_for_crtc - set mode for CRTC >>>>>>>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct >>>>>>>>> drm_crtc_state *state, >>>>>>>>> state->mode_blob = NULL; >>>>>>>>> if (mode) { >>>>>>>>> - drm_mode_convert_to_umode(&umode, mode); >>>>>>>>> + struct drm_display_mode *ar_mode; >>>>>>>>> + >>>>>>>>> + ar_mode = >>>>>>>>> drm_atomic_allow_aspect_ratio_for_kmode(state, mode); >>>>>>>>> + drm_mode_convert_to_umode(&umode, ar_mode); >>>>>>>> This still looks sketchy. >>>>>>>> >>>>>>>> I believe there are just two places where we need to filter out the >>>>>>>> aspect ratio flags from the umode, namely the getblob and getcrtc >>>>>>>> ioctls. >>>>>>> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob, >>>>>>> etc. apart from the mode blob. >>>>>>> Is there any way to check from blob id (or by any other means), >>>>>>> if the data is for the mode, or edid, or gamma etc. >>>>>> I think we'd have to somehow mark the mode blobs as special. Until we >>>>>> have more need for cleaning up blobs before returning them to >>>>>> userspace >>>>>> I think a simple flag should do. If we come up with more uses like >>>>>> this >>>>>> then it might make sense to introduce some kind of optional filter >>>>>> function for blobs. >>>>> If my understanding is correct, >>>>> 1. To be able to do this, we need to change in uapi. >>>> We should be fine with an internal flag. Obviously it won't be set >>>> for blobs freshly created by userspace, but that's fine because we >>>> do no other error checking for them either. The error checking will >>>> happen when the user tries to actually use the blob. >>> I am not sure why getblob ioctl should even come into picture. >>> (Perhaps I am missing something). >>> As per my understanding: >>> If a userspace needs to set a mode, it will just create a blob with >>> drm_mode_mode_info, >>> and get the blob-id. This blob-id would be supplied to >>> drm_mode_atomic_ioctl as a crtc >>> property mode_id. >>> At the kernel side, the crtc property mode_id is set by looking-up >>> for mode blob from blob id. >>> I am doing the change in the user mode aspect-ratio flags at this >>> junction. >>> >>> As far as getblob ioctl is concerned, if the user does call getblob >>> ioctl for getting mode blob from >>> the blob id, it will receive the same mode blob, with same aspect >>> ratio info which the user had >>> created using create blob ioctl. If it does want to use the blob, it >>> has to come through the >>> drm_mode_atomic_ioctl way, where aspect ratio flags are handled. >>> >>>>> 2. Whenever the userspace creates a blob for mode, the new flag/class >>>>> for blob type needs to be set. >>>>> >>>>> Do you think that it's worth the effort? >>>>>>> If we can check that, I can make sure that aspect-ratio flags are >>>>>>> taken >>>>>>> care of, if the blob is for mode, >>>>>>> and the aspect ratio is not supported. >>>>>>> >>>>>>>> And for checking the user input I think we should probably just >>>>>>>> stick that into drm_mode_convert_umode(). Looks like we never call >>>>>>>> that from anywhere but the atomic/setprop and setcrtc paths with >>>>>>>> a non-NULL argument. >>>>>>>> >>>>>>>> I *think* those three places should be sufficient to cover >>>>>>>> everything. >>>>>>> Agreed. Infact I tried to do that first, but the only problem we have >>>>>>> here, is that, the file_priv >>>>>>> which has the aspect ratio capability information is not supplied >>>>>>> to the >>>>>>> drm_mode_convert_umode() >>>>>>> or the functions calling, drm_mode_conver_umode(). At best we have >>>>>>> drm_crtc_state. >>>>>> Simply passing the file_priv down would seem like the most obvious >>>>>> solution. >>>>> This means we have to make change in various drivers and we have to >>>>> pull >>>>> the file_priv >>>>> parameter three levels down. Should we think of a better way for >>>>> doing this? >>>> Coccinelle is pretty good at modifying function parameters. >>> Thanks for pointing out Coccinelle, will defintely try that out. >>> But my question here is whether we really want to drag the file_priv >>> from drm_mode_atomic_ioctl --> drm_mode_atomic_set_property --> >>> drm_atomic_crtc_set_property --> drm_mode_set_mode_prop_for_crtc--> >>> drm_mode_convert_umode. >>> >>> Instead, I tried to add a new field (allow_aspect_ratio) in >>> drm_crtc_state which is >>> already passed from the drm_mode_atomic_ioctl all the way to >>> drm_mode_convert_umode. >>> >>> >>> -Regards >>> Ankit >>>>>>> I have added an aspect ratio allowed flag in >>>>>>> drm_crtc_state->state so >>>>>>> that its available in the >>>>>>> functions calling drm_mode_convert_umode. >>>>>>> >>>>>>> I am open to suggestion to avoid adding a new flag in >>>>>>> drm_atomic_state, >>>>>>> if there is a better >>>>>>> way to let the drm_mode_convert_umode( ) know that user supports >>>>>>> aspect >>>>>>> ratio capability. >>>>>>>>> state->mode_blob = >>>>>>>>> drm_property_create_blob(state->crtc->dev, >>>>>>>>> sizeof(umode), >>>>>>>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct >>>>>>>>> drm_crtc_state *state, >>>>>>>>> if (IS_ERR(state->mode_blob)) >>>>>>>>> return PTR_ERR(state->mode_blob); >>>>>>>>> - drm_mode_copy(&state->mode, mode); >>>>>>>>> + drm_mode_copy(&state->mode, ar_mode); >>>>>>>>> state->enable = true; >>>>>>>>> DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n", >>>>>>>>> - mode->name, state); >>>>>>>>> + ar_mode->name, state); >>>>>>>>> + drm_mode_destroy(state->crtc->dev, ar_mode); >>>>>>>>> } else { >>>>>>>>> memset(&state->mode, 0, sizeof(state->mode)); >>>>>>>>> state->enable = false; >>>>>>>>> @@ -436,6 +469,25 @@ >>>>>>>>> drm_atomic_replace_property_blob_from_id(struct drm_device *dev, >>>>>>>>> } >>>>>>>>> /** >>>>>>>>> + * drm_atomic_allow_aspect_ratio_for_umode >>>>>>>>> + * @state: the crtc state >>>>>>>>> + * @umode: userspace mode, whose aspect ratio flag bits are to >>>>>>>>> be updated. >>>>>>>>> + * >>>>>>>>> + * Allow the aspect ratio info in the userspace mode only if >>>>>>>>> aspect ratio is >>>>>>>>> + * supported. >>>>>>>>> + */ >>>>>>>>> +void >>>>>>>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state >>>>>>>>> *state, >>>>>>>>> + struct drm_mode_modeinfo *umode) >>>>>>>>> +{ >>>>>>>>> + struct drm_atomic_state *atomic_state = state->state; >>>>>>>>> + >>>>>>>>> + /* Reset the aspect ratio flags if aspect ratio is not >>>>>>>>> supported */ >>>>>>>>> + if (atomic_state && !atomic_state->allow_aspect_ratio) >>>>>>>>> + umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +/** >>>>>>>>> * drm_atomic_crtc_set_property - set property on CRTC >>>>>>>>> * @crtc: the drm CRTC to set a property on >>>>>>>>> * @state: the state object to update with the new property >>>>>>>>> value >>>>>>>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct >>>>>>>>> drm_crtc *crtc, >>>>>>>>> else if (property == config->prop_mode_id) { >>>>>>>>> struct drm_property_blob *mode = >>>>>>>>> drm_property_lookup_blob(dev, val); >>>>>>>>> + drm_atomic_allow_aspect_ratio_for_umode(state, >>>>>>>>> + (struct drm_mode_modeinfo *) >>>>>>>>> + mode->data); >>>>>>>>> ret = drm_atomic_set_mode_prop_for_crtc(state, mode); >>>>>>>>> drm_property_blob_put(mode); >>>>>>>>> return ret; >>>>>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c >>>>>>>>> b/drivers/gpu/drm/drm_crtc.c >>>>>>>>> index f0556e6..a2d34fa 100644 >>>>>>>>> --- a/drivers/gpu/drm/drm_crtc.c >>>>>>>>> +++ b/drivers/gpu/drm/drm_crtc.c >>>>>>>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev, >>>>>>>>> drm_modeset_lock(&crtc->mutex, NULL); >>>>>>>>> if (crtc->state) { >>>>>>>>> if (crtc->state->enable) { >>>>>>>>> + /* >>>>>>>>> + * If the aspect-ratio is not required by the, >>>>>>>>> + * userspace, do not set the aspect-ratio flags. >>>>>>>>> + */ >>>>>>>>> + if (!file_priv->aspect_ratio_allowed) >>>>>>>>> + crtc->state->mode.picture_aspect_ratio = >>>>>>>>> + HDMI_PICTURE_ASPECT_NONE; >>>>>>>> These would still clobber the current crtc state. So definitely >>>>>>>> don't >>>>>>>> want to do this. >>>>>>> Alright, so alternative way of doing this will be: >>>>>>> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel >>>>>>> mode structure, >>>>>>> but set the aspect ratio flags in the usermode to none, after >>>>>>> calling the >>>>>>> drm_mode_convert_to_umode(). >>>>>>> >>>>>>> Will take care of this in the next patchset. >>>>> I gave a thought about it again. As you have mentioned earlier that >>>>> get_crtc is one of the >>>>> place where the user mode aspect ratio flags must be filtered out, is >>>>> this what you have >>>>> in mind. I hope I did not misunderstand. >>>>> >>>>> Changing the usermode aspect ratio flag bits, without change in the >>>>> picture_aspect_ratio >>>>> kernel mode will not result in both structures out of sync? >>>> I don't think that should really matter. I suppose we might get one >>>> extra modeset when the user feeds that mode back via setcrtc/atomic, but >>>> meh. And actually the aspect ratio should only affect the infoframe, so >>>> we should be able to eventually eliminate that extra modeset and just >>>> update the infoframe instead. >>>> >>>>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); >>>>>>>>> crtc_resp->mode_valid = 1; >>>>>>>>> @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device >>>>>>>>> *dev, >>>>>>>>> crtc_resp->x = crtc->x; >>>>>>>>> crtc_resp->y = crtc->y; >>>>>>>>> if (crtc->enabled) { >>>>>>>>> + /* >>>>>>>>> + * If the aspect-ratio is not required by the, >>>>>>>>> + * userspace, do not set the aspect-ratio flags. >>>>>>>>> + */ >>>>>>>>> + if (!file_priv->aspect_ratio_allowed) >>>>>>>>> + crtc->mode.picture_aspect_ratio = >>>>>>>>> + HDMI_PICTURE_ASPECT_NONE; >>>>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode); >>>>>>>>> crtc_resp->mode_valid = 1; >>>>>>>>> @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device >>>>>>>>> *dev, void *data, >>>>>>>>> goto out; >>>>>>>>> } >>>>>>>>> + /* If the user does not ask for aspect ratio, reset >>>>>>>>> the aspect >>>>>>>>> + * ratio bits in the usermode. >>>>>>>>> + */ >>>>>>>>> + if (!file_priv->aspect_ratio_allowed) >>>>>>>>> + crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); >>>>>>>>> ret = drm_mode_convert_umode(mode, &crtc_req->mode); >>>>>>>>> if (ret) { >>>>>>>>> DRM_DEBUG_KMS("Invalid mode\n"); >>>>>>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h >>>>>>>>> index 1c27526..130dad9 100644 >>>>>>>>> --- a/include/drm/drm_atomic.h >>>>>>>>> +++ b/include/drm/drm_atomic.h >>>>>>>>> @@ -237,6 +237,7 @@ struct __drm_private_objs_state { >>>>>>>>> * @ref: count of all references to this state (will not be >>>>>>>>> freed until zero) >>>>>>>>> * @dev: parent DRM device >>>>>>>>> * @allow_modeset: allow full modeset >>>>>>>>> + * @allow_aspect_ratio: allow the aspect ratio info to be >>>>>>>>> passes to user >>>>>>>>> * @legacy_cursor_update: hint to enforce legacy cursor >>>>>>>>> IOCTL semantics >>>>>>>>> * @async_update: hint for asynchronous plane update >>>>>>>>> * @planes: pointer to array of structures with per-plane data >>>>>>>>> @@ -256,6 +257,7 @@ struct drm_atomic_state { >>>>>>>>> struct drm_device *dev; >>>>>>>>> bool allow_modeset : 1; >>>>>>>>> + bool allow_aspect_ratio : 1; >>>>>>>>> bool legacy_cursor_update : 1; >>>>>>>>> bool async_update : 1; >>>>>>>>> struct __drm_planes_state *planes; >>>>>>>>> -- >>>>>>>>> 2.7.4 >>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> dri-devel mailing list >>>>>>>>> dri-devel@lists.freedesktop.org >>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>>>> Regards, >>>>> Ankit [-- Attachment #1.2: Type: text/html, Size: 21353 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths 2018-02-13 16:23 ` Nautiyal, Ankit K @ 2018-02-13 16:45 ` Ville Syrjälä 2018-02-15 11:57 ` Nautiyal, Ankit K 0 siblings, 1 reply; 35+ messages in thread From: Ville Syrjälä @ 2018-02-13 16:45 UTC (permalink / raw) To: Nautiyal, Ankit K; +Cc: dri-devel On Tue, Feb 13, 2018 at 09:53:53PM +0530, Nautiyal, Ankit K wrote: > Hi Ville, > > Thanks yet again to look into this. > > I am still skeptical about rejecting the mode, if aspect ratio cap is > not set. > Perhaps I am not aware with the userspace expectations. > > Please find my response inline: > > On 2/13/2018 6:48 PM, Ville Syrjälä wrote: > > On Tue, Feb 13, 2018 at 10:21:15AM +0530, Nautiyal, Ankit K wrote: > >> Hi Ville, > >> > >> As per our last discussion, following points were discussed: > >> > >> 1. To suppress the aspect-ratio info from getblob ioctl to a user that > >> does not support it: > >> > >> i. A new flag must be added to drm_blob_property to mark if the > >> blob has mode data. > >> > >> ii. This flag must be set when the user tries do an atomic modeset. > >> > >> iii. In the getblob ioctl, based on the above flag, it can be > >> determined that if the blob > >> > >> has mode data, and the aspect ratio info can be suppressed in > >> getblob ioctl, if user does not support it. > >> > >> 2. Instead of adding aspect ratio capabilities in drm_atomic_state, pass > >> on the file_priv which already has > >> > >> the required information to the function drm_mode_convert_umode. > >> > >> It will require addition of an new argument file_priv to several > >> functions, but that is right thing to do, > >> > >> as file_priv is the correct structure for the capability information. > >> > >> 3. Changing the usermode aspect ratio flag bits, without change in the > >> picture_aspect_ratio would not matter, and does not need to be handled > >> in this patch. > >> > >> > >> I just have one query here. We have agreed to modify > >> drm_mode_convert_umode, to filter out the aspect-ratio > >> > >> info if user has not set aspect-ratio capability, but do we need a > >> similar change the drm_mode_convert_to_umode? > > I think you maybe had those backwards. > > > > drm_mode_convert_umode(), or more likely its relevant callers setcrtc > > and setproperty, need to reject the mode if the client cap is not set. > I guess, rejecting the mode, altogether can break the existing user-spaces. > Current user-spaces, oblivious of the aspect-ratio capability in drm, > will not set the aspect-ratio capability. > Some of them, might have some garbage value in the aspect ratio bits of > the user mode. If we reject such > modes, the user-spaces that were earlier working will break. > (But if we are sure, that the aspect ratio flag bits will all be reset, > then it makes sense to reject the mode.) We already reject all unspecified flags. > > Instead of rejecting such modes, if we just only ignore the aspect ratio > bits in such cases, and carry on with the > modeset for the given user mode, the behaviour would be intact, just as > prior to the aspect ratio changes. > > > drm_mode_convert_to_umode(), or rather its callers getblob and getcrtc, > > need to filter out the flags. > > Yes right. I'll be filtering out the flags in the getblob and getcrtc > functions. > > Thanks & Regards, > Ankit > >> If we do, I am not sure if it would be possible to have the file_priv to > >> /drm_atomic_set_mode_for_crtc/, which calls > >> > >> drm_mode_convert_to_umode. The function /drm_atomic_set_mode_for_crtc/ > >> is used to set mode, originating by kernel, > >> > >> and make a blob from the kernel mode, which it saves in crtc_state. > >> > >> This function /: drm_atomic_set_mode_for_crtc, /is called by several > >> helper functions and driver functions, and passing > >> > >> file_priv from all these functions does not seem to be plausible. > > I don't think we need to plumb it quite that deep. Doing the > > check in drm_atomic_crtc_set_property() should be sufficient. > > > >> Any suggestions on how to handle this situation? > >> > >> > >> Regards, > >> > >> Ankit > >> > >> > >> On 2/8/2018 9:59 AM, Nautiyal, Ankit K wrote: > >>> Hi Ville, > >>> > >>> I still have some queries regarding the handling of aspect ratio flags > >>> in getblob ioctl. > >>> > >>> Please find below my responses inline. > >>> > >>> > >>> On 2/1/2018 6:24 PM, Ville Syrjälä wrote: > >>>> On Thu, Feb 01, 2018 at 04:35:22PM +0530, Nautiyal, Ankit K wrote: > >>>>> Hi Ville, > >>>>> > >>>>> Appreciate your time and the suggestions. > >>>>> Please find my response inline: > >>>>> > >>>>> On 1/31/2018 6:39 PM, Ville Syrjälä wrote: > >>>>>> On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote: > >>>>>>> On 1/30/2018 12:23 AM, Ville Syrjälä wrote: > >>>>>>>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote: > >>>>>>>>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > >>>>>>>>> > >>>>>>>>> If the user mode does not support aspect-ratio, and requests for > >>>>>>>>> a modeset, then the flag bits representing aspect ratio in the > >>>>>>>>> given user-mode must be rejected. > >>>>>>>>> Similarly, while preparing a user-mode from kernel mode, the > >>>>>>>>> aspect-ratio info must not be given, if aspect-ratio is not > >>>>>>>>> supported by the user. > >>>>>>>>> > >>>>>>>>> This patch: > >>>>>>>>> 1. adds a new bit field aspect_ratio_allowed in drm_atomic_state, > >>>>>>>>> which is set only if the aspect-ratio is supported by the user. > >>>>>>>>> 2. discards the aspect-ratio info from the user mode while > >>>>>>>>> preparing kernel mode structure, during modeset, if the > >>>>>>>>> user does not support aspect ratio. > >>>>>>>>> 3. avoids setting the aspect-ratio info in user-mode, while > >>>>>>>>> converting user-mode from the kernel-mode. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > >>>>>>>>> > >>>>>>>>> V3: Addressed review comments from Ville: > >>>>>>>>> -Do not corrupt the current crtc state by updating aspect ratio > >>>>>>>>> on the fly. > >>>>>>>>> --- > >>>>>>>>> drivers/gpu/drm/drm_atomic.c | 61 > >>>>>>>>> +++++++++++++++++++++++++++++++++++++++++--- > >>>>>>>>> drivers/gpu/drm/drm_crtc.c | 19 ++++++++++++++ > >>>>>>>>> include/drm/drm_atomic.h | 2 ++ > >>>>>>>>> 3 files changed, 79 insertions(+), 3 deletions(-) > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c > >>>>>>>>> b/drivers/gpu/drm/drm_atomic.c > >>>>>>>>> index 69ff763..39313e2 100644 > >>>>>>>>> --- a/drivers/gpu/drm/drm_atomic.c > >>>>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c > >>>>>>>>> @@ -316,6 +316,35 @@ static s32 __user > >>>>>>>>> *get_out_fence_for_crtc(struct drm_atomic_state *state, > >>>>>>>>> return fence_ptr; > >>>>>>>>> } > >>>>>>>>> +/** > >>>>>>>>> + * drm_atomic_allow_aspect_ratio_for_kmode > >>>>>>>>> + * @state: the crtc state > >>>>>>>>> + * @mode: kernel-internal mode, which is used to create a > >>>>>>>>> duplicate mode, > >>>>>>>>> + * with updated picture aspect ratio. > >>>>>>>>> + * > >>>>>>>>> + * Allow the aspect ratio info in the kernel mode only if > >>>>>>>>> aspect ratio is > >>>>>>>>> + * supported. > >>>>>>>>> + * > >>>>>>>>> + * RETURNS: > >>>>>>>>> + * kernel-internal mode with updated picture aspect ratio value. > >>>>>>>>> + */ > >>>>>>>>> + > >>>>>>>>> +struct drm_display_mode* > >>>>>>>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state > >>>>>>>>> *state, > >>>>>>>>> + const struct drm_display_mode *mode) > >>>>>>>>> +{ > >>>>>>>>> + struct drm_atomic_state *atomic_state = state->state; > >>>>>>>>> + struct drm_display_mode *ar_kmode; > >>>>>>>>> + > >>>>>>>>> + ar_kmode = drm_mode_duplicate(state->crtc->dev, mode); > >>>>>>>>> + /* > >>>>>>>>> + * If aspect ratio is not supported, set the picture aspect > >>>>>>>>> ratio as > >>>>>>>>> + * NONE. > >>>>>>>>> + */ > >>>>>>>>> + if (atomic_state && !atomic_state->allow_aspect_ratio) > >>>>>>>>> + ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; > >>>>>>>>> + return ar_kmode; > >>>>>>>>> +} > >>>>>>>>> /** > >>>>>>>>> * drm_atomic_set_mode_for_crtc - set mode for CRTC > >>>>>>>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct > >>>>>>>>> drm_crtc_state *state, > >>>>>>>>> state->mode_blob = NULL; > >>>>>>>>> if (mode) { > >>>>>>>>> - drm_mode_convert_to_umode(&umode, mode); > >>>>>>>>> + struct drm_display_mode *ar_mode; > >>>>>>>>> + > >>>>>>>>> + ar_mode = > >>>>>>>>> drm_atomic_allow_aspect_ratio_for_kmode(state, mode); > >>>>>>>>> + drm_mode_convert_to_umode(&umode, ar_mode); > >>>>>>>> This still looks sketchy. > >>>>>>>> > >>>>>>>> I believe there are just two places where we need to filter out the > >>>>>>>> aspect ratio flags from the umode, namely the getblob and getcrtc > >>>>>>>> ioctls. > >>>>>>> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob, > >>>>>>> etc. apart from the mode blob. > >>>>>>> Is there any way to check from blob id (or by any other means), > >>>>>>> if the data is for the mode, or edid, or gamma etc. > >>>>>> I think we'd have to somehow mark the mode blobs as special. Until we > >>>>>> have more need for cleaning up blobs before returning them to > >>>>>> userspace > >>>>>> I think a simple flag should do. If we come up with more uses like > >>>>>> this > >>>>>> then it might make sense to introduce some kind of optional filter > >>>>>> function for blobs. > >>>>> If my understanding is correct, > >>>>> 1. To be able to do this, we need to change in uapi. > >>>> We should be fine with an internal flag. Obviously it won't be set > >>>> for blobs freshly created by userspace, but that's fine because we > >>>> do no other error checking for them either. The error checking will > >>>> happen when the user tries to actually use the blob. > >>> I am not sure why getblob ioctl should even come into picture. > >>> (Perhaps I am missing something). > >>> As per my understanding: > >>> If a userspace needs to set a mode, it will just create a blob with > >>> drm_mode_mode_info, > >>> and get the blob-id. This blob-id would be supplied to > >>> drm_mode_atomic_ioctl as a crtc > >>> property mode_id. > >>> At the kernel side, the crtc property mode_id is set by looking-up > >>> for mode blob from blob id. > >>> I am doing the change in the user mode aspect-ratio flags at this > >>> junction. > >>> > >>> As far as getblob ioctl is concerned, if the user does call getblob > >>> ioctl for getting mode blob from > >>> the blob id, it will receive the same mode blob, with same aspect > >>> ratio info which the user had > >>> created using create blob ioctl. If it does want to use the blob, it > >>> has to come through the > >>> drm_mode_atomic_ioctl way, where aspect ratio flags are handled. > >>> > >>>>> 2. Whenever the userspace creates a blob for mode, the new flag/class > >>>>> for blob type needs to be set. > >>>>> > >>>>> Do you think that it's worth the effort? > >>>>>>> If we can check that, I can make sure that aspect-ratio flags are > >>>>>>> taken > >>>>>>> care of, if the blob is for mode, > >>>>>>> and the aspect ratio is not supported. > >>>>>>> > >>>>>>>> And for checking the user input I think we should probably just > >>>>>>>> stick that into drm_mode_convert_umode(). Looks like we never call > >>>>>>>> that from anywhere but the atomic/setprop and setcrtc paths with > >>>>>>>> a non-NULL argument. > >>>>>>>> > >>>>>>>> I *think* those three places should be sufficient to cover > >>>>>>>> everything. > >>>>>>> Agreed. Infact I tried to do that first, but the only problem we have > >>>>>>> here, is that, the file_priv > >>>>>>> which has the aspect ratio capability information is not supplied > >>>>>>> to the > >>>>>>> drm_mode_convert_umode() > >>>>>>> or the functions calling, drm_mode_conver_umode(). At best we have > >>>>>>> drm_crtc_state. > >>>>>> Simply passing the file_priv down would seem like the most obvious > >>>>>> solution. > >>>>> This means we have to make change in various drivers and we have to > >>>>> pull > >>>>> the file_priv > >>>>> parameter three levels down. Should we think of a better way for > >>>>> doing this? > >>>> Coccinelle is pretty good at modifying function parameters. > >>> Thanks for pointing out Coccinelle, will defintely try that out. > >>> But my question here is whether we really want to drag the file_priv > >>> from drm_mode_atomic_ioctl --> drm_mode_atomic_set_property --> > >>> drm_atomic_crtc_set_property --> drm_mode_set_mode_prop_for_crtc--> > >>> drm_mode_convert_umode. > >>> > >>> Instead, I tried to add a new field (allow_aspect_ratio) in > >>> drm_crtc_state which is > >>> already passed from the drm_mode_atomic_ioctl all the way to > >>> drm_mode_convert_umode. > >>> > >>> > >>> -Regards > >>> Ankit > >>>>>>> I have added an aspect ratio allowed flag in > >>>>>>> drm_crtc_state->state so > >>>>>>> that its available in the > >>>>>>> functions calling drm_mode_convert_umode. > >>>>>>> > >>>>>>> I am open to suggestion to avoid adding a new flag in > >>>>>>> drm_atomic_state, > >>>>>>> if there is a better > >>>>>>> way to let the drm_mode_convert_umode( ) know that user supports > >>>>>>> aspect > >>>>>>> ratio capability. > >>>>>>>>> state->mode_blob = > >>>>>>>>> drm_property_create_blob(state->crtc->dev, > >>>>>>>>> sizeof(umode), > >>>>>>>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct > >>>>>>>>> drm_crtc_state *state, > >>>>>>>>> if (IS_ERR(state->mode_blob)) > >>>>>>>>> return PTR_ERR(state->mode_blob); > >>>>>>>>> - drm_mode_copy(&state->mode, mode); > >>>>>>>>> + drm_mode_copy(&state->mode, ar_mode); > >>>>>>>>> state->enable = true; > >>>>>>>>> DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n", > >>>>>>>>> - mode->name, state); > >>>>>>>>> + ar_mode->name, state); > >>>>>>>>> + drm_mode_destroy(state->crtc->dev, ar_mode); > >>>>>>>>> } else { > >>>>>>>>> memset(&state->mode, 0, sizeof(state->mode)); > >>>>>>>>> state->enable = false; > >>>>>>>>> @@ -436,6 +469,25 @@ > >>>>>>>>> drm_atomic_replace_property_blob_from_id(struct drm_device *dev, > >>>>>>>>> } > >>>>>>>>> /** > >>>>>>>>> + * drm_atomic_allow_aspect_ratio_for_umode > >>>>>>>>> + * @state: the crtc state > >>>>>>>>> + * @umode: userspace mode, whose aspect ratio flag bits are to > >>>>>>>>> be updated. > >>>>>>>>> + * > >>>>>>>>> + * Allow the aspect ratio info in the userspace mode only if > >>>>>>>>> aspect ratio is > >>>>>>>>> + * supported. > >>>>>>>>> + */ > >>>>>>>>> +void > >>>>>>>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state > >>>>>>>>> *state, > >>>>>>>>> + struct drm_mode_modeinfo *umode) > >>>>>>>>> +{ > >>>>>>>>> + struct drm_atomic_state *atomic_state = state->state; > >>>>>>>>> + > >>>>>>>>> + /* Reset the aspect ratio flags if aspect ratio is not > >>>>>>>>> supported */ > >>>>>>>>> + if (atomic_state && !atomic_state->allow_aspect_ratio) > >>>>>>>>> + umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); > >>>>>>>>> +} > >>>>>>>>> + > >>>>>>>>> +/** > >>>>>>>>> * drm_atomic_crtc_set_property - set property on CRTC > >>>>>>>>> * @crtc: the drm CRTC to set a property on > >>>>>>>>> * @state: the state object to update with the new property > >>>>>>>>> value > >>>>>>>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct > >>>>>>>>> drm_crtc *crtc, > >>>>>>>>> else if (property == config->prop_mode_id) { > >>>>>>>>> struct drm_property_blob *mode = > >>>>>>>>> drm_property_lookup_blob(dev, val); > >>>>>>>>> + drm_atomic_allow_aspect_ratio_for_umode(state, > >>>>>>>>> + (struct drm_mode_modeinfo *) > >>>>>>>>> + mode->data); > >>>>>>>>> ret = drm_atomic_set_mode_prop_for_crtc(state, mode); > >>>>>>>>> drm_property_blob_put(mode); > >>>>>>>>> return ret; > >>>>>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c > >>>>>>>>> b/drivers/gpu/drm/drm_crtc.c > >>>>>>>>> index f0556e6..a2d34fa 100644 > >>>>>>>>> --- a/drivers/gpu/drm/drm_crtc.c > >>>>>>>>> +++ b/drivers/gpu/drm/drm_crtc.c > >>>>>>>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev, > >>>>>>>>> drm_modeset_lock(&crtc->mutex, NULL); > >>>>>>>>> if (crtc->state) { > >>>>>>>>> if (crtc->state->enable) { > >>>>>>>>> + /* > >>>>>>>>> + * If the aspect-ratio is not required by the, > >>>>>>>>> + * userspace, do not set the aspect-ratio flags. > >>>>>>>>> + */ > >>>>>>>>> + if (!file_priv->aspect_ratio_allowed) > >>>>>>>>> + crtc->state->mode.picture_aspect_ratio = > >>>>>>>>> + HDMI_PICTURE_ASPECT_NONE; > >>>>>>>> These would still clobber the current crtc state. So definitely > >>>>>>>> don't > >>>>>>>> want to do this. > >>>>>>> Alright, so alternative way of doing this will be: > >>>>>>> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel > >>>>>>> mode structure, > >>>>>>> but set the aspect ratio flags in the usermode to none, after > >>>>>>> calling the > >>>>>>> drm_mode_convert_to_umode(). > >>>>>>> > >>>>>>> Will take care of this in the next patchset. > >>>>> I gave a thought about it again. As you have mentioned earlier that > >>>>> get_crtc is one of the > >>>>> place where the user mode aspect ratio flags must be filtered out, is > >>>>> this what you have > >>>>> in mind. I hope I did not misunderstand. > >>>>> > >>>>> Changing the usermode aspect ratio flag bits, without change in the > >>>>> picture_aspect_ratio > >>>>> kernel mode will not result in both structures out of sync? > >>>> I don't think that should really matter. I suppose we might get one > >>>> extra modeset when the user feeds that mode back via setcrtc/atomic, but > >>>> meh. And actually the aspect ratio should only affect the infoframe, so > >>>> we should be able to eventually eliminate that extra modeset and just > >>>> update the infoframe instead. > >>>> > >>>>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); > >>>>>>>>> crtc_resp->mode_valid = 1; > >>>>>>>>> @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device > >>>>>>>>> *dev, > >>>>>>>>> crtc_resp->x = crtc->x; > >>>>>>>>> crtc_resp->y = crtc->y; > >>>>>>>>> if (crtc->enabled) { > >>>>>>>>> + /* > >>>>>>>>> + * If the aspect-ratio is not required by the, > >>>>>>>>> + * userspace, do not set the aspect-ratio flags. > >>>>>>>>> + */ > >>>>>>>>> + if (!file_priv->aspect_ratio_allowed) > >>>>>>>>> + crtc->mode.picture_aspect_ratio = > >>>>>>>>> + HDMI_PICTURE_ASPECT_NONE; > >>>>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode); > >>>>>>>>> crtc_resp->mode_valid = 1; > >>>>>>>>> @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device > >>>>>>>>> *dev, void *data, > >>>>>>>>> goto out; > >>>>>>>>> } > >>>>>>>>> + /* If the user does not ask for aspect ratio, reset > >>>>>>>>> the aspect > >>>>>>>>> + * ratio bits in the usermode. > >>>>>>>>> + */ > >>>>>>>>> + if (!file_priv->aspect_ratio_allowed) > >>>>>>>>> + crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); > >>>>>>>>> ret = drm_mode_convert_umode(mode, &crtc_req->mode); > >>>>>>>>> if (ret) { > >>>>>>>>> DRM_DEBUG_KMS("Invalid mode\n"); > >>>>>>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > >>>>>>>>> index 1c27526..130dad9 100644 > >>>>>>>>> --- a/include/drm/drm_atomic.h > >>>>>>>>> +++ b/include/drm/drm_atomic.h > >>>>>>>>> @@ -237,6 +237,7 @@ struct __drm_private_objs_state { > >>>>>>>>> * @ref: count of all references to this state (will not be > >>>>>>>>> freed until zero) > >>>>>>>>> * @dev: parent DRM device > >>>>>>>>> * @allow_modeset: allow full modeset > >>>>>>>>> + * @allow_aspect_ratio: allow the aspect ratio info to be > >>>>>>>>> passes to user > >>>>>>>>> * @legacy_cursor_update: hint to enforce legacy cursor > >>>>>>>>> IOCTL semantics > >>>>>>>>> * @async_update: hint for asynchronous plane update > >>>>>>>>> * @planes: pointer to array of structures with per-plane data > >>>>>>>>> @@ -256,6 +257,7 @@ struct drm_atomic_state { > >>>>>>>>> struct drm_device *dev; > >>>>>>>>> bool allow_modeset : 1; > >>>>>>>>> + bool allow_aspect_ratio : 1; > >>>>>>>>> bool legacy_cursor_update : 1; > >>>>>>>>> bool async_update : 1; > >>>>>>>>> struct __drm_planes_state *planes; > >>>>>>>>> -- > >>>>>>>>> 2.7.4 > >>>>>>>>> > >>>>>>>>> _______________________________________________ > >>>>>>>>> dri-devel mailing list > >>>>>>>>> dri-devel@lists.freedesktop.org > >>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > >>>>> Regards, > >>>>> Ankit > -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths 2018-02-13 16:45 ` Ville Syrjälä @ 2018-02-15 11:57 ` Nautiyal, Ankit K 0 siblings, 0 replies; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-02-15 11:57 UTC (permalink / raw) To: Ville Syrjälä; +Cc: dri-devel [-- Attachment #1.1: Type: text/plain, Size: 21457 bytes --] Hi Ville, I have addressed your review comments (including the policy regarding, rejection of mode with aspect ratio, if no aspect ratio cap) and other suggestions in the next patch-set. I will be sending the next patch-set, shortly. Regards, Ankit On 2/13/2018 10:15 PM, Ville Syrjälä wrote: > On Tue, Feb 13, 2018 at 09:53:53PM +0530, Nautiyal, Ankit K wrote: >> Hi Ville, >> >> Thanks yet again to look into this. >> >> I am still skeptical about rejecting the mode, if aspect ratio cap is >> not set. >> Perhaps I am not aware with the userspace expectations. >> >> Please find my response inline: >> >> On 2/13/2018 6:48 PM, Ville Syrjälä wrote: >>> On Tue, Feb 13, 2018 at 10:21:15AM +0530, Nautiyal, Ankit K wrote: >>>> Hi Ville, >>>> >>>> As per our last discussion, following points were discussed: >>>> >>>> 1. To suppress the aspect-ratio info from getblob ioctl to a user that >>>> does not support it: >>>> >>>> i. A new flag must be added to drm_blob_property to mark if the >>>> blob has mode data. >>>> >>>> ii. This flag must be set when the user tries do an atomic modeset. >>>> >>>> iii. In the getblob ioctl, based on the above flag, it can be >>>> determined that if the blob >>>> >>>> has mode data, and the aspect ratio info can be suppressed in >>>> getblob ioctl, if user does not support it. >>>> >>>> 2. Instead of adding aspect ratio capabilities in drm_atomic_state, pass >>>> on the file_priv which already has >>>> >>>> the required information to the function drm_mode_convert_umode. >>>> >>>> It will require addition of an new argument file_priv to several >>>> functions, but that is right thing to do, >>>> >>>> as file_priv is the correct structure for the capability information. >>>> >>>> 3. Changing the usermode aspect ratio flag bits, without change in the >>>> picture_aspect_ratio would not matter, and does not need to be handled >>>> in this patch. >>>> >>>> >>>> I just have one query here. We have agreed to modify >>>> drm_mode_convert_umode, to filter out the aspect-ratio >>>> >>>> info if user has not set aspect-ratio capability, but do we need a >>>> similar change the drm_mode_convert_to_umode? >>> I think you maybe had those backwards. >>> >>> drm_mode_convert_umode(), or more likely its relevant callers setcrtc >>> and setproperty, need to reject the mode if the client cap is not set. >> I guess, rejecting the mode, altogether can break the existing user-spaces. >> Current user-spaces, oblivious of the aspect-ratio capability in drm, >> will not set the aspect-ratio capability. >> Some of them, might have some garbage value in the aspect ratio bits of >> the user mode. If we reject such >> modes, the user-spaces that were earlier working will break. >> (But if we are sure, that the aspect ratio flag bits will all be reset, >> then it makes sense to reject the mode.) > We already reject all unspecified flags. > >> Instead of rejecting such modes, if we just only ignore the aspect ratio >> bits in such cases, and carry on with the >> modeset for the given user mode, the behaviour would be intact, just as >> prior to the aspect ratio changes. >> >>> drm_mode_convert_to_umode(), or rather its callers getblob and getcrtc, >>> need to filter out the flags. >> Yes right. I'll be filtering out the flags in the getblob and getcrtc >> functions. >> >> Thanks & Regards, >> Ankit >>>> If we do, I am not sure if it would be possible to have the file_priv to >>>> /drm_atomic_set_mode_for_crtc/, which calls >>>> >>>> drm_mode_convert_to_umode. The function /drm_atomic_set_mode_for_crtc/ >>>> is used to set mode, originating by kernel, >>>> >>>> and make a blob from the kernel mode, which it saves in crtc_state. >>>> >>>> This function /: drm_atomic_set_mode_for_crtc, /is called by several >>>> helper functions and driver functions, and passing >>>> >>>> file_priv from all these functions does not seem to be plausible. >>> I don't think we need to plumb it quite that deep. Doing the >>> check in drm_atomic_crtc_set_property() should be sufficient. >>> >>>> Any suggestions on how to handle this situation? >>>> >>>> >>>> Regards, >>>> >>>> Ankit >>>> >>>> >>>> On 2/8/2018 9:59 AM, Nautiyal, Ankit K wrote: >>>>> Hi Ville, >>>>> >>>>> I still have some queries regarding the handling of aspect ratio flags >>>>> in getblob ioctl. >>>>> >>>>> Please find below my responses inline. >>>>> >>>>> >>>>> On 2/1/2018 6:24 PM, Ville Syrjälä wrote: >>>>>> On Thu, Feb 01, 2018 at 04:35:22PM +0530, Nautiyal, Ankit K wrote: >>>>>>> Hi Ville, >>>>>>> >>>>>>> Appreciate your time and the suggestions. >>>>>>> Please find my response inline: >>>>>>> >>>>>>> On 1/31/2018 6:39 PM, Ville Syrjälä wrote: >>>>>>>> On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote: >>>>>>>>> On 1/30/2018 12:23 AM, Ville Syrjälä wrote: >>>>>>>>>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote: >>>>>>>>>>> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>>>>>>>>>> >>>>>>>>>>> If the user mode does not support aspect-ratio, and requests for >>>>>>>>>>> a modeset, then the flag bits representing aspect ratio in the >>>>>>>>>>> given user-mode must be rejected. >>>>>>>>>>> Similarly, while preparing a user-mode from kernel mode, the >>>>>>>>>>> aspect-ratio info must not be given, if aspect-ratio is not >>>>>>>>>>> supported by the user. >>>>>>>>>>> >>>>>>>>>>> This patch: >>>>>>>>>>> 1. adds a new bit field aspect_ratio_allowed in drm_atomic_state, >>>>>>>>>>> which is set only if the aspect-ratio is supported by the user. >>>>>>>>>>> 2. discards the aspect-ratio info from the user mode while >>>>>>>>>>> preparing kernel mode structure, during modeset, if the >>>>>>>>>>> user does not support aspect ratio. >>>>>>>>>>> 3. avoids setting the aspect-ratio info in user-mode, while >>>>>>>>>>> converting user-mode from the kernel-mode. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >>>>>>>>>>> >>>>>>>>>>> V3: Addressed review comments from Ville: >>>>>>>>>>> -Do not corrupt the current crtc state by updating aspect ratio >>>>>>>>>>> on the fly. >>>>>>>>>>> --- >>>>>>>>>>> drivers/gpu/drm/drm_atomic.c | 61 >>>>>>>>>>> +++++++++++++++++++++++++++++++++++++++++--- >>>>>>>>>>> drivers/gpu/drm/drm_crtc.c | 19 ++++++++++++++ >>>>>>>>>>> include/drm/drm_atomic.h | 2 ++ >>>>>>>>>>> 3 files changed, 79 insertions(+), 3 deletions(-) >>>>>>>>>>> >>>>>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c >>>>>>>>>>> b/drivers/gpu/drm/drm_atomic.c >>>>>>>>>>> index 69ff763..39313e2 100644 >>>>>>>>>>> --- a/drivers/gpu/drm/drm_atomic.c >>>>>>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c >>>>>>>>>>> @@ -316,6 +316,35 @@ static s32 __user >>>>>>>>>>> *get_out_fence_for_crtc(struct drm_atomic_state *state, >>>>>>>>>>> return fence_ptr; >>>>>>>>>>> } >>>>>>>>>>> +/** >>>>>>>>>>> + * drm_atomic_allow_aspect_ratio_for_kmode >>>>>>>>>>> + * @state: the crtc state >>>>>>>>>>> + * @mode: kernel-internal mode, which is used to create a >>>>>>>>>>> duplicate mode, >>>>>>>>>>> + * with updated picture aspect ratio. >>>>>>>>>>> + * >>>>>>>>>>> + * Allow the aspect ratio info in the kernel mode only if >>>>>>>>>>> aspect ratio is >>>>>>>>>>> + * supported. >>>>>>>>>>> + * >>>>>>>>>>> + * RETURNS: >>>>>>>>>>> + * kernel-internal mode with updated picture aspect ratio value. >>>>>>>>>>> + */ >>>>>>>>>>> + >>>>>>>>>>> +struct drm_display_mode* >>>>>>>>>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state >>>>>>>>>>> *state, >>>>>>>>>>> + const struct drm_display_mode *mode) >>>>>>>>>>> +{ >>>>>>>>>>> + struct drm_atomic_state *atomic_state = state->state; >>>>>>>>>>> + struct drm_display_mode *ar_kmode; >>>>>>>>>>> + >>>>>>>>>>> + ar_kmode = drm_mode_duplicate(state->crtc->dev, mode); >>>>>>>>>>> + /* >>>>>>>>>>> + * If aspect ratio is not supported, set the picture aspect >>>>>>>>>>> ratio as >>>>>>>>>>> + * NONE. >>>>>>>>>>> + */ >>>>>>>>>>> + if (atomic_state && !atomic_state->allow_aspect_ratio) >>>>>>>>>>> + ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; >>>>>>>>>>> + return ar_kmode; >>>>>>>>>>> +} >>>>>>>>>>> /** >>>>>>>>>>> * drm_atomic_set_mode_for_crtc - set mode for CRTC >>>>>>>>>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct >>>>>>>>>>> drm_crtc_state *state, >>>>>>>>>>> state->mode_blob = NULL; >>>>>>>>>>> if (mode) { >>>>>>>>>>> - drm_mode_convert_to_umode(&umode, mode); >>>>>>>>>>> + struct drm_display_mode *ar_mode; >>>>>>>>>>> + >>>>>>>>>>> + ar_mode = >>>>>>>>>>> drm_atomic_allow_aspect_ratio_for_kmode(state, mode); >>>>>>>>>>> + drm_mode_convert_to_umode(&umode, ar_mode); >>>>>>>>>> This still looks sketchy. >>>>>>>>>> >>>>>>>>>> I believe there are just two places where we need to filter out the >>>>>>>>>> aspect ratio flags from the umode, namely the getblob and getcrtc >>>>>>>>>> ioctls. >>>>>>>>> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob, >>>>>>>>> etc. apart from the mode blob. >>>>>>>>> Is there any way to check from blob id (or by any other means), >>>>>>>>> if the data is for the mode, or edid, or gamma etc. >>>>>>>> I think we'd have to somehow mark the mode blobs as special. Until we >>>>>>>> have more need for cleaning up blobs before returning them to >>>>>>>> userspace >>>>>>>> I think a simple flag should do. If we come up with more uses like >>>>>>>> this >>>>>>>> then it might make sense to introduce some kind of optional filter >>>>>>>> function for blobs. >>>>>>> If my understanding is correct, >>>>>>> 1. To be able to do this, we need to change in uapi. >>>>>> We should be fine with an internal flag. Obviously it won't be set >>>>>> for blobs freshly created by userspace, but that's fine because we >>>>>> do no other error checking for them either. The error checking will >>>>>> happen when the user tries to actually use the blob. >>>>> I am not sure why getblob ioctl should even come into picture. >>>>> (Perhaps I am missing something). >>>>> As per my understanding: >>>>> If a userspace needs to set a mode, it will just create a blob with >>>>> drm_mode_mode_info, >>>>> and get the blob-id. This blob-id would be supplied to >>>>> drm_mode_atomic_ioctl as a crtc >>>>> property mode_id. >>>>> At the kernel side, the crtc property mode_id is set by looking-up >>>>> for mode blob from blob id. >>>>> I am doing the change in the user mode aspect-ratio flags at this >>>>> junction. >>>>> >>>>> As far as getblob ioctl is concerned, if the user does call getblob >>>>> ioctl for getting mode blob from >>>>> the blob id, it will receive the same mode blob, with same aspect >>>>> ratio info which the user had >>>>> created using create blob ioctl. If it does want to use the blob, it >>>>> has to come through the >>>>> drm_mode_atomic_ioctl way, where aspect ratio flags are handled. >>>>> >>>>>>> 2. Whenever the userspace creates a blob for mode, the new flag/class >>>>>>> for blob type needs to be set. >>>>>>> >>>>>>> Do you think that it's worth the effort? >>>>>>>>> If we can check that, I can make sure that aspect-ratio flags are >>>>>>>>> taken >>>>>>>>> care of, if the blob is for mode, >>>>>>>>> and the aspect ratio is not supported. >>>>>>>>> >>>>>>>>>> And for checking the user input I think we should probably just >>>>>>>>>> stick that into drm_mode_convert_umode(). Looks like we never call >>>>>>>>>> that from anywhere but the atomic/setprop and setcrtc paths with >>>>>>>>>> a non-NULL argument. >>>>>>>>>> >>>>>>>>>> I *think* those three places should be sufficient to cover >>>>>>>>>> everything. >>>>>>>>> Agreed. Infact I tried to do that first, but the only problem we have >>>>>>>>> here, is that, the file_priv >>>>>>>>> which has the aspect ratio capability information is not supplied >>>>>>>>> to the >>>>>>>>> drm_mode_convert_umode() >>>>>>>>> or the functions calling, drm_mode_conver_umode(). At best we have >>>>>>>>> drm_crtc_state. >>>>>>>> Simply passing the file_priv down would seem like the most obvious >>>>>>>> solution. >>>>>>> This means we have to make change in various drivers and we have to >>>>>>> pull >>>>>>> the file_priv >>>>>>> parameter three levels down. Should we think of a better way for >>>>>>> doing this? >>>>>> Coccinelle is pretty good at modifying function parameters. >>>>> Thanks for pointing out Coccinelle, will defintely try that out. >>>>> But my question here is whether we really want to drag the file_priv >>>>> from drm_mode_atomic_ioctl --> drm_mode_atomic_set_property --> >>>>> drm_atomic_crtc_set_property --> drm_mode_set_mode_prop_for_crtc--> >>>>> drm_mode_convert_umode. >>>>> >>>>> Instead, I tried to add a new field (allow_aspect_ratio) in >>>>> drm_crtc_state which is >>>>> already passed from the drm_mode_atomic_ioctl all the way to >>>>> drm_mode_convert_umode. >>>>> >>>>> >>>>> -Regards >>>>> Ankit >>>>>>>>> I have added an aspect ratio allowed flag in >>>>>>>>> drm_crtc_state->state so >>>>>>>>> that its available in the >>>>>>>>> functions calling drm_mode_convert_umode. >>>>>>>>> >>>>>>>>> I am open to suggestion to avoid adding a new flag in >>>>>>>>> drm_atomic_state, >>>>>>>>> if there is a better >>>>>>>>> way to let the drm_mode_convert_umode( ) know that user supports >>>>>>>>> aspect >>>>>>>>> ratio capability. >>>>>>>>>>> state->mode_blob = >>>>>>>>>>> drm_property_create_blob(state->crtc->dev, >>>>>>>>>>> sizeof(umode), >>>>>>>>>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct >>>>>>>>>>> drm_crtc_state *state, >>>>>>>>>>> if (IS_ERR(state->mode_blob)) >>>>>>>>>>> return PTR_ERR(state->mode_blob); >>>>>>>>>>> - drm_mode_copy(&state->mode, mode); >>>>>>>>>>> + drm_mode_copy(&state->mode, ar_mode); >>>>>>>>>>> state->enable = true; >>>>>>>>>>> DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n", >>>>>>>>>>> - mode->name, state); >>>>>>>>>>> + ar_mode->name, state); >>>>>>>>>>> + drm_mode_destroy(state->crtc->dev, ar_mode); >>>>>>>>>>> } else { >>>>>>>>>>> memset(&state->mode, 0, sizeof(state->mode)); >>>>>>>>>>> state->enable = false; >>>>>>>>>>> @@ -436,6 +469,25 @@ >>>>>>>>>>> drm_atomic_replace_property_blob_from_id(struct drm_device *dev, >>>>>>>>>>> } >>>>>>>>>>> /** >>>>>>>>>>> + * drm_atomic_allow_aspect_ratio_for_umode >>>>>>>>>>> + * @state: the crtc state >>>>>>>>>>> + * @umode: userspace mode, whose aspect ratio flag bits are to >>>>>>>>>>> be updated. >>>>>>>>>>> + * >>>>>>>>>>> + * Allow the aspect ratio info in the userspace mode only if >>>>>>>>>>> aspect ratio is >>>>>>>>>>> + * supported. >>>>>>>>>>> + */ >>>>>>>>>>> +void >>>>>>>>>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state >>>>>>>>>>> *state, >>>>>>>>>>> + struct drm_mode_modeinfo *umode) >>>>>>>>>>> +{ >>>>>>>>>>> + struct drm_atomic_state *atomic_state = state->state; >>>>>>>>>>> + >>>>>>>>>>> + /* Reset the aspect ratio flags if aspect ratio is not >>>>>>>>>>> supported */ >>>>>>>>>>> + if (atomic_state && !atomic_state->allow_aspect_ratio) >>>>>>>>>>> + umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); >>>>>>>>>>> +} >>>>>>>>>>> + >>>>>>>>>>> +/** >>>>>>>>>>> * drm_atomic_crtc_set_property - set property on CRTC >>>>>>>>>>> * @crtc: the drm CRTC to set a property on >>>>>>>>>>> * @state: the state object to update with the new property >>>>>>>>>>> value >>>>>>>>>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct >>>>>>>>>>> drm_crtc *crtc, >>>>>>>>>>> else if (property == config->prop_mode_id) { >>>>>>>>>>> struct drm_property_blob *mode = >>>>>>>>>>> drm_property_lookup_blob(dev, val); >>>>>>>>>>> + drm_atomic_allow_aspect_ratio_for_umode(state, >>>>>>>>>>> + (struct drm_mode_modeinfo *) >>>>>>>>>>> + mode->data); >>>>>>>>>>> ret = drm_atomic_set_mode_prop_for_crtc(state, mode); >>>>>>>>>>> drm_property_blob_put(mode); >>>>>>>>>>> return ret; >>>>>>>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c >>>>>>>>>>> b/drivers/gpu/drm/drm_crtc.c >>>>>>>>>>> index f0556e6..a2d34fa 100644 >>>>>>>>>>> --- a/drivers/gpu/drm/drm_crtc.c >>>>>>>>>>> +++ b/drivers/gpu/drm/drm_crtc.c >>>>>>>>>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev, >>>>>>>>>>> drm_modeset_lock(&crtc->mutex, NULL); >>>>>>>>>>> if (crtc->state) { >>>>>>>>>>> if (crtc->state->enable) { >>>>>>>>>>> + /* >>>>>>>>>>> + * If the aspect-ratio is not required by the, >>>>>>>>>>> + * userspace, do not set the aspect-ratio flags. >>>>>>>>>>> + */ >>>>>>>>>>> + if (!file_priv->aspect_ratio_allowed) >>>>>>>>>>> + crtc->state->mode.picture_aspect_ratio = >>>>>>>>>>> + HDMI_PICTURE_ASPECT_NONE; >>>>>>>>>> These would still clobber the current crtc state. So definitely >>>>>>>>>> don't >>>>>>>>>> want to do this. >>>>>>>>> Alright, so alternative way of doing this will be: >>>>>>>>> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel >>>>>>>>> mode structure, >>>>>>>>> but set the aspect ratio flags in the usermode to none, after >>>>>>>>> calling the >>>>>>>>> drm_mode_convert_to_umode(). >>>>>>>>> >>>>>>>>> Will take care of this in the next patchset. >>>>>>> I gave a thought about it again. As you have mentioned earlier that >>>>>>> get_crtc is one of the >>>>>>> place where the user mode aspect ratio flags must be filtered out, is >>>>>>> this what you have >>>>>>> in mind. I hope I did not misunderstand. >>>>>>> >>>>>>> Changing the usermode aspect ratio flag bits, without change in the >>>>>>> picture_aspect_ratio >>>>>>> kernel mode will not result in both structures out of sync? >>>>>> I don't think that should really matter. I suppose we might get one >>>>>> extra modeset when the user feeds that mode back via setcrtc/atomic, but >>>>>> meh. And actually the aspect ratio should only affect the infoframe, so >>>>>> we should be able to eventually eliminate that extra modeset and just >>>>>> update the infoframe instead. >>>>>> >>>>>>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); >>>>>>>>>>> crtc_resp->mode_valid = 1; >>>>>>>>>>> @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device >>>>>>>>>>> *dev, >>>>>>>>>>> crtc_resp->x = crtc->x; >>>>>>>>>>> crtc_resp->y = crtc->y; >>>>>>>>>>> if (crtc->enabled) { >>>>>>>>>>> + /* >>>>>>>>>>> + * If the aspect-ratio is not required by the, >>>>>>>>>>> + * userspace, do not set the aspect-ratio flags. >>>>>>>>>>> + */ >>>>>>>>>>> + if (!file_priv->aspect_ratio_allowed) >>>>>>>>>>> + crtc->mode.picture_aspect_ratio = >>>>>>>>>>> + HDMI_PICTURE_ASPECT_NONE; >>>>>>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode); >>>>>>>>>>> crtc_resp->mode_valid = 1; >>>>>>>>>>> @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device >>>>>>>>>>> *dev, void *data, >>>>>>>>>>> goto out; >>>>>>>>>>> } >>>>>>>>>>> + /* If the user does not ask for aspect ratio, reset >>>>>>>>>>> the aspect >>>>>>>>>>> + * ratio bits in the usermode. >>>>>>>>>>> + */ >>>>>>>>>>> + if (!file_priv->aspect_ratio_allowed) >>>>>>>>>>> + crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK); >>>>>>>>>>> ret = drm_mode_convert_umode(mode, &crtc_req->mode); >>>>>>>>>>> if (ret) { >>>>>>>>>>> DRM_DEBUG_KMS("Invalid mode\n"); >>>>>>>>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h >>>>>>>>>>> index 1c27526..130dad9 100644 >>>>>>>>>>> --- a/include/drm/drm_atomic.h >>>>>>>>>>> +++ b/include/drm/drm_atomic.h >>>>>>>>>>> @@ -237,6 +237,7 @@ struct __drm_private_objs_state { >>>>>>>>>>> * @ref: count of all references to this state (will not be >>>>>>>>>>> freed until zero) >>>>>>>>>>> * @dev: parent DRM device >>>>>>>>>>> * @allow_modeset: allow full modeset >>>>>>>>>>> + * @allow_aspect_ratio: allow the aspect ratio info to be >>>>>>>>>>> passes to user >>>>>>>>>>> * @legacy_cursor_update: hint to enforce legacy cursor >>>>>>>>>>> IOCTL semantics >>>>>>>>>>> * @async_update: hint for asynchronous plane update >>>>>>>>>>> * @planes: pointer to array of structures with per-plane data >>>>>>>>>>> @@ -256,6 +257,7 @@ struct drm_atomic_state { >>>>>>>>>>> struct drm_device *dev; >>>>>>>>>>> bool allow_modeset : 1; >>>>>>>>>>> + bool allow_aspect_ratio : 1; >>>>>>>>>>> bool legacy_cursor_update : 1; >>>>>>>>>>> bool async_update : 1; >>>>>>>>>>> struct __drm_planes_state *planes; >>>>>>>>>>> -- >>>>>>>>>>> 2.7.4 >>>>>>>>>>> >>>>>>>>>>> _______________________________________________ >>>>>>>>>>> dri-devel mailing list >>>>>>>>>>> dri-devel@lists.freedesktop.org >>>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>>>>>> Regards, >>>>>>> Ankit [-- Attachment #1.2: Type: text/html, Size: 22153 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 6/8] drm: Expose modes with aspect ratio, only if requested 2018-01-12 6:21 [PATCH v3 0/8] Aspect ratio support in DRM layer Nautiyal, Ankit K ` (4 preceding siblings ...) 2018-01-12 6:21 ` [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths Nautiyal, Ankit K @ 2018-01-12 6:21 ` Nautiyal, Ankit K 2018-01-29 18:58 ` Ville Syrjälä 2018-01-12 6:21 ` [PATCH v3 7/8] drm: Add aspect ratio parsing in DRM layer Nautiyal, Ankit K 2018-01-12 6:21 ` [PATCH v3 8/8] drm: Add and handle new aspect ratios " Nautiyal, Ankit K 7 siblings, 1 reply; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-01-12 6:21 UTC (permalink / raw) To: dri-devel; +Cc: Jose Abreu, Ankit Nautiyal From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> We parse the EDID and add all the modes in the connector's modelist. This adds CEA modes with aspect ratio information too, regadless of if user space requested this information or not. This patch prunes the modes with aspect-ratio information, from a connector's modelist, if the user-space has not set the aspect ratio DRM client cap. Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Shashank Sharma <shashank.sharma@intel.com> Cc: Jose Abreu <jose.abreu@synopsys.com> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> V3: As suggested by Ville, modified the mechanism of pruning of modes with aspect-ratio, if the aspect-ratio is not supported. Instead of straight away pruning such a mode, the mode is retained with aspect-ratio bits set to zero, provided it is unique. --- drivers/gpu/drm/drm_connector.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index b85a774..d968ec3 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1502,7 +1502,8 @@ static struct drm_encoder *drm_connector_get_encoder(struct drm_connector *conne return connector->encoder; } -static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode, +static bool drm_mode_expose_to_userspace(const struct drm_display_mode *last_mode, + const struct drm_display_mode *mode, const struct drm_file *file_priv) { /* @@ -1511,6 +1512,18 @@ static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode, */ if (!file_priv->stereo_allowed && drm_mode_is_stereo(mode)) return false; + /* + * If user-space hasn't configured the driver to expose the modes + * with aspect-ratio, don't expose them. + */ + if (!file_priv->aspect_ratio_allowed && + mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE && + drm_mode_match(mode, last_mode, + DRM_MODE_MATCH_TIMINGS | + DRM_MODE_MATCH_CLOCK | + DRM_MODE_MATCH_FLAGS | + DRM_MODE_MATCH_3D_FLAGS)) + return false; return true; } @@ -1522,6 +1535,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, struct drm_connector *connector; struct drm_encoder *encoder; struct drm_display_mode *mode; + struct drm_display_mode last_valid_mode; int mode_count = 0; int encoders_count = 0; int ret = 0; @@ -1577,9 +1591,12 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, out_resp->connection = connector->status; /* delayed so we get modes regardless of pre-fill_modes state */ + memset(&last_valid_mode, 0, sizeof(struct drm_display_mode)); list_for_each_entry(mode, &connector->modes, head) - if (drm_mode_expose_to_userspace(mode, file_priv)) + if (drm_mode_expose_to_userspace(&last_valid_mode, mode, file_priv)) { mode_count++; + drm_mode_copy(&last_valid_mode, mode); + } /* * This ioctl is called twice, once to determine how much space is @@ -1588,10 +1605,16 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, if ((out_resp->count_modes >= mode_count) && mode_count) { copied = 0; mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr; + memset(&last_valid_mode, 0, sizeof(struct drm_display_mode)); list_for_each_entry(mode, &connector->modes, head) { - if (!drm_mode_expose_to_userspace(mode, file_priv)) + if (!drm_mode_expose_to_userspace(&last_valid_mode, + mode, + file_priv)) continue; - + if (!file_priv->aspect_ratio_allowed) + mode->picture_aspect_ratio = + HDMI_PICTURE_ASPECT_NONE; + drm_mode_copy(&last_valid_mode, mode); drm_mode_convert_to_umode(&u_mode, mode); if (copy_to_user(mode_ptr + copied, &u_mode, sizeof(u_mode))) { -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH v3 6/8] drm: Expose modes with aspect ratio, only if requested 2018-01-12 6:21 ` [PATCH v3 6/8] drm: Expose modes with aspect ratio, only if requested Nautiyal, Ankit K @ 2018-01-29 18:58 ` Ville Syrjälä 2018-01-31 8:00 ` Nautiyal, Ankit K 0 siblings, 1 reply; 35+ messages in thread From: Ville Syrjälä @ 2018-01-29 18:58 UTC (permalink / raw) To: Nautiyal, Ankit K; +Cc: Jose Abreu, dri-devel On Fri, Jan 12, 2018 at 11:51:34AM +0530, Nautiyal, Ankit K wrote: > From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > We parse the EDID and add all the modes in the connector's > modelist. This adds CEA modes with aspect ratio information > too, regadless of if user space requested this information or > not. > > This patch prunes the modes with aspect-ratio information, from > a connector's modelist, if the user-space has not set the aspect > ratio DRM client cap. > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: Shashank Sharma <shashank.sharma@intel.com> > Cc: Jose Abreu <jose.abreu@synopsys.com> > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > V3: As suggested by Ville, modified the mechanism of pruning of > modes with aspect-ratio, if the aspect-ratio is not supported. > Instead of straight away pruning such a mode, the mode is > retained with aspect-ratio bits set to zero, provided it is > unique. > --- > drivers/gpu/drm/drm_connector.c | 31 +++++++++++++++++++++++++++---- > 1 file changed, 27 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index b85a774..d968ec3 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1502,7 +1502,8 @@ static struct drm_encoder *drm_connector_get_encoder(struct drm_connector *conne > return connector->encoder; > } > > -static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode, > +static bool drm_mode_expose_to_userspace(const struct drm_display_mode *last_mode, > + const struct drm_display_mode *mode, > const struct drm_file *file_priv) > { > /* > @@ -1511,6 +1512,18 @@ static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode, > */ > if (!file_priv->stereo_allowed && drm_mode_is_stereo(mode)) > return false; > + /* > + * If user-space hasn't configured the driver to expose the modes > + * with aspect-ratio, don't expose them. > + */ > + if (!file_priv->aspect_ratio_allowed && > + mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE && > + drm_mode_match(mode, last_mode, > + DRM_MODE_MATCH_TIMINGS | > + DRM_MODE_MATCH_CLOCK | > + DRM_MODE_MATCH_FLAGS | > + DRM_MODE_MATCH_3D_FLAGS)) > + return false; > > return true; > } > @@ -1522,6 +1535,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > struct drm_connector *connector; > struct drm_encoder *encoder; > struct drm_display_mode *mode; > + struct drm_display_mode last_valid_mode; A pointer should be sufficient. > int mode_count = 0; > int encoders_count = 0; > int ret = 0; > @@ -1577,9 +1591,12 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > out_resp->connection = connector->status; > > /* delayed so we get modes regardless of pre-fill_modes state */ > + memset(&last_valid_mode, 0, sizeof(struct drm_display_mode)); > list_for_each_entry(mode, &connector->modes, head) > - if (drm_mode_expose_to_userspace(mode, file_priv)) > + if (drm_mode_expose_to_userspace(&last_valid_mode, mode, file_priv)) { > mode_count++; > + drm_mode_copy(&last_valid_mode, mode); > + } > > /* > * This ioctl is called twice, once to determine how much space is > @@ -1588,10 +1605,16 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, > if ((out_resp->count_modes >= mode_count) && mode_count) { > copied = 0; > mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr; > + memset(&last_valid_mode, 0, sizeof(struct drm_display_mode)); > list_for_each_entry(mode, &connector->modes, head) { > - if (!drm_mode_expose_to_userspace(mode, file_priv)) > + if (!drm_mode_expose_to_userspace(&last_valid_mode, > + mode, > + file_priv)) > continue; > - > + if (!file_priv->aspect_ratio_allowed) > + mode->picture_aspect_ratio = > + HDMI_PICTURE_ASPECT_NONE; Here you're clobbering the internal mode structure. That's not acceptable. > + drm_mode_copy(&last_valid_mode, mode); > drm_mode_convert_to_umode(&u_mode, mode); > if (copy_to_user(mode_ptr + copied, > &u_mode, sizeof(u_mode))) { > -- > 2.7.4 -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 6/8] drm: Expose modes with aspect ratio, only if requested 2018-01-29 18:58 ` Ville Syrjälä @ 2018-01-31 8:00 ` Nautiyal, Ankit K 0 siblings, 0 replies; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-01-31 8:00 UTC (permalink / raw) To: Ville Syrjälä; +Cc: Jose Abreu, dri-devel [-- Attachment #1.1: Type: text/plain, Size: 4863 bytes --] Hi Ville, Thanks for the comments and suggestions. Please find my response inline: On 1/30/2018 12:28 AM, Ville Syrjälä wrote: > On Fri, Jan 12, 2018 at 11:51:34AM +0530, Nautiyal, Ankit K wrote: >> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >> >> We parse the EDID and add all the modes in the connector's >> modelist. This adds CEA modes with aspect ratio information >> too, regadless of if user space requested this information or >> not. >> >> This patch prunes the modes with aspect-ratio information, from >> a connector's modelist, if the user-space has not set the aspect >> ratio DRM client cap. >> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: Shashank Sharma <shashank.sharma@intel.com> >> Cc: Jose Abreu <jose.abreu@synopsys.com> >> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >> >> V3: As suggested by Ville, modified the mechanism of pruning of >> modes with aspect-ratio, if the aspect-ratio is not supported. >> Instead of straight away pruning such a mode, the mode is >> retained with aspect-ratio bits set to zero, provided it is >> unique. >> --- >> drivers/gpu/drm/drm_connector.c | 31 +++++++++++++++++++++++++++---- >> 1 file changed, 27 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c >> index b85a774..d968ec3 100644 >> --- a/drivers/gpu/drm/drm_connector.c >> +++ b/drivers/gpu/drm/drm_connector.c >> @@ -1502,7 +1502,8 @@ static struct drm_encoder *drm_connector_get_encoder(struct drm_connector *conne >> return connector->encoder; >> } >> >> -static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode, >> +static bool drm_mode_expose_to_userspace(const struct drm_display_mode *last_mode, >> + const struct drm_display_mode *mode, >> const struct drm_file *file_priv) >> { >> /* >> @@ -1511,6 +1512,18 @@ static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode, >> */ >> if (!file_priv->stereo_allowed && drm_mode_is_stereo(mode)) >> return false; >> + /* >> + * If user-space hasn't configured the driver to expose the modes >> + * with aspect-ratio, don't expose them. >> + */ >> + if (!file_priv->aspect_ratio_allowed && >> + mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE && >> + drm_mode_match(mode, last_mode, >> + DRM_MODE_MATCH_TIMINGS | >> + DRM_MODE_MATCH_CLOCK | >> + DRM_MODE_MATCH_FLAGS | >> + DRM_MODE_MATCH_3D_FLAGS)) >> + return false; >> >> return true; >> } >> @@ -1522,6 +1535,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, >> struct drm_connector *connector; >> struct drm_encoder *encoder; >> struct drm_display_mode *mode; >> + struct drm_display_mode last_valid_mode; > A pointer should be sufficient. Thanks for pointing that out, will just save the address of last valid mode. > >> int mode_count = 0; >> int encoders_count = 0; >> int ret = 0; >> @@ -1577,9 +1591,12 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, >> out_resp->connection = connector->status; >> >> /* delayed so we get modes regardless of pre-fill_modes state */ >> + memset(&last_valid_mode, 0, sizeof(struct drm_display_mode)); >> list_for_each_entry(mode, &connector->modes, head) >> - if (drm_mode_expose_to_userspace(mode, file_priv)) >> + if (drm_mode_expose_to_userspace(&last_valid_mode, mode, file_priv)) { >> mode_count++; >> + drm_mode_copy(&last_valid_mode, mode); >> + } >> >> /* >> * This ioctl is called twice, once to determine how much space is >> @@ -1588,10 +1605,16 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, >> if ((out_resp->count_modes >= mode_count) && mode_count) { >> copied = 0; >> mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr; >> + memset(&last_valid_mode, 0, sizeof(struct drm_display_mode)); >> list_for_each_entry(mode, &connector->modes, head) { >> - if (!drm_mode_expose_to_userspace(mode, file_priv)) >> + if (!drm_mode_expose_to_userspace(&last_valid_mode, >> + mode, >> + file_priv)) >> continue; >> - >> + if (!file_priv->aspect_ratio_allowed) >> + mode->picture_aspect_ratio = >> + HDMI_PICTURE_ASPECT_NONE; > Here you're clobbering the internal mode structure. That's not > acceptable. I assumed, that the usermode, and the internal mode structure both should have aspect ratio info as none, if the user does not support aspect ratio. If that's not required, I can set the aspect ratio flags in usermode after the call to drm_mode_convert_to_umode(). -Ankit > >> + drm_mode_copy(&last_valid_mode, mode); >> drm_mode_convert_to_umode(&u_mode, mode); >> if (copy_to_user(mode_ptr + copied, >> &u_mode, sizeof(u_mode))) { >> -- >> 2.7.4 [-- Attachment #1.2: Type: text/html, Size: 6241 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 7/8] drm: Add aspect ratio parsing in DRM layer 2018-01-12 6:21 [PATCH v3 0/8] Aspect ratio support in DRM layer Nautiyal, Ankit K ` (5 preceding siblings ...) 2018-01-12 6:21 ` [PATCH v3 6/8] drm: Expose modes with aspect ratio, only if requested Nautiyal, Ankit K @ 2018-01-12 6:21 ` Nautiyal, Ankit K 2018-01-12 6:21 ` [PATCH v3 8/8] drm: Add and handle new aspect ratios " Nautiyal, Ankit K 7 siblings, 0 replies; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-01-12 6:21 UTC (permalink / raw) To: dri-devel Cc: Jose Abreu, Jia, Akashdeep Sharma, Lin, Jim Bride, Ankit Nautiyal 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/ --- 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 8128dbb..c545552 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); @@ -1621,6 +1622,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; } @@ -1666,6 +1682,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; + } + out->status = drm_mode_validate_basic(out); if (out->status != MODE_OK) goto out; -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH v3 8/8] drm: Add and handle new aspect ratios in DRM layer 2018-01-12 6:21 [PATCH v3 0/8] Aspect ratio support in DRM layer Nautiyal, Ankit K ` (6 preceding siblings ...) 2018-01-12 6:21 ` [PATCH v3 7/8] drm: Add aspect ratio parsing in DRM layer Nautiyal, Ankit K @ 2018-01-12 6:21 ` Nautiyal, Ankit K 7 siblings, 0 replies; 35+ messages in thread From: Nautiyal, Ankit K @ 2018-01-12 6:21 UTC (permalink / raw) To: dri-devel; +Cc: Jose Abreu, Ankit Nautiyal From: "Sharma, Shashank" <shashank.sharma@intel.com> HDMI 2.0/CEA-861-F introduces two new aspect ratios: - 64:27 - 256:135 This patch: - Adds new DRM flags for to represent these new aspect ratios. - Adds new cases to handle these aspect ratios while converting from user->kernel mode or vise versa. This patch was once reviewed and merged, and later reverted due to lack of DRM client protection, while adding aspect ratio bits in user modes. This is a re-spin of the series, with DRM client cap protection. The previous series can be found here: https://pw-emeril.freedesktop.org/series/10850/ Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> Reviewed-by: Sean Paul <seanpaul@chromium.org> (V2) Reviewed-by: Jose Abreu <Jose.Abreu@synopsys.com> (V2) Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: Sean Paul <seanpaul@chromium.org> Cc: Jose Abreu <Jose.Abreu@synopsys.com> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com> V3: rebase --- drivers/gpu/drm/drm_modes.c | 12 ++++++++++++ include/uapi/drm/drm_mode.h | 6 ++++++ 2 files changed, 18 insertions(+) diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index c545552..5913d93 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1631,6 +1631,12 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out, case HDMI_PICTURE_ASPECT_16_9: out->flags |= DRM_MODE_FLAG_PIC_AR_16_9; break; + case HDMI_PICTURE_ASPECT_64_27: + out->flags |= DRM_MODE_FLAG_PIC_AR_64_27; + break; + case DRM_MODE_PICTURE_ASPECT_256_135: + out->flags |= DRM_MODE_FLAG_PIC_AR_256_135; + break; case HDMI_PICTURE_ASPECT_RESERVED: default: out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; @@ -1692,6 +1698,12 @@ int drm_mode_convert_umode(struct drm_display_mode *out, case DRM_MODE_FLAG_PIC_AR_16_9: out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9; break; + case DRM_MODE_FLAG_PIC_AR_64_27: + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_64_27; + break; + case DRM_MODE_FLAG_PIC_AR_256_135: + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_256_135; + break; default: out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; break; diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index d1a69ff..e7ee7f9 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -89,6 +89,8 @@ extern "C" { #define DRM_MODE_PICTURE_ASPECT_NONE 0 #define DRM_MODE_PICTURE_ASPECT_4_3 1 #define DRM_MODE_PICTURE_ASPECT_16_9 2 +#define DRM_MODE_PICTURE_ASPECT_64_27 3 +#define DRM_MODE_PICTURE_ASPECT_256_135 4 /* Aspect ratio flag bitmask (4 bits 22:19) */ #define DRM_MODE_FLAG_PIC_AR_MASK (0x0F<<19) @@ -98,6 +100,10 @@ extern "C" { (DRM_MODE_PICTURE_ASPECT_4_3<<19) #define DRM_MODE_FLAG_PIC_AR_16_9 \ (DRM_MODE_PICTURE_ASPECT_16_9<<19) +#define DRM_MODE_FLAG_PIC_AR_64_27 \ + (DRM_MODE_PICTURE_ASPECT_64_27<<19) +#define DRM_MODE_FLAG_PIC_AR_256_135 \ + (DRM_MODE_PICTURE_ASPECT_256_135<<19) /* DPMS flags */ /* bit compatible with the xorg definitions. */ -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 35+ messages in thread
end of thread, other threads:[~2018-02-15 11:57 UTC | newest] Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-12 6:21 [PATCH v3 0/8] Aspect ratio support in DRM layer Nautiyal, Ankit K 2018-01-12 6:21 ` [PATCH v3 1/8] drm/modes: Introduce drm_mode_match() Nautiyal, Ankit K 2018-01-17 8:41 ` Sharma, Shashank 2018-01-17 15:21 ` Ville Syrjälä 2018-01-18 6:10 ` Sharma, Shashank 2018-01-12 6:21 ` [PATCH v3 2/8] drm/edid: Use drm_mode_match_no_clocks_no_stereo() for consistentcy Nautiyal, Ankit K 2018-01-17 8:53 ` Sharma, Shashank 2018-01-12 6:21 ` [PATCH v3 3/8] drm/edid: Fix cea mode aspect ratio handling Nautiyal, Ankit K 2018-01-17 9:05 ` Sharma, Shashank 2018-01-17 15:29 ` Ville Syrjälä 2018-01-18 6:14 ` Sharma, Shashank 2018-01-12 6:21 ` [PATCH v3 4/8] drm: Add DRM client cap for aspect-ratio Nautiyal, Ankit K 2018-01-17 9:11 ` Sharma, Shashank 2018-01-18 10:16 ` Maarten Lankhorst 2018-01-18 15:41 ` ankit.k.nautiyal 2018-01-18 16:04 ` Ville Syrjälä 2018-01-19 5:44 ` Nautiyal, Ankit K 2018-01-22 4:34 ` [PATCH v4 " Nautiyal, Ankit K 2018-01-12 6:21 ` [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths Nautiyal, Ankit K 2018-01-29 18:53 ` Ville Syrjälä 2018-01-31 6:34 ` Nautiyal, Ankit K 2018-01-31 13:09 ` Ville Syrjälä 2018-02-01 11:05 ` Nautiyal, Ankit K 2018-02-01 12:54 ` Ville Syrjälä 2018-02-08 4:29 ` Nautiyal, Ankit K 2018-02-13 4:51 ` Nautiyal, Ankit K 2018-02-13 13:18 ` Ville Syrjälä 2018-02-13 16:23 ` Nautiyal, Ankit K 2018-02-13 16:45 ` Ville Syrjälä 2018-02-15 11:57 ` Nautiyal, Ankit K 2018-01-12 6:21 ` [PATCH v3 6/8] drm: Expose modes with aspect ratio, only if requested Nautiyal, Ankit K 2018-01-29 18:58 ` Ville Syrjälä 2018-01-31 8:00 ` Nautiyal, Ankit K 2018-01-12 6:21 ` [PATCH v3 7/8] drm: Add aspect ratio parsing in DRM layer Nautiyal, Ankit K 2018-01-12 6:21 ` [PATCH v3 8/8] drm: Add and handle new aspect ratios " Nautiyal, Ankit K
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).