[12/27] drm/sun4i: Add basic support for DE3

Message ID	20180902072643.4917-13-jernej.skrabec@siol.net (mailing list archive)
State	New, archived
Headers	show Return-Path: <dri-devel-bounces@lists.freedesktop.org> sender: 031275009) by mail.siol.net (Postfix) with ESMTPSA id E4313520BEF; Sun, 2 Sep 2018 09:27:26 +0200 (CEST) From: Jernej Skrabec <jernej.skrabec@siol.net> To: robh+dt@kernel.org, maxime.ripard@bootlin.com, wens@csie.org Subject: [PATCH 12/27] drm/sun4i: Add basic support for DE3 Date: Sun, 2 Sep 2018 09:26:28 +0200 Message-Id: <20180902072643.4917-13-jernej.skrabec@siol.net> In-Reply-To: <20180902072643.4917-1-jernej.skrabec@siol.net> References: <20180902072643.4917-1-jernej.skrabec@siol.net> Precedence: list Cc: mark.rutland@arm.com, airlied@linux.ie, devicetree@vger.kernel.org, sboyd@kernel.org, mturquette@baylibre.com, jernej.skrabec@siol.net, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-sunxi@googlegroups.com, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" <dri-devel-bounces@lists.freedesktop.org>
Series	Allwinner H6 DE3 and HDMI support \| expand [00/27] Allwinner H6 DE3 and HDMI support [01/27] dt-bindings: sunxi-sram: add binding for Allwinner H6 SRAM C [02/27] arm64: allwinner: h6: add system controller device tree node [03/27] dt-bindings: bus: add H6 DE3 bus binding [04/27] clk: sunxi-ng: Adjust MP clock parent rate when allowed [05/27] clk: sunxi-ng: Use u64 for calculation of NM rate [06/27] clk: sunxi-ng: h6: Set video PLLs limits [07/27] dt-bindings: clock: sun8i-de2: Add H6 DE3 clock description [08/27] clk: sunxi-ng: Add support for H6 DE3 clocks [09/27] dt-bindings: display: sun4i-drm: Add H6 display engine compatibles [10/27] drm/sun4i: Add compatible for H6 display engine [11/27] drm/sun4i: Rework DE2 register defines [12/27] drm/sun4i: Add basic support for DE3 [13/27] drm/sun4i: Add support for H6 DE3 mixer 0 [14/27] drm/bridge/synopsys: dw-hdmi: Enable workaround for v2.12a [15/27] drm/sun4i: Not all DW HDMI controllers has scrambled addresses [16/27] drm/sun4i: dw-hdmi: Make mode_valid function configurable [17/27] drm/sun4i: dw-hdmi: Add quirk for setting TMDS clock [18/27] dt-bindings: display: sunxi: add DT binding for Allwinner H6 DW HDMI [19/27] drm/sun4i: Add support for H6 DW HDMI controller [20/27] drm/sun4i: Add support for Synopsys HDMI PHY [21/27] drm/sun4i: Add support for H6 HDMI PHY [22/27] drm/sun4i: Initialize registers in tcon-top driver [23/27] drm: sun4i: add quirks for TCON TOP [24/27] dt-bindings: display: sun4i-drm: document H6 TCON TOP [25/27] drm: sun4i: add support for H6 TCON TOP [26/27] arm64: dts: sun50i: h6: Add HDMI pipeline [27/27] arm64: dts: sun50i: h6: Enable HDMI output on Pine H64 board

Jernej Škrabec Sept. 2, 2018, 7:26 a.m. UTC

Display Engine 3 is an upgrade of DE2 with new features like support for
10 bit color formats and support for AFBC.

Most of DE2 code works with DE3, except some small details.

Add support for it.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/sun4i/sun8i_csc.c       | 96 +++++++++++++++++++++++--
 drivers/gpu/drm/sun4i/sun8i_mixer.c     | 17 ++++-
 drivers/gpu/drm/sun4i/sun8i_mixer.h     | 21 +++++-
 drivers/gpu/drm/sun4i/sun8i_ui_scaler.c |  8 ++-
 drivers/gpu/drm/sun4i/sun8i_ui_scaler.h |  1 +
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c  |  8 +++
 drivers/gpu/drm/sun4i/sun8i_vi_layer.h  |  2 +
 drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 34 ++++++++-
 drivers/gpu/drm/sun4i/sun8i_vi_scaler.h | 23 ++++++
 9 files changed, 197 insertions(+), 13 deletions(-)

Chen-Yu Tsai Sept. 22, 2018, 1:19 p.m. UTC | #1

On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> wrote:
>
> Display Engine 3 is an upgrade of DE2 with new features like support for
> 10 bit color formats and support for AFBC.
>
> Most of DE2 code works with DE3, except some small details.
>
> Add support for it.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/gpu/drm/sun4i/sun8i_csc.c       | 96 +++++++++++++++++++++++--
>  drivers/gpu/drm/sun4i/sun8i_mixer.c     | 17 ++++-
>  drivers/gpu/drm/sun4i/sun8i_mixer.h     | 21 +++++-
>  drivers/gpu/drm/sun4i/sun8i_ui_scaler.c |  8 ++-
>  drivers/gpu/drm/sun4i/sun8i_ui_scaler.h |  1 +
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.c  |  8 +++
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.h  |  2 +
>  drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 34 ++++++++-
>  drivers/gpu/drm/sun4i/sun8i_vi_scaler.h | 23 ++++++
>  9 files changed, 197 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_csc.c b/drivers/gpu/drm/sun4i/sun8i_csc.c
> index b14925b40ccf..101901ccf2dc 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_csc.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_csc.c
> @@ -34,6 +34,34 @@ static const u32 yvu2rgb[] = {
>         0x000004A8, 0x00000000, 0x00000813, 0xFFFBAC4A,
>  };
>
> +/*
> + * DE3 has a bit different CSC units. Factors are in two's complement format.
> + * First three have 17 bits for fractinal part and last two 2 bits. First
> + * three values in each line are multiplication factor, 4th is difference,
> + * which is subtracted from the input value before the multiplication and
> + * last value is constant, which is added at the end.
> + *
> + * x' = c00 * (x + d0) + c01 * (y + d1) + c02 * (z + d2) + const0
> + * y' = c10 * (x + d0) + c11 * (y + d1) + c12 * (z + d2) + const1
> + * z' = c20 * (x + d0) + c21 * (y + d1) + c22 * (z + d2) + const2
> + *
> + * Please note that above formula is true only for Blender CSC. Other DE3 CSC
> + * units takes only positive value for difference. From what can be deducted
> + * from BSP driver code, those units probably automatically assume that
> + * difference has to be subtracted.
> + */
> +static const u32 yuv2rgb_de3[] = {
> +       0x0002542a, 0x00000000, 0x0003312a, 0xffffffc0, 0x00000000,
> +       0x0002542a, 0xffff376b, 0xfffe5fc3, 0xfffffe00, 0x00000000,
> +       0x0002542a, 0x000408d3, 0x00000000, 0xfffffe00, 0x00000000,
> +};
> +
> +static const u32 yvu2rgb_de3[] = {
> +       0x0002542a, 0x0003312a, 0x00000000, 0xffffffc0, 0x00000000,
> +       0x0002542a, 0xfffe5fc3, 0xffff376b, 0xfffffe00, 0x00000000,
> +       0x0002542a, 0x00000000, 0x000408d3, 0xfffffe00, 0x00000000,
> +};
> +
>  static void sun8i_csc_set_coefficients(struct regmap *map, u32 base,
>                                        enum sun8i_csc_mode mode)
>  {
> @@ -61,6 +89,38 @@ static void sun8i_csc_set_coefficients(struct regmap *map, u32 base,
>         }
>  }
>
> +static void sun8i_de3_ccsc_set_coefficients(struct regmap *map, int layer,
> +                                           enum sun8i_csc_mode mode)
> +{
> +       const u32 *table;
> +       int i, j;
> +
> +       switch (mode) {
> +       case SUN8I_CSC_MODE_YUV2RGB:
> +               table = yuv2rgb_de3;
> +               break;
> +       case SUN8I_CSC_MODE_YVU2RGB:
> +               table = yvu2rgb_de3;
> +               break;
> +       default:
> +               DRM_WARN("Wrong CSC mode specified.\n");
> +               return;
> +       }
> +
> +       for (i = 0; i < 3; i++) {
> +               for (j = 0; j < 3; j++)
> +                       regmap_write(map,
> +                                    SUN8I_MIXER_BLEND_CSC_COEFF(DE3_BLD_BASE,
> +                                                                layer, i, j),
> +                                    table[i * 5 + j]);

Given that the first three values occupy contiguous addresses,
you can use regmap_bulk_write() here.

> +               regmap_write(map,
> +                            SUN8I_MIXER_BLEND_CSC_CONST(DE3_BLD_BASE,
> +                                                        layer, i),
> +                            SUN8I_MIXER_BLEND_CSC_CONST_VAL(table[i * 5 + 3],
> +                                                            table[i * 5 + 4]));

Nit: Using a two-dimension array might make it easier to read.

> +       }
> +}
> +
>  static void sun8i_csc_enable(struct regmap *map, u32 base, bool enable)
>  {
>         u32 val;
> @@ -73,21 +133,45 @@ static void sun8i_csc_enable(struct regmap *map, u32 base, bool enable)
>         regmap_update_bits(map, SUN8I_CSC_CTRL(base), SUN8I_CSC_CTRL_EN, val);
>  }
>
> +static void sun8i_de3_ccsc_enable(struct regmap *map, int layer, bool enable)
> +{
> +       u32 val, mask;
> +
> +       mask = SUN8I_MIXER_BLEND_CSC_CTL_EN(layer);
> +
> +       if (enable)
> +               val = mask;
> +       else
> +               val = 0;
> +
> +       regmap_update_bits(map, SUN8I_MIXER_BLEND_CSC_CTL(DE3_BLD_BASE),
> +                          mask, val);
> +}
> +
>  void sun8i_csc_set_ccsc_coefficients(struct sun8i_mixer *mixer, int layer,
>                                      enum sun8i_csc_mode mode)
>  {
> -       u32 base;
> +       if (!mixer->cfg->is_de3) {
> +               u32 base;

You could rewrite this as

    if (mixer->cfg->is_de3) {
               sun8i_de3_ccsc_set_coefficients(mixer->engine.regs,
                                               layer, mode);
               return;
    }

That way you don't need to change the indentation on the existing lines.
I suppose this is more of a personal preference. The downside is the control
flow is slightly more complicated.

> -       base = ccsc_base[mixer->cfg->ccsc][layer];
> +               base = ccsc_base[mixer->cfg->ccsc][layer];
>
> -       sun8i_csc_set_coefficients(mixer->engine.regs, base, mode);
> +               sun8i_csc_set_coefficients(mixer->engine.regs, base, mode);
> +       } else {
> +               sun8i_de3_ccsc_set_coefficients(mixer->engine.regs,
> +                                               layer, mode);
> +       }
>  }
>
>  void sun8i_csc_enable_ccsc(struct sun8i_mixer *mixer, int layer, bool enable)
>  {
> -       u32 base;
> +       if (!mixer->cfg->is_de3) {
> +               u32 base;
>
> -       base = ccsc_base[mixer->cfg->ccsc][layer];
> +               base = ccsc_base[mixer->cfg->ccsc][layer];
>
> -       sun8i_csc_enable(mixer->engine.regs, base, enable);
> +               sun8i_csc_enable(mixer->engine.regs, base, enable);
> +       } else {
> +               sun8i_de3_ccsc_enable(mixer->engine.regs, layer, enable);
> +       }
>  }
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> index 743941a33d88..a9218abf0935 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -460,8 +460,21 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
>         base = sun8i_blender_base(mixer);
>
>         /* Reset the registers */
> -       for (i = 0x0; i < 0x20000; i += 4)
> -               regmap_write(mixer->engine.regs, i, 0);
> +       if (mixer->cfg->is_de3) {
> +               for (i = 0x0; i < 0x3000; i += 4)
> +                       regmap_write(mixer->engine.regs, i, 0);
> +               for (i = 0x20000; i < 0x40000; i += 4)
> +                       regmap_write(mixer->engine.regs, i, 0);
> +               for (i = 0x70000; i < 0x88000; i += 4)
> +                       regmap_write(mixer->engine.regs, i, 0);
> +               for (i = 0xa0000; i < 0xb0000; i += 4)
> +                       regmap_write(mixer->engine.regs, i, 0);
> +               for (i = 0xd0000; i < 0xe0000; i += 4)
> +                       regmap_write(mixer->engine.regs, i, 0);

Could you use the macros *_BASE and *_SIZE here? That would make
it more obviously what parts you're clearing.

The last offset, 0xd0000, isn't even defined in the DE3 spec.

> +       } else {
> +               for (i = 0x0; i < 0x20000; i += 4)
> +                       regmap_write(mixer->engine.regs, i, 0);
> +       }
>
>         /* Enable the mixer */
>         regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_CTL,
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> index 020b0a097c84..4c9a442bbb44 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> @@ -33,6 +33,10 @@
>  #define DE2_CH_BASE                            0x2000
>  #define DE2_CH_SIZE                            0x1000
>
> +#define DE3_BLD_BASE                           0x0800
> +#define DE3_CH_BASE                            0x1000
> +#define DE3_CH_SIZE                            0x0800
> +
>  #define SUN8I_MIXER_BLEND_PIPE_CTL(base)       ((base) + 0)
>  #define SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, x) ((base) + 0x4 + 0x10 * (x))
>  #define SUN8I_MIXER_BLEND_ATTR_INSIZE(base, x) ((base) + 0x8 + 0x10 * (x))
> @@ -47,6 +51,11 @@
>  #define SUN8I_MIXER_BLEND_CK_MAX(base, x)      ((base) + 0xc0 + 0x04 * (x))
>  #define SUN8I_MIXER_BLEND_CK_MIN(base, x)      ((base) + 0xe0 + 0x04 * (x))
>  #define SUN8I_MIXER_BLEND_OUTCTL(base)         ((base) + 0xfc)
> +#define SUN8I_MIXER_BLEND_CSC_CTL(base)                ((base) + 0x100)
> +#define SUN8I_MIXER_BLEND_CSC_COEFF(base, layer, x, y) \
> +       ((base) + 0x110 + (layer) * 0x30 +  (x) * 0x10 + 4 * (y))
> +#define SUN8I_MIXER_BLEND_CSC_CONST(base, layer, i) \
> +       ((base) + 0x110 + (layer) * 0x30 +  (i) * 0x10 + 0x0c)

Should these have a DE3 prefix?

>
>  #define SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK      GENMASK(12, 8)
>  #define SUN8I_MIXER_BLEND_PIPE_CTL_EN(pipe)    BIT(8 + pipe)
> @@ -61,6 +70,9 @@
>
>  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED    BIT(1)
>
> +#define SUN8I_MIXER_BLEND_CSC_CTL_EN(ch)       BIT(ch)
> +#define SUN8I_MIXER_BLEND_CSC_CONST_VAL(d, c)  (((d) << 16) | ((c) & 0xffff))
> +

Same here.

>  #define SUN8I_MIXER_FBFMT_ARGB8888     0
>  #define SUN8I_MIXER_FBFMT_ABGR8888     1
>  #define SUN8I_MIXER_FBFMT_RGBA8888     2

[...]

> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> index 46f0237c17bb..3d0ad64178ea 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> @@ -30,6 +30,8 @@
>  #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_RGB_MODE                BIT(15)
>  #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_OFFSET    8
>  #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_MASK      GENMASK(12, 8)
> +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK      GENMASK(31, 24)
> +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x)                ((x) << 24)

DE3 prefix?

>
>  struct sun8i_mixer;
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c
> index 8697afc36023..22e1ed5cd7a4 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c
> @@ -835,7 +835,10 @@ static const u32 bicubic4coefftab32[480] = {
>
>  static inline u32 sun8i_vi_scaler_base(struct sun8i_mixer *mixer, int channel)
>  {
> -       return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel;
> +       if (mixer->cfg->is_de3)
> +               return DE3_VI_SCALER_BASE + DE3_VI_SCALER_SIZE * channel;
> +       else
> +               return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel;
>  }
>
>  static int sun8i_vi_scaler_coef_index(unsigned int step)
> @@ -951,6 +954,35 @@ void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer, int layer,
>                 cvphase = vphase;
>         }
>
> +       if (mixer->cfg->is_de3) {
> +               u32 val;
> +
> +               if (format->hsub == 1 && format->vsub == 1)
> +                       val = SUN8I_SCALER_VSU_SCALE_MODE_UI;
> +               else
> +                       val = SUN8I_SCALER_VSU_SCALE_MODE_NORMAL;
> +
> +               regmap_write(mixer->engine.regs,
> +                            SUN8I_SCALER_VSU_SCALE_MODE(base), val);

The remaining settings seem to be related to the edge detection scaling
method. Since you aren't supporting it, are they necessary?

> +               regmap_write(mixer->engine.regs,
> +                            SUN8I_SCALER_VSU_DIR_THR(base),
> +                            SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(0) |
> +                            SUN8I_SCALER_VSU_ZERO_DIR_THR(0) |
> +                            SUN8I_SCALER_VSU_HORZ_DIR_THR(0xff) |
> +                            SUN8I_SCALER_VSU_VERT_DIR_THR(1));
> +               regmap_write(mixer->engine.regs,
> +                            SUN8I_SCALER_VSU_EDGE_THR(base),
> +                            SUN8I_SCALER_VSU_EDGE_SHIFT(8) |
> +                            SUN8I_SCALER_VSU_EDGE_OFFSET(0));
> +               regmap_write(mixer->engine.regs,
> +                            SUN8I_SCALER_VSU_EDSCL_CTRL(base), 0);
> +               regmap_write(mixer->engine.regs,
> +                            SUN8I_SCALER_VSU_ANGLE_THR(base),
> +                            SUN8I_SCALER_VSU_ANGLE_SHIFT(2) |
> +                            SUN8I_SCALER_VSU_ANGLE_OFFSET(0));
> +       }
> +
>         regmap_write(mixer->engine.regs,
>                      SUN8I_SCALER_VSU_OUTSIZE(base), outsize);
>         regmap_write(mixer->engine.regs,
> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h
> index cd015405f66d..c322f5652481 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h
> @@ -15,6 +15,9 @@
>  #define DE2_VI_SCALER_BASE 0x20000
>  #define DE2_VI_SCALER_SIZE 0x20000
>
> +#define DE3_VI_SCALER_BASE 0x20000
> +#define DE3_VI_SCALER_SIZE 0x08000
> +
>  /* this two macros assumes 16 fractional bits which is standard in DRM */
>  #define SUN8I_VI_SCALER_SCALE_MIN              1
>  #define SUN8I_VI_SCALER_SCALE_MAX              ((1UL << 20) - 1)
> @@ -25,6 +28,11 @@
>  #define SUN8I_VI_SCALER_SIZE(w, h)             (((h) - 1) << 16 | ((w) - 1))
>
>  #define SUN8I_SCALER_VSU_CTRL(base)            ((base) + 0x0)
> +#define SUN8I_SCALER_VSU_SCALE_MODE(base)      ((base) + 0x10)
> +#define SUN8I_SCALER_VSU_DIR_THR(base)         ((base) + 0x20)
> +#define SUN8I_SCALER_VSU_EDGE_THR(base)                ((base) + 0x24)
> +#define SUN8I_SCALER_VSU_EDSCL_CTRL(base)      ((base) + 0x28)
> +#define SUN8I_SCALER_VSU_ANGLE_THR(base)       ((base) + 0x2c)

DE3 prefix for the new ones please.

>  #define SUN8I_SCALER_VSU_OUTSIZE(base)         ((base) + 0x40)
>  #define SUN8I_SCALER_VSU_YINSIZE(base)         ((base) + 0x80)
>  #define SUN8I_SCALER_VSU_YHSTEP(base)          ((base) + 0x88)
> @@ -46,6 +54,21 @@
>  #define SUN8I_SCALER_VSU_CTRL_EN               BIT(0)
>  #define SUN8I_SCALER_VSU_CTRL_COEFF_RDY                BIT(4)
>
> +#define SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(x)   (((x) << 24) & 0xFF)
> +#define SUN8I_SCALER_VSU_ZERO_DIR_THR(x)       (((x) << 16) & 0xFF)
> +#define SUN8I_SCALER_VSU_HORZ_DIR_THR(x)       (((x) << 8) & 0xFF)
> +#define SUN8I_SCALER_VSU_VERT_DIR_THR(x)       ((x) & 0xFF)
> +
> +#define SUN8I_SCALER_VSU_SCALE_MODE_UI         0
> +#define SUN8I_SCALER_VSU_SCALE_MODE_NORMAL     1
> +#define SUN8I_SCALER_VSU_SCALE_MODE_ED_SCALE   2
> +
> +#define SUN8I_SCALER_VSU_EDGE_SHIFT(x)         (((x) << 16) & 0xF)
> +#define SUN8I_SCALER_VSU_EDGE_OFFSET(x)                ((x) & 0xFF)
> +
> +#define SUN8I_SCALER_VSU_ANGLE_SHIFT(x)                (((x) << 16) & 0xF)
> +#define SUN8I_SCALER_VSU_ANGLE_OFFSET(x)       ((x) & 0xFF)
> +

Same here.

Regards
ChenYu

>  void sun8i_vi_scaler_enable(struct sun8i_mixer *mixer, int layer, bool enable);
>  void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer, int layer,
>                            u32 src_w, u32 src_h, u32 dst_w, u32 dst_h,
> --
> 2.18.0
>

Jernej Škrabec Sept. 23, 2018, 7:51 p.m. UTC | #2

Dne sobota, 22. september 2018 ob 15:19:12 CEST je Chen-Yu Tsai napisal(a):
> On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> 
wrote:
> > Display Engine 3 is an upgrade of DE2 with new features like support for
> > 10 bit color formats and support for AFBC.
> > 
> > Most of DE2 code works with DE3, except some small details.
> > 
> > Add support for it.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > 
> >  drivers/gpu/drm/sun4i/sun8i_csc.c       | 96 +++++++++++++++++++++++--
> >  drivers/gpu/drm/sun4i/sun8i_mixer.c     | 17 ++++-
> >  drivers/gpu/drm/sun4i/sun8i_mixer.h     | 21 +++++-
> >  drivers/gpu/drm/sun4i/sun8i_ui_scaler.c |  8 ++-
> >  drivers/gpu/drm/sun4i/sun8i_ui_scaler.h |  1 +
> >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c  |  8 +++
> >  drivers/gpu/drm/sun4i/sun8i_vi_layer.h  |  2 +
> >  drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 34 ++++++++-
> >  drivers/gpu/drm/sun4i/sun8i_vi_scaler.h | 23 ++++++
> >  9 files changed, 197 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_csc.c
> > b/drivers/gpu/drm/sun4i/sun8i_csc.c index b14925b40ccf..101901ccf2dc
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_csc.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_csc.c
> > @@ -34,6 +34,34 @@ static const u32 yvu2rgb[] = {
> > 
> >         0x000004A8, 0x00000000, 0x00000813, 0xFFFBAC4A,
> >  
> >  };
> > 
> > +/*
> > + * DE3 has a bit different CSC units. Factors are in two's complement
> > format. + * First three have 17 bits for fractinal part and last two 2
> > bits. First + * three values in each line are multiplication factor, 4th
> > is difference, + * which is subtracted from the input value before the
> > multiplication and + * last value is constant, which is added at the end.
> > + *
> > + * x' = c00 * (x + d0) + c01 * (y + d1) + c02 * (z + d2) + const0
> > + * y' = c10 * (x + d0) + c11 * (y + d1) + c12 * (z + d2) + const1
> > + * z' = c20 * (x + d0) + c21 * (y + d1) + c22 * (z + d2) + const2
> > + *
> > + * Please note that above formula is true only for Blender CSC. Other DE3
> > CSC + * units takes only positive value for difference. From what can be
> > deducted + * from BSP driver code, those units probably automatically
> > assume that + * difference has to be subtracted.
> > + */
> > +static const u32 yuv2rgb_de3[] = {
> > +       0x0002542a, 0x00000000, 0x0003312a, 0xffffffc0, 0x00000000,
> > +       0x0002542a, 0xffff376b, 0xfffe5fc3, 0xfffffe00, 0x00000000,
> > +       0x0002542a, 0x000408d3, 0x00000000, 0xfffffe00, 0x00000000,
> > +};
> > +
> > +static const u32 yvu2rgb_de3[] = {
> > +       0x0002542a, 0x0003312a, 0x00000000, 0xffffffc0, 0x00000000,
> > +       0x0002542a, 0xfffe5fc3, 0xffff376b, 0xfffffe00, 0x00000000,
> > +       0x0002542a, 0x00000000, 0x000408d3, 0xfffffe00, 0x00000000,
> > +};
> > +
> > 
> >  static void sun8i_csc_set_coefficients(struct regmap *map, u32 base,
> >  
> >                                        enum sun8i_csc_mode mode)
> >  
> >  {
> > 
> > @@ -61,6 +89,38 @@ static void sun8i_csc_set_coefficients(struct regmap
> > *map, u32 base,> 
> >         }
> >  
> >  }
> > 
> > +static void sun8i_de3_ccsc_set_coefficients(struct regmap *map, int
> > layer,
> > +                                           enum sun8i_csc_mode mode)
> > +{
> > +       const u32 *table;
> > +       int i, j;
> > +
> > +       switch (mode) {
> > +       case SUN8I_CSC_MODE_YUV2RGB:
> > +               table = yuv2rgb_de3;
> > +               break;
> > +       case SUN8I_CSC_MODE_YVU2RGB:
> > +               table = yvu2rgb_de3;
> > +               break;
> > +       default:
> > +               DRM_WARN("Wrong CSC mode specified.\n");
> > +               return;
> > +       }
> > +
> > +       for (i = 0; i < 3; i++) {
> > +               for (j = 0; j < 3; j++)
> > +                       regmap_write(map,
> > +                                   
> > SUN8I_MIXER_BLEND_CSC_COEFF(DE3_BLD_BASE, +                              
> >                                  layer, i, j), +                         
> >           table[i * 5 + j]);
> 
> Given that the first three values occupy contiguous addresses,
> you can use regmap_bulk_write() here.
> 
> > +               regmap_write(map,
> > +                            SUN8I_MIXER_BLEND_CSC_CONST(DE3_BLD_BASE,
> > +                                                        layer, i),
> > +                            SUN8I_MIXER_BLEND_CSC_CONST_VAL(table[i * 5 +
> > 3], +                                                            table[i
> > * 5 + 4]));
> Nit: Using a two-dimension array might make it easier to read.
> 
> > +       }
> > +}
> > +
> > 
> >  static void sun8i_csc_enable(struct regmap *map, u32 base, bool enable)
> >  {
> >  
> >         u32 val;
> > 
> > @@ -73,21 +133,45 @@ static void sun8i_csc_enable(struct regmap *map, u32
> > base, bool enable)> 
> >         regmap_update_bits(map, SUN8I_CSC_CTRL(base), SUN8I_CSC_CTRL_EN,
> >         val);
> >  
> >  }
> > 
> > +static void sun8i_de3_ccsc_enable(struct regmap *map, int layer, bool
> > enable) +{
> > +       u32 val, mask;
> > +
> > +       mask = SUN8I_MIXER_BLEND_CSC_CTL_EN(layer);
> > +
> > +       if (enable)
> > +               val = mask;
> > +       else
> > +               val = 0;
> > +
> > +       regmap_update_bits(map, SUN8I_MIXER_BLEND_CSC_CTL(DE3_BLD_BASE),
> > +                          mask, val);
> > +}
> > +
> > 
> >  void sun8i_csc_set_ccsc_coefficients(struct sun8i_mixer *mixer, int
> >  layer,
> >  
> >                                      enum sun8i_csc_mode mode)
> >  
> >  {
> > 
> > -       u32 base;
> > +       if (!mixer->cfg->is_de3) {
> > +               u32 base;
> 
> You could rewrite this as
> 
>     if (mixer->cfg->is_de3) {
>                sun8i_de3_ccsc_set_coefficients(mixer->engine.regs,
>                                                layer, mode);
>                return;
>     }
> 
> That way you don't need to change the indentation on the existing lines.
> I suppose this is more of a personal preference. The downside is the control
> flow is slightly more complicated.
> 
> > -       base = ccsc_base[mixer->cfg->ccsc][layer];
> > +               base = ccsc_base[mixer->cfg->ccsc][layer];
> > 
> > -       sun8i_csc_set_coefficients(mixer->engine.regs, base, mode);
> > +               sun8i_csc_set_coefficients(mixer->engine.regs, base,
> > mode);
> > +       } else {
> > +               sun8i_de3_ccsc_set_coefficients(mixer->engine.regs,
> > +                                               layer, mode);
> > +       }
> > 
> >  }
> >  
> >  void sun8i_csc_enable_ccsc(struct sun8i_mixer *mixer, int layer, bool
> >  enable) {
> > 
> > -       u32 base;
> > +       if (!mixer->cfg->is_de3) {
> > +               u32 base;
> > 
> > -       base = ccsc_base[mixer->cfg->ccsc][layer];
> > +               base = ccsc_base[mixer->cfg->ccsc][layer];
> > 
> > -       sun8i_csc_enable(mixer->engine.regs, base, enable);
> > +               sun8i_csc_enable(mixer->engine.regs, base, enable);
> > +       } else {
> > +               sun8i_de3_ccsc_enable(mixer->engine.regs, layer, enable);
> > +       }
> > 
> >  }
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index 743941a33d88..a9218abf0935
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > @@ -460,8 +460,21 @@ static int sun8i_mixer_bind(struct device *dev,
> > struct device *master,> 
> >         base = sun8i_blender_base(mixer);
> >         
> >         /* Reset the registers */
> > 
> > -       for (i = 0x0; i < 0x20000; i += 4)
> > -               regmap_write(mixer->engine.regs, i, 0);
> > +       if (mixer->cfg->is_de3) {
> > +               for (i = 0x0; i < 0x3000; i += 4)
> > +                       regmap_write(mixer->engine.regs, i, 0);
> > +               for (i = 0x20000; i < 0x40000; i += 4)
> > +                       regmap_write(mixer->engine.regs, i, 0);
> > +               for (i = 0x70000; i < 0x88000; i += 4)
> > +                       regmap_write(mixer->engine.regs, i, 0);
> > +               for (i = 0xa0000; i < 0xb0000; i += 4)
> > +                       regmap_write(mixer->engine.regs, i, 0);
> > +               for (i = 0xd0000; i < 0xe0000; i += 4)
> > +                       regmap_write(mixer->engine.regs, i, 0);
> 
> Could you use the macros *_BASE and *_SIZE here? That would make
> it more obviously what parts you're clearing.

Ok, but where should I define them? In sun8i-mixer.h?

> 
> The last offset, 0xd0000, isn't even defined in the DE3 spec.

It is, check chapter 2.3.2, table 2-3.

> 
> > +       } else {
> > +               for (i = 0x0; i < 0x20000; i += 4)
> > +                       regmap_write(mixer->engine.regs, i, 0);
> > +       }
> > 
> >         /* Enable the mixer */
> >         regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_CTL,
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 020b0a097c84..4c9a442bbb44
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > @@ -33,6 +33,10 @@
> > 
> >  #define DE2_CH_BASE                            0x2000
> >  #define DE2_CH_SIZE                            0x1000
> > 
> > +#define DE3_BLD_BASE                           0x0800
> > +#define DE3_CH_BASE                            0x1000
> > +#define DE3_CH_SIZE                            0x0800
> > +
> > 
> >  #define SUN8I_MIXER_BLEND_PIPE_CTL(base)       ((base) + 0)
> >  #define SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, x) ((base) + 0x4 + 0x10 *
> >  (x))
> >  #define SUN8I_MIXER_BLEND_ATTR_INSIZE(base, x) ((base) + 0x8 + 0x10 *
> >  (x))
> > 
> > @@ -47,6 +51,11 @@
> > 
> >  #define SUN8I_MIXER_BLEND_CK_MAX(base, x)      ((base) + 0xc0 + 0x04 *
> >  (x))
> >  #define SUN8I_MIXER_BLEND_CK_MIN(base, x)      ((base) + 0xe0 + 0x04 *
> >  (x))
> >  #define SUN8I_MIXER_BLEND_OUTCTL(base)         ((base) + 0xfc)
> > 
> > +#define SUN8I_MIXER_BLEND_CSC_CTL(base)                ((base) + 0x100)
> > +#define SUN8I_MIXER_BLEND_CSC_COEFF(base, layer, x, y) \
> > +       ((base) + 0x110 + (layer) * 0x30 +  (x) * 0x10 + 4 * (y))
> > +#define SUN8I_MIXER_BLEND_CSC_CONST(base, layer, i) \
> > +       ((base) + 0x110 + (layer) * 0x30 +  (i) * 0x10 + 0x0c)
> 
> Should these have a DE3 prefix?

Ok for all prefix comments.

> 
> >  #define SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK      GENMASK(12, 8)
> >  #define SUN8I_MIXER_BLEND_PIPE_CTL_EN(pipe)    BIT(8 + pipe)
> > 
> > @@ -61,6 +70,9 @@
> > 
> >  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED    BIT(1)
> > 
> > +#define SUN8I_MIXER_BLEND_CSC_CTL_EN(ch)       BIT(ch)
> > +#define SUN8I_MIXER_BLEND_CSC_CONST_VAL(d, c)  (((d) << 16) | ((c) &
> > 0xffff)) +
> 
> Same here.
> 
> >  #define SUN8I_MIXER_FBFMT_ARGB8888     0
> >  #define SUN8I_MIXER_FBFMT_ABGR8888     1
> >  #define SUN8I_MIXER_FBFMT_RGBA8888     2
> 
> [...]
> 
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 46f0237c17bb..3d0ad64178ea
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > @@ -30,6 +30,8 @@
> > 
> >  #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_RGB_MODE                BIT(15)
> >  #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_OFFSET    8
> >  #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_MASK      GENMASK(12, 8)
> > 
> > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK      GENMASK(31, 24)
> > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x)                ((x) <<
> > 24)
> 
> DE3 prefix?
> 
> >  struct sun8i_mixer;
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c
> > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c index
> > 8697afc36023..22e1ed5cd7a4 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c
> > @@ -835,7 +835,10 @@ static const u32 bicubic4coefftab32[480] = {
> > 
> >  static inline u32 sun8i_vi_scaler_base(struct sun8i_mixer *mixer, int
> >  channel) {
> > 
> > -       return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel;
> > +       if (mixer->cfg->is_de3)
> > +               return DE3_VI_SCALER_BASE + DE3_VI_SCALER_SIZE * channel;
> > +       else
> > +               return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel;
> > 
> >  }
> >  
> >  static int sun8i_vi_scaler_coef_index(unsigned int step)
> > 
> > @@ -951,6 +954,35 @@ void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer,
> > int layer,> 
> >                 cvphase = vphase;
> >         
> >         }
> > 
> > +       if (mixer->cfg->is_de3) {
> > +               u32 val;
> > +
> > +               if (format->hsub == 1 && format->vsub == 1)
> > +                       val = SUN8I_SCALER_VSU_SCALE_MODE_UI;
> > +               else
> > +                       val = SUN8I_SCALER_VSU_SCALE_MODE_NORMAL;
> > +
> > +               regmap_write(mixer->engine.regs,
> > +                            SUN8I_SCALER_VSU_SCALE_MODE(base), val);
> 
> The remaining settings seem to be related to the edge detection scaling
> method. Since you aren't supporting it, are they necessary?

Didn't really tried without them IIRC. I will for next revision.

Best regards,
Jernej

> 
> > +               regmap_write(mixer->engine.regs,
> > +                            SUN8I_SCALER_VSU_DIR_THR(base),
> > +                            SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(0) |
> > +                            SUN8I_SCALER_VSU_ZERO_DIR_THR(0) |
> > +                            SUN8I_SCALER_VSU_HORZ_DIR_THR(0xff) |
> > +                            SUN8I_SCALER_VSU_VERT_DIR_THR(1));
> > +               regmap_write(mixer->engine.regs,
> > +                            SUN8I_SCALER_VSU_EDGE_THR(base),
> > +                            SUN8I_SCALER_VSU_EDGE_SHIFT(8) |
> > +                            SUN8I_SCALER_VSU_EDGE_OFFSET(0));
> > +               regmap_write(mixer->engine.regs,
> > +                            SUN8I_SCALER_VSU_EDSCL_CTRL(base), 0);
> > +               regmap_write(mixer->engine.regs,
> > +                            SUN8I_SCALER_VSU_ANGLE_THR(base),
> > +                            SUN8I_SCALER_VSU_ANGLE_SHIFT(2) |
> > +                            SUN8I_SCALER_VSU_ANGLE_OFFSET(0));
> > +       }
> > +
> > 
> >         regmap_write(mixer->engine.regs,
> >         
> >                      SUN8I_SCALER_VSU_OUTSIZE(base), outsize);
> >         
> >         regmap_write(mixer->engine.regs,
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h
> > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h index
> > cd015405f66d..c322f5652481 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h
> > @@ -15,6 +15,9 @@
> > 
> >  #define DE2_VI_SCALER_BASE 0x20000
> >  #define DE2_VI_SCALER_SIZE 0x20000
> > 
> > +#define DE3_VI_SCALER_BASE 0x20000
> > +#define DE3_VI_SCALER_SIZE 0x08000
> > +
> > 
> >  /* this two macros assumes 16 fractional bits which is standard in DRM */
> >  #define SUN8I_VI_SCALER_SCALE_MIN              1
> >  #define SUN8I_VI_SCALER_SCALE_MAX              ((1UL << 20) - 1)
> > 
> > @@ -25,6 +28,11 @@
> > 
> >  #define SUN8I_VI_SCALER_SIZE(w, h)             (((h) - 1) << 16 | ((w) -
> >  1))
> >  
> >  #define SUN8I_SCALER_VSU_CTRL(base)            ((base) + 0x0)
> > 
> > +#define SUN8I_SCALER_VSU_SCALE_MODE(base)      ((base) + 0x10)
> > +#define SUN8I_SCALER_VSU_DIR_THR(base)         ((base) + 0x20)
> > +#define SUN8I_SCALER_VSU_EDGE_THR(base)                ((base) + 0x24)
> > +#define SUN8I_SCALER_VSU_EDSCL_CTRL(base)      ((base) + 0x28)
> > +#define SUN8I_SCALER_VSU_ANGLE_THR(base)       ((base) + 0x2c)
> 
> DE3 prefix for the new ones please.
> 
> >  #define SUN8I_SCALER_VSU_OUTSIZE(base)         ((base) + 0x40)
> >  #define SUN8I_SCALER_VSU_YINSIZE(base)         ((base) + 0x80)
> >  #define SUN8I_SCALER_VSU_YHSTEP(base)          ((base) + 0x88)
> > 
> > @@ -46,6 +54,21 @@
> > 
> >  #define SUN8I_SCALER_VSU_CTRL_EN               BIT(0)
> >  #define SUN8I_SCALER_VSU_CTRL_COEFF_RDY                BIT(4)
> > 
> > +#define SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(x)   (((x) << 24) & 0xFF)
> > +#define SUN8I_SCALER_VSU_ZERO_DIR_THR(x)       (((x) << 16) & 0xFF)
> > +#define SUN8I_SCALER_VSU_HORZ_DIR_THR(x)       (((x) << 8) & 0xFF)
> > +#define SUN8I_SCALER_VSU_VERT_DIR_THR(x)       ((x) & 0xFF)
> > +
> > +#define SUN8I_SCALER_VSU_SCALE_MODE_UI         0
> > +#define SUN8I_SCALER_VSU_SCALE_MODE_NORMAL     1
> > +#define SUN8I_SCALER_VSU_SCALE_MODE_ED_SCALE   2
> > +
> > +#define SUN8I_SCALER_VSU_EDGE_SHIFT(x)         (((x) << 16) & 0xF)
> > +#define SUN8I_SCALER_VSU_EDGE_OFFSET(x)                ((x) & 0xFF)
> > +
> > +#define SUN8I_SCALER_VSU_ANGLE_SHIFT(x)                (((x) << 16) &
> > 0xF)
> > +#define SUN8I_SCALER_VSU_ANGLE_OFFSET(x)       ((x) & 0xFF)
> > +
> 
> Same here.
> 
> Regards
> ChenYu
> 
> >  void sun8i_vi_scaler_enable(struct sun8i_mixer *mixer, int layer, bool
> >  enable); void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer, int
> >  layer,
> >  
> >                            u32 src_w, u32 src_h, u32 dst_w, u32 dst_h,
> > 
> > --
> > 2.18.0

Chen-Yu Tsai Sept. 24, 2018, 2:04 a.m. UTC | #3

On Mon, Sep 24, 2018 at 3:51 AM Jernej Škrabec <jernej.skrabec@siol.net> wrote:
>
> Dne sobota, 22. september 2018 ob 15:19:12 CEST je Chen-Yu Tsai napisal(a):
> > On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net>
> wrote:
> > > Display Engine 3 is an upgrade of DE2 with new features like support for
> > > 10 bit color formats and support for AFBC.
> > >
> > > Most of DE2 code works with DE3, except some small details.
> > >
> > > Add support for it.
> > >
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > ---
> > >
> > >  drivers/gpu/drm/sun4i/sun8i_csc.c       | 96 +++++++++++++++++++++++--
> > >  drivers/gpu/drm/sun4i/sun8i_mixer.c     | 17 ++++-
> > >  drivers/gpu/drm/sun4i/sun8i_mixer.h     | 21 +++++-
> > >  drivers/gpu/drm/sun4i/sun8i_ui_scaler.c |  8 ++-
> > >  drivers/gpu/drm/sun4i/sun8i_ui_scaler.h |  1 +
> > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c  |  8 +++
> > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.h  |  2 +
> > >  drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 34 ++++++++-
> > >  drivers/gpu/drm/sun4i/sun8i_vi_scaler.h | 23 ++++++
> > >  9 files changed, 197 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_csc.c
> > > b/drivers/gpu/drm/sun4i/sun8i_csc.c index b14925b40ccf..101901ccf2dc
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_csc.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_csc.c
> > > @@ -34,6 +34,34 @@ static const u32 yvu2rgb[] = {
> > >
> > >         0x000004A8, 0x00000000, 0x00000813, 0xFFFBAC4A,
> > >
> > >  };
> > >
> > > +/*
> > > + * DE3 has a bit different CSC units. Factors are in two's complement
> > > format. + * First three have 17 bits for fractinal part and last two 2
> > > bits. First + * three values in each line are multiplication factor, 4th
> > > is difference, + * which is subtracted from the input value before the
> > > multiplication and + * last value is constant, which is added at the end.
> > > + *
> > > + * x' = c00 * (x + d0) + c01 * (y + d1) + c02 * (z + d2) + const0
> > > + * y' = c10 * (x + d0) + c11 * (y + d1) + c12 * (z + d2) + const1
> > > + * z' = c20 * (x + d0) + c21 * (y + d1) + c22 * (z + d2) + const2
> > > + *
> > > + * Please note that above formula is true only for Blender CSC. Other DE3
> > > CSC + * units takes only positive value for difference. From what can be
> > > deducted + * from BSP driver code, those units probably automatically
> > > assume that + * difference has to be subtracted.
> > > + */
> > > +static const u32 yuv2rgb_de3[] = {
> > > +       0x0002542a, 0x00000000, 0x0003312a, 0xffffffc0, 0x00000000,
> > > +       0x0002542a, 0xffff376b, 0xfffe5fc3, 0xfffffe00, 0x00000000,
> > > +       0x0002542a, 0x000408d3, 0x00000000, 0xfffffe00, 0x00000000,
> > > +};
> > > +
> > > +static const u32 yvu2rgb_de3[] = {
> > > +       0x0002542a, 0x0003312a, 0x00000000, 0xffffffc0, 0x00000000,
> > > +       0x0002542a, 0xfffe5fc3, 0xffff376b, 0xfffffe00, 0x00000000,
> > > +       0x0002542a, 0x00000000, 0x000408d3, 0xfffffe00, 0x00000000,
> > > +};
> > > +
> > >
> > >  static void sun8i_csc_set_coefficients(struct regmap *map, u32 base,
> > >
> > >                                        enum sun8i_csc_mode mode)
> > >
> > >  {
> > >
> > > @@ -61,6 +89,38 @@ static void sun8i_csc_set_coefficients(struct regmap
> > > *map, u32 base,>
> > >         }
> > >
> > >  }
> > >
> > > +static void sun8i_de3_ccsc_set_coefficients(struct regmap *map, int
> > > layer,
> > > +                                           enum sun8i_csc_mode mode)
> > > +{
> > > +       const u32 *table;
> > > +       int i, j;
> > > +
> > > +       switch (mode) {
> > > +       case SUN8I_CSC_MODE_YUV2RGB:
> > > +               table = yuv2rgb_de3;
> > > +               break;
> > > +       case SUN8I_CSC_MODE_YVU2RGB:
> > > +               table = yvu2rgb_de3;
> > > +               break;
> > > +       default:
> > > +               DRM_WARN("Wrong CSC mode specified.\n");
> > > +               return;
> > > +       }
> > > +
> > > +       for (i = 0; i < 3; i++) {
> > > +               for (j = 0; j < 3; j++)
> > > +                       regmap_write(map,
> > > +
> > > SUN8I_MIXER_BLEND_CSC_COEFF(DE3_BLD_BASE, +
> > >                                  layer, i, j), +
> > >           table[i * 5 + j]);
> >
> > Given that the first three values occupy contiguous addresses,
> > you can use regmap_bulk_write() here.
> >
> > > +               regmap_write(map,
> > > +                            SUN8I_MIXER_BLEND_CSC_CONST(DE3_BLD_BASE,
> > > +                                                        layer, i),
> > > +                            SUN8I_MIXER_BLEND_CSC_CONST_VAL(table[i * 5 +
> > > 3], +                                                            table[i
> > > * 5 + 4]));
> > Nit: Using a two-dimension array might make it easier to read.
> >
> > > +       }
> > > +}
> > > +
> > >
> > >  static void sun8i_csc_enable(struct regmap *map, u32 base, bool enable)
> > >  {
> > >
> > >         u32 val;
> > >
> > > @@ -73,21 +133,45 @@ static void sun8i_csc_enable(struct regmap *map, u32
> > > base, bool enable)>
> > >         regmap_update_bits(map, SUN8I_CSC_CTRL(base), SUN8I_CSC_CTRL_EN,
> > >         val);
> > >
> > >  }
> > >
> > > +static void sun8i_de3_ccsc_enable(struct regmap *map, int layer, bool
> > > enable) +{
> > > +       u32 val, mask;
> > > +
> > > +       mask = SUN8I_MIXER_BLEND_CSC_CTL_EN(layer);
> > > +
> > > +       if (enable)
> > > +               val = mask;
> > > +       else
> > > +               val = 0;
> > > +
> > > +       regmap_update_bits(map, SUN8I_MIXER_BLEND_CSC_CTL(DE3_BLD_BASE),
> > > +                          mask, val);
> > > +}
> > > +
> > >
> > >  void sun8i_csc_set_ccsc_coefficients(struct sun8i_mixer *mixer, int
> > >  layer,
> > >
> > >                                      enum sun8i_csc_mode mode)
> > >
> > >  {
> > >
> > > -       u32 base;
> > > +       if (!mixer->cfg->is_de3) {
> > > +               u32 base;
> >
> > You could rewrite this as
> >
> >     if (mixer->cfg->is_de3) {
> >                sun8i_de3_ccsc_set_coefficients(mixer->engine.regs,
> >                                                layer, mode);
> >                return;
> >     }
> >
> > That way you don't need to change the indentation on the existing lines.
> > I suppose this is more of a personal preference. The downside is the control
> > flow is slightly more complicated.
> >
> > > -       base = ccsc_base[mixer->cfg->ccsc][layer];
> > > +               base = ccsc_base[mixer->cfg->ccsc][layer];
> > >
> > > -       sun8i_csc_set_coefficients(mixer->engine.regs, base, mode);
> > > +               sun8i_csc_set_coefficients(mixer->engine.regs, base,
> > > mode);
> > > +       } else {
> > > +               sun8i_de3_ccsc_set_coefficients(mixer->engine.regs,
> > > +                                               layer, mode);
> > > +       }
> > >
> > >  }
> > >
> > >  void sun8i_csc_enable_ccsc(struct sun8i_mixer *mixer, int layer, bool
> > >  enable) {
> > >
> > > -       u32 base;
> > > +       if (!mixer->cfg->is_de3) {
> > > +               u32 base;
> > >
> > > -       base = ccsc_base[mixer->cfg->ccsc][layer];
> > > +               base = ccsc_base[mixer->cfg->ccsc][layer];
> > >
> > > -       sun8i_csc_enable(mixer->engine.regs, base, enable);
> > > +               sun8i_csc_enable(mixer->engine.regs, base, enable);
> > > +       } else {
> > > +               sun8i_de3_ccsc_enable(mixer->engine.regs, layer, enable);
> > > +       }
> > >
> > >  }
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index 743941a33d88..a9218abf0935
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > @@ -460,8 +460,21 @@ static int sun8i_mixer_bind(struct device *dev,
> > > struct device *master,>
> > >         base = sun8i_blender_base(mixer);
> > >
> > >         /* Reset the registers */
> > >
> > > -       for (i = 0x0; i < 0x20000; i += 4)
> > > -               regmap_write(mixer->engine.regs, i, 0);
> > > +       if (mixer->cfg->is_de3) {
> > > +               for (i = 0x0; i < 0x3000; i += 4)
> > > +                       regmap_write(mixer->engine.regs, i, 0);
> > > +               for (i = 0x20000; i < 0x40000; i += 4)
> > > +                       regmap_write(mixer->engine.regs, i, 0);
> > > +               for (i = 0x70000; i < 0x88000; i += 4)
> > > +                       regmap_write(mixer->engine.regs, i, 0);
> > > +               for (i = 0xa0000; i < 0xb0000; i += 4)
> > > +                       regmap_write(mixer->engine.regs, i, 0);
> > > +               for (i = 0xd0000; i < 0xe0000; i += 4)
> > > +                       regmap_write(mixer->engine.regs, i, 0);
> >
> > Could you use the macros *_BASE and *_SIZE here? That would make
> > it more obviously what parts you're clearing.
>
> Ok, but where should I define them? In sun8i-mixer.h?

If it's only used here you could put them just above this function.

> >
> > The last offset, 0xd0000, isn't even defined in the DE3 spec.
>
> It is, check chapter 2.3.2, table 2-3.

I must have read something incorrectly. I see it now. Thanks.

ChenYu

> >
> > > +       } else {
> > > +               for (i = 0x0; i < 0x20000; i += 4)
> > > +                       regmap_write(mixer->engine.regs, i, 0);
> > > +       }
> > >
> > >         /* Enable the mixer */
> > >         regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_CTL,
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 020b0a097c84..4c9a442bbb44
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > @@ -33,6 +33,10 @@
> > >
> > >  #define DE2_CH_BASE                            0x2000
> > >  #define DE2_CH_SIZE                            0x1000
> > >
> > > +#define DE3_BLD_BASE                           0x0800
> > > +#define DE3_CH_BASE                            0x1000
> > > +#define DE3_CH_SIZE                            0x0800
> > > +
> > >
> > >  #define SUN8I_MIXER_BLEND_PIPE_CTL(base)       ((base) + 0)
> > >  #define SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, x) ((base) + 0x4 + 0x10 *
> > >  (x))
> > >  #define SUN8I_MIXER_BLEND_ATTR_INSIZE(base, x) ((base) + 0x8 + 0x10 *
> > >  (x))
> > >
> > > @@ -47,6 +51,11 @@
> > >
> > >  #define SUN8I_MIXER_BLEND_CK_MAX(base, x)      ((base) + 0xc0 + 0x04 *
> > >  (x))
> > >  #define SUN8I_MIXER_BLEND_CK_MIN(base, x)      ((base) + 0xe0 + 0x04 *
> > >  (x))
> > >  #define SUN8I_MIXER_BLEND_OUTCTL(base)         ((base) + 0xfc)
> > >
> > > +#define SUN8I_MIXER_BLEND_CSC_CTL(base)                ((base) + 0x100)
> > > +#define SUN8I_MIXER_BLEND_CSC_COEFF(base, layer, x, y) \
> > > +       ((base) + 0x110 + (layer) * 0x30 +  (x) * 0x10 + 4 * (y))
> > > +#define SUN8I_MIXER_BLEND_CSC_CONST(base, layer, i) \
> > > +       ((base) + 0x110 + (layer) * 0x30 +  (i) * 0x10 + 0x0c)
> >
> > Should these have a DE3 prefix?
>
> Ok for all prefix comments.
>
> >
> > >  #define SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK      GENMASK(12, 8)
> > >  #define SUN8I_MIXER_BLEND_PIPE_CTL_EN(pipe)    BIT(8 + pipe)
> > >
> > > @@ -61,6 +70,9 @@
> > >
> > >  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED    BIT(1)
> > >
> > > +#define SUN8I_MIXER_BLEND_CSC_CTL_EN(ch)       BIT(ch)
> > > +#define SUN8I_MIXER_BLEND_CSC_CONST_VAL(d, c)  (((d) << 16) | ((c) &
> > > 0xffff)) +
> >
> > Same here.
> >
> > >  #define SUN8I_MIXER_FBFMT_ARGB8888     0
> > >  #define SUN8I_MIXER_FBFMT_ABGR8888     1
> > >  #define SUN8I_MIXER_FBFMT_RGBA8888     2
> >
> > [...]
> >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 46f0237c17bb..3d0ad64178ea
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > @@ -30,6 +30,8 @@
> > >
> > >  #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_RGB_MODE                BIT(15)
> > >  #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_OFFSET    8
> > >  #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_MASK      GENMASK(12, 8)
> > >
> > > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK      GENMASK(31, 24)
> > > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x)                ((x) <<
> > > 24)
> >
> > DE3 prefix?
> >
> > >  struct sun8i_mixer;
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c
> > > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c index
> > > 8697afc36023..22e1ed5cd7a4 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c
> > > @@ -835,7 +835,10 @@ static const u32 bicubic4coefftab32[480] = {
> > >
> > >  static inline u32 sun8i_vi_scaler_base(struct sun8i_mixer *mixer, int
> > >  channel) {
> > >
> > > -       return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel;
> > > +       if (mixer->cfg->is_de3)
> > > +               return DE3_VI_SCALER_BASE + DE3_VI_SCALER_SIZE * channel;
> > > +       else
> > > +               return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel;
> > >
> > >  }
> > >
> > >  static int sun8i_vi_scaler_coef_index(unsigned int step)
> > >
> > > @@ -951,6 +954,35 @@ void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer,
> > > int layer,>
> > >                 cvphase = vphase;
> > >
> > >         }
> > >
> > > +       if (mixer->cfg->is_de3) {
> > > +               u32 val;
> > > +
> > > +               if (format->hsub == 1 && format->vsub == 1)
> > > +                       val = SUN8I_SCALER_VSU_SCALE_MODE_UI;
> > > +               else
> > > +                       val = SUN8I_SCALER_VSU_SCALE_MODE_NORMAL;
> > > +
> > > +               regmap_write(mixer->engine.regs,
> > > +                            SUN8I_SCALER_VSU_SCALE_MODE(base), val);
> >
> > The remaining settings seem to be related to the edge detection scaling
> > method. Since you aren't supporting it, are they necessary?
>
> Didn't really tried without them IIRC. I will for next revision.
>
> Best regards,
> Jernej
>
> >
> > > +               regmap_write(mixer->engine.regs,
> > > +                            SUN8I_SCALER_VSU_DIR_THR(base),
> > > +                            SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(0) |
> > > +                            SUN8I_SCALER_VSU_ZERO_DIR_THR(0) |
> > > +                            SUN8I_SCALER_VSU_HORZ_DIR_THR(0xff) |
> > > +                            SUN8I_SCALER_VSU_VERT_DIR_THR(1));
> > > +               regmap_write(mixer->engine.regs,
> > > +                            SUN8I_SCALER_VSU_EDGE_THR(base),
> > > +                            SUN8I_SCALER_VSU_EDGE_SHIFT(8) |
> > > +                            SUN8I_SCALER_VSU_EDGE_OFFSET(0));
> > > +               regmap_write(mixer->engine.regs,
> > > +                            SUN8I_SCALER_VSU_EDSCL_CTRL(base), 0);
> > > +               regmap_write(mixer->engine.regs,
> > > +                            SUN8I_SCALER_VSU_ANGLE_THR(base),
> > > +                            SUN8I_SCALER_VSU_ANGLE_SHIFT(2) |
> > > +                            SUN8I_SCALER_VSU_ANGLE_OFFSET(0));
> > > +       }
> > > +
> > >
> > >         regmap_write(mixer->engine.regs,
> > >
> > >                      SUN8I_SCALER_VSU_OUTSIZE(base), outsize);
> > >
> > >         regmap_write(mixer->engine.regs,
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h
> > > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h index
> > > cd015405f66d..c322f5652481 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h
> > > @@ -15,6 +15,9 @@
> > >
> > >  #define DE2_VI_SCALER_BASE 0x20000
> > >  #define DE2_VI_SCALER_SIZE 0x20000
> > >
> > > +#define DE3_VI_SCALER_BASE 0x20000
> > > +#define DE3_VI_SCALER_SIZE 0x08000
> > > +
> > >
> > >  /* this two macros assumes 16 fractional bits which is standard in DRM */
> > >  #define SUN8I_VI_SCALER_SCALE_MIN              1
> > >  #define SUN8I_VI_SCALER_SCALE_MAX              ((1UL << 20) - 1)
> > >
> > > @@ -25,6 +28,11 @@
> > >
> > >  #define SUN8I_VI_SCALER_SIZE(w, h)             (((h) - 1) << 16 | ((w) -
> > >  1))
> > >
> > >  #define SUN8I_SCALER_VSU_CTRL(base)            ((base) + 0x0)
> > >
> > > +#define SUN8I_SCALER_VSU_SCALE_MODE(base)      ((base) + 0x10)
> > > +#define SUN8I_SCALER_VSU_DIR_THR(base)         ((base) + 0x20)
> > > +#define SUN8I_SCALER_VSU_EDGE_THR(base)                ((base) + 0x24)
> > > +#define SUN8I_SCALER_VSU_EDSCL_CTRL(base)      ((base) + 0x28)
> > > +#define SUN8I_SCALER_VSU_ANGLE_THR(base)       ((base) + 0x2c)
> >
> > DE3 prefix for the new ones please.
> >
> > >  #define SUN8I_SCALER_VSU_OUTSIZE(base)         ((base) + 0x40)
> > >  #define SUN8I_SCALER_VSU_YINSIZE(base)         ((base) + 0x80)
> > >  #define SUN8I_SCALER_VSU_YHSTEP(base)          ((base) + 0x88)
> > >
> > > @@ -46,6 +54,21 @@
> > >
> > >  #define SUN8I_SCALER_VSU_CTRL_EN               BIT(0)
> > >  #define SUN8I_SCALER_VSU_CTRL_COEFF_RDY                BIT(4)
> > >
> > > +#define SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(x)   (((x) << 24) & 0xFF)
> > > +#define SUN8I_SCALER_VSU_ZERO_DIR_THR(x)       (((x) << 16) & 0xFF)
> > > +#define SUN8I_SCALER_VSU_HORZ_DIR_THR(x)       (((x) << 8) & 0xFF)
> > > +#define SUN8I_SCALER_VSU_VERT_DIR_THR(x)       ((x) & 0xFF)
> > > +
> > > +#define SUN8I_SCALER_VSU_SCALE_MODE_UI         0
> > > +#define SUN8I_SCALER_VSU_SCALE_MODE_NORMAL     1
> > > +#define SUN8I_SCALER_VSU_SCALE_MODE_ED_SCALE   2
> > > +
> > > +#define SUN8I_SCALER_VSU_EDGE_SHIFT(x)         (((x) << 16) & 0xF)
> > > +#define SUN8I_SCALER_VSU_EDGE_OFFSET(x)                ((x) & 0xFF)
> > > +
> > > +#define SUN8I_SCALER_VSU_ANGLE_SHIFT(x)                (((x) << 16) &
> > > 0xF)
> > > +#define SUN8I_SCALER_VSU_ANGLE_OFFSET(x)       ((x) & 0xFF)
> > > +
> >
> > Same here.
> >
> > Regards
> > ChenYu
> >
> > >  void sun8i_vi_scaler_enable(struct sun8i_mixer *mixer, int layer, bool
> > >  enable); void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer, int
> > >  layer,
> > >
> > >                            u32 src_w, u32 src_h, u32 dst_w, u32 dst_h,
> > >
> > > --
> > > 2.18.0
>
>
>
>

Jernej Škrabec Oct. 5, 2018, 5:51 p.m. UTC | #4

Dne sobota, 22. september 2018 ob 15:19:12 CEST je Chen-Yu Tsai napisal(a):
> On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net> 
wrote:
> > Display Engine 3 is an upgrade of DE2 with new features like support for
> > 10 bit color formats and support for AFBC.
> > 
> > Most of DE2 code works with DE3, except some small details.
> > 
> > Add support for it.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > 
> >  drivers/gpu/drm/sun4i/sun8i_csc.c       | 96 +++++++++++++++++++++++--
> >  drivers/gpu/drm/sun4i/sun8i_mixer.c     | 17 ++++-
> >  drivers/gpu/drm/sun4i/sun8i_mixer.h     | 21 +++++-
> >  drivers/gpu/drm/sun4i/sun8i_ui_scaler.c |  8 ++-
> >  drivers/gpu/drm/sun4i/sun8i_ui_scaler.h |  1 +
> >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c  |  8 +++
> >  drivers/gpu/drm/sun4i/sun8i_vi_layer.h  |  2 +
> >  drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 34 ++++++++-
> >  drivers/gpu/drm/sun4i/sun8i_vi_scaler.h | 23 ++++++
> >  9 files changed, 197 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_csc.c
> > b/drivers/gpu/drm/sun4i/sun8i_csc.c index b14925b40ccf..101901ccf2dc
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_csc.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_csc.c
> > @@ -34,6 +34,34 @@ static const u32 yvu2rgb[] = {
> > 
> >         0x000004A8, 0x00000000, 0x00000813, 0xFFFBAC4A,
> >  
> >  };
> > 
> > +/*
> > + * DE3 has a bit different CSC units. Factors are in two's complement
> > format. + * First three have 17 bits for fractinal part and last two 2
> > bits. First + * three values in each line are multiplication factor, 4th
> > is difference, + * which is subtracted from the input value before the
> > multiplication and + * last value is constant, which is added at the end.
> > + *
> > + * x' = c00 * (x + d0) + c01 * (y + d1) + c02 * (z + d2) + const0
> > + * y' = c10 * (x + d0) + c11 * (y + d1) + c12 * (z + d2) + const1
> > + * z' = c20 * (x + d0) + c21 * (y + d1) + c22 * (z + d2) + const2
> > + *
> > + * Please note that above formula is true only for Blender CSC. Other DE3
> > CSC + * units takes only positive value for difference. From what can be
> > deducted + * from BSP driver code, those units probably automatically
> > assume that + * difference has to be subtracted.
> > + */
> > +static const u32 yuv2rgb_de3[] = {
> > +       0x0002542a, 0x00000000, 0x0003312a, 0xffffffc0, 0x00000000,
> > +       0x0002542a, 0xffff376b, 0xfffe5fc3, 0xfffffe00, 0x00000000,
> > +       0x0002542a, 0x000408d3, 0x00000000, 0xfffffe00, 0x00000000,
> > +};
> > +
> > +static const u32 yvu2rgb_de3[] = {
> > +       0x0002542a, 0x0003312a, 0x00000000, 0xffffffc0, 0x00000000,
> > +       0x0002542a, 0xfffe5fc3, 0xffff376b, 0xfffffe00, 0x00000000,
> > +       0x0002542a, 0x00000000, 0x000408d3, 0xfffffe00, 0x00000000,
> > +};
> > +
> > 
> >  static void sun8i_csc_set_coefficients(struct regmap *map, u32 base,
> >  
> >                                        enum sun8i_csc_mode mode)
> >  
> >  {
> > 
> > @@ -61,6 +89,38 @@ static void sun8i_csc_set_coefficients(struct regmap
> > *map, u32 base,> 
> >         }
> >  
> >  }
> > 
> > +static void sun8i_de3_ccsc_set_coefficients(struct regmap *map, int
> > layer,
> > +                                           enum sun8i_csc_mode mode)
> > +{
> > +       const u32 *table;
> > +       int i, j;
> > +
> > +       switch (mode) {
> > +       case SUN8I_CSC_MODE_YUV2RGB:
> > +               table = yuv2rgb_de3;
> > +               break;
> > +       case SUN8I_CSC_MODE_YVU2RGB:
> > +               table = yvu2rgb_de3;
> > +               break;
> > +       default:
> > +               DRM_WARN("Wrong CSC mode specified.\n");
> > +               return;
> > +       }
> > +
> > +       for (i = 0; i < 3; i++) {
> > +               for (j = 0; j < 3; j++)
> > +                       regmap_write(map,
> > +                                   
> > SUN8I_MIXER_BLEND_CSC_COEFF(DE3_BLD_BASE, +                              
> >                                  layer, i, j), +                         
> >           table[i * 5 + j]);
> 
> Given that the first three values occupy contiguous addresses,
> you can use regmap_bulk_write() here.

Do you mind if I rework table in a way that directly fits in registers and use 
regmap_bulk_write() on whole table? This means I must combine two values into 
one for "const" register, but I would explain it better in a comment. It would 
be most elegant solution, one call to regmap_bulk_write() instead of two for 
loops and some arithmetics.

I guess macro for "const" part can stay as documentation.

> 
> > +               regmap_write(map,
> > +                            SUN8I_MIXER_BLEND_CSC_CONST(DE3_BLD_BASE,
> > +                                                        layer, i),
> > +                            SUN8I_MIXER_BLEND_CSC_CONST_VAL(table[i * 5 +
> > 3], +                                                            table[i
> > * 5 + 4]));
> Nit: Using a two-dimension array might make it easier to read.
> 
> > +       }
> > +}
> > +
> > 
> >  static void sun8i_csc_enable(struct regmap *map, u32 base, bool enable)
> >  {
> >  
> >         u32 val;
> > 
> > @@ -73,21 +133,45 @@ static void sun8i_csc_enable(struct regmap *map, u32
> > base, bool enable)> 
> >         regmap_update_bits(map, SUN8I_CSC_CTRL(base), SUN8I_CSC_CTRL_EN,
> >         val);
> >  
> >  }
> > 
> > +static void sun8i_de3_ccsc_enable(struct regmap *map, int layer, bool
> > enable) +{
> > +       u32 val, mask;
> > +
> > +       mask = SUN8I_MIXER_BLEND_CSC_CTL_EN(layer);
> > +
> > +       if (enable)
> > +               val = mask;
> > +       else
> > +               val = 0;
> > +
> > +       regmap_update_bits(map, SUN8I_MIXER_BLEND_CSC_CTL(DE3_BLD_BASE),
> > +                          mask, val);
> > +}
> > +
> > 
> >  void sun8i_csc_set_ccsc_coefficients(struct sun8i_mixer *mixer, int
> >  layer,
> >  
> >                                      enum sun8i_csc_mode mode)
> >  
> >  {
> > 
> > -       u32 base;
> > +       if (!mixer->cfg->is_de3) {
> > +               u32 base;
> 
> You could rewrite this as
> 
>     if (mixer->cfg->is_de3) {
>                sun8i_de3_ccsc_set_coefficients(mixer->engine.regs,
>                                                layer, mode);
>                return;
>     }
> 
> That way you don't need to change the indentation on the existing lines.
> I suppose this is more of a personal preference. The downside is the control
> flow is slightly more complicated.
> 
> > -       base = ccsc_base[mixer->cfg->ccsc][layer];
> > +               base = ccsc_base[mixer->cfg->ccsc][layer];
> > 
> > -       sun8i_csc_set_coefficients(mixer->engine.regs, base, mode);
> > +               sun8i_csc_set_coefficients(mixer->engine.regs, base,
> > mode);
> > +       } else {
> > +               sun8i_de3_ccsc_set_coefficients(mixer->engine.regs,
> > +                                               layer, mode);
> > +       }
> > 
> >  }
> >  
> >  void sun8i_csc_enable_ccsc(struct sun8i_mixer *mixer, int layer, bool
> >  enable) {
> > 
> > -       u32 base;
> > +       if (!mixer->cfg->is_de3) {
> > +               u32 base;
> > 
> > -       base = ccsc_base[mixer->cfg->ccsc][layer];
> > +               base = ccsc_base[mixer->cfg->ccsc][layer];
> > 
> > -       sun8i_csc_enable(mixer->engine.regs, base, enable);
> > +               sun8i_csc_enable(mixer->engine.regs, base, enable);
> > +       } else {
> > +               sun8i_de3_ccsc_enable(mixer->engine.regs, layer, enable);
> > +       }
> > 
> >  }
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index 743941a33d88..a9218abf0935
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > @@ -460,8 +460,21 @@ static int sun8i_mixer_bind(struct device *dev,
> > struct device *master,> 
> >         base = sun8i_blender_base(mixer);
> >         
> >         /* Reset the registers */
> > 
> > -       for (i = 0x0; i < 0x20000; i += 4)
> > -               regmap_write(mixer->engine.regs, i, 0);
> > +       if (mixer->cfg->is_de3) {
> > +               for (i = 0x0; i < 0x3000; i += 4)
> > +                       regmap_write(mixer->engine.regs, i, 0);
> > +               for (i = 0x20000; i < 0x40000; i += 4)
> > +                       regmap_write(mixer->engine.regs, i, 0);
> > +               for (i = 0x70000; i < 0x88000; i += 4)
> > +                       regmap_write(mixer->engine.regs, i, 0);
> > +               for (i = 0xa0000; i < 0xb0000; i += 4)
> > +                       regmap_write(mixer->engine.regs, i, 0);
> > +               for (i = 0xd0000; i < 0xe0000; i += 4)
> > +                       regmap_write(mixer->engine.regs, i, 0);
> 
> Could you use the macros *_BASE and *_SIZE here? That would make
> it more obviously what parts you're clearing.
> 
> The last offset, 0xd0000, isn't even defined in the DE3 spec.
> 
> > +       } else {
> > +               for (i = 0x0; i < 0x20000; i += 4)
> > +                       regmap_write(mixer->engine.regs, i, 0);
> > +       }
> > 
> >         /* Enable the mixer */
> >         regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_CTL,
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 020b0a097c84..4c9a442bbb44
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > @@ -33,6 +33,10 @@
> > 
> >  #define DE2_CH_BASE                            0x2000
> >  #define DE2_CH_SIZE                            0x1000
> > 
> > +#define DE3_BLD_BASE                           0x0800
> > +#define DE3_CH_BASE                            0x1000
> > +#define DE3_CH_SIZE                            0x0800
> > +
> > 
> >  #define SUN8I_MIXER_BLEND_PIPE_CTL(base)       ((base) + 0)
> >  #define SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, x) ((base) + 0x4 + 0x10 *
> >  (x))
> >  #define SUN8I_MIXER_BLEND_ATTR_INSIZE(base, x) ((base) + 0x8 + 0x10 *
> >  (x))
> > 
> > @@ -47,6 +51,11 @@
> > 
> >  #define SUN8I_MIXER_BLEND_CK_MAX(base, x)      ((base) + 0xc0 + 0x04 *
> >  (x))
> >  #define SUN8I_MIXER_BLEND_CK_MIN(base, x)      ((base) + 0xe0 + 0x04 *
> >  (x))
> >  #define SUN8I_MIXER_BLEND_OUTCTL(base)         ((base) + 0xfc)
> > 
> > +#define SUN8I_MIXER_BLEND_CSC_CTL(base)                ((base) + 0x100)
> > +#define SUN8I_MIXER_BLEND_CSC_COEFF(base, layer, x, y) \
> > +       ((base) + 0x110 + (layer) * 0x30 +  (x) * 0x10 + 4 * (y))
> > +#define SUN8I_MIXER_BLEND_CSC_CONST(base, layer, i) \
> > +       ((base) + 0x110 + (layer) * 0x30 +  (i) * 0x10 + 0x0c)
> 
> Should these have a DE3 prefix?

How do you want this to look like?

SUN50I_DE3_MIXER_BLEND_CSC_CONST?

SUN8I prefix doesn't fit for DE3, but as usual, there is a chance that AW 
releases 32 bit SoC with DE3.

At the end, it would be better to rename everything to have just DE2 or DE3 
prefix without mentioning any SoC family. After all, DE2 is also supported on 
sun50i family, which makes any family name in DE2/3 macros confusing...

Do you think I can squeeze dropping SUN8I prefix to a previous patch, where 
macros are reworked anyway?

Best regards,
Jernej

> 
> >  #define SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK      GENMASK(12, 8)
> >  #define SUN8I_MIXER_BLEND_PIPE_CTL_EN(pipe)    BIT(8 + pipe)
> > 
> > @@ -61,6 +70,9 @@
> > 
> >  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED    BIT(1)
> > 
> > +#define SUN8I_MIXER_BLEND_CSC_CTL_EN(ch)       BIT(ch)
> > +#define SUN8I_MIXER_BLEND_CSC_CONST_VAL(d, c)  (((d) << 16) | ((c) &
> > 0xffff)) +
> 
> Same here.
> 
> >  #define SUN8I_MIXER_FBFMT_ARGB8888     0
> >  #define SUN8I_MIXER_FBFMT_ABGR8888     1
> >  #define SUN8I_MIXER_FBFMT_RGBA8888     2
> 
> [...]
> 
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 46f0237c17bb..3d0ad64178ea
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > @@ -30,6 +30,8 @@
> > 
> >  #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_RGB_MODE                BIT(15)
> >  #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_OFFSET    8
> >  #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_MASK      GENMASK(12, 8)
> > 
> > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK      GENMASK(31, 24)
> > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x)                ((x) <<
> > 24)
> 
> DE3 prefix?
> 
> >  struct sun8i_mixer;
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c
> > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c index
> > 8697afc36023..22e1ed5cd7a4 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c
> > @@ -835,7 +835,10 @@ static const u32 bicubic4coefftab32[480] = {
> > 
> >  static inline u32 sun8i_vi_scaler_base(struct sun8i_mixer *mixer, int
> >  channel) {
> > 
> > -       return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel;
> > +       if (mixer->cfg->is_de3)
> > +               return DE3_VI_SCALER_BASE + DE3_VI_SCALER_SIZE * channel;
> > +       else
> > +               return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel;
> > 
> >  }
> >  
> >  static int sun8i_vi_scaler_coef_index(unsigned int step)
> > 
> > @@ -951,6 +954,35 @@ void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer,
> > int layer,> 
> >                 cvphase = vphase;
> >         
> >         }
> > 
> > +       if (mixer->cfg->is_de3) {
> > +               u32 val;
> > +
> > +               if (format->hsub == 1 && format->vsub == 1)
> > +                       val = SUN8I_SCALER_VSU_SCALE_MODE_UI;
> > +               else
> > +                       val = SUN8I_SCALER_VSU_SCALE_MODE_NORMAL;
> > +
> > +               regmap_write(mixer->engine.regs,
> > +                            SUN8I_SCALER_VSU_SCALE_MODE(base), val);
> 
> The remaining settings seem to be related to the edge detection scaling
> method. Since you aren't supporting it, are they necessary?
> 
> > +               regmap_write(mixer->engine.regs,
> > +                            SUN8I_SCALER_VSU_DIR_THR(base),
> > +                            SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(0) |
> > +                            SUN8I_SCALER_VSU_ZERO_DIR_THR(0) |
> > +                            SUN8I_SCALER_VSU_HORZ_DIR_THR(0xff) |
> > +                            SUN8I_SCALER_VSU_VERT_DIR_THR(1));
> > +               regmap_write(mixer->engine.regs,
> > +                            SUN8I_SCALER_VSU_EDGE_THR(base),
> > +                            SUN8I_SCALER_VSU_EDGE_SHIFT(8) |
> > +                            SUN8I_SCALER_VSU_EDGE_OFFSET(0));
> > +               regmap_write(mixer->engine.regs,
> > +                            SUN8I_SCALER_VSU_EDSCL_CTRL(base), 0);
> > +               regmap_write(mixer->engine.regs,
> > +                            SUN8I_SCALER_VSU_ANGLE_THR(base),
> > +                            SUN8I_SCALER_VSU_ANGLE_SHIFT(2) |
> > +                            SUN8I_SCALER_VSU_ANGLE_OFFSET(0));
> > +       }
> > +
> > 
> >         regmap_write(mixer->engine.regs,
> >         
> >                      SUN8I_SCALER_VSU_OUTSIZE(base), outsize);
> >         
> >         regmap_write(mixer->engine.regs,
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h
> > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h index
> > cd015405f66d..c322f5652481 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h
> > @@ -15,6 +15,9 @@
> > 
> >  #define DE2_VI_SCALER_BASE 0x20000
> >  #define DE2_VI_SCALER_SIZE 0x20000
> > 
> > +#define DE3_VI_SCALER_BASE 0x20000
> > +#define DE3_VI_SCALER_SIZE 0x08000
> > +
> > 
> >  /* this two macros assumes 16 fractional bits which is standard in DRM */
> >  #define SUN8I_VI_SCALER_SCALE_MIN              1
> >  #define SUN8I_VI_SCALER_SCALE_MAX              ((1UL << 20) - 1)
> > 
> > @@ -25,6 +28,11 @@
> > 
> >  #define SUN8I_VI_SCALER_SIZE(w, h)             (((h) - 1) << 16 | ((w) -
> >  1))
> >  
> >  #define SUN8I_SCALER_VSU_CTRL(base)            ((base) + 0x0)
> > 
> > +#define SUN8I_SCALER_VSU_SCALE_MODE(base)      ((base) + 0x10)
> > +#define SUN8I_SCALER_VSU_DIR_THR(base)         ((base) + 0x20)
> > +#define SUN8I_SCALER_VSU_EDGE_THR(base)                ((base) + 0x24)
> > +#define SUN8I_SCALER_VSU_EDSCL_CTRL(base)      ((base) + 0x28)
> > +#define SUN8I_SCALER_VSU_ANGLE_THR(base)       ((base) + 0x2c)
> 
> DE3 prefix for the new ones please.
> 
> >  #define SUN8I_SCALER_VSU_OUTSIZE(base)         ((base) + 0x40)
> >  #define SUN8I_SCALER_VSU_YINSIZE(base)         ((base) + 0x80)
> >  #define SUN8I_SCALER_VSU_YHSTEP(base)          ((base) + 0x88)
> > 
> > @@ -46,6 +54,21 @@
> > 
> >  #define SUN8I_SCALER_VSU_CTRL_EN               BIT(0)
> >  #define SUN8I_SCALER_VSU_CTRL_COEFF_RDY                BIT(4)
> > 
> > +#define SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(x)   (((x) << 24) & 0xFF)
> > +#define SUN8I_SCALER_VSU_ZERO_DIR_THR(x)       (((x) << 16) & 0xFF)
> > +#define SUN8I_SCALER_VSU_HORZ_DIR_THR(x)       (((x) << 8) & 0xFF)
> > +#define SUN8I_SCALER_VSU_VERT_DIR_THR(x)       ((x) & 0xFF)
> > +
> > +#define SUN8I_SCALER_VSU_SCALE_MODE_UI         0
> > +#define SUN8I_SCALER_VSU_SCALE_MODE_NORMAL     1
> > +#define SUN8I_SCALER_VSU_SCALE_MODE_ED_SCALE   2
> > +
> > +#define SUN8I_SCALER_VSU_EDGE_SHIFT(x)         (((x) << 16) & 0xF)
> > +#define SUN8I_SCALER_VSU_EDGE_OFFSET(x)                ((x) & 0xFF)
> > +
> > +#define SUN8I_SCALER_VSU_ANGLE_SHIFT(x)                (((x) << 16) &
> > 0xF)
> > +#define SUN8I_SCALER_VSU_ANGLE_OFFSET(x)       ((x) & 0xFF)
> > +
> 
> Same here.
> 
> Regards
> ChenYu
> 
> >  void sun8i_vi_scaler_enable(struct sun8i_mixer *mixer, int layer, bool
> >  enable); void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer, int
> >  layer,
> >  
> >                            u32 src_w, u32 src_h, u32 dst_w, u32 dst_h,
> > 
> > --
> > 2.18.0

Chen-Yu Tsai Oct. 6, 2018, 3:34 p.m. UTC | #5

On Sat, Oct 6, 2018 at 1:51 AM Jernej Škrabec <jernej.skrabec@siol.net> wrote:
>
> Dne sobota, 22. september 2018 ob 15:19:12 CEST je Chen-Yu Tsai napisal(a):
> > On Sun, Sep 2, 2018 at 3:27 PM Jernej Skrabec <jernej.skrabec@siol.net>
> wrote:
> > > Display Engine 3 is an upgrade of DE2 with new features like support for
> > > 10 bit color formats and support for AFBC.
> > >
> > > Most of DE2 code works with DE3, except some small details.
> > >
> > > Add support for it.
> > >
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > ---
> > >
> > >  drivers/gpu/drm/sun4i/sun8i_csc.c       | 96 +++++++++++++++++++++++--
> > >  drivers/gpu/drm/sun4i/sun8i_mixer.c     | 17 ++++-
> > >  drivers/gpu/drm/sun4i/sun8i_mixer.h     | 21 +++++-
> > >  drivers/gpu/drm/sun4i/sun8i_ui_scaler.c |  8 ++-
> > >  drivers/gpu/drm/sun4i/sun8i_ui_scaler.h |  1 +
> > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c  |  8 +++
> > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.h  |  2 +
> > >  drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 34 ++++++++-
> > >  drivers/gpu/drm/sun4i/sun8i_vi_scaler.h | 23 ++++++
> > >  9 files changed, 197 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_csc.c
> > > b/drivers/gpu/drm/sun4i/sun8i_csc.c index b14925b40ccf..101901ccf2dc
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_csc.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_csc.c
> > > @@ -34,6 +34,34 @@ static const u32 yvu2rgb[] = {
> > >
> > >         0x000004A8, 0x00000000, 0x00000813, 0xFFFBAC4A,
> > >
> > >  };
> > >
> > > +/*
> > > + * DE3 has a bit different CSC units. Factors are in two's complement
> > > format. + * First three have 17 bits for fractinal part and last two 2
> > > bits. First + * three values in each line are multiplication factor, 4th
> > > is difference, + * which is subtracted from the input value before the
> > > multiplication and + * last value is constant, which is added at the end.
> > > + *
> > > + * x' = c00 * (x + d0) + c01 * (y + d1) + c02 * (z + d2) + const0
> > > + * y' = c10 * (x + d0) + c11 * (y + d1) + c12 * (z + d2) + const1
> > > + * z' = c20 * (x + d0) + c21 * (y + d1) + c22 * (z + d2) + const2
> > > + *
> > > + * Please note that above formula is true only for Blender CSC. Other DE3
> > > CSC + * units takes only positive value for difference. From what can be
> > > deducted + * from BSP driver code, those units probably automatically
> > > assume that + * difference has to be subtracted.
> > > + */
> > > +static const u32 yuv2rgb_de3[] = {
> > > +       0x0002542a, 0x00000000, 0x0003312a, 0xffffffc0, 0x00000000,
> > > +       0x0002542a, 0xffff376b, 0xfffe5fc3, 0xfffffe00, 0x00000000,
> > > +       0x0002542a, 0x000408d3, 0x00000000, 0xfffffe00, 0x00000000,
> > > +};
> > > +
> > > +static const u32 yvu2rgb_de3[] = {
> > > +       0x0002542a, 0x0003312a, 0x00000000, 0xffffffc0, 0x00000000,
> > > +       0x0002542a, 0xfffe5fc3, 0xffff376b, 0xfffffe00, 0x00000000,
> > > +       0x0002542a, 0x00000000, 0x000408d3, 0xfffffe00, 0x00000000,
> > > +};
> > > +
> > >
> > >  static void sun8i_csc_set_coefficients(struct regmap *map, u32 base,
> > >
> > >                                        enum sun8i_csc_mode mode)
> > >
> > >  {
> > >
> > > @@ -61,6 +89,38 @@ static void sun8i_csc_set_coefficients(struct regmap
> > > *map, u32 base,>
> > >         }
> > >
> > >  }
> > >
> > > +static void sun8i_de3_ccsc_set_coefficients(struct regmap *map, int
> > > layer,
> > > +                                           enum sun8i_csc_mode mode)
> > > +{
> > > +       const u32 *table;
> > > +       int i, j;
> > > +
> > > +       switch (mode) {
> > > +       case SUN8I_CSC_MODE_YUV2RGB:
> > > +               table = yuv2rgb_de3;
> > > +               break;
> > > +       case SUN8I_CSC_MODE_YVU2RGB:
> > > +               table = yvu2rgb_de3;
> > > +               break;
> > > +       default:
> > > +               DRM_WARN("Wrong CSC mode specified.\n");
> > > +               return;
> > > +       }
> > > +
> > > +       for (i = 0; i < 3; i++) {
> > > +               for (j = 0; j < 3; j++)
> > > +                       regmap_write(map,
> > > +
> > > SUN8I_MIXER_BLEND_CSC_COEFF(DE3_BLD_BASE, +
> > >                                  layer, i, j), +
> > >           table[i * 5 + j]);
> >
> > Given that the first three values occupy contiguous addresses,
> > you can use regmap_bulk_write() here.
>
> Do you mind if I rework table in a way that directly fits in registers and use
> regmap_bulk_write() on whole table? This means I must combine two values into
> one for "const" register, but I would explain it better in a comment. It would
> be most elegant solution, one call to regmap_bulk_write() instead of two for
> loops and some arithmetics.

With proper comments I see no problem with it. The constants are hexidecimal
and only loaded once. It's unlikely others would use it any other way.

> I guess macro for "const" part can stay as documentation.

I agree.

>
> >
> > > +               regmap_write(map,
> > > +                            SUN8I_MIXER_BLEND_CSC_CONST(DE3_BLD_BASE,
> > > +                                                        layer, i),
> > > +                            SUN8I_MIXER_BLEND_CSC_CONST_VAL(table[i * 5 +
> > > 3], +                                                            table[i
> > > * 5 + 4]));
> > Nit: Using a two-dimension array might make it easier to read.
> >
> > > +       }
> > > +}
> > > +
> > >
> > >  static void sun8i_csc_enable(struct regmap *map, u32 base, bool enable)
> > >  {
> > >
> > >         u32 val;
> > >
> > > @@ -73,21 +133,45 @@ static void sun8i_csc_enable(struct regmap *map, u32
> > > base, bool enable)>
> > >         regmap_update_bits(map, SUN8I_CSC_CTRL(base), SUN8I_CSC_CTRL_EN,
> > >         val);
> > >
> > >  }
> > >
> > > +static void sun8i_de3_ccsc_enable(struct regmap *map, int layer, bool
> > > enable) +{
> > > +       u32 val, mask;
> > > +
> > > +       mask = SUN8I_MIXER_BLEND_CSC_CTL_EN(layer);
> > > +
> > > +       if (enable)
> > > +               val = mask;
> > > +       else
> > > +               val = 0;
> > > +
> > > +       regmap_update_bits(map, SUN8I_MIXER_BLEND_CSC_CTL(DE3_BLD_BASE),
> > > +                          mask, val);
> > > +}
> > > +
> > >
> > >  void sun8i_csc_set_ccsc_coefficients(struct sun8i_mixer *mixer, int
> > >  layer,
> > >
> > >                                      enum sun8i_csc_mode mode)
> > >
> > >  {
> > >
> > > -       u32 base;
> > > +       if (!mixer->cfg->is_de3) {
> > > +               u32 base;
> >
> > You could rewrite this as
> >
> >     if (mixer->cfg->is_de3) {
> >                sun8i_de3_ccsc_set_coefficients(mixer->engine.regs,
> >                                                layer, mode);
> >                return;
> >     }
> >
> > That way you don't need to change the indentation on the existing lines.
> > I suppose this is more of a personal preference. The downside is the control
> > flow is slightly more complicated.
> >
> > > -       base = ccsc_base[mixer->cfg->ccsc][layer];
> > > +               base = ccsc_base[mixer->cfg->ccsc][layer];
> > >
> > > -       sun8i_csc_set_coefficients(mixer->engine.regs, base, mode);
> > > +               sun8i_csc_set_coefficients(mixer->engine.regs, base,
> > > mode);
> > > +       } else {
> > > +               sun8i_de3_ccsc_set_coefficients(mixer->engine.regs,
> > > +                                               layer, mode);
> > > +       }
> > >
> > >  }
> > >
> > >  void sun8i_csc_enable_ccsc(struct sun8i_mixer *mixer, int layer, bool
> > >  enable) {
> > >
> > > -       u32 base;
> > > +       if (!mixer->cfg->is_de3) {
> > > +               u32 base;
> > >
> > > -       base = ccsc_base[mixer->cfg->ccsc][layer];
> > > +               base = ccsc_base[mixer->cfg->ccsc][layer];
> > >
> > > -       sun8i_csc_enable(mixer->engine.regs, base, enable);
> > > +               sun8i_csc_enable(mixer->engine.regs, base, enable);
> > > +       } else {
> > > +               sun8i_de3_ccsc_enable(mixer->engine.regs, layer, enable);
> > > +       }
> > >
> > >  }
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index 743941a33d88..a9218abf0935
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> > > @@ -460,8 +460,21 @@ static int sun8i_mixer_bind(struct device *dev,
> > > struct device *master,>
> > >         base = sun8i_blender_base(mixer);
> > >
> > >         /* Reset the registers */
> > >
> > > -       for (i = 0x0; i < 0x20000; i += 4)
> > > -               regmap_write(mixer->engine.regs, i, 0);
> > > +       if (mixer->cfg->is_de3) {
> > > +               for (i = 0x0; i < 0x3000; i += 4)
> > > +                       regmap_write(mixer->engine.regs, i, 0);
> > > +               for (i = 0x20000; i < 0x40000; i += 4)
> > > +                       regmap_write(mixer->engine.regs, i, 0);
> > > +               for (i = 0x70000; i < 0x88000; i += 4)
> > > +                       regmap_write(mixer->engine.regs, i, 0);
> > > +               for (i = 0xa0000; i < 0xb0000; i += 4)
> > > +                       regmap_write(mixer->engine.regs, i, 0);
> > > +               for (i = 0xd0000; i < 0xe0000; i += 4)
> > > +                       regmap_write(mixer->engine.regs, i, 0);
> >
> > Could you use the macros *_BASE and *_SIZE here? That would make
> > it more obviously what parts you're clearing.
> >
> > The last offset, 0xd0000, isn't even defined in the DE3 spec.
> >
> > > +       } else {
> > > +               for (i = 0x0; i < 0x20000; i += 4)
> > > +                       regmap_write(mixer->engine.regs, i, 0);
> > > +       }
> > >
> > >         /* Enable the mixer */
> > >         regmap_write(mixer->engine.regs, SUN8I_MIXER_GLOBAL_CTL,
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 020b0a097c84..4c9a442bbb44
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > @@ -33,6 +33,10 @@
> > >
> > >  #define DE2_CH_BASE                            0x2000
> > >  #define DE2_CH_SIZE                            0x1000
> > >
> > > +#define DE3_BLD_BASE                           0x0800
> > > +#define DE3_CH_BASE                            0x1000
> > > +#define DE3_CH_SIZE                            0x0800
> > > +
> > >
> > >  #define SUN8I_MIXER_BLEND_PIPE_CTL(base)       ((base) + 0)
> > >  #define SUN8I_MIXER_BLEND_ATTR_FCOLOR(base, x) ((base) + 0x4 + 0x10 *
> > >  (x))
> > >  #define SUN8I_MIXER_BLEND_ATTR_INSIZE(base, x) ((base) + 0x8 + 0x10 *
> > >  (x))
> > >
> > > @@ -47,6 +51,11 @@
> > >
> > >  #define SUN8I_MIXER_BLEND_CK_MAX(base, x)      ((base) + 0xc0 + 0x04 *
> > >  (x))
> > >  #define SUN8I_MIXER_BLEND_CK_MIN(base, x)      ((base) + 0xe0 + 0x04 *
> > >  (x))
> > >  #define SUN8I_MIXER_BLEND_OUTCTL(base)         ((base) + 0xfc)
> > >
> > > +#define SUN8I_MIXER_BLEND_CSC_CTL(base)                ((base) + 0x100)
> > > +#define SUN8I_MIXER_BLEND_CSC_COEFF(base, layer, x, y) \
> > > +       ((base) + 0x110 + (layer) * 0x30 +  (x) * 0x10 + 4 * (y))
> > > +#define SUN8I_MIXER_BLEND_CSC_CONST(base, layer, i) \
> > > +       ((base) + 0x110 + (layer) * 0x30 +  (i) * 0x10 + 0x0c)
> >
> > Should these have a DE3 prefix?
>
> How do you want this to look like?
>
> SUN50I_DE3_MIXER_BLEND_CSC_CONST?
>
> SUN8I prefix doesn't fit for DE3, but as usual, there is a chance that AW
> releases 32 bit SoC with DE3.
>
> At the end, it would be better to rename everything to have just DE2 or DE3
> prefix without mentioning any SoC family. After all, DE2 is also supported on
> sun50i family, which makes any family name in DE2/3 macros confusing...

Probably SUNXI_DEx_MIXER_foo or just DEx_MIXER_foo ...
Since it's contained in the driver I think the latter would suffice.

> Do you think I can squeeze dropping SUN8I prefix to a previous patch, where
> macros are reworked anyway?

Given there's a lot of them, and you're only changing about half of them
in that patch, I suggest you have a separate patch to do all the renaming.

ChenYu

> Best regards,
> Jernej
>
> >
> > >  #define SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK      GENMASK(12, 8)
> > >  #define SUN8I_MIXER_BLEND_PIPE_CTL_EN(pipe)    BIT(8 + pipe)
> > >
> > > @@ -61,6 +70,9 @@
> > >
> > >  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED    BIT(1)
> > >
> > > +#define SUN8I_MIXER_BLEND_CSC_CTL_EN(ch)       BIT(ch)
> > > +#define SUN8I_MIXER_BLEND_CSC_CONST_VAL(d, c)  (((d) << 16) | ((c) &
> > > 0xffff)) +
> >
> > Same here.
> >
> > >  #define SUN8I_MIXER_FBFMT_ARGB8888     0
> > >  #define SUN8I_MIXER_FBFMT_ABGR8888     1
> > >  #define SUN8I_MIXER_FBFMT_RGBA8888     2
> >
> > [...]
> >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 46f0237c17bb..3d0ad64178ea
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > @@ -30,6 +30,8 @@
> > >
> > >  #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_RGB_MODE                BIT(15)
> > >  #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_OFFSET    8
> > >  #define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_MASK      GENMASK(12, 8)
> > >
> > > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK      GENMASK(31, 24)
> > > +#define SUN8I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x)                ((x) <<
> > > 24)
> >
> > DE3 prefix?
> >
> > >  struct sun8i_mixer;
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c
> > > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c index
> > > 8697afc36023..22e1ed5cd7a4 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.c
> > > @@ -835,7 +835,10 @@ static const u32 bicubic4coefftab32[480] = {
> > >
> > >  static inline u32 sun8i_vi_scaler_base(struct sun8i_mixer *mixer, int
> > >  channel) {
> > >
> > > -       return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel;
> > > +       if (mixer->cfg->is_de3)
> > > +               return DE3_VI_SCALER_BASE + DE3_VI_SCALER_SIZE * channel;
> > > +       else
> > > +               return DE2_VI_SCALER_BASE + DE2_VI_SCALER_SIZE * channel;
> > >
> > >  }
> > >
> > >  static int sun8i_vi_scaler_coef_index(unsigned int step)
> > >
> > > @@ -951,6 +954,35 @@ void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer,
> > > int layer,>
> > >                 cvphase = vphase;
> > >
> > >         }
> > >
> > > +       if (mixer->cfg->is_de3) {
> > > +               u32 val;
> > > +
> > > +               if (format->hsub == 1 && format->vsub == 1)
> > > +                       val = SUN8I_SCALER_VSU_SCALE_MODE_UI;
> > > +               else
> > > +                       val = SUN8I_SCALER_VSU_SCALE_MODE_NORMAL;
> > > +
> > > +               regmap_write(mixer->engine.regs,
> > > +                            SUN8I_SCALER_VSU_SCALE_MODE(base), val);
> >
> > The remaining settings seem to be related to the edge detection scaling
> > method. Since you aren't supporting it, are they necessary?
> >
> > > +               regmap_write(mixer->engine.regs,
> > > +                            SUN8I_SCALER_VSU_DIR_THR(base),
> > > +                            SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(0) |
> > > +                            SUN8I_SCALER_VSU_ZERO_DIR_THR(0) |
> > > +                            SUN8I_SCALER_VSU_HORZ_DIR_THR(0xff) |
> > > +                            SUN8I_SCALER_VSU_VERT_DIR_THR(1));
> > > +               regmap_write(mixer->engine.regs,
> > > +                            SUN8I_SCALER_VSU_EDGE_THR(base),
> > > +                            SUN8I_SCALER_VSU_EDGE_SHIFT(8) |
> > > +                            SUN8I_SCALER_VSU_EDGE_OFFSET(0));
> > > +               regmap_write(mixer->engine.regs,
> > > +                            SUN8I_SCALER_VSU_EDSCL_CTRL(base), 0);
> > > +               regmap_write(mixer->engine.regs,
> > > +                            SUN8I_SCALER_VSU_ANGLE_THR(base),
> > > +                            SUN8I_SCALER_VSU_ANGLE_SHIFT(2) |
> > > +                            SUN8I_SCALER_VSU_ANGLE_OFFSET(0));
> > > +       }
> > > +
> > >
> > >         regmap_write(mixer->engine.regs,
> > >
> > >                      SUN8I_SCALER_VSU_OUTSIZE(base), outsize);
> > >
> > >         regmap_write(mixer->engine.regs,
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h
> > > b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h index
> > > cd015405f66d..c322f5652481 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_scaler.h
> > > @@ -15,6 +15,9 @@
> > >
> > >  #define DE2_VI_SCALER_BASE 0x20000
> > >  #define DE2_VI_SCALER_SIZE 0x20000
> > >
> > > +#define DE3_VI_SCALER_BASE 0x20000
> > > +#define DE3_VI_SCALER_SIZE 0x08000
> > > +
> > >
> > >  /* this two macros assumes 16 fractional bits which is standard in DRM */
> > >  #define SUN8I_VI_SCALER_SCALE_MIN              1
> > >  #define SUN8I_VI_SCALER_SCALE_MAX              ((1UL << 20) - 1)
> > >
> > > @@ -25,6 +28,11 @@
> > >
> > >  #define SUN8I_VI_SCALER_SIZE(w, h)             (((h) - 1) << 16 | ((w) -
> > >  1))
> > >
> > >  #define SUN8I_SCALER_VSU_CTRL(base)            ((base) + 0x0)
> > >
> > > +#define SUN8I_SCALER_VSU_SCALE_MODE(base)      ((base) + 0x10)
> > > +#define SUN8I_SCALER_VSU_DIR_THR(base)         ((base) + 0x20)
> > > +#define SUN8I_SCALER_VSU_EDGE_THR(base)                ((base) + 0x24)
> > > +#define SUN8I_SCALER_VSU_EDSCL_CTRL(base)      ((base) + 0x28)
> > > +#define SUN8I_SCALER_VSU_ANGLE_THR(base)       ((base) + 0x2c)
> >
> > DE3 prefix for the new ones please.
> >
> > >  #define SUN8I_SCALER_VSU_OUTSIZE(base)         ((base) + 0x40)
> > >  #define SUN8I_SCALER_VSU_YINSIZE(base)         ((base) + 0x80)
> > >  #define SUN8I_SCALER_VSU_YHSTEP(base)          ((base) + 0x88)
> > >
> > > @@ -46,6 +54,21 @@
> > >
> > >  #define SUN8I_SCALER_VSU_CTRL_EN               BIT(0)
> > >  #define SUN8I_SCALER_VSU_CTRL_COEFF_RDY                BIT(4)
> > >
> > > +#define SUN8I_SCALER_VSU_SUB_ZERO_DIR_THR(x)   (((x) << 24) & 0xFF)
> > > +#define SUN8I_SCALER_VSU_ZERO_DIR_THR(x)       (((x) << 16) & 0xFF)
> > > +#define SUN8I_SCALER_VSU_HORZ_DIR_THR(x)       (((x) << 8) & 0xFF)
> > > +#define SUN8I_SCALER_VSU_VERT_DIR_THR(x)       ((x) & 0xFF)
> > > +
> > > +#define SUN8I_SCALER_VSU_SCALE_MODE_UI         0
> > > +#define SUN8I_SCALER_VSU_SCALE_MODE_NORMAL     1
> > > +#define SUN8I_SCALER_VSU_SCALE_MODE_ED_SCALE   2
> > > +
> > > +#define SUN8I_SCALER_VSU_EDGE_SHIFT(x)         (((x) << 16) & 0xF)
> > > +#define SUN8I_SCALER_VSU_EDGE_OFFSET(x)                ((x) & 0xFF)
> > > +
> > > +#define SUN8I_SCALER_VSU_ANGLE_SHIFT(x)                (((x) << 16) &
> > > 0xF)
> > > +#define SUN8I_SCALER_VSU_ANGLE_OFFSET(x)       ((x) & 0xFF)
> > > +
> >
> > Same here.
> >
> > Regards
> > ChenYu
> >
> > >  void sun8i_vi_scaler_enable(struct sun8i_mixer *mixer, int layer, bool
> > >  enable); void sun8i_vi_scaler_setup(struct sun8i_mixer *mixer, int
> > >  layer,
> > >
> > >                            u32 src_w, u32 src_h, u32 dst_w, u32 dst_h,
> > >
> > > --
> > > 2.18.0
>
>
>
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[12/27] drm/sun4i: Add basic support for DE3

Commit Message

Comments

Patch