Re: [PATCH v3 1/2] drm/panic: Add ABGR2101010 support
Jocelyn Falempe writes:
> On 13/09/2024 09:22, Javier Martinez Canillas wrote:
>> Jocelyn Falempe writes:
>>
>> Hello Jocelyn,
>>
>>> Add support for ABGR2101010, used by the nouveau driver.
>>>
>>> Signed-off-by: Jocelyn Falempe
>>> ---
>>> drivers/gpu/drm/drm_panic.c | 10 ++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
>>> index 74412b7bf936..0a9ecc1380d2 100644
>>> --- a/drivers/gpu/drm/drm_panic.c
>>> +++ b/drivers/gpu/drm/drm_panic.c
>>> @@ -209,6 +209,14 @@ static u32 convert_xrgb_to_argb2101010(u32 pix)
>>> return GENMASK(31, 30) /* set alpha bits */ | pix | ((pix >> 8) &
>>> 0x00300C03);
>>> }
>>>
>>> +static u32 convert_xrgb_to_abgr2101010(u32 pix)
>>> +{
>>> + pix = ((pix & 0x00FF) >> 14) |
>>> + ((pix & 0xFF00) << 4) |
>>> + ((pix & 0x00FF) << 22);
>>> + return GENMASK(31, 30) /* set alpha bits */ | pix | ((pix >> 8) &
>>> 0x00300C03);
>>> +}
>>
>> Maybe we can move this format conversion helper and the others in the
>> driver to drivers/gpu/drm/drm_format_helper.c ?
>
> I think there are still a few issues with that. First is that
> drm_format_helper.c is in a separate module, so you can't call its
> functions from the main drm module, where drm_panic is.
>
I see.
> In my drm_log series, https://patchwork.freedesktop.org/series/136789/ I
> moved this to drm_draw.c, and maybe drm_format_helper could re-use that ?
>
That makes sense to me as well. Thomas, what do you think ?
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH v3 1/2] drm/panic: Add ABGR2101010 support
Jocelyn Falempe writes:
Hello Jocelyn,
> Add support for ABGR2101010, used by the nouveau driver.
>
> Signed-off-by: Jocelyn Falempe
> ---
> drivers/gpu/drm/drm_panic.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index 74412b7bf936..0a9ecc1380d2 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -209,6 +209,14 @@ static u32 convert_xrgb_to_argb2101010(u32 pix)
> return GENMASK(31, 30) /* set alpha bits */ | pix | ((pix >> 8) &
> 0x00300C03);
> }
>
> +static u32 convert_xrgb_to_abgr2101010(u32 pix)
> +{
> + pix = ((pix & 0x00FF) >> 14) |
> + ((pix & 0xFF00) << 4) |
> + ((pix & 0x00FF) << 22);
> + return GENMASK(31, 30) /* set alpha bits */ | pix | ((pix >> 8) &
> 0x00300C03);
> +}
Maybe we can move this format conversion helper and the others in the
driver to drivers/gpu/drm/drm_format_helper.c ?
> +
> /*
> * convert_from_xrgb - convert one pixel from xrgb to the desired
> format
> * @color: input color, in xrgb format
> @@ -242,6 +250,8 @@ static u32 convert_from_xrgb(u32 color, u32 format)
> return convert_xrgb_to_xrgb2101010(color);
> case DRM_FORMAT_ARGB2101010:
> return convert_xrgb_to_argb2101010(color);
> + case DRM_FORMAT_ABGR2101010:
> + return convert_xrgb_to_abgr2101010(color);
> default:
> WARN_ONCE(1, "Can't convert to %p4cc\n", &format);
> return 0;
> --
> 2.46.0
>
The patch looks good to me.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH v3 81/81] drm/omapdrm: Run DRM default client setup
Thomas Zimmermann writes: > Rework fbdev probing to support fbdev_probe in struct drm_driver > and remove the old fb_probe callback. Provide an initializer macro > for struct drm_driver that sets the callback according to the kernel > configuration. > > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The omapdrm driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > Signed-off-by: Thomas Zimmermann > Cc: Tomi Valkeinen > --- > drivers/gpu/drm/omapdrm/omap_drv.c | 1 + > drivers/gpu/drm/omapdrm/omap_fbdev.c | 131 ++- > drivers/gpu/drm/omapdrm/omap_fbdev.h | 8 ++ > 3 files changed, 38 insertions(+), 102 deletions(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 80/81] drm/omapdrm: Remove struct drm_fb_helper from struct omap_fbdev.
Thomas Zimmermann writes: > Store instances of drm_fb_helper and struct omap_fbdev separately. > This will allow omapdrm to use the common fbdev client, which allocates > its own instance of struct drm_fb_helper. > > There is at most one instance of each per DRM device, so both can be > referenced directly from the omap and DRM device structures. A later > patchset might rework the common fbdev client to allow for storing > both, drm_fb_helper and omap_fbdev, together in the same place. > > Signed-off-by: Thomas Zimmermann > Cc: Tomi Valkeinen > --- > drivers/gpu/drm/omapdrm/omap_drv.h | 3 ++ > drivers/gpu/drm/omapdrm/omap_fbdev.c | 42 +++- > 2 files changed, 26 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h > b/drivers/gpu/drm/omapdrm/omap_drv.h > index 4c7217b35f6b..d903568fd8cc 100644 > --- a/drivers/gpu/drm/omapdrm/omap_drv.h > +++ b/drivers/gpu/drm/omapdrm/omap_drv.h > @@ -32,6 +32,7 @@ > #define MODULE_NAME "omapdrm" > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 79/81] drm/tegra: Run DRM default client setup
Thomas Zimmermann writes: > Rework fbdev probing to support fbdev_probe in struct drm_driver > and remove the old fb_probe callback. Provide an initializer macro > for struct drm_driver that sets the callback according to the kernel > configuration. > > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The tegra driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > Signed-off-by: Thomas Zimmermann > Cc: Thierry Reding > Cc: Mikko Perttunen > Cc: Jonathan Hunter > --- > drivers/gpu/drm/tegra/drm.c | 5 +- > drivers/gpu/drm/tegra/drm.h | 12 +++-- > drivers/gpu/drm/tegra/fbdev.c | 98 +++ > 3 files changed, 19 insertions(+), 96 deletions(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 77/81] drm/msm: Run DRM default client setup
Thomas Zimmermann writes: > Rework fbdev probing to support fbdev_probe in struct drm_driver > and remove the old fb_probe callback. Provide an initializer macro > for struct drm_driver that sets the callback according to the kernel > configuration. > > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The msm driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > Signed-off-by: Thomas Zimmermann > Cc: Rob Clark > Cc: Abhinav Kumar > Cc: Dmitry Baryshkov > Cc: Sean Paul > Cc: Marijn Suijten > --- > drivers/gpu/drm/msm/msm_drv.c | 4 +- > drivers/gpu/drm/msm/msm_drv.h | 13 ++- > drivers/gpu/drm/msm/msm_fbdev.c | 144 ++-- > 3 files changed, 38 insertions(+), 123 deletions(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 76/81] drm/gma500: Run DRM default client setup
Thomas Zimmermann writes: > Rework fbdev probing to support fbdev_probe in struct drm_driver > and remove the old fb_probe callback. Provide an initializer macro > for struct drm_driver that sets the callback according to the kernel > configuration. > > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > Signed-off-by: Thomas Zimmermann > Cc: Patrik Jakobsson > --- > drivers/gpu/drm/gma500/fbdev.c | 100 +++ > drivers/gpu/drm/gma500/psb_drv.c | 4 +- > drivers/gpu/drm/gma500/psb_drv.h | 12 +++- > 3 files changed, 19 insertions(+), 97 deletions(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 75/81] drm/exynos-drm: Run DRM default client setup
Thomas Zimmermann writes:
> Rework fbdev probing to support fbdev_probe in struct drm_driver
> and remove the old fb_probe callback. Provide an initializer macro
> for struct drm_driver that sets the callback according to the kernel
> configuration.
>
> Call drm_client_setup() to run the kernel's default client setup
> for DRM. Set fbdev_probe in struct drm_driver, so that the client
> setup can start the common fbdev client.
>
> The exynos-drm driver specifies a preferred color mode of 32. As this
> is the default if no format has been given, leave it out entirely.
>
> Signed-off-by: Thomas Zimmermann
> Cc: Inki Dae
> Cc: Seung-Woo Kim
> Cc: Kyungmin Park
> ---
> drivers/gpu/drm/exynos/exynos_drm_drv.c | 4 +-
> drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 101 ++
> drivers/gpu/drm/exynos/exynos_drm_fbdev.h | 15 ++--
> 3 files changed, 19 insertions(+), 101 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 7c59e1164a48..2a466d8179f4 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -15,6 +15,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -111,6 +112,7 @@ static const struct drm_driver exynos_drm_driver = {
> .dumb_create= exynos_drm_gem_dumb_create,
> .gem_prime_import = exynos_drm_gem_prime_import,
> .gem_prime_import_sg_table = exynos_drm_gem_prime_import_sg_table,
> + EXYNOS_DRM_FBDEV_DRIVER_OPS,
> .ioctls = exynos_ioctls,
> .num_ioctls = ARRAY_SIZE(exynos_ioctls),
> .fops = &exynos_drm_driver_fops,
> @@ -288,7 +290,7 @@ static int exynos_drm_bind(struct device *dev)
> if (ret < 0)
> goto err_cleanup_poll;
>
> - exynos_drm_fbdev_setup(drm);
> + drm_client_setup(drm, NULL);
>
> return 0;
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index a379c8ca435a..73fa7b77d8d0 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -22,9 +22,6 @@
> #include "exynos_drm_fb.h"
> #include "exynos_drm_fbdev.h"
>
> -#define MAX_CONNECTOR4
I see this constant was unused. Is an unrelated cleanup but I guess is OK too.
Acked-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH v3 74/81] drm/armada: Run DRM default client setup
Thomas Zimmermann writes: > Rework fbdev probing to support fbdev_probe in struct drm_driver > and remove the old fb_probe callback. Provide an initializer macro > for struct drm_driver that sets the callback according to the kernel > configuration. > > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The armada driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > Signed-off-by: Thomas Zimmermann > Cc: Russell King > --- > drivers/gpu/drm/armada/armada_drm.h | 11 ++- > drivers/gpu/drm/armada/armada_drv.c | 4 +- > drivers/gpu/drm/armada/armada_fbdev.c | 115 ++ > 3 files changed, 17 insertions(+), 113 deletions(-) > > diff --git a/drivers/gpu/drm/armada/armada_drm.h > b/drivers/gpu/drm/armada/armada_drm.h > index c303e8c7ff6c..3c0ff221a43b 100644 [...] > @@ -108,113 +111,7 @@ static int armada_fbdev_create(struct drm_fb_helper > *fbh, > > return 0; > > - err_fballoc: > +err_fballoc: Unrelated cleanup but probably not worth to split in a separate patch... > dfb->fb.funcs->destroy(&dfb->fb); > return ret; > } Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 73/81] drm/fbdev-ttm: Remove obsolete setup function
Thomas Zimmermann writes: > The old setup function drm_fbdev_ttm_setup() is unused. Remove it and > its internal callbacks. New drivers should call drm_client_setup() > instead. > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/drm_fbdev_ttm.c | 119 > include/drm/drm_fbdev_ttm.h | 6 -- > 2 files changed, 125 deletions(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 72/81] drm/vmwgfx: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > Signed-off-by: Thomas Zimmermann > Cc: Zack Rusin > Cc: Broadcom internal kernel review list > > --- > drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 71/81] drm/vboxvideo: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The vboxvideo driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > Signed-off-by: Thomas Zimmermann > Cc: Hans de Goede > --- > drivers/gpu/drm/vboxvideo/vbox_drv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 70/81] drm/qxl: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The qxl driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > Signed-off-by: Thomas Zimmermann > Cc: Dave Airlie > Cc: Gerd Hoffmann > --- > drivers/gpu/drm/qxl/qxl_drv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 67/81] drm/hisilicon/hibmc: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The hibmc driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > Signed-off-by: Thomas Zimmermann > Cc: Xinliang Liu > Cc: Tian Tao > Cc: Xinwei Kong > Cc: Sumit Semwal > Cc: Yongqin Liu > Cc: John Stultz > --- > drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 66/81] drm/bochs: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The bochs driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > Signed-off-by: Thomas Zimmermann > Cc: Gerd Hoffmann > --- > drivers/gpu/drm/tiny/bochs.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 64/81] drm/fbdev-ttm: Support struct drm_driver.fbdev_probe
Thomas Zimmermann writes: > Rework fbdev probing to support fbdev_probe in struct drm_driver > and reimplement the old fb_probe callback on top of it. Provide an > initializer macro for struct drm_driver that sets the callback > according to the kernel configuration. > > This change allows the common fbdev client to run on top of TTM- > based DRM drivers. > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/drm_fbdev_ttm.c | 142 +--- > include/drm/drm_fbdev_ttm.h | 13 +++ > 2 files changed, 90 insertions(+), 65 deletions(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 63/81] drm/fbdev-shmem: Remove obsolete setup function
Thomas Zimmermann writes: > The old setup function drm_fbdev_shmem_setup() is unused. Remove it > and its internal callbacks. New drivers should call drm_client_setup() > instead. > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/drm_fbdev_shmem.c | 120 +- > include/drm/drm_fbdev_shmem.h | 6 -- > 2 files changed, 1 insertion(+), 125 deletions(-) > Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 62/81] drm/vkms: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > Signed-off-by: Thomas Zimmermann > Cc: Rodrigo Siqueira > Cc: Melissa Wen > Cc: "Maíra Canal" > Cc: Haneen Mohammed > --- > drivers/gpu/drm/vkms/vkms_drv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 61/81] drm/virtgpu: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The virtgpu driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > Signed-off-by: Thomas Zimmermann > Cc: David Airlie > Cc: Gerd Hoffmann > Cc: Gurchetan Singh > Cc: Chia-I Wu > --- > drivers/gpu/drm/virtio/virtgpu_drv.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 60/81] drm/udl: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > Signed-off-by: Thomas Zimmermann > Cc: Dave Airlie > Cc: Sean Paul > Cc: Thomas Zimmermann > --- > drivers/gpu/drm/udl/udl_drv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 53/81] drm/gm12u320: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > Signed-off-by: Thomas Zimmermann > Cc: Hans de Goede > --- > drivers/gpu/drm/tiny/gm12u320.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 52/81] drm/cirrus: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The cirrus driver requests the same client pixel format as the value > stored in struct drm_mode_config.preferred_depth. The fbdev client > also looks at this value for the default pixel format. Thus remove > the format selection from cirrus. > > Signed-off-by: Thomas Zimmermann > Cc: Dave Airlie > Cc: Gerd Hoffmann > --- > drivers/gpu/drm/tiny/cirrus.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 50/81] drm/fbdev-shmem: Support struct drm_driver.fbdev_probe
Thomas Zimmermann writes:
> Rework fbdev probing to support fbdev_probe in struct drm_driver
> and reimplement the old fb_probe callback on top of it. Provide an
> initializer macro for struct drm_driver that sets the callback
> according to the kernel configuration.
>
> This change allows the common fbdev client to run on top of SHMEM-
> based DRM drivers.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_fbdev_shmem.c | 60 ++-
> include/drm/drm_fbdev_shmem.h | 11 ++
> 2 files changed, 47 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fbdev_shmem.c
> b/drivers/gpu/drm/drm_fbdev_shmem.c
> index 0c785007f11b..3bca333917d1 100644
> --- a/drivers/gpu/drm/drm_fbdev_shmem.c
> +++ b/drivers/gpu/drm/drm_fbdev_shmem.c
> @@ -107,6 +107,40 @@ static struct page *drm_fbdev_shmem_get_page(struct
> fb_info *info, unsigned long
>
> static int drm_fbdev_shmem_helper_fb_probe(struct drm_fb_helper *fb_helper,
> struct drm_fb_helper_surface_size
> *sizes)
> +{
I was going to ask if this whould be static inline too but I guess is just
a transition change and will be removed by a later patch, as was the case
for the fbdev-dma setup function. So it doesn't really matter that much...
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH v3 49/81] drm/fbdev-dma: Remove obsolete setup function
Thomas Zimmermann writes: > The old setup function drm_fbdev_dma_setup() is unused. Remove it and > its internal callbacks. New drivers should call drm_client_setup() > instead. > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/drm_fbdev_dma.c | 120 +--- > include/drm/drm_fbdev_dma.h | 7 -- > 2 files changed, 1 insertion(+), 126 deletions(-) > Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 43/81] drm/tilcdc: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup_with_color_mode() to run the kernel's default > client setup for DRM. Set fbdev_probe in struct drm_driver, so that > the client setup can start the common fbdev client. > > v3: > - add DRM_FBDEV_DMA_DRIVER_OPS macro > > Signed-off-by: Thomas Zimmermann > Cc: Jyri Sarha > Cc: Tomi Valkeinen > --- > drivers/gpu/drm/tilcdc/tilcdc_drv.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 42/81] drm/tidss: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The tidss driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > Signed-off-by: Thomas Zimmermann > Cc: Jyri Sarha > Cc: Tomi Valkeinen > --- > drivers/gpu/drm/tidss/tidss_drv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 41/81] drm/sun4i: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The sun4i driver specifies as preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > Signed-off-by: Thomas Zimmermann > Cc: Maxime Ripard > Cc: Chen-Yu Tsai > Cc: Jernej Skrabec > Cc: Samuel Holland > --- > drivers/gpu/drm/sun4i/sun4i_drv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 39/81] drm/sti: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The sti driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > Signed-off-by: Thomas Zimmermann > Cc: Alain Volmat > --- > drivers/gpu/drm/sti/sti_drv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 33/81] drm/pl111: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup_with_color_mode() to run the kernel's default > client setup for DRM. Set fbdev_probe in struct drm_driver, so that > the client setup can start the common fbdev client. > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/pl111/pl111_drv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 30/81] drm/mxsfb: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The mxsfb driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > v3: > - fix driver name "msxfb" to "mxsfb" > > Signed-off-by: Thomas Zimmermann > Cc: Marek Vasut > Cc: Stefan Agner > Cc: Shawn Guo > Cc: Sascha Hauer > Cc: Pengutronix Kernel Team > Cc: Fabio Estevam > --- > drivers/gpu/drm/mxsfb/mxsfb_drv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 29/81] drm/mxsfb/lcdif: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The lcdif driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > Signed-off-by: Thomas Zimmermann > Cc: Marek Vasut > Cc: Stefan Agner > Cc: Shawn Guo > Cc: Sascha Hauer > Cc: Pengutronix Kernel Team > Cc: Fabio Estevam > --- > drivers/gpu/drm/mxsfb/lcdif_drv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 25/81] drm/mcde: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The mcde driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > Signed-off-by: Thomas Zimmermann > Cc: Linus Walleij > Acked-by: Linus Walleij > --- > drivers/gpu/drm/mcde/mcde_drv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 24/81] drm/logicvc: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The logicvc driver specifies a preferred color mode from the value > in struct drm_mode_config.preferred_depth. The fbdev client also > looks at this value for the default pixel format. Thus remove the > format selection from logicvc. > > Signed-off-by: Thomas Zimmermann > Cc: Paul Kocialkowski > --- > drivers/gpu/drm/logicvc/logicvc_drm.c | 16 > 1 file changed, 4 insertions(+), 12 deletions(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 23/81] drm/kmb: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > Signed-off-by: Thomas Zimmermann > Cc: Anitha Chrisanthus > Cc: Edmund Dea > --- > drivers/gpu/drm/kmb/kmb_drv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 21/81] drm/imx/lcdc: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > Signed-off-by: Thomas Zimmermann > Cc: Shawn Guo > Cc: Sascha Hauer > Cc: Pengutronix Kernel Team > Cc: Fabio Estevam > --- > drivers/gpu/drm/imx/lcdc/imx-lcdc.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 20/81] drm/imx/ipuv3: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup_with_color_mode() to run the kernel's default > client setup for DRM. Set fbdev_probe in struct drm_driver, so that > the client setup can start the common fbdev client. > > Signed-off-by: Thomas Zimmermann > Cc: Philipp Zabel > Cc: Shawn Guo > Cc: Sascha Hauer > Cc: Pengutronix Kernel Team > Cc: Fabio Estevam > --- > drivers/gpu/drm/imx/ipuv3/imx-drm-core.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 19/81] drm/imx/dcss: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The dcss driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > Signed-off-by: Thomas Zimmermann > Cc: Laurentiu Palcu > Cc: Lucas Stach > Cc: Shawn Guo > Cc: Sascha Hauer > Cc: Pengutronix Kernel Team > Cc: Fabio Estevam > --- > drivers/gpu/drm/imx/dcss/dcss-kms.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 18/81] drm/ili9486: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > Signed-off-by: Thomas Zimmermann > Cc: Kamlesh Gurudasani > --- > drivers/gpu/drm/tiny/ili9486.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 17/81] drm/ili9341: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/tiny/ili9341.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 15/81] drm/ili9163: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/tiny/ili9163.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 14/81] drm/hx8357d: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/tiny/hx8357d.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 02/81] drm/fbdev-helper: Set and clear VGA switcheroo client from fb_info
Thomas Zimmermann writes:
> Hi Javier
>
> Am 03.09.24 um 12:18 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann writes:
>>
>> Hello Thomas,
>>
>>> Call vga_switcheroo_client_fb_set() with the PCI device from the
>>> instance of struct fb_info. All fbdev clients now run these calls.
>>> For non-PCI devices or drivers without vga-switcheroo, this does
>>> nothing. For i915 and radeon, it allows these drivers to use a
>>> common fbdev client.
>>>
>>> The device is the same as the one stored in struct drm_client and
>>> struct drm_fb_helper, so there is no difference in behavior. Some
>>> NULL-pointer checks are being removed, where those pointers cannot
>>> be NULL.
>>>
>>> Signed-off-by: Thomas Zimmermann
>>> ---
>>> drivers/gpu/drm/drm_fb_helper.c | 15 +++
>>> 1 file changed, 11 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c
>>> b/drivers/gpu/drm/drm_fb_helper.c
>>> index af1fe79c701d..13095d38aa42 100644
>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>> @@ -562,8 +562,12 @@ EXPORT_SYMBOL(drm_fb_helper_release_info);
>>>*/
>>> void drm_fb_helper_unregister_info(struct drm_fb_helper *fb_helper)
>>> {
>>> - if (fb_helper && fb_helper->info)
>>> - unregister_framebuffer(fb_helper->info);
>> I'm not sure if we can assume these won't be NULL... AFAICT some drivers
>> still have their own struct drm_client_funcs vtable and could potentially
>> pass a NULL struct drm_fb_helper ?
>
> I did a
>
> git grep -B4 drm_fb_helper_unregister_info
>
> on drm-tip and all callers, such as [1], test fb_helper->info before
> calling the function. So it's safe to remove the test.
>
Yes, I also noticed that all callers were already checking if is not NULL
but still is more robust for the function to check it in case there is a
bug in a driver.
But as a said, I'm OK with dropping it as long as it is mentioned in the
kernel-doc that the parameter must not be NULL.
> [1]
> https://elixir.bootlin.com/linux/v6.10/source/drivers/gpu/drm/drm_fbdev_dma.c#L162
>
>>
>> If you think that's safe to do this and the function semantics should be
>> changed, then I think that the kernel-doc needs to be updated:
>>
>> - * @fb_helper: driver-allocated fbdev helper, can be NULL
>> + * @fb_helper: driver-allocated fbdev helper, must not be NULL
>
> Ok.
>
>>
>> Other than that, the patch looks good to me:
>>
>> Reviewed-by: Javier Martinez Canillas
>
> Thanks.
>
> Best regards
> Thomas
>
>>
>
> --
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH v3 12/81] drm/fsl-dcu: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup_with_color_mode() to run the kernel's default > client setup for DRM. Set fbdev_probe in struct drm_driver, so that > the client setup can start the common fbdev client. > > Signed-off-by: Thomas Zimmermann > Cc: Stefan Agner > Cc: Alison Wang > --- > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 10/81] drm/aspeed: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The aspeed driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > v3: > - add DRM_FBDEV_DMA_DRIVER_OPS macro > > Signed-off-by: Thomas Zimmermann > Cc: Joel Stanley > Cc: Andrew Jeffery > --- > drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 06/81] drm/arcgpu: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup_with_fourcc() to run the kernel's default client > setup for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > v2: > - use drm_client_setup_with_fourcc() > > Signed-off-by: Thomas Zimmermann Acked-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 05/81] drm/fbdev-dma: Support struct drm_driver.fbdev_probe
Thomas Zimmermann writes:
> Rework fbdev probing to support fbdev_probe in struct drm_driver
> and reimplement the old fb_probe callback on top of it. Provide an
> initializer macro for struct drm_driver that sets the callback
> according to the kernel configuration.
>
> This change allows the common fbdev client to run on top of DMA-
> based DRM drivers.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_fbdev_dma.c | 60 -
> include/drm/drm_fbdev_dma.h | 12 +++
> 2 files changed, 48 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c
> index 7ef5a48c8029..aeccf7f7a522 100644
> --- a/drivers/gpu/drm/drm_fbdev_dma.c
> +++ b/drivers/gpu/drm/drm_fbdev_dma.c
> @@ -86,6 +86,40 @@ static const struct fb_ops drm_fbdev_dma_fb_ops = {
>
> static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper,
>struct drm_fb_helper_surface_size
> *sizes)
> +{
static inline for this wrapper maybe ?
> + return drm_fbdev_dma_driver_fbdev_probe(fb_helper, sizes);
> +}
> +
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH v3 03/81] drm/fbdev: Add memory-agnostic fbdev client
Thomas Zimmermann writes: > Add an fbdev client that can work with any memory manager. The > client implementation is the same as existing code in fbdev-dma or > fbdev-shmem. > > Provide struct drm_driver.fbdev_probe for the new client to allocate > the surface GEM buffer. The new callback replaces fb_probe of struct > drm_fb_helper_funcs, which does the same. > > To use the new client, DRM drivers set fbdev_probe in their struct > drm_driver instance and call drm_fbdev_client_setup(). Probing and > creating the fbdev surface buffer is now independent from the other > operations in struct drm_fb_helper. For the pixel format, the fbdev > client either uses a specified format, the value in preferred_depth > or 32-bit RGB. > > v2: > - test for struct drm_fb_helper.funcs for NULL (Sui) > - respect struct drm_mode_config.preferred_depth for default format > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 02/81] drm/fbdev-helper: Set and clear VGA switcheroo client from fb_info
Thomas Zimmermann writes:
Hello Thomas,
> Call vga_switcheroo_client_fb_set() with the PCI device from the
> instance of struct fb_info. All fbdev clients now run these calls.
> For non-PCI devices or drivers without vga-switcheroo, this does
> nothing. For i915 and radeon, it allows these drivers to use a
> common fbdev client.
>
> The device is the same as the one stored in struct drm_client and
> struct drm_fb_helper, so there is no difference in behavior. Some
> NULL-pointer checks are being removed, where those pointers cannot
> be NULL.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_fb_helper.c | 15 +++
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index af1fe79c701d..13095d38aa42 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -562,8 +562,12 @@ EXPORT_SYMBOL(drm_fb_helper_release_info);
> */
> void drm_fb_helper_unregister_info(struct drm_fb_helper *fb_helper)
> {
> - if (fb_helper && fb_helper->info)
> - unregister_framebuffer(fb_helper->info);
I'm not sure if we can assume these won't be NULL... AFAICT some drivers
still have their own struct drm_client_funcs vtable and could potentially
pass a NULL struct drm_fb_helper ?
If you think that's safe to do this and the function semantics should be
changed, then I think that the kernel-doc needs to be updated:
- * @fb_helper: driver-allocated fbdev helper, can be NULL
+ * @fb_helper: driver-allocated fbdev helper, must not be NULL
Other than that, the patch looks good to me:
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH 59/86] drm/solomon: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > The solomon driver specifies a preferred color mode of 32. As this > is the default if no format has been given, leave it out entirely. > > Signed-off-by: Thomas Zimmermann > Cc: Javier Martinez Canillas > --- > drivers/gpu/drm/solomon/ssd130x.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 58/86] drm/simpledrm: Run DRM default client setup
Thomas Zimmermann writes: > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > Signed-off-by: Thomas Zimmermann > Cc: Thomas Zimmermann > Cc: Javier Martinez Canillas > --- > drivers/gpu/drm/tiny/simpledrm.c | 9 +++-- > 1 file changed, 3 insertions(+), 6 deletions(-) > Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 57/86] drm/ofdrm: Use DRM default client setup
Thomas Zimmermann writes: Hello Thomas, > Call drm_client_setup() to run the kernel's default client setup > for DRM. Set fbdev_probe in struct drm_driver, so that the client > setup can start the common fbdev client. > > Signed-off-by: Thomas Zimmermann > Cc: Thomas Zimmermann > Cc: Javier Martinez Canillas > --- > drivers/gpu/drm/tiny/ofdrm.c | 9 +++-- > 1 file changed, 3 insertions(+), 6 deletions(-) Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH 00/53] drm: Convert to platform remove callback returning void
Uwe Kleine-König writes: Hello Uwe, > Hello, > > this patch series adapts the platform drivers below drivers/gpu/drm > to use the .remove_new() callback. Compared to the traditional .remove() > callback .remove_new() returns no value. This is a good thing because > the driver core doesn't (and cannot) cope for errors during remove. The > only effect of a non-zero return value in .remove() is that the driver > core emits a warning. The device is removed anyhow and an early return > from .remove() usually yields a resource leak. > > By changing the remove callback to return void driver authors cannot > reasonably (but wrongly) assume any more that there happens some kind of > cleanup later. > > Best regards > Uwe > > Uwe Kleine-König (53): [...] > drm/imx/ipuv3: Convert to platform remove callback returning void > drm/ingenic: Convert to platform remove callback returning void [...] > drm/mediatek: Convert to platform remove callback returning void > drm/mediatek: Convert to platform remove callback returning void [...] > drm/msm: Convert to platform remove callback returning void [...] > drm/shmobile: Convert to platform remove callback returning void Pushed these to drm-misc (drm-misc-next). Thanks! -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2] drm/nouveau: Remove file nouveau_fbcon.c
Hello Thomas,
On 1/10/23 13:35, Thomas Zimmermann wrote:
> Commit 4a16dd9d18a0 ("drm/nouveau/kms: switch to drm fbdev helpers")
> converted nouveau to generic fbdev emulation. The driver's internal
> implementation later got accidentally restored during a merge commit.
> Remove the file from the driver. No functional changes.
>
> v2:
> * point Fixes tag to merge commit (Alex)
>
> Signed-off-by: Thomas Zimmermann
> Reviewed-by: Alex Deucher
> Fixes: 4e291f2f5853 ("Merge tag 'drm-misc-next-2022-11-10-1' of
> git://anongit.freedesktop.org/drm/drm-misc into drm-next")
I believe the fixes tag should be before the S-o-B ? At least that is
the case in most commits and Documentation/process/maintainer-tip.rst
example. But you could fix it just before applying.
The patch looks good to me.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Nouveau] [PATCH v3] drm: Only select I2C_ALGOBIT for drivers that actually need it
On 12/19/22 09:49, Javier Martinez Canillas wrote: > Hello Uwe, > > On 12/19/22 09:36, Uwe Kleine-König wrote: >> While working on a drm driver that doesn't need the i2c algobit stuff I >> noticed that DRM selects this code even though only 8 drivers actually use >> it. While also only some drivers use i2c, keep the select for I2C for the >> next cleanup patch. Still prepare this already by also selecting I2C for >> the individual drivers. >> >> Signed-off-by: Uwe Kleine-König >> --- > > Thanks for sending a v3 of this. > > Reviewed-by: Javier Martinez Canillas > I've pushed this to drm-misc (dri-misc-next) now. Thanks! -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v3] drm: Only select I2C_ALGOBIT for drivers that actually need it
Hello Uwe, On 12/19/22 09:36, Uwe Kleine-König wrote: > While working on a drm driver that doesn't need the i2c algobit stuff I > noticed that DRM selects this code even though only 8 drivers actually use > it. While also only some drivers use i2c, keep the select for I2C for the > next cleanup patch. Still prepare this already by also selecting I2C for > the individual drivers. > > Signed-off-by: Uwe Kleine-König > --- Thanks for sending a v3 of this. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v3 23/23] drm/fb-helper: Clarify use of last_close and output_poll_changed
On 11/3/22 16:14, Thomas Zimmermann wrote: > Clarify documentation in the use of struct drm_driver.last_close and > struct drm_mode_config_funcs.output_poll_changed. Those callbacks should > not be said for fbdev implementations on top of struct drm_client_funcs. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v3 20/23] drm/fb-helper: Set flag in struct drm_fb_helper for leaking physical addresses
On 11/3/22 16:14, Thomas Zimmermann wrote: > Uncouple the parameter drm_leak_fbdev_smem from the implementation by > setting a flag in struct drm_fb_helper. This will help to move the > generic fbdev emulation into its own source file, while keeping the > parameter in drm_fb_helper.c. No functional changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 17/21] drm/fb-helper: Perform all fbdev I/O with the same implementation
On 11/2/22 11:33, Thomas Zimmermann wrote:
[...]
>>
>>> +static ssize_t __drm_fb_helper_write(struct fb_info *info, const char
>>> __user *buf, size_t count,
>>> +loff_t *ppos, drm_fb_helper_write_screen
>>> write_screen)
>>> +{
>>
>> [...]
>>
>>> + /*
>>> +* Copy to framebuffer even if we already logged an error. Emulates
>>> +* the behavior of the original fbdev implementation.
>>> +*/
>>> + ret = write_screen(info, buf, count, pos);
>>> + if (ret < 0)
>>> + return ret; /* return last error, if any */
>>> + else if (!ret)
>>> + return err; /* return previous error, if any */
>>> +
>>> + *ppos += ret;
>>> +
>>
>> Should *ppos be incremented even if the previous error is returned?
>
> Yes. It emulates the original fbdev code at [1]. Further down in that
> function, the position is being updated even if an error occured. We
> only return the initial error if no bytes got written.
>
> It could happen that some userspace program hits to error, but still
> relies on the output and position being updated. IIRC I even added
> validation of this behavior to the IGT fbdev tests. I agree that this
> is somewhat bogus behavior, but changing it would change long-standing
> userspace semantics.
>
Thanks for the explanation, feel free then to also add to this patch:
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Nouveau] [PATCH v2 21/21] drm/fb-helper: Remove unnecessary include statements
On 10/24/22 13:19, Thomas Zimmermann wrote: > Remove include statements for where it is not > required (i.e., most of them). In a few places include other header > files that are required by the source code. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 20/21] drm/fb-helper: Move generic fbdev emulation into separate source file
On 10/24/22 13:19, Thomas Zimmermann wrote: > Move the generic fbdev implementation into its own source and header > file. Adapt drivers. No functonal changes, but some of the internal > helpers have been renamed to fit into the drm_fbdev_ naming scheme. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 19/21] drm/fb-helper: Always initialize generic fbdev emulation
On 10/24/22 13:19, Thomas Zimmermann wrote: > Initialize the generic fbdev emulation even if it has been disabled > on the kernel command line. The hotplug and mode initialization will > fail accordingly. > > The kernel parameter can still be changed at runtime and the emulation > will initialize after hotplugging the connector. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 18/21] drm/fb_helper: Minimize damage-helper overhead
On 10/24/22 13:19, Thomas Zimmermann wrote:
> Pull the test for fb_dirty into the caller to avoid extra work
> if no callback has been set. In this case no damage handling is
> required and no damage area needs to be computed. Print a warning
> if the damage worker runs without getting an fb_dirty callback.
>
> Signed-off-by: Thomas Zimmermann
> ---
Reviewed-by: Javier Martinez Canillas
But I've a trivial comment below:
> drivers/gpu/drm/drm_fb_helper.c | 90 ++---
> 1 file changed, 60 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 836523aef6a27..fbc5c5445fdb0 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -449,12 +449,13 @@ static int drm_fb_helper_damage_blit(struct
> drm_fb_helper *fb_helper,
> static void drm_fb_helper_damage_work(struct work_struct *work)
> {
> struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper,
> damage_work);
> + struct drm_device *dev = helper->dev;
You removed this in patch #15, maybe just leaving it in that patch if you
plan to use it again here?
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Nouveau] [PATCH v2 17/21] drm/fb-helper: Perform all fbdev I/O with the same implementation
On 10/24/22 13:19, Thomas Zimmermann wrote:
> Implement the fbdev's read/write helpers with the same functions. Use
> the generic fbdev's code as template. Convert all drivers.
>
> DRM's fb helpers must implement regular I/O functionality in struct
> fb_ops and possibly perform a damage update. Handle all this in the
> same functions and convert drivers. The functionality has been used
> as part of the generic fbdev code for some time. The drivers don't
> set struct drm_fb_helper.fb_dirty, so they will not be affected by
> damage handling.
>
> For I/O memory, fb helpers now provide drm_fb_helper_cfb_read() and
> drm_fb_helper_cfb_write(). Several drivers require these. Until now
> tegra used I/O read and write, although the memory buffer appears to
> be in system memory. So use _sys_ helpers now.
>
> Signed-off-by: Thomas Zimmermann
> ---
[...]
> +static ssize_t __drm_fb_helper_write(struct fb_info *info, const char __user
> *buf, size_t count,
> + loff_t *ppos, drm_fb_helper_write_screen
> write_screen)
> +{
[...]
> + /*
> + * Copy to framebuffer even if we already logged an error. Emulates
> + * the behavior of the original fbdev implementation.
> + */
> + ret = write_screen(info, buf, count, pos);
> + if (ret < 0)
> + return ret; /* return last error, if any */
> + else if (!ret)
> + return err; /* return previous error, if any */
> +
> + *ppos += ret;
> +
Should *ppos be incremented even if the previous error is returned?
The write_screen() succeeded anyways, even when the count written was
smaller than what the caller asked for.
> /**
> - * drm_fb_helper_sys_read - wrapper around fb_sys_read
> + * drm_fb_helper_sys_read - Implements struct &fb_ops.fb_read for system
> 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
> *
> - * A wrapper around fb_sys_read implemented by fbdev core
> + * Returns:
> + * The number of read bytes on success, or an error code otherwise.
> */
This sentence sounds a little bit off to me. Shouldn't be "number of bytes read"
instead? I'm not a native English speaker though, so feel free to just ignore
me.
[...]
>
> +static ssize_t fb_read_screen_base(struct fb_info *info, char __user *buf,
> size_t count,
> +loff_t pos)
> +{
> + const char __iomem *src = info->screen_base + pos;
> + size_t alloc_size = min_t(size_t, count, PAGE_SIZE);
> + ssize_t ret = 0;
> + int err = 0;
Do you really need these two? AFAIK ssize_t is a signed type
so you can just use the ret variable to store and return the
errno value.
[...]
> +static ssize_t fb_write_screen_base(struct fb_info *info, const char __user
> *buf, size_t count,
> + loff_t pos)
> +{
> + char __iomem *dst = info->screen_base + pos;
> + size_t alloc_size = min_t(size_t, count, PAGE_SIZE);
> + ssize_t ret = 0;
> + int err = 0;
Same here.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Nouveau] [PATCH v2 16/21] drm/fb-helper: Call fb_sync in I/O functions
On 10/24/22 13:19, Thomas Zimmermann wrote:
> Call struct fb_ops.fb_sync in drm_fbdev_{read,write}() to mimic the
> behavior of fbdev. Fbdev implementations of fb_read and fb_write in
> struct fb_ops invoke fb_sync to synchronize with outstanding operations
> before I/O. Doing the same in DRM implementations will allow us to use
> them throughout DRM drivers.
>
> Signed-off-by: Thomas Zimmermann
> ---
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Nouveau] [PATCH v2 15/21] drm/fb-helper: Disconnect damage worker from update logic
On 10/24/22 13:19, Thomas Zimmermann wrote:
> The fbdev helpers implement a damage worker that forwards fbdev
> updates to the DRM driver. The worker's update logic depends on
> the generic fbdev emulation. Separate the two via function pointer.
>
> The generic fbdev emulation sets struct drm_fb_helper_funcs.fb_dirty,
> a new callback that hides the update logic from the damage worker.
> It's not possible to use the generic logic with other fbdev emulation,
> because it contains additional code for the shadow buffering that
> the generic emulation employs.
>
> DRM drivers with internal fbdev emulation can set fb_dirty to their
> own implementation if they require damage handling; although no such
> drivers currently exist.
>
> Signed-off-by: Thomas Zimmermann
> ---
[...]
> static void drm_fb_helper_damage_work(struct work_struct *work)
> {
> - struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper,
> - damage_work);
> - struct drm_device *dev = helper->dev;
> + struct drm_fb_helper *helper = container_of(work, struct drm_fb_helper,
> damage_work);
This line is an unrelated code style change. But I guess it's OK.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Nouveau] [PATCH v2 14/21] drm/fb-helper: Rename drm_fb_helper_unregister_fbi() to use _info postfix
On 10/24/22 13:19, Thomas Zimmermann wrote: > Rename drm_fb_helper_unregister_fbi() to drm_fb_helper_unregister_info() > as part of unifying the naming within fbdev helpers. Adapt drivers. No > functional changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 13/21] drm/fb-helper: Rename drm_fb_helper_alloc_fbi() to use _info postfix
On 10/24/22 13:19, Thomas Zimmermann wrote: > Rename drm_fb_helper_alloc_fbi() to drm_fb_helper_alloc_info() as > part of unifying the naming within fbdev helpers. Adapt drivers. No > functional changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 12/21] drm/fb_helper: Rename field fbdev to info in struct drm_fb_helper
On 10/24/22 13:19, Thomas Zimmermann wrote: > Rename struct drm_fb_helper.fbdev to info. The current name is > misleading as it overlaps with generic fbdev naming conventions. > Adapt to the usual naming in fbdev drivers by calling the field > 'info'. No functional changes. > > Signed-off-by: Thomas Zimmermann > --- Agreed. I got confused by this naming in the past. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 11/21] drm/fb-helper: Cleanup include statements in header file
On 10/24/22 13:19, Thomas Zimmermann wrote: > Only include what we have to. > > Signed-off-by: Thomas Zimmermann > --- Nice cleanup. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 10/21] drm/tve200: Include
On 10/24/22 13:19, Thomas Zimmermann wrote: > Include for of_match_ptr(). > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 09/21] drm/panel-ili9341: Include
On 10/24/22 13:19, Thomas Zimmermann wrote: > Include for devm_of_find_backlight(). > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 08/21] drm/rockchip: Don't set struct drm_driver.output_poll_changed
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.output_poll_changed. It's used to restore > the fbdev console. But as rockchip uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See the > functions drm_kms_helper_hotplug_event() and > drm_kms_helper_connector_hotplug_event() in drm_probe_helper.c. > > v2: > * fix commit description (Christian) > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 07/21] drm/logicvc: Don't set struct drm_driver.output_poll_changed
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.output_poll_changed. It's used to restore > the fbdev console. But as logicvc uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See the > functions drm_kms_helper_hotplug_event() and > drm_kms_helper_connector_hotplug_event() in drm_probe_helper.c. > > v2: > * fix commit description (Christian) > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 06/21] drm/ingenic: Don't set struct drm_driver.output_poll_changed
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.output_poll_changed. It's used to restore > the fbdev console. But as ingenic uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See the > functions drm_kms_helper_hotplug_event() and > drm_kms_helper_connector_hotplug_event() in drm_probe_helper.c. > > v2: > * fix commit description (Christian, Sergey) > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 05/21] drm/imx/dcss: Don't set struct drm_driver.output_poll_changed
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.output_poll_changed. It's used to restore > the fbdev console. But as DCSS uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See the > functions drm_kms_helper_hotplug_event() and > drm_kms_helper_connector_hotplug_event() in drm_probe_helper.c. > > v2: > * fix commit description (Christian) > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 04/21] drm/amdgpu: Don't set struct drm_driver.output_poll_changed
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.output_poll_changed. It's used to restore > the fbdev console. But as amdgpu uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See the > functions drm_kms_helper_hotplug_event() and > drm_kms_helper_connector_hotplug_event() in drm_probe_helper.c. > > v2: > * fix commit description (Christian) > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 04/21] drm/amdgpu: Don't set struct drm_driver.output_poll_changed
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.output_poll_changed. It's used to restore > the fbdev console. But as amdgpu uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See the > functions drm_kms_helper_hotplug_event() and > drm_kms_helper_connector_hotplug_event() in drm_probe_helper.c. > > v2: > * fix commit description (Christian) > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas Do you think that the fbdev helpers kernel doc has to be updated to mention that drm_fb_helper_lastclose() and drm_fb_helper_output_poll_changed() are not needed when generic fbdev emulation is used? Because by reading that is not clear that's the case: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_fb_helper.c#L86 -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 03/21] drm/vboxvideo: Don't set struct drm_driver.lastclose
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.lastclose. It's used to restore the > fbdev console. But as vboxvideo uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See > the call to drm_client_dev_restore() in drm_lastclose(). > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 02/21] drm/mcde: Don't set struct drm_driver.lastclose
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.lastclose. It's used to restore the > fbdev console. But as mcde uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See > the call to drm_client_dev_restore() in drm_lastclose(). > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 01/21] drm/komeda: Don't set struct drm_driver.lastclose
On 10/24/22 13:19, Thomas Zimmermann wrote: > Don't set struct drm_driver.lastclose. It's used to restore the > fbdev console. But as komeda uses generic fbdev emulation, the > console is being restored by the DRM client helpers already. See > the call to drm_client_dev_restore() in drm_lastclose(). > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 13/33] drm/client: Add some tests for drm_connector_pick_cmdline_mode()
On 9/23/22 12:30, Thomas Zimmermann wrote:
[...]
>>>> +
>>>> +#ifdef CONFIG_DRM_KUNIT_TEST
>>>> +#include "tests/drm_client_modeset_test.c"
>>>> +#endif
>>>
>>> I strongly dislike this style of including source files in each other.
>>> It's a recipe for all kind of build errors. Can you do something else?
>>>
>>
>> This seems to be the convention used to test static functions and what's
>> documented in the Kunit docs:
>> https://kunit.dev/third_party/kernel/docs/tips.html#testing-static-functions
>
> That document says "...one option is to conditionally #include the test
> file...". This doesn't sound like a strong requirement.
>
That's true.
>>
>> I agree with you that's not ideal but I think that consistency with how
>> it is done by other subsystems is also important.
>>
>>> As the tested function is an internal interface, maybe export a wrapper
>>> if tests are enabled, like this:
>>>
>>> #ifdef CONFIG_DRM_KUNIT_TEST
>>> struct drm_display_mode *
>>> drm_connector_pick_cmdline_mode_kunit(drm_conenctor)
>>> {
>>> return drm_connector_pick_cmdline_mode(connector)
>>> }
>>> EXPORT_SYMBOL(drm_connector_pick_cmdline_mode_kunit)
>>> #endif
>>>
>>> The wrapper's declaration can be located in the kunit test file.
>>>
>>
>> But that's also not nice since we are artificially exposing these only
>> to allow the static functions to be called from unit tests. And would
>> be a different approach than the one used by all other subsystems...
>>
>
> There's the problem of interference between the source files when
> building the code. It's also not the same source code after including
> the test file. At a minimum, including the tests' source file further
> includes more files. also includes quite a few of Linux
> header files.
>
> IMHO the current convention (if any) is far from optimal and we should
> consider breaking it.
>
Yes, I agree with that. But probably we should be explicit about the
wrapper export symbols to access static functions pattern in the KUnit
docs so other subsystems could do the same and it becomes a convention ?
> Best regards
> Thomas
>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Nouveau] [PATCH v2 13/33] drm/client: Add some tests for drm_connector_pick_cmdline_mode()
On 9/23/22 11:15, Thomas Zimmermann wrote:
> Hi
>
> Am 22.09.22 um 16:25 schrieb Maxime Ripard:
>> drm_connector_pick_cmdline_mode() is in charge of finding a proper
>> drm_display_mode from the definition we got in the video= command line
>> argument.
>>
>> Let's add some unit tests to make sure we're not getting any regressions
>> there.
>>
>> Signed-off-by: Maxime Ripard
>>
>> diff --git a/drivers/gpu/drm/drm_client_modeset.c
>> b/drivers/gpu/drm/drm_client_modeset.c
>> index bbc535cc50dd..d553e793e673 100644
>> --- a/drivers/gpu/drm/drm_client_modeset.c
>> +++ b/drivers/gpu/drm/drm_client_modeset.c
>> @@ -1237,3 +1237,7 @@ int drm_client_modeset_dpms(struct drm_client_dev
>> *client, int mode)
>> return ret;
>> }
>> EXPORT_SYMBOL(drm_client_modeset_dpms);
>> +
>> +#ifdef CONFIG_DRM_KUNIT_TEST
>> +#include "tests/drm_client_modeset_test.c"
>> +#endif
>
> I strongly dislike this style of including source files in each other.
> It's a recipe for all kind of build errors. Can you do something else?
>
This seems to be the convention used to test static functions and what's
documented in the Kunit docs:
https://kunit.dev/third_party/kernel/docs/tips.html#testing-static-functions
I agree with you that's not ideal but I think that consistency with how
it is done by other subsystems is also important.
> As the tested function is an internal interface, maybe export a wrapper
> if tests are enabled, like this:
>
> #ifdef CONFIG_DRM_KUNIT_TEST
> struct drm_display_mode *
> drm_connector_pick_cmdline_mode_kunit(drm_conenctor)
> {
>return drm_connector_pick_cmdline_mode(connector)
> }
> EXPORT_SYMBOL(drm_connector_pick_cmdline_mode_kunit)
> #endif
>
> The wrapper's declaration can be located in the kunit test file.
>
But that's also not nice since we are artificially exposing these only
to allow the static functions to be called from unit tests. And would
be a different approach than the one used by all other subsystems...
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Nouveau] [PATCH 4/4] drm/plane-helper: Provide DRM_PLANE_NON_ATOMIC_FUNCS initializer macro
On 9/9/22 12:59, Thomas Zimmermann wrote: > Provide DRM_PLANE_NON_ATOMIC_FUNCS, which initializes plane functions > of non-atomic drivers to default values. The macro is not supposed to > be used in new code, but helps with documenting and finding existing > users. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH 1/4] drm/plane: Remove drm_plane_init()
Hello Thomas, On 9/9/22 12:59, Thomas Zimmermann wrote: > Open-code drm_plane_init() and remove the function from DRM. The > implementation of drm_plane_init() is a simple wrapper around a call > to drm_universal_plane_init(), so drivers can just use that instead. > > Signed-off-by: Thomas Zimmermann > --- [...] > diff --git a/drivers/gpu/drm/nouveau/dispnv04/overlay.c > b/drivers/gpu/drm/nouveau/dispnv04/overlay.c > index 37e63e98cd08..33f29736024a 100644 > --- a/drivers/gpu/drm/nouveau/dispnv04/overlay.c > +++ b/drivers/gpu/drm/nouveau/dispnv04/overlay.c > @@ -296,9 +296,10 @@ nv10_overlay_init(struct drm_device *device) > break; > } > > - ret = drm_plane_init(device, &plane->base, 3 /* both crtc's */, > - &nv10_plane_funcs, > - formats, num_formats, false); > + ret = drm_universal_plane_init(device, &plane->base, 3 /* both crtc's > */, > +&nv10_plane_funcs, > +formats, num_formats, NULL, > +DRM_PLANE_TYPE_OVERLAY, NULL); Not only drm_plane_init() doesn't add much value but makes the code harder to read. Since by calling drm_universal_plane_init() instead, it's explicit whether the initialized plane is primary or an overlay. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()
On 9/16/22 13:41, Thomas Zimmermann wrote:
[...]
>>
>>> + * @dev: DRM device
>>> + * @type: the type of the struct which contains struct &drm_plane
>>> + * @member: the name of the &drm_plane within @type
>>> + * @possible_crtcs: bitmask of possible CRTCs
>>> + * @funcs: callbacks for the new plane
>>> + * @formats: array of supported formats (DRM_FORMAT\_\*)
>>> + * @format_count: number of elements in @formats
>>> + * @format_modifiers: array of struct drm_format modifiers terminated by
>>> + *DRM_FORMAT_MOD_INVALID
>>> + * @plane_type: type of plane (overlay, primary, cursor)
>>> + * @name: printf style format string for the plane name, or NULL for
>>> default name
>>> + *
>>> + * Allocates and initializes a plane object of type @type. The caller
>>> + * is responsible for releasing the allocated memory with kfree().
>>> + *
>>
>> I thought that all the drmm_*_alloc() managed interfaces should use the
>> drmm_k{z,m}alloc() helpers, so that the memory is automatically freed
>> on the last drm_dev_put() call ?
>
> For new drivers, managed cleanup is preferred. But this is an existing
> unmanaged cleanup.
>
> I don't know if drmm_ changes the semantics in unexpected ways (well,
> probably not). With managed memory releases, I was worried that plane
> memory might stay around longer than expected. And we'd have to fix the
> plane-destroy function in each user as well.
>
> Adding the existing code as a new function is the trivial solution and
> allows to address each driver individually. Also see my reply to
> Laurent's question on that topic.
>
Ah, never mind. I misread the function name definition and thought that
you had a drmm_ suffix but, now on second read I see that is only drm_
so just ignore my comment about using managed memory alloc / release.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Nouveau] [PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()
On 9/9/22 12:59, Thomas Zimmermann wrote:
> Provide drm_univeral_plane_alloc(), which allocated an initializes a
> plane. Code for non-atomic drivers uses this pattern. Convert it to
> the new function. The modeset helpers contain a quirk for handling their
> color formats differently. Set the flag outside plane allocation.
>
> The new function is already deprecated to some extend. Drivers should
> rather use drmm_univeral_plane_alloc() or drm_universal_plane_init().
>
> Signed-off-by: Thomas Zimmermann
> ---
[...]
>
> +__printf(10, 11)
> +void *__drm_universal_plane_alloc(struct drm_device *dev,
> + size_t size, size_t offset,
> + uint32_t possible_crtcs,
> + const struct drm_plane_funcs *funcs,
> + const uint32_t *formats,
> + unsigned int format_count,
> + const uint64_t *format_modifiers,
> + enum drm_plane_type plane_type,
> + const char *name, ...);
> +
> +/**
> + * drm_universal_plane_alloc - Allocate and initialize an universal plane
> object
Should functions kernel-doc definitions use parenthesis or not? I see in
https://elixir.bootlin.com/linux/latest/source/Documentation/doc-guide/kernel-doc.rst#L59
that the example has it but notice that there is not consistency in DRM
about this.
> + * @dev: DRM device
> + * @type: the type of the struct which contains struct &drm_plane
> + * @member: the name of the &drm_plane within @type
> + * @possible_crtcs: bitmask of possible CRTCs
> + * @funcs: callbacks for the new plane
> + * @formats: array of supported formats (DRM_FORMAT\_\*)
> + * @format_count: number of elements in @formats
> + * @format_modifiers: array of struct drm_format modifiers terminated by
> + *DRM_FORMAT_MOD_INVALID
> + * @plane_type: type of plane (overlay, primary, cursor)
> + * @name: printf style format string for the plane name, or NULL for default
> name
> + *
> + * Allocates and initializes a plane object of type @type. The caller
> + * is responsible for releasing the allocated memory with kfree().
> + *
I thought that all the drmm_*_alloc() managed interfaces should use the
drmm_k{z,m}alloc() helpers, so that the memory is automatically freed
on the last drm_dev_put() call ?
Other than those two nits, the patch looks good to me.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Nouveau] [PATCH 3/4] drm/plane-helper: Warn if atomic drivers call non-atomic helpers
On 9/9/22 12:59, Thomas Zimmermann wrote: > The plane update and disable helpers are only useful for non-atomic > drivers. Print a warning if an atomic driver calls them. > > Suggested-by: Daniel Vetter > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Nouveau] [PATCH v2 3/5] drm/dp: Move DisplayPort helpers into separate helper module
On Wed, Dec 15, 2021 at 12:12 PM Thomas Zimmermann wrote: > > Hi > > Am 15.12.21 um 12:04 schrieb Jani Nikula: > > On Wed, 15 Dec 2021, Thomas Zimmermann wrote: > >> * move DP helper code into dp/ (Jani) > > > > I suggested adding the subdirectory, but I'm going to bikeshed the name, > > which I didn't suggest. > > > > $ find drivers/gpu/drm -mindepth 1 -maxdepth 1 -type d | wc -l > > 68 > > > > Assuming we move more of the drm modules to subdirectories, how are they > > going to stand out from drivers? > > > > I suggested drm_dp, which I understand results in tautology, but hey, > > all the filenames under drm/ also have drm_*.[ch]. And I find that very > > useful for git greps and other code archeology. With just the dp name, > > you'd have to know and list all the drm subdirectories when looking up > > stuff that's part of drm but not drivers. > > I think we have enough filename prefixes already. drm/drm_dp/drm_dp_ is > just ridiculous. > Maybe what can be done is to just add a drivers/gpu/drm/core subdirectory that would contain all the DRM core code ? Then the dp helpers could be moved to drivers/gpu/drm/core/dp/drm_dp.c for example. This would also make easy to differentiate the drm modules from the drivers with just: $ find drivers/gpu/drm -mindepth 1 -maxdepth 1 -type d -not -name core Best regards, Javier
Re: [Nouveau] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem
On 11/5/21 10:00, Thomas Zimmermann wrote:
[snip]
>> +
>> +static int __init disable_modeset(char *str)
>> +{
>> +drm_nomodeset = true;
>> +
>> +pr_warn("You have booted with nomodeset. This means your GPU drivers
>> are DISABLED\n");
>> +pr_warn("Any video related functionality will be severely degraded, and
>> you may not even be able to suspend the system properly\n");
>> +pr_warn("Unless you actually understand what nomodeset does, you should
>> reboot without enabling it\n");
>
> I'd update this text to be less sensational.
>
This is indeed quite sensational. But think we can do it as a follow-up patch.
Since we will have to stick with nomodeset for backward compatibility, I was
planning to add documentation to explain what this parameter is about and what
is the actual effect of setting it.
So I think we can change this as a part of that patch-set.
>> +
>> +return 1;
>> +}
>> +
>> +/* Disable kernel modesetting */
>> +__setup("nomodeset", disable_modeset);
>> diff --git a/drivers/gpu/drm/i915/i915_module.c
>> b/drivers/gpu/drm/i915/i915_module.c
>> index 45cb3e540eff..c890c1ca20c4 100644
>> --- a/drivers/gpu/drm/i915/i915_module.c
>> +++ b/drivers/gpu/drm/i915/i915_module.c
>> @@ -4,8 +4,6 @@
>>* Copyright © 2021 Intel Corporation
>>*/
>>
>> -#include
>> -
>
> These changes should be in patch 1?
>
Yes, I forgot to move these when changed the order of the patches.
Best regards, --
Javier Martinez Canillas
Linux Engineering
Red Hat
[Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
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
---
Changes in v2:
- Squash patch to add drm_drv_enabled() and make drivers use it.
- Make the drivers changes before moving nomodeset logic to DRM.
- Make drm_drv_enabled() return an errno and -ENODEV if nomodeset.
- Remove debug and error messages in drivers.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 7 +++
drivers/gpu/drm/ast/ast_drv.c | 7 +--
drivers/gpu/drm/drm_drv.c | 20
drivers/gpu/drm/i915/i915_module.c | 6 +-
drivers/gpu/drm/mgag200/mgag200_drv.c | 7 +--
drivers/gpu/drm/nouveau/nouveau_drm.c | 5 -
drivers/gpu/drm/qxl/qxl_drv.c | 7 +--
drivers/gpu/drm/radeon/radeon_drv.c | 6 --
drivers/gpu/drm/tiny/bochs.c| 7 +--
drivers/gpu/drm/tiny/cirrus.c | 8 ++--
drivers/gpu/drm/vboxvideo/vbox_drv.c| 9 +
drivers/gpu/drm/virtio/virtgpu_drv.c| 5 +++--
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 5 +++--
include/drm/drm_drv.h | 1 +
14 files changed, 74 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index c718fb5f3f8a..7fde40d06181 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2514,10 +2514,9 @@ static int __init amdgpu_init(void)
{
int r;
- if (vgacon_text_force()) {
- DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
- return -EINVAL;
- }
+ r = drm_drv_enabled(&amdgpu_kms_driver)
+ if (r)
+ return r;
r = amdgpu_sync_init();
if (r)
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 86d5cd7b6318..802063279b86 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -233,8 +233,11 @@ static struct pci_driver ast_pci_driver = {
static int __init ast_init(void)
{
- if (vgacon_text_force() && ast_modeset == -1)
- return -EINVAL;
+ int ret;
+
+ ret = drm_drv_enabled(&ast_driver);
+ if (ret && ast_modeset == -1)
+ return ret;
if (ast_modeset == 0)
return -EINVAL;
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);
+ 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;
+
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(&driver);
+ if (ret && i915_modparams.modeset == -1)
use_kms = false;
if (!use_kms) {
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 6b9243713b3c..2a581094ba2b 100644
--- a/drivers/gpu/drm/mgag
Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hello Sam,
On 11/4/21 18:57, Jani Nikula wrote:
> On Thu, 04 Nov 2021, Sam Ravnborg wrote:
>> 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?
>>
Thanks, I didn't know that. Right, they had an error but I do wonder
if that was correct though. After all isn't an error but an explicit
disable due "nomodeset" being set in the kernel command line.
[snip]
>>>
>>> - if (vgacon_text_force() && i915_modparams.modeset == -1)
>>> + ret = drm_drv_enabled(&driver);
>>
>> 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;
static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
{
@@ -1777,7 +1777,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
DRM_IOCTL_DEF_DRV(I915_GEM_VM_DESTROY, i915_gem_vm_destroy_ioctl,
DRM_RENDER_ALLOW),
};
-static const struct drm_driver driver = {
+const struct drm_driver driver = {
/* Don't use MTRRs here; the Xserver or userspace app should
* deal with them for Intel hardware.
*/
diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
index c890c1ca20c4..88f770920324 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -16,7 +16,7 @@
#include "i915_selftest.h"
#include "i915_vma.h"
-static const struct drm_driver driver;
+extern const struct drm_driver driver;
static int i915_check_nomodeset(void)
{
That should work and I actually got a laptop with an i915 and tested the change
both when CONFIG_DRM_I915=m and CONFIG_DRM_I915=y is set.
Another option is to declare it in i915_drv.h and not make the definition
static.
> Indeed.
>
>> 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?
>
> We don't really want that, though. This check is pretty much as early as
> it can be, and there's a ton of useless initialization that would happen
> if we waited until drm_driver is available.
>
Agreed.
> From my POV, drm_drv_enabled() is a solution that creates a worse
> problem for us than it solves.
>
I don't have a strong opinion on this. I could just do patch #2 without adding
a level of indirection through drm_drv_enabled(). But Thomas and Daniel Vetter
suggested that we should do this change before.
That is, the drivers could just check if should be enabled by calling to the
drm_check_modeset() function directly if people agree that encapsulating that
logic in a drm_drv_enabled() is not needed.
>
> BR,
> Jani.
>
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
[Nouveau] [RESEND PATCH 3/5] drm: Rename vgacon_text_force() function to drm_modeset_disabled()
This function is used by some DRM drivers to determine if the "nomodeset"
kernel command line parameter was set and prevent these drivers to probe.
But the function name is quite confusing and does not reflect what really
the drivers are testing when calling it. Use a better naming now that it
is part of the DRM subsystem.
Also, vgacon_text_force() is guarded by #ifdef CONFIG_VGA_CONSOLE already
so there is no need to do the same when calling the function.
Suggested-by: Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
---
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
drivers/gpu/drm/ast/ast_drv.c | 2 +-
drivers/gpu/drm/drm_nomodeset.c | 16
drivers/gpu/drm/i915/i915_module.c | 2 +-
drivers/gpu/drm/mgag200/mgag200_drv.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_drm.c | 2 +-
drivers/gpu/drm/qxl/qxl_drv.c | 2 +-
drivers/gpu/drm/radeon/radeon_drv.c | 2 +-
drivers/gpu/drm/tiny/bochs.c| 2 +-
drivers/gpu/drm/tiny/cirrus.c | 2 +-
drivers/gpu/drm/vboxvideo/vbox_drv.c| 4 +---
drivers/gpu/drm/virtio/virtgpu_drv.c| 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 2 +-
include/drm/drm_mode_config.h | 4 ++--
14 files changed, 22 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 2680a2aaa877..f7bd2616cf23 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2513,7 +2513,7 @@ static int __init amdgpu_init(void)
{
int r;
- if (vgacon_text_force()) {
+ if (drm_modeset_disabled()) {
DRM_ERROR("amdgpu kernel modesetting disabled.\n");
return -EINVAL;
}
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 048be607b182..6706050414c3 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -232,7 +232,7 @@ static struct pci_driver ast_pci_driver = {
static int __init ast_init(void)
{
- if (vgacon_text_force() && ast_modeset == -1)
+ if (drm_modeset_disabled() && ast_modeset == -1)
return -EINVAL;
if (ast_modeset == 0)
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
index 1ac9a8d5a8fe..dfc8b30f0625 100644
--- a/drivers/gpu/drm/drm_nomodeset.c
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -3,17 +3,17 @@
#include
#include
-static bool vgacon_text_mode_force;
+static bool drm_nomodeset;
-bool vgacon_text_force(void)
+bool drm_modeset_disabled(void)
{
- return vgacon_text_mode_force;
+ return drm_nomodeset;
}
-EXPORT_SYMBOL(vgacon_text_force);
+EXPORT_SYMBOL(drm_modeset_disabled);
-static int __init text_mode(char *str)
+static int __init disable_modeset(char *str)
{
- vgacon_text_mode_force = true;
+ drm_nomodeset = true;
pr_warn("You have booted with nomodeset. This means your GPU drivers
are DISABLED\n");
pr_warn("Any video related functionality will be severely degraded, and
you may not even be able to suspend the system properly\n");
@@ -22,5 +22,5 @@ static int __init text_mode(char *str)
return 1;
}
-/* force text mode - used by kernel modesetting */
-__setup("nomodeset", text_mode);
+/* Disable kernel modesetting */
+__setup("nomodeset", disable_modeset);
diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
index 14a59226519d..3e5531040e4d 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -29,7 +29,7 @@ static int i915_check_nomodeset(void)
if (i915_modparams.modeset == 0)
use_kms = false;
- if (vgacon_text_force() && i915_modparams.modeset == -1)
+ if (drm_modeset_disabled() && i915_modparams.modeset == -1)
use_kms = false;
if (!use_kms) {
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 685e766db6a4..7ee87564bade 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -377,7 +377,7 @@ static struct pci_driver mgag200_pci_driver = {
static int __init mgag200_init(void)
{
- if (vgacon_text_force() && mgag200_modeset == -1)
+ if (drm_modeset_disabled() && mgag200_modeset == -1)
return -EINVAL;
if (mgag200_modeset == 0)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 029997f50d1a..903d0e626954 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1321,7 +1321,7 @@ nouveau_drm_init(void)
nouveau_display_options();
if (nouveau_modeset == -1) {
- if (vgacon_text_force())
+ if (drm_modeset_disab
[Nouveau] [RESEND PATCH 0/5] Cleanups for the nomodeset kernel command line parameter logic
[ resend with all relevant people as Cc now, sorry to others for the spam ] There is a lot of historical baggage on this parameter. It's defined in the vgacon driver as a "nomodeset" parameter, but it's handler function is called text_mode() that sets a variable named vgacon_text_mode_force whose value is queried with a function named vgacon_text_force(). All this implies that it's about forcing text mode for VGA, yet it is not used in neither vgacon nor other console driver. The only users for these are DRM drivers, that check for the vgacon_text_force() return value to determine whether the driver could be loaded or not. That makes it quite confusing to read the code, because the variables and function names don't reflect what they actually do and also are not in the same subsystem as the drivers that make use of them. This patch-set attempts to cleanup the code by moving the nomodseset param to the DRM subsystem and do some renaming to make their intention clearer. There is also another aspect that could be improved, and is the fact that drivers are checking for the nomodeset being set as an indication if have to be loaded. But there may be other reasons why this could be the case, so it is better to encapsulate the logic in a separate function to make clear what's about. Patch #1 is just a trivial fix for a comment that isn't referring to the correct kernel parameter. Patch #2 moves the nomodeset logic to the DRM subsystem. Patch #3 renames the vgacon_text_force() function and accompaning logic as drm_modeset_disabled(), which is what this function is really about. Patch #4 adds a drm_drv_enabled() function that could be used by drivers to check if could be enabled. Patch #5 uses the drm_drv_enabled() function to check this instead of just checking if nomodeset has been set. Javier Martinez Canillas (5): drm/i915: Fix comment about modeset parameters drm: Move nomodeset kernel parameter handler to the DRM subsystem drm: Rename vgacon_text_force() function to drm_modeset_disabled() drm: Add a drm_drv_enabled() helper function drm: Use drm_drv_enabled() instead of drm_modeset_disabled() drivers/gpu/drm/Makefile| 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 5 ++--- drivers/gpu/drm/ast/ast_drv.c | 3 +-- drivers/gpu/drm/drm_drv.c | 21 drivers/gpu/drm/drm_nomodeset.c | 26 + drivers/gpu/drm/i915/i915_module.c | 10 +- drivers/gpu/drm/mgag200/mgag200_drv.c | 3 +-- drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +-- drivers/gpu/drm/qxl/qxl_drv.c | 3 +-- drivers/gpu/drm/radeon/radeon_drv.c | 3 +-- drivers/gpu/drm/tiny/bochs.c| 3 +-- drivers/gpu/drm/tiny/cirrus.c | 3 +-- drivers/gpu/drm/vboxvideo/vbox_drv.c| 5 + drivers/gpu/drm/virtio/virtgpu_drv.c| 3 +-- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 3 +-- drivers/video/console/vgacon.c | 21 include/drm/drm_drv.h | 1 + include/drm/drm_mode_config.h | 6 ++ include/linux/console.h | 6 -- 19 files changed, 73 insertions(+), 57 deletions(-) create mode 100644 drivers/gpu/drm/drm_nomodeset.c -- 2.33.1
Re: [Nouveau] [RESEND PATCH 0/5] Cleanups for the nomodeset kernel command line parameter logic
Hello Thomas, On 11/3/21 14:01, Thomas Zimmermann wrote: [snip] >> >> >> Javier Martinez Canillas (5): >>drm/i915: Fix comment about modeset parameters >>drm: Move nomodeset kernel parameter handler to the DRM subsystem >>drm: Rename vgacon_text_force() function to drm_modeset_disabled() >>drm: Add a drm_drv_enabled() helper function >>drm: Use drm_drv_enabled() instead of drm_modeset_disabled() > > There's too much churn here IMHO. Please merge patches 2+3 and 4+5. And > I'd put patch (4+5) first, so you have the drivers out of the way. After > that patch (2+3) should only modify drm_drv_enabled(). > Sure, I'm happy with less patches. Thanks for your feedback. > Best regards > Thomas > Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
[Nouveau] [PATCH v2 0/2] Cleanups for the nomodeset kernel command line parameter logic
There is a lot of historical baggage on this parameter. It is defined in the vgacon driver as nomodeset, but its set function is called text_mode() and the value queried with a function named vgacon_text_force(). All this implies that it's about forcing text mode for VGA, yet it is not used in neither vgacon nor other console driver. The only users for these are DRM drivers, that check for the vgacon_text_force() return value to determine whether the driver should be loaded or not. That makes it quite confusing to read the code, because the variables and function names don't reflect what they actually do and also are not in the same subsystem as the drivers that make use of them. This patch-set attempts to cleanup the code by moving the nomodseset param to the DRM subsystem and do some renaming to make their intention clearer. There is also another aspect that could be improved, and is the fact that drivers are checking for the nomodeset being set as an indication if have to be loaded. But there may be other reasons why this could be the case, so it is better to encapsulate the logic in a separate function to make clear what's about. This is a v2 of the patches, that address the issues pointed out by Thomas Zimmermann and Jani Nikula in v1: https://lore.kernel.org/lkml/5b4e4534-4786-d231-e331-78fdb5d84...@redhat.com/T/ Patch #1 adds a drm_drv_enabled() function that could be used by drivers to check if these could be enabled, instead of just using vgacon_text_force(). Patch #2 moves the nomodeset logic to the DRM subsystem and renames the functions and variables to better explain what these actually do. Changes in v2: - Squash patch to add drm_drv_enabled() and make drivers use it. - Make the drivers changes before moving nomodeset logic to DRM. - Make drm_drv_enabled() return an errno and -ENODEV if nomodeset. - Remove debug and error messages in drivers. - Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set. - Squash patches to move nomodeset logic to DRM and do the renaming. - Name the function drm_check_modeset() and make it return -ENODEV. Javier Martinez Canillas (2): drm: Add a drm_drv_enabled() to check if drivers should be enabled drm: Move nomodeset kernel parameter to the DRM subsystem drivers/gpu/drm/Makefile| 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 8 +++- drivers/gpu/drm/ast/ast_drv.c | 8 +--- drivers/gpu/drm/drm_drv.c | 21 drivers/gpu/drm/drm_nomodeset.c | 26 + drivers/gpu/drm/i915/i915_module.c | 8 +--- drivers/gpu/drm/mgag200/mgag200_drv.c | 8 +--- drivers/gpu/drm/nouveau/nouveau_drm.c | 6 -- drivers/gpu/drm/qxl/qxl_drv.c | 8 +--- drivers/gpu/drm/radeon/radeon_drv.c | 7 --- drivers/gpu/drm/tiny/bochs.c| 8 +--- drivers/gpu/drm/tiny/cirrus.c | 9 ++--- drivers/gpu/drm/vboxvideo/vbox_drv.c| 10 +- drivers/gpu/drm/virtio/virtgpu_drv.c| 6 +++--- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 6 +++--- drivers/video/console/vgacon.c | 21 include/drm/drm_drv.h | 1 + include/drm/drm_mode_config.h | 6 ++ include/linux/console.h | 6 -- 19 files changed, 109 insertions(+), 66 deletions(-) create mode 100644 drivers/gpu/drm/drm_nomodeset.c -- 2.33.1
Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hello Thomas,
On 11/5/21 09:43, Thomas Zimmermann wrote:
> Hi
>
> Am 04.11.21 um 21:09 schrieb Javier Martinez Canillas:
>> Hello Jani,
>>
>> On 11/4/21 20:57, Jani Nikula wrote:
>>> On Thu, 04 Nov 2021, Javier Martinez Canillas wrote:
>>>> +/**
>>>> + * 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)
>
> Jani mentioned that i915 absolutely wants this to run from the
> module_init function. Best is to drop the parameter.
>
Ok. I now wonder though how much value would add this function since
it will just be a wrapper around the nomodeset check.
We talked about adding a new DRIVER_GENERIC feature flag and check for
this, but as danvet mentioned that is not really needed. We just need
to avoid testing for nomodeset in the simpledrm driver.
Do you envision other condition that could be added later to disable a
DRM driver ? Or do you think that just from a code readability point of
view makes worth it ?
>>>> +{
>>>> + if (vgacon_text_force()) {
>>>> + DRM_INFO("%s driver is disabled\n", driver->name);
>>>> + return -ENODEV;
>>>> + }
>
> If we run this from within a module_init function, we'd get plenty of
> these warnings if drivers are compiled into the kernel. Maybe simply
> remove the message. There's already a warning printed by the nomodeset
> handler.
>
Indeed. I'll just drop it.
>>>> +
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_drv_enabled);
>>>
>>> The name implies a bool return, but it's not.
>>>
>>> if (drm_drv_enabled(...)) {
>>> /* surprise, it's disabled! */
>>> }
>>>
>>
>> It used to return a bool in v2 but Thomas suggested an int instead to
>> have consistency on the errno code that was returned by the callers.
>>
>> I should probably name that function differently to avoid confusion.
>
> Yes, please.
>
drm_driver_check() maybe ?
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
[Nouveau] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem
The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
but the exported vgacon_text_force() symbol is only used by DRM drivers.
It makes much more sense for the parameter logic to be in the subsystem
of the drivers that are making use of it.
Let's move the vgacon_text_force() function and related logic to the DRM
subsystem. While doing that, rename the function to drm_check_modeset()
which better reflects what the function is really used to test for.
Suggested-by: Daniel Vetter
Signed-off-by: Javier Martinez Canillas
---
Changes in v2:
- Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set.
- Squash patches to move nomodeset logic to DRM and do the renaming.
- Name the function drm_check_modeset() and make it return -ENODEV.
drivers/gpu/drm/Makefile| 2 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 -
drivers/gpu/drm/ast/ast_drv.c | 1 -
drivers/gpu/drm/drm_drv.c | 9 +
drivers/gpu/drm/drm_nomodeset.c | 26 +
drivers/gpu/drm/i915/i915_module.c | 2 --
drivers/gpu/drm/mgag200/mgag200_drv.c | 1 -
drivers/gpu/drm/nouveau/nouveau_drm.c | 1 -
drivers/gpu/drm/qxl/qxl_drv.c | 1 -
drivers/gpu/drm/radeon/radeon_drv.c | 1 -
drivers/gpu/drm/tiny/bochs.c| 1 -
drivers/gpu/drm/tiny/cirrus.c | 1 -
drivers/gpu/drm/vboxvideo/vbox_drv.c| 1 -
drivers/gpu/drm/virtio/virtgpu_drv.c| 1 -
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 1 -
drivers/video/console/vgacon.c | 21
include/drm/drm_mode_config.h | 6 ++
include/linux/console.h | 6 --
18 files changed, 39 insertions(+), 44 deletions(-)
create mode 100644 drivers/gpu/drm/drm_nomodeset.c
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1c41156deb5f..c74810c285af 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o
drm_privacy_screen_x86.
obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
+obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
+
drm_cma_helper-y := drm_gem_cma_helper.o
obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 7fde40d06181..b4b6993861e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -31,7 +31,6 @@
#include "amdgpu_drv.h"
#include
-#include
#include
#include
#include
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 802063279b86..6222082c3082 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -26,7 +26,6 @@
* Authors: Dave Airlie
*/
-#include
#include
#include
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 3fb567d62881..80b85b8ea776 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -986,12 +986,13 @@ EXPORT_SYMBOL(drm_dev_set_unique);
*/
int drm_drv_enabled(const struct drm_driver *driver)
{
- if (vgacon_text_force()) {
+ int ret;
+
+ ret = drm_check_modeset();
+ if (ret)
DRM_INFO("%s driver is disabled\n", driver->name);
- return -ENODEV;
- }
- return 0;
+ return ret;
}
EXPORT_SYMBOL(drm_drv_enabled);
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
new file mode 100644
index ..6683e396d2c5
--- /dev/null
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include
+#include
+
+static bool drm_nomodeset;
+
+int drm_check_modeset(void)
+{
+ return drm_nomodeset ? -ENODEV : 0;
+}
+EXPORT_SYMBOL(drm_check_modeset);
+
+static int __init disable_modeset(char *str)
+{
+ drm_nomodeset = true;
+
+ pr_warn("You have booted with nomodeset. This means your GPU drivers
are DISABLED\n");
+ pr_warn("Any video related functionality will be severely degraded, and
you may not even be able to suspend the system properly\n");
+ pr_warn("Unless you actually understand what nomodeset does, you should
reboot without enabling it\n");
+
+ return 1;
+}
+
+/* Disable kernel modesetting */
+__setup("nomodeset", disable_modeset);
diff --git a/drivers/gpu/drm/i915/i915_module.c
b/drivers/gpu/drm/i915/i915_module.c
index 45cb3e540eff..c890c1ca20c4 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -4,8 +4,6 @@
* Copyright © 2021 Intel Corporation
*/
-#include
-
#include "gem/i915_gem_context.h"
#include "gem/i915_gem_object.h"
#include "i915_active.h"
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 2a581094ba2b.
Re: [Nouveau] [RESEND PATCH 3/5] drm: Rename vgacon_text_force() function to drm_modeset_disabled()
On 11/3/21 13:57, Thomas Zimmermann wrote:
[snip]
>>
>> -if (vgacon_text_force()) {
>> +if (drm_modeset_disabled()) {
>> DRM_ERROR("amdgpu kernel modesetting disabled.\n");
>
> Please remove all such error messages from drivers.
> drm_modeset_disabled() should print a unified message instead.
>
Agreed.
>> -static bool vgacon_text_mode_force;
>> +static bool drm_nomodeset;
>>
>> -bool vgacon_text_force(void)
>> +bool drm_modeset_disabled(void)
>
> I suggest to rename this function to drm_check_modeset() and have it
> return a negative errno code on failure. This gives maximum flexibility
> and reduces errors in drivers. Right now the drivers return something
> like -EINVAL, which seems wrong. Returning -ENODEV seems more appropriate.
>
Good idea. I'll do it in v2 as well.
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
Hello Jani,
On 11/4/21 20:57, Jani Nikula wrote:
> On Thu, 04 Nov 2021, Javier Martinez Canillas wrote:
>> +/**
>> + * 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);
>> +return -ENODEV;
>> +}
>> +
>> +return 0;
>> +}
>> +EXPORT_SYMBOL(drm_drv_enabled);
>
> The name implies a bool return, but it's not.
>
> if (drm_drv_enabled(...)) {
> /* surprise, it's disabled! */
> }
>
It used to return a bool in v2 but Thomas suggested an int instead to
have consistency on the errno code that was returned by the callers.
I should probably name that function differently to avoid confusion.
But I think you are correct and this change is caused too much churn
for not that much benefit, specially since is unclear that there might
be another condition to prevent a DRM driver to load besides nomodeset.
I'll just drop this patch and post only #2 but making drivers to test
using the drm_check_modeset() function (which doesn't have a name that
implies a bool return).
>
> BR,
> Jani.
>
>
>
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
On 11/5/21 11:04, Jani Nikula wrote: > On Fri, 05 Nov 2021, Javier Martinez Canillas wrote: [snip] >> >> Do you envision other condition that could be added later to disable a >> DRM driver ? Or do you think that just from a code readability point of >> view makes worth it ? > > Taking a step back for perspective. > > I think there's broad consensus in moving the parameter to drm, naming > the check function to drm_something_something(), and breaking the ties > to CONFIG_VGA_CONSOLE. I appreciate the work you're doing to that > effect. > Thanks, I appreciate your feedback and comments. > I think everything beyond that is still a bit vague and/or > contentious. So how about making the first 2-3 patches just that? > Something we can all agree on, makes good progress, improves the kernel, > and gives us something to build on? > That works for me. Thomas, do you agree with that approach ? Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
