From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753279AbdEEC4m (ORCPT ); Thu, 4 May 2017 22:56:42 -0400 Received: from smtp.csie.ntu.edu.tw ([140.112.30.61]:38018 "EHLO smtp.csie.ntu.edu.tw" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751159AbdEEC4k (ORCPT ); Thu, 4 May 2017 22:56:40 -0400 MIME-Version: 1.0 In-Reply-To: <20170504114858.9008-6-icenowy@aosc.io> References: <20170504114858.9008-1-icenowy@aosc.io> <20170504114858.9008-6-icenowy@aosc.io> From: Chen-Yu Tsai Date: Fri, 5 May 2017 10:56:14 +0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [linux-sunxi] [PATCH v6 05/13] drm/sun4i: abstract a engine type To: Icenowy Zheng Cc: Maxime Ripard , Rob Herring , Chen-Yu Tsai , linux-clk , devicetree , linux-arm-kernel , linux-kernel , dri-devel , linux-sunxi Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 4, 2017 at 7:48 PM, 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. These engines composite different layers into a final image which is then sent out to the TCONs. As such, "compositor" would be an accurate name. However, "engine" is OK, since Allwinner calls this stuff Display Engine 1.0 and 2.0. Hope there won't be a 3.0 ... Maybe you should note that in your commit message. That is justifies the name. > > Abstract the engine type to a new struct with an ops struct, which contains > functions that should be called outside the engine-specified code (in > TCON, CRTC or TV Encoder code). > > Signed-off-by: Icenowy Zheng > --- > Changes in v6: > - Rebased on wens's multi-pipeline patchset. > - Split out Makefile changes. > Changes in v5: > - Really made a sunxi_engine struct type, and moved ops pointer > into it. > - Added checked ops wrappers. > - Changed the second parameter of layers_init from crtc to engine. > 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/sun4i_backend.c | 68 ++++++++++++--------- > drivers/gpu/drm/sun4i/sun4i_backend.h | 17 +++--- > drivers/gpu/drm/sun4i/sun4i_crtc.c | 11 ++-- > drivers/gpu/drm/sun4i/sun4i_crtc.h | 4 +- > drivers/gpu/drm/sun4i/sun4i_drv.c | 2 +- > drivers/gpu/drm/sun4i/sun4i_drv.h | 2 +- > drivers/gpu/drm/sun4i/sun4i_layer.c | 8 +-- > drivers/gpu/drm/sun4i/sun4i_layer.h | 5 +- > drivers/gpu/drm/sun4i/sun4i_tcon.c | 36 ++++++----- > drivers/gpu/drm/sun4i/sun4i_tv.c | 9 ++- > drivers/gpu/drm/sun4i/sunxi_engine.h | 112 ++++++++++++++++++++++++++++++++++ > 11 files changed, 198 insertions(+), 76 deletions(-) > create mode 100644 drivers/gpu/drm/sun4i/sunxi_engine.h > > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c > index e53107418add..611cdcb9c182 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c > @@ -25,6 +25,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, > @@ -32,41 +34,38 @@ 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(struct sunxi_engine *engine) > { > int i; > > DRM_DEBUG_DRIVER("Applying RGB to YUV color correction\n"); > > /* Set color correction */ > - regmap_write(backend->regs, SUN4I_BACKEND_OCCTL_REG, > + regmap_write(engine->regs, SUN4I_BACKEND_OCCTL_REG, > SUN4I_BACKEND_OCCTL_ENABLE); > > for (i = 0; i < 12; i++) > - regmap_write(backend->regs, SUN4I_BACKEND_OCRCOEF_REG(i), > + regmap_write(engine->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(struct sunxi_engine *engine) > { > DRM_DEBUG_DRIVER("Disabling color correction\n"); > > /* Disable color correction */ > - regmap_update_bits(backend->regs, SUN4I_BACKEND_OCCTL_REG, > + regmap_update_bits(engine->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(struct sunxi_engine *engine) > { > DRM_DEBUG_DRIVER("Committing changes\n"); > > - regmap_write(backend->regs, SUN4I_BACKEND_REGBUFFCTL_REG, > + regmap_write(engine->regs, SUN4I_BACKEND_REGBUFFCTL_REG, > SUN4I_BACKEND_REGBUFFCTL_AUTOLOAD_DIS | > SUN4I_BACKEND_REGBUFFCTL_LOADCTL); > } > -EXPORT_SYMBOL(sun4i_backend_commit); > > void sun4i_backend_layer_enable(struct sun4i_backend *backend, > int layer, bool enable) > @@ -81,7 +80,7 @@ void sun4i_backend_layer_enable(struct sun4i_backend *backend, > else > val = 0; > > - regmap_update_bits(backend->regs, SUN4I_BACKEND_MODCTL_REG, > + regmap_update_bits(backend->engine.regs, SUN4I_BACKEND_MODCTL_REG, > SUN4I_BACKEND_MODCTL_LAY_EN(layer), val); > } > EXPORT_SYMBOL(sun4i_backend_layer_enable); > @@ -144,27 +143,28 @@ int sun4i_backend_update_layer_coord(struct sun4i_backend *backend, > if (plane->type == DRM_PLANE_TYPE_PRIMARY) { > DRM_DEBUG_DRIVER("Primary layer, updating global size W: %u H: %u\n", > state->crtc_w, state->crtc_h); > - regmap_write(backend->regs, SUN4I_BACKEND_DISSIZE_REG, > + regmap_write(backend->engine.regs, SUN4I_BACKEND_DISSIZE_REG, > SUN4I_BACKEND_DISSIZE(state->crtc_w, > state->crtc_h)); > } > > /* Set the line width */ > DRM_DEBUG_DRIVER("Layer line width: %d bits\n", fb->pitches[0] * 8); > - regmap_write(backend->regs, SUN4I_BACKEND_LAYLINEWIDTH_REG(layer), > + regmap_write(backend->engine.regs, > + SUN4I_BACKEND_LAYLINEWIDTH_REG(layer), > fb->pitches[0] * 8); > > /* Set height and width */ > DRM_DEBUG_DRIVER("Layer size W: %u H: %u\n", > state->crtc_w, state->crtc_h); > - regmap_write(backend->regs, SUN4I_BACKEND_LAYSIZE_REG(layer), > + regmap_write(backend->engine.regs, SUN4I_BACKEND_LAYSIZE_REG(layer), > SUN4I_BACKEND_LAYSIZE(state->crtc_w, > state->crtc_h)); > > /* Set base coordinates */ > DRM_DEBUG_DRIVER("Layer coordinates X: %d Y: %d\n", > state->crtc_x, state->crtc_y); > - regmap_write(backend->regs, SUN4I_BACKEND_LAYCOOR_REG(layer), > + regmap_write(backend->engine.regs, SUN4I_BACKEND_LAYCOOR_REG(layer), > SUN4I_BACKEND_LAYCOOR(state->crtc_x, > state->crtc_y)); > > @@ -185,7 +185,7 @@ int sun4i_backend_update_layer_formats(struct sun4i_backend *backend, > interlaced = plane->state->crtc->state->adjusted_mode.flags > & DRM_MODE_FLAG_INTERLACE; > > - regmap_update_bits(backend->regs, SUN4I_BACKEND_MODCTL_REG, > + regmap_update_bits(backend->engine.regs, SUN4I_BACKEND_MODCTL_REG, > SUN4I_BACKEND_MODCTL_ITLMOD_EN, > interlaced ? SUN4I_BACKEND_MODCTL_ITLMOD_EN : 0); > > @@ -199,7 +199,8 @@ int sun4i_backend_update_layer_formats(struct sun4i_backend *backend, > return ret; > } > > - regmap_update_bits(backend->regs, SUN4I_BACKEND_ATTCTL_REG1(layer), > + regmap_update_bits(backend->engine.regs, > + SUN4I_BACKEND_ATTCTL_REG1(layer), > SUN4I_BACKEND_ATTCTL_REG1_LAY_FBFMT, val); > > return 0; > @@ -232,13 +233,14 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend, > /* Write the 32 lower bits of the address (in bits) */ > lo_paddr = paddr << 3; > DRM_DEBUG_DRIVER("Setting address lower bits to 0x%x\n", lo_paddr); > - regmap_write(backend->regs, SUN4I_BACKEND_LAYFB_L32ADD_REG(layer), > + regmap_write(backend->engine.regs, > + SUN4I_BACKEND_LAYFB_L32ADD_REG(layer), > lo_paddr); > > /* And the upper bits */ > hi_paddr = paddr >> 29; > DRM_DEBUG_DRIVER("Setting address high bits to 0x%x\n", hi_paddr); > - regmap_update_bits(backend->regs, SUN4I_BACKEND_LAYFB_H4ADD_REG, > + regmap_update_bits(backend->engine.regs, SUN4I_BACKEND_LAYFB_H4ADD_REG, > SUN4I_BACKEND_LAYFB_H4ADD_MSK(layer), > SUN4I_BACKEND_LAYFB_H4ADD(layer, hi_paddr)); > > @@ -330,6 +332,13 @@ static int sun4i_backend_of_get_id(struct device_node *node) > return ret; > } > > +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, > +}; > + > static struct regmap_config sun4i_backend_regmap_config = { > .reg_bits = 32, > .val_bits = 32, > @@ -353,7 +362,8 @@ static int sun4i_backend_bind(struct device *dev, struct device *master, > return -ENOMEM; > dev_set_drvdata(dev, backend); > > - backend->node = dev->of_node; > + backend->engine.node = dev->of_node; > + backend->engine.ops = &sun4i_backend_engine_ops; > backend->id = sun4i_backend_of_get_id(dev->of_node); > if (backend->id < 0) > return backend->id; > @@ -363,11 +373,11 @@ static int sun4i_backend_bind(struct device *dev, struct device *master, > if (IS_ERR(regs)) > return PTR_ERR(regs); > > - backend->regs = devm_regmap_init_mmio(dev, regs, > - &sun4i_backend_regmap_config); > - if (IS_ERR(backend->regs)) { > + backend->engine.regs = devm_regmap_init_mmio(dev, regs, > + &sun4i_backend_regmap_config); > + if (IS_ERR(backend->engine.regs)) { > dev_err(dev, "Couldn't create the backend regmap\n"); > - return PTR_ERR(backend->regs); > + return PTR_ERR(backend->engine.regs); > } > > backend->reset = devm_reset_control_get(dev, NULL); > @@ -415,18 +425,18 @@ static int sun4i_backend_bind(struct device *dev, struct device *master, > } > } > > - list_add_tail(&backend->list, &drv->backend_list); > + list_add_tail(&backend->engine.list, &drv->engine_list); > > /* Reset the registers */ > for (i = 0x800; i < 0x1000; i += 4) > - regmap_write(backend->regs, i, 0); > + regmap_write(backend->engine.regs, i, 0); > > /* Disable registers autoloading */ > - regmap_write(backend->regs, SUN4I_BACKEND_REGBUFFCTL_REG, > + regmap_write(backend->engine.regs, SUN4I_BACKEND_REGBUFFCTL_REG, > SUN4I_BACKEND_REGBUFFCTL_AUTOLOAD_DIS); > > /* Enable the backend */ > - regmap_write(backend->regs, SUN4I_BACKEND_MODCTL_REG, > + regmap_write(backend->engine.regs, SUN4I_BACKEND_MODCTL_REG, > SUN4I_BACKEND_MODCTL_DEBE_EN | > SUN4I_BACKEND_MODCTL_START_CTL); > > @@ -448,7 +458,7 @@ static void sun4i_backend_unbind(struct device *dev, struct device *master, > { > struct sun4i_backend *backend = dev_get_drvdata(dev); > > - list_del(&backend->list); > + list_del(&backend->engine.list); > > if (of_device_is_compatible(dev->of_node, > "allwinner,sun8i-a33-display-backend")) > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.h b/drivers/gpu/drm/sun4i/sun4i_backend.h > index 6327a2985fe6..b022a37e8e5b 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_backend.h > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.h > @@ -19,6 +19,8 @@ > #include > #include > > +#include "sunxi_engine.h" > + > #define SUN4I_BACKEND_MODCTL_REG 0x800 > #define SUN4I_BACKEND_MODCTL_LINE_SEL BIT(29) > #define SUN4I_BACKEND_MODCTL_ITLMOD_EN BIT(28) > @@ -141,8 +143,7 @@ > #define SUN4I_BACKEND_PIPE_OFF(p) (0x5000 + (0x400 * (p))) > > struct sun4i_backend { > - struct device_node *node; > - struct regmap *regs; > + struct sunxi_engine engine; > > struct reset_control *reset; > > @@ -154,15 +155,13 @@ struct sun4i_backend { > struct reset_control *sat_reset; > > int id; > - > - /* Backend list management */ > - struct list_head list; > }; > > -void sun4i_backend_apply_color_correction(struct sun4i_backend *backend); > -void sun4i_backend_disable_color_correction(struct sun4i_backend *backend); > - > -void sun4i_backend_commit(struct sun4i_backend *backend); > +static inline struct sun4i_backend * > +engine_to_sun4i_backend(struct sunxi_engine *engine) > +{ > + return container_of(engine, struct sun4i_backend, engine); > +} > > void sun4i_backend_layer_enable(struct sun4i_backend *backend, > int layer, bool enable); > diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c > index 708b3543d4e9..f8c70439d1e2 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c > +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c > @@ -25,10 +25,9 @@ > > #include