* [linux-sunxi] Re: [PATCH v3 04/11] drm/sun4i: abstract the layer type [not found] <20170405052325.98B958A2E94@relay.mailchannels.net> @ 2017-04-05 8:09 ` Maxime Ripard 0 siblings, 0 replies; 3+ messages in thread From: Maxime Ripard @ 2017-04-05 8:09 UTC (permalink / raw) To: linux-arm-kernel On Wed, Apr 05, 2017 at 01:23:15PM +0800, Icenowy Zheng wrote: > > 2017?4?5? 10:27? Chen-Yu Tsai <wens@csie.org>??? > > > > On Wed, Apr 5, 2017 at 3:53 AM, Icenowy Zheng <icenowy@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@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). You'll always have one primary and one cursor plane, no matter how much planes you support. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170405/bd177b26/attachment.sig> ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v3 00/11] Initial Allwinner Display Engine 2.0 Support @ 2017-03-29 19:46 Icenowy Zheng 2017-03-29 19:46 ` [PATCH v3 04/11] drm/sun4i: abstract the layer type Icenowy Zheng 0 siblings, 1 reply; 3+ messages in thread From: Icenowy Zheng @ 2017-03-29 19:46 UTC (permalink / raw) To: linux-arm-kernel This patchset is the initial patchset for Allwinner DE2 support. It contains the support of clocks in DE2 and the mixers in DE2. The SoC used to develop this patchset is V3s, as V3s is the simplest one of the SoCs that have DE2. (Allwinner V3s features only one mixer, although its clock control unit contains support for second mixer's clock; and its only video output is RGB LCD, which is already supported in our TCON driver) The last patch is only a testing patch, it shouldn't be merged; and for the patch to be really usable, the RFC fix of the TCON driver [1] is needed. No HDMI, TV encoder or other internal bridges' support is included in this patchset, which makes it currently not usable on H3. Thanks to Jean-Francois Moine and Jernej Skrabec for their efforts to discover the internal of DE2! [1] https://lists.freedesktop.org/archives/dri-devel/2016-December/126264.html Icenowy Zheng (11): dt-bindings: add binding for the Allwinner DE2 CCU clk: sunxi-ng: add support for DE2 CCU dt-bindings: add bindings for DE2 on V3s SoC drm/sun4i: abstruct the layer type drm/sun4i: abstract a mixer type drm/sun4i: add support for Allwinner DE2 mixers drm/sun4i: Add compatible string for V3s display engine drm/sun4i: tcon: add support for V3s TCON ARM: dts: sun8i: add DE2 nodes for V3s SoC ARM: dts: sun8i: add pinmux for LCD pins of V3s SoC [DO NOT MERGE] ARM: dts: sun8i: enable LCD panel of Lichee Pi Zero .../devicetree/bindings/clock/sun8i-de2.txt | 31 ++ .../bindings/display/sunxi/sun4i-drm.txt | 37 +- arch/arm/boot/dts/sun8i-v3s-licheepi-zero.dts | 36 ++ arch/arm/boot/dts/sun8i-v3s.dtsi | 96 +++++ drivers/clk/sunxi-ng/Kconfig | 5 + drivers/clk/sunxi-ng/Makefile | 1 + drivers/clk/sunxi-ng/ccu-sun8i-de2.c | 204 +++++++++++ drivers/clk/sunxi-ng/ccu-sun8i-de2.h | 28 ++ drivers/gpu/drm/sun4i/Kconfig | 20 ++ drivers/gpu/drm/sun4i/Makefile | 10 +- drivers/gpu/drm/sun4i/sun4i_backend.c | 26 +- drivers/gpu/drm/sun4i/sun4i_backend.h | 5 - drivers/gpu/drm/sun4i/sun4i_crtc.c | 31 +- drivers/gpu/drm/sun4i/sun4i_crtc.h | 10 +- drivers/gpu/drm/sun4i/sun4i_drv.c | 4 +- drivers/gpu/drm/sun4i/sun4i_drv.h | 3 +- drivers/gpu/drm/sun4i/sun4i_layer.c | 22 +- drivers/gpu/drm/sun4i/sun4i_layer.h | 6 +- drivers/gpu/drm/sun4i/sun4i_tcon.c | 7 +- drivers/gpu/drm/sun4i/sun4i_tv.c | 8 +- drivers/gpu/drm/sun4i/sun8i_layer.c | 156 +++++++++ drivers/gpu/drm/sun4i/sun8i_layer.h | 35 ++ drivers/gpu/drm/sun4i/sun8i_mixer.c | 386 +++++++++++++++++++++ drivers/gpu/drm/sun4i/sun8i_mixer.h | 131 +++++++ drivers/gpu/drm/sun4i/sunxi_layer.h | 17 + drivers/gpu/drm/sun4i/sunxi_mixer.h | 22 ++ include/dt-bindings/clock/sun8i-de2.h | 54 +++ include/dt-bindings/reset/sun8i-de2.h | 50 +++ 28 files changed, 1391 insertions(+), 50 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/sun8i-de2.txt create mode 100644 drivers/clk/sunxi-ng/ccu-sun8i-de2.c create mode 100644 drivers/clk/sunxi-ng/ccu-sun8i-de2.h create mode 100644 drivers/gpu/drm/sun4i/sun8i_layer.c create mode 100644 drivers/gpu/drm/sun4i/sun8i_layer.h create mode 100644 drivers/gpu/drm/sun4i/sun8i_mixer.c create mode 100644 drivers/gpu/drm/sun4i/sun8i_mixer.h create mode 100644 drivers/gpu/drm/sun4i/sunxi_layer.h create mode 100644 drivers/gpu/drm/sun4i/sunxi_mixer.h create mode 100644 include/dt-bindings/clock/sun8i-de2.h create mode 100644 include/dt-bindings/reset/sun8i-de2.h -- 2.12.0 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v3 04/11] drm/sun4i: abstract the layer type 2017-03-29 19:46 [PATCH v3 00/11] Initial Allwinner Display Engine 2.0 Support Icenowy Zheng @ 2017-03-29 19:46 ` Icenowy Zheng 2017-04-04 19:28 ` Sean Paul 0 siblings, 1 reply; 3+ messages in thread From: Icenowy Zheng @ 2017-03-29 19:46 UTC (permalink / raw) To: linux-arm-kernel 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@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; }; 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@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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH v3 04/11] drm/sun4i: abstract the layer type 2017-03-29 19:46 ` [PATCH v3 04/11] drm/sun4i: abstract the layer type Icenowy Zheng @ 2017-04-04 19:28 ` Sean Paul 2017-04-04 19:53 ` Icenowy Zheng 0 siblings, 1 reply; 3+ messages in thread From: Sean Paul @ 2017-04-04 19:28 UTC (permalink / raw) To: linux-arm-kernel 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@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. 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@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 -- Sean Paul, Software Engineer, Google / Chromium OS ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v3 04/11] drm/sun4i: abstract the layer type 2017-04-04 19:28 ` Sean Paul @ 2017-04-04 19:53 ` Icenowy Zheng 2017-04-05 2:27 ` [linux-sunxi] " Chen-Yu Tsai 0 siblings, 1 reply; 3+ messages in thread From: Icenowy Zheng @ 2017-04-04 19:53 UTC (permalink / raw) To: linux-arm-kernel ? 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@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. > > 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@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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* [linux-sunxi] Re: [PATCH v3 04/11] drm/sun4i: abstract the layer type 2017-04-04 19:53 ` Icenowy Zheng @ 2017-04-05 2:27 ` Chen-Yu Tsai 2017-04-05 17:14 ` icenowy at aosc.io 0 siblings, 1 reply; 3+ messages in thread From: Chen-Yu Tsai @ 2017-04-05 2:27 UTC (permalink / raw) To: linux-arm-kernel On Wed, Apr 5, 2017 at 3:53 AM, Icenowy Zheng <icenowy@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@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. 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@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. ^ permalink raw reply [flat|nested] 3+ messages in thread
* [linux-sunxi] Re: [PATCH v3 04/11] drm/sun4i: abstract the layer type 2017-04-05 2:27 ` [linux-sunxi] " Chen-Yu Tsai @ 2017-04-05 17:14 ` icenowy at aosc.io 0 siblings, 0 replies; 3+ messages in thread From: icenowy at aosc.io @ 2017-04-05 17:14 UTC (permalink / raw) To: linux-arm-kernel ? 2017-04-05 10:27?Chen-Yu Tsai ??? > On Wed, Apr 5, 2017 at 3:53 AM, Icenowy Zheng <icenowy@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@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. I feel that it's still more proper to offload plane creation code to *_layers_init function, as: 1. We cannot assume the cursor layer's existance. In fact currently no code in sun4i-drm (including this patchset) create a cursor layer. 2. The format of planes heavily depend on the engine type ( sun4i-backend or sun8i-mixer). 3. We should create planes according to the type of engine. Currently the *_layers_init function is part of engine code (See my Makefile change). 4. If we do so we will have two codes for plane creating -- one for primary in sun4i_crtc_init, another for overlays in *_layers_init. > > 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. It's correct, layers doesn't need to be kept. And the struct sunxi_layer refactor also makes sense. > > 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@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. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-04-05 17:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20170405052325.98B958A2E94@relay.mailchannels.net>
2017-04-05 8:09 ` [linux-sunxi] Re: [PATCH v3 04/11] drm/sun4i: abstract the layer type Maxime Ripard
2017-03-29 19:46 [PATCH v3 00/11] Initial Allwinner Display Engine 2.0 Support Icenowy Zheng
2017-03-29 19:46 ` [PATCH v3 04/11] drm/sun4i: abstract the layer type Icenowy Zheng
2017-04-04 19:28 ` Sean Paul
2017-04-04 19:53 ` Icenowy Zheng
2017-04-05 2:27 ` [linux-sunxi] " Chen-Yu Tsai
2017-04-05 17:14 ` icenowy at aosc.io
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox