Re: [Nouveau] [Intel-gfx] [PATCH v3 4/4] PCI/VGA: introduce is_boot_device function callback to vga_client_register
On Thu, Jun 08, 2023 at 07:43:22PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> The vga_is_firmware_default() function is arch-dependent, which doesn't
> sound right. At least, it also works on the Mips and LoongArch platforms.
> Tested with the drm/amdgpu and drm/radeon drivers. However, it's difficult
> to enumerate all arch-driver combinations. I'm wrong if there is only one
> exception.
>
> With the observation that device drivers typically have better knowledge
> about which PCI bar contains the firmware framebuffer, which could avoid
> the need to iterate all of the PCI BARs.
>
> But as a PCI function at pci/vgaarb.c, vga_is_firmware_default() is
> probably not suitable to make such an optimization for a specific device.
>
> There are PCI display controllers that don't have a dedicated VRAM bar,
> this function will lose its effectiveness in such a case. Luckily, the
> device driver can provide an accurate workaround.
>
> Therefore, this patch introduces a callback that allows the device driver
> to tell the VGAARB if the device is the default boot device. This patch
> only intends to introduce the mechanism, while the implementation is left
> to the device driver authors. Also honor the comment: "Clients have two
> callback mechanisms they can use"
s/bar/BAR/ (several)
Nothing here uses the callback. I don't want to merge this until we
have a user.
I'm not sure why the device driver should know whether its device is
the default boot device.
> Signed-off-by: Sui Jingfeng
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
> drivers/gpu/drm/i915/display/intel_vga.c | 3 +--
> drivers/gpu/drm/nouveau/nouveau_vga.c | 2 +-
> drivers/gpu/drm/radeon/radeon_device.c | 2 +-
> drivers/pci/vgaarb.c | 22 ++
> drivers/vfio/pci/vfio_pci_core.c | 2 +-
> include/linux/vgaarb.h | 8 +---
> 7 files changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 5c7d40873ee2..7a096f2d5c16 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3960,7 +3960,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> /* this will fail for cards that aren't VGA class devices, just
>* ignore it */
> if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> - vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
> + vga_client_register(adev->pdev, amdgpu_device_vga_set_decode,
> NULL);
>
> px = amdgpu_device_supports_px(ddev);
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vga.c
> b/drivers/gpu/drm/i915/display/intel_vga.c
> index 286a0bdd28c6..98d7d4dffe9f 100644
> --- a/drivers/gpu/drm/i915/display/intel_vga.c
> +++ b/drivers/gpu/drm/i915/display/intel_vga.c
> @@ -115,7 +115,6 @@ intel_vga_set_decode(struct pci_dev *pdev, bool
> enable_decode)
>
> int intel_vga_register(struct drm_i915_private *i915)
> {
> -
> struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
> int ret;
>
> @@ -127,7 +126,7 @@ int intel_vga_register(struct drm_i915_private *i915)
>* then we do not take part in VGA arbitration and the
>* vga_client_register() fails with -ENODEV.
>*/
> - ret = vga_client_register(pdev, intel_vga_set_decode);
> + ret = vga_client_register(pdev, intel_vga_set_decode, NULL);
> if (ret && ret != -ENODEV)
> return ret;
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c
> b/drivers/gpu/drm/nouveau/nouveau_vga.c
> index f8bf0ec26844..162b4f4676c7 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_vga.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
> @@ -92,7 +92,7 @@ nouveau_vga_init(struct nouveau_drm *drm)
> return;
> pdev = to_pci_dev(dev->dev);
>
> - vga_client_register(pdev, nouveau_vga_set_decode);
> + vga_client_register(pdev, nouveau_vga_set_decode, NULL);
>
> /* don't register Thunderbolt eGPU with vga_switcheroo */
> if (pci_is_thunderbolt_attached(pdev))
> diff --git a/drivers/gpu/drm/radeon/radeon_device.c
> b/drivers/gpu/drm/radeon/radeon_device.c
> index afbb3a80c0c6..71f2ff39d6a1 100644
> --- a/drivers/gpu/drm/radeon/radeon_device.c
> +++ b/drivers/gpu/drm/radeon/radeon_device.c
> @@ -1425,7 +1425,7 @@ int radeon_device_init(struct radeon_device *rdev,
> /* if we have > 1 VGA cards, then disable the radeon VGA resources */
> /* this will fail for cards that aren't VGA class devices, just
>* ignore it */
> - vga_client_register(rdev->pdev, radeon_vga_set_decode);
> + vga_client_register(rdev->pdev, radeon_vga_set_decode, NULL);
>
> if (rdev->flags & RADEON_IS_PX)
> runtime = true;
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index b0bf4952a95d..d3dab61e0ef2 100644
> --- a/drivers/pci/vgaarb.c
>
Re: [Nouveau] [Intel-gfx] [PATCH v3 3/4] PCI/VGA: only deal with VGA class devices
Start with verb and capitalize to match ("Deal only with ...")
On Thu, Jun 08, 2023 at 07:43:21PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> vgaarb only deal with the VGA devcie(pdev->class == 0x0300), so replace the
> pci_get_subsys() function with pci_get_class(). Filter the non pci display
> device out. There no need to process the non display PCI device.
s/non pci/non-PCI/
s/non display/non-display/
This is fine, and I'll merge this, but someday I would like to get rid
of the bus_register_notifier() and call vga_arbiter_add_pci_device()
and vga_arbiter_del_pci_device() directly from the PCI core.
Or if you wanted to do that now, that would be even better :)
Bjorn
> Signed-off-by: Sui Jingfeng
> ---
> drivers/pci/vgaarb.c | 22 --
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 7f0347f4f6fd..b0bf4952a95d 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -756,10 +756,6 @@ static bool vga_arbiter_add_pci_device(struct pci_dev
> *pdev)
> struct pci_dev *bridge;
> u16 cmd;
>
> - /* Only deal with VGA class devices */
> - if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> - return false;
> -
> /* Allocate structure */
> vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
> if (vgadev == NULL) {
> @@ -1499,7 +1495,9 @@ static int pci_notify(struct notifier_block *nb,
> unsigned long action,
> struct pci_dev *pdev = to_pci_dev(dev);
> bool notify = false;
>
> - vgaarb_dbg(dev, "%s\n", __func__);
> + /* Only deal with VGA class devices */
> + if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> + return 0;
>
> /* For now we're only intereted in devices added and removed. I didn't
>* test this thing here, so someone needs to double check for the
> @@ -1509,6 +1507,8 @@ static int pci_notify(struct notifier_block *nb,
> unsigned long action,
> else if (action == BUS_NOTIFY_DEL_DEVICE)
> notify = vga_arbiter_del_pci_device(pdev);
>
> + vgaarb_dbg(dev, "%s: action = %lu\n", __func__, action);
> +
> if (notify)
> vga_arbiter_notify_clients();
> return 0;
> @@ -1533,8 +1533,8 @@ static struct miscdevice vga_arb_device = {
>
> static int __init vga_arb_device_init(void)
> {
> + struct pci_dev *pdev = NULL;
> int rc;
> - struct pci_dev *pdev;
>
> rc = misc_register(_arb_device);
> if (rc < 0)
> @@ -1545,11 +1545,13 @@ static int __init vga_arb_device_init(void)
> /* We add all PCI devices satisfying VGA class in the arbiter by
>* default
>*/
> - pdev = NULL;
> - while ((pdev =
> - pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> -PCI_ANY_ID, pdev)) != NULL)
> + while (1) {
> + pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev);
> + if (!pdev)
> + break;
> +
> vga_arbiter_add_pci_device(pdev);
> + };
>
> pr_info("loaded\n");
> return rc;
> --
> 2.25.1
>
Re: [Nouveau] [Intel-gfx] [PATCH v3 1/4] PCI/VGA: tidy up the code and comment format
Capitalize subject to match ("Tidy ...")
On Thu, Jun 08, 2023 at 07:43:19PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> This patch replaces the leading space with a tab and removes the double
> blank line, no functional change.
Can you move this to the end of the series? The functional changes
are more likely to be backported, and I think the backport may be a
little easier without the cleanup in the middle.
> /* we could in theory hand out locks on IO and mem
> - * separately to userspace but it can cause deadlocks */
> + * separately to userspace but it can cause deadlocks
> + */
Since you're touching this anyway, can you update it to the
conventional multi-line comment style:
/*
* We could in theory ...
*/
And capitalize "We", add a period at end, and rewrap to fill 78
columns or so? Same for other comments below.
> +++ b/include/linux/vgaarb.h
> @@ -23,9 +23,7 @@
> * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> - * DEALINGS
> - * IN THE SOFTWARE.
> - *
> + * DEALINGS IN THE SOFTWARE.
> */
Can you make a separate patch to replace this entire copyright notice
with the appropriate SPDX-License-Identifier header?
Documentation/process/license-rules.rst has details.
Bjorn
Re: [Nouveau] [PATCH drm-next v4 13/14] drm/nouveau: implement new VM_BIND uAPI
Hi Danilo, kernel test robot noticed the following build warnings: [auto build test WARNING on 33a86170888b7e4aa0cea94ebb9c67180139cea9] url: https://github.com/intel-lab-lkp/linux/commits/Danilo-Krummrich/drm-execution-context-for-GEM-buffers-v4/20230607-063442 base: 33a86170888b7e4aa0cea94ebb9c67180139cea9 patch link: https://lore.kernel.org/r/20230606223130.6132-14-dakr%40redhat.com patch subject: [PATCH drm-next v4 13/14] drm/nouveau: implement new VM_BIND uAPI config: alpha-randconfig-s041-20230608 (https://download.01.org/0day-ci/archive/20230608/202306082035.j4zjw2he-...@intel.com/config) compiler: alpha-linux-gcc (GCC) 12.3.0 reproduce: mkdir -p ~/bin wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.4-39-gce1a6720-dirty # https://github.com/intel-lab-lkp/linux/commit/28d9f3973f9ed165312943fb05304fad878abb33 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Danilo-Krummrich/drm-execution-context-for-GEM-buffers-v4/20230607-063442 git checkout 28d9f3973f9ed165312943fb05304fad878abb33 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=alpha olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=alpha SHELL=/bin/bash drivers/gpu/drm/ If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202306082035.j4zjw2he-...@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/gpu/drm/nouveau/nouveau_drm.c:1194:9: sparse: sparse: incorrect type >> in initializer (incompatible argument 2 (different address spaces)) @@ >> expected int ( [usertype] *func )( ... ) @@ got int ( * )( ... ) @@ drivers/gpu/drm/nouveau/nouveau_drm.c:1194:9: sparse: expected int ( [usertype] *func )( ... ) drivers/gpu/drm/nouveau/nouveau_drm.c:1194:9: sparse: got int ( * )( ... ) drivers/gpu/drm/nouveau/nouveau_drm.c:1195:9: sparse: sparse: incorrect type in initializer (incompatible argument 2 (different address spaces)) @@ expected int ( [usertype] *func )( ... ) @@ got int ( * )( ... ) @@ drivers/gpu/drm/nouveau/nouveau_drm.c:1195:9: sparse: expected int ( [usertype] *func )( ... ) drivers/gpu/drm/nouveau/nouveau_drm.c:1195:9: sparse: got int ( * )( ... ) drivers/gpu/drm/nouveau/nouveau_drm.c:1196:9: sparse: sparse: incorrect type in initializer (incompatible argument 2 (different address spaces)) @@ expected int ( [usertype] *func )( ... ) @@ got int ( * )( ... ) @@ drivers/gpu/drm/nouveau/nouveau_drm.c:1196:9: sparse: expected int ( [usertype] *func )( ... ) drivers/gpu/drm/nouveau/nouveau_drm.c:1196:9: sparse: got int ( * )( ... ) -- >> drivers/gpu/drm/nouveau/nouveau_exec.c:305:19: sparse: sparse: dereference >> of noderef expression drivers/gpu/drm/nouveau/nouveau_exec.c:306:19: sparse: sparse: dereference of noderef expression drivers/gpu/drm/nouveau/nouveau_exec.c:307:20: sparse: sparse: dereference of noderef expression drivers/gpu/drm/nouveau/nouveau_exec.c:308:20: sparse: sparse: dereference of noderef expression drivers/gpu/drm/nouveau/nouveau_exec.c:309:21: sparse: sparse: dereference of noderef expression drivers/gpu/drm/nouveau/nouveau_exec.c:310:21: sparse: sparse: dereference of noderef expression drivers/gpu/drm/nouveau/nouveau_exec.c:378:43: sparse: sparse: dereference of noderef expression drivers/gpu/drm/nouveau/nouveau_exec.c:393:13: sparse: sparse: dereference of noderef expression drivers/gpu/drm/nouveau/nouveau_exec.c:396:13: sparse: sparse: dereference of noderef expression drivers/gpu/drm/nouveau/nouveau_exec.c:397:17: sparse: sparse: dereference of noderef expression -- drivers/gpu/drm/nouveau/nouveau_uvmm.c:1637:1: sparse: sparse: symbol 'nouveau_uvmm_ioctl_vm_init' redeclared with different type (incompatible argument 2 (different address spaces)): >> drivers/gpu/drm/nouveau/nouveau_uvmm.c:1637:1: sparse:int extern >> [addressable] [signed] [toplevel] nouveau_uvmm_ioctl_vm_init( ... ) drivers/gpu/drm/nouveau/nouveau_uvmm.c: note: in included file (through drivers/gpu/drm/nouveau/nouveau_drv.h): drivers/gpu/drm/nouveau/nouveau_uvmm.h:91:5: sparse: note: previously declared as: >> drivers/gpu/drm/nouveau/nouveau_uvmm.h:91:5: sparse:int extern >> [addressable] [signed] [toplevel] nouveau_uvmm_i
