[PATCH 2/6] drm: Introduce DRM_MODE_DUMB_KERNEL_MAP flag

[Rob Herring]

On Tue, Oct 22, 2019 at 6:14 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Rob,
>
> Thank you for the patch.
>
> On Mon, Oct 21, 2019 at 04:45:46PM -0500, Rob Herring wrote:
> > Introduce a new flag, DRM_MODE_DUMB_KERNEL_MAP, for struct
> > drm_mode_create_dumb. This flag is for internal kernel use to indicate
> > if dumb buffer allocation needs a kernel mapping. This is needed only for
> > CMA where creating a kernel mapping or not has to be decided at allocation
> > time because creating a mapping on demand (with vmap()) is not guaranteed
> > to work. Several drivers are using CMA, but not the CMA helpers because
> > they distinguish between kernel and userspace allocations to create a
> > kernel mapping or not.
> >
> > Update the callers of drm_mode_dumb_create() to set
> > drm_mode_dumb_create.flags to appropriate defaults. Currently, flags can
> > be set to anything by userspace, but is unused within the kernel. Let's
> > force flags to zero (no kernel mapping) for userspace callers by default.
> > For in kernel clients, set DRM_MODE_DUMB_KERNEL_MAP by default. Drivers
> > can override this as needed.
> >
> > Cc: David Airlie <airlied at linux.ie>
> > Cc: Daniel Vetter <daniel at ffwll.ch>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Cc: Maxime Ripard <mripard at kernel.org>
> > Cc: Sean Paul <sean at poorly.run>
> > Signed-off-by: Rob Herring <robh at kernel.org>
> > ---
> >  drivers/gpu/drm/drm_client.c       | 1 +
> >  drivers/gpu/drm/drm_dumb_buffers.c | 5 ++++-
> >  include/uapi/drm/drm_mode.h        | 2 ++
> >  3 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> > index d9a2e3695525..dbfc8061b392 100644
> > --- a/drivers/gpu/drm/drm_client.c
> > +++ b/drivers/gpu/drm/drm_client.c
> > @@ -264,6 +264,7 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u
> >       dumb_args.width = width;
> >       dumb_args.height = height;
> >       dumb_args.bpp = info->cpp[0] * 8;
> > +     dumb_args.flags = DRM_MODE_DUMB_KERNEL_MAP;
> >       ret = drm_mode_create_dumb(dev, &dumb_args, client->file);
> >       if (ret)
> >               goto err_delete;
> > diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c
> > index d18a740fe0f1..74a13f14c173 100644
> > --- a/drivers/gpu/drm/drm_dumb_buffers.c
> > +++ b/drivers/gpu/drm/drm_dumb_buffers.c
> > @@ -97,7 +97,10 @@ int drm_mode_create_dumb(struct drm_device *dev,
> >  int drm_mode_create_dumb_ioctl(struct drm_device *dev,
> >                              void *data, struct drm_file *file_priv)
> >  {
> > -     return drm_mode_create_dumb(dev, data, file_priv);
> > +     struct drm_mode_create_dumb *args = data;
> > +
> > +     args->flags = 0;
>
> I would prefer returning an error if flags is set to a non-zero value,
> to catch userspace errors early, but I'm also worried it would break
> existing userspace by uncovering existing bugs. Still, if we later add
> flags that userspace can set, those existing bugs will be triggered, so
> we'll have to deal with them anyway, and we could already give it a try.

I would like that too, but the comment just above this code tells me
that's likely to break things:

        /*
         * handle, pitch and size are output parameters. Zero them out to
         * prevent drivers from accidentally using uninitialized data. Since
         * not all existing userspace is clearing these fields properly we
         * cannot reject IOCTL with garbage in them.
         */

Maybe userspace does correctly zero out input params.

Rob