On 2023-01-01 15:32:11, Dmitry Baryshkov wrote:
> On 31/12/2022 23:50, Marijn Suijten wrote:
> > Since DPU 5.0.0 the TEARCHECK registers and interrupts moved out of the
> > PINGPONG block and into the INTF. Implement the necessary callbacks in
> > the INTF block, and use these callbacks together with the INTF_TEAR
> > interrupts
> >
> > Signed-off-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>
>
> Generally I have the same question as for the patch 2. Can we have some
> higher level functions in the hw_pp and hw_intf files?
That is mostly because patch 2 only cleaned up non-optional handling of
hw_pp callbacks in dpu_encoder_phys_cmd_prepare_commit, which utilizes
hw_intf's autorefresh callbacks since this patch. I don't think there's
any logic in the encoder currently that is unique to either PP or INTF?
There are quite a few functions that check for NULL hw_pp only, while -
especially after this patch - should also check hw_intf to raise
"invalid encoder". Should I extend those checks as well?
> Moreover, as I
> review your patch I have the feeling that it would make sense to have to
> two sets of encoder callbacks, one for the hw_pp tearing handling and
> another set for hw_intf-based one.
Do you mean to duplicate most phy_cmd functions and switch them based on
has_intf_te in dpu_encoder_phys_cmd_init_ops? Or introduce an entirely
new set of callbacks that simply hide / abstract away the check on
has_intf_te? The former would duplicate a bunch of code unless that is
abstracted away into other functions, mainly in
dpu_encoder_phys_cmd_tearcheck_config and
dpu_encoder_phys_cmd_prepare_commit.
Alternatively, could we find a way where these PP and INTF ops share the
same struct and function signature? That might be tricky for passing in
the hw_pp or hw_intf struct without leaking those details to the
callback and/or have the switching logic in there.
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 11 +
> > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 10 +-
> > .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 113 +++++++---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 206 ++++++++++++++++++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 29 +++
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 2 +
> > 6 files changed, 340 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 9c6817b5a194..8b9070220ab2 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -673,6 +673,7 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
> > struct dpu_kms *dpu_kms;
> > struct dpu_hw_mdp *hw_mdptop;
> > struct drm_encoder *drm_enc;
> > + struct dpu_encoder_phys *phys_enc;
> > int i;
> >
> > if (!dpu_enc || !disp_info) {
> > @@ -703,12 +704,22 @@ static void _dpu_encoder_update_vsync_source(struct dpu_encoder_virt *dpu_enc,
> > vsync_cfg.ppnumber[i] = dpu_enc->hw_pp[i]->idx;
> >
> > vsync_cfg.pp_count = dpu_enc->num_phys_encs;
> > + vsync_cfg.frame_rate = drm_mode_vrefresh(&dpu_enc->base.crtc->state->adjusted_mode);
> > +
> > if (disp_info->is_te_using_watchdog_timer)
> > vsync_cfg.vsync_source = DPU_VSYNC_SOURCE_WD_TIMER_0;
> > else
> > vsync_cfg.vsync_source = DPU_VSYNC0_SOURCE_GPIO;
> >
> > hw_mdptop->ops.setup_vsync_source(hw_mdptop, &vsync_cfg);
> > +
> > + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > + phys_enc = dpu_enc->phys_encs[i];
> > +
> > + if (phys_enc->has_intf_te && phys_enc->hw_intf->ops.vsync_sel)
> > + phys_enc->hw_intf->ops.vsync_sel(phys_enc->hw_intf,
> > + vsync_cfg.vsync_source);
> > + }
> > }
> > }
> >
> > 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 f2af07d87f56..47e79401032c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> > @@ -148,10 +148,10 @@ struct dpu_encoder_phys_ops {
> > /**
> > * enum dpu_intr_idx - dpu encoder interrupt index
> > * @INTR_IDX_VSYNC: Vsync interrupt for video mode panel
> > - * @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
> > + * @INTR_IDX_PINGPONG: Pingpong done interrupt for cmd mode panel
> > + * @INTR_IDX_UNDERRUN: Underrun interrupt for video and cmd mode panel
> > + * @INTR_IDX_RDPTR: Readpointer done interrupt for cmd mode panel
> > + * @INTR_IDX_WB_DONE: Writeback done interrupt for virtual connector
> > */
> > enum dpu_intr_idx {
> > INTR_IDX_VSYNC,
> > @@ -195,6 +195,7 @@ enum dpu_intr_idx {
> > * pending.
> > * @pending_kickoff_wq: Wait queue for blocking until kickoff completes
> > * @irq: IRQ indices
> > + * @has_intf_te: Interface TE configuration support
> > */
> > struct dpu_encoder_phys {
> > struct drm_encoder *parent;
> > @@ -220,6 +221,7 @@ struct dpu_encoder_phys {
> > atomic_t pending_kickoff_cnt;
> > wait_queue_head_t pending_kickoff_wq;
> > int irq[INTR_IDX_MAX];
> > + bool has_intf_te;
> > };
> >
> > static inline int dpu_encoder_phys_inc_pending(struct dpu_encoder_phys *phys)
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > index 7e5ba52197cd..ca44a8087f01 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > @@ -100,12 +100,12 @@ static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx)
> > DPU_ATRACE_END("pp_done_irq");
> > }
> >
> > -static void dpu_encoder_phys_cmd_pp_rd_ptr_irq(void *arg, int irq_idx)
> > +static void dpu_encoder_phys_cmd_te_rd_ptr_irq(void *arg, int irq_idx)
> > {
> > struct dpu_encoder_phys *phys_enc = arg;
> > struct dpu_encoder_phys_cmd *cmd_enc;
> >
> > - if (!phys_enc->hw_pp)
> > + if (!phys_enc->hw_pp || !phys_enc->hw_intf)
> > return;
> >
> > DPU_ATRACE_BEGIN("rd_ptr_irq");
> > @@ -147,11 +147,19 @@ static void dpu_encoder_phys_cmd_atomic_mode_set(
> > struct drm_crtc_state *crtc_state,
> > struct drm_connector_state *conn_state)
> > {
> > + if (phys_enc->has_intf_te && !phys_enc->hw_intf) {
> > + DPU_ERROR("invalid intf configuration\n");
> > + return;
> > + }
> > +
> > phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
> >
> > phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
> >
> > - phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr;
> > + if (phys_enc->has_intf_te)
> > + phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_intf->cap->intr_tear_rd_ptr;
> > + else
> > + phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr;
> >
> > phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
> > }
> > @@ -264,7 +272,7 @@ static int dpu_encoder_phys_cmd_control_vblank_irq(
> > if (enable && atomic_inc_return(&phys_enc->vblank_refcount) == 1)
> > ret = dpu_core_irq_register_callback(phys_enc->dpu_kms,
> > phys_enc->irq[INTR_IDX_RDPTR],
> > - dpu_encoder_phys_cmd_pp_rd_ptr_irq,
> > + dpu_encoder_phys_cmd_te_rd_ptr_irq,
> > phys_enc);
> > else if (!enable && atomic_dec_return(&phys_enc->vblank_refcount) == 0)
> > ret = dpu_core_irq_unregister_callback(phys_enc->dpu_kms,
Fwiw looks like this function is a prime candidate to get updated with
hw_intf information (in error checking and logging), as this callback is
now shared between PP and INTF.
> > @@ -336,10 +344,18 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
> >
> > DPU_DEBUG_CMDENC(cmd_enc, "pp %d\n", phys_enc->hw_pp->idx - PINGPONG_0);
> >
> > - if (!phys_enc->hw_pp->ops.setup_tearcheck ||
> > - !phys_enc->hw_pp->ops.enable_tearcheck) {
> > - DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
> > - return;
> > + if (phys_enc->has_intf_te) {
> > + if (!phys_enc->hw_intf->ops.setup_tearcheck ||
> > + !phys_enc->hw_intf->ops.enable_tearcheck) {
> > + DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
> > + return;
> > + }
> > + } else {
> > + if (!phys_enc->hw_pp->ops.setup_tearcheck ||
> > + !phys_enc->hw_pp->ops.enable_tearcheck) {
> > + DPU_DEBUG_CMDENC(cmd_enc, "tearcheck not supported\n");
> > + return;
> > + }
> > }
> >
> > dpu_kms = phys_enc->dpu_kms;
> > @@ -392,8 +408,13 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
> > phys_enc->hw_pp->idx - PINGPONG_0, tc_cfg.sync_cfg_height,
> > tc_cfg.sync_threshold_start, tc_cfg.sync_threshold_continue);
> >
> > - phys_enc->hw_pp->ops.setup_tearcheck(phys_enc->hw_pp, &tc_cfg);
> > - phys_enc->hw_pp->ops.enable_tearcheck(phys_enc->hw_pp, tc_enable);
>
> A simple random example: setup_tearcheck is always followed with the
> enable_tearcheck. If we merge them, the code would be simpler.
setup_tearcheck could include this functionality, but note that
dpu_encoder_phys_cmd_disable currently calls enable_tearcheck(false)
without setup_tearcheck.
- Marijn
> > <snip>
