于 2017年4月18日 GMT+08:00 下午4:55:48, Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx> 写到:
>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@xxxxxxx>
>> ---
>> 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;
>
>};
Sounds good ;-)
>
>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...
If it's really NULL how should the function return?
>
>> @@ -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@xxxxxxx>
>> + *
>> + * 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
