Re: [linux-sunxi] Re: [PATCH v3 04/11] drm/sun4i: abstract the layer type

[Maxime Ripard]

On Wed, Apr 05, 2017 at 01:23:15PM +0800, Icenowy Zheng wrote:
> 
> 2017å4æ5æ 10:27ä Chen-Yu Tsai <wens@xxxxxxxx>åéï
> >
> > On Wed, Apr 5, 2017 at 3:53 AM, Icenowy Zheng <icenowy@xxxxxxx> 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@xxxxxxx> 
> > >>> --- 
> > >>> 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

Attachment: signature.asc
Description: PGP signature