From: Jeykumar Sankaran <jsanka-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
To: Sean Paul <sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org>
Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
abhinavk-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org,
Rob Clark <robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>,
hoegsberg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org,
freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH 2/2] drm/msm: dpu: Make legacy cursor updates asynchronous
Date: Tue, 02 Oct 2018 18:19:52 -0700 [thread overview]
Message-ID: <ca688d3db87e0665b0de64717c1917c6@codeaurora.org> (raw)
In-Reply-To: <20181001203056.GA72545@art_vandelay>
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, ¶ms);
>> > + dpu_encoder_prepare_for_kickoff(encoder, ¶ms, 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
next prev parent reply other threads:[~2018-10-03 1:19 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-19 18:56 [PATCH 0/2] drm/msm: dpu: Fix cursor updates Sean Paul
[not found] ` <20180919185627.206368-1-sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org>
2018-09-19 18:56 ` [PATCH 1/2] drm/msm: dpu: Only check flush register against pending flushes Sean Paul
[not found] ` <20180919185627.206368-2-sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org>
2018-09-26 18:51 ` Jeykumar Sankaran
[not found] ` <709b39ac985c0687e248710c75d16599-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2018-10-01 20:29 ` Sean Paul
2018-10-03 1:14 ` Jeykumar Sankaran
[not found] ` <7ee73b3551e82df746d478a6ac02e8be-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2018-10-03 15:45 ` Sean Paul
2018-09-19 18:56 ` [PATCH 2/2] drm/msm: dpu: Make legacy cursor updates asynchronous Sean Paul
[not found] ` <20180919185627.206368-3-sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org>
2018-09-26 18:56 ` Jeykumar Sankaran
[not found] ` <ac73bcb9bd81301b2a58a6eef29b3dc2-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2018-10-01 20:30 ` Sean Paul
2018-10-03 1:19 ` Jeykumar Sankaran [this message]
[not found] ` <ca688d3db87e0665b0de64717c1917c6-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2018-10-03 14:33 ` Sean Paul
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ca688d3db87e0665b0de64717c1917c6@codeaurora.org \
--to=jsanka-sgv2jx0feol9jmxxk+q4oq@public.gmane.org \
--cc=abhinavk-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org \
--cc=freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \
--cc=hoegsberg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org \
--cc=linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=robdclark-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
--cc=sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org \
--cc=seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.