From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756162AbdDRLGq convert rfc822-to-8bit (ORCPT ); Tue, 18 Apr 2017 07:06:46 -0400 Received: from hermes.aosc.io ([199.195.250.187]:36796 "EHLO hermes.aosc.io" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751488AbdDRLFa (ORCPT ); Tue, 18 Apr 2017 07:05:30 -0400 Date: Tue, 18 Apr 2017 19:05:12 +0800 In-Reply-To: <20170418085548.4cisone2yfcuizcp@lukather> References: <20170416120849.54542-1-icenowy@aosc.io> <20170416120849.54542-6-icenowy@aosc.io> <20170418085548.4cisone2yfcuizcp@lukather> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Subject: Re: [PATCH v4 05/11] drm/sun4i: abstract a engine type To: Maxime Ripard CC: Rob Herring , Chen-Yu Tsai , David Airlie , Jernej Skrabec , linux-clk@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-sunxi@googlegroups.com From: Icenowy Zheng Message-ID: <5F52665D-8A24-40D3-B84B-E8991B3BE457@aosc.io> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 于 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] = { >> 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 >> + * >> + * 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