linux-kernel - Re: [PATCH 2/3] drm/atomic: Use kthread worker for nonblocking commits

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200922131813.GL438822@phenom.ffwll.local>
Date:   Tue, 22 Sep 2020 15:18:13 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Rob Clark <robdclark@...il.com>
Cc:     dri-devel <dri-devel@...ts.freedesktop.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Tejun Heo <tj@...nel.org>, timmurray@...gle.com,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Rob Clark <robdclark@...omium.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...ux.ie>,
        open list <linux-kernel@...r.kernel.org>,
        Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH 2/3] drm/atomic: Use kthread worker for nonblocking
 commits

On Mon, Sep 21, 2020 at 07:55:42AM -0700, Rob Clark wrote:
> On Mon, Sep 21, 2020 at 2:23 AM Daniel Vetter <daniel@...ll.ch> wrote:
> >
> > On Sat, Sep 19, 2020 at 12:37:25PM -0700, Rob Clark wrote:
> > > From: Rob Clark <robdclark@...omium.org>
> > >
> > > This will allow us to more easily switch scheduling rules based on what
> > > userspace wants.
> > >
> > > Signed-off-by: Rob Clark <robdclark@...omium.org>
> >
> > I still think switching to the highpriority systemwq as a start (like i915
> > already does) would be a good first step no matter what we end up doing
> > for the android thing.
> 
> highpri wq is probably better than the current state, but it doesn't
> really address the problem.  You'll still end up with surfaceflinger
> preempting commit_work..
> 
> And with non-RT priority, you'll still occasionally get lower priority
> threads which haven't had a chance to run for a while preempting you.

Sure the priority inversion is still there and needs a different fix. But
maybe it'll make everyone else at least a bit happier.

Plus it's really hard to make kms drivers rt, it's not really been part of
the design (nor are gpus really rt friendly, if they even can preempt it
generally takes forever compared to the deadline you might want for some
present work).
-Daniel
> 
> BR,
> -R
> 
> 
> > -Daniel
> >
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++++----
> > >  include/drm/drm_atomic.h            | 31 +++++++++++++++++++++++++++++
> > >  2 files changed, 40 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 9e1ad493e689..75eeec5e7b10 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -1659,11 +1659,11 @@ static void commit_tail(struct drm_atomic_state *old_state)
> > >       drm_atomic_state_put(old_state);
> > >  }
> > >
> > > -static void commit_work(struct work_struct *work)
> > > +static void commit_work(struct kthread_work *work)
> > >  {
> > >       struct drm_atomic_state *state = container_of(work,
> > >                                                     struct drm_atomic_state,
> > > -                                                   commit_work);
> > > +                                                   commit_kwork);
> > >       commit_tail(state);
> > >  }
> > >
> > > @@ -1797,6 +1797,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
> > >                            struct drm_atomic_state *state,
> > >                            bool nonblock)
> > >  {
> > > +     struct kthread_worker *worker = NULL;
> > >       int ret;
> > >
> > >       if (state->async_update) {
> > > @@ -1814,7 +1815,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
> > >       if (ret)
> > >               return ret;
> > >
> > > -     INIT_WORK(&state->commit_work, commit_work);
> > > +     kthread_init_work(&state->commit_kwork, commit_work);
> > >
> > >       ret = drm_atomic_helper_prepare_planes(dev, state);
> > >       if (ret)
> > > @@ -1857,8 +1858,12 @@ int drm_atomic_helper_commit(struct drm_device *dev,
> > >        */
> > >
> > >       drm_atomic_state_get(state);
> > > +
> > >       if (nonblock)
> > > -             queue_work(system_unbound_wq, &state->commit_work);
> > > +             worker = drm_atomic_pick_worker(state);
> > > +
> > > +     if (worker)
> > > +             kthread_queue_work(worker, &state->commit_kwork);
> > >       else
> > >               commit_tail(state);
> > >
> > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > > index d07c851d255b..8d0ee19953df 100644
> > > --- a/include/drm/drm_atomic.h
> > > +++ b/include/drm/drm_atomic.h
> > > @@ -373,8 +373,18 @@ struct drm_atomic_state {
> > >        *
> > >        * Work item which can be used by the driver or helpers to execute the
> > >        * commit without blocking.
> > > +      *
> > > +      * This is deprecated, use commit_kwork.
> > >        */
> > >       struct work_struct commit_work;
> > > +
> > > +     /**
> > > +      * @commit_kwork:
> > > +      *
> > > +      * Work item which can be used by the driver or helpers to execute the
> > > +      * commit without blocking.
> > > +      */
> > > +     struct kthread_work commit_kwork;
> > >  };
> > >
> > >  void __drm_crtc_commit_free(struct kref *kref);
> > > @@ -954,6 +964,27 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p);
> > >                     (new_obj_state) = (__state)->private_objs[__i].new_state, 1); \
> > >            (__i)++)
> > >
> > > +/**
> > > + * drm_atomic_pick_worker - helper to get kworker to use for nonblocking commit
> > > + * @state: the &drm_atomic_state for the commit
> > > + *
> > > + * Pick an appropriate worker for a given atomic update.  The first CRTC
> > > + * invovled in the atomic update is used to pick the worker, to prevent
> > > + * serializing multiple pageflips / atomic-updates on indenpendent CRTCs.
> > > + */
> > > +static inline struct kthread_worker *
> > > +drm_atomic_pick_worker(const struct drm_atomic_state *state)
> > > +{
> > > +     struct drm_crtc_state *crtc_state;
> > > +     struct drm_crtc *crtc;
> > > +     unsigned i;
> > > +
> > > +     for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> > > +             return crtc->worker;
> > > +
> > > +     return NULL;
> > > +}
> > > +
> > >  /**
> > >   * drm_atomic_crtc_needs_modeset - compute combined modeset need
> > >   * @state: &drm_crtc_state for the CRTC
> > > --
> > > 2.26.2
> > >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch