[PATCH v3 26/32] drm/via: Add via_drm.h
Thomas Zimmermann
tzimmermann at suse.de
Tue Jul 26 18:18:51 UTC 2022
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.
Best regards
Thomas
>
>> +
>> +struct drm_via_gem_map {
>> + uint32_t handle;
>> + uint64_t map_offset;
>> +};
> Same here with the alignment.
>
> If drm_via_gem_create.offset and drm_via_gem_map.map_offset is the same
> the field should have the same name - to make the API easier to use.
>
>
>> +
>> +struct drm_via_gem_unmap {
>> + uint32_t handle;
>> +};
>> +
>> #if defined(__cplusplus)
>> }
>> #endif
>> --
>> 2.35.1
--
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/20220726/41192279/attachment.sig>
More information about the dri-devel
mailing list