Re: [Spice-devel] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
> Hello Jani,
>
> On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
>> On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
>> > Hello,
>> >
>> > while I debugged an issue in the imx-lcdc driver I was constantly
>> > irritated about struct drm_device pointer variables being named "dev"
>> > because with that name I usually expect a struct device pointer.
>> >
>> > I think there is a big benefit when these are all renamed to "drm_dev".
>> > I have no strong preference here though, so "drmdev" or "drm" are fine
>> > for me, too. Let the bikesheding begin!
>> >
>> > Some statistics:
>> >
>> > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq
>> > -c | sort -n
>> > 1 struct drm_device *adev_to_drm
>> > 1 struct drm_device *drm_
>> > 1 struct drm_device *drm_dev
>> > 1 struct drm_device*drm_dev
>> > 1 struct drm_device *pdev
>> > 1 struct drm_device *rdev
>> > 1 struct drm_device *vdev
>> > 2 struct drm_device *dcss_drv_dev_to_drm
>> > 2 struct drm_device **ddev
>> > 2 struct drm_device *drm_dev_alloc
>> > 2 struct drm_device *mock
>> > 2 struct drm_device *p_ddev
>> > 5 struct drm_device *device
>> > 9 struct drm_device * dev
>> > 25 struct drm_device *d
>> > 95 struct drm_device *
>> > 216 struct drm_device *ddev
>> > 234 struct drm_device *drm_dev
>> > 611 struct drm_device *drm
>> >4190 struct drm_device *dev
>> >
>> > This series starts with renaming struct drm_crtc::dev to drm_dev. If
>> > it's not only me and others like the result of this effort it should be
>> > followed up by adapting the other structs and the individual usages in
>> > the different drivers.
>>
>> I think this is an unnecessary change. In drm, a dev is usually a drm
>> device, i.e. struct drm_device *.
>
> Well, unless it's not. Prominently there is
>
> struct drm_device {
> ...
> struct device *dev;
> ...
> };
>
> which yields quite a few code locations using dev->dev which is
> IMHO unnecessary irritating:
>
> $ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
> 1633
>
> Also the functions that deal with both a struct device and a struct
> drm_device often use "dev" for the struct device and then "ddev" for
> the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
Why is specifically struct drm_device *dev so irritating to you?
You lead us to believe it's an outlier in kernel, something that goes
against common kernel style, but it's really not:
$ git grep -how "struct [A-Za-z0-9_]\+ \*dev" | sort | uniq -c | sort -rn |
head -20
38494 struct device *dev
16388 struct net_device *dev
4184 struct drm_device *dev
2780 struct pci_dev *dev
1916 struct comedi_device *dev
1510 struct mlx5_core_dev *dev
1057 struct mlx4_dev *dev
894 struct b43_wldev *dev
762 struct input_dev *dev
623 struct usbnet *dev
561 struct mlx5_ib_dev *dev
525 struct mt76_dev *dev
465 struct mt76x02_dev *dev
435 struct platform_device *dev
431 struct usb_device *dev
411 struct mt7915_dev *dev
398 struct cx231xx *dev
378 struct mei_device *dev
363 struct ksz_device *dev
359 struct mthca_dev *dev
A good portion of the above also have a dev member.
Are you planning on changing all of the above too, or are you only
annoyed by drm?
I'm really not convinced at all.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Spice-devel] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Wed, 12 Jul 2023, Uwe Kleine-König wrote: > Hello, > > while I debugged an issue in the imx-lcdc driver I was constantly > irritated about struct drm_device pointer variables being named "dev" > because with that name I usually expect a struct device pointer. > > I think there is a big benefit when these are all renamed to "drm_dev". > I have no strong preference here though, so "drmdev" or "drm" are fine > for me, too. Let the bikesheding begin! > > Some statistics: > > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c > | sort -n > 1 struct drm_device *adev_to_drm > 1 struct drm_device *drm_ > 1 struct drm_device *drm_dev > 1 struct drm_device*drm_dev > 1 struct drm_device *pdev > 1 struct drm_device *rdev > 1 struct drm_device *vdev > 2 struct drm_device *dcss_drv_dev_to_drm > 2 struct drm_device **ddev > 2 struct drm_device *drm_dev_alloc > 2 struct drm_device *mock > 2 struct drm_device *p_ddev > 5 struct drm_device *device > 9 struct drm_device * dev > 25 struct drm_device *d > 95 struct drm_device * > 216 struct drm_device *ddev > 234 struct drm_device *drm_dev > 611 struct drm_device *drm >4190 struct drm_device *dev > > This series starts with renaming struct drm_crtc::dev to drm_dev. If > it's not only me and others like the result of this effort it should be > followed up by adapting the other structs and the individual usages in > the different drivers. I think this is an unnecessary change. In drm, a dev is usually a drm device, i.e. struct drm_device *. As shown by the numbers above. If folks insist on following through with this anyway, I'm firmly in the camp the name should be "drm" and nothing else. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [Spice-devel] [PATCH v3 07/38] drm: handle HAS_IOPORT dependencies
On Tue, 14 Mar 2023, Niklas Schnelle wrote:
> In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> not being declared. We thus need to add HAS_IOPORT as dependency for
> those drivers using them. In the bochs driver there is optional MMIO
> support detected at runtime, warn if this isn't taken when
> HAS_IOPORT is not defined.
Not that I care a whole lot, but there should really only be one warning
or even failure to probe at bochs_hw_init() for !bochs->mmio &&
!IS_ENABLED(CONFIG_HAS_IOPORT), not warnings all over the place.
Moreover, the config macro is CONFIG_HAS_IOPORT instead of HAS_IOPORT
that you check for below.
BR,
Jani.
> There is also a direct and hard coded use in cirrus.c which according to
> the comment is only necessary during resume. Let's just skip this as
> for example s390 which doesn't have I/O port support also doesen't
> support suspend/resume.
>
> Co-developed-by: Arnd Bergmann
> Signed-off-by: Niklas Schnelle
> ---
> drivers/gpu/drm/qxl/Kconfig | 1 +
> drivers/gpu/drm/tiny/bochs.c | 19 +++
> drivers/gpu/drm/tiny/cirrus.c | 2 ++
> 3 files changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/qxl/Kconfig b/drivers/gpu/drm/qxl/Kconfig
> index ca3f51c2a8fe..d0e0d440c8d9 100644
> --- a/drivers/gpu/drm/qxl/Kconfig
> +++ b/drivers/gpu/drm/qxl/Kconfig
> @@ -2,6 +2,7 @@
> config DRM_QXL
> tristate "QXL virtual GPU"
> depends on DRM && PCI && MMU
> + depends on HAS_IOPORT
> select DRM_KMS_HELPER
> select DRM_TTM
> select DRM_TTM_HELPER
> diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
> index 024346054c70..da4a5d53b003 100644
> --- a/drivers/gpu/drm/tiny/bochs.c
> +++ b/drivers/gpu/drm/tiny/bochs.c
> @@ -2,6 +2,7 @@
>
> #include
> #include
> +#include
>
> #include
> #include
> @@ -105,7 +106,11 @@ static void bochs_vga_writeb(struct bochs_device *bochs,
> u16 ioport, u8 val)
>
> writeb(val, bochs->mmio + offset);
> } else {
> +#ifdef HAS_IOPORT
> outb(val, ioport);
> +#else
> + WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT");
> +#endif
> }
> }
>
> @@ -119,7 +124,12 @@ static u8 bochs_vga_readb(struct bochs_device *bochs,
> u16 ioport)
>
> return readb(bochs->mmio + offset);
> } else {
> +#ifdef HAS_IOPORT
> return inb(ioport);
> +#else
> + WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT");
> + return 0xff;
> +#endif
> }
> }
>
> @@ -132,8 +142,13 @@ static u16 bochs_dispi_read(struct bochs_device *bochs,
> u16 reg)
>
> ret = readw(bochs->mmio + offset);
> } else {
> +#ifdef HAS_IOPORT
> outw(reg, VBE_DISPI_IOPORT_INDEX);
> ret = inw(VBE_DISPI_IOPORT_DATA);
> +#else
> + WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT");
> + ret = 0x;
> +#endif
> }
> return ret;
> }
> @@ -145,8 +160,12 @@ static void bochs_dispi_write(struct bochs_device
> *bochs, u16 reg, u16 val)
>
> writew(val, bochs->mmio + offset);
> } else {
> +#ifdef HAS_IOPORT
> outw(reg, VBE_DISPI_IOPORT_INDEX);
> outw(val, VBE_DISPI_IOPORT_DATA);
> +#else
> + WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT");
> +#endif
> }
> }
>
> diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
> index accfa52e78c5..9da89732c5ac 100644
> --- a/drivers/gpu/drm/tiny/cirrus.c
> +++ b/drivers/gpu/drm/tiny/cirrus.c
> @@ -308,8 +308,10 @@ static int cirrus_mode_set(struct cirrus_device *cirrus,
>
> cirrus_set_start_address(cirrus, 0);
>
> +#ifdef CONFIG_HAS_IOPORT
> /* Unblank (needed on S3 resume, vgabios doesn't do it then) */
> outb(0x20, 0x3c0);
> +#endif
>
> drm_dev_exit(idx);
> return 0;
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
On Fri, 05 Nov 2021, Javier Martinez Canillas wrote:
> 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 ?
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.
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?
BR,
Jani.
>
>>>>> +{
>>>>> + 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,
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Spice-devel] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem
c
>> b/drivers/gpu/drm/virtio/virtgpu_drv.c
>> index 28200dfba2d1..ba9c0c2f8ae6 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
>> @@ -27,7 +27,6 @@
>>*/
>>
>> #include
>> -#include
>> #include
>> #include
>> #include
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
>> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
>> index 05e9949293d5..115ec9518277 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
>> @@ -25,7 +25,6 @@
>>*
>>
>> **/
>>
>> -#include
>> #include
>> #include
>> #include
>> diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
>> index ef9c57ce0906..d4320b147956 100644
>> --- a/drivers/video/console/vgacon.c
>> +++ b/drivers/video/console/vgacon.c
>> @@ -97,30 +97,9 @@ static intvga_video_font_height;
>> static int vga_scan_lines __read_mostly;
>> static unsigned intvga_rolled_over; /* last vc_origin offset
>> before wrap */
>>
>> -static bool vgacon_text_mode_force;
>> static bool vga_hardscroll_enabled;
>> static bool vga_hardscroll_user_enable = true;
>>
>> -bool vgacon_text_force(void)
>> -{
>> -return vgacon_text_mode_force;
>> -}
>> -EXPORT_SYMBOL(vgacon_text_force);
>> -
>> -static int __init text_mode(char *str)
>> -{
>> -vgacon_text_mode_force = 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;
>> -}
>> -
>> -/* force text mode - used by kernel modesetting */
>> -__setup("nomodeset", text_mode);
>> -
>> static int __init no_scroll(char *str)
>> {
>> /*
>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>> index 48b7de80daf5..18982d3507e4 100644
>> --- a/include/drm/drm_mode_config.h
>> +++ b/include/drm/drm_mode_config.h
>> @@ -969,4 +969,10 @@ static inline int drm_mode_config_init(struct
>> drm_device *dev)
>> void drm_mode_config_reset(struct drm_device *dev);
>> void drm_mode_config_cleanup(struct drm_device *dev);
>>
>> +#ifdef CONFIG_VGA_CONSOLE
>> +extern int drm_check_modeset(void);
>> +#else
>> +static inline int drm_check_modeset(void) { return 0; }
>> +#endif
>> +
>> #endif
>> diff --git a/include/linux/console.h b/include/linux/console.h
>> index 20874db50bc8..d4dd8384898b 100644
>> --- a/include/linux/console.h
>> +++ b/include/linux/console.h
>> @@ -217,12 +217,6 @@ extern atomic_t ignore_console_lock_warning;
>> #define VESA_HSYNC_SUSPEND 2
>> #define VESA_POWERDOWN 3
>>
>> -#ifdef CONFIG_VGA_CONSOLE
>> -extern bool vgacon_text_force(void);
>> -#else
>> -static inline bool vgacon_text_force(void) { return false; }
>> -#endif
>> -
>> extern void console_init(void);
>>
>> /* For deferred console takeover */
>>
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
On Thu, 04 Nov 2021, Sam Ravnborg wrote:
> Hi Javier,
>
>>
>> >>>
>> >>> -if (vgacon_text_force() && i915_modparams.modeset == -1)
>> >>> +ret = drm_drv_enabled();
>> >>
>> >> You pass the local driver variable here - which looks wrong as this is
>> >> not the same as the driver variable declared in another file.
>> >
>>
>> Yes, Jani mentioned it already. I got confused and thought that the driver
>> variable was also defined in the same compilation unit...
>>
>> Maybe I could squash the following change?
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index b18a250e5d2e..b8f399b76363 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -89,7 +89,7 @@
>> #include "intel_region_ttm.h"
>> #include "vlv_suspend.h"
>>
>> -static const struct drm_driver driver;
>> +const struct drm_driver driver;
> No, variables with such a generic name will clash.
>
> Just add a
> const drm_driver * i915_drm_driver(void)
> {
> return
> }
>
> And then use this function to access the drm_driver variable.
Agreed on the general principle of exposing interfaces over data.
But... why? I'm still at a loss what problem this solves. We pass
to exactly one place, devm_drm_dev_alloc(), and it's neatly
hidden away.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
_driver driver = {
>
> static int __init vbox_init(void)
> {
> -#ifdef CONFIG_VGA_CONSOLE
> - if (vgacon_text_force() && vbox_modeset == -1)
> - return -EINVAL;
> -#endif
> + int ret;
> +
> + ret = drm_drv_enabled();
> + if (ret && vbox_modeset == -1)
> + return ret;
>
> if (vbox_modeset == 0)
> return -EINVAL;
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
> b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index 749db18dcfa2..28200dfba2d1 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -104,8 +104,9 @@ static int virtio_gpu_probe(struct virtio_device *vdev)
> struct drm_device *dev;
> int ret;
>
> - if (vgacon_text_force() && virtio_gpu_modeset == -1)
> - return -EINVAL;
> + ret = drm_drv_enabled();
> + if (ret && virtio_gpu_modeset == -1)
> + return ret;
>
> if (virtio_gpu_modeset == 0)
> return -EINVAL;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index ab9a1750e1df..05e9949293d5 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1651,8 +1651,9 @@ static int __init vmwgfx_init(void)
> {
> int ret;
>
> - if (vgacon_text_force())
> - return -EINVAL;
> + ret = drm_drv_enabled();
> + if (ret)
> + return ret;
>
> ret = pci_register_driver(_pci_driver);
> if (ret)
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 0cd95953cdf5..77abfc7e078b 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -598,5 +598,6 @@ static inline bool drm_drv_uses_atomic_modeset(struct
> drm_device *dev)
>
> int drm_dev_set_unique(struct drm_device *dev, const char *name);
>
> +int drm_drv_enabled(const struct drm_driver *driver);
>
> #endif
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
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?
>
>
>> +return -ENODEV;
>> +}
>> +
>> +return 0;
>> +}
>> +EXPORT_SYMBOL(drm_drv_enabled);
>> +
>> /*
>> * DRM Core
>> * The DRM core module initializes all global DRM objects and makes them
>> diff --git a/drivers/gpu/drm/i915/i915_module.c
>> b/drivers/gpu/drm/i915/i915_module.c
>> index ab2295dd4500..45cb3e540eff 100644
>> --- a/drivers/gpu/drm/i915/i915_module.c
>> +++ b/drivers/gpu/drm/i915/i915_module.c
>> @@ -18,9 +18,12 @@
>> #include "i915_selftest.h"
>> #include "i915_vma.h"
>>
>> +static const struct drm_driver driver;
> Hmmm...
>
>> +
>> static int i915_check_nomodeset(void)
>> {
>> bool use_kms = true;
>> +int ret;
>>
>> /*
>> * Enable KMS by default, unless explicitly overriden by
>> @@ -31,7 +34,8 @@ static int i915_check_nomodeset(void)
>> if (i915_modparams.modeset == 0)
>> use_kms = false;
>>
>> -if (vgacon_text_force() && i915_modparams.modeset == -1)
>> +ret = drm_drv_enabled();
>
> You pass the local driver variable here - which looks wrong as this is
> not the same as the driver variable declared in another file.
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.
>From my POV, drm_drv_enabled() is a solution that creates a worse
problem for us than it solves.
BR,
Jani.
>
>
> Sam
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Spice-devel] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled
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! */
}
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Spice-devel] [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem
understand what nomodeset does, you should
> reboot without enabling it\n");
> -
> - return 1;
> -}
> -
> -/* force text mode - used by kernel modesetting */
> -__setup("nomodeset", text_mode);
> -
> static int __init no_scroll(char *str)
> {
> /*
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 48b7de80daf5..e1d2042a7b77 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -969,4 +969,10 @@ static inline int drm_mode_config_init(struct drm_device
> *dev)
> void drm_mode_config_reset(struct drm_device *dev);
> void drm_mode_config_cleanup(struct drm_device *dev);
>
> +#ifdef CONFIG_VGA_CONSOLE
> +extern bool vgacon_text_force(void);
> +#else
> +static inline bool vgacon_text_force(void) { return false; }
> +#endif
> +
As said, maybe the CONFIG_VGA_CONSOLE ifdef should be dropped.
Also, this seems like a completely arbitrary choice of header to place
this.
BR,
Jani.
> #endif
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 20874db50bc8..d4dd8384898b 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -217,12 +217,6 @@ extern atomic_t ignore_console_lock_warning;
> #define VESA_HSYNC_SUSPEND 2
> #define VESA_POWERDOWN 3
>
> -#ifdef CONFIG_VGA_CONSOLE
> -extern bool vgacon_text_force(void);
> -#else
> -static inline bool vgacon_text_force(void) { return false; }
> -#endif
> -
> extern void console_init(void);
>
> /* For deferred console takeover */
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Spice-devel] [PATCH v2 09/20] drm/i915: Remove references to struct drm_device.pdev
On Tue, 08 Dec 2020, Thomas Zimmermann wrote: > ping for a review of the i915 patches What did you have in mind regarding merging the series? Should we just pick the patches up? BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH][next] drm: Replace zero-length array with flexible-array member
On Tue, 25 Feb 2020, "Gustavo A. R. Silva" wrote:
> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array member[1][2],
> introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on.
>
> Also, notice that, dynamic memory allocations won't be affected by
> this change:
>
> "Flexible array members have incomplete type, and so the sizeof operator
> may not be applied. As a quirk of the original implementation of
> zero-length arrays, sizeof evaluates to zero."[1]
>
> This issue was found with the help of Coccinelle.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/gpu/drm/etnaviv/etnaviv_gem.h | 2 +-
> drivers/gpu/drm/gma500/intel_bios.h | 2 +-
> drivers/gpu/drm/i915/display/intel_vbt_defs.h | 4 ++--
> drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +-
> drivers/gpu/drm/i915/i915_gpu_error.h | 2 +-
Please split out the i915 changes to a separate patch.
> drivers/gpu/drm/msm/msm_gem.h | 2 +-
> drivers/gpu/drm/qxl/qxl_cmd.c | 2 +-
> drivers/gpu/drm/vboxvideo/vboxvideo.h | 2 +-
> drivers/gpu/drm/vc4/vc4_drv.h | 2 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c| 2 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 2 +-
> include/drm/bridge/mhl.h | 4 ++--
> include/drm/drm_displayid.h | 2 +-
> include/uapi/drm/i915_drm.h | 4 ++--
Not sure it's worth touching uapi headers. They're full of both [0] and
[]. Again, please at least split it to a separate patch to be decided
separately.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH] drm: Split out drm_probe_helper.h
On Tue, 15 Jan 2019, Daniel Vetter wrote: > Having the probe helper stuff (which pretty much everyone needs) in > the drm_crtc_helper.h file (which atomic drivers should never need) is > confusing. Split them out. > > To make sure I actually achieved the goal here I went through all > drivers. And indeed, all atomic drivers are now free of > drm_crtc_helper.h includes. > > v2: Make it compile. There was so much compile fail on arm drivers > that I figured I'll better not include any of the acks on v1. > > v3: Massive rebase because i915 has lost a lot of drmP.h includes, but > not all: Through drm_crtc_helper.h > drm_modeset_helper.h -> drmP.h > there was still one, which this patch largely removes. Which means > rolling out lots more includes all over. > > This will also conflict with ongoing drmP.h cleanup by others I > expect. > > v3: Rebase on top of atomic bochs. > > Cc: Sam Ravnborg > Cc: Jani Nikula > Cc: Laurent Pinchart > Acked-by: Rodrigo Vivi (v2) > Acked-by: Benjamin Gaignard (v2) > Signed-off-by: Daniel Vetter > Cc: linux-arm-ker...@lists.infradead.org > Cc: virtualizat...@lists.linux-foundation.org > Cc: etna...@lists.freedesktop.org > Cc: linux-samsung-...@vger.kernel.org > Cc: intel-...@lists.freedesktop.org > Cc: linux-media...@lists.infradead.org > Cc: linux-amlo...@lists.infradead.org > Cc: linux-arm-...@vger.kernel.org > Cc: freedr...@lists.freedesktop.org > Cc: nouv...@lists.freedesktop.org > Cc: spice-devel@lists.freedesktop.org > Cc: amd-...@lists.freedesktop.org > Cc: linux-renesas-...@vger.kernel.org > Cc: linux-rockc...@lists.infradead.org > Cc: linux-st...@st-md-mailman.stormreply.com > Cc: linux-te...@vger.kernel.org > Cc: xen-de...@lists.xen.org > --- > Merging this is going to be a bit a mess due to all the ongoing drmP.h > cleanups. I think the following should work: > - Apply Sam's prep patches for removing drmP.h from > drm_modeset_helper.h > - Get the i915 drmP.h cleanup backmerged into drm-misc-next > - Apply this patch. > - Apply Sam's patch to remove drmP.h from drm_modeset_helper.h > - All through drm-misc-next, which has some potential for trivial > conflicts around #includes with other drivers unfortunately. > > I hope there's no other driver who'll blow up accidentally because > someone else is doing a drmP.h cleanup. Laurent maybe? > > Jani, ack on this? Acked-by: Jani Nikula -- Jani Nikula, Intel Open Source Graphics Center ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
