[08/12] drm/msm/dpu: introduce the dpu_encoder_phys_* for writeback

Introduce the dpu_encoder_phys_* for the writeback interface
to handle writeback specific hardware programming.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/Makefile                       |   1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |  36 +-
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c    | 813 +++++++++++++++++++++
 3 files changed, 849 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c

On 05/02/2022 00:17, Abhinav Kumar wrote:
> Introduce the dpu_encoder_phys_* for the writeback interface
> to handle writeback specific hardware programming.
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Mostly looks ok, see minor comments bellow.

> ---
>   drivers/gpu/drm/msm/Makefile                       |   1 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |  36 +-
>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c    | 813 +++++++++++++++++++++
>   3 files changed, 849 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> 
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index c43ef35..3abaf84 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -53,6 +53,7 @@ msm-y := \
>   	disp/dpu1/dpu_encoder.o \
>   	disp/dpu1/dpu_encoder_phys_cmd.o \
>   	disp/dpu1/dpu_encoder_phys_vid.o \
> +	disp/dpu1/dpu_encoder_phys_wb.o \
>   	disp/dpu1/dpu_formats.o \
>   	disp/dpu1/dpu_hw_catalog.o \
>   	disp/dpu1/dpu_hw_ctl.o \
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> index 7b3354d..80da0a9 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -159,6 +159,7 @@ struct dpu_encoder_phys_ops {
>    * @INTR_IDX_PINGPONG: Pingpong done unterrupt for cmd mode panel
>    * @INTR_IDX_UNDERRUN: Underrun unterrupt for video and cmd mode panel
>    * @INTR_IDX_RDPTR:    Readpointer done unterrupt for cmd mode panel
> + * @INTR_IDX_WB_DONE:  Writeback fone interrupt for virtual connector
>    */
>   enum dpu_intr_idx {
>   	INTR_IDX_VSYNC,
> @@ -166,6 +167,7 @@ enum dpu_intr_idx {
>   	INTR_IDX_UNDERRUN,
>   	INTR_IDX_CTL_START,
>   	INTR_IDX_RDPTR,
> +	INTR_IDX_WB_DONE,
>   	INTR_IDX_MAX,
>   };
>   
> @@ -196,7 +198,7 @@ struct dpu_encoder_irq {
>    * @hw_ctl:		Hardware interface to the ctl registers
>    * @hw_pp:		Hardware interface to the ping pong registers
>    * @hw_intf:		Hardware interface to the intf registers
> - * @hw_wb:             Hardware interface to the wb registers
> + * @hw_wb:		Hardware interface to the wb registers
>    * @dpu_kms:		Pointer to the dpu_kms top level
>    * @cached_mode:	DRM mode cached at mode_set time, acted on in enable
>    * @enabled:		Whether the encoder has enabled and running a mode
> @@ -250,6 +252,31 @@ static inline int dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
>   }
>   
>   /**
> + * struct dpu_encoder_phys_wb - sub-class of dpu_encoder_phys to handle command
> + *	mode specific operations
> + * @base:	Baseclass physical encoder structure
> + * @wbirq_refcount:     Reference count of writeback interrupt
> + * @wb_done_timeout_cnt: number of wb done irq timeout errors
> + * @wb_cfg:  writeback block config to store fb related details
> + * @cdp_cfg: chroma down prefetch block config for wb
> + * @aspace: address space to be used for wb framebuffer
> + * @wb_conn: backpointer to writeback connector
> + * @wb_job: backpointer to current writeback job
> + * @dest:   dpu buffer layout for current writeback output buffer
> + */
> +struct dpu_encoder_phys_wb {
> +	struct dpu_encoder_phys base;
> +	atomic_t wbirq_refcount;
> +	int wb_done_timeout_cnt;
> +	struct dpu_hw_wb_cfg wb_cfg;
> +	struct dpu_hw_wb_cdp_cfg cdp_cfg;

What about moving them to the stack rather storing them in the structure?

> +	struct msm_gem_address_space *aspace;
> +	struct drm_writeback_connector *wb_conn;
> +	struct drm_writeback_job *wb_job;
> +	struct dpu_hw_fmt_layout dest;
> +};
> +
> +/**
>    * struct dpu_encoder_phys_cmd - sub-class of dpu_encoder_phys to handle command
>    *	mode specific operations
>    * @base:	Baseclass physical encoder structure
> @@ -317,6 +344,13 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>   		struct dpu_enc_phys_init_params *p);
>   
>   /**
> + * dpu_encoder_phys_wb_init - initialize writeback encoder
> + * @init:	Pointer to init info structure with initialization params
> + */
> +struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
> +		struct dpu_enc_phys_init_params *p);
> +
> +/**
>    * dpu_encoder_helper_trigger_start - control start helper function
>    *	This helper function may be optionally specified by physical
>    *	encoders if they require ctl_start triggering.
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> new file mode 100644
> index 0000000..783f83e
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> @@ -0,0 +1,813 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#define pr_fmt(fmt)	"[drm:%s:%d] " fmt, __func__, __LINE__
> +
> +#include <linux/debugfs.h>

the header is unused

> +
> +#include "dpu_encoder_phys.h"
> +#include "dpu_formats.h"
> +#include "dpu_hw_top.h"
> +#include "dpu_hw_wb.h"
> +#include "dpu_hw_lm.h"
> +#include "dpu_hw_blk.h"
> +#include "dpu_hw_merge3d.h"
> +#include "dpu_hw_interrupts.h"
> +#include "dpu_core_irq.h"
> +#include "dpu_vbif.h"
> +#include "dpu_crtc.h"
> +#include "disp/msm_disp_snapshot.h"
> +
> +#define DEFAULT_MAX_WRITEBACK_WIDTH 2048
> +
> +#define to_dpu_encoder_phys_wb(x) \
> +	container_of(x, struct dpu_encoder_phys_wb, base)
> +
> +/**
> + * dpu_encoder_phys_wb_is_master - report wb always as master encoder
> + */
> +static bool dpu_encoder_phys_wb_is_master(struct dpu_encoder_phys *phys_enc)
> +{
> +	return true;
> +}

A comment that there is only one physical enc for WB would be helpful 
here. But maybe we should just pull a generic is_master into 
dpu_encoder_phys.h (it would work here too).

> +
> +/**
> + * dpu_encoder_phys_wb_set_ot_limit - set OT limit for writeback interface
> + * @phys_enc:	Pointer to physical encoder
> + */
> +static void dpu_encoder_phys_wb_set_ot_limit(
> +		struct dpu_encoder_phys *phys_enc)
> +{
> +	struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
> +	struct dpu_vbif_set_ot_params ot_params;
> +
> +	memset(&ot_params, 0, sizeof(ot_params));
> +	ot_params.xin_id = hw_wb->caps->xin_id;
> +	ot_params.num = hw_wb->idx - WB_0;
> +	ot_params.width = phys_enc->cached_mode.hdisplay;
> +	ot_params.height = phys_enc->cached_mode.vdisplay;
> +	ot_params.is_wfd = true;
> +	ot_params.frame_rate = drm_mode_vrefresh(&phys_enc->cached_mode);
> +	ot_params.vbif_idx = hw_wb->caps->vbif_idx;
> +	ot_params.clk_ctrl = hw_wb->caps->clk_ctrl;
> +	ot_params.rd = false;
> +
> +	dpu_vbif_set_ot_limit(phys_enc->dpu_kms, &ot_params);
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_set_qos_remap - set QoS remapper for writeback
> + * @phys_enc:	Pointer to physical encoder
> + */
> +static void dpu_encoder_phys_wb_set_qos_remap(
> +		struct dpu_encoder_phys *phys_enc)
> +{
> +	struct dpu_hw_wb *hw_wb;
> +	struct dpu_vbif_set_qos_params qos_params;
> +
> +	if (!phys_enc || !phys_enc->parent || !phys_enc->parent->crtc) {
> +		DPU_ERROR("invalid arguments\n");
> +		return;
> +	}
> +
> +	if (!phys_enc->hw_wb || !phys_enc->hw_wb->caps) {
> +		DPU_ERROR("invalid writeback hardware\n");
> +		return;
> +	}
> +
> +	hw_wb = phys_enc->hw_wb;
> +
> +	memset(&qos_params, 0, sizeof(qos_params));
> +	qos_params.vbif_idx = hw_wb->caps->vbif_idx;
> +	qos_params.xin_id = hw_wb->caps->xin_id;
> +	qos_params.clk_ctrl = hw_wb->caps->clk_ctrl;
> +	qos_params.num = hw_wb->idx - WB_0;
> +	qos_params.is_rt = false;
> +
> +	DPU_DEBUG("[qos_remap] wb:%d vbif:%d xin:%d is_rt:%d\n",
> +			qos_params.num,
> +			qos_params.vbif_idx,
> +			qos_params.xin_id, qos_params.is_rt);
> +
> +	dpu_vbif_set_qos_remap(phys_enc->dpu_kms, &qos_params);
> +}
> +
> +//this can be moved to some common file?

C99 comment and missing "FIXME: " :-D

Yes, let's pull it into dpu_hw_util.c

And if you (by chance) would see a value in unifying other 
QoS/CDP-related functions let's move them to the dpu_hw_util.c too.

> +static u64 _dpu_encoder_phys_wb_get_qos_lut(struct dpu_qos_lut_tbl *tbl,
> +		u32 total_fl)
> +{
> +	int i;
> +
> +	if (!tbl || !tbl->nentry || !tbl->entries)
> +		return 0;
> +
> +	for (i = 0; i < tbl->nentry; i++)
> +		if (total_fl <= tbl->entries[i].fl)
> +			return tbl->entries[i].lut;
> +
> +	/* if last fl is zero, use as default */
> +	if (!tbl->entries[i-1].fl)
> +		return tbl->entries[i-1].lut;
> +
> +	return 0;
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_set_qos - set QoS/danger/safe LUTs for writeback
> + * @phys_enc:	Pointer to physical encoder
> + */
> +static void dpu_encoder_phys_wb_set_qos(struct dpu_encoder_phys *phys_enc)
> +{
> +	struct dpu_hw_wb *hw_wb;
> +	struct dpu_hw_wb_qos_cfg qos_cfg;
> +	struct dpu_mdss_cfg *catalog;
> +	struct dpu_qos_lut_tbl *qos_lut_tb;
> +
> +	if (!phys_enc || !phys_enc->dpu_kms || !phys_enc->dpu_kms->catalog) {
> +		DPU_ERROR("invalid parameter(s)\n");
> +		return;
> +	}
> +
> +	catalog = phys_enc->dpu_kms->catalog;
> +
> +	hw_wb = phys_enc->hw_wb;
> +
> +	memset(&qos_cfg, 0, sizeof(struct dpu_hw_wb_qos_cfg));
> +	qos_cfg.danger_safe_en = true;
> +	qos_cfg.danger_lut =
> +		catalog->perf.danger_lut_tbl[DPU_QOS_LUT_USAGE_NRT];
> +
> +	qos_cfg.safe_lut = catalog->perf.safe_lut_tbl[DPU_QOS_LUT_USAGE_NRT];
> +
> +	qos_lut_tb = &catalog->perf.qos_lut_tbl[DPU_QOS_LUT_USAGE_NRT];
> +	qos_cfg.creq_lut = _dpu_encoder_phys_wb_get_qos_lut(qos_lut_tb, 0);
> +
> +	if (hw_wb->ops.setup_qos_lut)
> +		hw_wb->ops.setup_qos_lut(hw_wb, &qos_cfg);
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_setup_fb - setup output framebuffer
> + * @phys_enc:	Pointer to physical encoder
> + * @fb:		Pointer to output framebuffer
> + * @wb_roi:	Pointer to output region of interest
> + */
> +static void dpu_encoder_phys_wb_setup_fb(struct dpu_encoder_phys *phys_enc,
> +		struct drm_framebuffer *fb)
> +{
> +	struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc);
> +	struct dpu_hw_wb *hw_wb;
> +	struct dpu_hw_wb_cfg *wb_cfg;
> +	struct dpu_hw_wb_cdp_cfg *cdp_cfg;
> +
> +	if (!phys_enc || !phys_enc->dpu_kms || !phys_enc->dpu_kms->catalog ||
> +			!phys_enc->connector) {
> +		DPU_ERROR("invalid encoder\n");
> +		return;
> +	}
> +
> +	hw_wb = phys_enc->hw_wb;
> +	wb_cfg = &wb_enc->wb_cfg;
> +	cdp_cfg = &wb_enc->cdp_cfg;
> +
> +	wb_cfg->intf_mode = phys_enc->intf_mode;
> +	wb_cfg->roi.x1 = 0;
> +	wb_cfg->roi.x2 = phys_enc->cached_mode.hdisplay;
> +	wb_cfg->roi.y1 = 0;
> +	wb_cfg->roi.y2 = phys_enc->cached_mode.vdisplay;
> +
> +	if (hw_wb->ops.setup_roi)
> +		hw_wb->ops.setup_roi(hw_wb, wb_cfg);
> +
> +	if (hw_wb->ops.setup_outformat)
> +		hw_wb->ops.setup_outformat(hw_wb, wb_cfg);
> +
> +	if (hw_wb->ops.setup_cdp) {
> +		memset(cdp_cfg, 0, sizeof(struct dpu_hw_wb_cdp_cfg));
> +
> +		cdp_cfg->enable = phys_enc->dpu_kms->catalog->perf.cdp_cfg
> +				[DPU_PERF_CDP_USAGE_NRT].wr_enable;
> +		cdp_cfg->ubwc_meta_enable =
> +				DPU_FORMAT_IS_UBWC(wb_cfg->dest.format);
> +		cdp_cfg->tile_amortize_enable =
> +				DPU_FORMAT_IS_UBWC(wb_cfg->dest.format) ||
> +				DPU_FORMAT_IS_TILE(wb_cfg->dest.format);
> +		cdp_cfg->preload_ahead = DPU_WB_CDP_PRELOAD_AHEAD_64;
> +
> +		hw_wb->ops.setup_cdp(hw_wb, cdp_cfg);
> +	}
> +
> +	if (hw_wb->ops.setup_outaddress)
> +		hw_wb->ops.setup_outaddress(hw_wb, wb_cfg);
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_setup_cdp - setup chroma down prefetch block
> + * @phys_enc:Pointer to physical encoder
> + */
> +static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc)
> +{
> +	struct dpu_hw_wb *hw_wb;
> +	struct dpu_hw_ctl *ctl;
> +
> +	if (!phys_enc) {
> +		DPU_ERROR("invalid encoder\n");
> +		return;
> +	}
> +
> +	hw_wb = phys_enc->hw_wb;
> +	ctl = phys_enc->hw_ctl;
> +
> +	if (test_bit(DPU_CTL_ACTIVE_CFG, &ctl->caps->features) &&
> +		(phys_enc->hw_ctl &&
> +		 phys_enc->hw_ctl->ops.setup_intf_cfg)) {
> +		struct dpu_hw_intf_cfg intf_cfg = {0};
> +		struct dpu_hw_pingpong *hw_pp = phys_enc->hw_pp;
> +		enum dpu_3d_blend_mode mode_3d;
> +
> +		mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> +
> +		intf_cfg.intf = DPU_NONE;
> +		intf_cfg.wb = hw_wb->idx;
> +
> +		if (mode_3d && hw_pp && hw_pp->merge_3d)
> +			intf_cfg.merge_3d = hw_pp->merge_3d->idx;
> +
> +		if (phys_enc->hw_pp->merge_3d && phys_enc->hw_pp->merge_3d->ops.setup_3d_mode)
> +			phys_enc->hw_pp->merge_3d->ops.setup_3d_mode(phys_enc->hw_pp->merge_3d,
> +					mode_3d);
> +
> +		/* setup which pp blk will connect to this wb */
> +		if (hw_pp && phys_enc->hw_wb->ops.bind_pingpong_blk)
> +			phys_enc->hw_wb->ops.bind_pingpong_blk(phys_enc->hw_wb, true,
> +					phys_enc->hw_pp->idx);
> +
> +		phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl,
> +				&intf_cfg, true);
> +	} else if (phys_enc->hw_ctl && phys_enc->hw_ctl->ops.setup_intf_cfg) {
> +		struct dpu_hw_intf_cfg intf_cfg = {0};
> +
> +		intf_cfg.intf = DPU_NONE;
> +		intf_cfg.wb = hw_wb->idx;
> +		intf_cfg.mode_3d =
> +			dpu_encoder_helper_get_3d_blend_mode(phys_enc);
> +		phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, &intf_cfg, true);
> +	}
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_atomic_check - verify and fixup given atomic states
> + * @phys_enc:	Pointer to physical encoder
> + * @crtc_state:	Pointer to CRTC atomic state
> + * @conn_state:	Pointer to connector atomic state
> + */
> +static int dpu_encoder_phys_wb_atomic_check(
> +		struct dpu_encoder_phys *phys_enc,
> +		struct drm_crtc_state *crtc_state,
> +		struct drm_connector_state *conn_state)
> +{
> +	struct drm_framebuffer *fb;
> +	const struct drm_display_mode *mode;
> +
> +	DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
> +			phys_enc->intf_idx, mode->name, mode->hdisplay, mode->vdisplay);
> +
> +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> +		return 0;
> +
> +	fb = conn_state->writeback_job->fb;
> +	mode = &crtc_state->mode;
> +
> +	if (!conn_state || !conn_state->connector) {
> +		DPU_ERROR("invalid connector state\n");
> +		return -EINVAL;
> +	} else if (conn_state->connector->status !=
> +			connector_status_connected) {
> +		DPU_ERROR("connector not connected %d\n",
> +				conn_state->connector->status);
> +		return -EINVAL;
> +	}
> +
> +	DPU_DEBUG("[fb_id:%u][fb:%u,%u]\n", fb->base.id,
> +			fb->width, fb->height);
> +
> +	if (fb->width != mode->hdisplay) {
> +		DPU_ERROR("invalid fb w=%d, mode w=%d\n", fb->width,
> +				mode->hdisplay);
> +		return -EINVAL;
> +	} else if (fb->height != mode->vdisplay) {
> +		DPU_ERROR("invalid fb h=%d, mode h=%d\n", fb->height,
> +				  mode->vdisplay);
> +		return -EINVAL;
> +	} else if (fb->width > DEFAULT_MAX_WRITEBACK_WIDTH) {
> +		DPU_ERROR("invalid fb w=%d, maxlinewidth=%u\n",
> +				  fb->width, DEFAULT_MAX_WRITEBACK_WIDTH);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +
> +/**
> + * _dpu_encoder_phys_wb_update_flush - flush hardware update
> + * @phys_enc:	Pointer to physical encoder
> + */
> +static void _dpu_encoder_phys_wb_update_flush(struct dpu_encoder_phys *phys_enc)
> +{
> +	struct dpu_hw_wb *hw_wb;
> +	struct dpu_hw_ctl *hw_ctl;
> +	struct dpu_hw_pingpong *hw_pp;
> +	u32 pending_flush = 0;
> +
> +	if (!phys_enc)
> +		return;
> +
> +	hw_wb = phys_enc->hw_wb;
> +	hw_pp = phys_enc->hw_pp;
> +	hw_ctl = phys_enc->hw_ctl;
> +
> +	DPU_DEBUG("[wb:%d]\n", hw_wb->idx - WB_0);
> +
> +	if (!hw_ctl) {
> +		DPU_DEBUG("[wb:%d] no ctl assigned\n", hw_wb->idx - WB_0);
> +		return;
> +	}
> +
> +	if (hw_ctl->ops.update_pending_flush_wb)
> +		hw_ctl->ops.update_pending_flush_wb(hw_ctl, hw_wb->idx);
> +
> +	if (hw_ctl->ops.update_pending_flush_merge_3d && hw_pp && hw_pp->merge_3d)
> +		hw_ctl->ops.update_pending_flush_merge_3d(hw_ctl,
> +				hw_pp->merge_3d->idx);
> +
> +	if (hw_ctl->ops.get_pending_flush)
> +		pending_flush = hw_ctl->ops.get_pending_flush(hw_ctl);
> +
> +	DPU_DEBUG("Pending flush mask for CTL_%d is 0x%x, WB %d\n",
> +			hw_ctl->idx - CTL_0, pending_flush,
> +			hw_wb->idx - WB_0);
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_setup - setup writeback encoder
> + * @phys_enc:	Pointer to physical encoder
> + */
> +static void dpu_encoder_phys_wb_setup(
> +		struct dpu_encoder_phys *phys_enc)
> +{
> +	struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
> +	struct drm_display_mode mode = phys_enc->cached_mode;
> +	struct drm_framebuffer *fb = NULL;
> +
> +	DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n",
> +			hw_wb->idx - WB_0, mode.name,
> +			mode.hdisplay, mode.vdisplay);
> +
> +	dpu_encoder_phys_wb_set_ot_limit(phys_enc);
> +
> +	dpu_encoder_phys_wb_set_qos_remap(phys_enc);
> +
> +	dpu_encoder_phys_wb_set_qos(phys_enc);
> +
> +	dpu_encoder_phys_wb_setup_fb(phys_enc, fb);
> +
> +	dpu_encoder_phys_wb_setup_cdp(phys_enc);
> +
> +}
> +
> +static void _dpu_encoder_phys_wb_frame_done_helper(void *arg)
> +{
> +	struct dpu_encoder_phys *phys_enc = arg;
> +	struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc);
> +
> +	struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
> +	unsigned long lock_flags;
> +	u32 event = DPU_ENCODER_FRAME_EVENT_DONE;
> +
> +	DPU_DEBUG("[wb:%d]\n", hw_wb->idx - WB_0);
> +
> +	if (phys_enc->parent_ops->handle_frame_done)
> +		phys_enc->parent_ops->handle_frame_done(phys_enc->parent,
> +				phys_enc, event);
> +
> +	if (phys_enc->parent_ops->handle_vblank_virt)
> +		phys_enc->parent_ops->handle_vblank_virt(phys_enc->parent,
> +				phys_enc);
> +
> +	spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
> +	atomic_add_unless(&phys_enc->pending_kickoff_cnt, -1, 0);
> +	spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
> +
> +	if (wb_enc->wb_conn)
> +		drm_writeback_signal_completion(wb_enc->wb_conn, 0);
> +
> +	/* Signal any waiting atomic commit thread */
> +	wake_up_all(&phys_enc->pending_kickoff_wq);
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_done_irq - writeback interrupt handler
> + * @arg:	Pointer to writeback encoder
> + * @irq_idx:	interrupt index
> + */
> +static void dpu_encoder_phys_wb_done_irq(void *arg, int irq_idx)
> +{
> +	_dpu_encoder_phys_wb_frame_done_helper(arg);
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_irq_ctrl - irq control of WB
> + * @phys:	Pointer to physical encoder
> + * @enable:	indicates enable or disable interrupts
> + */
> +static void dpu_encoder_phys_wb_irq_ctrl(
> +		struct dpu_encoder_phys *phys, bool enable)
> +{
> +
> +	struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys);
> +	int ret = 0;
> +	int refcount;
> +
> +	refcount = atomic_read(&wb_enc->wbirq_refcount);
> +
> +	if (enable && atomic_inc_return(&wb_enc->wbirq_refcount) == 1) {
> +		dpu_encoder_helper_register_irq(phys, INTR_IDX_WB_DONE);
> +		if (ret)
> +			atomic_dec_return(&wb_enc->wbirq_refcount);
> +	} else if (!enable &&
> +			atomic_dec_return(&wb_enc->wbirq_refcount) == 0) {
> +		dpu_encoder_helper_unregister_irq(phys, INTR_IDX_WB_DONE);
> +		if (ret)
> +			atomic_inc_return(&wb_enc->wbirq_refcount);
> +	}
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_mode_set - set display mode
> + * @phys_enc:	Pointer to physical encoder
> + * @mode:	Pointer to requested display mode
> + * @adj_mode:	Pointer to adjusted display mode
> + */
> +static void dpu_encoder_phys_wb_mode_set(
> +		struct dpu_encoder_phys *phys_enc,
> +		struct drm_display_mode *mode,
> +		struct drm_display_mode *adj_mode)
> +{
> +	struct dpu_encoder_irq *irq;
> +
> +	if (adj_mode) {
> +		phys_enc->cached_mode = *adj_mode;
> +		drm_mode_debug_printmodeline(adj_mode);
> +		DPU_DEBUG("caching mode:\n");
> +	}
> +
> +	irq = &phys_enc->irq[INTR_IDX_WB_DONE];
> +	irq->irq_idx = phys_enc->hw_wb->caps->intr_wb_done;
> +}
> +
> +static void _dpu_encoder_phys_wb_handle_wbdone_timeout(
> +		struct dpu_encoder_phys *phys_enc)
> +{
> +	struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc);
> +	u32 frame_event = DPU_ENCODER_FRAME_EVENT_ERROR;
> +
> +	wb_enc->wb_done_timeout_cnt++;
> +
> +	if (wb_enc->wb_done_timeout_cnt == 1)
> +		msm_disp_snapshot_state(phys_enc->parent->dev);
> +
> +	atomic_add_unless(&phys_enc->pending_kickoff_cnt, -1, 0);
> +
> +	/* request a ctl reset before the next kickoff */
> +	phys_enc->enable_state = DPU_ENC_ERR_NEEDS_HW_RESET;
> +
> +	if (wb_enc->wb_conn)
> +		drm_writeback_signal_completion(wb_enc->wb_conn, 0);
> +
> +	if (phys_enc->parent_ops->handle_frame_done)
> +		phys_enc->parent_ops->handle_frame_done(
> +				phys_enc->parent, phys_enc, frame_event);
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_wait_for_commit_done - wait until request is committed
> + * @phys_enc:	Pointer to physical encoder
> + */
> +static int dpu_encoder_phys_wb_wait_for_commit_done(
> +		struct dpu_encoder_phys *phys_enc)
> +{
> +	unsigned long ret;
> +	struct dpu_encoder_wait_info wait_info;
> +	struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc);
> +
> +	wait_info.wq = &phys_enc->pending_kickoff_wq;
> +	wait_info.atomic_cnt = &phys_enc->pending_kickoff_cnt;
> +	wait_info.timeout_ms = KICKOFF_TIMEOUT_MS;
> +
> +	ret = dpu_encoder_helper_wait_for_irq(phys_enc, INTR_IDX_WB_DONE,
> +			&wait_info);
> +	if (ret == -ETIMEDOUT)
> +		_dpu_encoder_phys_wb_handle_wbdone_timeout(phys_enc);
> +	else if (!ret)
> +		wb_enc->wb_done_timeout_cnt = 0;
> +
> +	return ret;
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_prepare_for_kickoff - pre-kickoff processing
> + * @phys_enc:	Pointer to physical encoder
> + * Returns:	Zero on success
> + */
> +static void dpu_encoder_phys_wb_prepare_for_kickoff(
> +		struct dpu_encoder_phys *phys_enc)
> +{
> +	struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc);
> +	struct drm_connector *drm_conn;
> +	struct drm_connector_state *state;
> +
> +	DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
> +
> +	if (!wb_enc->wb_conn || !wb_enc->wb_job) {
> +		DPU_ERROR("invalid wb_conn or wb_job\n");
> +		return;
> +	}
> +
> +	drm_conn = wb_enc->wb_conn->base;
> +	state = drm_conn->state;
> +
> +	if (wb_enc->wb_conn && wb_enc->wb_job)
> +		drm_writeback_queue_job(wb_enc->wb_conn, state);
> +
> +	dpu_encoder_phys_wb_setup(phys_enc);
> +
> +	_dpu_encoder_phys_wb_update_flush(phys_enc);
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_needs_single_flush - trigger flush processing
> + * @phys_enc:	Pointer to physical encoder
> + */
> +static bool dpu_encoder_phys_wb_needs_single_flush(struct dpu_encoder_phys *phys_enc)
> +{
> +	DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
> +	return false;
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_handle_post_kickoff - post-kickoff processing
> + * @phys_enc:	Pointer to physical encoder
> + */
> +static void dpu_encoder_phys_wb_handle_post_kickoff(
> +		struct dpu_encoder_phys *phys_enc)
> +{
> +	DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
> +
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_enable - enable writeback encoder
> + * @phys_enc:	Pointer to physical encoder
> + */
> +static void dpu_encoder_phys_wb_enable(struct dpu_encoder_phys *phys_enc)
> +{
> +	DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
> +	phys_enc->enable_state = DPU_ENC_ENABLED;
> +}
> +/**
> + * dpu_encoder_phys_wb_disable - disable writeback encoder
> + * @phys_enc:	Pointer to physical encoder
> + */
> +static void dpu_encoder_phys_wb_disable(struct dpu_encoder_phys *phys_enc)
> +{
> +	struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
> +
> +	DPU_DEBUG("[wb:%d]\n", hw_wb->idx - WB_0);
> +
> +	if (phys_enc->enable_state == DPU_ENC_DISABLED) {
> +		DPU_ERROR("encoder is already disabled\n");
> +		return;
> +	}
> +
> +	/* reset h/w before final flush */
> +	if (phys_enc->hw_ctl->ops.clear_pending_flush)
> +		phys_enc->hw_ctl->ops.clear_pending_flush(phys_enc->hw_ctl);
> +
> +	/*
> +	 * New CTL reset sequence from 5.0 MDP onwards.
> +	 * If has_3d_merge_reset is not set, legacy reset
> +	 * sequence is executed.
> +	 *
> +	 * Legacy reset sequence has not been implemented yet.
> +	 * Any target earlier than SM8150 will need it and when
> +	 * WB support is added to those targets will need to add
> +	 * the legacy teardown sequence as well.
> +	 */
> +	if (phys_enc->hw_pp->merge_3d)

This is a bit.. counterintuitive. I'd prefer hw_ctl->caps->features & 
BIT(DPU_ACTIVE_CTL).

> +		dpu_encoder_helper_phys_cleanup(phys_enc);
> +
> +	phys_enc->enable_state = DPU_ENC_DISABLED;
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_get_hw_resources - get hardware resources
> + * @phys_enc:	Pointer to physical encoder
> + * @hw_res:	Pointer to encoder resources
> + */
> +static void dpu_encoder_phys_wb_get_hw_resources(
> +		struct dpu_encoder_phys *phys_enc,
> +		struct dpu_encoder_hw_resources *hw_res)
> +{
> +	if (!phys_enc) {
> +		DPU_ERROR("invalid encoder\n");
> +		return;
> +	}
> +
> +	hw_res->wbs[phys_enc->intf_idx - WB_0] = INTF_MODE_WB_LINE;
> +	DPU_DEBUG("[wb:%d] intf_mode=%d\n", phys_enc->intf_idx - WB_0,
> +			hw_res->wbs[phys_enc->intf_idx - WB_0]);
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_destroy - destroy writeback encoder
> + * @phys_enc:	Pointer to physical encoder
> + */
> +static void dpu_encoder_phys_wb_destroy(struct dpu_encoder_phys *phys_enc)
> +{
> +	DPU_DEBUG("[wb:%d]\n", phys_enc->intf_idx - INTF_0);
> +
> +	if (!phys_enc)
> +		return;
> +
> +	kfree(phys_enc);
> +}
> +
> +static void dpu_encoder_phys_wb_prepare_wb_job(struct dpu_encoder_phys *phys_enc,
> +		struct drm_writeback_job *job)
> +{
> +	const struct msm_format *format;
> +	struct dpu_hw_wb_cfg *wb_cfg;
> +	int ret;
> +	struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc);
> +
> +	if (!job->fb)
> +		return;
> +
> +	wb_enc->wb_job = job;
> +	wb_enc->wb_conn = job->connector;
> +	wb_enc->aspace = phys_enc->dpu_kms->base.aspace;

Any particular reason to cache it in wb_enc?

> +
> +	wb_cfg = &wb_enc->wb_cfg;
> +
> +	memset(wb_cfg, 0, sizeof(struct dpu_hw_wb_cfg));
> +
> +	ret = msm_framebuffer_prepare(job->fb, wb_enc->aspace);
> +	if (ret) {
> +		DPU_ERROR("prep fb failed, %d\n", ret);
> +		return;
> +	}
> +
> +	format = msm_framebuffer_format(job->fb);
> +
> +	wb_cfg->dest.format = dpu_get_dpu_format_ext(
> +			format->pixel_format, job->fb->modifier);
> +	if (!wb_cfg->dest.format) {
> +		/* this error should be detected during atomic_check */
> +		DPU_ERROR("failed to get format %x\n", format->pixel_format);
> +		return;
> +	}
> +
> +	ret = dpu_format_populate_layout(wb_enc->aspace, job->fb, &wb_cfg->dest);
> +	if (ret) {
> +		DPU_DEBUG("failed to populate layout %d\n", ret);
> +		return;
> +	}
> +
> +	wb_cfg->dest.width = job->fb->width;
> +	wb_cfg->dest.height = job->fb->height;
> +	wb_cfg->dest.num_planes = wb_cfg->dest.format->num_planes;
> +
> +	if ((wb_cfg->dest.format->fetch_planes == DPU_PLANE_PLANAR) &&
> +			(wb_cfg->dest.format->element[0] == C1_B_Cb))
> +		swap(wb_cfg->dest.plane_addr[1], wb_cfg->dest.plane_addr[2]);
> +
> +	DPU_DEBUG("[fb_offset:%8.8x,%8.8x,%8.8x,%8.8x]\n",
> +			wb_cfg->dest.plane_addr[0], wb_cfg->dest.plane_addr[1],
> +			wb_cfg->dest.plane_addr[2], wb_cfg->dest.plane_addr[3]);
> +
> +	DPU_DEBUG("[fb_stride:%8.8x,%8.8x,%8.8x,%8.8x]\n",
> +			wb_cfg->dest.plane_pitch[0], wb_cfg->dest.plane_pitch[1],
> +			wb_cfg->dest.plane_pitch[2], wb_cfg->dest.plane_pitch[3]);
> +}
> +
> +static void dpu_encoder_phys_wb_cleanup_wb_job(struct dpu_encoder_phys *phys_enc,
> +		struct drm_writeback_job *job)
> +{
> +	struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys_enc);
> +
> +	if (!job->fb)
> +		return;
> +
> +	msm_framebuffer_cleanup(job->fb, wb_enc->aspace);
> +	// revisit this after everything else works

Everything works now, doesn't it?

> +	wb_enc->wb_job = NULL;
> +	wb_enc->wb_conn = NULL;
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_init_ops - initialize writeback operations
> + * @ops:	Pointer to encoder operation table
> + */
> +static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
> +{
> +	ops->is_master = dpu_encoder_phys_wb_is_master;
> +	ops->mode_set = dpu_encoder_phys_wb_mode_set;
> +	ops->enable = dpu_encoder_phys_wb_enable;
> +	ops->disable = dpu_encoder_phys_wb_disable;
> +	ops->destroy = dpu_encoder_phys_wb_destroy;
> +	ops->atomic_check = dpu_encoder_phys_wb_atomic_check;
> +	ops->get_hw_resources = dpu_encoder_phys_wb_get_hw_resources;
> +	ops->wait_for_commit_done = dpu_encoder_phys_wb_wait_for_commit_done;
> +	ops->prepare_for_kickoff = dpu_encoder_phys_wb_prepare_for_kickoff;
> +	ops->handle_post_kickoff = dpu_encoder_phys_wb_handle_post_kickoff;
> +	ops->needs_single_flush = dpu_encoder_phys_wb_needs_single_flush;
> +	ops->trigger_start = dpu_encoder_helper_trigger_start;
> +	ops->prepare_wb_job = dpu_encoder_phys_wb_prepare_wb_job;
> +	ops->cleanup_wb_job = dpu_encoder_phys_wb_cleanup_wb_job;
> +	ops->irq_control = dpu_encoder_phys_wb_irq_ctrl;
> +}
> +
> +/**
> + * dpu_encoder_phys_wb_init - initialize writeback encoder
> + * @init:	Pointer to init info structure with initialization params
> + */
> +struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
> +		struct dpu_enc_phys_init_params *p)
> +{
> +	struct dpu_encoder_phys *phys_enc = NULL;
> +	struct dpu_encoder_phys_wb *wb_enc = NULL;
> +
> +	struct dpu_encoder_irq *irq;
> +	int ret = 0;
> +	int i;
> +
> +	DPU_DEBUG("\n");
> +
> +	if (!p || !p->parent) {
> +		DPU_ERROR("invalid params\n");
> +		ret = -EINVAL;
> +		goto fail_alloc;
> +	}
> +
> +	wb_enc = kzalloc(sizeof(*wb_enc), GFP_KERNEL);
> +	if (!wb_enc) {
> +		DPU_ERROR("failed to allocate wb phys_enc enc\n");
> +		ret = -ENOMEM;
> +		goto fail_alloc;
> +	}
> +
> +	phys_enc = &wb_enc->base;
> +	phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
> +	phys_enc->intf_idx = p->intf_idx;
> +
> +	dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
> +	phys_enc->parent = p->parent;
> +	phys_enc->parent_ops = p->parent_ops;
> +	phys_enc->dpu_kms = p->dpu_kms;
> +	phys_enc->split_role = p->split_role;
> +	phys_enc->intf_mode = INTF_MODE_WB_LINE;
> +	phys_enc->intf_idx = p->intf_idx;
> +	phys_enc->enc_spinlock = p->enc_spinlock;
> +
> +	atomic_set(&wb_enc->wbirq_refcount, 0);
> +
> +	for (i = 0; i < INTR_IDX_MAX; i++) {
> +		irq = &phys_enc->irq[i];
> +		INIT_LIST_HEAD(&irq->cb.list);
> +		irq->irq_idx = -EINVAL;
> +		irq->cb.arg = phys_enc;
> +	}
> +
> +	irq = &phys_enc->irq[INTR_IDX_WB_DONE];
> +	irq->name = "wb_done";
> +	irq->intr_idx = INTR_IDX_WB_DONE;
> +	irq->cb.func = dpu_encoder_phys_wb_done_irq;
> +
> +	atomic_set(&phys_enc->pending_kickoff_cnt, 0);
> +	atomic_set(&phys_enc->vblank_refcount, 0);
> +	wb_enc->wb_done_timeout_cnt = 0;
> +
> +	init_waitqueue_head(&phys_enc->pending_kickoff_wq);
> +	phys_enc->enable_state = DPU_ENC_DISABLED;
> +
> +	DPU_DEBUG("Created dpu_encoder_phys for wb %d\n",
> +			phys_enc->intf_idx);
> +
> +	return phys_enc;
> +
> +fail_alloc:
> +	return ERR_PTR(ret);
> +}

On 2/4/2022 3:19 PM, Dmitry Baryshkov wrote:
> On 05/02/2022 00:17, Abhinav Kumar wrote:
>> Introduce the dpu_encoder_phys_* for the writeback interface
>> to handle writeback specific hardware programming.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> 
> Mostly looks ok, see minor comments bellow.
> 
>> ---
>>   drivers/gpu/drm/msm/Makefile                       |   1 +
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |  36 +-
>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c    | 813 
>> +++++++++++++++++++++
>>   3 files changed, 849 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>
>> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
>> index c43ef35..3abaf84 100644
>> --- a/drivers/gpu/drm/msm/Makefile
>> +++ b/drivers/gpu/drm/msm/Makefile
>> @@ -53,6 +53,7 @@ msm-y := \
>>       disp/dpu1/dpu_encoder.o \
>>       disp/dpu1/dpu_encoder_phys_cmd.o \
>>       disp/dpu1/dpu_encoder_phys_vid.o \
>> +    disp/dpu1/dpu_encoder_phys_wb.o \
>>       disp/dpu1/dpu_formats.o \
>>       disp/dpu1/dpu_hw_catalog.o \
>>       disp/dpu1/dpu_hw_ctl.o \
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> index 7b3354d..80da0a9 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> @@ -159,6 +159,7 @@ struct dpu_encoder_phys_ops {
>>    * @INTR_IDX_PINGPONG: Pingpong done unterrupt for cmd mode panel
>>    * @INTR_IDX_UNDERRUN: Underrun unterrupt for video and cmd mode panel
>>    * @INTR_IDX_RDPTR:    Readpointer done unterrupt for cmd mode panel
>> + * @INTR_IDX_WB_DONE:  Writeback fone interrupt for virtual connector
>>    */
>>   enum dpu_intr_idx {
>>       INTR_IDX_VSYNC,
>> @@ -166,6 +167,7 @@ enum dpu_intr_idx {
>>       INTR_IDX_UNDERRUN,
>>       INTR_IDX_CTL_START,
>>       INTR_IDX_RDPTR,
>> +    INTR_IDX_WB_DONE,
>>       INTR_IDX_MAX,
>>   };
>> @@ -196,7 +198,7 @@ struct dpu_encoder_irq {
>>    * @hw_ctl:        Hardware interface to the ctl registers
>>    * @hw_pp:        Hardware interface to the ping pong registers
>>    * @hw_intf:        Hardware interface to the intf registers
>> - * @hw_wb:             Hardware interface to the wb registers
>> + * @hw_wb:        Hardware interface to the wb registers
>>    * @dpu_kms:        Pointer to the dpu_kms top level
>>    * @cached_mode:    DRM mode cached at mode_set time, acted on in 
>> enable
>>    * @enabled:        Whether the encoder has enabled and running a mode
>> @@ -250,6 +252,31 @@ static inline int 
>> dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
>>   }
>>   /**
>> + * struct dpu_encoder_phys_wb - sub-class of dpu_encoder_phys to 
>> handle command
>> + *    mode specific operations
>> + * @base:    Baseclass physical encoder structure
>> + * @wbirq_refcount:     Reference count of writeback interrupt
>> + * @wb_done_timeout_cnt: number of wb done irq timeout errors
>> + * @wb_cfg:  writeback block config to store fb related details
>> + * @cdp_cfg: chroma down prefetch block config for wb
>> + * @aspace: address space to be used for wb framebuffer
>> + * @wb_conn: backpointer to writeback connector
>> + * @wb_job: backpointer to current writeback job
>> + * @dest:   dpu buffer layout for current writeback output buffer
>> + */
>> +struct dpu_encoder_phys_wb {
>> +    struct dpu_encoder_phys base;
>> +    atomic_t wbirq_refcount;
>> +    int wb_done_timeout_cnt;
>> +    struct dpu_hw_wb_cfg wb_cfg;
>> +    struct dpu_hw_wb_cdp_cfg cdp_cfg;
> 
> What about moving them to the stack rather storing them in the structure?

Yes cdp_cfg can be moved to the stack. Will do it in the next version.

wb_cfg cannot because for drm_wb, prepare_wb_job has the job's fb 
attributes which are used to setup the wb.

But we commit it only in the encoder_kickoff in 
dpu_encoder_phys_wb_setup_fb().

So its shared across two methods.

> 
>> +    struct msm_gem_address_space *aspace;
>> +    struct drm_writeback_connector *wb_conn;
>> +    struct drm_writeback_job *wb_job;
>> +    struct dpu_hw_fmt_layout dest;
>> +};
>> +
>> +/**
>>    * struct dpu_encoder_phys_cmd - sub-class of dpu_encoder_phys to 
>> handle command
>>    *    mode specific operations
>>    * @base:    Baseclass physical encoder structure
>> @@ -317,6 +344,13 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>>           struct dpu_enc_phys_init_params *p);
>>   /**
>> + * dpu_encoder_phys_wb_init - initialize writeback encoder
>> + * @init:    Pointer to init info structure with initialization params
>> + */
>> +struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
>> +        struct dpu_enc_phys_init_params *p);
>> +
>> +/**
>>    * dpu_encoder_helper_trigger_start - control start helper function
>>    *    This helper function may be optionally specified by physical
>>    *    encoders if they require ctl_start triggering.
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>> new file mode 100644
>> index 0000000..783f83e
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>> @@ -0,0 +1,813 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + */
>> +
>> +#define pr_fmt(fmt)    "[drm:%s:%d] " fmt, __func__, __LINE__
>> +
>> +#include <linux/debugfs.h>
> 
> the header is unused
ack
> 
>> +
>> +#include "dpu_encoder_phys.h"
>> +#include "dpu_formats.h"
>> +#include "dpu_hw_top.h"
>> +#include "dpu_hw_wb.h"
>> +#include "dpu_hw_lm.h"
>> +#include "dpu_hw_blk.h"
>> +#include "dpu_hw_merge3d.h"
>> +#include "dpu_hw_interrupts.h"
>> +#include "dpu_core_irq.h"
>> +#include "dpu_vbif.h"
>> +#include "dpu_crtc.h"
>> +#include "disp/msm_disp_snapshot.h"
>> +
>> +#define DEFAULT_MAX_WRITEBACK_WIDTH 2048
>> +
>> +#define to_dpu_encoder_phys_wb(x) \
>> +    container_of(x, struct dpu_encoder_phys_wb, base)
>> +
>> +/**
>> + * dpu_encoder_phys_wb_is_master - report wb always as master encoder
>> + */
>> +static bool dpu_encoder_phys_wb_is_master(struct dpu_encoder_phys 
>> *phys_enc)
>> +{
>> +    return true;
>> +}
> 
> A comment that there is only one physical enc for WB would be helpful 
> here. But maybe we should just pull a generic is_master into 
> dpu_encoder_phys.h (it would work here too).

I am just following what we have today for phys_vid and phys_cmd.
But yes, I can leave a comment for sure.

> 
>> +
>> +/**
>> + * dpu_encoder_phys_wb_set_ot_limit - set OT limit for writeback 
>> interface
>> + * @phys_enc:    Pointer to physical encoder
>> + */
>> +static void dpu_encoder_phys_wb_set_ot_limit(
>> +        struct dpu_encoder_phys *phys_enc)
>> +{
>> +    struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
>> +    struct dpu_vbif_set_ot_params ot_params;
>> +
>> +    memset(&ot_params, 0, sizeof(ot_params));
>> +    ot_params.xin_id = hw_wb->caps->xin_id;
>> +    ot_params.num = hw_wb->idx - WB_0;
>> +    ot_params.width = phys_enc->cached_mode.hdisplay;
>> +    ot_params.height = phys_enc->cached_mode.vdisplay;
>> +    ot_params.is_wfd = true;
>> +    ot_params.frame_rate = drm_mode_vrefresh(&phys_enc->cached_mode);
>> +    ot_params.vbif_idx = hw_wb->caps->vbif_idx;
>> +    ot_params.clk_ctrl = hw_wb->caps->clk_ctrl;
>> +    ot_params.rd = false;
>> +
>> +    dpu_vbif_set_ot_limit(phys_enc->dpu_kms, &ot_params);
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_set_qos_remap - set QoS remapper for writeback
>> + * @phys_enc:    Pointer to physical encoder
>> + */
>> +static void dpu_encoder_phys_wb_set_qos_remap(
>> +        struct dpu_encoder_phys *phys_enc)
>> +{
>> +    struct dpu_hw_wb *hw_wb;
>> +    struct dpu_vbif_set_qos_params qos_params;
>> +
>> +    if (!phys_enc || !phys_enc->parent || !phys_enc->parent->crtc) {
>> +        DPU_ERROR("invalid arguments\n");
>> +        return;
>> +    }
>> +
>> +    if (!phys_enc->hw_wb || !phys_enc->hw_wb->caps) {
>> +        DPU_ERROR("invalid writeback hardware\n");
>> +        return;
>> +    }
>> +
>> +    hw_wb = phys_enc->hw_wb;
>> +
>> +    memset(&qos_params, 0, sizeof(qos_params));
>> +    qos_params.vbif_idx = hw_wb->caps->vbif_idx;
>> +    qos_params.xin_id = hw_wb->caps->xin_id;
>> +    qos_params.clk_ctrl = hw_wb->caps->clk_ctrl;
>> +    qos_params.num = hw_wb->idx - WB_0;
>> +    qos_params.is_rt = false;
>> +
>> +    DPU_DEBUG("[qos_remap] wb:%d vbif:%d xin:%d is_rt:%d\n",
>> +            qos_params.num,
>> +            qos_params.vbif_idx,
>> +            qos_params.xin_id, qos_params.is_rt);
>> +
>> +    dpu_vbif_set_qos_remap(phys_enc->dpu_kms, &qos_params);
>> +}
>> +
>> +//this can be moved to some common file?
> 
> C99 comment and missing "FIXME: " :-D
Yes I will remove this since i am addressing this now.
> 
> Yes, let's pull it into dpu_hw_util.c
> 
> And if you (by chance) would see a value in unifying other 
> QoS/CDP-related functions let's move them to the dpu_hw_util.c too.

Yes I think I can move some utility functions like below to the util 
file. Will take care of it in next version.

> 
>> +static u64 _dpu_encoder_phys_wb_get_qos_lut(struct dpu_qos_lut_tbl *tbl,
>> +        u32 total_fl)
>> +{
>> +    int i;
>> +
>> +    if (!tbl || !tbl->nentry || !tbl->entries)
>> +        return 0;
>> +
>> +    for (i = 0; i < tbl->nentry; i++)
>> +        if (total_fl <= tbl->entries[i].fl)
>> +            return tbl->entries[i].lut;
>> +
>> +    /* if last fl is zero, use as default */
>> +    if (!tbl->entries[i-1].fl)
>> +        return tbl->entries[i-1].lut;
>> +
>> +    return 0;
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_set_qos - set QoS/danger/safe LUTs for writeback
>> + * @phys_enc:    Pointer to physical encoder
>> + */
>> +static void dpu_encoder_phys_wb_set_qos(struct dpu_encoder_phys 
>> *phys_enc)
>> +{
>> +    struct dpu_hw_wb *hw_wb;
>> +    struct dpu_hw_wb_qos_cfg qos_cfg;
>> +    struct dpu_mdss_cfg *catalog;
>> +    struct dpu_qos_lut_tbl *qos_lut_tb;
>> +
>> +    if (!phys_enc || !phys_enc->dpu_kms || 
>> !phys_enc->dpu_kms->catalog) {
>> +        DPU_ERROR("invalid parameter(s)\n");
>> +        return;
>> +    }
>> +
>> +    catalog = phys_enc->dpu_kms->catalog;
>> +
>> +    hw_wb = phys_enc->hw_wb;
>> +
>> +    memset(&qos_cfg, 0, sizeof(struct dpu_hw_wb_qos_cfg));
>> +    qos_cfg.danger_safe_en = true;
>> +    qos_cfg.danger_lut =
>> +        catalog->perf.danger_lut_tbl[DPU_QOS_LUT_USAGE_NRT];
>> +
>> +    qos_cfg.safe_lut = 
>> catalog->perf.safe_lut_tbl[DPU_QOS_LUT_USAGE_NRT];
>> +
>> +    qos_lut_tb = &catalog->perf.qos_lut_tbl[DPU_QOS_LUT_USAGE_NRT];
>> +    qos_cfg.creq_lut = _dpu_encoder_phys_wb_get_qos_lut(qos_lut_tb, 0);
>> +
>> +    if (hw_wb->ops.setup_qos_lut)
>> +        hw_wb->ops.setup_qos_lut(hw_wb, &qos_cfg);
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_setup_fb - setup output framebuffer
>> + * @phys_enc:    Pointer to physical encoder
>> + * @fb:        Pointer to output framebuffer
>> + * @wb_roi:    Pointer to output region of interest
>> + */
>> +static void dpu_encoder_phys_wb_setup_fb(struct dpu_encoder_phys 
>> *phys_enc,
>> +        struct drm_framebuffer *fb)
>> +{
>> +    struct dpu_encoder_phys_wb *wb_enc = 
>> to_dpu_encoder_phys_wb(phys_enc);
>> +    struct dpu_hw_wb *hw_wb;
>> +    struct dpu_hw_wb_cfg *wb_cfg;
>> +    struct dpu_hw_wb_cdp_cfg *cdp_cfg;
>> +
>> +    if (!phys_enc || !phys_enc->dpu_kms || 
>> !phys_enc->dpu_kms->catalog ||
>> +            !phys_enc->connector) {
>> +        DPU_ERROR("invalid encoder\n");
>> +        return;
>> +    }
>> +
>> +    hw_wb = phys_enc->hw_wb;
>> +    wb_cfg = &wb_enc->wb_cfg;
>> +    cdp_cfg = &wb_enc->cdp_cfg;
>> +
>> +    wb_cfg->intf_mode = phys_enc->intf_mode;
>> +    wb_cfg->roi.x1 = 0;
>> +    wb_cfg->roi.x2 = phys_enc->cached_mode.hdisplay;
>> +    wb_cfg->roi.y1 = 0;
>> +    wb_cfg->roi.y2 = phys_enc->cached_mode.vdisplay;
>> +
>> +    if (hw_wb->ops.setup_roi)
>> +        hw_wb->ops.setup_roi(hw_wb, wb_cfg);
>> +
>> +    if (hw_wb->ops.setup_outformat)
>> +        hw_wb->ops.setup_outformat(hw_wb, wb_cfg);
>> +
>> +    if (hw_wb->ops.setup_cdp) {
>> +        memset(cdp_cfg, 0, sizeof(struct dpu_hw_wb_cdp_cfg));
>> +
>> +        cdp_cfg->enable = phys_enc->dpu_kms->catalog->perf.cdp_cfg
>> +                [DPU_PERF_CDP_USAGE_NRT].wr_enable;
>> +        cdp_cfg->ubwc_meta_enable =
>> +                DPU_FORMAT_IS_UBWC(wb_cfg->dest.format);
>> +        cdp_cfg->tile_amortize_enable =
>> +                DPU_FORMAT_IS_UBWC(wb_cfg->dest.format) ||
>> +                DPU_FORMAT_IS_TILE(wb_cfg->dest.format);
>> +        cdp_cfg->preload_ahead = DPU_WB_CDP_PRELOAD_AHEAD_64;
>> +
>> +        hw_wb->ops.setup_cdp(hw_wb, cdp_cfg);
>> +    }
>> +
>> +    if (hw_wb->ops.setup_outaddress)
>> +        hw_wb->ops.setup_outaddress(hw_wb, wb_cfg);
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_setup_cdp - setup chroma down prefetch block
>> + * @phys_enc:Pointer to physical encoder
>> + */
>> +static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys 
>> *phys_enc)
>> +{
>> +    struct dpu_hw_wb *hw_wb;
>> +    struct dpu_hw_ctl *ctl;
>> +
>> +    if (!phys_enc) {
>> +        DPU_ERROR("invalid encoder\n");
>> +        return;
>> +    }
>> +
>> +    hw_wb = phys_enc->hw_wb;
>> +    ctl = phys_enc->hw_ctl;
>> +
>> +    if (test_bit(DPU_CTL_ACTIVE_CFG, &ctl->caps->features) &&
>> +        (phys_enc->hw_ctl &&
>> +         phys_enc->hw_ctl->ops.setup_intf_cfg)) {
>> +        struct dpu_hw_intf_cfg intf_cfg = {0};
>> +        struct dpu_hw_pingpong *hw_pp = phys_enc->hw_pp;
>> +        enum dpu_3d_blend_mode mode_3d;
>> +
>> +        mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>> +
>> +        intf_cfg.intf = DPU_NONE;
>> +        intf_cfg.wb = hw_wb->idx;
>> +
>> +        if (mode_3d && hw_pp && hw_pp->merge_3d)
>> +            intf_cfg.merge_3d = hw_pp->merge_3d->idx;
>> +
>> +        if (phys_enc->hw_pp->merge_3d && 
>> phys_enc->hw_pp->merge_3d->ops.setup_3d_mode)
>> +            
>> phys_enc->hw_pp->merge_3d->ops.setup_3d_mode(phys_enc->hw_pp->merge_3d,
>> +                    mode_3d);
>> +
>> +        /* setup which pp blk will connect to this wb */
>> +        if (hw_pp && phys_enc->hw_wb->ops.bind_pingpong_blk)
>> +            phys_enc->hw_wb->ops.bind_pingpong_blk(phys_enc->hw_wb, 
>> true,
>> +                    phys_enc->hw_pp->idx);
>> +
>> +        phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl,
>> +                &intf_cfg, true);
>> +    } else if (phys_enc->hw_ctl && 
>> phys_enc->hw_ctl->ops.setup_intf_cfg) {
>> +        struct dpu_hw_intf_cfg intf_cfg = {0};
>> +
>> +        intf_cfg.intf = DPU_NONE;
>> +        intf_cfg.wb = hw_wb->idx;
>> +        intf_cfg.mode_3d =
>> +            dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>> +        phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, 
>> &intf_cfg, true);
>> +    }
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_atomic_check - verify and fixup given atomic 
>> states
>> + * @phys_enc:    Pointer to physical encoder
>> + * @crtc_state:    Pointer to CRTC atomic state
>> + * @conn_state:    Pointer to connector atomic state
>> + */
>> +static int dpu_encoder_phys_wb_atomic_check(
>> +        struct dpu_encoder_phys *phys_enc,
>> +        struct drm_crtc_state *crtc_state,
>> +        struct drm_connector_state *conn_state)
>> +{
>> +    struct drm_framebuffer *fb;
>> +    const struct drm_display_mode *mode;
>> +
>> +    DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
>> +            phys_enc->intf_idx, mode->name, mode->hdisplay, 
>> mode->vdisplay);
>> +
>> +    if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
>> +        return 0;
>> +
>> +    fb = conn_state->writeback_job->fb;
>> +    mode = &crtc_state->mode;
>> +
>> +    if (!conn_state || !conn_state->connector) {
>> +        DPU_ERROR("invalid connector state\n");
>> +        return -EINVAL;
>> +    } else if (conn_state->connector->status !=
>> +            connector_status_connected) {
>> +        DPU_ERROR("connector not connected %d\n",
>> +                conn_state->connector->status);
>> +        return -EINVAL;
>> +    }
>> +
>> +    DPU_DEBUG("[fb_id:%u][fb:%u,%u]\n", fb->base.id,
>> +            fb->width, fb->height);
>> +
>> +    if (fb->width != mode->hdisplay) {
>> +        DPU_ERROR("invalid fb w=%d, mode w=%d\n", fb->width,
>> +                mode->hdisplay);
>> +        return -EINVAL;
>> +    } else if (fb->height != mode->vdisplay) {
>> +        DPU_ERROR("invalid fb h=%d, mode h=%d\n", fb->height,
>> +                  mode->vdisplay);
>> +        return -EINVAL;
>> +    } else if (fb->width > DEFAULT_MAX_WRITEBACK_WIDTH) {
>> +        DPU_ERROR("invalid fb w=%d, maxlinewidth=%u\n",
>> +                  fb->width, DEFAULT_MAX_WRITEBACK_WIDTH);
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +
>> +/**
>> + * _dpu_encoder_phys_wb_update_flush - flush hardware update
>> + * @phys_enc:    Pointer to physical encoder
>> + */
>> +static void _dpu_encoder_phys_wb_update_flush(struct dpu_encoder_phys 
>> *phys_enc)
>> +{
>> +    struct dpu_hw_wb *hw_wb;
>> +    struct dpu_hw_ctl *hw_ctl;
>> +    struct dpu_hw_pingpong *hw_pp;
>> +    u32 pending_flush = 0;
>> +
>> +    if (!phys_enc)
>> +        return;
>> +
>> +    hw_wb = phys_enc->hw_wb;
>> +    hw_pp = phys_enc->hw_pp;
>> +    hw_ctl = phys_enc->hw_ctl;
>> +
>> +    DPU_DEBUG("[wb:%d]\n", hw_wb->idx - WB_0);
>> +
>> +    if (!hw_ctl) {
>> +        DPU_DEBUG("[wb:%d] no ctl assigned\n", hw_wb->idx - WB_0);
>> +        return;
>> +    }
>> +
>> +    if (hw_ctl->ops.update_pending_flush_wb)
>> +        hw_ctl->ops.update_pending_flush_wb(hw_ctl, hw_wb->idx);
>> +
>> +    if (hw_ctl->ops.update_pending_flush_merge_3d && hw_pp && 
>> hw_pp->merge_3d)
>> +        hw_ctl->ops.update_pending_flush_merge_3d(hw_ctl,
>> +                hw_pp->merge_3d->idx);
>> +
>> +    if (hw_ctl->ops.get_pending_flush)
>> +        pending_flush = hw_ctl->ops.get_pending_flush(hw_ctl);
>> +
>> +    DPU_DEBUG("Pending flush mask for CTL_%d is 0x%x, WB %d\n",
>> +            hw_ctl->idx - CTL_0, pending_flush,
>> +            hw_wb->idx - WB_0);
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_setup - setup writeback encoder
>> + * @phys_enc:    Pointer to physical encoder
>> + */
>> +static void dpu_encoder_phys_wb_setup(
>> +        struct dpu_encoder_phys *phys_enc)
>> +{
>> +    struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
>> +    struct drm_display_mode mode = phys_enc->cached_mode;
>> +    struct drm_framebuffer *fb = NULL;
>> +
>> +    DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n",
>> +            hw_wb->idx - WB_0, mode.name,
>> +            mode.hdisplay, mode.vdisplay);
>> +
>> +    dpu_encoder_phys_wb_set_ot_limit(phys_enc);
>> +
>> +    dpu_encoder_phys_wb_set_qos_remap(phys_enc);
>> +
>> +    dpu_encoder_phys_wb_set_qos(phys_enc);
>> +
>> +    dpu_encoder_phys_wb_setup_fb(phys_enc, fb);
>> +
>> +    dpu_encoder_phys_wb_setup_cdp(phys_enc);
>> +
>> +}
>> +
>> +static void _dpu_encoder_phys_wb_frame_done_helper(void *arg)
>> +{
>> +    struct dpu_encoder_phys *phys_enc = arg;
>> +    struct dpu_encoder_phys_wb *wb_enc = 
>> to_dpu_encoder_phys_wb(phys_enc);
>> +
>> +    struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
>> +    unsigned long lock_flags;
>> +    u32 event = DPU_ENCODER_FRAME_EVENT_DONE;
>> +
>> +    DPU_DEBUG("[wb:%d]\n", hw_wb->idx - WB_0);
>> +
>> +    if (phys_enc->parent_ops->handle_frame_done)
>> +        phys_enc->parent_ops->handle_frame_done(phys_enc->parent,
>> +                phys_enc, event);
>> +
>> +    if (phys_enc->parent_ops->handle_vblank_virt)
>> +        phys_enc->parent_ops->handle_vblank_virt(phys_enc->parent,
>> +                phys_enc);
>> +
>> +    spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
>> +    atomic_add_unless(&phys_enc->pending_kickoff_cnt, -1, 0);
>> +    spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
>> +
>> +    if (wb_enc->wb_conn)
>> +        drm_writeback_signal_completion(wb_enc->wb_conn, 0);
>> +
>> +    /* Signal any waiting atomic commit thread */
>> +    wake_up_all(&phys_enc->pending_kickoff_wq);
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_done_irq - writeback interrupt handler
>> + * @arg:    Pointer to writeback encoder
>> + * @irq_idx:    interrupt index
>> + */
>> +static void dpu_encoder_phys_wb_done_irq(void *arg, int irq_idx)
>> +{
>> +    _dpu_encoder_phys_wb_frame_done_helper(arg);
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_irq_ctrl - irq control of WB
>> + * @phys:    Pointer to physical encoder
>> + * @enable:    indicates enable or disable interrupts
>> + */
>> +static void dpu_encoder_phys_wb_irq_ctrl(
>> +        struct dpu_encoder_phys *phys, bool enable)
>> +{
>> +
>> +    struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys);
>> +    int ret = 0;
>> +    int refcount;
>> +
>> +    refcount = atomic_read(&wb_enc->wbirq_refcount);
>> +
>> +    if (enable && atomic_inc_return(&wb_enc->wbirq_refcount) == 1) {
>> +        dpu_encoder_helper_register_irq(phys, INTR_IDX_WB_DONE);
>> +        if (ret)
>> +            atomic_dec_return(&wb_enc->wbirq_refcount);
>> +    } else if (!enable &&
>> +            atomic_dec_return(&wb_enc->wbirq_refcount) == 0) {
>> +        dpu_encoder_helper_unregister_irq(phys, INTR_IDX_WB_DONE);
>> +        if (ret)
>> +            atomic_inc_return(&wb_enc->wbirq_refcount);
>> +    }
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_mode_set - set display mode
>> + * @phys_enc:    Pointer to physical encoder
>> + * @mode:    Pointer to requested display mode
>> + * @adj_mode:    Pointer to adjusted display mode
>> + */
>> +static void dpu_encoder_phys_wb_mode_set(
>> +        struct dpu_encoder_phys *phys_enc,
>> +        struct drm_display_mode *mode,
>> +        struct drm_display_mode *adj_mode)
>> +{
>> +    struct dpu_encoder_irq *irq;
>> +
>> +    if (adj_mode) {
>> +        phys_enc->cached_mode = *adj_mode;
>> +        drm_mode_debug_printmodeline(adj_mode);
>> +        DPU_DEBUG("caching mode:\n");
>> +    }
>> +
>> +    irq = &phys_enc->irq[INTR_IDX_WB_DONE];
>> +    irq->irq_idx = phys_enc->hw_wb->caps->intr_wb_done;
>> +}
>> +
>> +static void _dpu_encoder_phys_wb_handle_wbdone_timeout(
>> +        struct dpu_encoder_phys *phys_enc)
>> +{
>> +    struct dpu_encoder_phys_wb *wb_enc = 
>> to_dpu_encoder_phys_wb(phys_enc);
>> +    u32 frame_event = DPU_ENCODER_FRAME_EVENT_ERROR;
>> +
>> +    wb_enc->wb_done_timeout_cnt++;
>> +
>> +    if (wb_enc->wb_done_timeout_cnt == 1)
>> +        msm_disp_snapshot_state(phys_enc->parent->dev);
>> +
>> +    atomic_add_unless(&phys_enc->pending_kickoff_cnt, -1, 0);
>> +
>> +    /* request a ctl reset before the next kickoff */
>> +    phys_enc->enable_state = DPU_ENC_ERR_NEEDS_HW_RESET;
>> +
>> +    if (wb_enc->wb_conn)
>> +        drm_writeback_signal_completion(wb_enc->wb_conn, 0);
>> +
>> +    if (phys_enc->parent_ops->handle_frame_done)
>> +        phys_enc->parent_ops->handle_frame_done(
>> +                phys_enc->parent, phys_enc, frame_event);
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_wait_for_commit_done - wait until request is 
>> committed
>> + * @phys_enc:    Pointer to physical encoder
>> + */
>> +static int dpu_encoder_phys_wb_wait_for_commit_done(
>> +        struct dpu_encoder_phys *phys_enc)
>> +{
>> +    unsigned long ret;
>> +    struct dpu_encoder_wait_info wait_info;
>> +    struct dpu_encoder_phys_wb *wb_enc = 
>> to_dpu_encoder_phys_wb(phys_enc);
>> +
>> +    wait_info.wq = &phys_enc->pending_kickoff_wq;
>> +    wait_info.atomic_cnt = &phys_enc->pending_kickoff_cnt;
>> +    wait_info.timeout_ms = KICKOFF_TIMEOUT_MS;
>> +
>> +    ret = dpu_encoder_helper_wait_for_irq(phys_enc, INTR_IDX_WB_DONE,
>> +            &wait_info);
>> +    if (ret == -ETIMEDOUT)
>> +        _dpu_encoder_phys_wb_handle_wbdone_timeout(phys_enc);
>> +    else if (!ret)
>> +        wb_enc->wb_done_timeout_cnt = 0;
>> +
>> +    return ret;
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_prepare_for_kickoff - pre-kickoff processing
>> + * @phys_enc:    Pointer to physical encoder
>> + * Returns:    Zero on success
>> + */
>> +static void dpu_encoder_phys_wb_prepare_for_kickoff(
>> +        struct dpu_encoder_phys *phys_enc)
>> +{
>> +    struct dpu_encoder_phys_wb *wb_enc = 
>> to_dpu_encoder_phys_wb(phys_enc);
>> +    struct drm_connector *drm_conn;
>> +    struct drm_connector_state *state;
>> +
>> +    DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
>> +
>> +    if (!wb_enc->wb_conn || !wb_enc->wb_job) {
>> +        DPU_ERROR("invalid wb_conn or wb_job\n");
>> +        return;
>> +    }
>> +
>> +    drm_conn = wb_enc->wb_conn->base;
>> +    state = drm_conn->state;
>> +
>> +    if (wb_enc->wb_conn && wb_enc->wb_job)
>> +        drm_writeback_queue_job(wb_enc->wb_conn, state);
>> +
>> +    dpu_encoder_phys_wb_setup(phys_enc);
>> +
>> +    _dpu_encoder_phys_wb_update_flush(phys_enc);
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_needs_single_flush - trigger flush processing
>> + * @phys_enc:    Pointer to physical encoder
>> + */
>> +static bool dpu_encoder_phys_wb_needs_single_flush(struct 
>> dpu_encoder_phys *phys_enc)
>> +{
>> +    DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
>> +    return false;
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_handle_post_kickoff - post-kickoff processing
>> + * @phys_enc:    Pointer to physical encoder
>> + */
>> +static void dpu_encoder_phys_wb_handle_post_kickoff(
>> +        struct dpu_encoder_phys *phys_enc)
>> +{
>> +    DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
>> +
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_enable - enable writeback encoder
>> + * @phys_enc:    Pointer to physical encoder
>> + */
>> +static void dpu_encoder_phys_wb_enable(struct dpu_encoder_phys 
>> *phys_enc)
>> +{
>> +    DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
>> +    phys_enc->enable_state = DPU_ENC_ENABLED;
>> +}
>> +/**
>> + * dpu_encoder_phys_wb_disable - disable writeback encoder
>> + * @phys_enc:    Pointer to physical encoder
>> + */
>> +static void dpu_encoder_phys_wb_disable(struct dpu_encoder_phys 
>> *phys_enc)
>> +{
>> +    struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
>> +
>> +    DPU_DEBUG("[wb:%d]\n", hw_wb->idx - WB_0);
>> +
>> +    if (phys_enc->enable_state == DPU_ENC_DISABLED) {
>> +        DPU_ERROR("encoder is already disabled\n");
>> +        return;
>> +    }
>> +
>> +    /* reset h/w before final flush */
>> +    if (phys_enc->hw_ctl->ops.clear_pending_flush)
>> +        phys_enc->hw_ctl->ops.clear_pending_flush(phys_enc->hw_ctl);
>> +
>> +    /*
>> +     * New CTL reset sequence from 5.0 MDP onwards.
>> +     * If has_3d_merge_reset is not set, legacy reset
>> +     * sequence is executed.
>> +     *
>> +     * Legacy reset sequence has not been implemented yet.
>> +     * Any target earlier than SM8150 will need it and when
>> +     * WB support is added to those targets will need to add
>> +     * the legacy teardown sequence as well.
>> +     */
>> +    if (phys_enc->hw_pp->merge_3d)
> 
> This is a bit.. counterintuitive. I'd prefer hw_ctl->caps->features & 
> BIT(DPU_ACTIVE_CTL).
Your suggestion should work.

But consider it this way.

Checking caps is telling us if the chipset supports merge3d which is 
sufficient in this case but phys_enc->hw_pp->merge_3d is telling us 
whether merge_3d is also being used in this RM session.

So i just thought its a better check to cover both.

> 
>> +        dpu_encoder_helper_phys_cleanup(phys_enc);
>> +
>> +    phys_enc->enable_state = DPU_ENC_DISABLED;
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_get_hw_resources - get hardware resources
>> + * @phys_enc:    Pointer to physical encoder
>> + * @hw_res:    Pointer to encoder resources
>> + */
>> +static void dpu_encoder_phys_wb_get_hw_resources(
>> +        struct dpu_encoder_phys *phys_enc,
>> +        struct dpu_encoder_hw_resources *hw_res)
>> +{
>> +    if (!phys_enc) {
>> +        DPU_ERROR("invalid encoder\n");
>> +        return;
>> +    }
>> +
>> +    hw_res->wbs[phys_enc->intf_idx - WB_0] = INTF_MODE_WB_LINE;
>> +    DPU_DEBUG("[wb:%d] intf_mode=%d\n", phys_enc->intf_idx - WB_0,
>> +            hw_res->wbs[phys_enc->intf_idx - WB_0]);
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_destroy - destroy writeback encoder
>> + * @phys_enc:    Pointer to physical encoder
>> + */
>> +static void dpu_encoder_phys_wb_destroy(struct dpu_encoder_phys 
>> *phys_enc)
>> +{
>> +    DPU_DEBUG("[wb:%d]\n", phys_enc->intf_idx - INTF_0);
>> +
>> +    if (!phys_enc)
>> +        return;
>> +
>> +    kfree(phys_enc);
>> +}
>> +
>> +static void dpu_encoder_phys_wb_prepare_wb_job(struct 
>> dpu_encoder_phys *phys_enc,
>> +        struct drm_writeback_job *job)
>> +{
>> +    const struct msm_format *format;
>> +    struct dpu_hw_wb_cfg *wb_cfg;
>> +    int ret;
>> +    struct dpu_encoder_phys_wb *wb_enc = 
>> to_dpu_encoder_phys_wb(phys_enc);
>> +
>> +    if (!job->fb)
>> +        return;
>> +
>> +    wb_enc->wb_job = job;
>> +    wb_enc->wb_conn = job->connector;
>> +    wb_enc->aspace = phys_enc->dpu_kms->base.aspace;
> 
> Any particular reason to cache it in wb_enc?

We can technically get this from phys_enc->dpu_kms->base.aspace so I can 
drop this.

> 
>> +
>> +    wb_cfg = &wb_enc->wb_cfg;
>> +
>> +    memset(wb_cfg, 0, sizeof(struct dpu_hw_wb_cfg));
>> +
>> +    ret = msm_framebuffer_prepare(job->fb, wb_enc->aspace);
>> +    if (ret) {
>> +        DPU_ERROR("prep fb failed, %d\n", ret);
>> +        return;
>> +    }
>> +
>> +    format = msm_framebuffer_format(job->fb);
>> +
>> +    wb_cfg->dest.format = dpu_get_dpu_format_ext(
>> +            format->pixel_format, job->fb->modifier);
>> +    if (!wb_cfg->dest.format) {
>> +        /* this error should be detected during atomic_check */
>> +        DPU_ERROR("failed to get format %x\n", format->pixel_format);
>> +        return;
>> +    }
>> +
>> +    ret = dpu_format_populate_layout(wb_enc->aspace, job->fb, 
>> &wb_cfg->dest);
>> +    if (ret) {
>> +        DPU_DEBUG("failed to populate layout %d\n", ret);
>> +        return;
>> +    }
>> +
>> +    wb_cfg->dest.width = job->fb->width;
>> +    wb_cfg->dest.height = job->fb->height;
>> +    wb_cfg->dest.num_planes = wb_cfg->dest.format->num_planes;
>> +
>> +    if ((wb_cfg->dest.format->fetch_planes == DPU_PLANE_PLANAR) &&
>> +            (wb_cfg->dest.format->element[0] == C1_B_Cb))
>> +        swap(wb_cfg->dest.plane_addr[1], wb_cfg->dest.plane_addr[2]);
>> +
>> +    DPU_DEBUG("[fb_offset:%8.8x,%8.8x,%8.8x,%8.8x]\n",
>> +            wb_cfg->dest.plane_addr[0], wb_cfg->dest.plane_addr[1],
>> +            wb_cfg->dest.plane_addr[2], wb_cfg->dest.plane_addr[3]);
>> +
>> +    DPU_DEBUG("[fb_stride:%8.8x,%8.8x,%8.8x,%8.8x]\n",
>> +            wb_cfg->dest.plane_pitch[0], wb_cfg->dest.plane_pitch[1],
>> +            wb_cfg->dest.plane_pitch[2], wb_cfg->dest.plane_pitch[3]);
>> +}
>> +
>> +static void dpu_encoder_phys_wb_cleanup_wb_job(struct 
>> dpu_encoder_phys *phys_enc,
>> +        struct drm_writeback_job *job)
>> +{
>> +    struct dpu_encoder_phys_wb *wb_enc = 
>> to_dpu_encoder_phys_wb(phys_enc);
>> +
>> +    if (!job->fb)
>> +        return;
>> +
>> +    msm_framebuffer_cleanup(job->fb, wb_enc->aspace);
>> +    // revisit this after everything else works
> 
> Everything works now, doesn't it?

Yeah, everything works.
i forgot to get rid of this comment :)
Had it in a bunch of other places too and missed one while cleaning up.

> 
>> +    wb_enc->wb_job = NULL;
>> +    wb_enc->wb_conn = NULL;
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_init_ops - initialize writeback operations
>> + * @ops:    Pointer to encoder operation table
>> + */
>> +static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops 
>> *ops)
>> +{
>> +    ops->is_master = dpu_encoder_phys_wb_is_master;
>> +    ops->mode_set = dpu_encoder_phys_wb_mode_set;
>> +    ops->enable = dpu_encoder_phys_wb_enable;
>> +    ops->disable = dpu_encoder_phys_wb_disable;
>> +    ops->destroy = dpu_encoder_phys_wb_destroy;
>> +    ops->atomic_check = dpu_encoder_phys_wb_atomic_check;
>> +    ops->get_hw_resources = dpu_encoder_phys_wb_get_hw_resources;
>> +    ops->wait_for_commit_done = 
>> dpu_encoder_phys_wb_wait_for_commit_done;
>> +    ops->prepare_for_kickoff = dpu_encoder_phys_wb_prepare_for_kickoff;
>> +    ops->handle_post_kickoff = dpu_encoder_phys_wb_handle_post_kickoff;
>> +    ops->needs_single_flush = dpu_encoder_phys_wb_needs_single_flush;
>> +    ops->trigger_start = dpu_encoder_helper_trigger_start;
>> +    ops->prepare_wb_job = dpu_encoder_phys_wb_prepare_wb_job;
>> +    ops->cleanup_wb_job = dpu_encoder_phys_wb_cleanup_wb_job;
>> +    ops->irq_control = dpu_encoder_phys_wb_irq_ctrl;
>> +}
>> +
>> +/**
>> + * dpu_encoder_phys_wb_init - initialize writeback encoder
>> + * @init:    Pointer to init info structure with initialization params
>> + */
>> +struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
>> +        struct dpu_enc_phys_init_params *p)
>> +{
>> +    struct dpu_encoder_phys *phys_enc = NULL;
>> +    struct dpu_encoder_phys_wb *wb_enc = NULL;
>> +
>> +    struct dpu_encoder_irq *irq;
>> +    int ret = 0;
>> +    int i;
>> +
>> +    DPU_DEBUG("\n");
>> +
>> +    if (!p || !p->parent) {
>> +        DPU_ERROR("invalid params\n");
>> +        ret = -EINVAL;
>> +        goto fail_alloc;
>> +    }
>> +
>> +    wb_enc = kzalloc(sizeof(*wb_enc), GFP_KERNEL);
>> +    if (!wb_enc) {
>> +        DPU_ERROR("failed to allocate wb phys_enc enc\n");
>> +        ret = -ENOMEM;
>> +        goto fail_alloc;
>> +    }
>> +
>> +    phys_enc = &wb_enc->base;
>> +    phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
>> +    phys_enc->intf_idx = p->intf_idx;
>> +
>> +    dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
>> +    phys_enc->parent = p->parent;
>> +    phys_enc->parent_ops = p->parent_ops;
>> +    phys_enc->dpu_kms = p->dpu_kms;
>> +    phys_enc->split_role = p->split_role;
>> +    phys_enc->intf_mode = INTF_MODE_WB_LINE;
>> +    phys_enc->intf_idx = p->intf_idx;
>> +    phys_enc->enc_spinlock = p->enc_spinlock;
>> +
>> +    atomic_set(&wb_enc->wbirq_refcount, 0);
>> +
>> +    for (i = 0; i < INTR_IDX_MAX; i++) {
>> +        irq = &phys_enc->irq[i];
>> +        INIT_LIST_HEAD(&irq->cb.list);
>> +        irq->irq_idx = -EINVAL;
>> +        irq->cb.arg = phys_enc;
>> +    }
>> +
>> +    irq = &phys_enc->irq[INTR_IDX_WB_DONE];
>> +    irq->name = "wb_done";
>> +    irq->intr_idx = INTR_IDX_WB_DONE;
>> +    irq->cb.func = dpu_encoder_phys_wb_done_irq;
>> +
>> +    atomic_set(&phys_enc->pending_kickoff_cnt, 0);
>> +    atomic_set(&phys_enc->vblank_refcount, 0);
>> +    wb_enc->wb_done_timeout_cnt = 0;
>> +
>> +    init_waitqueue_head(&phys_enc->pending_kickoff_wq);
>> +    phys_enc->enable_state = DPU_ENC_DISABLED;
>> +
>> +    DPU_DEBUG("Created dpu_encoder_phys for wb %d\n",
>> +            phys_enc->intf_idx);
>> +
>> +    return phys_enc;
>> +
>> +fail_alloc:
>> +    return ERR_PTR(ret);
>> +}
> 
>

On 15/04/2022 01:16, Abhinav Kumar wrote:
> 
> 
> On 2/4/2022 3:19 PM, Dmitry Baryshkov wrote:
>> On 05/02/2022 00:17, Abhinav Kumar wrote:
>>> Introduce the dpu_encoder_phys_* for the writeback interface
>>> to handle writeback specific hardware programming.
>>>
>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>
>> Mostly looks ok, see minor comments bellow.
>>
>>> ---
>>>   drivers/gpu/drm/msm/Makefile                       |   1 +
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |  36 +-
>>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c    | 813 
>>> +++++++++++++++++++++
>>>   3 files changed, 849 insertions(+), 1 deletion(-)
>>>   create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>
>>> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
>>> index c43ef35..3abaf84 100644
>>> --- a/drivers/gpu/drm/msm/Makefile
>>> +++ b/drivers/gpu/drm/msm/Makefile
>>> @@ -53,6 +53,7 @@ msm-y := \
>>>       disp/dpu1/dpu_encoder.o \
>>>       disp/dpu1/dpu_encoder_phys_cmd.o \
>>>       disp/dpu1/dpu_encoder_phys_vid.o \
>>> +    disp/dpu1/dpu_encoder_phys_wb.o \
>>>       disp/dpu1/dpu_formats.o \
>>>       disp/dpu1/dpu_hw_catalog.o \
>>>       disp/dpu1/dpu_hw_ctl.o \
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> index 7b3354d..80da0a9 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> @@ -159,6 +159,7 @@ struct dpu_encoder_phys_ops {
>>>    * @INTR_IDX_PINGPONG: Pingpong done unterrupt for cmd mode panel
>>>    * @INTR_IDX_UNDERRUN: Underrun unterrupt for video and cmd mode panel
>>>    * @INTR_IDX_RDPTR:    Readpointer done unterrupt for cmd mode panel
>>> + * @INTR_IDX_WB_DONE:  Writeback fone interrupt for virtual connector
>>>    */
>>>   enum dpu_intr_idx {
>>>       INTR_IDX_VSYNC,
>>> @@ -166,6 +167,7 @@ enum dpu_intr_idx {
>>>       INTR_IDX_UNDERRUN,
>>>       INTR_IDX_CTL_START,
>>>       INTR_IDX_RDPTR,
>>> +    INTR_IDX_WB_DONE,
>>>       INTR_IDX_MAX,
>>>   };
>>> @@ -196,7 +198,7 @@ struct dpu_encoder_irq {
>>>    * @hw_ctl:        Hardware interface to the ctl registers
>>>    * @hw_pp:        Hardware interface to the ping pong registers
>>>    * @hw_intf:        Hardware interface to the intf registers
>>> - * @hw_wb:             Hardware interface to the wb registers
>>> + * @hw_wb:        Hardware interface to the wb registers
>>>    * @dpu_kms:        Pointer to the dpu_kms top level
>>>    * @cached_mode:    DRM mode cached at mode_set time, acted on in 
>>> enable
>>>    * @enabled:        Whether the encoder has enabled and running a mode
>>> @@ -250,6 +252,31 @@ static inline int 
>>> dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
>>>   }
>>>   /**
>>> + * struct dpu_encoder_phys_wb - sub-class of dpu_encoder_phys to 
>>> handle command
>>> + *    mode specific operations
>>> + * @base:    Baseclass physical encoder structure
>>> + * @wbirq_refcount:     Reference count of writeback interrupt
>>> + * @wb_done_timeout_cnt: number of wb done irq timeout errors
>>> + * @wb_cfg:  writeback block config to store fb related details
>>> + * @cdp_cfg: chroma down prefetch block config for wb
>>> + * @aspace: address space to be used for wb framebuffer
>>> + * @wb_conn: backpointer to writeback connector
>>> + * @wb_job: backpointer to current writeback job
>>> + * @dest:   dpu buffer layout for current writeback output buffer
>>> + */
>>> +struct dpu_encoder_phys_wb {
>>> +    struct dpu_encoder_phys base;
>>> +    atomic_t wbirq_refcount;
>>> +    int wb_done_timeout_cnt;
>>> +    struct dpu_hw_wb_cfg wb_cfg;
>>> +    struct dpu_hw_wb_cdp_cfg cdp_cfg;
>>
>> What about moving them to the stack rather storing them in the structure?
> 
> Yes cdp_cfg can be moved to the stack. Will do it in the next version.
> 
> wb_cfg cannot because for drm_wb, prepare_wb_job has the job's fb 
> attributes which are used to setup the wb.
> 
> But we commit it only in the encoder_kickoff in 
> dpu_encoder_phys_wb_setup_fb().
> 
> So its shared across two methods.
> 
>>
>>> +    struct msm_gem_address_space *aspace;
>>> +    struct drm_writeback_connector *wb_conn;
>>> +    struct drm_writeback_job *wb_job;
>>> +    struct dpu_hw_fmt_layout dest;
>>> +};
>>> +
>>> +/**
>>>    * struct dpu_encoder_phys_cmd - sub-class of dpu_encoder_phys to 
>>> handle command
>>>    *    mode specific operations
>>>    * @base:    Baseclass physical encoder structure
>>> @@ -317,6 +344,13 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>>>           struct dpu_enc_phys_init_params *p);
>>>   /**
>>> + * dpu_encoder_phys_wb_init - initialize writeback encoder
>>> + * @init:    Pointer to init info structure with initialization params
>>> + */
>>> +struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
>>> +        struct dpu_enc_phys_init_params *p);
>>> +
>>> +/**
>>>    * dpu_encoder_helper_trigger_start - control start helper function
>>>    *    This helper function may be optionally specified by physical
>>>    *    encoders if they require ctl_start triggering.
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>> new file mode 100644
>>> index 0000000..783f83e
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>> @@ -0,0 +1,813 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights 
>>> reserved.
>>> + */
>>> +
>>> +#define pr_fmt(fmt)    "[drm:%s:%d] " fmt, __func__, __LINE__
>>> +
>>> +#include <linux/debugfs.h>
>>
>> the header is unused
> ack
>>
>>> +
>>> +#include "dpu_encoder_phys.h"
>>> +#include "dpu_formats.h"
>>> +#include "dpu_hw_top.h"
>>> +#include "dpu_hw_wb.h"
>>> +#include "dpu_hw_lm.h"
>>> +#include "dpu_hw_blk.h"
>>> +#include "dpu_hw_merge3d.h"
>>> +#include "dpu_hw_interrupts.h"
>>> +#include "dpu_core_irq.h"
>>> +#include "dpu_vbif.h"
>>> +#include "dpu_crtc.h"
>>> +#include "disp/msm_disp_snapshot.h"
>>> +
>>> +#define DEFAULT_MAX_WRITEBACK_WIDTH 2048
>>> +
>>> +#define to_dpu_encoder_phys_wb(x) \
>>> +    container_of(x, struct dpu_encoder_phys_wb, base)
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_is_master - report wb always as master encoder
>>> + */
>>> +static bool dpu_encoder_phys_wb_is_master(struct dpu_encoder_phys 
>>> *phys_enc)
>>> +{
>>> +    return true;
>>> +}
>>
>> A comment that there is only one physical enc for WB would be helpful 
>> here. But maybe we should just pull a generic is_master into 
>> dpu_encoder_phys.h (it would work here too).
> 
> I am just following what we have today for phys_vid and phys_cmd.
> But yes, I can leave a comment for sure.
> 
>>
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_set_ot_limit - set OT limit for writeback 
>>> interface
>>> + * @phys_enc:    Pointer to physical encoder
>>> + */
>>> +static void dpu_encoder_phys_wb_set_ot_limit(
>>> +        struct dpu_encoder_phys *phys_enc)
>>> +{
>>> +    struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
>>> +    struct dpu_vbif_set_ot_params ot_params;
>>> +
>>> +    memset(&ot_params, 0, sizeof(ot_params));
>>> +    ot_params.xin_id = hw_wb->caps->xin_id;
>>> +    ot_params.num = hw_wb->idx - WB_0;
>>> +    ot_params.width = phys_enc->cached_mode.hdisplay;
>>> +    ot_params.height = phys_enc->cached_mode.vdisplay;
>>> +    ot_params.is_wfd = true;
>>> +    ot_params.frame_rate = drm_mode_vrefresh(&phys_enc->cached_mode);
>>> +    ot_params.vbif_idx = hw_wb->caps->vbif_idx;
>>> +    ot_params.clk_ctrl = hw_wb->caps->clk_ctrl;
>>> +    ot_params.rd = false;
>>> +
>>> +    dpu_vbif_set_ot_limit(phys_enc->dpu_kms, &ot_params);
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_set_qos_remap - set QoS remapper for writeback
>>> + * @phys_enc:    Pointer to physical encoder
>>> + */
>>> +static void dpu_encoder_phys_wb_set_qos_remap(
>>> +        struct dpu_encoder_phys *phys_enc)
>>> +{
>>> +    struct dpu_hw_wb *hw_wb;
>>> +    struct dpu_vbif_set_qos_params qos_params;
>>> +
>>> +    if (!phys_enc || !phys_enc->parent || !phys_enc->parent->crtc) {
>>> +        DPU_ERROR("invalid arguments\n");
>>> +        return;
>>> +    }
>>> +
>>> +    if (!phys_enc->hw_wb || !phys_enc->hw_wb->caps) {
>>> +        DPU_ERROR("invalid writeback hardware\n");
>>> +        return;
>>> +    }
>>> +
>>> +    hw_wb = phys_enc->hw_wb;
>>> +
>>> +    memset(&qos_params, 0, sizeof(qos_params));
>>> +    qos_params.vbif_idx = hw_wb->caps->vbif_idx;
>>> +    qos_params.xin_id = hw_wb->caps->xin_id;
>>> +    qos_params.clk_ctrl = hw_wb->caps->clk_ctrl;
>>> +    qos_params.num = hw_wb->idx - WB_0;
>>> +    qos_params.is_rt = false;
>>> +
>>> +    DPU_DEBUG("[qos_remap] wb:%d vbif:%d xin:%d is_rt:%d\n",
>>> +            qos_params.num,
>>> +            qos_params.vbif_idx,
>>> +            qos_params.xin_id, qos_params.is_rt);
>>> +
>>> +    dpu_vbif_set_qos_remap(phys_enc->dpu_kms, &qos_params);
>>> +}
>>> +
>>> +//this can be moved to some common file?
>>
>> C99 comment and missing "FIXME: " :-D
> Yes I will remove this since i am addressing this now.
>>
>> Yes, let's pull it into dpu_hw_util.c
>>
>> And if you (by chance) would see a value in unifying other 
>> QoS/CDP-related functions let's move them to the dpu_hw_util.c too.
> 
> Yes I think I can move some utility functions like below to the util 
> file. Will take care of it in next version.
> 
>>
>>> +static u64 _dpu_encoder_phys_wb_get_qos_lut(struct dpu_qos_lut_tbl 
>>> *tbl,
>>> +        u32 total_fl)
>>> +{
>>> +    int i;
>>> +
>>> +    if (!tbl || !tbl->nentry || !tbl->entries)
>>> +        return 0;
>>> +
>>> +    for (i = 0; i < tbl->nentry; i++)
>>> +        if (total_fl <= tbl->entries[i].fl)
>>> +            return tbl->entries[i].lut;
>>> +
>>> +    /* if last fl is zero, use as default */
>>> +    if (!tbl->entries[i-1].fl)
>>> +        return tbl->entries[i-1].lut;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_set_qos - set QoS/danger/safe LUTs for writeback
>>> + * @phys_enc:    Pointer to physical encoder
>>> + */
>>> +static void dpu_encoder_phys_wb_set_qos(struct dpu_encoder_phys 
>>> *phys_enc)
>>> +{
>>> +    struct dpu_hw_wb *hw_wb;
>>> +    struct dpu_hw_wb_qos_cfg qos_cfg;
>>> +    struct dpu_mdss_cfg *catalog;
>>> +    struct dpu_qos_lut_tbl *qos_lut_tb;
>>> +
>>> +    if (!phys_enc || !phys_enc->dpu_kms || 
>>> !phys_enc->dpu_kms->catalog) {
>>> +        DPU_ERROR("invalid parameter(s)\n");
>>> +        return;
>>> +    }
>>> +
>>> +    catalog = phys_enc->dpu_kms->catalog;
>>> +
>>> +    hw_wb = phys_enc->hw_wb;
>>> +
>>> +    memset(&qos_cfg, 0, sizeof(struct dpu_hw_wb_qos_cfg));
>>> +    qos_cfg.danger_safe_en = true;
>>> +    qos_cfg.danger_lut =
>>> +        catalog->perf.danger_lut_tbl[DPU_QOS_LUT_USAGE_NRT];
>>> +
>>> +    qos_cfg.safe_lut = 
>>> catalog->perf.safe_lut_tbl[DPU_QOS_LUT_USAGE_NRT];
>>> +
>>> +    qos_lut_tb = &catalog->perf.qos_lut_tbl[DPU_QOS_LUT_USAGE_NRT];
>>> +    qos_cfg.creq_lut = _dpu_encoder_phys_wb_get_qos_lut(qos_lut_tb, 0);
>>> +
>>> +    if (hw_wb->ops.setup_qos_lut)
>>> +        hw_wb->ops.setup_qos_lut(hw_wb, &qos_cfg);
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_setup_fb - setup output framebuffer
>>> + * @phys_enc:    Pointer to physical encoder
>>> + * @fb:        Pointer to output framebuffer
>>> + * @wb_roi:    Pointer to output region of interest
>>> + */
>>> +static void dpu_encoder_phys_wb_setup_fb(struct dpu_encoder_phys 
>>> *phys_enc,
>>> +        struct drm_framebuffer *fb)
>>> +{
>>> +    struct dpu_encoder_phys_wb *wb_enc = 
>>> to_dpu_encoder_phys_wb(phys_enc);
>>> +    struct dpu_hw_wb *hw_wb;
>>> +    struct dpu_hw_wb_cfg *wb_cfg;
>>> +    struct dpu_hw_wb_cdp_cfg *cdp_cfg;
>>> +
>>> +    if (!phys_enc || !phys_enc->dpu_kms || 
>>> !phys_enc->dpu_kms->catalog ||
>>> +            !phys_enc->connector) {
>>> +        DPU_ERROR("invalid encoder\n");
>>> +        return;
>>> +    }
>>> +
>>> +    hw_wb = phys_enc->hw_wb;
>>> +    wb_cfg = &wb_enc->wb_cfg;
>>> +    cdp_cfg = &wb_enc->cdp_cfg;
>>> +
>>> +    wb_cfg->intf_mode = phys_enc->intf_mode;
>>> +    wb_cfg->roi.x1 = 0;
>>> +    wb_cfg->roi.x2 = phys_enc->cached_mode.hdisplay;
>>> +    wb_cfg->roi.y1 = 0;
>>> +    wb_cfg->roi.y2 = phys_enc->cached_mode.vdisplay;
>>> +
>>> +    if (hw_wb->ops.setup_roi)
>>> +        hw_wb->ops.setup_roi(hw_wb, wb_cfg);
>>> +
>>> +    if (hw_wb->ops.setup_outformat)
>>> +        hw_wb->ops.setup_outformat(hw_wb, wb_cfg);
>>> +
>>> +    if (hw_wb->ops.setup_cdp) {
>>> +        memset(cdp_cfg, 0, sizeof(struct dpu_hw_wb_cdp_cfg));
>>> +
>>> +        cdp_cfg->enable = phys_enc->dpu_kms->catalog->perf.cdp_cfg
>>> +                [DPU_PERF_CDP_USAGE_NRT].wr_enable;
>>> +        cdp_cfg->ubwc_meta_enable =
>>> +                DPU_FORMAT_IS_UBWC(wb_cfg->dest.format);
>>> +        cdp_cfg->tile_amortize_enable =
>>> +                DPU_FORMAT_IS_UBWC(wb_cfg->dest.format) ||
>>> +                DPU_FORMAT_IS_TILE(wb_cfg->dest.format);
>>> +        cdp_cfg->preload_ahead = DPU_WB_CDP_PRELOAD_AHEAD_64;
>>> +
>>> +        hw_wb->ops.setup_cdp(hw_wb, cdp_cfg);
>>> +    }
>>> +
>>> +    if (hw_wb->ops.setup_outaddress)
>>> +        hw_wb->ops.setup_outaddress(hw_wb, wb_cfg);
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_setup_cdp - setup chroma down prefetch block
>>> + * @phys_enc:Pointer to physical encoder
>>> + */
>>> +static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys 
>>> *phys_enc)
>>> +{
>>> +    struct dpu_hw_wb *hw_wb;
>>> +    struct dpu_hw_ctl *ctl;
>>> +
>>> +    if (!phys_enc) {
>>> +        DPU_ERROR("invalid encoder\n");
>>> +        return;
>>> +    }
>>> +
>>> +    hw_wb = phys_enc->hw_wb;
>>> +    ctl = phys_enc->hw_ctl;
>>> +
>>> +    if (test_bit(DPU_CTL_ACTIVE_CFG, &ctl->caps->features) &&
>>> +        (phys_enc->hw_ctl &&
>>> +         phys_enc->hw_ctl->ops.setup_intf_cfg)) {
>>> +        struct dpu_hw_intf_cfg intf_cfg = {0};
>>> +        struct dpu_hw_pingpong *hw_pp = phys_enc->hw_pp;
>>> +        enum dpu_3d_blend_mode mode_3d;
>>> +
>>> +        mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>> +
>>> +        intf_cfg.intf = DPU_NONE;
>>> +        intf_cfg.wb = hw_wb->idx;
>>> +
>>> +        if (mode_3d && hw_pp && hw_pp->merge_3d)
>>> +            intf_cfg.merge_3d = hw_pp->merge_3d->idx;
>>> +
>>> +        if (phys_enc->hw_pp->merge_3d && 
>>> phys_enc->hw_pp->merge_3d->ops.setup_3d_mode)
>>> + 
>>> phys_enc->hw_pp->merge_3d->ops.setup_3d_mode(phys_enc->hw_pp->merge_3d,
>>> +                    mode_3d);
>>> +
>>> +        /* setup which pp blk will connect to this wb */
>>> +        if (hw_pp && phys_enc->hw_wb->ops.bind_pingpong_blk)
>>> +            phys_enc->hw_wb->ops.bind_pingpong_blk(phys_enc->hw_wb, 
>>> true,
>>> +                    phys_enc->hw_pp->idx);
>>> +
>>> +        phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl,
>>> +                &intf_cfg, true);
>>> +    } else if (phys_enc->hw_ctl && 
>>> phys_enc->hw_ctl->ops.setup_intf_cfg) {
>>> +        struct dpu_hw_intf_cfg intf_cfg = {0};
>>> +
>>> +        intf_cfg.intf = DPU_NONE;
>>> +        intf_cfg.wb = hw_wb->idx;
>>> +        intf_cfg.mode_3d =
>>> +            dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>> +        phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, 
>>> &intf_cfg, true);
>>> +    }
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_atomic_check - verify and fixup given atomic 
>>> states
>>> + * @phys_enc:    Pointer to physical encoder
>>> + * @crtc_state:    Pointer to CRTC atomic state
>>> + * @conn_state:    Pointer to connector atomic state
>>> + */
>>> +static int dpu_encoder_phys_wb_atomic_check(
>>> +        struct dpu_encoder_phys *phys_enc,
>>> +        struct drm_crtc_state *crtc_state,
>>> +        struct drm_connector_state *conn_state)
>>> +{
>>> +    struct drm_framebuffer *fb;
>>> +    const struct drm_display_mode *mode;
>>> +
>>> +    DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
>>> +            phys_enc->intf_idx, mode->name, mode->hdisplay, 
>>> mode->vdisplay);
>>> +
>>> +    if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
>>> +        return 0;
>>> +
>>> +    fb = conn_state->writeback_job->fb;
>>> +    mode = &crtc_state->mode;
>>> +
>>> +    if (!conn_state || !conn_state->connector) {
>>> +        DPU_ERROR("invalid connector state\n");
>>> +        return -EINVAL;
>>> +    } else if (conn_state->connector->status !=
>>> +            connector_status_connected) {
>>> +        DPU_ERROR("connector not connected %d\n",
>>> +                conn_state->connector->status);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    DPU_DEBUG("[fb_id:%u][fb:%u,%u]\n", fb->base.id,
>>> +            fb->width, fb->height);
>>> +
>>> +    if (fb->width != mode->hdisplay) {
>>> +        DPU_ERROR("invalid fb w=%d, mode w=%d\n", fb->width,
>>> +                mode->hdisplay);
>>> +        return -EINVAL;
>>> +    } else if (fb->height != mode->vdisplay) {
>>> +        DPU_ERROR("invalid fb h=%d, mode h=%d\n", fb->height,
>>> +                  mode->vdisplay);
>>> +        return -EINVAL;
>>> +    } else if (fb->width > DEFAULT_MAX_WRITEBACK_WIDTH) {
>>> +        DPU_ERROR("invalid fb w=%d, maxlinewidth=%u\n",
>>> +                  fb->width, DEFAULT_MAX_WRITEBACK_WIDTH);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +
>>> +/**
>>> + * _dpu_encoder_phys_wb_update_flush - flush hardware update
>>> + * @phys_enc:    Pointer to physical encoder
>>> + */
>>> +static void _dpu_encoder_phys_wb_update_flush(struct 
>>> dpu_encoder_phys *phys_enc)
>>> +{
>>> +    struct dpu_hw_wb *hw_wb;
>>> +    struct dpu_hw_ctl *hw_ctl;
>>> +    struct dpu_hw_pingpong *hw_pp;
>>> +    u32 pending_flush = 0;
>>> +
>>> +    if (!phys_enc)
>>> +        return;
>>> +
>>> +    hw_wb = phys_enc->hw_wb;
>>> +    hw_pp = phys_enc->hw_pp;
>>> +    hw_ctl = phys_enc->hw_ctl;
>>> +
>>> +    DPU_DEBUG("[wb:%d]\n", hw_wb->idx - WB_0);
>>> +
>>> +    if (!hw_ctl) {
>>> +        DPU_DEBUG("[wb:%d] no ctl assigned\n", hw_wb->idx - WB_0);
>>> +        return;
>>> +    }
>>> +
>>> +    if (hw_ctl->ops.update_pending_flush_wb)
>>> +        hw_ctl->ops.update_pending_flush_wb(hw_ctl, hw_wb->idx);
>>> +
>>> +    if (hw_ctl->ops.update_pending_flush_merge_3d && hw_pp && 
>>> hw_pp->merge_3d)
>>> +        hw_ctl->ops.update_pending_flush_merge_3d(hw_ctl,
>>> +                hw_pp->merge_3d->idx);
>>> +
>>> +    if (hw_ctl->ops.get_pending_flush)
>>> +        pending_flush = hw_ctl->ops.get_pending_flush(hw_ctl);
>>> +
>>> +    DPU_DEBUG("Pending flush mask for CTL_%d is 0x%x, WB %d\n",
>>> +            hw_ctl->idx - CTL_0, pending_flush,
>>> +            hw_wb->idx - WB_0);
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_setup - setup writeback encoder
>>> + * @phys_enc:    Pointer to physical encoder
>>> + */
>>> +static void dpu_encoder_phys_wb_setup(
>>> +        struct dpu_encoder_phys *phys_enc)
>>> +{
>>> +    struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
>>> +    struct drm_display_mode mode = phys_enc->cached_mode;
>>> +    struct drm_framebuffer *fb = NULL;
>>> +
>>> +    DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n",
>>> +            hw_wb->idx - WB_0, mode.name,
>>> +            mode.hdisplay, mode.vdisplay);
>>> +
>>> +    dpu_encoder_phys_wb_set_ot_limit(phys_enc);
>>> +
>>> +    dpu_encoder_phys_wb_set_qos_remap(phys_enc);
>>> +
>>> +    dpu_encoder_phys_wb_set_qos(phys_enc);
>>> +
>>> +    dpu_encoder_phys_wb_setup_fb(phys_enc, fb);
>>> +
>>> +    dpu_encoder_phys_wb_setup_cdp(phys_enc);
>>> +
>>> +}
>>> +
>>> +static void _dpu_encoder_phys_wb_frame_done_helper(void *arg)
>>> +{
>>> +    struct dpu_encoder_phys *phys_enc = arg;
>>> +    struct dpu_encoder_phys_wb *wb_enc = 
>>> to_dpu_encoder_phys_wb(phys_enc);
>>> +
>>> +    struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
>>> +    unsigned long lock_flags;
>>> +    u32 event = DPU_ENCODER_FRAME_EVENT_DONE;
>>> +
>>> +    DPU_DEBUG("[wb:%d]\n", hw_wb->idx - WB_0);
>>> +
>>> +    if (phys_enc->parent_ops->handle_frame_done)
>>> +        phys_enc->parent_ops->handle_frame_done(phys_enc->parent,
>>> +                phys_enc, event);
>>> +
>>> +    if (phys_enc->parent_ops->handle_vblank_virt)
>>> +        phys_enc->parent_ops->handle_vblank_virt(phys_enc->parent,
>>> +                phys_enc);
>>> +
>>> +    spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
>>> +    atomic_add_unless(&phys_enc->pending_kickoff_cnt, -1, 0);
>>> +    spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
>>> +
>>> +    if (wb_enc->wb_conn)
>>> +        drm_writeback_signal_completion(wb_enc->wb_conn, 0);
>>> +
>>> +    /* Signal any waiting atomic commit thread */
>>> +    wake_up_all(&phys_enc->pending_kickoff_wq);
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_done_irq - writeback interrupt handler
>>> + * @arg:    Pointer to writeback encoder
>>> + * @irq_idx:    interrupt index
>>> + */
>>> +static void dpu_encoder_phys_wb_done_irq(void *arg, int irq_idx)
>>> +{
>>> +    _dpu_encoder_phys_wb_frame_done_helper(arg);
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_irq_ctrl - irq control of WB
>>> + * @phys:    Pointer to physical encoder
>>> + * @enable:    indicates enable or disable interrupts
>>> + */
>>> +static void dpu_encoder_phys_wb_irq_ctrl(
>>> +        struct dpu_encoder_phys *phys, bool enable)
>>> +{
>>> +
>>> +    struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys);
>>> +    int ret = 0;
>>> +    int refcount;
>>> +
>>> +    refcount = atomic_read(&wb_enc->wbirq_refcount);
>>> +
>>> +    if (enable && atomic_inc_return(&wb_enc->wbirq_refcount) == 1) {
>>> +        dpu_encoder_helper_register_irq(phys, INTR_IDX_WB_DONE);
>>> +        if (ret)
>>> +            atomic_dec_return(&wb_enc->wbirq_refcount);
>>> +    } else if (!enable &&
>>> +            atomic_dec_return(&wb_enc->wbirq_refcount) == 0) {
>>> +        dpu_encoder_helper_unregister_irq(phys, INTR_IDX_WB_DONE);
>>> +        if (ret)
>>> +            atomic_inc_return(&wb_enc->wbirq_refcount);
>>> +    }
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_mode_set - set display mode
>>> + * @phys_enc:    Pointer to physical encoder
>>> + * @mode:    Pointer to requested display mode
>>> + * @adj_mode:    Pointer to adjusted display mode
>>> + */
>>> +static void dpu_encoder_phys_wb_mode_set(
>>> +        struct dpu_encoder_phys *phys_enc,
>>> +        struct drm_display_mode *mode,
>>> +        struct drm_display_mode *adj_mode)
>>> +{
>>> +    struct dpu_encoder_irq *irq;
>>> +
>>> +    if (adj_mode) {
>>> +        phys_enc->cached_mode = *adj_mode;
>>> +        drm_mode_debug_printmodeline(adj_mode);
>>> +        DPU_DEBUG("caching mode:\n");
>>> +    }
>>> +
>>> +    irq = &phys_enc->irq[INTR_IDX_WB_DONE];
>>> +    irq->irq_idx = phys_enc->hw_wb->caps->intr_wb_done;
>>> +}
>>> +
>>> +static void _dpu_encoder_phys_wb_handle_wbdone_timeout(
>>> +        struct dpu_encoder_phys *phys_enc)
>>> +{
>>> +    struct dpu_encoder_phys_wb *wb_enc = 
>>> to_dpu_encoder_phys_wb(phys_enc);
>>> +    u32 frame_event = DPU_ENCODER_FRAME_EVENT_ERROR;
>>> +
>>> +    wb_enc->wb_done_timeout_cnt++;
>>> +
>>> +    if (wb_enc->wb_done_timeout_cnt == 1)
>>> +        msm_disp_snapshot_state(phys_enc->parent->dev);
>>> +
>>> +    atomic_add_unless(&phys_enc->pending_kickoff_cnt, -1, 0);
>>> +
>>> +    /* request a ctl reset before the next kickoff */
>>> +    phys_enc->enable_state = DPU_ENC_ERR_NEEDS_HW_RESET;
>>> +
>>> +    if (wb_enc->wb_conn)
>>> +        drm_writeback_signal_completion(wb_enc->wb_conn, 0);
>>> +
>>> +    if (phys_enc->parent_ops->handle_frame_done)
>>> +        phys_enc->parent_ops->handle_frame_done(
>>> +                phys_enc->parent, phys_enc, frame_event);
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_wait_for_commit_done - wait until request is 
>>> committed
>>> + * @phys_enc:    Pointer to physical encoder
>>> + */
>>> +static int dpu_encoder_phys_wb_wait_for_commit_done(
>>> +        struct dpu_encoder_phys *phys_enc)
>>> +{
>>> +    unsigned long ret;
>>> +    struct dpu_encoder_wait_info wait_info;
>>> +    struct dpu_encoder_phys_wb *wb_enc = 
>>> to_dpu_encoder_phys_wb(phys_enc);
>>> +
>>> +    wait_info.wq = &phys_enc->pending_kickoff_wq;
>>> +    wait_info.atomic_cnt = &phys_enc->pending_kickoff_cnt;
>>> +    wait_info.timeout_ms = KICKOFF_TIMEOUT_MS;
>>> +
>>> +    ret = dpu_encoder_helper_wait_for_irq(phys_enc, INTR_IDX_WB_DONE,
>>> +            &wait_info);
>>> +    if (ret == -ETIMEDOUT)
>>> +        _dpu_encoder_phys_wb_handle_wbdone_timeout(phys_enc);
>>> +    else if (!ret)
>>> +        wb_enc->wb_done_timeout_cnt = 0;
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_prepare_for_kickoff - pre-kickoff processing
>>> + * @phys_enc:    Pointer to physical encoder
>>> + * Returns:    Zero on success
>>> + */
>>> +static void dpu_encoder_phys_wb_prepare_for_kickoff(
>>> +        struct dpu_encoder_phys *phys_enc)
>>> +{
>>> +    struct dpu_encoder_phys_wb *wb_enc = 
>>> to_dpu_encoder_phys_wb(phys_enc);
>>> +    struct drm_connector *drm_conn;
>>> +    struct drm_connector_state *state;
>>> +
>>> +    DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
>>> +
>>> +    if (!wb_enc->wb_conn || !wb_enc->wb_job) {
>>> +        DPU_ERROR("invalid wb_conn or wb_job\n");
>>> +        return;
>>> +    }
>>> +
>>> +    drm_conn = wb_enc->wb_conn->base;
>>> +    state = drm_conn->state;
>>> +
>>> +    if (wb_enc->wb_conn && wb_enc->wb_job)
>>> +        drm_writeback_queue_job(wb_enc->wb_conn, state);
>>> +
>>> +    dpu_encoder_phys_wb_setup(phys_enc);
>>> +
>>> +    _dpu_encoder_phys_wb_update_flush(phys_enc);
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_needs_single_flush - trigger flush processing
>>> + * @phys_enc:    Pointer to physical encoder
>>> + */
>>> +static bool dpu_encoder_phys_wb_needs_single_flush(struct 
>>> dpu_encoder_phys *phys_enc)
>>> +{
>>> +    DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
>>> +    return false;
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_handle_post_kickoff - post-kickoff processing
>>> + * @phys_enc:    Pointer to physical encoder
>>> + */
>>> +static void dpu_encoder_phys_wb_handle_post_kickoff(
>>> +        struct dpu_encoder_phys *phys_enc)
>>> +{
>>> +    DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
>>> +
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_enable - enable writeback encoder
>>> + * @phys_enc:    Pointer to physical encoder
>>> + */
>>> +static void dpu_encoder_phys_wb_enable(struct dpu_encoder_phys 
>>> *phys_enc)
>>> +{
>>> +    DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
>>> +    phys_enc->enable_state = DPU_ENC_ENABLED;
>>> +}
>>> +/**
>>> + * dpu_encoder_phys_wb_disable - disable writeback encoder
>>> + * @phys_enc:    Pointer to physical encoder
>>> + */
>>> +static void dpu_encoder_phys_wb_disable(struct dpu_encoder_phys 
>>> *phys_enc)
>>> +{
>>> +    struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
>>> +
>>> +    DPU_DEBUG("[wb:%d]\n", hw_wb->idx - WB_0);
>>> +
>>> +    if (phys_enc->enable_state == DPU_ENC_DISABLED) {
>>> +        DPU_ERROR("encoder is already disabled\n");
>>> +        return;
>>> +    }
>>> +
>>> +    /* reset h/w before final flush */
>>> +    if (phys_enc->hw_ctl->ops.clear_pending_flush)
>>> +        phys_enc->hw_ctl->ops.clear_pending_flush(phys_enc->hw_ctl);
>>> +
>>> +    /*
>>> +     * New CTL reset sequence from 5.0 MDP onwards.
>>> +     * If has_3d_merge_reset is not set, legacy reset
>>> +     * sequence is executed.
>>> +     *
>>> +     * Legacy reset sequence has not been implemented yet.
>>> +     * Any target earlier than SM8150 will need it and when
>>> +     * WB support is added to those targets will need to add
>>> +     * the legacy teardown sequence as well.
>>> +     */
>>> +    if (phys_enc->hw_pp->merge_3d)
>>
>> This is a bit.. counterintuitive. I'd prefer hw_ctl->caps->features & 
>> BIT(DPU_ACTIVE_CTL).
> Your suggestion should work.
> 
> But consider it this way.
> 
> Checking caps is telling us if the chipset supports merge3d which is 
> sufficient in this case but phys_enc->hw_pp->merge_3d is telling us 
> whether merge_3d is also being used in this RM session.
> 
> So i just thought its a better check to cover both.

hw_pp->merge_3d is set when PP has a MERGE_3D attached in hardware, no 
matter if it is used in the current pipeline or not.

As I wrote, the code is counterintuitive. The comment talks about MDP 
5.0, but the condition checks hw_pp->merge_3d, which might not be 
present for tons of other reasons (starting from somebody forgetting to 
list it in the hw_catalog).

And in fact there is a followup question. Can we push this check (and a 
legacy reset sequence) into the dpu_encoder_helper_phys_cleanup() 
itself? Thus the dpu_encoder_phys will just call _helper_phys_cleanup() 
and let the helper care about hardware details.


>>> +        dpu_encoder_helper_phys_cleanup(phys_enc);
>>> +
>>> +    phys_enc->enable_state = DPU_ENC_DISABLED;
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_get_hw_resources - get hardware resources
>>> + * @phys_enc:    Pointer to physical encoder
>>> + * @hw_res:    Pointer to encoder resources
>>> + */
>>> +static void dpu_encoder_phys_wb_get_hw_resources(
>>> +        struct dpu_encoder_phys *phys_enc,
>>> +        struct dpu_encoder_hw_resources *hw_res)
>>> +{
>>> +    if (!phys_enc) {
>>> +        DPU_ERROR("invalid encoder\n");
>>> +        return;
>>> +    }
>>> +
>>> +    hw_res->wbs[phys_enc->intf_idx - WB_0] = INTF_MODE_WB_LINE;
>>> +    DPU_DEBUG("[wb:%d] intf_mode=%d\n", phys_enc->intf_idx - WB_0,
>>> +            hw_res->wbs[phys_enc->intf_idx - WB_0]);
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_destroy - destroy writeback encoder
>>> + * @phys_enc:    Pointer to physical encoder
>>> + */
>>> +static void dpu_encoder_phys_wb_destroy(struct dpu_encoder_phys 
>>> *phys_enc)
>>> +{
>>> +    DPU_DEBUG("[wb:%d]\n", phys_enc->intf_idx - INTF_0);
>>> +
>>> +    if (!phys_enc)
>>> +        return;
>>> +
>>> +    kfree(phys_enc);
>>> +}
>>> +
>>> +static void dpu_encoder_phys_wb_prepare_wb_job(struct 
>>> dpu_encoder_phys *phys_enc,
>>> +        struct drm_writeback_job *job)
>>> +{
>>> +    const struct msm_format *format;
>>> +    struct dpu_hw_wb_cfg *wb_cfg;
>>> +    int ret;
>>> +    struct dpu_encoder_phys_wb *wb_enc = 
>>> to_dpu_encoder_phys_wb(phys_enc);
>>> +
>>> +    if (!job->fb)
>>> +        return;
>>> +
>>> +    wb_enc->wb_job = job;
>>> +    wb_enc->wb_conn = job->connector;
>>> +    wb_enc->aspace = phys_enc->dpu_kms->base.aspace;
>>
>> Any particular reason to cache it in wb_enc?
> 
> We can technically get this from phys_enc->dpu_kms->base.aspace so I can 
> drop this.
> 
>>
>>> +
>>> +    wb_cfg = &wb_enc->wb_cfg;
>>> +
>>> +    memset(wb_cfg, 0, sizeof(struct dpu_hw_wb_cfg));
>>> +
>>> +    ret = msm_framebuffer_prepare(job->fb, wb_enc->aspace);
>>> +    if (ret) {
>>> +        DPU_ERROR("prep fb failed, %d\n", ret);
>>> +        return;
>>> +    }
>>> +
>>> +    format = msm_framebuffer_format(job->fb);
>>> +
>>> +    wb_cfg->dest.format = dpu_get_dpu_format_ext(
>>> +            format->pixel_format, job->fb->modifier);
>>> +    if (!wb_cfg->dest.format) {
>>> +        /* this error should be detected during atomic_check */
>>> +        DPU_ERROR("failed to get format %x\n", format->pixel_format);
>>> +        return;
>>> +    }
>>> +
>>> +    ret = dpu_format_populate_layout(wb_enc->aspace, job->fb, 
>>> &wb_cfg->dest);
>>> +    if (ret) {
>>> +        DPU_DEBUG("failed to populate layout %d\n", ret);
>>> +        return;
>>> +    }
>>> +
>>> +    wb_cfg->dest.width = job->fb->width;
>>> +    wb_cfg->dest.height = job->fb->height;
>>> +    wb_cfg->dest.num_planes = wb_cfg->dest.format->num_planes;
>>> +
>>> +    if ((wb_cfg->dest.format->fetch_planes == DPU_PLANE_PLANAR) &&
>>> +            (wb_cfg->dest.format->element[0] == C1_B_Cb))
>>> +        swap(wb_cfg->dest.plane_addr[1], wb_cfg->dest.plane_addr[2]);
>>> +
>>> +    DPU_DEBUG("[fb_offset:%8.8x,%8.8x,%8.8x,%8.8x]\n",
>>> +            wb_cfg->dest.plane_addr[0], wb_cfg->dest.plane_addr[1],
>>> +            wb_cfg->dest.plane_addr[2], wb_cfg->dest.plane_addr[3]);
>>> +
>>> +    DPU_DEBUG("[fb_stride:%8.8x,%8.8x,%8.8x,%8.8x]\n",
>>> +            wb_cfg->dest.plane_pitch[0], wb_cfg->dest.plane_pitch[1],
>>> +            wb_cfg->dest.plane_pitch[2], wb_cfg->dest.plane_pitch[3]);
>>> +}
>>> +
>>> +static void dpu_encoder_phys_wb_cleanup_wb_job(struct 
>>> dpu_encoder_phys *phys_enc,
>>> +        struct drm_writeback_job *job)
>>> +{
>>> +    struct dpu_encoder_phys_wb *wb_enc = 
>>> to_dpu_encoder_phys_wb(phys_enc);
>>> +
>>> +    if (!job->fb)
>>> +        return;
>>> +
>>> +    msm_framebuffer_cleanup(job->fb, wb_enc->aspace);
>>> +    // revisit this after everything else works
>>
>> Everything works now, doesn't it?
> 
> Yeah, everything works.
> i forgot to get rid of this comment :)
> Had it in a bunch of other places too and missed one while cleaning up.
> 
>>
>>> +    wb_enc->wb_job = NULL;
>>> +    wb_enc->wb_conn = NULL;
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_init_ops - initialize writeback operations
>>> + * @ops:    Pointer to encoder operation table
>>> + */
>>> +static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops 
>>> *ops)
>>> +{
>>> +    ops->is_master = dpu_encoder_phys_wb_is_master;
>>> +    ops->mode_set = dpu_encoder_phys_wb_mode_set;
>>> +    ops->enable = dpu_encoder_phys_wb_enable;
>>> +    ops->disable = dpu_encoder_phys_wb_disable;
>>> +    ops->destroy = dpu_encoder_phys_wb_destroy;
>>> +    ops->atomic_check = dpu_encoder_phys_wb_atomic_check;
>>> +    ops->get_hw_resources = dpu_encoder_phys_wb_get_hw_resources;
>>> +    ops->wait_for_commit_done = 
>>> dpu_encoder_phys_wb_wait_for_commit_done;
>>> +    ops->prepare_for_kickoff = dpu_encoder_phys_wb_prepare_for_kickoff;
>>> +    ops->handle_post_kickoff = dpu_encoder_phys_wb_handle_post_kickoff;
>>> +    ops->needs_single_flush = dpu_encoder_phys_wb_needs_single_flush;
>>> +    ops->trigger_start = dpu_encoder_helper_trigger_start;
>>> +    ops->prepare_wb_job = dpu_encoder_phys_wb_prepare_wb_job;
>>> +    ops->cleanup_wb_job = dpu_encoder_phys_wb_cleanup_wb_job;
>>> +    ops->irq_control = dpu_encoder_phys_wb_irq_ctrl;
>>> +}
>>> +
>>> +/**
>>> + * dpu_encoder_phys_wb_init - initialize writeback encoder
>>> + * @init:    Pointer to init info structure with initialization params
>>> + */
>>> +struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
>>> +        struct dpu_enc_phys_init_params *p)
>>> +{
>>> +    struct dpu_encoder_phys *phys_enc = NULL;
>>> +    struct dpu_encoder_phys_wb *wb_enc = NULL;
>>> +
>>> +    struct dpu_encoder_irq *irq;
>>> +    int ret = 0;
>>> +    int i;
>>> +
>>> +    DPU_DEBUG("\n");
>>> +
>>> +    if (!p || !p->parent) {
>>> +        DPU_ERROR("invalid params\n");
>>> +        ret = -EINVAL;
>>> +        goto fail_alloc;
>>> +    }
>>> +
>>> +    wb_enc = kzalloc(sizeof(*wb_enc), GFP_KERNEL);
>>> +    if (!wb_enc) {
>>> +        DPU_ERROR("failed to allocate wb phys_enc enc\n");
>>> +        ret = -ENOMEM;
>>> +        goto fail_alloc;
>>> +    }
>>> +
>>> +    phys_enc = &wb_enc->base;
>>> +    phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
>>> +    phys_enc->intf_idx = p->intf_idx;
>>> +
>>> +    dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
>>> +    phys_enc->parent = p->parent;
>>> +    phys_enc->parent_ops = p->parent_ops;
>>> +    phys_enc->dpu_kms = p->dpu_kms;
>>> +    phys_enc->split_role = p->split_role;
>>> +    phys_enc->intf_mode = INTF_MODE_WB_LINE;
>>> +    phys_enc->intf_idx = p->intf_idx;
>>> +    phys_enc->enc_spinlock = p->enc_spinlock;
>>> +
>>> +    atomic_set(&wb_enc->wbirq_refcount, 0);
>>> +
>>> +    for (i = 0; i < INTR_IDX_MAX; i++) {
>>> +        irq = &phys_enc->irq[i];
>>> +        INIT_LIST_HEAD(&irq->cb.list);
>>> +        irq->irq_idx = -EINVAL;
>>> +        irq->cb.arg = phys_enc;
>>> +    }
>>> +
>>> +    irq = &phys_enc->irq[INTR_IDX_WB_DONE];
>>> +    irq->name = "wb_done";
>>> +    irq->intr_idx = INTR_IDX_WB_DONE;
>>> +    irq->cb.func = dpu_encoder_phys_wb_done_irq;
>>> +
>>> +    atomic_set(&phys_enc->pending_kickoff_cnt, 0);
>>> +    atomic_set(&phys_enc->vblank_refcount, 0);
>>> +    wb_enc->wb_done_timeout_cnt = 0;
>>> +
>>> +    init_waitqueue_head(&phys_enc->pending_kickoff_wq);
>>> +    phys_enc->enable_state = DPU_ENC_DISABLED;
>>> +
>>> +    DPU_DEBUG("Created dpu_encoder_phys for wb %d\n",
>>> +            phys_enc->intf_idx);
>>> +
>>> +    return phys_enc;
>>> +
>>> +fail_alloc:
>>> +    return ERR_PTR(ret);
>>> +}
>>
>>

On 4/14/2022 5:24 PM, Dmitry Baryshkov wrote:
> On 15/04/2022 01:16, Abhinav Kumar wrote:
>>
>>
>> On 2/4/2022 3:19 PM, Dmitry Baryshkov wrote:
>>> On 05/02/2022 00:17, Abhinav Kumar wrote:
>>>> Introduce the dpu_encoder_phys_* for the writeback interface
>>>> to handle writeback specific hardware programming.
>>>>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>
>>> Mostly looks ok, see minor comments bellow.
>>>
>>>> ---
>>>>   drivers/gpu/drm/msm/Makefile                       |   1 +
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |  36 +-
>>>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c    | 813 
>>>> +++++++++++++++++++++
>>>>   3 files changed, 849 insertions(+), 1 deletion(-)
>>>>   create mode 100644 
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/Makefile 
>>>> b/drivers/gpu/drm/msm/Makefile
>>>> index c43ef35..3abaf84 100644
>>>> --- a/drivers/gpu/drm/msm/Makefile
>>>> +++ b/drivers/gpu/drm/msm/Makefile
>>>> @@ -53,6 +53,7 @@ msm-y := \
>>>>       disp/dpu1/dpu_encoder.o \
>>>>       disp/dpu1/dpu_encoder_phys_cmd.o \
>>>>       disp/dpu1/dpu_encoder_phys_vid.o \
>>>> +    disp/dpu1/dpu_encoder_phys_wb.o \
>>>>       disp/dpu1/dpu_formats.o \
>>>>       disp/dpu1/dpu_hw_catalog.o \
>>>>       disp/dpu1/dpu_hw_ctl.o \
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>> index 7b3354d..80da0a9 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>> @@ -159,6 +159,7 @@ struct dpu_encoder_phys_ops {
>>>>    * @INTR_IDX_PINGPONG: Pingpong done unterrupt for cmd mode panel
>>>>    * @INTR_IDX_UNDERRUN: Underrun unterrupt for video and cmd mode 
>>>> panel
>>>>    * @INTR_IDX_RDPTR:    Readpointer done unterrupt for cmd mode panel
>>>> + * @INTR_IDX_WB_DONE:  Writeback fone interrupt for virtual connector
>>>>    */
>>>>   enum dpu_intr_idx {
>>>>       INTR_IDX_VSYNC,
>>>> @@ -166,6 +167,7 @@ enum dpu_intr_idx {
>>>>       INTR_IDX_UNDERRUN,
>>>>       INTR_IDX_CTL_START,
>>>>       INTR_IDX_RDPTR,
>>>> +    INTR_IDX_WB_DONE,
>>>>       INTR_IDX_MAX,
>>>>   };
>>>> @@ -196,7 +198,7 @@ struct dpu_encoder_irq {
>>>>    * @hw_ctl:        Hardware interface to the ctl registers
>>>>    * @hw_pp:        Hardware interface to the ping pong registers
>>>>    * @hw_intf:        Hardware interface to the intf registers
>>>> - * @hw_wb:             Hardware interface to the wb registers
>>>> + * @hw_wb:        Hardware interface to the wb registers
>>>>    * @dpu_kms:        Pointer to the dpu_kms top level
>>>>    * @cached_mode:    DRM mode cached at mode_set time, acted on in 
>>>> enable
>>>>    * @enabled:        Whether the encoder has enabled and running a 
>>>> mode
>>>> @@ -250,6 +252,31 @@ static inline int 
>>>> dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
>>>>   }
>>>>   /**
>>>> + * struct dpu_encoder_phys_wb - sub-class of dpu_encoder_phys to 
>>>> handle command
>>>> + *    mode specific operations
>>>> + * @base:    Baseclass physical encoder structure
>>>> + * @wbirq_refcount:     Reference count of writeback interrupt
>>>> + * @wb_done_timeout_cnt: number of wb done irq timeout errors
>>>> + * @wb_cfg:  writeback block config to store fb related details
>>>> + * @cdp_cfg: chroma down prefetch block config for wb
>>>> + * @aspace: address space to be used for wb framebuffer
>>>> + * @wb_conn: backpointer to writeback connector
>>>> + * @wb_job: backpointer to current writeback job
>>>> + * @dest:   dpu buffer layout for current writeback output buffer
>>>> + */
>>>> +struct dpu_encoder_phys_wb {
>>>> +    struct dpu_encoder_phys base;
>>>> +    atomic_t wbirq_refcount;
>>>> +    int wb_done_timeout_cnt;
>>>> +    struct dpu_hw_wb_cfg wb_cfg;
>>>> +    struct dpu_hw_wb_cdp_cfg cdp_cfg;
>>>
>>> What about moving them to the stack rather storing them in the 
>>> structure?
>>
>> Yes cdp_cfg can be moved to the stack. Will do it in the next version.
>>
>> wb_cfg cannot because for drm_wb, prepare_wb_job has the job's fb 
>> attributes which are used to setup the wb.
>>
>> But we commit it only in the encoder_kickoff in 
>> dpu_encoder_phys_wb_setup_fb().
>>
>> So its shared across two methods.
>>
>>>
>>>> +    struct msm_gem_address_space *aspace;
>>>> +    struct drm_writeback_connector *wb_conn;
>>>> +    struct drm_writeback_job *wb_job;
>>>> +    struct dpu_hw_fmt_layout dest;
>>>> +};
>>>> +
>>>> +/**
>>>>    * struct dpu_encoder_phys_cmd - sub-class of dpu_encoder_phys to 
>>>> handle command
>>>>    *    mode specific operations
>>>>    * @base:    Baseclass physical encoder structure
>>>> @@ -317,6 +344,13 @@ struct dpu_encoder_phys 
>>>> *dpu_encoder_phys_cmd_init(
>>>>           struct dpu_enc_phys_init_params *p);
>>>>   /**
>>>> + * dpu_encoder_phys_wb_init - initialize writeback encoder
>>>> + * @init:    Pointer to init info structure with initialization params
>>>> + */
>>>> +struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
>>>> +        struct dpu_enc_phys_init_params *p);
>>>> +
>>>> +/**
>>>>    * dpu_encoder_helper_trigger_start - control start helper function
>>>>    *    This helper function may be optionally specified by physical
>>>>    *    encoders if they require ctl_start triggering.
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>> new file mode 100644
>>>> index 0000000..783f83e
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>>> @@ -0,0 +1,813 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights 
>>>> reserved.
>>>> + */
>>>> +
>>>> +#define pr_fmt(fmt)    "[drm:%s:%d] " fmt, __func__, __LINE__
>>>> +
>>>> +#include <linux/debugfs.h>
>>>
>>> the header is unused
>> ack
>>>
>>>> +
>>>> +#include "dpu_encoder_phys.h"
>>>> +#include "dpu_formats.h"
>>>> +#include "dpu_hw_top.h"
>>>> +#include "dpu_hw_wb.h"
>>>> +#include "dpu_hw_lm.h"
>>>> +#include "dpu_hw_blk.h"
>>>> +#include "dpu_hw_merge3d.h"
>>>> +#include "dpu_hw_interrupts.h"
>>>> +#include "dpu_core_irq.h"
>>>> +#include "dpu_vbif.h"
>>>> +#include "dpu_crtc.h"
>>>> +#include "disp/msm_disp_snapshot.h"
>>>> +
>>>> +#define DEFAULT_MAX_WRITEBACK_WIDTH 2048
>>>> +
>>>> +#define to_dpu_encoder_phys_wb(x) \
>>>> +    container_of(x, struct dpu_encoder_phys_wb, base)
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_is_master - report wb always as master encoder
>>>> + */
>>>> +static bool dpu_encoder_phys_wb_is_master(struct dpu_encoder_phys 
>>>> *phys_enc)
>>>> +{
>>>> +    return true;
>>>> +}
>>>
>>> A comment that there is only one physical enc for WB would be helpful 
>>> here. But maybe we should just pull a generic is_master into 
>>> dpu_encoder_phys.h (it would work here too).
>>
>> I am just following what we have today for phys_vid and phys_cmd.
>> But yes, I can leave a comment for sure.
>>
>>>
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_set_ot_limit - set OT limit for writeback 
>>>> interface
>>>> + * @phys_enc:    Pointer to physical encoder
>>>> + */
>>>> +static void dpu_encoder_phys_wb_set_ot_limit(
>>>> +        struct dpu_encoder_phys *phys_enc)
>>>> +{
>>>> +    struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
>>>> +    struct dpu_vbif_set_ot_params ot_params;
>>>> +
>>>> +    memset(&ot_params, 0, sizeof(ot_params));
>>>> +    ot_params.xin_id = hw_wb->caps->xin_id;
>>>> +    ot_params.num = hw_wb->idx - WB_0;
>>>> +    ot_params.width = phys_enc->cached_mode.hdisplay;
>>>> +    ot_params.height = phys_enc->cached_mode.vdisplay;
>>>> +    ot_params.is_wfd = true;
>>>> +    ot_params.frame_rate = drm_mode_vrefresh(&phys_enc->cached_mode);
>>>> +    ot_params.vbif_idx = hw_wb->caps->vbif_idx;
>>>> +    ot_params.clk_ctrl = hw_wb->caps->clk_ctrl;
>>>> +    ot_params.rd = false;
>>>> +
>>>> +    dpu_vbif_set_ot_limit(phys_enc->dpu_kms, &ot_params);
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_set_qos_remap - set QoS remapper for writeback
>>>> + * @phys_enc:    Pointer to physical encoder
>>>> + */
>>>> +static void dpu_encoder_phys_wb_set_qos_remap(
>>>> +        struct dpu_encoder_phys *phys_enc)
>>>> +{
>>>> +    struct dpu_hw_wb *hw_wb;
>>>> +    struct dpu_vbif_set_qos_params qos_params;
>>>> +
>>>> +    if (!phys_enc || !phys_enc->parent || !phys_enc->parent->crtc) {
>>>> +        DPU_ERROR("invalid arguments\n");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (!phys_enc->hw_wb || !phys_enc->hw_wb->caps) {
>>>> +        DPU_ERROR("invalid writeback hardware\n");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    hw_wb = phys_enc->hw_wb;
>>>> +
>>>> +    memset(&qos_params, 0, sizeof(qos_params));
>>>> +    qos_params.vbif_idx = hw_wb->caps->vbif_idx;
>>>> +    qos_params.xin_id = hw_wb->caps->xin_id;
>>>> +    qos_params.clk_ctrl = hw_wb->caps->clk_ctrl;
>>>> +    qos_params.num = hw_wb->idx - WB_0;
>>>> +    qos_params.is_rt = false;
>>>> +
>>>> +    DPU_DEBUG("[qos_remap] wb:%d vbif:%d xin:%d is_rt:%d\n",
>>>> +            qos_params.num,
>>>> +            qos_params.vbif_idx,
>>>> +            qos_params.xin_id, qos_params.is_rt);
>>>> +
>>>> +    dpu_vbif_set_qos_remap(phys_enc->dpu_kms, &qos_params);
>>>> +}
>>>> +
>>>> +//this can be moved to some common file?
>>>
>>> C99 comment and missing "FIXME: " :-D
>> Yes I will remove this since i am addressing this now.
>>>
>>> Yes, let's pull it into dpu_hw_util.c
>>>
>>> And if you (by chance) would see a value in unifying other 
>>> QoS/CDP-related functions let's move them to the dpu_hw_util.c too.
>>
>> Yes I think I can move some utility functions like below to the util 
>> file. Will take care of it in next version.
>>
>>>
>>>> +static u64 _dpu_encoder_phys_wb_get_qos_lut(struct dpu_qos_lut_tbl 
>>>> *tbl,
>>>> +        u32 total_fl)
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    if (!tbl || !tbl->nentry || !tbl->entries)
>>>> +        return 0;
>>>> +
>>>> +    for (i = 0; i < tbl->nentry; i++)
>>>> +        if (total_fl <= tbl->entries[i].fl)
>>>> +            return tbl->entries[i].lut;
>>>> +
>>>> +    /* if last fl is zero, use as default */
>>>> +    if (!tbl->entries[i-1].fl)
>>>> +        return tbl->entries[i-1].lut;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_set_qos - set QoS/danger/safe LUTs for 
>>>> writeback
>>>> + * @phys_enc:    Pointer to physical encoder
>>>> + */
>>>> +static void dpu_encoder_phys_wb_set_qos(struct dpu_encoder_phys 
>>>> *phys_enc)
>>>> +{
>>>> +    struct dpu_hw_wb *hw_wb;
>>>> +    struct dpu_hw_wb_qos_cfg qos_cfg;
>>>> +    struct dpu_mdss_cfg *catalog;
>>>> +    struct dpu_qos_lut_tbl *qos_lut_tb;
>>>> +
>>>> +    if (!phys_enc || !phys_enc->dpu_kms || 
>>>> !phys_enc->dpu_kms->catalog) {
>>>> +        DPU_ERROR("invalid parameter(s)\n");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    catalog = phys_enc->dpu_kms->catalog;
>>>> +
>>>> +    hw_wb = phys_enc->hw_wb;
>>>> +
>>>> +    memset(&qos_cfg, 0, sizeof(struct dpu_hw_wb_qos_cfg));
>>>> +    qos_cfg.danger_safe_en = true;
>>>> +    qos_cfg.danger_lut =
>>>> +        catalog->perf.danger_lut_tbl[DPU_QOS_LUT_USAGE_NRT];
>>>> +
>>>> +    qos_cfg.safe_lut = 
>>>> catalog->perf.safe_lut_tbl[DPU_QOS_LUT_USAGE_NRT];
>>>> +
>>>> +    qos_lut_tb = &catalog->perf.qos_lut_tbl[DPU_QOS_LUT_USAGE_NRT];
>>>> +    qos_cfg.creq_lut = _dpu_encoder_phys_wb_get_qos_lut(qos_lut_tb, 
>>>> 0);
>>>> +
>>>> +    if (hw_wb->ops.setup_qos_lut)
>>>> +        hw_wb->ops.setup_qos_lut(hw_wb, &qos_cfg);
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_setup_fb - setup output framebuffer
>>>> + * @phys_enc:    Pointer to physical encoder
>>>> + * @fb:        Pointer to output framebuffer
>>>> + * @wb_roi:    Pointer to output region of interest
>>>> + */
>>>> +static void dpu_encoder_phys_wb_setup_fb(struct dpu_encoder_phys 
>>>> *phys_enc,
>>>> +        struct drm_framebuffer *fb)
>>>> +{
>>>> +    struct dpu_encoder_phys_wb *wb_enc = 
>>>> to_dpu_encoder_phys_wb(phys_enc);
>>>> +    struct dpu_hw_wb *hw_wb;
>>>> +    struct dpu_hw_wb_cfg *wb_cfg;
>>>> +    struct dpu_hw_wb_cdp_cfg *cdp_cfg;
>>>> +
>>>> +    if (!phys_enc || !phys_enc->dpu_kms || 
>>>> !phys_enc->dpu_kms->catalog ||
>>>> +            !phys_enc->connector) {
>>>> +        DPU_ERROR("invalid encoder\n");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    hw_wb = phys_enc->hw_wb;
>>>> +    wb_cfg = &wb_enc->wb_cfg;
>>>> +    cdp_cfg = &wb_enc->cdp_cfg;
>>>> +
>>>> +    wb_cfg->intf_mode = phys_enc->intf_mode;
>>>> +    wb_cfg->roi.x1 = 0;
>>>> +    wb_cfg->roi.x2 = phys_enc->cached_mode.hdisplay;
>>>> +    wb_cfg->roi.y1 = 0;
>>>> +    wb_cfg->roi.y2 = phys_enc->cached_mode.vdisplay;
>>>> +
>>>> +    if (hw_wb->ops.setup_roi)
>>>> +        hw_wb->ops.setup_roi(hw_wb, wb_cfg);
>>>> +
>>>> +    if (hw_wb->ops.setup_outformat)
>>>> +        hw_wb->ops.setup_outformat(hw_wb, wb_cfg);
>>>> +
>>>> +    if (hw_wb->ops.setup_cdp) {
>>>> +        memset(cdp_cfg, 0, sizeof(struct dpu_hw_wb_cdp_cfg));
>>>> +
>>>> +        cdp_cfg->enable = phys_enc->dpu_kms->catalog->perf.cdp_cfg
>>>> +                [DPU_PERF_CDP_USAGE_NRT].wr_enable;
>>>> +        cdp_cfg->ubwc_meta_enable =
>>>> +                DPU_FORMAT_IS_UBWC(wb_cfg->dest.format);
>>>> +        cdp_cfg->tile_amortize_enable =
>>>> +                DPU_FORMAT_IS_UBWC(wb_cfg->dest.format) ||
>>>> +                DPU_FORMAT_IS_TILE(wb_cfg->dest.format);
>>>> +        cdp_cfg->preload_ahead = DPU_WB_CDP_PRELOAD_AHEAD_64;
>>>> +
>>>> +        hw_wb->ops.setup_cdp(hw_wb, cdp_cfg);
>>>> +    }
>>>> +
>>>> +    if (hw_wb->ops.setup_outaddress)
>>>> +        hw_wb->ops.setup_outaddress(hw_wb, wb_cfg);
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_setup_cdp - setup chroma down prefetch block
>>>> + * @phys_enc:Pointer to physical encoder
>>>> + */
>>>> +static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys 
>>>> *phys_enc)
>>>> +{
>>>> +    struct dpu_hw_wb *hw_wb;
>>>> +    struct dpu_hw_ctl *ctl;
>>>> +
>>>> +    if (!phys_enc) {
>>>> +        DPU_ERROR("invalid encoder\n");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    hw_wb = phys_enc->hw_wb;
>>>> +    ctl = phys_enc->hw_ctl;
>>>> +
>>>> +    if (test_bit(DPU_CTL_ACTIVE_CFG, &ctl->caps->features) &&
>>>> +        (phys_enc->hw_ctl &&
>>>> +         phys_enc->hw_ctl->ops.setup_intf_cfg)) {
>>>> +        struct dpu_hw_intf_cfg intf_cfg = {0};
>>>> +        struct dpu_hw_pingpong *hw_pp = phys_enc->hw_pp;
>>>> +        enum dpu_3d_blend_mode mode_3d;
>>>> +
>>>> +        mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>>> +
>>>> +        intf_cfg.intf = DPU_NONE;
>>>> +        intf_cfg.wb = hw_wb->idx;
>>>> +
>>>> +        if (mode_3d && hw_pp && hw_pp->merge_3d)
>>>> +            intf_cfg.merge_3d = hw_pp->merge_3d->idx;
>>>> +
>>>> +        if (phys_enc->hw_pp->merge_3d && 
>>>> phys_enc->hw_pp->merge_3d->ops.setup_3d_mode)
>>>> + 
>>>> phys_enc->hw_pp->merge_3d->ops.setup_3d_mode(phys_enc->hw_pp->merge_3d,
>>>> +                    mode_3d);
>>>> +
>>>> +        /* setup which pp blk will connect to this wb */
>>>> +        if (hw_pp && phys_enc->hw_wb->ops.bind_pingpong_blk)
>>>> +            phys_enc->hw_wb->ops.bind_pingpong_blk(phys_enc->hw_wb, 
>>>> true,
>>>> +                    phys_enc->hw_pp->idx);
>>>> +
>>>> +        phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl,
>>>> +                &intf_cfg, true);
>>>> +    } else if (phys_enc->hw_ctl && 
>>>> phys_enc->hw_ctl->ops.setup_intf_cfg) {
>>>> +        struct dpu_hw_intf_cfg intf_cfg = {0};
>>>> +
>>>> +        intf_cfg.intf = DPU_NONE;
>>>> +        intf_cfg.wb = hw_wb->idx;
>>>> +        intf_cfg.mode_3d =
>>>> +            dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>>> +        phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, 
>>>> &intf_cfg, true);
>>>> +    }
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_atomic_check - verify and fixup given atomic 
>>>> states
>>>> + * @phys_enc:    Pointer to physical encoder
>>>> + * @crtc_state:    Pointer to CRTC atomic state
>>>> + * @conn_state:    Pointer to connector atomic state
>>>> + */
>>>> +static int dpu_encoder_phys_wb_atomic_check(
>>>> +        struct dpu_encoder_phys *phys_enc,
>>>> +        struct drm_crtc_state *crtc_state,
>>>> +        struct drm_connector_state *conn_state)
>>>> +{
>>>> +    struct drm_framebuffer *fb;
>>>> +    const struct drm_display_mode *mode;
>>>> +
>>>> +    DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
>>>> +            phys_enc->intf_idx, mode->name, mode->hdisplay, 
>>>> mode->vdisplay);
>>>> +
>>>> +    if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
>>>> +        return 0;
>>>> +
>>>> +    fb = conn_state->writeback_job->fb;
>>>> +    mode = &crtc_state->mode;
>>>> +
>>>> +    if (!conn_state || !conn_state->connector) {
>>>> +        DPU_ERROR("invalid connector state\n");
>>>> +        return -EINVAL;
>>>> +    } else if (conn_state->connector->status !=
>>>> +            connector_status_connected) {
>>>> +        DPU_ERROR("connector not connected %d\n",
>>>> +                conn_state->connector->status);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    DPU_DEBUG("[fb_id:%u][fb:%u,%u]\n", fb->base.id,
>>>> +            fb->width, fb->height);
>>>> +
>>>> +    if (fb->width != mode->hdisplay) {
>>>> +        DPU_ERROR("invalid fb w=%d, mode w=%d\n", fb->width,
>>>> +                mode->hdisplay);
>>>> +        return -EINVAL;
>>>> +    } else if (fb->height != mode->vdisplay) {
>>>> +        DPU_ERROR("invalid fb h=%d, mode h=%d\n", fb->height,
>>>> +                  mode->vdisplay);
>>>> +        return -EINVAL;
>>>> +    } else if (fb->width > DEFAULT_MAX_WRITEBACK_WIDTH) {
>>>> +        DPU_ERROR("invalid fb w=%d, maxlinewidth=%u\n",
>>>> +                  fb->width, DEFAULT_MAX_WRITEBACK_WIDTH);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +
>>>> +/**
>>>> + * _dpu_encoder_phys_wb_update_flush - flush hardware update
>>>> + * @phys_enc:    Pointer to physical encoder
>>>> + */
>>>> +static void _dpu_encoder_phys_wb_update_flush(struct 
>>>> dpu_encoder_phys *phys_enc)
>>>> +{
>>>> +    struct dpu_hw_wb *hw_wb;
>>>> +    struct dpu_hw_ctl *hw_ctl;
>>>> +    struct dpu_hw_pingpong *hw_pp;
>>>> +    u32 pending_flush = 0;
>>>> +
>>>> +    if (!phys_enc)
>>>> +        return;
>>>> +
>>>> +    hw_wb = phys_enc->hw_wb;
>>>> +    hw_pp = phys_enc->hw_pp;
>>>> +    hw_ctl = phys_enc->hw_ctl;
>>>> +
>>>> +    DPU_DEBUG("[wb:%d]\n", hw_wb->idx - WB_0);
>>>> +
>>>> +    if (!hw_ctl) {
>>>> +        DPU_DEBUG("[wb:%d] no ctl assigned\n", hw_wb->idx - WB_0);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (hw_ctl->ops.update_pending_flush_wb)
>>>> +        hw_ctl->ops.update_pending_flush_wb(hw_ctl, hw_wb->idx);
>>>> +
>>>> +    if (hw_ctl->ops.update_pending_flush_merge_3d && hw_pp && 
>>>> hw_pp->merge_3d)
>>>> +        hw_ctl->ops.update_pending_flush_merge_3d(hw_ctl,
>>>> +                hw_pp->merge_3d->idx);
>>>> +
>>>> +    if (hw_ctl->ops.get_pending_flush)
>>>> +        pending_flush = hw_ctl->ops.get_pending_flush(hw_ctl);
>>>> +
>>>> +    DPU_DEBUG("Pending flush mask for CTL_%d is 0x%x, WB %d\n",
>>>> +            hw_ctl->idx - CTL_0, pending_flush,
>>>> +            hw_wb->idx - WB_0);
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_setup - setup writeback encoder
>>>> + * @phys_enc:    Pointer to physical encoder
>>>> + */
>>>> +static void dpu_encoder_phys_wb_setup(
>>>> +        struct dpu_encoder_phys *phys_enc)
>>>> +{
>>>> +    struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
>>>> +    struct drm_display_mode mode = phys_enc->cached_mode;
>>>> +    struct drm_framebuffer *fb = NULL;
>>>> +
>>>> +    DPU_DEBUG("[mode_set:%d, \"%s\",%d,%d]\n",
>>>> +            hw_wb->idx - WB_0, mode.name,
>>>> +            mode.hdisplay, mode.vdisplay);
>>>> +
>>>> +    dpu_encoder_phys_wb_set_ot_limit(phys_enc);
>>>> +
>>>> +    dpu_encoder_phys_wb_set_qos_remap(phys_enc);
>>>> +
>>>> +    dpu_encoder_phys_wb_set_qos(phys_enc);
>>>> +
>>>> +    dpu_encoder_phys_wb_setup_fb(phys_enc, fb);
>>>> +
>>>> +    dpu_encoder_phys_wb_setup_cdp(phys_enc);
>>>> +
>>>> +}
>>>> +
>>>> +static void _dpu_encoder_phys_wb_frame_done_helper(void *arg)
>>>> +{
>>>> +    struct dpu_encoder_phys *phys_enc = arg;
>>>> +    struct dpu_encoder_phys_wb *wb_enc = 
>>>> to_dpu_encoder_phys_wb(phys_enc);
>>>> +
>>>> +    struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
>>>> +    unsigned long lock_flags;
>>>> +    u32 event = DPU_ENCODER_FRAME_EVENT_DONE;
>>>> +
>>>> +    DPU_DEBUG("[wb:%d]\n", hw_wb->idx - WB_0);
>>>> +
>>>> +    if (phys_enc->parent_ops->handle_frame_done)
>>>> +        phys_enc->parent_ops->handle_frame_done(phys_enc->parent,
>>>> +                phys_enc, event);
>>>> +
>>>> +    if (phys_enc->parent_ops->handle_vblank_virt)
>>>> +        phys_enc->parent_ops->handle_vblank_virt(phys_enc->parent,
>>>> +                phys_enc);
>>>> +
>>>> +    spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
>>>> +    atomic_add_unless(&phys_enc->pending_kickoff_cnt, -1, 0);
>>>> +    spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
>>>> +
>>>> +    if (wb_enc->wb_conn)
>>>> +        drm_writeback_signal_completion(wb_enc->wb_conn, 0);
>>>> +
>>>> +    /* Signal any waiting atomic commit thread */
>>>> +    wake_up_all(&phys_enc->pending_kickoff_wq);
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_done_irq - writeback interrupt handler
>>>> + * @arg:    Pointer to writeback encoder
>>>> + * @irq_idx:    interrupt index
>>>> + */
>>>> +static void dpu_encoder_phys_wb_done_irq(void *arg, int irq_idx)
>>>> +{
>>>> +    _dpu_encoder_phys_wb_frame_done_helper(arg);
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_irq_ctrl - irq control of WB
>>>> + * @phys:    Pointer to physical encoder
>>>> + * @enable:    indicates enable or disable interrupts
>>>> + */
>>>> +static void dpu_encoder_phys_wb_irq_ctrl(
>>>> +        struct dpu_encoder_phys *phys, bool enable)
>>>> +{
>>>> +
>>>> +    struct dpu_encoder_phys_wb *wb_enc = to_dpu_encoder_phys_wb(phys);
>>>> +    int ret = 0;
>>>> +    int refcount;
>>>> +
>>>> +    refcount = atomic_read(&wb_enc->wbirq_refcount);
>>>> +
>>>> +    if (enable && atomic_inc_return(&wb_enc->wbirq_refcount) == 1) {
>>>> +        dpu_encoder_helper_register_irq(phys, INTR_IDX_WB_DONE);
>>>> +        if (ret)
>>>> +            atomic_dec_return(&wb_enc->wbirq_refcount);
>>>> +    } else if (!enable &&
>>>> +            atomic_dec_return(&wb_enc->wbirq_refcount) == 0) {
>>>> +        dpu_encoder_helper_unregister_irq(phys, INTR_IDX_WB_DONE);
>>>> +        if (ret)
>>>> +            atomic_inc_return(&wb_enc->wbirq_refcount);
>>>> +    }
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_mode_set - set display mode
>>>> + * @phys_enc:    Pointer to physical encoder
>>>> + * @mode:    Pointer to requested display mode
>>>> + * @adj_mode:    Pointer to adjusted display mode
>>>> + */
>>>> +static void dpu_encoder_phys_wb_mode_set(
>>>> +        struct dpu_encoder_phys *phys_enc,
>>>> +        struct drm_display_mode *mode,
>>>> +        struct drm_display_mode *adj_mode)
>>>> +{
>>>> +    struct dpu_encoder_irq *irq;
>>>> +
>>>> +    if (adj_mode) {
>>>> +        phys_enc->cached_mode = *adj_mode;
>>>> +        drm_mode_debug_printmodeline(adj_mode);
>>>> +        DPU_DEBUG("caching mode:\n");
>>>> +    }
>>>> +
>>>> +    irq = &phys_enc->irq[INTR_IDX_WB_DONE];
>>>> +    irq->irq_idx = phys_enc->hw_wb->caps->intr_wb_done;
>>>> +}
>>>> +
>>>> +static void _dpu_encoder_phys_wb_handle_wbdone_timeout(
>>>> +        struct dpu_encoder_phys *phys_enc)
>>>> +{
>>>> +    struct dpu_encoder_phys_wb *wb_enc = 
>>>> to_dpu_encoder_phys_wb(phys_enc);
>>>> +    u32 frame_event = DPU_ENCODER_FRAME_EVENT_ERROR;
>>>> +
>>>> +    wb_enc->wb_done_timeout_cnt++;
>>>> +
>>>> +    if (wb_enc->wb_done_timeout_cnt == 1)
>>>> +        msm_disp_snapshot_state(phys_enc->parent->dev);
>>>> +
>>>> +    atomic_add_unless(&phys_enc->pending_kickoff_cnt, -1, 0);
>>>> +
>>>> +    /* request a ctl reset before the next kickoff */
>>>> +    phys_enc->enable_state = DPU_ENC_ERR_NEEDS_HW_RESET;
>>>> +
>>>> +    if (wb_enc->wb_conn)
>>>> +        drm_writeback_signal_completion(wb_enc->wb_conn, 0);
>>>> +
>>>> +    if (phys_enc->parent_ops->handle_frame_done)
>>>> +        phys_enc->parent_ops->handle_frame_done(
>>>> +                phys_enc->parent, phys_enc, frame_event);
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_wait_for_commit_done - wait until request is 
>>>> committed
>>>> + * @phys_enc:    Pointer to physical encoder
>>>> + */
>>>> +static int dpu_encoder_phys_wb_wait_for_commit_done(
>>>> +        struct dpu_encoder_phys *phys_enc)
>>>> +{
>>>> +    unsigned long ret;
>>>> +    struct dpu_encoder_wait_info wait_info;
>>>> +    struct dpu_encoder_phys_wb *wb_enc = 
>>>> to_dpu_encoder_phys_wb(phys_enc);
>>>> +
>>>> +    wait_info.wq = &phys_enc->pending_kickoff_wq;
>>>> +    wait_info.atomic_cnt = &phys_enc->pending_kickoff_cnt;
>>>> +    wait_info.timeout_ms = KICKOFF_TIMEOUT_MS;
>>>> +
>>>> +    ret = dpu_encoder_helper_wait_for_irq(phys_enc, INTR_IDX_WB_DONE,
>>>> +            &wait_info);
>>>> +    if (ret == -ETIMEDOUT)
>>>> +        _dpu_encoder_phys_wb_handle_wbdone_timeout(phys_enc);
>>>> +    else if (!ret)
>>>> +        wb_enc->wb_done_timeout_cnt = 0;
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_prepare_for_kickoff - pre-kickoff processing
>>>> + * @phys_enc:    Pointer to physical encoder
>>>> + * Returns:    Zero on success
>>>> + */
>>>> +static void dpu_encoder_phys_wb_prepare_for_kickoff(
>>>> +        struct dpu_encoder_phys *phys_enc)
>>>> +{
>>>> +    struct dpu_encoder_phys_wb *wb_enc = 
>>>> to_dpu_encoder_phys_wb(phys_enc);
>>>> +    struct drm_connector *drm_conn;
>>>> +    struct drm_connector_state *state;
>>>> +
>>>> +    DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
>>>> +
>>>> +    if (!wb_enc->wb_conn || !wb_enc->wb_job) {
>>>> +        DPU_ERROR("invalid wb_conn or wb_job\n");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    drm_conn = wb_enc->wb_conn->base;
>>>> +    state = drm_conn->state;
>>>> +
>>>> +    if (wb_enc->wb_conn && wb_enc->wb_job)
>>>> +        drm_writeback_queue_job(wb_enc->wb_conn, state);
>>>> +
>>>> +    dpu_encoder_phys_wb_setup(phys_enc);
>>>> +
>>>> +    _dpu_encoder_phys_wb_update_flush(phys_enc);
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_needs_single_flush - trigger flush processing
>>>> + * @phys_enc:    Pointer to physical encoder
>>>> + */
>>>> +static bool dpu_encoder_phys_wb_needs_single_flush(struct 
>>>> dpu_encoder_phys *phys_enc)
>>>> +{
>>>> +    DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
>>>> +    return false;
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_handle_post_kickoff - post-kickoff processing
>>>> + * @phys_enc:    Pointer to physical encoder
>>>> + */
>>>> +static void dpu_encoder_phys_wb_handle_post_kickoff(
>>>> +        struct dpu_encoder_phys *phys_enc)
>>>> +{
>>>> +    DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
>>>> +
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_enable - enable writeback encoder
>>>> + * @phys_enc:    Pointer to physical encoder
>>>> + */
>>>> +static void dpu_encoder_phys_wb_enable(struct dpu_encoder_phys 
>>>> *phys_enc)
>>>> +{
>>>> +    DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
>>>> +    phys_enc->enable_state = DPU_ENC_ENABLED;
>>>> +}
>>>> +/**
>>>> + * dpu_encoder_phys_wb_disable - disable writeback encoder
>>>> + * @phys_enc:    Pointer to physical encoder
>>>> + */
>>>> +static void dpu_encoder_phys_wb_disable(struct dpu_encoder_phys 
>>>> *phys_enc)
>>>> +{
>>>> +    struct dpu_hw_wb *hw_wb = phys_enc->hw_wb;
>>>> +
>>>> +    DPU_DEBUG("[wb:%d]\n", hw_wb->idx - WB_0);
>>>> +
>>>> +    if (phys_enc->enable_state == DPU_ENC_DISABLED) {
>>>> +        DPU_ERROR("encoder is already disabled\n");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /* reset h/w before final flush */
>>>> +    if (phys_enc->hw_ctl->ops.clear_pending_flush)
>>>> +        phys_enc->hw_ctl->ops.clear_pending_flush(phys_enc->hw_ctl);
>>>> +
>>>> +    /*
>>>> +     * New CTL reset sequence from 5.0 MDP onwards.
>>>> +     * If has_3d_merge_reset is not set, legacy reset
>>>> +     * sequence is executed.
>>>> +     *
>>>> +     * Legacy reset sequence has not been implemented yet.
>>>> +     * Any target earlier than SM8150 will need it and when
>>>> +     * WB support is added to those targets will need to add
>>>> +     * the legacy teardown sequence as well.
>>>> +     */
>>>> +    if (phys_enc->hw_pp->merge_3d)
>>>
>>> This is a bit.. counterintuitive. I'd prefer hw_ctl->caps->features & 
>>> BIT(DPU_ACTIVE_CTL).
>> Your suggestion should work.
>>
>> But consider it this way.
>>
>> Checking caps is telling us if the chipset supports merge3d which is 
>> sufficient in this case but phys_enc->hw_pp->merge_3d is telling us 
>> whether merge_3d is also being used in this RM session.
>>
>> So i just thought its a better check to cover both.
> 
> hw_pp->merge_3d is set when PP has a MERGE_3D attached in hardware, no 
> matter if it is used in the current pipeline or not.
> 
> As I wrote, the code is counterintuitive. The comment talks about MDP 
> 5.0, but the condition checks hw_pp->merge_3d, which might not be 
> present for tons of other reasons (starting from somebody forgetting to 
> list it in the hw_catalog).
Alright, I will uset the hw_ctl->caps->features & BIT(DPU_ACTIVE_CTL)
> 
> And in fact there is a followup question. Can we push this check (and a 
> legacy reset sequence) into the dpu_encoder_helper_phys_cleanup() 
> itself? Thus the dpu_encoder_phys will just call _helper_phys_cleanup() 
> and let the helper care about hardware details.

So like I mentioned, at the moment, I am not fully confident of the 
testing of the sequence for all use-cases to use if for all interfaces 
(although downstream is already doing it ).

I will certainly, keep testing this and if it looks fine, I can easily 
move this one level up in a follow-up change and move this out of 
phys_wb. Right now, its part of phys_wb because only wb uses it.

> 
> 
>>>> +        dpu_encoder_helper_phys_cleanup(phys_enc);
>>>> +
>>>> +    phys_enc->enable_state = DPU_ENC_DISABLED;
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_get_hw_resources - get hardware resources
>>>> + * @phys_enc:    Pointer to physical encoder
>>>> + * @hw_res:    Pointer to encoder resources
>>>> + */
>>>> +static void dpu_encoder_phys_wb_get_hw_resources(
>>>> +        struct dpu_encoder_phys *phys_enc,
>>>> +        struct dpu_encoder_hw_resources *hw_res)
>>>> +{
>>>> +    if (!phys_enc) {
>>>> +        DPU_ERROR("invalid encoder\n");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    hw_res->wbs[phys_enc->intf_idx - WB_0] = INTF_MODE_WB_LINE;
>>>> +    DPU_DEBUG("[wb:%d] intf_mode=%d\n", phys_enc->intf_idx - WB_0,
>>>> +            hw_res->wbs[phys_enc->intf_idx - WB_0]);
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_destroy - destroy writeback encoder
>>>> + * @phys_enc:    Pointer to physical encoder
>>>> + */
>>>> +static void dpu_encoder_phys_wb_destroy(struct dpu_encoder_phys 
>>>> *phys_enc)
>>>> +{
>>>> +    DPU_DEBUG("[wb:%d]\n", phys_enc->intf_idx - INTF_0);
>>>> +
>>>> +    if (!phys_enc)
>>>> +        return;
>>>> +
>>>> +    kfree(phys_enc);
>>>> +}
>>>> +
>>>> +static void dpu_encoder_phys_wb_prepare_wb_job(struct 
>>>> dpu_encoder_phys *phys_enc,
>>>> +        struct drm_writeback_job *job)
>>>> +{
>>>> +    const struct msm_format *format;
>>>> +    struct dpu_hw_wb_cfg *wb_cfg;
>>>> +    int ret;
>>>> +    struct dpu_encoder_phys_wb *wb_enc = 
>>>> to_dpu_encoder_phys_wb(phys_enc);
>>>> +
>>>> +    if (!job->fb)
>>>> +        return;
>>>> +
>>>> +    wb_enc->wb_job = job;
>>>> +    wb_enc->wb_conn = job->connector;
>>>> +    wb_enc->aspace = phys_enc->dpu_kms->base.aspace;
>>>
>>> Any particular reason to cache it in wb_enc?
>>
>> We can technically get this from phys_enc->dpu_kms->base.aspace so I 
>> can drop this.
>>
>>>
>>>> +
>>>> +    wb_cfg = &wb_enc->wb_cfg;
>>>> +
>>>> +    memset(wb_cfg, 0, sizeof(struct dpu_hw_wb_cfg));
>>>> +
>>>> +    ret = msm_framebuffer_prepare(job->fb, wb_enc->aspace);
>>>> +    if (ret) {
>>>> +        DPU_ERROR("prep fb failed, %d\n", ret);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    format = msm_framebuffer_format(job->fb);
>>>> +
>>>> +    wb_cfg->dest.format = dpu_get_dpu_format_ext(
>>>> +            format->pixel_format, job->fb->modifier);
>>>> +    if (!wb_cfg->dest.format) {
>>>> +        /* this error should be detected during atomic_check */
>>>> +        DPU_ERROR("failed to get format %x\n", format->pixel_format);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    ret = dpu_format_populate_layout(wb_enc->aspace, job->fb, 
>>>> &wb_cfg->dest);
>>>> +    if (ret) {
>>>> +        DPU_DEBUG("failed to populate layout %d\n", ret);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    wb_cfg->dest.width = job->fb->width;
>>>> +    wb_cfg->dest.height = job->fb->height;
>>>> +    wb_cfg->dest.num_planes = wb_cfg->dest.format->num_planes;
>>>> +
>>>> +    if ((wb_cfg->dest.format->fetch_planes == DPU_PLANE_PLANAR) &&
>>>> +            (wb_cfg->dest.format->element[0] == C1_B_Cb))
>>>> +        swap(wb_cfg->dest.plane_addr[1], wb_cfg->dest.plane_addr[2]);
>>>> +
>>>> +    DPU_DEBUG("[fb_offset:%8.8x,%8.8x,%8.8x,%8.8x]\n",
>>>> +            wb_cfg->dest.plane_addr[0], wb_cfg->dest.plane_addr[1],
>>>> +            wb_cfg->dest.plane_addr[2], wb_cfg->dest.plane_addr[3]);
>>>> +
>>>> +    DPU_DEBUG("[fb_stride:%8.8x,%8.8x,%8.8x,%8.8x]\n",
>>>> +            wb_cfg->dest.plane_pitch[0], wb_cfg->dest.plane_pitch[1],
>>>> +            wb_cfg->dest.plane_pitch[2], wb_cfg->dest.plane_pitch[3]);
>>>> +}
>>>> +
>>>> +static void dpu_encoder_phys_wb_cleanup_wb_job(struct 
>>>> dpu_encoder_phys *phys_enc,
>>>> +        struct drm_writeback_job *job)
>>>> +{
>>>> +    struct dpu_encoder_phys_wb *wb_enc = 
>>>> to_dpu_encoder_phys_wb(phys_enc);
>>>> +
>>>> +    if (!job->fb)
>>>> +        return;
>>>> +
>>>> +    msm_framebuffer_cleanup(job->fb, wb_enc->aspace);
>>>> +    // revisit this after everything else works
>>>
>>> Everything works now, doesn't it?
>>
>> Yeah, everything works.
>> i forgot to get rid of this comment :)
>> Had it in a bunch of other places too and missed one while cleaning up.
>>
>>>
>>>> +    wb_enc->wb_job = NULL;
>>>> +    wb_enc->wb_conn = NULL;
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_init_ops - initialize writeback operations
>>>> + * @ops:    Pointer to encoder operation table
>>>> + */
>>>> +static void dpu_encoder_phys_wb_init_ops(struct 
>>>> dpu_encoder_phys_ops *ops)
>>>> +{
>>>> +    ops->is_master = dpu_encoder_phys_wb_is_master;
>>>> +    ops->mode_set = dpu_encoder_phys_wb_mode_set;
>>>> +    ops->enable = dpu_encoder_phys_wb_enable;
>>>> +    ops->disable = dpu_encoder_phys_wb_disable;
>>>> +    ops->destroy = dpu_encoder_phys_wb_destroy;
>>>> +    ops->atomic_check = dpu_encoder_phys_wb_atomic_check;
>>>> +    ops->get_hw_resources = dpu_encoder_phys_wb_get_hw_resources;
>>>> +    ops->wait_for_commit_done = 
>>>> dpu_encoder_phys_wb_wait_for_commit_done;
>>>> +    ops->prepare_for_kickoff = 
>>>> dpu_encoder_phys_wb_prepare_for_kickoff;
>>>> +    ops->handle_post_kickoff = 
>>>> dpu_encoder_phys_wb_handle_post_kickoff;
>>>> +    ops->needs_single_flush = dpu_encoder_phys_wb_needs_single_flush;
>>>> +    ops->trigger_start = dpu_encoder_helper_trigger_start;
>>>> +    ops->prepare_wb_job = dpu_encoder_phys_wb_prepare_wb_job;
>>>> +    ops->cleanup_wb_job = dpu_encoder_phys_wb_cleanup_wb_job;
>>>> +    ops->irq_control = dpu_encoder_phys_wb_irq_ctrl;
>>>> +}
>>>> +
>>>> +/**
>>>> + * dpu_encoder_phys_wb_init - initialize writeback encoder
>>>> + * @init:    Pointer to init info structure with initialization params
>>>> + */
>>>> +struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
>>>> +        struct dpu_enc_phys_init_params *p)
>>>> +{
>>>> +    struct dpu_encoder_phys *phys_enc = NULL;
>>>> +    struct dpu_encoder_phys_wb *wb_enc = NULL;
>>>> +
>>>> +    struct dpu_encoder_irq *irq;
>>>> +    int ret = 0;
>>>> +    int i;
>>>> +
>>>> +    DPU_DEBUG("\n");
>>>> +
>>>> +    if (!p || !p->parent) {
>>>> +        DPU_ERROR("invalid params\n");
>>>> +        ret = -EINVAL;
>>>> +        goto fail_alloc;
>>>> +    }
>>>> +
>>>> +    wb_enc = kzalloc(sizeof(*wb_enc), GFP_KERNEL);
>>>> +    if (!wb_enc) {
>>>> +        DPU_ERROR("failed to allocate wb phys_enc enc\n");
>>>> +        ret = -ENOMEM;
>>>> +        goto fail_alloc;
>>>> +    }
>>>> +
>>>> +    phys_enc = &wb_enc->base;
>>>> +    phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
>>>> +    phys_enc->intf_idx = p->intf_idx;
>>>> +
>>>> +    dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
>>>> +    phys_enc->parent = p->parent;
>>>> +    phys_enc->parent_ops = p->parent_ops;
>>>> +    phys_enc->dpu_kms = p->dpu_kms;
>>>> +    phys_enc->split_role = p->split_role;
>>>> +    phys_enc->intf_mode = INTF_MODE_WB_LINE;
>>>> +    phys_enc->intf_idx = p->intf_idx;
>>>> +    phys_enc->enc_spinlock = p->enc_spinlock;
>>>> +
>>>> +    atomic_set(&wb_enc->wbirq_refcount, 0);
>>>> +
>>>> +    for (i = 0; i < INTR_IDX_MAX; i++) {
>>>> +        irq = &phys_enc->irq[i];
>>>> +        INIT_LIST_HEAD(&irq->cb.list);
>>>> +        irq->irq_idx = -EINVAL;
>>>> +        irq->cb.arg = phys_enc;
>>>> +    }
>>>> +
>>>> +    irq = &phys_enc->irq[INTR_IDX_WB_DONE];
>>>> +    irq->name = "wb_done";
>>>> +    irq->intr_idx = INTR_IDX_WB_DONE;
>>>> +    irq->cb.func = dpu_encoder_phys_wb_done_irq;
>>>> +
>>>> +    atomic_set(&phys_enc->pending_kickoff_cnt, 0);
>>>> +    atomic_set(&phys_enc->vblank_refcount, 0);
>>>> +    wb_enc->wb_done_timeout_cnt = 0;
>>>> +
>>>> +    init_waitqueue_head(&phys_enc->pending_kickoff_wq);
>>>> +    phys_enc->enable_state = DPU_ENC_DISABLED;
>>>> +
>>>> +    DPU_DEBUG("Created dpu_encoder_phys for wb %d\n",
>>>> +            phys_enc->intf_idx);
>>>> +
>>>> +    return phys_enc;
>>>> +
>>>> +fail_alloc:
>>>> +    return ERR_PTR(ret);
>>>> +}
>>>
>>>
> 
>