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

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

Hi Sean

Thanks for reviewing.

Reply inline.

On 2018-04-16 10:07, Sean Paul wrote:
> On Fri, Apr 13, 2018 at 03:04:48PM -0700, abhinavk@codeaurora.org 
> wrote:
>> 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.
> 
> Ok, thanks for confirming. Could you also please split this patch into 
> the
> wait4video_done ret fix and the ->enabled addition? The 2 seem mostly 
> unrelated.
> 
> Sean

They are quite related actually. So we were not able to catch this 
earlier
because the wait was silently timing out. Hence along with the fix I 
wanted
to change the ret to add the error log because such condition should not
happen with the ->enabled fix. To signify that I clubbed them together.
I can split them, if this still feels unrelated.

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