[PATCH] drm/vmwgfx: respect 'nomodeset'

[Rob Clark]

On Wed, Oct 15, 2014 at 5:06 PM, Thomas Hellstrom <thellstrom at vmware.com> wrote:
> On 10/15/2014 10:51 PM, Rob Clark wrote:
>> On Wed, Oct 15, 2014 at 4:44 PM, Thomas Hellstrom <thellstrom at vmware.com> wrote:
>>> On 10/15/2014 10:38 PM, Rob Clark wrote:
>>>> On Wed, Oct 15, 2014 at 4:35 PM, Rob Clark <robdclark at gmail.com> wrote:
>>>>> On Wed, Oct 15, 2014 at 4:17 PM, Thomas Hellstrom <thellstrom at vmware.com> wrote:
>>>>>> On 10/15/2014 09:46 PM, Rob Clark wrote:
>>>>>>> On Wed, Oct 15, 2014 at 3:24 PM, Thomas Hellstrom <thellstrom at vmware.com> wrote:
>>>>>>>> On 10/15/2014 09:00 PM, Rob Clark wrote:
>>>>>>>>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>>>>>>>>> ---
>>>>>>>>>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 7 +++++++
>>>>>>>>>  1 file changed, 7 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
>>>>>>>>> index 18b54ac..f0267b8 100644
>>>>>>>>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
>>>>>>>>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
>>>>>>>>> @@ -25,6 +25,7 @@
>>>>>>>>>   *
>>>>>>>>>   **************************************************************************/
>>>>>>>>>  #include <linux/module.h>
>>>>>>>>> +#include <linux/console.h>
>>>>>>>>>
>>>>>>>>>  #include <drm/drmP.h>
>>>>>>>>>  #include "vmwgfx_drv.h"
>>>>>>>>> @@ -1453,6 +1454,12 @@ static int vmw_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>>>>>>>  static int __init vmwgfx_init(void)
>>>>>>>>>  {
>>>>>>>>>       int ret;
>>>>>>>>> +
>>>>>>>>> +#ifdef CONFIG_VGA_CONSOLE
>>>>>>>>> +     if (vgacon_text_force())
>>>>>>>>> +             return -EINVAL;
>>>>>>>>> +#endif
>>>>>>>>> +
>>>>>>>> Hmm,
>>>>>>>>
>>>>>>>> From the function name vgacon_text_force() it sounds like this should
>>>>>>>> just stop the driver from initializing fbcon? Not refuse to load?
>>>>>>> yeah, the function is badly named.. it perhaps should be
>>>>>>> vgacon_is_text_forced() or something like that.  But basically it
>>>>>>> returns whether we are forced to text mode.
>>>>>>>
>>>>>>> BR,
>>>>>>> -R
>>>>>> So then I guess it would be more correct to use the output of that
>>>>>> function when determining the value of dev_priv->enable_fb (vmwgfx can
>>>>>> enable the user-space modesetting API without turning on vmwgfx fbcon).
>>>>> well, other drivers, 'nomodeset' forces the driver not to load (to
>>>>> work around buggyness, etc)..  I suppose for vmwgfx perhaps oddball
>>>>> "hardware" is less of a concern.  Although it seems like it would be
>>>>> nice if vmwgfx behaved consistently with the other drivers.
>>>>>
>>>>> Most/all for the drivers have an additional module param that lets you
>>>>> override this and load the driver for UMS in case of 'nomodeset'..
>>>>> which would give you the behaviour you describe.  But I think in the
>>>>> absence of an additional module param specified, the default
>>>>> 'nomodeset' behaviour should be that the driver does not load.
>>>>>
>>>>> But I can add such a module param if you think it is useful..
>>>> oh, wait.. you already have an 'enable_fbdev'..  although I guess that
>>>> is actually meaning "enable_kms"?
>>> It should actually have been "enable_fbcon_and_fbdev" instead of
>>> "enable_fbdev". KMS is always enabled with the vmwgfx driver, we have no
>>> UMS driver using DRM functionality.
>>>
>>> I think what confuses me is how "nomodeset" translates to
>>> "use_text_console", (sounds orthogonal to me) but if that's the way that
>>> parameter is presented to the drivers, I guess the correct way after all
>>> is to stop the driver from loading....
>> the:
>>
>>   __setup("nomodeset", text_mode);
>>
>> bit causes text_mode() to be called which sets vgacon_text_mode_force
>>
>> (assuming that was the question)
> Yes. Then IMHO whoever wrote that didn't do his homework since you can
> have user-space modesetting API and a text mode console working
> perfectly well together, but never mind.
>
>>At least users seem to expect that 'nomodeset' means don't load the
> modesetting driver, since I got a bug about it ;-) BR, -R
>
> I agree.
> Reviewed-by: Thomas Hellstrom <thellstrom at vmware.com>.
>
> I'll include the patch in the next pull request.
>

btw, looks like this patch is still MIA..

BR,
-R