<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> Hi Ville, I agree to all the comments here, and will correct the required things in the next patch-set. <div class="moz-cite-prefix">On 2/23/2018 7:58 PM, Ville Syrjälä wrote: </div> <blockquote cite="mid:20180223142845.GL5453@intel.com" type="cite"> <pre wrap="">On Thu, Feb 15, 2018 at 05:50:59PM +0530, Nautiyal, Ankit K wrote: </pre> <blockquote type="cite"> <pre wrap="">From: Ankit Nautiyal <a class="moz-txt-link-rfc2396E" href="mailto:ankit.k.nautiyal@intel.com"><ankit.k.nautiyal@intel.com></a> 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. passes the file_priv structure from the drm_mode_atomic_ioctl till the drm_mode_crtc_set_mode_prop, to get the user capability. 2. rejects the modes with aspect-ratio info, during modeset, if the user does not support aspect ratio. 3. does not load the aspect-ratio info in user-mode structure, if aspect ratio is not supported. Signed-off-by: Ankit Nautiyal <a class="moz-txt-link-rfc2396E" href="mailto:ankit.k.nautiyal@intel.com"><ankit.k.nautiyal@intel.com></a> 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. --- drivers/gpu/drm/drm_atomic.c | 30 ++++++++++++++++++++++-------- drivers/gpu/drm/drm_atomic_helper.c | 6 +++--- drivers/gpu/drm/drm_crtc.c | 15 +++++++++++++++ drivers/gpu/drm/drm_crtc_internal.h | 3 ++- drivers/gpu/drm/drm_mode_object.c | 9 ++++++--- drivers/gpu/drm/drm_modes.c | 1 + include/drm/drm_atomic.h | 5 +++-- 7 files changed, 52 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 86b483e..7e78305 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -368,6 +368,7 @@ EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc); * drm_atomic_set_mode_prop_for_crtc - set mode for CRTC * @state: the CRTC whose incoming state to update * @blob: pointer to blob property to use for mode + * @file_priv: file priv structure, to get the userspace capabilities * * Set a mode (originating from a blob property) on the desired CRTC state. * This function will take a reference on the blob property for the CRTC state, @@ -378,7 +379,8 @@ EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc); * Zero on success, error code on failure. Cannot return -EDEADLK. */ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state, - struct drm_property_blob *blob) + struct drm_property_blob *blob, + struct drm_file *file_priv) { if (blob == state->mode_blob) return 0; @@ -389,10 +391,20 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state, memset(&state->mode, 0, sizeof(state->mode)); if (blob) { + struct drm_mode_modeinfo *u_mode; + + u_mode = (struct drm_mode_modeinfo *) blob->data; + if (!file_priv->aspect_ratio_allowed && + (u_mode->flags & DRM_MODE_FLAG_PIC_AR_MASK) != + DRM_MODE_FLAG_PIC_AR_NONE) { + DRM_DEBUG_ATOMIC("Unexpected aspect-ratio flag bits\n"); + return -EINVAL; + } </pre> </blockquote> <pre wrap=""> We should probably pull this logic into a small helper so that we don't have to duplicate it in both the setprop and setcrtc code paths.</pre> </blockquote> Agreed. I would add the required helper function, in the next patch-set. <blockquote cite="mid:20180223142845.GL5453@intel.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap="">+ if (blob->length != sizeof(struct drm_mode_modeinfo) || </pre> </blockquote> <pre wrap=""> The blob length check has to be done before you access the data. </pre> </blockquote> Right. Will take care in the next patch-set. <blockquote cite="mid:20180223142845.GL5453@intel.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap=""> drm_mode_convert_umode(state->crtc->dev, &state->mode, - (const struct drm_mode_modeinfo *) - blob->data)) + (const struct drm_mode_modeinfo *) + u_mode)) return -EINVAL; state->mode_blob = drm_property_blob_get(blob); @@ -441,6 +453,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, * @state: the state object to update with the new property value * @property: the property to set * @val: the new property value + * @file_priv: the file private structure, to get the user capabilities * * This function handles generic/core properties and calls out to driver's * &drm_crtc_funcs.atomic_set_property for driver properties. To ensure @@ -452,7 +465,7 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev, */ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, struct drm_crtc_state *state, struct drm_property *property, - uint64_t val) + uint64_t val, struct drm_file *file_priv) { struct drm_device *dev = crtc->dev; struct drm_mode_config *config = &dev->mode_config; @@ -465,7 +478,7 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, struct drm_property_blob *mode = drm_property_lookup_blob(dev, val); mode->is_video_mode = true; - ret = drm_atomic_set_mode_prop_for_crtc(state, mode); + ret = drm_atomic_set_mode_prop_for_crtc(state, mode, file_priv); drm_property_blob_put(mode); return ret; } else if (property == config->degamma_lut_property) { @@ -1918,7 +1931,8 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state, int drm_atomic_set_property(struct drm_atomic_state *state, struct drm_mode_object *obj, struct drm_property *prop, - uint64_t prop_value) + uint64_t prop_value, + struct drm_file *file_priv) { struct drm_mode_object *ref; int ret; @@ -1952,7 +1966,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state, } ret = drm_atomic_crtc_set_property(crtc, - crtc_state, prop, prop_value); + crtc_state, prop, prop_value, file_priv); break; } case DRM_MODE_OBJECT_PLANE: { @@ -2341,7 +2355,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev, } ret = drm_atomic_set_property(state, obj, prop, - prop_value); + prop_value, file_priv); if (ret) { drm_mode_object_put(obj); goto out; diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ae3cbfe..781ebd6 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -186,7 +186,7 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state, if (!crtc_state->connector_mask) { ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, - NULL); + NULL, NULL); if (ret < 0) goto out; @@ -2749,7 +2749,7 @@ static int update_output_state(struct drm_atomic_state *state, if (!new_crtc_state->connector_mask) { ret = drm_atomic_set_mode_prop_for_crtc(new_crtc_state, - NULL); + NULL, NULL); if (ret < 0) return ret; @@ -2930,7 +2930,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, crtc_state->active = false; - ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL); + ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL, NULL); if (ret < 0) goto free; diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 353e24f..e36a971 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -430,6 +430,9 @@ int drm_mode_getcrtc(struct drm_device *dev, if (crtc->state) { if (crtc->state->enable) { drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); + if (!file_priv->aspect_ratio_allowed) + crtc->state->mode.flags &= + (~DRM_MODE_FLAG_PIC_AR_MASK); </pre> </blockquote> <pre wrap=""> Should be manling crtc_resp->mode instead of the internal mode. </pre> </blockquote> Agreed. Will correct this in the next patch-set. <blockquote cite="mid:20180223142845.GL5453@intel.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap=""> crtc_resp->mode_valid = 1; } else { @@ -440,6 +443,9 @@ int drm_mode_getcrtc(struct drm_device *dev, crtc_resp->y = crtc->y; if (crtc->enabled) { drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode); + if (!file_priv->aspect_ratio_allowed) + crtc->mode.flags &= + (~DRM_MODE_FLAG_PIC_AR_MASK); </pre> </blockquote> <pre wrap=""> ditto Should probably pull this flag filtering logic into a small helper as well.</pre> </blockquote> Agreed. Will correct this in the next patch-set. Regards, Ankit <blockquote cite="mid:20180223142845.GL5453@intel.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap=""> crtc_resp->mode_valid = 1; } else { @@ -614,6 +620,15 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, goto out; } + if (!file_priv->aspect_ratio_allowed && + (crtc_req->mode.flags & DRM_MODE_FLAG_PIC_AR_MASK) != + DRM_MODE_FLAG_PIC_AR_NONE) { + 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) { DRM_DEBUG_KMS("Invalid mode\n"); diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index af00f42..7c3338f 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -186,7 +186,8 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state, int drm_atomic_set_property(struct drm_atomic_state *state, struct drm_mode_object *obj, struct drm_property *prop, - uint64_t prop_value); + uint64_t prop_value, + struct drm_file *file_priv); int drm_atomic_get_property(struct drm_mode_object *obj, struct drm_property *property, uint64_t *val); int drm_mode_atomic_ioctl(struct drm_device *dev, diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c index ce4d2fb..1c3d498 100644 --- a/drivers/gpu/drm/drm_mode_object.c +++ b/drivers/gpu/drm/drm_mode_object.c @@ -452,7 +452,8 @@ static int set_property_legacy(struct drm_mode_object *obj, static int set_property_atomic(struct drm_mode_object *obj, struct drm_property *prop, - uint64_t prop_value) + uint64_t prop_value, + struct drm_file *file_priv) { struct drm_device *dev = prop->dev; struct drm_atomic_state *state; @@ -476,7 +477,8 @@ static int set_property_atomic(struct drm_mode_object *obj, obj_to_connector(obj), prop_value); } else { - ret = drm_atomic_set_property(state, obj, prop, prop_value); + ret = drm_atomic_set_property(state, obj, prop, prop_value, + file_priv); if (ret) goto out; ret = drm_atomic_commit(state); @@ -519,7 +521,8 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data, goto out_unref; if (drm_drv_uses_atomic_modeset(property->dev)) - ret = set_property_atomic(arg_obj, property, arg->value); + ret = set_property_atomic(arg_obj, property, arg->value, + file_priv); else ret = set_property_legacy(arg_obj, property, arg->value); diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 6ca1f3b..ca4c5cc 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1620,6 +1620,7 @@ EXPORT_SYMBOL(drm_mode_create_from_cmdline_mode); * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo * @out: drm_mode_modeinfo struct to return to the user * @in: drm_display_mode to use + * @file_priv: file_priv structure, to get the user capabilities * * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to * the user. diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index cf13842..d3ad5d1 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -367,7 +367,7 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state, struct drm_crtc *crtc); int drm_atomic_crtc_set_property(struct drm_crtc *crtc, struct drm_crtc_state *state, struct drm_property *property, - uint64_t val); + uint64_t val, struct drm_file *file_priv); struct drm_plane_state * __must_check drm_atomic_get_plane_state(struct drm_atomic_state *state, struct drm_plane *plane); @@ -583,7 +583,8 @@ drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, const struct drm_display_mode *mode); int __must_check drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state, - struct drm_property_blob *blob); + struct drm_property_blob *blob, + struct drm_file *file_priv); int __must_check drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state, struct drm_crtc *crtc); -- 2.7.4 </pre> </blockquote> <pre wrap=""> </pre> </blockquote> </body> </html>