Hi Am 26.07.22 um 22:20 schrieb Dave Airlie: > On Wed, 27 Jul 2022 at 04:18, Thomas Zimmermann 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 >>>> >>>> Changed the uAPI. >>>> >>>> Signed-off-by: Kevin Brace >>> >>> 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