Re: [PATCH v3 5/8] drm: Handle aspect ratio info in atomic and legacy modeset paths

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote:
> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> 
> If the user mode does not support aspect-ratio, and requests for
> a modeset, then the flag bits representing aspect ratio in the
> given user-mode must be rejected.
> Similarly, while preparing a user-mode from kernel mode, the
> aspect-ratio info must not be given, if aspect-ratio is not
> supported by the user.
> 
> This patch:
> 1. adds a new bit field aspect_ratio_allowed in drm_atomic_state,
> which is set only if the aspect-ratio is supported by the user.
> 2. discards the aspect-ratio info from the user mode while
> preparing kernel mode structure, during modeset, if the
> user does not support aspect ratio.
> 3. avoids setting the aspect-ratio info in user-mode, while
> converting user-mode from the kernel-mode.
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> 
> V3: Addressed review comments from Ville:
> -Do not corrupt the current crtc state by updating aspect ratio
> on the fly.
> ---
>  drivers/gpu/drm/drm_atomic.c | 61 +++++++++++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/drm_crtc.c   | 19 ++++++++++++++
>  include/drm/drm_atomic.h     |  2 ++
>  3 files changed, 79 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 69ff763..39313e2 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -316,6 +316,35 @@ static s32 __user *get_out_fence_for_crtc(struct drm_atomic_state *state,
>  
>  	return fence_ptr;
>  }
> +/**
> + * drm_atomic_allow_aspect_ratio_for_kmode
> + * @state: the crtc state
> + * @mode: kernel-internal mode, which is used to create a duplicate mode,
> + * with updated picture aspect ratio.
> + *
> + * Allow the aspect ratio info in the kernel mode only if aspect ratio is
> + * supported.
> + *
> + * RETURNS:
> + * kernel-internal mode with updated picture aspect ratio value.
> + */
> +
> +struct drm_display_mode*
> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state *state,
> +					const struct drm_display_mode *mode)
> +{
> +	struct drm_atomic_state *atomic_state = state->state;
> +	struct drm_display_mode *ar_kmode;
> +
> +	ar_kmode = drm_mode_duplicate(state->crtc->dev, mode);
> +	/*
> +	 * If aspect ratio is not supported, set the picture aspect ratio as
> +	 * NONE.
> +	 */
> +	if (atomic_state && !atomic_state->allow_aspect_ratio)
> +		ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> +	return ar_kmode;
> +}
>  
>  /**
>   * drm_atomic_set_mode_for_crtc - set mode for CRTC
> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>  	state->mode_blob = NULL;
>  
>  	if (mode) {
> -		drm_mode_convert_to_umode(&umode, mode);
> +		struct drm_display_mode *ar_mode;
> +
> +		ar_mode = drm_atomic_allow_aspect_ratio_for_kmode(state, mode);
> +		drm_mode_convert_to_umode(&umode, ar_mode);

This still looks sketchy.

I believe there are just two places where we need to filter out the
aspect ratio flags from the umode, namely the getblob and getcrtc
ioctls.

And for checking the user input I think we should probably just
stick that into drm_mode_convert_umode(). Looks like we never call
that from anywhere but the atomic/setprop and setcrtc paths with
a non-NULL argument.

I *think* those three places should be sufficient to cover everything.

>  		state->mode_blob =
>  			drm_property_create_blob(state->crtc->dev,
>  		                                 sizeof(umode),
> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>  		if (IS_ERR(state->mode_blob))
>  			return PTR_ERR(state->mode_blob);
>  
> -		drm_mode_copy(&state->mode, mode);
> +		drm_mode_copy(&state->mode, ar_mode);
>  		state->enable = true;
>  		DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
> -				 mode->name, state);
> +				 ar_mode->name, state);
> +		drm_mode_destroy(state->crtc->dev, ar_mode);
>  	} else {
>  		memset(&state->mode, 0, sizeof(state->mode));
>  		state->enable = false;
> @@ -436,6 +469,25 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
>  }
>  
>  /**
> + * drm_atomic_allow_aspect_ratio_for_umode
> + * @state: the crtc state
> + * @umode: userspace mode, whose aspect ratio flag bits are to be updated.
> + *
> + * Allow the aspect ratio info in the userspace mode only if aspect ratio is
> + * supported.
> + */
> +void
> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state *state,
> +					struct drm_mode_modeinfo *umode)
> +{
> +	struct drm_atomic_state *atomic_state = state->state;
> +
> +	/* Reset the aspect ratio flags if aspect ratio is not supported */
> +	if (atomic_state && !atomic_state->allow_aspect_ratio)
> +		umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
> +}
> +
> +/**
>   * drm_atomic_crtc_set_property - set property on CRTC
>   * @crtc: the drm CRTC to set a property on
>   * @state: the state object to update with the new property value
> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  	else if (property == config->prop_mode_id) {
>  		struct drm_property_blob *mode =
>  			drm_property_lookup_blob(dev, val);
> +		drm_atomic_allow_aspect_ratio_for_umode(state,
> +					(struct drm_mode_modeinfo *)
> +					mode->data);
>  		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>  		drm_property_blob_put(mode);
>  		return ret;
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index f0556e6..a2d34fa 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  	drm_modeset_lock(&crtc->mutex, NULL);
>  	if (crtc->state) {
>  		if (crtc->state->enable) {
> +			/*
> +			 * If the aspect-ratio is not required by the,
> +			 * userspace, do not set the aspect-ratio flags.
> +			 */
> +			if (!file_priv->aspect_ratio_allowed)
> +				crtc->state->mode.picture_aspect_ratio =
> +					HDMI_PICTURE_ASPECT_NONE;

These would still clobber the current crtc state. So definitely don't
want to do this.

>  			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
>  			crtc_resp->mode_valid = 1;
>  
> @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
>  		crtc_resp->x = crtc->x;
>  		crtc_resp->y = crtc->y;
>  		if (crtc->enabled) {
> +			/*
> +			 * If the aspect-ratio is not required by the,
> +			 * userspace, do not set the aspect-ratio flags.
> +			 */
> +			if (!file_priv->aspect_ratio_allowed)
> +				crtc->mode.picture_aspect_ratio =
> +					HDMI_PICTURE_ASPECT_NONE;
>  			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
>  			crtc_resp->mode_valid = 1;
>  
> @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  			goto out;
>  		}
>  
> +		/* If the user does not ask for aspect ratio, reset the aspect
> +		 * ratio bits in the usermode.
> +		 */
> +		if (!file_priv->aspect_ratio_allowed)
> +			crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
>  		ret = drm_mode_convert_umode(mode, &crtc_req->mode);
>  		if (ret) {
>  			DRM_DEBUG_KMS("Invalid mode\n");
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 1c27526..130dad9 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -237,6 +237,7 @@ struct __drm_private_objs_state {
>   * @ref: count of all references to this state (will not be freed until zero)
>   * @dev: parent DRM device
>   * @allow_modeset: allow full modeset
> + * @allow_aspect_ratio: allow the aspect ratio info to be passes to user
>   * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
>   * @async_update: hint for asynchronous plane update
>   * @planes: pointer to array of structures with per-plane data
> @@ -256,6 +257,7 @@ struct drm_atomic_state {
>  
>  	struct drm_device *dev;
>  	bool allow_modeset : 1;
> +	bool allow_aspect_ratio : 1;
>  	bool legacy_cursor_update : 1;
>  	bool async_update : 1;
>  	struct __drm_planes_state *planes;
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel