[{"id":1745341,"web_url":"http://patchwork.ozlabs.org/comment/1745341/","msgid":"<20170811214438.GI17039@bhelgaas-glaptop.roam.corp.google.com>","date":"2017-08-11T21:44:38","subject":"Re: [PATCH 1/3] powerpc: simplify and fix VGA default device\n\tbehaviour","submitter":{"id":67298,"url":"http://patchwork.ozlabs.org/api/people/67298/","name":"Bjorn Helgaas","email":"helgaas@kernel.org"},"content":"On Fri, Aug 04, 2017 at 08:20:31PM +1000, Daniel Axtens wrote:\n> Some powerpc devices provide a PCI display that isn't picked up by\n> the VGA arbiter, presumably because it doesn't support the PCI\n> legacy VGA ranges.\n> \n> Commit c2e1d84523ad (\"powerpc: Set default VGA device\") introduced\n> an arch quirk to mark these devices as default to fix X autoconfig.\n> \n> The commit message stated that the patch:\n> \n> Ensures a default VGA is always set if a graphics adapter is present,\n> even if firmware did not initialize it. If more than one graphics\n> adapter is present, ensure the one initialized by firmware is set\n> as the default VGA device.\n> \n> The patch used the following test to decide whether or not to mark\n> a device as default:\n> \n> pci_read_config_word(pdev, PCI_COMMAND, &cmd);\n> if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || !vga_default_device())\n> vga_set_default_device(pdev);\n> \n> This doesn't seem like it works quite as intended. Because of the\n> logical OR, the default device will be set in 2 cases:\n> \n> 1) if there is no default device\n> OR\n> 2) if this device has normal memory/IO decoding turned on\n> \n> This will work as intended if there is only one device, but if\n> there are multiple devices, we may override the device the VGA\n> arbiter picked.\n\nThis quirk only runs on VGA class devices. If there's more than one\nVGA device in the system, and we assume that firmware only enables\nPCI_COMMAND_IO or PCI_COMMAND_MEMORY on \"the one initialized by\nfirmware\", which seems reasonable to me, I think the existing code\ndoes match the commit message.\n\nWe set the first VGA device we find to be the default. Then, if we\nfind another VGA device that's enabled, we make *it* the default\ninstead.\n\n> Instead, set a device as default if there is no default device AND\n> this device decodes.\n> \n> This will not change behaviour on single-headed systems.\n\nIf there is no enabled VGA device on the system, your new code means\nthere will be no default VGA device.\n\nIt's not clear from this changelog what problem this patch solves.\nMaybe it's the \"some displays not being picked up by the VGA arbiter\"\nyou mentioned, but there's not enough detail to connect it with the\npatch, especially since the patch means we'll set the default device\nin fewer cases than we did before.\n\nWith the patch, we only set the default if we find an enabled VGA\ndevice. Previously we also set the default if we found a VGA device\nthat had not been enabled.\n\n> Cc: Brian King \n> Signed-off-by: Daniel Axtens \n> \n> ---\n> \n> Tested in TCG (the card provided by qemu doesn't automatically\n> register with vgaarb, so the relevant code path has been tested)\n> but I would appreciate any tests on real hardware.\n> \n> Informal benh ack: https://patchwork.kernel.org/patch/9850235/\n> ---\n> arch/powerpc/kernel/pci-common.c | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c\n> index 341a7469cab8..c95fdda3a2dc 100644\n> --- a/arch/powerpc/kernel/pci-common.c\n> +++ b/arch/powerpc/kernel/pci-common.c\n> @@ -1746,8 +1746,11 @@ static void fixup_vga(struct pci_dev *pdev)\n> {\n> \tu16 cmd;\n> \n> +\tif (vga_default_device())\n> +\t\treturn;\n> +\n> \tpci_read_config_word(pdev, PCI_COMMAND, &cmd);\n> -\tif ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || !vga_default_device())\n> +\tif (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY))\n> \t\tvga_set_default_device(pdev);\n> \n> }\n> -- \n> 2.11.0\n>","headers":{"Return-Path":"","X-Original-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xTdq34tffz9t2V\n\tfor ;\n\tSat, 12 Aug 2017 07:45:51 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xTdq342g2zDrFK\n\tfor ;\n\tSat, 12 Aug 2017 07:45:51 +1000 (AEST)","from mail.kernel.org (mail.kernel.org [198.145.29.99])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xTdnk0zSvzDr4c\n\tfor ;\n\tSat, 12 Aug 2017 07:44:42 +1000 (AEST)","from localhost (unknown [69.55.156.165])\n\t(using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits))\n\t(No client certificate requested)\n\tby mail.kernel.org (Postfix) with ESMTPSA id 5B9AE22BCD;\n\tFri, 11 Aug 2017 21:44:39 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mail.kernel.org 5B9AE22BCD","Authentication-Results":["mail.kernel.org;\n\tdmarc=none (p=none dis=none) header.from=kernel.org","mail.kernel.org;\n\tspf=none smtp.mailfrom=helgaas@kernel.org"],"Date":"Fri, 11 Aug 2017 16:44:38 -0500","From":"Bjorn Helgaas ","To":"Daniel Axtens ","Subject":"Re: [PATCH 1/3] powerpc: simplify and fix VGA default device\n\tbehaviour","Message-ID":"<20170811214438.GI17039@bhelgaas-glaptop.roam.corp.google.com>","References":"<20170804102033.27731-1-dja@axtens.net>\n\t<20170804102033.27731-2-dja@axtens.net>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20170804102033.27731-2-dja@axtens.net>","User-Agent":"Mutt/1.5.21 (2010-09-15)","X-BeenThere":"linuxppc-dev@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Linux on PowerPC Developers Mail List\n\t","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"catalin.marinas@arm.com, gabriele.paoloni@huawei.com, airlied@linux.ie, \n\tlinux-pci@vger.kernel.org, Brian King ,\n\twill.deacon@arm.com, z.liuxinliang@hisilicon.com, bhelgaas@google.com,\n\talex.williamson@redhat.com, zourongrong@gmail.com,\n\tdaniel.vetter@intel.com, \n\tlinuxppc-dev@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org","Errors-To":"linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org","Sender":"\"Linuxppc-dev\"\n\t"}},{"id":1745992,"web_url":"http://patchwork.ozlabs.org/comment/1745992/","msgid":"<87tw1bqeqq.fsf@linkitivity.dja.id.au>","date":"2017-08-13T22:34:05","subject":"Re: [PATCH 1/3] powerpc: simplify and fix VGA default device\n\tbehaviour","submitter":{"id":65792,"url":"http://patchwork.ozlabs.org/api/people/65792/","name":"Daniel Axtens","email":"dja@axtens.net"},"content":"Hi Bjorn,\n\nThanks for reading the patch and providing some feedback.\n\n>> This will work as intended if there is only one device, but if\n>> there are multiple devices, we may override the device the VGA\n>> arbiter picked.\n>\n> This quirk only runs on VGA class devices. If there's more than one\n> VGA device in the system, and we assume that firmware only enables\n> PCI_COMMAND_IO or PCI_COMMAND_MEMORY on \"the one initialized by\n> firmware\", which seems reasonable to me, I think the existing code\n> does match the commit message.\n>\n> We set the first VGA device we find to be the default. Then, if we\n> find another VGA device that's enabled, we make *it* the default\n> instead.\n>\n>> Instead, set a device as default if there is no default device AND\n>> this device decodes.\n>> \n>> This will not change behaviour on single-headed systems.\n>\n> If there is no enabled VGA device on the system, your new code means\n> there will be no default VGA device.\n\nAh. Right you are.\n\n>\n> It's not clear from this changelog what problem this patch solves.\n> Maybe it's the \"some displays not being picked up by the VGA arbiter\"\n> you mentioned, but there's not enough detail to connect it with the\n> patch, especially since the patch means we'll set the default device\n> in fewer cases than we did before.\n>\n> With the patch, we only set the default if we find an enabled VGA\n> device. Previously we also set the default if we found a VGA device\n> that had not been enabled.\n\nMy overall problem is not having default devices on some\nmachines. Initially, to solve this, I wanted to make this code\ngeneric. To do that I wanted to make sure it played nice with the vga\narbiter, so not overriding default devices willy-nilly was a\nrequirement. Evidently, I was overly keen and restricted its behaviour\ntoo much.\n\nRegardless, in this current approach I don't make this powerpc code\ngeneric after all, so this patch is unnecessary. I will remove it for\nv2, which I will post after further testing.\n\nI document the effects of the new approach for powerpc in more detail in\npatch 3 of this series. If powerpc wants to keep the existing approach\nwe can arrange for that too; it's a simple matter of ifdefs.\n\nRegards,\nDaniel\n>\n>> Cc: Brian King \n>> Signed-off-by: Daniel Axtens \n>> \n>> ---\n>> \n>> Tested in TCG (the card provided by qemu doesn't automatically\n>> register with vgaarb, so the relevant code path has been tested)\n>> but I would appreciate any tests on real hardware.\n>> \n>> Informal benh ack: https://patchwork.kernel.org/patch/9850235/\n>> ---\n>> arch/powerpc/kernel/pci-common.c | 5 ++++-\n>> 1 file changed, 4 insertions(+), 1 deletion(-)\n>> \n>> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c\n>> index 341a7469cab8..c95fdda3a2dc 100644\n>> --- a/arch/powerpc/kernel/pci-common.c\n>> +++ b/arch/powerpc/kernel/pci-common.c\n>> @@ -1746,8 +1746,11 @@ static void fixup_vga(struct pci_dev *pdev)\n>> {\n>> \tu16 cmd;\n>> \n>> +\tif (vga_default_device())\n>> +\t\treturn;\n>> +\n>> \tpci_read_config_word(pdev, PCI_COMMAND, &cmd);\n>> -\tif ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || !vga_default_device())\n>> +\tif (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY))\n>> \t\tvga_set_default_device(pdev);\n>> \n>> }\n>> -- \n>> 2.11.0\n>>","headers":{"Return-Path":"","X-Original-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xVtqg5HGzz9sDB\n\tfor ;\n\tMon, 14 Aug 2017 08:35:43 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xVtqg47gjzDqj4\n\tfor ;\n\tMon, 14 Aug 2017 08:35:43 +1000 (AEST)","from mail-pg0-x244.google.com (mail-pg0-x244.google.com\n\t[IPv6:2607:f8b0:400e:c05::244])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xVtnx3dKQzDqb5\n\tfor ;\n\tMon, 14 Aug 2017 08:34:12 +1000 (AEST)","by mail-pg0-x244.google.com with SMTP id 83so7599544pgb.4\n\tfor ;\n\tSun, 13 Aug 2017 15:34:12 -0700 (PDT)","from localhost (124-171-162-147.dyn.iinet.net.au.\n\t[124.171.162.147]) by smtp.gmail.com with ESMTPSA id\n\tu67sm10980056pfd.164.2017.08.13.15.34.08\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSun, 13 Aug 2017 15:34:09 -0700 (PDT)"],"Authentication-Results":["ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=axtens.net header.i=@axtens.net\n\theader.b=\"aVdD9nec\"; dkim-atps=neutral","lists.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=axtens.net header.i=@axtens.net\n\theader.b=\"aVdD9nec\"; dkim-atps=neutral","lists.ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=axtens.net header.i=@axtens.net\n\theader.b=\"aVdD9nec\"; dkim-atps=neutral"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=axtens.net; s=google;\n\th=from:to:cc:subject:in-reply-to:references:date:message-id\n\t:mime-version; bh=puxfpAAWocfioA6vSN4p1vWcV92lf+GbOvb25Hv+QSE=;\n\tb=aVdD9nec+qPLEWSiNqXJmKEMUTJs4+MEkIOELWZyoabfmxI2cPC3fZkmM5OmrSi0sJ\n\tI8UoEbHvZawSsB5nbJYbg+laKsPdfMw2iwyYJpf+Jha+F61acVij8GSKNsg3lLLXSGvK\n\tzaGPADHsU8+iQS3NmhIi1yFY6urngyEdN8hDI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:from:to:cc:subject:in-reply-to:references:date\n\t:message-id:mime-version;\n\tbh=puxfpAAWocfioA6vSN4p1vWcV92lf+GbOvb25Hv+QSE=;\n\tb=WuClmKCpXpFmNzhZST+iaToITQFapq2gc2M1PvLJZpKxWfczAVeCHFIhmRyDjFVbr3\n\t0XMW4cA7B+4hm4eaVLKpazAk89vYTK5Pujb5faoOCZZaiMyyTdrA/tmeBtRY2rwkV9yc\n\tgrSMflxv3uOKGDDI1tg/ht0NIcw8CNad5KcGhPbcLxKebZnyb4wWRTH650X5w9S5tj9x\n\towbY8ylZLtqiG5UmBq9xu8/eicGbG0EiO9hO6z7Bspb35y0QIcPJrEDkQZoAbOCMxIgV\n\tDq7F8x+X/kIFyPcMgCbUyBH7JsnJOl1wytx+Fpjeov3p+jUHJClTeRhAackROJwxk4v5\n\tB8rA==","X-Gm-Message-State":"AHYfb5g7jLuPGiT+PQuTwfkrNFXGoYrJEdYzIIzvSwxhbYnMRazcE06T\n\tFNQEamyomjxqLjkD","X-Received":"by 10.84.231.130 with SMTP id g2mr25752177plk.342.1502663650695; \n\tSun, 13 Aug 2017 15:34:10 -0700 (PDT)","From":"Daniel Axtens ","To":"Bjorn Helgaas ","Subject":"Re: [PATCH 1/3] powerpc: simplify and fix VGA default device\n\tbehaviour","In-Reply-To":"<20170811214438.GI17039@bhelgaas-glaptop.roam.corp.google.com>","References":"<20170804102033.27731-1-dja@axtens.net>\n\t<20170804102033.27731-2-dja@axtens.net>\n\t<20170811214438.GI17039@bhelgaas-glaptop.roam.corp.google.com>","Date":"Mon, 14 Aug 2017 08:34:05 +1000","Message-ID":"<87tw1bqeqq.fsf@linkitivity.dja.id.au>","MIME-Version":"1.0","Content-Type":"text/plain","X-BeenThere":"linuxppc-dev@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Linux on PowerPC Developers Mail List\n\t","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"catalin.marinas@arm.com, gabriele.paoloni@huawei.com, airlied@linux.ie, \n\tlinux-pci@vger.kernel.org, Brian King ,\n\twill.deacon@arm.com, z.liuxinliang@hisilicon.com, bhelgaas@google.com,\n\talex.williamson@redhat.com, zourongrong@gmail.com,\n\tdaniel.vetter@intel.com, \n\tlinuxppc-dev@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org","Errors-To":"linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org","Sender":"\"Linuxppc-dev\"\n\t"}}]