Hi Sam, I will study vc4_drm.h and update newer portions (i.e., the section I am actively adding to via_drm.h) with new comments that can be converted to kernel-doc automatically. I also plan to rework the uAPI somewhat. - Remove DRM_VIA_GEM_UNMAP - Revive a uAPI that can pass a PCI Device ID back to userspace. Regards, Kevin Brace Brace Computer Laboratory blog https://bracecomputerlab.com > Sent: Tuesday, July 26, 2022 at 10:41 AM > From: "Sam Ravnborg" <sam@xxxxxxxxxxxx> > To: "Kevin Brace" <kevinbrace@xxxxxxx> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx, "Kevin Brace" <kevinbrace@xxxxxxxxxxxxxxxxxxxx> > Subject: Re: [PATCH v3 26/32] drm/via: Add via_drm.h > > Hi Kevin. > > On Mon, Jul 25, 2022 at 04:53:53PM -0700, Kevin Brace wrote: > > From: Kevin Brace <kevinbrace@xxxxxxxxxxxxxxxxxxxx> > > > > Changed the uAPI. > > > > Signed-off-by: Kevin Brace <kevinbrace@xxxxxxxxxxxxxxxxxxxx> > > 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. > > > + > > +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 >
