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 5 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, and also ensures that no bogus aspect-ratios are sent in the AVI infoframes.
This patch series, adds a DRM client option for aspect ratio, and loads aspect ratio flags, only when the client sets this cap.
To test this patch, the testdiplay IGT test is modified to have an option to do a modeset with only aspect ratio modes. Also, there is a userspace implementation 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 legacy modeset path 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ä (5): 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 drm/edid: Don't send bogus aspect ratios in AVI infoframes video/hdmi: Reject illegal picture aspect ratios
drivers/gpu/drm/drm_connector.c | 45 +++++-- drivers/gpu/drm/drm_crtc.c | 8 ++ drivers/gpu/drm/drm_crtc_internal.h | 6 + drivers/gpu/drm/drm_edid.c | 41 +++++-- drivers/gpu/drm/drm_fb_helper.c | 12 +- drivers/gpu/drm/drm_ioctl.c | 9 ++ drivers/gpu/drm/drm_modes.c | 226 +++++++++++++++++++++++++++++++----- drivers/video/hdmi.c | 3 + include/drm/drm_file.h | 8 ++ include/drm/drm_modes.h | 22 ++++ include/uapi/drm/drm.h | 7 ++ include/uapi/drm/drm_mode.h | 6 + 12 files changed, 343 insertions(+), 50 deletions(-)
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 Reviewed-by: Shashank Sharma shashank.sharma@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 e82b61e..c395a24 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -939,17 +939,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; @@ -957,15 +1008,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;
- return drm_mode_equal_no_clocks(mode1, mode2); + 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; + + 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);
@@ -980,13 +1064,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);
@@ -1004,21 +1088,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 0d310be..2f78b7e 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -147,6 +147,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 @@ -490,6 +496,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,
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 Reviewed-by: Shashank Sharma shashank.sharma@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 08d33b4..5475f31 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -3047,7 +3047,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; }
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 Reviewed-by: Shashank Sharma shashank.sharma@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 5475f31..3f157c8 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2930,11 +2930,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; @@ -2948,7 +2952,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)); } @@ -2965,11 +2969,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; @@ -2983,7 +2991,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)); } @@ -3030,6 +3038,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) @@ -3047,7 +3056,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; }
@@ -3064,6 +3073,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) @@ -3079,7 +3089,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;
From: Ville Syrjälä ville.syrjala@linux.intel.com
If the user mode would specify an aspect ratio other than 4:3 or 16:9 we now silently ignore it. Maybe a better apporoach is to return an error? Let's try that.
Also we must be careful that we don't try to send illegal picture aspect in the infoframe as it's only capable of signalling none, 4:3, and 16:9. Currently we're sending these bogus infoframes whenever the cea mode specifies some other aspect ratio.
Cc: Shashank Sharma shashank.sharma@intel.com Cc: Sean Paul seanpaul@chromium.org 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 Reviewed-by: Shashank Sharma shashank.sharma@intel.com --- drivers/gpu/drm/drm_edid.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 3f157c8..40e1e24 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -4833,6 +4833,7 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame, const struct drm_display_mode *mode, bool is_hdmi2_sink) { + enum hdmi_picture_aspect picture_aspect; int err;
if (!frame || !mode) @@ -4875,13 +4876,23 @@ drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame, * Populate picture aspect ratio from either * user input (if specified) or from the CEA mode list. */ - if (mode->picture_aspect_ratio == HDMI_PICTURE_ASPECT_4_3 || - mode->picture_aspect_ratio == HDMI_PICTURE_ASPECT_16_9) - frame->picture_aspect = mode->picture_aspect_ratio; - else if (frame->video_code > 0) - frame->picture_aspect = drm_get_cea_aspect_ratio( - frame->video_code); + picture_aspect = mode->picture_aspect_ratio; + if (picture_aspect == HDMI_PICTURE_ASPECT_NONE) + picture_aspect = drm_get_cea_aspect_ratio(frame->video_code);
+ /* + * The infoframe can't convey anything but none, 4:3 + * and 16:9, so if the user has asked for anything else + * we can only satisfy it by specifying the right VIC. + */ + if (picture_aspect > HDMI_PICTURE_ASPECT_16_9) { + if (picture_aspect != + drm_get_cea_aspect_ratio(frame->video_code)) + return -EINVAL; + picture_aspect = HDMI_PICTURE_ASPECT_NONE; + } + + frame->picture_aspect = picture_aspect; frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE; frame->scan_mode = HDMI_SCAN_MODE_UNDERSCAN;
From: Ville Syrjälä ville.syrjala@linux.intel.com
AVI infoframe can only carry none, 4:3, or 16:9 picture aspect ratios. Return an error if the user asked for something different.
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 Cc: Thierry Reding thierry.reding@gmail.com Cc: Hans Verkuil hans.verkuil@cisco.com Cc: linux-media@vger.kernel.org Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com Reviewed-by: Jose Abreu joabreu@synopsys.com --- drivers/video/hdmi.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c index 111a0ab..38716eb5 100644 --- a/drivers/video/hdmi.c +++ b/drivers/video/hdmi.c @@ -93,6 +93,9 @@ ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infoframe *frame, void *buffer, if (size < length) return -ENOSPC;
+ if (frame->picture_aspect > HDMI_PICTURE_ASPECT_16_9) + return -EINVAL; + memset(buffer, 0, size);
ptr[0] = frame->type;
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 non-atomic 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 non-atomic 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 info in modes or not.
This patch adds the client cap for aspect-ratio.
Since no atomic-userspaces blow up on receiving aspect-ratio information, the client cap for aspect-ratio is always enabled for atomic clients.
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. V5: rebase V6: rebase V7: rebase V8: rebase V9: rebase V10: rebase V11: rebase V12: As suggested by Daniel Vetter and Ville Syrjala, always enable aspect-ratio client cap for atomic userspaces, if no atomic userspace breaks on aspect-ratio bits. V13: rebase
Reviewed-by: Shashank Sharma shashank.sharma@intel.com --- drivers/gpu/drm/drm_ioctl.c | 9 +++++++++ include/drm/drm_file.h | 8 ++++++++ include/uapi/drm/drm.h | 7 +++++++ 3 files changed, 24 insertions(+)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index af78291..0379983 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -324,6 +324,15 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv) return -EINVAL; file_priv->atomic = req->value; file_priv->universal_planes = req->value; + /* + * No atomic user-space blows up on aspect ratio mode bits. + */ + file_priv->aspect_ratio_allowed = 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 5176c37..02b7dde 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..9c660e1 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;
On Wed, May 02, 2018 at 12:20:18PM +0530, Nautiyal, Ankit K wrote:
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 non-atomic 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 non-atomic 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 info in modes or not.
This patch adds the client cap for aspect-ratio.
Since no atomic-userspaces blow up on receiving aspect-ratio information, the client cap for aspect-ratio is always enabled for atomic clients.
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. V5: rebase V6: rebase V7: rebase V8: rebase V9: rebase V10: rebase V11: rebase V12: As suggested by Daniel Vetter and Ville Syrjala, always enable aspect-ratio client cap for atomic userspaces, if no atomic userspace breaks on aspect-ratio bits. V13: rebase
Reviewed-by: Shashank Sharma shashank.sharma@intel.com
Yeah this is what I had in mind with auto-enabling this for atomic clients.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_ioctl.c | 9 +++++++++ include/drm/drm_file.h | 8 ++++++++ include/uapi/drm/drm.h | 7 +++++++ 3 files changed, 24 insertions(+)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index af78291..0379983 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -324,6 +324,15 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv) return -EINVAL; file_priv->atomic = req->value; file_priv->universal_planes = req->value;
/** No atomic user-space blows up on aspect ratio mode bits.*/file_priv->aspect_ratio_allowed = req->value;break;- case DRM_CLIENT_CAP_ASPECT_RATIO:
if (req->value > 1)return -EINVAL;file_priv->aspect_ratio_allowed = req->value;diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index 5176c37..02b7dde 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..9c660e1 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
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
From: Ankit Nautiyal ankit.k.nautiyal@intel.com
If the user-space does not support aspect-ratio, and requests for a modeset with mode having aspect ratio bits set, then the given user-mode must be rejected. Secondly, 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. rejects the modes with aspect-ratio info, during modeset, if the user does not support aspect ratio. 2. does not load the aspect-ratio info in user-mode structure, if aspect ratio is not supported. 3. adds helper functions for determining if aspect-ratio is expected in user-mode and for allowing/disallowing the aspect-ratio, if its not expected.
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. V4: rebase V5: As suggested by Ville, rejected the modeset calls for modes with aspect ratio, if the user does not set aspect-ratio cap. V6: Used the helper functions for determining if aspect-ratio is expected in the user-mode. V7: rebase V8: rebase V9: rebase V10: Modified the commit-message V11: rebase V12: Merged the patch for adding aspect-ratio helper functions with this patch. V13: Minor modifications as suggested by Ville. --- drivers/gpu/drm/drm_crtc.c | 8 +++++++ drivers/gpu/drm/drm_crtc_internal.h | 6 +++++ drivers/gpu/drm/drm_modes.c | 47 +++++++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index a231dd5..98323f4 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -449,6 +449,7 @@ int drm_mode_getcrtc(struct drm_device *dev, crtc_resp->mode_valid = 0; } } + drm_mode_filter_aspect_ratio_flags(file_priv, &crtc_resp->mode); drm_modeset_unlock(&crtc->mutex);
return 0; @@ -628,6 +629,13 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, ret = -ENOMEM; goto out; } + if (!drm_mode_aspect_ratio_allowed(file_priv, + &crtc_req->mode)) { + DRM_DEBUG_KMS("Unexpected aspect-ratio flag bits\n"); + ret = -EINVAL; + goto out; + } +
ret = drm_mode_convert_umode(dev, mode, &crtc_req->mode); if (ret) { diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index 5d307b2..31d6c77 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -222,3 +222,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, void drm_mode_fixup_1366x768(struct drm_display_mode *mode); void drm_reset_display_info(struct drm_connector *connector); u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid); + +/* drm_modes.c */ +bool drm_mode_aspect_ratio_allowed(const struct drm_file *file_priv, + struct drm_mode_modeinfo *umode); +void drm_mode_filter_aspect_ratio_flags(const struct drm_file *file_priv, + struct drm_mode_modeinfo *umode); diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index c395a24..2bf2f0b 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1759,3 +1759,50 @@ bool drm_mode_is_420(const struct drm_display_info *display, drm_mode_is_420_also(display, mode); } EXPORT_SYMBOL(drm_mode_is_420); + +/** + * drm_mode_aspect_ratio_allowed - checks if the aspect-ratio information + * is expected from the user-mode. + * + * If the user has set aspect-ratio cap, then the flag of the user-mode is + * allowed to contain aspect-ratio value. + * If the user does not set aspect-ratio cap, then the only value allowed in the + * flags bits is aspect-ratio NONE. + * + * @file_priv: file private structure to get the user capabilities + * @umode: drm_mode_modeinfo struct, whose flag carry the aspect ratio + * information. + * + * Returns: + * true if the aspect-ratio info is allowed in the user-mode flags. + * false, otherwise. + */ +bool +drm_mode_aspect_ratio_allowed(const struct drm_file *file_priv, + struct drm_mode_modeinfo *umode) +{ + if (file_priv->aspect_ratio_allowed || + (umode->flags & DRM_MODE_FLAG_PIC_AR_MASK) == DRM_MODE_FLAG_PIC_AR_NONE) + return true; + return false; +} + +/** + * drm_mode_filter_aspect_ratio_flags - filters the aspect-ratio bits in the + * user-mode flags. + * + * Checks if the aspect-ratio information is allowed. Resets the aspect-ratio + * bits in the user-mode flags, if aspect-ratio info is not allowed. + * + * @file_priv: file private structure to get the user capabilities. + * @umode: drm_mode_modeinfo struct, whose flags' aspect-ratio bits needs to + * be filtered. + * + */ +void +drm_mode_filter_aspect_ratio_flags(const struct drm_file *file_priv, + struct drm_mode_modeinfo *umode) +{ + if (!drm_mode_aspect_ratio_allowed(file_priv, umode)) + umode->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; +}
On Wed, May 02, 2018 at 12:20:19PM +0530, Nautiyal, Ankit K wrote:
From: Ankit Nautiyal ankit.k.nautiyal@intel.com
If the user-space does not support aspect-ratio, and requests for a modeset with mode having aspect ratio bits set, then the given user-mode must be rejected. Secondly, 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:
- rejects the modes with aspect-ratio info, during modeset, if the user does not support aspect ratio.
- does not load the aspect-ratio info in user-mode structure, if aspect ratio is not supported.
- adds helper functions for determining if aspect-ratio is expected in user-mode and for allowing/disallowing the aspect-ratio, if its not expected.
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. V4: rebase V5: As suggested by Ville, rejected the modeset calls for modes with aspect ratio, if the user does not set aspect-ratio cap. V6: Used the helper functions for determining if aspect-ratio is expected in the user-mode. V7: rebase V8: rebase V9: rebase V10: Modified the commit-message V11: rebase V12: Merged the patch for adding aspect-ratio helper functions with this patch. V13: Minor modifications as suggested by Ville.
drivers/gpu/drm/drm_crtc.c | 8 +++++++ drivers/gpu/drm/drm_crtc_internal.h | 6 +++++ drivers/gpu/drm/drm_modes.c | 47 +++++++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index a231dd5..98323f4 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -449,6 +449,7 @@ int drm_mode_getcrtc(struct drm_device *dev, crtc_resp->mode_valid = 0; } }
drm_mode_filter_aspect_ratio_flags(file_priv, &crtc_resp->mode); drm_modeset_unlock(&crtc->mutex);
return 0;
@@ -628,6 +629,13 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, ret = -ENOMEM; goto out; }
if (!drm_mode_aspect_ratio_allowed(file_priv,&crtc_req->mode)) {DRM_DEBUG_KMS("Unexpected aspect-ratio flag bits\n");ret = -EINVAL;goto out;}ret = drm_mode_convert_umode(dev, mode, &crtc_req->mode); if (ret) {
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index 5d307b2..31d6c77 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -222,3 +222,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, void drm_mode_fixup_1366x768(struct drm_display_mode *mode); void drm_reset_display_info(struct drm_connector *connector); u32 drm_add_display_info(struct drm_connector *connector, const struct edid *edid);
+/* drm_modes.c */ +bool drm_mode_aspect_ratio_allowed(const struct drm_file *file_priv,
struct drm_mode_modeinfo *umode);+void drm_mode_filter_aspect_ratio_flags(const struct drm_file *file_priv,
struct drm_mode_modeinfo *umode);diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index c395a24..2bf2f0b 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1759,3 +1759,50 @@ bool drm_mode_is_420(const struct drm_display_info *display, drm_mode_is_420_also(display, mode); } EXPORT_SYMBOL(drm_mode_is_420);
+/**
- drm_mode_aspect_ratio_allowed - checks if the aspect-ratio information
- is expected from the user-mode.
- If the user has set aspect-ratio cap, then the flag of the user-mode is
- allowed to contain aspect-ratio value.
- If the user does not set aspect-ratio cap, then the only value allowed in the
- flags bits is aspect-ratio NONE.
- @file_priv: file private structure to get the user capabilities
- @umode: drm_mode_modeinfo struct, whose flag carry the aspect ratio
- information.
- Returns:
- true if the aspect-ratio info is allowed in the user-mode flags.
- false, otherwise.
- */
We generally don't do full kerneldoc for drm.ko internal functions (which these both are), only for driver functions. I'd remove them both because they don't add a hole lot really.
+bool +drm_mode_aspect_ratio_allowed(const struct drm_file *file_priv,
struct drm_mode_modeinfo *umode)
I don't really see the point of this helper. It has a bit a confusing name, and open-code it looks simpler to me in setcrtc.
+{
- if (file_priv->aspect_ratio_allowed ||
(umode->flags & DRM_MODE_FLAG_PIC_AR_MASK) == DRM_MODE_FLAG_PIC_AR_NONE)return true;- return false;
+}
+/**
- drm_mode_filter_aspect_ratio_flags - filters the aspect-ratio bits in the
- user-mode flags.
- Checks if the aspect-ratio information is allowed. Resets the aspect-ratio
- bits in the user-mode flags, if aspect-ratio info is not allowed.
- @file_priv: file private structure to get the user capabilities.
- @umode: drm_mode_modeinfo struct, whose flags' aspect-ratio bits needs to
- be filtered.
- */
+void +drm_mode_filter_aspect_ratio_flags(const struct drm_file *file_priv,
struct drm_mode_modeinfo *umode)+{
- if (!drm_mode_aspect_ratio_allowed(file_priv, umode))
You only need to check for file_priv->aspect_ratio_allowed here, then clear unconditionally. -Daniel
umode->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;+}
2.7.4
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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, regardless of whether 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. However if such a mode is unique in the list, it is kept in the list, with aspect-ratio flags reset. -prepares a list of exposed modes, which is used to find unique modes if aspect-ratio is not allowed. -adds a new list_head 'exposed_head' in drm_mode_display, to traverse the list of exposed modes.
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. V4: rebase V5: Addressed review comments from Ville: -used a pointer to store last valid mode. -avoided, modifying of picture_aspect_ratio in kernel mode, instead only flags bits of user mode are reset (if aspect-ratio is not supported). V6: As suggested by Ville, corrected the mode pruning logic and elaborated the mode pruning logic and the assumptions taken. V7: rebase V8: rebase V9: rebase V10: rebase V11: Fixed the issue caused in kms_3d test, and enhanced the pruning logic to correctly identify and prune modes with aspect-ratio, if aspect-ratio cap is not set. V12: As suggested by Ville, added another list_head in drm_mode_display to traverse the list of exposed modes and avoided duplication of modes. V13: Minor modifications, as suggested by Ville. --- drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++------- include/drm/drm_modes.h | 13 ++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index dfc8ca1..8ca1149 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1531,15 +1531,36 @@ 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, - const struct drm_file *file_priv) +static bool +drm_mode_expose_to_userspace(const struct drm_display_mode *mode, + const struct list_head *export_list, + const struct drm_file *file_priv) { /* * If user-space hasn't configured the driver to expose the stereo 3D * modes, don't expose them. */ + 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. However if such a mode + * is unique, let it be exposed, but reset the aspect-ratio flags + * while preparing the list of user-modes. + */ + if (!file_priv->aspect_ratio_allowed && + mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) { + struct drm_display_mode *mode_itr; + + list_for_each_entry(mode_itr, export_list, export_head) + if (drm_mode_match(mode_itr, mode, + DRM_MODE_MATCH_TIMINGS | + DRM_MODE_MATCH_CLOCK | + DRM_MODE_MATCH_FLAGS | + DRM_MODE_MATCH_3D_FLAGS)) + return false; + }
return true; } @@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, struct drm_mode_modeinfo u_mode; struct drm_mode_modeinfo __user *mode_ptr; uint32_t __user *encoder_ptr; + LIST_HEAD(export_list);
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL; @@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
/* delayed so we get modes regardless of pre-fill_modes state */ list_for_each_entry(mode, &connector->modes, head) - if (drm_mode_expose_to_userspace(mode, file_priv)) + if (drm_mode_expose_to_userspace(mode, &export_list, + file_priv)) { + list_add_tail(&mode->export_head, &export_list); mode_count++; + }
/* * This ioctl is called twice, once to determine how much space is * needed, and the 2nd time to fill it. + * The modes that need to be exposed to the user are maintained in the + * 'export_list'. When the ioctl is called first time to determine the, + * space, the export_list gets filled, to find the no.of modes. In the + * 2nd time, the user modes are filled, one by one from the export_list. */ if ((out_resp->count_modes >= mode_count) && mode_count) { copied = 0; mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr; - list_for_each_entry(mode, &connector->modes, head) { - if (!drm_mode_expose_to_userspace(mode, file_priv)) - continue; - + list_for_each_entry(mode, &export_list, export_head) { drm_mode_convert_to_umode(&u_mode, mode); + /* + * Reset aspect ratio flags of user-mode, if modes with + * aspect-ratio are not supported. + */ + drm_mode_filter_aspect_ratio_flags(file_priv, &u_mode); if (copy_to_user(mode_ptr + copied, &u_mode, sizeof(u_mode))) { ret = -EFAULT; diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index 2f78b7e..b159fe0 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -411,6 +411,19 @@ struct drm_display_mode { * Field for setting the HDMI picture aspect ratio of a mode. */ enum hdmi_picture_aspect picture_aspect_ratio; + + /** + * @export_head: + * + * struct list_head for modes to be exposed to the userspace. + * This is to maintain a list of exposed modes while preparing + * user-mode's list in drm_mode_getconnector ioctl. The purpose of this + * list_head only lies in the ioctl function, and is not expected to be + * used outside the function. + * Once used, the stale pointers are not reset, but left as it is, to + * avoid overhead of protecting it by mode_config.mutex. + */ + struct list_head export_head; };
/**
On Wed, May 02, 2018 at 12:20:20PM +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, regardless of whether 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. However if such a mode is unique in the list, it is kept in the list, with aspect-ratio flags reset. -prepares a list of exposed modes, which is used to find unique modes if aspect-ratio is not allowed. -adds a new list_head 'exposed_head' in drm_mode_display, to traverse the list of exposed modes.
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. V4: rebase V5: Addressed review comments from Ville: -used a pointer to store last valid mode. -avoided, modifying of picture_aspect_ratio in kernel mode, instead only flags bits of user mode are reset (if aspect-ratio is not supported). V6: As suggested by Ville, corrected the mode pruning logic and elaborated the mode pruning logic and the assumptions taken. V7: rebase V8: rebase V9: rebase V10: rebase V11: Fixed the issue caused in kms_3d test, and enhanced the pruning logic to correctly identify and prune modes with aspect-ratio, if aspect-ratio cap is not set. V12: As suggested by Ville, added another list_head in drm_mode_display to traverse the list of exposed modes and avoided duplication of modes. V13: Minor modifications, as suggested by Ville.
drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++------- include/drm/drm_modes.h | 13 ++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index dfc8ca1..8ca1149 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1531,15 +1531,36 @@ 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,
const struct drm_file *file_priv)+static bool +drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
const struct list_head *export_list,const struct drm_file *file_priv){ /* * If user-space hasn't configured the driver to expose the stereo 3D * modes, don't expose them. */
- 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. However if such a mode* is unique, let it be exposed, but reset the aspect-ratio flags* while preparing the list of user-modes.*/- if (!file_priv->aspect_ratio_allowed &&
mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {struct drm_display_mode *mode_itr;list_for_each_entry(mode_itr, export_list, export_head)
By walking the list of only the modes already added to the export list we rely on ASPECT_NONE being first if present. That seems to be a bit fragile. If we instead walk over all present modes (i.e. connector->modes) then I think that's avoided. With that changed:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
if (drm_mode_match(mode_itr, mode,DRM_MODE_MATCH_TIMINGS |DRM_MODE_MATCH_CLOCK |DRM_MODE_MATCH_FLAGS |DRM_MODE_MATCH_3D_FLAGS))return false;}
return true;
} @@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, struct drm_mode_modeinfo u_mode; struct drm_mode_modeinfo __user *mode_ptr; uint32_t __user *encoder_ptr;
LIST_HEAD(export_list);
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;
@@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
/* delayed so we get modes regardless of pre-fill_modes state */ list_for_each_entry(mode, &connector->modes, head)
if (drm_mode_expose_to_userspace(mode, file_priv))
if (drm_mode_expose_to_userspace(mode, &export_list,file_priv)) {list_add_tail(&mode->export_head, &export_list); mode_count++;}/*
- This ioctl is called twice, once to determine how much space is
- needed, and the 2nd time to fill it.
* The modes that need to be exposed to the user are maintained in the* 'export_list'. When the ioctl is called first time to determine the,* space, the export_list gets filled, to find the no.of modes. In the* 2nd time, the user modes are filled, one by one from the export_list.*/ if ((out_resp->count_modes >= mode_count) && mode_count) { copied = 0; mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
list_for_each_entry(mode, &connector->modes, head) {if (!drm_mode_expose_to_userspace(mode, file_priv))continue;
list_for_each_entry(mode, &export_list, export_head) { drm_mode_convert_to_umode(&u_mode, mode);/** Reset aspect ratio flags of user-mode, if modes with* aspect-ratio are not supported.*/drm_mode_filter_aspect_ratio_flags(file_priv, &u_mode); if (copy_to_user(mode_ptr + copied, &u_mode, sizeof(u_mode))) { ret = -EFAULT;diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index 2f78b7e..b159fe0 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -411,6 +411,19 @@ struct drm_display_mode { * Field for setting the HDMI picture aspect ratio of a mode. */ enum hdmi_picture_aspect picture_aspect_ratio;
- /**
* @export_head:** struct list_head for modes to be exposed to the userspace.* This is to maintain a list of exposed modes while preparing* user-mode's list in drm_mode_getconnector ioctl. The purpose of this* list_head only lies in the ioctl function, and is not expected to be* used outside the function.* Once used, the stale pointers are not reset, but left as it is, to* avoid overhead of protecting it by mode_config.mutex.*/- struct list_head export_head;
};
/**
2.7.4
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:
On Wed, May 02, 2018 at 12:20:20PM +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, regardless of whether 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. However if such a mode is unique in the list, it is kept in the list, with aspect-ratio flags reset. -prepares a list of exposed modes, which is used to find unique modes if aspect-ratio is not allowed. -adds a new list_head 'exposed_head' in drm_mode_display, to traverse the list of exposed modes.
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. V4: rebase V5: Addressed review comments from Ville: -used a pointer to store last valid mode. -avoided, modifying of picture_aspect_ratio in kernel mode, instead only flags bits of user mode are reset (if aspect-ratio is not supported). V6: As suggested by Ville, corrected the mode pruning logic and elaborated the mode pruning logic and the assumptions taken. V7: rebase V8: rebase V9: rebase V10: rebase V11: Fixed the issue caused in kms_3d test, and enhanced the pruning logic to correctly identify and prune modes with aspect-ratio, if aspect-ratio cap is not set. V12: As suggested by Ville, added another list_head in drm_mode_display to traverse the list of exposed modes and avoided duplication of modes. V13: Minor modifications, as suggested by Ville.
drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++------- include/drm/drm_modes.h | 13 ++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index dfc8ca1..8ca1149 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1531,15 +1531,36 @@ 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,
const struct drm_file *file_priv)+static bool +drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
const struct list_head *export_list,const struct drm_file *file_priv){ /* * If user-space hasn't configured the driver to expose the stereo 3D * modes, don't expose them. */
- 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. However if such a mode* is unique, let it be exposed, but reset the aspect-ratio flags* while preparing the list of user-modes.*/- if (!file_priv->aspect_ratio_allowed &&
mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {struct drm_display_mode *mode_itr;list_for_each_entry(mode_itr, export_list, export_head)By walking the list of only the modes already added to the export list we rely on ASPECT_NONE being first if present. That seems to be a bit fragile. If we instead walk over all present modes (i.e. connector->modes) then I think that's avoided.
I don't think that would work. If we just walk over all the modes then wouldn't we always find a duplicate when the same mode with two different aspect ratios is on the list? And then we'd export neither? Or maybe I misunderstood what you mean here.
To me it seems like the correct option is to check against the export_list unconditionally, no matter whether the current mode has the aspect ratio set or not. If an identical mode is already on the list we don't export again, if it's not there then we export.
With that changed:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
if (drm_mode_match(mode_itr, mode,DRM_MODE_MATCH_TIMINGS |DRM_MODE_MATCH_CLOCK |DRM_MODE_MATCH_FLAGS |DRM_MODE_MATCH_3D_FLAGS))return false;}
return true;
} @@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, struct drm_mode_modeinfo u_mode; struct drm_mode_modeinfo __user *mode_ptr; uint32_t __user *encoder_ptr;
LIST_HEAD(export_list);
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;
@@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
/* delayed so we get modes regardless of pre-fill_modes state */ list_for_each_entry(mode, &connector->modes, head)
if (drm_mode_expose_to_userspace(mode, file_priv))
if (drm_mode_expose_to_userspace(mode, &export_list,file_priv)) {list_add_tail(&mode->export_head, &export_list); mode_count++;}/*
- This ioctl is called twice, once to determine how much space is
- needed, and the 2nd time to fill it.
* The modes that need to be exposed to the user are maintained in the* 'export_list'. When the ioctl is called first time to determine the,* space, the export_list gets filled, to find the no.of modes. In the* 2nd time, the user modes are filled, one by one from the export_list.*/ if ((out_resp->count_modes >= mode_count) && mode_count) { copied = 0; mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
list_for_each_entry(mode, &connector->modes, head) {if (!drm_mode_expose_to_userspace(mode, file_priv))continue;
list_for_each_entry(mode, &export_list, export_head) { drm_mode_convert_to_umode(&u_mode, mode);/** Reset aspect ratio flags of user-mode, if modes with* aspect-ratio are not supported.*/drm_mode_filter_aspect_ratio_flags(file_priv, &u_mode); if (copy_to_user(mode_ptr + copied, &u_mode, sizeof(u_mode))) { ret = -EFAULT;diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index 2f78b7e..b159fe0 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -411,6 +411,19 @@ struct drm_display_mode { * Field for setting the HDMI picture aspect ratio of a mode. */ enum hdmi_picture_aspect picture_aspect_ratio;
- /**
* @export_head:** struct list_head for modes to be exposed to the userspace.* This is to maintain a list of exposed modes while preparing* user-mode's list in drm_mode_getconnector ioctl. The purpose of this* list_head only lies in the ioctl function, and is not expected to be* used outside the function.* Once used, the stale pointers are not reset, but left as it is, to* avoid overhead of protecting it by mode_config.mutex.*/- struct list_head export_head;
};
/**
2.7.4
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 5/5/2018 1:49 AM, Ville Syrjälä wrote:
On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:
On Wed, May 02, 2018 at 12:20:20PM +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, regardless of whether 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. However if such a mode is unique in the list, it is kept in the list, with aspect-ratio flags reset. -prepares a list of exposed modes, which is used to find unique modes if aspect-ratio is not allowed. -adds a new list_head 'exposed_head' in drm_mode_display, to traverse the list of exposed modes.
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. V4: rebase V5: Addressed review comments from Ville: -used a pointer to store last valid mode. -avoided, modifying of picture_aspect_ratio in kernel mode, instead only flags bits of user mode are reset (if aspect-ratio is not supported). V6: As suggested by Ville, corrected the mode pruning logic and elaborated the mode pruning logic and the assumptions taken. V7: rebase V8: rebase V9: rebase V10: rebase V11: Fixed the issue caused in kms_3d test, and enhanced the pruning logic to correctly identify and prune modes with aspect-ratio, if aspect-ratio cap is not set. V12: As suggested by Ville, added another list_head in drm_mode_display to traverse the list of exposed modes and avoided duplication of modes. V13: Minor modifications, as suggested by Ville.
drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++------- include/drm/drm_modes.h | 13 ++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index dfc8ca1..8ca1149 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1531,15 +1531,36 @@ 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,
const struct drm_file *file_priv)+static bool +drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
const struct list_head *export_list,const struct drm_file *file_priv)*/
- If user-space hasn't configured the driver to expose the stereo 3D
- modes, don't expose them.
- 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. However if such a mode* is unique, let it be exposed, but reset the aspect-ratio flags* while preparing the list of user-modes.*/- if (!file_priv->aspect_ratio_allowed &&
mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {struct drm_display_mode *mode_itr;list_for_each_entry(mode_itr, export_list, export_head)By walking the list of only the modes already added to the export list we rely on ASPECT_NONE being first if present. That seems to be a bit fragile. If we instead walk over all present modes (i.e. connector->modes) then I think that's avoided.
I don't think that would work. If we just walk over all the modes then wouldn't we always find a duplicate when the same mode with two different aspect ratios is on the list? And then we'd export neither? Or maybe I misunderstood what you mean here.
To me it seems like the correct option is to check against the export_list unconditionally, no matter whether the current mode has the aspect ratio set or not. If an identical mode is already on the list we don't export again, if it's not there then we export.
Agreed. The current code does have a problem rightly pointed by Daniel Vetter and the main reason for that is - we are checking for duplicates only if the mode has some aspect-ratio. As you have suggested, If we can do away with the condition to check for duplicates for only "modes having aspect ratio", it will solve the problem. In that case, if the aspect-ratio cap is not set, we will be removing the duplicates for all modes, irrespective of having aspect-ratio information or not, but if aspect-ratio is allowed, we would do no such checking.
Or do you suggest that we remove duplicates in any case, whether aspect-ratio cap is set by the user or not?
With that changed:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
if (drm_mode_match(mode_itr, mode,DRM_MODE_MATCH_TIMINGS |DRM_MODE_MATCH_CLOCK |DRM_MODE_MATCH_FLAGS |DRM_MODE_MATCH_3D_FLAGS))return false;}
return true; }
@@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, struct drm_mode_modeinfo u_mode; struct drm_mode_modeinfo __user *mode_ptr; uint32_t __user *encoder_ptr;
LIST_HEAD(export_list);
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;
@@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
/* delayed so we get modes regardless of pre-fill_modes state */ list_for_each_entry(mode, &connector->modes, head)
if (drm_mode_expose_to_userspace(mode, file_priv))
if (drm_mode_expose_to_userspace(mode, &export_list,file_priv)) {list_add_tail(&mode->export_head, &export_list); mode_count++;}/*
- This ioctl is called twice, once to determine how much space is
- needed, and the 2nd time to fill it.
* The modes that need to be exposed to the user are maintained in the* 'export_list'. When the ioctl is called first time to determine the,* space, the export_list gets filled, to find the no.of modes. In the* 2nd time, the user modes are filled, one by one from the export_list.*/ if ((out_resp->count_modes >= mode_count) && mode_count) { copied = 0; mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
list_for_each_entry(mode, &connector->modes, head) {if (!drm_mode_expose_to_userspace(mode, file_priv))continue;
list_for_each_entry(mode, &export_list, export_head) { drm_mode_convert_to_umode(&u_mode, mode);/** Reset aspect ratio flags of user-mode, if modes with* aspect-ratio are not supported.*/drm_mode_filter_aspect_ratio_flags(file_priv, &u_mode); if (copy_to_user(mode_ptr + copied, &u_mode, sizeof(u_mode))) { ret = -EFAULT;diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index 2f78b7e..b159fe0 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -411,6 +411,19 @@ struct drm_display_mode { * Field for setting the HDMI picture aspect ratio of a mode. */ enum hdmi_picture_aspect picture_aspect_ratio;
/**
* @export_head:** struct list_head for modes to be exposed to the userspace.* This is to maintain a list of exposed modes while preparing* user-mode's list in drm_mode_getconnector ioctl. The purpose of this* list_head only lies in the ioctl function, and is not expected to be* used outside the function.* Once used, the stale pointers are not reset, but left as it is, to* avoid overhead of protecting it by mode_config.mutex.*/struct list_head export_head; };
/**
-- 2.7.4
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, May 07, 2018 at 10:34:53AM +0530, Nautiyal, Ankit K wrote:
On 5/5/2018 1:49 AM, Ville Syrjälä wrote:
On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:
On Wed, May 02, 2018 at 12:20:20PM +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, regardless of whether 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. However if such a mode is unique in the list, it is kept in the list, with aspect-ratio flags reset. -prepares a list of exposed modes, which is used to find unique modes if aspect-ratio is not allowed. -adds a new list_head 'exposed_head' in drm_mode_display, to traverse the list of exposed modes.
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. V4: rebase V5: Addressed review comments from Ville: -used a pointer to store last valid mode. -avoided, modifying of picture_aspect_ratio in kernel mode, instead only flags bits of user mode are reset (if aspect-ratio is not supported). V6: As suggested by Ville, corrected the mode pruning logic and elaborated the mode pruning logic and the assumptions taken. V7: rebase V8: rebase V9: rebase V10: rebase V11: Fixed the issue caused in kms_3d test, and enhanced the pruning logic to correctly identify and prune modes with aspect-ratio, if aspect-ratio cap is not set. V12: As suggested by Ville, added another list_head in drm_mode_display to traverse the list of exposed modes and avoided duplication of modes. V13: Minor modifications, as suggested by Ville.
drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++------- include/drm/drm_modes.h | 13 ++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index dfc8ca1..8ca1149 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1531,15 +1531,36 @@ 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,
const struct drm_file *file_priv)+static bool +drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
const struct list_head *export_list,const struct drm_file *file_priv)*/
- If user-space hasn't configured the driver to expose the stereo 3D
- modes, don't expose them.
- 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. However if such a mode* is unique, let it be exposed, but reset the aspect-ratio flags* while preparing the list of user-modes.*/- if (!file_priv->aspect_ratio_allowed &&
mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {struct drm_display_mode *mode_itr;list_for_each_entry(mode_itr, export_list, export_head)By walking the list of only the modes already added to the export list we rely on ASPECT_NONE being first if present. That seems to be a bit fragile. If we instead walk over all present modes (i.e. connector->modes) then I think that's avoided.
I don't think that would work. If we just walk over all the modes then wouldn't we always find a duplicate when the same mode with two different aspect ratios is on the list? And then we'd export neither? Or maybe I misunderstood what you mean here.
To me it seems like the correct option is to check against the export_list unconditionally, no matter whether the current mode has the aspect ratio set or not. If an identical mode is already on the list we don't export again, if it's not there then we export.
Agreed. The current code does have a problem rightly pointed by Daniel Vetter and the main reason for that is - we are checking for duplicates only if the mode has some aspect-ratio. As you have suggested, If we can do away with the condition to check for duplicates for only "modes having aspect ratio", it will solve the problem. In that case, if the aspect-ratio cap is not set, we will be removing the duplicates for all modes, irrespective of having aspect-ratio information or not, but if aspect-ratio is allowed, we would do no such checking.
Or do you suggest that we remove duplicates in any case, whether aspect-ratio cap is set by the user or not?
If there are duplicates on the connector mode list we've done something wrong when populating the list. So no, we should not have to check for duplicates in that case.
With that changed:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
if (drm_mode_match(mode_itr, mode,DRM_MODE_MATCH_TIMINGS |DRM_MODE_MATCH_CLOCK |DRM_MODE_MATCH_FLAGS |DRM_MODE_MATCH_3D_FLAGS))return false;}
return true; }
@@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, struct drm_mode_modeinfo u_mode; struct drm_mode_modeinfo __user *mode_ptr; uint32_t __user *encoder_ptr;
LIST_HEAD(export_list);
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;
@@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
/* delayed so we get modes regardless of pre-fill_modes state */ list_for_each_entry(mode, &connector->modes, head)
if (drm_mode_expose_to_userspace(mode, file_priv))
if (drm_mode_expose_to_userspace(mode, &export_list,file_priv)) {list_add_tail(&mode->export_head, &export_list); mode_count++;}/*
- This ioctl is called twice, once to determine how much space is
- needed, and the 2nd time to fill it.
* The modes that need to be exposed to the user are maintained in the* 'export_list'. When the ioctl is called first time to determine the,* space, the export_list gets filled, to find the no.of modes. In the* 2nd time, the user modes are filled, one by one from the export_list.*/ if ((out_resp->count_modes >= mode_count) && mode_count) { copied = 0; mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
list_for_each_entry(mode, &connector->modes, head) {if (!drm_mode_expose_to_userspace(mode, file_priv))continue;
list_for_each_entry(mode, &export_list, export_head) { drm_mode_convert_to_umode(&u_mode, mode);/** Reset aspect ratio flags of user-mode, if modes with* aspect-ratio are not supported.*/drm_mode_filter_aspect_ratio_flags(file_priv, &u_mode); if (copy_to_user(mode_ptr + copied, &u_mode, sizeof(u_mode))) { ret = -EFAULT;diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index 2f78b7e..b159fe0 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -411,6 +411,19 @@ struct drm_display_mode { * Field for setting the HDMI picture aspect ratio of a mode. */ enum hdmi_picture_aspect picture_aspect_ratio;
/**
* @export_head:** struct list_head for modes to be exposed to the userspace.* This is to maintain a list of exposed modes while preparing* user-mode's list in drm_mode_getconnector ioctl. The purpose of this* list_head only lies in the ioctl function, and is not expected to be* used outside the function.* Once used, the stale pointers are not reset, but left as it is, to* avoid overhead of protecting it by mode_config.mutex.*/struct list_head export_head; };
/**
-- 2.7.4
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 5/7/2018 5:58 PM, Ville Syrjälä wrote:
On Mon, May 07, 2018 at 10:34:53AM +0530, Nautiyal, Ankit K wrote:
On 5/5/2018 1:49 AM, Ville Syrjälä wrote:
On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:
On Wed, May 02, 2018 at 12:20:20PM +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, regardless of whether 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. However if such a mode is unique in the list, it is kept in the list, with aspect-ratio flags reset. -prepares a list of exposed modes, which is used to find unique modes if aspect-ratio is not allowed. -adds a new list_head 'exposed_head' in drm_mode_display, to traverse the list of exposed modes.
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. V4: rebase V5: Addressed review comments from Ville: -used a pointer to store last valid mode. -avoided, modifying of picture_aspect_ratio in kernel mode, instead only flags bits of user mode are reset (if aspect-ratio is not supported). V6: As suggested by Ville, corrected the mode pruning logic and elaborated the mode pruning logic and the assumptions taken. V7: rebase V8: rebase V9: rebase V10: rebase V11: Fixed the issue caused in kms_3d test, and enhanced the pruning logic to correctly identify and prune modes with aspect-ratio, if aspect-ratio cap is not set. V12: As suggested by Ville, added another list_head in drm_mode_display to traverse the list of exposed modes and avoided duplication of modes. V13: Minor modifications, as suggested by Ville.
drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++------- include/drm/drm_modes.h | 13 ++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index dfc8ca1..8ca1149 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1531,15 +1531,36 @@ 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,
const struct drm_file *file_priv)+static bool +drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
const struct list_head *export_list,const struct drm_file *file_priv)*/
- If user-space hasn't configured the driver to expose the stereo 3D
- modes, don't expose them.
- 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. However if such a mode* is unique, let it be exposed, but reset the aspect-ratio flags* while preparing the list of user-modes.*/- if (!file_priv->aspect_ratio_allowed &&
mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {struct drm_display_mode *mode_itr;list_for_each_entry(mode_itr, export_list, export_head)By walking the list of only the modes already added to the export list we rely on ASPECT_NONE being first if present. That seems to be a bit fragile. If we instead walk over all present modes (i.e. connector->modes) then I think that's avoided.
I don't think that would work. If we just walk over all the modes then wouldn't we always find a duplicate when the same mode with two different aspect ratios is on the list? And then we'd export neither? Or maybe I misunderstood what you mean here.
To me it seems like the correct option is to check against the export_list unconditionally, no matter whether the current mode has the aspect ratio set or not. If an identical mode is already on the list we don't export again, if it's not there then we export.
Agreed. The current code does have a problem rightly pointed by Daniel Vetter and the main reason for that is - we are checking for duplicates only if the mode has some aspect-ratio. As you have suggested, If we can do away with the condition to check for duplicates for only "modes having aspect ratio", it will solve the problem. In that case, if the aspect-ratio cap is not set, we will be removing the duplicates for all modes, irrespective of having aspect-ratio information or not, but if aspect-ratio is allowed, we would do no such checking.
Or do you suggest that we remove duplicates in any case, whether aspect-ratio cap is set by the user or not?
If there are duplicates on the connector mode list we've done something wrong when populating the list. So no, we should not have to check for duplicates in that case.
Alright. So I'll just drop the condition to check the mode's aspect-ratio and so the export_list will be checked unconditionally, if aspect-ratio cap is not set. However if the aspect-ratio cap is set, we just move the mode to export-list without checking for its duplicates.
I will send the next-patch set with this change, along with the removal of helper functions as discussed earlier.
With that changed:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
if (drm_mode_match(mode_itr, mode,DRM_MODE_MATCH_TIMINGS |DRM_MODE_MATCH_CLOCK |DRM_MODE_MATCH_FLAGS |DRM_MODE_MATCH_3D_FLAGS))return false;}
return true; }
@@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, struct drm_mode_modeinfo u_mode; struct drm_mode_modeinfo __user *mode_ptr; uint32_t __user *encoder_ptr;
LIST_HEAD(export_list);
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;
@@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
/* delayed so we get modes regardless of pre-fill_modes state */ list_for_each_entry(mode, &connector->modes, head)
if (drm_mode_expose_to_userspace(mode, file_priv))
if (drm_mode_expose_to_userspace(mode, &export_list,file_priv)) {list_add_tail(&mode->export_head, &export_list); mode_count++;}/*
- This ioctl is called twice, once to determine how much space is
- needed, and the 2nd time to fill it.
* The modes that need to be exposed to the user are maintained in the* 'export_list'. When the ioctl is called first time to determine the,* space, the export_list gets filled, to find the no.of modes. In the* 2nd time, the user modes are filled, one by one from the export_list.*/ if ((out_resp->count_modes >= mode_count) && mode_count) { copied = 0; mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
list_for_each_entry(mode, &connector->modes, head) {if (!drm_mode_expose_to_userspace(mode, file_priv))continue;
list_for_each_entry(mode, &export_list, export_head) { drm_mode_convert_to_umode(&u_mode, mode);/** Reset aspect ratio flags of user-mode, if modes with* aspect-ratio are not supported.*/drm_mode_filter_aspect_ratio_flags(file_priv, &u_mode); if (copy_to_user(mode_ptr + copied, &u_mode, sizeof(u_mode))) { ret = -EFAULT;diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index 2f78b7e..b159fe0 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -411,6 +411,19 @@ struct drm_display_mode { * Field for setting the HDMI picture aspect ratio of a mode. */ enum hdmi_picture_aspect picture_aspect_ratio;
/**
* @export_head:** struct list_head for modes to be exposed to the userspace.* This is to maintain a list of exposed modes while preparing* user-mode's list in drm_mode_getconnector ioctl. The purpose of this* list_head only lies in the ioctl function, and is not expected to be* used outside the function.* Once used, the stale pointers are not reset, but left as it is, to* avoid overhead of protecting it by mode_config.mutex.*/struct list_head export_head; };
/**
-- 2.7.4
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, May 04, 2018 at 11:19:45PM +0300, Ville Syrjälä wrote:
On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:
On Wed, May 02, 2018 at 12:20:20PM +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, regardless of whether 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. However if such a mode is unique in the list, it is kept in the list, with aspect-ratio flags reset. -prepares a list of exposed modes, which is used to find unique modes if aspect-ratio is not allowed. -adds a new list_head 'exposed_head' in drm_mode_display, to traverse the list of exposed modes.
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. V4: rebase V5: Addressed review comments from Ville: -used a pointer to store last valid mode. -avoided, modifying of picture_aspect_ratio in kernel mode, instead only flags bits of user mode are reset (if aspect-ratio is not supported). V6: As suggested by Ville, corrected the mode pruning logic and elaborated the mode pruning logic and the assumptions taken. V7: rebase V8: rebase V9: rebase V10: rebase V11: Fixed the issue caused in kms_3d test, and enhanced the pruning logic to correctly identify and prune modes with aspect-ratio, if aspect-ratio cap is not set. V12: As suggested by Ville, added another list_head in drm_mode_display to traverse the list of exposed modes and avoided duplication of modes. V13: Minor modifications, as suggested by Ville.
drivers/gpu/drm/drm_connector.c | 45 ++++++++++++++++++++++++++++++++++------- include/drm/drm_modes.h | 13 ++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index dfc8ca1..8ca1149 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1531,15 +1531,36 @@ 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,
const struct drm_file *file_priv)+static bool +drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
const struct list_head *export_list,const struct drm_file *file_priv){ /* * If user-space hasn't configured the driver to expose the stereo 3D * modes, don't expose them. */
- 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. However if such a mode* is unique, let it be exposed, but reset the aspect-ratio flags* while preparing the list of user-modes.*/- if (!file_priv->aspect_ratio_allowed &&
mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {struct drm_display_mode *mode_itr;list_for_each_entry(mode_itr, export_list, export_head)By walking the list of only the modes already added to the export list we rely on ASPECT_NONE being first if present. That seems to be a bit fragile. If we instead walk over all present modes (i.e. connector->modes) then I think that's avoided.
I don't think that would work. If we just walk over all the modes then wouldn't we always find a duplicate when the same mode with two different aspect ratios is on the list? And then we'd export neither? Or maybe I misunderstood what you mean here.
We'd export both. Let's assume we have 2 modes, only difference is aspect ratio.
1. We check the first mode, it has aspect_ratio != NONE. But because there's no other exported mode yet on export_list, we export it (and filter out the aspect_ratio by setting it to NONE).
2. 2nd mode has aspect_ratio == NONE, so we export it.
Results in duped modes exported. If you always guarantee that aspect_ratio == NONE is before the same mode with aspect ratio, then this algo here works. But not for the more general case I think.
To me it seems like the correct option is to check against the export_list unconditionally, no matter whether the current mode has the aspect ratio set or not. If an identical mode is already on the list we don't export again, if it's not there then we export.
That might be another option to fix the bug. -Daniel
With that changed:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
if (drm_mode_match(mode_itr, mode,DRM_MODE_MATCH_TIMINGS |DRM_MODE_MATCH_CLOCK |DRM_MODE_MATCH_FLAGS |DRM_MODE_MATCH_3D_FLAGS))return false;}
return true;
} @@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, struct drm_mode_modeinfo u_mode; struct drm_mode_modeinfo __user *mode_ptr; uint32_t __user *encoder_ptr;
LIST_HEAD(export_list);
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;
@@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
/* delayed so we get modes regardless of pre-fill_modes state */ list_for_each_entry(mode, &connector->modes, head)
if (drm_mode_expose_to_userspace(mode, file_priv))
if (drm_mode_expose_to_userspace(mode, &export_list,file_priv)) {list_add_tail(&mode->export_head, &export_list); mode_count++;}/*
- This ioctl is called twice, once to determine how much space is
- needed, and the 2nd time to fill it.
* The modes that need to be exposed to the user are maintained in the* 'export_list'. When the ioctl is called first time to determine the,* space, the export_list gets filled, to find the no.of modes. In the* 2nd time, the user modes are filled, one by one from the export_list.*/ if ((out_resp->count_modes >= mode_count) && mode_count) { copied = 0; mode_ptr = (struct drm_mode_modeinfo __user *)(unsigned long)out_resp->modes_ptr;
list_for_each_entry(mode, &connector->modes, head) {if (!drm_mode_expose_to_userspace(mode, file_priv))continue;
list_for_each_entry(mode, &export_list, export_head) { drm_mode_convert_to_umode(&u_mode, mode);/** Reset aspect ratio flags of user-mode, if modes with* aspect-ratio are not supported.*/drm_mode_filter_aspect_ratio_flags(file_priv, &u_mode); if (copy_to_user(mode_ptr + copied, &u_mode, sizeof(u_mode))) { ret = -EFAULT;diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index 2f78b7e..b159fe0 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -411,6 +411,19 @@ struct drm_display_mode { * Field for setting the HDMI picture aspect ratio of a mode. */ enum hdmi_picture_aspect picture_aspect_ratio;
- /**
* @export_head:** struct list_head for modes to be exposed to the userspace.* This is to maintain a list of exposed modes while preparing* user-mode's list in drm_mode_getconnector ioctl. The purpose of this* list_head only lies in the ioctl function, and is not expected to be* used outside the function.* Once used, the stale pointers are not reset, but left as it is, to* avoid overhead of protecting it by mode_config.mutex.*/- struct list_head export_head;
};
/**
2.7.4
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Ville Syrjälä Intel
From: "Sharma, Shashank" shashank.sharma@intel.com
Current DRM layer functions don't parse aspect ratio information while converting a user mode->kernel mode or vice versa. This causes modeset to pick mode with wrong aspect ratio, eventually causing failures in HDMI compliance test cases, due to wrong VIC.
This patch adds aspect ratio information in DRM's mode conversion and mode comparision functions, to make sure kernel picks mode with right aspect ratio (as per the VIC).
Background: This patch was once reviewed and merged, and later reverted due to lack of DRM cap protection. This is a re-spin of this patch, this time with DRM cap protection, to avoid aspect ratio information, when the client doesn't request for it.
Review link: https://pw-emeril.freedesktop.org/patch/104068/ Background discussion: https://patchwork.kernel.org/patch/9379057/
Signed-off-by: Shashank Sharma shashank.sharma@intel.com Signed-off-by: Lin, Jia lin.a.jia@intel.com Signed-off-by: Akashdeep Sharma akashdeep.sharma@intel.com Reviewed-by: Jim Bride jim.bride@linux.intel.com (V2) Reviewed-by: Jose Abreu Jose.Abreu@synopsys.com (V4)
Cc: Ville Syrjala ville.syrjala@linux.intel.com Cc: Jim Bride jim.bride@linux.intel.com Cc: Jose Abreu Jose.Abreu@synopsys.com Cc: Ankit Nautiyal ankit.k.nautiyal@intel.com
V3: modified the aspect-ratio check in drm_mode_equal as per new flags provided by Ville. https://patchwork.freedesktop.org/patch/188043/ V4: rebase V5: rebase V6: As recommended by Ville, avoided matching of aspect-ratio in drm_fb_helper, while trying to find a common mode among connectors for the target clone mode. V7: rebase V8: rebase V9: rebase V10: rebase V11: rebase V12: rebase V13: rebase --- drivers/gpu/drm/drm_fb_helper.c | 12 ++++++++++-- drivers/gpu/drm/drm_modes.c | 35 ++++++++++++++++++++++++++++++++++- 2 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 0646b10..2ee1eaa 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -2183,7 +2183,11 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper, for (j = 0; j < i; j++) { if (!enabled[j]) continue; - if (!drm_mode_equal(modes[j], modes[i])) + if (!drm_mode_match(modes[j], modes[i], + DRM_MODE_MATCH_TIMINGS | + DRM_MODE_MATCH_CLOCK | + DRM_MODE_MATCH_FLAGS | + DRM_MODE_MATCH_3D_FLAGS)) can_clone = false; } } @@ -2203,7 +2207,11 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper,
fb_helper_conn = fb_helper->connector_info[i]; list_for_each_entry(mode, &fb_helper_conn->connector->modes, head) { - if (drm_mode_equal(mode, dmt_mode)) + if (drm_mode_match(mode, dmt_mode, + DRM_MODE_MATCH_TIMINGS | + DRM_MODE_MATCH_CLOCK | + DRM_MODE_MATCH_FLAGS | + DRM_MODE_MATCH_3D_FLAGS)) modes[i] = mode; } if (!modes[i]) diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 2bf2f0b..c91aec5 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1049,7 +1049,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);
@@ -1647,6 +1648,20 @@ void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out, out->vrefresh = in->vrefresh; out->flags = in->flags; out->type = in->type; + + switch (in->picture_aspect_ratio) { + case HDMI_PICTURE_ASPECT_4_3: + out->flags |= DRM_MODE_FLAG_PIC_AR_4_3; + break; + case HDMI_PICTURE_ASPECT_16_9: + out->flags |= DRM_MODE_FLAG_PIC_AR_16_9; + break; + case HDMI_PICTURE_ASPECT_RESERVED: + default: + out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; + break; + } + strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); out->name[DRM_DISPLAY_MODE_LEN-1] = 0; } @@ -1693,6 +1708,24 @@ int drm_mode_convert_umode(struct drm_device *dev, strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
+ /* Clearing picture aspect ratio bits from out flags, + * as the aspect-ratio information is not stored in + * flags for kernel-mode, but in picture_aspect_ratio. + */ + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; + + switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) { + case DRM_MODE_FLAG_PIC_AR_4_3: + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3; + break; + case DRM_MODE_FLAG_PIC_AR_16_9: + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9; + break; + default: + out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; + break; + } + out->status = drm_mode_validate_driver(dev, out); if (out->status != MODE_OK) return -EINVAL;
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 V4: rebase V5: corrected the macro name for an aspect ratio, in a switch case. V6: rebase V7: rebase V8: rebase V9: rebase V10: rebase V11: rebase V12: rebase V13: 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 c91aec5..9d70bc0 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1656,6 +1656,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 HDMI_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; @@ -1721,6 +1727,12 @@ int drm_mode_convert_umode(struct drm_device *dev, 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 50bcf42..4b3a1bb 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -93,6 +93,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) @@ -102,6 +104,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)
#define DRM_MODE_FLAG_ALL (DRM_MODE_FLAG_PHSYNC | \ DRM_MODE_FLAG_NHSYNC | \
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Nautiyal, Ankit K -
Ville Syrjälä