Re: [Intel-gfx] [PATCH v13 07/10] drm: Handle aspect ratio info in legacy modeset path

2018-05-02 Thread Daniel Vetter 
On Wed, May 02, 2018 at 12:20:19PM +0530, Nautiyal, Ankit K wrote:
> From: Ankit Nautiyal 
> 
> If the user-space does not support aspect-ratio, and requests for a
> modeset with mode having aspect ratio bits set, then the given
> user-mode must be rejected. Secondly, while preparing a user-mode from
> kernel mode, the aspect-ratio info must not be given, if aspect-ratio
> is not supported by the user.
> 
> This patch:
> 1. rejects the modes with aspect-ratio info, during modeset, if the
>user does not support aspect ratio.
> 2. does not load the aspect-ratio info in user-mode structure, if
>aspect ratio is not supported.
> 3. adds helper functions for determining if aspect-ratio is expected
>in user-mode and for allowing/disallowing the aspect-ratio, if its
>not expected.
> 
> Signed-off-by: Ankit Nautiyal 
> 
> V3: Addressed review comments from Ville:
> Do not corrupt the current crtc state by updating aspect-ratio on
> the fly.
> V4: rebase
> V5: As suggested by Ville, rejected the modeset calls for modes with
> aspect ratio, if the user does not set aspect-ratio cap.
> V6: Used the helper functions for determining if aspect-ratio is
> expected in the user-mode.
> V7: rebase
> V8: rebase
> V9: rebase
> V10: Modified the commit-message
> V11: rebase
> V12: Merged the patch for adding aspect-ratio helper functions
>  with this patch.
> V13: Minor modifications as suggested by Ville.
> ---
>  drivers/gpu/drm/drm_crtc.c  |  8 +++
>  drivers/gpu/drm/drm_crtc_internal.h |  6 +
>  drivers/gpu/drm/drm_modes.c | 47 
> +
>  3 files changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index a231dd5..98323f4 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -449,6 +449,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
>   crtc_resp->mode_valid = 0;
>   }
>   }
> + drm_mode_filter_aspect_ratio_flags(file_priv, _resp->mode);
>   drm_modeset_unlock(>mutex);
>  
>   return 0;
> @@ -628,6 +629,13 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>   ret = -ENOMEM;
>   goto out;
>   }
> + if (!drm_mode_aspect_ratio_allowed(file_priv,
> +_req->mode)) {
> + DRM_DEBUG_KMS("Unexpected aspect-ratio flag bits\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
>  
>   ret = drm_mode_convert_umode(dev, mode, _req->mode);
>   if (ret) {
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h 
> b/drivers/gpu/drm/drm_crtc_internal.h
> index 5d307b2..31d6c77 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -222,3 +222,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  void drm_mode_fixup_1366x768(struct drm_display_mode *mode);
>  void drm_reset_display_info(struct drm_connector *connector);
>  u32 drm_add_display_info(struct drm_connector *connector, const struct edid 
> *edid);
> +
> +/* drm_modes.c */
> +bool drm_mode_aspect_ratio_allowed(const struct drm_file *file_priv,
> +struct drm_mode_modeinfo *umode);
> +void drm_mode_filter_aspect_ratio_flags(const struct drm_file *file_priv,
> + struct drm_mode_modeinfo *umode);
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index c395a24..2bf2f0b 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -1759,3 +1759,50 @@ bool drm_mode_is_420(const struct drm_display_info 
> *display,
>   drm_mode_is_420_also(display, mode);
>  }
>  EXPORT_SYMBOL(drm_mode_is_420);
> +
> +/**
> + * drm_mode_aspect_ratio_allowed - checks if the aspect-ratio information
> + * is expected from the user-mode.
> + *
> + * If the user has set aspect-ratio cap, then the flag of the user-mode is
> + * allowed to contain aspect-ratio value.
> + * If the user does not set aspect-ratio cap, then the only value allowed in 
> the
> + * flags bits is aspect-ratio NONE.
> + *
> + * @file_priv: file private structure to get the user capabilities
> + * @umode: drm_mode_modeinfo struct, whose flag carry the aspect ratio
> + * information.
> + *
> + * Returns:
> + * true if the aspect-ratio info is allowed in the user-mode flags.
> + * false, otherwise.
> + */

We generally don't do full kerneldoc for drm.ko internal functions (which
these both are), only for driver functions. I'd remove them both because
they don't add a hole lot really.

> +bool
> +drm_mode_aspect_ratio_allowed(const struct drm_file *file_priv,
> +   struct drm_mode_modeinfo *umode)

I don't really see the point of this helper. It has a bit a confusing

[Intel-gfx] [PATCH v13 07/10] drm: Handle aspect ratio info in legacy modeset path

2018-05-02 Thread Nautiyal, Ankit K 
From: Ankit Nautiyal 

If the user-space does not support aspect-ratio, and requests for a
modeset with mode having aspect ratio bits set, then the given
user-mode must be rejected. Secondly, while preparing a user-mode from
kernel mode, the aspect-ratio info must not be given, if aspect-ratio
is not supported by the user.

This patch:
1. rejects the modes with aspect-ratio info, during modeset, if the
   user does not support aspect ratio.
2. does not load the aspect-ratio info in user-mode structure, if
   aspect ratio is not supported.
3. adds helper functions for determining if aspect-ratio is expected
   in user-mode and for allowing/disallowing the aspect-ratio, if its
   not expected.

Signed-off-by: Ankit Nautiyal 

V3: Addressed review comments from Ville:
Do not corrupt the current crtc state by updating aspect-ratio on
the fly.
V4: rebase
V5: As suggested by Ville, rejected the modeset calls for modes with
aspect ratio, if the user does not set aspect-ratio cap.
V6: Used the helper functions for determining if aspect-ratio is
expected in the user-mode.
V7: rebase
V8: rebase
V9: rebase
V10: Modified the commit-message
V11: rebase
V12: Merged the patch for adding aspect-ratio helper functions
 with this patch.
V13: Minor modifications as suggested by Ville.
---
 drivers/gpu/drm/drm_crtc.c  |  8 +++
 drivers/gpu/drm/drm_crtc_internal.h |  6 +
 drivers/gpu/drm/drm_modes.c | 47 +
 3 files changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index a231dd5..98323f4 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -449,6 +449,7 @@ int drm_mode_getcrtc(struct drm_device *dev,
crtc_resp->mode_valid = 0;
}
}
+   drm_mode_filter_aspect_ratio_flags(file_priv, _resp->mode);
drm_modeset_unlock(>mutex);
 
return 0;
@@ -628,6 +629,13 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
ret = -ENOMEM;
goto out;
}
+   if (!drm_mode_aspect_ratio_allowed(file_priv,
+  _req->mode)) {
+   DRM_DEBUG_KMS("Unexpected aspect-ratio flag bits\n");
+   ret = -EINVAL;
+   goto out;
+   }
+
 
ret = drm_mode_convert_umode(dev, mode, _req->mode);
if (ret) {
diff --git a/drivers/gpu/drm/drm_crtc_internal.h 
b/drivers/gpu/drm/drm_crtc_internal.h
index 5d307b2..31d6c77 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -222,3 +222,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 void drm_mode_fixup_1366x768(struct drm_display_mode *mode);
 void drm_reset_display_info(struct drm_connector *connector);
 u32 drm_add_display_info(struct drm_connector *connector, const struct edid 
*edid);
+
+/* drm_modes.c */
+bool drm_mode_aspect_ratio_allowed(const struct drm_file *file_priv,
+  struct drm_mode_modeinfo *umode);
+void drm_mode_filter_aspect_ratio_flags(const struct drm_file *file_priv,
+   struct drm_mode_modeinfo *umode);
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index c395a24..2bf2f0b 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1759,3 +1759,50 @@ bool drm_mode_is_420(const struct drm_display_info 
*display,
drm_mode_is_420_also(display, mode);
 }
 EXPORT_SYMBOL(drm_mode_is_420);
+
+/**
+ * drm_mode_aspect_ratio_allowed - checks if the aspect-ratio information
+ * is expected from the user-mode.
+ *
+ * If the user has set aspect-ratio cap, then the flag of the user-mode is
+ * allowed to contain aspect-ratio value.
+ * If the user does not set aspect-ratio cap, then the only value allowed in 
the
+ * flags bits is aspect-ratio NONE.
+ *
+ * @file_priv: file private structure to get the user capabilities
+ * @umode: drm_mode_modeinfo struct, whose flag carry the aspect ratio
+ * information.
+ *
+ * Returns:
+ * true if the aspect-ratio info is allowed in the user-mode flags.
+ * false, otherwise.
+ */
+bool
+drm_mode_aspect_ratio_allowed(const struct drm_file *file_priv,
+ struct drm_mode_modeinfo *umode)
+{
+   if (file_priv->aspect_ratio_allowed ||
+   (umode->flags & DRM_MODE_FLAG_PIC_AR_MASK) == 
DRM_MODE_FLAG_PIC_AR_NONE)
+   return true;
+   return false;
+}
+
+/**
+ * drm_mode_filter_aspect_ratio_flags - filters the aspect-ratio bits in the
+ * user-mode flags.
+ *
+ * Checks if the aspect-ratio information is allowed. Resets the aspect-ratio
+ * bits in the user-mode flags, if aspect-ratio info is not allowed.
+ *
+ * @file_priv: file private structure to get the user