| Message ID | e0f41d90ee4e024e5a441d5ef8753e8513202165.1518780268.git.jsarha@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/omap: DSS6 with dynamically allocated objects | expand |
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
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.
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;
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(-)