Re: [PATCH 00/17] fbdev: Remove FBINFO_DEFAULT and FBINFO_FLAG_DEFAULT flags
Hi Thomas,
On Tue, Jul 11, 2023 at 08:24:40AM +0200, Thomas Zimmermann wrote:
> Hi Sam
>
> Am 10.07.23 um 19:19 schrieb Sam Ravnborg:
> > Hi Thomas,
> >
> > On Mon, Jul 10, 2023 at 02:50:04PM +0200, Thomas Zimmermann wrote:
> > > Remove the unused flags FBINFO_DEFAULT and FBINFO_FLAG_DEFAULT from
> > > fbdev and drivers, as briefly discussed at [1]. Both flags were maybe
> > > useful when fbdev had special handling for driver modules. With
> > > commit 376b3ff54c9a ("fbdev: Nuke FBINFO_MODULE"), they are both 0
> > > and have no further effect.
> > >
> > > Patches 1 to 7 remove FBINFO_DEFAULT from drivers. Patches 2 to 5
> > > split this by the way the fb_info struct is being allocated. All flags
> > > are cleared to zero during the allocation.
> > >
> > > Patches 8 to 16 do the same for FBINFO_FLAG_DEFAULT. Patch 8 fixes
> > > an actual bug in how arch/sh uses the tokne for struct fb_videomode,
> > > which is unrelated.
> > >
> > > Patch 17 removes both flag constants from
> >
> > We have a few more flags that are unused - should they be nuked too?
> > FBINFO_HWACCEL_FILLRECT
> > FBINFO_HWACCEL_ROTATE
> > FBINFO_HWACCEL_XPAN
>
> It seems those are there for completeness. Nothing sets _ROTATE, the others
> are simply never checked. According to the comments, some are required, some
> are optional. I don't know what that means.
>
> IIRC there were complains about performance when Daniel tried to remove
> fbcon acceleration, so not all _HWACCEL_ flags are unneeded.
>
> Leaving them in for reference/completeness might be an option; or not. I
> have no strong feelings about those flags.
>
> >
> > Unused as in no references from fbdev/core/*
> >
> > I would rather see one series nuke all unused FBINFO flags in one go.
> > Assuming my quick grep are right and the above can be dropped.
>
> I would not want to extend this series. I'm removing _DEFAULT as it's
> absolutely pointless and confusing.
OK, makes sense and thanks for the explanation.
The series is:
Acked-by: Sam Ravnborg
Re: [PATCH 00/17] fbdev: Remove FBINFO_DEFAULT and FBINFO_FLAG_DEFAULT flags
Hi Thomas,
On Mon, Jul 10, 2023 at 02:50:04PM +0200, Thomas Zimmermann wrote:
> Remove the unused flags FBINFO_DEFAULT and FBINFO_FLAG_DEFAULT from
> fbdev and drivers, as briefly discussed at [1]. Both flags were maybe
> useful when fbdev had special handling for driver modules. With
> commit 376b3ff54c9a ("fbdev: Nuke FBINFO_MODULE"), they are both 0
> and have no further effect.
>
> Patches 1 to 7 remove FBINFO_DEFAULT from drivers. Patches 2 to 5
> split this by the way the fb_info struct is being allocated. All flags
> are cleared to zero during the allocation.
>
> Patches 8 to 16 do the same for FBINFO_FLAG_DEFAULT. Patch 8 fixes
> an actual bug in how arch/sh uses the tokne for struct fb_videomode,
> which is unrelated.
>
> Patch 17 removes both flag constants from
We have a few more flags that are unused - should they be nuked too?
FBINFO_HWACCEL_FILLRECT
FBINFO_HWACCEL_ROTATE
FBINFO_HWACCEL_XPAN
Unused as in no references from fbdev/core/*
I would rather see one series nuke all unused FBINFO flags in one go.
Assuming my quick grep are right and the above can be dropped.
Sam
Re: [PATCH v4 13/13] drm/i915: Implement dedicated fbdev I/O helpers
Hi Thomas,
> > > - if (helper->funcs->fb_dirty) {
> > > - drm_fb_helper_memory_range_to_clip(info, pos, ret,
> > > _area);
> > > - drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
> > > - drm_rect_width(_area),
> > > - drm_rect_height(_area));
> > > - }
> >
> > The generated helpers do not have the if (helper->funcs->fb_dirty)
> > check.
> > Is this implemented somewhere else that I missed?
>
> It's not needed any longer. We used to test if the fbdev code needs damage
> handling by looking for a fb_dirty callback. If so, we ran the damage
> handling code.
>
> With the patchset applied, the fbdev code that does not need damage handling
> calls fb_{io,sys}_write() directly. The fbdev code that needs damage
> handling (generic, i915, msm) uses the generator macro that creates
> necessary the calls unconditionally. We know each case at build time.
>
> (I think I have to move the msm patch after patch 10/13 to make it
> bisectable.)
>
> AFAICT the missing test for fb_dirty is also one of the reasons why there's
> a difference in performance.
>
> Hopefully, this answers your question?
Makes perfect sense - thanks.
That also implies that my conditional r-b's are now OK.
Sam
Re: [PATCH v4 13/13] drm/i915: Implement dedicated fbdev I/O helpers
Hi Thomas, On Wed, May 24, 2023 at 11:21:50AM +0200, Thomas Zimmermann wrote: > Implement dedicated fbdev helpers for framebuffer I/O instead > of using DRM's helpers. Use an fbdev generator macro for > deferred I/O to create the fbdev callbacks. i915 was the only > caller of the DRM helpers, so remove them from the helper module. > > i915's fbdev emulation is still incomplete as it doesn't implement > deferred I/O and damage handling for mmaped pages. > > v4: > * generate deferred-I/O helpers > * use initializer macros for fb_ops > v2: > * use FB_IO_HELPERS options > > Signed-off-by: Thomas Zimmermann > Cc: Jani Nikula > Cc: Joonas Lahtinen > Cc: Rodrigo Vivi > Cc: Tvrtko Ursulin > Cc: "Ville Syrjälä" > --- > drivers/gpu/drm/Kconfig| 3 - > drivers/gpu/drm/drm_fb_helper.c| 107 - > drivers/gpu/drm/i915/Kconfig | 1 + > drivers/gpu/drm/i915/display/intel_fbdev.c | 14 +-- > include/drm/drm_fb_helper.h| 39 > 5 files changed, 9 insertions(+), 155 deletions(-) Nice diffstat! Assuming there is a good explanation on the dirty check: Reviewed-by: Sam Ravnborg
Re: [PATCH v4 11/13] drm/fb-helper: Export helpers for marking damage areas
On Wed, May 24, 2023 at 11:21:48AM +0200, Thomas Zimmermann wrote:
> Export drm_fb_helper_damage() and drm_fb_helper_damage_range(), which
> handle damage areas for fbdev emulation. This is a temporary export
> that allows to move the DRM I/O helpers for fbdev into drivers. Only
> fbdev-generic and i915 need them. Both will be updated to implement
> damage handling by themselves and the exported functions will be removed.
>
> v4:
> * update interfaces
>
> Signed-off-by: Thomas Zimmermann
Assuming there is a good answer why there is no dirty check:
Reviewed-by: Sam Ravnborg
> ---
> drivers/gpu/drm/drm_fb_helper.c | 22 ++
> include/drm/drm_fb_helper.h | 3 +++
> 2 files changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index f0e9549b6bd7..cb03099fd2e3 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -670,6 +670,28 @@ static void drm_fb_helper_memory_range_to_clip(struct
> fb_info *info, off_t off,
> drm_rect_init(clip, x1, y1, x2 - x1, y2 - y1);
> }
>
> +/* Don't use in new code. */
> +void drm_fb_helper_damage_range(struct fb_info *info, off_t off, size_t len)
> +{
> + struct drm_fb_helper *fb_helper = info->par;
> + struct drm_rect damage_area;
> +
> + drm_fb_helper_memory_range_to_clip(info, off, len, _area);
> + drm_fb_helper_damage(fb_helper, damage_area.x1, damage_area.y1,
> + drm_rect_width(_area),
> + drm_rect_height(_area));
> +}
> +EXPORT_SYMBOL(drm_fb_helper_damage_range);
> +
> +/* Don't use in new code. */
> +void drm_fb_helper_damage_area(struct fb_info *info, u32 x, u32 y, u32
> width, u32 height)
> +{
> + struct drm_fb_helper *fb_helper = info->par;
> +
> + drm_fb_helper_damage(fb_helper, x, y, width, height);
> +}
> +EXPORT_SYMBOL(drm_fb_helper_damage_area);
> +
> /**
> * drm_fb_helper_deferred_io() - fbdev deferred_io callback function
> * @info: fb_info struct pointer
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 72032c354a30..7d5804882be7 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -253,6 +253,9 @@ void drm_fb_helper_fill_info(struct fb_info *info,
>struct drm_fb_helper *fb_helper,
>struct drm_fb_helper_surface_size *sizes);
>
> +void drm_fb_helper_damage_range(struct fb_info *info, off_t off, size_t len);
> +void drm_fb_helper_damage_area(struct fb_info *info, u32 x, u32 y, u32
> width, u32 height);
> +
> void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head
> *pagereflist);
>
> ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
> --
> 2.40.1
Re: [PATCH v4 00/13] drm/fbdev: Remove DRM's helpers for fbdev I/O
Hi Thomas. > v4: > * use initializer and generator macros for struct fb_ops > * partially support damage handling in msm (Dmitri) I like the macros. They make it simpler and we do not spread the _cfb_ misname to more files. > v3: > * fix Kconfig options (Jingfeng) > * minimize changes to exynos (Sam) > v2: > * simplify Kconfig handling (Sam) > > Thomas Zimmermann (13): > fbdev: Add Kconfig options to select different fb_ops helpers > fbdev: Add initializer macros for struct fb_ops > drm/armada: Use regular fbdev I/O helpers > drm/exynos: Use regular fbdev I/O helpers > drm/gma500: Use regular fbdev I/O helpers > drm/radeon: Use regular fbdev I/O helpers > drm/fbdev-dma: Use regular fbdev I/O helpers > drm/msm: Use regular fbdev I/O helpers > drm/omapdrm: Use regular fbdev I/O helpers > drm/tegra: Use regular fbdev I/O helpers These are all: Acked-by: Sam Ravnborg
Re: [PATCH v4 13/13] drm/i915: Implement dedicated fbdev I/O helpers
Hi Thomas,
On Wed, May 24, 2023 at 11:21:50AM +0200, Thomas Zimmermann wrote:
> Implement dedicated fbdev helpers for framebuffer I/O instead
> of using DRM's helpers. Use an fbdev generator macro for
> deferred I/O to create the fbdev callbacks. i915 was the only
> caller of the DRM helpers, so remove them from the helper module.
>
> i915's fbdev emulation is still incomplete as it doesn't implement
> deferred I/O and damage handling for mmaped pages.
>
> v4:
> * generate deferred-I/O helpers
> * use initializer macros for fb_ops
> v2:
> * use FB_IO_HELPERS options
>
> Signed-off-by: Thomas Zimmermann
> Cc: Jani Nikula
> Cc: Joonas Lahtinen
> Cc: Rodrigo Vivi
> Cc: Tvrtko Ursulin
> Cc: "Ville Syrjälä"
> ---
> drivers/gpu/drm/Kconfig| 3 -
> drivers/gpu/drm/drm_fb_helper.c| 107 -
> drivers/gpu/drm/i915/Kconfig | 1 +
> drivers/gpu/drm/i915/display/intel_fbdev.c | 14 +--
> include/drm/drm_fb_helper.h| 39
> 5 files changed, 9 insertions(+), 155 deletions(-)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 92a782827b7b..bb2e48cc6cd6 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -133,9 +133,6 @@ config DRM_FBDEV_EMULATION
> bool "Enable legacy fbdev support for your modesetting driver"
> depends on DRM_KMS_HELPER
> depends on FB=y || FB=DRM_KMS_HELPER
> - select FB_CFB_FILLRECT
> - select FB_CFB_COPYAREA
> - select FB_CFB_IMAGEBLIT
> select FRAMEBUFFER_CONSOLE if !EXPERT
> select FRAMEBUFFER_CONSOLE_DETECT_PRIMARY if FRAMEBUFFER_CONSOLE
> default y
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index bab6b252f02a..9978147bbc8a 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -736,113 +736,6 @@ void drm_fb_helper_deferred_io(struct fb_info *info,
> struct list_head *pagerefli
> }
> EXPORT_SYMBOL(drm_fb_helper_deferred_io);
>
> -/**
> - * drm_fb_helper_cfb_read - Implements struct _ops.fb_read for I/O memory
> - * @info: fb_info struct pointer
> - * @buf: userspace buffer to read from framebuffer memory
> - * @count: number of bytes to read from framebuffer memory
> - * @ppos: read offset within framebuffer memory
> - *
> - * Returns:
> - * The number of bytes read on success, or an error code otherwise.
> - */
> -ssize_t drm_fb_helper_cfb_read(struct fb_info *info, char __user *buf,
> -size_t count, loff_t *ppos)
> -{
> - return fb_io_read(info, buf, count, ppos);
> -}
> -EXPORT_SYMBOL(drm_fb_helper_cfb_read);
> -
> -/**
> - * drm_fb_helper_cfb_write - Implements struct _ops.fb_write for I/O
> memory
> - * @info: fb_info struct pointer
> - * @buf: userspace buffer to write to framebuffer memory
> - * @count: number of bytes to write to framebuffer memory
> - * @ppos: write offset within framebuffer memory
> - *
> - * Returns:
> - * The number of bytes written on success, or an error code otherwise.
> - */
> -ssize_t drm_fb_helper_cfb_write(struct fb_info *info, const char __user *buf,
> - size_t count, loff_t *ppos)
> -{
> - struct drm_fb_helper *helper = info->par;
> - loff_t pos = *ppos;
> - ssize_t ret;
> - struct drm_rect damage_area;
> -
> - ret = fb_io_write(info, buf, count, ppos);
> - if (ret <= 0)
> - return ret;
> -
> - if (helper->funcs->fb_dirty) {
> - drm_fb_helper_memory_range_to_clip(info, pos, ret,
> _area);
> - drm_fb_helper_damage(helper, damage_area.x1, damage_area.y1,
> - drm_rect_width(_area),
> - drm_rect_height(_area));
> - }
The generated helpers do not have the if (helper->funcs->fb_dirty)
check.
Is this implemented somewhere else that I missed?
Sam
> -
> - return ret;
> -}
> -EXPORT_SYMBOL(drm_fb_helper_cfb_write);
> -
> -/**
> - * drm_fb_helper_cfb_fillrect - wrapper around cfb_fillrect
> - * @info: fbdev registered by the helper
> - * @rect: info about rectangle to fill
> - *
> - * A wrapper around cfb_fillrect implemented by fbdev core
> - */
> -void drm_fb_helper_cfb_fillrect(struct fb_info *info,
> - const struct fb_fillrect *rect)
> -{
> - struct drm_fb_helper *helper = info->par;
> -
> - cfb_fillrect(info, rect);
> -
> - if (helper->funcs->fb_dirty)
> - drm_fb_helper_damage(helper, rect->dx, rect->dy, rect->width,
> rect->height);
> -}
> -EXPORT_SYMBOL(drm_fb_helper_cfb_fillrect);
> -
> -/**
> - * drm_fb_helper_cfb_copyarea - wrapper around cfb_copyarea
> - * @info: fbdev registered by the helper
> - * @area: info about area to copy
> - *
> - * A wrapper around cfb_copyarea implemented by fbdev core
> - */
> -void drm_fb_helper_cfb_copyarea(struct fb_info *info,
> - const struct
Re: [PATCH v4 03/13] drm/armada: Use regular fbdev I/O helpers
On Wed, May 24, 2023 at 11:21:40AM +0200, Thomas Zimmermann wrote: > Use the regular fbdev helpers for framebuffer I/O instead of DRM's > helpers. Armada does not use damage handling, so DRM's fbdev helpers > are mere wrappers around the fbdev code. > > By using fbdev helpers directly within each DRM fbdev emulation, > we can eventually remove DRM's wrapper functions entirely. > > v4: > * use initializer macros for struct fb_ops > v2: > * use FB_IO_HELPERS option > > Signed-off-by: Thomas Zimmermann > Cc: Russell King Acked-by: Sam Ravnborg
Re: [PATCH v4 02/13] fbdev: Add initializer macros for struct fb_ops
On Wed, May 24, 2023 at 11:21:39AM +0200, Thomas Zimmermann wrote: > For framebuffers in I/O and system memory, add macros that set > struct fb_ops to the respective callback functions. > > For deferred I/O, add macros that generate callback functions with > damage handling. Add initializer macros that set struct fb_ops to > the generated callbacks. > > These macros can remove a lot boilerplate code from fbdev drivers. > The drivers are supposed to use the macro that is required for its > framebuffer. Each macro is split into smaller helpers, so that > drivers with non-standard callbacks can pick and customize callbacks > as needed. There are individual helper macros for read/write, mmap > and drawing. > > Signed-off-by: Thomas Zimmermann I am not a fan of public functions/macros names __something. But I see it used in so many places, so maybe it is just me. And everything looks consistent, so OK. With the white space issues fixed: Reviewed-by: Sam Ravnborg
Re: [PATCH v4 01/13] fbdev: Add Kconfig options to select different fb_ops helpers
Hi Thomas, On Wed, May 24, 2023 at 11:21:38AM +0200, Thomas Zimmermann wrote: > Many fbdev drivers use the same set of fb_ops helpers. Add Kconfig > options to select them at once. This will help with making DRM's > fbdev emulation code more modular, but can also be used to simplify > fbdev's driver configs. > > v3: > * fix select statement (Jingfeng) > > Signed-off-by: Thomas Zimmermann I like these, thanks. Reviewed-by: Sam Ravnborg
Re: [PATCH v2 02/12] drm/armada: Use regular fbdev I/O helpers
Hi Thomas,
On Mon, May 15, 2023 at 11:40:23AM +0200, Thomas Zimmermann wrote:
> Use the regular fbdev helpers for framebuffer I/O instead of DRM's
> helpers. Armada does not use damage handling, so DRM's fbdev helpers
> are mere wrappers around the fbdev code.
>
> By using fbdev helpers directly within each DRM fbdev emulation,
> we can eventually remove DRM's wrapper functions entirely.
>
> v2:
> * use FB_IO_HELPERS option
>
> Signed-off-by: Thomas Zimmermann
> Cc: Russell King
> ---
> drivers/gpu/drm/armada/Kconfig| 1 +
> drivers/gpu/drm/armada/armada_fbdev.c | 9 -
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/armada/Kconfig b/drivers/gpu/drm/armada/Kconfig
> index f5c66d89ba99..5afade25e217 100644
> --- a/drivers/gpu/drm/armada/Kconfig
> +++ b/drivers/gpu/drm/armada/Kconfig
> @@ -3,6 +3,7 @@ config DRM_ARMADA
> tristate "DRM support for Marvell Armada SoCs"
> depends on DRM && HAVE_CLK && ARM && MMU
> select DRM_KMS_HELPER
> + select FB_IO_HELPERS if DRM_FBDEV_EMULATION
> help
> Support the "LCD" controllers found on the Marvell Armada 510
> devices. There are two controllers on the device, each controller
> diff --git a/drivers/gpu/drm/armada/armada_fbdev.c
> b/drivers/gpu/drm/armada/armada_fbdev.c
> index 0a5fd1aa86eb..6c3bbaf53569 100644
> --- a/drivers/gpu/drm/armada/armada_fbdev.c
> +++ b/drivers/gpu/drm/armada/armada_fbdev.c
> @@ -5,6 +5,7 @@
> */
>
> #include
> +#include
> #include
> #include
>
> @@ -34,11 +35,9 @@ static void armada_fbdev_fb_destroy(struct fb_info *info)
> static const struct fb_ops armada_fb_ops = {
> .owner = THIS_MODULE,
> DRM_FB_HELPER_DEFAULT_OPS,
> - .fb_read= drm_fb_helper_cfb_read,
> - .fb_write = drm_fb_helper_cfb_write,
I had expected to see
.fb_read = fb_io_read,
But maybe this only used when using damage handling?
Likewise for drm_fb_helper_cfb_write.
??
> - .fb_fillrect= drm_fb_helper_cfb_fillrect,
> - .fb_copyarea= drm_fb_helper_cfb_copyarea,
> - .fb_imageblit = drm_fb_helper_cfb_imageblit,
> + .fb_fillrect= cfb_fillrect,
> + .fb_copyarea= cfb_copyarea,
> + .fb_imageblit = cfb_imageblit,
This part is as expected.
Sam
> .fb_destroy = armada_fbdev_fb_destroy,
> };
>
> --
> 2.40.1
Re: [PATCH v2 03/12] drm/exynos: Use regular fbdev I/O helpers
Hi Thomas,
On Mon, May 15, 2023 at 11:40:24AM +0200, Thomas Zimmermann wrote:
> Use the regular fbdev helpers for framebuffer I/O instead of DRM's
> helpers. Exynos does not use damage handling, so DRM's fbdev helpers
> are mere wrappers around the fbdev code.
>
> By using fbdev helpers directly within each DRM fbdev emulation,
> we can eventually remove DRM's wrapper functions entirely.
>
> v2:
> * use FB_IO_HELPERS option
>
> Signed-off-by: Thomas Zimmermann
> Cc: Inki Dae
> Cc: Seung-Woo Kim
> Cc: Kyungmin Park
> Cc: Krzysztof Kozlowski
> Cc: Alim Akhtar
> ---
> drivers/gpu/drm/exynos/Kconfig| 1 +
> drivers/gpu/drm/exynos/Makefile | 2 +-
> drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 10 +-
> 3 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig
> index 0cb92d651ff1..7ca7e1dab52c 100644
> --- a/drivers/gpu/drm/exynos/Kconfig
> +++ b/drivers/gpu/drm/exynos/Kconfig
> @@ -7,6 +7,7 @@ config DRM_EXYNOS
> select DRM_DISPLAY_HELPER if DRM_EXYNOS_DP
> select DRM_KMS_HELPER
> select VIDEOMODE_HELPERS
> + select FB_IO_HELPERS if DRM_FBDEV_EMULATION
> select SND_SOC_HDMI_CODEC if SND_SOC
> help
> Choose this option if you have a Samsung SoC Exynos chipset.
> diff --git a/drivers/gpu/drm/exynos/Makefile b/drivers/gpu/drm/exynos/Makefile
> index 2fd2f3ee4fcf..233a66036584 100644
> --- a/drivers/gpu/drm/exynos/Makefile
> +++ b/drivers/gpu/drm/exynos/Makefile
> @@ -6,7 +6,6 @@
> exynosdrm-y := exynos_drm_drv.o exynos_drm_crtc.o exynos_drm_fb.o \
> exynos_drm_gem.o exynos_drm_plane.o exynos_drm_dma.o
>
> -exynosdrm-$(CONFIG_DRM_FBDEV_EMULATION) += exynos_drm_fbdev.o
> exynosdrm-$(CONFIG_DRM_EXYNOS_FIMD) += exynos_drm_fimd.o
> exynosdrm-$(CONFIG_DRM_EXYNOS5433_DECON) += exynos5433_drm_decon.o
> exynosdrm-$(CONFIG_DRM_EXYNOS7_DECON)+= exynos7_drm_decon.o
> @@ -23,5 +22,6 @@ exynosdrm-$(CONFIG_DRM_EXYNOS_ROTATOR) +=
> exynos_drm_rotator.o
> exynosdrm-$(CONFIG_DRM_EXYNOS_SCALER)+= exynos_drm_scaler.o
> exynosdrm-$(CONFIG_DRM_EXYNOS_GSC) += exynos_drm_gsc.o
> exynosdrm-$(CONFIG_DRM_EXYNOS_MIC) += exynos_drm_mic.o
> +exynosdrm-$(CONFIG_DRM_FBDEV_EMULATION) += exynos_drm_fbdev.o
What does this change do?
Maybe something that was left by accident?
Sam
>
> obj-$(CONFIG_DRM_EXYNOS) += exynosdrm.o
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index ea4b3d248aac..bdd1d087 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -8,6 +8,8 @@
> * Seung-Woo Kim
> */
>
> +#include
> +
> #include
> #include
> #include
> @@ -49,11 +51,9 @@ static const struct fb_ops exynos_drm_fb_ops = {
> .owner = THIS_MODULE,
> DRM_FB_HELPER_DEFAULT_OPS,
> .fb_mmap= exynos_drm_fb_mmap,
> - .fb_read= drm_fb_helper_cfb_read,
> - .fb_write = drm_fb_helper_cfb_write,
> - .fb_fillrect= drm_fb_helper_cfb_fillrect,
> - .fb_copyarea= drm_fb_helper_cfb_copyarea,
> - .fb_imageblit = drm_fb_helper_cfb_imageblit,
> + .fb_fillrect= cfb_fillrect,
> + .fb_copyarea= cfb_copyarea,
> + .fb_imageblit = cfb_imageblit,
> .fb_destroy = exynos_drm_fb_destroy,
> };
>
> --
> 2.40.1
Re: [PATCH 00/11] drm/fbdev: Remove DRM's helpers for fbdev I/O
Hi Thomas, > > > > Nice cleanup. > > > > From one of the patches: > > > > > +config DRM_ARMADA_FBDEV_EMULATION > > > + bool > > > + depends on DRM_ARMADA > > > + select FB_CFB_COPYAREA > > > + select FB_CFB_FILLRECT > > > + select FB_CFB_IMAGEBLIT > > > > This seems like a hard to maintain way to select a few helper functions. > > Today we have LD_DEAD_CODE_DATA_ELIMINATION for the configs that care > > about size - and that should work here as well. > > I wasn't too happy about this solution either as it is quite verbose. But I > don't want to rely on the linker either. It certainly cannot remove exported > symbols. I forgot about exported symbols - that makes the idea futile. > > But the pattern is very common among the fbdev drivers. We could introduce > common Kconfig options in fbdev and selcet those instead. Like this: > > const FB_IO_HELPERS > bool > depends on FB > select FB_CFB_COPYAREA > select FB_CFB_FILLRECT > select FB_CFB_IMAGEBLIT > > const FB_SYS_HELPERS > bool > depends on FB > select FB_SYS_COPYAREA > select FB_SYS_FILLRECT > select FB_SYS_FOPS > select FB_SYS_IMAGEBLIT > > Apart from DRM, most of the fbdev drivers could use these as well. That's a much nicer way to express it - and with this we do not introduce the IMO confusing CFB (Color Frame Buffer) abbreviation in every driver. Sam
Re: [PATCH 00/11] drm/fbdev: Remove DRM's helpers for fbdev I/O
Hi Thomas, On Fri, May 12, 2023 at 10:41:41AM +0200, Thomas Zimmermann wrote: > DRM provides a number of wrappers around fbdev cfb_() sys_(), fb_io_() > and fb_sys_() helpers. The DRM functions don't provide any additional > functionality for most DRM drivers. So remove them and call the fbdev > I/O helpers directly. > > The DRM fbdev I/O wrappers were originally added because > does not protect its content with CONFIG_FB. DRM fbdev emulation did > not build if the the config option had been disabled. This has been > fixed. For fbdev-generic and i915, the wrappers added support for damage > handling. But this is better handled within the two callers, as each > is special in its damage handling. > > Patches 1 to 8 replace the DRM wrappers in a number of fbdev emulations. > Patch 9 exports two helpers for damage handling. Patches 10 and 11 > update fbdev-generic and i915 with the help of the exported functions. > The patches also remove DRM's fbdev I/O helpers, which are now unused. > > DRM's fbdev helpers had to select fbdev I/O helpers for I/O and for > system memory. Each fbdev emulation now selects the correct helpers > for itself. Depending on the selected DRM drivers, kernel builds will > now only contain the necessary fbdev I/O helpers and might be slightly > smaller in size. Nice cleanup. >From one of the patches: > +config DRM_ARMADA_FBDEV_EMULATION > + bool > + depends on DRM_ARMADA > + select FB_CFB_COPYAREA > + select FB_CFB_FILLRECT > + select FB_CFB_IMAGEBLIT This seems like a hard to maintain way to select a few helper functions. Today we have LD_DEAD_CODE_DATA_ELIMINATION for the configs that care about size - and that should work here as well. I understand where this comes from and I am not against the solution, but wanted to point at a more modern approach to deal with the bloat. Maybe some of the embbedded folks can tell if LD_DEAD_CODE_DATA_ELIMINATION can be trusted yet or that is something for the future. In barebox -ffunction-section (what LD_DEAD_CODE_DATA_ELIMINATION enables) is used with success - there it really helps when generating different barebox images where size matters a lot. Sam
Re: [PATCH v3 04/10] drm/fbdev-generic: Initialize fb-helper structure in generic setup
On Fri, Jan 27, 2023 at 03:21:30PM +0100, Thomas Zimmermann wrote: > Hi > > Am 25.01.23 um 22:03 schrieb Sam Ravnborg: > > Hi Thomas, > > > > On Wed, Jan 25, 2023 at 09:04:09PM +0100, Thomas Zimmermann wrote: > > > Initialize the fb-helper structure immediately after its allocation > > > in drm_fbdev_generic_setup(). That will make it easier to fill it with > > > driver-specific values, such as the preferred BPP. > > > > > > Signed-off-by: Thomas Zimmermann > > > Reviewed-by: Javier Martinez Canillas > > > --- > > > drivers/gpu/drm/drm_fbdev_generic.c | 13 + > > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_fbdev_generic.c > > > b/drivers/gpu/drm/drm_fbdev_generic.c > > > index 135d58b8007b..63f66325a8a5 100644 > > > --- a/drivers/gpu/drm/drm_fbdev_generic.c > > > +++ b/drivers/gpu/drm/drm_fbdev_generic.c > > > @@ -385,8 +385,6 @@ static int drm_fbdev_client_hotplug(struct > > > drm_client_dev *client) > > > if (dev->fb_helper) > > > return drm_fb_helper_hotplug_event(dev->fb_helper); > > > - drm_fb_helper_prepare(dev, fb_helper, _fb_helper_generic_funcs); > > > - > > > ret = drm_fb_helper_init(dev, fb_helper); > > > if (ret) > > > goto err; > > > > From the documentation: > > The drm_fb_helper_prepare() > > helper must be called first to initialize the minimum required to make > > hotplug detection work. > > ... > > To finish up the fbdev helper initialization, the > > drm_fb_helper_init() function is called. > > > > So this change do not follow the documentation as drm_fb_helper_init() > > is now called before drm_fb_helper_prepare() > > No, we now call drm_fb_helper_prepare() from within > drm_fbdev_generic_setup(), right after allocating the fb_helper. > drm_fb_helper_init() will only be called after the client received a > hot-plug event. > > > > > I did not follow all the code - but my gut feeling is that the > > documentation is right. > > The docs are of low quality. The _prepare() helper is the actual init > function and _init() only sets the fb_helper in the device instance. OK, thanks for the follow-up. Sam
Re: [PATCH v3 01/10] drm/client: Test for connectors before sending hotplug event
On Fri, Jan 27, 2023 at 03:13:50PM +0100, Thomas Zimmermann wrote:
> Hi
>
> Am 25.01.23 um 21:52 schrieb Sam Ravnborg:
> > Hi Thomas,
> >
> > On Wed, Jan 25, 2023 at 09:04:06PM +0100, Thomas Zimmermann wrote:
> > > Test for connectors in the client code and remove a similar test
> > > from the generic fbdev emulation. Do nothing if the test fails.
> > > Not having connectors indicates a driver bug.
> > >
> > > Signed-off-by: Thomas Zimmermann
> > > Reviewed-by: Javier Martinez Canillas
> > > ---
> > > drivers/gpu/drm/drm_client.c| 5 +
> > > drivers/gpu/drm/drm_fbdev_generic.c | 5 -
> > > 2 files changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> > > index 262ec64d4397..09ac191c202d 100644
> > > --- a/drivers/gpu/drm/drm_client.c
> > > +++ b/drivers/gpu/drm/drm_client.c
> > > @@ -198,6 +198,11 @@ void drm_client_dev_hotplug(struct drm_device *dev)
> > > if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > > return;
> > > + if (!dev->mode_config.num_connector) {
> > > + drm_dbg_kms(dev, "No connectors found, will not send hotplug
> > > events!\n");
> > > + return;
> > This deserves a more visible logging - if a driver fails here it would
> > be good to spot it in the normal kernel log.
> > drm_info or drm_notice?
>
> But is that really noteworthy? AFAIK, this situation can legally happen. So
> if it's expected, why should we print a message about it?
I was reading it as a driver error - as that's not the case current code
is fine.
Sam
Re: [PATCH v3 04/10] drm/fbdev-generic: Initialize fb-helper structure in generic setup
Hi Thomas,
On Wed, Jan 25, 2023 at 09:04:09PM +0100, Thomas Zimmermann wrote:
> Initialize the fb-helper structure immediately after its allocation
> in drm_fbdev_generic_setup(). That will make it easier to fill it with
> driver-specific values, such as the preferred BPP.
>
> Signed-off-by: Thomas Zimmermann
> Reviewed-by: Javier Martinez Canillas
> ---
> drivers/gpu/drm/drm_fbdev_generic.c | 13 +
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c
> b/drivers/gpu/drm/drm_fbdev_generic.c
> index 135d58b8007b..63f66325a8a5 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -385,8 +385,6 @@ static int drm_fbdev_client_hotplug(struct drm_client_dev
> *client)
> if (dev->fb_helper)
> return drm_fb_helper_hotplug_event(dev->fb_helper);
>
> - drm_fb_helper_prepare(dev, fb_helper, _fb_helper_generic_funcs);
> -
> ret = drm_fb_helper_init(dev, fb_helper);
> if (ret)
> goto err;
>From the documentation:
The drm_fb_helper_prepare()
helper must be called first to initialize the minimum required to make
hotplug detection work.
...
To finish up the fbdev helper initialization, the
drm_fb_helper_init() function is called.
So this change do not follow the documentation as drm_fb_helper_init()
is now called before drm_fb_helper_prepare()
I did not follow all the code - but my gut feeling is that the
documentation is right.
Sam
> @@ -456,12 +454,12 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
> fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
> if (!fb_helper)
> return;
> + drm_fb_helper_prepare(dev, fb_helper, _fb_helper_generic_funcs);
>
> ret = drm_client_init(dev, _helper->client, "fbdev",
> _fbdev_client_funcs);
> if (ret) {
> - kfree(fb_helper);
> drm_err(dev, "Failed to register client: %d\n", ret);
> - return;
> + goto err_drm_client_init;
> }
>
> /*
> @@ -484,5 +482,12 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
> drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
>
> drm_client_register(_helper->client);
> +
> + return;
> +
> +err_drm_client_init:
> + drm_fb_helper_unprepare(fb_helper);
> + kfree(fb_helper);
> + return;
> }
> EXPORT_SYMBOL(drm_fbdev_generic_setup);
> --
> 2.39.0
Re: [PATCH v3 02/10] drm/client: Add hotplug_failed flag
Hi Thomas,
On Wed, Jan 25, 2023 at 09:04:07PM +0100, Thomas Zimmermann wrote:
> Signal failed hotplugging with a flag in struct drm_client_dev. If set,
> the client helpers will not further try to set up the fbdev display.
>
> This used to be signalled with a combination of cleared pointers in
> struct drm_fb_helper,
I failed to find where we clear the pointers. What do I miss?
(I had assumed we would stop clearing the pointers after this change).
Sam
which prevents us from initializing these pointers
> early after allocation.
>
> The change also harmonizes behavior among DRM clients. Additional DRM
> clients will now handle failed hotplugging like fbdev does.
>
> Signed-off-by: Thomas Zimmermann
> Reviewed-by: Javier Martinez Canillas
> ---
> drivers/gpu/drm/drm_client.c| 5 +
> drivers/gpu/drm/drm_fbdev_generic.c | 4
> include/drm/drm_client.h| 8
> 3 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index 09ac191c202d..009e7b10455c 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -208,8 +208,13 @@ void drm_client_dev_hotplug(struct drm_device *dev)
> if (!client->funcs || !client->funcs->hotplug)
> continue;
>
> + if (client->hotplug_failed)
> + continue;
> +
> ret = client->funcs->hotplug(client);
> drm_dbg_kms(dev, "%s: ret=%d\n", client->name, ret);
> + if (ret)
> + client->hotplug_failed = true;
> }
> mutex_unlock(>clientlist_mutex);
> }
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c
> b/drivers/gpu/drm/drm_fbdev_generic.c
> index 3d455a2e3fb5..135d58b8007b 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -382,10 +382,6 @@ static int drm_fbdev_client_hotplug(struct
> drm_client_dev *client)
> struct drm_device *dev = client->dev;
> int ret;
>
> - /* Setup is not retried if it has failed */
> - if (!fb_helper->dev && fb_helper->funcs)
> - return 0;
> -
> if (dev->fb_helper)
> return drm_fb_helper_hotplug_event(dev->fb_helper);
>
> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> index 4fc8018eddda..39482527a775 100644
> --- a/include/drm/drm_client.h
> +++ b/include/drm/drm_client.h
> @@ -106,6 +106,14 @@ struct drm_client_dev {
>* @modesets: CRTC configurations
>*/
> struct drm_mode_set *modesets;
> +
> + /**
> + * @hotplug failed:
> + *
> + * Set by client hotplug helpers if the hotplugging failed
> + * before. It is usually not tried again.
> + */
> + bool hotplug_failed;
> };
>
> int drm_client_init(struct drm_device *dev, struct drm_client_dev *client,
> --
> 2.39.0
Re: [PATCH v3 01/10] drm/client: Test for connectors before sending hotplug event
Hi Thomas,
On Wed, Jan 25, 2023 at 09:04:06PM +0100, Thomas Zimmermann wrote:
> Test for connectors in the client code and remove a similar test
> from the generic fbdev emulation. Do nothing if the test fails.
> Not having connectors indicates a driver bug.
>
> Signed-off-by: Thomas Zimmermann
> Reviewed-by: Javier Martinez Canillas
> ---
> drivers/gpu/drm/drm_client.c| 5 +
> drivers/gpu/drm/drm_fbdev_generic.c | 5 -
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index 262ec64d4397..09ac191c202d 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -198,6 +198,11 @@ void drm_client_dev_hotplug(struct drm_device *dev)
> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> return;
>
> + if (!dev->mode_config.num_connector) {
> + drm_dbg_kms(dev, "No connectors found, will not send hotplug
> events!\n");
> + return;
This deserves a more visible logging - if a driver fails here it would
be good to spot it in the normal kernel log.
drm_info or drm_notice?
The original code had this on the debug level, but when moving the log
level could also be updated.
Sam
> + }
> +
> mutex_lock(>clientlist_mutex);
> list_for_each_entry(client, >clientlist, list) {
> if (!client->funcs || !client->funcs->hotplug)
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c
> b/drivers/gpu/drm/drm_fbdev_generic.c
> index 0a4c160e0e58..3d455a2e3fb5 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -389,11 +389,6 @@ static int drm_fbdev_client_hotplug(struct
> drm_client_dev *client)
> if (dev->fb_helper)
> return drm_fb_helper_hotplug_event(dev->fb_helper);
>
> - if (!dev->mode_config.num_connector) {
> - drm_dbg_kms(dev, "No connectors found, will not create
> framebuffer!\n");
> - return 0;
> - }
> -
> drm_fb_helper_prepare(dev, fb_helper, _fb_helper_generic_funcs);
>
> ret = drm_fb_helper_init(dev, fb_helper);
> --
> 2.39.0
Re: [PATCH 10/10] drm/fbdev-generic: Rename struct fb_info 'fbi' to 'info'
Hi Thomas,
a quick drive-by comment.
On Mon, Jan 23, 2023 at 11:05:59AM +0100, Thomas Zimmermann wrote:
> The generic fbdev emulation names variables of type struct fb_info
> both 'fbi' and 'info'. The latter seems to be more common in fbdev
> code, so name fbi accordingly.
>
> Also replace the duplicate variable in drm_fbdev_fb_destroy().
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_fbdev_generic.c | 49 ++---
> 1 file changed, 24 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c
> b/drivers/gpu/drm/drm_fbdev_generic.c
> index 49a0bba86ce7..7633da5c13c3 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -46,17 +46,16 @@ static int drm_fbdev_fb_release(struct fb_info *info, int
> user)
> static void drm_fbdev_fb_destroy(struct fb_info *info)
> {
> struct drm_fb_helper *fb_helper = info->par;
> - struct fb_info *fbi = fb_helper->info;
> void *shadow = NULL;
>
> if (!fb_helper->dev)
> return;
>
> - if (fbi) {
> - if (fbi->fbdefio)
> - fb_deferred_io_cleanup(fbi);
> + if (info) {
As info is already used above to find fb_helper, this check is
redundant.
Sam
> + if (info->fbdefio)
> + fb_deferred_io_cleanup(info);
> if (drm_fbdev_use_shadow_fb(fb_helper))
> - shadow = fbi->screen_buffer;
> + shadow = info->screen_buffer;
> }
>
> drm_fb_helper_fini(fb_helper);
> @@ -173,7 +172,7 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper
> *fb_helper,
> struct drm_device *dev = fb_helper->dev;
> struct drm_client_buffer *buffer;
> struct drm_framebuffer *fb;
> - struct fb_info *fbi;
> + struct fb_info *info;
> u32 format;
> struct iosys_map map;
> int ret;
> @@ -192,35 +191,35 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper
> *fb_helper,
> fb_helper->fb = buffer->fb;
> fb = buffer->fb;
>
> - fbi = drm_fb_helper_alloc_info(fb_helper);
> - if (IS_ERR(fbi))
> - return PTR_ERR(fbi);
> + info = drm_fb_helper_alloc_info(fb_helper);
> + if (IS_ERR(info))
> + return PTR_ERR(info);
>
> - fbi->fbops = _fbdev_fb_ops;
> - fbi->screen_size = sizes->surface_height * fb->pitches[0];
> - fbi->fix.smem_len = fbi->screen_size;
> - fbi->flags = FBINFO_DEFAULT;
> + info->fbops = _fbdev_fb_ops;
> + info->screen_size = sizes->surface_height * fb->pitches[0];
> + info->fix.smem_len = info->screen_size;
> + info->flags = FBINFO_DEFAULT;
>
> - drm_fb_helper_fill_info(fbi, fb_helper, sizes);
> + drm_fb_helper_fill_info(info, fb_helper, sizes);
>
> if (drm_fbdev_use_shadow_fb(fb_helper)) {
> - fbi->screen_buffer = vzalloc(fbi->screen_size);
> - if (!fbi->screen_buffer)
> + info->screen_buffer = vzalloc(info->screen_size);
> + if (!info->screen_buffer)
> return -ENOMEM;
> - fbi->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
> + info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
>
> - fbi->fbdefio = _fbdev_defio;
> - fb_deferred_io_init(fbi);
> + info->fbdefio = _fbdev_defio;
> + fb_deferred_io_init(info);
> } else {
> /* buffer is mapped for HW framebuffer */
> ret = drm_client_buffer_vmap(fb_helper->buffer, );
> if (ret)
> return ret;
> if (map.is_iomem) {
> - fbi->screen_base = map.vaddr_iomem;
> + info->screen_base = map.vaddr_iomem;
> } else {
> - fbi->screen_buffer = map.vaddr;
> - fbi->flags |= FBINFO_VIRTFB;
> + info->screen_buffer = map.vaddr;
> + info->flags |= FBINFO_VIRTFB;
> }
>
> /*
> @@ -229,10 +228,10 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper
> *fb_helper,
>* case.
>*/
> #if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> - if (fb_helper->hint_leak_smem_start && fbi->fix.smem_start == 0
> &&
> + if (fb_helper->hint_leak_smem_start && info->fix.smem_start ==
> 0 &&
> !drm_WARN_ON_ONCE(dev, map.is_iomem))
> - fbi->fix.smem_start =
> - page_to_phys(virt_to_page(fbi->screen_buffer));
> + info->fix.smem_start =
> + page_to_phys(virt_to_page(info->screen_buffer));
> #endif
> }
>
> --
> 2.39.0
Re: [PATCH v2] drm/radeon: Do not use deprecated drm log API
Hi Nirmoy, On Tue, Jan 17, 2023 at 07:04:16PM +0100, Nirmoy Das wrote: > Replace deprecated DRM_DEBUG_KMS_RATELIMITED() and DRM_ERROR() > with proper APIs. Thanks for working on this! Also thanks for Christian to take care and apply the patches. Sam
Re: [PATCH 2/2] drm_print: Remove deprecated DRM_DEBUG_KMS_RATELIMITED()
On Tue, Jan 17, 2023 at 06:44:47PM +0100, Nirmoy Das wrote: > There are no current users of DRM_DEBUG_KMS_RATELIMITED() > so remove it. Thanks > > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: David Airlie > Cc: Daniel Vetter > Cc: Sam Ravnborg > > Signed-off-by: Nirmoy Das Reviewed-by: Sam Ravnborg > --- > include/drm/drm_print.h | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h > index a44fb7ef257f..c3753da97c4e 100644 > --- a/include/drm/drm_print.h > +++ b/include/drm/drm_print.h > @@ -605,9 +605,6 @@ void __drm_err(const char *format, ...); > #define drm_dbg_kms_ratelimited(drm, fmt, ...) \ > __DRM_DEFINE_DBG_RATELIMITED(KMS, drm, fmt, ## __VA_ARGS__) > > -/* NOTE: this is deprecated in favor of drm_dbg_kms_ratelimited(NULL, ...). > */ > -#define DRM_DEBUG_KMS_RATELIMITED(fmt, ...) drm_dbg_kms_ratelimited(NULL, > fmt, ## __VA_ARGS__) > - > /* > * struct drm_device based WARNs > * > -- > 2.39.0
Re: [PATCH 00/22] drm: Remove includes for drm_crtc_helper.h
Hi Thomas. On Mon, Jan 16, 2023 at 02:12:13PM +0100, Thomas Zimmermann wrote: > A lot of source files include drm_crtc_helper.h for its contained > include statements. This leads to excessive compile-time dependencies. > > Where possible, remove the include statements for drm_crtc_helper.h > and include the required source files directly. Also remove the > include statements from drm_crtc_helper.h itself, which doesn't need > most of them. With this patchset drm_crtc_helper usage is reduced from 85 places to 35 places. And the 35 places is only .c files. This is a very nice reduction of bloat! I hope this has a measureable effect on building times. I was working on something similar, but that approach only added missing includes, and did not kill all the unnessesary includes - which I think is the biggest win here. All patches are: Reviewed-by: Sam Ravnborg For a few of them the r-b is conditional, see the specific comments posted. I did a build check here with the archs and config I verifies with. This covers "alpha arm arm64 sparc64 i386 x86 powerpc s390 riscv sh" and everything was fine. I have a few specific configs to pull in drivers that need a bit extra to be built. So I consider build coverage OK for applying, but it would be nice to wait a few days for the bots to verify too. My own work on slimming drm_atomic_helper.h and drm_print.h will be rebased on top of your work before I continue it. I need to look into removing unused includes too. Sam > > I built this patchset on x86-64, aarch64 and arm. Hopefully I found > all include dependencies. > > Thanks to Sam Ravnborg for bringing this to my attention. > > Thomas Zimmermann (22): > drm/amdgpu: Fix coding style > drm: Remove unnecessary include statements for drm_crtc_helper.h > drm/amdgpu: Remove unnecessary include statements for > drm_crtc_helper.h > drm/arm/komeda: Remove unnecessary include statements for > drm_crtc_helper.h > drm/aspeed: Remove unnecessary include statements for > drm_crtc_helper.h > drm/ast: Remove unnecessary include statements for drm_crtc_helper.h > drm/bridge: Remove unnecessary include statements for > drm_crtc_helper.h > drm/gma500: Remove unnecessary include statements for > drm_crtc_helper.h > drm/i2c/ch7006: Remove unnecessary include statements for > drm_crtc_helper.h > drm/ingenic: Remove unnecessary include statements for > drm_crtc_helper.h > drm/kmb: Remove unnecessary include statements for drm_crtc_helper.h > drm/logicvc: Remove unnecessary include statements for > drm_crtc_helper.h > drm/nouveau: Remove unnecessary include statements for > drm_crtc_helper.h > drm/radeon: Remove unnecessary include statements for > drm_crtc_helper.h > drm/rockchip: Remove unnecessary include statements for > drm_crtc_helper.h > drm/shmobile: Remove unnecessary include statements for > drm_crtc_helper.h > drm/sprd: Remove unnecessary include statements for drm_crtc_helper.h > drm/sun4i: Remove unnecessary include statements for drm_crtc_helper.h > drm/tidss: Remove unnecessary include statements for drm_crtc_helper.h > drm/udl: Remove unnecessary include statements for drm_crtc_helper.h > drm/vboxvideo: Remove unnecessary include statements for > drm_crtc_helper.h > drm/crtc-helper: Remove most include statements from drm_crtc_helper.h > > drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 2 ++ > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +++-- > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c| 1 + > drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c | 1 - > drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c| 1 - > drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 1 - > drivers/gpu/drm/amd/amdgpu/atombios_crtc.c | 1 - > drivers/gpu/drm/amd/amdgpu/atombios_encoders.c | 1 - > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 2 ++ > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 2 ++ > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 2 ++ > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 ++ > drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 1 - > drivers/gpu/drm/arm/display/komeda/komeda_kms.h| 1 - > drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c | 1 - > drivers/gpu/drm/aspeed/aspeed_gfx_drv.c| 1 - > drivers/gpu/drm/aspeed/aspeed_gfx_out.c| 1 - > drivers/gpu/drm/ast/ast_drv.c | 1 - > drivers/gpu/drm/ast/ast_main.c | 1 - > drivers/gpu/drm/ast/ast_mode.c | 1 - > drivers/gpu/drm/bridge/analogix/analogix-anx6345.c | 1 - > drivers/gpu/drm/bridge/analogix/anx7625.c | 1 - > .../gpu/d
Re: [PATCH 18/22] drm/sun4i: Remove unnecessary include statements for drm_crtc_helper.h
On Mon, Jan 16, 2023 at 02:12:31PM +0100, Thomas Zimmermann wrote: > Several source files include drm_crtc_helper.h without needing it or > only to get its transitive include statements; leading to unnecessary > compile-time dependencies. > > Directly include required headers and drop drm_crtc_helper.h where > possible. > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > index 477cb6985b4d..37dc66332bbd 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > @@ -8,8 +8,8 @@ > #include > #include > > -#include > #include > +#include Move one up to maintain sorting. With that fixed: Reviewed-by: Sam Ravnborg > #include > > #include "sun8i_dw_hdmi.h" > -- > 2.39.0
Re: [PATCH 05/22] drm/aspeed: Remove unnecessary include statements for drm_crtc_helper.h
On Mon, Jan 16, 2023 at 02:12:18PM +0100, Thomas Zimmermann wrote: > Several source files include drm_crtc_helper.h without needing it or > only to get its transitive include statements; leading to unnecessary > compile-time dependencies. > > Directly include required headers and drop drm_crtc_helper.h where > possible. Nitpicking... The above is the standard text you use. But in many cases, like this case, the patch just drop the wrong use of the header and do not include any required headers. If you want to rephrase for future changelog diggers or leave it as is, is up to you. Sam > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c | 1 - > drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 1 - > drivers/gpu/drm/aspeed/aspeed_gfx_out.c | 1 - > 3 files changed, 3 deletions(-) > > diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c > b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c > index 55a3444a51d8..7877a57b8e26 100644 > --- a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c > +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c > @@ -5,7 +5,6 @@ > #include > #include > > -#include > #include > #include > #include > diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c > b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c > index 718119e168a6..ecfb060d2557 100644 > --- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c > +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c > @@ -14,7 +14,6 @@ > #include > > #include > -#include > #include > #include > #include > diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_out.c > b/drivers/gpu/drm/aspeed/aspeed_gfx_out.c > index 4f2187025a21..78775e0c853f 100644 > --- a/drivers/gpu/drm/aspeed/aspeed_gfx_out.c > +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_out.c > @@ -3,7 +3,6 @@ > > #include > #include > -#include > #include > #include > > -- > 2.39.0
Re: [PATCH 03/22] drm/amdgpu: Remove unnecessary include statements for drm_crtc_helper.h
On Mon, Jan 16, 2023 at 02:12:16PM +0100, Thomas Zimmermann wrote: > Several source files include drm_crtc_helper.h without needing it or > only to get its transitive include statements; leading to unnecessary > compile-time dependencies. > > Directly include required headers and drop drm_crtc_helper.h where > possible. > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c | 2 ++ > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 + > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c| 1 + > drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c | 1 - > drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c| 1 - > drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 1 - > drivers/gpu/drm/amd/amdgpu/atombios_crtc.c | 1 - > drivers/gpu/drm/amd/amdgpu/atombios_encoders.c | 1 - > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 2 ++ > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 2 ++ > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 2 ++ > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 ++ > 12 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > index 2ebbc6382a06..3c962d0214cc 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c > @@ -25,7 +25,9 @@ > */ > > #include > +#include > #include > +#include > #include > #include > #include "amdgpu.h" > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index 0993ee91fe18..63122482208d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -39,6 +39,7 @@ > #include > #include > #include > +#include Move it one line up to keep the sorting. With this fixed: Reviewed-by: Sam Ravnborg > #include > #include > #include > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > index b22471b3bd63..c5b98e9a69e9 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > @@ -42,6 +42,7 @@ > #include > #include > #include > +#include > #include > > /** > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c > index c96e458ed088..27a782a9dc72 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_encoders.c > @@ -24,7 +24,6 @@ > * Alex Deucher > */ > > -#include > #include > #include "amdgpu.h" > #include "amdgpu_connectors.h" > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c > index a6aef488a822..d0a1cc88832c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c > @@ -45,7 +45,6 @@ > #include > #include > > -#include > #include > #include > #include > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > index 8a39300b1a84..cf4b6e8d7d1e 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h > @@ -35,7 +35,6 @@ > #include > #include > #include > -#include > #include > #include > #include > diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_crtc.c > b/drivers/gpu/drm/amd/amdgpu/atombios_crtc.c > index afad094f84c2..10098fdd33fc 100644 > --- a/drivers/gpu/drm/amd/amdgpu/atombios_crtc.c > +++ b/drivers/gpu/drm/amd/amdgpu/atombios_crtc.c > @@ -24,7 +24,6 @@ > * Alex Deucher > */ > > -#include > #include > #include > #include "amdgpu.h" > diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c > b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c > index 18ae9433e463..d95b2dc78063 100644 > --- a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c > +++ b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c > @@ -28,7 +28,6 @@ > > #include > > -#include > #include > #include "amdgpu.h" > #include "amdgpu_connectors.h" > diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > index a2452fc304c5..01d1e2a631be 100644 > --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c > @@ -22,6 +22,8 @@ > */ > > #include > +#include > +#include > #include > > #include "amdgpu.h" &g
Re: [PATCH 02/22] drm: Remove unnecessary include statements for drm_crtc_helper.h
Hi Thomas.
On Mon, Jan 16, 2023 at 02:12:15PM +0100, Thomas Zimmermann wrote:
> Several DRM core and helper source files include drm_crtc_helper.h
> without needing it or only to get its transitive include statements;
> leading to unnecessary compile-time dependencies.
>
> Directly include required headers and drop drm_crtc_helper.h where
> possible. The header file, drm_fixed.h, includes
> for lower_32_bits().
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_crtc_helper.c | 1 -
> drivers/gpu/drm/drm_lease.c| 2 +-
> drivers/gpu/drm/drm_plane_helper.c | 1 -
> include/drm/drm_fixed.h| 1 +
> 4 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c
> b/drivers/gpu/drm/drm_crtc_helper.c
> index a209659a996c..e7a23e18140c 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -39,7 +39,6 @@
> #include
> #include
> #include
> -#include
drm_crtc_helper.c may not require drm/drm_crtc_helper.h, but it should
include it so we get a warning in case there is a mismatch between the
header file and the implementation.
I think sparse would also complain that the function is not declared
or something like that.
With this fixed:
Reviewed-by: Sam Ravnborg
> #include
> #include
> #include
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index 08ab75303a00..150fe1555068 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -6,7 +6,7 @@
> #include
>
> #include
> -#include
> +#include
> #include
> #include
> #include
> diff --git a/drivers/gpu/drm/drm_plane_helper.c
> b/drivers/gpu/drm/drm_plane_helper.c
> index ba6a9136a065..c91e454eba09 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -28,7 +28,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
> index 553210c02ee0..255645c1f9a8 100644
> --- a/include/drm/drm_fixed.h
> +++ b/include/drm/drm_fixed.h
> @@ -25,6 +25,7 @@
> #ifndef DRM_FIXED_H
> #define DRM_FIXED_H
>
> +#include
> #include
>
> typedef union dfixed {
> --
> 2.39.0
Re: [PATCH v2 05/10] drm/panel: Do not include
On Wed, Jan 11, 2023 at 02:02:01PM +0100, Thomas Zimmermann wrote: > Remove unnecessary include statements for . No functional > changes. Include where the driver got the header file via > . > > Signed-off-by: Thomas Zimmermann Reviewed-by: Sam Ravnborg > --- > drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 1 - > drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c | 1 - > drivers/gpu/drm/panel/panel-ronbo-rb070d30.c | 1 - > 3 files changed, 3 deletions(-) > > diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c > b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c > index cbb68caa36f2..1ec696adf9de 100644 > --- a/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c > +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9881c.c > @@ -7,7 +7,6 @@ > #include > #include > #include > -#include > #include > #include > #include > diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c > b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c > index 79f852465a84..35d568da342f 100644 > --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c > +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c > @@ -43,7 +43,6 @@ > > #include > #include > -#include > #include > #include > #include > diff --git a/drivers/gpu/drm/panel/panel-ronbo-rb070d30.c > b/drivers/gpu/drm/panel/panel-ronbo-rb070d30.c > index 866d1bf5530e..2ef5ea5eaeeb 100644 > --- a/drivers/gpu/drm/panel/panel-ronbo-rb070d30.c > +++ b/drivers/gpu/drm/panel/panel-ronbo-rb070d30.c > @@ -11,7 +11,6 @@ > #include > #include > #include > -#include > #include > #include > #include > -- > 2.39.0
Re: [PATCH v2 02/10] drm: Include where needed
Hi Ville, On Wed, Jan 11, 2023 at 06:08:42PM +0200, Ville Syrjälä wrote: > On Wed, Jan 11, 2023 at 02:01:58PM +0100, Thomas Zimmermann wrote: > > Include in source files that need it. Some of DRM's > > source code gets OF header via drm_crtc_helper.h and , > > which can leed to unnecessary recompilation. > > > > In drm_modes.c, add a comment on the reason for still including > > . The header file is required to get KHZ2PICOS(). The > > macro is part of the UAPI headers, so it cannot be moved to a less > > prominent location. > > I never liked that KHZ2PICOS() thing in there. Maybe we should > just nuke it and see if anyone notices? https://github.com/search?q=KHZ2PICOS=code tells me that it will be noticed. So it is part of the UAPI Sam
Re: [PATCH v2 01/10] drm: Include where needed
Hi Thomas, On Wed, Jan 11, 2023 at 02:01:57PM +0100, Thomas Zimmermann wrote: > Include in source files that need it. Some of > DRM's source code gets the backlight header via drm_crtc_helper.h > and , which can leed to unnecessary recompilation. If > possible, do not include drm_crtc_helper.h any longer. Are you planning a clean-up of drm_crtc_helper.h later? With a handful of forward it could losse all includes. > > Signed-off-by: Thomas Zimmermann > Acked-by: Christian König # amd Reviewed-by: Sam Ravnborg > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 2 +- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 1 + > drivers/gpu/drm/gma500/backlight.c| 2 ++ > drivers/gpu/drm/radeon/radeon_acpi.c | 2 +- > 4 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c > index 57b5e11446c6..f29c1d0ad4c1 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c > @@ -24,6 +24,7 @@ > > #include > #include > +#include > #include > #include > #include > @@ -31,7 +32,6 @@ > #include > #include > > -#include > #include "amdgpu.h" > #include "amdgpu_pm.h" > #include "amdgpu_display.h" > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index 1b7f20a9d4ae..55a845eb0c6d 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -67,6 +67,7 @@ > #include "ivsrcid/ivsrcid_vislands30.h" > > #include "i2caux_interface.h" > +#include > #include > #include > #include > diff --git a/drivers/gpu/drm/gma500/backlight.c > b/drivers/gpu/drm/gma500/backlight.c > index 577a4987b193..8711a7a5b8da 100644 > --- a/drivers/gpu/drm/gma500/backlight.c > +++ b/drivers/gpu/drm/gma500/backlight.c > @@ -7,6 +7,8 @@ > * Authors: Eric Knopp > */ > > +#include > + > #include > > #include "psb_drv.h" > diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c > b/drivers/gpu/drm/radeon/radeon_acpi.c > index b603c0b77075..5771d1fcb073 100644 > --- a/drivers/gpu/drm/radeon/radeon_acpi.c > +++ b/drivers/gpu/drm/radeon/radeon_acpi.c > @@ -22,6 +22,7 @@ > */ > > #include > +#include > #include > #include > #include > @@ -30,7 +31,6 @@ > #include > #include > > -#include > #include > > #include "atom.h" > -- > 2.39.0
Re: [PATCH v2 00/10] drm: Do not include unnecessarily
Hi Thomas. On Wed, Jan 11, 2023 at 02:01:56PM +0100, Thomas Zimmermann wrote: > Remove unnecessary include statements for . I recently > changed this header and had to rebuild a good part of DRM. So avoid > this by removing the dependency. > > Several files include via drm_fb_helper.h. So in v2 I > added additional patches that remove some of those include statements > as well. > > Some source files require the OF or backlight headers. Include those > instead. > > v2: > * add more patches to handle drm_fb_helper.h includes > * fix komeda build (kernel test robot) Whole series are: Acked-by: Sam Ravnborg Except for the patches where I added an explicit r-b. Sam
Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hi Javier,
>
> >>>
> >>> - if (vgacon_text_force() && i915_modparams.modeset == -1)
> >>> + ret = drm_drv_enabled();
> >>
> >> You pass the local driver variable here - which looks wrong as this is
> >> not the same as the driver variable declared in another file.
> >
>
> Yes, Jani mentioned it already. I got confused and thought that the driver
> variable was also defined in the same compilation unit...
>
> Maybe I could squash the following change?
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b18a250e5d2e..b8f399b76363 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -89,7 +89,7 @@
> #include "intel_region_ttm.h"
> #include "vlv_suspend.h"
>
> -static const struct drm_driver driver;
> +const struct drm_driver driver;
No, variables with such a generic name will clash.
Just add a
const drm_driver * i915_drm_driver(void)
{
return
}
And then use this function to access the drm_driver variable.
Sam
Re: [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hi Javier,
On Thu, Nov 04, 2021 at 05:07:06PM +0100, Javier Martinez Canillas wrote:
> Some DRM drivers check the vgacon_text_force() function return value as an
> indication on whether they should be allowed to be enabled or not.
>
> This function returns true if the nomodeset kernel command line parameter
> was set. But there may be other conditions besides this to determine if a
> driver should be enabled.
>
> Let's add a drm_drv_enabled() helper function to encapsulate that logic so
> can be later extended if needed, without having to modify all the drivers.
>
> Also, while being there do some cleanup. The vgacon_text_force() function
> is guarded by CONFIG_VGA_CONSOLE and there's no need for callers to do it.
>
> Suggested-by: Thomas Zimmermann
> Signed-off-by: Javier Martinez Canillas
> ---
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 8214a0b1ab7f..3fb567d62881 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -975,6 +975,26 @@ int drm_dev_set_unique(struct drm_device *dev, const
> char *name)
> }
> EXPORT_SYMBOL(drm_dev_set_unique);
>
> +/**
> + * drm_drv_enabled - Checks if a DRM driver can be enabled
> + * @driver: DRM driver to check
> + *
> + * Checks whether a DRM driver can be enabled or not. This may be the case
> + * if the "nomodeset" kernel command line parameter is used.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int drm_drv_enabled(const struct drm_driver *driver)
> +{
> + if (vgacon_text_force()) {
> + DRM_INFO("%s driver is disabled\n", driver->name);
DRM_INFO is deprecated, please do not use it in new code.
Also other users had an error message and not just info - is info
enough?
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_drv_enabled);
> +
> /*
> * DRM Core
> * The DRM core module initializes all global DRM objects and makes them
> diff --git a/drivers/gpu/drm/i915/i915_module.c
> b/drivers/gpu/drm/i915/i915_module.c
> index ab2295dd4500..45cb3e540eff 100644
> --- a/drivers/gpu/drm/i915/i915_module.c
> +++ b/drivers/gpu/drm/i915/i915_module.c
> @@ -18,9 +18,12 @@
> #include "i915_selftest.h"
> #include "i915_vma.h"
>
> +static const struct drm_driver driver;
Hmmm...
> +
> static int i915_check_nomodeset(void)
> {
> bool use_kms = true;
> + int ret;
>
> /*
>* Enable KMS by default, unless explicitly overriden by
> @@ -31,7 +34,8 @@ static int i915_check_nomodeset(void)
> if (i915_modparams.modeset == 0)
> use_kms = false;
>
> - if (vgacon_text_force() && i915_modparams.modeset == -1)
> + ret = drm_drv_enabled();
You pass the local driver variable here - which looks wrong as this is
not the same as the driver variable declared in another file.
Maybe move the check to new function you can add to init_funcs,
and locate the new function in i915_drv - so it has access to driver?
Sam
Re: [PATCH v2 11/14] drm/tilcdc: Convert to Linux IRQ interfaces
Hi Thomas, On Wed, Aug 04, 2021 at 08:30:41PM +0200, Thomas Zimmermann wrote: > Hi > > Am 03.08.21 um 17:00 schrieb Sam Ravnborg: > > Hi Thomas, > > > > On Tue, Aug 03, 2021 at 11:07:01AM +0200, Thomas Zimmermann wrote: > > > Drop the DRM IRQ midlayer in favor of Linux IRQ interfaces. DRM's > > > IRQ helpers are mostly useful for UMS drivers. Modern KMS drivers > > > don't benefit from using it. > > > > > > DRM IRQ callbacks are now being called directly or inlined. > > > > > > Calls to platform_get_irq() can fail with a negative errno code. > > > Abort initialization in this case. The DRM IRQ midlayer does not > > > handle this case correctly. > > > > I cannot see why the irq_enabled flag is needed here, and the changelog > > do not help me. > > What do I miss? > > That's a good point. Usually, only the DRM IRQ helpers use irq_enabled in > struct drm_device. It'll become legacy and we can ignore it for the driver > conversion. > > The exception is tilcdc, which also uses irq_enabled to make its error > rollback work correctly. So I duplicated the state in the local private > structure. > > I'll add this explanation to the commit message. With this added: Acked-by: Sam Ravnborg
Re: [PATCH v2 00/14] drm: Make DRM's IRQ helpers legacy
Hi Thomas, On Tue, Aug 03, 2021 at 11:06:50AM +0200, Thomas Zimmermann wrote: > DRM's IRQ helpers are only helpful for old, non-KMS drivers. Move > the code behind CONFIG_DRM_LEGACY. Convert KMS drivers to Linux > IRQ interfaces. > > DRM provides IRQ helpers for setting up, receiving and removing IRQ > handlers. It's an abstraction over plain Linux functions. The code > is mid-layerish with several callbacks to hook into the rsp drivers. > Old UMS driver have their interrupts enabled via ioctl, so these > abstractions makes some sense. Modern KMS manage all their interrupts > internally. Using the DRM helpers adds indirection without benefits. > > Most KMS drivers already use Linux IRQ functions instead of DRM's > abstraction layer. Patches 1 to 12 convert the remaining ones. > The patches also resolve a bug for devices without assigned interrupt > number. DRM helpers don't test for IRQ_NOTCONNECTED, so drivers do > not detect if the device has no interrupt assigned. > > Patch 13 removes an unused function. > > Patch 14 moves the DRM IRQ helpers behind CONFIG_DRM_LEGACY. Only > the old non-KMS drivers still use the functionality. > > v2: > * drop IRQ_NOTCONNECTED test from atmel-hlcdc (Sam) > * use devm_request_irq() in atmel-hlcdc (Sam) > * unify variable names in arm/hlcdc (Sam) > > Thomas Zimmermann (14): The following patches are all: Acked-by: Sam Ravnborg > drm/fsl-dcu: Convert to Linux IRQ interfaces > drm/gma500: Convert to Linux IRQ interfaces > drm/kmb: Convert to Linux IRQ interfaces > drm/msm: Convert to Linux IRQ interfaces > drm/mxsfb: Convert to Linux IRQ interfaces > drm/tidss: Convert to Linux IRQ interfaces > drm/vc4: Convert to Linux IRQ interfaces > drm: Remove unused devm_drm_irq_install() The remaining patches I either skipped or already had a feedback from me or I asked a question. Sam
Re: [PATCH v2 11/14] drm/tilcdc: Convert to Linux IRQ interfaces
Hi Thomas,
On Tue, Aug 03, 2021 at 11:07:01AM +0200, Thomas Zimmermann wrote:
> Drop the DRM IRQ midlayer in favor of Linux IRQ interfaces. DRM's
> IRQ helpers are mostly useful for UMS drivers. Modern KMS drivers
> don't benefit from using it.
>
> DRM IRQ callbacks are now being called directly or inlined.
>
> Calls to platform_get_irq() can fail with a negative errno code.
> Abort initialization in this case. The DRM IRQ midlayer does not
> handle this case correctly.
I cannot see why the irq_enabled flag is needed here, and the changelog
do not help me.
What do I miss?
Sam
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/tilcdc/tilcdc_drv.c | 51 ++---
> drivers/gpu/drm/tilcdc/tilcdc_drv.h | 3 ++
> 2 files changed, 43 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index f1d3a9f919fd..6b03f89a98d4 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -20,7 +20,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -124,6 +123,39 @@ static int cpufreq_transition(struct notifier_block *nb,
> }
> #endif
>
> +static irqreturn_t tilcdc_irq(int irq, void *arg)
> +{
> + struct drm_device *dev = arg;
> + struct tilcdc_drm_private *priv = dev->dev_private;
> +
> + return tilcdc_crtc_irq(priv->crtc);
> +}
> +
> +static int tilcdc_irq_install(struct drm_device *dev, unsigned int irq)
> +{
> + struct tilcdc_drm_private *priv = dev->dev_private;
> + int ret;
> +
> + ret = request_irq(irq, tilcdc_irq, 0, dev->driver->name, dev);
> + if (ret)
> + return ret;
> +
> + priv->irq_enabled = false;
> +
> + return 0;
> +}
> +
> +static void tilcdc_irq_uninstall(struct drm_device *dev)
> +{
> + struct tilcdc_drm_private *priv = dev->dev_private;
> +
> + if (!priv->irq_enabled)
> + return;
> +
> + free_irq(priv->irq, dev);
> + priv->irq_enabled = false;
> +}
> +
> /*
> * DRM operations:
> */
> @@ -145,7 +177,7 @@ static void tilcdc_fini(struct drm_device *dev)
> drm_dev_unregister(dev);
>
> drm_kms_helper_poll_fini(dev);
> - drm_irq_uninstall(dev);
> + tilcdc_irq_uninstall(dev);
> drm_mode_config_cleanup(dev);
>
> if (priv->clk)
> @@ -336,7 +368,12 @@ static int tilcdc_init(const struct drm_driver *ddrv,
> struct device *dev)
> goto init_failed;
> }
>
> - ret = drm_irq_install(ddev, platform_get_irq(pdev, 0));
> + ret = platform_get_irq(pdev, 0);
> + if (ret < 0)
> + goto init_failed;
> + priv->irq = ret;
> +
> + ret = tilcdc_irq_install(ddev, priv->irq);
> if (ret < 0) {
> dev_err(dev, "failed to install IRQ handler\n");
> goto init_failed;
> @@ -360,13 +397,6 @@ static int tilcdc_init(const struct drm_driver *ddrv,
> struct device *dev)
> return ret;
> }
>
> -static irqreturn_t tilcdc_irq(int irq, void *arg)
> -{
> - struct drm_device *dev = arg;
> - struct tilcdc_drm_private *priv = dev->dev_private;
> - return tilcdc_crtc_irq(priv->crtc);
> -}
> -
> #if defined(CONFIG_DEBUG_FS)
> static const struct {
> const char *name;
> @@ -454,7 +484,6 @@ DEFINE_DRM_GEM_CMA_FOPS(fops);
>
> static const struct drm_driver tilcdc_driver = {
> .driver_features= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> - .irq_handler= tilcdc_irq,
> DRM_GEM_CMA_DRIVER_OPS,
> #ifdef CONFIG_DEBUG_FS
> .debugfs_init = tilcdc_debugfs_init,
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> index d29806ca8817..b818448c83f6 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> @@ -46,6 +46,8 @@ struct tilcdc_drm_private {
> struct clk *clk; /* functional clock */
> int rev; /* IP revision */
>
> + unsigned int irq;
> +
> /* don't attempt resolutions w/ higher W * H * Hz: */
> uint32_t max_bandwidth;
> /*
> @@ -82,6 +84,7 @@ struct tilcdc_drm_private {
>
> bool is_registered;
> bool is_componentized;
> + bool irq_enabled;
> };
>
> /* Sub-module for display. Since we don't know at compile time what panels
> --
> 2.32.0
Re: [PATCH 00/14] drm: Make DRM's IRQ helpers legacy
Hi Thomas, > > > > 1) IRQ_NOTCONNECTED > > > > We do not have this check in drm_irq today and we should avoid spreading > > it all over. We are either carrying it forever or we wil lsee patches > > floating in to drop the check again. > > The current use in the kernel is minimal: > > https://elixir.bootlin.com/linux/latest/A/ident/IRQ_NOTCONNECTED > > > > So as a minimum drop it from atmel_hlcdc and preferably from the rest as > > it is really not used. (Speaking as atmel_hlcdc maintainer) > > I'll drop it from atmel_hlcdc then. > > But saying that it's not used is not correct. My point is the drm_irq do not check this - so adding this check add something there was not needed/done before. > > 2) devm_request_irq() > > > > We are moving towards managed allocation so we do not fail to free > > resources. And an irq has a lifetime equal the device itself - so an > > obvious cnadidate for devm_request_irq. > > If we do not introduce it now we will see a revisit of this later. > > I can be convinced to wait with this as we will have to do much more in > > each driver, but I cannot see any good arguments to avoid the more > > modern way to use devm_request_irq. > > I'll change this in atmel_hdlcd and maybe I can find trivial cases where > devm_request_irq() can be used. But drivers that had an uninstall callback > before should not have the cleanup logic altered by a patch as this one. I > suspect that most of the IRQ cleanup > is actually a vblank cleanup and should be done in response to > drm_vblank_init(). But that's again not something for this patchset here. We > cannot change multiple things at once and still expect any of it to work. > > I welcome the use of devm_ et al. But these changes are better done in a > per-driver patchset that changes all of the driver to managed release. Fair enough, and fine with me. I have yet to read through all patches - will do so in the coming week. Sam
Re: [PATCH 00/14] drm: Make DRM's IRQ helpers legacy
Hi Thomas, On Tue, Jul 27, 2021 at 08:27:07PM +0200, Thomas Zimmermann wrote: > DRM's IRQ helpers are only helpful for old, non-KMS drivers. Move > the code behind CONFIG_DRM_LEGACY. Convert KMS drivers to Linux > IRQ interfaces. > > DRM provides IRQ helpers for setting up, receiving and removing IRQ > handlers. It's an abstraction over plain Linux functions. The code > is mid-layerish with several callbacks to hook into the rsp drivers. > Old UMS driver have their interrupts enabled via ioctl, so these > abstractions makes some sense. Modern KMS manage all their interrupts > internally. Using the DRM helpers adds indirection without benefits. > > Most KMs drivers already use Linux IRQ functions instead of DRM's > abstraction layer. Patches 1 to 12 convert the remaining ones. > The patches also resolve a bug for devices without assigned interrupt > number. DRM helpers don't test for IRQ_NOTCONNECTED, so drivers do > not detect if the device has no interrupt assigned. > > Patch 13 removes an unused function. > > Patch 14 moves the DRM IRQ helpers behind CONFIG_DRM_LEGACY. Only > the old non-KMS drivers still use the functionality. > > Thomas Zimmermann (14): > drm/amdgpu: Convert to Linux IRQ interfaces > drm/arm/hdlcd: Convert to Linux IRQ interfaces > drm/atmel-hlcdc: Convert to Linux IRQ interfaces > drm/fsl-dcu: Convert to Linux IRQ interfaces > drm/gma500: Convert to Linux IRQ interfaces > drm/kmb: Convert to Linux IRQ interfaces > drm/msm: Convert to Linux IRQ interfaces > drm/mxsfb: Convert to Linux IRQ interfaces > drm/radeon: Convert to Linux IRQ interfaces > drm/tidss: Convert to Linux IRQ interfaces > drm/tilcdc: Convert to Linux IRQ interfaces > drm/vc4: Convert to Linux IRQ interfaces > drm: Remove unused devm_drm_irq_install() > drm: IRQ midlayer is now legacy With the irq_enabled confusion out of the way I want to re-address two issues here that I know you have answered but I am just not convinced. 1) IRQ_NOTCONNECTED We do not have this check in drm_irq today and we should avoid spreading it all over. We are either carrying it forever or we wil lsee patches floating in to drop the check again. The current use in the kernel is minimal: https://elixir.bootlin.com/linux/latest/A/ident/IRQ_NOTCONNECTED So as a minimum drop it from atmel_hlcdc and preferably from the rest as it is really not used. (Speaking as atmel_hlcdc maintainer) 2) devm_request_irq() We are moving towards managed allocation so we do not fail to free resources. And an irq has a lifetime equal the device itself - so an obvious cnadidate for devm_request_irq. If we do not introduce it now we will see a revisit of this later. I can be convinced to wait with this as we will have to do much more in each driver, but I cannot see any good arguments to avoid the more modern way to use devm_request_irq. Sam
Re: [PATCH 03/14] drm/atmel-hlcdc: Convert to Linux IRQ interfaces
Hi Thomas,
>
> Are you sure, you're testing with the latest drm-misc-next or drm-tip?
> Because using irq_enabled is deprecated and the flag was recently replaced
> by commit 1e4cd78ed493 ("drm: Don't test for IRQ support in VBLANK ioctls").
I was looking at drm-misc-fixes which did not have this commit :-(
Just my silly excuse why I was convinced this was the issue.
Sam
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 03/14] drm/atmel-hlcdc: Convert to Linux IRQ interfaces
Hi Dan,
> >
> > I think I got it - we need to set irq_enabled to true.
> > The documentation says so:
> > "
> > * @irq_enabled:
> > *
> > * Indicates that interrupt handling is enabled, specifically
> > vblank
> > * handling. Drivers which don't use drm_irq_install() need to set
> > this
> > * to true manually.
> > "
> >
> > Can you try to add the following line:
> >
> >
> > +static int atmel_hlcdc_dc_irq_install(struct drm_device *dev, unsigned int
> > irq)
> > +{
> > + int ret;
> > +
> > + if (irq == IRQ_NOTCONNECTED)
> > + return -ENOTCONN;
> > +
> >
> > dev->irq_enabled = true;<= THIS LINE
> >
> >
> > + atmel_hlcdc_dc_irq_disable(dev);
> > +
> > + ret = request_irq(irq, atmel_hlcdc_dc_irq_handler, 0,
> > dev->driver->name, dev);
> > + if (ret)
> > + return ret;
> >
> > I hope this fixes it.
>
> It does! With the irq_enabled line added everything is looking good.
Great, thanks for testing.
Thomas - I assume you will do a re-spin and there is likely some fixes
for the applied IRQ conversions too.
Note - irq_enabled must be cleared if request_irq fails. I did not
include this in the testing here.
Sam
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: Re: [PATCH 03/14] drm/atmel-hlcdc: Convert to Linux IRQ interfaces
Hi Dan,
> >>
> >> Just to be sure...
> >> Can you confirm that vbltest is working OK *before* this patch?
> >
> > Yes, can you please verify that it regressed. If so, this would mean
> > that the driver misses vblank interrupts with the patch applied.
>
> Yes, unfortunately the vbltest works before this patch, but fails after
> this patch is applied.
I think I got it - we need to set irq_enabled to true.
The documentation says so:
"
* @irq_enabled:
*
* Indicates that interrupt handling is enabled, specifically vblank
* handling. Drivers which don't use drm_irq_install() need to set this
* to true manually.
"
Can you try to add the following line:
+static int atmel_hlcdc_dc_irq_install(struct drm_device *dev, unsigned int irq)
+{
+ int ret;
+
+ if (irq == IRQ_NOTCONNECTED)
+ return -ENOTCONN;
+
dev->irq_enabled = true;<= THIS LINE
+ atmel_hlcdc_dc_irq_disable(dev);
+
+ ret = request_irq(irq, atmel_hlcdc_dc_irq_handler, 0,
dev->driver->name, dev);
+ if (ret)
+ return ret;
I hope this fixes it.
Sam
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 03/14] drm/atmel-hlcdc: Convert to Linux IRQ interfaces
Hi Dan, thanks for the quick feedback! On Wed, Jul 28, 2021 at 05:50:34PM +, dan.sned...@microchip.com wrote: > On 7/28/21 8:44 AM, Sam Ravnborg wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > content is safe > > > > Hi Dan, > > > > On Wed, Jul 28, 2021 at 03:11:08PM +, dan.sned...@microchip.com wrote: > >> On 7/28/21 7:00 AM, Sam Ravnborg wrote: > >>> [You don't often get email from s...@ravnborg.org. Learn why this is > >>> important at http://aka.ms/LearnAboutSenderIdentification.] > >>> > >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know > >>> the content is safe > >>> > >>> Hi Dan, > >>> > >>> I hope you can fine to test this patch from Thomas. > >>> If this works then we can forget about my attempt to do the same. > >> > >> I'll test this as soon as I can and let you know. > > > > Thanks, crossing my fingers... (which explains the funny spelling from > > time to time) > > > > Sam > > So I ran the test on an A5D27 XULT board with a PDA5 display. Our > graphics demos that come with our linux4sam releases seem to run just > fine. modetest -v seems to run just fine. However, vbltest returns > "drmWaitVBlank (relative) failed ret: -1". I don't understand why > modetest -v is working and vbltest isn't, but that's what I'm seeing. Strange indeed. Just to be sure... Can you confirm that vbltest is working OK *before* this patch? Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 03/14] drm/atmel-hlcdc: Convert to Linux IRQ interfaces
Hi Dan, On Wed, Jul 28, 2021 at 03:11:08PM +, dan.sned...@microchip.com wrote: > On 7/28/21 7:00 AM, Sam Ravnborg wrote: > > [You don't often get email from s...@ravnborg.org. Learn why this is > > important at http://aka.ms/LearnAboutSenderIdentification.] > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > > content is safe > > > > Hi Dan, > > > > I hope you can fine to test this patch from Thomas. > > If this works then we can forget about my attempt to do the same. > > I'll test this as soon as I can and let you know. Thanks, crossing my fingers... (which explains the funny spelling from time to time) Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 14/14] drm: IRQ midlayer is now legacy
Hi Thomas, On Tue, Jul 27, 2021 at 08:27:21PM +0200, Thomas Zimmermann wrote: > Hide the DRM midlayer behind CONFIG_DRM_LEGACY, make functions use > the prefix drm_legacy_, and move declarations to drm_legacy.h. > In struct drm_device, move the fields irq and irq_enabled behind > CONFIG_DRM_LEGACY. > > All callers have been updated. > > Signed-off-by: Thomas Zimmermann Acked-by: Sam Ravnborg > --- > drivers/gpu/drm/drm_irq.c | 63 --- You could have pulled it all into drm_legacy_misc.c. > drivers/gpu/drm/drm_legacy_misc.c | 3 +- > drivers/gpu/drm/drm_vblank.c | 8 ++-- > drivers/gpu/drm/i810/i810_dma.c | 3 +- > drivers/gpu/drm/mga/mga_dma.c | 2 +- > drivers/gpu/drm/mga/mga_drv.h | 1 - > drivers/gpu/drm/r128/r128_cce.c | 3 +- > drivers/gpu/drm/via/via_mm.c | 3 +- > include/drm/drm_device.h | 18 ++--- > include/drm/drm_drv.h | 44 ++--- > include/drm/drm_irq.h | 31 --- > include/drm/drm_legacy.h | 3 ++ > 12 files changed, 27 insertions(+), 155 deletions(-) > delete mode 100644 include/drm/drm_irq.h Nice cleanup. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 03/14] drm/atmel-hlcdc: Convert to Linux IRQ interfaces
Hi Dan,
I hope you can fine to test this patch from Thomas.
If this works then we can forget about my attempt to do the same.
Hi Thomas,
IRQ_NOTCONNECTED check seems redundant, as mentioned in another patch
already.
With that considered:
Reviewed-by: Sam Ravnborg
We shall wait for testing from Dan before you apply it as I had made a
similar patch that failed to work. I assume my patch was buggy but I
prefer to be sure.
Sam
On Tue, Jul 27, 2021 at 08:27:10PM +0200, Thomas Zimmermann wrote:
> Drop the DRM IRQ midlayer in favor of Linux IRQ interfaces. DRM's
> IRQ helpers are mostly useful for UMS drivers. Modern KMS drivers
> don't benefit from using it. DRM IRQ callbacks are now being called
> directly or inlined.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 85
> 1 file changed, 52 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index f09b6dd8754c..cfa8c2c9c8aa 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -22,7 +22,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
>
> @@ -557,6 +556,56 @@ static irqreturn_t atmel_hlcdc_dc_irq_handler(int irq,
> void *data)
> return IRQ_HANDLED;
> }
>
> +static void atmel_hlcdc_dc_irq_postinstall(struct drm_device *dev)
> +{
> + struct atmel_hlcdc_dc *dc = dev->dev_private;
> + unsigned int cfg = 0;
> + int i;
> +
> + /* Enable interrupts on activated layers */
> + for (i = 0; i < ATMEL_HLCDC_MAX_LAYERS; i++) {
> + if (dc->layers[i])
> + cfg |= ATMEL_HLCDC_LAYER_STATUS(i);
> + }
> +
> + regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, cfg);
> +}
> +
> +static void atmel_hlcdc_dc_irq_disable(struct drm_device *dev)
> +{
> + struct atmel_hlcdc_dc *dc = dev->dev_private;
> + unsigned int isr;
> +
> + regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IDR, 0x);
> + regmap_read(dc->hlcdc->regmap, ATMEL_HLCDC_ISR, );
> +}
> +
> +static int atmel_hlcdc_dc_irq_install(struct drm_device *dev, unsigned int
> irq)
> +{
> + int ret;
> +
> + if (irq == IRQ_NOTCONNECTED)
> + return -ENOTCONN;
> +
> + atmel_hlcdc_dc_irq_disable(dev);
> +
> + ret = request_irq(irq, atmel_hlcdc_dc_irq_handler, 0,
> dev->driver->name, dev);
> + if (ret)
> + return ret;
> +
> + atmel_hlcdc_dc_irq_postinstall(dev);
> +
> + return 0;
> +}
> +
> +static void atmel_hlcdc_dc_irq_uninstall(struct drm_device *dev)
> +{
> + struct atmel_hlcdc_dc *dc = dev->dev_private;
> +
> + atmel_hlcdc_dc_irq_disable(dev);
> + free_irq(dc->hlcdc->irq, dev);
> +}
> +
> static const struct drm_mode_config_funcs mode_config_funcs = {
> .fb_create = drm_gem_fb_create,
> .atomic_check = drm_atomic_helper_check,
> @@ -647,7 +696,7 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
> drm_mode_config_reset(dev);
>
> pm_runtime_get_sync(dev->dev);
> - ret = drm_irq_install(dev, dc->hlcdc->irq);
> + ret = atmel_hlcdc_dc_irq_install(dev, dc->hlcdc->irq);
> pm_runtime_put_sync(dev->dev);
> if (ret < 0) {
> dev_err(dev->dev, "failed to install IRQ handler\n");
> @@ -676,7 +725,7 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev)
> drm_mode_config_cleanup(dev);
>
> pm_runtime_get_sync(dev->dev);
> - drm_irq_uninstall(dev);
> + atmel_hlcdc_dc_irq_uninstall(dev);
> pm_runtime_put_sync(dev->dev);
>
> dev->dev_private = NULL;
> @@ -685,40 +734,10 @@ static void atmel_hlcdc_dc_unload(struct drm_device
> *dev)
> clk_disable_unprepare(dc->hlcdc->periph_clk);
> }
>
> -static int atmel_hlcdc_dc_irq_postinstall(struct drm_device *dev)
> -{
> - struct atmel_hlcdc_dc *dc = dev->dev_private;
> - unsigned int cfg = 0;
> - int i;
> -
> - /* Enable interrupts on activated layers */
> - for (i = 0; i < ATMEL_HLCDC_MAX_LAYERS; i++) {
> - if (dc->layers[i])
> - cfg |= ATMEL_HLCDC_LAYER_STATUS(i);
> - }
> -
> - regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, cfg);
> -
> - return 0;
> -}
> -
> -static void atmel_hlcdc_dc_irq_uninstall(struct drm_device *dev)
> -{
> - struct atmel_hlcdc_dc *dc = dev->dev_private;
> - unsigned int isr;
> -
Re: [PATCH 02/14] drm/arm/hdlcd: Convert to Linux IRQ interfaces
Hi Thomas,
On Tue, Jul 27, 2021 at 08:27:09PM +0200, Thomas Zimmermann wrote:
> Drop the DRM IRQ midlayer in favor of Linux IRQ interfaces. DRM's
> IRQ helpers are mostly useful for UMS drivers. Modern KMS drivers
> don't benefit from using it.
>
> DRM IRQ callbacks are now being called directly or inlined.
>
> Calls to platform_get_irq() can fail with a negative errno code.
> Abort initialization in this case. The DRM IRQ midlayer does not
> handle this case correctly.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/arm/hdlcd_drv.c | 174 ++--
> drivers/gpu/drm/arm/hdlcd_drv.h | 1 +
> 2 files changed, 97 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> index 81ae92390736..b9998fe3982f 100644
> --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> @@ -29,7 +29,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -38,6 +37,94 @@
> #include "hdlcd_drv.h"
> #include "hdlcd_regs.h"
>
> +static irqreturn_t hdlcd_irq(int irq, void *arg)
> +{
> + struct drm_device *drm = arg;
> + struct hdlcd_drm_private *hdlcd = drm->dev_private;
> + unsigned long irq_status;
> +
> + irq_status = hdlcd_read(hdlcd, HDLCD_REG_INT_STATUS);
> +
> +#ifdef CONFIG_DEBUG_FS
> + if (irq_status & HDLCD_INTERRUPT_UNDERRUN)
> + atomic_inc(>buffer_underrun_count);
> +
> + if (irq_status & HDLCD_INTERRUPT_DMA_END)
> + atomic_inc(>dma_end_count);
> +
> + if (irq_status & HDLCD_INTERRUPT_BUS_ERROR)
> + atomic_inc(>bus_error_count);
> +
> + if (irq_status & HDLCD_INTERRUPT_VSYNC)
> + atomic_inc(>vsync_count);
> +
> +#endif
> + if (irq_status & HDLCD_INTERRUPT_VSYNC)
> + drm_crtc_handle_vblank(>crtc);
> +
> + /* acknowledge interrupt(s) */
> + hdlcd_write(hdlcd, HDLCD_REG_INT_CLEAR, irq_status);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void hdlcd_irq_preinstall(struct drm_device *drm)
> +{
> + struct hdlcd_drm_private *hdlcd = drm->dev_private;
> + /* Ensure interrupts are disabled */
> + hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, 0);
> + hdlcd_write(hdlcd, HDLCD_REG_INT_CLEAR, ~0);
> +}
> +
> +static void hdlcd_irq_postinstall(struct drm_device *drm)
> +{
> +#ifdef CONFIG_DEBUG_FS
> + struct hdlcd_drm_private *hdlcd = drm->dev_private;
> + unsigned long irq_mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
> +
> + /* enable debug interrupts */
> + irq_mask |= HDLCD_DEBUG_INT_MASK;
> +
> + hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, irq_mask);
> +#endif
> +}
> +
> +static int hdlcd_irq_install(struct drm_device *dev, int irq)
It is inconsistent that the drm_device * is named "dev", as similar
functions in this patch uses the name "drm".
> +{
> + int ret;
> +
> + if (irq == IRQ_NOTCONNECTED)
> + return -ENOTCONN;
The code above is almost redundandt as request_irq has the same check.
The only benefit of this check is that we avoid calling
hdlcd_irq_preinstall().
And IRQ_NOTCONNECTED is only set for PCI devices which this is not.
So I would thing the if () should be dropped here. ??
With the inputs considered/addressed:
Acked-by: Sam Ravnborg
> +
> + hdlcd_irq_preinstall(dev);
> +
> + ret = request_irq(irq, hdlcd_irq, 0, dev->driver->name, dev);
> + if (ret)
> + return ret;
> +
> + hdlcd_irq_postinstall(dev);
> +
> + return 0;
> +}
> +
> +static void hdlcd_irq_uninstall(struct drm_device *drm)
> +{
> + struct hdlcd_drm_private *hdlcd = drm->dev_private;
> + /* disable all the interrupts that we might have enabled */
> + unsigned long irq_mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
> +
> +#ifdef CONFIG_DEBUG_FS
> + /* disable debug interrupts */
> + irq_mask &= ~HDLCD_DEBUG_INT_MASK;
> +#endif
> +
> + /* disable vsync interrupts */
> + irq_mask &= ~HDLCD_INTERRUPT_VSYNC;
> + hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, irq_mask);
> +
> + free_irq(hdlcd->irq, drm);
> +}
> +
> static int hdlcd_load(struct drm_device *drm, unsigned long flags)
> {
> struct hdlcd_drm_private *hdlcd = drm->dev_private;
> @@ -90,7 +177,12 @@ static int hdlcd_load(struct drm_device *drm, unsigned
> long flags)
> goto setup_fail;
> }
>
> - ret = drm_irq_install(drm, platform_get_irq(pdev, 0));
> + ret = platform_get_irq(pd
Re: [PATCH 00/14] drm: Make DRM's IRQ helpers legacy
Hi Thomas, On Tue, Jul 27, 2021 at 08:27:07PM +0200, Thomas Zimmermann wrote: > DRM's IRQ helpers are only helpful for old, non-KMS drivers. Move > the code behind CONFIG_DRM_LEGACY. Convert KMS drivers to Linux > IRQ interfaces. > > DRM provides IRQ helpers for setting up, receiving and removing IRQ > handlers. It's an abstraction over plain Linux functions. The code > is mid-layerish with several callbacks to hook into the rsp drivers. > Old UMS driver have their interrupts enabled via ioctl, so these > abstractions makes some sense. Modern KMS manage all their interrupts > internally. Using the DRM helpers adds indirection without benefits. > > Most KMs drivers already use Linux IRQ functions instead of DRM's > abstraction layer. Patches 1 to 12 convert the remaining ones. > The patches also resolve a bug for devices without assigned interrupt > number. DRM helpers don't test for IRQ_NOTCONNECTED, so drivers do > not detect if the device has no interrupt assigned. Before diving into a review of these.. Any specific reason devm_request_irq is not used? Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [pull] amdgpu, amdkfd drm-fixes-5.14
Hi Alex, On Wed, Jul 14, 2021 at 06:08:58PM -0400, Alex Deucher wrote: > Hi Dave, Daniel, > > Fixes for 5.14. The big change here is unifying the SMU13 code. This was > new code added in 5.14 to support Yellow Carp, but we've since cleaned it > up and removed a lot of duplication, so better to merge it now to facilitate > any bug fixes in the future that need to go back to this kernel via stable. > Only affects Yellow Carp which is new for 5.14 anyway so not much chance for > regressions. The rest is just standard bug fixes. This pull seems not to include any fixes for the W=1 warnings that has crept in again. It would be nice if the amdgpu could be warning free again, this would maybe motivate the others to fix theirs too so we could keep most/all of drivers/gpu/ free of W=1 warnings. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 00/15] drm: Move struct drm_device.pdev to legacy
Hi Thomas. Nice clean-up series - quite an effort to move one member to deprecated! I have read through most of the patches. I left a few notes in my replies but nothing buggy. Just nitpicks. On Tue, Nov 24, 2020 at 12:38:09PM +0100, Thomas Zimmermann wrote: > The pdev field in struct drm_device points to a PCI device structure and > goes back to UMS-only days when all DRM drivers where for PCI devices. > Meanwhile we also support USB, SPI and platform devices. Each of those > uses the generic device stored in struct drm_device.dev. > > To reduce duplications and remove the special case of PCI, this patchset > converts all modesetting drivers from pdev to dev and makes pdev a field > for legacy UMS drivers. > > For PCI devices, the pointer in struct drm_device.dev can be upcasted to > struct pci_device; or tested for PCI with dev_is_pci(). In several places > the code can use the dev field directly. > > After converting all drivers and the DRM core, the pdev fields becomes > only relevant for legacy drivers. In a later patchset, we may want to > convert these as well and remove pdev entirely. > > The patchset touches many files, but the individual changes are mostly > trivial. I suggest to merge each driver's patch through the respective > tree and later the rest through drm-misc-next. > > Thomas Zimmermann (15): > drm/amdgpu: Remove references to struct drm_device.pdev > drm/ast: Remove references to struct drm_device.pdev > drm/bochs: Remove references to struct drm_device.pdev > drm/cirrus: Remove references to struct drm_device.pdev > drm/gma500: Remove references to struct drm_device.pdev > drm/hibmc: Remove references to struct drm_device.pdev > drm/mgag200: Remove references to struct drm_device.pdev > drm/qxl: Remove references to struct drm_device.pdev > drm/vboxvideo: Remove references to struct drm_device.pdev > drm/virtgpu: Remove references to struct drm_device.pdev > drm/vmwgfx: Remove references to struct drm_device.pdev > drm: Upcast struct drm_device.dev to struct pci_device; replace pdev All above are: Acked-by: Sam Ravnborg > drm/nouveau: Remove references to struct drm_device.pdev I lost my confidence in my reading of this code. > drm/i915: Remove references to struct drm_device.pdev > drm/radeon: Remove references to struct drm_device.pdev I did not look at these at all. I hope someone else find time to do so. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 15/15] drm: Upcast struct drm_device.dev to struct pci_device; replace pdev
Hi Thomas,
On Tue, Nov 24, 2020 at 12:38:24PM +0100, Thomas Zimmermann wrote:
> We have DRM drivers based on USB, SPI and platform devices. All of them
> are fine with storing their device reference in struct drm_device.dev.
> PCI devices should be no exception. Therefore struct drm_device.pdev is
> deprecated.
>
> Instead upcast from struct drm_device.dev with to_pci_dev(). PCI-specific
> code can use dev_is_pci() to test for a PCI device. This patch changes
> the DRM core code and documentation accordingly. Struct drm_device.pdev
> is being moved to legacy status.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_agpsupport.c | 9 ++---
> drivers/gpu/drm/drm_bufs.c | 4 ++--
> drivers/gpu/drm/drm_edid.c | 7 ++-
> drivers/gpu/drm/drm_irq.c| 12 +++-
> drivers/gpu/drm/drm_pci.c| 26 +++---
> drivers/gpu/drm/drm_vm.c | 2 +-
> include/drm/drm_device.h | 12 +---
> 7 files changed, 46 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_agpsupport.c
> b/drivers/gpu/drm/drm_agpsupport.c
> index 4c7ad46fdd21..a4040fe4f4ba 100644
> --- a/drivers/gpu/drm/drm_agpsupport.c
> +++ b/drivers/gpu/drm/drm_agpsupport.c
> @@ -103,11 +103,13 @@ int drm_agp_info_ioctl(struct drm_device *dev, void
> *data,
> */
> int drm_agp_acquire(struct drm_device *dev)
> {
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> +
> if (!dev->agp)
> return -ENODEV;
> if (dev->agp->acquired)
> return -EBUSY;
> - dev->agp->bridge = agp_backend_acquire(dev->pdev);
> + dev->agp->bridge = agp_backend_acquire(pdev);
> if (!dev->agp->bridge)
> return -ENODEV;
> dev->agp->acquired = 1;
> @@ -402,14 +404,15 @@ int drm_agp_free_ioctl(struct drm_device *dev, void
> *data,
> */
> struct drm_agp_head *drm_agp_init(struct drm_device *dev)
> {
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> struct drm_agp_head *head = NULL;
>
> head = kzalloc(sizeof(*head), GFP_KERNEL);
> if (!head)
> return NULL;
> - head->bridge = agp_find_bridge(dev->pdev);
> + head->bridge = agp_find_bridge(pdev);
> if (!head->bridge) {
> - head->bridge = agp_backend_acquire(dev->pdev);
> + head->bridge = agp_backend_acquire(pdev);
> if (!head->bridge) {
> kfree(head);
> return NULL;
> diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
> index 7a01d0918861..1da8b360b60a 100644
> --- a/drivers/gpu/drm/drm_bufs.c
> +++ b/drivers/gpu/drm/drm_bufs.c
> @@ -325,7 +325,7 @@ static int drm_addmap_core(struct drm_device *dev,
> resource_size_t offset,
>* As we're limiting the address to 2^32-1 (or less),
>* casting it down to 32 bits is no problem, but we
>* need to point to a 64bit variable first. */
> - map->handle = dma_alloc_coherent(>pdev->dev,
> + map->handle = dma_alloc_coherent(dev->dev,
>map->size,
>>offset,
>GFP_KERNEL);
> @@ -555,7 +555,7 @@ int drm_legacy_rmmap_locked(struct drm_device *dev,
> struct drm_local_map *map)
> case _DRM_SCATTER_GATHER:
> break;
> case _DRM_CONSISTENT:
> - dma_free_coherent(>pdev->dev,
> + dma_free_coherent(dev->dev,
> map->size,
> map->handle,
> map->offset);
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 74f5a3197214..555a04ce2179 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -32,6 +32,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -2075,9 +2076,13 @@ EXPORT_SYMBOL(drm_get_edid);
> struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
>struct i2c_adapter *adapter)
> {
> - struct pci_dev *pdev = connector->dev->pdev;
> + struct drm_device *dev = connector->dev;
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> struct edid *edid;
Maybe add a comment that explain why this can trigger - so people are
helped it they are catched by this.
As it is now it is not even mentioned in the changelog.
> + if (drm_WARN_ON_ONCE(dev, !dev_is_pci(dev->dev)))
> + return NULL;
> +
> vga_switcheroo_lock_ddc(pdev);
> edid = drm_get_edid(connector, adapter);
> vga_switcheroo_unlock_ddc(pdev);
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 09d6e9e2e075..22986a9a593b 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -122,7 +122,7 @@ int drm_irq_install(struct drm_device *dev, int
Re: [PATCH 09/15] drm/nouveau: Remove references to struct drm_device.pdev
Hi Thomas.
On Tue, Nov 24, 2020 at 12:38:18PM +0100, Thomas Zimmermann wrote:
> Using struct drm_device.pdev is deprecated. Convert nouveau to struct
> drm_device.dev. No functional changes.
>
> Signed-off-by: Thomas Zimmermann
> Cc: Ben Skeggs
Suggestion to an alternative implmentation below.
> ---
> drivers/gpu/drm/nouveau/dispnv04/arb.c | 12 +++-
> drivers/gpu/drm/nouveau/dispnv04/disp.h | 14 --
> drivers/gpu/drm/nouveau/dispnv04/hw.c | 10 ++
> drivers/gpu/drm/nouveau/nouveau_abi16.c | 7 ---
> drivers/gpu/drm/nouveau/nouveau_acpi.c | 2 +-
> drivers/gpu/drm/nouveau/nouveau_bios.c | 11 ---
> drivers/gpu/drm/nouveau/nouveau_connector.c | 10 ++
> drivers/gpu/drm/nouveau/nouveau_drm.c | 5 ++---
> drivers/gpu/drm/nouveau/nouveau_fbcon.c | 6 --
> drivers/gpu/drm/nouveau/nouveau_vga.c | 20
> 10 files changed, 58 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bios.c
> b/drivers/gpu/drm/nouveau/nouveau_bios.c
> index d204ea8a5618..7cc683b8dc7a 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bios.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bios.c
> @@ -110,6 +110,9 @@ static int call_lvds_manufacturer_script(struct
> drm_device *dev, struct dcb_outp
> struct nvbios *bios = >vbios;
> uint8_t sub = bios->data[bios->fp.xlated_entry + script] +
> (bios->fp.link_c_increment && dcbent->or & DCB_OUTPUT_C ? 1 : 0);
> uint16_t scriptofs = ROM16(bios->data[bios->init_script_tbls_ptr + sub
> * 2]);
> +#ifdef __powerpc__
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> +#endif
Or
int device = 0;
>
> if (!bios->fp.xlated_entry || !sub || !scriptofs)
> return -EINVAL;
> @@ -123,8 +126,8 @@ static int call_lvds_manufacturer_script(struct
> drm_device *dev, struct dcb_outp
> #ifdef __powerpc__
> /* Powerbook specific quirks */
device = to_pci_dev(dev->dev)->device;
if (script == LVDS_RESET && (device == 0x0179 || device == 0x0189 ||
device == 0x0329))
> if (script == LVDS_RESET &&
> - (dev->pdev->device == 0x0179 || dev->pdev->device == 0x0189 ||
> - dev->pdev->device == 0x0329))
> + (pdev->device == 0x0179 || pdev->device == 0x0189 ||
> + pdev->device == 0x0329))
> nv_write_tmds(dev, dcbent->or, 0, 0x02, 0x72);
> #endif
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> index 24ec5339efb4..4fc0fa696461 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
> @@ -396,7 +396,9 @@ nouveau_fbcon_create(struct drm_fb_helper *helper,
> NV_INFO(drm, "allocated %dx%d fb: 0x%llx, bo %p\n",
> fb->width, fb->height, nvbo->offset, nvbo);
>
> - vga_switcheroo_client_fb_set(dev->pdev, info);
> + if (dev_is_pci(dev->dev))
> + vga_switcheroo_client_fb_set(to_pci_dev(dev->dev), info);
> +
I cannot see why dev_is_pci() is needed here.
So I am obviously missing something :-(
> return 0;
>
> out_unlock:
> @@ -548,7 +550,7 @@ nouveau_fbcon_init(struct drm_device *dev)
> int ret;
>
> if (!dev->mode_config.num_crtc ||
> - (dev->pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> + (to_pci_dev(dev->dev)->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> return 0;
>
> fbcon = kzalloc(sizeof(struct nouveau_fbdev), GFP_KERNEL);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c
> b/drivers/gpu/drm/nouveau/nouveau_vga.c
> index c85dd8afa3c3..7c4b374b3eca 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_vga.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
> @@ -87,18 +87,20 @@ nouveau_vga_init(struct nouveau_drm *drm)
> {
> struct drm_device *dev = drm->dev;
> bool runtime = nouveau_pmops_runtime();
> + struct pci_dev *pdev;
>
> /* only relevant for PCI devices */
> - if (!dev->pdev)
> + if (!dev_is_pci(dev->dev))
> return;
> + pdev = to_pci_dev(dev->dev);
>
> - vga_client_register(dev->pdev, dev, NULL, nouveau_vga_set_decode);
> + vga_client_register(pdev, dev, NULL, nouveau_vga_set_decode);
>
> /* don't register Thunderbolt eGPU with vga_switcheroo */
> - if (pci_is_thunderbolt_attached(dev->pdev))
> + if (pci_is_thunderbolt_attached(pdev))
> return;
>
> - vga_switcheroo_register_client(dev->pdev, _switcheroo_ops,
> runtime);
> + vga_switcheroo_register_client(pdev, _switcheroo_ops, runtime);
>
> if (runtime && nouveau_is_v1_dsm() && !nouveau_is_optimus())
> vga_switcheroo_init_domain_pm_ops(drm->dev->dev,
> >vga_pm_domain);
> @@ -109,17 +111,19 @@ nouveau_vga_fini(struct nouveau_drm *drm)
> {
> struct drm_device *dev = drm->dev;
> bool runtime = nouveau_pmops_runtime();
> + struct pci_dev *pdev;
>
> /* only
Re: [PATCH 05/15] drm/gma500: Remove references to struct drm_device.pdev
Hi Thomas.
On Tue, Nov 24, 2020 at 12:38:14PM +0100, Thomas Zimmermann wrote:
> Using struct drm_device.pdev is deprecated. Convert gma500 to struct
> drm_device.dev. No functional changes.
>
> Signed-off-by: Thomas Zimmermann
> Cc: Patrik Jakobsson
This patch includes several whitespace changes too.
It would be nice to avoid these as the patch is already large enough.
Browsing the patch it was not so many, it looked like more in the start
of the patch.
Sam
> ---
> drivers/gpu/drm/gma500/cdv_device.c| 30 +++---
> drivers/gpu/drm/gma500/cdv_intel_crt.c | 3 +-
> drivers/gpu/drm/gma500/cdv_intel_lvds.c| 4 +--
> drivers/gpu/drm/gma500/framebuffer.c | 9 +++---
> drivers/gpu/drm/gma500/gma_device.c| 3 +-
> drivers/gpu/drm/gma500/gma_display.c | 4 +--
> drivers/gpu/drm/gma500/gtt.c | 20 ++--
> drivers/gpu/drm/gma500/intel_bios.c| 6 ++--
> drivers/gpu/drm/gma500/intel_gmbus.c | 4 +--
> drivers/gpu/drm/gma500/intel_i2c.c | 2 +-
> drivers/gpu/drm/gma500/mdfld_device.c | 4 ++-
> drivers/gpu/drm/gma500/mdfld_dsi_dpi.c | 8 ++---
> drivers/gpu/drm/gma500/mid_bios.c | 9 --
> drivers/gpu/drm/gma500/oaktrail_device.c | 5 +--
> drivers/gpu/drm/gma500/oaktrail_lvds.c | 2 +-
> drivers/gpu/drm/gma500/oaktrail_lvds_i2c.c | 2 +-
> drivers/gpu/drm/gma500/opregion.c | 3 +-
> drivers/gpu/drm/gma500/power.c | 13
> drivers/gpu/drm/gma500/psb_drv.c | 16 +-
> drivers/gpu/drm/gma500/psb_drv.h | 8 ++---
> drivers/gpu/drm/gma500/psb_intel_lvds.c| 6 ++--
> drivers/gpu/drm/gma500/psb_intel_sdvo.c| 2 +-
> drivers/gpu/drm/gma500/tc35876x-dsi-lvds.c | 36 +++---
> 23 files changed, 109 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/cdv_device.c
> b/drivers/gpu/drm/gma500/cdv_device.c
> index e75293e4a52f..19e055dbd4c2 100644
> --- a/drivers/gpu/drm/gma500/cdv_device.c
> +++ b/drivers/gpu/drm/gma500/cdv_device.c
> @@ -95,13 +95,14 @@ static u32 cdv_get_max_backlight(struct drm_device *dev)
> static int cdv_get_brightness(struct backlight_device *bd)
> {
> struct drm_device *dev = bl_get_data(bd);
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> u32 val = REG_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
>
> if (cdv_backlight_combination_mode(dev)) {
> u8 lbpc;
>
> val &= ~1;
> - pci_read_config_byte(dev->pdev, 0xF4, );
> + pci_read_config_byte(pdev, 0xF4, );
> val *= lbpc;
> }
> return (val * 100)/cdv_get_max_backlight(dev);
> @@ -111,6 +112,7 @@ static int cdv_get_brightness(struct backlight_device *bd)
> static int cdv_set_brightness(struct backlight_device *bd)
> {
> struct drm_device *dev = bl_get_data(bd);
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> int level = bd->props.brightness;
> u32 blc_pwm_ctl;
>
> @@ -128,7 +130,7 @@ static int cdv_set_brightness(struct backlight_device *bd)
> lbpc = level * 0xfe / max + 1;
> level /= lbpc;
>
> - pci_write_config_byte(dev->pdev, 0xF4, lbpc);
> + pci_write_config_byte(pdev, 0xF4, lbpc);
> }
>
> blc_pwm_ctl = REG_READ(BLC_PWM_CTL) & ~BACKLIGHT_DUTY_CYCLE_MASK;
> @@ -205,8 +207,9 @@ static inline void CDV_MSG_WRITE32(int domain, uint port,
> uint offset,
> static void cdv_init_pm(struct drm_device *dev)
> {
> struct drm_psb_private *dev_priv = dev->dev_private;
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> u32 pwr_cnt;
> - int domain = pci_domain_nr(dev->pdev->bus);
> + int domain = pci_domain_nr(pdev->bus);
> int i;
>
> dev_priv->apm_base = CDV_MSG_READ32(domain, PSB_PUNIT_PORT,
> @@ -234,6 +237,8 @@ static void cdv_init_pm(struct drm_device *dev)
>
> static void cdv_errata(struct drm_device *dev)
> {
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> +
> /* Disable bonus launch.
>* CPU and GPU competes for memory and display misses updates and
>* flickers. Worst with dual core, dual displays.
> @@ -242,7 +247,7 @@ static void cdv_errata(struct drm_device *dev)
>* Bonus Launch to work around the issue, by degrading
>* performance.
>*/
> - CDV_MSG_WRITE32(pci_domain_nr(dev->pdev->bus), 3, 0x30, 0x08027108);
> + CDV_MSG_WRITE32(pci_domain_nr(pdev->bus), 3, 0x30, 0x08027108);
> }
>
> /**
> @@ -255,12 +260,13 @@ static void cdv_errata(struct drm_device *dev)
> static int cdv_save_display_registers(struct drm_device *dev)
> {
> struct drm_psb_private *dev_priv = dev->dev_private;
> + struct pci_dev *pdev = to_pci_dev(dev->dev);
> struct psb_save_area *regs = _priv->regs;
> struct drm_connector *connector;
>
> dev_dbg(dev->dev, "Saving GPU registers.\n");
>
Re: [PATCH 000/141] Fix fall-through warnings for Clang
Hi James. > > > If none of the 140 patches here fix a real bug, and there is no > > > change to machine code then it sounds to me like a W=2 kind of a > > > warning. > > > > FWIW, this series has found at least one bug so far: > > https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/ > > > Well, it's a problem in an error leg, sure, but it's not a really > compelling reason for a 141 patch series, is it? All that fixing this > error will do is get the driver to print "oh dear there's a problem" > under four more conditions than it previously did. You are asking the wrong question here. Yuo should ask how many hours could have been saved by all the bugs people have been fighting with and then fixed *before* the code hit the kernel at all. My personal experience is that I, more than once, have had errors related to a missing break in my code. So this warnings is IMO a win. And if we are only ~100 patches to have it globally enabled then it is a no-brainer in my book. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 15/20] drm/radeon/r600d: Move 'rc600_*' prototypes into shared header
Hi Lee, > > the *d.h headers are supposed to just be hardware definitions. I'd > > prefer to keep driver stuff out of them. > > That's fine (I did wonder if that were the case). > > I need an answer from you and Sam whether I can create new headers. > > For me, it is the right thing to do. Please follow the advice of Alex for the radeon driver. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 17/19] drm/radeon/radeon_kms: Fix misnaming of 'radeon_info_ioctl's dev param
On Mon, Nov 09, 2020 at 08:10:13PM +, Lee Jones wrote: > On Mon, 09 Nov 2020, Sam Ravnborg wrote: > > > Hi Alex, > > On Mon, Nov 09, 2020 at 02:50:35PM -0500, Alex Deucher wrote: > > > On Fri, Nov 6, 2020 at 4:50 PM Lee Jones wrote: > > > > > > > > Fixes the following W=1 kernel build warning(s): > > > > > > > > drivers/gpu/drm/radeon/radeon_kms.c:226: warning: Function parameter > > > > or member 'dev' not described in 'radeon_info_ioctl' > > > > drivers/gpu/drm/radeon/radeon_kms.c:226: warning: Excess function > > > > parameter 'rdev' description in 'radeon_info_ioctl' > > > > > > > > Cc: Alex Deucher > > > > Cc: "Christian König" > > > > Cc: David Airlie > > > > Cc: Daniel Vetter > > > > Cc: amd-gfx@lists.freedesktop.org > > > > Cc: dri-de...@lists.freedesktop.org > > > > Signed-off-by: Lee Jones > > > > --- > > > > drivers/gpu/drm/radeon/radeon_kms.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/radeon/radeon_kms.c > > > > b/drivers/gpu/drm/radeon/radeon_kms.c > > > > index 0d8fbabffcead..21c206795c364 100644 > > > > --- a/drivers/gpu/drm/radeon/radeon_kms.c > > > > +++ b/drivers/gpu/drm/radeon/radeon_kms.c > > > > @@ -213,7 +213,7 @@ static void radeon_set_filp_rights(struct > > > > drm_device *dev, > > > > /** > > > > * radeon_info_ioctl - answer a device specific request. > > > > * > > > > - * @rdev: radeon device pointer > > > > + * @dev: radeon device pointer > > > > > > This should be: > > > + * @dev: drm device pointer > > > > good spot. I am continuing the work on radeon and will post a patchset > > that contains only radeon fixes with Lee's patches and a few more by me. > > I will fix the above. > > What do you mean by "continuing on"? > > How will you prevent your work from conflicting with my current effort? Quoting from previous mail in this thread: " > > How would you like me to move forward? > > Fix the thousand of warnings in other places :-) > I will take a look at radeon and post a new series based on your work > with all W=1 warnings fixed. I'll drop this patch and carry on ploughing through the rest of them. " Here I promised you to fix all warnings in the radeon driver. And despite this being more work than anticipated a promise is a promise. So therefore I started working on this. If you want to do the rest of the radeon driver you are welcome and I will gladly drop this again. Just let me know your preference. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 17/19] drm/radeon/radeon_kms: Fix misnaming of 'radeon_info_ioctl's dev param
Hi Alex, On Mon, Nov 09, 2020 at 02:50:35PM -0500, Alex Deucher wrote: > On Fri, Nov 6, 2020 at 4:50 PM Lee Jones wrote: > > > > Fixes the following W=1 kernel build warning(s): > > > > drivers/gpu/drm/radeon/radeon_kms.c:226: warning: Function parameter or > > member 'dev' not described in 'radeon_info_ioctl' > > drivers/gpu/drm/radeon/radeon_kms.c:226: warning: Excess function > > parameter 'rdev' description in 'radeon_info_ioctl' > > > > Cc: Alex Deucher > > Cc: "Christian König" > > Cc: David Airlie > > Cc: Daniel Vetter > > Cc: amd-gfx@lists.freedesktop.org > > Cc: dri-de...@lists.freedesktop.org > > Signed-off-by: Lee Jones > > --- > > drivers/gpu/drm/radeon/radeon_kms.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/radeon/radeon_kms.c > > b/drivers/gpu/drm/radeon/radeon_kms.c > > index 0d8fbabffcead..21c206795c364 100644 > > --- a/drivers/gpu/drm/radeon/radeon_kms.c > > +++ b/drivers/gpu/drm/radeon/radeon_kms.c > > @@ -213,7 +213,7 @@ static void radeon_set_filp_rights(struct drm_device > > *dev, > > /** > > * radeon_info_ioctl - answer a device specific request. > > * > > - * @rdev: radeon device pointer > > + * @dev: radeon device pointer > > This should be: > + * @dev: drm device pointer good spot. I am continuing the work on radeon and will post a patchset that contains only radeon fixes with Lee's patches and a few more by me. I will fix the above. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 10/19] drm/radeon/radeon: Move prototype into shared header
Hi Lee, > > > > Other public functions in radeon_device.c have their prototype in > > radeon.h - for example radeon_is_px() > > > > Add radeon_device_is_virtual() there so we avoiid this new header. > > Oh yes, I remember why this wasn't a suitable solution now: > > The macro "radeon_init" in radeon.h clashes with the init function of > the same name in radeon_drv.c: > > In file included from drivers/gpu/drm/radeon/radeon_drv.c:53: > drivers/gpu/drm/radeon/radeon_drv.c:620:31: error: expected identifier or > ‘(’ before ‘void’ > 620 | static int __init radeon_init(void) ... > > How would you like me to move forward? Fix the thousand of warnings in other places :-) I will take a look at radeon and post a new series based on your work with all W=1 warnings fixed. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 00/19] [Set 2] Rid W=1 warnings from GPU
On Sat, Nov 07, 2020 at 06:41:38PM +, Lee Jones wrote: > On Sat, 07 Nov 2020, Sam Ravnborg wrote: > > > Hi Christian. > > > > > I'm not sure if we want to do some of the suggested changes to radeon. > > > > All patches for radeon looks good to me except "drm/radeon/radeon: Move > > prototype into shared header". > > Was that the one where the prototype needs moving to radeon.h? Yes, Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 00/19] [Set 2] Rid W=1 warnings from GPU
Hi Christian. > I'm not sure if we want to do some of the suggested changes to radeon. All patches for radeon looks good to me except "drm/radeon/radeon: Move prototype into shared header". Acked-by: Sam Ravnborg from me to have them applied (except the shared header one). I can reply to the individual patches if you like. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 17/19] drm/radeon/radeon_kms: Fix misnaming of 'radeon_info_ioctl's dev param
Hi Lee, On Fri, Nov 06, 2020 at 09:49:47PM +, Lee Jones wrote: > Fixes the following W=1 kernel build warning(s): > > drivers/gpu/drm/radeon/radeon_kms.c:226: warning: Function parameter or > member 'dev' not described in 'radeon_info_ioctl' > drivers/gpu/drm/radeon/radeon_kms.c:226: warning: Excess function parameter > 'rdev' description in 'radeon_info_ioctl' > > Cc: Alex Deucher > Cc: "Christian König" > Cc: David Airlie > Cc: Daniel Vetter > Cc: amd-gfx@lists.freedesktop.org > Cc: dri-de...@lists.freedesktop.org > Signed-off-by: Lee Jones > --- > drivers/gpu/drm/radeon/radeon_kms.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/radeon/radeon_kms.c > b/drivers/gpu/drm/radeon/radeon_kms.c > index 0d8fbabffcead..21c206795c364 100644 > --- a/drivers/gpu/drm/radeon/radeon_kms.c > +++ b/drivers/gpu/drm/radeon/radeon_kms.c > @@ -213,7 +213,7 @@ static void radeon_set_filp_rights(struct drm_device *dev, > /** > * radeon_info_ioctl - answer a device specific request. > * > - * @rdev: radeon device pointer > + * @dev: radeon device pointer > * @data: request object > * @filp: drm filp > * Delete all the kernel-doc annotation as we do not pull this file into the kernel-doc anyway. Keep the /* Answer a device specific request */ part. At least thats what I see as the best way to deal with it. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 10/19] drm/radeon/radeon: Move prototype into shared header
Hi Lee, On Fri, Nov 06, 2020 at 09:49:40PM +, Lee Jones wrote: > Unfortunately, a suitable one didn't already exist. > > Fixes the following W=1 kernel build warning(s): > > drivers/gpu/drm/radeon/radeon_device.c:637:6: warning: no previous prototype > for ‘radeon_device_is_virtual’ [-Wmissing-prototypes] > 637 | bool radeon_device_is_virtual(void) > | ^~~~ > > Cc: Alex Deucher > Cc: "Christian König" > Cc: David Airlie > Cc: Daniel Vetter > Cc: Sumit Semwal > Cc: amd-gfx@lists.freedesktop.org > Cc: dri-de...@lists.freedesktop.org > Cc: linux-me...@vger.kernel.org > Cc: linaro-mm-...@lists.linaro.org > Signed-off-by: Lee Jones > --- > drivers/gpu/drm/radeon/radeon_device.c | 1 + > drivers/gpu/drm/radeon/radeon_device.h | 32 ++ > drivers/gpu/drm/radeon/radeon_drv.c| 3 +-- > 3 files changed, 34 insertions(+), 2 deletions(-) > create mode 100644 drivers/gpu/drm/radeon/radeon_device.h Other public functions in radeon_device.c have their prototype in radeon.h - for example radeon_is_px() Add radeon_device_is_virtual() there so we avoiid this new header. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 00/19] [Set 2] Rid W=1 warnings from GPU
Hi Lee and DRM folks.
On Fri, Nov 06, 2020 at 09:49:30PM +, Lee Jones wrote:
> This set is part of a larger effort attempting to clean-up W=1
> kernel builds, which are currently overwhelmingly riddled with
> niggly little warnings.
>
> There are 5000 warnings to work through. It will take a couple more
> sets. Although, ("drm/amd/display/dc/basics/fixpt31_32: Move
> variables to where they're used") does take care of 2000 of them!
>
> Lee Jones (19):
> drm/ttm/ttm_range_manager: Demote non-conformant kernel-doc header
> drm/r128/ati_pcigart: Source file headers are not good candidates for
> kernel-doc
Applied
> drm/selftests/test-drm_dp_mst_helper: Move
> 'sideband_msg_req_encode_decode' onto the heap
> drm/mga/mga_dma: Demote kernel-doc abusers to standard comment blocks
> drm/mga/mga_state: Remove unused variable 'buf_priv'
Applied x2
> drm/radeon/atom: Move prototype into shared location
> drm/radeon/radeon_kms: Include header containing our own prototypes
> drm/omapdrm/omap_gem: Fix misnamed and missing parameter descriptions
> drm/omapdrm/omap_dmm_tiler: Demote abusive use of kernel-doc format
> drm/radeon/radeon: Move prototype into shared header
> drm/radeon/radeon_drv: Source file headers are not good candidates for
> kernel-doc
> drm/amd/display/dc/basics/fixpt31_32: Move variables to where they're
> used
> drm/radeon/radeon_drv: Move prototypes to a shared headerfile
> drm/amd/amdgpu/amdgpu_device: Provide documentation for 'reg_addr'
> params
> drm/radeon: Move prototypes to shared header
> drm/amd/amdgpu/amdgpu_kms: Remove 'struct drm_amdgpu_info_device
> dev_info' from the stack
> drm/radeon/radeon_kms: Fix misnaming of 'radeon_info_ioctl's dev param
> drm/radeon/atombios_crtc: Remove description of non-existent function
> param 'encoder'
> drm/v3d/v3d_drv: Remove unused static variable 'v3d_v3d_pm_ops'
I have applied the three patches that has no obvious maintainer as indicated
above. I assume the respective maintaines to pick radeon, omapdrm, ttm,
amd, v3d and selftest patches.
Sam
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 00/19] [Set 1] Rid W=1 warnings from GPU
Hi Lee. On Thu, Nov 05, 2020 at 02:44:58PM +, Lee Jones wrote: > This set is part of a larger effort attempting to clean-up W=1 > kernel builds, which are currently overwhelmingly riddled with > niggly little warnings. Thanks for looking into this. > There are 5000 warnings to work through. > > It will take a couple more sets. :-) > gpu: drm: panel: panel-simple: Fix 'struct panel_desc's header I have a patch here that inline the comments - and fix the warning as a side effect. I will get it posted tonight as this is better. > gpu: drm: bridge: analogix: analogix_dp_reg: Remove unused function > 'analogix_dp_write_byte_to_dpcd' When I looked at his I had another unused function after removing the first. > gpu: drm: panel: panel-ilitek-ili9322: Demote non-conformant > kernel-doc header Agree on this simple approch, will apply. > gpu: drm: bridge: analogix: analogix_dp_reg: Remove unused function > 'analogix_dp_start_aux_transaction' OK, this was the one I referred to above. They should be squashed into one patch. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v5 10/10] drm/fb_helper: Support framebuffers in I/O memory
Hi Thomas.
On Tue, Oct 20, 2020 at 02:20:46PM +0200, Thomas Zimmermann wrote:
> At least sparc64 requires I/O-specific access to framebuffers. This
> patch updates the fbdev console accordingly.
>
> For drivers with direct access to the framebuffer memory, the callback
> functions in struct fb_ops test for the type of memory and call the rsp
> fb_sys_ of fb_cfb_ functions. Read and write operations are implemented
> internally by DRM's fbdev helper.
>
> For drivers that employ a shadow buffer, fbdev's blit function retrieves
> the framebuffer address as struct dma_buf_map, and uses dma_buf_map
> interfaces to access the buffer.
>
> The bochs driver on sparc64 uses a workaround to flag the framebuffer as
> I/O memory and avoid a HW exception. With the introduction of struct
> dma_buf_map, this is not required any longer. The patch removes the rsp
> code from both, bochs and fbdev.
>
> v5:
> * implement fb_read/fb_write internally (Daniel, Sam)
> v4:
> * move dma_buf_map changes into separate patch (Daniel)
> * TODO list: comment on fbdev updates (Daniel)
>
> Signed-off-by: Thomas Zimmermann
> Tested-by: Sam Ravnborg
Reviewed-by: Sam Ravnborg
But see a few comments below on naming for you to consider.
Sam
> ---
> Documentation/gpu/todo.rst| 19 ++-
> drivers/gpu/drm/bochs/bochs_kms.c | 1 -
> drivers/gpu/drm/drm_fb_helper.c | 227 --
> include/drm/drm_mode_config.h | 12 --
> 4 files changed, 230 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 7e6fc3c04add..638b7f704339 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup()
>
>
> Most drivers can use drm_fbdev_generic_setup(). Driver have to implement
> -atomic modesetting and GEM vmap support. Current generic fbdev emulation
> -expects the framebuffer in system memory (or system-like memory).
> +atomic modesetting and GEM vmap support. Historically, generic fbdev
> emulation
> +expected the framebuffer in system memory or system-like memory. By employing
> +struct dma_buf_map, drivers with frambuffers in I/O memory can be supported
> +as well.
>
> Contact: Maintainer of the driver you plan to convert
>
> Level: Intermediate
>
> +Reimplement functions in drm_fbdev_fb_ops without fbdev
> +---
> +
> +A number of callback functions in drm_fbdev_fb_ops could benefit from
> +being rewritten without dependencies on the fbdev module. Some of the
> +helpers could further benefit from using struct dma_buf_map instead of
> +raw pointers.
> +
> +Contact: Thomas Zimmermann , Daniel Vetter
> +
> +Level: Advanced
> +
> +
> drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
> -
>
> diff --git a/drivers/gpu/drm/bochs/bochs_kms.c
> b/drivers/gpu/drm/bochs/bochs_kms.c
> index 13d0d04c4457..853081d186d5 100644
> --- a/drivers/gpu/drm/bochs/bochs_kms.c
> +++ b/drivers/gpu/drm/bochs/bochs_kms.c
> @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs)
> bochs->dev->mode_config.preferred_depth = 24;
> bochs->dev->mode_config.prefer_shadow = 0;
> bochs->dev->mode_config.prefer_shadow_fbdev = 1;
> - bochs->dev->mode_config.fbdev_use_iomem = true;
> bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true;
>
> bochs->dev->mode_config.funcs = _mode_funcs;
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 6212cd7cde1d..1d3180841778 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -372,24 +372,22 @@ static void drm_fb_helper_resume_worker(struct
> work_struct *work)
> }
>
> static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper,
> - struct drm_clip_rect *clip)
> + struct drm_clip_rect *clip,
> + struct dma_buf_map *dst)
> {
> struct drm_framebuffer *fb = fb_helper->fb;
> unsigned int cpp = fb->format->cpp[0];
> size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
> void *src = fb_helper->fbdev->screen_buffer + offset;
> - void *dst = fb_helper->buffer->map.vaddr + offset;
> size_t len = (clip->x2 - clip->x1) * cpp;
> unsigned int y;
>
Re: [PATCH v4 10/10] drm/fb_helper: Support framebuffers in I/O memory
On Fri, Oct 16, 2020 at 02:19:42PM +0200, Thomas Zimmermann wrote: > Hi > > On Fri, 16 Oct 2020 14:03:47 +0200 Sam Ravnborg wrote: > > > Hi Thomas. > > > > On Thu, Oct 15, 2020 at 02:38:06PM +0200, Thomas Zimmermann wrote: > > > At least sparc64 requires I/O-specific access to framebuffers. This > > > patch updates the fbdev console accordingly. > > > > > > For drivers with direct access to the framebuffer memory, the callback > > > functions in struct fb_ops test for the type of memory and call the rsp > > > fb_sys_ of fb_cfb_ functions. > > > > > > For drivers that employ a shadow buffer, fbdev's blit function retrieves > > > the framebuffer address as struct dma_buf_map, and uses dma_buf_map > > > interfaces to access the buffer. > > > > > > The bochs driver on sparc64 uses a workaround to flag the framebuffer as > > > I/O memory and avoid a HW exception. With the introduction of struct > > > dma_buf_map, this is not required any longer. The patch removes the rsp > > > code from both, bochs and fbdev. > > > > > > v4: > > > * move dma_buf_map changes into separate patch (Daniel) > > > * TODO list: comment on fbdev updates (Daniel) > > > > > > Signed-off-by: Thomas Zimmermann > > > > The original workaround fixed it so we could run qemu with the > > -nographic option. > > > > So I went ahead and tried to run quemu version: > > v5.0.0-1970-g0b100c8e72-dirty. > > And with the BOCHS driver built-in. > > > > With the following command line: > > qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -nographic > > > > Behaviour was the same before and after applying this patch. > > (panic due to VFS: Unable to mount root fs on unknown-block(0,0)) > > So I consider it fixed for real now and not just a workaround. > > > > I also tested with: > > qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -serial > > stdio > > > > and it worked in both cases too. > > FTR, you booted a kernel and got graphics output. The error is simply that > there was no disk to mount? The short version "Yes". The longer version: With "qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -serial stdio" I got graphical output - one penguin. With "qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -nographic" I got no graphical output, as implied by the -nographic option. But the boot continued - where it would panic before when we accessed IO memory as system memory. In both cases I got an error because I had not specified any rootfs, so qemu failed to mount any rootfs. So expected. Sam > > Best regards > Thomas > > > > > All the comments above so future-me have an easier time finding how to > > reproduce. > > > > Tested-by: Sam Ravnborg > > > > Sam > > > > > --- > > > Documentation/gpu/todo.rst| 19 ++- > > > drivers/gpu/drm/bochs/bochs_kms.c | 1 - > > > drivers/gpu/drm/drm_fb_helper.c | 217 -- > > > include/drm/drm_mode_config.h | 12 -- > > > 4 files changed, 220 insertions(+), 29 deletions(-) > > > > > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > > > index 7e6fc3c04add..638b7f704339 100644 > > > --- a/Documentation/gpu/todo.rst > > > +++ b/Documentation/gpu/todo.rst > > > @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup() > > > > > > > > > Most drivers can use drm_fbdev_generic_setup(). Driver have to implement > > > -atomic modesetting and GEM vmap support. Current generic fbdev emulation > > > -expects the framebuffer in system memory (or system-like memory). > > > +atomic modesetting and GEM vmap support. Historically, generic fbdev > > > emulation +expected the framebuffer in system memory or system-like > > > memory. By employing +struct dma_buf_map, drivers with frambuffers in I/O > > > memory can be supported +as well. > > > > > > Contact: Maintainer of the driver you plan to convert > > > > > > Level: Intermediate > > > > > > +Reimplement functions in drm_fbdev_fb_ops without fbdev > > > +--- > > > + > > > +A number of callback functions in drm_fbdev_fb_ops could benefit from > > > +being rewritten without depen
Re: [PATCH v4 10/10] drm/fb_helper: Support framebuffers in I/O memory
Hi Thomas. On Thu, Oct 15, 2020 at 02:38:06PM +0200, Thomas Zimmermann wrote: > At least sparc64 requires I/O-specific access to framebuffers. This > patch updates the fbdev console accordingly. > > For drivers with direct access to the framebuffer memory, the callback > functions in struct fb_ops test for the type of memory and call the rsp > fb_sys_ of fb_cfb_ functions. > > For drivers that employ a shadow buffer, fbdev's blit function retrieves > the framebuffer address as struct dma_buf_map, and uses dma_buf_map > interfaces to access the buffer. > > The bochs driver on sparc64 uses a workaround to flag the framebuffer as > I/O memory and avoid a HW exception. With the introduction of struct > dma_buf_map, this is not required any longer. The patch removes the rsp > code from both, bochs and fbdev. > > v4: > * move dma_buf_map changes into separate patch (Daniel) > * TODO list: comment on fbdev updates (Daniel) > > Signed-off-by: Thomas Zimmermann The original workaround fixed it so we could run qemu with the -nographic option. So I went ahead and tried to run quemu version: v5.0.0-1970-g0b100c8e72-dirty. And with the BOCHS driver built-in. With the following command line: qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -nographic Behaviour was the same before and after applying this patch. (panic due to VFS: Unable to mount root fs on unknown-block(0,0)) So I consider it fixed for real now and not just a workaround. I also tested with: qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -serial stdio and it worked in both cases too. All the comments above so future-me have an easier time finding how to reproduce. Tested-by: Sam Ravnborg Sam > --- > Documentation/gpu/todo.rst| 19 ++- > drivers/gpu/drm/bochs/bochs_kms.c | 1 - > drivers/gpu/drm/drm_fb_helper.c | 217 -- > include/drm/drm_mode_config.h | 12 -- > 4 files changed, 220 insertions(+), 29 deletions(-) > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > index 7e6fc3c04add..638b7f704339 100644 > --- a/Documentation/gpu/todo.rst > +++ b/Documentation/gpu/todo.rst > @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup() > > > Most drivers can use drm_fbdev_generic_setup(). Driver have to implement > -atomic modesetting and GEM vmap support. Current generic fbdev emulation > -expects the framebuffer in system memory (or system-like memory). > +atomic modesetting and GEM vmap support. Historically, generic fbdev > emulation > +expected the framebuffer in system memory or system-like memory. By employing > +struct dma_buf_map, drivers with frambuffers in I/O memory can be supported > +as well. > > Contact: Maintainer of the driver you plan to convert > > Level: Intermediate > > +Reimplement functions in drm_fbdev_fb_ops without fbdev > +--- > + > +A number of callback functions in drm_fbdev_fb_ops could benefit from > +being rewritten without dependencies on the fbdev module. Some of the > +helpers could further benefit from using struct dma_buf_map instead of > +raw pointers. > + > +Contact: Thomas Zimmermann , Daniel Vetter > + > +Level: Advanced > + > + > drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup > - > > diff --git a/drivers/gpu/drm/bochs/bochs_kms.c > b/drivers/gpu/drm/bochs/bochs_kms.c > index 13d0d04c4457..853081d186d5 100644 > --- a/drivers/gpu/drm/bochs/bochs_kms.c > +++ b/drivers/gpu/drm/bochs/bochs_kms.c > @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs) > bochs->dev->mode_config.preferred_depth = 24; > bochs->dev->mode_config.prefer_shadow = 0; > bochs->dev->mode_config.prefer_shadow_fbdev = 1; > - bochs->dev->mode_config.fbdev_use_iomem = true; > bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true; > > bochs->dev->mode_config.funcs = _mode_funcs; > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 6212cd7cde1d..462b0c130ebb 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -372,24 +372,22 @@ static void drm_fb_helper_resume_worker(struct > work_struct *work) > } > > static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper, > - struct drm_clip_rect *clip) > + struct drm_clip_rect *clip, > + struct dma_bu
Re: [PATCH v4 09/10] dma-buf-map: Add memcpy and pointer-increment interfaces
Hi Thomas.
On Thu, Oct 15, 2020 at 02:38:05PM +0200, Thomas Zimmermann wrote:
> To do framebuffer updates, one needs memcpy from system memory and a
> pointer-increment function. Add both interfaces with documentation.
>
> Signed-off-by: Thomas Zimmermann
> ---
> include/linux/dma-buf-map.h | 72 +++--
> 1 file changed, 62 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
> index 2e8bbecb5091..6ca0f304dda2 100644
> --- a/include/linux/dma-buf-map.h
> +++ b/include/linux/dma-buf-map.h
> @@ -32,6 +32,14 @@
> * accessing the buffer. Use the returned instance and the helper functions
> * to access the buffer's memory in the correct way.
> *
> + * The type :c:type:`struct dma_buf_map ` and its helpers are
> + * actually independent from the dma-buf infrastructure. When sharing buffers
> + * among devices, drivers have to know the location of the memory to access
> + * the buffers in a safe way. :c:type:`struct dma_buf_map `
> + * solves this problem for dma-buf and its users. If other drivers or
> + * sub-systems require similar functionality, the type could be generalized
> + * and moved to a more prominent header file.
> + *
> * Open-coding access to :c:type:`struct dma_buf_map ` is
> * considered bad style. Rather then accessing its fields directly, use one
> * of the provided helper functions, or implement your own. For example,
> @@ -51,6 +59,14 @@
> *
> * dma_buf_map_set_vaddr_iomem( 0xdeadbeaf);
> *
> + * Instances of struct dma_buf_map do not have to be cleaned up, but
> + * can be cleared to NULL with dma_buf_map_clear(). Cleared mappings
> + * always refer to system memory.
> + *
> + * .. code-block:: c
> + *
> + * dma_buf_map_clear();
> + *
> * Test if a mapping is valid with either dma_buf_map_is_set() or
> * dma_buf_map_is_null().
> *
> @@ -73,17 +89,19 @@
> * if (dma_buf_map_is_equal(_map, _map))
> * // always false
> *
> - * Instances of struct dma_buf_map do not have to be cleaned up, but
> - * can be cleared to NULL with dma_buf_map_clear(). Cleared mappings
> - * always refer to system memory.
> + * A set up instance of struct dma_buf_map can be used to access or
> manipulate
> + * the buffer memory. Depending on the location of the memory, the provided
> + * helpers will pick the correct operations. Data can be copied into the
> memory
> + * with dma_buf_map_memcpy_to(). The address can be manipulated with
> + * dma_buf_map_incr().
> *
> - * The type :c:type:`struct dma_buf_map ` and its helpers are
> - * actually independent from the dma-buf infrastructure. When sharing buffers
> - * among devices, drivers have to know the location of the memory to access
> - * the buffers in a safe way. :c:type:`struct dma_buf_map `
> - * solves this problem for dma-buf and its users. If other drivers or
> - * sub-systems require similar functionality, the type could be generalized
> - * and moved to a more prominent header file.
> + * .. code-block:: c
> + *
> + * const void *src = ...; // source buffer
> + * size_t len = ...; // length of src
> + *
> + * dma_buf_map_memcpy_to(, src, len);
> + * dma_buf_map_incr(, len); // go to first byte after the memcpy
> */
>
> /**
> @@ -210,4 +228,38 @@ static inline void dma_buf_map_clear(struct dma_buf_map
> *map)
> }
> }
>
> +/**
> + * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
> + * @dst: The dma-buf mapping structure
> + * @src: The source buffer
> + * @len: The number of byte in src
> + *
> + * Copies data into a dma-buf mapping. The source buffer is in system
> + * memory. Depending on the buffer's location, the helper picks the correct
> + * method of accessing the memory.
> + */
> +static inline void dma_buf_map_memcpy_to(struct dma_buf_map *dst, const void
> *src, size_t len)
> +{
> + if (dst->is_iomem)
> + memcpy_toio(dst->vaddr_iomem, src, len);
> + else
> + memcpy(dst->vaddr, src, len);
sparc64 needs "#include " to build as is does not get
this via io.h
Sam
> +}
> +
> +/**
> + * dma_buf_map_incr - Increments the address stored in a dma-buf mapping
> + * @map: The dma-buf mapping structure
> + * @incr:The number of bytes to increment
> + *
> + * Increments the address stored in a dma-buf mapping. Depending on the
> + * buffer's location, the correct value will be updated.
> + */
> +static inline void dma_buf_map_incr(struct dma_buf_map *map, size_t incr)
> +{
> + if (map->is_iomem)
> + map->vaddr_iomem += incr;
> + else
> + map->vaddr += incr;
> +}
> +
> #endif /* __DMA_BUF_MAP_H__ */
> --
> 2.28.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v4 10/10] drm/fb_helper: Support framebuffers in I/O memory
Hi Thomas.
On Thu, Oct 15, 2020 at 02:38:06PM +0200, Thomas Zimmermann wrote:
> At least sparc64 requires I/O-specific access to framebuffers. This
> patch updates the fbdev console accordingly.
>
> For drivers with direct access to the framebuffer memory, the callback
> functions in struct fb_ops test for the type of memory and call the rsp
> fb_sys_ of fb_cfb_ functions.
>
> For drivers that employ a shadow buffer, fbdev's blit function retrieves
> the framebuffer address as struct dma_buf_map, and uses dma_buf_map
> interfaces to access the buffer.
>
> The bochs driver on sparc64 uses a workaround to flag the framebuffer as
> I/O memory and avoid a HW exception. With the introduction of struct
> dma_buf_map, this is not required any longer. The patch removes the rsp
> code from both, bochs and fbdev.
>
> v4:
> * move dma_buf_map changes into separate patch (Daniel)
> * TODO list: comment on fbdev updates (Daniel)
I have been offline for a while so have not followed all the threads on
this. So may comments below may well be addressed but I failed to see
it.
If the point about fb_sync is already addressed/considered then:
Reviewed-by: Sam Ravnborg
> Signed-off-by: Thomas Zimmermann
> ---
> Documentation/gpu/todo.rst| 19 ++-
> drivers/gpu/drm/bochs/bochs_kms.c | 1 -
> drivers/gpu/drm/drm_fb_helper.c | 217 --
> include/drm/drm_mode_config.h | 12 --
> 4 files changed, 220 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 7e6fc3c04add..638b7f704339 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup()
>
>
> Most drivers can use drm_fbdev_generic_setup(). Driver have to implement
> -atomic modesetting and GEM vmap support. Current generic fbdev emulation
> -expects the framebuffer in system memory (or system-like memory).
> +atomic modesetting and GEM vmap support. Historically, generic fbdev
> emulation
> +expected the framebuffer in system memory or system-like memory. By employing
> +struct dma_buf_map, drivers with frambuffers in I/O memory can be supported
> +as well.
>
> Contact: Maintainer of the driver you plan to convert
>
> Level: Intermediate
>
> +Reimplement functions in drm_fbdev_fb_ops without fbdev
> +---
> +
> +A number of callback functions in drm_fbdev_fb_ops could benefit from
> +being rewritten without dependencies on the fbdev module. Some of the
> +helpers could further benefit from using struct dma_buf_map instead of
> +raw pointers.
> +
> +Contact: Thomas Zimmermann , Daniel Vetter
> +
> +Level: Advanced
> +
> +
> drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
> -
>
> diff --git a/drivers/gpu/drm/bochs/bochs_kms.c
> b/drivers/gpu/drm/bochs/bochs_kms.c
> index 13d0d04c4457..853081d186d5 100644
> --- a/drivers/gpu/drm/bochs/bochs_kms.c
> +++ b/drivers/gpu/drm/bochs/bochs_kms.c
> @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs)
> bochs->dev->mode_config.preferred_depth = 24;
> bochs->dev->mode_config.prefer_shadow = 0;
> bochs->dev->mode_config.prefer_shadow_fbdev = 1;
> - bochs->dev->mode_config.fbdev_use_iomem = true;
> bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true;
>
> bochs->dev->mode_config.funcs = _mode_funcs;
Good to see this workaround gone again!
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 6212cd7cde1d..462b0c130ebb 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -372,24 +372,22 @@ static void drm_fb_helper_resume_worker(struct
> work_struct *work)
> }
>
> static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper,
> - struct drm_clip_rect *clip)
> + struct drm_clip_rect *clip,
> + struct dma_buf_map *dst)
> {
> struct drm_framebuffer *fb = fb_helper->fb;
> unsigned int cpp = fb->format->cpp[0];
> size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
> void *src = fb_helper->fbdev->screen_buffer + offset;
> - void *dst = fb_helper->buffer->map.vaddr + offset;
> size_t len = (clip->x2 - clip->x1) * cpp;
> unsigned int y;
>
> - fo
Re: [PATCH v4 09/10] dma-buf-map: Add memcpy and pointer-increment interfaces
Hi Thomas.
On Thu, Oct 15, 2020 at 02:38:05PM +0200, Thomas Zimmermann wrote:
> To do framebuffer updates, one needs memcpy from system memory and a
> pointer-increment function. Add both interfaces with documentation.
>
> Signed-off-by: Thomas Zimmermann
Looks good.
Reviewed-by: Sam Ravnborg
> ---
> include/linux/dma-buf-map.h | 72 +++--
> 1 file changed, 62 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
> index 2e8bbecb5091..6ca0f304dda2 100644
> --- a/include/linux/dma-buf-map.h
> +++ b/include/linux/dma-buf-map.h
> @@ -32,6 +32,14 @@
> * accessing the buffer. Use the returned instance and the helper functions
> * to access the buffer's memory in the correct way.
> *
> + * The type :c:type:`struct dma_buf_map ` and its helpers are
> + * actually independent from the dma-buf infrastructure. When sharing buffers
> + * among devices, drivers have to know the location of the memory to access
> + * the buffers in a safe way. :c:type:`struct dma_buf_map `
> + * solves this problem for dma-buf and its users. If other drivers or
> + * sub-systems require similar functionality, the type could be generalized
> + * and moved to a more prominent header file.
> + *
> * Open-coding access to :c:type:`struct dma_buf_map ` is
> * considered bad style. Rather then accessing its fields directly, use one
> * of the provided helper functions, or implement your own. For example,
> @@ -51,6 +59,14 @@
> *
> * dma_buf_map_set_vaddr_iomem( 0xdeadbeaf);
> *
> + * Instances of struct dma_buf_map do not have to be cleaned up, but
> + * can be cleared to NULL with dma_buf_map_clear(). Cleared mappings
> + * always refer to system memory.
> + *
> + * .. code-block:: c
> + *
> + * dma_buf_map_clear();
> + *
> * Test if a mapping is valid with either dma_buf_map_is_set() or
> * dma_buf_map_is_null().
> *
> @@ -73,17 +89,19 @@
> * if (dma_buf_map_is_equal(_map, _map))
> * // always false
> *
> - * Instances of struct dma_buf_map do not have to be cleaned up, but
> - * can be cleared to NULL with dma_buf_map_clear(). Cleared mappings
> - * always refer to system memory.
> + * A set up instance of struct dma_buf_map can be used to access or
> manipulate
> + * the buffer memory. Depending on the location of the memory, the provided
> + * helpers will pick the correct operations. Data can be copied into the
> memory
> + * with dma_buf_map_memcpy_to(). The address can be manipulated with
> + * dma_buf_map_incr().
> *
> - * The type :c:type:`struct dma_buf_map ` and its helpers are
> - * actually independent from the dma-buf infrastructure. When sharing buffers
> - * among devices, drivers have to know the location of the memory to access
> - * the buffers in a safe way. :c:type:`struct dma_buf_map `
> - * solves this problem for dma-buf and its users. If other drivers or
> - * sub-systems require similar functionality, the type could be generalized
> - * and moved to a more prominent header file.
> + * .. code-block:: c
> + *
> + * const void *src = ...; // source buffer
> + * size_t len = ...; // length of src
> + *
> + * dma_buf_map_memcpy_to(, src, len);
> + * dma_buf_map_incr(, len); // go to first byte after the memcpy
> */
>
> /**
> @@ -210,4 +228,38 @@ static inline void dma_buf_map_clear(struct dma_buf_map
> *map)
> }
> }
>
> +/**
> + * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
> + * @dst: The dma-buf mapping structure
> + * @src: The source buffer
> + * @len: The number of byte in src
> + *
> + * Copies data into a dma-buf mapping. The source buffer is in system
> + * memory. Depending on the buffer's location, the helper picks the correct
> + * method of accessing the memory.
> + */
> +static inline void dma_buf_map_memcpy_to(struct dma_buf_map *dst, const void
> *src, size_t len)
> +{
> + if (dst->is_iomem)
> + memcpy_toio(dst->vaddr_iomem, src, len);
> + else
> + memcpy(dst->vaddr, src, len);
> +}
> +
> +/**
> + * dma_buf_map_incr - Increments the address stored in a dma-buf mapping
> + * @map: The dma-buf mapping structure
> + * @incr:The number of bytes to increment
> + *
> + * Increments the address stored in a dma-buf mapping. Depending on the
> + * buffer's location, the correct value will be updated.
> + */
> +static inline void dma_buf_map_incr(struct dma_buf_map *map, size_t incr)
> +{
> + if (map->is_iomem)
> + map->vaddr_iomem += incr;
> + else
> + map->vaddr += incr;
> +}
> +
> #endif /* __DMA_BUF_MAP_H__ */
> --
> 2.28.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v2 20/24] drm/radeon: Backlight update
- Use macros for initialization
- Replace direct access to backlight_properties with get and set
operations
Signed-off-by: Sam Ravnborg
Cc: Alex Deucher
Cc: Christian König
Cc: amd-gfx@lists.freedesktop.org
---
drivers/gpu/drm/radeon/atombios_encoders.c| 24 +--
.../gpu/drm/radeon/radeon_legacy_encoders.c | 16 +
2 files changed, 18 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c
b/drivers/gpu/drm/radeon/atombios_encoders.c
index cc5ee1b3af84..300bec94dbff 100644
--- a/drivers/gpu/drm/radeon/atombios_encoders.c
+++ b/drivers/gpu/drm/radeon/atombios_encoders.c
@@ -145,14 +145,15 @@ atombios_set_backlight_level(struct radeon_encoder
*radeon_encoder, u8 level)
static u8 radeon_atom_bl_level(struct backlight_device *bd)
{
u8 level;
+ int brightness = backlight_get_brightness(bd);
/* Convert brightness to hardware level */
- if (bd->props.brightness < 0)
+ if (brightness < 0)
level = 0;
- else if (bd->props.brightness > RADEON_MAX_BL_LEVEL)
+ else if (brightness > RADEON_MAX_BL_LEVEL)
level = RADEON_MAX_BL_LEVEL;
else
- level = bd->props.brightness;
+ level = brightness;
return level;
}
@@ -185,12 +186,13 @@ static const struct backlight_ops
radeon_atom_backlight_ops = {
void radeon_atom_backlight_init(struct radeon_encoder *radeon_encoder,
struct drm_connector *drm_connector)
{
+ DECLARE_BACKLIGHT_INIT_RAW(props, 0, RADEON_MAX_BL_LEVEL);
struct drm_device *dev = radeon_encoder->base.dev;
struct radeon_device *rdev = dev->dev_private;
struct backlight_device *bd;
- struct backlight_properties props;
struct radeon_backlight_privdata *pdata;
struct radeon_encoder_atom_dig *dig;
+ int brightness;
char bl_name[16];
/* Mac laptops with multiple GPUs use the gmux driver for backlight
@@ -215,9 +217,6 @@ void radeon_atom_backlight_init(struct radeon_encoder
*radeon_encoder,
goto error;
}
- memset(, 0, sizeof(props));
- props.max_brightness = RADEON_MAX_BL_LEVEL;
- props.type = BACKLIGHT_RAW;
snprintf(bl_name, sizeof(bl_name),
"radeon_bl%d", dev->primary->index);
bd = backlight_device_register(bl_name, drm_connector->kdev,
@@ -232,16 +231,17 @@ void radeon_atom_backlight_init(struct radeon_encoder
*radeon_encoder,
dig = radeon_encoder->enc_priv;
dig->bl_dev = bd;
- bd->props.brightness = radeon_atom_backlight_get_brightness(bd);
+ brightness = radeon_atom_backlight_get_brightness(bd);
/* Set a reasonable default here if the level is 0 otherwise
* fbdev will attempt to turn the backlight on after console
* unblanking and it will try and restore 0 which turns the backlight
* off again.
*/
- if (bd->props.brightness == 0)
- bd->props.brightness = RADEON_MAX_BL_LEVEL;
- bd->props.power = FB_BLANK_UNBLANK;
- backlight_update_status(bd);
+
+ if (brightness == 0)
+ brightness = RADEON_MAX_BL_LEVEL;
+
+ backlight_enable_brightness(bd, brightness);
DRM_INFO("radeon atom DIG backlight initialized\n");
rdev->mode_info.bl_encoder = radeon_encoder;
diff --git a/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
b/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
index 44d060f75318..55e656acaedb 100644
--- a/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
+++ b/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
@@ -323,14 +323,15 @@ static uint8_t radeon_legacy_lvds_level(struct
backlight_device *bd)
{
struct radeon_backlight_privdata *pdata = bl_get_data(bd);
uint8_t level;
+ int brightness = backlight_get_brightness(bd);
/* Convert brightness to hardware level */
- if (bd->props.brightness < 0)
+ if (brightness < 0)
level = 0;
- else if (bd->props.brightness > RADEON_MAX_BL_LEVEL)
+ else if (brightness > RADEON_MAX_BL_LEVEL)
level = RADEON_MAX_BL_LEVEL;
else
- level = bd->props.brightness;
+ level = brightness;
if (pdata->negative)
level = RADEON_MAX_BL_LEVEL - level;
@@ -371,10 +372,10 @@ static const struct backlight_ops radeon_backlight_ops = {
void radeon_legacy_backlight_init(struct radeon_encoder *radeon_encoder,
struct drm_connector *drm_connector)
{
+ DECLARE_BACKLIGHT_INIT_RAW(props, 0, RADEON_MAX_BL_LEVEL);
struct drm_device *dev = radeon_encoder->base.dev;
struct radeon_device *rdev = dev->dev_private;
struct backlight_device *bd;
- struct backlight_prope
[PATCH v2 0/24] backlight: add init macros and accessors
The first patch trims backlight_update_status() so it can be called with a NULL
backlight_device. Then the caller do not need to add this check just to avoid
a NULL reference.
The backlight drivers uses several different patterns when registering
a backlight:
- Register backlight and assign properties later
- Define a local backlight_properties variable and use memset
- Define a const backlight_properties and assign relevant properties
On top of this there was differences in what members was assigned.
To align how backlight drivers are initialized introduce following helper
macros:
- DECLARE_BACKLIGHT_INIT_FIRMWARE()
- DECLARE_BACKLIGHT_INIT_PLATFORM()
- DECLARE_BACKLIGHT_INIT_RAW()
The macros are introduced in patch 2.
The backlight drivers used direct access to backlight_properties.
Encapsulate these in get/set access operations resulting in following benefits:
- The access methods can be called with a NULL pointer so logic around the
access can be made simpler.
- The update_brightness and enable_brightness simplifies the users
- The code is in most cases more readable with the access operations.
- When everyone uses the access methods refactoring in the backlight core is
simpler.
The get/set operations are introduced in patch 3.
The gpio backlight driver received a small overhaul in a set of three patches.
The result is a smaller and more readable driver.
The remaining patches updates all backlight users in drivers/gpu/drm/*
With this patch set all of drivers/gpu/drm/:
- All backlight references to FB_BLANK* are gone from drm/*
- All direct references to backlight properties are gone
- All panel drivers uses the devm_ variant for registering backlight
Daniel Vetter had some concerns with this for future updates,
but we are aligned now and can update if refoctoring demands it
- All panel drivers uses the backlight support in drm_panel
Individual patches are only sent to the people listed in the patch + a few more.
Please check https://lore.kernel.org/dri-devel/ for the full series.
v2:
- Documented BACKLIGHT_PROPS as it may be used by drivers
- Dropped backlight_set_power_{on,off}, they were a mistake (Daniel)
- Added backlight_update_brightness() and use it (Daniel)
- Added backlight_enable_brightness() and use it
- Moved remaining drm_panel driver to use backlight support in drm_panel
- gpio backlight driver overhaul
The patches are made on top of the for-backlight-next branch at
https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git
The branch needs v5.8-rc1 backported to build as dev_err_probe()
is used.
The first 6 patches are candidates for the backlight tree.
If they are applied then this should preferably be to an immutable
branch we can merge to drm-misc-next where the drm patches shall go.
The drm patches has known conflics and shall *not* be applied to the
backlight tree, they are included in this patchset to show how the
new functions are used.
Diffstat for the drm bits alone looks nice:
25 files changed, 243 insertions(+), 460 deletions(-)
Feedback welcome!
Sam
Cc: Alex Deucher
Cc: amd-gfx@lists.freedesktop.org
Cc: Andrzej Hajda
Cc: Christian König
Cc: Chris Wilson
Cc: Daniel Thompson
Cc: Ezequiel Garcia
Cc: Hans de Goede
Cc: Hoegeun Kwon
Cc: Inki Dae
Cc: Jani Nikula
Cc: Jernej Skrabec
Cc: Jingoo Han
Cc: Jonas Karlman
Cc: Joonas Lahtinen
Cc: Jyri Sarha
Cc: Kieran Bingham
Cc: Konrad Dybcio
Cc: Laurent Pinchart
Cc: Lee Jones
Cc: Linus Walleij
Cc: linux-renesas-...@vger.kernel.org
Cc: Maarten Lankhorst
Cc: Manasi Navare
Cc: Neil Armstrong
Cc: Patrik Jakobsson
Cc: Paweł Chmiel
Cc: Philippe CORNU
Cc: Rob Clark
Cc: Robert Chiras
Cc: Rodrigo Vivi
Cc: Sam Ravnborg
Cc: Sebastian Reichel
Cc: Thierry Reding
Cc: Tomi Valkeinen
Cc: "Ville Syrjälä"
Cc: Vinay Simha BN
Cc: Wambui Karuga
Cc: Zheng Bin
Sam Ravnborg (24):
backlight: Silently fail backlight_update_status() if no device
backlight: Add DECLARE_* macro for device registration
backlight: Add get/set operations for brightness properties
backlight: gpio: Introduce backlight_{enable,disable}
backlight: gpio: Use dev_err_probe()
backlight: gpio: Use DECLARE_BACKLIGHT_INIT_RAW
drm/gma500: Backlight update
drm/panel: asus-z00t-tm5p5-n35596: Backlight update
drm/panel: jdi-lt070me05000: Backlight update
drm/panel: novatek-nt35510: Backlight update
drm/panel: orisetech-otm8009a: Backlight update
drm/panel: raydium-rm67191: Backlight update
drm/panel: samsung-s6e63m0: Backlight update
drm/panel: samsung-s6e63j0x03: Backlight update
drm/panel: samsung-s6e3ha2: Backlight update
drm/panel: sony-acx424akp: Backlight update
drm/panel: sony-acx565akm: Backlight update
drm/bridge: parade-ps8622: Backlight update
drm/tilcdc: Backlight update
drm/radeon: Backlight update
drm/amdgpu/atom: Backlight update
drm/i915: Backlight update
[PATCH v2 21/24] drm/amdgpu/atom: Backlight update
- Use macros for initialization
- Replace direct access to backlight_properties with get and set
operations
Signed-off-by: Sam Ravnborg
Cc: Alex Deucher
Cc: Christian König
Cc: amd-gfx@lists.freedesktop.org
Cc: Sam Ravnborg
---
.../gpu/drm/amd/amdgpu/atombios_encoders.c| 19 ++-
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
index 1e94a9b652f7..882e1a3dad8b 100644
--- a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
+++ b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
@@ -122,15 +122,16 @@ amdgpu_atombios_encoder_set_backlight_level(struct
amdgpu_encoder *amdgpu_encode
static u8 amdgpu_atombios_encoder_backlight_level(struct backlight_device *bd)
{
+ int brightness = backlight_get_brightness(bd);
u8 level;
/* Convert brightness to hardware level */
- if (bd->props.brightness < 0)
+ if (brightness < 0)
level = 0;
- else if (bd->props.brightness > AMDGPU_MAX_BL_LEVEL)
+ else if (brightness > AMDGPU_MAX_BL_LEVEL)
level = AMDGPU_MAX_BL_LEVEL;
else
- level = bd->props.brightness;
+ level = brightness;
return level;
}
@@ -165,13 +166,12 @@ static const struct backlight_ops
amdgpu_atombios_encoder_backlight_ops = {
void amdgpu_atombios_encoder_init_backlight(struct amdgpu_encoder
*amdgpu_encoder,
struct drm_connector *drm_connector)
{
+ DECLARE_BACKLIGHT_INIT_RAW(props, 0, AMDGPU_MAX_BL_LEVEL);
struct drm_device *dev = amdgpu_encoder->base.dev;
struct amdgpu_device *adev = dev->dev_private;
struct backlight_device *bd;
- struct backlight_properties props;
struct amdgpu_backlight_privdata *pdata;
struct amdgpu_encoder_atom_dig *dig;
- u8 backlight_level;
char bl_name[16];
/* Mac laptops with multiple GPUs use the gmux driver for backlight
@@ -193,9 +193,6 @@ void amdgpu_atombios_encoder_init_backlight(struct
amdgpu_encoder *amdgpu_encode
goto error;
}
- memset(, 0, sizeof(props));
- props.max_brightness = AMDGPU_MAX_BL_LEVEL;
- props.type = BACKLIGHT_RAW;
snprintf(bl_name, sizeof(bl_name),
"amdgpu_bl%d", dev->primary->index);
bd = backlight_device_register(bl_name, drm_connector->kdev,
@@ -207,14 +204,10 @@ void amdgpu_atombios_encoder_init_backlight(struct
amdgpu_encoder *amdgpu_encode
pdata->encoder = amdgpu_encoder;
- backlight_level =
amdgpu_atombios_encoder_get_backlight_level_from_reg(adev);
-
dig = amdgpu_encoder->enc_priv;
dig->bl_dev = bd;
- bd->props.brightness =
amdgpu_atombios_encoder_get_backlight_brightness(bd);
- bd->props.power = FB_BLANK_UNBLANK;
- backlight_update_status(bd);
+ backlight_enable_brightness(bd,
amdgpu_atombios_encoder_get_backlight_brightness(bd));
DRM_INFO("amdgpu atom DIG backlight initialized\n");
--
2.25.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 20/20] drm: Remove obsolete GEM and PRIME callbacks from struct drm_driver
Hi Thomas.
On Thu, Aug 13, 2020 at 10:36:44AM +0200, Thomas Zimmermann wrote:
> Several GEM and PRIME callbacks have been deprecated in favor of
> per-instance GEM object functions. Remove the callbacks as they are
> now unused. The only exception is .gem_prime_mmap, which is still
> in use by several drivers.
>
> What is also gone is gem_vm_ops in struct drm_driver. All drivers now
> use struct drm_gem_object_funcs.vm_ops instead.
>
> While at it, the patch also improves error handling around calls
> to .free and .get_sg_table callbacks.
>
> Signed-off-by: Thomas Zimmermann
After this following entry in todo.rst is done?
"
struct drm_gem_object_funcs
---
GEM objects can now have a function table instead of having the callbacks on the
DRM driver struct. This is now the preferred way and drivers can be moved over.
We also need a 2nd version of the CMA define that doesn't require the
vmapping to be present (different hook for prime importing). Plus this needs to
be rolled out to all drivers using their own implementations, too.
"
If yes, then delete it too.
Sam
> ---
> drivers/gpu/drm/drm_gem.c| 35 +++-
> drivers/gpu/drm/drm_gem_cma_helper.c | 6 +-
> drivers/gpu/drm/drm_prime.c | 17 +++---
> include/drm/drm_drv.h| 85 ++--
> 4 files changed, 23 insertions(+), 120 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 19d73868490e..96945bed8291 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -247,12 +247,9 @@ drm_gem_object_release_handle(int id, void *ptr, void
> *data)
> {
> struct drm_file *file_priv = data;
> struct drm_gem_object *obj = ptr;
> - struct drm_device *dev = obj->dev;
>
> if (obj->funcs && obj->funcs->close)
> obj->funcs->close(obj, file_priv);
> - else if (dev->driver->gem_close_object)
> - dev->driver->gem_close_object(obj, file_priv);
>
> drm_gem_remove_prime_handles(obj, file_priv);
> drm_vma_node_revoke(>vma_node, file_priv);
> @@ -407,10 +404,6 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
> ret = obj->funcs->open(obj, file_priv);
> if (ret)
> goto err_revoke;
> - } else if (dev->driver->gem_open_object) {
> - ret = dev->driver->gem_open_object(obj, file_priv);
> - if (ret)
> - goto err_revoke;
> }
>
> *handlep = handle;
> @@ -982,12 +975,11 @@ drm_gem_object_free(struct kref *kref)
> {
> struct drm_gem_object *obj =
> container_of(kref, struct drm_gem_object, refcount);
> - struct drm_device *dev = obj->dev;
>
> - if (obj->funcs)
> - obj->funcs->free(obj);
> - else if (dev->driver->gem_free_object_unlocked)
> - dev->driver->gem_free_object_unlocked(obj);
> + if (drm_WARN_ON_ONCE(obj->dev, !obj->funcs || !obj->funcs->free))
> + return;
> +
> + obj->funcs->free(obj);
> }
> EXPORT_SYMBOL(drm_gem_object_free);
>
> @@ -1049,9 +1041,9 @@ EXPORT_SYMBOL(drm_gem_vm_close);
> * @obj_size: the object size to be mapped, in bytes
> * @vma: VMA for the area to be mapped
> *
> - * Set up the VMA to prepare mapping of the GEM object using the gem_vm_ops
> - * provided by the driver. Depending on their requirements, drivers can
> either
> - * provide a fault handler in their gem_vm_ops (in which case any accesses to
> + * Set up the VMA to prepare mapping of the GEM object using the GEM object's
> + * vm_ops. Depending on their requirements, GEM objects can either
> + * provide a fault handler in their vm_ops (in which case any accesses to
> * the object will be trapped, to perform migration, GTT binding, surface
> * register allocation, or performance monitoring), or mmap the buffer memory
> * synchronously after calling drm_gem_mmap_obj.
> @@ -1065,12 +1057,11 @@ EXPORT_SYMBOL(drm_gem_vm_close);
> * callers must verify access restrictions before calling this helper.
> *
> * Return 0 or success or -EINVAL if the object size is smaller than the VMA
> - * size, or if no gem_vm_ops are provided.
> + * size, or if no vm_ops are provided.
> */
> int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>struct vm_area_struct *vma)
> {
> - struct drm_device *dev = obj->dev;
> int ret;
>
> /* Check for valid size. */
> @@ -1095,8 +1086,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj,
> unsigned long obj_size,
> } else {
> if (obj->funcs && obj->funcs->vm_ops)
> vma->vm_ops = obj->funcs->vm_ops;
> - else if (dev->driver->gem_vm_ops)
> - vma->vm_ops = dev->driver->gem_vm_ops;
> else {
> drm_gem_object_put(obj);
> return -EINVAL;
> @@ -1206,8
[PATCH v1 19/22] drm/amdgpu/atom: Backlight update
- Use macros for initialization
- Replace direct access to backlight_properties with get and set
operations
Signed-off-by: Sam Ravnborg
Cc: Alex Deucher
Cc: Christian König
Cc: amd-gfx@lists.freedesktop.org
Cc: Sam Ravnborg
---
drivers/gpu/drm/amd/amdgpu/atombios_encoders.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
index 1e94a9b652f7..4338577eb7ba 100644
--- a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
+++ b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
@@ -122,15 +122,16 @@ amdgpu_atombios_encoder_set_backlight_level(struct
amdgpu_encoder *amdgpu_encode
static u8 amdgpu_atombios_encoder_backlight_level(struct backlight_device *bd)
{
+ int brightness = backlight_get_brightness(bd);
u8 level;
/* Convert brightness to hardware level */
- if (bd->props.brightness < 0)
+ if (brightness < 0)
level = 0;
- else if (bd->props.brightness > AMDGPU_MAX_BL_LEVEL)
+ else if (brightness > AMDGPU_MAX_BL_LEVEL)
level = AMDGPU_MAX_BL_LEVEL;
else
- level = bd->props.brightness;
+ level = brightness;
return level;
}
@@ -165,6 +166,7 @@ static const struct backlight_ops
amdgpu_atombios_encoder_backlight_ops = {
void amdgpu_atombios_encoder_init_backlight(struct amdgpu_encoder
*amdgpu_encoder,
struct drm_connector *drm_connector)
{
+ DECLARE_BACKLIGHT_INIT_RAW(props, 0, AMDGPU_MAX_BL_LEVEL);
struct drm_device *dev = amdgpu_encoder->base.dev;
struct amdgpu_device *adev = dev->dev_private;
struct backlight_device *bd;
@@ -193,9 +195,6 @@ void amdgpu_atombios_encoder_init_backlight(struct
amdgpu_encoder *amdgpu_encode
goto error;
}
- memset(, 0, sizeof(props));
- props.max_brightness = AMDGPU_MAX_BL_LEVEL;
- props.type = BACKLIGHT_RAW;
snprintf(bl_name, sizeof(bl_name),
"amdgpu_bl%d", dev->primary->index);
bd = backlight_device_register(bl_name, drm_connector->kdev,
@@ -212,8 +211,8 @@ void amdgpu_atombios_encoder_init_backlight(struct
amdgpu_encoder *amdgpu_encode
dig = amdgpu_encoder->enc_priv;
dig->bl_dev = bd;
- bd->props.brightness =
amdgpu_atombios_encoder_get_backlight_brightness(bd);
- bd->props.power = FB_BLANK_UNBLANK;
+ backlight_set_brightness(bd,
amdgpu_atombios_encoder_get_backlight_brightness(bd));
+ backlight_set_power_on(bd);
backlight_update_status(bd);
DRM_INFO("amdgpu atom DIG backlight initialized\n");
--
2.25.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v1 18/22] drm/radeon: Backlight update
- Use macros for initialization
- Replace direct access to backlight_properties with get and set
operations
Signed-off-by: Sam Ravnborg
Cc: Alex Deucher
Cc: Christian König
Cc: amd-gfx@lists.freedesktop.org
---
drivers/gpu/drm/radeon/atombios_encoders.c| 23 ++-
.../gpu/drm/radeon/radeon_legacy_encoders.c | 15 ++--
2 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c
b/drivers/gpu/drm/radeon/atombios_encoders.c
index cc5ee1b3af84..c9431af12fed 100644
--- a/drivers/gpu/drm/radeon/atombios_encoders.c
+++ b/drivers/gpu/drm/radeon/atombios_encoders.c
@@ -145,14 +145,15 @@ atombios_set_backlight_level(struct radeon_encoder
*radeon_encoder, u8 level)
static u8 radeon_atom_bl_level(struct backlight_device *bd)
{
u8 level;
+ int brightness = backlight_get_brightness(bd);
/* Convert brightness to hardware level */
- if (bd->props.brightness < 0)
+ if (brightness < 0)
level = 0;
- else if (bd->props.brightness > RADEON_MAX_BL_LEVEL)
+ else if (brightness > RADEON_MAX_BL_LEVEL)
level = RADEON_MAX_BL_LEVEL;
else
- level = bd->props.brightness;
+ level = brightness;
return level;
}
@@ -185,12 +186,13 @@ static const struct backlight_ops
radeon_atom_backlight_ops = {
void radeon_atom_backlight_init(struct radeon_encoder *radeon_encoder,
struct drm_connector *drm_connector)
{
+ DECLARE_BACKLIGHT_INIT_RAW(props, 0, RADEON_MAX_BL_LEVEL);
struct drm_device *dev = radeon_encoder->base.dev;
struct radeon_device *rdev = dev->dev_private;
struct backlight_device *bd;
- struct backlight_properties props;
struct radeon_backlight_privdata *pdata;
struct radeon_encoder_atom_dig *dig;
+ int brightness;
char bl_name[16];
/* Mac laptops with multiple GPUs use the gmux driver for backlight
@@ -215,9 +217,6 @@ void radeon_atom_backlight_init(struct radeon_encoder
*radeon_encoder,
goto error;
}
- memset(, 0, sizeof(props));
- props.max_brightness = RADEON_MAX_BL_LEVEL;
- props.type = BACKLIGHT_RAW;
snprintf(bl_name, sizeof(bl_name),
"radeon_bl%d", dev->primary->index);
bd = backlight_device_register(bl_name, drm_connector->kdev,
@@ -232,15 +231,17 @@ void radeon_atom_backlight_init(struct radeon_encoder
*radeon_encoder,
dig = radeon_encoder->enc_priv;
dig->bl_dev = bd;
- bd->props.brightness = radeon_atom_backlight_get_brightness(bd);
+ brightness = radeon_atom_backlight_get_brightness(bd);
/* Set a reasonable default here if the level is 0 otherwise
* fbdev will attempt to turn the backlight on after console
* unblanking and it will try and restore 0 which turns the backlight
* off again.
*/
- if (bd->props.brightness == 0)
- bd->props.brightness = RADEON_MAX_BL_LEVEL;
- bd->props.power = FB_BLANK_UNBLANK;
+
+ if (brightness == 0)
+ brightness = RADEON_MAX_BL_LEVEL;
+ backlight_set_brightness(bd, brightness);
+ backlight_set_power_on(bd);
backlight_update_status(bd);
DRM_INFO("radeon atom DIG backlight initialized\n");
diff --git a/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
b/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
index 44d060f75318..cf2d1776b975 100644
--- a/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
+++ b/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
@@ -323,14 +323,15 @@ static uint8_t radeon_legacy_lvds_level(struct
backlight_device *bd)
{
struct radeon_backlight_privdata *pdata = bl_get_data(bd);
uint8_t level;
+ int brightness = backlight_get_brightness(bd);
/* Convert brightness to hardware level */
- if (bd->props.brightness < 0)
+ if (brightness < 0)
level = 0;
- else if (bd->props.brightness > RADEON_MAX_BL_LEVEL)
+ else if (brightness > RADEON_MAX_BL_LEVEL)
level = RADEON_MAX_BL_LEVEL;
else
- level = bd->props.brightness;
+ level = brightness;
if (pdata->negative)
level = RADEON_MAX_BL_LEVEL - level;
@@ -371,6 +372,7 @@ static const struct backlight_ops radeon_backlight_ops = {
void radeon_legacy_backlight_init(struct radeon_encoder *radeon_encoder,
struct drm_connector *drm_connector)
{
+ DECLARE_BACKLIGHT_INIT_RAW(props, 0, RADEON_MAX_BL_LEVEL);
struct drm_device *dev = radeon_encoder->base.dev;
struct radeon_device *rdev = dev->dev_private;
struct backlight_device *bd;
@@ -394,9 +396,6 @@ void radeon_legacy_backlight_init(struct r
[RFC PATCH v1 0/22] backlight: add init macros and accessors
The backlight drivers uses several different patterns when registering a backlight: - Register backlight and assign properties later - Define a local backlight_properties variable and use memset - Define a const backlight_properties and assign relevant properties On top of this there was differences in what members was assigned in backlight_properties. To align how backlight drivers are initialized introduce following helper macros: - DECLARE_BACKLIGHT_INIT_FIRMWARE() - DECLARE_BACKLIGHT_INIT_PLATFORM() - DECLARE_BACKLIGHT_INIT_RAW() The macros are introduced in patch 2. The backlight drivers used direct access to backlight_properties. Encapsulate these in get/set access operations resulting in following benefits: - The drivers no longer need to be concerned about the confusing power states, as there is now only a set_power_on() and set_power_off() operation. - The access methods can be called with a NULL pointer so logic around the access can be made simpler. - The code is in most cases more readable with the access operations. - When everyone uses the access methods refactroring in the backlight core is simpler. The get/set operations are introduced in patch 3. The first patch trims backlight_update_status() so it can be called with a NULL backlight_device. Then the called do not need to add this check just to avoid a NULL reference. The fourth patch introduce the new macros and get/set operations for the gpio backlight driver, as an example. The remaining patches updates most backlight users in drivers/gpu/drm/* With this patch set applied then: - Almost all references to FB_BLANK* are gone from drm/* - All panel drivers uses devm_ variant for registering backlight - Almost all direct references to backlight properties are gone The drm/* patches are used as examples how drivers can benefit from the new macros and get/set operations. Individual patches are only sent to the people listed in the patch + a few more. Please check https://lore.kernel.org/dri-devel/ for the full series. Feedback welcome! Sam Cc: Alex Deucher Cc: amd-gfx@lists.freedesktop.org Cc: Andrzej Hajda Cc: Christian König Cc: Chris Wilson Cc: Daniel Thompson Cc: Ezequiel Garcia Cc: Hans de Goede Cc: Hoegeun Kwon Cc: Inki Dae Cc: Jani Nikula Cc: Jernej Skrabec Cc: Jingoo Han Cc: Jonas Karlman Cc: Joonas Lahtinen Cc: Jyri Sarha Cc: Kieran Bingham Cc: Konrad Dybcio Cc: Laurent Pinchart Cc: Lee Jones Cc: Linus Walleij Cc: linux-renesas-...@vger.kernel.org Cc: Maarten Lankhorst Cc: Manasi Navare Cc: Neil Armstrong Cc: Patrik Jakobsson Cc: Paweł Chmiel Cc: Philippe CORNU Cc: Rob Clark Cc: Robert Chiras Cc: Rodrigo Vivi Cc: Sam Ravnborg Cc: Sebastian Reichel Cc: Thierry Reding Cc: Tomi Valkeinen Cc: "Ville Syrjälä" Cc: Vinay Simha BN Cc: Wambui Karuga Cc: Zheng Bin Sam Ravnborg (22): backlight: Silently fail backlight_update_status() if no device backlight: Add DECLARE_* macro for device registration backlight: Add get/set operations for brightness/power properties backlight: gpio: Use DECLARE_BACKLIGHT_INIT_RAW and get/setters drm/gma500: Backlight support drm/panel: asus-z00t-tm5p5-n35596: Backlight update drm/panel: jdi-lt070me05000: Backlight update drm/panel: novatek-nt35510: Backlight update drm/panel: orisetech-otm8009a: Backlight update drm/panel: raydium-rm67191: Backlight update drm/panel: samsung-s6e63m0: Backlight update drm/panel: samsung-s6e63j0x03: Backlight update drm/panel: samsung-s6e3ha2: Backlight update drm/panel: sony-acx424akp: Backlight update drm/panel: sony-acx565akm: Backlight update drm/bridge: parade-ps8622: Backlight update drm/tilcdc: Backlight update drm/radeon: Backlight update drm/amdgpu/atom: Backlight update drm/i915: Backlight update drm/omap: display: Backlight update drm/shmobile: Backlight update drivers/gpu/drm/amd/amdgpu/atombios_encoders.c | 15 ++- drivers/gpu/drm/bridge/parade-ps8622.c | 43 drivers/gpu/drm/gma500/backlight.c | 35 ++ drivers/gpu/drm/gma500/cdv_device.c| 29 +++-- drivers/gpu/drm/gma500/mdfld_device.c | 9 +- drivers/gpu/drm/gma500/oaktrail_device.c | 10 +- drivers/gpu/drm/gma500/opregion.c | 2 +- drivers/gpu/drm/gma500/psb_device.c| 10 +- drivers/gpu/drm/gma500/psb_drv.c | 8 +- drivers/gpu/drm/i915/display/intel_panel.c | 88 +++ drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c| 35 ++ .../gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c | 15 +-- drivers/gpu/drm/panel/panel-jdi-lt070me05000.c | 17 ++- drivers/gpu/drm/panel/panel-novatek-nt35510.c | 9 +- drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 14 +-- drivers/gpu/drm/panel/panel-raydium-rm67191.c | 11 +- drive
Re: [PATCH 06/36] drm/amdgpu: use the unlocked drm_gem_object_put
On Fri, May 08, 2020 at 10:55:42AM +0100, Emil Velikov wrote:
> On Fri, 8 May 2020 at 09:16, Christian König wrote:
> >
> > Am 07.05.20 um 20:03 schrieb Sam Ravnborg:
> > > Hi Emil.
> > >
> > > On Thu, May 07, 2020 at 04:07:52PM +0100, Emil Velikov wrote:
> > >> From: Emil Velikov
> > >>
> > >> The driver does not hold struct_mutex, thus using the locked version of
> > >> the helper is incorrect.
> > >>
> > >> Cc: Alex Deucher
> > >> Cc: Christian König
> > >> Cc: amd-gfx@lists.freedesktop.org
> > >> Fixes: a39414716ca0 ("drm/amdgpu: add independent DMA-buf import v9"):
> > >> Signed-off-by: Emil Velikov
> > >> ---
> > >> drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 2 +-
> > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> > >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> > >> index 43d8ed7dbd00..652c57a3b847 100644
> > >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> > >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> > >> @@ -587,7 +587,7 @@ struct drm_gem_object
> > >> *amdgpu_gem_prime_import(struct drm_device *dev,
> > >> attach = dma_buf_dynamic_attach(dma_buf, dev->dev,
> > >> _dma_buf_attach_ops, obj);
> > >> if (IS_ERR(attach)) {
> > >> -drm_gem_object_put(obj);
> > >> +drm_gem_object_put_unlocked(obj);
> > >> return ERR_CAST(attach);
> > >> }
> > > Likewise previous patch.
> > > Drop this as the patch is correct after the rename a few pathces later.
> >
> > Well this is a bug fix in the error path and should probably be back
> > ported, so having a separate patch is certainly a good idea.
> >
> Precisely the goal here. The fixes tag should allow Greg and team to
> pick/port it where applicable.
I got it now... Thanks for spelling it out for my dense mind.
Sam
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 06/36] drm/amdgpu: use the unlocked drm_gem_object_put
Hi Emil.
On Thu, May 07, 2020 at 04:07:52PM +0100, Emil Velikov wrote:
> From: Emil Velikov
>
> The driver does not hold struct_mutex, thus using the locked version of
> the helper is incorrect.
>
> Cc: Alex Deucher
> Cc: Christian König
> Cc: amd-gfx@lists.freedesktop.org
> Fixes: a39414716ca0 ("drm/amdgpu: add independent DMA-buf import v9"):
> Signed-off-by: Emil Velikov
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> index 43d8ed7dbd00..652c57a3b847 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> @@ -587,7 +587,7 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct
> drm_device *dev,
> attach = dma_buf_dynamic_attach(dma_buf, dev->dev,
> _dma_buf_attach_ops, obj);
> if (IS_ERR(attach)) {
> - drm_gem_object_put(obj);
> + drm_gem_object_put_unlocked(obj);
> return ERR_CAST(attach);
> }
Likewise previous patch.
Drop this as the patch is correct after the rename a few pathces later.
Sam
>
> --
> 2.25.1
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 0/6] gpu: convert to use new I2C API
On Thu, Mar 26, 2020 at 10:09:58PM +0100, Wolfram Sang wrote: > We are deprecating calls which return NULL in favor of new variants which > return an ERR_PTR. Only build tested. > > > Wolfram Sang (6): > drm/amdgpu: convert to use i2c_new_client_device() > drm/gma500: convert to use i2c_new_client_device() > drm/i2c/sil164: convert to use i2c_new_client_device() > drm/i2c/tda998x: convert to use i2c_new_client_device() > drm/nouveau/therm: convert to use i2c_new_client_device() > drm/radeon: convert to use i2c_new_client_device() With the ack from Alex I went ahead and applied the patches to drm-misc-next. Sam > > drivers/gpu/drm/amd/amdgpu/amdgpu_dpm.c| 2 +- > drivers/gpu/drm/gma500/tc35876x-dsi-lvds.c | 8 > drivers/gpu/drm/i2c/sil164_drv.c | 7 +-- > drivers/gpu/drm/i2c/tda998x_drv.c | 6 +++--- > drivers/gpu/drm/nouveau/nvkm/subdev/therm/ic.c | 4 ++-- > drivers/gpu/drm/radeon/radeon_atombios.c | 4 ++-- > drivers/gpu/drm/radeon/radeon_combios.c| 4 ++-- > 7 files changed, 19 insertions(+), 16 deletions(-) > > -- > 2.20.1 > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 6/6] drm/radeon: convert to use i2c_new_client_device()
On Fri, Mar 27, 2020 at 04:45:09PM +0100, Wolfram Sang wrote: > > > > > > Move away from the deprecated API. > > > > > > > > > > Signed-off-by: Wolfram Sang > > > > > > > > patches 1,6, are: > > > > Acked-by: Alex Deucher > > > Should we commit all to drm-misc-next? > > > > I'm fine to see it go through whatever tree makes sense. > > I'd suggest drm-misc-next to minimize merge conflicts. But I can take it > via I2C tree, too, if desired. If no-one else speaks up until tomorrow I will land them in drm-misc-next. Just wanted to make sure it was OK. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 6/6] drm/radeon: convert to use i2c_new_client_device()
On Fri, Mar 27, 2020 at 10:25:16AM -0400, Alex Deucher wrote:
> On Thu, Mar 26, 2020 at 5:35 PM Wolfram Sang
> wrote:
> >
> > Move away from the deprecated API.
> >
> > Signed-off-by: Wolfram Sang
>
> patches 1,6, are:
> Acked-by: Alex Deucher
Should we commit all to drm-misc-next?
Sam
>
> > ---
> > drivers/gpu/drm/radeon/radeon_atombios.c | 4 ++--
> > drivers/gpu/drm/radeon/radeon_combios.c | 4 ++--
> > 2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c
> > b/drivers/gpu/drm/radeon/radeon_atombios.c
> > index 848ef68d9086..5d2591725189 100644
> > --- a/drivers/gpu/drm/radeon/radeon_atombios.c
> > +++ b/drivers/gpu/drm/radeon/radeon_atombios.c
> > @@ -2111,7 +2111,7 @@ static int
> > radeon_atombios_parse_power_table_1_3(struct radeon_device *rdev)
> >
> > ucOverdriveThermalController];
> > info.addr =
> > power_info->info.ucOverdriveControllerAddress >> 1;
> > strlcpy(info.type, name, sizeof(info.type));
> > - i2c_new_device(>pm.i2c_bus->adapter, );
> > + i2c_new_client_device(>pm.i2c_bus->adapter,
> > );
> > }
> > }
> > num_modes = power_info->info.ucNumOfPowerModeEntries;
> > @@ -2351,7 +2351,7 @@ static void
> > radeon_atombios_add_pplib_thermal_controller(struct radeon_device *r
> > const char *name =
> > pp_lib_thermal_controller_names[controller->ucType];
> > info.addr = controller->ucI2cAddress >> 1;
> > strlcpy(info.type, name, sizeof(info.type));
> > - i2c_new_device(>pm.i2c_bus->adapter,
> > );
> > +
> > i2c_new_client_device(>pm.i2c_bus->adapter, );
> > }
> > } else {
> > DRM_INFO("Unknown thermal controller type %d at
> > 0x%02x %s fan control\n",
> > diff --git a/drivers/gpu/drm/radeon/radeon_combios.c
> > b/drivers/gpu/drm/radeon/radeon_combios.c
> > index c3e49c973812..d3c04df7e75d 100644
> > --- a/drivers/gpu/drm/radeon/radeon_combios.c
> > +++ b/drivers/gpu/drm/radeon/radeon_combios.c
> > @@ -2704,7 +2704,7 @@ void radeon_combios_get_power_modes(struct
> > radeon_device *rdev)
> > const char *name =
> > thermal_controller_names[thermal_controller];
> > info.addr = i2c_addr >> 1;
> > strlcpy(info.type, name, sizeof(info.type));
> > - i2c_new_device(>pm.i2c_bus->adapter,
> > );
> > +
> > i2c_new_client_device(>pm.i2c_bus->adapter, );
> > }
> > }
> > } else {
> > @@ -2721,7 +2721,7 @@ void radeon_combios_get_power_modes(struct
> > radeon_device *rdev)
> > const char *name = "f75375";
> > info.addr = 0x28;
> > strlcpy(info.type, name, sizeof(info.type));
> > - i2c_new_device(>pm.i2c_bus->adapter,
> > );
> > +
> > i2c_new_client_device(>pm.i2c_bus->adapter, );
> > DRM_INFO("Possible %s thermal controller at
> > 0x%02x\n",
> > name, info.addr);
> > }
> > --
> > 2.20.1
> >
> > ___
> > dri-devel mailing list
> > dri-de...@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v1 0/3] drm: drm_encoder_init() => drm_encoder_init_funcs()
On Thu, Mar 19, 2020 at 03:19:54PM +0100, Sam Ravnborg wrote: > On Fri, Mar 13, 2020 at 09:17:41PM +0100, Sam Ravnborg wrote: > > Thomas Zimmermann had made a nice patch-set that introduced > > drm_simple_encoder_init() which is already present in drm-misc-next. > > > > While looking at this it was suddenly obvious to me that > > this was functionalty that really should be included in drm_encoder.c > > The case where the core could handle the callback is pretty > > common and not part of the simple pipe line. > > > > So after some dialog on dri-devel the conclusion was to go for > > a change like this: > > > > drm_encoder_init_funcs() for all users that specified a > > drm_encoder_funcs to extend the functionality. > > > > drm_encoder_init() for all users that did not > > need to extend the basic functionality with > > drm_encoder_funcs. > > > > A similar approach with a _funcs() prefix is used elsewhere in drm/ > > > > This required a rename of the existing users, and > > a follow-up patch that moves drm_simple_encoder_init() > > to drm_encoder.c > > > > Patches 3 in this set demonstrate the use of drm_encoder_init(). > > There are many more drivers that can be converted as Thomas > > has already demonstrated. > > > > This is all based on work done by Thomas Zimmermann, > > I just wanted to implement my suggestion so > > we could select the best way forward. > > > > Note: Daniel Vetter has hinted the approach implemented > > here smelled like middle-layer. > > IMO this is not so, it is just a way to handle cleanup > > for the simple cases. > > We discussed this patch-set briefly on irc. Just to clarify, we in this context was Daniel Vetter & me. Sam > With the upcoming drmm_ changes and such this is bad timing.. > And in the end this may be pointless code-chrunch. > > Patch-set shelfed for now - may re-visit it later. > > Sam > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v1 0/3] drm: drm_encoder_init() => drm_encoder_init_funcs()
On Fri, Mar 13, 2020 at 09:17:41PM +0100, Sam Ravnborg wrote: > Thomas Zimmermann had made a nice patch-set that introduced > drm_simple_encoder_init() which is already present in drm-misc-next. > > While looking at this it was suddenly obvious to me that > this was functionalty that really should be included in drm_encoder.c > The case where the core could handle the callback is pretty > common and not part of the simple pipe line. > > So after some dialog on dri-devel the conclusion was to go for > a change like this: > > drm_encoder_init_funcs() for all users that specified a > drm_encoder_funcs to extend the functionality. > > drm_encoder_init() for all users that did not > need to extend the basic functionality with > drm_encoder_funcs. > > A similar approach with a _funcs() prefix is used elsewhere in drm/ > > This required a rename of the existing users, and > a follow-up patch that moves drm_simple_encoder_init() > to drm_encoder.c > > Patches 3 in this set demonstrate the use of drm_encoder_init(). > There are many more drivers that can be converted as Thomas > has already demonstrated. > > This is all based on work done by Thomas Zimmermann, > I just wanted to implement my suggestion so > we could select the best way forward. > > Note: Daniel Vetter has hinted the approach implemented > here smelled like middle-layer. > IMO this is not so, it is just a way to handle cleanup > for the simple cases. We discussed this patch-set briefly on irc. With the upcoming drmm_ changes and such this is bad timing.. And in the end this may be pointless code-chrunch. Patch-set shelfed for now - may re-visit it later. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v1 0/3] drm: drm_encoder_init() => drm_encoder_init_funcs()
Thomas Zimmermann had made a nice patch-set that introduced drm_simple_encoder_init() which is already present in drm-misc-next. While looking at this it was suddenly obvious to me that this was functionalty that really should be included in drm_encoder.c The case where the core could handle the callback is pretty common and not part of the simple pipe line. So after some dialog on dri-devel the conclusion was to go for a change like this: drm_encoder_init_funcs() for all users that specified a drm_encoder_funcs to extend the functionality. drm_encoder_init() for all users that did not need to extend the basic functionality with drm_encoder_funcs. A similar approach with a _funcs() prefix is used elsewhere in drm/ This required a rename of the existing users, and a follow-up patch that moves drm_simple_encoder_init() to drm_encoder.c Patches 3 in this set demonstrate the use of drm_encoder_init(). There are many more drivers that can be converted as Thomas has already demonstrated. This is all based on work done by Thomas Zimmermann, I just wanted to implement my suggestion so we could select the best way forward. Note: Daniel Vetter has hinted the approach implemented here smelled like middle-layer. IMO this is not so, it is just a way to handle cleanup for the simple cases. Sam Sam Ravnborg (3): drm: drm_encoder_init() => drm_encoder_init_funcs() drm: drm_simple_encoder_init() => drm_encoder_init() drm/atmel-hlcdc: Use drm_encoder_init() drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 28 ++--- drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 28 ++--- drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 28 ++--- drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 28 ++--- drivers/gpu/drm/amd/amdgpu/dce_virtual.c | 4 +- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++--- .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c| 10 ++--- drivers/gpu/drm/arc/arcpgu_hdmi.c | 4 +- drivers/gpu/drm/arc/arcpgu_sim.c | 4 +- drivers/gpu/drm/ast/ast_mode.c | 3 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 8 +--- drivers/gpu/drm/drm_encoder.c | 49 +++--- drivers/gpu/drm/drm_encoder_slave.c| 2 +- drivers/gpu/drm/drm_simple_kms_helper.c| 45 +--- drivers/gpu/drm/drm_writeback.c| 6 +-- drivers/gpu/drm/exynos/exynos_dp.c | 4 +- drivers/gpu/drm/exynos/exynos_drm_dpi.c| 4 +- drivers/gpu/drm/exynos/exynos_drm_dsi.c| 4 +- drivers/gpu/drm/exynos/exynos_drm_vidi.c | 4 +- drivers/gpu/drm/exynos/exynos_hdmi.c | 4 +- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 4 +- drivers/gpu/drm/gma500/cdv_intel_crt.c | 5 ++- drivers/gpu/drm/gma500/cdv_intel_dp.c | 4 +- drivers/gpu/drm/gma500/cdv_intel_hdmi.c| 4 +- drivers/gpu/drm/gma500/cdv_intel_lvds.c| 6 +-- drivers/gpu/drm/gma500/mdfld_dsi_dpi.c | 7 ++-- drivers/gpu/drm/gma500/oaktrail_hdmi.c | 6 +-- drivers/gpu/drm/gma500/oaktrail_lvds.c | 4 +- drivers/gpu/drm/gma500/psb_intel_lvds.c| 6 +-- drivers/gpu/drm/gma500/psb_intel_sdvo.c| 4 +- drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 4 +- drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 4 +- drivers/gpu/drm/i2c/tda998x_drv.c | 5 ++- drivers/gpu/drm/i915/display/icl_dsi.c | 4 +- drivers/gpu/drm/i915/display/intel_crt.c | 5 ++- drivers/gpu/drm/i915/display/intel_ddi.c | 6 ++- drivers/gpu/drm/i915/display/intel_dp.c| 6 +-- drivers/gpu/drm/i915/display/intel_dp_mst.c| 6 ++- drivers/gpu/drm/i915/display/intel_dvo.c | 6 +-- drivers/gpu/drm/i915/display/intel_hdmi.c | 6 +-- drivers/gpu/drm/i915/display/intel_lvds.c | 4 +- drivers/gpu/drm/i915/display/intel_sdvo.c | 6 +-- drivers/gpu/drm/i915/display/intel_tv.c| 4 +- drivers/gpu/drm/i915/display/vlv_dsi.c | 5 ++- drivers/gpu/drm/imx/dw_hdmi-imx.c | 4 +- drivers/gpu/drm/imx/imx-ldb.c | 4 +- drivers/gpu/drm/imx/imx-tve.c | 4 +- drivers/gpu/drm/imx/parallel-display.c | 4 +- drivers/gpu/drm/ingenic/ingenic-drm.c | 5 ++- drivers/gpu/drm/mediatek/mtk_dpi.c | 5 ++- drivers/gpu/drm/mediatek/mtk_dsi.c | 4 +- drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 ++- drivers/gpu/drm/meson/meson_venc_cvbs.c| 5 ++- drivers/gpu/drm/mgag200/mgag200_mode.c | 7 +--- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 4 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_dsi_encoder.c
[PATCH v1 2/3] drm: drm_simple_encoder_init() => drm_encoder_init()
A lot of drivers requires only a basic encoder with no need
to extend the functionality.
This was previously implemented in drm_simple_kms_helper.c
but encoders are not necessarily simple despite no
need for a drm_encoder_funcs for adding functionality.
Move the init function to drm_encoder.c to reflect this.
And adjust the name to drm_encoder_init().
Drop include of drm_simple_kms_helper.h in the touched
drivers as it is no logner required.
Signed-off-by: Sam Ravnborg
Cc: Dave Airlie
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: Thomas Zimmermann
Cc: Laurent Pinchart
Cc: David Airlie
Cc: Daniel Vetter
Cc: Gerd Hoffmann
Cc: Sam Ravnborg
Cc: Emil Velikov
Cc: Andrzej Pietrasiewicz
Cc: "José Roberto de Souza"
---
drivers/gpu/drm/ast/ast_mode.c | 3 +-
drivers/gpu/drm/drm_encoder.c | 37
drivers/gpu/drm/drm_simple_kms_helper.c | 45 +
drivers/gpu/drm/mgag200/mgag200_mode.c | 7 ++--
drivers/gpu/drm/qxl/qxl_display.c | 7 ++--
include/drm/drm_encoder.h | 3 ++
include/drm/drm_simple_kms_helper.h | 4 ---
7 files changed, 46 insertions(+), 60 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index cdd6c46d6557..4f6ace1afaf0 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -40,7 +40,6 @@
#include
#include
#include
-#include
#include "ast_drv.h"
#include "ast_tables.h"
@@ -964,7 +963,7 @@ static int ast_encoder_init(struct drm_device *dev)
struct drm_encoder *encoder = >encoder;
int ret;
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
+ ret = drm_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index a76a5f04ab39..e1e90652094c 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -152,6 +152,43 @@ int drm_encoder_init_funcs(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_encoder_init_funcs);
+static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
+ .destroy = drm_encoder_cleanup,
+};
+
+/**
+ * drm_simple_init - Initialize a preallocated encoder with basic
functionality.
+ * @dev: drm device
+ * @encoder: the encoder to initialize
+ * @encoder_type: user visible type of the encoder
+ *
+ * Initialises a preallocated encoder that has no further functionality.
+ * Settings for possible CRTC and clones are left to their initial values.
+ * The encoder will be cleaned up automatically as part of the mode-setting
+ * cleanup.
+ *
+ * The caller of drm_encoder_init() is responsible for freeing
+ * the encoder's memory after the encoder has been cleaned up. At the
+ * moment this only works reliably if the encoder data structure is
+ * stored in the device structure. Free the encoder's memory as part of
+ * the device release function.
+ *
+ * FIXME: Later improvements to DRM's resource management may allow for
+ *an automated kfree() of the encoder's memory.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_simple_init(struct drm_device *dev,
+ struct drm_encoder *encoder,
+ int encoder_type)
+{
+ return drm_encoder_init_funcs(dev, encoder,
+ _simple_encoder_funcs_cleanup,
+ encoder_type, NULL);
+}
+EXPORT_SYMBOL(drm_encoder_init);
+
/**
* drm_encoder_cleanup - cleans up an initialised encoder
* @encoder: encoder to cleanup
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
b/drivers/gpu/drm/drm_simple_kms_helper.c
index 24d4433c347b..d70170980839 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -26,51 +26,8 @@
* entity. Some flexibility for code reuse is provided through a separately
* allocated _connector object and supporting optional _bridge
* encoder drivers.
- *
- * Many drivers require only a very simple encoder that fulfills the minimum
- * requirements of the display pipeline and does not add additional
- * functionality. The function drm_simple_encoder_init() provides an
- * implementation of such an encoder.
*/
-static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
- .destroy = drm_encoder_cleanup,
-};
-
-/**
- * drm_simple_encoder_init - Initialize a preallocated encoder with
- * basic functionality.
- * @dev: drm device
- * @encoder: the encoder to initialize
- * @encoder_type: user visible type of the encoder
- *
- * Initialises a preallocated encoder that has no further functionality.
- * Settings for possible CRTC and clones are left to their initial values.
- * The encoder will be cleaned up automatically as part of the mode-setting
- * cleanup.
- *
- * The caller of drm_simple_encoder_
[PATCH v1 1/3] drm: drm_encoder_init() => drm_encoder_init_funcs()
Many callers of drm_encoder_init use a drm_encoder_funcs that contains only a drm_encoder_cleanup() callback. drm_simple_encoder_init() was introduced to cover the common case where an encoder with only basic functionality was needed. But it really do not belong in drm_simple_* Rename drm_encoder_init() to drm_encoder_funcs(), so we can then in a subsequent patch rename drm_simple_encoder_init() to drm_encoder_init(). And then move the functionality to drm_encoder where it bleongs. checkpatch complains about long lines in amd and radeon. Surrounding lines are too long too, so warnings were ignored. Signed-off-by: Sam Ravnborg Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: David Airlie Cc: Daniel Vetter Cc: NXP Linux Team Cc: Laurent Pinchart Cc: linux-arm-ker...@lists.infradead.org Cc: linux-samsung-...@vger.kernel.org Cc: linux-media...@lists.infradead.org Cc: linux-amlo...@lists.infradead.org Cc: amd-gfx@lists.freedesktop.org Cc: linux-renesas-...@vger.kernel.org Cc: linux-rockc...@lists.infradead.org Cc: linux-st...@st-md-mailman.stormreply.com Cc: linux-te...@vger.kernel.org Cc: virtualizat...@lists.linux-foundation.org --- drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 28 ++--- drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 28 ++--- drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 28 ++--- drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 28 ++--- drivers/gpu/drm/amd/amdgpu/dce_virtual.c | 4 +- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++--- .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 10 ++--- drivers/gpu/drm/arc/arcpgu_hdmi.c | 4 +- drivers/gpu/drm/arc/arcpgu_sim.c | 4 +- .../gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 6 +-- drivers/gpu/drm/drm_encoder.c | 14 +++ drivers/gpu/drm/drm_encoder_slave.c | 2 +- drivers/gpu/drm/drm_simple_kms_helper.c | 6 +-- drivers/gpu/drm/drm_writeback.c | 6 +-- drivers/gpu/drm/exynos/exynos_dp.c| 4 +- drivers/gpu/drm/exynos/exynos_drm_dpi.c | 4 +- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 4 +- drivers/gpu/drm/exynos/exynos_drm_vidi.c | 4 +- drivers/gpu/drm/exynos/exynos_hdmi.c | 4 +- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c | 4 +- drivers/gpu/drm/gma500/cdv_intel_crt.c| 5 ++- drivers/gpu/drm/gma500/cdv_intel_dp.c | 4 +- drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 4 +- drivers/gpu/drm/gma500/cdv_intel_lvds.c | 6 +-- drivers/gpu/drm/gma500/mdfld_dsi_dpi.c| 7 ++-- drivers/gpu/drm/gma500/oaktrail_hdmi.c| 6 +-- drivers/gpu/drm/gma500/oaktrail_lvds.c| 4 +- drivers/gpu/drm/gma500/psb_intel_lvds.c | 6 +-- drivers/gpu/drm/gma500/psb_intel_sdvo.c | 4 +- .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 4 +- drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 4 +- drivers/gpu/drm/i2c/tda998x_drv.c | 5 ++- drivers/gpu/drm/i915/display/icl_dsi.c| 4 +- drivers/gpu/drm/i915/display/intel_crt.c | 5 ++- drivers/gpu/drm/i915/display/intel_ddi.c | 6 ++- drivers/gpu/drm/i915/display/intel_dp.c | 6 +-- drivers/gpu/drm/i915/display/intel_dp_mst.c | 6 ++- drivers/gpu/drm/i915/display/intel_dvo.c | 6 +-- drivers/gpu/drm/i915/display/intel_hdmi.c | 6 +-- drivers/gpu/drm/i915/display/intel_lvds.c | 4 +- drivers/gpu/drm/i915/display/intel_sdvo.c | 6 +-- drivers/gpu/drm/i915/display/intel_tv.c | 4 +- drivers/gpu/drm/i915/display/vlv_dsi.c| 5 ++- drivers/gpu/drm/imx/dw_hdmi-imx.c | 4 +- drivers/gpu/drm/imx/imx-ldb.c | 4 +- drivers/gpu/drm/imx/imx-tve.c | 4 +- drivers/gpu/drm/imx/parallel-display.c| 4 +- drivers/gpu/drm/ingenic/ingenic-drm.c | 5 ++- drivers/gpu/drm/mediatek/mtk_dpi.c| 5 ++- drivers/gpu/drm/mediatek/mtk_dsi.c| 4 +- drivers/gpu/drm/meson/meson_dw_hdmi.c | 5 ++- drivers/gpu/drm/meson/meson_venc_cvbs.c | 5 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 +- .../gpu/drm/msm/disp/mdp4/mdp4_dsi_encoder.c | 4 +- .../gpu/drm/msm/disp/mdp4/mdp4_dtv_encoder.c | 4 +- .../gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 4 +- drivers/gpu/drm/msm/disp/mdp5/mdp5_encoder.c | 3 +- drivers/gpu/drm/nouveau/dispnv04/dac.c| 4 +- drivers/gpu/drm/nouveau/dispnv04/dfp.c| 3 +- drivers/gpu/drm/nouveau/dispnv04/tvnv04.c | 4 +- drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 4 +- drivers/gpu/drm/nouveau/dispnv50/disp.c | 16 drivers/gpu/drm/omapdrm/omap_encoder.c| 4 +- drivers/gpu/drm/radeon/atombios_encoders.c| 40 +-- drivers/gpu/drm/radeon/radeon_dp_mst.c| 4 +- .../gpu/drm/radeon/radeon_legacy_encoders.c | 20 +- drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 4 +- .../gpu/drm/rockchip
[PATCH v1 3/3] drm/atmel-hlcdc: Use drm_encoder_init()
atmel-hlcdc has no need to extend the functionality of the
encoder, so use drm_encoder_init().
Signed-off-by: Sam Ravnborg
Cc: Sam Ravnborg
Cc: Boris Brezillon
Cc: Nicolas Ferre
Cc: Alexandre Belloni
Cc: Ludovic Desroches
Cc: Thomas Zimmermann
Cc: Laurent Pinchart
---
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
index a845d587c315..96e0d85748d2 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
@@ -22,10 +22,6 @@ struct atmel_hlcdc_rgb_output {
int bus_fmt;
};
-static const struct drm_encoder_funcs atmel_hlcdc_panel_encoder_funcs = {
- .destroy = drm_encoder_cleanup,
-};
-
static struct atmel_hlcdc_rgb_output *
atmel_hlcdc_encoder_to_rgb_output(struct drm_encoder *encoder)
{
@@ -98,9 +94,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device
*dev, int endpoint)
return -EINVAL;
}
- ret = drm_encoder_init_funcs(dev, >encoder,
-_hlcdc_panel_encoder_funcs,
-DRM_MODE_ENCODER_NONE, NULL);
+ ret = drm_encoder_init(dev, >encoder, DRM_MODE_ENCODER_NONE);
if (ret)
return ret;
--
2.20.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/8] drm/amdgpu: drop drmP.h in amdgpu_amdkfd_arcturus.c
Hi Alex. On Wed, Jul 31, 2019 at 10:52:39AM -0500, Alex Deucher wrote: > Unused. > > Signed-off-by: Alex Deucher > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c > index 4d9101834ba7..c79aaebeeaf0 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_arcturus.c > @@ -28,7 +28,6 @@ > #include > #include > #include > -#include > #include "amdgpu.h" > #include "amdgpu_amdkfd.h" > #include "sdma0/sdma0_4_2_2_offset.h" Thanks! All patches are: Acked-by: Sam Ravnborg Actual status in drm-misc: $ git grep drmP | cut -d '/' -f 1 | sort | uniq -c 6 amd <= fixed by this patchset 8 arm <= patch sent. Needs to rebase and resend 6 armada <= patch sent. Needs to rebase and resend 1 etnaviv <= already fixed in etnaviv repo 2 exynos <= Somehow missed these. Patch ready, needs to send it out 1 i2c <= patch sent. Needs to rebase and resend 2 msm <= patch sent. Needs to rebase and resend 27 nouveau <= already fixed in nouveau repo 4 tegra <= patch sent. Needs to reabse and resend 13 vmwgfx <= already fixed in vmwgfx repo So things looks doable. I just need to find a few hours.. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Review required [Was: Associate ddc adapters with connectors]
Hi all. Andrzej have done a good job following up on feedback and this series is now ready. We need ack on the patches touching the individual drivers before we can proceed. Please check your drivers and get back. Sam > Hi Andezej. > > On Fri, Jul 26, 2019 at 07:22:54PM +0200, Andrzej Pietrasiewicz wrote: > > It is difficult for a user to know which of the i2c adapters is for which > > drm connector. This series addresses this problem. > > > > The idea is to have a symbolic link in connector's sysfs directory, e.g.: > > > > ls -l /sys/class/drm/card0-HDMI-A-1/ddc > > lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/ddc \ > > -> ../../../../soc/1388.i2c/i2c-2 > > > > The user then knows that their card0-HDMI-A-1 uses i2c-2 and can e.g. run > > ddcutil: > > > > ddcutil -b 2 getvcp 0x10 > > VCP code 0x10 (Brightness): current value =90, max value = 100 > > > > The first patch in the series adds struct i2c_adapter pointer to struct > > drm_connector. If the field is used by a particular driver, then an > > appropriate symbolic link is created by the generic code, which is also > > added > > by this patch. > > > > Patch 2 adds a new variant of drm_connector_init(), see the changelog > > below. > > > > Patches 3..24 are examples of how to convert a driver to this new scheme. > > > ... > > > > v5..v6: > > > > - improved subject line of patch 1 > > - added kernel-doc for drm_connector_init_with_ddc() > > - improved kernel-doc for the ddc field of struct drm_connector > > - added Reviewed-by in patches 17 and 18 > > - added Acked-by in patch 2 > > - made the ownership of ddc i2c_adapter explicit in all patches, > > this made the affected patches much simpler > > Looks good now. > Patch 1 and 2 are: > Reviewed-by: Sam Ravnborg > > The remaining patches are: > Acked-by: Sam Ravnborg > > Sam > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v6 00/24] Associate ddc adapters with connectors
Hi Andezej. On Fri, Jul 26, 2019 at 07:22:54PM +0200, Andrzej Pietrasiewicz wrote: > It is difficult for a user to know which of the i2c adapters is for which > drm connector. This series addresses this problem. > > The idea is to have a symbolic link in connector's sysfs directory, e.g.: > > ls -l /sys/class/drm/card0-HDMI-A-1/ddc > lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/ddc \ > -> ../../../../soc/1388.i2c/i2c-2 > > The user then knows that their card0-HDMI-A-1 uses i2c-2 and can e.g. run > ddcutil: > > ddcutil -b 2 getvcp 0x10 > VCP code 0x10 (Brightness): current value =90, max value = 100 > > The first patch in the series adds struct i2c_adapter pointer to struct > drm_connector. If the field is used by a particular driver, then an > appropriate symbolic link is created by the generic code, which is also added > by this patch. > > Patch 2 adds a new variant of drm_connector_init(), see the changelog > below. > > Patches 3..24 are examples of how to convert a driver to this new scheme. > ... > > v5..v6: > > - improved subject line of patch 1 > - added kernel-doc for drm_connector_init_with_ddc() > - improved kernel-doc for the ddc field of struct drm_connector > - added Reviewed-by in patches 17 and 18 > - added Acked-by in patch 2 > - made the ownership of ddc i2c_adapter explicit in all patches, > this made the affected patches much simpler Looks good now. Patch 1 and 2 are: Reviewed-by: Sam Ravnborg The remaining patches are: Acked-by: Sam Ravnborg Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v5 01/24] drm: Include ddc adapter pointer in struct drm_connector
Hi Andrzej.
After reading through the series a few more comments.
1) The subject of this patch could be improved.
Consider something like:
drm: add ddc link in sysfs created by drm_connector
This spells out much better what the patch achieve.
2) The purpsoe of drm_connector.ddc is to provide drm_connector with
info to create the symlink.
Yet in many follow-up patches the drivers are changed so
drm_connector.ddc is their only reference to struct i2c_adapter.
But the ownership is not clear here.
Who owns the reference to i2c_adapter - and who has the
responsibility to call put() on the adapter.
Looking at the conversions done then some drivers are converted
so they only use drm_connector.ddc, and other drivers have their own
reference to i2c_adapter.
The latter looks like the correct solution as the ownership of the
reference belongs to the driver and not drm_connector.
In other words - a conversion where the drivers only assigned
drm_connector.ddc (using drm_connector_init_with_ddc()),
seems like a more clean design that does not mix up ownership.
This is at least how I see it.
I did not take this type of look at the patches before. Sorry
for providing feedback this late.
Sam
On Fri, Jul 26, 2019 at 08:37:59AM +0200, Sam Ravnborg wrote:
> Hi Andrzej.
>
> Patch looks good, but one kernel-doc detail.
>
> On Wed, Jul 24, 2019 at 03:59:23PM +0200, Andrzej Pietrasiewicz wrote:
> > Add generic code which creates symbolic links in sysfs, pointing to ddc
> > interface used by a particular video output. For example:
> >
> > ls -l /sys/class/drm/card0-HDMI-A-1/ddc
> > lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/ddc \
> > -> ../../../../soc/1388.i2c/i2c-2
> >
> > This makes it easy for user to associate a display with its ddc adapter
> > and use e.g. ddcutil to control the chosen monitor.
> >
> > This patch adds an i2c_adapter pointer to struct drm_connector. Particular
> > drivers can then use it instead of using their own private instance. If a
> > connector contains a ddc, then create a symbolic link in sysfs.
> >
> > Signed-off-by: Andrzej Pietrasiewicz
> > Acked-by: Daniel Vetter
> > Reviewed-by: Andrzej Hajda
> > ---
> > drivers/gpu/drm/drm_sysfs.c | 8
> > include/drm/drm_connector.h | 11 +++
> > 2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> > index ad10810bc972..e962a9d45f7e 100644
> > --- a/drivers/gpu/drm/drm_sysfs.c
> > +++ b/drivers/gpu/drm/drm_sysfs.c
> > @@ -14,6 +14,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -294,6 +295,9 @@ int drm_sysfs_connector_add(struct drm_connector
> > *connector)
> > /* Let userspace know we have a new connector */
> > drm_sysfs_hotplug_event(dev);
> >
> > + if (connector->ddc)
> > + return sysfs_create_link(>kdev->kobj,
> > +>ddc->dev.kobj, "ddc");
> > return 0;
> > }
> >
> > @@ -301,6 +305,10 @@ void drm_sysfs_connector_remove(struct drm_connector
> > *connector)
> > {
> > if (!connector->kdev)
> > return;
> > +
> > + if (connector->ddc)
> > + sysfs_remove_link(>kdev->kobj, "ddc");
> > +
> > DRM_DEBUG("removing \"%s\" from sysfs\n",
> > connector->name);
> >
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 4c30d751487a..33a6fff85fdb 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -41,6 +41,7 @@ struct drm_property;
> > struct drm_property_blob;
> > struct drm_printer;
> > struct edid;
> > +struct i2c_adapter;
> >
> > enum drm_connector_force {
> > DRM_FORCE_UNSPECIFIED,
> > @@ -1311,6 +1312,16 @@ struct drm_connector {
> > * [0]: progressive, [1]: interlaced
> > */
> > int audio_latency[2];
> > +
> > + /**
> > +* @ddc: associated ddc adapter.
> > +* A connector usually has its associated ddc adapter. If a driver uses
> > +* this field, then an appropriate symbolic link is created in connector
> > +* sysfs directory to make it easy for the user to tell which i2c
> > +* adapter is for a particular display.
> > +*/
> > + struct i2c_adapter *ddc;
>
> To help the reader could you add in the above a ref
Re: [PATCH v5 01/24] drm: Include ddc adapter pointer in struct drm_connector
Hi Andrzej.
Patch looks good, but one kernel-doc detail.
On Wed, Jul 24, 2019 at 03:59:23PM +0200, Andrzej Pietrasiewicz wrote:
> Add generic code which creates symbolic links in sysfs, pointing to ddc
> interface used by a particular video output. For example:
>
> ls -l /sys/class/drm/card0-HDMI-A-1/ddc
> lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/ddc \
> -> ../../../../soc/1388.i2c/i2c-2
>
> This makes it easy for user to associate a display with its ddc adapter
> and use e.g. ddcutil to control the chosen monitor.
>
> This patch adds an i2c_adapter pointer to struct drm_connector. Particular
> drivers can then use it instead of using their own private instance. If a
> connector contains a ddc, then create a symbolic link in sysfs.
>
> Signed-off-by: Andrzej Pietrasiewicz
> Acked-by: Daniel Vetter
> Reviewed-by: Andrzej Hajda
> ---
> drivers/gpu/drm/drm_sysfs.c | 8
> include/drm/drm_connector.h | 11 +++
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index ad10810bc972..e962a9d45f7e 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -14,6 +14,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -294,6 +295,9 @@ int drm_sysfs_connector_add(struct drm_connector
> *connector)
> /* Let userspace know we have a new connector */
> drm_sysfs_hotplug_event(dev);
>
> + if (connector->ddc)
> + return sysfs_create_link(>kdev->kobj,
> + >ddc->dev.kobj, "ddc");
> return 0;
> }
>
> @@ -301,6 +305,10 @@ void drm_sysfs_connector_remove(struct drm_connector
> *connector)
> {
> if (!connector->kdev)
> return;
> +
> + if (connector->ddc)
> + sysfs_remove_link(>kdev->kobj, "ddc");
> +
> DRM_DEBUG("removing \"%s\" from sysfs\n",
> connector->name);
>
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 4c30d751487a..33a6fff85fdb 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -41,6 +41,7 @@ struct drm_property;
> struct drm_property_blob;
> struct drm_printer;
> struct edid;
> +struct i2c_adapter;
>
> enum drm_connector_force {
> DRM_FORCE_UNSPECIFIED,
> @@ -1311,6 +1312,16 @@ struct drm_connector {
>* [0]: progressive, [1]: interlaced
>*/
> int audio_latency[2];
> +
> + /**
> + * @ddc: associated ddc adapter.
> + * A connector usually has its associated ddc adapter. If a driver uses
> + * this field, then an appropriate symbolic link is created in connector
> + * sysfs directory to make it easy for the user to tell which i2c
> + * adapter is for a particular display.
> + */
> + struct i2c_adapter *ddc;
To help the reader could you add in the above a reference to
drm_connector_init_with_ddc() sp the reader is told how to init this
field.
Either add it in PATCH 2 - or merge patch 1 and 2.
Sam
> +
> /**
>* @null_edid_counter: track sinks that give us all zeros for the EDID.
>* Needed to workaround some HW bugs where we get all 0s
> --
> 2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v5 02/24] drm: Add drm_connector_init() variant with ddc
Hi Andrzej.
On Wed, Jul 24, 2019 at 03:59:24PM +0200, Andrzej Pietrasiewicz wrote:
> Allow passing ddc adapter pointer to the init function. Even if
> drm_connector_init() sometime in the future decides to e.g. memset() all
> connector fields to zeros, the newly added function ensures that at its
> completion the ddc member of connector is correctly set.
>
> Signed-off-by: Andrzej Pietrasiewicz
> ---
> drivers/gpu/drm/drm_connector.c | 19 +++
> include/drm/drm_connector.h | 5 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 068d4b05f1be..06fbfc44fb48 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -296,6 +296,25 @@ int drm_connector_init(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_connector_init);
>
> +int drm_connector_init_with_ddc(struct drm_device *dev,
> + struct drm_connector *connector,
> + const struct drm_connector_funcs *funcs,
> + int connector_type,
> + struct i2c_adapter *ddc)
> +{
This is good, with this helper there is no longer any confusion about
ordering.
drm_connector_init_with_ddc() is part of the public interface for
drm_connector and needs kernel-doc documentation.
Sam
> + int ret;
> +
> + ret = drm_connector_init(dev, connector, funcs, connector_type);
> + if (ret)
> + return ret;
> +
> + /* provide ddc symlink in sysfs */
> + connector->ddc = ddc;
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_connector_init_with_ddc);
> +
> /**
> * drm_connector_attach_edid_property - attach edid property.
> * @connector: the connector
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 33a6fff85fdb..937fda9c1374 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1410,6 +1410,11 @@ int drm_connector_init(struct drm_device *dev,
> struct drm_connector *connector,
> const struct drm_connector_funcs *funcs,
> int connector_type);
> +int drm_connector_init_with_ddc(struct drm_device *dev,
> + struct drm_connector *connector,
> + const struct drm_connector_funcs *funcs,
> + int connector_type,
> + struct i2c_adapter *ddc);
> void drm_connector_attach_edid_property(struct drm_connector *connector);
> int drm_connector_register(struct drm_connector *connector);
> void drm_connector_unregister(struct drm_connector *connector);
> --
> 2.17.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v4 14/23] drm/tilcdc: Provide ddc symlink in connector sysfs directory
Hi Andrej. On Tue, Jul 23, 2019 at 02:44:50PM +0200, Andrzej Pietrasiewicz wrote: > Hi Sam, > > W dniu 23.07.2019 o 11:05, Sam Ravnborg pisze: > > Hi Andrzej > > > > On Thu, Jul 11, 2019 at 01:26:41PM +0200, Andrzej Pietrasiewicz wrote: > > > Use the ddc pointer provided by the generic connector. > > > > > > Signed-off-by: Andrzej Pietrasiewicz > > > --- > > > drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c > > > b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c > > > index 62d014c20988..c373edb95666 100644 > > > --- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c > > > +++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c > > > @@ -219,6 +219,7 @@ static struct drm_connector > > > *tfp410_connector_create(struct drm_device *dev, > > > tfp410_connector->mod = mod; > > > connector = _connector->base; > > > + connector->ddc = mod->i2c; > > > drm_connector_init(dev, connector, _connector_funcs, > > > DRM_MODE_CONNECTOR_DVID); > > > > When reading this code, it looks strange that we set connector->ddc > > *before* the call to init the connector. > > One could risk that drm_connector_init() used memset(..) to clear all > > fields or so, and it would break this order. > > I verified the code of drm_connector_init() and cannot find any memset() > invocations there. What is your actual concern? My concern is that drm_connector_init() maybe sometime in the future will init all fileds in drm_connector, so we loose any assingments done to drm_connector from *before* we called the init function. Moving the assignment to after drm_connector_init() would not let us depend on the actual implmentation of drm_connector_init(). Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v4 17/23] drm/ast: Provide ddc symlink in connector sysfs directory
Hi Andrzej.
On Thu, Jul 11, 2019 at 01:26:44PM +0200, Andrzej Pietrasiewicz wrote:
> Use the ddc pointer provided by the generic connector.
>
> Signed-off-by: Andrzej Pietrasiewicz
> ---
> drivers/gpu/drm/ast/ast_mode.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index ffccbef962a4..1ca9bc4aa3bb 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -890,6 +890,11 @@ static int ast_connector_init(struct drm_device *dev)
> return -ENOMEM;
>
> connector = _connector->base;
> + ast_connector->i2c = ast_i2c_create(dev);
> + if (!ast_connector->i2c)
> + DRM_ERROR("failed to add ddc bus for connector\n");
> +
> + connector->ddc = _connector->i2c->adapter;
> drm_connector_init(dev, connector, _connector_funcs,
> DRM_MODE_CONNECTOR_VGA);
>
> drm_connector_helper_add(connector, _connector_helper_funcs);
Again, assigning before drm_connector_init().
I did not audit the remaining patches - you got the idea.
Sam
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v4 16/23] drm/mgag200: Provide ddc symlink in connector sysfs directory
Hi Andrzej.
On Thu, Jul 11, 2019 at 01:26:43PM +0200, Andrzej Pietrasiewicz wrote:
> Use the ddc pointer provided by the generic connector.
>
> Signed-off-by: Andrzej Pietrasiewicz
> ---
> drivers/gpu/drm/mgag200/mgag200_mode.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
> b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index a25054015e8c..8fb9444b2142 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -1703,6 +1703,11 @@ static struct drm_connector *mga_vga_init(struct
> drm_device *dev)
> return NULL;
>
> connector = _connector->base;
> + mga_connector->i2c = mgag200_i2c_create(dev);
> + if (!mga_connector->i2c)
> + DRM_ERROR("failed to add ddc bus\n");
> +
> + connector->ddc = _connector->i2c->adapter;
>
> drm_connector_init(dev, connector,
> _vga_connector_funcs, DRM_MODE_CONNECTOR_VGA);
Like on other patch, assigning connector->ddc before
drm_connector_init() looks wrong.
Sam
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v4 14/23] drm/tilcdc: Provide ddc symlink in connector sysfs directory
Hi Andrzej On Thu, Jul 11, 2019 at 01:26:41PM +0200, Andrzej Pietrasiewicz wrote: > Use the ddc pointer provided by the generic connector. > > Signed-off-by: Andrzej Pietrasiewicz > --- > drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c > b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c > index 62d014c20988..c373edb95666 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c > @@ -219,6 +219,7 @@ static struct drm_connector > *tfp410_connector_create(struct drm_device *dev, > tfp410_connector->mod = mod; > > connector = _connector->base; > + connector->ddc = mod->i2c; > > drm_connector_init(dev, connector, _connector_funcs, > DRM_MODE_CONNECTOR_DVID); When reading this code, it looks strange that we set connector->ddc *before* the call to init the connector. One could risk that drm_connector_init() used memset(..) to clear all fields or so, and it would break this order. As it is today the code works as I read it. Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/7] drm/radeon: Fill out gem_object->resv
On Tue, Jun 25, 2019 at 10:42:05PM +0200, Daniel Vetter wrote: > That way we can ditch our gem_prime_res_obj implementation. Since ttm > absolutely needs the right reservation object all the boilerplate is > already there and we just have to wire it up correctly. > > Note that gem/prime doesn't care when we do this, as long as we do it > before the bo is registered and someone can call the handle2fd ioctl > on it. > > Aside: ttm_buffer_object.ttm_resv could probably be ditched in favour > of always passing a non-NULL resv to ttm_bo_init(). At least for gem > drivers that would avoid having two of these, on in ttm_buffer_object > and the other in drm_gem_object, one just there for confusion. Something for todo.rst - so this does not get lost in a changelog people will soon forget? Sam ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: include missing linux/delay.h
Hi Arnd.
On Mon, Jun 17, 2019 at 02:38:55PM +0200, Arnd Bergmann wrote:
> Some randconfig builds fail to compile the dcn10 code because of
> a missing declaration:
>
> drivers/gpu/drm/amd/amdgpu/../display/dc/dcn10/dcn10_hw_sequencer.c: In
> function 'dcn10_apply_ctx_for_surface':
> drivers/gpu/drm/amd/amdgpu/../display/dc/dcn10/dcn10_hw_sequencer.c:2378:3:
> error: implicit declaration of function 'udelay'
> [-Werror=implicit-function-declaration]
>
> Include the appropriate kernel header.
>
> Fixes: 9ed43ef84d9d ("drm/amd/display: Add Underflow Asserts to dc")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
> b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
> index 1ac9a4f03990..d87ddc7de9c6 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
> @@ -22,6 +22,7 @@
> * Authors: AMD
> *
> */
> +#include
>
> #include
> #include "dm_services.h"
Something has gone wrong here, as you add a second include of linux/delay.h.
We had this problem before, which Alex fixed by applying a patch to
include linux/delay.h
Sam
Re: [PATCH 06/59] drm/prime: Actually remove DRIVER_PRIME everywhere
Hi Daniel.
Minor nitpick..
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 65d599065709..4fd09a9ad67a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -3193,7 +3193,7 @@ static struct drm_driver driver = {
>* deal with them for Intel hardware.
>*/
> .driver_features =
> - DRIVER_GEM | DRIVER_PRIME |
> + DRIVER_GEM |
> DRIVER_RENDER | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_SYNCOBJ,
Adds a whitespace.
Sam
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
