Re: [RFC PATCH 6/7] drm/msm/dpu: Implement tearcheck support on INTF block

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 02/01/2023 13:06, Marijn Suijten wrote:
> 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?

I think it would be better to duplicate the logic rather than having a 
spaghetti of hw_pp and hw_intf calls.

> 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?

I think so. However in most of cases these checks should be void, since 
cmd encoder can not be instantiated without intf.

> > 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.


For the dpu_encoder_phys_cmd_tearcheck_config() it seems logical to fill 
in the tc_cfg and then to call either the single hw_pp callback or a 
single hw_intf callback.

I'd move most of the code from prepare_commit to dpu_hw_pp and then 
duplicate it to dpu_hw_intf. This seems like the lesser evil.
This function really stands out, since if you inline 
_dpu_encoder_phys_cmd_connect_te() and 
dpu_encoder_phys_cmd_is_ongoing_pptx() it becomes obvious that the whole 
function is a mixture of linked calls to hw_pp ops. And judging from 
your patch it doesn't make sense to check them one by one. Either we 
have all of them, or none.

> 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.

Yes, so enable_tearcheck() and disable_tearcheck() sound more useful.

> - Marijn
>
> > > <snip>

--
With best wishes
Dmitry