On Fri, Apr 13, 2018 at 03:04:48PM -0700, abhinavk@xxxxxxxxxxxxxx wrote:
> On 2018-04-13 14:10, abhinavk@xxxxxxxxxxxxxx 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@xxxxxxxxxxxxxx>
> > > > ---
> > > > 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
> > >
> > >
> > > > return 0;
> > > > }
> > > >
> > > > --
> > > > The Qualcomm Innovation Center, Inc. is a member of the Code
> > > > Aurora Forum,
> > > > a Linux Foundation Collaborative Project
> > > >
> > > > _______________________________________________
> > > > Freedreno mailing list
> > > > Freedreno@xxxxxxxxxxxxxxxxxxxxx
> > > > 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@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Sean Paul, Software Engineer, Google / Chromium OS
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
