[PATCH v3 26/32] drm/via: Add via_drm.h
Thomas Zimmermann
tzimmermann at suse.de
Wed Jul 27 07:36:07 UTC 2022
Hi
Am 26.07.22 um 22:20 schrieb Dave Airlie:
> On Wed, 27 Jul 2022 at 04:18, Thomas Zimmermann <tzimmermann at suse.de> wrote:
>>
>> Hi
>>
>> Am 26.07.22 um 19:41 schrieb Sam Ravnborg:
>>> Hi Kevin.
>>>
>>> On Mon, Jul 25, 2022 at 04:53:53PM -0700, Kevin Brace wrote:
>>>> From: Kevin Brace <kevinbrace at bracecomputerlab.com>
>>>>
>>>> Changed the uAPI.
>>>>
>>>> Signed-off-by: Kevin Brace <kevinbrace at bracecomputerlab.com>
>>>
>>> It would be great to have the new extensions to the UAPI documented
>>> using kernel-doc.
>>> As an example see: vc4_drm.h
>>>
>>> There are a lot of UAPI that is missing documentation, but if we do not
>>> add it for new UAPI then this situation never improves.
>>>
>>> Please use __u32, __u64 like you see in other drm UAPI files.
>>>
>>> PS. If you reply to this mail, then please keep the full mail as
>>> usually my mails to Kevin bounces (something with STARTTLS).
>>>
>>> Sam
>>>
>>>> ---
>>>> include/uapi/drm/via_drm.h | 35 +++++++++++++++++++++++++++++++----
>>>> 1 file changed, 31 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/include/uapi/drm/via_drm.h b/include/uapi/drm/via_drm.h
>>>> index a1e125d42208..e9da45ce130a 100644
>>>> --- a/include/uapi/drm/via_drm.h
>>>> +++ b/include/uapi/drm/via_drm.h
>>>> @@ -1,4 +1,5 @@
>>>> /*
>>>> + * Copyright © 2020 Kevin Brace
>>>> * Copyright 1998-2003 VIA Technologies, Inc. All Rights Reserved.
>>>> * Copyright 2001-2003 S3 Graphics, Inc. All Rights Reserved.
>>>> *
>>>> @@ -16,10 +17,10 @@
>>>> * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>>> * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>>> * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
>>>> - * VIA, S3 GRAPHICS, AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>>>> - * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>>> - * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>>>> - * DEALINGS IN THE SOFTWARE.
>>>> + * THE AUTHORS, COPYRIGHT HOLDERS, AND/OR ITS SUPPLIERS BE LIABLE FOR ANY
>>>> + * CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT
>>>> + * OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR
>>>> + * THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>>>> */
>>> Do not mix license changes with other changes - and use SPDX tag if
>>> possible for the updated license.
>>> See other drm UAPI files for examples.
>>>
>>>
>>>> #ifndef _VIA_DRM_H_
>>>> #define _VIA_DRM_H_
>>>> @@ -81,6 +82,11 @@ extern "C" {
>>>> #define DRM_VIA_DMA_BLIT 0x0e
>>>> #define DRM_VIA_BLIT_SYNC 0x0f
>>>>
>>>> +#define DRM_VIA_GEM_CREATE 0x10
>>>> +#define DRM_VIA_GEM_MAP 0x11
>>>> +#define DRM_VIA_GEM_UNMAP 0x12
>>>> +
>>> Use the same alignment as the previous lines.
>>>> +
>>> Drop extra empty line.
>>>
>>>> #define DRM_IOCTL_VIA_ALLOCMEM DRM_IOWR(DRM_COMMAND_BASE + DRM_VIA_ALLOCMEM, drm_via_mem_t)
>>>> #define DRM_IOCTL_VIA_FREEMEM DRM_IOW( DRM_COMMAND_BASE + DRM_VIA_FREEMEM, drm_via_mem_t)
>>>> #define DRM_IOCTL_VIA_AGP_INIT DRM_IOWR(DRM_COMMAND_BASE + DRM_VIA_AGP_INIT, drm_via_agp_t)
>>>> @@ -97,6 +103,10 @@ extern "C" {
>>>> #define DRM_IOCTL_VIA_DMA_BLIT DRM_IOW(DRM_COMMAND_BASE + DRM_VIA_DMA_BLIT, drm_via_dmablit_t)
>>>> #define DRM_IOCTL_VIA_BLIT_SYNC DRM_IOW(DRM_COMMAND_BASE + DRM_VIA_BLIT_SYNC, drm_via_blitsync_t)
>>>>
>>>> +#define DRM_IOCTL_VIA_GEM_CREATE DRM_IOWR(DRM_COMMAND_BASE + DRM_VIA_GEM_CREATE, struct drm_via_gem_create)
>>>> +#define DRM_IOCTL_VIA_GEM_MAP DRM_IOWR(DRM_COMMAND_BASE + DRM_VIA_GEM_MAP, struct drm_via_gem_map)
>>>> +#define DRM_IOCTL_VIA_GEM_UNMAP DRM_IOR(DRM_COMMAND_BASE + DRM_VIA_GEM_UNMAP, struct drm_via_gem_unmap)
>>>> +
>>> Use same alignment as previous lines.
>>>
>>>> /* Indices into buf.Setup where various bits of state are mirrored per
>>>> * context and per buffer. These can be fired at the card as a unit,
>>>> * or in a piecewise fashion as required.
>>>> @@ -275,6 +285,23 @@ typedef struct drm_via_dmablit {
>>>> drm_via_blitsync_t sync;
>>>> } drm_via_dmablit_t;
>>>>
>>>> +struct drm_via_gem_create {
>>>> + uint64_t size;
>>>> + uint32_t alignment;
>>>> + uint32_t domain;
>>>> + uint32_t handle;
>>>> + uint64_t offset;
>>>> +};
>>> I do not know if this is relevant, but adding a 64 bit parameter
>>> (offset) that is only 32 bit aligned looks like trouble to me.
>>> I hope others that knows this better can comment here.
>>
>> The compiler will leave a 4-byte gap between handle and offset.
>> Structure allocation guarantees a minimal alignment of 8 bytes, so the
>> field alignment will be correct. It's all dependend on architecture,
>> platofrm, calling convention, but that's the rule of thumb.
>>
>> Have a look at the tool 'pahole' to inspect data-structure alignment in
>> object files. You'll find plenty of gaps in compiled structure.
>>
>> It's still questionable to leave the gap there. Either declare it
>> explicity (e.g., __u32 empty0; ) or declare the structure with
>> __attribute__((__packed__)). Personally, I'd use the former.
>
> It's not allowed at all to use packed or leave the gap.
>
> https://www.kernel.org/doc/html/latest/process/botching-up-ioctls.html
>
> The 2nd prereq covers this.
I wasn't aware of this page. Thanks.
Best regards
Thomas
>
> Dave.
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220727/88650fa4/attachment.sig>
More information about the dri-devel
mailing list