[PATCH 0/2] drm/msm: dpu: Fix cursor updates

[PATCH 0/2] drm/msm: dpu: Fix cursor updates

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

Hey all,
So here's the async cursor patchset, it works pretty well on my device
(video mode dsi). I do have a couple of concerns that hopefully people
might have solutions for.

The first patch masks off the flush register to exclude inactive bits.
I think it'd be nicer to clear those bits if the plane was being
disabled, but it wasn't obvious how I could do this. Any suggestions?

The second patch seems a bit more spread out than I would like. I'd
rather not propagate the async checks all over, but this seemed like the
minimum amount in order to get things working reliably. If you have
better suggestions, please speak up.

Thanks,

Sean

Sean Paul (2):
  drm/msm: dpu: Only check flush register against pending flushes
  drm/msm: dpu: Make legacy cursor updates asynchronous

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      | 44 ++++++++++---------
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h      |  3 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 21 +++++----
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h   |  6 ++-
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  5 ++-
 drivers/gpu/drm/msm/msm_atomic.c              |  3 +-
 7 files changed, 49 insertions(+), 35 deletions(-)

-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

[PATCH 1/2] drm/msm: dpu: Only check flush register against pending flushes

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

There exists a case where a flush of a plane/dma may have been triggered
& started from an async commit. If that plane/dma is subsequently disabled
by the next commit, the flush register will continue to hold the flush
bit for the disabled plane. Since the bit remains active,
pending_kickoff_cnt will never decrement and we'll miss frame_done
events.

This patch limits the check of flush_register to include only those bits
which have been updated with the latest commit.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 84de385a9f62..60f146f02b77 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -331,7 +331,7 @@ static void dpu_encoder_phys_vid_vblank_irq(void *arg, int irq_idx)
 	if (hw_ctl && hw_ctl->ops.get_flush_register)
 		flush_register = hw_ctl->ops.get_flush_register(hw_ctl);
 
-	if (flush_register == 0)
+	if (!(flush_register & hw_ctl->ops.get_pending_flush(hw_ctl)))
 		new_cnt = atomic_add_unless(&phys_enc->pending_kickoff_cnt,
 				-1, 0);
 	spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

Re: [PATCH 1/2] drm/msm: dpu: Only check flush register against pending flushes

[Jeykumar Sankaran]

On 2018-09-19 11:56, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> There exists a case where a flush of a plane/dma may have been 
> triggered
> & started from an async commit. If that plane/dma is subsequently 
> disabled
> by the next commit, the flush register will continue to hold the flush
> bit for the disabled plane. Since the bit remains active,
> pending_kickoff_cnt will never decrement and we'll miss frame_done
> events.
> 
I suppose this is the vblank in between the async commit and the next 
commit (one where
the plane is disabled).

If this vblank had consumed the flush bits, it means the HW
has read the configuration and it should have cleared bits.

If you still see the flush bit active, it means the async commit has 
missed the VBLANK boundary
and the HW has not yet taken the cursor configuration. So you are not 
supposed to
get frame_done event.

Comments outside the scope of this patch: To support async and sync 
updates
on the same display commit thread, we should be adding more protection 
to support
concurrency scenarios to avoid more than one ctl flushes per VBLANK 
period.

Thanks,
Jeykumar S.



> This patch limits the check of flush_register to include only those 
> bits
> which have been updated with the latest commit.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index 84de385a9f62..60f146f02b77 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -331,7 +331,7 @@ static void dpu_encoder_phys_vid_vblank_irq(void 
> *arg,
> int irq_idx)
>  	if (hw_ctl && hw_ctl->ops.get_flush_register)
>  		flush_register = hw_ctl->ops.get_flush_register(hw_ctl);
> 
> -	if (flush_register == 0)
> +	if (!(flush_register & hw_ctl->ops.get_pending_flush(hw_ctl)))
>  		new_cnt =
> atomic_add_unless(&phys_enc->pending_kickoff_cnt,
>  				-1, 0);
>  	spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);

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

Re: [PATCH 1/2] drm/msm: dpu: Only check flush register against pending flushes

[Sean Paul]

On Wed, Sep 26, 2018 at 11:51:35AM -0700, Jeykumar Sankaran wrote:
> On 2018-09-19 11:56, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > There exists a case where a flush of a plane/dma may have been triggered
> > & started from an async commit. If that plane/dma is subsequently
> > disabled
> > by the next commit, the flush register will continue to hold the flush
> > bit for the disabled plane. Since the bit remains active,
> > pending_kickoff_cnt will never decrement and we'll miss frame_done
> > events.
> > 
> I suppose this is the vblank in between the async commit and the next commit
> (one where
> the plane is disabled).
> 
> If this vblank had consumed the flush bits, it means the HW
> has read the configuration and it should have cleared bits.
> 
> If you still see the flush bit active, it means the async commit has missed
> the VBLANK boundary
> and the HW has not yet taken the cursor configuration. So you are not
> supposed to
> get frame_done event.

Right, we're not getting frame_done until the next frame comes in. The issue is
that we get 2 commits in between vblanks, the first commit triggers the cursor
for flush and the second one disables it. Unfortunately the first commit has
already called CTL_START and made it impossible for the second commit to clear
that flush bit (afaict).

The frame_done events seem to flow properly, only being triggered once per
vblank and only when a non-async commit has happened.

So is there a way to clear the CTL_FLUSH register on subsequent commits? I've
poked around some more and can't seem to figure it out.

> 
> Comments outside the scope of this patch: To support async and sync updates
> on the same display commit thread, we should be adding more protection to
> support
> concurrency scenarios to avoid more than one ctl flushes per VBLANK period.

Yeah, certainly easier said than done. I'm not really sure how to implement
that, tbh.

There's no way to know how many commits you'll have, and there's no way to
delay the FLUSH until right before vblank. Do you have any ideas that I might be
missing?

Sean

> 
> Thanks,
> Jeykumar S.
> 
> 
> 
> > This patch limits the check of flush_register to include only those bits
> > which have been updated with the latest commit.
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > index 84de385a9f62..60f146f02b77 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > @@ -331,7 +331,7 @@ static void dpu_encoder_phys_vid_vblank_irq(void
> > *arg,
> > int irq_idx)
> >  	if (hw_ctl && hw_ctl->ops.get_flush_register)
> >  		flush_register = hw_ctl->ops.get_flush_register(hw_ctl);
> > 
> > -	if (flush_register == 0)
> > +	if (!(flush_register & hw_ctl->ops.get_pending_flush(hw_ctl)))
> >  		new_cnt =
> > atomic_add_unless(&phys_enc->pending_kickoff_cnt,
> >  				-1, 0);
> >  	spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
> 
> -- 
> Jeykumar S

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

Re: [PATCH 1/2] drm/msm: dpu: Only check flush register against pending flushes

[Jeykumar Sankaran]

On 2018-10-01 13:29, Sean Paul wrote:
> On Wed, Sep 26, 2018 at 11:51:35AM -0700, Jeykumar Sankaran wrote:
>> On 2018-09-19 11:56, Sean Paul wrote:
>> > From: Sean Paul <seanpaul@chromium.org>
>> >
>> > There exists a case where a flush of a plane/dma may have been
> triggered
>> > & started from an async commit. If that plane/dma is subsequently
>> > disabled
>> > by the next commit, the flush register will continue to hold the flush
>> > bit for the disabled plane. Since the bit remains active,
>> > pending_kickoff_cnt will never decrement and we'll miss frame_done
>> > events.
>> >
>> I suppose this is the vblank in between the async commit and the next
> commit
>> (one where
>> the plane is disabled).
>> 
>> If this vblank had consumed the flush bits, it means the HW
>> has read the configuration and it should have cleared bits.
>> 
>> If you still see the flush bit active, it means the async commit has
> missed
>> the VBLANK boundary
>> and the HW has not yet taken the cursor configuration. So you are not
>> supposed to
>> get frame_done event.
> 
> Right, we're not getting frame_done until the next frame comes in. The
> issue is
> that we get 2 commits in between vblanks, the first commit triggers the
> cursor
> for flush and the second one disables it. Unfortunately the first 
> commit
> has
> already called CTL_START and made it impossible for the second commit 
> to
> clear
> that flush bit (afaict).
> 
> The frame_done events seem to flow properly, only being triggered once 
> per
> vblank and only when a non-async commit has happened.
> 
> So is there a way to clear the CTL_FLUSH register on subsequent 
> commits?
> I've
> poked around some more and can't seem to figure it out.
> 
We shouldn't be explicitly clearing the FLUSH register. Uncleared flush 
bits
generally indicates that the config is not programmed yet. What is the
observation if there is no followup non-async commit? Are you seeing a 
hang
or delay in frame_done? Eventually, the next VBLANK should pick the 
configuration
and wipe off the flush register and the vblank irq handler should 
trigger the event.

>> 
>> Comments outside the scope of this patch: To support async and sync
> updates
>> on the same display commit thread, we should be adding more protection
> to
>> support
>> concurrency scenarios to avoid more than one ctl flushes per VBLANK
> period.
> 
> Yeah, certainly easier said than done. I'm not really sure how to
> implement
> that, tbh.
> 
> There's no way to know how many commits you'll have, and there's no way 
> to
> delay the FLUSH until right before vblank. Do you have any ideas that I
> might be
> missing?
> 
One way to freeze hw reading from CTL_FLUSH is by writing 0x1 to 
CTL_FLUSH_MASK.
This guarantees no half-way programmed commit is flushed by the HW.

The other method - probably the non-conventional one - would be to 
update
cursor / async commits outside the display commit thread. But ofc, we 
need to make
architectural changes to the current driver to support this path.

Thanks,
Jeykumar S.
> Sean
> 
>> 
>> Thanks,
>> Jeykumar S.
>> 
>> 
>> 
>> > This patch limits the check of flush_register to include only those
> bits
>> > which have been updated with the latest commit.
>> >
>> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> > ---
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> > index 84de385a9f62..60f146f02b77 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> > @@ -331,7 +331,7 @@ static void dpu_encoder_phys_vid_vblank_irq(void
>> > *arg,
>> > int irq_idx)
>> >  	if (hw_ctl && hw_ctl->ops.get_flush_register)
>> >  		flush_register = hw_ctl->ops.get_flush_register(hw_ctl);
>> >
>> > -	if (flush_register == 0)
>> > +	if (!(flush_register & hw_ctl->ops.get_pending_flush(hw_ctl)))
>> >  		new_cnt =
>> > atomic_add_unless(&phys_enc->pending_kickoff_cnt,
>> >  				-1, 0);
>> >  	spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
>> 
>> --
>> Jeykumar S

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

Re: [PATCH 1/2] drm/msm: dpu: Only check flush register against pending flushes

[Sean Paul]

On Tue, Oct 02, 2018 at 06:14:38PM -0700, Jeykumar Sankaran wrote:
> On 2018-10-01 13:29, Sean Paul wrote:
> > On Wed, Sep 26, 2018 at 11:51:35AM -0700, Jeykumar Sankaran wrote:
> > > On 2018-09-19 11:56, Sean Paul wrote:
> > > > From: Sean Paul <seanpaul@chromium.org>
> > > >
> > > > There exists a case where a flush of a plane/dma may have been
> > triggered
> > > > & started from an async commit. If that plane/dma is subsequently
> > > > disabled
> > > > by the next commit, the flush register will continue to hold the flush
> > > > bit for the disabled plane. Since the bit remains active,
> > > > pending_kickoff_cnt will never decrement and we'll miss frame_done
> > > > events.
> > > >
> > > I suppose this is the vblank in between the async commit and the next
> > commit
> > > (one where
> > > the plane is disabled).
> > > 
> > > If this vblank had consumed the flush bits, it means the HW
> > > has read the configuration and it should have cleared bits.
> > > 
> > > If you still see the flush bit active, it means the async commit has
> > missed
> > > the VBLANK boundary
> > > and the HW has not yet taken the cursor configuration. So you are not
> > > supposed to
> > > get frame_done event.
> > 
> > Right, we're not getting frame_done until the next frame comes in. The
> > issue is
> > that we get 2 commits in between vblanks, the first commit triggers the
> > cursor
> > for flush and the second one disables it. Unfortunately the first commit
> > has
> > already called CTL_START and made it impossible for the second commit to
> > clear
> > that flush bit (afaict).
> > 
> > The frame_done events seem to flow properly, only being triggered once
> > per
> > vblank and only when a non-async commit has happened.
> > 
> > So is there a way to clear the CTL_FLUSH register on subsequent commits?
> > I've
> > poked around some more and can't seem to figure it out.
> > 
> We shouldn't be explicitly clearing the FLUSH register. Uncleared flush bits
> generally indicates that the config is not programmed yet. What is the
> observation if there is no followup non-async commit? Are you seeing a hang
> or delay in frame_done? 

There's no problems if there is no followup. Perhaps the issue is better
illustrated with some tracing. The following are traces when chrome changes the
UI and hides the cursor at the same time.


182.607515: dpu_hw_ctl_clear_pending_flush: pending_mask=10200c1 CTL_FLUSH=0
182.607569: dpu_hw_ctl_update_pending_flush: new=1020041 existing=0
182.607581: dpu_hw_ctl_update_pending_flush: new=1020081 existing=1020041
182.607822: dpu_kms_commit: id=46
182.607867: dpu_hw_ctl_trigger_pending_flush: pending_mask=10200c1 CTL_FLUSH=0

This is a normal atomic commit where chrome is providing updates to both planes
(primary and cursor). We clear the pending flush mask, figure out which planes
need to be flushed and trigger the flush.


182.607999: dpu_hw_ctl_clear_pending_flush: pending_mask=10200c1 CTL_FLUSH=10200c1
182.608042: dpu_hw_ctl_update_pending_flush: new=20041 existing=0
182.608054: dpu_hw_ctl_update_pending_flush: new=20081 existing=20041
182.608136: dpu_plane_disable: id:42 is_virtual:false multirect_mode:0
182.608222: dpu_kms_commit: id=46
182.608246: dpu_hw_ctl_trigger_pending_flush: pending_mask=200c1 CTL_FLUSH=10200c1

This commit comes in asynchronously before vblank and disables the cursor. As
you can see, CTL_FLUSH is still set at 10200c1 from the previous commit when
we first clear the pending mask. The new mask drops bit 24 (DMA2) and writes
200c1 (the current value of pending_mask) to CTL_FLUSH.

182.609924: dpu_enc_phys_vid_vblank: mask=0, register=1000000                     

When the vblank does come in, the hardware has flushed the visible planes, but
CTL_FLUSH still contains the bit for DMA2 since it was not flushed. The criteria
for frame_done is that the bitwise & of the CTL_FLUSH and pending_mask. Since
pending mask doesn't contain DMA1 and the flush register does, we don't get a
frame_done for the commit.


So there are 2 ways to solve this, afaict:

1- Clear CTL_FLUSH when we clear pending flush. AFAICT, there's no good way to
   do this, and it's inherently racy with vblank.
2- Ensure that the bits we care about in pending_flush_mask are cleared as a
   criteria for frame_done.

I'm certainly open to ideas if there's a better way, however I haven't found one
yet.

Sean

> Eventually, the next VBLANK should pick the
> configuration
> and wipe off the flush register and the vblank irq handler should trigger
> the event.
> 
> > > 
> > > Comments outside the scope of this patch: To support async and sync
> > updates
> > > on the same display commit thread, we should be adding more protection
> > to
> > > support
> > > concurrency scenarios to avoid more than one ctl flushes per VBLANK
> > period.
> > 
> > Yeah, certainly easier said than done. I'm not really sure how to
> > implement
> > that, tbh.
> > 
> > There's no way to know how many commits you'll have, and there's no way
> > to
> > delay the FLUSH until right before vblank. Do you have any ideas that I
> > might be
> > missing?
> > 
> One way to freeze hw reading from CTL_FLUSH is by writing 0x1 to
> CTL_FLUSH_MASK.
> This guarantees no half-way programmed commit is flushed by the HW.
> 
> The other method - probably the non-conventional one - would be to update
> cursor / async commits outside the display commit thread. But ofc, we need
> to make
> architectural changes to the current driver to support this path.
> 
> Thanks,
> Jeykumar S.
> > Sean
> > 
> > > 
> > > Thanks,
> > > Jeykumar S.
> > > 
> > > 
> > > 
> > > > This patch limits the check of flush_register to include only those
> > bits
> > > > which have been updated with the latest commit.
> > > >
> > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > ---
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > > > index 84de385a9f62..60f146f02b77 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > > > @@ -331,7 +331,7 @@ static void dpu_encoder_phys_vid_vblank_irq(void
> > > > *arg,
> > > > int irq_idx)
> > > >  	if (hw_ctl && hw_ctl->ops.get_flush_register)
> > > >  		flush_register = hw_ctl->ops.get_flush_register(hw_ctl);
> > > >
> > > > -	if (flush_register == 0)
> > > > +	if (!(flush_register & hw_ctl->ops.get_pending_flush(hw_ctl)))
> > > >  		new_cnt =
> > > > atomic_add_unless(&phys_enc->pending_kickoff_cnt,
> > > >  				-1, 0);
> > > >  	spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
> > > 
> > > --
> > > Jeykumar S
> 
> -- 
> Jeykumar S

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

[PATCH 2/2] drm/msm: dpu: Make legacy cursor updates asynchronous

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

This patch sprinkles a few async/legacy_cursor_update checks
through commit to ensure that cursor updates aren't blocked on vsync.
There are 2 main components to this, the first is that we don't want to
wait_for_commit_done in msm_atomic  before returning from atomic_complete.
The second is that in dpu we don't want to wait for frame_done events when
updating the cursor.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 44 +++++++++++----------
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  3 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 21 ++++++----
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  5 ++-
 drivers/gpu/drm/msm/msm_atomic.c            |  3 +-
 6 files changed, 48 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index a8f2dd7a37c7..b23f00a2554b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -816,7 +816,7 @@ static int _dpu_crtc_wait_for_frame_done(struct drm_crtc *crtc)
 	return rc;
 }
 
-void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
+void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async)
 {
 	struct drm_encoder *encoder;
 	struct drm_device *dev;
@@ -862,27 +862,30 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 		 * Encoder will flush/start now, unless it has a tx pending.
 		 * If so, it may delay and flush at an irq event (e.g. ppdone)
 		 */
-		dpu_encoder_prepare_for_kickoff(encoder, &params);
+		dpu_encoder_prepare_for_kickoff(encoder, &params, async);
 	}
 
-	/* wait for frame_event_done completion */
-	DPU_ATRACE_BEGIN("wait_for_frame_done_event");
-	ret = _dpu_crtc_wait_for_frame_done(crtc);
-	DPU_ATRACE_END("wait_for_frame_done_event");
-	if (ret) {
-		DPU_ERROR("crtc%d wait for frame done failed;frame_pending%d\n",
-				crtc->base.id,
-				atomic_read(&dpu_crtc->frame_pending));
-		goto end;
-	}
 
-	if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) {
-		/* acquire bandwidth and other resources */
-		DPU_DEBUG("crtc%d first commit\n", crtc->base.id);
-	} else
-		DPU_DEBUG("crtc%d commit\n", crtc->base.id);
+	if (!async) {
+		/* wait for frame_event_done completion */
+		DPU_ATRACE_BEGIN("wait_for_frame_done_event");
+		ret = _dpu_crtc_wait_for_frame_done(crtc);
+		DPU_ATRACE_END("wait_for_frame_done_event");
+		if (ret) {
+			DPU_ERROR("crtc%d wait for frame done failed;frame_pending%d\n",
+					crtc->base.id,
+					atomic_read(&dpu_crtc->frame_pending));
+			goto end;
+		}
+
+		if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) {
+			/* acquire bandwidth and other resources */
+			DPU_DEBUG("crtc%d first commit\n", crtc->base.id);
+		} else
+			DPU_DEBUG("crtc%d commit\n", crtc->base.id);
 
-	dpu_crtc->play_count++;
+		dpu_crtc->play_count++;
+	}
 
 	dpu_vbif_clear_errors(dpu_kms);
 
@@ -890,11 +893,12 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 		if (encoder->crtc != crtc)
 			continue;
 
-		dpu_encoder_kickoff(encoder);
+		dpu_encoder_kickoff(encoder, async);
 	}
 
 end:
-	reinit_completion(&dpu_crtc->frame_done_comp);
+	if (!async)
+		reinit_completion(&dpu_crtc->frame_done_comp);
 	DPU_ATRACE_END("crtc_commit");
 }
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 3723b4830335..67c9f59997cf 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -285,8 +285,9 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
 /**
  * dpu_crtc_commit_kickoff - trigger kickoff of the commit for this crtc
  * @crtc: Pointer to drm crtc object
+ * @async: true if the commit is asynchronous, false otherwise
  */
-void dpu_crtc_commit_kickoff(struct drm_crtc *crtc);
+void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async);
 
 /**
  * dpu_crtc_complete_commit - callback signalling completion of current commit
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index c2e8985b4c54..fc729fc8dd8c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1410,7 +1410,7 @@ static void dpu_encoder_off_work(struct kthread_work *work)
  * extra_flush_bits: Additional bit mask to include in flush trigger
  */
 static inline void _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc,
-		struct dpu_encoder_phys *phys, uint32_t extra_flush_bits)
+		struct dpu_encoder_phys *phys, uint32_t extra_flush_bits, bool async)
 {
 	struct dpu_hw_ctl *ctl;
 	int pending_kickoff_cnt;
@@ -1433,7 +1433,10 @@ static inline void _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc,
 		return;
 	}
 
-	pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
+	if (!async)
+		pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
+	else
+		pending_kickoff_cnt = atomic_read(&phys->pending_kickoff_cnt);
 
 	if (extra_flush_bits && ctl->ops.update_pending_flush)
 		ctl->ops.update_pending_flush(ctl, extra_flush_bits);
@@ -1545,7 +1548,8 @@ void dpu_encoder_helper_hw_reset(struct dpu_encoder_phys *phys_enc)
  *	a time.
  * dpu_enc: Pointer to virtual encoder structure
  */
-static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc)
+static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc,
+				      bool async)
 {
 	struct dpu_hw_ctl *ctl;
 	uint32_t i, pending_flush;
@@ -1576,7 +1580,8 @@ static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc)
 			set_bit(i, dpu_enc->frame_busy_mask);
 		if (!phys->ops.needs_single_flush ||
 				!phys->ops.needs_single_flush(phys))
-			_dpu_encoder_trigger_flush(&dpu_enc->base, phys, 0x0);
+			_dpu_encoder_trigger_flush(&dpu_enc->base, phys, 0x0,
+						   async);
 		else if (ctl->ops.get_pending_flush)
 			pending_flush |= ctl->ops.get_pending_flush(ctl);
 	}
@@ -1586,7 +1591,7 @@ static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc)
 		_dpu_encoder_trigger_flush(
 				&dpu_enc->base,
 				dpu_enc->cur_master,
-				pending_flush);
+				pending_flush, async);
 	}
 
 	_dpu_encoder_trigger_start(dpu_enc->cur_master);
@@ -1770,7 +1775,7 @@ static void dpu_encoder_vsync_event_work_handler(struct kthread_work *work)
 }
 
 void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc,
-		struct dpu_encoder_kickoff_params *params)
+		struct dpu_encoder_kickoff_params *params, bool async)
 {
 	struct dpu_encoder_virt *dpu_enc;
 	struct dpu_encoder_phys *phys;
@@ -1811,7 +1816,7 @@ void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc,
 	}
 }
 
-void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
+void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
 {
 	struct dpu_encoder_virt *dpu_enc;
 	struct dpu_encoder_phys *phys;
@@ -1834,7 +1839,7 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
 		((atomic_read(&dpu_enc->frame_done_timeout) * HZ) / 1000));
 
 	/* All phys encs are ready to go, trigger the kickoff */
-	_dpu_encoder_kickoff_phys(dpu_enc);
+	_dpu_encoder_kickoff_phys(dpu_enc, async);
 
 	/* allow phys encs to handle any post-kickoff business */
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 9dbf38f446d9..c2044122d609 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -81,9 +81,10 @@ void dpu_encoder_register_frame_event_callback(struct drm_encoder *encoder,
  *	Delayed: Block until next trigger can be issued.
  * @encoder:	encoder pointer
  * @params:	kickoff time parameters
+ * @async:	true if this is an asynchronous commit
  */
 void dpu_encoder_prepare_for_kickoff(struct drm_encoder *encoder,
-		struct dpu_encoder_kickoff_params *params);
+		struct dpu_encoder_kickoff_params *params, bool async);
 
 /**
  * dpu_encoder_trigger_kickoff_pending - Clear the flush bits from previous
@@ -96,8 +97,9 @@ void dpu_encoder_trigger_kickoff_pending(struct drm_encoder *encoder);
  * dpu_encoder_kickoff - trigger a double buffer flip of the ctl path
  *	(i.e. ctl flush and start) immediately.
  * @encoder:	encoder pointer
+ * @async:	true if this is an asynchronous commit
  */
-void dpu_encoder_kickoff(struct drm_encoder *encoder);
+void dpu_encoder_kickoff(struct drm_encoder *encoder, bool async);
 
 /**
  * dpu_encoder_wait_for_event - Waits for encoder events
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 0a683e65a9f3..1cba21edd862 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -352,7 +352,7 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder)
 
 	if (crtc && crtc->state->active) {
 		trace_dpu_kms_enc_enable(DRMID(crtc));
-		dpu_crtc_commit_kickoff(crtc);
+		dpu_crtc_commit_kickoff(crtc, false);
 	}
 }
 
@@ -369,7 +369,8 @@ static void dpu_kms_commit(struct msm_kms *kms, struct drm_atomic_state *state)
 
 		if (crtc->state->active) {
 			trace_dpu_kms_commit(DRMID(crtc));
-			dpu_crtc_commit_kickoff(crtc);
+			dpu_crtc_commit_kickoff(crtc,
+						state->legacy_cursor_update);
 		}
 	}
 }
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index c1f1779c980f..7912130ce5ce 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -76,7 +76,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 		kms->funcs->commit(kms, state);
 	}
 
-	msm_atomic_wait_for_commit_done(dev, state);
+	if (!state->legacy_cursor_update)
+		msm_atomic_wait_for_commit_done(dev, state);
 
 	kms->funcs->complete_commit(kms, state);
 
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

Re: [PATCH 2/2] drm/msm: dpu: Make legacy cursor updates asynchronous

[Jeykumar Sankaran]

On 2018-09-19 11:56, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> This patch sprinkles a few async/legacy_cursor_update checks
> through commit to ensure that cursor updates aren't blocked on vsync.
> There are 2 main components to this, the first is that we don't want to
> wait_for_commit_done in msm_atomic  before returning from 
> atomic_complete.
> The second is that in dpu we don't want to wait for frame_done events 
> when
> updating the cursor.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 44 +++++++++++----------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  3 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 21 ++++++----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  5 ++-
>  drivers/gpu/drm/msm/msm_atomic.c            |  3 +-
>  6 files changed, 48 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index a8f2dd7a37c7..b23f00a2554b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -816,7 +816,7 @@ static int _dpu_crtc_wait_for_frame_done(struct
> drm_crtc *crtc)
>  	return rc;
>  }
> 
> -void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
> +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async)
>  {
>  	struct drm_encoder *encoder;
>  	struct drm_device *dev;
> @@ -862,27 +862,30 @@ void dpu_crtc_commit_kickoff(struct drm_crtc 
> *crtc)
>  		 * Encoder will flush/start now, unless it has a tx
> pending.
>  		 * If so, it may delay and flush at an irq event (e.g.
> ppdone)
>  		 */
> -		dpu_encoder_prepare_for_kickoff(encoder, &params);
> +		dpu_encoder_prepare_for_kickoff(encoder, &params, async);
>  	}
> 
> -	/* wait for frame_event_done completion */
> -	DPU_ATRACE_BEGIN("wait_for_frame_done_event");
> -	ret = _dpu_crtc_wait_for_frame_done(crtc);
> -	DPU_ATRACE_END("wait_for_frame_done_event");
> -	if (ret) {
> -		DPU_ERROR("crtc%d wait for frame done
> failed;frame_pending%d\n",
> -				crtc->base.id,
> -				atomic_read(&dpu_crtc->frame_pending));
> -		goto end;
> -	}
> 
> -	if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) {
> -		/* acquire bandwidth and other resources */
> -		DPU_DEBUG("crtc%d first commit\n", crtc->base.id);
> -	} else
> -		DPU_DEBUG("crtc%d commit\n", crtc->base.id);
> +	if (!async) {
> +		/* wait for frame_event_done completion */
> +		DPU_ATRACE_BEGIN("wait_for_frame_done_event");
> +		ret = _dpu_crtc_wait_for_frame_done(crtc);
> +		DPU_ATRACE_END("wait_for_frame_done_event");
> +		if (ret) {
> +			DPU_ERROR("crtc%d wait for frame done
> failed;frame_pending%d\n",
> +					crtc->base.id,
> +
> atomic_read(&dpu_crtc->frame_pending));
> +			goto end;
> +		}
> +
> +		if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) {
> +			/* acquire bandwidth and other resources */
> +			DPU_DEBUG("crtc%d first commit\n", crtc->base.id);
> +		} else
> +			DPU_DEBUG("crtc%d commit\n", crtc->base.id);
> 
> -	dpu_crtc->play_count++;
> +		dpu_crtc->play_count++;
> +	}
> 
>  	dpu_vbif_clear_errors(dpu_kms);
> 
> @@ -890,11 +893,12 @@ void dpu_crtc_commit_kickoff(struct drm_crtc 
> *crtc)
>  		if (encoder->crtc != crtc)
>  			continue;
> 
> -		dpu_encoder_kickoff(encoder);
> +		dpu_encoder_kickoff(encoder, async);
>  	}
> 
>  end:
> -	reinit_completion(&dpu_crtc->frame_done_comp);
> +	if (!async)
> +		reinit_completion(&dpu_crtc->frame_done_comp);
>  	DPU_ATRACE_END("crtc_commit");
>  }
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 3723b4830335..67c9f59997cf 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -285,8 +285,9 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool 
> en);
>  /**
>   * dpu_crtc_commit_kickoff - trigger kickoff of the commit for this 
> crtc
>   * @crtc: Pointer to drm crtc object
> + * @async: true if the commit is asynchronous, false otherwise
>   */
> -void dpu_crtc_commit_kickoff(struct drm_crtc *crtc);
> +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async);
> 
>  /**
>   * dpu_crtc_complete_commit - callback signalling completion of 
> current
> commit
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index c2e8985b4c54..fc729fc8dd8c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1410,7 +1410,7 @@ static void dpu_encoder_off_work(struct 
> kthread_work
> *work)
>   * extra_flush_bits: Additional bit mask to include in flush trigger
>   */
>  static inline void _dpu_encoder_trigger_flush(struct drm_encoder
> *drm_enc,
> -		struct dpu_encoder_phys *phys, uint32_t extra_flush_bits)
> +		struct dpu_encoder_phys *phys, uint32_t extra_flush_bits,
> bool async)
>  {
>  	struct dpu_hw_ctl *ctl;
>  	int pending_kickoff_cnt;
> @@ -1433,7 +1433,10 @@ static inline void
> _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc,
>  		return;
>  	}
> 
> -	pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
> +	if (!async)
> +		pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
> +	else
> +		pending_kickoff_cnt =
> atomic_read(&phys->pending_kickoff_cnt);
> 
>  	if (extra_flush_bits && ctl->ops.update_pending_flush)
>  		ctl->ops.update_pending_flush(ctl, extra_flush_bits);
> @@ -1545,7 +1548,8 @@ void dpu_encoder_helper_hw_reset(struct
> dpu_encoder_phys *phys_enc)
>   *	a time.
>   * dpu_enc: Pointer to virtual encoder structure
>   */
> -static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt 
> *dpu_enc)
> +static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt 
> *dpu_enc,
> +				      bool async)
>  {
>  	struct dpu_hw_ctl *ctl;
>  	uint32_t i, pending_flush;
> @@ -1576,7 +1580,8 @@ static void _dpu_encoder_kickoff_phys(struct
> dpu_encoder_virt *dpu_enc)
>  			set_bit(i, dpu_enc->frame_busy_mask);
>  		if (!phys->ops.needs_single_flush ||
>  				!phys->ops.needs_single_flush(phys))
> -			_dpu_encoder_trigger_flush(&dpu_enc->base, phys,
> 0x0);
> +			_dpu_encoder_trigger_flush(&dpu_enc->base, phys,
> 0x0,
> +						   async);
>  		else if (ctl->ops.get_pending_flush)
>  			pending_flush |= ctl->ops.get_pending_flush(ctl);
>  	}
> @@ -1586,7 +1591,7 @@ static void _dpu_encoder_kickoff_phys(struct
> dpu_encoder_virt *dpu_enc)
>  		_dpu_encoder_trigger_flush(
>  				&dpu_enc->base,
>  				dpu_enc->cur_master,
> -				pending_flush);
> +				pending_flush, async);
>  	}
> 
>  	_dpu_encoder_trigger_start(dpu_enc->cur_master);
> @@ -1770,7 +1775,7 @@ static void
> dpu_encoder_vsync_event_work_handler(struct kthread_work *work)
>  }
> 
>  void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc,
> -		struct dpu_encoder_kickoff_params *params)
> +		struct dpu_encoder_kickoff_params *params, bool async)
>  {
>  	struct dpu_encoder_virt *dpu_enc;
>  	struct dpu_encoder_phys *phys;
> @@ -1811,7 +1816,7 @@ void dpu_encoder_prepare_for_kickoff(struct
> drm_encoder *drm_enc,
>  	}
>  }
> 
> -void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
> +void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
>  {
>  	struct dpu_encoder_virt *dpu_enc;
>  	struct dpu_encoder_phys *phys;
> @@ -1834,7 +1839,7 @@ void dpu_encoder_kickoff(struct drm_encoder
> *drm_enc)
>  		((atomic_read(&dpu_enc->frame_done_timeout) * HZ) /
> 1000));
> 
>  	/* All phys encs are ready to go, trigger the kickoff */
> -	_dpu_encoder_kickoff_phys(dpu_enc);
> +	_dpu_encoder_kickoff_phys(dpu_enc, async);
> 
>  	/* allow phys encs to handle any post-kickoff business */
>  	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 9dbf38f446d9..c2044122d609 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -81,9 +81,10 @@ void 
> dpu_encoder_register_frame_event_callback(struct
> drm_encoder *encoder,
>   *	Delayed: Block until next trigger can be issued.
>   * @encoder:	encoder pointer
>   * @params:	kickoff time parameters
> + * @async:	true if this is an asynchronous commit
>   */
>  void dpu_encoder_prepare_for_kickoff(struct drm_encoder *encoder,
> -		struct dpu_encoder_kickoff_params *params);
> +		struct dpu_encoder_kickoff_params *params, bool async);
> 
>  /**
>   * dpu_encoder_trigger_kickoff_pending - Clear the flush bits from
> previous
> @@ -96,8 +97,9 @@ void dpu_encoder_trigger_kickoff_pending(struct
> drm_encoder *encoder);
>   * dpu_encoder_kickoff - trigger a double buffer flip of the ctl path
>   *	(i.e. ctl flush and start) immediately.
>   * @encoder:	encoder pointer
> + * @async:	true if this is an asynchronous commit
>   */
> -void dpu_encoder_kickoff(struct drm_encoder *encoder);
> +void dpu_encoder_kickoff(struct drm_encoder *encoder, bool async);
> 
>  /**
>   * dpu_encoder_wait_for_event - Waits for encoder events
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 0a683e65a9f3..1cba21edd862 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -352,7 +352,7 @@ void dpu_kms_encoder_enable(struct drm_encoder
> *encoder)
> 
>  	if (crtc && crtc->state->active) {
>  		trace_dpu_kms_enc_enable(DRMID(crtc));
> -		dpu_crtc_commit_kickoff(crtc);
> +		dpu_crtc_commit_kickoff(crtc, false);
>  	}
>  }
> 
> @@ -369,7 +369,8 @@ static void dpu_kms_commit(struct msm_kms *kms, 
> struct
> drm_atomic_state *state)
> 
>  		if (crtc->state->active) {
>  			trace_dpu_kms_commit(DRMID(crtc));
> -			dpu_crtc_commit_kickoff(crtc);
> +			dpu_crtc_commit_kickoff(crtc,
> +
> state->legacy_cursor_update);
>  		}
>  	}
>  }
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c
> b/drivers/gpu/drm/msm/msm_atomic.c
> index c1f1779c980f..7912130ce5ce 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -76,7 +76,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state
> *state)
>  		kms->funcs->commit(kms, state);
>  	}
> 
> -	msm_atomic_wait_for_commit_done(dev, state);
> +	if (!state->legacy_cursor_update)
I see state->async_update is updated after validation checks on the 
async capabilities. Shouldn't we use
that var instead of state->legacy_cursor_update?
> +		msm_atomic_wait_for_commit_done(dev, state);
> 
>  	kms->funcs->complete_commit(kms, state);

Do we need to introduce plane helpers atomic_async_update and 
atomic_async_check in DPU before supporting
these wait skips? or are they irrelevant for this legacy async path?

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

Re: [PATCH 2/2] drm/msm: dpu: Make legacy cursor updates asynchronous

[Sean Paul]

On Wed, Sep 26, 2018 at 11:56:47AM -0700, Jeykumar Sankaran wrote:
> On 2018-09-19 11:56, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > This patch sprinkles a few async/legacy_cursor_update checks
> > through commit to ensure that cursor updates aren't blocked on vsync.
> > There are 2 main components to this, the first is that we don't want to
> > wait_for_commit_done in msm_atomic  before returning from
> > atomic_complete.
> > The second is that in dpu we don't want to wait for frame_done events
> > when
> > updating the cursor.
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 44 +++++++++++----------
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  3 +-
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 21 ++++++----
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++-
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  5 ++-
> >  drivers/gpu/drm/msm/msm_atomic.c            |  3 +-
> >  6 files changed, 48 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index a8f2dd7a37c7..b23f00a2554b 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -816,7 +816,7 @@ static int _dpu_crtc_wait_for_frame_done(struct
> > drm_crtc *crtc)
> >  	return rc;
> >  }
> > 
> > -void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
> > +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async)
> >  {
> >  	struct drm_encoder *encoder;
> >  	struct drm_device *dev;
> > @@ -862,27 +862,30 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
> > *crtc)
> >  		 * Encoder will flush/start now, unless it has a tx
> > pending.
> >  		 * If so, it may delay and flush at an irq event (e.g.
> > ppdone)
> >  		 */
> > -		dpu_encoder_prepare_for_kickoff(encoder, &params);
> > +		dpu_encoder_prepare_for_kickoff(encoder, &params, async);
> >  	}
> > 
> > -	/* wait for frame_event_done completion */
> > -	DPU_ATRACE_BEGIN("wait_for_frame_done_event");
> > -	ret = _dpu_crtc_wait_for_frame_done(crtc);
> > -	DPU_ATRACE_END("wait_for_frame_done_event");
> > -	if (ret) {
> > -		DPU_ERROR("crtc%d wait for frame done
> > failed;frame_pending%d\n",
> > -				crtc->base.id,
> > -				atomic_read(&dpu_crtc->frame_pending));
> > -		goto end;
> > -	}
> > 
> > -	if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) {
> > -		/* acquire bandwidth and other resources */
> > -		DPU_DEBUG("crtc%d first commit\n", crtc->base.id);
> > -	} else
> > -		DPU_DEBUG("crtc%d commit\n", crtc->base.id);
> > +	if (!async) {
> > +		/* wait for frame_event_done completion */
> > +		DPU_ATRACE_BEGIN("wait_for_frame_done_event");
> > +		ret = _dpu_crtc_wait_for_frame_done(crtc);
> > +		DPU_ATRACE_END("wait_for_frame_done_event");
> > +		if (ret) {
> > +			DPU_ERROR("crtc%d wait for frame done
> > failed;frame_pending%d\n",
> > +					crtc->base.id,
> > +
> > atomic_read(&dpu_crtc->frame_pending));
> > +			goto end;
> > +		}
> > +
> > +		if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) {
> > +			/* acquire bandwidth and other resources */
> > +			DPU_DEBUG("crtc%d first commit\n", crtc->base.id);
> > +		} else
> > +			DPU_DEBUG("crtc%d commit\n", crtc->base.id);
> > 
> > -	dpu_crtc->play_count++;
> > +		dpu_crtc->play_count++;
> > +	}
> > 
> >  	dpu_vbif_clear_errors(dpu_kms);
> > 
> > @@ -890,11 +893,12 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
> > *crtc)
> >  		if (encoder->crtc != crtc)
> >  			continue;
> > 
> > -		dpu_encoder_kickoff(encoder);
> > +		dpu_encoder_kickoff(encoder, async);
> >  	}
> > 
> >  end:
> > -	reinit_completion(&dpu_crtc->frame_done_comp);
> > +	if (!async)
> > +		reinit_completion(&dpu_crtc->frame_done_comp);
> >  	DPU_ATRACE_END("crtc_commit");
> >  }
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > index 3723b4830335..67c9f59997cf 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > @@ -285,8 +285,9 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
> >  /**
> >   * dpu_crtc_commit_kickoff - trigger kickoff of the commit for this
> > crtc
> >   * @crtc: Pointer to drm crtc object
> > + * @async: true if the commit is asynchronous, false otherwise
> >   */
> > -void dpu_crtc_commit_kickoff(struct drm_crtc *crtc);
> > +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async);
> > 
> >  /**
> >   * dpu_crtc_complete_commit - callback signalling completion of current
> > commit
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index c2e8985b4c54..fc729fc8dd8c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -1410,7 +1410,7 @@ static void dpu_encoder_off_work(struct
> > kthread_work
> > *work)
> >   * extra_flush_bits: Additional bit mask to include in flush trigger
> >   */
> >  static inline void _dpu_encoder_trigger_flush(struct drm_encoder
> > *drm_enc,
> > -		struct dpu_encoder_phys *phys, uint32_t extra_flush_bits)
> > +		struct dpu_encoder_phys *phys, uint32_t extra_flush_bits,
> > bool async)
> >  {
> >  	struct dpu_hw_ctl *ctl;
> >  	int pending_kickoff_cnt;
> > @@ -1433,7 +1433,10 @@ static inline void
> > _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc,
> >  		return;
> >  	}
> > 
> > -	pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
> > +	if (!async)
> > +		pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
> > +	else
> > +		pending_kickoff_cnt =
> > atomic_read(&phys->pending_kickoff_cnt);
> > 
> >  	if (extra_flush_bits && ctl->ops.update_pending_flush)
> >  		ctl->ops.update_pending_flush(ctl, extra_flush_bits);
> > @@ -1545,7 +1548,8 @@ void dpu_encoder_helper_hw_reset(struct
> > dpu_encoder_phys *phys_enc)
> >   *	a time.
> >   * dpu_enc: Pointer to virtual encoder structure
> >   */
> > -static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc)
> > +static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc,
> > +				      bool async)
> >  {
> >  	struct dpu_hw_ctl *ctl;
> >  	uint32_t i, pending_flush;
> > @@ -1576,7 +1580,8 @@ static void _dpu_encoder_kickoff_phys(struct
> > dpu_encoder_virt *dpu_enc)
> >  			set_bit(i, dpu_enc->frame_busy_mask);
> >  		if (!phys->ops.needs_single_flush ||
> >  				!phys->ops.needs_single_flush(phys))
> > -			_dpu_encoder_trigger_flush(&dpu_enc->base, phys,
> > 0x0);
> > +			_dpu_encoder_trigger_flush(&dpu_enc->base, phys,
> > 0x0,
> > +						   async);
> >  		else if (ctl->ops.get_pending_flush)
> >  			pending_flush |= ctl->ops.get_pending_flush(ctl);
> >  	}
> > @@ -1586,7 +1591,7 @@ static void _dpu_encoder_kickoff_phys(struct
> > dpu_encoder_virt *dpu_enc)
> >  		_dpu_encoder_trigger_flush(
> >  				&dpu_enc->base,
> >  				dpu_enc->cur_master,
> > -				pending_flush);
> > +				pending_flush, async);
> >  	}
> > 
> >  	_dpu_encoder_trigger_start(dpu_enc->cur_master);
> > @@ -1770,7 +1775,7 @@ static void
> > dpu_encoder_vsync_event_work_handler(struct kthread_work *work)
> >  }
> > 
> >  void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc,
> > -		struct dpu_encoder_kickoff_params *params)
> > +		struct dpu_encoder_kickoff_params *params, bool async)
> >  {
> >  	struct dpu_encoder_virt *dpu_enc;
> >  	struct dpu_encoder_phys *phys;
> > @@ -1811,7 +1816,7 @@ void dpu_encoder_prepare_for_kickoff(struct
> > drm_encoder *drm_enc,
> >  	}
> >  }
> > 
> > -void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
> > +void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
> >  {
> >  	struct dpu_encoder_virt *dpu_enc;
> >  	struct dpu_encoder_phys *phys;
> > @@ -1834,7 +1839,7 @@ void dpu_encoder_kickoff(struct drm_encoder
> > *drm_enc)
> >  		((atomic_read(&dpu_enc->frame_done_timeout) * HZ) /
> > 1000));
> > 
> >  	/* All phys encs are ready to go, trigger the kickoff */
> > -	_dpu_encoder_kickoff_phys(dpu_enc);
> > +	_dpu_encoder_kickoff_phys(dpu_enc, async);
> > 
> >  	/* allow phys encs to handle any post-kickoff business */
> >  	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > index 9dbf38f446d9..c2044122d609 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > @@ -81,9 +81,10 @@ void dpu_encoder_register_frame_event_callback(struct
> > drm_encoder *encoder,
> >   *	Delayed: Block until next trigger can be issued.
> >   * @encoder:	encoder pointer
> >   * @params:	kickoff time parameters
> > + * @async:	true if this is an asynchronous commit
> >   */
> >  void dpu_encoder_prepare_for_kickoff(struct drm_encoder *encoder,
> > -		struct dpu_encoder_kickoff_params *params);
> > +		struct dpu_encoder_kickoff_params *params, bool async);
> > 
> >  /**
> >   * dpu_encoder_trigger_kickoff_pending - Clear the flush bits from
> > previous
> > @@ -96,8 +97,9 @@ void dpu_encoder_trigger_kickoff_pending(struct
> > drm_encoder *encoder);
> >   * dpu_encoder_kickoff - trigger a double buffer flip of the ctl path
> >   *	(i.e. ctl flush and start) immediately.
> >   * @encoder:	encoder pointer
> > + * @async:	true if this is an asynchronous commit
> >   */
> > -void dpu_encoder_kickoff(struct drm_encoder *encoder);
> > +void dpu_encoder_kickoff(struct drm_encoder *encoder, bool async);
> > 
> >  /**
> >   * dpu_encoder_wait_for_event - Waits for encoder events
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 0a683e65a9f3..1cba21edd862 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -352,7 +352,7 @@ void dpu_kms_encoder_enable(struct drm_encoder
> > *encoder)
> > 
> >  	if (crtc && crtc->state->active) {
> >  		trace_dpu_kms_enc_enable(DRMID(crtc));
> > -		dpu_crtc_commit_kickoff(crtc);
> > +		dpu_crtc_commit_kickoff(crtc, false);
> >  	}
> >  }
> > 
> > @@ -369,7 +369,8 @@ static void dpu_kms_commit(struct msm_kms *kms,
> > struct
> > drm_atomic_state *state)
> > 
> >  		if (crtc->state->active) {
> >  			trace_dpu_kms_commit(DRMID(crtc));
> > -			dpu_crtc_commit_kickoff(crtc);
> > +			dpu_crtc_commit_kickoff(crtc,
> > +
> > state->legacy_cursor_update);
> >  		}
> >  	}
> >  }
> > diff --git a/drivers/gpu/drm/msm/msm_atomic.c
> > b/drivers/gpu/drm/msm/msm_atomic.c
> > index c1f1779c980f..7912130ce5ce 100644
> > --- a/drivers/gpu/drm/msm/msm_atomic.c
> > +++ b/drivers/gpu/drm/msm/msm_atomic.c
> > @@ -76,7 +76,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state
> > *state)
> >  		kms->funcs->commit(kms, state);
> >  	}
> > 
> > -	msm_atomic_wait_for_commit_done(dev, state);
> > +	if (!state->legacy_cursor_update)
> I see state->async_update is updated after validation checks on the async
> capabilities. Shouldn't we use
> that var instead of state->legacy_cursor_update?
> > +		msm_atomic_wait_for_commit_done(dev, state);
> > 
> >  	kms->funcs->complete_commit(kms, state);
> 
> Do we need to introduce plane helpers atomic_async_update and
> atomic_async_check in DPU before supporting
> these wait skips? or are they irrelevant for this legacy async path?

I was trying to limit the scope of this to just cursor updates. I think once/if
support is added for generic async it makes sense to change over to that
verbage.

Sean

> 
> -- 
> Jeykumar S

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

Re: [PATCH 2/2] drm/msm: dpu: Make legacy cursor updates asynchronous

[Jeykumar Sankaran]

On 2018-10-01 13:30, Sean Paul wrote:
> On Wed, Sep 26, 2018 at 11:56:47AM -0700, Jeykumar Sankaran wrote:
>> On 2018-09-19 11:56, Sean Paul wrote:
>> > From: Sean Paul <seanpaul@chromium.org>
>> >
>> > This patch sprinkles a few async/legacy_cursor_update checks
>> > through commit to ensure that cursor updates aren't blocked on vsync.
>> > There are 2 main components to this, the first is that we don't want
> to
>> > wait_for_commit_done in msm_atomic  before returning from
>> > atomic_complete.
>> > The second is that in dpu we don't want to wait for frame_done events
>> > when
>> > updating the cursor.
>> >
>> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> > ---
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 44
> +++++++++++----------
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  3 +-
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 21 ++++++----
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++-
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  5 ++-
>> >  drivers/gpu/drm/msm/msm_atomic.c            |  3 +-
>> >  6 files changed, 48 insertions(+), 34 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > index a8f2dd7a37c7..b23f00a2554b 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > @@ -816,7 +816,7 @@ static int _dpu_crtc_wait_for_frame_done(struct
>> > drm_crtc *crtc)
>> >  	return rc;
>> >  }
>> >
>> > -void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
>> > +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async)
>> >  {
>> >  	struct drm_encoder *encoder;
>> >  	struct drm_device *dev;
>> > @@ -862,27 +862,30 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
>> > *crtc)
>> >  		 * Encoder will flush/start now, unless it has a tx
>> > pending.
>> >  		 * If so, it may delay and flush at an irq event (e.g.
>> > ppdone)
>> >  		 */
>> > -		dpu_encoder_prepare_for_kickoff(encoder, &params);
>> > +		dpu_encoder_prepare_for_kickoff(encoder, &params, async);
>> >  	}
>> >
>> > -	/* wait for frame_event_done completion */
>> > -	DPU_ATRACE_BEGIN("wait_for_frame_done_event");
>> > -	ret = _dpu_crtc_wait_for_frame_done(crtc);
>> > -	DPU_ATRACE_END("wait_for_frame_done_event");
>> > -	if (ret) {
>> > -		DPU_ERROR("crtc%d wait for frame done
>> > failed;frame_pending%d\n",
>> > -				crtc->base.id,
>> > -				atomic_read(&dpu_crtc->frame_pending));
>> > -		goto end;
>> > -	}
>> >
>> > -	if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) {
>> > -		/* acquire bandwidth and other resources */
>> > -		DPU_DEBUG("crtc%d first commit\n", crtc->base.id);
>> > -	} else
>> > -		DPU_DEBUG("crtc%d commit\n", crtc->base.id);
>> > +	if (!async) {
>> > +		/* wait for frame_event_done completion */
>> > +		DPU_ATRACE_BEGIN("wait_for_frame_done_event");
>> > +		ret = _dpu_crtc_wait_for_frame_done(crtc);
>> > +		DPU_ATRACE_END("wait_for_frame_done_event");
>> > +		if (ret) {
>> > +			DPU_ERROR("crtc%d wait for frame done
>> > failed;frame_pending%d\n",
>> > +					crtc->base.id,
>> > +
>> > atomic_read(&dpu_crtc->frame_pending));
>> > +			goto end;
>> > +		}
>> > +
>> > +		if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) {
>> > +			/* acquire bandwidth and other resources */
>> > +			DPU_DEBUG("crtc%d first commit\n", crtc->base.id);
>> > +		} else
>> > +			DPU_DEBUG("crtc%d commit\n", crtc->base.id);
>> >
>> > -	dpu_crtc->play_count++;
>> > +		dpu_crtc->play_count++;
>> > +	}
>> >
>> >  	dpu_vbif_clear_errors(dpu_kms);
>> >
>> > @@ -890,11 +893,12 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
>> > *crtc)
>> >  		if (encoder->crtc != crtc)
>> >  			continue;
>> >
>> > -		dpu_encoder_kickoff(encoder);
>> > +		dpu_encoder_kickoff(encoder, async);
>> >  	}
>> >
>> >  end:
>> > -	reinit_completion(&dpu_crtc->frame_done_comp);
>> > +	if (!async)
>> > +		reinit_completion(&dpu_crtc->frame_done_comp);
>> >  	DPU_ATRACE_END("crtc_commit");
>> >  }
>> >
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> > index 3723b4830335..67c9f59997cf 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> > @@ -285,8 +285,9 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool
> en);
>> >  /**
>> >   * dpu_crtc_commit_kickoff - trigger kickoff of the commit for this
>> > crtc
>> >   * @crtc: Pointer to drm crtc object
>> > + * @async: true if the commit is asynchronous, false otherwise
>> >   */
>> > -void dpu_crtc_commit_kickoff(struct drm_crtc *crtc);
>> > +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async);
>> >
>> >  /**
>> >   * dpu_crtc_complete_commit - callback signalling completion of
> current
>> > commit
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > index c2e8985b4c54..fc729fc8dd8c 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > @@ -1410,7 +1410,7 @@ static void dpu_encoder_off_work(struct
>> > kthread_work
>> > *work)
>> >   * extra_flush_bits: Additional bit mask to include in flush trigger
>> >   */
>> >  static inline void _dpu_encoder_trigger_flush(struct drm_encoder
>> > *drm_enc,
>> > -		struct dpu_encoder_phys *phys, uint32_t extra_flush_bits)
>> > +		struct dpu_encoder_phys *phys, uint32_t extra_flush_bits,
>> > bool async)
>> >  {
>> >  	struct dpu_hw_ctl *ctl;
>> >  	int pending_kickoff_cnt;
>> > @@ -1433,7 +1433,10 @@ static inline void
>> > _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc,
>> >  		return;
>> >  	}
>> >
>> > -	pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
>> > +	if (!async)
>> > +		pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
>> > +	else
>> > +		pending_kickoff_cnt =
>> > atomic_read(&phys->pending_kickoff_cnt);
>> >
>> >  	if (extra_flush_bits && ctl->ops.update_pending_flush)
>> >  		ctl->ops.update_pending_flush(ctl, extra_flush_bits);
>> > @@ -1545,7 +1548,8 @@ void dpu_encoder_helper_hw_reset(struct
>> > dpu_encoder_phys *phys_enc)
>> >   *	a time.
>> >   * dpu_enc: Pointer to virtual encoder structure
>> >   */
>> > -static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt
> *dpu_enc)
>> > +static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt
> *dpu_enc,
>> > +				      bool async)
>> >  {
>> >  	struct dpu_hw_ctl *ctl;
>> >  	uint32_t i, pending_flush;
>> > @@ -1576,7 +1580,8 @@ static void _dpu_encoder_kickoff_phys(struct
>> > dpu_encoder_virt *dpu_enc)
>> >  			set_bit(i, dpu_enc->frame_busy_mask);
>> >  		if (!phys->ops.needs_single_flush ||
>> >  				!phys->ops.needs_single_flush(phys))
>> > -			_dpu_encoder_trigger_flush(&dpu_enc->base, phys,
>> > 0x0);
>> > +			_dpu_encoder_trigger_flush(&dpu_enc->base, phys,
>> > 0x0,
>> > +						   async);
>> >  		else if (ctl->ops.get_pending_flush)
>> >  			pending_flush |= ctl->ops.get_pending_flush(ctl);
>> >  	}
>> > @@ -1586,7 +1591,7 @@ static void _dpu_encoder_kickoff_phys(struct
>> > dpu_encoder_virt *dpu_enc)
>> >  		_dpu_encoder_trigger_flush(
>> >  				&dpu_enc->base,
>> >  				dpu_enc->cur_master,
>> > -				pending_flush);
>> > +				pending_flush, async);
>> >  	}
>> >
>> >  	_dpu_encoder_trigger_start(dpu_enc->cur_master);
>> > @@ -1770,7 +1775,7 @@ static void
>> > dpu_encoder_vsync_event_work_handler(struct kthread_work *work)
>> >  }
>> >
>> >  void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc,
>> > -		struct dpu_encoder_kickoff_params *params)
>> > +		struct dpu_encoder_kickoff_params *params, bool async)
>> >  {
>> >  	struct dpu_encoder_virt *dpu_enc;
>> >  	struct dpu_encoder_phys *phys;
>> > @@ -1811,7 +1816,7 @@ void dpu_encoder_prepare_for_kickoff(struct
>> > drm_encoder *drm_enc,
>> >  	}
>> >  }
>> >
>> > -void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
>> > +void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
>> >  {
>> >  	struct dpu_encoder_virt *dpu_enc;
>> >  	struct dpu_encoder_phys *phys;
>> > @@ -1834,7 +1839,7 @@ void dpu_encoder_kickoff(struct drm_encoder
>> > *drm_enc)
>> >  		((atomic_read(&dpu_enc->frame_done_timeout) * HZ) /
>> > 1000));
>> >
>> >  	/* All phys encs are ready to go, trigger the kickoff */
>> > -	_dpu_encoder_kickoff_phys(dpu_enc);
>> > +	_dpu_encoder_kickoff_phys(dpu_enc, async);
>> >
>> >  	/* allow phys encs to handle any post-kickoff business */
>> >  	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> > index 9dbf38f446d9..c2044122d609 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> > @@ -81,9 +81,10 @@ void
> dpu_encoder_register_frame_event_callback(struct
>> > drm_encoder *encoder,
>> >   *	Delayed: Block until next trigger can be issued.
>> >   * @encoder:	encoder pointer
>> >   * @params:	kickoff time parameters
>> > + * @async:	true if this is an asynchronous commit
>> >   */
>> >  void dpu_encoder_prepare_for_kickoff(struct drm_encoder *encoder,
>> > -		struct dpu_encoder_kickoff_params *params);
>> > +		struct dpu_encoder_kickoff_params *params, bool async);
>> >
>> >  /**
>> >   * dpu_encoder_trigger_kickoff_pending - Clear the flush bits from
>> > previous
>> > @@ -96,8 +97,9 @@ void dpu_encoder_trigger_kickoff_pending(struct
>> > drm_encoder *encoder);
>> >   * dpu_encoder_kickoff - trigger a double buffer flip of the ctl path
>> >   *	(i.e. ctl flush and start) immediately.
>> >   * @encoder:	encoder pointer
>> > + * @async:	true if this is an asynchronous commit
>> >   */
>> > -void dpu_encoder_kickoff(struct drm_encoder *encoder);
>> > +void dpu_encoder_kickoff(struct drm_encoder *encoder, bool async);
>> >
>> >  /**
>> >   * dpu_encoder_wait_for_event - Waits for encoder events
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> > index 0a683e65a9f3..1cba21edd862 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> > @@ -352,7 +352,7 @@ void dpu_kms_encoder_enable(struct drm_encoder
>> > *encoder)
>> >
>> >  	if (crtc && crtc->state->active) {
>> >  		trace_dpu_kms_enc_enable(DRMID(crtc));
>> > -		dpu_crtc_commit_kickoff(crtc);
>> > +		dpu_crtc_commit_kickoff(crtc, false);
>> >  	}
>> >  }
>> >
>> > @@ -369,7 +369,8 @@ static void dpu_kms_commit(struct msm_kms *kms,
>> > struct
>> > drm_atomic_state *state)
>> >
>> >  		if (crtc->state->active) {
>> >  			trace_dpu_kms_commit(DRMID(crtc));
>> > -			dpu_crtc_commit_kickoff(crtc);
>> > +			dpu_crtc_commit_kickoff(crtc,
>> > +
>> > state->legacy_cursor_update);
>> >  		}
>> >  	}
>> >  }
>> > diff --git a/drivers/gpu/drm/msm/msm_atomic.c
>> > b/drivers/gpu/drm/msm/msm_atomic.c
>> > index c1f1779c980f..7912130ce5ce 100644
>> > --- a/drivers/gpu/drm/msm/msm_atomic.c
>> > +++ b/drivers/gpu/drm/msm/msm_atomic.c
>> > @@ -76,7 +76,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state
>> > *state)
>> >  		kms->funcs->commit(kms, state);
>> >  	}
>> >
>> > -	msm_atomic_wait_for_commit_done(dev, state);
>> > +	if (!state->legacy_cursor_update)
>> I see state->async_update is updated after validation checks on the
> async
>> capabilities. Shouldn't we use
>> that var instead of state->legacy_cursor_update?
>> > +		msm_atomic_wait_for_commit_done(dev, state);
>> >
>> >  	kms->funcs->complete_commit(kms, state);
>> 
>> Do we need to introduce plane helpers atomic_async_update and
>> atomic_async_check in DPU before supporting
>> these wait skips? or are they irrelevant for this legacy async path?
> 
> I was trying to limit the scope of this to just cursor updates. I think
> once/if
> support is added for generic async it makes sense to change over to 
> that
> verbage.
> 
Since SDM845 doesnt support dedicated CURSOR stages, I think the right
way to add the cursor support should be to introduce the atomic async 
support
in the driver and let the cursor frame update like regulary async 
commit.

I need to explore on the right way to fit that in.

Thanks,
Jeykumar S.
> Sean
> 
>> 
>> --
>> Jeykumar S

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

Re: [PATCH 2/2] drm/msm: dpu: Make legacy cursor updates asynchronous

[Sean Paul]

On Tue, Oct 02, 2018 at 06:19:52PM -0700, Jeykumar Sankaran wrote:
> On 2018-10-01 13:30, Sean Paul wrote:
> > On Wed, Sep 26, 2018 at 11:56:47AM -0700, Jeykumar Sankaran wrote:
> > > On 2018-09-19 11:56, Sean Paul wrote:
> > > > From: Sean Paul <seanpaul@chromium.org>
> > > >
> > > > This patch sprinkles a few async/legacy_cursor_update checks
> > > > through commit to ensure that cursor updates aren't blocked on vsync.
> > > > There are 2 main components to this, the first is that we don't want
> > to
> > > > wait_for_commit_done in msm_atomic  before returning from
> > > > atomic_complete.
> > > > The second is that in dpu we don't want to wait for frame_done events
> > > > when
> > > > updating the cursor.
> > > >
> > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > ---

/snip

> > > > diff --git a/drivers/gpu/drm/msm/msm_atomic.c
> > > > b/drivers/gpu/drm/msm/msm_atomic.c
> > > > index c1f1779c980f..7912130ce5ce 100644
> > > > --- a/drivers/gpu/drm/msm/msm_atomic.c
> > > > +++ b/drivers/gpu/drm/msm/msm_atomic.c
> > > > @@ -76,7 +76,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state
> > > > *state)
> > > >  		kms->funcs->commit(kms, state);
> > > >  	}
> > > >
> > > > -	msm_atomic_wait_for_commit_done(dev, state);
> > > > +	if (!state->legacy_cursor_update)
> > > I see state->async_update is updated after validation checks on the
> > async
> > > capabilities. Shouldn't we use
> > > that var instead of state->legacy_cursor_update?
> > > > +		msm_atomic_wait_for_commit_done(dev, state);
> > > >
> > > >  	kms->funcs->complete_commit(kms, state);
> > > 
> > > Do we need to introduce plane helpers atomic_async_update and
> > > atomic_async_check in DPU before supporting
> > > these wait skips? or are they irrelevant for this legacy async path?
> > 
> > I was trying to limit the scope of this to just cursor updates. I think
> > once/if
> > support is added for generic async it makes sense to change over to that
> > verbage.
> > 
> Since SDM845 doesnt support dedicated CURSOR stages, 

Well, it has CURSOR type planes, which unlocks the cursor ioctls, so it kind of
does :)

> I think the right
> way to add the cursor support should be to introduce the atomic async
> support
> in the driver and let the cursor frame update like regulary async commit.
> 
> I need to explore on the right way to fit that in.

It's as easy as implementing the async atomic hooks. If they're present, the
cursor path goes through them. Upgrading from legacy cursor to generic async is
pretty trivial in light of the other issues we're facing here.

Sean

> 
> Thanks,
> Jeykumar S.
> > Sean
> > 
> > > 
> > > --
> > > Jeykumar S
> 
> -- 
> Jeykumar S

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