[PATCH v3 26/32] drm/via: Add via_drm.h

[Kevin Brace]

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 at ravnborg.org>
> To: "Kevin Brace" <kevinbrace at gmx.com>
> Cc: dri-devel at lists.freedesktop.org, "Kevin Brace" <kevinbrace at bracecomputerlab.com>
> 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 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.
> 
> > +
> > +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
>