[PATCH 02/12] drm/msm/dpu: add dpu_hw_wb abstraction for writeback blocks
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Thu Apr 14 21:41:31 UTC 2022
On 15/04/2022 00:28, Abhinav Kumar wrote:
> Hi Dmitry
>
> Thanks for the review.
>
> Finally got back to this series after getting acks on the drm_writeback
> core changes.
>
>
> On 2/4/2022 2:56 PM, Dmitry Baryshkov wrote:
>> 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 at linaro.org>
>>
>> Few minor comments bellow.
>>
>>>
>>> Signed-off-by: Abhinav Kumar <quic_abhinavk at 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.
>>
> I understand why you might think that way. Some bitfields in the
> register have the same name.
Yes. I was looking at the way the DS and SSPP share the scaler code. So
came the idea of sharing the CDP.
>
> Both SSPP and WB do have CDP_CNTL but with some different bit
> definitions and ofcourse the register offsets are different.
>
> So I think we are better off them being separate right now.
Agree. Let's land this code first. Later we can check it would make
sense to unify the ops or not.
>
>>> +
>>> +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?
> Yes correct. I can also preserve the upper bits here while posting the
> next version.
Thank you!
>>
>>> +
>>> + DPU_REG_WRITE(c, WB_MUX, mux_cfg);
>>> +}
>>> +
--
With best wishes
Dmitry
More information about the dri-devel
mailing list