Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hello Thomas, On Wed, Jul 12, 2023 at 12:19:37PM +0200, Thomas Zimmermann wrote: > Am 12.07.23 um 11:46 schrieb Uwe Kleine-König: > > Hello, > > > > while I debugged an issue in the imx-lcdc driver I was constantly > > irritated about struct drm_device pointer variables being named "dev" > > because with that name I usually expect a struct device pointer. > > > > I think there is a big benefit when these are all renamed to "drm_dev". > > If you rename drm_crtc.dev, you should also address *all* other data > structures. Yes. Changing drm_crtc::dev was some effort, so I thought to send that one out before doing the same to drm_dp_mst_topology_mgr drm_atomic_state drm_master drm_bridge drm_client_dev drm_connector drm_debugfs_entry drm_encoder drm_fb_helper drm_minor drm_framebuffer drm_gem_object drm_plane drm_property drm_property_blob drm_vblank_crtc when in the end the intention isn't welcome. > > I have no strong preference here though, so "drmdev" or "drm" are fine > > for me, too. Let the bikesheding begin! > > We've discussed this to death. IIRC 'drm' would be the prefered choice. "drm" at least has the advantage to be the 2nd most common name. With Paul Kocialkowski prefering "drm_dev" there is no clear favourite yet. Maybe all the other people with strong opinions are dead if this was "discussed to death" before? :-) Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On 12.07.2023 13:07, Julia Lawall wrote: On Wed, 12 Jul 2023, Uwe Kleine-König wrote: On Wed, Jul 12, 2023 at 12:46:33PM +0200, Christian König wrote: Am 12.07.23 um 11:46 schrieb Uwe Kleine-König: Hello, while I debugged an issue in the imx-lcdc driver I was constantly irritated about struct drm_device pointer variables being named "dev" because with that name I usually expect a struct device pointer. I think there is a big benefit when these are all renamed to "drm_dev". I have no strong preference here though, so "drmdev" or "drm" are fine for me, too. Let the bikesheding begin! Some statistics: $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | sort -n 1 struct drm_device *adev_to_drm 1 struct drm_device *drm_ 1 struct drm_device *drm_dev 1 struct drm_device*drm_dev 1 struct drm_device *pdev 1 struct drm_device *rdev 1 struct drm_device *vdev 2 struct drm_device *dcss_drv_dev_to_drm 2 struct drm_device **ddev 2 struct drm_device *drm_dev_alloc 2 struct drm_device *mock 2 struct drm_device *p_ddev 5 struct drm_device *device 9 struct drm_device * dev 25 struct drm_device *d 95 struct drm_device * 216 struct drm_device *ddev 234 struct drm_device *drm_dev 611 struct drm_device *drm 4190 struct drm_device *dev This series starts with renaming struct drm_crtc::dev to drm_dev. If it's not only me and others like the result of this effort it should be followed up by adapting the other structs and the individual usages in the different drivers. To make this series a bit easier handleable, I first added an alias for drm_crtc::dev, then converted the drivers one after another and the last patch drops the "dev" name. This has the advantage of being easier to review, and if I should have missed an instance only the last patch must be dropped/reverted. Also this series might conflict with other patches, in this case the remaining patches can still go in (apart from the last one of course). Maybe it also makes sense to delay applying the last patch by one development cycle? When you automatically generate the patch (with cocci for example) I usually prefer a single patch instead. Maybe I'm too stupid, but only parts of this patch were created by coccinelle. I failed to convert code like - spin_lock_irq(>dev->event_lock); + spin_lock_irq(>drm_dev->event_lock); Added Julia to Cc, maybe she has a hint?! A priori, I see no reason why the rule below should not apply to the above code. Is there a parsing problem in the containing function? You can run spatch --parse-c file.c If there is a paring problem, please let me know and i will try to fix it so the while thing can be done automatically. I guess some clever macros can fool spatch, at least I observe such things in i915 which often uses custom iterators. Regards Andrzej julia (Up to now it's only @@ struct drm_crtc *crtc; @@ -crtc->dev +crtc->drm_dev ) Background is that this makes merge conflicts easier to handle and detect. Really? Each file (apart from include/drm/drm_crtc.h) is only touched once. So unless I'm missing something you don't get less or easier conflicts by doing it all in a single patch. But you gain the freedom to drop a patch for one driver without having to drop the rest with it. So I still like the split version better, but I'm open to a more verbose reasoning from your side. When you have multiple patches and a merge conflict because of some added lines using the old field the build breaks only on the last patch which removes the old field. Then you can revert/drop the last patch without having to respin the whole single patch and thus caring for still more conflicts that arise until the new version is sent. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | >
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Wed, 12 Jul 2023, Uwe Kleine-König wrote: > Hello, > > while I debugged an issue in the imx-lcdc driver I was constantly > irritated about struct drm_device pointer variables being named "dev" > because with that name I usually expect a struct device pointer. > > I think there is a big benefit when these are all renamed to "drm_dev". > I have no strong preference here though, so "drmdev" or "drm" are fine > for me, too. Let the bikesheding begin! > > Some statistics: > > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c > | sort -n > 1 struct drm_device *adev_to_drm > 1 struct drm_device *drm_ > 1 struct drm_device *drm_dev > 1 struct drm_device*drm_dev > 1 struct drm_device *pdev > 1 struct drm_device *rdev > 1 struct drm_device *vdev > 2 struct drm_device *dcss_drv_dev_to_drm > 2 struct drm_device **ddev > 2 struct drm_device *drm_dev_alloc > 2 struct drm_device *mock > 2 struct drm_device *p_ddev > 5 struct drm_device *device > 9 struct drm_device * dev > 25 struct drm_device *d > 95 struct drm_device * > 216 struct drm_device *ddev > 234 struct drm_device *drm_dev > 611 struct drm_device *drm >4190 struct drm_device *dev > > This series starts with renaming struct drm_crtc::dev to drm_dev. If > it's not only me and others like the result of this effort it should be > followed up by adapting the other structs and the individual usages in > the different drivers. I think this is an unnecessary change. In drm, a dev is usually a drm device, i.e. struct drm_device *. As shown by the numbers above. If folks insist on following through with this anyway, I'm firmly in the camp the name should be "drm" and nothing else. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hello Maxime, On Wed, Jul 12, 2023 at 02:52:38PM +0200, Maxime Ripard wrote: > On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote: > > > Background is that this makes merge conflicts easier to handle and detect. > > > > Really? > > FWIW, I agree with Christian here. > > > Each file (apart from include/drm/drm_crtc.h) is only touched once. So > > unless I'm missing something you don't get less or easier conflicts by > > doing it all in a single patch. But you gain the freedom to drop a > > patch for one driver without having to drop the rest with it. > > Not really, because the last patch removed the union anyway. So you have > to revert both the last patch, plus that driver one. And then you need > to add a TODO to remove that union eventually. Yes, with a single patch you have only one revert (but 194 files changed, 1264 insertions(+), 1296 deletions(-)) instead of two (one of them: 1 file changed, 9 insertions(+), 1 deletion(-); the other maybe a bit bigger). (And maybe you get away with just reverting the last patch.) With a single patch the TODO after a revert is "redo it all again (and prepare for a different set of conflicts)" while with the split series it's only "fix that one driver that was forgotten/borked" + reapply that 10 line patch. As the one who gets that TODO, I prefer the latter. So in sum: If your metric is "small count of reverted commits", you're right. If however your metric is: Better get 95% of this series' change in than maybe 0%, the split series is the way to do it. With me having spend ~3h on this series' changes, it's maybe understandable that I did it the way I did. FTR: This series was created on top of v6.5-rc1. If you apply it to drm-misc-next you get a (trivial) conflict in patch #2. If I consider to be the responsible maintainer who applies this series, I like being able to just do git am --skip then. FTR#2: In drm-misc-next is a new driver (drivers/gpu/drm/loongson/lsdc_crtc.c) so skipping the last patch for now might indeed be a good idea. > > So I still like the split version better, but I'm open to a more > > verbose reasoning from your side. > > You're doing only one thing here, really: you change the name of a > structure field. If it was shared between multiple maintainers, then > sure, splitting that up is easier for everyone, but this will go through > drm-misc, so I can't see the benefit it brings. I see your argument, but I think mine weights more. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hi Am 12.07.23 um 11:46 schrieb Uwe Kleine-König: Hello, while I debugged an issue in the imx-lcdc driver I was constantly irritated about struct drm_device pointer variables being named "dev" because with that name I usually expect a struct device pointer. Rather than renaming dev in all the DRM structs, did you consider renaming struct drm_device.dev instead? Everyone in DRM-land knows that 'dev' is the DRM device. But for struct drm_device.dev a more expressive name would be helpful. Maybe 'parent'. (It's also much less churn.) Best regards Thomas I think there is a big benefit when these are all renamed to "drm_dev". I have no strong preference here though, so "drmdev" or "drm" are fine for me, too. Let the bikesheding begin! Some statistics: $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | sort -n 1 struct drm_device *adev_to_drm 1 struct drm_device *drm_ 1 struct drm_device *drm_dev 1 struct drm_device*drm_dev 1 struct drm_device *pdev 1 struct drm_device *rdev 1 struct drm_device *vdev 2 struct drm_device *dcss_drv_dev_to_drm 2 struct drm_device **ddev 2 struct drm_device *drm_dev_alloc 2 struct drm_device *mock 2 struct drm_device *p_ddev 5 struct drm_device *device 9 struct drm_device * dev 25 struct drm_device *d 95 struct drm_device * 216 struct drm_device *ddev 234 struct drm_device *drm_dev 611 struct drm_device *drm 4190 struct drm_device *dev This series starts with renaming struct drm_crtc::dev to drm_dev. If it's not only me and others like the result of this effort it should be followed up by adapting the other structs and the individual usages in the different drivers. To make this series a bit easier handleable, I first added an alias for drm_crtc::dev, then converted the drivers one after another and the last patch drops the "dev" name. This has the advantage of being easier to review, and if I should have missed an instance only the last patch must be dropped/reverted. Also this series might conflict with other patches, in this case the remaining patches can still go in (apart from the last one of course). Maybe it also makes sense to delay applying the last patch by one development cycle? The series was compile tested for arm, arm64, powerpc and amd64 using an allmodconfig (though I only build drivers/gpu/). Best regards Uwe Uwe Kleine-König (52): drm/crtc: Start renaming struct drm_crtc::dev to drm_dev drm/core: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/amd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/armada: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/arm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/aspeed: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/ast: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/atmel-hlcdc: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/exynos: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/fsl-dcu: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/gma500: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/gud: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/hisilicon: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/hyperv: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/i915: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/imx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/ingenic: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/kmb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/logicvc: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/mcde: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/mediatek: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/meson: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/mgag200: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/msm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/mxsfb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/nouveau: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/omapdrm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/panel-ili9341: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/pl111: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/qxl: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/radeon: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/renesas: Use struct drm_crtc::drm_dev instead of struct
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Wed, Jul 12, 2023 at 03:38:03PM +0200, Uwe Kleine-König wrote: > Hello Maxime, > > On Wed, Jul 12, 2023 at 02:52:38PM +0200, Maxime Ripard wrote: > > On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote: > > > > Background is that this makes merge conflicts easier to handle and > > > > detect. > > > > > > Really? > > > > FWIW, I agree with Christian here. > > > > > Each file (apart from include/drm/drm_crtc.h) is only touched once. So > > > unless I'm missing something you don't get less or easier conflicts by > > > doing it all in a single patch. But you gain the freedom to drop a > > > patch for one driver without having to drop the rest with it. > > > > Not really, because the last patch removed the union anyway. So you have > > to revert both the last patch, plus that driver one. And then you need > > to add a TODO to remove that union eventually. > > Yes, with a single patch you have only one revert (but 194 files changed, > 1264 insertions(+), 1296 deletions(-)) instead of two (one of them: 1 > file changed, 9 insertions(+), 1 deletion(-); the other maybe a bit > bigger). (And maybe you get away with just reverting the last patch.) > > With a single patch the TODO after a revert is "redo it all again (and > prepare for a different set of conflicts)" while with the split series > it's only "fix that one driver that was forgotten/borked" + reapply that > 10 line patch. As the one who gets that TODO, I prefer the latter. > > So in sum: If your metric is "small count of reverted commits", you're > right. If however your metric is: Better get 95% of this series' change > in than maybe 0%, the split series is the way to do it. I guess that's where we disagree: I don't see the point of having 95% of it, either 0 or 100. > With me having spend ~3h on this series' changes, it's maybe > understandable that I did it the way I did. I'm sorry, but that's never been an argument? I'm sure you and I both have had series that took much longer dropped because it wasn't the right approach. > FTR: This series was created on top of v6.5-rc1. If you apply it to > drm-misc-next you get a (trivial) conflict in patch #2. If I consider to > be the responsible maintainer who applies this series, I like being able > to just do git am --skip then. Or we can ask that the driver is based on drm-misc-next ... > FTR#2: In drm-misc-next is a new driver > (drivers/gpu/drm/loongson/lsdc_crtc.c) so skipping the last patch for > now might indeed be a good idea. ... which is going to fix that one too. Maxime signature.asc Description: PGP signature
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hello Jani,
On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
> On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
> > Hello,
> >
> > while I debugged an issue in the imx-lcdc driver I was constantly
> > irritated about struct drm_device pointer variables being named "dev"
> > because with that name I usually expect a struct device pointer.
> >
> > I think there is a big benefit when these are all renamed to "drm_dev".
> > I have no strong preference here though, so "drmdev" or "drm" are fine
> > for me, too. Let the bikesheding begin!
> >
> > Some statistics:
> >
> > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq
> > -c | sort -n
> > 1 struct drm_device *adev_to_drm
> > 1 struct drm_device *drm_
> > 1 struct drm_device *drm_dev
> > 1 struct drm_device*drm_dev
> > 1 struct drm_device *pdev
> > 1 struct drm_device *rdev
> > 1 struct drm_device *vdev
> > 2 struct drm_device *dcss_drv_dev_to_drm
> > 2 struct drm_device **ddev
> > 2 struct drm_device *drm_dev_alloc
> > 2 struct drm_device *mock
> > 2 struct drm_device *p_ddev
> > 5 struct drm_device *device
> > 9 struct drm_device * dev
> > 25 struct drm_device *d
> > 95 struct drm_device *
> > 216 struct drm_device *ddev
> > 234 struct drm_device *drm_dev
> > 611 struct drm_device *drm
> >4190 struct drm_device *dev
> >
> > This series starts with renaming struct drm_crtc::dev to drm_dev. If
> > it's not only me and others like the result of this effort it should be
> > followed up by adapting the other structs and the individual usages in
> > the different drivers.
>
> I think this is an unnecessary change. In drm, a dev is usually a drm
> device, i.e. struct drm_device *.
Well, unless it's not. Prominently there is
struct drm_device {
...
struct device *dev;
...
};
which yields quite a few code locations using dev->dev which is
IMHO unnecessary irritating:
$ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
1633
Also the functions that deal with both a struct device and a struct
drm_device often use "dev" for the struct device and then "ddev" for
the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
> If folks insist on following through with this anyway, I'm firmly in the
> camp the name should be "drm" and nothing else.
Up to now positive feedback is in the majority.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hi Jani,
On Thu, Jul 13, 2023 at 11:03 AM Jani Nikula wrote:
> On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
> > On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
> >> On Wed, 12 Jul 2023, Uwe Kleine-König
> >> wrote:
> >> > while I debugged an issue in the imx-lcdc driver I was constantly
> >> > irritated about struct drm_device pointer variables being named "dev"
> >> > because with that name I usually expect a struct device pointer.
> >> >
> >> > I think there is a big benefit when these are all renamed to "drm_dev".
> >> > I have no strong preference here though, so "drmdev" or "drm" are fine
> >> > for me, too. Let the bikesheding begin!
> >> >
> >> > Some statistics:
> >> >
> >> > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort |
> >> > uniq -c | sort -n
> >> > 1 struct drm_device *adev_to_drm
> >> > 1 struct drm_device *drm_
> >> > 1 struct drm_device *drm_dev
> >> > 1 struct drm_device*drm_dev
> >> > 1 struct drm_device *pdev
> >> > 1 struct drm_device *rdev
> >> > 1 struct drm_device *vdev
> >> > 2 struct drm_device *dcss_drv_dev_to_drm
> >> > 2 struct drm_device **ddev
> >> > 2 struct drm_device *drm_dev_alloc
> >> > 2 struct drm_device *mock
> >> > 2 struct drm_device *p_ddev
> >> > 5 struct drm_device *device
> >> > 9 struct drm_device * dev
> >> > 25 struct drm_device *d
> >> > 95 struct drm_device *
> >> > 216 struct drm_device *ddev
> >> > 234 struct drm_device *drm_dev
> >> > 611 struct drm_device *drm
> >> >4190 struct drm_device *dev
> >> >
> >> > This series starts with renaming struct drm_crtc::dev to drm_dev. If
> >> > it's not only me and others like the result of this effort it should be
> >> > followed up by adapting the other structs and the individual usages in
> >> > the different drivers.
> >>
> >> I think this is an unnecessary change. In drm, a dev is usually a drm
> >> device, i.e. struct drm_device *.
> >
> > Well, unless it's not. Prominently there is
> >
> > struct drm_device {
> > ...
> > struct device *dev;
> > ...
> > };
> >
> > which yields quite a few code locations using dev->dev which is
> > IMHO unnecessary irritating:
> >
> > $ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
> > 1633
> >
> > Also the functions that deal with both a struct device and a struct
> > drm_device often use "dev" for the struct device and then "ddev" for
> > the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
>
> Why is specifically struct drm_device *dev so irritating to you?
>
> You lead us to believe it's an outlier in kernel, something that goes
> against common kernel style, but it's really not:
>
> $ git grep -how "struct [A-Za-z0-9_]\+ \*dev" | sort | uniq -c | sort -rn |
> head -20
> 38494 struct device *dev
> 16388 struct net_device *dev
>4184 struct drm_device *dev
>2780 struct pci_dev *dev
>1916 struct comedi_device *dev
>1510 struct mlx5_core_dev *dev
>1057 struct mlx4_dev *dev
> 894 struct b43_wldev *dev
> 762 struct input_dev *dev
> 623 struct usbnet *dev
> 561 struct mlx5_ib_dev *dev
> 525 struct mt76_dev *dev
> 465 struct mt76x02_dev *dev
> 435 struct platform_device *dev
> 431 struct usb_device *dev
> 411 struct mt7915_dev *dev
> 398 struct cx231xx *dev
> 378 struct mei_device *dev
> 363 struct ksz_device *dev
> 359 struct mthca_dev *dev
>
> A good portion of the above also have a dev member.
Not all of them access both the foo_device and the device pointers.
Let's put the number of 435 platform_device pointers named "dev"
into perspective:
10095 struct platform_device *pdev
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Wed, 12 Jul 2023, Uwe Kleine-König wrote: > On Wed, Jul 12, 2023 at 12:46:33PM +0200, Christian König wrote: > > Am 12.07.23 um 11:46 schrieb Uwe Kleine-König: > > > Hello, > > > > > > while I debugged an issue in the imx-lcdc driver I was constantly > > > irritated about struct drm_device pointer variables being named "dev" > > > because with that name I usually expect a struct device pointer. > > > > > > I think there is a big benefit when these are all renamed to "drm_dev". > > > I have no strong preference here though, so "drmdev" or "drm" are fine > > > for me, too. Let the bikesheding begin! > > > > > > Some statistics: > > > > > > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq > > > -c | sort -n > > >1 struct drm_device *adev_to_drm > > >1 struct drm_device *drm_ > > >1 struct drm_device *drm_dev > > >1 struct drm_device*drm_dev > > >1 struct drm_device *pdev > > >1 struct drm_device *rdev > > >1 struct drm_device *vdev > > >2 struct drm_device *dcss_drv_dev_to_drm > > >2 struct drm_device **ddev > > >2 struct drm_device *drm_dev_alloc > > >2 struct drm_device *mock > > >2 struct drm_device *p_ddev > > >5 struct drm_device *device > > >9 struct drm_device * dev > > > 25 struct drm_device *d > > > 95 struct drm_device * > > > 216 struct drm_device *ddev > > > 234 struct drm_device *drm_dev > > > 611 struct drm_device *drm > > > 4190 struct drm_device *dev > > > > > > This series starts with renaming struct drm_crtc::dev to drm_dev. If > > > it's not only me and others like the result of this effort it should be > > > followed up by adapting the other structs and the individual usages in > > > the different drivers. > > > > > > To make this series a bit easier handleable, I first added an alias for > > > drm_crtc::dev, then converted the drivers one after another and the last > > > patch drops the "dev" name. This has the advantage of being easier to > > > review, and if I should have missed an instance only the last patch must > > > be dropped/reverted. Also this series might conflict with other patches, > > > in this case the remaining patches can still go in (apart from the last > > > one of course). Maybe it also makes sense to delay applying the last > > > patch by one development cycle? > > > > When you automatically generate the patch (with cocci for example) I usually > > prefer a single patch instead. > > Maybe I'm too stupid, but only parts of this patch were created by > coccinelle. I failed to convert code like > > - spin_lock_irq(>dev->event_lock); > + spin_lock_irq(>drm_dev->event_lock); > > Added Julia to Cc, maybe she has a hint?! A priori, I see no reason why the rule below should not apply to the above code. Is there a parsing problem in the containing function? You can run spatch --parse-c file.c If there is a paring problem, please let me know and i will try to fix it so the while thing can be done automatically. julia > > (Up to now it's only > > @@ > struct drm_crtc *crtc; > @@ > -crtc->dev > +crtc->drm_dev > > ) > > > Background is that this makes merge conflicts easier to handle and detect. > > Really? Each file (apart from include/drm/drm_crtc.h) is only touched > once. So unless I'm missing something you don't get less or easier > conflicts by doing it all in a single patch. But you gain the freedom to > drop a patch for one driver without having to drop the rest with it. So > I still like the split version better, but I'm open to a more verbose > reasoning from your side. > > > When you have multiple patches and a merge conflict because of some added > > lines using the old field the build breaks only on the last patch which > > removes the old field. > > Then you can revert/drop the last patch without having to respin the > whole single patch and thus caring for still more conflicts that arise > until the new version is sent. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König| > Industrial Linux Solutions | https://www.pengutronix.de/ | >
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
> Hello Jani,
>
> On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
>> On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
>> > Hello,
>> >
>> > while I debugged an issue in the imx-lcdc driver I was constantly
>> > irritated about struct drm_device pointer variables being named "dev"
>> > because with that name I usually expect a struct device pointer.
>> >
>> > I think there is a big benefit when these are all renamed to "drm_dev".
>> > I have no strong preference here though, so "drmdev" or "drm" are fine
>> > for me, too. Let the bikesheding begin!
>> >
>> > Some statistics:
>> >
>> > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq
>> > -c | sort -n
>> > 1 struct drm_device *adev_to_drm
>> > 1 struct drm_device *drm_
>> > 1 struct drm_device *drm_dev
>> > 1 struct drm_device*drm_dev
>> > 1 struct drm_device *pdev
>> > 1 struct drm_device *rdev
>> > 1 struct drm_device *vdev
>> > 2 struct drm_device *dcss_drv_dev_to_drm
>> > 2 struct drm_device **ddev
>> > 2 struct drm_device *drm_dev_alloc
>> > 2 struct drm_device *mock
>> > 2 struct drm_device *p_ddev
>> > 5 struct drm_device *device
>> > 9 struct drm_device * dev
>> > 25 struct drm_device *d
>> > 95 struct drm_device *
>> > 216 struct drm_device *ddev
>> > 234 struct drm_device *drm_dev
>> > 611 struct drm_device *drm
>> >4190 struct drm_device *dev
>> >
>> > This series starts with renaming struct drm_crtc::dev to drm_dev. If
>> > it's not only me and others like the result of this effort it should be
>> > followed up by adapting the other structs and the individual usages in
>> > the different drivers.
>>
>> I think this is an unnecessary change. In drm, a dev is usually a drm
>> device, i.e. struct drm_device *.
>
> Well, unless it's not. Prominently there is
>
> struct drm_device {
> ...
> struct device *dev;
> ...
> };
>
> which yields quite a few code locations using dev->dev which is
> IMHO unnecessary irritating:
>
> $ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
> 1633
>
> Also the functions that deal with both a struct device and a struct
> drm_device often use "dev" for the struct device and then "ddev" for
> the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
Why is specifically struct drm_device *dev so irritating to you?
You lead us to believe it's an outlier in kernel, something that goes
against common kernel style, but it's really not:
$ git grep -how "struct [A-Za-z0-9_]\+ \*dev" | sort | uniq -c | sort -rn |
head -20
38494 struct device *dev
16388 struct net_device *dev
4184 struct drm_device *dev
2780 struct pci_dev *dev
1916 struct comedi_device *dev
1510 struct mlx5_core_dev *dev
1057 struct mlx4_dev *dev
894 struct b43_wldev *dev
762 struct input_dev *dev
623 struct usbnet *dev
561 struct mlx5_ib_dev *dev
525 struct mt76_dev *dev
465 struct mt76x02_dev *dev
435 struct platform_device *dev
431 struct usb_device *dev
411 struct mt7915_dev *dev
398 struct cx231xx *dev
378 struct mei_device *dev
363 struct ksz_device *dev
359 struct mthca_dev *dev
A good portion of the above also have a dev member.
Are you planning on changing all of the above too, or are you only
annoyed by drm?
I'm really not convinced at all.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Thu, Jul 13, 2023 at 08:52:12AM +0200, Geert Uytterhoeven wrote:
> Hi Uwe,
>
> Let's add some fuel to keep the thread alive ;-)
>
> On Wed, Jul 12, 2023 at 6:13 PM Uwe Kleine-König
> wrote:
> > On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
> > > I think this is an unnecessary change. In drm, a dev is usually a drm
> > > device, i.e. struct drm_device *.
> >
> > Well, unless it's not. Prominently there is
> >
> > struct drm_device {
> > ...
> > struct device *dev;
> > ...
> > };
> >
> > which yields quite a few code locations using dev->dev which is
> > IMHO unnecessary irritating:
> >
> > $ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
> > 1633
>
> I find that irritating as well...
>
> Same for e.g. crtc->crtc.
>
> Hence that's why I had sent patches to rename the base members in the
> shmob_drm-specific subclasses of drm_{crtc,connector,plane} to "base".
> https://lore.kernel.org/dri-devel/b3daca80f82625ba14e3aeaf2fca6dcefa056e47.1687423204.git.geert+rene...@glider.be
>
> > Also the functions that deal with both a struct device and a struct
> > drm_device often use "dev" for the struct device and then "ddev" for
> > the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
>
> I guess you considered "drm_dev", because it is still a short name?
I considered drm_dev because it is still moderately short and a good
approximation of "drm_device". Other than that the main driving force to
pick "drm_dev" was that it's unique enough that I could have done
s/\/$nameofchoice/ on the initial patch and get it mostly
right.
> Code dealing with platform devices usually uses "pdev" and "dev".
> Same for PCI drivers (despite "pci_dev" being a short name).
pci_dev and platform_device both typlically using pdev already annoyed
me in the past. However less than drm_device *dev because for pci_dev +
platform_device there is little overlap.
> So my personal preference goes to "ddev".
I sticked to "drm" for the new series. I think this provides less fuel.
Best regards and thanks for your thoughts,
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Thu, Jul 13, 2023 at 12:03:05PM +0300, Jani Nikula wrote:
> On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
> > Hello Jani,
> >
> > On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
> >> On Wed, 12 Jul 2023, Uwe Kleine-König
> >> wrote:
> >> > Hello,
> >> >
> >> > while I debugged an issue in the imx-lcdc driver I was constantly
> >> > irritated about struct drm_device pointer variables being named "dev"
> >> > because with that name I usually expect a struct device pointer.
> >> >
> >> > I think there is a big benefit when these are all renamed to "drm_dev".
> >> > I have no strong preference here though, so "drmdev" or "drm" are fine
> >> > for me, too. Let the bikesheding begin!
> >> >
> >> > Some statistics:
> >> >
> >> > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort |
> >> > uniq -c | sort -n
> >> > 1 struct drm_device *adev_to_drm
> >> > 1 struct drm_device *drm_
> >> > 1 struct drm_device *drm_dev
> >> > 1 struct drm_device*drm_dev
> >> > 1 struct drm_device *pdev
> >> > 1 struct drm_device *rdev
> >> > 1 struct drm_device *vdev
> >> > 2 struct drm_device *dcss_drv_dev_to_drm
> >> > 2 struct drm_device **ddev
> >> > 2 struct drm_device *drm_dev_alloc
> >> > 2 struct drm_device *mock
> >> > 2 struct drm_device *p_ddev
> >> > 5 struct drm_device *device
> >> > 9 struct drm_device * dev
> >> > 25 struct drm_device *d
> >> > 95 struct drm_device *
> >> > 216 struct drm_device *ddev
> >> > 234 struct drm_device *drm_dev
> >> > 611 struct drm_device *drm
> >> >4190 struct drm_device *dev
> >> >
> >> > This series starts with renaming struct drm_crtc::dev to drm_dev. If
> >> > it's not only me and others like the result of this effort it should be
> >> > followed up by adapting the other structs and the individual usages in
> >> > the different drivers.
> >>
> >> I think this is an unnecessary change. In drm, a dev is usually a drm
> >> device, i.e. struct drm_device *.
> >
> > Well, unless it's not. Prominently there is
> >
> > struct drm_device {
> > ...
> > struct device *dev;
> > ...
> > };
> >
> > which yields quite a few code locations using dev->dev which is
> > IMHO unnecessary irritating:
> >
> > $ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
> > 1633
> >
> > Also the functions that deal with both a struct device and a struct
> > drm_device often use "dev" for the struct device and then "ddev" for
> > the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
>
> Why is specifically struct drm_device *dev so irritating to you?
>
> You lead us to believe it's an outlier in kernel, something that goes
> against common kernel style, but it's really not:
>
> $ git grep -how "struct [A-Za-z0-9_]\+ \*dev" | sort | uniq -c | sort -rn |
> head -20
> 38494 struct device *dev
> 16388 struct net_device *dev
>4184 struct drm_device *dev
>2780 struct pci_dev *dev
>1916 struct comedi_device *dev
>1510 struct mlx5_core_dev *dev
>1057 struct mlx4_dev *dev
> 894 struct b43_wldev *dev
> 762 struct input_dev *dev
> 623 struct usbnet *dev
> 561 struct mlx5_ib_dev *dev
> 525 struct mt76_dev *dev
> 465 struct mt76x02_dev *dev
> 435 struct platform_device *dev
> 431 struct usb_device *dev
> 411 struct mt7915_dev *dev
> 398 struct cx231xx *dev
> 378 struct mei_device *dev
> 363 struct ksz_device *dev
> 359 struct mthca_dev *dev
>
> A good portion of the above also have a dev member.
Yeah, other subsystems and drivers have the same problem. You're lucky
that I noticed drm first and invested some effort to improve it. IMHO
other subsystems being bad shouldn't stop drm to improve here.
And note that for example for pci_dev there are 5794 instances that are
named "pdev" and there are 9971 struct platform_device that are called
"pdev", too. So the majority for these does it better. And agreed,
net_device and others are also inconsistent. If you want an area that is
better, look at the naming of i2c_client or spi_device. (And take into
account that these are spread all over the tree and so are not in
control of a single maintainer team.)
> Are you planning on changing all of the above too, or are you only
> annoyed by drm?
Would you be more welcoming if I promised to tackle some of the above,
too? If so: I might. I hesitate a bit because I didn't suffer from the
others. (Apart from asking ctags for "dev" is a nightmare.)
And regarding the second part of your question: I was annoyed by drm
because that was the one that offended me while researching a problem in
a drm driver. And the variable/struct member name irritated me enough to
believe that with consistent naming I would have found it quicker.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hi Am 12.07.23 um 11:46 schrieb Uwe Kleine-König: Hello, while I debugged an issue in the imx-lcdc driver I was constantly irritated about struct drm_device pointer variables being named "dev" because with that name I usually expect a struct device pointer. I think there is a big benefit when these are all renamed to "drm_dev". If you rename drm_crtc.dev, you should also address *all* other data structures. I have no strong preference here though, so "drmdev" or "drm" are fine for me, too. Let the bikesheding begin! We've discussed this to death. IIRC 'drm' would be the prefered choice. Best regards Thomas Some statistics: $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | sort -n 1 struct drm_device *adev_to_drm 1 struct drm_device *drm_ 1 struct drm_device *drm_dev 1 struct drm_device*drm_dev 1 struct drm_device *pdev 1 struct drm_device *rdev 1 struct drm_device *vdev 2 struct drm_device *dcss_drv_dev_to_drm 2 struct drm_device **ddev 2 struct drm_device *drm_dev_alloc 2 struct drm_device *mock 2 struct drm_device *p_ddev 5 struct drm_device *device 9 struct drm_device * dev 25 struct drm_device *d 95 struct drm_device * 216 struct drm_device *ddev 234 struct drm_device *drm_dev 611 struct drm_device *drm 4190 struct drm_device *dev This series starts with renaming struct drm_crtc::dev to drm_dev. If it's not only me and others like the result of this effort it should be followed up by adapting the other structs and the individual usages in the different drivers. To make this series a bit easier handleable, I first added an alias for drm_crtc::dev, then converted the drivers one after another and the last patch drops the "dev" name. This has the advantage of being easier to review, and if I should have missed an instance only the last patch must be dropped/reverted. Also this series might conflict with other patches, in this case the remaining patches can still go in (apart from the last one of course). Maybe it also makes sense to delay applying the last patch by one development cycle? The series was compile tested for arm, arm64, powerpc and amd64 using an allmodconfig (though I only build drivers/gpu/). Best regards Uwe Uwe Kleine-König (52): drm/crtc: Start renaming struct drm_crtc::dev to drm_dev drm/core: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/amd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/armada: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/arm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/aspeed: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/ast: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/atmel-hlcdc: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/exynos: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/fsl-dcu: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/gma500: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/gud: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/hisilicon: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/hyperv: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/i915: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/imx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/ingenic: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/kmb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/logicvc: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/mcde: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/mediatek: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/meson: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/mgag200: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/msm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/mxsfb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/nouveau: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/omapdrm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/panel-ili9341: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/pl111: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/qxl: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/radeon: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/renesas: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/rockchip: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/solomon: Use
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hi Uwe, On Wed 12 Jul 23, 11:46, Uwe Kleine-König wrote: > Hello, > > while I debugged an issue in the imx-lcdc driver I was constantly > irritated about struct drm_device pointer variables being named "dev" > because with that name I usually expect a struct device pointer. Well personally I usually expect that the "dev" member of a subsystem-specific struct refers to a device of that subsystem, so for me having "dev" refer to a drm_device for e.g. drm_crtc makes good sense. I would only expect dev to refer to a struct device in the subsystem-specific device structure (drm_device). I don't think it makes much sense to carry the struct device in any other subsystem-specific structure anyway. So IMO things are fine as-is but this is not a very strong opinion either. > I think there is a big benefit when these are all renamed to "drm_dev". > I have no strong preference here though, so "drmdev" or "drm" are fine > for me, too. Let the bikesheding begin! I would definitely prefer "drm_dev" over "drmdev" (hard to read, feels like aborted camelcase, pretty ugly) or "drm" (too vague). Cheers, Paul > Some statistics: > > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c > | sort -n > 1 struct drm_device *adev_to_drm > 1 struct drm_device *drm_ > 1 struct drm_device *drm_dev > 1 struct drm_device*drm_dev > 1 struct drm_device *pdev > 1 struct drm_device *rdev > 1 struct drm_device *vdev > 2 struct drm_device *dcss_drv_dev_to_drm > 2 struct drm_device **ddev > 2 struct drm_device *drm_dev_alloc > 2 struct drm_device *mock > 2 struct drm_device *p_ddev > 5 struct drm_device *device > 9 struct drm_device * dev > 25 struct drm_device *d > 95 struct drm_device * > 216 struct drm_device *ddev > 234 struct drm_device *drm_dev > 611 struct drm_device *drm >4190 struct drm_device *dev > > This series starts with renaming struct drm_crtc::dev to drm_dev. If > it's not only me and others like the result of this effort it should be > followed up by adapting the other structs and the individual usages in > the different drivers. > > To make this series a bit easier handleable, I first added an alias for > drm_crtc::dev, then converted the drivers one after another and the last > patch drops the "dev" name. This has the advantage of being easier to > review, and if I should have missed an instance only the last patch must > be dropped/reverted. Also this series might conflict with other patches, > in this case the remaining patches can still go in (apart from the last > one of course). Maybe it also makes sense to delay applying the last > patch by one development cycle? > > The series was compile tested for arm, arm64, powerpc and amd64 using an > allmodconfig (though I only build drivers/gpu/). > > Best regards > Uwe > > Uwe Kleine-König (52): > drm/crtc: Start renaming struct drm_crtc::dev to drm_dev > drm/core: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/amd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/armada: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/arm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/aspeed: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/ast: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/atmel-hlcdc: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/exynos: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/fsl-dcu: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/gma500: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/gud: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/hisilicon: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/hyperv: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/i915: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/imx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/ingenic: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/kmb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/logicvc: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/mcde: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/mediatek: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/meson: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/mgag200: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/msm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/mxsfb: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/nouveau: Use struct drm_crtc::drm_dev instead of struct >
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote: > > Background is that this makes merge conflicts easier to handle and detect. > > Really? FWIW, I agree with Christian here. > Each file (apart from include/drm/drm_crtc.h) is only touched once. So > unless I'm missing something you don't get less or easier conflicts by > doing it all in a single patch. But you gain the freedom to drop a > patch for one driver without having to drop the rest with it. Not really, because the last patch removed the union anyway. So you have to revert both the last patch, plus that driver one. And then you need to add a TODO to remove that union eventually. > So I still like the split version better, but I'm open to a more > verbose reasoning from your side. You're doing only one thing here, really: you change the name of a structure field. If it was shared between multiple maintainers, then sure, splitting that up is easier for everyone, but this will go through drm-misc, so I can't see the benefit it brings. Maxime signature.asc Description: PGP signature
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Wed, Jul 12, 2023 at 12:46:33PM +0200, Christian König wrote: > Am 12.07.23 um 11:46 schrieb Uwe Kleine-König: > > Hello, > > > > while I debugged an issue in the imx-lcdc driver I was constantly > > irritated about struct drm_device pointer variables being named "dev" > > because with that name I usually expect a struct device pointer. > > > > I think there is a big benefit when these are all renamed to "drm_dev". > > I have no strong preference here though, so "drmdev" or "drm" are fine > > for me, too. Let the bikesheding begin! > > > > Some statistics: > > > > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq > > -c | sort -n > >1 struct drm_device *adev_to_drm > >1 struct drm_device *drm_ > >1 struct drm_device *drm_dev > >1 struct drm_device*drm_dev > >1 struct drm_device *pdev > >1 struct drm_device *rdev > >1 struct drm_device *vdev > >2 struct drm_device *dcss_drv_dev_to_drm > >2 struct drm_device **ddev > >2 struct drm_device *drm_dev_alloc > >2 struct drm_device *mock > >2 struct drm_device *p_ddev > >5 struct drm_device *device > >9 struct drm_device * dev > > 25 struct drm_device *d > > 95 struct drm_device * > > 216 struct drm_device *ddev > > 234 struct drm_device *drm_dev > > 611 struct drm_device *drm > > 4190 struct drm_device *dev > > > > This series starts with renaming struct drm_crtc::dev to drm_dev. If > > it's not only me and others like the result of this effort it should be > > followed up by adapting the other structs and the individual usages in > > the different drivers. > > > > To make this series a bit easier handleable, I first added an alias for > > drm_crtc::dev, then converted the drivers one after another and the last > > patch drops the "dev" name. This has the advantage of being easier to > > review, and if I should have missed an instance only the last patch must > > be dropped/reverted. Also this series might conflict with other patches, > > in this case the remaining patches can still go in (apart from the last > > one of course). Maybe it also makes sense to delay applying the last > > patch by one development cycle? > > When you automatically generate the patch (with cocci for example) I usually > prefer a single patch instead. Maybe I'm too stupid, but only parts of this patch were created by coccinelle. I failed to convert code like - spin_lock_irq(>dev->event_lock); + spin_lock_irq(>drm_dev->event_lock); Added Julia to Cc, maybe she has a hint?! (Up to now it's only @@ struct drm_crtc *crtc; @@ -crtc->dev +crtc->drm_dev ) > Background is that this makes merge conflicts easier to handle and detect. Really? Each file (apart from include/drm/drm_crtc.h) is only touched once. So unless I'm missing something you don't get less or easier conflicts by doing it all in a single patch. But you gain the freedom to drop a patch for one driver without having to drop the rest with it. So I still like the split version better, but I'm open to a more verbose reasoning from your side. > When you have multiple patches and a merge conflict because of some added > lines using the old field the build breaks only on the last patch which > removes the old field. Then you can revert/drop the last patch without having to respin the whole single patch and thus caring for still more conflicts that arise until the new version is sent. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hi
Am 12.07.23 um 18:10 schrieb Uwe Kleine-König:
Hello Jani,
On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
Hello,
while I debugged an issue in the imx-lcdc driver I was constantly
irritated about struct drm_device pointer variables being named "dev"
because with that name I usually expect a struct device pointer.
I think there is a big benefit when these are all renamed to "drm_dev".
I have no strong preference here though, so "drmdev" or "drm" are fine
for me, too. Let the bikesheding begin!
Some statistics:
$ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c |
sort -n
1 struct drm_device *adev_to_drm
1 struct drm_device *drm_
1 struct drm_device *drm_dev
1 struct drm_device*drm_dev
1 struct drm_device *pdev
1 struct drm_device *rdev
1 struct drm_device *vdev
2 struct drm_device *dcss_drv_dev_to_drm
2 struct drm_device **ddev
2 struct drm_device *drm_dev_alloc
2 struct drm_device *mock
2 struct drm_device *p_ddev
5 struct drm_device *device
9 struct drm_device * dev
25 struct drm_device *d
95 struct drm_device *
216 struct drm_device *ddev
234 struct drm_device *drm_dev
611 struct drm_device *drm
4190 struct drm_device *dev
This series starts with renaming struct drm_crtc::dev to drm_dev. If
it's not only me and others like the result of this effort it should be
followed up by adapting the other structs and the individual usages in
the different drivers.
I think this is an unnecessary change. In drm, a dev is usually a drm
device, i.e. struct drm_device *.
Well, unless it's not. Prominently there is
struct drm_device {
...
struct device *dev;
...
};
Jani's point is that it's only inconvenient at the first time. Everyone
gets use to it.
Best regards
Thomas
which yields quite a few code locations using dev->dev which is
IMHO unnecessary irritating:
$ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
1633
Also the functions that deal with both a struct device and a struct
drm_device often use "dev" for the struct device and then "ddev" for
the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
If folks insist on following through with this anyway, I'm firmly in the
camp the name should be "drm" and nothing else.
Up to now positive feedback is in the majority.
Best regards
Uwe
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
OpenPGP_signature
Description: OpenPGP digital signature
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hi Uwe,
Let's add some fuel to keep the thread alive ;-)
On Wed, Jul 12, 2023 at 6:13 PM Uwe Kleine-König
wrote:
> On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
> > On Wed, 12 Jul 2023, Uwe Kleine-König
> > wrote:
> > > Hello,
> > >
> > > while I debugged an issue in the imx-lcdc driver I was constantly
> > > irritated about struct drm_device pointer variables being named "dev"
> > > because with that name I usually expect a struct device pointer.
> > >
> > > I think there is a big benefit when these are all renamed to "drm_dev".
> > > I have no strong preference here though, so "drmdev" or "drm" are fine
> > > for me, too. Let the bikesheding begin!
> > >
> > > Some statistics:
> > >
> > > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq
> > > -c | sort -n
> > > 1 struct drm_device *adev_to_drm
> > > 1 struct drm_device *drm_
> > > 1 struct drm_device *drm_dev
> > > 1 struct drm_device*drm_dev
> > > 1 struct drm_device *pdev
> > > 1 struct drm_device *rdev
> > > 1 struct drm_device *vdev
> > > 2 struct drm_device *dcss_drv_dev_to_drm
> > > 2 struct drm_device **ddev
> > > 2 struct drm_device *drm_dev_alloc
> > > 2 struct drm_device *mock
> > > 2 struct drm_device *p_ddev
> > > 5 struct drm_device *device
> > > 9 struct drm_device * dev
> > > 25 struct drm_device *d
> > > 95 struct drm_device *
> > > 216 struct drm_device *ddev
> > > 234 struct drm_device *drm_dev
> > > 611 struct drm_device *drm
> > >4190 struct drm_device *dev
> > >
> > > This series starts with renaming struct drm_crtc::dev to drm_dev. If
> > > it's not only me and others like the result of this effort it should be
> > > followed up by adapting the other structs and the individual usages in
> > > the different drivers.
> >
> > I think this is an unnecessary change. In drm, a dev is usually a drm
> > device, i.e. struct drm_device *.
>
> Well, unless it's not. Prominently there is
>
> struct drm_device {
> ...
> struct device *dev;
> ...
> };
>
> which yields quite a few code locations using dev->dev which is
> IMHO unnecessary irritating:
>
> $ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
> 1633
I find that irritating as well...
Same for e.g. crtc->crtc.
Hence that's why I had sent patches to rename the base members in the
shmob_drm-specific subclasses of drm_{crtc,connector,plane} to "base".
https://lore.kernel.org/dri-devel/b3daca80f82625ba14e3aeaf2fca6dcefa056e47.1687423204.git.geert+rene...@glider.be
> Also the functions that deal with both a struct device and a struct
> drm_device often use "dev" for the struct device and then "ddev" for
> the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
I guess you considered "drm_dev", because it is still a short name?
Code dealing with platform devices usually uses "pdev" and "dev".
Same for PCI drivers (despite "pci_dev" being a short name).
So my personal preference goes to "ddev".
EOF (End-of-Fuel ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On 2023-07-12 09:53, Christian König wrote: > Am 12.07.23 um 15:38 schrieb Uwe Kleine-König: >> Hello Maxime, >> >> On Wed, Jul 12, 2023 at 02:52:38PM +0200, Maxime Ripard wrote: >>> On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote: > Background is that this makes merge conflicts easier to handle and detect. Really? >>> FWIW, I agree with Christian here. >>> Each file (apart from include/drm/drm_crtc.h) is only touched once. So unless I'm missing something you don't get less or easier conflicts by doing it all in a single patch. But you gain the freedom to drop a patch for one driver without having to drop the rest with it. >>> Not really, because the last patch removed the union anyway. So you have >>> to revert both the last patch, plus that driver one. And then you need >>> to add a TODO to remove that union eventually. >> Yes, with a single patch you have only one revert (but 194 files changed, >> 1264 insertions(+), 1296 deletions(-)) instead of two (one of them: 1 >> file changed, 9 insertions(+), 1 deletion(-); the other maybe a bit >> bigger). (And maybe you get away with just reverting the last patch.) >> >> With a single patch the TODO after a revert is "redo it all again (and >> prepare for a different set of conflicts)" while with the split series >> it's only "fix that one driver that was forgotten/borked" + reapply that >> 10 line patch. > > Yeah, but for a maintainer the size of the patches doesn't matter. > That's only interesting if you need to manually review the patch, which > you hopefully doesn't do in case of something auto-generated. > > In other words if the patch is auto-generated re-applying it completely > is less work than fixing things up individually. > >> As the one who gets that TODO, I prefer the latter. > > Yeah, but your personal preferences are not a technical relevant > argument to a maintainer. > > At the end of the day Dave or Daniel need to decide, because they need > to live with it. > > Regards, > Christian. > >> >> So in sum: If your metric is "small count of reverted commits", you're >> right. If however your metric is: Better get 95% of this series' change >> in than maybe 0%, the split series is the way to do it. >> >> With me having spend ~3h on this series' changes, it's maybe >> understandable that I did it the way I did. >> >> FTR: This series was created on top of v6.5-rc1. If you apply it to >> drm-misc-next you get a (trivial) conflict in patch #2. If I consider to >> be the responsible maintainer who applies this series, I like being able >> to just do git am --skip then. >> >> FTR#2: In drm-misc-next is a new driver >> (drivers/gpu/drm/loongson/lsdc_crtc.c) so skipping the last patch for >> now might indeed be a good idea. >> So I still like the split version better, but I'm open to a more verbose reasoning from your side. >>> You're doing only one thing here, really: you change the name of a >>> structure field. If it was shared between multiple maintainers, then >>> sure, splitting that up is easier for everyone, but this will go through >>> drm-misc, so I can't see the benefit it brings. >> I see your argument, but I think mine weights more. I'm with Maxime and Christian on this--a single action necessitates a single patch. One single movement. As Maxime said "either 0 or 100." As to the name, perhaps "drm_dev" is more descriptive than just "drm". What is "drm"? Ah it's a "dev", as in "drm dev"... Then why not rename it to "drm_dev"? You are renaming it from "dev" to something more descriptive after all. "dev" --> "drm" is no better, but "dev" --> "drm_dev" is just right. -- Regards, Luben
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Am 12.07.23 um 11:46 schrieb Uwe Kleine-König: Hello, while I debugged an issue in the imx-lcdc driver I was constantly irritated about struct drm_device pointer variables being named "dev" because with that name I usually expect a struct device pointer. I think there is a big benefit when these are all renamed to "drm_dev". I have no strong preference here though, so "drmdev" or "drm" are fine for me, too. Let the bikesheding begin! Some statistics: $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | sort -n 1 struct drm_device *adev_to_drm 1 struct drm_device *drm_ 1 struct drm_device *drm_dev 1 struct drm_device*drm_dev 1 struct drm_device *pdev 1 struct drm_device *rdev 1 struct drm_device *vdev 2 struct drm_device *dcss_drv_dev_to_drm 2 struct drm_device **ddev 2 struct drm_device *drm_dev_alloc 2 struct drm_device *mock 2 struct drm_device *p_ddev 5 struct drm_device *device 9 struct drm_device * dev 25 struct drm_device *d 95 struct drm_device * 216 struct drm_device *ddev 234 struct drm_device *drm_dev 611 struct drm_device *drm 4190 struct drm_device *dev This series starts with renaming struct drm_crtc::dev to drm_dev. If it's not only me and others like the result of this effort it should be followed up by adapting the other structs and the individual usages in the different drivers. To make this series a bit easier handleable, I first added an alias for drm_crtc::dev, then converted the drivers one after another and the last patch drops the "dev" name. This has the advantage of being easier to review, and if I should have missed an instance only the last patch must be dropped/reverted. Also this series might conflict with other patches, in this case the remaining patches can still go in (apart from the last one of course). Maybe it also makes sense to delay applying the last patch by one development cycle? When you automatically generate the patch (with cocci for example) I usually prefer a single patch instead. Background is that this makes merge conflicts easier to handle and detect. When you have multiple patches and a merge conflict because of some added lines using the old field the build breaks only on the last patch which removes the old field. In such cases reviewing the patch just means automatically re-generating it and double checking that you don't see anything funky. Apart from that I honestly absolutely don't care what the name is. Cheers, Christian. The series was compile tested for arm, arm64, powerpc and amd64 using an allmodconfig (though I only build drivers/gpu/). Best regards Uwe Uwe Kleine-König (52): drm/crtc: Start renaming struct drm_crtc::dev to drm_dev drm/core: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/amd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/armada: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/arm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/aspeed: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/ast: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/atmel-hlcdc: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/exynos: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/fsl-dcu: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/gma500: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/gud: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/hisilicon: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/hyperv: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/i915: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/imx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/ingenic: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/kmb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/logicvc: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/mcde: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/mediatek: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/meson: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/mgag200: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/msm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/mxsfb: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/nouveau: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/omapdrm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/panel-ili9341: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/pl111: Use struct
Re: [Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Am 12.07.23 um 15:38 schrieb Uwe Kleine-König: Hello Maxime, On Wed, Jul 12, 2023 at 02:52:38PM +0200, Maxime Ripard wrote: On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote: Background is that this makes merge conflicts easier to handle and detect. Really? FWIW, I agree with Christian here. Each file (apart from include/drm/drm_crtc.h) is only touched once. So unless I'm missing something you don't get less or easier conflicts by doing it all in a single patch. But you gain the freedom to drop a patch for one driver without having to drop the rest with it. Not really, because the last patch removed the union anyway. So you have to revert both the last patch, plus that driver one. And then you need to add a TODO to remove that union eventually. Yes, with a single patch you have only one revert (but 194 files changed, 1264 insertions(+), 1296 deletions(-)) instead of two (one of them: 1 file changed, 9 insertions(+), 1 deletion(-); the other maybe a bit bigger). (And maybe you get away with just reverting the last patch.) With a single patch the TODO after a revert is "redo it all again (and prepare for a different set of conflicts)" while with the split series it's only "fix that one driver that was forgotten/borked" + reapply that 10 line patch. Yeah, but for a maintainer the size of the patches doesn't matter. That's only interesting if you need to manually review the patch, which you hopefully doesn't do in case of something auto-generated. In other words if the patch is auto-generated re-applying it completely is less work than fixing things up individually. As the one who gets that TODO, I prefer the latter. Yeah, but your personal preferences are not a technical relevant argument to a maintainer. At the end of the day Dave or Daniel need to decide, because they need to live with it. Regards, Christian. So in sum: If your metric is "small count of reverted commits", you're right. If however your metric is: Better get 95% of this series' change in than maybe 0%, the split series is the way to do it. With me having spend ~3h on this series' changes, it's maybe understandable that I did it the way I did. FTR: This series was created on top of v6.5-rc1. If you apply it to drm-misc-next you get a (trivial) conflict in patch #2. If I consider to be the responsible maintainer who applies this series, I like being able to just do git am --skip then. FTR#2: In drm-misc-next is a new driver (drivers/gpu/drm/loongson/lsdc_crtc.c) so skipping the last patch for now might indeed be a good idea. So I still like the split version better, but I'm open to a more verbose reasoning from your side. You're doing only one thing here, really: you change the name of a structure field. If it was shared between multiple maintainers, then sure, splitting that up is easier for everyone, but this will go through drm-misc, so I can't see the benefit it brings. I see your argument, but I think mine weights more. Best regards Uwe
