| Message ID | 20230712094702.1770121-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/crtc: Rename struct drm_crtc::dev to drm_dev | expand |
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 > 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 struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/sprd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/sti: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/stm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/sun4i: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/tegra: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/tidss: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/tilcdc: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/tiny: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/tve200: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/udl: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/vboxvideo: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/vc4: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/virtio: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/vkms: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/vmwgfx: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/xen: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/xlnx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/crtc: Complete renaming struct drm_crtc::dev to drm_dev > > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 18 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 6 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_pll.c | 6 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c | 8 +- > drivers/gpu/drm/amd/amdgpu/atombios_crtc.c | 22 +-- > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 26 +-- > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 28 ++-- > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 26 +-- > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 26 +-- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 29 ++-- > .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 20 +-- > .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 8 +- > .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 22 +-- > .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 2 +- > .../gpu/drm/arm/display/komeda/komeda_crtc.c | 24 +-- > .../gpu/drm/arm/display/komeda/komeda_kms.c | 2 +- > drivers/gpu/drm/arm/hdlcd_crtc.c | 4 +- > drivers/gpu/drm/arm/malidp_crtc.c | 7 +- > drivers/gpu/drm/armada/armada_crtc.c | 10 +- > drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c | 6 +- > drivers/gpu/drm/ast/ast_dp.c | 2 +- > drivers/gpu/drm/ast/ast_mode.c | 26 +-- > .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 10 +- > drivers/gpu/drm/drm_atomic.c | 22 +-- > drivers/gpu/drm/drm_atomic_helper.c | 20 ++- > drivers/gpu/drm/drm_atomic_state_helper.c | 2 +- > drivers/gpu/drm/drm_atomic_uapi.c | 22 +-- > drivers/gpu/drm/drm_blend.c | 2 +- > drivers/gpu/drm/drm_color_mgmt.c | 10 +- > drivers/gpu/drm/drm_crtc.c | 19 ++- > drivers/gpu/drm/drm_crtc_helper.c | 10 +- > drivers/gpu/drm/drm_debugfs.c | 2 +- > drivers/gpu/drm/drm_debugfs_crc.c | 2 +- > drivers/gpu/drm/drm_fb_helper.c | 6 +- > drivers/gpu/drm/drm_mipi_dbi.c | 4 +- > drivers/gpu/drm/drm_plane.c | 2 +- > drivers/gpu/drm/drm_plane_helper.c | 2 +- > drivers/gpu/drm/drm_self_refresh_helper.c | 2 +- > drivers/gpu/drm/drm_vblank.c | 40 ++--- > drivers/gpu/drm/drm_vblank_work.c | 2 +- > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 8 +- > drivers/gpu/drm/exynos/exynos_drm_plane.c | 4 +- > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 16 +- > drivers/gpu/drm/gma500/cdv_intel_display.c | 2 +- > drivers/gpu/drm/gma500/cdv_intel_dp.c | 2 +- > drivers/gpu/drm/gma500/gma_display.c | 20 +-- > drivers/gpu/drm/gma500/oaktrail_crtc.c | 8 +- > drivers/gpu/drm/gma500/oaktrail_hdmi.c | 4 +- > drivers/gpu/drm/gma500/psb_intel_display.c | 2 +- > drivers/gpu/drm/gma500/psb_irq.c | 6 +- > drivers/gpu/drm/gud/gud_pipe.c | 6 +- > .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 20 +-- > .../gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 4 +- > drivers/gpu/drm/hyperv/hyperv_drm_modeset.c | 6 +- > drivers/gpu/drm/i915/display/g4x_dp.c | 4 +- > drivers/gpu/drm/i915/display/hsw_ips.c | 16 +- > drivers/gpu/drm/i915/display/i9xx_plane.c | 4 +- > drivers/gpu/drm/i915/display/i9xx_wm.c | 40 ++--- > drivers/gpu/drm/i915/display/icl_dsi.c | 2 +- > drivers/gpu/drm/i915/display/intel_atomic.c | 2 +- > .../gpu/drm/i915/display/intel_atomic_plane.c | 4 +- > drivers/gpu/drm/i915/display/intel_audio.c | 2 +- > drivers/gpu/drm/i915/display/intel_bw.c | 10 +- > drivers/gpu/drm/i915/display/intel_cdclk.c | 6 +- > drivers/gpu/drm/i915/display/intel_color.c | 124 +++++++------- > drivers/gpu/drm/i915/display/intel_crtc.c | 20 +-- > .../drm/i915/display/intel_crtc_state_dump.c | 4 +- > drivers/gpu/drm/i915/display/intel_cursor.c | 2 +- > drivers/gpu/drm/i915/display/intel_ddi.c | 28 ++-- > drivers/gpu/drm/i915/display/intel_display.c | 154 +++++++++--------- > .../gpu/drm/i915/display/intel_display_irq.c | 22 +-- > .../gpu/drm/i915/display/intel_display_rps.c | 2 +- > .../drm/i915/display/intel_display_trace.h | 12 +- > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > drivers/gpu/drm/i915/display/intel_dpll.c | 38 ++--- > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 44 ++--- > drivers/gpu/drm/i915/display/intel_dpt.c | 2 +- > drivers/gpu/drm/i915/display/intel_drrs.c | 10 +- > drivers/gpu/drm/i915/display/intel_dsb.c | 8 +- > drivers/gpu/drm/i915/display/intel_fbc.c | 2 +- > drivers/gpu/drm/i915/display/intel_fdi.c | 22 +-- > .../drm/i915/display/intel_fifo_underrun.c | 6 +- > drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- > .../drm/i915/display/intel_modeset_setup.c | 22 +-- > .../drm/i915/display/intel_modeset_verify.c | 2 +- > drivers/gpu/drm/i915/display/intel_panel.c | 4 +- > .../gpu/drm/i915/display/intel_pch_display.c | 32 ++-- > .../gpu/drm/i915/display/intel_pch_refclk.c | 2 +- > drivers/gpu/drm/i915/display/intel_pipe_crc.c | 10 +- > .../drm/i915/display/intel_plane_initial.c | 6 +- > drivers/gpu/drm/i915/display/intel_psr.c | 14 +- > drivers/gpu/drm/i915/display/intel_sdvo.c | 2 +- > drivers/gpu/drm/i915/display/intel_vblank.c | 24 +-- > drivers/gpu/drm/i915/display/intel_vdsc.c | 18 +- > drivers/gpu/drm/i915/display/intel_vrr.c | 18 +- > drivers/gpu/drm/i915/display/skl_scaler.c | 10 +- > .../drm/i915/display/skl_universal_plane.c | 6 +- > drivers/gpu/drm/i915/display/skl_watermark.c | 42 ++--- > drivers/gpu/drm/i915/display/vlv_dsi.c | 2 +- > drivers/gpu/drm/imx/dcss/dcss-crtc.c | 20 +-- > drivers/gpu/drm/imx/ipuv3/ipuv3-crtc.c | 15 +- > drivers/gpu/drm/imx/lcdc/imx-lcdc.c | 16 +- > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 4 +- > drivers/gpu/drm/kmb/kmb_crtc.c | 16 +- > drivers/gpu/drm/logicvc/logicvc_crtc.c | 14 +- > drivers/gpu/drm/mcde/mcde_display.c | 18 +- > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 22 +-- > drivers/gpu/drm/meson/meson_crtc.c | 12 +- > drivers/gpu/drm/mgag200/mgag200_g200.c | 4 +- > drivers/gpu/drm/mgag200/mgag200_g200eh.c | 2 +- > drivers/gpu/drm/mgag200/mgag200_g200er.c | 4 +- > drivers/gpu/drm/mgag200/mgag200_g200ev.c | 4 +- > drivers/gpu/drm/mgag200/mgag200_g200se.c | 6 +- > drivers/gpu/drm/mgag200/mgag200_g200wb.c | 2 +- > drivers/gpu/drm/mgag200/mgag200_mode.c | 10 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 6 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 70 ++++---- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- > drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 12 +- > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 20 +-- > drivers/gpu/drm/msm/msm_drv.c | 4 +- > drivers/gpu/drm/mxsfb/lcdif_kms.c | 18 +- > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 16 +- > drivers/gpu/drm/nouveau/dispnv04/crtc.c | 58 +++---- > drivers/gpu/drm/nouveau/dispnv04/cursor.c | 10 +- > drivers/gpu/drm/nouveau/dispnv50/atom.h | 2 +- > drivers/gpu/drm/nouveau/dispnv50/crc.c | 30 ++-- > drivers/gpu/drm/nouveau/dispnv50/crc907d.c | 6 +- > drivers/gpu/drm/nouveau/dispnv50/crcc37d.c | 6 +- > drivers/gpu/drm/nouveau/dispnv50/crcc57d.c | 2 +- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +- > drivers/gpu/drm/nouveau/dispnv50/head.c | 4 +- > drivers/gpu/drm/nouveau/dispnv50/head507d.c | 26 +-- > drivers/gpu/drm/nouveau/dispnv50/head827d.c | 10 +- > drivers/gpu/drm/nouveau/dispnv50/head907d.c | 26 +-- > drivers/gpu/drm/nouveau/dispnv50/head917d.c | 6 +- > drivers/gpu/drm/nouveau/dispnv50/headc37d.c | 18 +- > drivers/gpu/drm/nouveau/dispnv50/headc57d.c | 10 +- > drivers/gpu/drm/nouveau/nouveau_connector.h | 2 +- > drivers/gpu/drm/nouveau/nouveau_display.c | 2 +- > drivers/gpu/drm/omapdrm/omap_crtc.c | 56 +++---- > drivers/gpu/drm/omapdrm/omap_irq.c | 6 +- > drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 4 +- > drivers/gpu/drm/pl111/pl111_display.c | 16 +- > drivers/gpu/drm/qxl/qxl_display.c | 2 +- > drivers/gpu/drm/radeon/atombios_crtc.c | 54 +++--- > drivers/gpu/drm/radeon/radeon_cursor.c | 14 +- > drivers/gpu/drm/radeon/radeon_display.c | 28 ++-- > drivers/gpu/drm/radeon/radeon_kms.c | 6 +- > drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 16 +- > .../gpu/drm/renesas/rcar-du/rcar_du_crtc.c | 14 +- > .../gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 20 +-- > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 8 +- > drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 15 +- > drivers/gpu/drm/solomon/ssd130x.c | 2 +- > drivers/gpu/drm/sprd/sprd_dpu.c | 6 +- > drivers/gpu/drm/sti/sti_crtc.c | 14 +- > drivers/gpu/drm/stm/ltdc.c | 12 +- > drivers/gpu/drm/sun4i/sun4i_crtc.c | 12 +- > drivers/gpu/drm/tegra/dc.c | 12 +- > drivers/gpu/drm/tidss/tidss_crtc.c | 19 ++- > drivers/gpu/drm/tidss/tidss_irq.c | 4 +- > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 43 ++--- > drivers/gpu/drm/tiny/bochs.c | 6 +- > drivers/gpu/drm/tiny/cirrus.c | 2 +- > drivers/gpu/drm/tiny/gm12u320.c | 4 +- > drivers/gpu/drm/tiny/hx8357d.c | 4 +- > drivers/gpu/drm/tiny/ili9163.c | 4 +- > drivers/gpu/drm/tiny/ili9225.c | 8 +- > drivers/gpu/drm/tiny/ili9341.c | 4 +- > drivers/gpu/drm/tiny/ili9486.c | 4 +- > drivers/gpu/drm/tiny/mi0283qt.c | 4 +- > drivers/gpu/drm/tiny/ofdrm.c | 8 +- > drivers/gpu/drm/tiny/panel-mipi-dbi.c | 6 +- > drivers/gpu/drm/tiny/repaper.c | 8 +- > drivers/gpu/drm/tiny/simpledrm.c | 2 +- > drivers/gpu/drm/tiny/st7586.c | 6 +- > drivers/gpu/drm/tiny/st7735r.c | 4 +- > drivers/gpu/drm/tve200/tve200_display.c | 14 +- > drivers/gpu/drm/udl/udl_modeset.c | 4 +- > drivers/gpu/drm/vboxvideo/vbox_mode.c | 6 +- > drivers/gpu/drm/vc4/vc4_crtc.c | 38 ++--- > drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- > drivers/gpu/drm/vc4/vc4_hvs.c | 12 +- > drivers/gpu/drm/vc4/vc4_txp.c | 2 +- > drivers/gpu/drm/virtio/virtgpu_display.c | 4 +- > drivers/gpu/drm/vkms/vkms_crtc.c | 12 +- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +- > drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 10 +- > drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 8 +- > drivers/gpu/drm/xen/xen_drm_front_kms.c | 10 +- > drivers/gpu/drm/xlnx/zynqmp_kms.c | 8 +- > include/drm/drm_atomic_helper.h | 2 +- > include/drm/drm_crtc.h | 4 +- > 194 files changed, 1296 insertions(+), 1264 deletions(-) > > base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5 > -- > 2.39.2 >
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 struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/sprd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/sti: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/stm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/sun4i: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/tegra: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/tidss: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/tilcdc: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/tiny: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/tve200: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/udl: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/vboxvideo: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/vc4: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/virtio: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/vkms: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/vmwgfx: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/xen: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/xlnx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/crtc: Complete renaming struct drm_crtc::dev to drm_dev > > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 18 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 6 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_pll.c | 6 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c | 8 +- > drivers/gpu/drm/amd/amdgpu/atombios_crtc.c | 22 +-- > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 26 +-- > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 28 ++-- > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 26 +-- > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 26 +-- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 29 ++-- > .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 20 +-- > .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 8 +- > .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 22 +-- > .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 2 +- > .../gpu/drm/arm/display/komeda/komeda_crtc.c | 24 +-- > .../gpu/drm/arm/display/komeda/komeda_kms.c | 2 +- > drivers/gpu/drm/arm/hdlcd_crtc.c | 4 +- > drivers/gpu/drm/arm/malidp_crtc.c | 7 +- > drivers/gpu/drm/armada/armada_crtc.c | 10 +- > drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c | 6 +- > drivers/gpu/drm/ast/ast_dp.c | 2 +- > drivers/gpu/drm/ast/ast_mode.c | 26 +-- > .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 10 +- > drivers/gpu/drm/drm_atomic.c | 22 +-- > drivers/gpu/drm/drm_atomic_helper.c | 20 ++- > drivers/gpu/drm/drm_atomic_state_helper.c | 2 +- > drivers/gpu/drm/drm_atomic_uapi.c | 22 +-- > drivers/gpu/drm/drm_blend.c | 2 +- > drivers/gpu/drm/drm_color_mgmt.c | 10 +- > drivers/gpu/drm/drm_crtc.c | 19 ++- > drivers/gpu/drm/drm_crtc_helper.c | 10 +- > drivers/gpu/drm/drm_debugfs.c | 2 +- > drivers/gpu/drm/drm_debugfs_crc.c | 2 +- > drivers/gpu/drm/drm_fb_helper.c | 6 +- > drivers/gpu/drm/drm_mipi_dbi.c | 4 +- > drivers/gpu/drm/drm_plane.c | 2 +- > drivers/gpu/drm/drm_plane_helper.c | 2 +- > drivers/gpu/drm/drm_self_refresh_helper.c | 2 +- > drivers/gpu/drm/drm_vblank.c | 40 ++--- > drivers/gpu/drm/drm_vblank_work.c | 2 +- > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 8 +- > drivers/gpu/drm/exynos/exynos_drm_plane.c | 4 +- > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 16 +- > drivers/gpu/drm/gma500/cdv_intel_display.c | 2 +- > drivers/gpu/drm/gma500/cdv_intel_dp.c | 2 +- > drivers/gpu/drm/gma500/gma_display.c | 20 +-- > drivers/gpu/drm/gma500/oaktrail_crtc.c | 8 +- > drivers/gpu/drm/gma500/oaktrail_hdmi.c | 4 +- > drivers/gpu/drm/gma500/psb_intel_display.c | 2 +- > drivers/gpu/drm/gma500/psb_irq.c | 6 +- > drivers/gpu/drm/gud/gud_pipe.c | 6 +- > .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 20 +-- > .../gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 4 +- > drivers/gpu/drm/hyperv/hyperv_drm_modeset.c | 6 +- > drivers/gpu/drm/i915/display/g4x_dp.c | 4 +- > drivers/gpu/drm/i915/display/hsw_ips.c | 16 +- > drivers/gpu/drm/i915/display/i9xx_plane.c | 4 +- > drivers/gpu/drm/i915/display/i9xx_wm.c | 40 ++--- > drivers/gpu/drm/i915/display/icl_dsi.c | 2 +- > drivers/gpu/drm/i915/display/intel_atomic.c | 2 +- > .../gpu/drm/i915/display/intel_atomic_plane.c | 4 +- > drivers/gpu/drm/i915/display/intel_audio.c | 2 +- > drivers/gpu/drm/i915/display/intel_bw.c | 10 +- > drivers/gpu/drm/i915/display/intel_cdclk.c | 6 +- > drivers/gpu/drm/i915/display/intel_color.c | 124 +++++++------- > drivers/gpu/drm/i915/display/intel_crtc.c | 20 +-- > .../drm/i915/display/intel_crtc_state_dump.c | 4 +- > drivers/gpu/drm/i915/display/intel_cursor.c | 2 +- > drivers/gpu/drm/i915/display/intel_ddi.c | 28 ++-- > drivers/gpu/drm/i915/display/intel_display.c | 154 +++++++++--------- > .../gpu/drm/i915/display/intel_display_irq.c | 22 +-- > .../gpu/drm/i915/display/intel_display_rps.c | 2 +- > .../drm/i915/display/intel_display_trace.h | 12 +- > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > drivers/gpu/drm/i915/display/intel_dpll.c | 38 ++--- > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 44 ++--- > drivers/gpu/drm/i915/display/intel_dpt.c | 2 +- > drivers/gpu/drm/i915/display/intel_drrs.c | 10 +- > drivers/gpu/drm/i915/display/intel_dsb.c | 8 +- > drivers/gpu/drm/i915/display/intel_fbc.c | 2 +- > drivers/gpu/drm/i915/display/intel_fdi.c | 22 +-- > .../drm/i915/display/intel_fifo_underrun.c | 6 +- > drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- > .../drm/i915/display/intel_modeset_setup.c | 22 +-- > .../drm/i915/display/intel_modeset_verify.c | 2 +- > drivers/gpu/drm/i915/display/intel_panel.c | 4 +- > .../gpu/drm/i915/display/intel_pch_display.c | 32 ++-- > .../gpu/drm/i915/display/intel_pch_refclk.c | 2 +- > drivers/gpu/drm/i915/display/intel_pipe_crc.c | 10 +- > .../drm/i915/display/intel_plane_initial.c | 6 +- > drivers/gpu/drm/i915/display/intel_psr.c | 14 +- > drivers/gpu/drm/i915/display/intel_sdvo.c | 2 +- > drivers/gpu/drm/i915/display/intel_vblank.c | 24 +-- > drivers/gpu/drm/i915/display/intel_vdsc.c | 18 +- > drivers/gpu/drm/i915/display/intel_vrr.c | 18 +- > drivers/gpu/drm/i915/display/skl_scaler.c | 10 +- > .../drm/i915/display/skl_universal_plane.c | 6 +- > drivers/gpu/drm/i915/display/skl_watermark.c | 42 ++--- > drivers/gpu/drm/i915/display/vlv_dsi.c | 2 +- > drivers/gpu/drm/imx/dcss/dcss-crtc.c | 20 +-- > drivers/gpu/drm/imx/ipuv3/ipuv3-crtc.c | 15 +- > drivers/gpu/drm/imx/lcdc/imx-lcdc.c | 16 +- > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 4 +- > drivers/gpu/drm/kmb/kmb_crtc.c | 16 +- > drivers/gpu/drm/logicvc/logicvc_crtc.c | 14 +- > drivers/gpu/drm/mcde/mcde_display.c | 18 +- > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 22 +-- > drivers/gpu/drm/meson/meson_crtc.c | 12 +- > drivers/gpu/drm/mgag200/mgag200_g200.c | 4 +- > drivers/gpu/drm/mgag200/mgag200_g200eh.c | 2 +- > drivers/gpu/drm/mgag200/mgag200_g200er.c | 4 +- > drivers/gpu/drm/mgag200/mgag200_g200ev.c | 4 +- > drivers/gpu/drm/mgag200/mgag200_g200se.c | 6 +- > drivers/gpu/drm/mgag200/mgag200_g200wb.c | 2 +- > drivers/gpu/drm/mgag200/mgag200_mode.c | 10 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 6 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 70 ++++---- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- > drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 12 +- > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 20 +-- > drivers/gpu/drm/msm/msm_drv.c | 4 +- > drivers/gpu/drm/mxsfb/lcdif_kms.c | 18 +- > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 16 +- > drivers/gpu/drm/nouveau/dispnv04/crtc.c | 58 +++---- > drivers/gpu/drm/nouveau/dispnv04/cursor.c | 10 +- > drivers/gpu/drm/nouveau/dispnv50/atom.h | 2 +- > drivers/gpu/drm/nouveau/dispnv50/crc.c | 30 ++-- > drivers/gpu/drm/nouveau/dispnv50/crc907d.c | 6 +- > drivers/gpu/drm/nouveau/dispnv50/crcc37d.c | 6 +- > drivers/gpu/drm/nouveau/dispnv50/crcc57d.c | 2 +- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +- > drivers/gpu/drm/nouveau/dispnv50/head.c | 4 +- > drivers/gpu/drm/nouveau/dispnv50/head507d.c | 26 +-- > drivers/gpu/drm/nouveau/dispnv50/head827d.c | 10 +- > drivers/gpu/drm/nouveau/dispnv50/head907d.c | 26 +-- > drivers/gpu/drm/nouveau/dispnv50/head917d.c | 6 +- > drivers/gpu/drm/nouveau/dispnv50/headc37d.c | 18 +- > drivers/gpu/drm/nouveau/dispnv50/headc57d.c | 10 +- > drivers/gpu/drm/nouveau/nouveau_connector.h | 2 +- > drivers/gpu/drm/nouveau/nouveau_display.c | 2 +- > drivers/gpu/drm/omapdrm/omap_crtc.c | 56 +++---- > drivers/gpu/drm/omapdrm/omap_irq.c | 6 +- > drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 4 +- > drivers/gpu/drm/pl111/pl111_display.c | 16 +- > drivers/gpu/drm/qxl/qxl_display.c | 2 +- > drivers/gpu/drm/radeon/atombios_crtc.c | 54 +++--- > drivers/gpu/drm/radeon/radeon_cursor.c | 14 +- > drivers/gpu/drm/radeon/radeon_display.c | 28 ++-- > drivers/gpu/drm/radeon/radeon_kms.c | 6 +- > drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 16 +- > .../gpu/drm/renesas/rcar-du/rcar_du_crtc.c | 14 +- > .../gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 20 +-- > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 8 +- > drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 15 +- > drivers/gpu/drm/solomon/ssd130x.c | 2 +- > drivers/gpu/drm/sprd/sprd_dpu.c | 6 +- > drivers/gpu/drm/sti/sti_crtc.c | 14 +- > drivers/gpu/drm/stm/ltdc.c | 12 +- > drivers/gpu/drm/sun4i/sun4i_crtc.c | 12 +- > drivers/gpu/drm/tegra/dc.c | 12 +- > drivers/gpu/drm/tidss/tidss_crtc.c | 19 ++- > drivers/gpu/drm/tidss/tidss_irq.c | 4 +- > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 43 ++--- > drivers/gpu/drm/tiny/bochs.c | 6 +- > drivers/gpu/drm/tiny/cirrus.c | 2 +- > drivers/gpu/drm/tiny/gm12u320.c | 4 +- > drivers/gpu/drm/tiny/hx8357d.c | 4 +- > drivers/gpu/drm/tiny/ili9163.c | 4 +- > drivers/gpu/drm/tiny/ili9225.c | 8 +- > drivers/gpu/drm/tiny/ili9341.c | 4 +- > drivers/gpu/drm/tiny/ili9486.c | 4 +- > drivers/gpu/drm/tiny/mi0283qt.c | 4 +- > drivers/gpu/drm/tiny/ofdrm.c | 8 +- > drivers/gpu/drm/tiny/panel-mipi-dbi.c | 6 +- > drivers/gpu/drm/tiny/repaper.c | 8 +- > drivers/gpu/drm/tiny/simpledrm.c | 2 +- > drivers/gpu/drm/tiny/st7586.c | 6 +- > drivers/gpu/drm/tiny/st7735r.c | 4 +- > drivers/gpu/drm/tve200/tve200_display.c | 14 +- > drivers/gpu/drm/udl/udl_modeset.c | 4 +- > drivers/gpu/drm/vboxvideo/vbox_mode.c | 6 +- > drivers/gpu/drm/vc4/vc4_crtc.c | 38 ++--- > drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- > drivers/gpu/drm/vc4/vc4_hvs.c | 12 +- > drivers/gpu/drm/vc4/vc4_txp.c | 2 +- > drivers/gpu/drm/virtio/virtgpu_display.c | 4 +- > drivers/gpu/drm/vkms/vkms_crtc.c | 12 +- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +- > drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 10 +- > drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 8 +- > drivers/gpu/drm/xen/xen_drm_front_kms.c | 10 +- > drivers/gpu/drm/xlnx/zynqmp_kms.c | 8 +- > include/drm/drm_atomic_helper.h | 2 +- > include/drm/drm_crtc.h | 4 +- > 194 files changed, 1296 insertions(+), 1264 deletions(-) > > base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
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 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 struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/sprd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/sti: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/stm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/sun4i: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/tegra: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/tidss: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/tilcdc: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/tiny: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/tve200: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/udl: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/vboxvideo: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/vc4: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/virtio: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/vkms: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/vmwgfx: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/xen: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/xlnx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/crtc: Complete renaming struct drm_crtc::dev to drm_dev > > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 18 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 6 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_pll.c | 6 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c | 8 +- > drivers/gpu/drm/amd/amdgpu/atombios_crtc.c | 22 +-- > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 26 +-- > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 28 ++-- > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 26 +-- > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 26 +-- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 29 ++-- > .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 20 +-- > .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 8 +- > .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 22 +-- > .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 2 +- > .../gpu/drm/arm/display/komeda/komeda_crtc.c | 24 +-- > .../gpu/drm/arm/display/komeda/komeda_kms.c | 2 +- > drivers/gpu/drm/arm/hdlcd_crtc.c | 4 +- > drivers/gpu/drm/arm/malidp_crtc.c | 7 +- > drivers/gpu/drm/armada/armada_crtc.c | 10 +- > drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c | 6 +- > drivers/gpu/drm/ast/ast_dp.c | 2 +- > drivers/gpu/drm/ast/ast_mode.c | 26 +-- > .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 10 +- > drivers/gpu/drm/drm_atomic.c | 22 +-- > drivers/gpu/drm/drm_atomic_helper.c | 20 ++- > drivers/gpu/drm/drm_atomic_state_helper.c | 2 +- > drivers/gpu/drm/drm_atomic_uapi.c | 22 +-- > drivers/gpu/drm/drm_blend.c | 2 +- > drivers/gpu/drm/drm_color_mgmt.c | 10 +- > drivers/gpu/drm/drm_crtc.c | 19 ++- > drivers/gpu/drm/drm_crtc_helper.c | 10 +- > drivers/gpu/drm/drm_debugfs.c | 2 +- > drivers/gpu/drm/drm_debugfs_crc.c | 2 +- > drivers/gpu/drm/drm_fb_helper.c | 6 +- > drivers/gpu/drm/drm_mipi_dbi.c | 4 +- > drivers/gpu/drm/drm_plane.c | 2 +- > drivers/gpu/drm/drm_plane_helper.c | 2 +- > drivers/gpu/drm/drm_self_refresh_helper.c | 2 +- > drivers/gpu/drm/drm_vblank.c | 40 ++--- > drivers/gpu/drm/drm_vblank_work.c | 2 +- > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 8 +- > drivers/gpu/drm/exynos/exynos_drm_plane.c | 4 +- > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 16 +- > drivers/gpu/drm/gma500/cdv_intel_display.c | 2 +- > drivers/gpu/drm/gma500/cdv_intel_dp.c | 2 +- > drivers/gpu/drm/gma500/gma_display.c | 20 +-- > drivers/gpu/drm/gma500/oaktrail_crtc.c | 8 +- > drivers/gpu/drm/gma500/oaktrail_hdmi.c | 4 +- > drivers/gpu/drm/gma500/psb_intel_display.c | 2 +- > drivers/gpu/drm/gma500/psb_irq.c | 6 +- > drivers/gpu/drm/gud/gud_pipe.c | 6 +- > .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 20 +-- > .../gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 4 +- > drivers/gpu/drm/hyperv/hyperv_drm_modeset.c | 6 +- > drivers/gpu/drm/i915/display/g4x_dp.c | 4 +- > drivers/gpu/drm/i915/display/hsw_ips.c | 16 +- > drivers/gpu/drm/i915/display/i9xx_plane.c | 4 +- > drivers/gpu/drm/i915/display/i9xx_wm.c | 40 ++--- > drivers/gpu/drm/i915/display/icl_dsi.c | 2 +- > drivers/gpu/drm/i915/display/intel_atomic.c | 2 +- > .../gpu/drm/i915/display/intel_atomic_plane.c | 4 +- > drivers/gpu/drm/i915/display/intel_audio.c | 2 +- > drivers/gpu/drm/i915/display/intel_bw.c | 10 +- > drivers/gpu/drm/i915/display/intel_cdclk.c | 6 +- > drivers/gpu/drm/i915/display/intel_color.c | 124 +++++++------- > drivers/gpu/drm/i915/display/intel_crtc.c | 20 +-- > .../drm/i915/display/intel_crtc_state_dump.c | 4 +- > drivers/gpu/drm/i915/display/intel_cursor.c | 2 +- > drivers/gpu/drm/i915/display/intel_ddi.c | 28 ++-- > drivers/gpu/drm/i915/display/intel_display.c | 154 +++++++++--------- > .../gpu/drm/i915/display/intel_display_irq.c | 22 +-- > .../gpu/drm/i915/display/intel_display_rps.c | 2 +- > .../drm/i915/display/intel_display_trace.h | 12 +- > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > drivers/gpu/drm/i915/display/intel_dpll.c | 38 ++--- > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 44 ++--- > drivers/gpu/drm/i915/display/intel_dpt.c | 2 +- > drivers/gpu/drm/i915/display/intel_drrs.c | 10 +- > drivers/gpu/drm/i915/display/intel_dsb.c | 8 +- > drivers/gpu/drm/i915/display/intel_fbc.c | 2 +- > drivers/gpu/drm/i915/display/intel_fdi.c | 22 +-- > .../drm/i915/display/intel_fifo_underrun.c | 6 +- > drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- > .../drm/i915/display/intel_modeset_setup.c | 22 +-- > .../drm/i915/display/intel_modeset_verify.c | 2 +- > drivers/gpu/drm/i915/display/intel_panel.c | 4 +- > .../gpu/drm/i915/display/intel_pch_display.c | 32 ++-- > .../gpu/drm/i915/display/intel_pch_refclk.c | 2 +- > drivers/gpu/drm/i915/display/intel_pipe_crc.c | 10 +- > .../drm/i915/display/intel_plane_initial.c | 6 +- > drivers/gpu/drm/i915/display/intel_psr.c | 14 +- > drivers/gpu/drm/i915/display/intel_sdvo.c | 2 +- > drivers/gpu/drm/i915/display/intel_vblank.c | 24 +-- > drivers/gpu/drm/i915/display/intel_vdsc.c | 18 +- > drivers/gpu/drm/i915/display/intel_vrr.c | 18 +- > drivers/gpu/drm/i915/display/skl_scaler.c | 10 +- > .../drm/i915/display/skl_universal_plane.c | 6 +- > drivers/gpu/drm/i915/display/skl_watermark.c | 42 ++--- > drivers/gpu/drm/i915/display/vlv_dsi.c | 2 +- > drivers/gpu/drm/imx/dcss/dcss-crtc.c | 20 +-- > drivers/gpu/drm/imx/ipuv3/ipuv3-crtc.c | 15 +- > drivers/gpu/drm/imx/lcdc/imx-lcdc.c | 16 +- > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 4 +- > drivers/gpu/drm/kmb/kmb_crtc.c | 16 +- > drivers/gpu/drm/logicvc/logicvc_crtc.c | 14 +- > drivers/gpu/drm/mcde/mcde_display.c | 18 +- > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 22 +-- > drivers/gpu/drm/meson/meson_crtc.c | 12 +- > drivers/gpu/drm/mgag200/mgag200_g200.c | 4 +- > drivers/gpu/drm/mgag200/mgag200_g200eh.c | 2 +- > drivers/gpu/drm/mgag200/mgag200_g200er.c | 4 +- > drivers/gpu/drm/mgag200/mgag200_g200ev.c | 4 +- > drivers/gpu/drm/mgag200/mgag200_g200se.c | 6 +- > drivers/gpu/drm/mgag200/mgag200_g200wb.c | 2 +- > drivers/gpu/drm/mgag200/mgag200_mode.c | 10 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 6 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 70 ++++---- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- > drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 12 +- > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 20 +-- > drivers/gpu/drm/msm/msm_drv.c | 4 +- > drivers/gpu/drm/mxsfb/lcdif_kms.c | 18 +- > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 16 +- > drivers/gpu/drm/nouveau/dispnv04/crtc.c | 58 +++---- > drivers/gpu/drm/nouveau/dispnv04/cursor.c | 10 +- > drivers/gpu/drm/nouveau/dispnv50/atom.h | 2 +- > drivers/gpu/drm/nouveau/dispnv50/crc.c | 30 ++-- > drivers/gpu/drm/nouveau/dispnv50/crc907d.c | 6 +- > drivers/gpu/drm/nouveau/dispnv50/crcc37d.c | 6 +- > drivers/gpu/drm/nouveau/dispnv50/crcc57d.c | 2 +- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +- > drivers/gpu/drm/nouveau/dispnv50/head.c | 4 +- > drivers/gpu/drm/nouveau/dispnv50/head507d.c | 26 +-- > drivers/gpu/drm/nouveau/dispnv50/head827d.c | 10 +- > drivers/gpu/drm/nouveau/dispnv50/head907d.c | 26 +-- > drivers/gpu/drm/nouveau/dispnv50/head917d.c | 6 +- > drivers/gpu/drm/nouveau/dispnv50/headc37d.c | 18 +- > drivers/gpu/drm/nouveau/dispnv50/headc57d.c | 10 +- > drivers/gpu/drm/nouveau/nouveau_connector.h | 2 +- > drivers/gpu/drm/nouveau/nouveau_display.c | 2 +- > drivers/gpu/drm/omapdrm/omap_crtc.c | 56 +++---- > drivers/gpu/drm/omapdrm/omap_irq.c | 6 +- > drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 4 +- > drivers/gpu/drm/pl111/pl111_display.c | 16 +- > drivers/gpu/drm/qxl/qxl_display.c | 2 +- > drivers/gpu/drm/radeon/atombios_crtc.c | 54 +++--- > drivers/gpu/drm/radeon/radeon_cursor.c | 14 +- > drivers/gpu/drm/radeon/radeon_display.c | 28 ++-- > drivers/gpu/drm/radeon/radeon_kms.c | 6 +- > drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 16 +- > .../gpu/drm/renesas/rcar-du/rcar_du_crtc.c | 14 +- > .../gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 20 +-- > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 8 +- > drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 15 +- > drivers/gpu/drm/solomon/ssd130x.c | 2 +- > drivers/gpu/drm/sprd/sprd_dpu.c | 6 +- > drivers/gpu/drm/sti/sti_crtc.c | 14 +- > drivers/gpu/drm/stm/ltdc.c | 12 +- > drivers/gpu/drm/sun4i/sun4i_crtc.c | 12 +- > drivers/gpu/drm/tegra/dc.c | 12 +- > drivers/gpu/drm/tidss/tidss_crtc.c | 19 ++- > drivers/gpu/drm/tidss/tidss_irq.c | 4 +- > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 43 ++--- > drivers/gpu/drm/tiny/bochs.c | 6 +- > drivers/gpu/drm/tiny/cirrus.c | 2 +- > drivers/gpu/drm/tiny/gm12u320.c | 4 +- > drivers/gpu/drm/tiny/hx8357d.c | 4 +- > drivers/gpu/drm/tiny/ili9163.c | 4 +- > drivers/gpu/drm/tiny/ili9225.c | 8 +- > drivers/gpu/drm/tiny/ili9341.c | 4 +- > drivers/gpu/drm/tiny/ili9486.c | 4 +- > drivers/gpu/drm/tiny/mi0283qt.c | 4 +- > drivers/gpu/drm/tiny/ofdrm.c | 8 +- > drivers/gpu/drm/tiny/panel-mipi-dbi.c | 6 +- > drivers/gpu/drm/tiny/repaper.c | 8 +- > drivers/gpu/drm/tiny/simpledrm.c | 2 +- > drivers/gpu/drm/tiny/st7586.c | 6 +- > drivers/gpu/drm/tiny/st7735r.c | 4 +- > drivers/gpu/drm/tve200/tve200_display.c | 14 +- > drivers/gpu/drm/udl/udl_modeset.c | 4 +- > drivers/gpu/drm/vboxvideo/vbox_mode.c | 6 +- > drivers/gpu/drm/vc4/vc4_crtc.c | 38 ++--- > drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- > drivers/gpu/drm/vc4/vc4_hvs.c | 12 +- > drivers/gpu/drm/vc4/vc4_txp.c | 2 +- > drivers/gpu/drm/virtio/virtgpu_display.c | 4 +- > drivers/gpu/drm/vkms/vkms_crtc.c | 12 +- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +- > drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 10 +- > drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 8 +- > drivers/gpu/drm/xen/xen_drm_front_kms.c | 10 +- > drivers/gpu/drm/xlnx/zynqmp_kms.c | 8 +- > include/drm/drm_atomic_helper.h | 2 +- > include/drm/drm_crtc.h | 4 +- > 194 files changed, 1296 insertions(+), 1264 deletions(-) > > base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
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
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(&crtc->dev->event_lock); + spin_lock_irq(&crtc->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
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(&crtc->dev->event_lock); > + spin_lock_irq(&crtc->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/ | >
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(&crtc->dev->event_lock); >> + spin_lock_irq(&crtc->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/ | > >
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: [dropping some recipients since my SMTP server was complaining about the size] > 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. I think that either "drm" or "drm_dev" would be more clear than "dev", which I also found it confusing and thinking about a "struct device". Probably leaning to "drm", since as you said is the second most used name in drivers that assign crtc->dev to a local variable. > 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/ |
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
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
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
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 >
On Wed, 12 Jul 2023, Uwe Kleine-König <u.kleine-koenig@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.
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@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
On Wed, Jul 12, 2023 at 10:52 AM Jani Nikula <jani.nikula@intel.com> wrote: > > On Wed, 12 Jul 2023, Uwe Kleine-König <u.kleine-koenig@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 12/07/2023 20:31, Sean Paul wrote: >>> 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. You know, every code cleanup and style adjustment is interfering with backporting. The only argument for a fast-pacing kernel should be whether the developers of this code find it more readable with such cleanup. Best regards, Krzysztof
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.
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 <u.kleine-koenig@pengutronix.de> wrote: > 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@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 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+renesas@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
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 <u.kleine-koenig@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; > ... > }; 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->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 >
Hi Am 12.07.23 um 20:31 schrieb Sean Paul: > On Wed, Jul 12, 2023 at 10:52 AM Jani Nikula <jani.nikula@intel.com> wrote: >> >> On Wed, 12 Jul 2023, Uwe Kleine-König <u.kleine-koenig@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. I agree. > > 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
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 > drm_crtc::dev > drm/rockchip: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/solomon: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/sprd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/sti: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/stm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/sun4i: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/tegra: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/tidss: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/tilcdc: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/tiny: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/tve200: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/udl: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/vboxvideo: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/vc4: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/virtio: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/vkms: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/vmwgfx: Use struct drm_crtc::drm_dev instead of struct > drm_crtc::dev > drm/xen: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/xlnx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev > drm/crtc: Complete renaming struct drm_crtc::dev to drm_dev > > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 18 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 6 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_pll.c | 6 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c | 8 +- > drivers/gpu/drm/amd/amdgpu/atombios_crtc.c | 22 +-- > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 26 +-- > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 28 ++-- > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 26 +-- > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 26 +-- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 29 ++-- > .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 20 +-- > .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 8 +- > .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 22 +-- > .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 2 +- > .../gpu/drm/arm/display/komeda/komeda_crtc.c | 24 +-- > .../gpu/drm/arm/display/komeda/komeda_kms.c | 2 +- > drivers/gpu/drm/arm/hdlcd_crtc.c | 4 +- > drivers/gpu/drm/arm/malidp_crtc.c | 7 +- > drivers/gpu/drm/armada/armada_crtc.c | 10 +- > drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c | 6 +- > drivers/gpu/drm/ast/ast_dp.c | 2 +- > drivers/gpu/drm/ast/ast_mode.c | 26 +-- > .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 10 +- > drivers/gpu/drm/drm_atomic.c | 22 +-- > drivers/gpu/drm/drm_atomic_helper.c | 20 ++- > drivers/gpu/drm/drm_atomic_state_helper.c | 2 +- > drivers/gpu/drm/drm_atomic_uapi.c | 22 +-- > drivers/gpu/drm/drm_blend.c | 2 +- > drivers/gpu/drm/drm_color_mgmt.c | 10 +- > drivers/gpu/drm/drm_crtc.c | 19 ++- > drivers/gpu/drm/drm_crtc_helper.c | 10 +- > drivers/gpu/drm/drm_debugfs.c | 2 +- > drivers/gpu/drm/drm_debugfs_crc.c | 2 +- > drivers/gpu/drm/drm_fb_helper.c | 6 +- > drivers/gpu/drm/drm_mipi_dbi.c | 4 +- > drivers/gpu/drm/drm_plane.c | 2 +- > drivers/gpu/drm/drm_plane_helper.c | 2 +- > drivers/gpu/drm/drm_self_refresh_helper.c | 2 +- > drivers/gpu/drm/drm_vblank.c | 40 ++--- > drivers/gpu/drm/drm_vblank_work.c | 2 +- > drivers/gpu/drm/exynos/exynos_drm_crtc.c | 8 +- > drivers/gpu/drm/exynos/exynos_drm_plane.c | 4 +- > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 16 +- > drivers/gpu/drm/gma500/cdv_intel_display.c | 2 +- > drivers/gpu/drm/gma500/cdv_intel_dp.c | 2 +- > drivers/gpu/drm/gma500/gma_display.c | 20 +-- > drivers/gpu/drm/gma500/oaktrail_crtc.c | 8 +- > drivers/gpu/drm/gma500/oaktrail_hdmi.c | 4 +- > drivers/gpu/drm/gma500/psb_intel_display.c | 2 +- > drivers/gpu/drm/gma500/psb_irq.c | 6 +- > drivers/gpu/drm/gud/gud_pipe.c | 6 +- > .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 20 +-- > .../gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 4 +- > drivers/gpu/drm/hyperv/hyperv_drm_modeset.c | 6 +- > drivers/gpu/drm/i915/display/g4x_dp.c | 4 +- > drivers/gpu/drm/i915/display/hsw_ips.c | 16 +- > drivers/gpu/drm/i915/display/i9xx_plane.c | 4 +- > drivers/gpu/drm/i915/display/i9xx_wm.c | 40 ++--- > drivers/gpu/drm/i915/display/icl_dsi.c | 2 +- > drivers/gpu/drm/i915/display/intel_atomic.c | 2 +- > .../gpu/drm/i915/display/intel_atomic_plane.c | 4 +- > drivers/gpu/drm/i915/display/intel_audio.c | 2 +- > drivers/gpu/drm/i915/display/intel_bw.c | 10 +- > drivers/gpu/drm/i915/display/intel_cdclk.c | 6 +- > drivers/gpu/drm/i915/display/intel_color.c | 124 +++++++------- > drivers/gpu/drm/i915/display/intel_crtc.c | 20 +-- > .../drm/i915/display/intel_crtc_state_dump.c | 4 +- > drivers/gpu/drm/i915/display/intel_cursor.c | 2 +- > drivers/gpu/drm/i915/display/intel_ddi.c | 28 ++-- > drivers/gpu/drm/i915/display/intel_display.c | 154 +++++++++--------- > .../gpu/drm/i915/display/intel_display_irq.c | 22 +-- > .../gpu/drm/i915/display/intel_display_rps.c | 2 +- > .../drm/i915/display/intel_display_trace.h | 12 +- > drivers/gpu/drm/i915/display/intel_dp.c | 2 +- > drivers/gpu/drm/i915/display/intel_dpll.c | 38 ++--- > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 44 ++--- > drivers/gpu/drm/i915/display/intel_dpt.c | 2 +- > drivers/gpu/drm/i915/display/intel_drrs.c | 10 +- > drivers/gpu/drm/i915/display/intel_dsb.c | 8 +- > drivers/gpu/drm/i915/display/intel_fbc.c | 2 +- > drivers/gpu/drm/i915/display/intel_fdi.c | 22 +-- > .../drm/i915/display/intel_fifo_underrun.c | 6 +- > drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- > .../drm/i915/display/intel_modeset_setup.c | 22 +-- > .../drm/i915/display/intel_modeset_verify.c | 2 +- > drivers/gpu/drm/i915/display/intel_panel.c | 4 +- > .../gpu/drm/i915/display/intel_pch_display.c | 32 ++-- > .../gpu/drm/i915/display/intel_pch_refclk.c | 2 +- > drivers/gpu/drm/i915/display/intel_pipe_crc.c | 10 +- > .../drm/i915/display/intel_plane_initial.c | 6 +- > drivers/gpu/drm/i915/display/intel_psr.c | 14 +- > drivers/gpu/drm/i915/display/intel_sdvo.c | 2 +- > drivers/gpu/drm/i915/display/intel_vblank.c | 24 +-- > drivers/gpu/drm/i915/display/intel_vdsc.c | 18 +- > drivers/gpu/drm/i915/display/intel_vrr.c | 18 +- > drivers/gpu/drm/i915/display/skl_scaler.c | 10 +- > .../drm/i915/display/skl_universal_plane.c | 6 +- > drivers/gpu/drm/i915/display/skl_watermark.c | 42 ++--- > drivers/gpu/drm/i915/display/vlv_dsi.c | 2 +- > drivers/gpu/drm/imx/dcss/dcss-crtc.c | 20 +-- > drivers/gpu/drm/imx/ipuv3/ipuv3-crtc.c | 15 +- > drivers/gpu/drm/imx/lcdc/imx-lcdc.c | 16 +- > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 4 +- > drivers/gpu/drm/kmb/kmb_crtc.c | 16 +- > drivers/gpu/drm/logicvc/logicvc_crtc.c | 14 +- > drivers/gpu/drm/mcde/mcde_display.c | 18 +- > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 22 +-- > drivers/gpu/drm/meson/meson_crtc.c | 12 +- > drivers/gpu/drm/mgag200/mgag200_g200.c | 4 +- > drivers/gpu/drm/mgag200/mgag200_g200eh.c | 2 +- > drivers/gpu/drm/mgag200/mgag200_g200er.c | 4 +- > drivers/gpu/drm/mgag200/mgag200_g200ev.c | 4 +- > drivers/gpu/drm/mgag200/mgag200_g200se.c | 6 +- > drivers/gpu/drm/mgag200/mgag200_g200wb.c | 2 +- > drivers/gpu/drm/mgag200/mgag200_mode.c | 10 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 6 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 70 ++++---- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- > drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 12 +- > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 20 +-- > drivers/gpu/drm/msm/msm_drv.c | 4 +- > drivers/gpu/drm/mxsfb/lcdif_kms.c | 18 +- > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 16 +- > drivers/gpu/drm/nouveau/dispnv04/crtc.c | 58 +++---- > drivers/gpu/drm/nouveau/dispnv04/cursor.c | 10 +- > drivers/gpu/drm/nouveau/dispnv50/atom.h | 2 +- > drivers/gpu/drm/nouveau/dispnv50/crc.c | 30 ++-- > drivers/gpu/drm/nouveau/dispnv50/crc907d.c | 6 +- > drivers/gpu/drm/nouveau/dispnv50/crcc37d.c | 6 +- > drivers/gpu/drm/nouveau/dispnv50/crcc57d.c | 2 +- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +- > drivers/gpu/drm/nouveau/dispnv50/head.c | 4 +- > drivers/gpu/drm/nouveau/dispnv50/head507d.c | 26 +-- > drivers/gpu/drm/nouveau/dispnv50/head827d.c | 10 +- > drivers/gpu/drm/nouveau/dispnv50/head907d.c | 26 +-- > drivers/gpu/drm/nouveau/dispnv50/head917d.c | 6 +- > drivers/gpu/drm/nouveau/dispnv50/headc37d.c | 18 +- > drivers/gpu/drm/nouveau/dispnv50/headc57d.c | 10 +- > drivers/gpu/drm/nouveau/nouveau_connector.h | 2 +- > drivers/gpu/drm/nouveau/nouveau_display.c | 2 +- > drivers/gpu/drm/omapdrm/omap_crtc.c | 56 +++---- > drivers/gpu/drm/omapdrm/omap_irq.c | 6 +- > drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 4 +- > drivers/gpu/drm/pl111/pl111_display.c | 16 +- > drivers/gpu/drm/qxl/qxl_display.c | 2 +- > drivers/gpu/drm/radeon/atombios_crtc.c | 54 +++--- > drivers/gpu/drm/radeon/radeon_cursor.c | 14 +- > drivers/gpu/drm/radeon/radeon_display.c | 28 ++-- > drivers/gpu/drm/radeon/radeon_kms.c | 6 +- > drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 16 +- > .../gpu/drm/renesas/rcar-du/rcar_du_crtc.c | 14 +- > .../gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 20 +-- > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 8 +- > drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 15 +- > drivers/gpu/drm/solomon/ssd130x.c | 2 +- > drivers/gpu/drm/sprd/sprd_dpu.c | 6 +- > drivers/gpu/drm/sti/sti_crtc.c | 14 +- > drivers/gpu/drm/stm/ltdc.c | 12 +- > drivers/gpu/drm/sun4i/sun4i_crtc.c | 12 +- > drivers/gpu/drm/tegra/dc.c | 12 +- > drivers/gpu/drm/tidss/tidss_crtc.c | 19 ++- > drivers/gpu/drm/tidss/tidss_irq.c | 4 +- > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 43 ++--- > drivers/gpu/drm/tiny/bochs.c | 6 +- > drivers/gpu/drm/tiny/cirrus.c | 2 +- > drivers/gpu/drm/tiny/gm12u320.c | 4 +- > drivers/gpu/drm/tiny/hx8357d.c | 4 +- > drivers/gpu/drm/tiny/ili9163.c | 4 +- > drivers/gpu/drm/tiny/ili9225.c | 8 +- > drivers/gpu/drm/tiny/ili9341.c | 4 +- > drivers/gpu/drm/tiny/ili9486.c | 4 +- > drivers/gpu/drm/tiny/mi0283qt.c | 4 +- > drivers/gpu/drm/tiny/ofdrm.c | 8 +- > drivers/gpu/drm/tiny/panel-mipi-dbi.c | 6 +- > drivers/gpu/drm/tiny/repaper.c | 8 +- > drivers/gpu/drm/tiny/simpledrm.c | 2 +- > drivers/gpu/drm/tiny/st7586.c | 6 +- > drivers/gpu/drm/tiny/st7735r.c | 4 +- > drivers/gpu/drm/tve200/tve200_display.c | 14 +- > drivers/gpu/drm/udl/udl_modeset.c | 4 +- > drivers/gpu/drm/vboxvideo/vbox_mode.c | 6 +- > drivers/gpu/drm/vc4/vc4_crtc.c | 38 ++--- > drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- > drivers/gpu/drm/vc4/vc4_hvs.c | 12 +- > drivers/gpu/drm/vc4/vc4_txp.c | 2 +- > drivers/gpu/drm/virtio/virtgpu_display.c | 4 +- > drivers/gpu/drm/vkms/vkms_crtc.c | 12 +- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +- > drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 10 +- > drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 8 +- > drivers/gpu/drm/xen/xen_drm_front_kms.c | 10 +- > drivers/gpu/drm/xlnx/zynqmp_kms.c | 8 +- > include/drm/drm_atomic_helper.h | 2 +- > include/drm/drm_crtc.h | 4 +- > 194 files changed, 1296 insertions(+), 1264 deletions(-) > > base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
On Wed, 12 Jul 2023, Uwe Kleine-König <u.kleine-koenig@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@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.
Hi Jani, On Thu, Jul 13, 2023 at 11:03 AM Jani Nikula <jani.nikula@intel.com> wrote: > On Wed, 12 Jul 2023, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > 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@pengutronix.de> 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->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
On Thu, Jul 13, 2023 at 12:03:05PM +0300, Jani Nikula wrote: > On Wed, 12 Jul 2023, Uwe Kleine-König <u.kleine-koenig@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@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. 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
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 > <u.kleine-koenig@pengutronix.de> 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->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+renesas@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/\<drm_dev\>/$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
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
On Thu, Jul 13, 2023 at 9:04 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > 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. Patch #1 avoids the need to backport the entire set, however every change occuring after the rename patches will cause conflicts on future cherry-picks. Downstream kernels will have to backport the whole set. Backporting the entire set will create an epoch in downstream kernels where cherry-picking patches preceding this set will need to undergo conflict resolution as well. As mentioned in my previous email, I don't expect sympathy here, it's part of maintaining a downstream kernel, but there is a real cost to kernel consumers. > > 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. You've rightly identified the conjecture in your statement. I've been on both sides of the argument, having written/maintained drm code upstream and cherry-picked changes to a downstream kernel. Perhaps it's because drm's definition of dev is ingrained in my muscle memory, or maybe it's because I don't do a lot of upstream development these days, but I just have a hard time seeing the benefit here. I appreciate your engagement on the topic, thank you! Sean > > Thanks for your input, best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Am 13.07.23 um 16:41 schrieb Sean Paul: > On Thu, Jul 13, 2023 at 9:04 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: >> >> 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. > > Patch #1 avoids the need to backport the entire set, however every > change occuring after the rename patches will cause conflicts on > future cherry-picks. Downstream kernels will have to backport the > whole set. Backporting the entire set will create an epoch in > downstream kernels where cherry-picking patches preceding this set > will need to undergo conflict resolution as well. As mentioned in my > previous email, I don't expect sympathy here, it's part of maintaining > a downstream kernel, but there is a real cost to kernel consumers. > >> >> 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. > > You've rightly identified the conjecture in your statement. I've been > on both sides of the argument, having written/maintained drm code > upstream and cherry-picked changes to a downstream kernel. Perhaps > it's because drm's definition of dev is ingrained in my muscle memory, > or maybe it's because I don't do a lot of upstream development these > days, but I just have a hard time seeing the benefit here. I can only second what Sean writes. I've done quite a bit of backporting of DRM code. It's hard already. And this kind of change is going to to affect almost every backported DRM patch in the coming years. Not just for distribution kernels, but also for upstream's stable series. It's really only possible to do this change over many releases while keeping compatible with the old name. So the more I think about it, the less I like this change. Best regards Thomas > > I appreciate your engagement on the topic, thank you! > > Sean > >> >> Thanks for your input, best regards >> Uwe >> >> -- >> Pengutronix e.K. | Uwe Kleine-König | >> Industrial Linux Solutions | https://www.pengutronix.de/ |
On 13/07/2023 16:09, Thomas Zimmermann wrote: > Hi > > Am 13.07.23 um 16:41 schrieb Sean Paul: >> On Thu, Jul 13, 2023 at 9:04 AM Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >>> >>> 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. >> >> Patch #1 avoids the need to backport the entire set, however every >> change occuring after the rename patches will cause conflicts on >> future cherry-picks. Downstream kernels will have to backport the >> whole set. Backporting the entire set will create an epoch in >> downstream kernels where cherry-picking patches preceding this set >> will need to undergo conflict resolution as well. As mentioned in my >> previous email, I don't expect sympathy here, it's part of maintaining >> a downstream kernel, but there is a real cost to kernel consumers. >> >>> >>> 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. >> >> You've rightly identified the conjecture in your statement. I've been >> on both sides of the argument, having written/maintained drm code >> upstream and cherry-picked changes to a downstream kernel. Perhaps >> it's because drm's definition of dev is ingrained in my muscle memory, >> or maybe it's because I don't do a lot of upstream development these >> days, but I just have a hard time seeing the benefit here. > > I can only second what Sean writes. I've done quite a bit of backporting > of DRM code. It's hard already. And this kind of change is going to to > affect almost every backported DRM patch in the coming years. Not just > for distribution kernels, but also for upstream's stable series. It's > really only possible to do this change over many releases while keeping > compatible with the old name. So the more I think about it, the less I > like this change. I've done my share of backporting, and still am doing it, so I can say I dislike it as much as anyone, however.. Is this an argument which the kernel as a wider entity typically accepts? If not could it be a slippery slope to start a precedent? It is a honest question - I am not familiar if there were or were not any similar discussions in the past. My gut feeling is that *if* there is a consensus that something _improves_ the code base significantly, backporting pains should probably not be weighted very heavily as a contra argument. Regards, Tvrtko
On Thu, Jul 13, 2023 at 04:14:55PM +0100, Tvrtko Ursulin wrote: > > On 13/07/2023 16:09, Thomas Zimmermann wrote: > > Hi > > > > Am 13.07.23 um 16:41 schrieb Sean Paul: > > > On Thu, Jul 13, 2023 at 9:04 AM Uwe Kleine-König > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > > 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. > > > > > > Patch #1 avoids the need to backport the entire set, however every > > > change occuring after the rename patches will cause conflicts on > > > future cherry-picks. Downstream kernels will have to backport the > > > whole set. Backporting the entire set will create an epoch in > > > downstream kernels where cherry-picking patches preceding this set > > > will need to undergo conflict resolution as well. As mentioned in my > > > previous email, I don't expect sympathy here, it's part of maintaining > > > a downstream kernel, but there is a real cost to kernel consumers. > > > > > > > > > > > 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. > > > > > > You've rightly identified the conjecture in your statement. I've been > > > on both sides of the argument, having written/maintained drm code > > > upstream and cherry-picked changes to a downstream kernel. Perhaps > > > it's because drm's definition of dev is ingrained in my muscle memory, > > > or maybe it's because I don't do a lot of upstream development these > > > days, but I just have a hard time seeing the benefit here. > > > > I can only second what Sean writes. I've done quite a bit of backporting > > of DRM code. It's hard already. And this kind of change is going to to > > affect almost every backported DRM patch in the coming years. Not just > > for distribution kernels, but also for upstream's stable series. It's > > really only possible to do this change over many releases while keeping > > compatible with the old name. So the more I think about it, the less I > > like this change. > > I've done my share of backporting, and still am doing it, so I can say I > dislike it as much as anyone, however.. Is this an argument which the kernel > as a wider entity typically accepts? If not could it be a slippery slope to > start a precedent? > > It is a honest question - I am not familiar if there were or were not any > similar discussions in the past. Eventually, it's a trade-off. There's always pros and cons to merging every patch, and "backporting pains" is indeed not a very strong con. But it's definitely the kind of patch where everyone and their mother will have their opinion, without every reaching a clear consensus, and there's no clear benefit either (but I might be biaised on that one). So imo, while that downside is fairly weak, the pros are certainly weaker. > My gut feeling is that *if* there is a consensus that something _improves_ > the code base significantly, backporting pains should probably not be > weighted very heavily as a contra argument. 100% agreed here, but I'm afraid we're far from that point. Maxime
On Thu, Jul 13, 2023 at 10:41:45AM -0400, Sean Paul wrote: > On Thu, Jul 13, 2023 at 9:04 AM Uwe Kleine-König > > 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. > > You've rightly identified the conjecture in your statement. I've been > on both sides of the argument, having written/maintained drm code > upstream and cherry-picked changes to a downstream kernel. Perhaps > it's because drm's definition of dev is ingrained in my muscle memory, > or maybe it's because I don't do a lot of upstream development these > days, but I just have a hard time seeing the benefit here. I see that my change doesn't immediately benefit those who are already at home in drivers/gpu/drm. So it's quite understandable that someone in a senior role (long time contributor or maintainer) doesn't see a big upside. In contrast I think my change (with its indisputable cost) lowers the bar for new contributors to drm. IMHO that's something a maintainer has to have on their radar, too, and that's easily undervalued in my experience. Of course in the end it's about weighting the ups and downs. Thanks Uwe
Hi Am 13.07.23 um 17:14 schrieb Tvrtko Ursulin: > > On 13/07/2023 16:09, Thomas Zimmermann wrote: >> Hi >> >> Am 13.07.23 um 16:41 schrieb Sean Paul: >>> On Thu, Jul 13, 2023 at 9:04 AM Uwe Kleine-König >>> <u.kleine-koenig@pengutronix.de> wrote: >>>> >>>> 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. >>> >>> Patch #1 avoids the need to backport the entire set, however every >>> change occuring after the rename patches will cause conflicts on >>> future cherry-picks. Downstream kernels will have to backport the >>> whole set. Backporting the entire set will create an epoch in >>> downstream kernels where cherry-picking patches preceding this set >>> will need to undergo conflict resolution as well. As mentioned in my >>> previous email, I don't expect sympathy here, it's part of maintaining >>> a downstream kernel, but there is a real cost to kernel consumers. >>> >>>> >>>> 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. >>> >>> You've rightly identified the conjecture in your statement. I've been >>> on both sides of the argument, having written/maintained drm code >>> upstream and cherry-picked changes to a downstream kernel. Perhaps >>> it's because drm's definition of dev is ingrained in my muscle memory, >>> or maybe it's because I don't do a lot of upstream development these >>> days, but I just have a hard time seeing the benefit here. >> >> I can only second what Sean writes. I've done quite a bit of >> backporting of DRM code. It's hard already. And this kind of change is >> going to to affect almost every backported DRM patch in the coming >> years. Not just for distribution kernels, but also for upstream's >> stable series. It's really only possible to do this change over many >> releases while keeping compatible with the old name. So the more I >> think about it, the less I like this change. > > I've done my share of backporting, and still am doing it, so I can say I > dislike it as much as anyone, however.. Is this an argument which the > kernel as a wider entity typically accepts? If not could it be a > slippery slope to start a precedent? IMHO upstream patches should only be judged by their effect on the upstream. Backporting, API stability, out-of-tree drivers, etc should not be a concern. I think that we (the DRM devs) are mostly living up to that ideal. OTOH if a change has been accepted, it's fair to ask how to make it in the least intrusive way. But with this change, it doesn't add up for me. The benefit to Linux is rather cosmetic. And the possible downsides are significant even if we ignore downstream distribution kernels. Merging between DRM trees will be affected, backporting into stable kernels as well, the rename will mess up git blame with rename commits. Best regards Thomas > > It is a honest question - I am not familiar if there were or were not > any similar discussions in the past. > > My gut feeling is that *if* there is a consensus that something > _improves_ the code base significantly, backporting pains should > probably not be weighted very heavily as a contra argument. > > Regards, > > Tvrtko
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? 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 struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/sprd: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/sti: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/stm: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/sun4i: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/tegra: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/tidss: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/tilcdc: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/tiny: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/tve200: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/udl: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/vboxvideo: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/vc4: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/virtio: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/vkms: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/vmwgfx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/xen: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/xlnx: Use struct drm_crtc::drm_dev instead of struct drm_crtc::dev drm/crtc: Complete renaming struct drm_crtc::dev to drm_dev drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 18 +- drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 6 +- drivers/gpu/drm/amd/amdgpu/amdgpu_pll.c | 6 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c | 8 +- drivers/gpu/drm/amd/amdgpu/atombios_crtc.c | 22 +-- drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 26 +-- drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 28 ++-- drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 26 +-- drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 26 +-- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 29 ++-- .../drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 20 +-- .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 8 +- .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 22 +-- .../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 2 +- .../gpu/drm/arm/display/komeda/komeda_crtc.c | 24 +-- .../gpu/drm/arm/display/komeda/komeda_kms.c | 2 +- drivers/gpu/drm/arm/hdlcd_crtc.c | 4 +- drivers/gpu/drm/arm/malidp_crtc.c | 7 +- drivers/gpu/drm/armada/armada_crtc.c | 10 +- drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c | 6 +- drivers/gpu/drm/ast/ast_dp.c | 2 +- drivers/gpu/drm/ast/ast_mode.c | 26 +-- .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 10 +- drivers/gpu/drm/drm_atomic.c | 22 +-- drivers/gpu/drm/drm_atomic_helper.c | 20 ++- drivers/gpu/drm/drm_atomic_state_helper.c | 2 +- drivers/gpu/drm/drm_atomic_uapi.c | 22 +-- drivers/gpu/drm/drm_blend.c | 2 +- drivers/gpu/drm/drm_color_mgmt.c | 10 +- drivers/gpu/drm/drm_crtc.c | 19 ++- drivers/gpu/drm/drm_crtc_helper.c | 10 +- drivers/gpu/drm/drm_debugfs.c | 2 +- drivers/gpu/drm/drm_debugfs_crc.c | 2 +- drivers/gpu/drm/drm_fb_helper.c | 6 +- drivers/gpu/drm/drm_mipi_dbi.c | 4 +- drivers/gpu/drm/drm_plane.c | 2 +- drivers/gpu/drm/drm_plane_helper.c | 2 +- drivers/gpu/drm/drm_self_refresh_helper.c | 2 +- drivers/gpu/drm/drm_vblank.c | 40 ++--- drivers/gpu/drm/drm_vblank_work.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 8 +- drivers/gpu/drm/exynos/exynos_drm_plane.c | 4 +- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c | 16 +- drivers/gpu/drm/gma500/cdv_intel_display.c | 2 +- drivers/gpu/drm/gma500/cdv_intel_dp.c | 2 +- drivers/gpu/drm/gma500/gma_display.c | 20 +-- drivers/gpu/drm/gma500/oaktrail_crtc.c | 8 +- drivers/gpu/drm/gma500/oaktrail_hdmi.c | 4 +- drivers/gpu/drm/gma500/psb_intel_display.c | 2 +- drivers/gpu/drm/gma500/psb_irq.c | 6 +- drivers/gpu/drm/gud/gud_pipe.c | 6 +- .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 20 +-- .../gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 4 +- drivers/gpu/drm/hyperv/hyperv_drm_modeset.c | 6 +- drivers/gpu/drm/i915/display/g4x_dp.c | 4 +- drivers/gpu/drm/i915/display/hsw_ips.c | 16 +- drivers/gpu/drm/i915/display/i9xx_plane.c | 4 +- drivers/gpu/drm/i915/display/i9xx_wm.c | 40 ++--- drivers/gpu/drm/i915/display/icl_dsi.c | 2 +- drivers/gpu/drm/i915/display/intel_atomic.c | 2 +- .../gpu/drm/i915/display/intel_atomic_plane.c | 4 +- drivers/gpu/drm/i915/display/intel_audio.c | 2 +- drivers/gpu/drm/i915/display/intel_bw.c | 10 +- drivers/gpu/drm/i915/display/intel_cdclk.c | 6 +- drivers/gpu/drm/i915/display/intel_color.c | 124 +++++++------- drivers/gpu/drm/i915/display/intel_crtc.c | 20 +-- .../drm/i915/display/intel_crtc_state_dump.c | 4 +- drivers/gpu/drm/i915/display/intel_cursor.c | 2 +- drivers/gpu/drm/i915/display/intel_ddi.c | 28 ++-- drivers/gpu/drm/i915/display/intel_display.c | 154 +++++++++--------- .../gpu/drm/i915/display/intel_display_irq.c | 22 +-- .../gpu/drm/i915/display/intel_display_rps.c | 2 +- .../drm/i915/display/intel_display_trace.h | 12 +- drivers/gpu/drm/i915/display/intel_dp.c | 2 +- drivers/gpu/drm/i915/display/intel_dpll.c | 38 ++--- drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 44 ++--- drivers/gpu/drm/i915/display/intel_dpt.c | 2 +- drivers/gpu/drm/i915/display/intel_drrs.c | 10 +- drivers/gpu/drm/i915/display/intel_dsb.c | 8 +- drivers/gpu/drm/i915/display/intel_fbc.c | 2 +- drivers/gpu/drm/i915/display/intel_fdi.c | 22 +-- .../drm/i915/display/intel_fifo_underrun.c | 6 +- drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- .../drm/i915/display/intel_modeset_setup.c | 22 +-- .../drm/i915/display/intel_modeset_verify.c | 2 +- drivers/gpu/drm/i915/display/intel_panel.c | 4 +- .../gpu/drm/i915/display/intel_pch_display.c | 32 ++-- .../gpu/drm/i915/display/intel_pch_refclk.c | 2 +- drivers/gpu/drm/i915/display/intel_pipe_crc.c | 10 +- .../drm/i915/display/intel_plane_initial.c | 6 +- drivers/gpu/drm/i915/display/intel_psr.c | 14 +- drivers/gpu/drm/i915/display/intel_sdvo.c | 2 +- drivers/gpu/drm/i915/display/intel_vblank.c | 24 +-- drivers/gpu/drm/i915/display/intel_vdsc.c | 18 +- drivers/gpu/drm/i915/display/intel_vrr.c | 18 +- drivers/gpu/drm/i915/display/skl_scaler.c | 10 +- .../drm/i915/display/skl_universal_plane.c | 6 +- drivers/gpu/drm/i915/display/skl_watermark.c | 42 ++--- drivers/gpu/drm/i915/display/vlv_dsi.c | 2 +- drivers/gpu/drm/imx/dcss/dcss-crtc.c | 20 +-- drivers/gpu/drm/imx/ipuv3/ipuv3-crtc.c | 15 +- drivers/gpu/drm/imx/lcdc/imx-lcdc.c | 16 +- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 4 +- drivers/gpu/drm/kmb/kmb_crtc.c | 16 +- drivers/gpu/drm/logicvc/logicvc_crtc.c | 14 +- drivers/gpu/drm/mcde/mcde_display.c | 18 +- drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 22 +-- drivers/gpu/drm/meson/meson_crtc.c | 12 +- drivers/gpu/drm/mgag200/mgag200_g200.c | 4 +- drivers/gpu/drm/mgag200/mgag200_g200eh.c | 2 +- drivers/gpu/drm/mgag200/mgag200_g200er.c | 4 +- drivers/gpu/drm/mgag200/mgag200_g200ev.c | 4 +- drivers/gpu/drm/mgag200/mgag200_g200se.c | 6 +- drivers/gpu/drm/mgag200/mgag200_g200wb.c | 2 +- drivers/gpu/drm/mgag200/mgag200_mode.c | 10 +- drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 6 +- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 70 ++++---- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 12 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 20 +-- drivers/gpu/drm/msm/msm_drv.c | 4 +- drivers/gpu/drm/mxsfb/lcdif_kms.c | 18 +- drivers/gpu/drm/mxsfb/mxsfb_kms.c | 16 +- drivers/gpu/drm/nouveau/dispnv04/crtc.c | 58 +++---- drivers/gpu/drm/nouveau/dispnv04/cursor.c | 10 +- drivers/gpu/drm/nouveau/dispnv50/atom.h | 2 +- drivers/gpu/drm/nouveau/dispnv50/crc.c | 30 ++-- drivers/gpu/drm/nouveau/dispnv50/crc907d.c | 6 +- drivers/gpu/drm/nouveau/dispnv50/crcc37d.c | 6 +- drivers/gpu/drm/nouveau/dispnv50/crcc57d.c | 2 +- drivers/gpu/drm/nouveau/dispnv50/disp.c | 5 +- drivers/gpu/drm/nouveau/dispnv50/head.c | 4 +- drivers/gpu/drm/nouveau/dispnv50/head507d.c | 26 +-- drivers/gpu/drm/nouveau/dispnv50/head827d.c | 10 +- drivers/gpu/drm/nouveau/dispnv50/head907d.c | 26 +-- drivers/gpu/drm/nouveau/dispnv50/head917d.c | 6 +- drivers/gpu/drm/nouveau/dispnv50/headc37d.c | 18 +- drivers/gpu/drm/nouveau/dispnv50/headc57d.c | 10 +- drivers/gpu/drm/nouveau/nouveau_connector.h | 2 +- drivers/gpu/drm/nouveau/nouveau_display.c | 2 +- drivers/gpu/drm/omapdrm/omap_crtc.c | 56 +++---- drivers/gpu/drm/omapdrm/omap_irq.c | 6 +- drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 4 +- drivers/gpu/drm/pl111/pl111_display.c | 16 +- drivers/gpu/drm/qxl/qxl_display.c | 2 +- drivers/gpu/drm/radeon/atombios_crtc.c | 54 +++--- drivers/gpu/drm/radeon/radeon_cursor.c | 14 +- drivers/gpu/drm/radeon/radeon_display.c | 28 ++-- drivers/gpu/drm/radeon/radeon_kms.c | 6 +- drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 16 +- .../gpu/drm/renesas/rcar-du/rcar_du_crtc.c | 14 +- .../gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 20 +-- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 8 +- drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 15 +- drivers/gpu/drm/solomon/ssd130x.c | 2 +- drivers/gpu/drm/sprd/sprd_dpu.c | 6 +- drivers/gpu/drm/sti/sti_crtc.c | 14 +- drivers/gpu/drm/stm/ltdc.c | 12 +- drivers/gpu/drm/sun4i/sun4i_crtc.c | 12 +- drivers/gpu/drm/tegra/dc.c | 12 +- drivers/gpu/drm/tidss/tidss_crtc.c | 19 ++- drivers/gpu/drm/tidss/tidss_irq.c | 4 +- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 43 ++--- drivers/gpu/drm/tiny/bochs.c | 6 +- drivers/gpu/drm/tiny/cirrus.c | 2 +- drivers/gpu/drm/tiny/gm12u320.c | 4 +- drivers/gpu/drm/tiny/hx8357d.c | 4 +- drivers/gpu/drm/tiny/ili9163.c | 4 +- drivers/gpu/drm/tiny/ili9225.c | 8 +- drivers/gpu/drm/tiny/ili9341.c | 4 +- drivers/gpu/drm/tiny/ili9486.c | 4 +- drivers/gpu/drm/tiny/mi0283qt.c | 4 +- drivers/gpu/drm/tiny/ofdrm.c | 8 +- drivers/gpu/drm/tiny/panel-mipi-dbi.c | 6 +- drivers/gpu/drm/tiny/repaper.c | 8 +- drivers/gpu/drm/tiny/simpledrm.c | 2 +- drivers/gpu/drm/tiny/st7586.c | 6 +- drivers/gpu/drm/tiny/st7735r.c | 4 +- drivers/gpu/drm/tve200/tve200_display.c | 14 +- drivers/gpu/drm/udl/udl_modeset.c | 4 +- drivers/gpu/drm/vboxvideo/vbox_mode.c | 6 +- drivers/gpu/drm/vc4/vc4_crtc.c | 38 ++--- drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- drivers/gpu/drm/vc4/vc4_hvs.c | 12 +- drivers/gpu/drm/vc4/vc4_txp.c | 2 +- drivers/gpu/drm/virtio/virtgpu_display.c | 4 +- drivers/gpu/drm/vkms/vkms_crtc.c | 12 +- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 4 +- drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 10 +- drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 8 +- drivers/gpu/drm/xen/xen_drm_front_kms.c | 10 +- drivers/gpu/drm/xlnx/zynqmp_kms.c | 8 +- include/drm/drm_atomic_helper.h | 2 +- include/drm/drm_crtc.h | 4 +- 194 files changed, 1296 insertions(+), 1264 deletions(-) base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5