From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Nautiyal, Ankit K" Subject: Re: [PATCH v12 07/10] drm: Handle aspect ratio info in legacy modeset path Date: Mon, 30 Apr 2018 11:51:29 +0530 Message-ID: References: <1524831296-11198-1-git-send-email-ankit.k.nautiyal@intel.com> <1524831296-11198-8-git-send-email-ankit.k.nautiyal@intel.com> <20180427135453.GW23723@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0412797088==" Return-path: In-Reply-To: <20180427135453.GW23723@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= Cc: "intel-gfx@lists.freedesktop.org" , "ppaalanen@gmail.com" , "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org This is a multi-part message in MIME format. --===============0412797088== Content-Type: multipart/alternative; boundary="------------17789AE2B15DFE4B722FE010" This is a multi-part message in MIME format. --------------17789AE2B15DFE4B722FE010 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit On 4/27/2018 7:24 PM, Ville Syrjälä wrote: > On Fri, Apr 27, 2018 at 05:44:53PM +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. >> --- >> drivers/gpu/drm/drm_crtc.c | 8 ++++++++ >> drivers/gpu/drm/drm_modes.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ >> include/drm/drm_modes.h | 4 ++++ >> 3 files changed, 57 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, &crtc_resp->mode); >> drm_modeset_unlock(&crtc->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, >> + &crtc_req->mode)) { >> + DRM_DEBUG_KMS("Unexpected aspect-ratio flag bits\n"); >> + ret = -EINVAL; >> + goto out; >> + } >> + >> >> ret = drm_mode_convert_umode(dev, mode, &crtc_req->mode); >> if (ret) { >> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c >> index c395a24..d6f68c8 100644 >> --- a/drivers/gpu/drm/drm_modes.c >> +++ b/drivers/gpu/drm/drm_modes.c >> @@ -1759,3 +1759,48 @@ 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) >> +{ >> + return file_priv->aspect_ratio_allowed || (umode->flags & >> + DRM_MODE_FLAG_PIC_AR_MASK) == DRM_MODE_FLAG_PIC_AR_NONE; > Still looks funny. Alright. Let me change it to: 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 capabilities. >> + * @umode: drm_mode_modeinfo struct, whose flags' aspect-ratio bits needs to >> + * be filtered. >> + * >> + */ >> +void >> +drm_mode_filter_aspect_ratio_flags(const struct drm_file *file_priv, >> + struct drm_mode_modeinfo *umode) >> +{ >> + if (!drm_mode_aspect_ratio_allowed(file_priv, umode)) >> + umode->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; >> +} >> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h >> index 2f78b7e..e0b060d 100644 >> --- a/include/drm/drm_modes.h >> +++ b/include/drm/drm_modes.h >> @@ -461,6 +461,10 @@ bool drm_mode_is_420_also(const struct drm_display_info *display, >> const struct drm_display_mode *mode); >> bool drm_mode_is_420(const struct drm_display_info *display, >> const struct drm_display_mode *mode); >> +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); > Since these are no longer exported the prototypes should go into > drivers/gpu/drm/drm_crtc_internal.h Got it, will move the prototypes to the correct place. Thanks for pointing that out. Regards, Ankit > >> struct drm_display_mode *drm_cvt_mode(struct drm_device *dev, >> int hdisplay, int vdisplay, int vrefresh, >> -- >> 2.7.4 > -- > Ville Syrjälä > Intel --------------17789AE2B15DFE4B722FE010 Content-Type: text/html; charset=windows-1252 Content-Transfer-Encoding: 8bit
On 4/27/2018 7:24 PM, Ville Syrjälä wrote:
On Fri, Apr 27, 2018 at 05:44:53PM +0530, Nautiyal, Ankit K wrote:

From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

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

This patch:
1. 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 <ankit.k.nautiyal@intel.com>

V3: Addressed review comments from Ville:
    Do not corrupt the current crtc state by updating aspect-ratio on
    the fly.
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.
---
 drivers/gpu/drm/drm_crtc.c  |  8 ++++++++
 drivers/gpu/drm/drm_modes.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_modes.h     |  4 ++++
 3 files changed, 57 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, &crtc_resp->mode);
      drm_modeset_unlock(&crtc->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,
+                                                &crtc_req->mode)) {
+                     DRM_DEBUG_KMS("Unexpected aspect-ratio flag bits\n");
+                     ret = -EINVAL;
+                     goto out;
+             }
+

              ret = drm_mode_convert_umode(dev, mode, &crtc_req->mode);
              if (ret) {
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index c395a24..d6f68c8 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1759,3 +1759,48 @@ 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)
+{
+     return file_priv->aspect_ratio_allowed || (umode->flags &
+             DRM_MODE_FLAG_PIC_AR_MASK) == DRM_MODE_FLAG_PIC_AR_NONE;

Still looks funny.

Alright. Let me change it to:
        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 capabilities.
+ * @umode: drm_mode_modeinfo struct, whose flags' aspect-ratio bits needs to
+ * be filtered.
+ *
+ */
+void
+drm_mode_filter_aspect_ratio_flags(const struct drm_file *file_priv,
+                                struct drm_mode_modeinfo *umode)
+{
+     if (!drm_mode_aspect_ratio_allowed(file_priv, umode))
+             umode->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK;
+}
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index 2f78b7e..e0b060d 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -461,6 +461,10 @@ bool drm_mode_is_420_also(const struct drm_display_info *display,
                        const struct drm_display_mode *mode);
 bool drm_mode_is_420(const struct drm_display_info *display,
                   const struct drm_display_mode *mode);
+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);


      

      
Since these are no longer exported the prototypes should go into
drivers/gpu/drm/drm_crtc_internal.h

    

    

    Got it, will move the prototypes to the correct
      place. 

      Thanks for pointing that out.

      

      Regards,

      Ankit

      

    

    

      



      

        
 struct drm_display_mode *drm_cvt_mode(struct drm_device *dev,
                                    int hdisplay, int vdisplay, int vrefresh,
--
2.7.4


      

      
--
Ville Syrjälä
Intel


    

    

  


--------------17789AE2B15DFE4B722FE010--

--===============0412797088==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline

X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4
IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz
dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg==

--===============0412797088==--