Nouveau - Jul 2023 - [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

On Wed, 12 Jul 2023, Uwe Kleine-K?nig <u.kleine-koenig at pengutronix.de>
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

Hello Jani,

On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula
wrote:
> On Wed, 12 Jul 2023, Uwe Kleine-K?nig <u.kleine-koenig at
pengutronix.de> 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->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/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20230712/317276cb/attachment-0001.sig>

On Wed, Jul 12, 2023 at 10:52?AM Jani Nikula <jani.nikula at intel.com>
wrote:
>
> On Wed, 12 Jul 2023, Uwe Kleine-K?nig <u.kleine-koenig at
pengutronix.de> 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.
>
I'd really prefer this patch (series or single) is not accepted. This
will cause problems for everyone cherry-picking patches to a
downstream kernel (LTS or distro tree). I usually wouldn't expect
sympathy here, but the questionable benefit does not outweigh the cost
IM[biased]O.

Sean
> 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

On Wed, 12 Jul 2023, Uwe Kleine-K?nig <u.kleine-koenig at pengutronix.de>
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 <u.kleine-koenig at
pengutronix.de> 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->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

hello Sean,

On Wed, Jul 12, 2023 at 02:31:02PM -0400, Sean Paul
wrote:
> I'd really prefer this patch (series or single) is not accepted. This
> will cause problems for everyone cherry-picking patches to a
> downstream kernel (LTS or distro tree). I usually wouldn't expect
> sympathy here, but the questionable benefit does not outweigh the cost
> IM[biased]O.
I agree that for backports this isn't so nice. However with the split
approach (that was argumented against here) it's not soo bad. Patch #1
(and similar changes for the other affected structures) could be
trivially backported and with that it doesn't matter if you write dev or
drm (or whatever name is chosen in the end); both work in the same way.

But even with the one-patch-per-rename approach I'd consider the
renaming a net win, because ease of understanding code has a big value.
It's value is not so easy measurable as "conflicts when
backporting",
but it also matters in say two years from now, while backporting
shouldn't be an issue then any more.

Thanks for your input, best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20230713/c890c45b/attachment-0001.sig>