Re: [PATCH v3 26/32] drm/via: Add via_drm.h
Hi Kevin, > 1) Get an un-accelerated driver merged without new uapi, ioctls, etc. All > you need is there already. We already agreed to take the code mostly as-is > and do the cleanups later on. > > 2a) Stay around on dri-devel, send improvements and fixes for the merged > code. > > 2b) In parallel, you can work on video, 3d, etc in both kernel and > userspace. And on top of this the driver will see all the refactoring going on in drm drivers all the time, and we may find cases where the driver can use more of the drm helpers. I really hope to see next revision where there is focus on the general un-accelerated case and without the extras as Thomas mentions. Sam
Re: [PATCH v3 26/32] drm/via: Add via_drm.h
Hi Am 03.08.22 um 03:49 schrieb Kevin Brace: Hi Thomas, I am honestly surprised of the e-mail back and forth regarding the mainlining of OpenChrome DRM, but let me state my position. That's because your expectations are unrealistic. We don't know you, you apparently have little prior kernel experience and want to merge code that is essentially your pet project. It's also not a good start to insist on merging new uapi calls. Upstream kernel is not a place to dump code that you declare ready. You're becoming part of a community. Your first priority should be to earn the trust of the other developers on this mailing list. Considering the age of the hardware I am dealing with (i.e., pre-OpenGL 2.x generation 3D engine), I do not anticipate being able to run OpenChrome on Wayland with acceleration. Acceleration and wayland are independent from each other. As a first step after mainlining, I am looking to add uAPI to pass 2D / 3D acceleration commands to the graphics engine, but frankly, I am going to focus on the 2D acceleration side first. Did you read https://blog.ffwll.ch/2018/08/no-2d-in-drm.html ? Considering the age of the hardware, I do not think limiting the use to X.Org X Server is a bad choice. If you're not interested in running modern userspace, then why are you doing this anyway? Everyone with an X server can already use the existing DRI1 code. You also don't seem to understand the state of the Linux graphics stack. First of all, you're not building a driver for one particular program, but for all of them. There's the X server, a number of Wayland compositors, and a few more outlandish things like kmscon. And secondly, did you read https://ajaxnwnk.blogspot.com/2020/10/on-abandoning-x-server.html ? Xorg-on-hardware is pretty much dead. There hasn't been a release for years. What will remain is Xorg-on-Wayland, which is provided by xwayland. I do OpenChrome development on Gentoo Linux where 32-bit x86 ISA and X.Org X Server are still fully supported. Adding 3D acceleration will likely be done after 2D and video accelerations are mostly working. Video acceleration seems realistic. 2d and 3d not so much. 2d for reasons given in Daniel's blog. 3d because the hardware might be too old already. If VIA really only supports OpenGL-1.x-era features, you'd have to revive the old mesa branches to use it. But I can't say for sure. The proposed OpenChrome uAPI is essentially a cutdown version of the mostly 2D acceleration focused implementation (my opinion) being worked on and off since 2011. The limited addition of uAPI calls is merely a preparatory step for adding 2D acceleration in the near future (I have not started the work yet.). I assume you are aware that OpenChrome DDX is a user of DRM_VIA_GEM_CREATE, DRM_VIA_GEM_MAP, and DRM_VIA_GEM_UNMAP uAPIs. To my understanding, the DDX code only exists in a repository in a git tree of yours. That's not "userspace". For those who still choose to use older generation hardware, I think X.Org X Server still has a lot of life left in it, and I plan to continue modernizing the graphics stack for what I call "underserved" (i.e., neglected) graphics hardware. As I said we welcome drivers for old hardware. But your current proposal is not feasible and will cause friction with upstream. Here's a suggestion that is more promising. 1) Get an un-accelerated driver merged without new uapi, ioctls, etc. All you need is there already. We already agreed to take the code mostly as-is and do the cleanups later on. 2a) Stay around on dri-devel, send improvements and fixes for the merged code. 2b) In parallel, you can work on video, 3d, etc in both kernel and userspace. 3) When you have a feature ready, send a patchset to dri-devel with the kernel changes and another patchset to the respective userspace project (e.g., Mesa). They should refer to each other, so that reviewers can see both sides of the interface at the same time. Best regards Thomas Regards, Kevin Brace Brace Computer Laboratory blog https://bracecomputerlab.com Sent: Tuesday, August 02, 2022 at 4:38 AM From: "Thomas Zimmermann" To: "Kevin Brace" Cc: dri-devel@lists.freedesktop.org Subject: Re: [PATCH v3 26/32] drm/via: Add via_drm.h Hi Am 02.08.22 um 05:45 schrieb Kevin Brace: Hi Thomas, I hope I am comprehending this right. Yes, I am adding 3 new uAPI calls, not 6 of them. Correspondingly, there are 3 new structs added. That's understood. While I may drop one (unmap uAPI), I personally do not wish to drop the other two at this point. Instead, I may need to add setparam and / or getparam uAPI to pass PCI Device ID back to the userspace. This is mainly needed by Mesa, although there is no code for Mesa at this point. Exactly my point! There's no userspace for it. That's why
Re: [PATCH v3 26/32] drm/via: Add via_drm.h
Hi Thomas, I am honestly surprised of the e-mail back and forth regarding the mainlining of OpenChrome DRM, but let me state my position. Considering the age of the hardware I am dealing with (i.e., pre-OpenGL 2.x generation 3D engine), I do not anticipate being able to run OpenChrome on Wayland with acceleration. As a first step after mainlining, I am looking to add uAPI to pass 2D / 3D acceleration commands to the graphics engine, but frankly, I am going to focus on the 2D acceleration side first. Considering the age of the hardware, I do not think limiting the use to X.Org X Server is a bad choice. I do OpenChrome development on Gentoo Linux where 32-bit x86 ISA and X.Org X Server are still fully supported. Adding 3D acceleration will likely be done after 2D and video accelerations are mostly working. The proposed OpenChrome uAPI is essentially a cutdown version of the mostly 2D acceleration focused implementation (my opinion) being worked on and off since 2011. The limited addition of uAPI calls is merely a preparatory step for adding 2D acceleration in the near future (I have not started the work yet.). I assume you are aware that OpenChrome DDX is a user of DRM_VIA_GEM_CREATE, DRM_VIA_GEM_MAP, and DRM_VIA_GEM_UNMAP uAPIs. For those who still choose to use older generation hardware, I think X.Org X Server still has a lot of life left in it, and I plan to continue modernizing the graphics stack for what I call "underserved" (i.e., neglected) graphics hardware. Regards, Kevin Brace Brace Computer Laboratory blog https://bracecomputerlab.com > Sent: Tuesday, August 02, 2022 at 4:38 AM > From: "Thomas Zimmermann" > To: "Kevin Brace" > Cc: dri-devel@lists.freedesktop.org > Subject: Re: [PATCH v3 26/32] drm/via: Add via_drm.h > > Hi > > Am 02.08.22 um 05:45 schrieb Kevin Brace: > > Hi Thomas, > > > > I hope I am comprehending this right. > > Yes, I am adding 3 new uAPI calls, not 6 of them. > > Correspondingly, there are 3 new structs added. > > That's understood. > > > While I may drop one (unmap uAPI), I personally do not wish to drop the > > other two at this point. > > Instead, I may need to add setparam and / or getparam uAPI to pass PCI > > Device ID back to the userspace. > > This is mainly needed by Mesa, although there is no code for Mesa at this > > point. > > Exactly my point! There's no userspace for it. That's why Sam and me are > asking you to remove all kinds if uapi changes or ioctls from the > patchset until Mesa (or some other component) requires it. > > > I fear dropping the remaining two will require substantial redesign, and I > > will like to avoid this since the code is already working. > > No, it won't require a redesign. You'll have to remove the changes to > the uapi header and any new ioctls that are in the patchset. Userspace > programs; such as X11's modesetting driver, Weston or Gnome; will use > the kernel's dumb-buffer ioctls to create unaccelerated buffers. You > won't need any via-specific code in userspace. It's all there already > and fully driver independent. Mesa will do software rendering. For the > kernel's dumb buffers, please see [1]. > > > It is my plan to proceed to adding acceleration after the code is added to > > the mainline kernel tree, so I will like to do it the way it is set up now. > > You can still send the current uapi changes when you add 3d acceleration > to the kernel and Mesa. But once these interfaces have been added to > the kernel, they are nearly impossible to change or remove. That's why > we don't want to do this now. > > Best regards > Thomas > > [1] > https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#dumb-buffer-objects > > > > > Regards, > > > > Kevin Brace > > Brace Computer Laboratory blog > > https://bracecomputerlab.com > > > > > >> Sent: Monday, August 01, 2022 at 11:49 AM > >> From: "Thomas Zimmermann" > >> To: "Kevin Brace" > >> Cc: dri-devel@lists.freedesktop.org > >> Subject: Re: [PATCH v3 26/32] drm/via: Add via_drm.h > >> > >> Hi Kevin > >> > >> Am 31.07.22 um 00:48 schrieb Kevin Brace: > >>> Hi Thomas, > >>> > >>> I cannot drop the older DRI1 based uAPI calls. > >>> This is because include/uapi/drm/via_drm.h needs to retain backward > >>> compatibility with the existing OpenChrome DDX's XvMC library (it gets > >>> compiled when OpenChrome DDX is built) and likely with the existing DDX > >>> Xv code as well. > >>> If
Re: [PATCH v3 26/32] drm/via: Add via_drm.h
Hi Sam, Regarding DRI1 era uAPI, I believe I am handling the matter similar to how Radeon DRM header (include/uapi/drm/radeon_drm.h) handled the matter. The header still contains old DRI1 uAPI calls, and it then adds new KMS generation uAPI calls. At this point, using drm_invalid_op() for OpenChrome DRM is the least intrusive option, and that's the way I will like to keep it. It is just that I did not know its existence, so it was not in the code. The older OpenChrome DDX releases might assume uAPI backward compatibility, but the use of drm_invalid_op() should gracefully tell the DDX that the legacy DRI1 VIA DRM uAPI is no longer supported. Personally, I do not anticipate Wayland use with OpenChrome. It will end up living out its life as X.Org X Server only solution considering its hardware age. Although it is somewhat hard, I use Gentoo Linux as development platform for OpenChrome, and 32-bit x86 ISA and X.Org X Server are still fully supported. Regards, Kevin Brace Brace Computer Laboratory blog https://bracecomputerlab.com > Sent: Tuesday, August 02, 2022 at 4:09 AM > From: "Sam Ravnborg" > To: "Kevin Brace" > Cc: "Thomas Zimmermann" , dri-devel@lists.freedesktop.org > Subject: Re: [PATCH v3 26/32] drm/via: Add via_drm.h > > Hi Kevin, > > > > OpenChrome DDX carries lots of legacy code. > > > > https://cgit.freedesktop.org/openchrome/xf86-video-openchrome/tree/src/via_drm.h?h=main&id=dc661c59257e855cd9b29c14b91a8ee2d9b86ccb > > > > There is a requirement to use the same via_drm.h with both DDX and DRM. > > Hence, I need to keep a lot of the legacy DRI1 definitions inside via_drm.h. > > This part is fully understood. Also on top of this the via DRI1 driver > uses this. I am not asking to have anything deleted from the existing > uapi via_drm.h file. > > > My feedback is that the following code should be dropped from the > openchrome driver: > > + DRM_IOCTL_DEF_DRV(VIA_ALLOCMEM, drm_invalid_op, DRM_AUTH), > + DRM_IOCTL_DEF_DRV(VIA_FREEMEM, drm_invalid_op, DRM_AUTH), > + DRM_IOCTL_DEF_DRV(VIA_AGP_INIT, drm_invalid_op, DRM_AUTH | DRM_MASTER), > + DRM_IOCTL_DEF_DRV(VIA_FB_INIT, drm_invalid_op, DRM_AUTH | DRM_MASTER), > + DRM_IOCTL_DEF_DRV(VIA_MAP_INIT, drm_invalid_op, DRM_AUTH | DRM_MASTER), > + DRM_IOCTL_DEF_DRV(VIA_DEC_FUTEX, drm_invalid_op, DRM_AUTH), > + DRM_IOCTL_DEF_DRV(VIA_DMA_INIT, drm_invalid_op, DRM_AUTH), > + DRM_IOCTL_DEF_DRV(VIA_CMDBUFFER, drm_invalid_op, DRM_AUTH), > + DRM_IOCTL_DEF_DRV(VIA_FLUSH, drm_invalid_op, DRM_AUTH), > + DRM_IOCTL_DEF_DRV(VIA_PCICMD, drm_invalid_op, DRM_AUTH), > + DRM_IOCTL_DEF_DRV(VIA_CMDBUF_SIZE, drm_invalid_op, DRM_AUTH), > + DRM_IOCTL_DEF_DRV(VIA_WAIT_IRQ, drm_invalid_op, DRM_AUTH), > + DRM_IOCTL_DEF_DRV(VIA_DMA_BLIT, drm_invalid_op, DRM_AUTH), > + DRM_IOCTL_DEF_DRV(VIA_BLIT_SYNC, drm_invalid_op, DRM_AUTH), > > (Copied from openchrome-drm - I recall you did not post this code yet). > > The new openchrome driver should not care at all about the old UAPI, > so just drop the above. > > The comment above is based on the understanding that when we have a kms > compliant driver the user space is generic and we do not expect or need > any via specifics in user space. > > In other words - x86-video-openchrome should - according to my > understanding - not be needed. And we can have a fully operational > wayland (and maybe X) userspace using the generic UAPI. This is where > Thomas Zimmermann's comment about dumb buffers are relevant. > > Do I miss something obvious here? > > Sam >
Re: [PATCH v3 26/32] drm/via: Add via_drm.h
Hi Am 02.08.22 um 05:45 schrieb Kevin Brace: Hi Thomas, I hope I am comprehending this right. Yes, I am adding 3 new uAPI calls, not 6 of them. Correspondingly, there are 3 new structs added. That's understood. While I may drop one (unmap uAPI), I personally do not wish to drop the other two at this point. Instead, I may need to add setparam and / or getparam uAPI to pass PCI Device ID back to the userspace. This is mainly needed by Mesa, although there is no code for Mesa at this point. Exactly my point! There's no userspace for it. That's why Sam and me are asking you to remove all kinds if uapi changes or ioctls from the patchset until Mesa (or some other component) requires it. I fear dropping the remaining two will require substantial redesign, and I will like to avoid this since the code is already working. No, it won't require a redesign. You'll have to remove the changes to the uapi header and any new ioctls that are in the patchset. Userspace programs; such as X11's modesetting driver, Weston or Gnome; will use the kernel's dumb-buffer ioctls to create unaccelerated buffers. You won't need any via-specific code in userspace. It's all there already and fully driver independent. Mesa will do software rendering. For the kernel's dumb buffers, please see [1]. It is my plan to proceed to adding acceleration after the code is added to the mainline kernel tree, so I will like to do it the way it is set up now. You can still send the current uapi changes when you add 3d acceleration to the kernel and Mesa. But once these interfaces have been added to the kernel, they are nearly impossible to change or remove. That's why we don't want to do this now. Best regards Thomas [1] https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#dumb-buffer-objects Regards, Kevin Brace Brace Computer Laboratory blog https://bracecomputerlab.com Sent: Monday, August 01, 2022 at 11:49 AM From: "Thomas Zimmermann" To: "Kevin Brace" Cc: dri-devel@lists.freedesktop.org Subject: Re: [PATCH v3 26/32] drm/via: Add via_drm.h Hi Kevin Am 31.07.22 um 00:48 schrieb Kevin Brace: Hi Thomas, I cannot drop the older DRI1 based uAPI calls. This is because include/uapi/drm/via_drm.h needs to retain backward compatibility with the existing OpenChrome DDX's XvMC library (it gets compiled when OpenChrome DDX is built) and likely with the existing DDX Xv code as well. If I remove the DRI1 based uAPI calls, the XvMC library will not get compiled (compile error will occur since the XvMC library assumes the presence of DRI1 based uAPI), and I assume the same for the DDX Xv code (I cannot even reach here since the XvMC library is compiled first). Although the v3 patch does not contain it, v4 patch will utilize drm_invalid_op() for the discontinued (not deprecated since OpenChrome DRM does not support the older DRI1 based uAPI at all) DRI1 based uAPI. https://cgit.freedesktop.org/openchrome/drm-openchrome/commit/?h=drm-next-5.20&id=16b3d68f95c9ccd15b7a3310e5d752fabbc76518 drm_invalid_op() is related to drm_ioctl.c, and is meant for legacy DRMs like Radeon, i915, etc. Since OpenChrome DRM is not a clean sheet design (related to VIA DRM to some extent), I will use this function for properly handling discontinued legacy uAPI calls. I hope this explanation / reasoning is okay with you. I'm not sure I understand your reply ormaybe I'm just missing something here. I'm not asking you to remove the existing DRI1 uapi. I'm just asking to not add the 6 new _GEM_ defines and 3 new _gem_ structures now. You mentioned that the driver does not yet support acceleration of any kind. So there should be no need to extend to uapi now. You can still do this when you add acceleration to the driver. Until then, the Xorg modesetting driver or any Compositor can use the generic dumb-buffer ioctls that create buffers with no acceleration. Best regards Thomas Regards, Kevin Brace Brace Computer Laboratory blog https://bracecomputerlab.com Sent: Tuesday, July 26, 2022 at 11:24 AM From: "Thomas Zimmermann" To: "Kevin Brace" , dri-devel@lists.freedesktop.org Cc: "Kevin Brace" Subject: Re: [PATCH v3 26/32] drm/via: Add via_drm.h Hi Am 26.07.22 um 01:53 schrieb Kevin Brace: From: Kevin Brace Changed the uAPI. Signed-off-by: Kevin Brace --- 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 @@
Re: [PATCH v3 26/32] drm/via: Add via_drm.h
Hi Kevin, > > OpenChrome DDX carries lots of legacy code. > > https://cgit.freedesktop.org/openchrome/xf86-video-openchrome/tree/src/via_drm.h?h=main&id=dc661c59257e855cd9b29c14b91a8ee2d9b86ccb > > There is a requirement to use the same via_drm.h with both DDX and DRM. > Hence, I need to keep a lot of the legacy DRI1 definitions inside via_drm.h. This part is fully understood. Also on top of this the via DRI1 driver uses this. I am not asking to have anything deleted from the existing uapi via_drm.h file. My feedback is that the following code should be dropped from the openchrome driver: + DRM_IOCTL_DEF_DRV(VIA_ALLOCMEM, drm_invalid_op, DRM_AUTH), + DRM_IOCTL_DEF_DRV(VIA_FREEMEM, drm_invalid_op, DRM_AUTH), + DRM_IOCTL_DEF_DRV(VIA_AGP_INIT, drm_invalid_op, DRM_AUTH | DRM_MASTER), + DRM_IOCTL_DEF_DRV(VIA_FB_INIT, drm_invalid_op, DRM_AUTH | DRM_MASTER), + DRM_IOCTL_DEF_DRV(VIA_MAP_INIT, drm_invalid_op, DRM_AUTH | DRM_MASTER), + DRM_IOCTL_DEF_DRV(VIA_DEC_FUTEX, drm_invalid_op, DRM_AUTH), + DRM_IOCTL_DEF_DRV(VIA_DMA_INIT, drm_invalid_op, DRM_AUTH), + DRM_IOCTL_DEF_DRV(VIA_CMDBUFFER, drm_invalid_op, DRM_AUTH), + DRM_IOCTL_DEF_DRV(VIA_FLUSH, drm_invalid_op, DRM_AUTH), + DRM_IOCTL_DEF_DRV(VIA_PCICMD, drm_invalid_op, DRM_AUTH), + DRM_IOCTL_DEF_DRV(VIA_CMDBUF_SIZE, drm_invalid_op, DRM_AUTH), + DRM_IOCTL_DEF_DRV(VIA_WAIT_IRQ, drm_invalid_op, DRM_AUTH), + DRM_IOCTL_DEF_DRV(VIA_DMA_BLIT, drm_invalid_op, DRM_AUTH), + DRM_IOCTL_DEF_DRV(VIA_BLIT_SYNC, drm_invalid_op, DRM_AUTH), (Copied from openchrome-drm - I recall you did not post this code yet). The new openchrome driver should not care at all about the old UAPI, so just drop the above. The comment above is based on the understanding that when we have a kms compliant driver the user space is generic and we do not expect or need any via specifics in user space. In other words - x86-video-openchrome should - according to my understanding - not be needed. And we can have a fully operational wayland (and maybe X) userspace using the generic UAPI. This is where Thomas Zimmermann's comment about dumb buffers are relevant. Do I miss something obvious here? Sam
Re: [PATCH v3 26/32] drm/via: Add via_drm.h
Hi Thomas, I hope I am comprehending this right. Yes, I am adding 3 new uAPI calls, not 6 of them. Correspondingly, there are 3 new structs added. While I may drop one (unmap uAPI), I personally do not wish to drop the other two at this point. Instead, I may need to add setparam and / or getparam uAPI to pass PCI Device ID back to the userspace. This is mainly needed by Mesa, although there is no code for Mesa at this point. I fear dropping the remaining two will require substantial redesign, and I will like to avoid this since the code is already working. It is my plan to proceed to adding acceleration after the code is added to the mainline kernel tree, so I will like to do it the way it is set up now. Regards, Kevin Brace Brace Computer Laboratory blog https://bracecomputerlab.com > Sent: Monday, August 01, 2022 at 11:49 AM > From: "Thomas Zimmermann" > To: "Kevin Brace" > Cc: dri-devel@lists.freedesktop.org > Subject: Re: [PATCH v3 26/32] drm/via: Add via_drm.h > > Hi Kevin > > Am 31.07.22 um 00:48 schrieb Kevin Brace: > > Hi Thomas, > > > > I cannot drop the older DRI1 based uAPI calls. > > This is because include/uapi/drm/via_drm.h needs to retain backward > > compatibility with the existing OpenChrome DDX's XvMC library (it gets > > compiled when OpenChrome DDX is built) and likely with the existing DDX Xv > > code as well. > > If I remove the DRI1 based uAPI calls, the XvMC library will not get > > compiled (compile error will occur since the XvMC library assumes the > > presence of DRI1 based uAPI), and I assume the same for the DDX Xv code (I > > cannot even reach here since the XvMC library is compiled first). > > Although the v3 patch does not contain it, v4 patch will utilize > > drm_invalid_op() for the discontinued (not deprecated since OpenChrome DRM > > does not support the older DRI1 based uAPI at all) DRI1 based uAPI. > > > > https://cgit.freedesktop.org/openchrome/drm-openchrome/commit/?h=drm-next-5.20&id=16b3d68f95c9ccd15b7a3310e5d752fabbc76518 > > > > drm_invalid_op() is related to drm_ioctl.c, and is meant for legacy DRMs > > like Radeon, i915, etc. > > Since OpenChrome DRM is not a clean sheet design (related to VIA DRM to > > some extent), I will use this function for properly handling discontinued > > legacy uAPI calls. > > I hope this explanation / reasoning is okay with you. > > I'm not sure I understand your reply ormaybe I'm just missing something > here. > > I'm not asking you to remove the existing DRI1 uapi. I'm just asking to > not add the 6 new _GEM_ defines and 3 new _gem_ structures now. You > mentioned that the driver does not yet support acceleration of any kind. > So there should be no need to extend to uapi now. You can still do this > when you add acceleration to the driver. > > Until then, the Xorg modesetting driver or any Compositor can use the > generic dumb-buffer ioctls that create buffers with no acceleration. > > Best regards > Thomas > > > > > Regards, > > > > Kevin Brace > > Brace Computer Laboratory blog > > https://bracecomputerlab.com > > > >> Sent: Tuesday, July 26, 2022 at 11:24 AM > >> From: "Thomas Zimmermann" > >> To: "Kevin Brace" , dri-devel@lists.freedesktop.org > >> Cc: "Kevin Brace" > >> Subject: Re: [PATCH v3 26/32] drm/via: Add via_drm.h > >> > >> Hi > >> > >> Am 26.07.22 um 01:53 schrieb Kevin Brace: > >>> From: Kevin Brace > >>> > >>> Changed the uAPI. > >>> > >>> Signed-off-by: Kevin Brace > >>> --- > >>>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, > >>> *
Re: [PATCH v3 26/32] drm/via: Add via_drm.h
Hi Sam, OpenChrome DDX carries lots of legacy code. https://cgit.freedesktop.org/openchrome/xf86-video-openchrome/tree/src/via_drm.h?h=main&id=dc661c59257e855cd9b29c14b91a8ee2d9b86ccb There is a requirement to use the same via_drm.h with both DDX and DRM. Hence, I need to keep a lot of the legacy DRI1 definitions inside via_drm.h. Regards, Kevin Brace Brace Computer Laboratory blog https://bracecomputerlab.com > Sent: Monday, August 01, 2022 at 7:40 AM > From: "Sam Ravnborg" > To: "Kevin Brace" > Cc: "Thomas Zimmermann" , dri-devel@lists.freedesktop.org > Subject: Re: [PATCH v3 26/32] drm/via: Add via_drm.h > > Hi Kevin, > > > I cannot drop the older DRI1 based uAPI calls. > > This is because include/uapi/drm/via_drm.h needs to retain backward > > compatibility with the existing OpenChrome DDX's XvMC library > > (it gets compiled when OpenChrome DDX is built) and likely with the > > existing DDX Xv code as well. > > If I remove the DRI1 based uAPI calls, the XvMC library will not get > > compiled (compile error will occur since the XvMC library assumes the > > presence of DRI1 based uAPI), and I assume the same for the DDX Xv code > > (I cannot even reach here since the XvMC library is compiled first). > > If you just keep the relevant definitions in drm_via.h then the compile > issues should be OK - and then there is no need to implement anything in > the driver. Or did I not understand the problem you are trying to solve? > > Sam >
Re: [PATCH v3 26/32] drm/via: Add via_drm.h
Hi Kevin
Am 31.07.22 um 00:48 schrieb Kevin Brace:
Hi Thomas,
I cannot drop the older DRI1 based uAPI calls.
This is because include/uapi/drm/via_drm.h needs to retain backward
compatibility with the existing OpenChrome DDX's XvMC library (it gets compiled
when OpenChrome DDX is built) and likely with the existing DDX Xv code as well.
If I remove the DRI1 based uAPI calls, the XvMC library will not get compiled
(compile error will occur since the XvMC library assumes the presence of DRI1
based uAPI), and I assume the same for the DDX Xv code (I cannot even reach
here since the XvMC library is compiled first).
Although the v3 patch does not contain it, v4 patch will utilize
drm_invalid_op() for the discontinued (not deprecated since OpenChrome DRM does
not support the older DRI1 based uAPI at all) DRI1 based uAPI.
https://cgit.freedesktop.org/openchrome/drm-openchrome/commit/?h=drm-next-5.20&id=16b3d68f95c9ccd15b7a3310e5d752fabbc76518
drm_invalid_op() is related to drm_ioctl.c, and is meant for legacy DRMs like
Radeon, i915, etc.
Since OpenChrome DRM is not a clean sheet design (related to VIA DRM to some
extent), I will use this function for properly handling discontinued legacy
uAPI calls.
I hope this explanation / reasoning is okay with you.
I'm not sure I understand your reply ormaybe I'm just missing something
here.
I'm not asking you to remove the existing DRI1 uapi. I'm just asking to
not add the 6 new _GEM_ defines and 3 new _gem_ structures now. You
mentioned that the driver does not yet support acceleration of any kind.
So there should be no need to extend to uapi now. You can still do this
when you add acceleration to the driver.
Until then, the Xorg modesetting driver or any Compositor can use the
generic dumb-buffer ioctls that create buffers with no acceleration.
Best regards
Thomas
Regards,
Kevin Brace
Brace Computer Laboratory blog
https://bracecomputerlab.com
Sent: Tuesday, July 26, 2022 at 11:24 AM
From: "Thomas Zimmermann"
To: "Kevin Brace" , dri-devel@lists.freedesktop.org
Cc: "Kevin Brace"
Subject: Re: [PATCH v3 26/32] drm/via: Add via_drm.h
Hi
Am 26.07.22 um 01:53 schrieb Kevin Brace:
From: Kevin Brace
Changed the uAPI.
Signed-off-by: Kevin Brace
---
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.
*/
#ifndef _VIA_DRM_H_
#define _VIA_DRM_H_
@@ -81,6 +82,11 @@ extern "C" {
#define DRM_VIA_DMA_BLIT0x0e
#define DRM_VIA_BLIT_SYNC 0x0f
+#defineDRM_VIA_GEM_CREATE 0x10
+#defineDRM_VIA_GEM_MAP 0x11
+#defineDRM_VIA_GEM_UNMAP 0x12
This looks a lot like ioctl ops for using accelerated HW buffers. But
the patch is near the end of the series and you said in the series'
cover letter that there's no acceleration. I suspect that these
constants are currently unused? If so, please drop the patch from the
series. If can be merged later when something really depends on it.
Best regards
Thomas
+
+
#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_BLITDRM_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)
+#defineDRM_IOCTL_VIA_GEM_CREATEDRM_IOWR(DRM_COMMAND_BASE +
DRM_VIA_GEM_CREATE, struct drm_via_gem_create)
+
Re: [PATCH v3 26/32] drm/via: Add via_drm.h
Hi Kevin, > I cannot drop the older DRI1 based uAPI calls. > This is because include/uapi/drm/via_drm.h needs to retain backward > compatibility with the existing OpenChrome DDX's XvMC library > (it gets compiled when OpenChrome DDX is built) and likely with the > existing DDX Xv code as well. > If I remove the DRI1 based uAPI calls, the XvMC library will not get > compiled (compile error will occur since the XvMC library assumes the > presence of DRI1 based uAPI), and I assume the same for the DDX Xv code > (I cannot even reach here since the XvMC library is compiled first). If you just keep the relevant definitions in drm_via.h then the compile issues should be OK - and then there is no need to implement anything in the driver. Or did I not understand the problem you are trying to solve? Sam
Re: [PATCH v3 26/32] drm/via: Add via_drm.h
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"
> To: "Kevin Brace"
> Cc: dri-devel@lists.freedesktop.org, "Kevin Brace"
>
> 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
> >
> > Changed the uAPI.
> >
> > Signed-off-by: Kevin Brace
>
> 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_BLIT0x0e
> > #define DRM_VIA_BLIT_SYNC 0x0f
> >
> > +#defineDRM_VIA_GEM_CREATE 0x10
> > +#defineDRM_VIA_GEM_MAP 0x11
> > +#defineDRM_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_FREEMEMDRM_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_BLITDRM_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)
> >
> > +#defineDRM_IOCTL_VIA_GEM_CREATEDRM_IOWR(DRM_COMMAND_BASE +
> > DRM_VIA_GEM_CREATE, struct drm_via_gem_create)
> > +#defineDRM_IOCTL_VIA_GEM_MAP DRM_IOWR(DRM_COMMAND_BASE +
> > DRM_VIA_GEM_MAP, struct drm_via_gem_map)
> > +#defineDRM_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
Re: [PATCH v3 26/32] drm/via: Add via_drm.h
Hi Thomas,
I cannot drop the older DRI1 based uAPI calls.
This is because include/uapi/drm/via_drm.h needs to retain backward
compatibility with the existing OpenChrome DDX's XvMC library (it gets compiled
when OpenChrome DDX is built) and likely with the existing DDX Xv code as well.
If I remove the DRI1 based uAPI calls, the XvMC library will not get compiled
(compile error will occur since the XvMC library assumes the presence of DRI1
based uAPI), and I assume the same for the DDX Xv code (I cannot even reach
here since the XvMC library is compiled first).
Although the v3 patch does not contain it, v4 patch will utilize
drm_invalid_op() for the discontinued (not deprecated since OpenChrome DRM does
not support the older DRI1 based uAPI at all) DRI1 based uAPI.
https://cgit.freedesktop.org/openchrome/drm-openchrome/commit/?h=drm-next-5.20&id=16b3d68f95c9ccd15b7a3310e5d752fabbc76518
drm_invalid_op() is related to drm_ioctl.c, and is meant for legacy DRMs like
Radeon, i915, etc.
Since OpenChrome DRM is not a clean sheet design (related to VIA DRM to some
extent), I will use this function for properly handling discontinued legacy
uAPI calls.
I hope this explanation / reasoning is okay with you.
Regards,
Kevin Brace
Brace Computer Laboratory blog
https://bracecomputerlab.com
> Sent: Tuesday, July 26, 2022 at 11:24 AM
> From: "Thomas Zimmermann"
> To: "Kevin Brace" , dri-devel@lists.freedesktop.org
> Cc: "Kevin Brace"
> Subject: Re: [PATCH v3 26/32] drm/via: Add via_drm.h
>
> Hi
>
> Am 26.07.22 um 01:53 schrieb Kevin Brace:
> > From: Kevin Brace
> >
> > Changed the uAPI.
> >
> > Signed-off-by: Kevin Brace
> > ---
> > 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.
> >*/
> > #ifndef _VIA_DRM_H_
> > #define _VIA_DRM_H_
> > @@ -81,6 +82,11 @@ extern "C" {
> > #define DRM_VIA_DMA_BLIT0x0e
> > #define DRM_VIA_BLIT_SYNC 0x0f
> >
> > +#defineDRM_VIA_GEM_CREATE 0x10
> > +#defineDRM_VIA_GEM_MAP 0x11
> > +#defineDRM_VIA_GEM_UNMAP 0x12
>
> This looks a lot like ioctl ops for using accelerated HW buffers. But
> the patch is near the end of the series and you said in the series'
> cover letter that there's no acceleration. I suspect that these
> constants are currently unused? If so, please drop the patch from the
> series. If can be merged later when something really depends on it.
>
> Best regards
> Thomas
>
> > +
> > +
> > #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_BLITDRM_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)
> >
> > +#defineDRM_IOCTL_VIA_GEM_CREATEDRM_IOWR(DRM_COMMAND_BASE +
> > DRM_VIA_
Re: [PATCH v3 26/32] drm/via: Add via_drm.h
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"
> To: "Thomas Zimmermann"
> Cc: "Sam Ravnborg" , "Kevin Brace" ,
> "Kevin Brace" , "dri-devel"
>
> Subject: Re: [PATCH v3 26/32] drm/via: Add via_drm.h
>
> On Wed, 27 Jul 2022 at 04:18, Thomas Zimmermann 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
> > >>
> > >> Changed the uAPI.
> > >>
> > >> Signed-off-by: Kevin Brace
> > >
> > > 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_BLIT0x0e
> > >> #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_FREEMEMDRM_IOW( DRM_COMMAND_BASE +
> > >>
Re: [PATCH v3 26/32] drm/via: Add via_drm.h
Hi
Am 26.07.22 um 22:20 schrieb Dave Airlie:
On Wed, 27 Jul 2022 at 04:18, Thomas Zimmermann 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
Changed the uAPI.
Signed-off-by: Kevin Brace
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_BLIT0x0e
#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_FREEMEMDRM_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_BLITDRM_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_CREATEDRM_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 Develop
Re: [PATCH v3 26/32] drm/via: Add via_drm.h
On Wed, 27 Jul 2022 at 04:18, Thomas Zimmermann 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
> >>
> >> Changed the uAPI.
> >>
> >> Signed-off-by: Kevin Brace
> >
> > 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_BLIT0x0e
> >> #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_FREEMEMDRM_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_BLITDRM_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_CREATEDRM_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
Re: [PATCH v3 26/32] drm/via: Add via_drm.h
Hi
Am 26.07.22 um 01:53 schrieb Kevin Brace:
From: Kevin Brace
Changed the uAPI.
Signed-off-by: Kevin Brace
---
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.
*/
#ifndef _VIA_DRM_H_
#define _VIA_DRM_H_
@@ -81,6 +82,11 @@ extern "C" {
#define DRM_VIA_DMA_BLIT0x0e
#define DRM_VIA_BLIT_SYNC 0x0f
+#defineDRM_VIA_GEM_CREATE 0x10
+#defineDRM_VIA_GEM_MAP 0x11
+#defineDRM_VIA_GEM_UNMAP 0x12
This looks a lot like ioctl ops for using accelerated HW buffers. But
the patch is near the end of the series and you said in the series'
cover letter that there's no acceleration. I suspect that these
constants are currently unused? If so, please drop the patch from the
series. If can be merged later when something really depends on it.
Best regards
Thomas
+
+
#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_BLITDRM_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)
+#defineDRM_IOCTL_VIA_GEM_CREATEDRM_IOWR(DRM_COMMAND_BASE +
DRM_VIA_GEM_CREATE, struct drm_via_gem_create)
+#defineDRM_IOCTL_VIA_GEM_MAP DRM_IOWR(DRM_COMMAND_BASE +
DRM_VIA_GEM_MAP, struct drm_via_gem_map)
+#defineDRM_IOCTL_VIA_GEM_UNMAP DRM_IOR(DRM_COMMAND_BASE +
DRM_VIA_GEM_UNMAP, struct drm_via_gem_unmap)
+
/* 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;
+};
+
+struct drm_via_gem_map {
+ uint32_t handle;
+ uint64_t map_offset;
+};
+
+struct drm_via_gem_unmap {
+ uint32_t handle;
+};
+
#if defined(__cplusplus)
}
#endif
--
2.35.1
--
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
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v3 26/32] drm/via: Add via_drm.h
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
Changed the uAPI.
Signed-off-by: Kevin Brace
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_BLIT0x0e
#define DRM_VIA_BLIT_SYNC 0x0f
+#defineDRM_VIA_GEM_CREATE 0x10
+#defineDRM_VIA_GEM_MAP 0x11
+#defineDRM_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_BLITDRM_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)
+#defineDRM_IOCTL_VIA_GEM_CREATEDRM_IOWR(DRM_COMMAND_BASE +
DRM_VIA_GEM_CREATE, struct drm_via_gem_create)
+#defineDRM_IOCTL_VIA_GEM_MAP DRM_IOWR(DRM_COMMAND_BASE +
DRM_VIA_GEM_MAP, struct drm_via_gem_map)
+#defineDRM_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.
Best regards
Thomas
+
+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 de
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
>
> Changed the uAPI.
>
> Signed-off-by: Kevin Brace
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_BLIT0x0e
> #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_BLITDRM_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_CREATEDRM_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
