Re: [PATCH v3 05/11] drm/sun4i: abstract a mixer type

2017-04-03 Thread Maxime Ripard 
Hi,

That's much better thanks, but I have a bunch of (minor) comments.

On Thu, Mar 30, 2017 at 03:46:07AM +0800, Icenowy Zheng wrote:
> diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c 
> b/drivers/gpu/drm/sun4i/sun4i_crtc.c
> index 33854ee7f636..938dfe7188ff 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
> @@ -25,11 +25,10 @@
>  
>  #include 
>  
> -#include "sun4i_backend.h"
>  #include "sun4i_crtc.h"
>  #include "sun4i_drv.h"
> -#include "sun4i_layer.h"
>  #include "sunxi_layer.h"
> +#include "sunxi_mixer.h"
>  #include "sun4i_tcon.h"
>  
>  static void sun4i_crtc_atomic_begin(struct drm_crtc *crtc,
> @@ -57,7 +56,7 @@ static void sun4i_crtc_atomic_flush(struct drm_crtc *crtc,
>  
>   DRM_DEBUG_DRIVER("Committing plane changes\n");
>  
> - sun4i_backend_commit(scrtc->backend);
> + scrtc->mixer_ops->commit(scrtc->mixer);

The caller would also have to care about whether that pointer is NULL
or not. This is not really a big deal in the commit case that I expect
to always be filled, but it might be for the color correction.

How about defining some inline functions that would check the
mixer_ops pointer and return if it's NULL?

>   if (event) {
>   crtc->state->event = NULL;
> @@ -135,8 +134,8 @@ static const struct drm_crtc_funcs sun4i_crtc_funcs = {
>   .disable_vblank = sun4i_crtc_disable_vblank,
>  };
>  
> -struct sun4i_crtc *sun4i_crtc_init(struct drm_device *drm,
> -struct sun4i_backend *backend,
> +struct sun4i_crtc *sun4i_crtc_init(struct drm_device *drm, void *mixer,
> +const struct sunxi_mixer_ops *mixer_ops,
>  struct sun4i_tcon *tcon)
>  {
>   struct sun4i_crtc *scrtc;
> @@ -146,11 +145,12 @@ struct sun4i_crtc *sun4i_crtc_init(struct drm_device 
> *drm,
>   scrtc = devm_kzalloc(drm->dev, sizeof(*scrtc), GFP_KERNEL);
>   if (!scrtc)
>   return ERR_PTR(-ENOMEM);
> - scrtc->backend = backend;
> + scrtc->mixer = mixer;
> + scrtc->mixer_ops = mixer_ops;
>   scrtc->tcon = tcon;
>  
>   /* Create our layers */
> - scrtc->layers = (void **)sun4i_layers_init(drm, scrtc);
> + scrtc->layers = mixer_ops->layers_init(drm, scrtc);

I don't really see why we need the mixer and ops in the CRTC, we
already have it in sun4i_drv, don't we? Can't we just use that
instead?

That would allow just like in the previous patch to loosen the
relationship between the CRTC and what's before it, which will make
our life easier down the road.

> diff --git a/drivers/gpu/drm/sun4i/sunxi_mixer.h 
> b/drivers/gpu/drm/sun4i/sunxi_mixer.h
> new file mode 100644
> index ..11bdd20269ef
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sunxi_mixer.h
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright (C) 2017 Icenowy Zheng 
> + *
> + * 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_MIXER_H_
> +#define _SUNXI_MIXER_H_
> +
> +struct sun4i_crtc;
> +
> +struct sunxi_mixer_ops {
> + void **(*layers_init)(struct drm_device *drm, struct sun4i_crtc *crtc);
> + void (*apply_color_correction)(void *mixer);
> + void (*disable_color_correction)(void *mixer);
> + void (*commit)(void *mixer);
> +};
> +
> +#endif /* _SUNXI_MIXER_H_ */

I don't really like the "mixer" name here, and backend doesn't seem
appropriate either.

What about "engine"? It sounds more like it covers both cases.

Thanks,
Maxime

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


signature.asc
Description: PGP signature

Re: [PATCH v3 05/11] drm/sun4i: abstract a mixer type

2017-04-03 Thread Maxime Ripard 
Hi,

That's much better thanks, but I have a bunch of (minor) comments.

On Thu, Mar 30, 2017 at 03:46:07AM +0800, Icenowy Zheng wrote:
> diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c 
> b/drivers/gpu/drm/sun4i/sun4i_crtc.c
> index 33854ee7f636..938dfe7188ff 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
> @@ -25,11 +25,10 @@
>  
>  #include 
>  
> -#include "sun4i_backend.h"
>  #include "sun4i_crtc.h"
>  #include "sun4i_drv.h"
> -#include "sun4i_layer.h"
>  #include "sunxi_layer.h"
> +#include "sunxi_mixer.h"
>  #include "sun4i_tcon.h"
>  
>  static void sun4i_crtc_atomic_begin(struct drm_crtc *crtc,
> @@ -57,7 +56,7 @@ static void sun4i_crtc_atomic_flush(struct drm_crtc *crtc,
>  
>   DRM_DEBUG_DRIVER("Committing plane changes\n");
>  
> - sun4i_backend_commit(scrtc->backend);
> + scrtc->mixer_ops->commit(scrtc->mixer);

The caller would also have to care about whether that pointer is NULL
or not. This is not really a big deal in the commit case that I expect
to always be filled, but it might be for the color correction.

How about defining some inline functions that would check the
mixer_ops pointer and return if it's NULL?

>   if (event) {
>   crtc->state->event = NULL;
> @@ -135,8 +134,8 @@ static const struct drm_crtc_funcs sun4i_crtc_funcs = {
>   .disable_vblank = sun4i_crtc_disable_vblank,
>  };
>  
> -struct sun4i_crtc *sun4i_crtc_init(struct drm_device *drm,
> -struct sun4i_backend *backend,
> +struct sun4i_crtc *sun4i_crtc_init(struct drm_device *drm, void *mixer,
> +const struct sunxi_mixer_ops *mixer_ops,
>  struct sun4i_tcon *tcon)
>  {
>   struct sun4i_crtc *scrtc;
> @@ -146,11 +145,12 @@ struct sun4i_crtc *sun4i_crtc_init(struct drm_device 
> *drm,
>   scrtc = devm_kzalloc(drm->dev, sizeof(*scrtc), GFP_KERNEL);
>   if (!scrtc)
>   return ERR_PTR(-ENOMEM);
> - scrtc->backend = backend;
> + scrtc->mixer = mixer;
> + scrtc->mixer_ops = mixer_ops;
>   scrtc->tcon = tcon;
>  
>   /* Create our layers */
> - scrtc->layers = (void **)sun4i_layers_init(drm, scrtc);
> + scrtc->layers = mixer_ops->layers_init(drm, scrtc);

I don't really see why we need the mixer and ops in the CRTC, we
already have it in sun4i_drv, don't we? Can't we just use that
instead?

That would allow just like in the previous patch to loosen the
relationship between the CRTC and what's before it, which will make
our life easier down the road.

> diff --git a/drivers/gpu/drm/sun4i/sunxi_mixer.h 
> b/drivers/gpu/drm/sun4i/sunxi_mixer.h
> new file mode 100644
> index ..11bdd20269ef
> --- /dev/null
> +++ b/drivers/gpu/drm/sun4i/sunxi_mixer.h
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright (C) 2017 Icenowy Zheng 
> + *
> + * 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_MIXER_H_
> +#define _SUNXI_MIXER_H_
> +
> +struct sun4i_crtc;
> +
> +struct sunxi_mixer_ops {
> + void **(*layers_init)(struct drm_device *drm, struct sun4i_crtc *crtc);
> + void (*apply_color_correction)(void *mixer);
> + void (*disable_color_correction)(void *mixer);
> + void (*commit)(void *mixer);
> +};
> +
> +#endif /* _SUNXI_MIXER_H_ */

I don't really like the "mixer" name here, and backend doesn't seem
appropriate either.

What about "engine"? It sounds more like it covers both cases.

Thanks,
Maxime

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


signature.asc
Description: PGP signature

[PATCH v3 05/11] drm/sun4i: abstract a mixer type

2017-03-29 Thread Icenowy Zheng 
As we are going to add support for the Allwinner DE2 mixer in sun4i-drm
driver, we will finally have two types of display mixers -- 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 "mixer" here.

Abstract the mixer type to void * and a ops struct, which contains
functions that should be called outside the mixer-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 
---
Refactored patch in v3.

 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   |  7 +++
 drivers/gpu/drm/sun4i/sun4i_layer.h   |  6 +++---
 drivers/gpu/drm/sun4i/sun4i_tcon.c|  2 +-
 drivers/gpu/drm/sun4i/sun4i_tv.c  |  8 +++-
 drivers/gpu/drm/sun4i/sunxi_mixer.h   | 22 ++
 12 files changed, 78 insertions(+), 38 deletions(-)
 create mode 100644 drivers/gpu/drm/sun4i/sunxi_mixer.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..35ea51b686e6 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_mixer.h"
 
 static const u32 sunxi_rgb2yuv_coef[12] = {
0x0107, 0x0204, 0x0064, 0x0108,
@@ -30,9 +32,10 @@ static const u32 sunxi_rgb2yuv_coef[12] = {
0x01c1, 0x3e88, 0x3fb8, 0x0808
 };
 
-void sun4i_backend_apply_color_correction(struct sun4i_backend *backend)
+static void sun4i_backend_apply_color_correction(void *mixer)
 {
int i;
+   struct sun4i_backend *backend = mixer;
 
DRM_DEBUG_DRIVER("Applying RGB to YUV color correction\n");
 
@@ -44,27 +47,28 @@ void sun4i_backend_apply_color_correction(struct 
sun4i_backend *backend)
regmap_write(backend->regs, SUN4I_BACKEND_OCRCOEF_REG(i),
 sunxi_rgb2yuv_coef[i]);
 }
-EXPORT_SYMBOL(sun4i_backend_apply_color_correction);
 
-void sun4i_backend_disable_color_correction(struct sun4i_backend *backend)
+static void sun4i_backend_disable_color_correction(void *mixer)
 {
+   struct sun4i_backend *backend = mixer;
+
DRM_DEBUG_DRIVER("Disabling color correction\n");
 
/* Disable color correction */
regmap_update_bits(backend->regs, SUN4I_BACKEND_OCCTL_REG,
   SUN4I_BACKEND_OCCTL_ENABLE, 0);
 }
-EXPORT_SYMBOL(sun4i_backend_disable_color_correction);
 
-void sun4i_backend_commit(struct sun4i_backend *backend)
+static void sun4i_backend_commit(void *mixer)
 {
+   struct sun4i_backend *backend = mixer;
+
DRM_DEBUG_DRIVER("Committing changes\n");
 
regmap_write(backend->regs,

[PATCH v3 05/11] drm/sun4i: abstract a mixer type

2017-03-29 Thread Icenowy Zheng 
As we are going to add support for the Allwinner DE2 mixer in sun4i-drm
driver, we will finally have two types of display mixers -- 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 "mixer" here.

Abstract the mixer type to void * and a ops struct, which contains
functions that should be called outside the mixer-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 
---
Refactored patch in v3.

 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   |  7 +++
 drivers/gpu/drm/sun4i/sun4i_layer.h   |  6 +++---
 drivers/gpu/drm/sun4i/sun4i_tcon.c|  2 +-
 drivers/gpu/drm/sun4i/sun4i_tv.c  |  8 +++-
 drivers/gpu/drm/sun4i/sunxi_mixer.h   | 22 ++
 12 files changed, 78 insertions(+), 38 deletions(-)
 create mode 100644 drivers/gpu/drm/sun4i/sunxi_mixer.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..35ea51b686e6 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_mixer.h"
 
 static const u32 sunxi_rgb2yuv_coef[12] = {
0x0107, 0x0204, 0x0064, 0x0108,
@@ -30,9 +32,10 @@ static const u32 sunxi_rgb2yuv_coef[12] = {
0x01c1, 0x3e88, 0x3fb8, 0x0808
 };
 
-void sun4i_backend_apply_color_correction(struct sun4i_backend *backend)
+static void sun4i_backend_apply_color_correction(void *mixer)
 {
int i;
+   struct sun4i_backend *backend = mixer;
 
DRM_DEBUG_DRIVER("Applying RGB to YUV color correction\n");
 
@@ -44,27 +47,28 @@ void sun4i_backend_apply_color_correction(struct 
sun4i_backend *backend)
regmap_write(backend->regs, SUN4I_BACKEND_OCRCOEF_REG(i),
 sunxi_rgb2yuv_coef[i]);
 }
-EXPORT_SYMBOL(sun4i_backend_apply_color_correction);
 
-void sun4i_backend_disable_color_correction(struct sun4i_backend *backend)
+static void sun4i_backend_disable_color_correction(void *mixer)
 {
+   struct sun4i_backend *backend = mixer;
+
DRM_DEBUG_DRIVER("Disabling color correction\n");
 
/* Disable color correction */
regmap_update_bits(backend->regs, SUN4I_BACKEND_OCCTL_REG,
   SUN4I_BACKEND_OCCTL_ENABLE, 0);
 }
-EXPORT_SYMBOL(sun4i_backend_disable_color_correction);
 
-void sun4i_backend_commit(struct sun4i_backend *backend)
+static void sun4i_backend_commit(void *mixer)
 {
+   struct sun4i_backend *backend = mixer;
+
DRM_DEBUG_DRIVER("Committing changes\n");
 
regmap_write(backend->regs,