[PATCH v5 6/9] drm: Handle aspect ratio info in legacy and atomic modeset paths
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Mon Feb 26 15:10:18 UTC 2018
Hi Ville,
I agree to all the comments here, and will correct the required things
in the next patch-set.
On 2/23/2018 7:58 PM, Ville Syrjälä wrote:
> On Thu, Feb 15, 2018 at 05:50:59PM +0530, Nautiyal, Ankit K wrote:
>> From: Ankit Nautiyal <ankit.k.nautiyal at 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. 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 <ankit.k.nautiyal at 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.
>> ---
>> 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;
>> + }
> 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.
Agreed. I would add the required helper function, in the next patch-set.
>
>> +
>> if (blob->length != sizeof(struct drm_mode_modeinfo) ||
> The blob length check has to be done before you access the data.
Right. Will take care in the next patch-set.
>> 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);
> Should be manling crtc_resp->mode instead of the internal mode.
Agreed. Will correct this in the next patch-set.
>> 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);
> ditto
>
> Should probably pull this flag filtering logic into a small helper
> as well.
Agreed. Will correct this in the next patch-set.
Regards,
Ankit
>
>> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20180226/b494c2b5/attachment-0001.html>
More information about the dri-devel
mailing list