From: Qais Yousef <qais.yousef@arm.com> To: Rob Clark <robdclark@gmail.com> Cc: dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org, Tejun Heo <tj@kernel.org>, timmurray@google.com, Daniel Vetter <daniel@ffwll.ch>, Rob Clark <robdclark@chromium.org>, open list <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v2 0/3] drm: commit_work scheduling Date: Fri, 2 Oct 2020 12:01:06 +0100 [thread overview] Message-ID: <20201002110105.e56qrvzoqfioi4hs@e107158-lin.cambridge.arm.com> (raw) In-Reply-To: <20200930211723.3028059-1-robdclark@gmail.com> On 09/30/20 14:17, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > The android userspace treats the display pipeline as a realtime problem. > And arguably, if your goal is to not miss frame deadlines (ie. vblank), > it is. (See https://lwn.net/Articles/809545/ for the best explaination > that I found.) > > But this presents a problem with using workqueues for non-blocking > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can > preempt the worker. Which is not really the outcome you want.. once > the required fences are scheduled, you want to push the atomic commit > down to hw ASAP. For me thees 2 properties 1. Run ASAP 2. Finish the work un-interrupted Scream the workers need to be SCHED_FIFO by default. CFS can't give you these guarantees. IMO using sched_set_fifo() for these workers is the right thing. > > But the decision of whether commit_work should be RT or not really > depends on what userspace is doing. For a pure CFS userspace display > pipeline, commit_work() should remain SCHED_NORMAL. I'm not sure I agree with this. I think it's better to characterize tasks based on their properties/requirements rather than what the rest of the userspace is using. I do appreciate that maybe some of these tasks have varying requirements during their life time. e.g: they have RT property during specific critical section but otherwise are CFS tasks. I think the UI thread in Android behaves like that. It's worth IMO trying that approach I pointed out earlier to see if making RT try to pick an idle CPU rather than preempt CFS helps. Not sure if it'd be accepted but IMHO it's a better direction to consider and discuss. Or maybe you can wrap userspace pipeline critical section lock such that any task holding it will automatically be promoted to SCHED_FIFO and then demoted to CFS once it releases it. Haven't worked with display pipelines before, so hopefully this makes sense :-) Thanks -- Qais Yousef > > To handle this, convert non-blocking commit_work() to use per-CRTC > kthread workers, instead of system_unbound_wq. Per-CRTC workers are > used to avoid serializing commits when userspace is using a per-CRTC > update loop. And the last patch exposes the task id to userspace as > a CRTC property, so that userspace can adjust the priority and sched > policy to fit it's needs. > > > v2: Drop client cap and in-kernel setting of priority/policy in > favor of exposing the kworker tid to userspace so that user- > space can set priority/policy. > > Rob Clark (3): > drm/crtc: Introduce per-crtc kworker > drm/atomic: Use kthread worker for nonblocking commits > drm: Expose CRTC's kworker task id > > drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++++---- > drivers/gpu/drm/drm_crtc.c | 14 +++++++++++++ > drivers/gpu/drm/drm_mode_config.c | 14 +++++++++++++ > drivers/gpu/drm/drm_mode_object.c | 4 ++++ > include/drm/drm_atomic.h | 31 +++++++++++++++++++++++++++++ > include/drm/drm_crtc.h | 8 ++++++++ > include/drm/drm_mode_config.h | 9 +++++++++ > include/drm/drm_property.h | 9 +++++++++ > 8 files changed, 98 insertions(+), 4 deletions(-) > > -- > 2.26.2 >
WARNING: multiple messages have this Message-ID (diff)
From: Qais Yousef <qais.yousef@arm.com> To: Rob Clark <robdclark@gmail.com> Cc: Rob Clark <robdclark@chromium.org>, linux-arm-msm@vger.kernel.org, open list <linux-kernel@vger.kernel.org>, timmurray@google.com, dri-devel@lists.freedesktop.org, Tejun Heo <tj@kernel.org> Subject: Re: [PATCH v2 0/3] drm: commit_work scheduling Date: Fri, 2 Oct 2020 12:01:06 +0100 [thread overview] Message-ID: <20201002110105.e56qrvzoqfioi4hs@e107158-lin.cambridge.arm.com> (raw) In-Reply-To: <20200930211723.3028059-1-robdclark@gmail.com> On 09/30/20 14:17, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > The android userspace treats the display pipeline as a realtime problem. > And arguably, if your goal is to not miss frame deadlines (ie. vblank), > it is. (See https://lwn.net/Articles/809545/ for the best explaination > that I found.) > > But this presents a problem with using workqueues for non-blocking > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can > preempt the worker. Which is not really the outcome you want.. once > the required fences are scheduled, you want to push the atomic commit > down to hw ASAP. For me thees 2 properties 1. Run ASAP 2. Finish the work un-interrupted Scream the workers need to be SCHED_FIFO by default. CFS can't give you these guarantees. IMO using sched_set_fifo() for these workers is the right thing. > > But the decision of whether commit_work should be RT or not really > depends on what userspace is doing. For a pure CFS userspace display > pipeline, commit_work() should remain SCHED_NORMAL. I'm not sure I agree with this. I think it's better to characterize tasks based on their properties/requirements rather than what the rest of the userspace is using. I do appreciate that maybe some of these tasks have varying requirements during their life time. e.g: they have RT property during specific critical section but otherwise are CFS tasks. I think the UI thread in Android behaves like that. It's worth IMO trying that approach I pointed out earlier to see if making RT try to pick an idle CPU rather than preempt CFS helps. Not sure if it'd be accepted but IMHO it's a better direction to consider and discuss. Or maybe you can wrap userspace pipeline critical section lock such that any task holding it will automatically be promoted to SCHED_FIFO and then demoted to CFS once it releases it. Haven't worked with display pipelines before, so hopefully this makes sense :-) Thanks -- Qais Yousef > > To handle this, convert non-blocking commit_work() to use per-CRTC > kthread workers, instead of system_unbound_wq. Per-CRTC workers are > used to avoid serializing commits when userspace is using a per-CRTC > update loop. And the last patch exposes the task id to userspace as > a CRTC property, so that userspace can adjust the priority and sched > policy to fit it's needs. > > > v2: Drop client cap and in-kernel setting of priority/policy in > favor of exposing the kworker tid to userspace so that user- > space can set priority/policy. > > Rob Clark (3): > drm/crtc: Introduce per-crtc kworker > drm/atomic: Use kthread worker for nonblocking commits > drm: Expose CRTC's kworker task id > > drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++++---- > drivers/gpu/drm/drm_crtc.c | 14 +++++++++++++ > drivers/gpu/drm/drm_mode_config.c | 14 +++++++++++++ > drivers/gpu/drm/drm_mode_object.c | 4 ++++ > include/drm/drm_atomic.h | 31 +++++++++++++++++++++++++++++ > include/drm/drm_crtc.h | 8 ++++++++ > include/drm/drm_mode_config.h | 9 +++++++++ > include/drm/drm_property.h | 9 +++++++++ > 8 files changed, 98 insertions(+), 4 deletions(-) > > -- > 2.26.2 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2020-10-02 11:01 UTC|newest] Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-30 21:17 [PATCH v2 0/3] drm: commit_work scheduling Rob Clark 2020-09-30 21:17 ` Rob Clark 2020-09-30 21:17 ` [PATCH v2 1/3] drm/crtc: Introduce per-crtc kworker Rob Clark 2020-09-30 21:17 ` Rob Clark 2020-09-30 21:17 ` [PATCH v2 2/3] drm/atomic: Use kthread worker for nonblocking commits Rob Clark 2020-09-30 21:17 ` Rob Clark 2020-09-30 21:17 ` [PATCH v2 3/3] drm: Expose CRTC's kworker task id Rob Clark 2020-09-30 21:17 ` Rob Clark 2020-10-01 7:25 ` [PATCH v2 0/3] drm: commit_work scheduling Daniel Vetter 2020-10-01 7:25 ` Daniel Vetter 2020-10-01 15:15 ` Rob Clark 2020-10-01 15:15 ` Rob Clark 2020-10-01 15:25 ` Daniel Vetter 2020-10-01 15:25 ` Daniel Vetter 2020-10-02 10:52 ` Ville Syrjälä 2020-10-02 10:52 ` Ville Syrjälä 2020-10-02 11:05 ` Ville Syrjälä 2020-10-02 11:05 ` Ville Syrjälä 2020-10-02 17:55 ` Rob Clark 2020-10-02 17:55 ` Rob Clark 2020-10-05 12:15 ` Ville Syrjälä 2020-10-05 12:15 ` Ville Syrjälä 2020-10-05 14:15 ` Daniel Vetter 2020-10-05 14:15 ` Daniel Vetter 2020-10-05 22:58 ` Rob Clark 2020-10-05 22:58 ` Rob Clark 2020-10-07 16:44 ` Rob Clark 2020-10-07 16:44 ` Rob Clark 2020-10-08 8:24 ` Ville Syrjälä 2020-10-08 8:24 ` Ville Syrjälä 2020-10-16 16:27 ` Rob Clark 2020-10-16 16:27 ` Rob Clark 2020-10-02 11:01 ` Qais Yousef [this message] 2020-10-02 11:01 ` Qais Yousef 2020-10-02 18:07 ` Rob Clark 2020-10-02 18:07 ` Rob Clark 2020-10-05 15:00 ` Qais Yousef 2020-10-05 15:00 ` Qais Yousef 2020-10-05 23:24 ` Rob Clark 2020-10-05 23:24 ` Rob Clark 2020-10-06 9:08 ` Daniel Vetter 2020-10-06 9:08 ` Daniel Vetter 2020-10-06 10:01 ` Peter Zijlstra 2020-10-06 10:01 ` Peter Zijlstra 2020-10-06 10:59 ` Qais Yousef 2020-10-06 10:59 ` Qais Yousef 2020-10-06 20:04 ` Rob Clark 2020-10-06 20:04 ` Rob Clark 2020-10-07 10:36 ` Qais Yousef 2020-10-07 10:36 ` Qais Yousef 2020-10-07 15:57 ` Rob Clark 2020-10-07 15:57 ` Rob Clark 2020-10-07 16:30 ` Qais Yousef 2020-10-07 16:30 ` Qais Yousef 2020-10-08 9:10 ` Daniel Vetter 2020-10-08 9:10 ` Daniel Vetter
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=20201002110105.e56qrvzoqfioi4hs@e107158-lin.cambridge.arm.com \ --to=qais.yousef@arm.com \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=robdclark@chromium.org \ --cc=robdclark@gmail.com \ --cc=timmurray@google.com \ --cc=tj@kernel.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: linkBe 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.