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@xxxxxx>
> > ---
> >
> > 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.
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
