Hi Thomas, On Mon, Jul 11, 2022 at 09:01:50AM +0200, Thomas Zimmermann wrote: > Hi Sam, > > this looks like a good solution to me. I'd give you a detailed review, but > dri-devel decided to only send me the color letter. I had to use patchwork > get the patches. > > For the series > > Acked-by: Thomas Zimmermann <tzimmermann@xxxxxxx> Thanks! > > with suggestions below. > > Am 10.07.22 um 10:54 schrieb Sam Ravnborg: > > We have an upcoming openchrome driver for VIA where some > > of the files conflicts with the existing via driver. > > > > It is not acceptable to just delete the existing DRI1 > > based driver as there are most likely users out there, > > so a different approach was required. > > > > It was disccussed what to do and the least invasive > > solution was to keep the DRI1 driver in the current > > directory as a single file. > > > > Thomas Zimmermann already posted a patch to do the > > same but it attemped to have a single driver > > for the DRI1 and the upcoming new driver. > > After the openchrome patches land, there will be an option in Kconfig to > build the old driver instead of of the new one? > > > > > This patchset embeds the files one by one to make the > > diffs remotely readable and end up with an independent > > DRI1 driver. > > > > The driver was built tested for each step. > > > > The driver is in the end renamed to via_dri1. > > Some feedback on this would be good as I do not know > > what impact it will have on users. > > I don't know how Mesa decides about which userspace driver to load, but It > seems related to the name of the kernel driver. Renaming the module might > interfere somehow. But if the old and new driver are mutually exclusive at > build time, it should not be a problem to use the same name for both > modules. Another option could be to keep the "via.ko" name and come up with a new name for the openchrome variant (via_drm?). I think we need to allow both drivers to be built as a user may want to try out the old and the new driver without to much hassle. In the next iteration I will drop the rename of the driver - it is easy to do later as it is a simple one-liner. > > > > The very last patch synchronize the via_3d_reg header > > file with the one used in the openchrome driver. > > This was added to verify that the new header would not > > break the DRI1 driver. > > Some of the macros introduce line wraps. I don't know if you did that > intentionally, but it shouldn't be necessary. We have a 100-character limit > per line. The via_3d_reg file was copied verbatim from the openchrome repo. I will fix up relevant checkpatch warnings in a follow-up patch so it is obvious what is changed from the original source. > Maybe you can also add an extra patch that adds SPDX license tags at the top > of the files. Yep, will do. Thanks for the feedback! Sam
