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

[Kevin Brace <kevinbrace@gmx.com>]

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@suse.de>
> To: "Kevin Brace" <kevinbrace@gmx.com>
> 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" <tzimmermann@suse.de>
> >> To: "Kevin Brace" <kevinbrace@gmx.com>
> >> 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" <tzimmermann@suse.de>
> >>>> To: "Kevin Brace" <kevinbrace@gmx.com>, dri-devel@lists.freedesktop.org
> >>>> Cc: "Kevin Brace" <kevinbrace@bracecomputerlab.com>
> >>>> 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@bracecomputerlab.com>
> >>>>>
> >>>>> Changed the uAPI.
> >>>>>
> >>>>> Signed-off-by: Kevin Brace <kevinbrace@bracecomputerlab.com>
> >>>>> ---
> >>>>>     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
>