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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 26.07.22 um 22:20 schrieb Dave Airlie:
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.

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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux