Re: [PATCH 02/12] drm/msm/dpu: add dpu_hw_wb abstraction for writeback blocks

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 05/02/2022 00:17, Abhinav Kumar wrote:
> Add the dpu_hw_wb abstraction to program registers related to the
> writeback block. These will be invoked once all the configuration
> is set and ready to be programmed to the registers.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Few minor comments bellow.

> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/Makefile              |   1 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 267 ++++++++++++++++++++++++++++++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h | 145 ++++++++++++++++
>   3 files changed, 413 insertions(+)
>   create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
>   create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h
> 
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index 03ab55c..c43ef35 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -66,6 +66,7 @@ msm-y := \
>   	disp/dpu1/dpu_hw_top.o \
>   	disp/dpu1/dpu_hw_util.o \
>   	disp/dpu1/dpu_hw_vbif.o \
> +	disp/dpu1/dpu_hw_wb.o \
>   	disp/dpu1/dpu_io_util.o \
>   	disp/dpu1/dpu_kms.o \
>   	disp/dpu1/dpu_mdss.o \
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
> new file mode 100644
> index 0000000..d395475
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c
> @@ -0,0 +1,267 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> + /*
> +  * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved
> +  */
> +
> +#include "dpu_hw_mdss.h"
> +#include "dpu_hwio.h"
> +#include "dpu_hw_catalog.h"
> +#include "dpu_hw_wb.h"
> +#include "dpu_formats.h"
> +#include "dpu_kms.h"
> +
> +#define WB_DST_FORMAT                         0x000
> +#define WB_DST_OP_MODE                        0x004
> +#define WB_DST_PACK_PATTERN                   0x008
> +#define WB_DST0_ADDR                          0x00C
> +#define WB_DST1_ADDR                          0x010
> +#define WB_DST2_ADDR                          0x014
> +#define WB_DST3_ADDR                          0x018
> +#define WB_DST_YSTRIDE0                       0x01C
> +#define WB_DST_YSTRIDE1                       0x020
> +#define WB_DST_YSTRIDE1                       0x020
> +#define WB_DST_DITHER_BITDEPTH                0x024
> +#define WB_DST_MATRIX_ROW0                    0x030
> +#define WB_DST_MATRIX_ROW1                    0x034
> +#define WB_DST_MATRIX_ROW2                    0x038
> +#define WB_DST_MATRIX_ROW3                    0x03C
> +#define WB_DST_WRITE_CONFIG                   0x048
> +#define WB_ROTATION_DNSCALER                  0x050
> +#define WB_ROTATOR_PIPE_DOWNSCALER            0x054
> +#define WB_N16_INIT_PHASE_X_C03               0x060
> +#define WB_N16_INIT_PHASE_X_C12               0x064
> +#define WB_N16_INIT_PHASE_Y_C03               0x068
> +#define WB_N16_INIT_PHASE_Y_C12               0x06C
> +#define WB_OUT_SIZE                           0x074
> +#define WB_ALPHA_X_VALUE                      0x078
> +#define WB_DANGER_LUT                         0x084
> +#define WB_SAFE_LUT                           0x088
> +#define WB_QOS_CTRL                           0x090
> +#define WB_CREQ_LUT_0                         0x098
> +#define WB_CREQ_LUT_1                         0x09C
> +#define WB_UBWC_STATIC_CTRL                   0x144
> +#define WB_MUX                                0x150
> +#define WB_CROP_CTRL                          0x154
> +#define WB_CROP_OFFSET                        0x158
> +#define WB_CSC_BASE                           0x260
> +#define WB_DST_ADDR_SW_STATUS                 0x2B0
> +#define WB_CDP_CNTL                           0x2B4
> +#define WB_OUT_IMAGE_SIZE                     0x2C0
> +#define WB_OUT_XY                             0x2C4
> +
> +/* WB_QOS_CTRL */
> +#define WB_QOS_CTRL_DANGER_SAFE_EN            BIT(0)
> +
> +static const struct dpu_wb_cfg *_wb_offset(enum dpu_wb wb,
> +		const struct dpu_mdss_cfg *m, void __iomem *addr,
> +		struct dpu_hw_blk_reg_map *b)
> +{
> +	int i;
> +
> +	for (i = 0; i < m->wb_count; i++) {
> +		if (wb == m->wb[i].id) {
> +			b->base_off = addr;
> +			b->blk_off = m->wb[i].base;
> +			b->length = m->wb[i].len;
> +			b->hwversion = m->hwversion;
> +			return &m->wb[i];
> +		}
> +	}
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +static void dpu_hw_wb_setup_outaddress(struct dpu_hw_wb *ctx,
> +		struct dpu_hw_wb_cfg *data)
> +{
> +	struct dpu_hw_blk_reg_map *c = &ctx->hw;
> +
> +	DPU_REG_WRITE(c, WB_DST0_ADDR, data->dest.plane_addr[0]);
> +	DPU_REG_WRITE(c, WB_DST1_ADDR, data->dest.plane_addr[1]);
> +	DPU_REG_WRITE(c, WB_DST2_ADDR, data->dest.plane_addr[2]);
> +	DPU_REG_WRITE(c, WB_DST3_ADDR, data->dest.plane_addr[3]);
> +}
> +
> +static void dpu_hw_wb_setup_format(struct dpu_hw_wb *ctx,
> +		struct dpu_hw_wb_cfg *data)
> +{
> +	struct dpu_hw_blk_reg_map *c = &ctx->hw;
> +	const struct dpu_format *fmt = data->dest.format;
> +	u32 dst_format, pattern, ystride0, ystride1, outsize, chroma_samp;
> +	u32 write_config = 0;
> +	u32 opmode = 0;
> +	u32 dst_addr_sw = 0;
> +
> +	chroma_samp = fmt->chroma_sample;
> +
> +	dst_format = (chroma_samp << 23) |
> +		(fmt->fetch_planes << 19) |
> +		(fmt->bits[C3_ALPHA] << 6) |
> +		(fmt->bits[C2_R_Cr] << 4) |
> +		(fmt->bits[C1_B_Cb] << 2) |
> +		(fmt->bits[C0_G_Y] << 0);
> +
> +	if (fmt->bits[C3_ALPHA] || fmt->alpha_enable) {
> +		dst_format |= BIT(8); /* DSTC3_EN */
> +		if (!fmt->alpha_enable ||
> +			!(ctx->caps->features & BIT(DPU_WB_PIPE_ALPHA)))
> +			dst_format |= BIT(14); /* DST_ALPHA_X */
> +	}
> +
> +	pattern = (fmt->element[3] << 24) |
> +		(fmt->element[2] << 16) |
> +		(fmt->element[1] << 8)  |
> +		(fmt->element[0] << 0);
> +
> +	dst_format |= (fmt->unpack_align_msb << 18) |
> +		(fmt->unpack_tight << 17) |
> +		((fmt->unpack_count - 1) << 12) |
> +		((fmt->bpp - 1) << 9);
> +
> +	ystride0 = data->dest.plane_pitch[0] |
> +		(data->dest.plane_pitch[1] << 16);
> +	ystride1 = data->dest.plane_pitch[2] |
> +	(data->dest.plane_pitch[3] << 16);
> +
> +	if (drm_rect_height(&data->roi) && drm_rect_width(&data->roi))
> +		outsize = (drm_rect_height(&data->roi) << 16) | drm_rect_width(&data->roi);
> +	else
> +		outsize = (data->dest.height << 16) | data->dest.width;
> +
> +	DPU_REG_WRITE(c, WB_ALPHA_X_VALUE, 0xFF);
> +	DPU_REG_WRITE(c, WB_DST_FORMAT, dst_format);
> +	DPU_REG_WRITE(c, WB_DST_OP_MODE, opmode);
> +	DPU_REG_WRITE(c, WB_DST_PACK_PATTERN, pattern);
> +	DPU_REG_WRITE(c, WB_DST_YSTRIDE0, ystride0);
> +	DPU_REG_WRITE(c, WB_DST_YSTRIDE1, ystride1);
> +	DPU_REG_WRITE(c, WB_OUT_SIZE, outsize);
> +	DPU_REG_WRITE(c, WB_DST_WRITE_CONFIG, write_config);
> +	DPU_REG_WRITE(c, WB_DST_ADDR_SW_STATUS, dst_addr_sw);
> +}
> +
> +static void dpu_hw_wb_roi(struct dpu_hw_wb *ctx, struct dpu_hw_wb_cfg *wb)
> +{
> +	struct dpu_hw_blk_reg_map *c = &ctx->hw;
> +	u32 image_size, out_size, out_xy;
> +
> +	image_size = (wb->dest.height << 16) | wb->dest.width;
> +	out_xy = 0;
> +	out_size = (drm_rect_height(&wb->roi) << 16) | drm_rect_width(&wb->roi);
> +
> +	DPU_REG_WRITE(c, WB_OUT_IMAGE_SIZE, image_size);
> +	DPU_REG_WRITE(c, WB_OUT_XY, out_xy);
> +	DPU_REG_WRITE(c, WB_OUT_SIZE, out_size);
> +}
> +
> +static void dpu_hw_wb_setup_qos_lut(struct dpu_hw_wb *ctx,
> +		struct dpu_hw_wb_qos_cfg *cfg)
> +{
> +	struct dpu_hw_blk_reg_map *c = &ctx->hw;
> +	u32 qos_ctrl = 0;
> +
> +	if (!ctx || !cfg)
> +		return;
> +
> +	DPU_REG_WRITE(c, WB_DANGER_LUT, cfg->danger_lut);
> +	DPU_REG_WRITE(c, WB_SAFE_LUT, cfg->safe_lut);
> +
> +	if (ctx->caps && test_bit(DPU_WB_QOS_8LVL, &ctx->caps->features)) {
> +		DPU_REG_WRITE(c, WB_CREQ_LUT_0, cfg->creq_lut);
> +		DPU_REG_WRITE(c, WB_CREQ_LUT_1, cfg->creq_lut >> 32);
> +	}
> +
> +	if (cfg->danger_safe_en)
> +		qos_ctrl |= WB_QOS_CTRL_DANGER_SAFE_EN;
> +
> +	DPU_REG_WRITE(c, WB_QOS_CTRL, qos_ctrl);
> +}
> +
> +static void dpu_hw_wb_setup_cdp(struct dpu_hw_wb *ctx,
> +		struct dpu_hw_wb_cdp_cfg *cfg)
> +{
> +	struct dpu_hw_blk_reg_map *c;
> +	u32 cdp_cntl = 0;
> +
> +	if (!ctx || !cfg)
> +		return;
> +
> +	c = &ctx->hw;
> +
> +	if (cfg->enable)
> +		cdp_cntl |= BIT(0);
> +	if (cfg->ubwc_meta_enable)
> +		cdp_cntl |= BIT(1);
> +	if (cfg->preload_ahead == DPU_WB_CDP_PRELOAD_AHEAD_64)
> +		cdp_cntl |= BIT(3);
> +
> +	DPU_REG_WRITE(c, WB_CDP_CNTL, cdp_cntl);
> +}

I thought for a moment if we can unify these several functions with SSPP 
code. Most probably we can, but I'm not sure that it make sense.

> +
> +static void dpu_hw_wb_bind_pingpong_blk(
> +		struct dpu_hw_wb *ctx,
> +		bool enable, const enum dpu_pingpong pp)
> +{
> +	struct dpu_hw_blk_reg_map *c;
> +	int mux_cfg = 0xF;
> +
> +	if (!ctx)
> +		return;
> +
> +	c = &ctx->hw;
> +	if (enable)
> +		mux_cfg = (pp - PINGPONG_0) & 0x7;

Just noticed that dpu_hw_intf_bind_pingpong_blk() keeps higher bits 
rather than setting them to 0. Which policy should we follow here?

> +
> +	DPU_REG_WRITE(c, WB_MUX, mux_cfg);
> +}
> +
> +static void _setup_wb_ops(struct dpu_hw_wb_ops *ops,
> +		unsigned long features)
> +{
> +	ops->setup_outaddress = dpu_hw_wb_setup_outaddress;
> +	ops->setup_outformat = dpu_hw_wb_setup_format;
> +
> +	if (test_bit(DPU_WB_XY_ROI_OFFSET, &features))
> +		ops->setup_roi = dpu_hw_wb_roi;
> +
> +	if (test_bit(DPU_WB_QOS, &features))
> +		ops->setup_qos_lut = dpu_hw_wb_setup_qos_lut;
> +
> +	if (test_bit(DPU_WB_CDP, &features))
> +		ops->setup_cdp = dpu_hw_wb_setup_cdp;
> +
> +	if (test_bit(DPU_WB_INPUT_CTRL, &features))
> +		ops->bind_pingpong_blk = dpu_hw_wb_bind_pingpong_blk;
> +}
> +
> +struct dpu_hw_wb *dpu_hw_wb_init(enum dpu_wb idx,
> +		void __iomem *addr, const struct dpu_mdss_cfg *m)
> +{
> +	struct dpu_hw_wb *c;
> +	const struct dpu_wb_cfg *cfg;
> +
> +	if (!addr || !m)
> +		return ERR_PTR(-EINVAL);
> +
> +	c = kzalloc(sizeof(*c), GFP_KERNEL);
> +	if (!c)
> +		return ERR_PTR(-ENOMEM);
> +
> +	cfg = _wb_offset(idx, m, addr, &c->hw);
> +	if (IS_ERR(cfg)) {
> +		WARN(1, "Unable to find wb idx=%d\n", idx);
> +		kfree(c);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	/* Assign ops */
> +	c->mdp = &m->mdp[0];
> +	c->idx = idx;
> +	c->caps = cfg;
> +	_setup_wb_ops(&c->ops, c->caps->features);
> +
> +	return c;
> +}
> +
> +void dpu_hw_wb_destroy(struct dpu_hw_wb *hw_wb)
> +{
> +	kfree(hw_wb);
> +}
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h
> new file mode 100644
> index 0000000..39d745f
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h
> @@ -0,0 +1,145 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved
> + */
> +
> +#ifndef _DPU_HW_WB_H
> +#define _DPU_HW_WB_H
> +
> +#include "dpu_hw_catalog.h"
> +#include "dpu_hw_mdss.h"
> +#include "dpu_hw_top.h"
> +#include "dpu_hw_util.h"
> +#include "dpu_hw_pingpong.h"
> +
> +struct dpu_hw_wb;
> +
> +struct dpu_hw_wb_cfg {
> +	struct dpu_hw_fmt_layout dest;
> +	enum dpu_intf_mode intf_mode;
> +	struct drm_rect roi;
> +	struct drm_rect crop;
> +};
> +
> +/**
> + * enum CDP preload ahead address size
> + */
> +enum {
> +	DPU_WB_CDP_PRELOAD_AHEAD_32,
> +	DPU_WB_CDP_PRELOAD_AHEAD_64
> +};
> +
> +/**
> + * struct dpu_hw_wb_cdp_cfg : CDP configuration
> + * @enable: true to enable CDP
> + * @ubwc_meta_enable: true to enable ubwc metadata preload
> + * @tile_amortize_enable: true to enable amortization control for tile format
> + * @preload_ahead: number of request to preload ahead
> + * SDE_WB_CDP_PRELOAD_AHEAD_32,
> + * SDE_WB_CDP_PRELOAD_AHEAD_64
> + */
> +struct dpu_hw_wb_cdp_cfg {
> +	bool enable;
> +	bool ubwc_meta_enable;
> +	bool tile_amortize_enable;
> +	u32 preload_ahead;
> +};
> +
> +/**
> + * struct dpu_hw_wb_qos_cfg : Writeback pipe QoS configuration
> + * @danger_lut: LUT for generate danger level based on fill level
> + * @safe_lut: LUT for generate safe level based on fill level
> + * @creq_lut: LUT for generate creq level based on fill level
> + * @danger_safe_en: enable danger safe generation
> + */
> +struct dpu_hw_wb_qos_cfg {
> +	u32 danger_lut;
> +	u32 safe_lut;
> +	u64 creq_lut;
> +	bool danger_safe_en;
> +};
> +
> +/**
> + *
> + * struct dpu_hw_wb_ops : Interface to the wb hw driver functions
> + *  Assumption is these functions will be called after clocks are enabled
> + *  @setup_outaddress: setup output address from the writeback job
> + *  @setup_outformat: setup output format of writeback block from writeback job
> + *  @setup_qos_lut:   setup qos LUT for writeback block based on input
> + *  @setup_cdp:       setup chroma down prefetch block for writeback block
> + *  @bind_pingpong_blk: enable/disable the connection with ping-pong block
> + */
> +struct dpu_hw_wb_ops {
> +	void (*setup_outaddress)(struct dpu_hw_wb *ctx,
> +			struct dpu_hw_wb_cfg *wb);
> +
> +	void (*setup_outformat)(struct dpu_hw_wb *ctx,
> +			struct dpu_hw_wb_cfg *wb);
> +
> +	void (*setup_roi)(struct dpu_hw_wb *ctx,
> +			struct dpu_hw_wb_cfg *wb);
> +
> +	void (*setup_qos_lut)(struct dpu_hw_wb *ctx,
> +			struct dpu_hw_wb_qos_cfg *cfg);
> +
> +	void (*setup_cdp)(struct dpu_hw_wb *ctx,
> +			struct dpu_hw_wb_cdp_cfg *cfg);
> +
> +	void (*bind_pingpong_blk)(struct dpu_hw_wb *ctx,
> +			bool enable, const enum dpu_pingpong pp);
> +};
> +
> +/**
> + * struct dpu_hw_wb : WB driver object
> + * @base: hardware block base structure
> + * @hw: block hardware details
> + * @mdp: pointer to associated mdp portion of the catalog
> + * @idx: hardware index number within type
> + * @wb_hw_caps: hardware capabilities
> + * @ops: function pointers
> + * @hw_mdp: MDP top level hardware block
> + */
> +struct dpu_hw_wb {
> +	struct dpu_hw_blk base;
> +	struct dpu_hw_blk_reg_map hw;
> +	const struct dpu_mdp_cfg *mdp;
> +
> +	/* wb path */
> +	int idx;
> +	const struct dpu_wb_cfg *caps;
> +
> +	/* ops */
> +	struct dpu_hw_wb_ops ops;
> +
> +	struct dpu_hw_mdp *hw_mdp;
> +};
> +
> +/**
> + * dpu_hw_wb - convert base object dpu_hw_base to container
> + * @hw: Pointer to base hardware block
> + * return: Pointer to hardware block container
> + */
> +static inline struct dpu_hw_wb *to_dpu_hw_wb(struct dpu_hw_blk *hw)
> +{
> +	return container_of(hw, struct dpu_hw_wb, base);
> +}
> +
> +/**
> + * dpu_hw_wb_init(): Initializes and return writeback hw driver object.
> + * @idx:  wb_path index for which driver object is required
> + * @addr: mapped register io address of MDP
> + * @m :   pointer to mdss catalog data
> + */
> +struct dpu_hw_wb *dpu_hw_wb_init(enum dpu_wb idx,
> +		void __iomem *addr,
> +		const struct dpu_mdss_cfg *m);
> +
> +/**
> + * dpu_hw_wb_destroy(): Destroy writeback hw driver object.
> + * @hw_wb:  Pointer to writeback hw driver object
> + */
> +void dpu_hw_wb_destroy(struct dpu_hw_wb *hw_wb);
> +
> +#endif /*_DPU_HW_WB_H */
> +
> +

extra empty lines in the end

-- 
With best wishes
Dmitry