[PATCH] drm/msm: Grab a vblank reference when waiting for commit_done

[PATCH] drm/msm: Grab a vblank reference when waiting for commit_done

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

Similar to the atomic helpers, we should enable vblank while we're
waiting for the commit to finish. DPU needs this, MDP5 seems to work
fine without it.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/msm_atomic.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index c1f1779c980f..2b7bb6e166d3 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -32,7 +32,12 @@ static void msm_atomic_wait_for_commit_done(struct drm_device *dev,
 		if (!new_crtc_state->active)
 			continue;
 
+		if (drm_crtc_vblank_get(crtc))
+			continue;
+
 		kms->funcs->wait_for_crtc_commit_done(kms, crtc);
+
+		drm_crtc_vblank_put(crtc);
 	}
 }
 
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH] drm/msm: Grab a vblank reference when waiting for commit_done

[Abhinav Kumar]

On 2018-10-03 13:22, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> Similar to the atomic helpers, we should enable vblank while we're
> waiting for the commit to finish. DPU needs this, MDP5 seems to work
> fine without it.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
As such I dont see any issue with this patch but I have a question 
overall on chrome vblank handling.
For a video mode panel, we will keep the HW resources enabled (including 
vsync related clocks) till device
is suspended.

The vblank_get and vblank_put are only controlling the 
register/deregister of the vblank IRQ callback which
send the events to the userspace and anything pending on vsync 
completion.

Does the chrome userspace turn ON/OFF the vblank using vblank_ctrl IOCTL 
Or does it guarantee it to be?

If so, is this patch just more of a safety check to make sure that vsync 
events remain ON till the frame is done?

Because HW wise it should be and this shouldnt be needed.

>  drivers/gpu/drm/msm/msm_atomic.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c 
> b/drivers/gpu/drm/msm/msm_atomic.c
> index c1f1779c980f..2b7bb6e166d3 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -32,7 +32,12 @@ static void msm_atomic_wait_for_commit_done(struct
> drm_device *dev,
>  		if (!new_crtc_state->active)
>  			continue;
> 
> +		if (drm_crtc_vblank_get(crtc))
> +			continue;
> +
>  		kms->funcs->wait_for_crtc_commit_done(kms, crtc);
> +
> +		drm_crtc_vblank_put(crtc);
>  	}
>  }
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH] drm/msm: Grab a vblank reference when waiting for commit_done

[Sean Paul]

On Mon, Oct 08, 2018 at 06:38:11PM -0700, Abhinav Kumar wrote:
> On 2018-10-03 13:22, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > Similar to the atomic helpers, we should enable vblank while we're
> > waiting for the commit to finish. DPU needs this, MDP5 seems to work
> > fine without it.
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> As such I dont see any issue with this patch but I have a question overall
> on chrome vblank handling.
> For a video mode panel, we will keep the HW resources enabled (including
> vsync related clocks) till device
> is suspended.
> 
> The vblank_get and vblank_put are only controlling the register/deregister
> of the vblank IRQ callback which
> send the events to the userspace and anything pending on vsync completion.
> 
> Does the chrome userspace turn ON/OFF the vblank using vblank_ctrl IOCTL Or
> does it guarantee it to be?
>

No not explicitly. The issue I was seeing is that when userspace (this issue is
not specific to chrome) does a commit we get a frame_done timeout. So while the
hardware might still be active, the worker to signal frame_done is not unless
the vblank_ref is > 0.

> If so, is this patch just more of a safety check to make sure that vsync
> events remain ON till the frame is done?
> 
> Because HW wise it should be and this shouldnt be needed.

It is definitely needed, unless you want 60ms (the frame_done timeout) between
frames :-)

Sean

> 
> >  drivers/gpu/drm/msm/msm_atomic.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/msm/msm_atomic.c
> > b/drivers/gpu/drm/msm/msm_atomic.c
> > index c1f1779c980f..2b7bb6e166d3 100644
> > --- a/drivers/gpu/drm/msm/msm_atomic.c
> > +++ b/drivers/gpu/drm/msm/msm_atomic.c
> > @@ -32,7 +32,12 @@ static void msm_atomic_wait_for_commit_done(struct
> > drm_device *dev,
> >  		if (!new_crtc_state->active)
> >  			continue;
> > 
> > +		if (drm_crtc_vblank_get(crtc))
> > +			continue;
> > +
> >  		kms->funcs->wait_for_crtc_commit_done(kms, crtc);
> > +
> > +		drm_crtc_vblank_put(crtc);
> >  	}
> >  }

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH] drm/msm: Grab a vblank reference when waiting for commit_done

[Abhinav Kumar]

On 2018-10-09 06:59, Sean Paul wrote:
> On Mon, Oct 08, 2018 at 06:38:11PM -0700, Abhinav Kumar wrote:
>> On 2018-10-03 13:22, Sean Paul wrote:
>> > From: Sean Paul <seanpaul@chromium.org>
>> >
>> > Similar to the atomic helpers, we should enable vblank while we're
>> > waiting for the commit to finish. DPU needs this, MDP5 seems to work
>> > fine without it.
>> >
>> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> > ---
>> As such I dont see any issue with this patch but I have a question 
>> overall
>> on chrome vblank handling.
>> For a video mode panel, we will keep the HW resources enabled 
>> (including
>> vsync related clocks) till device
>> is suspended.
>> 
>> The vblank_get and vblank_put are only controlling the 
>> register/deregister
>> of the vblank IRQ callback which
>> send the events to the userspace and anything pending on vsync 
>> completion.
>> 
>> Does the chrome userspace turn ON/OFF the vblank using vblank_ctrl 
>> IOCTL Or
>> does it guarantee it to be?
>> 
> 
> No not explicitly. The issue I was seeing is that when userspace (this 
> issue is
> not specific to chrome) does a commit we get a frame_done timeout. So 
> while the
> hardware might still be active, the worker to signal frame_done is not 
> unless
> the vblank_ref is > 0.
> 
>> If so, is this patch just more of a safety check to make sure that 
>> vsync
>> events remain ON till the frame is done?
>> 
>> Because HW wise it should be and this shouldnt be needed.
> 
> It is definitely needed, unless you want 60ms (the frame_done timeout) 
> between
> frames :-)
> 
> Sean
> 
Alright, in that case

Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>

>> 
>> >  drivers/gpu/drm/msm/msm_atomic.c | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/msm/msm_atomic.c
>> > b/drivers/gpu/drm/msm/msm_atomic.c
>> > index c1f1779c980f..2b7bb6e166d3 100644
>> > --- a/drivers/gpu/drm/msm/msm_atomic.c
>> > +++ b/drivers/gpu/drm/msm/msm_atomic.c
>> > @@ -32,7 +32,12 @@ static void msm_atomic_wait_for_commit_done(struct
>> > drm_device *dev,
>> >  		if (!new_crtc_state->active)
>> >  			continue;
>> >
>> > +		if (drm_crtc_vblank_get(crtc))
>> > +			continue;
>> > +
>> >  		kms->funcs->wait_for_crtc_commit_done(kms, crtc);
>> > +
>> > +		drm_crtc_vblank_put(crtc);
>> >  	}
>> >  }
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno