[linux-sunxi] Re: [PATCH v3 04/11] drm/sun4i: abstract the layer type
Icenowy Zheng
icenowy at aosc.io
Wed Apr 5 05:23:15 UTC 2017
2017年4月5日 10:27于 Chen-Yu Tsai <wens at csie.org>写道：
>
> On Wed, Apr 5, 2017 at 3:53 AM, Icenowy Zheng <icenowy at aosc.io> wrote:
> >
> >
> > 在 2017年04月05日 03:28, Sean Paul 写道:
> >>
> >> On Thu, Mar 30, 2017 at 03:46:06AM +0800, Icenowy Zheng wrote:
> >>>
> >>> As we are going to add support for the Allwinner DE2 Mixer in sun4i-drm
> >>> driver, we will finally have two types of layer.
> >>>
> >>> Abstract the layer type to void * and a ops struct, which contains the
> >>> only function used by crtc -- get the drm_plane struct of the layer.
> >>>
> >>> Signed-off-by: Icenowy Zheng <icenowy at aosc.io>
> >>> ---
> >>> Refactored patch in v3.
> >>>
> >>> drivers/gpu/drm/sun4i/sun4i_crtc.c | 19 +++++++++++--------
> >>> drivers/gpu/drm/sun4i/sun4i_crtc.h | 3 ++-
> >>> drivers/gpu/drm/sun4i/sun4i_layer.c | 19 ++++++++++++++++++-
> >>> drivers/gpu/drm/sun4i/sun4i_layer.h | 2 +-
> >>> drivers/gpu/drm/sun4i/sunxi_layer.h | 17 +++++++++++++++++
> >>> 5 files changed, 49 insertions(+), 11 deletions(-)
> >>> create mode 100644 drivers/gpu/drm/sun4i/sunxi_layer.h
> >>>
> >>> diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c
> >>> b/drivers/gpu/drm/sun4i/sun4i_crtc.c
> >>> index 3c876c3a356a..33854ee7f636 100644
> >>> --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
> >>> +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
> >>> @@ -29,6 +29,7 @@
> >>> #include "sun4i_crtc.h"
> >>> #include "sun4i_drv.h"
> >>> #include "sun4i_layer.h"
> >>> +#include "sunxi_layer.h"
> >>> #include "sun4i_tcon.h"
> >>>
> >>> static void sun4i_crtc_atomic_begin(struct drm_crtc *crtc,
> >>> @@ -149,7 +150,7 @@ struct sun4i_crtc *sun4i_crtc_init(struct drm_device
> >>> *drm,
> >>> scrtc->tcon = tcon;
> >>>
> >>> /* Create our layers */
> >>> - scrtc->layers = sun4i_layers_init(drm, scrtc->backend);
> >>> + scrtc->layers = (void **)sun4i_layers_init(drm, scrtc);
> >>> if (IS_ERR(scrtc->layers)) {
> >>> dev_err(drm->dev, "Couldn't create the planes\n");
> >>> return NULL;
> >>> @@ -157,14 +158,15 @@ struct sun4i_crtc *sun4i_crtc_init(struct
> >>> drm_device *drm,
> >>>
> >>> /* find primary and cursor planes for drm_crtc_init_with_planes
> >>> */
> >>> for (i = 0; scrtc->layers[i]; i++) {
> >>> - struct sun4i_layer *layer = scrtc->layers[i];
> >>> + void *layer = scrtc->layers[i];
> >>> + struct drm_plane *plane =
> >>> scrtc->layer_ops->get_plane(layer);
> >>>
> >>> - switch (layer->plane.type) {
> >>> + switch (plane->type) {
> >>> case DRM_PLANE_TYPE_PRIMARY:
> >>> - primary = &layer->plane;
> >>> + primary = plane;
> >>> break;
> >>> case DRM_PLANE_TYPE_CURSOR:
> >>> - cursor = &layer->plane;
> >>> + cursor = plane;
> >>> break;
> >>> default:
> >>> break;
> >>> @@ -190,10 +192,11 @@ struct sun4i_crtc *sun4i_crtc_init(struct
> >>> drm_device *drm,
> >>> /* Set possible_crtcs to this crtc for overlay planes */
> >>> for (i = 0; scrtc->layers[i]; i++) {
> >>> uint32_t possible_crtcs =
> >>> BIT(drm_crtc_index(&scrtc->crtc));
> >>> - struct sun4i_layer *layer = scrtc->layers[i];
> >>> + void *layer = scrtc->layers[i];
> >>> + struct drm_plane *plane =
> >>> scrtc->layer_ops->get_plane(layer);
> >>>
> >>> - if (layer->plane.type == DRM_PLANE_TYPE_OVERLAY)
> >>> - layer->plane.possible_crtcs = possible_crtcs;
> >>> + if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> >>> + plane->possible_crtcs = possible_crtcs;
> >>> }
> >>>
> >>> return scrtc;
> >>> diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.h
> >>> b/drivers/gpu/drm/sun4i/sun4i_crtc.h
> >>> index 230cb8f0d601..a4036ee44cf8 100644
> >>> --- a/drivers/gpu/drm/sun4i/sun4i_crtc.h
> >>> +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.h
> >>> @@ -19,7 +19,8 @@ struct sun4i_crtc {
> >>>
> >>> struct sun4i_backend *backend;
> >>> struct sun4i_tcon *tcon;
> >>> - struct sun4i_layer **layers;
> >>> + void **layers;
> >>> + const struct sunxi_layer_ops *layer_ops;
> >>
> >>
> >> I think you should probably take a different approach to abstract the
> >> layer
> >> type. How about creating
> >>
> >> struct sunxi_layer {
> >> struct drm_plane plane;
> >> }
> >>
> >> base and then subclassing that for sun4i and sun8i? By doing this you can
> >> avoid
> >> the nasty casting and you can also get rid of the get_plane() hook and
> >> layer_ops.
> >
> >
> > For the situation that using ** things are easily to get weird.
>
> That code could be reworked, by initializing the layers directly within
> the crtc init code. If you look at rockchip's drm driver, you'll see
> they do this. There is a good reason to do it this way, as you need
> to first create the primary and cursor layers, pass them in when you
> create the crtc, then initialize any additional layers with the
> possible_crtcs bitmap.
But furthurly maybe more layers will be created for DE2 mixer, and may even depends on mixer type (On A83T/H3/A64/H5 mixer1 has fewer channel than mixer0).
>
> In our driver we are currently initializing all layers, then going
> back and filling in possible_crtcs for the extra layers.
>
> And as Maxime and I mentioned in the other thread, we don't really
> need to keep a reference to **layers.
>
> Regards
> ChenYu
>
> >
> >>
> >> Sean
> >>
> >>
> >>
> >>> };
> >>>
> >>> static inline struct sun4i_crtc *drm_crtc_to_sun4i_crtc(struct drm_crtc
> >>> *crtc)
> >>> diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c
> >>> b/drivers/gpu/drm/sun4i/sun4i_layer.c
> >>> index f26bde5b9117..bc4a70d6968b 100644
> >>> --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
> >>> +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
> >>> @@ -16,7 +16,9 @@
> >>> #include <drm/drmP.h>
> >>>
> >>> #include "sun4i_backend.h"
> >>> +#include "sun4i_crtc.h"
> >>> #include "sun4i_layer.h"
> >>> +#include "sunxi_layer.h"
> >>>
> >>> struct sun4i_plane_desc {
> >>> enum drm_plane_type type;
> >>> @@ -100,6 +102,17 @@ static const struct sun4i_plane_desc
> >>> sun4i_backend_planes[] = {
> >>> },
> >>> };
> >>>
> >>> +static struct drm_plane *sun4i_layer_get_plane(void *layer)
> >>> +{
> >>> + struct sun4i_layer *sun4i_layer = layer;
> >>> +
> >>> + return &sun4i_layer->plane;
> >>> +}
> >>> +
> >>> +static const struct sunxi_layer_ops layer_ops = {
> >>> + .get_plane = sun4i_layer_get_plane,
> >>> +};
> >>> +
> >>> static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm,
> >>> struct sun4i_backend
> >>> *backend,
> >>> const struct
> >>> sun4i_plane_desc *plane)
> >>> @@ -129,9 +142,10 @@ static struct sun4i_layer
> >>> *sun4i_layer_init_one(struct drm_device *drm,
> >>> }
> >>>
> >>> struct sun4i_layer **sun4i_layers_init(struct drm_device *drm,
> >>> - struct sun4i_backend *backend)
> >>> + struct sun4i_crtc *crtc)
> >>> {
> >>> struct sun4i_layer **layers;
> >>> + struct sun4i_backend *backend = crtc->backend;
> >>> int i;
> >>>
> >>> layers = devm_kcalloc(drm->dev, ARRAY_SIZE(sun4i_backend_planes)
> >>> + 1,
> >>> @@ -181,5 +195,8 @@ struct sun4i_layer **sun4i_layers_init(struct
> >>> drm_device *drm,
> >>> layers[i] = layer;
> >>> };
> >>>
> >>> + /* Assign layer ops to the CRTC */
> >>> + crtc->layer_ops = &layer_ops;
> >>> +
> >>> return layers;
> >>> }
> >>> diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.h
> >>> b/drivers/gpu/drm/sun4i/sun4i_layer.h
> >>> index 4be1f0919df2..425eea7b9e3b 100644
> >>> --- a/drivers/gpu/drm/sun4i/sun4i_layer.h
> >>> +++ b/drivers/gpu/drm/sun4i/sun4i_layer.h
> >>> @@ -27,6 +27,6 @@ plane_to_sun4i_layer(struct drm_plane *plane)
> >>> }
> >>>
> >>> struct sun4i_layer **sun4i_layers_init(struct drm_device *drm,
> >>> - struct sun4i_backend *backend);
> >>> + struct sun4i_crtc *crtc);
> >>>
> >>> #endif /* _SUN4I_LAYER_H_ */
> >>> diff --git a/drivers/gpu/drm/sun4i/sunxi_layer.h
> >>> b/drivers/gpu/drm/sun4i/sunxi_layer.h
> >>> new file mode 100644
> >>> index 000000000000..d8838ec39299
> >>> --- /dev/null
> >>> +++ b/drivers/gpu/drm/sun4i/sunxi_layer.h
> >>> @@ -0,0 +1,17 @@
> >>> +/*
> >>> + * Copyright (C) 2017 Icenowy Zheng <icenowy at aosc.xyz>
> >>> + *
> >>> + * This program is free software; you can redistribute it and/or
> >>> + * modify it under the terms of the GNU General Public License as
> >>> + * published by the Free Software Foundation; either version 2 of
> >>> + * the License, or (at your option) any later version.
> >>> + */
> >>> +
> >>> +#ifndef _SUNXI_LAYER_H_
> >>> +#define _SUNXI_LAYER_H_
> >>> +
> >>> +struct sunxi_layer_ops {
> >>> + struct drm_plane *(*get_plane)(void *layer);
> >>> +};
> >>> +
> >>> +#endif /* _SUNXI_LAYER_H_ */
> >>> --
> >>> 2.12.0
> >>>
> >>>
> >>> _______________________________________________
> >>> linux-arm-kernel mailing list
> >>> linux-arm-kernel at lists.infradead.org
> >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>
> >>
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "linux-sunxi" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to linux-sunxi+unsubscribe at googlegroups.com.
> > For more options, visit https://groups.google.com/d/optout.
More information about the dri-devel
mailing list