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. 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