[RFC,1/9] drm/omap: Update omapdss API to allow alternative DSS implementations

Message ID e0f41d90ee4e024e5a441d5ef8753e8513202165.1518780268.git.jsarha@ti.com
State New
Headers show
Series
  • drm/omap: DSS6 with dynamically allocated objects
Related show

Commit Message

Jyri Sarha Feb. 16, 2018, 11:25 a.m.
After this patch OMAP_DSS_BASE module is not including any OMAP2_DSS
headers, only the API omapdss.h. "sturct dss_data", the piece of the
data structure needed for base.c is defined in omapdss.h and added as
a member to struct dss_device, and later to struct dss6_device.

The patch is still a bit hackish. The struct dispc_device declaration
is currently shared between alternative dss implementations, with
different internal definitions. It should be relatively simple to use
a similar struct dispc_data as struct dss_data is for dss_device, move
some common parts - maybe the dispc_ops itself - there and find the
private data with container_of macro. Also the contents of struct
dss_data in side dss_device is currently redundant. These should be
easy enough to fix, if we decide to take this route.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/base.c    | 11 +++++------
 drivers/gpu/drm/omapdrm/dss/dispc.c   |  4 ++++
 drivers/gpu/drm/omapdrm/dss/dss.c     |  2 +-
 drivers/gpu/drm/omapdrm/dss/dss.h     |  2 ++
 drivers/gpu/drm/omapdrm/dss/omapdss.h | 13 +++++++++----
 drivers/gpu/drm/omapdrm/omap_drv.h    |  2 +-
 6 files changed, 22 insertions(+), 12 deletions(-)

Comments

Tomi Valkeinen Feb. 19, 2018, 12:01 p.m. | #1
Hi Jyri, Laurent,

On 16/02/18 13:25, Jyri Sarha wrote:
> After this patch OMAP_DSS_BASE module is not including any OMAP2_DSS
> headers, only the API omapdss.h. "sturct dss_data", the piece of the
> data structure needed for base.c is defined in omapdss.h and added as
> a member to struct dss_device, and later to struct dss6_device.
> 
> The patch is still a bit hackish. The struct dispc_device declaration
> is currently shared between alternative dss implementations, with
> different internal definitions. It should be relatively simple to use
> a similar struct dispc_data as struct dss_data is for dss_device, move
> some common parts - maybe the dispc_ops itself - there and find the
> private data with container_of macro. Also the contents of struct
> dss_data in side dss_device is currently redundant. These should be
> easy enough to fix, if we decide to take this route.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/base.c    | 11 +++++------
>  drivers/gpu/drm/omapdrm/dss/dispc.c   |  4 ++++
>  drivers/gpu/drm/omapdrm/dss/dss.c     |  2 +-
>  drivers/gpu/drm/omapdrm/dss/dss.h     |  2 ++
>  drivers/gpu/drm/omapdrm/dss/omapdss.h | 13 +++++++++----
>  drivers/gpu/drm/omapdrm/omap_drv.h    |  2 +-
>  6 files changed, 22 insertions(+), 12 deletions(-)

I think something in this direction would be good. I'd really like to keep it possible to run dss6 on top of omapdrm.

But I think this can be done slightly simpler like this:


diff --git a/drivers/gpu/drm/omapdrm/dss/base.c b/drivers/gpu/drm/omapdrm/dss/base.c
index 99e8cb8dc65b..08913e006e93 100644
--- a/drivers/gpu/drm/omapdrm/dss/base.c
+++ b/drivers/gpu/drm/omapdrm/dss/base.c
@@ -19,10 +19,11 @@
 #include <linux/of_graph.h>
 #include <linux/list.h>
 
-#include "dss.h"
 #include "omapdss.h"
 
 static struct dss_device *dss_device;
+static struct dispc_device *s_dispc_device;
+static const struct dispc_ops *s_dispc_ops;
 
 static struct list_head omapdss_comp_list;
 
@@ -46,16 +47,23 @@ EXPORT_SYMBOL(omapdss_set_dss);
 
 struct dispc_device *dispc_get_dispc(struct dss_device *dss)
 {
-	return dss->dispc;
+	return s_dispc_device;
 }
 EXPORT_SYMBOL(dispc_get_dispc);
 
 const struct dispc_ops *dispc_get_ops(struct dss_device *dss)
 {
-	return dss->dispc_ops;
+	return s_dispc_ops;
 }
 EXPORT_SYMBOL(dispc_get_ops);
 
+void omapdss_set_dispc(struct dispc_device *dispc, const struct dispc_ops* dispc_ops)
+{
+	s_dispc_device = dispc;
+	s_dispc_ops = dispc_ops;
+}
+EXPORT_SYMBOL(omapdss_set_dispc);
+
 static bool omapdss_list_contains(const struct device_node *node)
 {
 	struct omapdss_comp_node *comp;
diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index ce470b51e326..b72f981d660e 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -4786,7 +4786,7 @@ static int dispc_bind(struct device *dev, struct device *master, void *data)
 	dispc_runtime_put(dispc);
 
 	dss->dispc = dispc;
-	dss->dispc_ops = &dispc_ops;
+	omapdss_set_dispc(dispc, &dispc_ops);
 
 	dispc->debugfs = dss_debugfs_create_file(dss, "dispc", dispc_dump_regs,
 						 dispc);
@@ -4807,8 +4807,8 @@ static void dispc_unbind(struct device *dev, struct device *master, void *data)
 
 	dss_debugfs_remove_file(dispc->debugfs);
 
+	omapdss_set_dispc(NULL, NULL);
 	dss->dispc = NULL;
-	dss->dispc_ops = NULL;
 
 	pm_runtime_disable(dev);
 
diff --git a/drivers/gpu/drm/omapdrm/dss/dss.h b/drivers/gpu/drm/omapdrm/dss/dss.h
index 6f6fd3d1b159..3d23232ec1f7 100644
--- a/drivers/gpu/drm/omapdrm/dss/dss.h
+++ b/drivers/gpu/drm/omapdrm/dss/dss.h
@@ -274,7 +274,6 @@ struct dss_device {
 	struct dss_pll	*video2_pll;
 
 	struct dispc_device *dispc;
-	const struct dispc_ops *dispc_ops;
 };
 
 /* core */
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index a4f71e082c1c..b724dae22d7a 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -751,6 +751,7 @@ struct dispc_ops {
 
 struct dispc_device *dispc_get_dispc(struct dss_device *dss);
 const struct dispc_ops *dispc_get_ops(struct dss_device *dss);
+void omapdss_set_dispc(struct dispc_device *dispc, const struct dispc_ops* dispc_ops);
 
 bool omapdss_component_is_display(struct device_node *node);
 bool omapdss_component_is_output(struct device_node *node);


Yes, it adds two new globals. But I don't think those are a big issue. Note that I left the dss->dispc there, for dss internal use.

Laurent, what do you think? If this is fine, can you squash to your series? Or I can even have this on top as well. I think otherwise it's good for merging.

Can you also have a quick look at patches 2, 3, 4, 5, 6 and 7. While their aim is to get dss6 working, I think they're ok cleanups and shouldn't cause issues with the main dss rework.

 Tomi
Laurent Pinchart Feb. 27, 2018, 2:27 p.m. | #2
Hi Tomi,

On Monday, 19 February 2018 14:01:05 EET Tomi Valkeinen wrote:
> On 16/02/18 13:25, Jyri Sarha wrote:
> > After this patch OMAP_DSS_BASE module is not including any OMAP2_DSS
> > headers, only the API omapdss.h. "sturct dss_data", the piece of the
> > data structure needed for base.c is defined in omapdss.h and added as
> > a member to struct dss_device, and later to struct dss6_device.
> > 
> > The patch is still a bit hackish. The struct dispc_device declaration
> > is currently shared between alternative dss implementations, with
> > different internal definitions. It should be relatively simple to use
> > a similar struct dispc_data as struct dss_data is for dss_device, move
> > some common parts - maybe the dispc_ops itself - there and find the
> > private data with container_of macro. Also the contents of struct
> > dss_data in side dss_device is currently redundant. These should be
> > easy enough to fix, if we decide to take this route.
> > 
> > Signed-off-by: Jyri Sarha <jsarha@ti.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/dss/base.c    | 11 +++++------
> >  drivers/gpu/drm/omapdrm/dss/dispc.c   |  4 ++++
> >  drivers/gpu/drm/omapdrm/dss/dss.c     |  2 +-
> >  drivers/gpu/drm/omapdrm/dss/dss.h     |  2 ++
> >  drivers/gpu/drm/omapdrm/dss/omapdss.h | 13 +++++++++----
> >  drivers/gpu/drm/omapdrm/omap_drv.h    |  2 +-
> >  6 files changed, 22 insertions(+), 12 deletions(-)
> 
> I think something in this direction would be good. I'd really like to keep
> it possible to run dss6 on top of omapdrm.
> 
> But I think this can be done slightly simpler like this:
> 
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/base.c
> b/drivers/gpu/drm/omapdrm/dss/base.c index 99e8cb8dc65b..08913e006e93
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/base.c
> +++ b/drivers/gpu/drm/omapdrm/dss/base.c
> @@ -19,10 +19,11 @@
>  #include <linux/of_graph.h>
>  #include <linux/list.h>
> 
> -#include "dss.h"
>  #include "omapdss.h"
> 
>  static struct dss_device *dss_device;
> +static struct dispc_device *s_dispc_device;
> +static const struct dispc_ops *s_dispc_ops;
> 
>  static struct list_head omapdss_comp_list;
> 
> @@ -46,16 +47,23 @@ EXPORT_SYMBOL(omapdss_set_dss);
> 
>  struct dispc_device *dispc_get_dispc(struct dss_device *dss)
>  {
> -	return dss->dispc;
> +	return s_dispc_device;
>  }
>  EXPORT_SYMBOL(dispc_get_dispc);
> 
>  const struct dispc_ops *dispc_get_ops(struct dss_device *dss)
>  {
> -	return dss->dispc_ops;
> +	return s_dispc_ops;
>  }
>  EXPORT_SYMBOL(dispc_get_ops);
> 
> +void omapdss_set_dispc(struct dispc_device *dispc, const struct dispc_ops*
> dispc_ops)
> +{
> +	s_dispc_device = dispc;
> +	s_dispc_ops = dispc_ops;
> +}
> +EXPORT_SYMBOL(omapdss_set_dispc);
> +
>  static bool omapdss_list_contains(const struct device_node *node)
>  {
>  	struct omapdss_comp_node *comp;
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> b/drivers/gpu/drm/omapdrm/dss/dispc.c index ce470b51e326..b72f981d660e
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -4786,7 +4786,7 @@ static int dispc_bind(struct device *dev, struct
> device *master, void *data) dispc_runtime_put(dispc);
> 
>  	dss->dispc = dispc;
> -	dss->dispc_ops = &dispc_ops;
> +	omapdss_set_dispc(dispc, &dispc_ops);
> 
>  	dispc->debugfs = dss_debugfs_create_file(dss, "dispc", dispc_dump_regs,
>  						 dispc);
> @@ -4807,8 +4807,8 @@ static void dispc_unbind(struct device *dev, struct
> device *master, void *data)
> 
>  	dss_debugfs_remove_file(dispc->debugfs);
> 
> +	omapdss_set_dispc(NULL, NULL);
>  	dss->dispc = NULL;
> -	dss->dispc_ops = NULL;
> 
>  	pm_runtime_disable(dev);
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dss.h
> b/drivers/gpu/drm/omapdrm/dss/dss.h index 6f6fd3d1b159..3d23232ec1f7 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/dss.h
> @@ -274,7 +274,6 @@ struct dss_device {
>  	struct dss_pll	*video2_pll;
> 
>  	struct dispc_device *dispc;
> -	const struct dispc_ops *dispc_ops;
>  };
> 
>  /* core */
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> b/drivers/gpu/drm/omapdrm/dss/omapdss.h index a4f71e082c1c..b724dae22d7a
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -751,6 +751,7 @@ struct dispc_ops {
> 
>  struct dispc_device *dispc_get_dispc(struct dss_device *dss);
>  const struct dispc_ops *dispc_get_ops(struct dss_device *dss);
> +void omapdss_set_dispc(struct dispc_device *dispc, const struct dispc_ops*
> dispc_ops);
> 
>  bool omapdss_component_is_display(struct device_node *node);
>  bool omapdss_component_is_output(struct device_node *node);
> 
> 
> Yes, it adds two new globals. But I don't think those are a big issue. Note
> that I left the dss->dispc there, for dss internal use.
> 
> Laurent, what do you think? If this is fine, can you squash to your series?
> Or I can even have this on top as well. I think otherwise it's good for
> merging.

To be honest I like Jyri's approach better, with a small caveat: we really 
need to standardize how we name our data structures. It will be painful (as in 
generating conflicts) but should make the code much clearer. dss_data vs. 
dss_device vs. omap_dss_device is just too confusing. If you agree to a rename 
of data structure I'll make a proposal.

Additionally, one thing I like about this patch is that the dss_data structure 
(to be renamed...) can store data shared between the DSS2-5 and DSS6 
implementations, which would make it easier to share code where applicable. 
Being completely honest again I haven't reviewed the DSS6 implementation yet, 
so there might be no opportunity for code sharing.

> Can you also have a quick look at patches 2, 3, 4, 5, 6 and 7. While their
> aim is to get dss6 working, I think they're ok cleanups and shouldn't cause
> issues with the main dss rework.

Sure, I'll review them now.

Patch

diff --git a/drivers/gpu/drm/omapdrm/dss/base.c b/drivers/gpu/drm/omapdrm/dss/base.c
index 99e8cb8..42442f3 100644
--- a/drivers/gpu/drm/omapdrm/dss/base.c
+++ b/drivers/gpu/drm/omapdrm/dss/base.c
@@ -19,10 +19,9 @@ 
 #include <linux/of_graph.h>
 #include <linux/list.h>
 
-#include "dss.h"
 #include "omapdss.h"
 
-static struct dss_device *dss_device;
+static struct dss_data *dss_device;
 
 static struct list_head omapdss_comp_list;
 
@@ -32,25 +31,25 @@  struct omapdss_comp_node {
 	bool dss_core_component;
 };
 
-struct dss_device *omapdss_get_dss(void)
+struct dss_data *omapdss_get_dss(void)
 {
 	return dss_device;
 }
 EXPORT_SYMBOL(omapdss_get_dss);
 
-void omapdss_set_dss(struct dss_device *dss)
+void omapdss_set_dss(struct dss_data *dss)
 {
 	dss_device = dss;
 }
 EXPORT_SYMBOL(omapdss_set_dss);
 
-struct dispc_device *dispc_get_dispc(struct dss_device *dss)
+struct dispc_device *dispc_get_dispc(struct dss_data *dss)
 {
 	return dss->dispc;
 }
 EXPORT_SYMBOL(dispc_get_dispc);
 
-const struct dispc_ops *dispc_get_ops(struct dss_device *dss)
+const struct dispc_ops *dispc_get_ops(struct dss_data *dss)
 {
 	return dss->dispc_ops;
 }
diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index ce470b5..338490d 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -4791,6 +4791,10 @@  static int dispc_bind(struct device *dev, struct device *master, void *data)
 	dispc->debugfs = dss_debugfs_create_file(dss, "dispc", dispc_dump_regs,
 						 dispc);
 
+	// XXX get rid of the above redundant data members
+	dss->data.dispc = dss->dispc;
+	dss->data.dispc_ops = dss->dispc_ops;
+
 	return 0;
 
 err_runtime_get:
diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c b/drivers/gpu/drm/omapdrm/dss/dss.c
index 4a08bd1..5752328 100644
--- a/drivers/gpu/drm/omapdrm/dss/dss.c
+++ b/drivers/gpu/drm/omapdrm/dss/dss.c
@@ -1326,7 +1326,7 @@  static int dss_bind(struct device *dev)
 	pm_set_vt_switch(0);
 
 	omapdss_gather_components(dev);
-	omapdss_set_dss(dss);
+	omapdss_set_dss(&dss->data);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/omapdrm/dss/dss.h b/drivers/gpu/drm/omapdrm/dss/dss.h
index 6f6fd3d..434262a 100644
--- a/drivers/gpu/drm/omapdrm/dss/dss.h
+++ b/drivers/gpu/drm/omapdrm/dss/dss.h
@@ -273,6 +273,8 @@  struct dss_device {
 	struct dss_pll	*video1_pll;
 	struct dss_pll	*video2_pll;
 
+	struct dss_data data;
+	// XXX these are redundant, use data instead
 	struct dispc_device *dispc;
 	const struct dispc_ops *dispc_ops;
 };
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index a4f71e0..1299dd6 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -589,8 +589,13 @@  struct omap_dss_driver {
 		const struct hdmi_avi_infoframe *avi);
 };
 
-struct dss_device *omapdss_get_dss(void);
-void omapdss_set_dss(struct dss_device *dss);
+struct dss_data {
+	const struct dispc_ops *dispc_ops;
+	struct dispc_device *dispc;
+};
+
+struct dss_data *omapdss_get_dss(void);
+void omapdss_set_dss(struct dss_data *dss);
 static inline bool omapdss_is_initialized(void)
 {
 	return !!omapdss_get_dss();
@@ -749,8 +754,8 @@  struct dispc_ops {
 					  enum omap_plane_id plane);
 };
 
-struct dispc_device *dispc_get_dispc(struct dss_device *dss);
-const struct dispc_ops *dispc_get_ops(struct dss_device *dss);
+struct dispc_device *dispc_get_dispc(struct dss_data *dss);
+const struct dispc_ops *dispc_get_ops(struct dss_data *dss);
 
 bool omapdss_component_is_display(struct device_node *node);
 bool omapdss_component_is_output(struct device_node *node);
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 6eaee4d..3b7ec7e 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -50,7 +50,7 @@  struct omap_drm_private {
 	struct device *dev;
 	u32 omaprev;
 
-	struct dss_device *dss;
+	struct dss_data *dss;
 	struct dispc_device *dispc;
 	const struct dispc_ops *dispc_ops;