On Tue, Oct 22, 2019 at 6:14 AM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> 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@xxxxxxxx>
> > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> > Cc: Sean Paul <sean@xxxxxxxxxx>
> > Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> > ---
> > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
