Hi Dave, Thanks for pointing out the blog written by Daniel. I was thinking that I will need to make refinements to OpenChrome DRM uAPI, so I will rework the uAPI. I also plan to discontinue old VIA DRM uAPI by using drm_invalid_op(). No one pointed this out, but I was thinking that something like this had to be done. I will work on these, and post the updated code soon with taking into consideration the VIA DRM DRI1 compaction work done by Sam / Thomas. Regards, Kevin Brace Brace Computer Laboratory blog https://bracecomputerlab.com > Sent: Tuesday, July 26, 2022 at 1:20 PM > From: "Dave Airlie" <airlied@xxxxxxxxx> > To: "Thomas Zimmermann" <tzimmermann@xxxxxxx> > Cc: "Sam Ravnborg" <sam@xxxxxxxxxxxx>, "Kevin Brace" <kevinbrace@xxxxxxx>, "Kevin Brace" <kevinbrace@xxxxxxxxxxxxxxxxxxxx>, "dri-devel" <dri-devel@xxxxxxxxxxxxxxxxxxxxx> > Subject: Re: [PATCH v3 26/32] drm/via: Add via_drm.h > > On Wed, 27 Jul 2022 at 04:18, Thomas Zimmermann <tzimmermann@xxxxxxx> 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 <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. > > > > 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. > > Dave. >
