From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Hellstrom Subject: Re: [PATCH] drm/vmwgfx: respect 'nomodeset' Date: Wed, 15 Oct 2014 23:06:56 +0200 Message-ID: <543EE1F0.9080508@vmware.com> References: <1413399647-26998-1-git-send-email-robdclark@gmail.com> <543EC9FA.3000705@vmware.com> <543ED662.2070905@vmware.com> <543EDCB0.2030901@vmware.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from smtp-outbound-2.vmware.com (smtp-outbound-2.vmware.com [208.91.2.13]) by gabe.freedesktop.org (Postfix) with ESMTP id C848B6E0BF for ; Wed, 15 Oct 2014 14:07:02 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Rob Clark Cc: "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org On 10/15/2014 10:51 PM, Rob Clark wrote: > On Wed, Oct 15, 2014 at 4:44 PM, Thomas Hellstrom wrote: >> On 10/15/2014 10:38 PM, Rob Clark wrote: >>> On Wed, Oct 15, 2014 at 4:35 PM, Rob Clark wrote: >>>> On Wed, Oct 15, 2014 at 4:17 PM, Thomas Hellstrom wrote: >>>>> On 10/15/2014 09:46 PM, Rob Clark wrote: >>>>>> On Wed, Oct 15, 2014 at 3:24 PM, Thomas Hellstrom wrote: >>>>>>> On 10/15/2014 09:00 PM, Rob Clark wrote: >>>>>>>> Signed-off-by: Rob Clark >>>>>>>> --- >>>>>>>> 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 >>>>>>>> +#include >>>>>>>> >>>>>>>> #include >>>>>>>> #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 . I'll include the patch in the next pull request. Thanks, Thomas >> /Thomas >>