search

freebsd / drm-kmod

drm driver for FreeBSD
148 stars 68 forks source link

Bring back AGP support, fix it, and clean it up #202

Closed TijlCoosemans closed 1 year ago

TijlCoosemans commented 1 year ago

This series of patches restores AGP support on i386. The reverts are all clean and unmodified. The other commits fix a few issues, restore upstream Linux code, and put FreeBSD code behind #ifdef so it should be easier to merge Linux code.

lin72h commented 1 year ago

Please upgrade your GPU, don't bring the complexity into the code base

wulf7 commented 1 year ago

What a complexity? The code should remain in tree if it has users and maintainers.

I think the main reason it was removed is GPL-ed header. Unfortunately this issue still persists. But given the fact that the header does not contain any code or comments, it looks like we can just replace license line to make it BSD-compatible.

evadot commented 1 year ago

The GPLv2 header is indeed one of the reasons. The other are we are in 2022 and PCI-e is almost 20 years old now and that there have been talk to remove agp from main.

TijlCoosemans commented 1 year ago

The GPL header is a drm header, not a linuxkpi header. I believe the code was removed because FreeBSD modifications were all over the place and this caused problems when merging from upstream. This patch series should alleviate that because the diff with upstream is a lot smaller now and every difference is behind #ifdef FreeBSD. Linux still supports AGP, FreeBSD still supports AGP, the drm drivers still support AGP, so I don't see why we shouldn't support it. If FreeBSD main removes agp(4) then it will still be part of stable branches for several years.

evadot commented 1 year ago

The GPL header is a drm header, not a linuxkpi header.

Doesn't matter, I've almost replaced all GPL code so I can bring back drm into base so it should be rewritten.

I believe the code was removed because FreeBSD modifications were all over the place and this caused problems when merging from upstream.

Nope, as said before it's because we're in 2022.

This patch series should alleviate that because the diff with upstream is a lot smaller now and every difference is behind #ifdef FreeBSD. Linux still supports AGP, FreeBSD still supports AGP, the drm drivers still support AGP, so I don't see why we shouldn't support it.

Because Linux supports it isn't a reason. AGP code is now legacy in upstream Linux, see https://github.com/torvalds/linux/commit/04dfe19a5ed683e91d8285df5be1dbde2f2f39af It's part of 5.14, sure we're not at it yet but I don't want to bring back all the CONFIG_DRM_LEGACY stuff in base.

If FreeBSD main removes agp(4) then it will still be part of stable branches for several years.

TijlCoosemans commented 1 year ago

AFAICT that commit only removes support for user mode switching drivers. The radeon driver is kernel mode switching and still supports AGP separate from CONFIG_DRM_LEGACY according to https://github.com/torvalds/linux/commit/cf241e872fc203b59e8ff4b4eae6a6c2511d3747

evadot commented 1 year ago

AFAICT that commit only removes support for user mode switching drivers. The radeon driver is kernel mode switching and still supports AGP separate from CONFIG_DRM_LEGACY according to torvalds/linux@cf241e8

Mhm yeah looks like you're right. I'm ok to bring back this code but please rewrite the header.

TijlCoosemans commented 1 year ago

Just leaving this comment because I'm not sure if you were automatically notified of the last commit which changed the license. I ended up only changing the license because the code in the file is old, from before the GPL was slapped on it, and I assumed future merges would be easier if the file stayed as similar as possible to upstream. This could be a problem if future merges would add new GPL code, but there are no such commits. There's https://github.com/torvalds/linux/commit/ff28a9f8d3b4e2e6d07e37e0f5e15119ee4aff55 which only removes lines, and https://github.com/torvalds/linux/commit/04dfe19a5ed683e91d8285df5be1dbde2f2f39af which deletes the file. I can help with merging these commits because they'll need FreeBSD specific adjustments.