[PATCH v3 26/32] drm/via: Add via_drm.h
Thomas Zimmermann
tzimmermann at suse.de
Thu Aug 4 12:20:04 UTC 2022
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" <tzimmermann at suse.de>
>> To: "Kevin Brace" <kevinbrace at gmx.com>
>> Cc: dri-devel at 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 at suse.de>
>>>> To: "Kevin Brace" <kevinbrace at gmx.com>
>>>> Cc: dri-devel at 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 at suse.de>
>>>>>> To: "Kevin Brace" <kevinbrace at gmx.com>, dri-devel at lists.freedesktop.org
>>>>>> Cc: "Kevin Brace" <kevinbrace at 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 at bracecomputerlab.com>
>>>>>>>
>>>>>>> Changed the uAPI.
>>>>>>>
>>>>>>> Signed-off-by: Kevin Brace <kevinbrace at 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
>>
--
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220804/43759a63/attachment-0001.sig>
More information about the dri-devel
mailing list