Re: [PATCH v4 05/11] drm/sun4i: abstract a engine type

[Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 8347 bytes --]

Hi,

Thanks for that rework.

On Sun, Apr 16, 2017 at 08:08:43PM +0800, Icenowy Zheng wrote:
> As we are going to add support for the Allwinner DE2 engine in sun4i-drm
> driver, we will finally have two types of display engines -- the DE1
> backend and the DE2 mixer. They both do some display blending and feed
> graphics data to TCON, so I choose to call them both "engine" here.
> 
> Abstract the engine type to void * and a ops struct, which contains
> functions that should be called outside the engine-specified code (in
> TCON, CRTC or TV Encoder code).
> 
> A dedicated Kconfig option is also added to control whether
> sun4i-backend-specified code (sun4i_backend.c and sun4i_layer.c) should
> be built. As we removed the codes in CRTC code that directly call the
> layer code, we can now extract the layer part and combine it with the
> backend part into a new module, sun4i-backend.ko.
> 
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> ---
> Changes in v4:
> - Comments to tag the color correction functions as optional.
> - Check before calling the optional functions.
> - Change layers_init to satisfy new PATCH v4 04/11.
> 
>  drivers/gpu/drm/sun4i/Kconfig         | 10 ++++++++++
>  drivers/gpu/drm/sun4i/Makefile        |  6 ++++--
>  drivers/gpu/drm/sun4i/sun4i_backend.c | 26 +++++++++++++++++++-------
>  drivers/gpu/drm/sun4i/sun4i_backend.h |  5 -----
>  drivers/gpu/drm/sun4i/sun4i_crtc.c    | 14 +++++++-------
>  drivers/gpu/drm/sun4i/sun4i_crtc.h    |  7 ++++---
>  drivers/gpu/drm/sun4i/sun4i_drv.h     |  3 ++-
>  drivers/gpu/drm/sun4i/sun4i_layer.c   |  2 +-
>  drivers/gpu/drm/sun4i/sun4i_layer.h   |  3 ++-
>  drivers/gpu/drm/sun4i/sun4i_tcon.c    |  2 +-
>  drivers/gpu/drm/sun4i/sun4i_tv.c      | 11 ++++++-----
>  drivers/gpu/drm/sun4i/sunxi_engine.h  | 35 +++++++++++++++++++++++++++++++++++
>  12 files changed, 91 insertions(+), 33 deletions(-)
>  create mode 100644 drivers/gpu/drm/sun4i/sunxi_engine.h
> 
> diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig
> index a4b357db8856..5a8227f37cc4 100644
> --- a/drivers/gpu/drm/sun4i/Kconfig
> +++ b/drivers/gpu/drm/sun4i/Kconfig
> @@ -12,3 +12,13 @@ config DRM_SUN4I
>  	  Choose this option if you have an Allwinner SoC with a
>  	  Display Engine. If M is selected the module will be called
>  	  sun4i-drm.
> +
> +config DRM_SUN4I_BACKEND
> +	tristate "Support for Allwinner A10 Display Engine Backend"
> +	depends on DRM_SUN4I
> +	default DRM_SUN4I
> +	help
> +	  Choose this option if you have an Allwinner SoC with the
> +	  original Allwinner Display Engine, which has a backend to
> +	  do some alpha blending and feed graphics to TCON. If M is
> +	  selected the module will be called sun4i-backend.
> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile
> index 59b757350a1f..1db1068b9be1 100644
> --- a/drivers/gpu/drm/sun4i/Makefile
> +++ b/drivers/gpu/drm/sun4i/Makefile
> @@ -5,9 +5,11 @@ sun4i-tcon-y += sun4i_tcon.o
>  sun4i-tcon-y += sun4i_rgb.o
>  sun4i-tcon-y += sun4i_dotclock.o
>  sun4i-tcon-y += sun4i_crtc.o
> -sun4i-tcon-y += sun4i_layer.o
> +
> +sun4i-backend-y += sun4i_layer.o
> +sun4i-backend-y += sun4i_backend.o
>  
>  obj-$(CONFIG_DRM_SUN4I)		+= sun4i-drm.o sun4i-tcon.o
> -obj-$(CONFIG_DRM_SUN4I)		+= sun4i_backend.o
> +obj-$(CONFIG_DRM_SUN4I_BACKEND)	+= sun4i-backend.o
>  obj-$(CONFIG_DRM_SUN4I)		+= sun6i_drc.o
>  obj-$(CONFIG_DRM_SUN4I)		+= sun4i_tv.o
> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> index d660741ba475..a16c96a002a4 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> @@ -23,6 +23,8 @@
>  
>  #include "sun4i_backend.h"
>  #include "sun4i_drv.h"
> +#include "sun4i_layer.h"
> +#include "sunxi_engine.h"
>  
>  static const u32 sunxi_rgb2yuv_coef[12] = {
>  	0x00000107, 0x00000204, 0x00000064, 0x00000108,
> @@ -30,9 +32,10 @@ static const u32 sunxi_rgb2yuv_coef[12] = {
>  	0x000001c1, 0x00003e88, 0x00003fb8, 0x00000808
>  };
>  
> -void sun4i_backend_apply_color_correction(struct sun4i_backend *backend)
> +static void sun4i_backend_apply_color_correction(void *engine)
>  {
>  	int i;
> +	struct sun4i_backend *backend = engine;

I'm not really fond of passing an opaque pointer here, and hope that
things will work out.

I think having a common structure, holding the common thingsand a more
specific structure for that one would work better.

Something like

struct sunxi_engine {
       struct regmap	*regs;
};

struct sun4i_backend {
       struct sunxi_engine	engine;

	struct clk		*sat_clk;
	struct reset_control	*sat_reset;
	
};

static void sun4i_backend_apply_color_correction(struct sunxi_engine *engine)
       struct sun4i_backend *backend = container_of(engine, struct sun4i_backend, engine);

...

> +static const struct sunxi_engine_ops sun4i_backend_engine_ops = {
> +	.commit = sun4i_backend_commit,
> +	.layers_init = sun4i_layers_init,
> +	.apply_color_correction = sun4i_backend_apply_color_correction,
> +	.disable_color_correction = sun4i_backend_disable_color_correction,

Please align the values with tabs, like done in the other structures
created that way in this driver.

>  static void sun4i_crtc_atomic_begin(struct drm_crtc *crtc,
> @@ -56,7 +55,7 @@ static void sun4i_crtc_atomic_flush(struct drm_crtc *crtc,
>  
>  	DRM_DEBUG_DRIVER("Committing plane changes\n");
>  
> -	sun4i_backend_commit(scrtc->backend);
> +	scrtc->engine_ops->commit(scrtc->engine);

You rely on the backend having setup things properly, which is pretty
fragile. Ideally, you should have a function to check that engine_ops
and commit is !NULL, and call it, and the consumers would use that
function...

> @@ -362,7 +361,9 @@ static void sun4i_tv_disable(struct drm_encoder *encoder)
>  	regmap_update_bits(tv->regs, SUN4I_TVE_EN_REG,
>  			   SUN4I_TVE_EN_ENABLE,
>  			   0);
> -	sun4i_backend_disable_color_correction(backend);
> +
> +	if (crtc->engine_ops->disable_color_correction)
> +		crtc->engine_ops->disable_color_correction(crtc->engine);
>  }

... Instead of having to do it in some cases, but not always ...

>  static void sun4i_tv_enable(struct drm_encoder *encoder)
> @@ -370,11 +371,11 @@ static void sun4i_tv_enable(struct drm_encoder *encoder)
>  	struct sun4i_tv *tv = drm_encoder_to_sun4i_tv(encoder);
>  	struct sun4i_crtc *crtc = drm_crtc_to_sun4i_crtc(encoder->crtc);
>  	struct sun4i_tcon *tcon = crtc->tcon;
> -	struct sun4i_backend *backend = crtc->backend;
>  
>  	DRM_DEBUG_DRIVER("Enabling the TV Output\n");
>  
> -	sun4i_backend_apply_color_correction(backend);
> +	if (crtc->engine_ops->apply_color_correction)
> +		crtc->engine_ops->apply_color_correction(crtc->engine);
>  
>  	regmap_update_bits(tv->regs, SUN4I_TVE_EN_REG,
>  			   SUN4I_TVE_EN_ENABLE,
> diff --git a/drivers/gpu/drm/sun4i/sunxi_engine.h b/drivers/gpu/drm/sun4i/sunxi_engine.h
> new file mode 100644
> index 000000000000..a9128abda66f
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sunxi_engine.h
> @@ -0,0 +1,35 @@
> +/*
> + * Copyright (C) 2017 Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> + *
> + * 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_ENGINE_H_
> +#define _SUNXI_ENGINE_H_
> +
> +struct sun4i_crtc;
> +
> +struct sunxi_engine_ops {
> +	/* Commit the changes to the engine */
> +	void (*commit)(void *engine);
> +	/* Initialize layers (planes) for this engine */
> +	struct drm_plane **(*layers_init)(struct drm_device *drm,
> +					  struct sun4i_crtc *crtc);
> +
> +	/*
> +	 * These are optional functions for the TV Encoder. Please check
> +	 * their presence before calling them.
> +	 *
> +	 * The first function applies the color space correction needed
> +	 * for outputing correct TV signal.
> +	 *
> +	 * The second function disabled the correction.
> +	 */
> +	void (*apply_color_correction)(void *engine);
> +	void (*disable_color_correction)(void *engine);

... And have to document it.

Please also use kerneldoc for those comments.

Thanks again!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com