Re: [DPU PATCH v2 1/2] drm/msm/dsi: check video mode engine status before waiting

[abhinavk-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org]

On 2018-04-13 14:10, abhinavk@codeaurora.org wrote:
> Hi Sean
> 
> Thanks for the review.
> 
> Reply inline.
> 
> On 2018-04-13 13:26, Sean Paul wrote:
>> On Tue, Apr 10, 2018 at 06:54:06PM -0700, Abhinav Kumar wrote:
>>> Make sure the video mode engine is on before waiting
>>> for the video done interrupt.
>>> 
>>> Otherwise it leads to silent timeouts increasing display
>>> turn ON time.
>>> 
>>> Changes in v2:
>>> - Replace pr_err with dev_err
>>> - Changed error message
>>> 
>>> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
>>> ---
>>>  drivers/gpu/drm/msm/dsi/dsi_host.c | 15 +++++++++++----
>>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> index 7a03a94..5b7b290 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> @@ -173,6 +173,7 @@ struct msm_dsi_host {
>>> 
>>>  	bool registered;
>>>  	bool power_on;
>>> +	bool enabled;
>>>  	int irq;
>>>  };
>>> 
>>> @@ -986,13 +987,19 @@ static void dsi_set_tx_power_mode(int mode, 
>>> struct msm_dsi_host *msm_host)
>>> 
>>>  static void dsi_wait4video_done(struct msm_dsi_host *msm_host)
>>>  {
>>> +	u32 ret = 0;
>>> +	struct device *dev = &msm_host->pdev->dev;
>>> +
>>>  	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 1);
>>> 
>>>  	reinit_completion(&msm_host->video_comp);
>>> 
>>> -	wait_for_completion_timeout(&msm_host->video_comp,
>>> +	ret = wait_for_completion_timeout(&msm_host->video_comp,
>>>  			msecs_to_jiffies(70));
>>> 
>>> +	if (ret <= 0)
>>> +		dev_err(dev, "wait for video done timed out\n");
>>> +
>>>  	dsi_intr_ctrl(msm_host, DSI_IRQ_MASK_VIDEO_DONE, 0);
>>>  }
>>> 
>>> @@ -1001,7 +1008,7 @@ static void dsi_wait4video_eng_busy(struct 
>>> msm_dsi_host *msm_host)
>>>  	if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
>>>  		return;
>>> 
>>> -	if (msm_host->power_on) {
>>> +	if (msm_host->power_on && msm_host->enabled) {
>>>  		dsi_wait4video_done(msm_host);
>>>  		/* delay 4 ms to skip BLLP */
>>>  		usleep_range(2000, 4000);
>>> @@ -2203,7 +2210,7 @@ int msm_dsi_host_enable(struct mipi_dsi_host 
>>> *host)
>>>  	 *	pm_runtime_put_autosuspend(&msm_host->pdev->dev);
>>>  	 * }
>>>  	 */
>>> -
>>> +	msm_host->enabled = true;
>>>  	return 0;
>>>  }
>>> 
>>> @@ -2219,7 +2226,7 @@ int msm_dsi_host_disable(struct mipi_dsi_host 
>>> *host)
>>>  	 * Reset to disable video engine so that we can send off cmd.
>>>  	 */
>>>  	dsi_sw_reset(msm_host);
>>> -
>>> +	msm_host->enabled = false;
>> 
>> This should go at the start of the function. Also, it's unclear from 
>> this patch,
>> but I assume this is protected by a lock?
>> 
>> Sean
> [Abhinav] Yes, will move this to the start.
> No, there is no lock here but at this point doesnt need one.
> The reason is that, this variable will be written to and read by the
> same process
> (suspend thread OR resume thread which sends the panel ON/OFF 
> commands).
> If we decide to expose other interfaces to send commands like debugfs
> or sysfs and
> introduce more threads, we will add the locking.
[Abhinav] Correction to my prev comment, we do have the 
msm_host->cmd_mutex which will
ensure this entire process is protected. That should suffice.
>> 
>> 
>>>  	return 0;
>>>  }
>>> 
>>> --
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>>> Forum,
>>> a Linux Foundation Collaborative Project
>>> 
>>> _______________________________________________
>>> Freedreno mailing list
>>> Freedreno@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/freedreno
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno