Re: [PATCH v2 44/60] drm/omap: dss: Add for_each_dss_output() macro

[Sebastian Reichel]

Hi,

On Mon, Jun 11, 2018 at 08:11:09PM +0300, Laurent Pinchart wrote:
> Hi Sebastian,
> 
> On Monday, 11 June 2018 02:52:44 EEST Sebastian Reichel wrote:
> > On Sat, May 26, 2018 at 08:25:02PM +0300, Laurent Pinchart wrote:
> > > Similarly to for_each_dss_display(), the for_each_dss_output() macro
> > > iterates over all the DSS connected outputs.
> > > 
> > > Signed-off-by: Laurent Pinchart 
> > > ---
> > > 
> > >  drivers/gpu/drm/omapdrm/dss/base.c| 20 ++--
> > >  drivers/gpu/drm/omapdrm/dss/omapdss.h |  9 ++---
> > >  2 files changed, 20 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/omapdrm/dss/base.c
> > > b/drivers/gpu/drm/omapdrm/dss/base.c index 96be800a0f25..519682f18d36
> > > 100644
> > > --- a/drivers/gpu/drm/omapdrm/dss/base.c
> > > +++ b/drivers/gpu/drm/omapdrm/dss/base.c
> > > @@ -127,11 +127,13 @@ struct omap_dss_device
> > > *omapdss_find_device_by_port(struct device_node *src,> 
> > >  /*
> > >  
> > >   * Search for the next device starting at @from. If display_only is true,
> > >   skip> 
> > > - * non-display devices. Release the reference to the @from device, and
> > > acquire - * a reference to the returned device if found.
> > > + * non-display devices. If output_only is true, skip non-output devices
> > > and + * non-connected output devices. Release the reference to the @from
> > > device, and + * acquire a reference to the returned device if found.
> > > 
> > >   */
> > >  
> > >  struct omap_dss_device *omapdss_device_get_next(struct omap_dss_device
> > >  *from,> 
> > > - bool display_only)
> > > + bool display_only,
> > > + bool output_only)
> > 
> > display_only and output_only are mutually exclusive, so I think it
> > would be better to use this as parameter. It would also improve
> > code readability a bit:
> > 
> > enum omapdss_device_type {
> > OMAPDSS_DEVICE_TYPE_ALL,
> > OMAPDSS_DEVICE_TYPE_OUTPUT_ONLY,
> > OMAPDSS_DEVICE_TYPE_DISPLAY_ONLY,
> > };
> 
> That's a good point, even if all this code is meant to disappear.
> What would you think of
> 
> enum omap_dss_device_type {
>   OMAP_DSS_DEVICE_TYPE_OUTPUT = (1 << 0),
>   OMAP_DSS_DEVICE_TYPE_DISPLAY = (1 << 1),
> };
> 
> and combining the flags when passed to omapdss_device_get_next() ?

Sounds good to me.

-- Sebastian


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 44/60] drm/omap: dss: Add for_each_dss_output() macro

[Laurent Pinchart]

Hi Sebastian,

On Monday, 11 June 2018 02:52:44 EEST Sebastian Reichel wrote:
> On Sat, May 26, 2018 at 08:25:02PM +0300, Laurent Pinchart wrote:
> > Similarly to for_each_dss_display(), the for_each_dss_output() macro
> > iterates over all the DSS connected outputs.
> > 
> > Signed-off-by: Laurent Pinchart 
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/dss/base.c| 20 ++--
> >  drivers/gpu/drm/omapdrm/dss/omapdss.h |  9 ++---
> >  2 files changed, 20 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/dss/base.c
> > b/drivers/gpu/drm/omapdrm/dss/base.c index 96be800a0f25..519682f18d36
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/dss/base.c
> > +++ b/drivers/gpu/drm/omapdrm/dss/base.c
> > @@ -127,11 +127,13 @@ struct omap_dss_device
> > *omapdss_find_device_by_port(struct device_node *src,> 
> >  /*
> >  
> >   * Search for the next device starting at @from. If display_only is true,
> >   skip> 
> > - * non-display devices. Release the reference to the @from device, and
> > acquire - * a reference to the returned device if found.
> > + * non-display devices. If output_only is true, skip non-output devices
> > and + * non-connected output devices. Release the reference to the @from
> > device, and + * acquire a reference to the returned device if found.
> > 
> >   */
> >  
> >  struct omap_dss_device *omapdss_device_get_next(struct omap_dss_device
> >  *from,> 
> > -   bool display_only)
> > +   bool display_only,
> > +   bool output_only)
> 
> display_only and output_only are mutually exclusive, so I think it
> would be better to use this as parameter. It would also improve
> code readability a bit:
> 
> enum omapdss_device_type {
> OMAPDSS_DEVICE_TYPE_ALL,
> OMAPDSS_DEVICE_TYPE_OUTPUT_ONLY,
> OMAPDSS_DEVICE_TYPE_DISPLAY_ONLY,
> };

That's a good point, even if all this code is meant to disappear.

What would you think of

enum omap_dss_device_type {
OMAP_DSS_DEVICE_TYPE_OUTPUT = (1 << 0),
OMAP_DSS_DEVICE_TYPE_DISPLAY = (1 << 1),
};

and combining the flags when passed to omapdss_device_get_next() ?


> >  {
> >  
> > struct omap_dss_device *dssdev;
> > struct list_head *list;
> > 
> > @@ -159,9 +161,15 @@ struct omap_dss_device
> > *omapdss_device_get_next(struct omap_dss_device *from,> 
> > goto done;
> > 
> > }
> > 
> > -   /* Filter out non-display entries if display_only is set. */
> > -   if (!display_only || dssdev->driver)
> > -   goto done;
> > +   /*
> > +* Filter out non-display entries if display_only is set, and
> > +* non-output entries if output_only is set.
> > +*/
> > +   if (display_only && !dssdev->driver)
> > +   continue;
> > +   if (output_only && (!dssdev->id || !dssdev->next))
> > +   continue;
> > +   goto done;
> > 
> > }
> > 
> > dssdev = NULL;
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> > b/drivers/gpu/drm/omapdrm/dss/omapdss.h index 5d3e4ced73d1..723124f6e596
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> > +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> > @@ -488,9 +488,9 @@ static inline bool omapdss_is_initialized(void)
> > 
> > return !!omapdss_get_dss();
> >  
> >  }
> > 
> > -void omapdss_display_init(struct omap_dss_device *dssdev);
> > 
> >  #define for_each_dss_display(d) \
> > 
> > -   while ((d = omapdss_device_get_next(d, true)) != NULL)
> > +   while ((d = omapdss_device_get_next(d, true, false)) != NULL)
> > +void omapdss_display_init(struct omap_dss_device *dssdev);
> > 
> >  void omapdss_device_register(struct omap_dss_device *dssdev);
> >  void omapdss_device_unregister(struct omap_dss_device *dssdev);
> > 
> > @@ -499,7 +499,8 @@ void omapdss_device_put(struct omap_dss_device
> > *dssdev);> 
> >  struct omap_dss_device *omapdss_find_device_by_port(struct device_node
> >  *src,>  
> > unsigned int port);
> >  
> >  struct omap_dss_device *omapdss_device_get_next(struct omap_dss_device
> >  *from,> 
> > -   bool display_only);
> > +   bool display_only,
> > +   bool output_only);
> > 
> >  int omapdss_device_connect(struct dss_device *dss,
> >  
> >struct omap_dss_device *src,
> >struct omap_dss_device *dst);
> > 
> > @@ -511,6 +512,8 @@ int omap_dss_get_num_overlay_managers(void);
> > 
> >  int omap_dss_get_num_overlays(void);
> > 
> > +#define for_each_dss_output(d) \
> > +   while ((d = omapdss_device_get_next(d, false, true)) != NULL)
> > 
> >  int omapdss_output_set_device(struct omap_dss_device *out,
> >  
> > struct 

Re: [PATCH v2 44/60] drm/omap: dss: Add for_each_dss_output() macro

[Sebastian Reichel]

On Sat, May 26, 2018 at 08:25:02PM +0300, Laurent Pinchart wrote:
> Similarly to for_each_dss_display(), the for_each_dss_output() macro
> iterates over all the DSS connected outputs.
> 
> Signed-off-by: Laurent Pinchart 
> ---
>  drivers/gpu/drm/omapdrm/dss/base.c| 20 ++--
>  drivers/gpu/drm/omapdrm/dss/omapdss.h |  9 ++---
>  2 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/base.c 
> b/drivers/gpu/drm/omapdrm/dss/base.c
> index 96be800a0f25..519682f18d36 100644
> --- a/drivers/gpu/drm/omapdrm/dss/base.c
> +++ b/drivers/gpu/drm/omapdrm/dss/base.c
> @@ -127,11 +127,13 @@ struct omap_dss_device 
> *omapdss_find_device_by_port(struct device_node *src,
>  
>  /*
>   * Search for the next device starting at @from. If display_only is true, 
> skip
> - * non-display devices. Release the reference to the @from device, and 
> acquire
> - * a reference to the returned device if found.
> + * non-display devices. If output_only is true, skip non-output devices and
> + * non-connected output devices. Release the reference to the @from device, 
> and
> + * acquire a reference to the returned device if found.
>   */
>  struct omap_dss_device *omapdss_device_get_next(struct omap_dss_device *from,
> - bool display_only)
> + bool display_only,
> + bool output_only)

display_only and output_only are mutually exclusive, so I think it
would be better to use this as parameter. It would also improve
code readability a bit:

enum omapdss_device_type {
OMAPDSS_DEVICE_TYPE_ALL,
OMAPDSS_DEVICE_TYPE_OUTPUT_ONLY,
OMAPDSS_DEVICE_TYPE_DISPLAY_ONLY,
};

>  {
>   struct omap_dss_device *dssdev;
>   struct list_head *list;
> @@ -159,9 +161,15 @@ struct omap_dss_device *omapdss_device_get_next(struct 
> omap_dss_device *from,
>   goto done;
>   }
>  
> - /* Filter out non-display entries if display_only is set. */
> - if (!display_only || dssdev->driver)
> - goto done;
> + /*
> +  * Filter out non-display entries if display_only is set, and
> +  * non-output entries if output_only is set.
> +  */
> + if (display_only && !dssdev->driver)
> + continue;
> + if (output_only && (!dssdev->id || !dssdev->next))
> + continue;
> + goto done;
>   }
>  
>   dssdev = NULL;
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h 
> b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> index 5d3e4ced73d1..723124f6e596 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -488,9 +488,9 @@ static inline bool omapdss_is_initialized(void)
>   return !!omapdss_get_dss();
>  }
>  
> -void omapdss_display_init(struct omap_dss_device *dssdev);
>  #define for_each_dss_display(d) \
> - while ((d = omapdss_device_get_next(d, true)) != NULL)
> + while ((d = omapdss_device_get_next(d, true, false)) != NULL)
> +void omapdss_display_init(struct omap_dss_device *dssdev);
>  
>  void omapdss_device_register(struct omap_dss_device *dssdev);
>  void omapdss_device_unregister(struct omap_dss_device *dssdev);
> @@ -499,7 +499,8 @@ void omapdss_device_put(struct omap_dss_device *dssdev);
>  struct omap_dss_device *omapdss_find_device_by_port(struct device_node *src,
>   unsigned int port);
>  struct omap_dss_device *omapdss_device_get_next(struct omap_dss_device *from,
> - bool display_only);
> + bool display_only,
> + bool output_only);
>  int omapdss_device_connect(struct dss_device *dss,
>  struct omap_dss_device *src,
>  struct omap_dss_device *dst);
> @@ -511,6 +512,8 @@ int omap_dss_get_num_overlay_managers(void);
>  
>  int omap_dss_get_num_overlays(void);
>  
> +#define for_each_dss_output(d) \
> + while ((d = omapdss_device_get_next(d, false, true)) != NULL)
>  int omapdss_output_set_device(struct omap_dss_device *out,
>   struct omap_dss_device *dssdev);
>  int omapdss_output_unset_device(struct omap_dss_device *out);
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 44/60] drm/omap: dss: Add for_each_dss_output() macro

[Laurent Pinchart]

Similarly to for_each_dss_display(), the for_each_dss_output() macro
iterates over all the DSS connected outputs.

Signed-off-by: Laurent Pinchart 
---
 drivers/gpu/drm/omapdrm/dss/base.c| 20 ++--
 drivers/gpu/drm/omapdrm/dss/omapdss.h |  9 ++---
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/base.c 
b/drivers/gpu/drm/omapdrm/dss/base.c
index 96be800a0f25..519682f18d36 100644
--- a/drivers/gpu/drm/omapdrm/dss/base.c
+++ b/drivers/gpu/drm/omapdrm/dss/base.c
@@ -127,11 +127,13 @@ struct omap_dss_device 
*omapdss_find_device_by_port(struct device_node *src,
 
 /*
  * Search for the next device starting at @from. If display_only is true, skip
- * non-display devices. Release the reference to the @from device, and acquire
- * a reference to the returned device if found.
+ * non-display devices. If output_only is true, skip non-output devices and
+ * non-connected output devices. Release the reference to the @from device, and
+ * acquire a reference to the returned device if found.
  */
 struct omap_dss_device *omapdss_device_get_next(struct omap_dss_device *from,
-   bool display_only)
+   bool display_only,
+   bool output_only)
 {
struct omap_dss_device *dssdev;
struct list_head *list;
@@ -159,9 +161,15 @@ struct omap_dss_device *omapdss_device_get_next(struct 
omap_dss_device *from,
goto done;
}
 
-   /* Filter out non-display entries if display_only is set. */
-   if (!display_only || dssdev->driver)
-   goto done;
+   /*
+* Filter out non-display entries if display_only is set, and
+* non-output entries if output_only is set.
+*/
+   if (display_only && !dssdev->driver)
+   continue;
+   if (output_only && (!dssdev->id || !dssdev->next))
+   continue;
+   goto done;
}
 
dssdev = NULL;
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h 
b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index 5d3e4ced73d1..723124f6e596 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -488,9 +488,9 @@ static inline bool omapdss_is_initialized(void)
return !!omapdss_get_dss();
 }
 
-void omapdss_display_init(struct omap_dss_device *dssdev);
 #define for_each_dss_display(d) \
-   while ((d = omapdss_device_get_next(d, true)) != NULL)
+   while ((d = omapdss_device_get_next(d, true, false)) != NULL)
+void omapdss_display_init(struct omap_dss_device *dssdev);
 
 void omapdss_device_register(struct omap_dss_device *dssdev);
 void omapdss_device_unregister(struct omap_dss_device *dssdev);
@@ -499,7 +499,8 @@ void omapdss_device_put(struct omap_dss_device *dssdev);
 struct omap_dss_device *omapdss_find_device_by_port(struct device_node *src,
unsigned int port);
 struct omap_dss_device *omapdss_device_get_next(struct omap_dss_device *from,
-   bool display_only);
+   bool display_only,
+   bool output_only);
 int omapdss_device_connect(struct dss_device *dss,
   struct omap_dss_device *src,
   struct omap_dss_device *dst);
@@ -511,6 +512,8 @@ int omap_dss_get_num_overlay_managers(void);
 
 int omap_dss_get_num_overlays(void);
 
+#define for_each_dss_output(d) \
+   while ((d = omapdss_device_get_next(d, false, true)) != NULL)
 int omapdss_output_set_device(struct omap_dss_device *out,
struct omap_dss_device *dssdev);
 int omapdss_output_unset_device(struct omap_dss_device *out);
-- 
Regards,

Laurent Pinchart

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel