Hello Thomas, On 7/18/22 08:56, Thomas Zimmermann wrote: > Hi > > Am 16.07.22 um 20:17 schrieb Sam Ravnborg: >> While discussing the way forward for the via driver >> Javier came up with the proposal to move all DRI1 drivers >> to their own folder. >> >> The idea is to move the old DRI1 drivers so one do not >> accidentally consider them modern drivers. >> >> This set of patches implements this idea. >> >> To prepare the move, DRIVER_LEGACY and CONFIG_DRM_LEGACY >> are both renamed to *_DRI1. This makes it more obvious >> that we are dealing with DRI1 drivers, as we have >> a lot of other legacy support. >> >> The drivers continue to have their own sub-directory >> so the driver files are not mixed with the core files >> which are copied in the last commit. >> >> The DRI1 specific part of drm/Kconfig is likewise pulled >> out and located in the dri1/ folder. >> >> Feedback welcome! > > To be honest, I still don't like this rename. Especially in the case of > via, which has a KMS driver coming up. It will now have an include > statement that crosses several levels in the directory hierarchy. And That could be avoided by moving drivers/gpu/drm/via/via_3d_reg.h and other header files to include/drm/via_3d_reg.h for example. Other drivers (i.e: i915) do the same for headers that are shared across different subsystems. > what about the other DRI1 drivers? If we ever get KMS drivers for those, > do we want to move some header files back into their original locations? I believe for these we could also move them to include/drm/ if needed. > Patches 1 and 2 look reasonable to me. The other driver patches have > basically zero upside IMHO. > I disagree with the zero upside. It may be that the trade offs are not worth it but there are upsides of having all DRI1 drivers and core DRI1 bits in the same place. It makes grep-ing and reading files easier if one doesn't care about legacy DRI1 drivers. Also, it would ease the removal of this if we ever get rid of them. > In the case of moving the core files into dri1/, the resulting Makefile > rule looks really ugly. I'd suggest to move all code into a separate Maybe this could be sorted by splitting the DRI1 core bits in a separate drm_dri1.ko module? > file drm_dri1.c and be done with it. For something more elaborate, > there could by drm_dri1.c and drm_dri1_helper.c, where the latter > contains all DRI1 code that is only used by the drivers. > That would be an improvement but IMO moving them into a different dir as Sam did would be preferable. What would be the upside of having it in drivers/gpu/drm instead? Just to avoid the Makefile rule to have a dri1/ prefix in the included object files ? Regardless of this discussion, I think that at the very least we should rename the Kconfig symbols to CONFIG_DRM_DRI1_* even if DRI1 drivers are kept in drivers/gpu/drm/ instead of moved to a drivers/gpu/drm/dri1/ dir. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
