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;
> > +}
