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

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

 



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




[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