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

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

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@somainline.org>
>>
>> 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