Re: [PATCH v1 0/11] drm/via: Make via a single file driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux