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

2017-04-20 Thread Maxime Ripard 
On Tue, Apr 18, 2017 at 07:05:12PM +0800, Icenowy Zheng wrote:
> >> @@ -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...
> 
> If it's really NULL how should the function return?

It depends on the return code. ENOSYS if it returns an int, and simply
does nothing if it's a void. I don't think any of the current
functions return an error code at the moment though, so I'd just keep
the current behaviour and just call the function if it's set.

You cannot fail in atomic_flush anyway.

Maxime

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


signature.asc
Description: PGP signature

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

2017-04-20 Thread Maxime Ripard 
On Tue, Apr 18, 2017 at 07:05:12PM +0800, Icenowy Zheng wrote:
> >> @@ -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...
> 
> If it's really NULL how should the function return?

It depends on the return code. ENOSYS if it returns an int, and simply
does nothing if it's a void. I don't think any of the current
functions return an error code at the moment though, so I'd just keep
the current behaviour and just call the function if it's set.

You cannot fail in atomic_flush anyway.

Maxime

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


signature.asc
Description: PGP signature

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

2017-04-18 Thread Icenowy Zheng 


于 2017年4月18日 GMT+08:00 下午4:55:48, Maxime Ripard 
 写到:
>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 
>> ---
>> 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] = {
>>  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 *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;
>   
>};

Sounds good ;-)

>

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

2017-04-18 Thread Icenowy Zheng 


于 2017年4月18日 GMT+08:00 下午4:55:48, Maxime Ripard 
 写到:
>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 
>> ---
>> 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] = {
>>  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 *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;
>   
>};

Sounds good ;-)

>
>static void

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

2017-04-18 Thread Maxime Ripard 
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 
> ---
> 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] = {
>   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 *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

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

2017-04-18 Thread Maxime Ripard 
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 
> ---
> 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] = {
>   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 *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

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

2017-04-16 Thread Icenowy Zheng 
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 
---
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] = {
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 *engine)
 {
int i;
+   struct sun4i_backend *backend = engine;
 
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 *engine)
 {
+   struct sun4i_backend *backend = engine;
+
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

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

2017-04-16 Thread Icenowy Zheng 
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 
---
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] = {
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 *engine)
 {
int i;
+   struct sun4i_backend *backend = engine;
 
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 *engine)
 {
+   struct sun4i_backend *backend = engine;
+
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