Re: [PATCH v11 09/11] drm: Expose modes with aspect ratio, only if requested

[Ville Syrjälä]

On Mon, Apr 23, 2018 at 11:19:08AM +0530, Nautiyal, Ankit K wrote:
> 
> 
> On 4/20/2018 7:52 PM, Ville Syrjälä wrote:
> > On Fri, Apr 20, 2018 at 07:01:49PM +0530, Nautiyal, Ankit K wrote:
> >> From: Ankit Nautiyal 
> >>
> >> We parse the EDID and add all the modes in the connector's modelist.
> >> This adds CEA modes with aspect ratio information too, regardless of
> >> whether user space requested this information or not.
> >>
> >> This patch prunes the modes with aspect-ratio information, from a
> >> connector's modelist, if the user-space has not set the aspect ratio
> >> DRM client cap. However if such a mode is unique in the list, it is
> >> kept in the list, with aspect-ratio flags reset.
> >>
> >> Cc: Ville Syrjala 
> >> Cc: Shashank Sharma 
> >> Cc: Jose Abreu 
> >>
> >> Signed-off-by: Ankit Nautiyal 
> >>
> >> V3: As suggested by Ville, modified the mechanism of pruning of modes
> >>  with aspect-ratio, if the aspect-ratio is not supported. Instead
> >>  of straight away pruning such a mode, the mode is retained with
> >>  aspect ratio bits set to zero, provided it is unique.
> >> V4: rebase
> >> V5: Addressed review comments from Ville:
> >>  -used a pointer to store last valid mode.
> >>  -avoided, modifying of picture_aspect_ratio in kernel mode,
> >>   instead only flags bits of user mode are reset (if aspect-ratio
> >>   is not supported).
> >> V6: As suggested by Ville, corrected the mode pruning logic and
> >>  elaborated the mode pruning logic and the assumptions taken.
> >> V7: rebase
> >> V8: rebase
> >> V9: rebase
> >> V10: rebase
> >> V11: Fixed the issue caused in kms_3d test, and enhanced the pruning
> >>   logic to correctly identify and prune modes with aspect-ratio,
> >>   if aspect-ratio cap is not set.
> >> ---
> >>   drivers/gpu/drm/drm_connector.c | 56 
> >> +++--
> >>   1 file changed, 48 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_connector.c 
> >> b/drivers/gpu/drm/drm_connector.c
> >> index b3cde89..865ee354 100644
> >> --- a/drivers/gpu/drm/drm_connector.c
> >> +++ b/drivers/gpu/drm/drm_connector.c
> >> @@ -1531,15 +1531,35 @@ static struct drm_encoder 
> >> *drm_connector_get_encoder(struct drm_connector *conne
> >>return connector->encoder;
> >>   }
> >>   
> >> -static bool drm_mode_expose_to_userspace(const struct drm_display_mode 
> >> *mode,
> >> -   const struct drm_file *file_priv)
> >> +static bool
> >> +drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
> >> +   const struct drm_display_mode *modelist,
> >> +   const struct drm_file *file_priv)
> >>   {
> >>/*
> >> * If user-space hasn't configured the driver to expose the stereo 3D
> >> * modes, don't expose them.
> >> */
> >> +  struct drm_display_mode *mode_itr;
> >> +
> >>if (!file_priv->stereo_allowed && drm_mode_is_stereo(mode))
> >>return false;
> >> +  /*
> >> +   * If user-space hasn't configured the driver to expose the modes
> >> +   * with aspect-ratio, don't expose them. However if such a mode
> >> +   * is unique, let it be exposed, but reset the aspect-ratio flags
> >> +   * while preparing the list of user-modes.
> >> +   */
> >> +  if (!file_priv->aspect_ratio_allowed &&
> >> +  mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {
> >> +  list_for_each_entry(mode_itr, &modelist->head, head)
> >> +  if (drm_mode_match(mode_itr, mode,
> >> + DRM_MODE_MATCH_TIMINGS |
> >> + DRM_MODE_MATCH_CLOCK |
> >> + DRM_MODE_MATCH_FLAGS |
> >> + DRM_MODE_MATCH_3D_FLAGS))
> >> +  return false;
> >> +  }
> >>   
> >>return true;
> >>   }
> >> @@ -1550,7 +1570,7 @@ int drm_mode_getconnector(struct drm_device *dev, 
> >> void *data,
> >>struct drm_mode_get_connector *out_resp = data;
> >>struct drm_connector *connector;
> >>struct drm_encoder *encoder;
> >> -  struct drm_display_mode *mode;
> >> +  struct drm_display_mode *mode, *tmp, modelist;
> >>int mode_count = 0;
> >>int encoders_count = 0;
> >>int ret = 0;
> >> @@ -1605,23 +1625,37 @@ int drm_mode_getconnector(struct drm_device *dev, 
> >> void *data,
> >>out_resp->subpixel = connector->display_info.subpixel_order;
> >>out_resp->connection = connector->status;
> >>   
> >> +  INIT_LIST_HEAD(&modelist.head);
> > Why are we using a struct drm_display_mode to get a simple list_head?
> 
> Yes you are right, we can use the simple list_head here. I was trying to 
> use modelist as first mode, and goofed up.
> Its a mistake, I will correct it in next patchset.
> 
> >
> >> +
> >>/* delayed so we get modes regardless of pre-fill_modes state */
> >>list_for_each_entry(mode, &connector->modes, head)
> >> 

Re: [PATCH v11 09/11] drm: Expose modes with aspect ratio, only if requested

[Nautiyal, Ankit K]

On 4/20/2018 7:52 PM, Ville Syrjälä wrote:

On Fri, Apr 20, 2018 at 07:01:49PM +0530, Nautiyal, Ankit K wrote:

From: Ankit Nautiyal 

We parse the EDID and add all the modes in the connector's modelist.
This adds CEA modes with aspect ratio information too, regardless of
whether user space requested this information or not.

This patch prunes the modes with aspect-ratio information, from a
connector's modelist, if the user-space has not set the aspect ratio
DRM client cap. However if such a mode is unique in the list, it is
kept in the list, with aspect-ratio flags reset.

Cc: Ville Syrjala 
Cc: Shashank Sharma 
Cc: Jose Abreu 

Signed-off-by: Ankit Nautiyal 

V3: As suggested by Ville, modified the mechanism of pruning of modes
 with aspect-ratio, if the aspect-ratio is not supported. Instead
 of straight away pruning such a mode, the mode is retained with
 aspect ratio bits set to zero, provided it is unique.
V4: rebase
V5: Addressed review comments from Ville:
 -used a pointer to store last valid mode.
 -avoided, modifying of picture_aspect_ratio in kernel mode,
  instead only flags bits of user mode are reset (if aspect-ratio
  is not supported).
V6: As suggested by Ville, corrected the mode pruning logic and
 elaborated the mode pruning logic and the assumptions taken.
V7: rebase
V8: rebase
V9: rebase
V10: rebase
V11: Fixed the issue caused in kms_3d test, and enhanced the pruning
  logic to correctly identify and prune modes with aspect-ratio,
  if aspect-ratio cap is not set.
---
  drivers/gpu/drm/drm_connector.c | 56 +++--
  1 file changed, 48 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index b3cde89..865ee354 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1531,15 +1531,35 @@ static struct drm_encoder 
*drm_connector_get_encoder(struct drm_connector *conne
return connector->encoder;
  }
  
-static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,

-const struct drm_file *file_priv)
+static bool
+drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
+const struct drm_display_mode *modelist,
+const struct drm_file *file_priv)
  {
/*
 * If user-space hasn't configured the driver to expose the stereo 3D
 * modes, don't expose them.
 */
+   struct drm_display_mode *mode_itr;
+
if (!file_priv->stereo_allowed && drm_mode_is_stereo(mode))
return false;
+   /*
+* If user-space hasn't configured the driver to expose the modes
+* with aspect-ratio, don't expose them. However if such a mode
+* is unique, let it be exposed, but reset the aspect-ratio flags
+* while preparing the list of user-modes.
+*/
+   if (!file_priv->aspect_ratio_allowed &&
+   mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {
+   list_for_each_entry(mode_itr, &modelist->head, head)
+   if (drm_mode_match(mode_itr, mode,
+  DRM_MODE_MATCH_TIMINGS |
+  DRM_MODE_MATCH_CLOCK |
+  DRM_MODE_MATCH_FLAGS |
+  DRM_MODE_MATCH_3D_FLAGS))
+   return false;
+   }
  
  	return true;

  }
@@ -1550,7 +1570,7 @@ int drm_mode_getconnector(struct drm_device *dev, void 
*data,
struct drm_mode_get_connector *out_resp = data;
struct drm_connector *connector;
struct drm_encoder *encoder;
-   struct drm_display_mode *mode;
+   struct drm_display_mode *mode, *tmp, modelist;
int mode_count = 0;
int encoders_count = 0;
int ret = 0;
@@ -1605,23 +1625,37 @@ int drm_mode_getconnector(struct drm_device *dev, void 
*data,
out_resp->subpixel = connector->display_info.subpixel_order;
out_resp->connection = connector->status;
  
+	INIT_LIST_HEAD(&modelist.head);

Why are we using a struct drm_display_mode to get a simple list_head?


Yes you are right, we can use the simple list_head here. I was trying to 
use modelist as first mode, and goofed up.

Its a mistake, I will correct it in next patchset.




+
/* delayed so we get modes regardless of pre-fill_modes state */
list_for_each_entry(mode, &connector->modes, head)
-   if (drm_mode_expose_to_userspace(mode, file_priv))
+   if (drm_mode_expose_to_userspace(mode, &modelist,
+file_priv)) {
+   struct drm_display_mode *tmp_mode;
+
+   tmp_mode = drm_mode_duplicate(dev, mode);

Duplicating every mode seems rather wasteful. I suppose we could
just add another list_head t

Re: [PATCH v11 09/11] drm: Expose modes with aspect ratio, only if requested

[Ville Syrjälä]

On Fri, Apr 20, 2018 at 07:01:49PM +0530, Nautiyal, Ankit K wrote:
> From: Ankit Nautiyal 
> 
> We parse the EDID and add all the modes in the connector's modelist.
> This adds CEA modes with aspect ratio information too, regardless of
> whether user space requested this information or not.
> 
> This patch prunes the modes with aspect-ratio information, from a
> connector's modelist, if the user-space has not set the aspect ratio
> DRM client cap. However if such a mode is unique in the list, it is
> kept in the list, with aspect-ratio flags reset.
> 
> Cc: Ville Syrjala 
> Cc: Shashank Sharma 
> Cc: Jose Abreu 
> 
> Signed-off-by: Ankit Nautiyal 
> 
> V3: As suggested by Ville, modified the mechanism of pruning of modes
> with aspect-ratio, if the aspect-ratio is not supported. Instead
> of straight away pruning such a mode, the mode is retained with
> aspect ratio bits set to zero, provided it is unique.
> V4: rebase
> V5: Addressed review comments from Ville:
> -used a pointer to store last valid mode.
> -avoided, modifying of picture_aspect_ratio in kernel mode,
>  instead only flags bits of user mode are reset (if aspect-ratio
>  is not supported).
> V6: As suggested by Ville, corrected the mode pruning logic and
> elaborated the mode pruning logic and the assumptions taken.
> V7: rebase
> V8: rebase
> V9: rebase
> V10: rebase
> V11: Fixed the issue caused in kms_3d test, and enhanced the pruning
>  logic to correctly identify and prune modes with aspect-ratio,
>  if aspect-ratio cap is not set.
> ---
>  drivers/gpu/drm/drm_connector.c | 56 
> +++--
>  1 file changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index b3cde89..865ee354 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1531,15 +1531,35 @@ static struct drm_encoder 
> *drm_connector_get_encoder(struct drm_connector *conne
>   return connector->encoder;
>  }
>  
> -static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
> -  const struct drm_file *file_priv)
> +static bool
> +drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
> +  const struct drm_display_mode *modelist,
> +  const struct drm_file *file_priv)
>  {
>   /*
>* If user-space hasn't configured the driver to expose the stereo 3D
>* modes, don't expose them.
>*/
> + struct drm_display_mode *mode_itr;
> +
>   if (!file_priv->stereo_allowed && drm_mode_is_stereo(mode))
>   return false;
> + /*
> +  * If user-space hasn't configured the driver to expose the modes
> +  * with aspect-ratio, don't expose them. However if such a mode
> +  * is unique, let it be exposed, but reset the aspect-ratio flags
> +  * while preparing the list of user-modes.
> +  */
> + if (!file_priv->aspect_ratio_allowed &&
> + mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {
> + list_for_each_entry(mode_itr, &modelist->head, head)
> + if (drm_mode_match(mode_itr, mode,
> +DRM_MODE_MATCH_TIMINGS |
> +DRM_MODE_MATCH_CLOCK |
> +DRM_MODE_MATCH_FLAGS |
> +DRM_MODE_MATCH_3D_FLAGS))
> + return false;
> + }
>  
>   return true;
>  }
> @@ -1550,7 +1570,7 @@ int drm_mode_getconnector(struct drm_device *dev, void 
> *data,
>   struct drm_mode_get_connector *out_resp = data;
>   struct drm_connector *connector;
>   struct drm_encoder *encoder;
> - struct drm_display_mode *mode;
> + struct drm_display_mode *mode, *tmp, modelist;
>   int mode_count = 0;
>   int encoders_count = 0;
>   int ret = 0;
> @@ -1605,23 +1625,37 @@ int drm_mode_getconnector(struct drm_device *dev, 
> void *data,
>   out_resp->subpixel = connector->display_info.subpixel_order;
>   out_resp->connection = connector->status;
>  
> + INIT_LIST_HEAD(&modelist.head);

Why are we using a struct drm_display_mode to get a simple list_head?

> +
>   /* delayed so we get modes regardless of pre-fill_modes state */
>   list_for_each_entry(mode, &connector->modes, head)
> - if (drm_mode_expose_to_userspace(mode, file_priv))
> + if (drm_mode_expose_to_userspace(mode, &modelist,
> +  file_priv)) {
> + struct drm_display_mode *tmp_mode;
> +
> + tmp_mode = drm_mode_duplicate(dev, mode);

Duplicating every mode seems rather wasteful. I suppose we could
just add another list_head to struct drm_display_mode for this
purpose. An alternative could be to somehow mark each exposed mode
an

[PATCH v11 09/11] drm: Expose modes with aspect ratio, only if requested

[Nautiyal, Ankit K]

From: Ankit Nautiyal 

We parse the EDID and add all the modes in the connector's modelist.
This adds CEA modes with aspect ratio information too, regardless of
whether user space requested this information or not.

This patch prunes the modes with aspect-ratio information, from a
connector's modelist, if the user-space has not set the aspect ratio
DRM client cap. However if such a mode is unique in the list, it is
kept in the list, with aspect-ratio flags reset.

Cc: Ville Syrjala 
Cc: Shashank Sharma 
Cc: Jose Abreu 

Signed-off-by: Ankit Nautiyal 

V3: As suggested by Ville, modified the mechanism of pruning of modes
with aspect-ratio, if the aspect-ratio is not supported. Instead
of straight away pruning such a mode, the mode is retained with
aspect ratio bits set to zero, provided it is unique.
V4: rebase
V5: Addressed review comments from Ville:
-used a pointer to store last valid mode.
-avoided, modifying of picture_aspect_ratio in kernel mode,
 instead only flags bits of user mode are reset (if aspect-ratio
 is not supported).
V6: As suggested by Ville, corrected the mode pruning logic and
elaborated the mode pruning logic and the assumptions taken.
V7: rebase
V8: rebase
V9: rebase
V10: rebase
V11: Fixed the issue caused in kms_3d test, and enhanced the pruning
 logic to correctly identify and prune modes with aspect-ratio,
 if aspect-ratio cap is not set.
---
 drivers/gpu/drm/drm_connector.c | 56 +++--
 1 file changed, 48 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index b3cde89..865ee354 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1531,15 +1531,35 @@ static struct drm_encoder 
*drm_connector_get_encoder(struct drm_connector *conne
return connector->encoder;
 }
 
-static bool drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
-const struct drm_file *file_priv)
+static bool
+drm_mode_expose_to_userspace(const struct drm_display_mode *mode,
+const struct drm_display_mode *modelist,
+const struct drm_file *file_priv)
 {
/*
 * If user-space hasn't configured the driver to expose the stereo 3D
 * modes, don't expose them.
 */
+   struct drm_display_mode *mode_itr;
+
if (!file_priv->stereo_allowed && drm_mode_is_stereo(mode))
return false;
+   /*
+* If user-space hasn't configured the driver to expose the modes
+* with aspect-ratio, don't expose them. However if such a mode
+* is unique, let it be exposed, but reset the aspect-ratio flags
+* while preparing the list of user-modes.
+*/
+   if (!file_priv->aspect_ratio_allowed &&
+   mode->picture_aspect_ratio != HDMI_PICTURE_ASPECT_NONE) {
+   list_for_each_entry(mode_itr, &modelist->head, head)
+   if (drm_mode_match(mode_itr, mode,
+  DRM_MODE_MATCH_TIMINGS |
+  DRM_MODE_MATCH_CLOCK |
+  DRM_MODE_MATCH_FLAGS |
+  DRM_MODE_MATCH_3D_FLAGS))
+   return false;
+   }
 
return true;
 }
@@ -1550,7 +1570,7 @@ int drm_mode_getconnector(struct drm_device *dev, void 
*data,
struct drm_mode_get_connector *out_resp = data;
struct drm_connector *connector;
struct drm_encoder *encoder;
-   struct drm_display_mode *mode;
+   struct drm_display_mode *mode, *tmp, modelist;
int mode_count = 0;
int encoders_count = 0;
int ret = 0;
@@ -1605,23 +1625,37 @@ int drm_mode_getconnector(struct drm_device *dev, void 
*data,
out_resp->subpixel = connector->display_info.subpixel_order;
out_resp->connection = connector->status;
 
+   INIT_LIST_HEAD(&modelist.head);
+
/* delayed so we get modes regardless of pre-fill_modes state */
list_for_each_entry(mode, &connector->modes, head)
-   if (drm_mode_expose_to_userspace(mode, file_priv))
+   if (drm_mode_expose_to_userspace(mode, &modelist,
+file_priv)) {
+   struct drm_display_mode *tmp_mode;
+
+   tmp_mode = drm_mode_duplicate(dev, mode);
+   list_add_tail(&tmp_mode->head, &modelist.head);
mode_count++;
+   }
 
/*
 * This ioctl is called twice, once to determine how much space is
 * needed, and the 2nd time to fill it.
+* The modes that need to be exposed to the user are maintained in the
+* 'modelist'. When the ioctl is called first time to determine the,
+* space, the modelist gets filled,