diff mbox series

drm/msm/dpu: Increment vsync_cnt before waking up userspace

Message ID 20220622023855.2970913-1-swboyd@chromium.org (mailing list archive)
State New, archived
Headers show
Series drm/msm/dpu: Increment vsync_cnt before waking up userspace | expand

Commit Message

Stephen Boyd June 22, 2022, 2:38 a.m. UTC 
The 'vsync_cnt' is used to count the number of frames for a crtc.
Unfortunately, we increment the count after waking up userspace via
dpu_crtc_vblank_callback() calling drm_crtc_handle_vblank().
drm_crtc_handle_vblank() wakes up userspace processes that have called
drm_wait_vblank_ioctl(), and if that ioctl is expecting the count to
increase it won't.

Increment the count before calling into the drm APIs so that we don't
have to worry about ordering the increment with anything else in drm.
This fixes a software video decode test that fails to see frame counts
increase on Trogdor boards.

Cc: Mark Yacoub <markyacoub@chromium.org>
Cc: Jessica Zhang <quic_jesszhan@quicinc.com>
Fixes: 885455d6bf82 ("drm/msm: Change dpu_crtc_get_vblank_counter to use vsync count.")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56

Comments

Abhinav Kumar June 22, 2022, 5:24 p.m. UTC | #1 
On 6/21/2022 7:38 PM, Stephen Boyd wrote:
> The 'vsync_cnt' is used to count the number of frames for a crtc.
> Unfortunately, we increment the count after waking up userspace via
> dpu_crtc_vblank_callback() calling drm_crtc_handle_vblank().
> drm_crtc_handle_vblank() wakes up userspace processes that have called
> drm_wait_vblank_ioctl(), and if that ioctl is expecting the count to
> increase it won't.
> 
> Increment the count before calling into the drm APIs so that we don't
> have to worry about ordering the increment with anything else in drm.
> This fixes a software video decode test that fails to see frame counts
> increase on Trogdor boards.
> 
> Cc: Mark Yacoub <markyacoub@chromium.org>
> Cc: Jessica Zhang <quic_jesszhan@quicinc.com>
> Fixes: 885455d6bf82 ("drm/msm: Change dpu_crtc_get_vblank_counter to use vsync count.")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

This is right, we should increment before drm_crtc_handle_vblank() as 
that will query the vblank counter. This also matches what we do 
downstream, hence

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

One small nit though, shouldnt the fixes tag be

25fdd5933e4c ("drm/msm: Add SDM845 DPU support")

This code has been this way since that commit itself.

> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 3a462e327e0e..a1b8c4592943 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1251,12 +1251,13 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
>   	DPU_ATRACE_BEGIN("encoder_vblank_callback");
>   	dpu_enc = to_dpu_encoder_virt(drm_enc);
>   
> +	atomic_inc(&phy_enc->vsync_cnt);
> +
>   	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
>   	if (dpu_enc->crtc)
>   		dpu_crtc_vblank_callback(dpu_enc->crtc);
>   	spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
>   
> -	atomic_inc(&phy_enc->vsync_cnt);
>   	DPU_ATRACE_END("encoder_vblank_callback");
>   }
>   
> 
> base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
Rob Clark June 22, 2022, 5:33 p.m. UTC | #2 
On Wed, Jun 22, 2022 at 10:24 AM Abhinav Kumar
<quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/21/2022 7:38 PM, Stephen Boyd wrote:
> > The 'vsync_cnt' is used to count the number of frames for a crtc.
> > Unfortunately, we increment the count after waking up userspace via
> > dpu_crtc_vblank_callback() calling drm_crtc_handle_vblank().
> > drm_crtc_handle_vblank() wakes up userspace processes that have called
> > drm_wait_vblank_ioctl(), and if that ioctl is expecting the count to
> > increase it won't.
> >
> > Increment the count before calling into the drm APIs so that we don't
> > have to worry about ordering the increment with anything else in drm.
> > This fixes a software video decode test that fails to see frame counts
> > increase on Trogdor boards.
> >
> > Cc: Mark Yacoub <markyacoub@chromium.org>
> > Cc: Jessica Zhang <quic_jesszhan@quicinc.com>
> > Fixes: 885455d6bf82 ("drm/msm: Change dpu_crtc_get_vblank_counter to use vsync count.")
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>
> This is right, we should increment before drm_crtc_handle_vblank() as
> that will query the vblank counter. This also matches what we do
> downstream, hence
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>
> One small nit though, shouldnt the fixes tag be
>
> 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")

*Kinda*.. but the sw vblank counter wasn't used for reporting frame nr
to userspace until 885455d6bf82.  You could possibly list both,
perhaps, but 885455d6bf82 is the important one for folks backporting
to stable kernels to be aware of

BR,
-R

>
> This code has been this way since that commit itself.
>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 3a462e327e0e..a1b8c4592943 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -1251,12 +1251,13 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
> >       DPU_ATRACE_BEGIN("encoder_vblank_callback");
> >       dpu_enc = to_dpu_encoder_virt(drm_enc);
> >
> > +     atomic_inc(&phy_enc->vsync_cnt);
> > +
> >       spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> >       if (dpu_enc->crtc)
> >               dpu_crtc_vblank_callback(dpu_enc->crtc);
> >       spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
> >
> > -     atomic_inc(&phy_enc->vsync_cnt);
> >       DPU_ATRACE_END("encoder_vblank_callback");
> >   }
> >
> >
> > base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
Dmitry Baryshkov June 22, 2022, 5:41 p.m. UTC | #3 
On 22/06/2022 20:33, Rob Clark wrote:
> On Wed, Jun 22, 2022 at 10:24 AM Abhinav Kumar
> <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 6/21/2022 7:38 PM, Stephen Boyd wrote:
>>> The 'vsync_cnt' is used to count the number of frames for a crtc.
>>> Unfortunately, we increment the count after waking up userspace via
>>> dpu_crtc_vblank_callback() calling drm_crtc_handle_vblank().
>>> drm_crtc_handle_vblank() wakes up userspace processes that have called
>>> drm_wait_vblank_ioctl(), and if that ioctl is expecting the count to
>>> increase it won't.
>>>
>>> Increment the count before calling into the drm APIs so that we don't
>>> have to worry about ordering the increment with anything else in drm.
>>> This fixes a software video decode test that fails to see frame counts
>>> increase on Trogdor boards.
>>>
>>> Cc: Mark Yacoub <markyacoub@chromium.org>
>>> Cc: Jessica Zhang <quic_jesszhan@quicinc.com>
>>> Fixes: 885455d6bf82 ("drm/msm: Change dpu_crtc_get_vblank_counter to use vsync count.")
>>> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>>
>> This is right, we should increment before drm_crtc_handle_vblank() as
>> that will query the vblank counter. This also matches what we do
>> downstream, hence
>>
>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>
>> One small nit though, shouldnt the fixes tag be
>>
>> 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> 
> *Kinda*.. but the sw vblank counter wasn't used for reporting frame nr
> to userspace until 885455d6bf82.  You could possibly list both,
> perhaps, but 885455d6bf82 is the important one for folks backporting
> to stable kernels to be aware of

I'd agree, the original Fixes tag seems good to me.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Jessica Zhang June 22, 2022, 5:43 p.m. UTC | #4 
On 6/21/2022 7:38 PM, Stephen Boyd wrote:
> The 'vsync_cnt' is used to count the number of frames for a crtc.
> Unfortunately, we increment the count after waking up userspace via
> dpu_crtc_vblank_callback() calling drm_crtc_handle_vblank().
> drm_crtc_handle_vblank() wakes up userspace processes that have called
> drm_wait_vblank_ioctl(), and if that ioctl is expecting the count to
> increase it won't.
> 
> Increment the count before calling into the drm APIs so that we don't
> have to worry about ordering the increment with anything else in drm.
> This fixes a software video decode test that fails to see frame counts
> increase on Trogdor boards.
> 
> Cc: Mark Yacoub <markyacoub@chromium.org>
> Cc: Jessica Zhang <quic_jesszhan@quicinc.com>
> Fixes: 885455d6bf82 ("drm/msm: Change dpu_crtc_get_vblank_counter to use vsync count.")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>

Tested-by: Jessica Zhang <quic_jesszhan@quicinc.com> # Trogdor (sc7180)

> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 3a462e327e0e..a1b8c4592943 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1251,12 +1251,13 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
>   	DPU_ATRACE_BEGIN("encoder_vblank_callback");
>   	dpu_enc = to_dpu_encoder_virt(drm_enc);
>   
> +	atomic_inc(&phy_enc->vsync_cnt);
> +
>   	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
>   	if (dpu_enc->crtc)
>   		dpu_crtc_vblank_callback(dpu_enc->crtc);
>   	spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
>   
> -	atomic_inc(&phy_enc->vsync_cnt);
>   	DPU_ATRACE_END("encoder_vblank_callback");
>   }
>   
> 
> base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
> -- 
> https://chromeos.dev
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 3a462e327e0e..a1b8c4592943 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1251,12 +1251,13 @@  static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
 	DPU_ATRACE_BEGIN("encoder_vblank_callback");
 	dpu_enc = to_dpu_encoder_virt(drm_enc);
 
+	atomic_inc(&phy_enc->vsync_cnt);
+
 	spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
 	if (dpu_enc->crtc)
 		dpu_crtc_vblank_callback(dpu_enc->crtc);
 	spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
 
-	atomic_inc(&phy_enc->vsync_cnt);
 	DPU_ATRACE_END("encoder_vblank_callback");
 }