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" <tzimmermann@xxxxxxx> > To: "Kevin Brace" <kevinbrace@xxxxxxx> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx > 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" <tzimmermann@xxxxxxx> > >> To: "Kevin Brace" <kevinbrace@xxxxxxx> > >> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx > >> 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" <tzimmermann@xxxxxxx> > >>>> To: "Kevin Brace" <kevinbrace@xxxxxxx>, dri-devel@xxxxxxxxxxxxxxxxxxxxx > >>>> Cc: "Kevin Brace" <kevinbrace@xxxxxxxxxxxxxxxxxxxx> > >>>> 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 <kevinbrace@xxxxxxxxxxxxxxxxxxxx> > >>>>> > >>>>> Changed the uAPI. > >>>>> > >>>>> Signed-off-by: Kevin Brace <kevinbrace@xxxxxxxxxxxxxxxxxxxx> > >>>>> --- > >>>>> 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_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 > >>>> > >>>> 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_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) > >>>>> + > >>>>> /* 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 > >>>> > >> > >> -- > >> 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 > >> > > -- > 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 >
