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

Commit Message

Nautiyal, Ankit K Jan. 12, 2018, 6:21 a.m. UTC

From: Ankit Nautiyal <[e-mail]>

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 <[e-mail]>

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(-)

Comments

Ville Syrjälä Jan. 29, 2018, 6:53 p.m. UTC | #1

On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote:
> From: Ankit Nautiyal <[e-mail]>
> 
> 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 <[e-mail]>
> 
> 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
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Nautiyal, Ankit K Jan. 31, 2018, 6:34 a.m. UTC | #2

On 1/30/2018 12:23 AM, Ville Syrjälä wrote:
> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote:
>> From: Ankit Nautiyal <[e-mail]>
>>
>> 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 <[e-mail]>
>>
>> V3: Addressed review comments from Ville:
>> -Do not corrupt the current crtc state by updating aspect ratio
>> on the fly.
>> ---
>>   drivers/gpu/drm/drm_atomic.c | 61 +++++++++++++++++++++++++++++++++++++++++---
>>   drivers/gpu/drm/drm_crtc.c   | 19 ++++++++++++++
>>   include/drm/drm_atomic.h     |  2 ++
>>   3 files changed, 79 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 69ff763..39313e2 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -316,6 +316,35 @@ static s32 __user *get_out_fence_for_crtc(struct drm_atomic_state *state,
>>   
>>   	return fence_ptr;
>>   }
>> +/**
>> + * drm_atomic_allow_aspect_ratio_for_kmode
>> + * @state: the crtc state
>> + * @mode: kernel-internal mode, which is used to create a duplicate mode,
>> + * with updated picture aspect ratio.
>> + *
>> + * Allow the aspect ratio info in the kernel mode only if aspect ratio is
>> + * supported.
>> + *
>> + * RETURNS:
>> + * kernel-internal mode with updated picture aspect ratio value.
>> + */
>> +
>> +struct drm_display_mode*
>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state *state,
>> +					const struct drm_display_mode *mode)
>> +{
>> +	struct drm_atomic_state *atomic_state = state->state;
>> +	struct drm_display_mode *ar_kmode;
>> +
>> +	ar_kmode = drm_mode_duplicate(state->crtc->dev, mode);
>> +	/*
>> +	 * If aspect ratio is not supported, set the picture aspect ratio as
>> +	 * NONE.
>> +	 */
>> +	if (atomic_state && !atomic_state->allow_aspect_ratio)
>> +		ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>> +	return ar_kmode;
>> +}
>>   
>>   /**
>>    * drm_atomic_set_mode_for_crtc - set mode for CRTC
>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>>   	state->mode_blob = NULL;
>>   
>>   	if (mode) {
>> -		drm_mode_convert_to_umode(&umode, mode);
>> +		struct drm_display_mode *ar_mode;
>> +
>> +		ar_mode = drm_atomic_allow_aspect_ratio_for_kmode(state, mode);
>> +		drm_mode_convert_to_umode(&umode, ar_mode);
> This still looks sketchy.
>
> I believe there are just two places where we need to filter out the
> aspect ratio flags from the umode, namely the getblob and getcrtc
> ioctls.

AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob, 
etc. apart from the mode blob.
Is there any way to check from blob id (or by any other means),
if the data is for the mode, or edid, or gamma etc.

If we can check that, I can make sure that aspect-ratio flags are taken 
care of, if the blob is for mode,
and the aspect ratio is not supported.

>
> And for checking the user input I think we should probably just
> stick that into drm_mode_convert_umode(). Looks like we never call
> that from anywhere but the atomic/setprop and setcrtc paths with
> a non-NULL argument.
>
> I *think* those three places should be sufficient to cover everything.
Agreed. Infact I tried to do that first, but the only problem we have 
here, is that, the file_priv
which has the aspect ratio capability information is not supplied to the 
drm_mode_convert_umode()
or the functions calling, drm_mode_conver_umode(). At best we have 
drm_crtc_state.

I have added an aspect ratio allowed flag  in drm_crtc_state->state so 
that its available in the
functions calling drm_mode_convert_umode.

I am open to suggestion to avoid adding a new flag in drm_atomic_state, 
if there is a better
way to let the drm_mode_convert_umode( ) know that user supports aspect 
ratio capability.
>
>>   		state->mode_blob =
>>   			drm_property_create_blob(state->crtc->dev,
>>   		                                 sizeof(umode),
>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>>   		if (IS_ERR(state->mode_blob))
>>   			return PTR_ERR(state->mode_blob);
>>   
>> -		drm_mode_copy(&state->mode, mode);
>> +		drm_mode_copy(&state->mode, ar_mode);
>>   		state->enable = true;
>>   		DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
>> -				 mode->name, state);
>> +				 ar_mode->name, state);
>> +		drm_mode_destroy(state->crtc->dev, ar_mode);
>>   	} else {
>>   		memset(&state->mode, 0, sizeof(state->mode));
>>   		state->enable = false;
>> @@ -436,6 +469,25 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
>>   }
>>   
>>   /**
>> + * drm_atomic_allow_aspect_ratio_for_umode
>> + * @state: the crtc state
>> + * @umode: userspace mode, whose aspect ratio flag bits are to be updated.
>> + *
>> + * Allow the aspect ratio info in the userspace mode only if aspect ratio is
>> + * supported.
>> + */
>> +void
>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state *state,
>> +					struct drm_mode_modeinfo *umode)
>> +{
>> +	struct drm_atomic_state *atomic_state = state->state;
>> +
>> +	/* Reset the aspect ratio flags if aspect ratio is not supported */
>> +	if (atomic_state && !atomic_state->allow_aspect_ratio)
>> +		umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
>> +}
>> +
>> +/**
>>    * drm_atomic_crtc_set_property - set property on CRTC
>>    * @crtc: the drm CRTC to set a property on
>>    * @state: the state object to update with the new property value
>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>>   	else if (property == config->prop_mode_id) {
>>   		struct drm_property_blob *mode =
>>   			drm_property_lookup_blob(dev, val);
>> +		drm_atomic_allow_aspect_ratio_for_umode(state,
>> +					(struct drm_mode_modeinfo *)
>> +					mode->data);
>>   		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>>   		drm_property_blob_put(mode);
>>   		return ret;
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index f0556e6..a2d34fa 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
>>   	drm_modeset_lock(&crtc->mutex, NULL);
>>   	if (crtc->state) {
>>   		if (crtc->state->enable) {
>> +			/*
>> +			 * If the aspect-ratio is not required by the,
>> +			 * userspace, do not set the aspect-ratio flags.
>> +			 */
>> +			if (!file_priv->aspect_ratio_allowed)
>> +				crtc->state->mode.picture_aspect_ratio =
>> +					HDMI_PICTURE_ASPECT_NONE;
> These would still clobber the current crtc state. So definitely don't
> want to do this.
Alright, so alternative way of doing this will be:
Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel 
mode structure,
but set the aspect ratio flags in the usermode to none, after calling the
drm_mode_convert_to_umode().

Will take care of this in the next patchset.

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

Ville Syrjälä Jan. 31, 2018, 1:09 p.m. UTC | #3

On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote:
> 
> 
> On 1/30/2018 12:23 AM, Ville Syrjälä wrote:
> > On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote:
> >> From: Ankit Nautiyal <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >>
> >> V3: Addressed review comments from Ville:
> >> -Do not corrupt the current crtc state by updating aspect ratio
> >> on the fly.
> >> ---
> >>   drivers/gpu/drm/drm_atomic.c | 61 +++++++++++++++++++++++++++++++++++++++++---
> >>   drivers/gpu/drm/drm_crtc.c   | 19 ++++++++++++++
> >>   include/drm/drm_atomic.h     |  2 ++
> >>   3 files changed, 79 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index 69ff763..39313e2 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -316,6 +316,35 @@ static s32 __user *get_out_fence_for_crtc(struct drm_atomic_state *state,
> >>   
> >>   	return fence_ptr;
> >>   }
> >> +/**
> >> + * drm_atomic_allow_aspect_ratio_for_kmode
> >> + * @state: the crtc state
> >> + * @mode: kernel-internal mode, which is used to create a duplicate mode,
> >> + * with updated picture aspect ratio.
> >> + *
> >> + * Allow the aspect ratio info in the kernel mode only if aspect ratio is
> >> + * supported.
> >> + *
> >> + * RETURNS:
> >> + * kernel-internal mode with updated picture aspect ratio value.
> >> + */
> >> +
> >> +struct drm_display_mode*
> >> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state *state,
> >> +					const struct drm_display_mode *mode)
> >> +{
> >> +	struct drm_atomic_state *atomic_state = state->state;
> >> +	struct drm_display_mode *ar_kmode;
> >> +
> >> +	ar_kmode = drm_mode_duplicate(state->crtc->dev, mode);
> >> +	/*
> >> +	 * If aspect ratio is not supported, set the picture aspect ratio as
> >> +	 * NONE.
> >> +	 */
> >> +	if (atomic_state && !atomic_state->allow_aspect_ratio)
> >> +		ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >> +	return ar_kmode;
> >> +}
> >>   
> >>   /**
> >>    * drm_atomic_set_mode_for_crtc - set mode for CRTC
> >> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
> >>   	state->mode_blob = NULL;
> >>   
> >>   	if (mode) {
> >> -		drm_mode_convert_to_umode(&umode, mode);
> >> +		struct drm_display_mode *ar_mode;
> >> +
> >> +		ar_mode = drm_atomic_allow_aspect_ratio_for_kmode(state, mode);
> >> +		drm_mode_convert_to_umode(&umode, ar_mode);
> > This still looks sketchy.
> >
> > I believe there are just two places where we need to filter out the
> > aspect ratio flags from the umode, namely the getblob and getcrtc
> > ioctls.
> 
> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob, 
> etc. apart from the mode blob.
> Is there any way to check from blob id (or by any other means),
> if the data is for the mode, or edid, or gamma etc.

I think we'd have to somehow mark the mode blobs as special. Until we
have more need for cleaning up blobs before returning them to userspace
I think a simple flag should do. If we come up with more uses like this
then it might make sense to introduce some kind of optional filter
function for blobs.

> 
> If we can check that, I can make sure that aspect-ratio flags are taken 
> care of, if the blob is for mode,
> and the aspect ratio is not supported.
> 
> >
> > And for checking the user input I think we should probably just
> > stick that into drm_mode_convert_umode(). Looks like we never call
> > that from anywhere but the atomic/setprop and setcrtc paths with
> > a non-NULL argument.
> >
> > I *think* those three places should be sufficient to cover everything.
> Agreed. Infact I tried to do that first, but the only problem we have 
> here, is that, the file_priv
> which has the aspect ratio capability information is not supplied to the 
> drm_mode_convert_umode()
> or the functions calling, drm_mode_conver_umode(). At best we have 
> drm_crtc_state.

Simply passing the file_priv down would seem like the most obvious
solution.

> 
> I have added an aspect ratio allowed flag  in drm_crtc_state->state so 
> that its available in the
> functions calling drm_mode_convert_umode.
> 
> I am open to suggestion to avoid adding a new flag in drm_atomic_state, 
> if there is a better
> way to let the drm_mode_convert_umode( ) know that user supports aspect 
> ratio capability.
> >
> >>   		state->mode_blob =
> >>   			drm_property_create_blob(state->crtc->dev,
> >>   		                                 sizeof(umode),
> >> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
> >>   		if (IS_ERR(state->mode_blob))
> >>   			return PTR_ERR(state->mode_blob);
> >>   
> >> -		drm_mode_copy(&state->mode, mode);
> >> +		drm_mode_copy(&state->mode, ar_mode);
> >>   		state->enable = true;
> >>   		DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
> >> -				 mode->name, state);
> >> +				 ar_mode->name, state);
> >> +		drm_mode_destroy(state->crtc->dev, ar_mode);
> >>   	} else {
> >>   		memset(&state->mode, 0, sizeof(state->mode));
> >>   		state->enable = false;
> >> @@ -436,6 +469,25 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
> >>   }
> >>   
> >>   /**
> >> + * drm_atomic_allow_aspect_ratio_for_umode
> >> + * @state: the crtc state
> >> + * @umode: userspace mode, whose aspect ratio flag bits are to be updated.
> >> + *
> >> + * Allow the aspect ratio info in the userspace mode only if aspect ratio is
> >> + * supported.
> >> + */
> >> +void
> >> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state *state,
> >> +					struct drm_mode_modeinfo *umode)
> >> +{
> >> +	struct drm_atomic_state *atomic_state = state->state;
> >> +
> >> +	/* Reset the aspect ratio flags if aspect ratio is not supported */
> >> +	if (atomic_state && !atomic_state->allow_aspect_ratio)
> >> +		umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
> >> +}
> >> +
> >> +/**
> >>    * drm_atomic_crtc_set_property - set property on CRTC
> >>    * @crtc: the drm CRTC to set a property on
> >>    * @state: the state object to update with the new property value
> >> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> >>   	else if (property == config->prop_mode_id) {
> >>   		struct drm_property_blob *mode =
> >>   			drm_property_lookup_blob(dev, val);
> >> +		drm_atomic_allow_aspect_ratio_for_umode(state,
> >> +					(struct drm_mode_modeinfo *)
> >> +					mode->data);
> >>   		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
> >>   		drm_property_blob_put(mode);
> >>   		return ret;
> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >> index f0556e6..a2d34fa 100644
> >> --- a/drivers/gpu/drm/drm_crtc.c
> >> +++ b/drivers/gpu/drm/drm_crtc.c
> >> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
> >>   	drm_modeset_lock(&crtc->mutex, NULL);
> >>   	if (crtc->state) {
> >>   		if (crtc->state->enable) {
> >> +			/*
> >> +			 * If the aspect-ratio is not required by the,
> >> +			 * userspace, do not set the aspect-ratio flags.
> >> +			 */
> >> +			if (!file_priv->aspect_ratio_allowed)
> >> +				crtc->state->mode.picture_aspect_ratio =
> >> +					HDMI_PICTURE_ASPECT_NONE;
> > These would still clobber the current crtc state. So definitely don't
> > want to do this.
> Alright, so alternative way of doing this will be:
> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel 
> mode structure,
> but set the aspect ratio flags in the usermode to none, after calling the
> drm_mode_convert_to_umode().
> 
> Will take care of this in the next patchset.
> 
> >
> >>   			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
> >>   			crtc_resp->mode_valid = 1;
> >>   
> >> @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
> >>   		crtc_resp->x = crtc->x;
> >>   		crtc_resp->y = crtc->y;
> >>   		if (crtc->enabled) {
> >> +			/*
> >> +			 * If the aspect-ratio is not required by the,
> >> +			 * userspace, do not set the aspect-ratio flags.
> >> +			 */
> >> +			if (!file_priv->aspect_ratio_allowed)
> >> +				crtc->mode.picture_aspect_ratio =
> >> +					HDMI_PICTURE_ASPECT_NONE;
> >>   			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
> >>   			crtc_resp->mode_valid = 1;
> >>   
> >> @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
> >>   			goto out;
> >>   		}
> >>   
> >> +		/* If the user does not ask for aspect ratio, reset the aspect
> >> +		 * ratio bits in the usermode.
> >> +		 */
> >> +		if (!file_priv->aspect_ratio_allowed)
> >> +			crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
> >>   		ret = drm_mode_convert_umode(mode, &crtc_req->mode);
> >>   		if (ret) {
> >>   			DRM_DEBUG_KMS("Invalid mode\n");
> >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> >> index 1c27526..130dad9 100644
> >> --- a/include/drm/drm_atomic.h
> >> +++ b/include/drm/drm_atomic.h
> >> @@ -237,6 +237,7 @@ struct __drm_private_objs_state {
> >>    * @ref: count of all references to this state (will not be freed until zero)
> >>    * @dev: parent DRM device
> >>    * @allow_modeset: allow full modeset
> >> + * @allow_aspect_ratio: allow the aspect ratio info to be passes to user
> >>    * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
> >>    * @async_update: hint for asynchronous plane update
> >>    * @planes: pointer to array of structures with per-plane data
> >> @@ -256,6 +257,7 @@ struct drm_atomic_state {
> >>   
> >>   	struct drm_device *dev;
> >>   	bool allow_modeset : 1;
> >> +	bool allow_aspect_ratio : 1;
> >>   	bool legacy_cursor_update : 1;
> >>   	bool async_update : 1;
> >>   	struct __drm_planes_state *planes;
> >> -- 
> >> 2.7.4
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> [e-mail]
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Nautiyal, Ankit K Feb. 1, 2018, 11:05 a.m. UTC | #4

Hi Ville,

Appreciate your time and the suggestions.
Please find my response inline:

On 1/31/2018 6:39 PM, Ville Syrjälä wrote:
> On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote:
>>
>> On 1/30/2018 12:23 AM, Ville Syrjälä wrote:
>>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote:
>>>> From: Ankit Nautiyal <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>>
>>>> V3: Addressed review comments from Ville:
>>>> -Do not corrupt the current crtc state by updating aspect ratio
>>>> on the fly.
>>>> ---
>>>>    drivers/gpu/drm/drm_atomic.c | 61 +++++++++++++++++++++++++++++++++++++++++---
>>>>    drivers/gpu/drm/drm_crtc.c   | 19 ++++++++++++++
>>>>    include/drm/drm_atomic.h     |  2 ++
>>>>    3 files changed, 79 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>> index 69ff763..39313e2 100644
>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>> @@ -316,6 +316,35 @@ static s32 __user *get_out_fence_for_crtc(struct drm_atomic_state *state,
>>>>    
>>>>    	return fence_ptr;
>>>>    }
>>>> +/**
>>>> + * drm_atomic_allow_aspect_ratio_for_kmode
>>>> + * @state: the crtc state
>>>> + * @mode: kernel-internal mode, which is used to create a duplicate mode,
>>>> + * with updated picture aspect ratio.
>>>> + *
>>>> + * Allow the aspect ratio info in the kernel mode only if aspect ratio is
>>>> + * supported.
>>>> + *
>>>> + * RETURNS:
>>>> + * kernel-internal mode with updated picture aspect ratio value.
>>>> + */
>>>> +
>>>> +struct drm_display_mode*
>>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state *state,
>>>> +					const struct drm_display_mode *mode)
>>>> +{
>>>> +	struct drm_atomic_state *atomic_state = state->state;
>>>> +	struct drm_display_mode *ar_kmode;
>>>> +
>>>> +	ar_kmode = drm_mode_duplicate(state->crtc->dev, mode);
>>>> +	/*
>>>> +	 * If aspect ratio is not supported, set the picture aspect ratio as
>>>> +	 * NONE.
>>>> +	 */
>>>> +	if (atomic_state && !atomic_state->allow_aspect_ratio)
>>>> +		ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>>> +	return ar_kmode;
>>>> +}
>>>>    
>>>>    /**
>>>>     * drm_atomic_set_mode_for_crtc - set mode for CRTC
>>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>>>>    	state->mode_blob = NULL;
>>>>    
>>>>    	if (mode) {
>>>> -		drm_mode_convert_to_umode(&umode, mode);
>>>> +		struct drm_display_mode *ar_mode;
>>>> +
>>>> +		ar_mode = drm_atomic_allow_aspect_ratio_for_kmode(state, mode);
>>>> +		drm_mode_convert_to_umode(&umode, ar_mode);
>>> This still looks sketchy.
>>>
>>> I believe there are just two places where we need to filter out the
>>> aspect ratio flags from the umode, namely the getblob and getcrtc
>>> ioctls.
>> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob,
>> etc. apart from the mode blob.
>> Is there any way to check from blob id (or by any other means),
>> if the data is for the mode, or edid, or gamma etc.
> I think we'd have to somehow mark the mode blobs as special. Until we
> have more need for cleaning up blobs before returning them to userspace
> I think a simple flag should do. If we come up with more uses like this
> then it might make sense to introduce some kind of optional filter
> function for blobs.
If my understanding is correct,
1. To be able to do this, we need to change in uapi.
2. Whenever the userspace creates a blob for mode, the new flag/class 
for blob type needs to be set.

Do you think that it's worth the effort?
>> If we can check that, I can make sure that aspect-ratio flags are taken
>> care of, if the blob is for mode,
>> and the aspect ratio is not supported.
>>
>>> And for checking the user input I think we should probably just
>>> stick that into drm_mode_convert_umode(). Looks like we never call
>>> that from anywhere but the atomic/setprop and setcrtc paths with
>>> a non-NULL argument.
>>>
>>> I *think* those three places should be sufficient to cover everything.
>> Agreed. Infact I tried to do that first, but the only problem we have
>> here, is that, the file_priv
>> which has the aspect ratio capability information is not supplied to the
>> drm_mode_convert_umode()
>> or the functions calling, drm_mode_conver_umode(). At best we have
>> drm_crtc_state.
> Simply passing the file_priv down would seem like the most obvious
> solution.

This means we have to make change in various drivers and we have to pull 
the file_priv
parameter three levels down. Should we think of a better way for doing this?

>> I have added an aspect ratio allowed flag  in drm_crtc_state->state so
>> that its available in the
>> functions calling drm_mode_convert_umode.
>>
>> I am open to suggestion to avoid adding a new flag in drm_atomic_state,
>> if there is a better
>> way to let the drm_mode_convert_umode( ) know that user supports aspect
>> ratio capability.
>>>>    		state->mode_blob =
>>>>    			drm_property_create_blob(state->crtc->dev,
>>>>    		                                 sizeof(umode),
>>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>>>>    		if (IS_ERR(state->mode_blob))
>>>>    			return PTR_ERR(state->mode_blob);
>>>>    
>>>> -		drm_mode_copy(&state->mode, mode);
>>>> +		drm_mode_copy(&state->mode, ar_mode);
>>>>    		state->enable = true;
>>>>    		DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
>>>> -				 mode->name, state);
>>>> +				 ar_mode->name, state);
>>>> +		drm_mode_destroy(state->crtc->dev, ar_mode);
>>>>    	} else {
>>>>    		memset(&state->mode, 0, sizeof(state->mode));
>>>>    		state->enable = false;
>>>> @@ -436,6 +469,25 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
>>>>    }
>>>>    
>>>>    /**
>>>> + * drm_atomic_allow_aspect_ratio_for_umode
>>>> + * @state: the crtc state
>>>> + * @umode: userspace mode, whose aspect ratio flag bits are to be updated.
>>>> + *
>>>> + * Allow the aspect ratio info in the userspace mode only if aspect ratio is
>>>> + * supported.
>>>> + */
>>>> +void
>>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state *state,
>>>> +					struct drm_mode_modeinfo *umode)
>>>> +{
>>>> +	struct drm_atomic_state *atomic_state = state->state;
>>>> +
>>>> +	/* Reset the aspect ratio flags if aspect ratio is not supported */
>>>> +	if (atomic_state && !atomic_state->allow_aspect_ratio)
>>>> +		umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
>>>> +}
>>>> +
>>>> +/**
>>>>     * drm_atomic_crtc_set_property - set property on CRTC
>>>>     * @crtc: the drm CRTC to set a property on
>>>>     * @state: the state object to update with the new property value
>>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>>>>    	else if (property == config->prop_mode_id) {
>>>>    		struct drm_property_blob *mode =
>>>>    			drm_property_lookup_blob(dev, val);
>>>> +		drm_atomic_allow_aspect_ratio_for_umode(state,
>>>> +					(struct drm_mode_modeinfo *)
>>>> +					mode->data);
>>>>    		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>>>>    		drm_property_blob_put(mode);
>>>>    		return ret;
>>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>>> index f0556e6..a2d34fa 100644
>>>> --- a/drivers/gpu/drm/drm_crtc.c
>>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
>>>>    	drm_modeset_lock(&crtc->mutex, NULL);
>>>>    	if (crtc->state) {
>>>>    		if (crtc->state->enable) {
>>>> +			/*
>>>> +			 * If the aspect-ratio is not required by the,
>>>> +			 * userspace, do not set the aspect-ratio flags.
>>>> +			 */
>>>> +			if (!file_priv->aspect_ratio_allowed)
>>>> +				crtc->state->mode.picture_aspect_ratio =
>>>> +					HDMI_PICTURE_ASPECT_NONE;
>>> These would still clobber the current crtc state. So definitely don't
>>> want to do this.
>> Alright, so alternative way of doing this will be:
>> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel
>> mode structure,
>> but set the aspect ratio flags in the usermode to none, after calling the
>> drm_mode_convert_to_umode().
>>
>> Will take care of this in the next patchset.

I gave a thought about it again. As you have mentioned earlier that 
get_crtc is one of the
place where the user mode aspect ratio flags must be filtered out, is 
this what you have
in mind. I hope I did not misunderstand.

Changing the usermode aspect ratio flag bits, without change in the 
picture_aspect_ratio
kernel mode will not result in both structures out of sync?

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

Ville Syrjälä Feb. 1, 2018, 12:54 p.m. UTC | #5

On Thu, Feb 01, 2018 at 04:35:22PM +0530, Nautiyal, Ankit K wrote:
> Hi Ville,
> 
> Appreciate your time and the suggestions.
> Please find my response inline:
> 
> On 1/31/2018 6:39 PM, Ville Syrjälä wrote:
> > On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote:
> >>
> >> On 1/30/2018 12:23 AM, Ville Syrjälä wrote:
> >>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote:
> >>>> From: Ankit Nautiyal <[e-mail]>
> >>>>
> >>>> 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 <[e-mail]>
> >>>>
> >>>> V3: Addressed review comments from Ville:
> >>>> -Do not corrupt the current crtc state by updating aspect ratio
> >>>> on the fly.
> >>>> ---
> >>>>    drivers/gpu/drm/drm_atomic.c | 61 +++++++++++++++++++++++++++++++++++++++++---
> >>>>    drivers/gpu/drm/drm_crtc.c   | 19 ++++++++++++++
> >>>>    include/drm/drm_atomic.h     |  2 ++
> >>>>    3 files changed, 79 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>>> index 69ff763..39313e2 100644
> >>>> --- a/drivers/gpu/drm/drm_atomic.c
> >>>> +++ b/drivers/gpu/drm/drm_atomic.c
> >>>> @@ -316,6 +316,35 @@ static s32 __user *get_out_fence_for_crtc(struct drm_atomic_state *state,
> >>>>    
> >>>>    	return fence_ptr;
> >>>>    }
> >>>> +/**
> >>>> + * drm_atomic_allow_aspect_ratio_for_kmode
> >>>> + * @state: the crtc state
> >>>> + * @mode: kernel-internal mode, which is used to create a duplicate mode,
> >>>> + * with updated picture aspect ratio.
> >>>> + *
> >>>> + * Allow the aspect ratio info in the kernel mode only if aspect ratio is
> >>>> + * supported.
> >>>> + *
> >>>> + * RETURNS:
> >>>> + * kernel-internal mode with updated picture aspect ratio value.
> >>>> + */
> >>>> +
> >>>> +struct drm_display_mode*
> >>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state *state,
> >>>> +					const struct drm_display_mode *mode)
> >>>> +{
> >>>> +	struct drm_atomic_state *atomic_state = state->state;
> >>>> +	struct drm_display_mode *ar_kmode;
> >>>> +
> >>>> +	ar_kmode = drm_mode_duplicate(state->crtc->dev, mode);
> >>>> +	/*
> >>>> +	 * If aspect ratio is not supported, set the picture aspect ratio as
> >>>> +	 * NONE.
> >>>> +	 */
> >>>> +	if (atomic_state && !atomic_state->allow_aspect_ratio)
> >>>> +		ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >>>> +	return ar_kmode;
> >>>> +}
> >>>>    
> >>>>    /**
> >>>>     * drm_atomic_set_mode_for_crtc - set mode for CRTC
> >>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
> >>>>    	state->mode_blob = NULL;
> >>>>    
> >>>>    	if (mode) {
> >>>> -		drm_mode_convert_to_umode(&umode, mode);
> >>>> +		struct drm_display_mode *ar_mode;
> >>>> +
> >>>> +		ar_mode = drm_atomic_allow_aspect_ratio_for_kmode(state, mode);
> >>>> +		drm_mode_convert_to_umode(&umode, ar_mode);
> >>> This still looks sketchy.
> >>>
> >>> I believe there are just two places where we need to filter out the
> >>> aspect ratio flags from the umode, namely the getblob and getcrtc
> >>> ioctls.
> >> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob,
> >> etc. apart from the mode blob.
> >> Is there any way to check from blob id (or by any other means),
> >> if the data is for the mode, or edid, or gamma etc.
> > I think we'd have to somehow mark the mode blobs as special. Until we
> > have more need for cleaning up blobs before returning them to userspace
> > I think a simple flag should do. If we come up with more uses like this
> > then it might make sense to introduce some kind of optional filter
> > function for blobs.
> If my understanding is correct,
> 1. To be able to do this, we need to change in uapi.

We should be fine with an internal flag. Obviously it won't be set
for blobs freshly created by userspace, but that's fine because we
do no other error checking for them either. The error checking will
happen when the user tries to actually use the blob.

> 2. Whenever the userspace creates a blob for mode, the new flag/class 
> for blob type needs to be set.
> 
> Do you think that it's worth the effort?
> >> If we can check that, I can make sure that aspect-ratio flags are taken
> >> care of, if the blob is for mode,
> >> and the aspect ratio is not supported.
> >>
> >>> And for checking the user input I think we should probably just
> >>> stick that into drm_mode_convert_umode(). Looks like we never call
> >>> that from anywhere but the atomic/setprop and setcrtc paths with
> >>> a non-NULL argument.
> >>>
> >>> I *think* those three places should be sufficient to cover everything.
> >> Agreed. Infact I tried to do that first, but the only problem we have
> >> here, is that, the file_priv
> >> which has the aspect ratio capability information is not supplied to the
> >> drm_mode_convert_umode()
> >> or the functions calling, drm_mode_conver_umode(). At best we have
> >> drm_crtc_state.
> > Simply passing the file_priv down would seem like the most obvious
> > solution.
> 
> This means we have to make change in various drivers and we have to pull 
> the file_priv
> parameter three levels down. Should we think of a better way for doing this?

Coccinelle is pretty good at modifying function parameters.

> 
> >> I have added an aspect ratio allowed flag  in drm_crtc_state->state so
> >> that its available in the
> >> functions calling drm_mode_convert_umode.
> >>
> >> I am open to suggestion to avoid adding a new flag in drm_atomic_state,
> >> if there is a better
> >> way to let the drm_mode_convert_umode( ) know that user supports aspect
> >> ratio capability.
> >>>>    		state->mode_blob =
> >>>>    			drm_property_create_blob(state->crtc->dev,
> >>>>    		                                 sizeof(umode),
> >>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
> >>>>    		if (IS_ERR(state->mode_blob))
> >>>>    			return PTR_ERR(state->mode_blob);
> >>>>    
> >>>> -		drm_mode_copy(&state->mode, mode);
> >>>> +		drm_mode_copy(&state->mode, ar_mode);
> >>>>    		state->enable = true;
> >>>>    		DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
> >>>> -				 mode->name, state);
> >>>> +				 ar_mode->name, state);
> >>>> +		drm_mode_destroy(state->crtc->dev, ar_mode);
> >>>>    	} else {
> >>>>    		memset(&state->mode, 0, sizeof(state->mode));
> >>>>    		state->enable = false;
> >>>> @@ -436,6 +469,25 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
> >>>>    }
> >>>>    
> >>>>    /**
> >>>> + * drm_atomic_allow_aspect_ratio_for_umode
> >>>> + * @state: the crtc state
> >>>> + * @umode: userspace mode, whose aspect ratio flag bits are to be updated.
> >>>> + *
> >>>> + * Allow the aspect ratio info in the userspace mode only if aspect ratio is
> >>>> + * supported.
> >>>> + */
> >>>> +void
> >>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state *state,
> >>>> +					struct drm_mode_modeinfo *umode)
> >>>> +{
> >>>> +	struct drm_atomic_state *atomic_state = state->state;
> >>>> +
> >>>> +	/* Reset the aspect ratio flags if aspect ratio is not supported */
> >>>> +	if (atomic_state && !atomic_state->allow_aspect_ratio)
> >>>> +		umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
> >>>> +}
> >>>> +
> >>>> +/**
> >>>>     * drm_atomic_crtc_set_property - set property on CRTC
> >>>>     * @crtc: the drm CRTC to set a property on
> >>>>     * @state: the state object to update with the new property value
> >>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> >>>>    	else if (property == config->prop_mode_id) {
> >>>>    		struct drm_property_blob *mode =
> >>>>    			drm_property_lookup_blob(dev, val);
> >>>> +		drm_atomic_allow_aspect_ratio_for_umode(state,
> >>>> +					(struct drm_mode_modeinfo *)
> >>>> +					mode->data);
> >>>>    		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
> >>>>    		drm_property_blob_put(mode);
> >>>>    		return ret;
> >>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >>>> index f0556e6..a2d34fa 100644
> >>>> --- a/drivers/gpu/drm/drm_crtc.c
> >>>> +++ b/drivers/gpu/drm/drm_crtc.c
> >>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
> >>>>    	drm_modeset_lock(&crtc->mutex, NULL);
> >>>>    	if (crtc->state) {
> >>>>    		if (crtc->state->enable) {
> >>>> +			/*
> >>>> +			 * If the aspect-ratio is not required by the,
> >>>> +			 * userspace, do not set the aspect-ratio flags.
> >>>> +			 */
> >>>> +			if (!file_priv->aspect_ratio_allowed)
> >>>> +				crtc->state->mode.picture_aspect_ratio =
> >>>> +					HDMI_PICTURE_ASPECT_NONE;
> >>> These would still clobber the current crtc state. So definitely don't
> >>> want to do this.
> >> Alright, so alternative way of doing this will be:
> >> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel
> >> mode structure,
> >> but set the aspect ratio flags in the usermode to none, after calling the
> >> drm_mode_convert_to_umode().
> >>
> >> Will take care of this in the next patchset.
> 
> I gave a thought about it again. As you have mentioned earlier that 
> get_crtc is one of the
> place where the user mode aspect ratio flags must be filtered out, is 
> this what you have
> in mind. I hope I did not misunderstand.
> 
> Changing the usermode aspect ratio flag bits, without change in the 
> picture_aspect_ratio
> kernel mode will not result in both structures out of sync?

I don't think that should really matter. I suppose we might get one
extra modeset when the user feeds that mode back via setcrtc/atomic, but
meh. And actually the aspect ratio should only affect the infoframe, so
we should be able to eventually eliminate that extra modeset and just
update the infoframe instead.

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

Nautiyal, Ankit K Feb. 8, 2018, 4:29 a.m. UTC | #6

Hi Ville,

I still have some queries regarding the handling of aspect ratio flags 
in getblob ioctl.

Please find below my responses inline.


On 2/1/2018 6:24 PM, Ville Syrjälä wrote:
> On Thu, Feb 01, 2018 at 04:35:22PM +0530, Nautiyal, Ankit K wrote:
>> Hi Ville,
>>
>> Appreciate your time and the suggestions.
>> Please find my response inline:
>>
>> On 1/31/2018 6:39 PM, Ville Syrjälä wrote:
>>> On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote:
>>>> On 1/30/2018 12:23 AM, Ville Syrjälä wrote:
>>>>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote:
>>>>>> From: Ankit Nautiyal <[e-mail]>
>>>>>>
>>>>>> 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 <[e-mail]>
>>>>>>
>>>>>> V3: Addressed review comments from Ville:
>>>>>> -Do not corrupt the current crtc state by updating aspect ratio
>>>>>> on the fly.
>>>>>> ---
>>>>>>     drivers/gpu/drm/drm_atomic.c | 61 +++++++++++++++++++++++++++++++++++++++++---
>>>>>>     drivers/gpu/drm/drm_crtc.c   | 19 ++++++++++++++
>>>>>>     include/drm/drm_atomic.h     |  2 ++
>>>>>>     3 files changed, 79 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>>>>> index 69ff763..39313e2 100644
>>>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>>>> @@ -316,6 +316,35 @@ static s32 __user *get_out_fence_for_crtc(struct drm_atomic_state *state,
>>>>>>     
>>>>>>     	return fence_ptr;
>>>>>>     }
>>>>>> +/**
>>>>>> + * drm_atomic_allow_aspect_ratio_for_kmode
>>>>>> + * @state: the crtc state
>>>>>> + * @mode: kernel-internal mode, which is used to create a duplicate mode,
>>>>>> + * with updated picture aspect ratio.
>>>>>> + *
>>>>>> + * Allow the aspect ratio info in the kernel mode only if aspect ratio is
>>>>>> + * supported.
>>>>>> + *
>>>>>> + * RETURNS:
>>>>>> + * kernel-internal mode with updated picture aspect ratio value.
>>>>>> + */
>>>>>> +
>>>>>> +struct drm_display_mode*
>>>>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state *state,
>>>>>> +					const struct drm_display_mode *mode)
>>>>>> +{
>>>>>> +	struct drm_atomic_state *atomic_state = state->state;
>>>>>> +	struct drm_display_mode *ar_kmode;
>>>>>> +
>>>>>> +	ar_kmode = drm_mode_duplicate(state->crtc->dev, mode);
>>>>>> +	/*
>>>>>> +	 * If aspect ratio is not supported, set the picture aspect ratio as
>>>>>> +	 * NONE.
>>>>>> +	 */
>>>>>> +	if (atomic_state && !atomic_state->allow_aspect_ratio)
>>>>>> +		ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>>>>> +	return ar_kmode;
>>>>>> +}
>>>>>>     
>>>>>>     /**
>>>>>>      * drm_atomic_set_mode_for_crtc - set mode for CRTC
>>>>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>>>>>>     	state->mode_blob = NULL;
>>>>>>     
>>>>>>     	if (mode) {
>>>>>> -		drm_mode_convert_to_umode(&umode, mode);
>>>>>> +		struct drm_display_mode *ar_mode;
>>>>>> +
>>>>>> +		ar_mode = drm_atomic_allow_aspect_ratio_for_kmode(state, mode);
>>>>>> +		drm_mode_convert_to_umode(&umode, ar_mode);
>>>>> This still looks sketchy.
>>>>>
>>>>> I believe there are just two places where we need to filter out the
>>>>> aspect ratio flags from the umode, namely the getblob and getcrtc
>>>>> ioctls.
>>>> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob,
>>>> etc. apart from the mode blob.
>>>> Is there any way to check from blob id (or by any other means),
>>>> if the data is for the mode, or edid, or gamma etc.
>>> I think we'd have to somehow mark the mode blobs as special. Until we
>>> have more need for cleaning up blobs before returning them to userspace
>>> I think a simple flag should do. If we come up with more uses like this
>>> then it might make sense to introduce some kind of optional filter
>>> function for blobs.
>> If my understanding is correct,
>> 1. To be able to do this, we need to change in uapi.
> We should be fine with an internal flag. Obviously it won't be set
> for blobs freshly created by userspace, but that's fine because we
> do no other error checking for them either. The error checking will
> happen when the user tries to actually use the blob.

I am not sure why getblob ioctl should even come into picture. (Perhaps 
I am missing something).
As per my understanding:
If a userspace needs to set a mode, it will just create a blob with 
drm_mode_mode_info,
and get the blob-id. This blob-id would be supplied to 
drm_mode_atomic_ioctl as a crtc
property mode_id.
At the kernel side,  the crtc property mode_id is set by looking-up for 
mode blob from blob id.
I am doing the change in the user mode aspect-ratio flags at this junction.

As far as getblob ioctl is concerned, if the user does call getblob 
ioctl for getting mode blob from
the blob id, it will receive the same mode blob, with same aspect ratio 
info which the user had
created using create blob ioctl. If it does want to use the blob, it has 
to come through the
drm_mode_atomic_ioctl way, where aspect ratio flags are handled.

>> 2. Whenever the userspace creates a blob for mode, the new flag/class
>> for blob type needs to be set.
>>
>> Do you think that it's worth the effort?
>>>> If we can check that, I can make sure that aspect-ratio flags are taken
>>>> care of, if the blob is for mode,
>>>> and the aspect ratio is not supported.
>>>>
>>>>> And for checking the user input I think we should probably just
>>>>> stick that into drm_mode_convert_umode(). Looks like we never call
>>>>> that from anywhere but the atomic/setprop and setcrtc paths with
>>>>> a non-NULL argument.
>>>>>
>>>>> I *think* those three places should be sufficient to cover everything.
>>>> Agreed. Infact I tried to do that first, but the only problem we have
>>>> here, is that, the file_priv
>>>> which has the aspect ratio capability information is not supplied to the
>>>> drm_mode_convert_umode()
>>>> or the functions calling, drm_mode_conver_umode(). At best we have
>>>> drm_crtc_state.
>>> Simply passing the file_priv down would seem like the most obvious
>>> solution.
>> This means we have to make change in various drivers and we have to pull
>> the file_priv
>> parameter three levels down. Should we think of a better way for doing this?
> Coccinelle is pretty good at modifying function parameters.

Thanks for pointing out Coccinelle, will defintely try that out.
But my question here is whether we really want to drag the file_priv
from drm_mode_atomic_ioctl --> drm_mode_atomic_set_property -->
drm_atomic_crtc_set_property --> drm_mode_set_mode_prop_for_crtc-->
drm_mode_convert_umode.

Instead, I tried to add a new field (allow_aspect_ratio) in 
drm_crtc_state which is
already passed from the drm_mode_atomic_ioctl all the way to
drm_mode_convert_umode.


-Regards
Ankit
>
>>>> I have added an aspect ratio allowed flag  in drm_crtc_state->state so
>>>> that its available in the
>>>> functions calling drm_mode_convert_umode.
>>>>
>>>> I am open to suggestion to avoid adding a new flag in drm_atomic_state,
>>>> if there is a better
>>>> way to let the drm_mode_convert_umode( ) know that user supports aspect
>>>> ratio capability.
>>>>>>     		state->mode_blob =
>>>>>>     			drm_property_create_blob(state->crtc->dev,
>>>>>>     		                                 sizeof(umode),
>>>>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>>>>>>     		if (IS_ERR(state->mode_blob))
>>>>>>     			return PTR_ERR(state->mode_blob);
>>>>>>     
>>>>>> -		drm_mode_copy(&state->mode, mode);
>>>>>> +		drm_mode_copy(&state->mode, ar_mode);
>>>>>>     		state->enable = true;
>>>>>>     		DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
>>>>>> -				 mode->name, state);
>>>>>> +				 ar_mode->name, state);
>>>>>> +		drm_mode_destroy(state->crtc->dev, ar_mode);
>>>>>>     	} else {
>>>>>>     		memset(&state->mode, 0, sizeof(state->mode));
>>>>>>     		state->enable = false;
>>>>>> @@ -436,6 +469,25 @@ drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
>>>>>>     }
>>>>>>     
>>>>>>     /**
>>>>>> + * drm_atomic_allow_aspect_ratio_for_umode
>>>>>> + * @state: the crtc state
>>>>>> + * @umode: userspace mode, whose aspect ratio flag bits are to be updated.
>>>>>> + *
>>>>>> + * Allow the aspect ratio info in the userspace mode only if aspect ratio is
>>>>>> + * supported.
>>>>>> + */
>>>>>> +void
>>>>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state *state,
>>>>>> +					struct drm_mode_modeinfo *umode)
>>>>>> +{
>>>>>> +	struct drm_atomic_state *atomic_state = state->state;
>>>>>> +
>>>>>> +	/* Reset the aspect ratio flags if aspect ratio is not supported */
>>>>>> +	if (atomic_state && !atomic_state->allow_aspect_ratio)
>>>>>> +		umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>>      * drm_atomic_crtc_set_property - set property on CRTC
>>>>>>      * @crtc: the drm CRTC to set a property on
>>>>>>      * @state: the state object to update with the new property value
>>>>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>>>>>>     	else if (property == config->prop_mode_id) {
>>>>>>     		struct drm_property_blob *mode =
>>>>>>     			drm_property_lookup_blob(dev, val);
>>>>>> +		drm_atomic_allow_aspect_ratio_for_umode(state,
>>>>>> +					(struct drm_mode_modeinfo *)
>>>>>> +					mode->data);
>>>>>>     		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>>>>>>     		drm_property_blob_put(mode);
>>>>>>     		return ret;
>>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>>>>> index f0556e6..a2d34fa 100644
>>>>>> --- a/drivers/gpu/drm/drm_crtc.c
>>>>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>>>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
>>>>>>     	drm_modeset_lock(&crtc->mutex, NULL);
>>>>>>     	if (crtc->state) {
>>>>>>     		if (crtc->state->enable) {
>>>>>> +			/*
>>>>>> +			 * If the aspect-ratio is not required by the,
>>>>>> +			 * userspace, do not set the aspect-ratio flags.
>>>>>> +			 */
>>>>>> +			if (!file_priv->aspect_ratio_allowed)
>>>>>> +				crtc->state->mode.picture_aspect_ratio =
>>>>>> +					HDMI_PICTURE_ASPECT_NONE;
>>>>> These would still clobber the current crtc state. So definitely don't
>>>>> want to do this.
>>>> Alright, so alternative way of doing this will be:
>>>> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel
>>>> mode structure,
>>>> but set the aspect ratio flags in the usermode to none, after calling the
>>>> drm_mode_convert_to_umode().
>>>>
>>>> Will take care of this in the next patchset.
>> I gave a thought about it again. As you have mentioned earlier that
>> get_crtc is one of the
>> place where the user mode aspect ratio flags must be filtered out, is
>> this what you have
>> in mind. I hope I did not misunderstand.
>>
>> Changing the usermode aspect ratio flag bits, without change in the
>> picture_aspect_ratio
>> kernel mode will not result in both structures out of sync?
> I don't think that should really matter. I suppose we might get one
> extra modeset when the user feeds that mode back via setcrtc/atomic, but
> meh. And actually the aspect ratio should only affect the infoframe, so
> we should be able to eventually eliminate that extra modeset and just
> update the infoframe instead.
>
>>>>>>     			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
>>>>>>     			crtc_resp->mode_valid = 1;
>>>>>>     
>>>>>> @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
>>>>>>     		crtc_resp->x = crtc->x;
>>>>>>     		crtc_resp->y = crtc->y;
>>>>>>     		if (crtc->enabled) {
>>>>>> +			/*
>>>>>> +			 * If the aspect-ratio is not required by the,
>>>>>> +			 * userspace, do not set the aspect-ratio flags.
>>>>>> +			 */
>>>>>> +			if (!file_priv->aspect_ratio_allowed)
>>>>>> +				crtc->mode.picture_aspect_ratio =
>>>>>> +					HDMI_PICTURE_ASPECT_NONE;
>>>>>>     			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
>>>>>>     			crtc_resp->mode_valid = 1;
>>>>>>     
>>>>>> @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>>>>>>     			goto out;
>>>>>>     		}
>>>>>>     
>>>>>> +		/* If the user does not ask for aspect ratio, reset the aspect
>>>>>> +		 * ratio bits in the usermode.
>>>>>> +		 */
>>>>>> +		if (!file_priv->aspect_ratio_allowed)
>>>>>> +			crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
>>>>>>     		ret = drm_mode_convert_umode(mode, &crtc_req->mode);
>>>>>>     		if (ret) {
>>>>>>     			DRM_DEBUG_KMS("Invalid mode\n");
>>>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>>>>>> index 1c27526..130dad9 100644
>>>>>> --- a/include/drm/drm_atomic.h
>>>>>> +++ b/include/drm/drm_atomic.h
>>>>>> @@ -237,6 +237,7 @@ struct __drm_private_objs_state {
>>>>>>      * @ref: count of all references to this state (will not be freed until zero)
>>>>>>      * @dev: parent DRM device
>>>>>>      * @allow_modeset: allow full modeset
>>>>>> + * @allow_aspect_ratio: allow the aspect ratio info to be passes to user
>>>>>>      * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
>>>>>>      * @async_update: hint for asynchronous plane update
>>>>>>      * @planes: pointer to array of structures with per-plane data
>>>>>> @@ -256,6 +257,7 @@ struct drm_atomic_state {
>>>>>>     
>>>>>>     	struct drm_device *dev;
>>>>>>     	bool allow_modeset : 1;
>>>>>> +	bool allow_aspect_ratio : 1;
>>>>>>     	bool legacy_cursor_update : 1;
>>>>>>     	bool async_update : 1;
>>>>>>     	struct __drm_planes_state *planes;
>>>>>> -- 
>>>>>> 2.7.4
>>>>>>
>>>>>> _______________________________________________
>>>>>> dri-devel mailing list
>>>>>> [e-mail]
>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> Regards,
>> Ankit

Nautiyal, Ankit K Feb. 13, 2018, 4:51 a.m. UTC | #7

Hi Ville,

As per our last discussion, following points were discussed:

1. To suppress the aspect-ratio info from getblob ioctl to a user that 
does not support it:

     i. A new flag must be added to drm_blob_property to mark if the 
blob has mode data.

     ii. This flag must be set when the user tries do an atomic modeset.

     iii. In the getblob ioctl, based on the above flag, it can be 
determined that if the blob

     has mode data, and the aspect ratio info can be suppressed in 
getblob ioctl, if user does not support it.

2. Instead of adding aspect ratio capabilities in drm_atomic_state, pass 
on the file_priv which already has

the required information to the function drm_mode_convert_umode.

It will require addition of an new argument file_priv to several 
functions, but that is right thing to do,

as file_priv is the correct structure for the capability information.

3. Changing the usermode aspect ratio flag bits, without change in the
picture_aspect_ratio would not matter, and does not need to be handled 
in this patch.


I just have one query here. We have agreed to modify 
drm_mode_convert_umode, to filter out the aspect-ratio

info if user has not set aspect-ratio capability, but do we need a 
similar change the drm_mode_convert_to_umode?

If we do, I am not sure if it would be possible to have the file_priv to 
/drm_atomic_set_mode_for_crtc/, which calls

drm_mode_convert_to_umode.  The function /drm_atomic_set_mode_for_crtc/ 
is used to set mode, originating by kernel,

and make a blob from the kernel mode, which it saves in crtc_state.

This function /: drm_atomic_set_mode_for_crtc, /is called by several 
helper functions and driver functions, and passing

file_priv from all these functions does not seem to be plausible.

Any suggestions on how to handle this situation?


Regards,

Ankit


On 2/8/2018 9:59 AM, Nautiyal, Ankit K wrote:
> Hi Ville,
>
> I still have some queries regarding the handling of aspect ratio flags 
> in getblob ioctl.
>
> Please find below my responses inline.
>
>
> On 2/1/2018 6:24 PM, Ville Syrjälä wrote:
>> On Thu, Feb 01, 2018 at 04:35:22PM +0530, Nautiyal, Ankit K wrote:
>>> Hi Ville,
>>>
>>> Appreciate your time and the suggestions.
>>> Please find my response inline:
>>>
>>> On 1/31/2018 6:39 PM, Ville Syrjälä wrote:
>>>> On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote:
>>>>> On 1/30/2018 12:23 AM, Ville Syrjälä wrote:
>>>>>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote:
>>>>>>> From: Ankit Nautiyal <[e-mail]>
>>>>>>>
>>>>>>> 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 <[e-mail]>
>>>>>>>
>>>>>>> V3: Addressed review comments from Ville:
>>>>>>> -Do not corrupt the current crtc state by updating aspect ratio
>>>>>>> on the fly.
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/drm_atomic.c | 61 
>>>>>>> +++++++++++++++++++++++++++++++++++++++++---
>>>>>>>     drivers/gpu/drm/drm_crtc.c   | 19 ++++++++++++++
>>>>>>>     include/drm/drm_atomic.h     |  2 ++
>>>>>>>     3 files changed, 79 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c 
>>>>>>> b/drivers/gpu/drm/drm_atomic.c
>>>>>>> index 69ff763..39313e2 100644
>>>>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>>>>> @@ -316,6 +316,35 @@ static s32 __user 
>>>>>>> *get_out_fence_for_crtc(struct drm_atomic_state *state,
>>>>>>>             return fence_ptr;
>>>>>>>     }
>>>>>>> +/**
>>>>>>> + * drm_atomic_allow_aspect_ratio_for_kmode
>>>>>>> + * @state: the crtc state
>>>>>>> + * @mode: kernel-internal mode, which is used to create a 
>>>>>>> duplicate mode,
>>>>>>> + * with updated picture aspect ratio.
>>>>>>> + *
>>>>>>> + * Allow the aspect ratio info in the kernel mode only if 
>>>>>>> aspect ratio is
>>>>>>> + * supported.
>>>>>>> + *
>>>>>>> + * RETURNS:
>>>>>>> + * kernel-internal mode with updated picture aspect ratio value.
>>>>>>> + */
>>>>>>> +
>>>>>>> +struct drm_display_mode*
>>>>>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state 
>>>>>>> *state,
>>>>>>> +                    const struct drm_display_mode *mode)
>>>>>>> +{
>>>>>>> +    struct drm_atomic_state *atomic_state = state->state;
>>>>>>> +    struct drm_display_mode *ar_kmode;
>>>>>>> +
>>>>>>> +    ar_kmode = drm_mode_duplicate(state->crtc->dev, mode);
>>>>>>> +    /*
>>>>>>> +     * If aspect ratio is not supported, set the picture aspect 
>>>>>>> ratio as
>>>>>>> +     * NONE.
>>>>>>> +     */
>>>>>>> +    if (atomic_state && !atomic_state->allow_aspect_ratio)
>>>>>>> +        ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>>>>>> +    return ar_kmode;
>>>>>>> +}
>>>>>>>         /**
>>>>>>>      * drm_atomic_set_mode_for_crtc - set mode for CRTC
>>>>>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct 
>>>>>>> drm_crtc_state *state,
>>>>>>>         state->mode_blob = NULL;
>>>>>>>             if (mode) {
>>>>>>> -        drm_mode_convert_to_umode(&umode, mode);
>>>>>>> +        struct drm_display_mode *ar_mode;
>>>>>>> +
>>>>>>> +        ar_mode = 
>>>>>>> drm_atomic_allow_aspect_ratio_for_kmode(state, mode);
>>>>>>> +        drm_mode_convert_to_umode(&umode, ar_mode);
>>>>>> This still looks sketchy.
>>>>>>
>>>>>> I believe there are just two places where we need to filter out the
>>>>>> aspect ratio flags from the umode, namely the getblob and getcrtc
>>>>>> ioctls.
>>>>> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob,
>>>>> etc. apart from the mode blob.
>>>>> Is there any way to check from blob id (or by any other means),
>>>>> if the data is for the mode, or edid, or gamma etc.
>>>> I think we'd have to somehow mark the mode blobs as special. Until we
>>>> have more need for cleaning up blobs before returning them to 
>>>> userspace
>>>> I think a simple flag should do. If we come up with more uses like 
>>>> this
>>>> then it might make sense to introduce some kind of optional filter
>>>> function for blobs.
>>> If my understanding is correct,
>>> 1. To be able to do this, we need to change in uapi.
>> We should be fine with an internal flag. Obviously it won't be set
>> for blobs freshly created by userspace, but that's fine because we
>> do no other error checking for them either. The error checking will
>> happen when the user tries to actually use the blob.
>
> I am not sure why getblob ioctl should even come into picture. 
> (Perhaps I am missing something).
> As per my understanding:
> If a userspace needs to set a mode, it will just create a blob with 
> drm_mode_mode_info,
> and get the blob-id. This blob-id would be supplied to 
> drm_mode_atomic_ioctl as a crtc
> property mode_id.
> At the kernel side,  the crtc property mode_id is set by looking-up 
> for mode blob from blob id.
> I am doing the change in the user mode aspect-ratio flags at this 
> junction.
>
> As far as getblob ioctl is concerned, if the user does call getblob 
> ioctl for getting mode blob from
> the blob id, it will receive the same mode blob, with same aspect 
> ratio info which the user had
> created using create blob ioctl. If it does want to use the blob, it 
> has to come through the
> drm_mode_atomic_ioctl way, where aspect ratio flags are handled.
>
>>> 2. Whenever the userspace creates a blob for mode, the new flag/class
>>> for blob type needs to be set.
>>>
>>> Do you think that it's worth the effort?
>>>>> If we can check that, I can make sure that aspect-ratio flags are 
>>>>> taken
>>>>> care of, if the blob is for mode,
>>>>> and the aspect ratio is not supported.
>>>>>
>>>>>> And for checking the user input I think we should probably just
>>>>>> stick that into drm_mode_convert_umode(). Looks like we never call
>>>>>> that from anywhere but the atomic/setprop and setcrtc paths with
>>>>>> a non-NULL argument.
>>>>>>
>>>>>> I *think* those three places should be sufficient to cover 
>>>>>> everything.
>>>>> Agreed. Infact I tried to do that first, but the only problem we have
>>>>> here, is that, the file_priv
>>>>> which has the aspect ratio capability information is not supplied 
>>>>> to the
>>>>> drm_mode_convert_umode()
>>>>> or the functions calling, drm_mode_conver_umode(). At best we have
>>>>> drm_crtc_state.
>>>> Simply passing the file_priv down would seem like the most obvious
>>>> solution.
>>> This means we have to make change in various drivers and we have to 
>>> pull
>>> the file_priv
>>> parameter three levels down. Should we think of a better way for 
>>> doing this?
>> Coccinelle is pretty good at modifying function parameters.
>
> Thanks for pointing out Coccinelle, will defintely try that out.
> But my question here is whether we really want to drag the file_priv
> from drm_mode_atomic_ioctl --> drm_mode_atomic_set_property -->
> drm_atomic_crtc_set_property --> drm_mode_set_mode_prop_for_crtc-->
> drm_mode_convert_umode.
>
> Instead, I tried to add a new field (allow_aspect_ratio) in 
> drm_crtc_state which is
> already passed from the drm_mode_atomic_ioctl all the way to
> drm_mode_convert_umode.
>
>
> -Regards
> Ankit
>>
>>>>> I have added an aspect ratio allowed flag  in 
>>>>> drm_crtc_state->state so
>>>>> that its available in the
>>>>> functions calling drm_mode_convert_umode.
>>>>>
>>>>> I am open to suggestion to avoid adding a new flag in 
>>>>> drm_atomic_state,
>>>>> if there is a better
>>>>> way to let the drm_mode_convert_umode( ) know that user supports 
>>>>> aspect
>>>>> ratio capability.
>>>>>>>             state->mode_blob =
>>>>>>> drm_property_create_blob(state->crtc->dev,
>>>>>>> sizeof(umode),
>>>>>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct 
>>>>>>> drm_crtc_state *state,
>>>>>>>             if (IS_ERR(state->mode_blob))
>>>>>>>                 return PTR_ERR(state->mode_blob);
>>>>>>>     -        drm_mode_copy(&state->mode, mode);
>>>>>>> +        drm_mode_copy(&state->mode, ar_mode);
>>>>>>>             state->enable = true;
>>>>>>>             DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
>>>>>>> -                 mode->name, state);
>>>>>>> +                 ar_mode->name, state);
>>>>>>> +        drm_mode_destroy(state->crtc->dev, ar_mode);
>>>>>>>         } else {
>>>>>>>             memset(&state->mode, 0, sizeof(state->mode));
>>>>>>>             state->enable = false;
>>>>>>> @@ -436,6 +469,25 @@ 
>>>>>>> drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
>>>>>>>     }
>>>>>>>         /**
>>>>>>> + * drm_atomic_allow_aspect_ratio_for_umode
>>>>>>> + * @state: the crtc state
>>>>>>> + * @umode: userspace mode, whose aspect ratio flag bits are to 
>>>>>>> be updated.
>>>>>>> + *
>>>>>>> + * Allow the aspect ratio info in the userspace mode only if 
>>>>>>> aspect ratio is
>>>>>>> + * supported.
>>>>>>> + */
>>>>>>> +void
>>>>>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state 
>>>>>>> *state,
>>>>>>> +                    struct drm_mode_modeinfo *umode)
>>>>>>> +{
>>>>>>> +    struct drm_atomic_state *atomic_state = state->state;
>>>>>>> +
>>>>>>> +    /* Reset the aspect ratio flags if aspect ratio is not 
>>>>>>> supported */
>>>>>>> +    if (atomic_state && !atomic_state->allow_aspect_ratio)
>>>>>>> +        umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/**
>>>>>>>      * drm_atomic_crtc_set_property - set property on CRTC
>>>>>>>      * @crtc: the drm CRTC to set a property on
>>>>>>>      * @state: the state object to update with the new property 
>>>>>>> value
>>>>>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct 
>>>>>>> drm_crtc *crtc,
>>>>>>>         else if (property == config->prop_mode_id) {
>>>>>>>             struct drm_property_blob *mode =
>>>>>>>                 drm_property_lookup_blob(dev, val);
>>>>>>> + drm_atomic_allow_aspect_ratio_for_umode(state,
>>>>>>> +                    (struct drm_mode_modeinfo *)
>>>>>>> +                    mode->data);
>>>>>>>             ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>>>>>>>             drm_property_blob_put(mode);
>>>>>>>             return ret;
>>>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c 
>>>>>>> b/drivers/gpu/drm/drm_crtc.c
>>>>>>> index f0556e6..a2d34fa 100644
>>>>>>> --- a/drivers/gpu/drm/drm_crtc.c
>>>>>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>>>>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
>>>>>>>         drm_modeset_lock(&crtc->mutex, NULL);
>>>>>>>         if (crtc->state) {
>>>>>>>             if (crtc->state->enable) {
>>>>>>> +            /*
>>>>>>> +             * If the aspect-ratio is not required by the,
>>>>>>> +             * userspace, do not set the aspect-ratio flags.
>>>>>>> +             */
>>>>>>> +            if (!file_priv->aspect_ratio_allowed)
>>>>>>> + crtc->state->mode.picture_aspect_ratio =
>>>>>>> +                    HDMI_PICTURE_ASPECT_NONE;
>>>>>> These would still clobber the current crtc state. So definitely 
>>>>>> don't
>>>>>> want to do this.
>>>>> Alright, so alternative way of doing this will be:
>>>>> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel
>>>>> mode structure,
>>>>> but set the aspect ratio flags in the usermode to none, after 
>>>>> calling the
>>>>> drm_mode_convert_to_umode().
>>>>>
>>>>> Will take care of this in the next patchset.
>>> I gave a thought about it again. As you have mentioned earlier that
>>> get_crtc is one of the
>>> place where the user mode aspect ratio flags must be filtered out, is
>>> this what you have
>>> in mind. I hope I did not misunderstand.
>>>
>>> Changing the usermode aspect ratio flag bits, without change in the
>>> picture_aspect_ratio
>>> kernel mode will not result in both structures out of sync?
>> I don't think that should really matter. I suppose we might get one
>> extra modeset when the user feeds that mode back via setcrtc/atomic, but
>> meh. And actually the aspect ratio should only affect the infoframe, so
>> we should be able to eventually eliminate that extra modeset and just
>> update the infoframe instead.
>>
>>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
>>>>>>>                 crtc_resp->mode_valid = 1;
>>>>>>>     @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device 
>>>>>>> *dev,
>>>>>>>             crtc_resp->x = crtc->x;
>>>>>>>             crtc_resp->y = crtc->y;
>>>>>>>             if (crtc->enabled) {
>>>>>>> +            /*
>>>>>>> +             * If the aspect-ratio is not required by the,
>>>>>>> +             * userspace, do not set the aspect-ratio flags.
>>>>>>> +             */
>>>>>>> +            if (!file_priv->aspect_ratio_allowed)
>>>>>>> +                crtc->mode.picture_aspect_ratio =
>>>>>>> +                    HDMI_PICTURE_ASPECT_NONE;
>>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
>>>>>>>                 crtc_resp->mode_valid = 1;
>>>>>>>     @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device 
>>>>>>> *dev, void *data,
>>>>>>>                 goto out;
>>>>>>>             }
>>>>>>>     +        /* If the user does not ask for aspect ratio, reset 
>>>>>>> the aspect
>>>>>>> +         * ratio bits in the usermode.
>>>>>>> +         */
>>>>>>> +        if (!file_priv->aspect_ratio_allowed)
>>>>>>> +            crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
>>>>>>>             ret = drm_mode_convert_umode(mode, &crtc_req->mode);
>>>>>>>             if (ret) {
>>>>>>>                 DRM_DEBUG_KMS("Invalid mode\n");
>>>>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>>>>>>> index 1c27526..130dad9 100644
>>>>>>> --- a/include/drm/drm_atomic.h
>>>>>>> +++ b/include/drm/drm_atomic.h
>>>>>>> @@ -237,6 +237,7 @@ struct __drm_private_objs_state {
>>>>>>>      * @ref: count of all references to this state (will not be 
>>>>>>> freed until zero)
>>>>>>>      * @dev: parent DRM device
>>>>>>>      * @allow_modeset: allow full modeset
>>>>>>> + * @allow_aspect_ratio: allow the aspect ratio info to be 
>>>>>>> passes to user
>>>>>>>      * @legacy_cursor_update: hint to enforce legacy cursor 
>>>>>>> IOCTL semantics
>>>>>>>      * @async_update: hint for asynchronous plane update
>>>>>>>      * @planes: pointer to array of structures with per-plane data
>>>>>>> @@ -256,6 +257,7 @@ struct drm_atomic_state {
>>>>>>>             struct drm_device *dev;
>>>>>>>         bool allow_modeset : 1;
>>>>>>> +    bool allow_aspect_ratio : 1;
>>>>>>>         bool legacy_cursor_update : 1;
>>>>>>>         bool async_update : 1;
>>>>>>>         struct __drm_planes_state *planes;
>>>>>>> -- 
>>>>>>> 2.7.4
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> dri-devel mailing list
>>>>>>> [e-mail]
>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>> Regards,
>>> Ankit
>

Ville Syrjälä Feb. 13, 2018, 1:18 p.m. UTC | #8

On Tue, Feb 13, 2018 at 10:21:15AM +0530, Nautiyal, Ankit K wrote:
> Hi Ville,
> 
> As per our last discussion, following points were discussed:
> 
> 1. To suppress the aspect-ratio info from getblob ioctl to a user that 
> does not support it:
> 
>      i. A new flag must be added to drm_blob_property to mark if the 
> blob has mode data.
> 
>      ii. This flag must be set when the user tries do an atomic modeset.
> 
>      iii. In the getblob ioctl, based on the above flag, it can be 
> determined that if the blob
> 
>      has mode data, and the aspect ratio info can be suppressed in 
> getblob ioctl, if user does not support it.
> 
> 2. Instead of adding aspect ratio capabilities in drm_atomic_state, pass 
> on the file_priv which already has
> 
> the required information to the function drm_mode_convert_umode.
> 
> It will require addition of an new argument file_priv to several 
> functions, but that is right thing to do,
> 
> as file_priv is the correct structure for the capability information.
> 
> 3. Changing the usermode aspect ratio flag bits, without change in the
> picture_aspect_ratio would not matter, and does not need to be handled 
> in this patch.
> 
> 
> I just have one query here. We have agreed to modify 
> drm_mode_convert_umode, to filter out the aspect-ratio
> 
> info if user has not set aspect-ratio capability, but do we need a 
> similar change the drm_mode_convert_to_umode?

I think you maybe had those backwards.

drm_mode_convert_umode(), or more likely its relevant callers setcrtc
and setproperty, need to reject the mode if the client cap is not set.

drm_mode_convert_to_umode(), or rather its callers getblob and getcrtc,
need to filter out the flags.

> 
> If we do, I am not sure if it would be possible to have the file_priv to 
> /drm_atomic_set_mode_for_crtc/, which calls
> 
> drm_mode_convert_to_umode.  The function /drm_atomic_set_mode_for_crtc/ 
> is used to set mode, originating by kernel,
> 
> and make a blob from the kernel mode, which it saves in crtc_state.
> 
> This function /: drm_atomic_set_mode_for_crtc, /is called by several 
> helper functions and driver functions, and passing
> 
> file_priv from all these functions does not seem to be plausible.

I don't think we need to plumb it quite that deep. Doing the
check in drm_atomic_crtc_set_property() should be sufficient.

> 
> Any suggestions on how to handle this situation?
> 
> 
> Regards,
> 
> Ankit
> 
> 
> On 2/8/2018 9:59 AM, Nautiyal, Ankit K wrote:
> > Hi Ville,
> >
> > I still have some queries regarding the handling of aspect ratio flags 
> > in getblob ioctl.
> >
> > Please find below my responses inline.
> >
> >
> > On 2/1/2018 6:24 PM, Ville Syrjälä wrote:
> >> On Thu, Feb 01, 2018 at 04:35:22PM +0530, Nautiyal, Ankit K wrote:
> >>> Hi Ville,
> >>>
> >>> Appreciate your time and the suggestions.
> >>> Please find my response inline:
> >>>
> >>> On 1/31/2018 6:39 PM, Ville Syrjälä wrote:
> >>>> On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote:
> >>>>> On 1/30/2018 12:23 AM, Ville Syrjälä wrote:
> >>>>>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote:
> >>>>>>> From: Ankit Nautiyal <[e-mail]>
> >>>>>>>
> >>>>>>> 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 <[e-mail]>
> >>>>>>>
> >>>>>>> V3: Addressed review comments from Ville:
> >>>>>>> -Do not corrupt the current crtc state by updating aspect ratio
> >>>>>>> on the fly.
> >>>>>>> ---
> >>>>>>>     drivers/gpu/drm/drm_atomic.c | 61 
> >>>>>>> +++++++++++++++++++++++++++++++++++++++++---
> >>>>>>>     drivers/gpu/drm/drm_crtc.c   | 19 ++++++++++++++
> >>>>>>>     include/drm/drm_atomic.h     |  2 ++
> >>>>>>>     3 files changed, 79 insertions(+), 3 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c 
> >>>>>>> b/drivers/gpu/drm/drm_atomic.c
> >>>>>>> index 69ff763..39313e2 100644
> >>>>>>> --- a/drivers/gpu/drm/drm_atomic.c
> >>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
> >>>>>>> @@ -316,6 +316,35 @@ static s32 __user 
> >>>>>>> *get_out_fence_for_crtc(struct drm_atomic_state *state,
> >>>>>>>             return fence_ptr;
> >>>>>>>     }
> >>>>>>> +/**
> >>>>>>> + * drm_atomic_allow_aspect_ratio_for_kmode
> >>>>>>> + * @state: the crtc state
> >>>>>>> + * @mode: kernel-internal mode, which is used to create a 
> >>>>>>> duplicate mode,
> >>>>>>> + * with updated picture aspect ratio.
> >>>>>>> + *
> >>>>>>> + * Allow the aspect ratio info in the kernel mode only if 
> >>>>>>> aspect ratio is
> >>>>>>> + * supported.
> >>>>>>> + *
> >>>>>>> + * RETURNS:
> >>>>>>> + * kernel-internal mode with updated picture aspect ratio value.
> >>>>>>> + */
> >>>>>>> +
> >>>>>>> +struct drm_display_mode*
> >>>>>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state 
> >>>>>>> *state,
> >>>>>>> +                    const struct drm_display_mode *mode)
> >>>>>>> +{
> >>>>>>> +    struct drm_atomic_state *atomic_state = state->state;
> >>>>>>> +    struct drm_display_mode *ar_kmode;
> >>>>>>> +
> >>>>>>> +    ar_kmode = drm_mode_duplicate(state->crtc->dev, mode);
> >>>>>>> +    /*
> >>>>>>> +     * If aspect ratio is not supported, set the picture aspect 
> >>>>>>> ratio as
> >>>>>>> +     * NONE.
> >>>>>>> +     */
> >>>>>>> +    if (atomic_state && !atomic_state->allow_aspect_ratio)
> >>>>>>> +        ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >>>>>>> +    return ar_kmode;
> >>>>>>> +}
> >>>>>>>         /**
> >>>>>>>      * drm_atomic_set_mode_for_crtc - set mode for CRTC
> >>>>>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct 
> >>>>>>> drm_crtc_state *state,
> >>>>>>>         state->mode_blob = NULL;
> >>>>>>>             if (mode) {
> >>>>>>> -        drm_mode_convert_to_umode(&umode, mode);
> >>>>>>> +        struct drm_display_mode *ar_mode;
> >>>>>>> +
> >>>>>>> +        ar_mode = 
> >>>>>>> drm_atomic_allow_aspect_ratio_for_kmode(state, mode);
> >>>>>>> +        drm_mode_convert_to_umode(&umode, ar_mode);
> >>>>>> This still looks sketchy.
> >>>>>>
> >>>>>> I believe there are just two places where we need to filter out the
> >>>>>> aspect ratio flags from the umode, namely the getblob and getcrtc
> >>>>>> ioctls.
> >>>>> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob,
> >>>>> etc. apart from the mode blob.
> >>>>> Is there any way to check from blob id (or by any other means),
> >>>>> if the data is for the mode, or edid, or gamma etc.
> >>>> I think we'd have to somehow mark the mode blobs as special. Until we
> >>>> have more need for cleaning up blobs before returning them to 
> >>>> userspace
> >>>> I think a simple flag should do. If we come up with more uses like 
> >>>> this
> >>>> then it might make sense to introduce some kind of optional filter
> >>>> function for blobs.
> >>> If my understanding is correct,
> >>> 1. To be able to do this, we need to change in uapi.
> >> We should be fine with an internal flag. Obviously it won't be set
> >> for blobs freshly created by userspace, but that's fine because we
> >> do no other error checking for them either. The error checking will
> >> happen when the user tries to actually use the blob.
> >
> > I am not sure why getblob ioctl should even come into picture. 
> > (Perhaps I am missing something).
> > As per my understanding:
> > If a userspace needs to set a mode, it will just create a blob with 
> > drm_mode_mode_info,
> > and get the blob-id. This blob-id would be supplied to 
> > drm_mode_atomic_ioctl as a crtc
> > property mode_id.
> > At the kernel side,  the crtc property mode_id is set by looking-up 
> > for mode blob from blob id.
> > I am doing the change in the user mode aspect-ratio flags at this 
> > junction.
> >
> > As far as getblob ioctl is concerned, if the user does call getblob 
> > ioctl for getting mode blob from
> > the blob id, it will receive the same mode blob, with same aspect 
> > ratio info which the user had
> > created using create blob ioctl. If it does want to use the blob, it 
> > has to come through the
> > drm_mode_atomic_ioctl way, where aspect ratio flags are handled.
> >
> >>> 2. Whenever the userspace creates a blob for mode, the new flag/class
> >>> for blob type needs to be set.
> >>>
> >>> Do you think that it's worth the effort?
> >>>>> If we can check that, I can make sure that aspect-ratio flags are 
> >>>>> taken
> >>>>> care of, if the blob is for mode,
> >>>>> and the aspect ratio is not supported.
> >>>>>
> >>>>>> And for checking the user input I think we should probably just
> >>>>>> stick that into drm_mode_convert_umode(). Looks like we never call
> >>>>>> that from anywhere but the atomic/setprop and setcrtc paths with
> >>>>>> a non-NULL argument.
> >>>>>>
> >>>>>> I *think* those three places should be sufficient to cover 
> >>>>>> everything.
> >>>>> Agreed. Infact I tried to do that first, but the only problem we have
> >>>>> here, is that, the file_priv
> >>>>> which has the aspect ratio capability information is not supplied 
> >>>>> to the
> >>>>> drm_mode_convert_umode()
> >>>>> or the functions calling, drm_mode_conver_umode(). At best we have
> >>>>> drm_crtc_state.
> >>>> Simply passing the file_priv down would seem like the most obvious
> >>>> solution.
> >>> This means we have to make change in various drivers and we have to 
> >>> pull
> >>> the file_priv
> >>> parameter three levels down. Should we think of a better way for 
> >>> doing this?
> >> Coccinelle is pretty good at modifying function parameters.
> >
> > Thanks for pointing out Coccinelle, will defintely try that out.
> > But my question here is whether we really want to drag the file_priv
> > from drm_mode_atomic_ioctl --> drm_mode_atomic_set_property -->
> > drm_atomic_crtc_set_property --> drm_mode_set_mode_prop_for_crtc-->
> > drm_mode_convert_umode.
> >
> > Instead, I tried to add a new field (allow_aspect_ratio) in 
> > drm_crtc_state which is
> > already passed from the drm_mode_atomic_ioctl all the way to
> > drm_mode_convert_umode.
> >
> >
> > -Regards
> > Ankit
> >>
> >>>>> I have added an aspect ratio allowed flag  in 
> >>>>> drm_crtc_state->state so
> >>>>> that its available in the
> >>>>> functions calling drm_mode_convert_umode.
> >>>>>
> >>>>> I am open to suggestion to avoid adding a new flag in 
> >>>>> drm_atomic_state,
> >>>>> if there is a better
> >>>>> way to let the drm_mode_convert_umode( ) know that user supports 
> >>>>> aspect
> >>>>> ratio capability.
> >>>>>>>             state->mode_blob =
> >>>>>>> drm_property_create_blob(state->crtc->dev,
> >>>>>>> sizeof(umode),
> >>>>>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct 
> >>>>>>> drm_crtc_state *state,
> >>>>>>>             if (IS_ERR(state->mode_blob))
> >>>>>>>                 return PTR_ERR(state->mode_blob);
> >>>>>>>     -        drm_mode_copy(&state->mode, mode);
> >>>>>>> +        drm_mode_copy(&state->mode, ar_mode);
> >>>>>>>             state->enable = true;
> >>>>>>>             DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
> >>>>>>> -                 mode->name, state);
> >>>>>>> +                 ar_mode->name, state);
> >>>>>>> +        drm_mode_destroy(state->crtc->dev, ar_mode);
> >>>>>>>         } else {
> >>>>>>>             memset(&state->mode, 0, sizeof(state->mode));
> >>>>>>>             state->enable = false;
> >>>>>>> @@ -436,6 +469,25 @@ 
> >>>>>>> drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
> >>>>>>>     }
> >>>>>>>         /**
> >>>>>>> + * drm_atomic_allow_aspect_ratio_for_umode
> >>>>>>> + * @state: the crtc state
> >>>>>>> + * @umode: userspace mode, whose aspect ratio flag bits are to 
> >>>>>>> be updated.
> >>>>>>> + *
> >>>>>>> + * Allow the aspect ratio info in the userspace mode only if 
> >>>>>>> aspect ratio is
> >>>>>>> + * supported.
> >>>>>>> + */
> >>>>>>> +void
> >>>>>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state 
> >>>>>>> *state,
> >>>>>>> +                    struct drm_mode_modeinfo *umode)
> >>>>>>> +{
> >>>>>>> +    struct drm_atomic_state *atomic_state = state->state;
> >>>>>>> +
> >>>>>>> +    /* Reset the aspect ratio flags if aspect ratio is not 
> >>>>>>> supported */
> >>>>>>> +    if (atomic_state && !atomic_state->allow_aspect_ratio)
> >>>>>>> +        umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +/**
> >>>>>>>      * drm_atomic_crtc_set_property - set property on CRTC
> >>>>>>>      * @crtc: the drm CRTC to set a property on
> >>>>>>>      * @state: the state object to update with the new property 
> >>>>>>> value
> >>>>>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct 
> >>>>>>> drm_crtc *crtc,
> >>>>>>>         else if (property == config->prop_mode_id) {
> >>>>>>>             struct drm_property_blob *mode =
> >>>>>>>                 drm_property_lookup_blob(dev, val);
> >>>>>>> + drm_atomic_allow_aspect_ratio_for_umode(state,
> >>>>>>> +                    (struct drm_mode_modeinfo *)
> >>>>>>> +                    mode->data);
> >>>>>>>             ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
> >>>>>>>             drm_property_blob_put(mode);
> >>>>>>>             return ret;
> >>>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c 
> >>>>>>> b/drivers/gpu/drm/drm_crtc.c
> >>>>>>> index f0556e6..a2d34fa 100644
> >>>>>>> --- a/drivers/gpu/drm/drm_crtc.c
> >>>>>>> +++ b/drivers/gpu/drm/drm_crtc.c
> >>>>>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
> >>>>>>>         drm_modeset_lock(&crtc->mutex, NULL);
> >>>>>>>         if (crtc->state) {
> >>>>>>>             if (crtc->state->enable) {
> >>>>>>> +            /*
> >>>>>>> +             * If the aspect-ratio is not required by the,
> >>>>>>> +             * userspace, do not set the aspect-ratio flags.
> >>>>>>> +             */
> >>>>>>> +            if (!file_priv->aspect_ratio_allowed)
> >>>>>>> + crtc->state->mode.picture_aspect_ratio =
> >>>>>>> +                    HDMI_PICTURE_ASPECT_NONE;
> >>>>>> These would still clobber the current crtc state. So definitely 
> >>>>>> don't
> >>>>>> want to do this.
> >>>>> Alright, so alternative way of doing this will be:
> >>>>> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel
> >>>>> mode structure,
> >>>>> but set the aspect ratio flags in the usermode to none, after 
> >>>>> calling the
> >>>>> drm_mode_convert_to_umode().
> >>>>>
> >>>>> Will take care of this in the next patchset.
> >>> I gave a thought about it again. As you have mentioned earlier that
> >>> get_crtc is one of the
> >>> place where the user mode aspect ratio flags must be filtered out, is
> >>> this what you have
> >>> in mind. I hope I did not misunderstand.
> >>>
> >>> Changing the usermode aspect ratio flag bits, without change in the
> >>> picture_aspect_ratio
> >>> kernel mode will not result in both structures out of sync?
> >> I don't think that should really matter. I suppose we might get one
> >> extra modeset when the user feeds that mode back via setcrtc/atomic, but
> >> meh. And actually the aspect ratio should only affect the infoframe, so
> >> we should be able to eventually eliminate that extra modeset and just
> >> update the infoframe instead.
> >>
> >>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
> >>>>>>>                 crtc_resp->mode_valid = 1;
> >>>>>>>     @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device 
> >>>>>>> *dev,
> >>>>>>>             crtc_resp->x = crtc->x;
> >>>>>>>             crtc_resp->y = crtc->y;
> >>>>>>>             if (crtc->enabled) {
> >>>>>>> +            /*
> >>>>>>> +             * If the aspect-ratio is not required by the,
> >>>>>>> +             * userspace, do not set the aspect-ratio flags.
> >>>>>>> +             */
> >>>>>>> +            if (!file_priv->aspect_ratio_allowed)
> >>>>>>> +                crtc->mode.picture_aspect_ratio =
> >>>>>>> +                    HDMI_PICTURE_ASPECT_NONE;
> >>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
> >>>>>>>                 crtc_resp->mode_valid = 1;
> >>>>>>>     @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device 
> >>>>>>> *dev, void *data,
> >>>>>>>                 goto out;
> >>>>>>>             }
> >>>>>>>     +        /* If the user does not ask for aspect ratio, reset 
> >>>>>>> the aspect
> >>>>>>> +         * ratio bits in the usermode.
> >>>>>>> +         */
> >>>>>>> +        if (!file_priv->aspect_ratio_allowed)
> >>>>>>> +            crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
> >>>>>>>             ret = drm_mode_convert_umode(mode, &crtc_req->mode);
> >>>>>>>             if (ret) {
> >>>>>>>                 DRM_DEBUG_KMS("Invalid mode\n");
> >>>>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> >>>>>>> index 1c27526..130dad9 100644
> >>>>>>> --- a/include/drm/drm_atomic.h
> >>>>>>> +++ b/include/drm/drm_atomic.h
> >>>>>>> @@ -237,6 +237,7 @@ struct __drm_private_objs_state {
> >>>>>>>      * @ref: count of all references to this state (will not be 
> >>>>>>> freed until zero)
> >>>>>>>      * @dev: parent DRM device
> >>>>>>>      * @allow_modeset: allow full modeset
> >>>>>>> + * @allow_aspect_ratio: allow the aspect ratio info to be 
> >>>>>>> passes to user
> >>>>>>>      * @legacy_cursor_update: hint to enforce legacy cursor 
> >>>>>>> IOCTL semantics
> >>>>>>>      * @async_update: hint for asynchronous plane update
> >>>>>>>      * @planes: pointer to array of structures with per-plane data
> >>>>>>> @@ -256,6 +257,7 @@ struct drm_atomic_state {
> >>>>>>>             struct drm_device *dev;
> >>>>>>>         bool allow_modeset : 1;
> >>>>>>> +    bool allow_aspect_ratio : 1;
> >>>>>>>         bool legacy_cursor_update : 1;
> >>>>>>>         bool async_update : 1;
> >>>>>>>         struct __drm_planes_state *planes;
> >>>>>>> -- 
> >>>>>>> 2.7.4
> >>>>>>>
> >>>>>>> _______________________________________________
> >>>>>>> dri-devel mailing list
> >>>>>>> [e-mail]
> >>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>> Regards,
> >>> Ankit
> >
>

Nautiyal, Ankit K Feb. 13, 2018, 4:23 p.m. UTC | #9

Hi Ville,

Thanks yet again to look into this.

I am still skeptical about rejecting the mode, if aspect ratio cap is 
not set.
Perhaps I am not aware with the userspace expectations.

Please find my response inline:

On 2/13/2018 6:48 PM, Ville Syrjälä wrote:
> On Tue, Feb 13, 2018 at 10:21:15AM +0530, Nautiyal, Ankit K wrote:
>> Hi Ville,
>>
>> As per our last discussion, following points were discussed:
>>
>> 1. To suppress the aspect-ratio info from getblob ioctl to a user that
>> does not support it:
>>
>>       i. A new flag must be added to drm_blob_property to mark if the
>> blob has mode data.
>>
>>       ii. This flag must be set when the user tries do an atomic modeset.
>>
>>       iii. In the getblob ioctl, based on the above flag, it can be
>> determined that if the blob
>>
>>       has mode data, and the aspect ratio info can be suppressed in
>> getblob ioctl, if user does not support it.
>>
>> 2. Instead of adding aspect ratio capabilities in drm_atomic_state, pass
>> on the file_priv which already has
>>
>> the required information to the function drm_mode_convert_umode.
>>
>> It will require addition of an new argument file_priv to several
>> functions, but that is right thing to do,
>>
>> as file_priv is the correct structure for the capability information.
>>
>> 3. Changing the usermode aspect ratio flag bits, without change in the
>> picture_aspect_ratio would not matter, and does not need to be handled
>> in this patch.
>>
>>
>> I just have one query here. We have agreed to modify
>> drm_mode_convert_umode, to filter out the aspect-ratio
>>
>> info if user has not set aspect-ratio capability, but do we need a
>> similar change the drm_mode_convert_to_umode?
> I think you maybe had those backwards.
>
> drm_mode_convert_umode(), or more likely its relevant callers setcrtc
> and setproperty, need to reject the mode if the client cap is not set.
I guess, rejecting the mode, altogether can break the existing user-spaces.
Current user-spaces, oblivious of the aspect-ratio capability in drm, 
will not set the aspect-ratio capability.
Some of them, might have some garbage value in the aspect ratio bits of 
the user mode. If we reject such
modes, the user-spaces that were earlier working will break.
(But if we are sure, that the aspect ratio flag bits will all be reset, 
then it makes sense to reject the mode.)

Instead of rejecting such modes, if we just only ignore the aspect ratio 
bits in such cases, and carry on with the
modeset for the given user mode, the behaviour would be intact, just as 
prior to the aspect ratio changes.

> drm_mode_convert_to_umode(), or rather its callers getblob and getcrtc,
> need to filter out the flags.

Yes right. I'll be filtering out the flags in the getblob and getcrtc 
functions.

Thanks & Regards,
Ankit
>> If we do, I am not sure if it would be possible to have the file_priv to
>> /drm_atomic_set_mode_for_crtc/, which calls
>>
>> drm_mode_convert_to_umode.  The function /drm_atomic_set_mode_for_crtc/
>> is used to set mode, originating by kernel,
>>
>> and make a blob from the kernel mode, which it saves in crtc_state.
>>
>> This function /: drm_atomic_set_mode_for_crtc, /is called by several
>> helper functions and driver functions, and passing
>>
>> file_priv from all these functions does not seem to be plausible.
> I don't think we need to plumb it quite that deep. Doing the
> check in drm_atomic_crtc_set_property() should be sufficient.
>
>> Any suggestions on how to handle this situation?
>>
>>
>> Regards,
>>
>> Ankit
>>
>>
>> On 2/8/2018 9:59 AM, Nautiyal, Ankit K wrote:
>>> Hi Ville,
>>>
>>> I still have some queries regarding the handling of aspect ratio flags
>>> in getblob ioctl.
>>>
>>> Please find below my responses inline.
>>>
>>>
>>> On 2/1/2018 6:24 PM, Ville Syrjälä wrote:
>>>> On Thu, Feb 01, 2018 at 04:35:22PM +0530, Nautiyal, Ankit K wrote:
>>>>> Hi Ville,
>>>>>
>>>>> Appreciate your time and the suggestions.
>>>>> Please find my response inline:
>>>>>
>>>>> On 1/31/2018 6:39 PM, Ville Syrjälä wrote:
>>>>>> On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote:
>>>>>>> On 1/30/2018 12:23 AM, Ville Syrjälä wrote:
>>>>>>>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote:
>>>>>>>>> From: Ankit Nautiyal <[e-mail]>
>>>>>>>>>
>>>>>>>>> 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 <[e-mail]>
>>>>>>>>>
>>>>>>>>> V3: Addressed review comments from Ville:
>>>>>>>>> -Do not corrupt the current crtc state by updating aspect ratio
>>>>>>>>> on the fly.
>>>>>>>>> ---
>>>>>>>>>      drivers/gpu/drm/drm_atomic.c | 61
>>>>>>>>> +++++++++++++++++++++++++++++++++++++++++---
>>>>>>>>>      drivers/gpu/drm/drm_crtc.c   | 19 ++++++++++++++
>>>>>>>>>      include/drm/drm_atomic.h     |  2 ++
>>>>>>>>>      3 files changed, 79 insertions(+), 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c
>>>>>>>>> b/drivers/gpu/drm/drm_atomic.c
>>>>>>>>> index 69ff763..39313e2 100644
>>>>>>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>>>>>>> @@ -316,6 +316,35 @@ static s32 __user
>>>>>>>>> *get_out_fence_for_crtc(struct drm_atomic_state *state,
>>>>>>>>>              return fence_ptr;
>>>>>>>>>      }
>>>>>>>>> +/**
>>>>>>>>> + * drm_atomic_allow_aspect_ratio_for_kmode
>>>>>>>>> + * @state: the crtc state
>>>>>>>>> + * @mode: kernel-internal mode, which is used to create a
>>>>>>>>> duplicate mode,
>>>>>>>>> + * with updated picture aspect ratio.
>>>>>>>>> + *
>>>>>>>>> + * Allow the aspect ratio info in the kernel mode only if
>>>>>>>>> aspect ratio is
>>>>>>>>> + * supported.
>>>>>>>>> + *
>>>>>>>>> + * RETURNS:
>>>>>>>>> + * kernel-internal mode with updated picture aspect ratio value.
>>>>>>>>> + */
>>>>>>>>> +
>>>>>>>>> +struct drm_display_mode*
>>>>>>>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state
>>>>>>>>> *state,
>>>>>>>>> +                    const struct drm_display_mode *mode)
>>>>>>>>> +{
>>>>>>>>> +    struct drm_atomic_state *atomic_state = state->state;
>>>>>>>>> +    struct drm_display_mode *ar_kmode;
>>>>>>>>> +
>>>>>>>>> +    ar_kmode = drm_mode_duplicate(state->crtc->dev, mode);
>>>>>>>>> +    /*
>>>>>>>>> +     * If aspect ratio is not supported, set the picture aspect
>>>>>>>>> ratio as
>>>>>>>>> +     * NONE.
>>>>>>>>> +     */
>>>>>>>>> +    if (atomic_state && !atomic_state->allow_aspect_ratio)
>>>>>>>>> +        ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>>>>>>>> +    return ar_kmode;
>>>>>>>>> +}
>>>>>>>>>          /**
>>>>>>>>>       * drm_atomic_set_mode_for_crtc - set mode for CRTC
>>>>>>>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct
>>>>>>>>> drm_crtc_state *state,
>>>>>>>>>          state->mode_blob = NULL;
>>>>>>>>>              if (mode) {
>>>>>>>>> -        drm_mode_convert_to_umode(&umode, mode);
>>>>>>>>> +        struct drm_display_mode *ar_mode;
>>>>>>>>> +
>>>>>>>>> +        ar_mode =
>>>>>>>>> drm_atomic_allow_aspect_ratio_for_kmode(state, mode);
>>>>>>>>> +        drm_mode_convert_to_umode(&umode, ar_mode);
>>>>>>>> This still looks sketchy.
>>>>>>>>
>>>>>>>> I believe there are just two places where we need to filter out the
>>>>>>>> aspect ratio flags from the umode, namely the getblob and getcrtc
>>>>>>>> ioctls.
>>>>>>> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob,
>>>>>>> etc. apart from the mode blob.
>>>>>>> Is there any way to check from blob id (or by any other means),
>>>>>>> if the data is for the mode, or edid, or gamma etc.
>>>>>> I think we'd have to somehow mark the mode blobs as special. Until we
>>>>>> have more need for cleaning up blobs before returning them to
>>>>>> userspace
>>>>>> I think a simple flag should do. If we come up with more uses like
>>>>>> this
>>>>>> then it might make sense to introduce some kind of optional filter
>>>>>> function for blobs.
>>>>> If my understanding is correct,
>>>>> 1. To be able to do this, we need to change in uapi.
>>>> We should be fine with an internal flag. Obviously it won't be set
>>>> for blobs freshly created by userspace, but that's fine because we
>>>> do no other error checking for them either. The error checking will
>>>> happen when the user tries to actually use the blob.
>>> I am not sure why getblob ioctl should even come into picture.
>>> (Perhaps I am missing something).
>>> As per my understanding:
>>> If a userspace needs to set a mode, it will just create a blob with
>>> drm_mode_mode_info,
>>> and get the blob-id. This blob-id would be supplied to
>>> drm_mode_atomic_ioctl as a crtc
>>> property mode_id.
>>> At the kernel side,  the crtc property mode_id is set by looking-up
>>> for mode blob from blob id.
>>> I am doing the change in the user mode aspect-ratio flags at this
>>> junction.
>>>
>>> As far as getblob ioctl is concerned, if the user does call getblob
>>> ioctl for getting mode blob from
>>> the blob id, it will receive the same mode blob, with same aspect
>>> ratio info which the user had
>>> created using create blob ioctl. If it does want to use the blob, it
>>> has to come through the
>>> drm_mode_atomic_ioctl way, where aspect ratio flags are handled.
>>>
>>>>> 2. Whenever the userspace creates a blob for mode, the new flag/class
>>>>> for blob type needs to be set.
>>>>>
>>>>> Do you think that it's worth the effort?
>>>>>>> If we can check that, I can make sure that aspect-ratio flags are
>>>>>>> taken
>>>>>>> care of, if the blob is for mode,
>>>>>>> and the aspect ratio is not supported.
>>>>>>>
>>>>>>>> And for checking the user input I think we should probably just
>>>>>>>> stick that into drm_mode_convert_umode(). Looks like we never call
>>>>>>>> that from anywhere but the atomic/setprop and setcrtc paths with
>>>>>>>> a non-NULL argument.
>>>>>>>>
>>>>>>>> I *think* those three places should be sufficient to cover
>>>>>>>> everything.
>>>>>>> Agreed. Infact I tried to do that first, but the only problem we have
>>>>>>> here, is that, the file_priv
>>>>>>> which has the aspect ratio capability information is not supplied
>>>>>>> to the
>>>>>>> drm_mode_convert_umode()
>>>>>>> or the functions calling, drm_mode_conver_umode(). At best we have
>>>>>>> drm_crtc_state.
>>>>>> Simply passing the file_priv down would seem like the most obvious
>>>>>> solution.
>>>>> This means we have to make change in various drivers and we have to
>>>>> pull
>>>>> the file_priv
>>>>> parameter three levels down. Should we think of a better way for
>>>>> doing this?
>>>> Coccinelle is pretty good at modifying function parameters.
>>> Thanks for pointing out Coccinelle, will defintely try that out.
>>> But my question here is whether we really want to drag the file_priv
>>> from drm_mode_atomic_ioctl --> drm_mode_atomic_set_property -->
>>> drm_atomic_crtc_set_property --> drm_mode_set_mode_prop_for_crtc-->
>>> drm_mode_convert_umode.
>>>
>>> Instead, I tried to add a new field (allow_aspect_ratio) in
>>> drm_crtc_state which is
>>> already passed from the drm_mode_atomic_ioctl all the way to
>>> drm_mode_convert_umode.
>>>
>>>
>>> -Regards
>>> Ankit
>>>>>>> I have added an aspect ratio allowed flag  in
>>>>>>> drm_crtc_state->state so
>>>>>>> that its available in the
>>>>>>> functions calling drm_mode_convert_umode.
>>>>>>>
>>>>>>> I am open to suggestion to avoid adding a new flag in
>>>>>>> drm_atomic_state,
>>>>>>> if there is a better
>>>>>>> way to let the drm_mode_convert_umode( ) know that user supports
>>>>>>> aspect
>>>>>>> ratio capability.
>>>>>>>>>              state->mode_blob =
>>>>>>>>> drm_property_create_blob(state->crtc->dev,
>>>>>>>>> sizeof(umode),
>>>>>>>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct
>>>>>>>>> drm_crtc_state *state,
>>>>>>>>>              if (IS_ERR(state->mode_blob))
>>>>>>>>>                  return PTR_ERR(state->mode_blob);
>>>>>>>>>      -        drm_mode_copy(&state->mode, mode);
>>>>>>>>> +        drm_mode_copy(&state->mode, ar_mode);
>>>>>>>>>              state->enable = true;
>>>>>>>>>              DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
>>>>>>>>> -                 mode->name, state);
>>>>>>>>> +                 ar_mode->name, state);
>>>>>>>>> +        drm_mode_destroy(state->crtc->dev, ar_mode);
>>>>>>>>>          } else {
>>>>>>>>>              memset(&state->mode, 0, sizeof(state->mode));
>>>>>>>>>              state->enable = false;
>>>>>>>>> @@ -436,6 +469,25 @@
>>>>>>>>> drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
>>>>>>>>>      }
>>>>>>>>>          /**
>>>>>>>>> + * drm_atomic_allow_aspect_ratio_for_umode
>>>>>>>>> + * @state: the crtc state
>>>>>>>>> + * @umode: userspace mode, whose aspect ratio flag bits are to
>>>>>>>>> be updated.
>>>>>>>>> + *
>>>>>>>>> + * Allow the aspect ratio info in the userspace mode only if
>>>>>>>>> aspect ratio is
>>>>>>>>> + * supported.
>>>>>>>>> + */
>>>>>>>>> +void
>>>>>>>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state
>>>>>>>>> *state,
>>>>>>>>> +                    struct drm_mode_modeinfo *umode)
>>>>>>>>> +{
>>>>>>>>> +    struct drm_atomic_state *atomic_state = state->state;
>>>>>>>>> +
>>>>>>>>> +    /* Reset the aspect ratio flags if aspect ratio is not
>>>>>>>>> supported */
>>>>>>>>> +    if (atomic_state && !atomic_state->allow_aspect_ratio)
>>>>>>>>> +        umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +/**
>>>>>>>>>       * drm_atomic_crtc_set_property - set property on CRTC
>>>>>>>>>       * @crtc: the drm CRTC to set a property on
>>>>>>>>>       * @state: the state object to update with the new property
>>>>>>>>> value
>>>>>>>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct
>>>>>>>>> drm_crtc *crtc,
>>>>>>>>>          else if (property == config->prop_mode_id) {
>>>>>>>>>              struct drm_property_blob *mode =
>>>>>>>>>                  drm_property_lookup_blob(dev, val);
>>>>>>>>> + drm_atomic_allow_aspect_ratio_for_umode(state,
>>>>>>>>> +                    (struct drm_mode_modeinfo *)
>>>>>>>>> +                    mode->data);
>>>>>>>>>              ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>>>>>>>>>              drm_property_blob_put(mode);
>>>>>>>>>              return ret;
>>>>>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c
>>>>>>>>> b/drivers/gpu/drm/drm_crtc.c
>>>>>>>>> index f0556e6..a2d34fa 100644
>>>>>>>>> --- a/drivers/gpu/drm/drm_crtc.c
>>>>>>>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>>>>>>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
>>>>>>>>>          drm_modeset_lock(&crtc->mutex, NULL);
>>>>>>>>>          if (crtc->state) {
>>>>>>>>>              if (crtc->state->enable) {
>>>>>>>>> +            /*
>>>>>>>>> +             * If the aspect-ratio is not required by the,
>>>>>>>>> +             * userspace, do not set the aspect-ratio flags.
>>>>>>>>> +             */
>>>>>>>>> +            if (!file_priv->aspect_ratio_allowed)
>>>>>>>>> + crtc->state->mode.picture_aspect_ratio =
>>>>>>>>> +                    HDMI_PICTURE_ASPECT_NONE;
>>>>>>>> These would still clobber the current crtc state. So definitely
>>>>>>>> don't
>>>>>>>> want to do this.
>>>>>>> Alright, so alternative way of doing this will be:
>>>>>>> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel
>>>>>>> mode structure,
>>>>>>> but set the aspect ratio flags in the usermode to none, after
>>>>>>> calling the
>>>>>>> drm_mode_convert_to_umode().
>>>>>>>
>>>>>>> Will take care of this in the next patchset.
>>>>> I gave a thought about it again. As you have mentioned earlier that
>>>>> get_crtc is one of the
>>>>> place where the user mode aspect ratio flags must be filtered out, is
>>>>> this what you have
>>>>> in mind. I hope I did not misunderstand.
>>>>>
>>>>> Changing the usermode aspect ratio flag bits, without change in the
>>>>> picture_aspect_ratio
>>>>> kernel mode will not result in both structures out of sync?
>>>> I don't think that should really matter. I suppose we might get one
>>>> extra modeset when the user feeds that mode back via setcrtc/atomic, but
>>>> meh. And actually the aspect ratio should only affect the infoframe, so
>>>> we should be able to eventually eliminate that extra modeset and just
>>>> update the infoframe instead.
>>>>
>>>>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
>>>>>>>>>                  crtc_resp->mode_valid = 1;
>>>>>>>>>      @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device
>>>>>>>>> *dev,
>>>>>>>>>              crtc_resp->x = crtc->x;
>>>>>>>>>              crtc_resp->y = crtc->y;
>>>>>>>>>              if (crtc->enabled) {
>>>>>>>>> +            /*
>>>>>>>>> +             * If the aspect-ratio is not required by the,
>>>>>>>>> +             * userspace, do not set the aspect-ratio flags.
>>>>>>>>> +             */
>>>>>>>>> +            if (!file_priv->aspect_ratio_allowed)
>>>>>>>>> +                crtc->mode.picture_aspect_ratio =
>>>>>>>>> +                    HDMI_PICTURE_ASPECT_NONE;
>>>>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
>>>>>>>>>                  crtc_resp->mode_valid = 1;
>>>>>>>>>      @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device
>>>>>>>>> *dev, void *data,
>>>>>>>>>                  goto out;
>>>>>>>>>              }
>>>>>>>>>      +        /* If the user does not ask for aspect ratio, reset
>>>>>>>>> the aspect
>>>>>>>>> +         * ratio bits in the usermode.
>>>>>>>>> +         */
>>>>>>>>> +        if (!file_priv->aspect_ratio_allowed)
>>>>>>>>> +            crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
>>>>>>>>>              ret = drm_mode_convert_umode(mode, &crtc_req->mode);
>>>>>>>>>              if (ret) {
>>>>>>>>>                  DRM_DEBUG_KMS("Invalid mode\n");
>>>>>>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>>>>>>>>> index 1c27526..130dad9 100644
>>>>>>>>> --- a/include/drm/drm_atomic.h
>>>>>>>>> +++ b/include/drm/drm_atomic.h
>>>>>>>>> @@ -237,6 +237,7 @@ struct __drm_private_objs_state {
>>>>>>>>>       * @ref: count of all references to this state (will not be
>>>>>>>>> freed until zero)
>>>>>>>>>       * @dev: parent DRM device
>>>>>>>>>       * @allow_modeset: allow full modeset
>>>>>>>>> + * @allow_aspect_ratio: allow the aspect ratio info to be
>>>>>>>>> passes to user
>>>>>>>>>       * @legacy_cursor_update: hint to enforce legacy cursor
>>>>>>>>> IOCTL semantics
>>>>>>>>>       * @async_update: hint for asynchronous plane update
>>>>>>>>>       * @planes: pointer to array of structures with per-plane data
>>>>>>>>> @@ -256,6 +257,7 @@ struct drm_atomic_state {
>>>>>>>>>              struct drm_device *dev;
>>>>>>>>>          bool allow_modeset : 1;
>>>>>>>>> +    bool allow_aspect_ratio : 1;
>>>>>>>>>          bool legacy_cursor_update : 1;
>>>>>>>>>          bool async_update : 1;
>>>>>>>>>          struct __drm_planes_state *planes;
>>>>>>>>> -- 
>>>>>>>>> 2.7.4
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> dri-devel mailing list
>>>>>>>>> [e-mail]
>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>> Regards,
>>>>> Ankit

Ville Syrjälä Feb. 13, 2018, 4:45 p.m. UTC | #10

On Tue, Feb 13, 2018 at 09:53:53PM +0530, Nautiyal, Ankit K wrote:
> Hi Ville,
> 
> Thanks yet again to look into this.
> 
> I am still skeptical about rejecting the mode, if aspect ratio cap is 
> not set.
> Perhaps I am not aware with the userspace expectations.
> 
> Please find my response inline:
> 
> On 2/13/2018 6:48 PM, Ville Syrjälä wrote:
> > On Tue, Feb 13, 2018 at 10:21:15AM +0530, Nautiyal, Ankit K wrote:
> >> Hi Ville,
> >>
> >> As per our last discussion, following points were discussed:
> >>
> >> 1. To suppress the aspect-ratio info from getblob ioctl to a user that
> >> does not support it:
> >>
> >>       i. A new flag must be added to drm_blob_property to mark if the
> >> blob has mode data.
> >>
> >>       ii. This flag must be set when the user tries do an atomic modeset.
> >>
> >>       iii. In the getblob ioctl, based on the above flag, it can be
> >> determined that if the blob
> >>
> >>       has mode data, and the aspect ratio info can be suppressed in
> >> getblob ioctl, if user does not support it.
> >>
> >> 2. Instead of adding aspect ratio capabilities in drm_atomic_state, pass
> >> on the file_priv which already has
> >>
> >> the required information to the function drm_mode_convert_umode.
> >>
> >> It will require addition of an new argument file_priv to several
> >> functions, but that is right thing to do,
> >>
> >> as file_priv is the correct structure for the capability information.
> >>
> >> 3. Changing the usermode aspect ratio flag bits, without change in the
> >> picture_aspect_ratio would not matter, and does not need to be handled
> >> in this patch.
> >>
> >>
> >> I just have one query here. We have agreed to modify
> >> drm_mode_convert_umode, to filter out the aspect-ratio
> >>
> >> info if user has not set aspect-ratio capability, but do we need a
> >> similar change the drm_mode_convert_to_umode?
> > I think you maybe had those backwards.
> >
> > drm_mode_convert_umode(), or more likely its relevant callers setcrtc
> > and setproperty, need to reject the mode if the client cap is not set.
> I guess, rejecting the mode, altogether can break the existing user-spaces.
> Current user-spaces, oblivious of the aspect-ratio capability in drm, 
> will not set the aspect-ratio capability.
> Some of them, might have some garbage value in the aspect ratio bits of 
> the user mode. If we reject such
> modes, the user-spaces that were earlier working will break.
> (But if we are sure, that the aspect ratio flag bits will all be reset, 
> then it makes sense to reject the mode.)

We already reject all unspecified flags.

> 
> Instead of rejecting such modes, if we just only ignore the aspect ratio 
> bits in such cases, and carry on with the
> modeset for the given user mode, the behaviour would be intact, just as 
> prior to the aspect ratio changes.
> 
> > drm_mode_convert_to_umode(), or rather its callers getblob and getcrtc,
> > need to filter out the flags.
> 
> Yes right. I'll be filtering out the flags in the getblob and getcrtc 
> functions.
> 
> Thanks & Regards,
> Ankit
> >> If we do, I am not sure if it would be possible to have the file_priv to
> >> /drm_atomic_set_mode_for_crtc/, which calls
> >>
> >> drm_mode_convert_to_umode.  The function /drm_atomic_set_mode_for_crtc/
> >> is used to set mode, originating by kernel,
> >>
> >> and make a blob from the kernel mode, which it saves in crtc_state.
> >>
> >> This function /: drm_atomic_set_mode_for_crtc, /is called by several
> >> helper functions and driver functions, and passing
> >>
> >> file_priv from all these functions does not seem to be plausible.
> > I don't think we need to plumb it quite that deep. Doing the
> > check in drm_atomic_crtc_set_property() should be sufficient.
> >
> >> Any suggestions on how to handle this situation?
> >>
> >>
> >> Regards,
> >>
> >> Ankit
> >>
> >>
> >> On 2/8/2018 9:59 AM, Nautiyal, Ankit K wrote:
> >>> Hi Ville,
> >>>
> >>> I still have some queries regarding the handling of aspect ratio flags
> >>> in getblob ioctl.
> >>>
> >>> Please find below my responses inline.
> >>>
> >>>
> >>> On 2/1/2018 6:24 PM, Ville Syrjälä wrote:
> >>>> On Thu, Feb 01, 2018 at 04:35:22PM +0530, Nautiyal, Ankit K wrote:
> >>>>> Hi Ville,
> >>>>>
> >>>>> Appreciate your time and the suggestions.
> >>>>> Please find my response inline:
> >>>>>
> >>>>> On 1/31/2018 6:39 PM, Ville Syrjälä wrote:
> >>>>>> On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote:
> >>>>>>> On 1/30/2018 12:23 AM, Ville Syrjälä wrote:
> >>>>>>>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote:
> >>>>>>>>> From: Ankit Nautiyal <[e-mail]>
> >>>>>>>>>
> >>>>>>>>> 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 <[e-mail]>
> >>>>>>>>>
> >>>>>>>>> V3: Addressed review comments from Ville:
> >>>>>>>>> -Do not corrupt the current crtc state by updating aspect ratio
> >>>>>>>>> on the fly.
> >>>>>>>>> ---
> >>>>>>>>>      drivers/gpu/drm/drm_atomic.c | 61
> >>>>>>>>> +++++++++++++++++++++++++++++++++++++++++---
> >>>>>>>>>      drivers/gpu/drm/drm_crtc.c   | 19 ++++++++++++++
> >>>>>>>>>      include/drm/drm_atomic.h     |  2 ++
> >>>>>>>>>      3 files changed, 79 insertions(+), 3 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c
> >>>>>>>>> b/drivers/gpu/drm/drm_atomic.c
> >>>>>>>>> index 69ff763..39313e2 100644
> >>>>>>>>> --- a/drivers/gpu/drm/drm_atomic.c
> >>>>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
> >>>>>>>>> @@ -316,6 +316,35 @@ static s32 __user
> >>>>>>>>> *get_out_fence_for_crtc(struct drm_atomic_state *state,
> >>>>>>>>>              return fence_ptr;
> >>>>>>>>>      }
> >>>>>>>>> +/**
> >>>>>>>>> + * drm_atomic_allow_aspect_ratio_for_kmode
> >>>>>>>>> + * @state: the crtc state
> >>>>>>>>> + * @mode: kernel-internal mode, which is used to create a
> >>>>>>>>> duplicate mode,
> >>>>>>>>> + * with updated picture aspect ratio.
> >>>>>>>>> + *
> >>>>>>>>> + * Allow the aspect ratio info in the kernel mode only if
> >>>>>>>>> aspect ratio is
> >>>>>>>>> + * supported.
> >>>>>>>>> + *
> >>>>>>>>> + * RETURNS:
> >>>>>>>>> + * kernel-internal mode with updated picture aspect ratio value.
> >>>>>>>>> + */
> >>>>>>>>> +
> >>>>>>>>> +struct drm_display_mode*
> >>>>>>>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state
> >>>>>>>>> *state,
> >>>>>>>>> +                    const struct drm_display_mode *mode)
> >>>>>>>>> +{
> >>>>>>>>> +    struct drm_atomic_state *atomic_state = state->state;
> >>>>>>>>> +    struct drm_display_mode *ar_kmode;
> >>>>>>>>> +
> >>>>>>>>> +    ar_kmode = drm_mode_duplicate(state->crtc->dev, mode);
> >>>>>>>>> +    /*
> >>>>>>>>> +     * If aspect ratio is not supported, set the picture aspect
> >>>>>>>>> ratio as
> >>>>>>>>> +     * NONE.
> >>>>>>>>> +     */
> >>>>>>>>> +    if (atomic_state && !atomic_state->allow_aspect_ratio)
> >>>>>>>>> +        ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >>>>>>>>> +    return ar_kmode;
> >>>>>>>>> +}
> >>>>>>>>>          /**
> >>>>>>>>>       * drm_atomic_set_mode_for_crtc - set mode for CRTC
> >>>>>>>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct
> >>>>>>>>> drm_crtc_state *state,
> >>>>>>>>>          state->mode_blob = NULL;
> >>>>>>>>>              if (mode) {
> >>>>>>>>> -        drm_mode_convert_to_umode(&umode, mode);
> >>>>>>>>> +        struct drm_display_mode *ar_mode;
> >>>>>>>>> +
> >>>>>>>>> +        ar_mode =
> >>>>>>>>> drm_atomic_allow_aspect_ratio_for_kmode(state, mode);
> >>>>>>>>> +        drm_mode_convert_to_umode(&umode, ar_mode);
> >>>>>>>> This still looks sketchy.
> >>>>>>>>
> >>>>>>>> I believe there are just two places where we need to filter out the
> >>>>>>>> aspect ratio flags from the umode, namely the getblob and getcrtc
> >>>>>>>> ioctls.
> >>>>>>> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob,
> >>>>>>> etc. apart from the mode blob.
> >>>>>>> Is there any way to check from blob id (or by any other means),
> >>>>>>> if the data is for the mode, or edid, or gamma etc.
> >>>>>> I think we'd have to somehow mark the mode blobs as special. Until we
> >>>>>> have more need for cleaning up blobs before returning them to
> >>>>>> userspace
> >>>>>> I think a simple flag should do. If we come up with more uses like
> >>>>>> this
> >>>>>> then it might make sense to introduce some kind of optional filter
> >>>>>> function for blobs.
> >>>>> If my understanding is correct,
> >>>>> 1. To be able to do this, we need to change in uapi.
> >>>> We should be fine with an internal flag. Obviously it won't be set
> >>>> for blobs freshly created by userspace, but that's fine because we
> >>>> do no other error checking for them either. The error checking will
> >>>> happen when the user tries to actually use the blob.
> >>> I am not sure why getblob ioctl should even come into picture.
> >>> (Perhaps I am missing something).
> >>> As per my understanding:
> >>> If a userspace needs to set a mode, it will just create a blob with
> >>> drm_mode_mode_info,
> >>> and get the blob-id. This blob-id would be supplied to
> >>> drm_mode_atomic_ioctl as a crtc
> >>> property mode_id.
> >>> At the kernel side,  the crtc property mode_id is set by looking-up
> >>> for mode blob from blob id.
> >>> I am doing the change in the user mode aspect-ratio flags at this
> >>> junction.
> >>>
> >>> As far as getblob ioctl is concerned, if the user does call getblob
> >>> ioctl for getting mode blob from
> >>> the blob id, it will receive the same mode blob, with same aspect
> >>> ratio info which the user had
> >>> created using create blob ioctl. If it does want to use the blob, it
> >>> has to come through the
> >>> drm_mode_atomic_ioctl way, where aspect ratio flags are handled.
> >>>
> >>>>> 2. Whenever the userspace creates a blob for mode, the new flag/class
> >>>>> for blob type needs to be set.
> >>>>>
> >>>>> Do you think that it's worth the effort?
> >>>>>>> If we can check that, I can make sure that aspect-ratio flags are
> >>>>>>> taken
> >>>>>>> care of, if the blob is for mode,
> >>>>>>> and the aspect ratio is not supported.
> >>>>>>>
> >>>>>>>> And for checking the user input I think we should probably just
> >>>>>>>> stick that into drm_mode_convert_umode(). Looks like we never call
> >>>>>>>> that from anywhere but the atomic/setprop and setcrtc paths with
> >>>>>>>> a non-NULL argument.
> >>>>>>>>
> >>>>>>>> I *think* those three places should be sufficient to cover
> >>>>>>>> everything.
> >>>>>>> Agreed. Infact I tried to do that first, but the only problem we have
> >>>>>>> here, is that, the file_priv
> >>>>>>> which has the aspect ratio capability information is not supplied
> >>>>>>> to the
> >>>>>>> drm_mode_convert_umode()
> >>>>>>> or the functions calling, drm_mode_conver_umode(). At best we have
> >>>>>>> drm_crtc_state.
> >>>>>> Simply passing the file_priv down would seem like the most obvious
> >>>>>> solution.
> >>>>> This means we have to make change in various drivers and we have to
> >>>>> pull
> >>>>> the file_priv
> >>>>> parameter three levels down. Should we think of a better way for
> >>>>> doing this?
> >>>> Coccinelle is pretty good at modifying function parameters.
> >>> Thanks for pointing out Coccinelle, will defintely try that out.
> >>> But my question here is whether we really want to drag the file_priv
> >>> from drm_mode_atomic_ioctl --> drm_mode_atomic_set_property -->
> >>> drm_atomic_crtc_set_property --> drm_mode_set_mode_prop_for_crtc-->
> >>> drm_mode_convert_umode.
> >>>
> >>> Instead, I tried to add a new field (allow_aspect_ratio) in
> >>> drm_crtc_state which is
> >>> already passed from the drm_mode_atomic_ioctl all the way to
> >>> drm_mode_convert_umode.
> >>>
> >>>
> >>> -Regards
> >>> Ankit
> >>>>>>> I have added an aspect ratio allowed flag  in
> >>>>>>> drm_crtc_state->state so
> >>>>>>> that its available in the
> >>>>>>> functions calling drm_mode_convert_umode.
> >>>>>>>
> >>>>>>> I am open to suggestion to avoid adding a new flag in
> >>>>>>> drm_atomic_state,
> >>>>>>> if there is a better
> >>>>>>> way to let the drm_mode_convert_umode( ) know that user supports
> >>>>>>> aspect
> >>>>>>> ratio capability.
> >>>>>>>>>              state->mode_blob =
> >>>>>>>>> drm_property_create_blob(state->crtc->dev,
> >>>>>>>>> sizeof(umode),
> >>>>>>>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct
> >>>>>>>>> drm_crtc_state *state,
> >>>>>>>>>              if (IS_ERR(state->mode_blob))
> >>>>>>>>>                  return PTR_ERR(state->mode_blob);
> >>>>>>>>>      -        drm_mode_copy(&state->mode, mode);
> >>>>>>>>> +        drm_mode_copy(&state->mode, ar_mode);
> >>>>>>>>>              state->enable = true;
> >>>>>>>>>              DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
> >>>>>>>>> -                 mode->name, state);
> >>>>>>>>> +                 ar_mode->name, state);
> >>>>>>>>> +        drm_mode_destroy(state->crtc->dev, ar_mode);
> >>>>>>>>>          } else {
> >>>>>>>>>              memset(&state->mode, 0, sizeof(state->mode));
> >>>>>>>>>              state->enable = false;
> >>>>>>>>> @@ -436,6 +469,25 @@
> >>>>>>>>> drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
> >>>>>>>>>      }
> >>>>>>>>>          /**
> >>>>>>>>> + * drm_atomic_allow_aspect_ratio_for_umode
> >>>>>>>>> + * @state: the crtc state
> >>>>>>>>> + * @umode: userspace mode, whose aspect ratio flag bits are to
> >>>>>>>>> be updated.
> >>>>>>>>> + *
> >>>>>>>>> + * Allow the aspect ratio info in the userspace mode only if
> >>>>>>>>> aspect ratio is
> >>>>>>>>> + * supported.
> >>>>>>>>> + */
> >>>>>>>>> +void
> >>>>>>>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state
> >>>>>>>>> *state,
> >>>>>>>>> +                    struct drm_mode_modeinfo *umode)
> >>>>>>>>> +{
> >>>>>>>>> +    struct drm_atomic_state *atomic_state = state->state;
> >>>>>>>>> +
> >>>>>>>>> +    /* Reset the aspect ratio flags if aspect ratio is not
> >>>>>>>>> supported */
> >>>>>>>>> +    if (atomic_state && !atomic_state->allow_aspect_ratio)
> >>>>>>>>> +        umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
> >>>>>>>>> +}
> >>>>>>>>> +
> >>>>>>>>> +/**
> >>>>>>>>>       * drm_atomic_crtc_set_property - set property on CRTC
> >>>>>>>>>       * @crtc: the drm CRTC to set a property on
> >>>>>>>>>       * @state: the state object to update with the new property
> >>>>>>>>> value
> >>>>>>>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct
> >>>>>>>>> drm_crtc *crtc,
> >>>>>>>>>          else if (property == config->prop_mode_id) {
> >>>>>>>>>              struct drm_property_blob *mode =
> >>>>>>>>>                  drm_property_lookup_blob(dev, val);
> >>>>>>>>> + drm_atomic_allow_aspect_ratio_for_umode(state,
> >>>>>>>>> +                    (struct drm_mode_modeinfo *)
> >>>>>>>>> +                    mode->data);
> >>>>>>>>>              ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
> >>>>>>>>>              drm_property_blob_put(mode);
> >>>>>>>>>              return ret;
> >>>>>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c
> >>>>>>>>> b/drivers/gpu/drm/drm_crtc.c
> >>>>>>>>> index f0556e6..a2d34fa 100644
> >>>>>>>>> --- a/drivers/gpu/drm/drm_crtc.c
> >>>>>>>>> +++ b/drivers/gpu/drm/drm_crtc.c
> >>>>>>>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
> >>>>>>>>>          drm_modeset_lock(&crtc->mutex, NULL);
> >>>>>>>>>          if (crtc->state) {
> >>>>>>>>>              if (crtc->state->enable) {
> >>>>>>>>> +            /*
> >>>>>>>>> +             * If the aspect-ratio is not required by the,
> >>>>>>>>> +             * userspace, do not set the aspect-ratio flags.
> >>>>>>>>> +             */
> >>>>>>>>> +            if (!file_priv->aspect_ratio_allowed)
> >>>>>>>>> + crtc->state->mode.picture_aspect_ratio =
> >>>>>>>>> +                    HDMI_PICTURE_ASPECT_NONE;
> >>>>>>>> These would still clobber the current crtc state. So definitely
> >>>>>>>> don't
> >>>>>>>> want to do this.
> >>>>>>> Alright, so alternative way of doing this will be:
> >>>>>>> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel
> >>>>>>> mode structure,
> >>>>>>> but set the aspect ratio flags in the usermode to none, after
> >>>>>>> calling the
> >>>>>>> drm_mode_convert_to_umode().
> >>>>>>>
> >>>>>>> Will take care of this in the next patchset.
> >>>>> I gave a thought about it again. As you have mentioned earlier that
> >>>>> get_crtc is one of the
> >>>>> place where the user mode aspect ratio flags must be filtered out, is
> >>>>> this what you have
> >>>>> in mind. I hope I did not misunderstand.
> >>>>>
> >>>>> Changing the usermode aspect ratio flag bits, without change in the
> >>>>> picture_aspect_ratio
> >>>>> kernel mode will not result in both structures out of sync?
> >>>> I don't think that should really matter. I suppose we might get one
> >>>> extra modeset when the user feeds that mode back via setcrtc/atomic, but
> >>>> meh. And actually the aspect ratio should only affect the infoframe, so
> >>>> we should be able to eventually eliminate that extra modeset and just
> >>>> update the infoframe instead.
> >>>>
> >>>>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
> >>>>>>>>>                  crtc_resp->mode_valid = 1;
> >>>>>>>>>      @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device
> >>>>>>>>> *dev,
> >>>>>>>>>              crtc_resp->x = crtc->x;
> >>>>>>>>>              crtc_resp->y = crtc->y;
> >>>>>>>>>              if (crtc->enabled) {
> >>>>>>>>> +            /*
> >>>>>>>>> +             * If the aspect-ratio is not required by the,
> >>>>>>>>> +             * userspace, do not set the aspect-ratio flags.
> >>>>>>>>> +             */
> >>>>>>>>> +            if (!file_priv->aspect_ratio_allowed)
> >>>>>>>>> +                crtc->mode.picture_aspect_ratio =
> >>>>>>>>> +                    HDMI_PICTURE_ASPECT_NONE;
> >>>>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
> >>>>>>>>>                  crtc_resp->mode_valid = 1;
> >>>>>>>>>      @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device
> >>>>>>>>> *dev, void *data,
> >>>>>>>>>                  goto out;
> >>>>>>>>>              }
> >>>>>>>>>      +        /* If the user does not ask for aspect ratio, reset
> >>>>>>>>> the aspect
> >>>>>>>>> +         * ratio bits in the usermode.
> >>>>>>>>> +         */
> >>>>>>>>> +        if (!file_priv->aspect_ratio_allowed)
> >>>>>>>>> +            crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
> >>>>>>>>>              ret = drm_mode_convert_umode(mode, &crtc_req->mode);
> >>>>>>>>>              if (ret) {
> >>>>>>>>>                  DRM_DEBUG_KMS("Invalid mode\n");
> >>>>>>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> >>>>>>>>> index 1c27526..130dad9 100644
> >>>>>>>>> --- a/include/drm/drm_atomic.h
> >>>>>>>>> +++ b/include/drm/drm_atomic.h
> >>>>>>>>> @@ -237,6 +237,7 @@ struct __drm_private_objs_state {
> >>>>>>>>>       * @ref: count of all references to this state (will not be
> >>>>>>>>> freed until zero)
> >>>>>>>>>       * @dev: parent DRM device
> >>>>>>>>>       * @allow_modeset: allow full modeset
> >>>>>>>>> + * @allow_aspect_ratio: allow the aspect ratio info to be
> >>>>>>>>> passes to user
> >>>>>>>>>       * @legacy_cursor_update: hint to enforce legacy cursor
> >>>>>>>>> IOCTL semantics
> >>>>>>>>>       * @async_update: hint for asynchronous plane update
> >>>>>>>>>       * @planes: pointer to array of structures with per-plane data
> >>>>>>>>> @@ -256,6 +257,7 @@ struct drm_atomic_state {
> >>>>>>>>>              struct drm_device *dev;
> >>>>>>>>>          bool allow_modeset : 1;
> >>>>>>>>> +    bool allow_aspect_ratio : 1;
> >>>>>>>>>          bool legacy_cursor_update : 1;
> >>>>>>>>>          bool async_update : 1;
> >>>>>>>>>          struct __drm_planes_state *planes;
> >>>>>>>>> -- 
> >>>>>>>>> 2.7.4
> >>>>>>>>>
> >>>>>>>>> _______________________________________________
> >>>>>>>>> dri-devel mailing list
> >>>>>>>>> [e-mail]
> >>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>>> Regards,
> >>>>> Ankit
>

Nautiyal, Ankit K Feb. 15, 2018, 11:57 a.m. UTC | #11

Hi Ville,

I have addressed your review comments (including the policy regarding, 
rejection of mode with aspect ratio, if no aspect ratio cap)

and other suggestions in the next patch-set. I will be sending the next 
patch-set, shortly.

Regards,

Ankit


On 2/13/2018 10:15 PM, Ville Syrjälä wrote:
> On Tue, Feb 13, 2018 at 09:53:53PM +0530, Nautiyal, Ankit K wrote:
>> Hi Ville,
>>
>> Thanks yet again to look into this.
>>
>> I am still skeptical about rejecting the mode, if aspect ratio cap is
>> not set.
>> Perhaps I am not aware with the userspace expectations.
>>
>> Please find my response inline:
>>
>> On 2/13/2018 6:48 PM, Ville Syrjälä wrote:
>>> On Tue, Feb 13, 2018 at 10:21:15AM +0530, Nautiyal, Ankit K wrote:
>>>> Hi Ville,
>>>>
>>>> As per our last discussion, following points were discussed:
>>>>
>>>> 1. To suppress the aspect-ratio info from getblob ioctl to a user that
>>>> does not support it:
>>>>
>>>>        i. A new flag must be added to drm_blob_property to mark if the
>>>> blob has mode data.
>>>>
>>>>        ii. This flag must be set when the user tries do an atomic modeset.
>>>>
>>>>        iii. In the getblob ioctl, based on the above flag, it can be
>>>> determined that if the blob
>>>>
>>>>        has mode data, and the aspect ratio info can be suppressed in
>>>> getblob ioctl, if user does not support it.
>>>>
>>>> 2. Instead of adding aspect ratio capabilities in drm_atomic_state, pass
>>>> on the file_priv which already has
>>>>
>>>> the required information to the function drm_mode_convert_umode.
>>>>
>>>> It will require addition of an new argument file_priv to several
>>>> functions, but that is right thing to do,
>>>>
>>>> as file_priv is the correct structure for the capability information.
>>>>
>>>> 3. Changing the usermode aspect ratio flag bits, without change in the
>>>> picture_aspect_ratio would not matter, and does not need to be handled
>>>> in this patch.
>>>>
>>>>
>>>> I just have one query here. We have agreed to modify
>>>> drm_mode_convert_umode, to filter out the aspect-ratio
>>>>
>>>> info if user has not set aspect-ratio capability, but do we need a
>>>> similar change the drm_mode_convert_to_umode?
>>> I think you maybe had those backwards.
>>>
>>> drm_mode_convert_umode(), or more likely its relevant callers setcrtc
>>> and setproperty, need to reject the mode if the client cap is not set.
>> I guess, rejecting the mode, altogether can break the existing user-spaces.
>> Current user-spaces, oblivious of the aspect-ratio capability in drm,
>> will not set the aspect-ratio capability.
>> Some of them, might have some garbage value in the aspect ratio bits of
>> the user mode. If we reject such
>> modes, the user-spaces that were earlier working will break.
>> (But if we are sure, that the aspect ratio flag bits will all be reset,
>> then it makes sense to reject the mode.)
> We already reject all unspecified flags.
>
>> Instead of rejecting such modes, if we just only ignore the aspect ratio
>> bits in such cases, and carry on with the
>> modeset for the given user mode, the behaviour would be intact, just as
>> prior to the aspect ratio changes.
>>
>>> drm_mode_convert_to_umode(), or rather its callers getblob and getcrtc,
>>> need to filter out the flags.
>> Yes right. I'll be filtering out the flags in the getblob and getcrtc
>> functions.
>>
>> Thanks & Regards,
>> Ankit
>>>> If we do, I am not sure if it would be possible to have the file_priv to
>>>> /drm_atomic_set_mode_for_crtc/, which calls
>>>>
>>>> drm_mode_convert_to_umode.  The function /drm_atomic_set_mode_for_crtc/
>>>> is used to set mode, originating by kernel,
>>>>
>>>> and make a blob from the kernel mode, which it saves in crtc_state.
>>>>
>>>> This function /: drm_atomic_set_mode_for_crtc, /is called by several
>>>> helper functions and driver functions, and passing
>>>>
>>>> file_priv from all these functions does not seem to be plausible.
>>> I don't think we need to plumb it quite that deep. Doing the
>>> check in drm_atomic_crtc_set_property() should be sufficient.
>>>
>>>> Any suggestions on how to handle this situation?
>>>>
>>>>
>>>> Regards,
>>>>
>>>> Ankit
>>>>
>>>>
>>>> On 2/8/2018 9:59 AM, Nautiyal, Ankit K wrote:
>>>>> Hi Ville,
>>>>>
>>>>> I still have some queries regarding the handling of aspect ratio flags
>>>>> in getblob ioctl.
>>>>>
>>>>> Please find below my responses inline.
>>>>>
>>>>>
>>>>> On 2/1/2018 6:24 PM, Ville Syrjälä wrote:
>>>>>> On Thu, Feb 01, 2018 at 04:35:22PM +0530, Nautiyal, Ankit K wrote:
>>>>>>> Hi Ville,
>>>>>>>
>>>>>>> Appreciate your time and the suggestions.
>>>>>>> Please find my response inline:
>>>>>>>
>>>>>>> On 1/31/2018 6:39 PM, Ville Syrjälä wrote:
>>>>>>>> On Wed, Jan 31, 2018 at 12:04:52PM +0530, Nautiyal, Ankit K wrote:
>>>>>>>>> On 1/30/2018 12:23 AM, Ville Syrjälä wrote:
>>>>>>>>>> On Fri, Jan 12, 2018 at 11:51:33AM +0530, Nautiyal, Ankit K wrote:
>>>>>>>>>>> From: Ankit Nautiyal <[e-mail]>
>>>>>>>>>>>
>>>>>>>>>>> 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 <[e-mail]>
>>>>>>>>>>>
>>>>>>>>>>> V3: Addressed review comments from Ville:
>>>>>>>>>>> -Do not corrupt the current crtc state by updating aspect ratio
>>>>>>>>>>> on the fly.
>>>>>>>>>>> ---
>>>>>>>>>>>       drivers/gpu/drm/drm_atomic.c | 61
>>>>>>>>>>> +++++++++++++++++++++++++++++++++++++++++---
>>>>>>>>>>>       drivers/gpu/drm/drm_crtc.c   | 19 ++++++++++++++
>>>>>>>>>>>       include/drm/drm_atomic.h     |  2 ++
>>>>>>>>>>>       3 files changed, 79 insertions(+), 3 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/gpu/drm/drm_atomic.c
>>>>>>>>>>> b/drivers/gpu/drm/drm_atomic.c
>>>>>>>>>>> index 69ff763..39313e2 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/drm_atomic.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>>>>>>>>>> @@ -316,6 +316,35 @@ static s32 __user
>>>>>>>>>>> *get_out_fence_for_crtc(struct drm_atomic_state *state,
>>>>>>>>>>>               return fence_ptr;
>>>>>>>>>>>       }
>>>>>>>>>>> +/**
>>>>>>>>>>> + * drm_atomic_allow_aspect_ratio_for_kmode
>>>>>>>>>>> + * @state: the crtc state
>>>>>>>>>>> + * @mode: kernel-internal mode, which is used to create a
>>>>>>>>>>> duplicate mode,
>>>>>>>>>>> + * with updated picture aspect ratio.
>>>>>>>>>>> + *
>>>>>>>>>>> + * Allow the aspect ratio info in the kernel mode only if
>>>>>>>>>>> aspect ratio is
>>>>>>>>>>> + * supported.
>>>>>>>>>>> + *
>>>>>>>>>>> + * RETURNS:
>>>>>>>>>>> + * kernel-internal mode with updated picture aspect ratio value.
>>>>>>>>>>> + */
>>>>>>>>>>> +
>>>>>>>>>>> +struct drm_display_mode*
>>>>>>>>>>> +drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state
>>>>>>>>>>> *state,
>>>>>>>>>>> +                    const struct drm_display_mode *mode)
>>>>>>>>>>> +{
>>>>>>>>>>> +    struct drm_atomic_state *atomic_state = state->state;
>>>>>>>>>>> +    struct drm_display_mode *ar_kmode;
>>>>>>>>>>> +
>>>>>>>>>>> +    ar_kmode = drm_mode_duplicate(state->crtc->dev, mode);
>>>>>>>>>>> +    /*
>>>>>>>>>>> +     * If aspect ratio is not supported, set the picture aspect
>>>>>>>>>>> ratio as
>>>>>>>>>>> +     * NONE.
>>>>>>>>>>> +     */
>>>>>>>>>>> +    if (atomic_state && !atomic_state->allow_aspect_ratio)
>>>>>>>>>>> +        ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>>>>>>>>>> +    return ar_kmode;
>>>>>>>>>>> +}
>>>>>>>>>>>           /**
>>>>>>>>>>>        * drm_atomic_set_mode_for_crtc - set mode for CRTC
>>>>>>>>>>> @@ -341,7 +370,10 @@ int drm_atomic_set_mode_for_crtc(struct
>>>>>>>>>>> drm_crtc_state *state,
>>>>>>>>>>>           state->mode_blob = NULL;
>>>>>>>>>>>               if (mode) {
>>>>>>>>>>> -        drm_mode_convert_to_umode(&umode, mode);
>>>>>>>>>>> +        struct drm_display_mode *ar_mode;
>>>>>>>>>>> +
>>>>>>>>>>> +        ar_mode =
>>>>>>>>>>> drm_atomic_allow_aspect_ratio_for_kmode(state, mode);
>>>>>>>>>>> +        drm_mode_convert_to_umode(&umode, ar_mode);
>>>>>>>>>> This still looks sketchy.
>>>>>>>>>>
>>>>>>>>>> I believe there are just two places where we need to filter out the
>>>>>>>>>> aspect ratio flags from the umode, namely the getblob and getcrtc
>>>>>>>>>> ioctls.
>>>>>>>>> AFAIK The getblob ioctl can be called for edid blob, gamma, ctm blob,
>>>>>>>>> etc. apart from the mode blob.
>>>>>>>>> Is there any way to check from blob id (or by any other means),
>>>>>>>>> if the data is for the mode, or edid, or gamma etc.
>>>>>>>> I think we'd have to somehow mark the mode blobs as special. Until we
>>>>>>>> have more need for cleaning up blobs before returning them to
>>>>>>>> userspace
>>>>>>>> I think a simple flag should do. If we come up with more uses like
>>>>>>>> this
>>>>>>>> then it might make sense to introduce some kind of optional filter
>>>>>>>> function for blobs.
>>>>>>> If my understanding is correct,
>>>>>>> 1. To be able to do this, we need to change in uapi.
>>>>>> We should be fine with an internal flag. Obviously it won't be set
>>>>>> for blobs freshly created by userspace, but that's fine because we
>>>>>> do no other error checking for them either. The error checking will
>>>>>> happen when the user tries to actually use the blob.
>>>>> I am not sure why getblob ioctl should even come into picture.
>>>>> (Perhaps I am missing something).
>>>>> As per my understanding:
>>>>> If a userspace needs to set a mode, it will just create a blob with
>>>>> drm_mode_mode_info,
>>>>> and get the blob-id. This blob-id would be supplied to
>>>>> drm_mode_atomic_ioctl as a crtc
>>>>> property mode_id.
>>>>> At the kernel side,  the crtc property mode_id is set by looking-up
>>>>> for mode blob from blob id.
>>>>> I am doing the change in the user mode aspect-ratio flags at this
>>>>> junction.
>>>>>
>>>>> As far as getblob ioctl is concerned, if the user does call getblob
>>>>> ioctl for getting mode blob from
>>>>> the blob id, it will receive the same mode blob, with same aspect
>>>>> ratio info which the user had
>>>>> created using create blob ioctl. If it does want to use the blob, it
>>>>> has to come through the
>>>>> drm_mode_atomic_ioctl way, where aspect ratio flags are handled.
>>>>>
>>>>>>> 2. Whenever the userspace creates a blob for mode, the new flag/class
>>>>>>> for blob type needs to be set.
>>>>>>>
>>>>>>> Do you think that it's worth the effort?
>>>>>>>>> If we can check that, I can make sure that aspect-ratio flags are
>>>>>>>>> taken
>>>>>>>>> care of, if the blob is for mode,
>>>>>>>>> and the aspect ratio is not supported.
>>>>>>>>>
>>>>>>>>>> And for checking the user input I think we should probably just
>>>>>>>>>> stick that into drm_mode_convert_umode(). Looks like we never call
>>>>>>>>>> that from anywhere but the atomic/setprop and setcrtc paths with
>>>>>>>>>> a non-NULL argument.
>>>>>>>>>>
>>>>>>>>>> I *think* those three places should be sufficient to cover
>>>>>>>>>> everything.
>>>>>>>>> Agreed. Infact I tried to do that first, but the only problem we have
>>>>>>>>> here, is that, the file_priv
>>>>>>>>> which has the aspect ratio capability information is not supplied
>>>>>>>>> to the
>>>>>>>>> drm_mode_convert_umode()
>>>>>>>>> or the functions calling, drm_mode_conver_umode(). At best we have
>>>>>>>>> drm_crtc_state.
>>>>>>>> Simply passing the file_priv down would seem like the most obvious
>>>>>>>> solution.
>>>>>>> This means we have to make change in various drivers and we have to
>>>>>>> pull
>>>>>>> the file_priv
>>>>>>> parameter three levels down. Should we think of a better way for
>>>>>>> doing this?
>>>>>> Coccinelle is pretty good at modifying function parameters.
>>>>> Thanks for pointing out Coccinelle, will defintely try that out.
>>>>> But my question here is whether we really want to drag the file_priv
>>>>> from drm_mode_atomic_ioctl --> drm_mode_atomic_set_property -->
>>>>> drm_atomic_crtc_set_property --> drm_mode_set_mode_prop_for_crtc-->
>>>>> drm_mode_convert_umode.
>>>>>
>>>>> Instead, I tried to add a new field (allow_aspect_ratio) in
>>>>> drm_crtc_state which is
>>>>> already passed from the drm_mode_atomic_ioctl all the way to
>>>>> drm_mode_convert_umode.
>>>>>
>>>>>
>>>>> -Regards
>>>>> Ankit
>>>>>>>>> I have added an aspect ratio allowed flag  in
>>>>>>>>> drm_crtc_state->state so
>>>>>>>>> that its available in the
>>>>>>>>> functions calling drm_mode_convert_umode.
>>>>>>>>>
>>>>>>>>> I am open to suggestion to avoid adding a new flag in
>>>>>>>>> drm_atomic_state,
>>>>>>>>> if there is a better
>>>>>>>>> way to let the drm_mode_convert_umode( ) know that user supports
>>>>>>>>> aspect
>>>>>>>>> ratio capability.
>>>>>>>>>>>               state->mode_blob =
>>>>>>>>>>> drm_property_create_blob(state->crtc->dev,
>>>>>>>>>>> sizeof(umode),
>>>>>>>>>>> @@ -349,10 +381,11 @@ int drm_atomic_set_mode_for_crtc(struct
>>>>>>>>>>> drm_crtc_state *state,
>>>>>>>>>>>               if (IS_ERR(state->mode_blob))
>>>>>>>>>>>                   return PTR_ERR(state->mode_blob);
>>>>>>>>>>>       -        drm_mode_copy(&state->mode, mode);
>>>>>>>>>>> +        drm_mode_copy(&state->mode, ar_mode);
>>>>>>>>>>>               state->enable = true;
>>>>>>>>>>>               DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
>>>>>>>>>>> -                 mode->name, state);
>>>>>>>>>>> +                 ar_mode->name, state);
>>>>>>>>>>> +        drm_mode_destroy(state->crtc->dev, ar_mode);
>>>>>>>>>>>           } else {
>>>>>>>>>>>               memset(&state->mode, 0, sizeof(state->mode));
>>>>>>>>>>>               state->enable = false;
>>>>>>>>>>> @@ -436,6 +469,25 @@
>>>>>>>>>>> drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
>>>>>>>>>>>       }
>>>>>>>>>>>           /**
>>>>>>>>>>> + * drm_atomic_allow_aspect_ratio_for_umode
>>>>>>>>>>> + * @state: the crtc state
>>>>>>>>>>> + * @umode: userspace mode, whose aspect ratio flag bits are to
>>>>>>>>>>> be updated.
>>>>>>>>>>> + *
>>>>>>>>>>> + * Allow the aspect ratio info in the userspace mode only if
>>>>>>>>>>> aspect ratio is
>>>>>>>>>>> + * supported.
>>>>>>>>>>> + */
>>>>>>>>>>> +void
>>>>>>>>>>> +drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state
>>>>>>>>>>> *state,
>>>>>>>>>>> +                    struct drm_mode_modeinfo *umode)
>>>>>>>>>>> +{
>>>>>>>>>>> +    struct drm_atomic_state *atomic_state = state->state;
>>>>>>>>>>> +
>>>>>>>>>>> +    /* Reset the aspect ratio flags if aspect ratio is not
>>>>>>>>>>> supported */
>>>>>>>>>>> +    if (atomic_state && !atomic_state->allow_aspect_ratio)
>>>>>>>>>>> +        umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +/**
>>>>>>>>>>>        * drm_atomic_crtc_set_property - set property on CRTC
>>>>>>>>>>>        * @crtc: the drm CRTC to set a property on
>>>>>>>>>>>        * @state: the state object to update with the new property
>>>>>>>>>>> value
>>>>>>>>>>> @@ -464,6 +516,9 @@ int drm_atomic_crtc_set_property(struct
>>>>>>>>>>> drm_crtc *crtc,
>>>>>>>>>>>           else if (property == config->prop_mode_id) {
>>>>>>>>>>>               struct drm_property_blob *mode =
>>>>>>>>>>>                   drm_property_lookup_blob(dev, val);
>>>>>>>>>>> + drm_atomic_allow_aspect_ratio_for_umode(state,
>>>>>>>>>>> +                    (struct drm_mode_modeinfo *)
>>>>>>>>>>> +                    mode->data);
>>>>>>>>>>>               ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
>>>>>>>>>>>               drm_property_blob_put(mode);
>>>>>>>>>>>               return ret;
>>>>>>>>>>> diff --git a/drivers/gpu/drm/drm_crtc.c
>>>>>>>>>>> b/drivers/gpu/drm/drm_crtc.c
>>>>>>>>>>> index f0556e6..a2d34fa 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/drm_crtc.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>>>>>>>>>> @@ -425,6 +425,13 @@ int drm_mode_getcrtc(struct drm_device *dev,
>>>>>>>>>>>           drm_modeset_lock(&crtc->mutex, NULL);
>>>>>>>>>>>           if (crtc->state) {
>>>>>>>>>>>               if (crtc->state->enable) {
>>>>>>>>>>> +            /*
>>>>>>>>>>> +             * If the aspect-ratio is not required by the,
>>>>>>>>>>> +             * userspace, do not set the aspect-ratio flags.
>>>>>>>>>>> +             */
>>>>>>>>>>> +            if (!file_priv->aspect_ratio_allowed)
>>>>>>>>>>> + crtc->state->mode.picture_aspect_ratio =
>>>>>>>>>>> +                    HDMI_PICTURE_ASPECT_NONE;
>>>>>>>>>> These would still clobber the current crtc state. So definitely
>>>>>>>>>> don't
>>>>>>>>>> want to do this.
>>>>>>>>> Alright, so alternative way of doing this will be:
>>>>>>>>> Avoid changing the crtc->state->mode.picture_aspect_ratio i.e kernel
>>>>>>>>> mode structure,
>>>>>>>>> but set the aspect ratio flags in the usermode to none, after
>>>>>>>>> calling the
>>>>>>>>> drm_mode_convert_to_umode().
>>>>>>>>>
>>>>>>>>> Will take care of this in the next patchset.
>>>>>>> I gave a thought about it again. As you have mentioned earlier that
>>>>>>> get_crtc is one of the
>>>>>>> place where the user mode aspect ratio flags must be filtered out, is
>>>>>>> this what you have
>>>>>>> in mind. I hope I did not misunderstand.
>>>>>>>
>>>>>>> Changing the usermode aspect ratio flag bits, without change in the
>>>>>>> picture_aspect_ratio
>>>>>>> kernel mode will not result in both structures out of sync?
>>>>>> I don't think that should really matter. I suppose we might get one
>>>>>> extra modeset when the user feeds that mode back via setcrtc/atomic, but
>>>>>> meh. And actually the aspect ratio should only affect the infoframe, so
>>>>>> we should be able to eventually eliminate that extra modeset and just
>>>>>> update the infoframe instead.
>>>>>>
>>>>>>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
>>>>>>>>>>>                   crtc_resp->mode_valid = 1;
>>>>>>>>>>>       @@ -435,6 +442,13 @@ int drm_mode_getcrtc(struct drm_device
>>>>>>>>>>> *dev,
>>>>>>>>>>>               crtc_resp->x = crtc->x;
>>>>>>>>>>>               crtc_resp->y = crtc->y;
>>>>>>>>>>>               if (crtc->enabled) {
>>>>>>>>>>> +            /*
>>>>>>>>>>> +             * If the aspect-ratio is not required by the,
>>>>>>>>>>> +             * userspace, do not set the aspect-ratio flags.
>>>>>>>>>>> +             */
>>>>>>>>>>> +            if (!file_priv->aspect_ratio_allowed)
>>>>>>>>>>> +                crtc->mode.picture_aspect_ratio =
>>>>>>>>>>> +                    HDMI_PICTURE_ASPECT_NONE;
>>>>>>>>>>> drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
>>>>>>>>>>>                   crtc_resp->mode_valid = 1;
>>>>>>>>>>>       @@ -610,6 +624,11 @@ int drm_mode_setcrtc(struct drm_device
>>>>>>>>>>> *dev, void *data,
>>>>>>>>>>>                   goto out;
>>>>>>>>>>>               }
>>>>>>>>>>>       +        /* If the user does not ask for aspect ratio, reset
>>>>>>>>>>> the aspect
>>>>>>>>>>> +         * ratio bits in the usermode.
>>>>>>>>>>> +         */
>>>>>>>>>>> +        if (!file_priv->aspect_ratio_allowed)
>>>>>>>>>>> +            crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
>>>>>>>>>>>               ret = drm_mode_convert_umode(mode, &crtc_req->mode);
>>>>>>>>>>>               if (ret) {
>>>>>>>>>>>                   DRM_DEBUG_KMS("Invalid mode\n");
>>>>>>>>>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>>>>>>>>>>> index 1c27526..130dad9 100644
>>>>>>>>>>> --- a/include/drm/drm_atomic.h
>>>>>>>>>>> +++ b/include/drm/drm_atomic.h
>>>>>>>>>>> @@ -237,6 +237,7 @@ struct __drm_private_objs_state {
>>>>>>>>>>>        * @ref: count of all references to this state (will not be
>>>>>>>>>>> freed until zero)
>>>>>>>>>>>        * @dev: parent DRM device
>>>>>>>>>>>        * @allow_modeset: allow full modeset
>>>>>>>>>>> + * @allow_aspect_ratio: allow the aspect ratio info to be
>>>>>>>>>>> passes to user
>>>>>>>>>>>        * @legacy_cursor_update: hint to enforce legacy cursor
>>>>>>>>>>> IOCTL semantics
>>>>>>>>>>>        * @async_update: hint for asynchronous plane update
>>>>>>>>>>>        * @planes: pointer to array of structures with per-plane data
>>>>>>>>>>> @@ -256,6 +257,7 @@ struct drm_atomic_state {
>>>>>>>>>>>               struct drm_device *dev;
>>>>>>>>>>>           bool allow_modeset : 1;
>>>>>>>>>>> +    bool allow_aspect_ratio : 1;
>>>>>>>>>>>           bool legacy_cursor_update : 1;
>>>>>>>>>>>           bool async_update : 1;
>>>>>>>>>>>           struct __drm_planes_state *planes;
>>>>>>>>>>> -- 
>>>>>>>>>>> 2.7.4
>>>>>>>>>>>
>>>>>>>>>>> _______________________________________________
>>>>>>>>>>> dri-devel mailing list
>>>>>>>>>>> [e-mail]
>>>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>>> Regards,
>>>>>>> Ankit

Patch

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 69ff763..39313e2 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -316,6 +316,35 @@  static s32 __user *get_out_fence_for_crtc(struct drm_atomic_state *state,
 
 	return fence_ptr;
 }
+/**
+ * drm_atomic_allow_aspect_ratio_for_kmode
+ * @state: the crtc state
+ * @mode: kernel-internal mode, which is used to create a duplicate mode,
+ * with updated picture aspect ratio.
+ *
+ * Allow the aspect ratio info in the kernel mode only if aspect ratio is
+ * supported.
+ *
+ * RETURNS:
+ * kernel-internal mode with updated picture aspect ratio value.
+ */
+
+struct drm_display_mode*
+drm_atomic_allow_aspect_ratio_for_kmode(struct drm_crtc_state *state,
+					const struct drm_display_mode *mode)
+{
+	struct drm_atomic_state *atomic_state = state->state;
+	struct drm_display_mode *ar_kmode;
+
+	ar_kmode = drm_mode_duplicate(state->crtc->dev, mode);
+	/*
+	 * If aspect ratio is not supported, set the picture aspect ratio as
+	 * NONE.
+	 */
+	if (atomic_state && !atomic_state->allow_aspect_ratio)
+		ar_kmode->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
+	return ar_kmode;
+}
 
 /**
  * drm_atomic_set_mode_for_crtc - set mode for CRTC
@@ -341,7 +370,10 @@  int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
 	state->mode_blob = NULL;
 
 	if (mode) {
-		drm_mode_convert_to_umode(&umode, mode);
+		struct drm_display_mode *ar_mode;
+
+		ar_mode = drm_atomic_allow_aspect_ratio_for_kmode(state, mode);
+		drm_mode_convert_to_umode(&umode, ar_mode);
 		state->mode_blob =
 			drm_property_create_blob(state->crtc->dev,
 		                                 sizeof(umode),
@@ -349,10 +381,11 @@  int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
 		if (IS_ERR(state->mode_blob))
 			return PTR_ERR(state->mode_blob);
 
-		drm_mode_copy(&state->mode, mode);
+		drm_mode_copy(&state->mode, ar_mode);
 		state->enable = true;
 		DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
-				 mode->name, state);
+				 ar_mode->name, state);
+		drm_mode_destroy(state->crtc->dev, ar_mode);
 	} else {
 		memset(&state->mode, 0, sizeof(state->mode));
 		state->enable = false;
@@ -436,6 +469,25 @@  drm_atomic_replace_property_blob_from_id(struct drm_device *dev,
 }
 
 /**
+ * drm_atomic_allow_aspect_ratio_for_umode
+ * @state: the crtc state
+ * @umode: userspace mode, whose aspect ratio flag bits are to be updated.
+ *
+ * Allow the aspect ratio info in the userspace mode only if aspect ratio is
+ * supported.
+ */
+void
+drm_atomic_allow_aspect_ratio_for_umode(struct drm_crtc_state *state,
+					struct drm_mode_modeinfo *umode)
+{
+	struct drm_atomic_state *atomic_state = state->state;
+
+	/* Reset the aspect ratio flags if aspect ratio is not supported */
+	if (atomic_state && !atomic_state->allow_aspect_ratio)
+		umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
+}
+
+/**
  * drm_atomic_crtc_set_property - set property on CRTC
  * @crtc: the drm CRTC to set a property on
  * @state: the state object to update with the new property value
@@ -464,6 +516,9 @@  int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 	else if (property == config->prop_mode_id) {
 		struct drm_property_blob *mode =
 			drm_property_lookup_blob(dev, val);
+		drm_atomic_allow_aspect_ratio_for_umode(state,
+					(struct drm_mode_modeinfo *)
+					mode->data);
 		ret = drm_atomic_set_mode_prop_for_crtc(state, mode);
 		drm_property_blob_put(mode);
 		return ret;
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index f0556e6..a2d34fa 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -425,6 +425,13 @@  int drm_mode_getcrtc(struct drm_device *dev,
 	drm_modeset_lock(&crtc->mutex, NULL);
 	if (crtc->state) {
 		if (crtc->state->enable) {
+			/*
+			 * If the aspect-ratio is not required by the,
+			 * userspace, do not set the aspect-ratio flags.
+			 */
+			if (!file_priv->aspect_ratio_allowed)
+				crtc->state->mode.picture_aspect_ratio =
+					HDMI_PICTURE_ASPECT_NONE;
 			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
 			crtc_resp->mode_valid = 1;
 
@@ -435,6 +442,13 @@  int drm_mode_getcrtc(struct drm_device *dev,
 		crtc_resp->x = crtc->x;
 		crtc_resp->y = crtc->y;
 		if (crtc->enabled) {
+			/*
+			 * If the aspect-ratio is not required by the,
+			 * userspace, do not set the aspect-ratio flags.
+			 */
+			if (!file_priv->aspect_ratio_allowed)
+				crtc->mode.picture_aspect_ratio =
+					HDMI_PICTURE_ASPECT_NONE;
 			drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode);
 			crtc_resp->mode_valid = 1;
 
@@ -610,6 +624,11 @@  int drm_mode_setcrtc(struct drm_device *dev, void *data,
 			goto out;
 		}
 
+		/* If the user does not ask for aspect ratio, reset the aspect
+		 * ratio bits in the usermode.
+		 */
+		if (!file_priv->aspect_ratio_allowed)
+			crtc_req->mode.flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
 		ret = drm_mode_convert_umode(mode, &crtc_req->mode);
 		if (ret) {
 			DRM_DEBUG_KMS("Invalid mode\n");
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 1c27526..130dad9 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -237,6 +237,7 @@  struct __drm_private_objs_state {
  * @ref: count of all references to this state (will not be freed until zero)
  * @dev: parent DRM device
  * @allow_modeset: allow full modeset
+ * @allow_aspect_ratio: allow the aspect ratio info to be passes to user
  * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
  * @async_update: hint for asynchronous plane update
  * @planes: pointer to array of structures with per-plane data
@@ -256,6 +257,7 @@  struct drm_atomic_state {
 
 	struct drm_device *dev;
 	bool allow_modeset : 1;
+	bool allow_aspect_ratio : 1;
 	bool legacy_cursor_update : 1;
 	bool async_update : 1;
 	struct __drm_planes_state *planes;