Re: [REBASE 2/5] drm: Handle aspect ratio in modeset paths

[Nautiyal, Ankit K]

Hi Ville,

Thanks for the suggestions. Please find my response inline


On 11/21/2017 10:41 PM, Ville Syrjälä wrote:

On Fri, Nov 17, 2017 at 03:00:29PM +0530, Shashank Sharma wrote:

From: Ankit Nautiyal 

If the user mode does not support aspect-ratio, and requests for
a modeset with aspect ratio flags, then the flag bits reprsenting
aspect ratio must be ignored.

Rejected, not ignored. Rejection should happen when converting
the umode to mode.

And filtering should happen in getcrtc and getblob. The way you're
currently doing things will corrupt the current state, which is
not good.


Agreed.
The filtering is being done in getcrtc but getblob was missing.
Will add the changes in getblob and send the new patch.

Regards,
Ankit

Similarly, if user space doesn't set the aspect ratio client cap,
while preparing a usermode, the aspect-ratio info must not be given
into it.

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.

Cc: Ville Syrjala 
Cc: Shashank Sharma 
Cc: Jose Abreu 
Signed-off-by: Ankit Nautiyal 
---
  drivers/gpu/drm/drm_atomic.c | 40 +---
  drivers/gpu/drm/drm_crtc.c   | 19 +++
  include/drm/drm_atomic.h |  2 ++
  3 files changed, 54 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 37445d5..b9743af 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -338,18 +338,30 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state 
*state,
state->mode_blob = NULL;
  
  	if (mode) {

-   drm_mode_convert_to_umode(, mode);
+   /*
+* If the user has not asked for aspect ratio support,
+* take a copy of the 'mode' and set the aspect ratio as
+* None. This copy is used to prepare the user-mode with no
+* aspect-ratio info.
+*/
+   struct drm_display_mode ar_mode;
+   struct drm_atomic_state *atomic_state = state->state;
+
+   drm_mode_copy(_mode, mode);
+   if (atomic_state && !atomic_state->allow_aspect_ratio)
+   ar_mode.picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
+   drm_mode_convert_to_umode(, _mode);
state->mode_blob =
drm_property_create_blob(state->crtc->dev,
-sizeof(umode),
-);
+sizeof(umode),
+);
if (IS_ERR(state->mode_blob))
return PTR_ERR(state->mode_blob);
  
-		drm_mode_copy(>mode, mode);

+   drm_mode_copy(>mode, _mode);
state->enable = true;
DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
-mode->name, state);
+ar_mode.name, state);
} else {
memset(>mode, 0, sizeof(state->mode));
state->enable = false;
@@ -386,10 +398,23 @@ int drm_atomic_set_mode_prop_for_crtc(struct 
drm_crtc_state *state,
memset(>mode, 0, sizeof(state->mode));
  
  	if (blob) {

+   struct drm_mode_modeinfo *ar_umode;
+   struct drm_atomic_state *atomic_state;
+
+   /*
+* If the user-space does not ask for aspect-ratio
+* the aspect ratio bits in the drmModeModeinfo from user,
+* does not mean anything. Therefore these bits must be
+* discarded.
+*/
+   ar_umode = (struct drm_mode_modeinfo *) blob->data;
+   atomic_state = state->state;
+   if (atomic_state && !atomic_state->allow_aspect_ratio)
+   ar_umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
if (blob->length != sizeof(struct drm_mode_modeinfo) ||
drm_mode_convert_umode(>mode,
-  (const struct drm_mode_modeinfo *)
-   blob->data))
+  (const struct drm_mode_modeinfo *)
+   ar_umode))
return -EINVAL;
  
  		state->mode_blob = drm_property_blob_get(blob);

@@ -2229,6 +2254,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
  
  	

Re: [REBASE 2/5] drm: Handle aspect ratio in modeset paths

[Ville Syrjälä]

On Fri, Nov 17, 2017 at 03:00:29PM +0530, Shashank Sharma wrote:
> From: Ankit Nautiyal 
> 
> If the user mode does not support aspect-ratio, and requests for
> a modeset with aspect ratio flags, then the flag bits reprsenting
> aspect ratio must be ignored.

Rejected, not ignored. Rejection should happen when converting
the umode to mode.

And filtering should happen in getcrtc and getblob. The way you're
currently doing things will corrupt the current state, which is
not good.

> Similarly, if user space doesn't set the aspect ratio client cap,
> while preparing a usermode, the aspect-ratio info must not be given
> into it.
> 
> 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.
> 
> Cc: Ville Syrjala 
> Cc: Shashank Sharma 
> Cc: Jose Abreu 
> Signed-off-by: Ankit Nautiyal 
> ---
>  drivers/gpu/drm/drm_atomic.c | 40 +---
>  drivers/gpu/drm/drm_crtc.c   | 19 +++
>  include/drm/drm_atomic.h |  2 ++
>  3 files changed, 54 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 37445d5..b9743af 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -338,18 +338,30 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state 
> *state,
>   state->mode_blob = NULL;
>  
>   if (mode) {
> - drm_mode_convert_to_umode(, mode);
> + /*
> +  * If the user has not asked for aspect ratio support,
> +  * take a copy of the 'mode' and set the aspect ratio as
> +  * None. This copy is used to prepare the user-mode with no
> +  * aspect-ratio info.
> +  */
> + struct drm_display_mode ar_mode;
> + struct drm_atomic_state *atomic_state = state->state;
> +
> + drm_mode_copy(_mode, mode);
> + if (atomic_state && !atomic_state->allow_aspect_ratio)
> + ar_mode.picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> + drm_mode_convert_to_umode(, _mode);
>   state->mode_blob =
>   drm_property_create_blob(state->crtc->dev,
> -  sizeof(umode),
> -  );
> +  sizeof(umode),
> +  );
>   if (IS_ERR(state->mode_blob))
>   return PTR_ERR(state->mode_blob);
>  
> - drm_mode_copy(>mode, mode);
> + drm_mode_copy(>mode, _mode);
>   state->enable = true;
>   DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
> -  mode->name, state);
> +  ar_mode.name, state);
>   } else {
>   memset(>mode, 0, sizeof(state->mode));
>   state->enable = false;
> @@ -386,10 +398,23 @@ int drm_atomic_set_mode_prop_for_crtc(struct 
> drm_crtc_state *state,
>   memset(>mode, 0, sizeof(state->mode));
>  
>   if (blob) {
> + struct drm_mode_modeinfo *ar_umode;
> + struct drm_atomic_state *atomic_state;
> +
> + /*
> +  * If the user-space does not ask for aspect-ratio
> +  * the aspect ratio bits in the drmModeModeinfo from user,
> +  * does not mean anything. Therefore these bits must be
> +  * discarded.
> +  */
> + ar_umode = (struct drm_mode_modeinfo *) blob->data;
> + atomic_state = state->state;
> + if (atomic_state && !atomic_state->allow_aspect_ratio)
> + ar_umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
>   if (blob->length != sizeof(struct drm_mode_modeinfo) ||
>   drm_mode_convert_umode(>mode,
> -(const struct drm_mode_modeinfo *)
> - blob->data))
> +(const struct drm_mode_modeinfo *)
> + ar_umode))
>   return -EINVAL;
>  
>   state->mode_blob = drm_property_blob_get(blob);
> @@ -2229,6 +2254,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  
>   state->acquire_ctx = 
>   state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
> + state->allow_aspect_ratio = 

[REBASE 2/5] drm: Handle aspect ratio in modeset paths

[Shashank Sharma]

From: Ankit Nautiyal 

If the user mode does not support aspect-ratio, and requests for
a modeset with aspect ratio flags, then the flag bits reprsenting
aspect ratio must be ignored.

Similarly, if user space doesn't set the aspect ratio client cap,
while preparing a usermode, the aspect-ratio info must not be given
into it.

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.

Cc: Ville Syrjala 
Cc: Shashank Sharma 
Cc: Jose Abreu 
Signed-off-by: Ankit Nautiyal 
---
 drivers/gpu/drm/drm_atomic.c | 40 +---
 drivers/gpu/drm/drm_crtc.c   | 19 +++
 include/drm/drm_atomic.h |  2 ++
 3 files changed, 54 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 37445d5..b9743af 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -338,18 +338,30 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state 
*state,
state->mode_blob = NULL;
 
if (mode) {
-   drm_mode_convert_to_umode(, mode);
+   /*
+* If the user has not asked for aspect ratio support,
+* take a copy of the 'mode' and set the aspect ratio as
+* None. This copy is used to prepare the user-mode with no
+* aspect-ratio info.
+*/
+   struct drm_display_mode ar_mode;
+   struct drm_atomic_state *atomic_state = state->state;
+
+   drm_mode_copy(_mode, mode);
+   if (atomic_state && !atomic_state->allow_aspect_ratio)
+   ar_mode.picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
+   drm_mode_convert_to_umode(, _mode);
state->mode_blob =
drm_property_create_blob(state->crtc->dev,
-sizeof(umode),
-);
+sizeof(umode),
+);
if (IS_ERR(state->mode_blob))
return PTR_ERR(state->mode_blob);
 
-   drm_mode_copy(>mode, mode);
+   drm_mode_copy(>mode, _mode);
state->enable = true;
DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
-mode->name, state);
+ar_mode.name, state);
} else {
memset(>mode, 0, sizeof(state->mode));
state->enable = false;
@@ -386,10 +398,23 @@ int drm_atomic_set_mode_prop_for_crtc(struct 
drm_crtc_state *state,
memset(>mode, 0, sizeof(state->mode));
 
if (blob) {
+   struct drm_mode_modeinfo *ar_umode;
+   struct drm_atomic_state *atomic_state;
+
+   /*
+* If the user-space does not ask for aspect-ratio
+* the aspect ratio bits in the drmModeModeinfo from user,
+* does not mean anything. Therefore these bits must be
+* discarded.
+*/
+   ar_umode = (struct drm_mode_modeinfo *) blob->data;
+   atomic_state = state->state;
+   if (atomic_state && !atomic_state->allow_aspect_ratio)
+   ar_umode->flags &= (~DRM_MODE_FLAG_PIC_AR_MASK);
if (blob->length != sizeof(struct drm_mode_modeinfo) ||
drm_mode_convert_umode(>mode,
-  (const struct drm_mode_modeinfo *)
-   blob->data))
+  (const struct drm_mode_modeinfo *)
+   ar_umode))
return -EINVAL;
 
state->mode_blob = drm_property_blob_get(blob);
@@ -2229,6 +2254,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 
state->acquire_ctx = 
state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
+   state->allow_aspect_ratio = file_priv->aspect_ratio_allowed;
 
 retry:
plane_mask = 0;
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(>mutex, NULL);
if (crtc->state) {
if