Received-SPF: pass (google.com: domain of linux-kernel+bounces-194788-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <961cee30-33fa-5048-901e-b192a735b529@quicinc.com>
Date: Wed, 29 May 2024 16:44:59 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v2] Revert "drm/msm/dpu: drop
 dpu_encoder_phys_ops.atomic_mode_set"
Content-Language: en-US
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC: Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>,
        Marijn
 Suijten <marijn.suijten@somainline.org>,
        David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
        <linux-arm-msm@vger.kernel.org>, <dri-devel@lists.freedesktop.org>,
        <freedreno@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>
References: <20240522-dpu-revert-ams-v2-1-b37825d708e1@linaro.org>
 <d9a502a2-9ad3-3b95-670a-3a67887ef54b@quicinc.com>
 <2qm32wkjdxynmovw5gd5xh4awy2gh4byt3ddkpvuvi4lq7f67d@y3dntcusfwxs>
From: Abhinav Kumar <quic_abhinavk@quicinc.com>
In-Reply-To: <2qm32wkjdxynmovw5gd5xh4awy2gh4byt3ddkpvuvi4lq7f67d@y3dntcusfwxs>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 5/24/2024 1:22 PM, Dmitry Baryshkov wrote:
> On Fri, May 24, 2024 at 12:58:53PM -0700, Abhinav Kumar wrote:
>>
>>
>> On 5/22/2024 3:24 AM, Dmitry Baryshkov wrote:
>>> In the DPU driver blank IRQ handling is called from a vblank worker and
>>> can happen outside of the irq_enable / irq_disable pair. Using the
>>> worker makes that completely asynchronous with the rest of the code.
>>> Revert commit d13f638c9b88 ("drm/msm/dpu: drop
>>> dpu_encoder_phys_ops.atomic_mode_set") to fix vblank IRQ assignment for
>>> CMD DSI panels.
>>>
>>> Call trace:
>>>    dpu_encoder_phys_cmd_control_vblank_irq+0x218/0x294
>>>     dpu_encoder_toggle_vblank_for_crtc+0x160/0x194
>>>     dpu_crtc_vblank+0xbc/0x228
>>>     dpu_kms_enable_vblank+0x18/0x24
>>>     vblank_ctrl_worker+0x34/0x6c
>>>     process_one_work+0x218/0x620
>>>     worker_thread+0x1ac/0x37c
>>>     kthread+0x114/0x118
>>>     ret_from_fork+0x10/0x20
>>>
>>
>> Thanks for the stack.
>>
>> Agreed that vblank can be controlled asynchronously through the worker.
>>
>> And I am guessing that the worker thread ran and printed this error message
>> because phys_enc->irq[INTR_IDX_VSYNC] was not valid as modeset had not
>> happened by then?
> 
> modeset happened, but the vblanks can be enabled and disabled
> afterwards.
> 
>>
>> 272 end:
>> 273 	if (ret) {
>> 274 		DRM_ERROR("vblank irq err id:%u pp:%d ret:%d, enable %s/%d\n",
>> 275 			  DRMID(phys_enc->parent),
>> 276 			  phys_enc->hw_pp->idx - PINGPONG_0, ret,
>> 277 			  enable ? "true" : "false", refcount);
>> 278 	}
>>
>> But how come this did not happen even with this revert.
>>
>> IOW, I am still missing how moving the irq assignment back to modeset from
>> enable is fixing this?
> 
> It removes clearing of the IRQ fields in the irq_disable path, which
> removes a requirement that vblank IRQ manipulations are called only
> within the irq_enable/irq_disable brackets. I didn't have time to debug
> this further on, so I think it's better to revert it now and return to
> this cleanup later.
> 

I see your point.

So before we moved the irq assignment to enable/disable, modeset was 
setting it up but nothing was clearing it.

But after the change, disable was clearing it leading to the error.

Probably should have happened quite easily then?

Then if we kept only the assignment in enable and drop the parts in 
disable it should have worked too.

If you would like to post that separately after some more investigation, 
I am fine with this for now,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

>>
>>> Fixes: d13f638c9b88 ("drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set")
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>> Changes in v2:
>>> - Expanded commit message to describe the reason for revert and added a
>>>     call trace (Abhinav)
>>> - Link to v1: https://lore.kernel.org/r/20240514-dpu-revert-ams-v1-1-b13623d6cd5f@linaro.org
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        |  2 ++
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |  5 ++++
>>>    .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c   | 32 ++++++++++++----------
>>>    .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   | 13 +++++++--
>>>    .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c    | 11 +++++++-
>>>    5 files changed, 46 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index 119f3ea50a7c..a7d8ecf3f5be 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -1200,6 +1200,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>>    		phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
>>>    		phys->cached_mode = crtc_state->adjusted_mode;
>>> +		if (phys->ops.atomic_mode_set)
>>> +			phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
>>>    	}
>>>    }
>>> 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 002e89cc1705..30470cd15a48 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> @@ -69,6 +69,8 @@ struct dpu_encoder_phys;
>>>     * @is_master:			Whether this phys_enc is the current master
>>>     *				encoder. Can be switched at enable time. Based
>>>     *				on split_role and current mode (CMD/VID).
>>> + * @atomic_mode_set:		DRM Call. Set a DRM mode.
>>> + *				This likely caches the mode, for use at enable.
>>>     * @enable:			DRM Call. Enable a DRM mode.
>>>     * @disable:			DRM Call. Disable mode.
>>>     * @control_vblank_irq		Register/Deregister for VBLANK IRQ
>>> @@ -93,6 +95,9 @@ struct dpu_encoder_phys;
>>>    struct dpu_encoder_phys_ops {
>>>    	void (*prepare_commit)(struct dpu_encoder_phys *encoder);
>>>    	bool (*is_master)(struct dpu_encoder_phys *encoder);
>>> +	void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
>>> +			struct drm_crtc_state *crtc_state,
>>> +			struct drm_connector_state *conn_state);
>>>    	void (*enable)(struct dpu_encoder_phys *encoder);
>>>    	void (*disable)(struct dpu_encoder_phys *encoder);
>>>    	int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool enable);
>>> 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 489be1c0c704..95cd39b49668 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
>>> @@ -142,6 +142,23 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg)
>>>    	dpu_encoder_underrun_callback(phys_enc->parent, phys_enc);
>>>    }
>>> +static void dpu_encoder_phys_cmd_atomic_mode_set(
>>> +		struct dpu_encoder_phys *phys_enc,
>>> +		struct drm_crtc_state *crtc_state,
>>> +		struct drm_connector_state *conn_state)
>>> +{
>>> +	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;
>>> +
>>> +	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;
>>> +}
>>> +
>>>    static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
>>>    		struct dpu_encoder_phys *phys_enc)
>>>    {
>>> @@ -280,14 +297,6 @@ static void dpu_encoder_phys_cmd_irq_enable(struct dpu_encoder_phys *phys_enc)
>>>    					  phys_enc->hw_pp->idx - PINGPONG_0,
>>>    					  phys_enc->vblank_refcount);
>>> -	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;
>>> -
>>> -	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;
>>> -
>>>    	dpu_core_irq_register_callback(phys_enc->dpu_kms,
>>>    				       phys_enc->irq[INTR_IDX_PINGPONG],
>>>    				       dpu_encoder_phys_cmd_pp_tx_done_irq,
>>> @@ -318,10 +327,6 @@ static void dpu_encoder_phys_cmd_irq_disable(struct dpu_encoder_phys *phys_enc)
>>>    	dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_UNDERRUN]);
>>>    	dpu_encoder_phys_cmd_control_vblank_irq(phys_enc, false);
>>>    	dpu_core_irq_unregister_callback(phys_enc->dpu_kms, phys_enc->irq[INTR_IDX_PINGPONG]);
>>> -
>>> -	phys_enc->irq[INTR_IDX_CTL_START] = 0;
>>> -	phys_enc->irq[INTR_IDX_PINGPONG] = 0;
>>> -	phys_enc->irq[INTR_IDX_RDPTR] = 0;
>>>    }
>>>    static void dpu_encoder_phys_cmd_tearcheck_config(
>>> @@ -698,6 +703,7 @@ static void dpu_encoder_phys_cmd_init_ops(
>>>    		struct dpu_encoder_phys_ops *ops)
>>>    {
>>>    	ops->is_master = dpu_encoder_phys_cmd_is_master;
>>> +	ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set;
>>>    	ops->enable = dpu_encoder_phys_cmd_enable;
>>>    	ops->disable = dpu_encoder_phys_cmd_disable;
>>>    	ops->control_vblank_irq = dpu_encoder_phys_cmd_control_vblank_irq;
>>> @@ -736,8 +742,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(struct drm_device *dev,
>>>    	dpu_encoder_phys_cmd_init_ops(&phys_enc->ops);
>>>    	phys_enc->intf_mode = INTF_MODE_CMD;
>>> -	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
>>> -
>>>    	cmd_enc->stream_sel = 0;
>>>    	if (!phys_enc->hw_intf) {
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>> index ef69c2f408c3..636a97432d51 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>> @@ -356,6 +356,16 @@ static bool dpu_encoder_phys_vid_needs_single_flush(
>>>    	return phys_enc->split_role != ENC_ROLE_SOLO;
>>>    }
>>> +static void dpu_encoder_phys_vid_atomic_mode_set(
>>> +		struct dpu_encoder_phys *phys_enc,
>>> +		struct drm_crtc_state *crtc_state,
>>> +		struct drm_connector_state *conn_state)
>>> +{
>>> +	phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
>>> +
>>> +	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
>>> +}
>>> +
>>>    static int dpu_encoder_phys_vid_control_vblank_irq(
>>>    		struct dpu_encoder_phys *phys_enc,
>>>    		bool enable)
>>> @@ -699,6 +709,7 @@ static int dpu_encoder_phys_vid_get_frame_count(
>>>    static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
>>>    {
>>>    	ops->is_master = dpu_encoder_phys_vid_is_master;
>>> +	ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set;
>>>    	ops->enable = dpu_encoder_phys_vid_enable;
>>>    	ops->disable = dpu_encoder_phys_vid_disable;
>>>    	ops->control_vblank_irq = dpu_encoder_phys_vid_control_vblank_irq;
>>> @@ -737,8 +748,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(struct drm_device *dev,
>>>    	dpu_encoder_phys_vid_init_ops(&phys_enc->ops);
>>>    	phys_enc->intf_mode = INTF_MODE_VIDEO;
>>> -	phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
>>> -	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
>>>    	DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->hw_intf->idx);
>>> 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
>>> index d3ea91c1d7d2..356dca5e5ea9 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>>> @@ -404,6 +404,15 @@ static void dpu_encoder_phys_wb_irq_disable(struct dpu_encoder_phys *phys)
>>>    		dpu_core_irq_unregister_callback(phys->dpu_kms, phys->irq[INTR_IDX_WB_DONE]);
>>>    }
>>> +static void dpu_encoder_phys_wb_atomic_mode_set(
>>> +		struct dpu_encoder_phys *phys_enc,
>>> +		struct drm_crtc_state *crtc_state,
>>> +		struct drm_connector_state *conn_state)
>>> +{
>>> +
>>> +	phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done;
>>> +}
>>> +
>>>    static void _dpu_encoder_phys_wb_handle_wbdone_timeout(
>>>    		struct dpu_encoder_phys *phys_enc)
>>>    {
>>> @@ -640,6 +649,7 @@ static bool dpu_encoder_phys_wb_is_valid_for_commit(struct dpu_encoder_phys *phy
>>>    static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
>>>    {
>>>    	ops->is_master = dpu_encoder_phys_wb_is_master;
>>> +	ops->atomic_mode_set = dpu_encoder_phys_wb_atomic_mode_set;
>>>    	ops->enable = dpu_encoder_phys_wb_enable;
>>>    	ops->disable = dpu_encoder_phys_wb_disable;
>>>    	ops->wait_for_commit_done = dpu_encoder_phys_wb_wait_for_commit_done;
>>> @@ -685,7 +695,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(struct drm_device *dev,
>>>    	dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
>>>    	phys_enc->intf_mode = INTF_MODE_WB_LINE;
>>> -	phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done;
>>>    	atomic_set(&wb_enc->wbirq_refcount, 0);
>>>
>>> ---
>>> base-commit: 75fa778d74b786a1608d55d655d42b480a6fa8bd
>>> change-id: 20240514-dpu-revert-ams-9410abc1ee48
>>>
>>> Best regards,
>