[PATCH 1/2] drm/amdgpu: Fix memory leaks at amdgpu_init() error path

[PATCH 1/2] drm/amdgpu: Fix memory leaks at amdgpu_init() error path

[Takashi Iwai]

amdgpu driver checks vgacon_text_force() after some initializations
but without cleaning up.  This will result in leaks.

Move the check of vgacon_text_force() to the beginning of
amdgpu_init() for fixing it and also for optimization.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 50afcf65181a..e55792d3cd12 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -905,6 +905,11 @@ static int __init amdgpu_init(void)
 {
 	int r;
 
+	if (vgacon_text_force()) {
+		DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
+		return -EINVAL;
+	}
+
 	r = amdgpu_sync_init();
 	if (r)
 		goto error_sync;
@@ -913,10 +918,6 @@ static int __init amdgpu_init(void)
 	if (r)
 		goto error_fence;
 
-	if (vgacon_text_force()) {
-		DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
-		return -EINVAL;
-	}
 	DRM_INFO("amdgpu kernel modesetting enabled.\n");
 	driver = &kms_driver;
 	pdriver = &amdgpu_kms_pci_driver;
-- 
2.16.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/amdgpu: Add modeset module option

[Takashi Iwai]

amdgpu driver lacks of modeset module option other drm drivers provide
for enforcing or disabling the driver load.  Interestingly, the
amdgpu_mode variable declaration is already found in the header file,
but the actual implementation seems to have been forgotten.

This patch adds the missing piece.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index e55792d3cd12..029d95ecd26b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -78,6 +78,7 @@
 #define KMS_DRIVER_MINOR	23
 #define KMS_DRIVER_PATCHLEVEL	0
 
+int amdgpu_modeset = -1;
 int amdgpu_vram_limit = 0;
 int amdgpu_vis_vram_limit = 0;
 int amdgpu_gart_size = -1; /* auto */
@@ -130,6 +131,9 @@ int amdgpu_lbpw = -1;
 int amdgpu_compute_multipipe = -1;
 int amdgpu_gpu_recovery = -1; /* auto */
 
+MODULE_PARM_DESC(modeset, "Disable/Enable modesetting");
+module_param_named(modeset, amdgpu_modeset, int, 0400);
+
 MODULE_PARM_DESC(vramlimit, "Restrict VRAM for testing, in megabytes");
 module_param_named(vramlimit, amdgpu_vram_limit, int, 0600);
 
@@ -905,10 +909,12 @@ static int __init amdgpu_init(void)
 {
 	int r;
 
-	if (vgacon_text_force()) {
+	if (vgacon_text_force() && amdgpu_modeset == -1) {
 		DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
 		return -EINVAL;
 	}
+	if (amdgpu_modeset == 0)
+		return -EINVAL;
 
 	r = amdgpu_sync_init();
 	if (r)
-- 
2.16.2

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Christian König]

Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
> amdgpu driver lacks of modeset module option other drm drivers provide
> for enforcing or disabling the driver load.  Interestingly, the
> amdgpu_mode variable declaration is already found in the header file,
> but the actual implementation seems to have been forgotten.
>
> This patch adds the missing piece.

NAK, modesetting is mandatory for amdgpu and we should probably remove 
the option to disable it from other DRM drivers without UMS support as 
well (pretty much all of them now).

If you want to prevent a driver from loading I think the correct way to 
do so is to give modprobe.blacklist=amdgpu on the kernel commandline.

That would remove the possibility to prevent the driver from loading 
when it is compiled in, but I don't see much of a problem with that.

Christian.

>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index e55792d3cd12..029d95ecd26b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -78,6 +78,7 @@
>   #define KMS_DRIVER_MINOR	23
>   #define KMS_DRIVER_PATCHLEVEL	0
>   
> +int amdgpu_modeset = -1;
>   int amdgpu_vram_limit = 0;
>   int amdgpu_vis_vram_limit = 0;
>   int amdgpu_gart_size = -1; /* auto */
> @@ -130,6 +131,9 @@ int amdgpu_lbpw = -1;
>   int amdgpu_compute_multipipe = -1;
>   int amdgpu_gpu_recovery = -1; /* auto */
>   
> +MODULE_PARM_DESC(modeset, "Disable/Enable modesetting");
> +module_param_named(modeset, amdgpu_modeset, int, 0400);
> +
>   MODULE_PARM_DESC(vramlimit, "Restrict VRAM for testing, in megabytes");
>   module_param_named(vramlimit, amdgpu_vram_limit, int, 0600);
>   
> @@ -905,10 +909,12 @@ static int __init amdgpu_init(void)
>   {
>   	int r;
>   
> -	if (vgacon_text_force()) {
> +	if (vgacon_text_force() && amdgpu_modeset == -1) {
>   		DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
>   		return -EINVAL;
>   	}
> +	if (amdgpu_modeset == 0)
> +		return -EINVAL;
>   
>   	r = amdgpu_sync_init();
>   	if (r)

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Ilia Mirkin]

On Sun, Apr 1, 2018 at 1:39 PM, Christian König
<christian.koenig@amd.com> wrote:
> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
>>
>> amdgpu driver lacks of modeset module option other drm drivers provide
>> for enforcing or disabling the driver load.  Interestingly, the
>> amdgpu_mode variable declaration is already found in the header file,
>> but the actual implementation seems to have been forgotten.
>>
>> This patch adds the missing piece.
>
>
> NAK, modesetting is mandatory for amdgpu and we should probably remove the
> option to disable it from other DRM drivers without UMS support as well
> (pretty much all of them now).
>
> If you want to prevent a driver from loading I think the correct way to do
> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
>
> That would remove the possibility to prevent the driver from loading when it
> is compiled in, but I don't see much of a problem with that.

Having a way to kill the graphics driver is a very useful debugging
tool, and also a quick and easy way to get out of an unpleasant
situation where graphics are messed up / system hangs / etc. The
modprobe blacklist kernel arg only works in certain environments (and
only if it's a module).

Every other DRM driver has this and this is a well-documented
workaround for "graphics are messed up when I install linux", why not
allow a uniform experience for the end users who are just trying to
get their systems up and running?

  -ilia
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Christian König]

Am 01.04.2018 um 19:45 schrieb Ilia Mirkin:
> On Sun, Apr 1, 2018 at 1:39 PM, Christian König
> <christian.koenig@amd.com> wrote:
>> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
>>> amdgpu driver lacks of modeset module option other drm drivers provide
>>> for enforcing or disabling the driver load.  Interestingly, the
>>> amdgpu_mode variable declaration is already found in the header file,
>>> but the actual implementation seems to have been forgotten.
>>>
>>> This patch adds the missing piece.
>>
>> NAK, modesetting is mandatory for amdgpu and we should probably remove the
>> option to disable it from other DRM drivers without UMS support as well
>> (pretty much all of them now).
>>
>> If you want to prevent a driver from loading I think the correct way to do
>> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
>>
>> That would remove the possibility to prevent the driver from loading when it
>> is compiled in, but I don't see much of a problem with that.
> Having a way to kill the graphics driver is a very useful debugging
> tool, and also a quick and easy way to get out of an unpleasant
> situation where graphics are messed up / system hangs / etc. The
> modprobe blacklist kernel arg only works in certain environments (and
> only if it's a module).
>
> Every other DRM driver has this and this is a well-documented
> workaround for "graphics are messed up when I install linux", why not
> allow a uniform experience for the end users who are just trying to
> get their systems up and running?

Because it is not well documented and repeated over and over again in 
drivers.

The problem is that people don't realized that the driver isn't loaded 
at all without the modeset=0 module option and demand fixing the 
resulting bad performance without modesetting.

I can't count how often I had to explain that this approach won't work. 
We could rename it to something like disable_gfx_accel or something like 
that to make this clear.

Christian.

>
>    -ilia

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Takashi Iwai]

On Sun, 01 Apr 2018 19:58:11 +0200,
Christian K6nig wrote:
> 
> Am 01.04.2018 um 19:45 schrieb Ilia Mirkin:
> > On Sun, Apr 1, 2018 at 1:39 PM, Christian König
> > <christian.koenig@amd.com> wrote:
> >> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
> >>> amdgpu driver lacks of modeset module option other drm drivers provide
> >>> for enforcing or disabling the driver load.  Interestingly, the
> >>> amdgpu_mode variable declaration is already found in the header file,
> >>> but the actual implementation seems to have been forgotten.
> >>>
> >>> This patch adds the missing piece.
> >>
> >> NAK, modesetting is mandatory for amdgpu and we should probably remove the
> >> option to disable it from other DRM drivers without UMS support as well
> >> (pretty much all of them now).
> >>
> >> If you want to prevent a driver from loading I think the correct way to do
> >> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
> >>
> >> That would remove the possibility to prevent the driver from loading when it
> >> is compiled in, but I don't see much of a problem with that.
> > Having a way to kill the graphics driver is a very useful debugging
> > tool, and also a quick and easy way to get out of an unpleasant
> > situation where graphics are messed up / system hangs / etc. The
> > modprobe blacklist kernel arg only works in certain environments (and
> > only if it's a module).
> >
> > Every other DRM driver has this and this is a well-documented
> > workaround for "graphics are messed up when I install linux", why not
> > allow a uniform experience for the end users who are just trying to
> > get their systems up and running?
> 
> Because it is not well documented and repeated over and over again in
> drivers.
> 
> The problem is that people don't realized that the driver isn't loaded
> at all without the modeset=0 module option and demand fixing the
> resulting bad performance without modesetting.

Hm, I don't get it.  What this options has to do with performance for
a KMS-only driver...?

> I can't count how often I had to explain that this approach won't
> work. We could rename it to something like disable_gfx_accel or
> something like that to make this clear.

Maybe it's a different view from a different perspective.
 From the distro side, nomodeset and the force reload via modeset
option has been the most reliable way to achieve the purpose, so far.
It's easier especially when the system has a hybrid graphics.

It might be not same for developers, though.


thanks,

Takashi
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Christian König]

Am 01.04.2018 um 20:21 schrieb Takashi Iwai:
> On Sun, 01 Apr 2018 19:58:11 +0200,
> Christian K6nig wrote:
>> Am 01.04.2018 um 19:45 schrieb Ilia Mirkin:
>>> On Sun, Apr 1, 2018 at 1:39 PM, Christian König
>>> <christian.koenig@amd.com> wrote:
>>>> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
>>>>> amdgpu driver lacks of modeset module option other drm drivers provide
>>>>> for enforcing or disabling the driver load.  Interestingly, the
>>>>> amdgpu_mode variable declaration is already found in the header file,
>>>>> but the actual implementation seems to have been forgotten.
>>>>>
>>>>> This patch adds the missing piece.
>>>> NAK, modesetting is mandatory for amdgpu and we should probably remove the
>>>> option to disable it from other DRM drivers without UMS support as well
>>>> (pretty much all of them now).
>>>>
>>>> If you want to prevent a driver from loading I think the correct way to do
>>>> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
>>>>
>>>> That would remove the possibility to prevent the driver from loading when it
>>>> is compiled in, but I don't see much of a problem with that.
>>> Having a way to kill the graphics driver is a very useful debugging
>>> tool, and also a quick and easy way to get out of an unpleasant
>>> situation where graphics are messed up / system hangs / etc. The
>>> modprobe blacklist kernel arg only works in certain environments (and
>>> only if it's a module).
>>>
>>> Every other DRM driver has this and this is a well-documented
>>> workaround for "graphics are messed up when I install linux", why not
>>> allow a uniform experience for the end users who are just trying to
>>> get their systems up and running?
>> Because it is not well documented and repeated over and over again in
>> drivers.
>>
>> The problem is that people don't realized that the driver isn't loaded
>> at all without the modeset=0 module option and demand fixing the
>> resulting bad performance without modesetting.
> Hm, I don't get it.  What this options has to do with performance for
> a KMS-only driver...?

Well exactly that's the point, nothing.

The problem is that the option name is confusing to the end user because 
the expectation is that "nomodeset" just means that they only can't 
change the display mode.

Some (unfortunately quite a lot) people don't realize that for KMS 
drivers this means that the driver isn't even loaded and they also don't 
get any acceleration.

We had to explain that numerous times now. I think it would be best to 
give the option a more meaningful name.

Christian.

>> I can't count how often I had to explain that this approach won't
>> work. We could rename it to something like disable_gfx_accel or
>> something like that to make this clear.
> Maybe it's a different view from a different perspective.
>   From the distro side, nomodeset and the force reload via modeset
> option has been the most reliable way to achieve the purpose, so far.
> It's easier especially when the system has a hybrid graphics.
>
> It might be not same for developers, though.
>
>
> thanks,
>
> Takashi

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Daniel Vetter]

On Sun, Apr 01, 2018 at 10:12:10PM +0200, Christian König wrote:
> Am 01.04.2018 um 20:21 schrieb Takashi Iwai:
> > On Sun, 01 Apr 2018 19:58:11 +0200,
> > Christian K6nig wrote:
> > > Am 01.04.2018 um 19:45 schrieb Ilia Mirkin:
> > > > On Sun, Apr 1, 2018 at 1:39 PM, Christian König
> > > > <christian.koenig@amd.com> wrote:
> > > > > Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
> > > > > > amdgpu driver lacks of modeset module option other drm drivers provide
> > > > > > for enforcing or disabling the driver load.  Interestingly, the
> > > > > > amdgpu_mode variable declaration is already found in the header file,
> > > > > > but the actual implementation seems to have been forgotten.
> > > > > > 
> > > > > > This patch adds the missing piece.
> > > > > NAK, modesetting is mandatory for amdgpu and we should probably remove the
> > > > > option to disable it from other DRM drivers without UMS support as well
> > > > > (pretty much all of them now).
> > > > > 
> > > > > If you want to prevent a driver from loading I think the correct way to do
> > > > > so is to give modprobe.blacklist=amdgpu on the kernel commandline.
> > > > > 
> > > > > That would remove the possibility to prevent the driver from loading when it
> > > > > is compiled in, but I don't see much of a problem with that.
> > > > Having a way to kill the graphics driver is a very useful debugging
> > > > tool, and also a quick and easy way to get out of an unpleasant
> > > > situation where graphics are messed up / system hangs / etc. The
> > > > modprobe blacklist kernel arg only works in certain environments (and
> > > > only if it's a module).
> > > > 
> > > > Every other DRM driver has this and this is a well-documented
> > > > workaround for "graphics are messed up when I install linux", why not
> > > > allow a uniform experience for the end users who are just trying to
> > > > get their systems up and running?
> > > Because it is not well documented and repeated over and over again in
> > > drivers.
> > > 
> > > The problem is that people don't realized that the driver isn't loaded
> > > at all without the modeset=0 module option and demand fixing the
> > > resulting bad performance without modesetting.
> > Hm, I don't get it.  What this options has to do with performance for
> > a KMS-only driver...?
> 
> Well exactly that's the point, nothing.
> 
> The problem is that the option name is confusing to the end user because the
> expectation is that "nomodeset" just means that they only can't change the
> display mode.
> 
> Some (unfortunately quite a lot) people don't realize that for KMS drivers
> this means that the driver isn't even loaded and they also don't get any
> acceleration.
> 
> We had to explain that numerous times now. I think it would be best to give
> the option a more meaningful name.

Yeah, agreed with Christian. If we want a generic "pls disable all gfx
accel" knob then probably best to put that into the drm core. And just
outright fail loading the drm core if that happens, which will prevent all
gfx drivers from loading.

That likely means a hole bunch of stuff won't work (usually sound keels
over too), but that's what you get for this. Only disabling modesetting
without disabling the entire driver doesn't work (never has, except for
this UMS+GEM combo only i915 support, and not for long).

And once we have that knob, probably best to phase out all the per-driver
options.
-Daniel

> 
> Christian.
> 
> > > I can't count how often I had to explain that this approach won't
> > > work. We could rename it to something like disable_gfx_accel or
> > > something like that to make this clear.
> > Maybe it's a different view from a different perspective.
> >   From the distro side, nomodeset and the force reload via modeset
> > option has been the most reliable way to achieve the purpose, so far.
> > It's easier especially when the system has a hybrid graphics.
> > 
> > It might be not same for developers, though.
> > 
> > 
> > thanks,
> > 
> > Takashi
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Christian König]

Am 03.04.2018 um 11:29 schrieb Daniel Vetter:
> On Sun, Apr 01, 2018 at 10:12:10PM +0200, Christian König wrote:
>> Am 01.04.2018 um 20:21 schrieb Takashi Iwai:
>> [SNIP]
>> Well exactly that's the point, nothing.
>>
>> The problem is that the option name is confusing to the end user because the
>> expectation is that "nomodeset" just means that they only can't change the
>> display mode.
>>
>> Some (unfortunately quite a lot) people don't realize that for KMS drivers
>> this means that the driver isn't even loaded and they also don't get any
>> acceleration.
>>
>> We had to explain that numerous times now. I think it would be best to give
>> the option a more meaningful name.
> Yeah, agreed with Christian. If we want a generic "pls disable all gfx
> accel" knob then probably best to put that into the drm core. And just
> outright fail loading the drm core if that happens, which will prevent all
> gfx drivers from loading.
>
> That likely means a hole bunch of stuff won't work (usually sound keels
> over too), but that's what you get for this. Only disabling modesetting
> without disabling the entire driver doesn't work (never has, except for
> this UMS+GEM combo only i915 support, and not for long).
>
> And once we have that knob, probably best to phase out all the per-driver
> options.

+ some compability code to keep "nomodeset" working for a while longer 
and printing a warning that the option is deprecated.

If we approach it like that the whole idea sounds rather reasonable to me.

Christian.

> -Daniel
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Ilia Mirkin]

On Tue, Apr 3, 2018 at 5:29 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sun, Apr 01, 2018 at 10:12:10PM +0200, Christian König wrote:
>> Am 01.04.2018 um 20:21 schrieb Takashi Iwai:
>> > On Sun, 01 Apr 2018 19:58:11 +0200,
>> > Christian K6nig wrote:
>> > > Am 01.04.2018 um 19:45 schrieb Ilia Mirkin:
>> > > > On Sun, Apr 1, 2018 at 1:39 PM, Christian König
>> > > > <christian.koenig@amd.com> wrote:
>> > > > > Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
>> > > > > > amdgpu driver lacks of modeset module option other drm drivers provide
>> > > > > > for enforcing or disabling the driver load.  Interestingly, the
>> > > > > > amdgpu_mode variable declaration is already found in the header file,
>> > > > > > but the actual implementation seems to have been forgotten.
>> > > > > >
>> > > > > > This patch adds the missing piece.
>> > > > > NAK, modesetting is mandatory for amdgpu and we should probably remove the
>> > > > > option to disable it from other DRM drivers without UMS support as well
>> > > > > (pretty much all of them now).
>> > > > >
>> > > > > If you want to prevent a driver from loading I think the correct way to do
>> > > > > so is to give modprobe.blacklist=amdgpu on the kernel commandline.
>> > > > >
>> > > > > That would remove the possibility to prevent the driver from loading when it
>> > > > > is compiled in, but I don't see much of a problem with that.
>> > > > Having a way to kill the graphics driver is a very useful debugging
>> > > > tool, and also a quick and easy way to get out of an unpleasant
>> > > > situation where graphics are messed up / system hangs / etc. The
>> > > > modprobe blacklist kernel arg only works in certain environments (and
>> > > > only if it's a module).
>> > > >
>> > > > Every other DRM driver has this and this is a well-documented
>> > > > workaround for "graphics are messed up when I install linux", why not
>> > > > allow a uniform experience for the end users who are just trying to
>> > > > get their systems up and running?
>> > > Because it is not well documented and repeated over and over again in
>> > > drivers.
>> > >
>> > > The problem is that people don't realized that the driver isn't loaded
>> > > at all without the modeset=0 module option and demand fixing the
>> > > resulting bad performance without modesetting.
>> > Hm, I don't get it.  What this options has to do with performance for
>> > a KMS-only driver...?
>>
>> Well exactly that's the point, nothing.
>>
>> The problem is that the option name is confusing to the end user because the
>> expectation is that "nomodeset" just means that they only can't change the
>> display mode.
>>
>> Some (unfortunately quite a lot) people don't realize that for KMS drivers
>> this means that the driver isn't even loaded and they also don't get any
>> acceleration.
>>
>> We had to explain that numerous times now. I think it would be best to give
>> the option a more meaningful name.
>
> Yeah, agreed with Christian. If we want a generic "pls disable all gfx
> accel" knob then probably best to put that into the drm core. And just
> outright fail loading the drm core if that happens, which will prevent all
> gfx drivers from loading.
>
> That likely means a hole bunch of stuff won't work (usually sound keels
> over too), but that's what you get for this. Only disabling modesetting
> without disabling the entire driver doesn't work (never has, except for
> this UMS+GEM combo only i915 support, and not for long).
>
> And once we have that knob, probably best to phase out all the per-driver
> options.

Another use-case that the per-driver disables enable is "i915 works
but nouveau is broken due to crazy ACPI / PCIe PM / whatever". It
seems likely this could happen with amdgpu as well.

The current set of capabilities has served developers fairly well in
providing quick debug instructions to mildly-experienced users (i.e.
enough to be able to edit kernel cmdline). I definitely wouldn't want
to lose that.

The names are horrid. I think everyone agrees. In fact, when the
driver is disabled by nomodeset or modeset=0, I think they should
print like "driver completely disabled by modeset option" to avoid
some of the user confusion Christian is referring to (which I've run
into on a number of occasions as well). And/or maybe just refuse the
driver load entirely rather than load-but-do-nothing. However being
able to help users debug things seems more important than users being
occasionally confused by options they added to kernel cmdline.

  -ilia
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Michel Dänzer]

On 2018-04-03 03:26 PM, Ilia Mirkin wrote:
> On Tue, Apr 3, 2018 at 5:29 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Sun, Apr 01, 2018 at 10:12:10PM +0200, Christian König wrote:
>>> Am 01.04.2018 um 20:21 schrieb Takashi Iwai:
>>>> On Sun, 01 Apr 2018 19:58:11 +0200,
>>>> Christian K6nig wrote:
>>>>> Am 01.04.2018 um 19:45 schrieb Ilia Mirkin:
>>>>>> On Sun, Apr 1, 2018 at 1:39 PM, Christian König
>>>>>> <christian.koenig@amd.com> wrote:
>>>>>>> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
>>>>>>>> amdgpu driver lacks of modeset module option other drm drivers provide
>>>>>>>> for enforcing or disabling the driver load.  Interestingly, the
>>>>>>>> amdgpu_mode variable declaration is already found in the header file,
>>>>>>>> but the actual implementation seems to have been forgotten.
>>>>>>>>
>>>>>>>> This patch adds the missing piece.
>>>>>>> NAK, modesetting is mandatory for amdgpu and we should probably remove the
>>>>>>> option to disable it from other DRM drivers without UMS support as well
>>>>>>> (pretty much all of them now).
>>>>>>>
>>>>>>> If you want to prevent a driver from loading I think the correct way to do
>>>>>>> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
>>>>>>>
>>>>>>> That would remove the possibility to prevent the driver from loading when it
>>>>>>> is compiled in, but I don't see much of a problem with that.
>>>>>> Having a way to kill the graphics driver is a very useful debugging
>>>>>> tool, and also a quick and easy way to get out of an unpleasant
>>>>>> situation where graphics are messed up / system hangs / etc. The
>>>>>> modprobe blacklist kernel arg only works in certain environments (and
>>>>>> only if it's a module).
>>>>>>
>>>>>> Every other DRM driver has this and this is a well-documented
>>>>>> workaround for "graphics are messed up when I install linux", why not
>>>>>> allow a uniform experience for the end users who are just trying to
>>>>>> get their systems up and running?
>>>>> Because it is not well documented and repeated over and over again in
>>>>> drivers.
>>>>>
>>>>> The problem is that people don't realized that the driver isn't loaded
>>>>> at all without the modeset=0 module option and demand fixing the
>>>>> resulting bad performance without modesetting.
>>>> Hm, I don't get it.  What this options has to do with performance for
>>>> a KMS-only driver...?
>>>
>>> Well exactly that's the point, nothing.
>>>
>>> The problem is that the option name is confusing to the end user because the
>>> expectation is that "nomodeset" just means that they only can't change the
>>> display mode.
>>>
>>> Some (unfortunately quite a lot) people don't realize that for KMS drivers
>>> this means that the driver isn't even loaded and they also don't get any
>>> acceleration.
>>>
>>> We had to explain that numerous times now. I think it would be best to give
>>> the option a more meaningful name.
>>
>> Yeah, agreed with Christian. If we want a generic "pls disable all gfx
>> accel" knob then probably best to put that into the drm core. And just
>> outright fail loading the drm core if that happens, which will prevent all
>> gfx drivers from loading.
>>
>> That likely means a hole bunch of stuff won't work (usually sound keels
>> over too), but that's what you get for this. Only disabling modesetting
>> without disabling the entire driver doesn't work (never has, except for
>> this UMS+GEM combo only i915 support, and not for long).
>>
>> And once we have that knob, probably best to phase out all the per-driver
>> options.
> 
> Another use-case that the per-driver disables enable is "i915 works
> but nouveau is broken due to crazy ACPI / PCIe PM / whatever". It
> seems likely this could happen with amdgpu as well.

modprobe.blacklist=amdgpu

works as well as the modeset parameter for this.


> The names are horrid. I think everyone agrees. In fact, when the
> driver is disabled by nomodeset or modeset=0, I think they should
> print like "driver completely disabled by modeset option" to avoid
> some of the user confusion Christian is referring to (which I've run
> into on a number of occasions as well). And/or maybe just refuse the
> driver load entirely rather than load-but-do-nothing. However being
> able to help users debug things seems more important than users being
> occasionally confused by options they added to kernel cmdline.

FWIW, if a new parameter or other mechanism is added for this, ideally
it should allow preventing initialization on a per-GPU basis, instead of
only per-driver, for systems with multiple GPUs supported by the same
driver.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Ilia Mirkin]

On Tue, Apr 3, 2018 at 9:32 AM, Michel Dänzer <michel@daenzer.net> wrote:
> On 2018-04-03 03:26 PM, Ilia Mirkin wrote:
>> On Tue, Apr 3, 2018 at 5:29 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> On Sun, Apr 01, 2018 at 10:12:10PM +0200, Christian König wrote:
>>>> Am 01.04.2018 um 20:21 schrieb Takashi Iwai:
>>>>> On Sun, 01 Apr 2018 19:58:11 +0200,
>>>>> Christian K6nig wrote:
>>>>>> Am 01.04.2018 um 19:45 schrieb Ilia Mirkin:
>>>>>>> On Sun, Apr 1, 2018 at 1:39 PM, Christian König
>>>>>>> <christian.koenig@amd.com> wrote:
>>>>>>>> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
>>>>>>>>> amdgpu driver lacks of modeset module option other drm drivers provide
>>>>>>>>> for enforcing or disabling the driver load.  Interestingly, the
>>>>>>>>> amdgpu_mode variable declaration is already found in the header file,
>>>>>>>>> but the actual implementation seems to have been forgotten.
>>>>>>>>>
>>>>>>>>> This patch adds the missing piece.
>>>>>>>> NAK, modesetting is mandatory for amdgpu and we should probably remove the
>>>>>>>> option to disable it from other DRM drivers without UMS support as well
>>>>>>>> (pretty much all of them now).
>>>>>>>>
>>>>>>>> If you want to prevent a driver from loading I think the correct way to do
>>>>>>>> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
>>>>>>>>
>>>>>>>> That would remove the possibility to prevent the driver from loading when it
>>>>>>>> is compiled in, but I don't see much of a problem with that.
>>>>>>> Having a way to kill the graphics driver is a very useful debugging
>>>>>>> tool, and also a quick and easy way to get out of an unpleasant
>>>>>>> situation where graphics are messed up / system hangs / etc. The
>>>>>>> modprobe blacklist kernel arg only works in certain environments (and
>>>>>>> only if it's a module).
>>>>>>>
>>>>>>> Every other DRM driver has this and this is a well-documented
>>>>>>> workaround for "graphics are messed up when I install linux", why not
>>>>>>> allow a uniform experience for the end users who are just trying to
>>>>>>> get their systems up and running?
>>>>>> Because it is not well documented and repeated over and over again in
>>>>>> drivers.
>>>>>>
>>>>>> The problem is that people don't realized that the driver isn't loaded
>>>>>> at all without the modeset=0 module option and demand fixing the
>>>>>> resulting bad performance without modesetting.
>>>>> Hm, I don't get it.  What this options has to do with performance for
>>>>> a KMS-only driver...?
>>>>
>>>> Well exactly that's the point, nothing.
>>>>
>>>> The problem is that the option name is confusing to the end user because the
>>>> expectation is that "nomodeset" just means that they only can't change the
>>>> display mode.
>>>>
>>>> Some (unfortunately quite a lot) people don't realize that for KMS drivers
>>>> this means that the driver isn't even loaded and they also don't get any
>>>> acceleration.
>>>>
>>>> We had to explain that numerous times now. I think it would be best to give
>>>> the option a more meaningful name.
>>>
>>> Yeah, agreed with Christian. If we want a generic "pls disable all gfx
>>> accel" knob then probably best to put that into the drm core. And just
>>> outright fail loading the drm core if that happens, which will prevent all
>>> gfx drivers from loading.
>>>
>>> That likely means a hole bunch of stuff won't work (usually sound keels
>>> over too), but that's what you get for this. Only disabling modesetting
>>> without disabling the entire driver doesn't work (never has, except for
>>> this UMS+GEM combo only i915 support, and not for long).
>>>
>>> And once we have that knob, probably best to phase out all the per-driver
>>> options.
>>
>> Another use-case that the per-driver disables enable is "i915 works
>> but nouveau is broken due to crazy ACPI / PCIe PM / whatever". It
>> seems likely this could happen with amdgpu as well.
>
> modprobe.blacklist=amdgpu
>
> works as well as the modeset parameter for this.

People who build their own kernels run into trouble too. Also does
this work uniformly across all systems where it is a module? The
appeal of the kernel param is that it's fool-proof.

> FWIW, if a new parameter or other mechanism is added for this, ideally
> it should allow preventing initialization on a per-GPU basis, instead of
> only per-driver, for systems with multiple GPUs supported by the same
> driver.

Oh yeah, that'd be nice. (Optionally though, so that it's still easy
to use for the fairly common single gpu case.)
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Michel Dänzer]

On 2018-04-03 03:39 PM, Ilia Mirkin wrote:
> On Tue, Apr 3, 2018 at 9:32 AM, Michel Dänzer <michel@daenzer.net> wrote:
>> On 2018-04-03 03:26 PM, Ilia Mirkin wrote:
>>> On Tue, Apr 3, 2018 at 5:29 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>>> On Sun, Apr 01, 2018 at 10:12:10PM +0200, Christian König wrote:
>>>>> Am 01.04.2018 um 20:21 schrieb Takashi Iwai:
>>>>>> On Sun, 01 Apr 2018 19:58:11 +0200,
>>>>>> Christian K6nig wrote:
>>>>>>> Am 01.04.2018 um 19:45 schrieb Ilia Mirkin:
>>>>>>>> On Sun, Apr 1, 2018 at 1:39 PM, Christian König
>>>>>>>> <christian.koenig@amd.com> wrote:
>>>>>>>>> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
>>>>>>>>>> amdgpu driver lacks of modeset module option other drm drivers provide
>>>>>>>>>> for enforcing or disabling the driver load.  Interestingly, the
>>>>>>>>>> amdgpu_mode variable declaration is already found in the header file,
>>>>>>>>>> but the actual implementation seems to have been forgotten.
>>>>>>>>>>
>>>>>>>>>> This patch adds the missing piece.
>>>>>>>>> NAK, modesetting is mandatory for amdgpu and we should probably remove the
>>>>>>>>> option to disable it from other DRM drivers without UMS support as well
>>>>>>>>> (pretty much all of them now).
>>>>>>>>>
>>>>>>>>> If you want to prevent a driver from loading I think the correct way to do
>>>>>>>>> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
>>>>>>>>>
>>>>>>>>> That would remove the possibility to prevent the driver from loading when it
>>>>>>>>> is compiled in, but I don't see much of a problem with that.
>>>>>>>> Having a way to kill the graphics driver is a very useful debugging
>>>>>>>> tool, and also a quick and easy way to get out of an unpleasant
>>>>>>>> situation where graphics are messed up / system hangs / etc. The
>>>>>>>> modprobe blacklist kernel arg only works in certain environments (and
>>>>>>>> only if it's a module).
>>>>>>>>
>>>>>>>> Every other DRM driver has this and this is a well-documented
>>>>>>>> workaround for "graphics are messed up when I install linux", why not
>>>>>>>> allow a uniform experience for the end users who are just trying to
>>>>>>>> get their systems up and running?
>>>>>>> Because it is not well documented and repeated over and over again in
>>>>>>> drivers.
>>>>>>>
>>>>>>> The problem is that people don't realized that the driver isn't loaded
>>>>>>> at all without the modeset=0 module option and demand fixing the
>>>>>>> resulting bad performance without modesetting.
>>>>>> Hm, I don't get it.  What this options has to do with performance for
>>>>>> a KMS-only driver...?
>>>>>
>>>>> Well exactly that's the point, nothing.
>>>>>
>>>>> The problem is that the option name is confusing to the end user because the
>>>>> expectation is that "nomodeset" just means that they only can't change the
>>>>> display mode.
>>>>>
>>>>> Some (unfortunately quite a lot) people don't realize that for KMS drivers
>>>>> this means that the driver isn't even loaded and they also don't get any
>>>>> acceleration.
>>>>>
>>>>> We had to explain that numerous times now. I think it would be best to give
>>>>> the option a more meaningful name.
>>>>
>>>> Yeah, agreed with Christian. If we want a generic "pls disable all gfx
>>>> accel" knob then probably best to put that into the drm core. And just
>>>> outright fail loading the drm core if that happens, which will prevent all
>>>> gfx drivers from loading.
>>>>
>>>> That likely means a hole bunch of stuff won't work (usually sound keels
>>>> over too), but that's what you get for this. Only disabling modesetting
>>>> without disabling the entire driver doesn't work (never has, except for
>>>> this UMS+GEM combo only i915 support, and not for long).
>>>>
>>>> And once we have that knob, probably best to phase out all the per-driver
>>>> options.
>>>
>>> Another use-case that the per-driver disables enable is "i915 works
>>> but nouveau is broken due to crazy ACPI / PCIe PM / whatever". It
>>> seems likely this could happen with amdgpu as well.
>>
>> modprobe.blacklist=amdgpu
>>
>> works as well as the modeset parameter for this.
> 
> People who build their own kernels run into trouble too.

There have always been various more or less serious issues with building
amdgpu (and radeon) into the kernel. People who do so get to keep all
pieces when it breaks.


> Also does this work uniformly across all systems where it is a module?

AFAIK yes.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Ville Syrjälä]

On Tue, Apr 03, 2018 at 03:47:57PM +0200, Michel Dänzer wrote:
> On 2018-04-03 03:39 PM, Ilia Mirkin wrote:
> > On Tue, Apr 3, 2018 at 9:32 AM, Michel Dänzer <michel@daenzer.net> wrote:
> >> On 2018-04-03 03:26 PM, Ilia Mirkin wrote:
> >>> On Tue, Apr 3, 2018 at 5:29 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >>>> On Sun, Apr 01, 2018 at 10:12:10PM +0200, Christian König wrote:
> >>>>> Am 01.04.2018 um 20:21 schrieb Takashi Iwai:
> >>>>>> On Sun, 01 Apr 2018 19:58:11 +0200,
> >>>>>> Christian K6nig wrote:
> >>>>>>> Am 01.04.2018 um 19:45 schrieb Ilia Mirkin:
> >>>>>>>> On Sun, Apr 1, 2018 at 1:39 PM, Christian König
> >>>>>>>> <christian.koenig@amd.com> wrote:
> >>>>>>>>> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
> >>>>>>>>>> amdgpu driver lacks of modeset module option other drm drivers provide
> >>>>>>>>>> for enforcing or disabling the driver load.  Interestingly, the
> >>>>>>>>>> amdgpu_mode variable declaration is already found in the header file,
> >>>>>>>>>> but the actual implementation seems to have been forgotten.
> >>>>>>>>>>
> >>>>>>>>>> This patch adds the missing piece.
> >>>>>>>>> NAK, modesetting is mandatory for amdgpu and we should probably remove the
> >>>>>>>>> option to disable it from other DRM drivers without UMS support as well
> >>>>>>>>> (pretty much all of them now).
> >>>>>>>>>
> >>>>>>>>> If you want to prevent a driver from loading I think the correct way to do
> >>>>>>>>> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
> >>>>>>>>>
> >>>>>>>>> That would remove the possibility to prevent the driver from loading when it
> >>>>>>>>> is compiled in, but I don't see much of a problem with that.
> >>>>>>>> Having a way to kill the graphics driver is a very useful debugging
> >>>>>>>> tool, and also a quick and easy way to get out of an unpleasant
> >>>>>>>> situation where graphics are messed up / system hangs / etc. The
> >>>>>>>> modprobe blacklist kernel arg only works in certain environments (and
> >>>>>>>> only if it's a module).
> >>>>>>>>
> >>>>>>>> Every other DRM driver has this and this is a well-documented
> >>>>>>>> workaround for "graphics are messed up when I install linux", why not
> >>>>>>>> allow a uniform experience for the end users who are just trying to
> >>>>>>>> get their systems up and running?
> >>>>>>> Because it is not well documented and repeated over and over again in
> >>>>>>> drivers.
> >>>>>>>
> >>>>>>> The problem is that people don't realized that the driver isn't loaded
> >>>>>>> at all without the modeset=0 module option and demand fixing the
> >>>>>>> resulting bad performance without modesetting.
> >>>>>> Hm, I don't get it.  What this options has to do with performance for
> >>>>>> a KMS-only driver...?
> >>>>>
> >>>>> Well exactly that's the point, nothing.
> >>>>>
> >>>>> The problem is that the option name is confusing to the end user because the
> >>>>> expectation is that "nomodeset" just means that they only can't change the
> >>>>> display mode.
> >>>>>
> >>>>> Some (unfortunately quite a lot) people don't realize that for KMS drivers
> >>>>> this means that the driver isn't even loaded and they also don't get any
> >>>>> acceleration.
> >>>>>
> >>>>> We had to explain that numerous times now. I think it would be best to give
> >>>>> the option a more meaningful name.
> >>>>
> >>>> Yeah, agreed with Christian. If we want a generic "pls disable all gfx
> >>>> accel" knob then probably best to put that into the drm core. And just
> >>>> outright fail loading the drm core if that happens, which will prevent all
> >>>> gfx drivers from loading.
> >>>>
> >>>> That likely means a hole bunch of stuff won't work (usually sound keels
> >>>> over too), but that's what you get for this. Only disabling modesetting
> >>>> without disabling the entire driver doesn't work (never has, except for
> >>>> this UMS+GEM combo only i915 support, and not for long).
> >>>>
> >>>> And once we have that knob, probably best to phase out all the per-driver
> >>>> options.
> >>>
> >>> Another use-case that the per-driver disables enable is "i915 works
> >>> but nouveau is broken due to crazy ACPI / PCIe PM / whatever". It
> >>> seems likely this could happen with amdgpu as well.
> >>
> >> modprobe.blacklist=amdgpu
> >>
> >> works as well as the modeset parameter for this.
> > 
> > People who build their own kernels run into trouble too.
> 
> There have always been various more or less serious issues with building
> amdgpu (and radeon) into the kernel. People who do so get to keep all
> pieces when it breaks.
> 
> 
> > Also does this work uniformly across all systems where it is a module?
> 
> AFAIK yes.

Sadly modprobe.blacklist doesn't prevent X/whatever from loading the
module anyway.

I use modprobe.blacklist myself all the time for i915 development,
but I also have all GUI junk disabled as well so that I can load the
module myself when I'm actually ready for it (typically after I've
enabled netconsole).

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Daniel Vetter]

On Tue, Apr 03, 2018 at 05:06:03PM +0300, Ville Syrjälä wrote:
> On Tue, Apr 03, 2018 at 03:47:57PM +0200, Michel Dänzer wrote:
> > On 2018-04-03 03:39 PM, Ilia Mirkin wrote:
> > > On Tue, Apr 3, 2018 at 9:32 AM, Michel Dänzer <michel@daenzer.net> wrote:
> > >> On 2018-04-03 03:26 PM, Ilia Mirkin wrote:
> > >>> On Tue, Apr 3, 2018 at 5:29 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > >>>> On Sun, Apr 01, 2018 at 10:12:10PM +0200, Christian König wrote:
> > >>>>> Am 01.04.2018 um 20:21 schrieb Takashi Iwai:
> > >>>>>> On Sun, 01 Apr 2018 19:58:11 +0200,
> > >>>>>> Christian K6nig wrote:
> > >>>>>>> Am 01.04.2018 um 19:45 schrieb Ilia Mirkin:
> > >>>>>>>> On Sun, Apr 1, 2018 at 1:39 PM, Christian König
> > >>>>>>>> <christian.koenig@amd.com> wrote:
> > >>>>>>>>> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
> > >>>>>>>>>> amdgpu driver lacks of modeset module option other drm drivers provide
> > >>>>>>>>>> for enforcing or disabling the driver load.  Interestingly, the
> > >>>>>>>>>> amdgpu_mode variable declaration is already found in the header file,
> > >>>>>>>>>> but the actual implementation seems to have been forgotten.
> > >>>>>>>>>>
> > >>>>>>>>>> This patch adds the missing piece.
> > >>>>>>>>> NAK, modesetting is mandatory for amdgpu and we should probably remove the
> > >>>>>>>>> option to disable it from other DRM drivers without UMS support as well
> > >>>>>>>>> (pretty much all of them now).
> > >>>>>>>>>
> > >>>>>>>>> If you want to prevent a driver from loading I think the correct way to do
> > >>>>>>>>> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
> > >>>>>>>>>
> > >>>>>>>>> That would remove the possibility to prevent the driver from loading when it
> > >>>>>>>>> is compiled in, but I don't see much of a problem with that.
> > >>>>>>>> Having a way to kill the graphics driver is a very useful debugging
> > >>>>>>>> tool, and also a quick and easy way to get out of an unpleasant
> > >>>>>>>> situation where graphics are messed up / system hangs / etc. The
> > >>>>>>>> modprobe blacklist kernel arg only works in certain environments (and
> > >>>>>>>> only if it's a module).
> > >>>>>>>>
> > >>>>>>>> Every other DRM driver has this and this is a well-documented
> > >>>>>>>> workaround for "graphics are messed up when I install linux", why not
> > >>>>>>>> allow a uniform experience for the end users who are just trying to
> > >>>>>>>> get their systems up and running?
> > >>>>>>> Because it is not well documented and repeated over and over again in
> > >>>>>>> drivers.
> > >>>>>>>
> > >>>>>>> The problem is that people don't realized that the driver isn't loaded
> > >>>>>>> at all without the modeset=0 module option and demand fixing the
> > >>>>>>> resulting bad performance without modesetting.
> > >>>>>> Hm, I don't get it.  What this options has to do with performance for
> > >>>>>> a KMS-only driver...?
> > >>>>>
> > >>>>> Well exactly that's the point, nothing.
> > >>>>>
> > >>>>> The problem is that the option name is confusing to the end user because the
> > >>>>> expectation is that "nomodeset" just means that they only can't change the
> > >>>>> display mode.
> > >>>>>
> > >>>>> Some (unfortunately quite a lot) people don't realize that for KMS drivers
> > >>>>> this means that the driver isn't even loaded and they also don't get any
> > >>>>> acceleration.
> > >>>>>
> > >>>>> We had to explain that numerous times now. I think it would be best to give
> > >>>>> the option a more meaningful name.
> > >>>>
> > >>>> Yeah, agreed with Christian. If we want a generic "pls disable all gfx
> > >>>> accel" knob then probably best to put that into the drm core. And just
> > >>>> outright fail loading the drm core if that happens, which will prevent all
> > >>>> gfx drivers from loading.
> > >>>>
> > >>>> That likely means a hole bunch of stuff won't work (usually sound keels
> > >>>> over too), but that's what you get for this. Only disabling modesetting
> > >>>> without disabling the entire driver doesn't work (never has, except for
> > >>>> this UMS+GEM combo only i915 support, and not for long).
> > >>>>
> > >>>> And once we have that knob, probably best to phase out all the per-driver
> > >>>> options.
> > >>>
> > >>> Another use-case that the per-driver disables enable is "i915 works
> > >>> but nouveau is broken due to crazy ACPI / PCIe PM / whatever". It
> > >>> seems likely this could happen with amdgpu as well.
> > >>
> > >> modprobe.blacklist=amdgpu
> > >>
> > >> works as well as the modeset parameter for this.
> > > 
> > > People who build their own kernels run into trouble too.
> > 
> > There have always been various more or less serious issues with building
> > amdgpu (and radeon) into the kernel. People who do so get to keep all
> > pieces when it breaks.

fwiw, same roughly applies for i915. We try to make it work, by allowing
firmware to be loaded later on and asynchronously. But if you fail to
provide firmware the driver simply hangs forever. At least in certain
cases, for others we simply never clock gate/shutdown the chip. Also not
awesome (and we're closing all corresponding bug reports too).

We don't yet need firmware for basic display support, so you'll get at
least a picture to be able to look at dmesg. If you remember to start X
without accel, that is :-)

> > > Also does this work uniformly across all systems where it is a module?
> > 
> > AFAIK yes.
> 
> Sadly modprobe.blacklist doesn't prevent X/whatever from loading the
> module anyway.

A generic "please don't bind anything to this device path" kernel property
would be really nice for this. And hopefully something that could be
merged into the drivers/base core code.

Plus a generic and all-encompassing no_modeset^Wno_gfx_I_mean_it knob
would cover everything I think.
-Daniel

> I use modprobe.blacklist myself all the time for i915 development,
> but I also have all GUI junk disabled as well so that I can load the
> module myself when I'm actually ready for it (typically after I've
> enabled netconsole).

Oh, that's why modprobe.blacklist never works for me. TIL.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Ville Syrjälä]

On Tue, Apr 03, 2018 at 05:02:35PM +0200, Daniel Vetter wrote:
> On Tue, Apr 03, 2018 at 05:06:03PM +0300, Ville Syrjälä wrote:
> > On Tue, Apr 03, 2018 at 03:47:57PM +0200, Michel Dänzer wrote:
> > > On 2018-04-03 03:39 PM, Ilia Mirkin wrote:
> > > > On Tue, Apr 3, 2018 at 9:32 AM, Michel Dänzer <michel@daenzer.net> wrote:
> > > >> On 2018-04-03 03:26 PM, Ilia Mirkin wrote:
> > > >>> On Tue, Apr 3, 2018 at 5:29 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >>>> On Sun, Apr 01, 2018 at 10:12:10PM +0200, Christian König wrote:
> > > >>>>> Am 01.04.2018 um 20:21 schrieb Takashi Iwai:
> > > >>>>>> On Sun, 01 Apr 2018 19:58:11 +0200,
> > > >>>>>> Christian K6nig wrote:
> > > >>>>>>> Am 01.04.2018 um 19:45 schrieb Ilia Mirkin:
> > > >>>>>>>> On Sun, Apr 1, 2018 at 1:39 PM, Christian König
> > > >>>>>>>> <christian.koenig@amd.com> wrote:
> > > >>>>>>>>> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
> > > >>>>>>>>>> amdgpu driver lacks of modeset module option other drm drivers provide
> > > >>>>>>>>>> for enforcing or disabling the driver load.  Interestingly, the
> > > >>>>>>>>>> amdgpu_mode variable declaration is already found in the header file,
> > > >>>>>>>>>> but the actual implementation seems to have been forgotten.
> > > >>>>>>>>>>
> > > >>>>>>>>>> This patch adds the missing piece.
> > > >>>>>>>>> NAK, modesetting is mandatory for amdgpu and we should probably remove the
> > > >>>>>>>>> option to disable it from other DRM drivers without UMS support as well
> > > >>>>>>>>> (pretty much all of them now).
> > > >>>>>>>>>
> > > >>>>>>>>> If you want to prevent a driver from loading I think the correct way to do
> > > >>>>>>>>> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
> > > >>>>>>>>>
> > > >>>>>>>>> That would remove the possibility to prevent the driver from loading when it
> > > >>>>>>>>> is compiled in, but I don't see much of a problem with that.
> > > >>>>>>>> Having a way to kill the graphics driver is a very useful debugging
> > > >>>>>>>> tool, and also a quick and easy way to get out of an unpleasant
> > > >>>>>>>> situation where graphics are messed up / system hangs / etc. The
> > > >>>>>>>> modprobe blacklist kernel arg only works in certain environments (and
> > > >>>>>>>> only if it's a module).
> > > >>>>>>>>
> > > >>>>>>>> Every other DRM driver has this and this is a well-documented
> > > >>>>>>>> workaround for "graphics are messed up when I install linux", why not
> > > >>>>>>>> allow a uniform experience for the end users who are just trying to
> > > >>>>>>>> get their systems up and running?
> > > >>>>>>> Because it is not well documented and repeated over and over again in
> > > >>>>>>> drivers.
> > > >>>>>>>
> > > >>>>>>> The problem is that people don't realized that the driver isn't loaded
> > > >>>>>>> at all without the modeset=0 module option and demand fixing the
> > > >>>>>>> resulting bad performance without modesetting.
> > > >>>>>> Hm, I don't get it.  What this options has to do with performance for
> > > >>>>>> a KMS-only driver...?
> > > >>>>>
> > > >>>>> Well exactly that's the point, nothing.
> > > >>>>>
> > > >>>>> The problem is that the option name is confusing to the end user because the
> > > >>>>> expectation is that "nomodeset" just means that they only can't change the
> > > >>>>> display mode.
> > > >>>>>
> > > >>>>> Some (unfortunately quite a lot) people don't realize that for KMS drivers
> > > >>>>> this means that the driver isn't even loaded and they also don't get any
> > > >>>>> acceleration.
> > > >>>>>
> > > >>>>> We had to explain that numerous times now. I think it would be best to give
> > > >>>>> the option a more meaningful name.
> > > >>>>
> > > >>>> Yeah, agreed with Christian. If we want a generic "pls disable all gfx
> > > >>>> accel" knob then probably best to put that into the drm core. And just
> > > >>>> outright fail loading the drm core if that happens, which will prevent all
> > > >>>> gfx drivers from loading.
> > > >>>>
> > > >>>> That likely means a hole bunch of stuff won't work (usually sound keels
> > > >>>> over too), but that's what you get for this. Only disabling modesetting
> > > >>>> without disabling the entire driver doesn't work (never has, except for
> > > >>>> this UMS+GEM combo only i915 support, and not for long).
> > > >>>>
> > > >>>> And once we have that knob, probably best to phase out all the per-driver
> > > >>>> options.
> > > >>>
> > > >>> Another use-case that the per-driver disables enable is "i915 works
> > > >>> but nouveau is broken due to crazy ACPI / PCIe PM / whatever". It
> > > >>> seems likely this could happen with amdgpu as well.
> > > >>
> > > >> modprobe.blacklist=amdgpu
> > > >>
> > > >> works as well as the modeset parameter for this.
> > > > 
> > > > People who build their own kernels run into trouble too.
> > > 
> > > There have always been various more or less serious issues with building
> > > amdgpu (and radeon) into the kernel. People who do so get to keep all
> > > pieces when it breaks.
> 
> fwiw, same roughly applies for i915. We try to make it work, by allowing
> firmware to be loaded later on and asynchronously. But if you fail to
> provide firmware the driver simply hangs forever. At least in certain
> cases, for others we simply never clock gate/shutdown the chip. Also not
> awesome (and we're closing all corresponding bug reports too).
> 
> We don't yet need firmware for basic display support, so you'll get at
> least a picture to be able to look at dmesg. If you remember to start X
> without accel, that is :-)
> 
> > > > Also does this work uniformly across all systems where it is a module?
> > > 
> > > AFAIK yes.
> > 
> > Sadly modprobe.blacklist doesn't prevent X/whatever from loading the
> > module anyway.
> 
> A generic "please don't bind anything to this device path" kernel property
> would be really nice for this. And hopefully something that could be
> merged into the drivers/base core code.
> 
> Plus a generic and all-encompassing no_modeset^Wno_gfx_I_mean_it knob
> would cover everything I think.
> -Daniel
> 
> > I use modprobe.blacklist myself all the time for i915 development,
> > but I also have all GUI junk disabled as well so that I can load the
> > module myself when I'm actually ready for it (typically after I've
> > enabled netconsole).
> 
> Oh, that's why modprobe.blacklist never works for me. TIL.

The other common reason is snd_hda_intel. That one needs to be blacklisted
at the same time, on modern Intel hw at least. Also one must remember to
write it as "snd_hda_intel" instead of "snd-hda-intel", otherwise it doesn't
do anything.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Michel Dänzer]

On 2018-04-03 04:06 PM, Ville Syrjälä wrote:
> On Tue, Apr 03, 2018 at 03:47:57PM +0200, Michel Dänzer wrote:
>> On 2018-04-03 03:39 PM, Ilia Mirkin wrote:
>>> On Tue, Apr 3, 2018 at 9:32 AM, Michel Dänzer <michel@daenzer.net> wrote:
>>>> On 2018-04-03 03:26 PM, Ilia Mirkin wrote:
>>>>> On Tue, Apr 3, 2018 at 5:29 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>>>>> On Sun, Apr 01, 2018 at 10:12:10PM +0200, Christian König wrote:
>>>>>>> Am 01.04.2018 um 20:21 schrieb Takashi Iwai:
>>>>>>>> On Sun, 01 Apr 2018 19:58:11 +0200,
>>>>>>>> Christian K6nig wrote:
>>>>>>>>> Am 01.04.2018 um 19:45 schrieb Ilia Mirkin:
>>>>>>>>>> On Sun, Apr 1, 2018 at 1:39 PM, Christian König
>>>>>>>>>> <christian.koenig@amd.com> wrote:
>>>>>>>>>>> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
>>>>>>>>>>>> amdgpu driver lacks of modeset module option other drm drivers provide
>>>>>>>>>>>> for enforcing or disabling the driver load.  Interestingly, the
>>>>>>>>>>>> amdgpu_mode variable declaration is already found in the header file,
>>>>>>>>>>>> but the actual implementation seems to have been forgotten.
>>>>>>>>>>>>
>>>>>>>>>>>> This patch adds the missing piece.
>>>>>>>>>>> NAK, modesetting is mandatory for amdgpu and we should probably remove the
>>>>>>>>>>> option to disable it from other DRM drivers without UMS support as well
>>>>>>>>>>> (pretty much all of them now).
>>>>>>>>>>>
>>>>>>>>>>> If you want to prevent a driver from loading I think the correct way to do
>>>>>>>>>>> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
>>>>>>>>>>>
>>>>>>>>>>> That would remove the possibility to prevent the driver from loading when it
>>>>>>>>>>> is compiled in, but I don't see much of a problem with that.
>>>>>>>>>> Having a way to kill the graphics driver is a very useful debugging
>>>>>>>>>> tool, and also a quick and easy way to get out of an unpleasant
>>>>>>>>>> situation where graphics are messed up / system hangs / etc. The
>>>>>>>>>> modprobe blacklist kernel arg only works in certain environments (and
>>>>>>>>>> only if it's a module).
>>>>>>>>>>
>>>>>>>>>> Every other DRM driver has this and this is a well-documented
>>>>>>>>>> workaround for "graphics are messed up when I install linux", why not
>>>>>>>>>> allow a uniform experience for the end users who are just trying to
>>>>>>>>>> get their systems up and running?
>>>>>>>>> Because it is not well documented and repeated over and over again in
>>>>>>>>> drivers.
>>>>>>>>>
>>>>>>>>> The problem is that people don't realized that the driver isn't loaded
>>>>>>>>> at all without the modeset=0 module option and demand fixing the
>>>>>>>>> resulting bad performance without modesetting.
>>>>>>>> Hm, I don't get it.  What this options has to do with performance for
>>>>>>>> a KMS-only driver...?
>>>>>>>
>>>>>>> Well exactly that's the point, nothing.
>>>>>>>
>>>>>>> The problem is that the option name is confusing to the end user because the
>>>>>>> expectation is that "nomodeset" just means that they only can't change the
>>>>>>> display mode.
>>>>>>>
>>>>>>> Some (unfortunately quite a lot) people don't realize that for KMS drivers
>>>>>>> this means that the driver isn't even loaded and they also don't get any
>>>>>>> acceleration.
>>>>>>>
>>>>>>> We had to explain that numerous times now. I think it would be best to give
>>>>>>> the option a more meaningful name.
>>>>>>
>>>>>> Yeah, agreed with Christian. If we want a generic "pls disable all gfx
>>>>>> accel" knob then probably best to put that into the drm core. And just
>>>>>> outright fail loading the drm core if that happens, which will prevent all
>>>>>> gfx drivers from loading.
>>>>>>
>>>>>> That likely means a hole bunch of stuff won't work (usually sound keels
>>>>>> over too), but that's what you get for this. Only disabling modesetting
>>>>>> without disabling the entire driver doesn't work (never has, except for
>>>>>> this UMS+GEM combo only i915 support, and not for long).
>>>>>>
>>>>>> And once we have that knob, probably best to phase out all the per-driver
>>>>>> options.
>>>>>
>>>>> Another use-case that the per-driver disables enable is "i915 works
>>>>> but nouveau is broken due to crazy ACPI / PCIe PM / whatever". It
>>>>> seems likely this could happen with amdgpu as well.
>>>>
>>>> modprobe.blacklist=amdgpu
>>>>
>>>> works as well as the modeset parameter for this.
>>>
>>> People who build their own kernels run into trouble too.
>>
>> There have always been various more or less serious issues with building
>> amdgpu (and radeon) into the kernel. People who do so get to keep all
>> pieces when it breaks.
>>
>>
>>> Also does this work uniformly across all systems where it is a module?
>>
>> AFAIK yes.
> 
> Sadly modprobe.blacklist doesn't prevent X/whatever from loading the
> module anyway.

AFAICT xf86-video-amdgpu and modesetting have never even tried loading
the kernel module, and neither has xf86-video-ati for years at least.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Michel Dänzer]

On 2018-04-01 07:45 PM, Ilia Mirkin wrote:
> On Sun, Apr 1, 2018 at 1:39 PM, Christian König
> <christian.koenig@amd.com> wrote:
>> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
>>>
>>> amdgpu driver lacks of modeset module option other drm drivers provide
>>> for enforcing or disabling the driver load.  Interestingly, the
>>> amdgpu_mode variable declaration is already found in the header file,
>>> but the actual implementation seems to have been forgotten.
>>>
>>> This patch adds the missing piece.
>>
>>
>> NAK, modesetting is mandatory for amdgpu and we should probably remove the
>> option to disable it from other DRM drivers without UMS support as well
>> (pretty much all of them now).
>>
>> If you want to prevent a driver from loading I think the correct way to do
>> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
>>
>> That would remove the possibility to prevent the driver from loading when it
>> is compiled in, but I don't see much of a problem with that.
> 
> Having a way to kill the graphics driver is a very useful debugging
> tool, and also a quick and easy way to get out of an unpleasant
> situation where graphics are messed up / system hangs / etc. The
> modprobe blacklist kernel arg only works in certain environments (and
> only if it's a module).

Building amdgpu into the kernel isn't feasible for a generic kernel such
as a distro one, because it would require including all microcode into
the kernel as well (12M right now, and growing).

If a user decides to build amdgpu into their custom kernel and runs into
trouble due to that, that's "doctor, it hurts if I do this" territory.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Christian König]

Am 03.04.2018 um 10:36 schrieb Michel Dänzer:
> On 2018-04-01 07:45 PM, Ilia Mirkin wrote:
>> On Sun, Apr 1, 2018 at 1:39 PM, Christian König
>> <christian.koenig@amd.com> wrote:
>>> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
>>>> amdgpu driver lacks of modeset module option other drm drivers provide
>>>> for enforcing or disabling the driver load.  Interestingly, the
>>>> amdgpu_mode variable declaration is already found in the header file,
>>>> but the actual implementation seems to have been forgotten.
>>>>
>>>> This patch adds the missing piece.
>>>
>>> NAK, modesetting is mandatory for amdgpu and we should probably remove the
>>> option to disable it from other DRM drivers without UMS support as well
>>> (pretty much all of them now).
>>>
>>> If you want to prevent a driver from loading I think the correct way to do
>>> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
>>>
>>> That would remove the possibility to prevent the driver from loading when it
>>> is compiled in, but I don't see much of a problem with that.
>> Having a way to kill the graphics driver is a very useful debugging
>> tool, and also a quick and easy way to get out of an unpleasant
>> situation where graphics are messed up / system hangs / etc. The
>> modprobe blacklist kernel arg only works in certain environments (and
>> only if it's a module).
> Building amdgpu into the kernel isn't feasible for a generic kernel such
> as a distro one, because it would require including all microcode into
> the kernel as well (12M right now, and growing).
>
> If a user decides to build amdgpu into their custom kernel and runs into
> trouble due to that, that's "doctor, it hurts if I do this" territory.

Correct, but I agree that even in this situation it would be very 
helpful to prevent the gfx drivers from loading and fallback to 
efifb/vesafd (or whatever the platform provides).

It's just that the "nomodeset" and "amdgpu.modeset=0" options are really 
not well named for this task.

Christian.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Takashi Iwai]

On Tue, 03 Apr 2018 10:57:56 +0200,
Christian K6nig wrote:
> 
> Am 03.04.2018 um 10:36 schrieb Michel Dänzer:
> > On 2018-04-01 07:45 PM, Ilia Mirkin wrote:
> >> On Sun, Apr 1, 2018 at 1:39 PM, Christian König
> >> <christian.koenig@amd.com> wrote:
> >>> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
> >>>> amdgpu driver lacks of modeset module option other drm drivers provide
> >>>> for enforcing or disabling the driver load.  Interestingly, the
> >>>> amdgpu_mode variable declaration is already found in the header file,
> >>>> but the actual implementation seems to have been forgotten.
> >>>>
> >>>> This patch adds the missing piece.
> >>>
> >>> NAK, modesetting is mandatory for amdgpu and we should probably remove the
> >>> option to disable it from other DRM drivers without UMS support as well
> >>> (pretty much all of them now).
> >>>
> >>> If you want to prevent a driver from loading I think the correct way to do
> >>> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
> >>>
> >>> That would remove the possibility to prevent the driver from loading when it
> >>> is compiled in, but I don't see much of a problem with that.
> >> Having a way to kill the graphics driver is a very useful debugging
> >> tool, and also a quick and easy way to get out of an unpleasant
> >> situation where graphics are messed up / system hangs / etc. The
> >> modprobe blacklist kernel arg only works in certain environments (and
> >> only if it's a module).
> > Building amdgpu into the kernel isn't feasible for a generic kernel such
> > as a distro one, because it would require including all microcode into
> > the kernel as well (12M right now, and growing).
> >
> > If a user decides to build amdgpu into their custom kernel and runs into
> > trouble due to that, that's "doctor, it hurts if I do this" territory.
> 
> Correct, but I agree that even in this situation it would be very
> helpful to prevent the gfx drivers from loading and fallback to
> efifb/vesafd (or whatever the platform provides).
> 
> It's just that the "nomodeset" and "amdgpu.modeset=0" options are
> really not well named for this task.

Agreed with the naming mess.  But OTOH, it's already a thing that is
too popular to kill.  You can add a more suitable option name, but you
cannot drop these existing ones easily.  It's already in a gray zone
of the golden "don't break user-space" rule.


thanks,

Takashi
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Michel Dänzer]

On 2018-04-03 11:02 AM, Takashi Iwai wrote:
> On Tue, 03 Apr 2018 10:57:56 +0200,
> Christian K6nig wrote:
>>
>> Am 03.04.2018 um 10:36 schrieb Michel Dänzer:
>>> On 2018-04-01 07:45 PM, Ilia Mirkin wrote:
>>>> On Sun, Apr 1, 2018 at 1:39 PM, Christian König
>>>> <christian.koenig@amd.com> wrote:
>>>>> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
>>>>>> amdgpu driver lacks of modeset module option other drm drivers provide
>>>>>> for enforcing or disabling the driver load.  Interestingly, the
>>>>>> amdgpu_mode variable declaration is already found in the header file,
>>>>>> but the actual implementation seems to have been forgotten.
>>>>>>
>>>>>> This patch adds the missing piece.
>>>>>
>>>>> NAK, modesetting is mandatory for amdgpu and we should probably remove the
>>>>> option to disable it from other DRM drivers without UMS support as well
>>>>> (pretty much all of them now).
>>>>>
>>>>> If you want to prevent a driver from loading I think the correct way to do
>>>>> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
>>>>>
>>>>> That would remove the possibility to prevent the driver from loading when it
>>>>> is compiled in, but I don't see much of a problem with that.
>>>> Having a way to kill the graphics driver is a very useful debugging
>>>> tool, and also a quick and easy way to get out of an unpleasant
>>>> situation where graphics are messed up / system hangs / etc. The
>>>> modprobe blacklist kernel arg only works in certain environments (and
>>>> only if it's a module).
>>> Building amdgpu into the kernel isn't feasible for a generic kernel such
>>> as a distro one, because it would require including all microcode into
>>> the kernel as well (12M right now, and growing).
>>>
>>> If a user decides to build amdgpu into their custom kernel and runs into
>>> trouble due to that, that's "doctor, it hurts if I do this" territory.
>>
>> Correct, but I agree that even in this situation it would be very
>> helpful to prevent the gfx drivers from loading and fallback to
>> efifb/vesafd (or whatever the platform provides).
>>
>> It's just that the "nomodeset" and "amdgpu.modeset=0" options are
>> really not well named for this task.
> 
> Agreed with the naming mess.  But OTOH, it's already a thing that is
> too popular to kill.  You can add a more suitable option name, but you
> cannot drop these existing ones easily.  It's already in a gray zone
> of the golden "don't break user-space" rule.

That's quite a stretch argument, given that amdgpu has never supported
the modeset parameter. Also, module parameters aren't UAPI.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Takashi Iwai]

On Tue, 03 Apr 2018 11:18:34 +0200,
Michel D4nzer wrote:
> 
> On 2018-04-03 11:02 AM, Takashi Iwai wrote:
> > On Tue, 03 Apr 2018 10:57:56 +0200,
> > Christian K6nig wrote:
> >>
> >> Am 03.04.2018 um 10:36 schrieb Michel Dänzer:
> >>> On 2018-04-01 07:45 PM, Ilia Mirkin wrote:
> >>>> On Sun, Apr 1, 2018 at 1:39 PM, Christian König
> >>>> <christian.koenig@amd.com> wrote:
> >>>>> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
> >>>>>> amdgpu driver lacks of modeset module option other drm drivers provide
> >>>>>> for enforcing or disabling the driver load.  Interestingly, the
> >>>>>> amdgpu_mode variable declaration is already found in the header file,
> >>>>>> but the actual implementation seems to have been forgotten.
> >>>>>>
> >>>>>> This patch adds the missing piece.
> >>>>>
> >>>>> NAK, modesetting is mandatory for amdgpu and we should probably remove the
> >>>>> option to disable it from other DRM drivers without UMS support as well
> >>>>> (pretty much all of them now).
> >>>>>
> >>>>> If you want to prevent a driver from loading I think the correct way to do
> >>>>> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
> >>>>>
> >>>>> That would remove the possibility to prevent the driver from loading when it
> >>>>> is compiled in, but I don't see much of a problem with that.
> >>>> Having a way to kill the graphics driver is a very useful debugging
> >>>> tool, and also a quick and easy way to get out of an unpleasant
> >>>> situation where graphics are messed up / system hangs / etc. The
> >>>> modprobe blacklist kernel arg only works in certain environments (and
> >>>> only if it's a module).
> >>> Building amdgpu into the kernel isn't feasible for a generic kernel such
> >>> as a distro one, because it would require including all microcode into
> >>> the kernel as well (12M right now, and growing).
> >>>
> >>> If a user decides to build amdgpu into their custom kernel and runs into
> >>> trouble due to that, that's "doctor, it hurts if I do this" territory.
> >>
> >> Correct, but I agree that even in this situation it would be very
> >> helpful to prevent the gfx drivers from loading and fallback to
> >> efifb/vesafd (or whatever the platform provides).
> >>
> >> It's just that the "nomodeset" and "amdgpu.modeset=0" options are
> >> really not well named for this task.
> > 
> > Agreed with the naming mess.  But OTOH, it's already a thing that is
> > too popular to kill.  You can add a more suitable option name, but you
> > cannot drop these existing ones easily.  It's already in a gray zone
> > of the golden "don't break user-space" rule.
> 
> That's quite a stretch argument, given that amdgpu has never supported
> the modeset parameter.

Oh I don't mean about my own patch but the foreseen action Christian
mentioned.

> Also, module parameters aren't UAPI.

Right, but we care not only about UAPI.  If the kernel breaks
something, it's a regression.  It's what Linus suggested many times.
The same argument has been applied to proc or sysfs files in the past,
and we had to correct back again.

Don't get me wrong: I'm not always objecting to the removal of any
module existing options.  But if it break a major usage pattern, it's
a problem.  And the removal of nomodeset option would be a kind of
such things, IMO, that's why I mentioned as "a gray zone" in the
above.


thanks,

Takashi
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Michel Dänzer]

On 2018-04-03 11:44 AM, Takashi Iwai wrote:
> On Tue, 03 Apr 2018 11:18:34 +0200,
> Michel D4nzer wrote:
>>
>> On 2018-04-03 11:02 AM, Takashi Iwai wrote:
>>> On Tue, 03 Apr 2018 10:57:56 +0200,
>>> Christian K6nig wrote:
>>>>
>>>> Am 03.04.2018 um 10:36 schrieb Michel Dänzer:
>>>>> On 2018-04-01 07:45 PM, Ilia Mirkin wrote:
>>>>>> On Sun, Apr 1, 2018 at 1:39 PM, Christian König
>>>>>> <christian.koenig@amd.com> wrote:
>>>>>>> Am 30.03.2018 um 22:45 schrieb Takashi Iwai:
>>>>>>>> amdgpu driver lacks of modeset module option other drm drivers provide
>>>>>>>> for enforcing or disabling the driver load.  Interestingly, the
>>>>>>>> amdgpu_mode variable declaration is already found in the header file,
>>>>>>>> but the actual implementation seems to have been forgotten.
>>>>>>>>
>>>>>>>> This patch adds the missing piece.
>>>>>>>
>>>>>>> NAK, modesetting is mandatory for amdgpu and we should probably remove the
>>>>>>> option to disable it from other DRM drivers without UMS support as well
>>>>>>> (pretty much all of them now).
>>>>>>>
>>>>>>> If you want to prevent a driver from loading I think the correct way to do
>>>>>>> so is to give modprobe.blacklist=amdgpu on the kernel commandline.
>>>>>>>
>>>>>>> That would remove the possibility to prevent the driver from loading when it
>>>>>>> is compiled in, but I don't see much of a problem with that.
>>>>>> Having a way to kill the graphics driver is a very useful debugging
>>>>>> tool, and also a quick and easy way to get out of an unpleasant
>>>>>> situation where graphics are messed up / system hangs / etc. The
>>>>>> modprobe blacklist kernel arg only works in certain environments (and
>>>>>> only if it's a module).
>>>>> Building amdgpu into the kernel isn't feasible for a generic kernel such
>>>>> as a distro one, because it would require including all microcode into
>>>>> the kernel as well (12M right now, and growing).
>>>>>
>>>>> If a user decides to build amdgpu into their custom kernel and runs into
>>>>> trouble due to that, that's "doctor, it hurts if I do this" territory.
>>>>
>>>> Correct, but I agree that even in this situation it would be very
>>>> helpful to prevent the gfx drivers from loading and fallback to
>>>> efifb/vesafd (or whatever the platform provides).
>>>>
>>>> It's just that the "nomodeset" and "amdgpu.modeset=0" options are
>>>> really not well named for this task.
>>>
>>> Agreed with the naming mess.  But OTOH, it's already a thing that is
>>> too popular to kill.  You can add a more suitable option name, but you
>>> cannot drop these existing ones easily.  It's already in a gray zone
>>> of the golden "don't break user-space" rule.
>>
>> That's quite a stretch argument, given that amdgpu has never supported
>> the modeset parameter.
> 
> Oh I don't mean about my own patch but the foreseen action Christian
> mentioned.
> 
>> Also, module parameters aren't UAPI.
> 
> Right, but we care not only about UAPI.  If the kernel breaks
> something, it's a regression.  It's what Linus suggested many times.
> The same argument has been applied to proc or sysfs files in the past,
> and we had to correct back again.
> 
> Don't get me wrong: I'm not always objecting to the removal of any
> module existing options.  But if it break a major usage pattern, it's
> a problem.  And the removal of nomodeset option would be a kind of
> such things, IMO, that's why I mentioned as "a gray zone" in the
> above.

Note that nomodeset and the per-driver modeset parameters are separate
things. amdgpu has always supported the former, but has never supported
the latter.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Jani Nikula]

On Tue, 03 Apr 2018, Michel Dänzer <michel@daenzer.net> wrote:
> On 2018-04-03 11:02 AM, Takashi Iwai wrote:
>> Agreed with the naming mess.  But OTOH, it's already a thing that is
>> too popular to kill.  You can add a more suitable option name, but you
>> cannot drop these existing ones easily.  It's already in a gray zone
>> of the golden "don't break user-space" rule.
>
> That's quite a stretch argument, given that amdgpu has never supported
> the modeset parameter.

That much is clear.

> Also, module parameters aren't UAPI.

[citation needed]. I agree with Takashi it's a gray area.

You can have "unsafe" module parameters that taint the kernel when set,
see module_param_unsafe and module_param_named_unsafe. Using those
should make it clear all bets are off.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Dave Airlie]

On Sat, 31 Mar 2018 at 06:45, Takashi Iwai <tiwai@suse.de> wrote:
>
> amdgpu driver lacks of modeset module option other drm drivers provide
> for enforcing or disabling the driver load.  Interestingly, the
> amdgpu_mode variable declaration is already found in the header file,
> but the actual implementation seems to have been forgotten.
>
> This patch adds the missing piece.

I'd like to land this patch, I realise people have NAKed it but for
consistency across drivers I'm going to ask we land it or something
like it.

The main use case for this is actually where you have amdgpu crashes
on load, and you want to debug them, people boot with nomodeset and
then modprobe amdgpu modeset=1 to get the crash in a running system.
This works for numerous other drivers, I'm not sure why amdgpu needs
to be the odd one out.

Reviewed-by: Dave Airlie <airlied@redhat.com>

Dave.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/2] drm/amdgpu: Add modeset module option

[Koenig, Christian]

Am 25.09.19 um 10:07 schrieb Dave Airlie:
> On Sat, 31 Mar 2018 at 06:45, Takashi Iwai <tiwai@suse.de> wrote:
>> amdgpu driver lacks of modeset module option other drm drivers provide
>> for enforcing or disabling the driver load.  Interestingly, the
>> amdgpu_mode variable declaration is already found in the header file,
>> but the actual implementation seems to have been forgotten.
>>
>> This patch adds the missing piece.
> I'd like to land this patch, I realise people have NAKed it but for
> consistency across drivers I'm going to ask we land it or something
> like it.
>
> The main use case for this is actually where you have amdgpu crashes
> on load, and you want to debug them, people boot with nomodeset and
> then modprobe amdgpu modeset=1 to get the crash in a running system.
> This works for numerous other drivers, I'm not sure why amdgpu needs
> to be the odd one out.

Because this is essentially the wrong approach.

The correct way to prevent a module from automatically loading is to add 
modprobe.blacklist=$name to the kernel command line.

The modeset and nomodeset kernel options where used to switch between 
KMS and UMS and not to disable driver load.

We should have removed those options with the removal of UMS or 
otherwise it becomes just another ancient cruft we need to carry forward 
in potentially all drivers.

Regards,
Christian.

>
> Reviewed-by: Dave Airlie <airlied@redhat.com>
>
> Dave.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/amdgpu: Fix memory leaks at amdgpu_init() error path

[Alex Deucher]

On Fri, Mar 30, 2018 at 4:45 PM, Takashi Iwai <tiwai@suse.de> wrote:
> amdgpu driver checks vgacon_text_force() after some initializations
> but without cleaning up.  This will result in leaks.
>
> Move the check of vgacon_text_force() to the beginning of
> amdgpu_init() for fixing it and also for optimization.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Applied.  Thanks!

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 50afcf65181a..e55792d3cd12 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -905,6 +905,11 @@ static int __init amdgpu_init(void)
>  {
>         int r;
>
> +       if (vgacon_text_force()) {
> +               DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
> +               return -EINVAL;
> +       }
> +
>         r = amdgpu_sync_init();
>         if (r)
>                 goto error_sync;
> @@ -913,10 +918,6 @@ static int __init amdgpu_init(void)
>         if (r)
>                 goto error_fence;
>
> -       if (vgacon_text_force()) {
> -               DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
> -               return -EINVAL;
> -       }
>         DRM_INFO("amdgpu kernel modesetting enabled.\n");
>         driver = &kms_driver;
>         pdriver = &amdgpu_kms_pci_driver;
> --
> 2.16.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx