On Tue, Jun 25, 2024 at 09:07:12AM +0200, Pierre-Eric Pelloux-Prayer wrote:
>
> Le 20/06/2024 à 15:36, Christian König a écrit :
> > Am 20.06.24 um 15:06 schrieb Pierre-Eric Pelloux-Prayer:
> > > Le 19/06/2024 à 11:26, Christian König a écrit :
> > > > Am 18.06.24 um 17:23 schrieb Pierre-Eric Pelloux-Prayer:
> > > > > Waking up a device can take multiple seconds, so if it's not
> > > > > going to be used we might as well not resume it.
> > > > >
> > > > > The safest default behavior for all ioctls is to resume the GPU,
> > > > > so this change allows specific ioctls to opt-out of generic
> > > > > runtime pm.
> > > >
> > > > I'm really wondering if we shouldn't put that into the IOCTL
> > > > description.
> > > >
> > > > See amdgpu_ioctls_kms and DRM_IOCTL_DEF_DRV() for what I mean.
> > >
> > > Are you suggesting to add a new entry in enum drm_ioctl_flags to
> > > indicate ioctls which need the device to be awake?
> > >
> > > Something like: "DRM_NO_DEVICE = BIT(6)" and then use it for both
> > > core and amdgpu ioctls?
> >
> > Yeah something like that. Maybe name that DRM_SW_ONLY or something like
> > that.
>
> + dri-devel to gauge interest in adding such a flag in shared code.
Eh please no. That's just fundamentally the wrong way round to do runtime
pm, but amdgpu goes way, way back in desing in that record to the vga
switcheroo, where the simple hack was to just add runtime pm handling at
the entry points and call it done.
If you look at any other drm driver, the runtime pm handling is done way
down when touching the actual relevant hardware blocks. Because especially
on arm it's actually a pile of runtime pm domains.
So essentially this is like pushing a big driver lock down the callchain
until it's actually at the right level. First step would be to push it
into each of the amdgpu ioctl callbacks, so that you can drop
amdgpu_drm_ioctl and use drm_ioctl again directly. And then push it
further until you can remove it from all the places where it's not needed.
DRM_SW_ONLY otoh is pure midlayer mistake design.
Cheers, Sima
>
> Pierre-Eric
>
>
>
> >
> > Christian.
> >
> > >
> > > Pierre-Eric
> > >
> > >
> > >
> > >
> > > >
> > > > Regards,
> > > > Christian.
> > > >
> > > > >
> > > > > Signed-off-by: Pierre-Eric Pelloux-Prayer
> > > > > <pierre-eric.pelloux-prayer@xxxxxxx>
> > > > > ---
> > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 25
> > > > > ++++++++++++++++++++-----
> > > > > 1 file changed, 20 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > > index 60d5758939ae..a9831b243bfc 100644
> > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> > > > > @@ -2855,18 +2855,33 @@ long amdgpu_drm_ioctl(struct file *filp,
> > > > > {
> > > > > struct drm_file *file_priv = filp->private_data;
> > > > > struct drm_device *dev;
> > > > > + bool needs_device;
> > > > > long ret;
> > > > > dev = file_priv->minor->dev;
> > > > > - ret = pm_runtime_get_sync(dev->dev);
> > > > > - if (ret < 0)
> > > > > - goto out;
> > > > > +
> > > > > + /* Some ioctl can opt-out of powermanagement handling
> > > > > + * if they don't require the device to be resumed.
> > > > > + */
> > > > > + switch (cmd) {
> > > > > + default:
> > > > > + needs_device = true;
> > > > > + }
> > > > > +
> > > > > + if (needs_device) {
> > > > > + ret = pm_runtime_get_sync(dev->dev);
> > > > > + if (ret < 0)
> > > > > + goto out;
> > > > > + }
> > > > > ret = drm_ioctl(filp, cmd, arg);
> > > > > - pm_runtime_mark_last_busy(dev->dev);
> > > > > out:
> > > > > - pm_runtime_put_autosuspend(dev->dev);
> > > > > + if (needs_device) {
> > > > > + pm_runtime_mark_last_busy(dev->dev);
> > > > > + pm_runtime_put_autosuspend(dev->dev);
> > > > > + }
> > > > > +
> > > > > return ret;
> > > > > }
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
