Re: [Intel-gfx] [PATCH v13 08/10] drm: Expose modes with aspect ratio, only if requested

[Daniel Vetter]

On Fri, May 04, 2018 at 11:19:45PM +0300, Ville Syrjälä wrote:
> On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:
> > On Wed, May 02, 2018 at 12:20:20PM +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.
> > > -prepares a list of exposed modes, which is used to find unique modes
> > >  if aspect-ratio is not allowed.
> > > -adds a new list_head 'exposed_head' in drm_mode_display, to traverse
> > >  the list of exposed modes.
> > > 
> > > 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.
> > > V12: As suggested by Ville, added another list_head in
> > >  drm_mode_display to traverse the list of exposed modes and
> > >  avoided duplication of modes.
> > > V13: Minor modifications, as suggested by Ville.
> > > ---
> > >  drivers/gpu/drm/drm_connector.c | 45 
> > > ++---
> > >  include/drm/drm_modes.h | 13 
> > >  2 files changed, 51 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_connector.c 
> > > b/drivers/gpu/drm/drm_connector.c
> > > index dfc8ca1..8ca1149 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -1531,15 +1531,36 @@ 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 list_head *export_list,
> > > +  const struct drm_file *file_priv)
> > >  {
> > >   /*
> > >* If user-space hasn't configured the driver to expose the stereo 3D
> > >* modes, don't expose them.
> > >*/
> > > +
> > >   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) {
> > > + struct drm_display_mode *mode_itr;
> > > +
> > > + list_for_each_entry(mode_itr, export_list, export_head)
> > 
> > By walking the list of only the modes already added to the export list we
> > rely on ASPECT_NONE being first if present. That seems to be a bit
> > fragile. If we instead walk over all present modes (i.e. connector->modes)
> > then I think that's avoided.
> 
> I don't think that would work. If we just walk over all the modes then
> wouldn't we always find a duplicate when the same mode with two different
> aspect ratios is on the list? And then we'd export neither? Or maybe I
> misunderstood what you mean here.

We'd export both. Let's assume we have 2 modes, only difference is aspect
ratio.

1. We check the first mode, it has aspect_ratio != NONE. But because
there's no other exported mode yet on export_list, we export it (and
filter out 

Re: [Intel-gfx] [PATCH v13 08/10] drm: Expose modes with aspect ratio, only if requested

[Nautiyal, Ankit K]

On 5/7/2018 5:58 PM, Ville Syrjälä wrote:

On Mon, May 07, 2018 at 10:34:53AM +0530, Nautiyal, Ankit K wrote:


On 5/5/2018 1:49 AM, Ville Syrjälä wrote:

On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:

On Wed, May 02, 2018 at 12:20:20PM +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.
-prepares a list of exposed modes, which is used to find unique modes
   if aspect-ratio is not allowed.
-adds a new list_head 'exposed_head' in drm_mode_display, to traverse
   the list of exposed modes.

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.
V12: As suggested by Ville, added another list_head in
   drm_mode_display to traverse the list of exposed modes and
   avoided duplication of modes.
V13: Minor modifications, as suggested by Ville.
---
   drivers/gpu/drm/drm_connector.c | 45 
++---
   include/drm/drm_modes.h | 13 
   2 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index dfc8ca1..8ca1149 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1531,15 +1531,36 @@ 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 list_head *export_list,
+const struct drm_file *file_priv)
   {
/*
 * If user-space hasn't configured the driver to expose the stereo 3D
 * modes, don't expose them.
 */
+
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) {
+   struct drm_display_mode *mode_itr;
+
+   list_for_each_entry(mode_itr, export_list, export_head)

By walking the list of only the modes already added to the export list we
rely on ASPECT_NONE being first if present. That seems to be a bit
fragile. If we instead walk over all present modes (i.e. connector->modes)
then I think that's avoided.

I don't think that would work. If we just walk over all the modes then
wouldn't we always find a duplicate when the same mode with two different
aspect ratios is on the list? And then we'd export neither? Or maybe I
misunderstood what you mean here.

To me it seems like the correct option is to check against the
export_list unconditionally, no matter whether the current mode has the
aspect ratio set or not. If an identical mode is already on the list
we don't export again, if it's not there then we export.

Agreed. The current code does have a problem rightly pointed by Daniel
Vetter and the main reason
for that is - we are checking for duplicates only if the mode has some
aspect-ratio.
As you have suggested, If we can do away with the condition to check for
duplicates 

Re: [Intel-gfx] [PATCH v13 08/10] drm: Expose modes with aspect ratio, only if requested

[Ville Syrjälä]

On Mon, May 07, 2018 at 10:34:53AM +0530, Nautiyal, Ankit K wrote:
> 
> 
> On 5/5/2018 1:49 AM, Ville Syrjälä wrote:
> > On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:
> >> On Wed, May 02, 2018 at 12:20:20PM +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.
> >>> -prepares a list of exposed modes, which is used to find unique modes
> >>>   if aspect-ratio is not allowed.
> >>> -adds a new list_head 'exposed_head' in drm_mode_display, to traverse
> >>>   the list of exposed modes.
> >>>
> >>> 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.
> >>> V12: As suggested by Ville, added another list_head in
> >>>   drm_mode_display to traverse the list of exposed modes and
> >>>   avoided duplication of modes.
> >>> V13: Minor modifications, as suggested by Ville.
> >>> ---
> >>>   drivers/gpu/drm/drm_connector.c | 45 
> >>> ++---
> >>>   include/drm/drm_modes.h | 13 
> >>>   2 files changed, 51 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_connector.c 
> >>> b/drivers/gpu/drm/drm_connector.c
> >>> index dfc8ca1..8ca1149 100644
> >>> --- a/drivers/gpu/drm/drm_connector.c
> >>> +++ b/drivers/gpu/drm/drm_connector.c
> >>> @@ -1531,15 +1531,36 @@ 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 list_head *export_list,
> >>> +  const struct drm_file *file_priv)
> >>>   {
> >>>   /*
> >>>* If user-space hasn't configured the driver to expose the 
> >>> stereo 3D
> >>>* modes, don't expose them.
> >>>*/
> >>> +
> >>>   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) {
> >>> + struct drm_display_mode *mode_itr;
> >>> +
> >>> + list_for_each_entry(mode_itr, export_list, export_head)
> >> By walking the list of only the modes already added to the export list we
> >> rely on ASPECT_NONE being first if present. That seems to be a bit
> >> fragile. If we instead walk over all present modes (i.e. connector->modes)
> >> then I think that's avoided.
> > I don't think that would work. If we just walk over all the modes then
> > wouldn't we always find a duplicate when the same mode with two different
> > aspect ratios is on the list? And then we'd export neither? Or maybe I
> > misunderstood what you mean here.
> >
> > To me it seems like the correct option is to check against the
> > export_list 

Re: [Intel-gfx] [PATCH v13 08/10] drm: Expose modes with aspect ratio, only if requested

[Nautiyal, Ankit K]

On 5/5/2018 1:49 AM, Ville Syrjälä wrote:

On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:

On Wed, May 02, 2018 at 12:20:20PM +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.
-prepares a list of exposed modes, which is used to find unique modes
  if aspect-ratio is not allowed.
-adds a new list_head 'exposed_head' in drm_mode_display, to traverse
  the list of exposed modes.

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.
V12: As suggested by Ville, added another list_head in
  drm_mode_display to traverse the list of exposed modes and
  avoided duplication of modes.
V13: Minor modifications, as suggested by Ville.
---
  drivers/gpu/drm/drm_connector.c | 45 ++---
  include/drm/drm_modes.h | 13 
  2 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index dfc8ca1..8ca1149 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1531,15 +1531,36 @@ 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 list_head *export_list,
+const struct drm_file *file_priv)
  {
/*
 * If user-space hasn't configured the driver to expose the stereo 3D
 * modes, don't expose them.
 */
+
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) {
+   struct drm_display_mode *mode_itr;
+
+   list_for_each_entry(mode_itr, export_list, export_head)

By walking the list of only the modes already added to the export list we
rely on ASPECT_NONE being first if present. That seems to be a bit
fragile. If we instead walk over all present modes (i.e. connector->modes)
then I think that's avoided.

I don't think that would work. If we just walk over all the modes then
wouldn't we always find a duplicate when the same mode with two different
aspect ratios is on the list? And then we'd export neither? Or maybe I
misunderstood what you mean here.

To me it seems like the correct option is to check against the
export_list unconditionally, no matter whether the current mode has the
aspect ratio set or not. If an identical mode is already on the list
we don't export again, if it's not there then we export.


Agreed. The current code does have a problem rightly pointed by Daniel 
Vetter and the main reason
for that is - we are checking for duplicates only if the mode has some 
aspect-ratio.
As you have suggested, If we can do away with the condition to check for 
duplicates for only

"modes having aspect ratio", it will solve the problem.
In that case, if the aspect-ratio cap is not set, we will be 

Re: [Intel-gfx] [PATCH v13 08/10] drm: Expose modes with aspect ratio, only if requested

[Ville Syrjälä]

On Thu, May 03, 2018 at 08:26:26AM +0200, Daniel Vetter wrote:
> On Wed, May 02, 2018 at 12:20:20PM +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.
> > -prepares a list of exposed modes, which is used to find unique modes
> >  if aspect-ratio is not allowed.
> > -adds a new list_head 'exposed_head' in drm_mode_display, to traverse
> >  the list of exposed modes.
> > 
> > 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.
> > V12: As suggested by Ville, added another list_head in
> >  drm_mode_display to traverse the list of exposed modes and
> >  avoided duplication of modes.
> > V13: Minor modifications, as suggested by Ville.
> > ---
> >  drivers/gpu/drm/drm_connector.c | 45 
> > ++---
> >  include/drm/drm_modes.h | 13 
> >  2 files changed, 51 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_connector.c 
> > b/drivers/gpu/drm/drm_connector.c
> > index dfc8ca1..8ca1149 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -1531,15 +1531,36 @@ 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 list_head *export_list,
> > +const struct drm_file *file_priv)
> >  {
> > /*
> >  * If user-space hasn't configured the driver to expose the stereo 3D
> >  * modes, don't expose them.
> >  */
> > +
> > 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) {
> > +   struct drm_display_mode *mode_itr;
> > +
> > +   list_for_each_entry(mode_itr, export_list, export_head)
> 
> By walking the list of only the modes already added to the export list we
> rely on ASPECT_NONE being first if present. That seems to be a bit
> fragile. If we instead walk over all present modes (i.e. connector->modes)
> then I think that's avoided.

I don't think that would work. If we just walk over all the modes then
wouldn't we always find a duplicate when the same mode with two different
aspect ratios is on the list? And then we'd export neither? Or maybe I
misunderstood what you mean here.

To me it seems like the correct option is to check against the
export_list unconditionally, no matter whether the current mode has the
aspect ratio set or not. If an identical mode is already on the list
we don't export again, if it's not there then we export.

> With that changed:
> 
> Reviewed-by: Daniel Vetter 
> 
> > +   if (drm_mode_match(mode_itr, mode,
> > +  

Re: [Intel-gfx] [PATCH v13 08/10] drm: Expose modes with aspect ratio, only if requested

[Daniel Vetter]

On Wed, May 02, 2018 at 12:20:20PM +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.
> -prepares a list of exposed modes, which is used to find unique modes
>  if aspect-ratio is not allowed.
> -adds a new list_head 'exposed_head' in drm_mode_display, to traverse
>  the list of exposed modes.
> 
> 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.
> V12: As suggested by Ville, added another list_head in
>  drm_mode_display to traverse the list of exposed modes and
>  avoided duplication of modes.
> V13: Minor modifications, as suggested by Ville.
> ---
>  drivers/gpu/drm/drm_connector.c | 45 
> ++---
>  include/drm/drm_modes.h | 13 
>  2 files changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index dfc8ca1..8ca1149 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1531,15 +1531,36 @@ 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 list_head *export_list,
> +  const struct drm_file *file_priv)
>  {
>   /*
>* If user-space hasn't configured the driver to expose the stereo 3D
>* modes, don't expose them.
>*/
> +
>   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) {
> + struct drm_display_mode *mode_itr;
> +
> + list_for_each_entry(mode_itr, export_list, export_head)

By walking the list of only the modes already added to the export list we
rely on ASPECT_NONE being first if present. That seems to be a bit
fragile. If we instead walk over all present modes (i.e. connector->modes)
then I think that's avoided. With that changed:

Reviewed-by: Daniel Vetter 

> + 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;
>  }
> @@ -1559,6 +1580,7 @@ int drm_mode_getconnector(struct drm_device *dev, void 
> *data,
>   struct drm_mode_modeinfo u_mode;
>   struct drm_mode_modeinfo __user *mode_ptr;
>   uint32_t __user *encoder_ptr;
> + LIST_HEAD(export_list);
>  
>   if (!drm_core_check_feature(dev, DRIVER_MODESET))
>   return -EINVAL;
> @@ -1607,21 +1629,30 @@ int drm_mode_getconnector(struct drm_device *dev, 
> void