Re: [PATCH 3/4] drm/msm: Add SYSPROF param

[Rob Clark]

On Thu, Mar 3, 2022 at 12:47 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Quoting Rob Clark (2022-03-03 11:46:47)
> > diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> > index fde9a29f884e..0ba1dbd4e50f 100644
> > --- a/drivers/gpu/drm/msm/msm_gpu.h
> > +++ b/drivers/gpu/drm/msm/msm_gpu.h
> > @@ -330,6 +337,24 @@ struct msm_file_private {
> >         struct kref ref;
> >         int seqno;
> >
> > +       /**
> > +        * sysprof:
> > +        *
> > +        * The value of MSM_PARAM_SYSPROF set by userspace.  This is
> > +        * intended to be used by system profiling tools like Mesa's
> > +        * pps-producer (perfetto), and restricted to CAP_SYS_ADMIN.
> > +        *
> > +        * Setting a value of 1 will preserve performance counters across
> > +        * context switches.  Setting a value of 2 will in addition
> > +        * suppress suspend.  (Performance counters loose  state across
>
> s/loose  /lose/

fixed locally

> > +        * power collapse, which is undesirable for profiling in some
> > +        * cases.)
> > +        *
> > +        * The value automatically reverts to zero when the drm device
> > +        * file is closed.
> > +        */
> > +       int sysprof;
> > +
> >         /**
> >          * elapsed:
> >          *
> > diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> > index 7cb158bcbcf6..4179db54ac93 100644
> > --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> > +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
> > @@ -7,6 +7,40 @@
> >
> >  #include "msm_gpu.h"
> >
> > +int msm_file_private_set_sysprof(struct msm_file_private *ctx,
> > +                                struct msm_gpu *gpu, int sysprof)
> > +{
> > +       /* unwind old value first: */
> > +       switch (ctx->sysprof) {
> > +       case 2:
> > +               pm_runtime_put_autosuspend(&gpu->pdev->dev);
> > +               fallthrough;
> > +       case 1:
> > +               refcount_dec(&gpu->sysprof_active);
> > +               fallthrough;
> > +       case 0:
> > +               break;
> > +       }
> > +
> > +       /* then apply new value: */
>
> It would be safer to swap this. Otherwise a set when the values are at
> "1" would drop to "zero" here and potentially trigger some glitch,
> whereas incrementing one more time and then dropping the previous state
> would avoid that short blip.
>
> > +       switch (sysprof) {
> > +       default:
> > +               return -EINVAL;
>
> This will become more complicated though.

Right, that is why I took the "unwind first and then re-apply"
approach.. in practice I expect userspace to set the value before it
starts sampling counter values, so I wasn't too concerned about this
racing with a submit and clearing the counters.  (Plus any glitch if
userspace did decide to change it dynamically would just be transient
and not really a big deal.)

BR,
-R

> > +       case 2:
> > +               pm_runtime_get_sync(&gpu->pdev->dev);
> > +               fallthrough;
> > +       case 1:
> > +               refcount_inc(&gpu->sysprof_active);
> > +               fallthrough;
> > +       case 0:
> > +               break;
> > +       }
> > +
> > +       ctx->sysprof = sysprof;
> > +
> > +       return 0;
> > +}