Re: [RFC PATCH] drm: split up drm_ioctl to allow drivers to hook into "core" functions

[Daniel Vetter <daniel@ffwll.ch>]

On Sun, Dec 31, 2017 at 01:40:46PM -0500, Ilia Mirkin wrote:
> On Sun, Dec 31, 2017 at 1:15 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> > Currently there's no way to allow a driver to reimplement any ioctls
> > from the drm core. This can be desirable to, e.g., override fixed format
> > selection logic, without turning to a midlayer-like solution.
> >
> > Signed-off-by: Ilia Mirkin <imirkin@alum.mit.edu>
> > ---
> >
> > I want drm_mode_addfb to pick a different format for depth=30 than the
> > one it currently selects. Flipping it for all drivers would break a
> > bunch of existing ones, so this enables a driver to take control.
> >
> > Alternatively I can stash something into drm_device which specifies the
> > preferred depth=30 fb format. However from my cursory observations of
> > dri-devel discussions, midlayering is seen as a problem and not a
> > solution.
> 
> Ugh, of course this is turning into a disaster as well.
> drm_mode_addfb2 isn't exported, so I have to copy all the
> functionality into nouveau, or move it to drm_kms_helper or whatever.
> The flag in drm_device approach is sounding a lot more palatable. Let
> me know what the best way to proceed is.
> 
> This is all to fix a regression, btw, since previous to nouveau
> becoming atomic this all worked fine -- it just looked at the
> 30bpp'ness of the fb and assumed. Now it actually cares about specific
> formats, and we run into all these problems.

Can you just submit the hack to the core addfb code so we know what
exactly goes wrong? Overwriting core ioctls is imo not cool at all :-)
-Daniel


> 
> >
> >  drivers/gpu/drm/drm_ioctl.c | 36 ++++++++++++++++++++++++------------
> >  include/drm/drm_ioctl.h     |  2 ++
> >  2 files changed, 26 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index 4aafe4802099..698d69c6db0a 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -767,12 +767,7 @@ long drm_ioctl(struct file *filp,
> >         struct drm_file *file_priv = filp->private_data;
> >         struct drm_device *dev;
> >         const struct drm_ioctl_desc *ioctl = NULL;
> > -       drm_ioctl_t *func;
> >         unsigned int nr = DRM_IOCTL_NR(cmd);
> > -       int retcode = -EINVAL;
> > -       char stack_kdata[128];
> > -       char *kdata = NULL;
> > -       unsigned int in_size, out_size, drv_size, ksize;
> >         bool is_driver_ioctl;
> >
> >         dev = file_priv->minor->dev;
> > @@ -784,16 +779,33 @@ long drm_ioctl(struct file *filp,
> >
> >         if (is_driver_ioctl) {
> >                 /* driver ioctl */
> > -               if (nr - DRM_COMMAND_BASE >= dev->driver->num_ioctls)
> > -                       goto err_i1;
> > -               ioctl = &dev->driver->ioctls[nr - DRM_COMMAND_BASE];
> > +               if (nr - DRM_COMMAND_BASE < dev->driver->num_ioctls)
> > +                       ioctl = &dev->driver->ioctls[nr - DRM_COMMAND_BASE];
> >         } else {
> >                 /* core ioctl */
> > -               if (nr >= DRM_CORE_IOCTL_COUNT)
> > -                       goto err_i1;
> > -               ioctl = &drm_ioctls[nr];
> > +               if (nr < DRM_CORE_IOCTL_COUNT)
> > +                       ioctl = &drm_ioctls[nr];
> >         }
> >
> > +       return __drm_ioctl(filp, cmd, arg, ioctl);
> > +}
> > +EXPORT_SYMBOL(drm_ioctl);
> > +
> > +long __drm_ioctl(struct file *filp,
> > +                unsigned int cmd, unsigned long arg,
> > +                const struct drm_ioctl_desc *ioctl)
> > +{
> > +       struct drm_file *file_priv = filp->private_data;
> > +       drm_ioctl_t *func;
> > +       unsigned int nr = DRM_IOCTL_NR(cmd);
> > +       int retcode = -EINVAL;
> > +       char stack_kdata[128];
> > +       char *kdata = NULL;
> > +       unsigned int in_size, out_size, drv_size, ksize;
> > +
> > +       if (!ioctl)
> > +               goto err_i1;
> > +
> >         drv_size = _IOC_SIZE(ioctl->cmd);
> >         out_size = in_size = _IOC_SIZE(cmd);
> >         if ((cmd & ioctl->cmd & IOC_IN) == 0)
> > @@ -851,7 +863,7 @@ long drm_ioctl(struct file *filp,
> >                 DRM_DEBUG("ret = %d\n", retcode);
> >         return retcode;
> >  }
> > -EXPORT_SYMBOL(drm_ioctl);
> > +EXPORT_SYMBOL(__drm_ioctl);
> >
> >  /**
> >   * drm_ioctl_flags - Check for core ioctl and return ioctl permission flags
> > diff --git a/include/drm/drm_ioctl.h b/include/drm/drm_ioctl.h
> > index add42809642a..e08f8ea66f2a 100644
> > --- a/include/drm/drm_ioctl.h
> > +++ b/include/drm/drm_ioctl.h
> > @@ -172,6 +172,8 @@ struct drm_ioctl_desc {
> >
> >  int drm_ioctl_permit(u32 flags, struct drm_file *file_priv);
> >  long drm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
> > +long __drm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg,
> > +                const struct drm_ioctl_desc *ioctl);
> >  long drm_ioctl_kernel(struct file *, drm_ioctl_t, void *, u32);
> >  #ifdef CONFIG_COMPAT
> >  long drm_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
> > --
> > 2.13.6
> >
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel