Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1222042 > unrolled thread

Re: [PATCH 3/5] update virtio gpu driver: add 3d/virgl support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/5] update virtio gpu driver: add 3d/virgl support Emil Velikov <emil.l.velikov@gmail.com> - 2015-09-10 11:00 +0200
    Re: [PATCH 3/5] update virtio gpu driver: add 3d/virgl support Emil Velikov <emil.l.velikov@gmail.com> - 2015-09-10 14:20 +0200
    Re: [PATCH 3/5] update virtio gpu driver: add 3d/virgl support Gerd Hoffmann <kraxel@redhat.com> - 2015-09-10 16:50 +0200

#1222042 — Re: [PATCH 3/5] update virtio gpu driver: add 3d/virgl support

Hello Gert,

On 9 September 2015 at 12:42, Gerd Hoffmann <kraxel@redhat.com> wrote:
> Add the bits needed for opengl rendering support: query
> capabilities, new virtio commands, drm ioctls.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---

> +
> +struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
> +       DRM_IOCTL_DEF_DRV(VIRTGPU_MAP, virtio_gpu_map_ioctl,
> +                         DRM_AUTH|DRM_UNLOCKED),
> +
> +       DRM_IOCTL_DEF_DRV(VIRTGPU_EXECBUFFER, virtio_gpu_execbuffer_ioctl,
> +                         DRM_AUTH|DRM_UNLOCKED),
> +
> +       DRM_IOCTL_DEF_DRV(VIRTGPU_GETPARAM, virtio_gpu_getparam_ioctl,
> +                         DRM_AUTH|DRM_UNLOCKED),
> +
> +       DRM_IOCTL_DEF_DRV(VIRTGPU_RESOURCE_CREATE,
> +                         virtio_gpu_resource_create_ioctl,
> +                         DRM_AUTH|DRM_UNLOCKED),
> +
> +       DRM_IOCTL_DEF_DRV(VIRTGPU_RESOURCE_INFO, virtio_gpu_resource_info_ioctl,
> +                         DRM_AUTH|DRM_UNLOCKED),
> +
> +       /* make transfer async to the main ring? - no sure, can we
> +          thread these in the underlying GL */
> +       DRM_IOCTL_DEF_DRV(VIRTGPU_TRANSFER_FROM_HOST,
> +                         virtio_gpu_transfer_from_host_ioctl,
> +                         DRM_AUTH|DRM_UNLOCKED),
> +       DRM_IOCTL_DEF_DRV(VIRTGPU_TRANSFER_TO_HOST,
> +                         virtio_gpu_transfer_to_host_ioctl,
> +                         DRM_AUTH|DRM_UNLOCKED),
> +
> +       DRM_IOCTL_DEF_DRV(VIRTGPU_WAIT, virtio_gpu_wait_ioctl,
> +                         DRM_AUTH|DRM_UNLOCKED),
> +
> +       DRM_IOCTL_DEF_DRV(VIRTGPU_GET_CAPS, virtio_gpu_get_caps_ioctl,
> +                         DRM_AUTH|DRM_UNLOCKED),

Just a FYI - Daniel Vetter has a series in flight which deprecates
DRM_UNLOCKED for KMS drivers.

> --- /dev/null
> +++ b/include/uapi/drm/virtgpu_drm.h
> @@ -0,0 +1,163 @@

> +
> +struct drm_virtgpu_3d_box {
> +       uint32_t x, y, z;
> +       uint32_t w, h, d;
> +};
> +
There was a similar case (multiple variables declared on a single
line) in drm core that caused confusion and we broke the 32bit compat.
I thought I mention it - not advocating for/against the above declaration.

> +struct drm_virtgpu_3d_transfer_to_host {
> +       uint32_t bo_handle;
> +       struct drm_virtgpu_3d_box box;
> +       uint32_t level;
> +       uint32_t offset;
> +};
> +
> +struct drm_virtgpu_3d_transfer_from_host {
> +       uint32_t bo_handle;
> +       struct drm_virtgpu_3d_box box;
> +       uint32_t level;
> +       uint32_t offset;
> +};
> +
Afaics these seems to be used by the ioctls. If so the current
declarations are not 32bit compat safe. Things will also go badly if
you consider expanding struct drm_virtgpu_3d_box in the distant
future. A u32 pad after bo_handle and a 'pointer' to struct
drm_virtgpu_3d_box might be the more flexible solution.

I believe the original idea is to use __[su]XX types in the public drm
headers, although it seems that most drivers opted for [u]intXX_t
ones. Don't quote me on that last one :)

Cheers,
Emil
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1222155

On 10 September 2015 at 11:32, Dave Airlie <airlied@gmail.com> wrote:
>> > --- /dev/null
>> > +++ b/include/uapi/drm/virtgpu_drm.h
>> > @@ -0,0 +1,163 @@
>>
>> > +
>> > +struct drm_virtgpu_3d_box {
>> > +       uint32_t x, y, z;
>> > +       uint32_t w, h, d;
>> > +};
>> > +
>> There was a similar case (multiple variables declared on a single
>> line) in drm core that caused confusion and we broke the 32bit compat.
>> I thought I mention it - not advocating for/against the above declaration.
>
>
> probably should put some newlines alright,
>
>>
>
>> > +struct drm_virtgpu_3d_transfer_to_host {
>> > +       uint32_t bo_handle;
>> > +       struct drm_virtgpu_3d_box box;
>> > +       uint32_t level;
>> > +       uint32_t offset;
>> > +};
>> > +
>> > +struct drm_virtgpu_3d_transfer_from_host {
>> > +       uint32_t bo_handle;
>> > +       struct drm_virtgpu_3d_box box;
>> > +       uint32_t level;
>> > +       uint32_t offset;
>> > +};
>> > +
>> Afaics these seems to be used by the ioctls. If so the current
>> declarations are not 32bit compat safe. Things will also go badly if
>> you consider expanding struct drm_virtgpu_3d_box in the distant
>> future. A u32 pad after bo_handle and a 'pointer' to struct
>
> I'm curious what you think we can add to a 3d box.
>
None from the top of my head I'm afraid. But I'm wondering if one
cannot keep things flexible considering the cost/downside of the
approach is minimal.

> So this one is fine, no need to align it either I don't think, though I
> should confirm that.
>
> Maybe for safety we can pull the other uint32_t above the box.
>
pahole did complain on my system, but please do - was low on caffeine
so I could have botched something.

Regards
Emil
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222239

  Hi,

> Just a FYI - Daniel Vetter has a series in flight which deprecates
> DRM_UNLOCKED for KMS drivers.

Thanks for the heads up.

> 
> > --- /dev/null
> > +++ b/include/uapi/drm/virtgpu_drm.h
> > @@ -0,0 +1,163 @@
> 
> > +
> > +struct drm_virtgpu_3d_box {
> > +       uint32_t x, y, z;
> > +       uint32_t w, h, d;
> > +};
> > +
> There was a similar case (multiple variables declared on a single
> line) in drm core that caused confusion and we broke the 32bit compat.
> I thought I mention it - not advocating for/against the above declaration.

I highly doubt we'll ever change that.
But we can give each struct field its own row, sure.

> > +struct drm_virtgpu_3d_transfer_to_host {
> > +       uint32_t bo_handle;
> > +       struct drm_virtgpu_3d_box box;
> > +       uint32_t level;
> > +       uint32_t offset;
> > +};
> > +
> > +struct drm_virtgpu_3d_transfer_from_host {
> > +       uint32_t bo_handle;
> > +       struct drm_virtgpu_3d_box box;
> > +       uint32_t level;
> > +       uint32_t offset;
> > +};
> > +
> Afaics these seems to be used by the ioctls. If so the current
> declarations are not 32bit compat safe.

Why?  As long as we have only 32bit fields in the struct the struct
itself gets a 32bit alignment too.  So no layout differences between
32bit and 64bit.

> Things will also go badly if you consider expanding 
> struct drm_virtgpu_3d_box in the distant future.

See above, very unlikely.  And should that really happen we have bigger
problems anyway because it is quite likely that we have to touch the
virtio wire protocol too.

> A u32 pad after bo_handle and a 'pointer' to struct
> drm_virtgpu_3d_box might be the more flexible solution.

IMO this makes things more complicated for no good reason.

cheers,
  Gerd


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web