[v6,0/8] PCI/VGA: introduce is_boot_device function callback to vga_client_register

Message ID 20230612192550.197053-1-15330273260@189.cn
Headers
Series PCI/VGA: introduce is_boot_device function callback to vga_client_register |

Message

Sui Jingfeng June 12, 2023, 7:25 p.m. UTC
  From: Sui Jingfeng <suijingfeng@loongson.cn>

The vga_is_firmware_default() function is arch-dependent, it's probably
wrong if we simply remove the arch guard. As the VRAM BAR which contains
firmware framebuffer may move, while the lfb_base and lfb_size members of
the screen_info does not change accordingly. In short, it should take the
re-allocation of the PCI BAR into consideration.

With the observation that device drivers or video aperture helpers may
have better knowledge about which PCI bar contains the firmware fb,
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 not suitable to
make such an optimization since it is loaded too early.

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. Also honor
the comment: "Clients have two callback mechanisms they can use"

Sui Jingfeng (8):
  PCI/VGA: Use unsigned type for the io_state variable
  PCI/VGA: Deal only with VGA class devices
  PCI/VGA: Tidy up the code and comment format
  PCI/VGA: Replace full MIT license text with SPDX identifier
  video/aperture: Add a helper to detect if an aperture contains
    firmware FB
  PCI/VGA: Introduce is_boot_device function callback to
    vga_client_register
  drm/amdgpu: Implement the is_boot_device callback function
  drm/radeon: Implement the is_boot_device callback function

 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  12 +-
 drivers/gpu/drm/drm_aperture.c             |  16 +++
 drivers/gpu/drm/i915/display/intel_vga.c   |   3 +-
 drivers/gpu/drm/nouveau/nouveau_vga.c      |   2 +-
 drivers/gpu/drm/radeon/radeon_device.c     |  12 +-
 drivers/pci/vgaarb.c                       | 153 +++++++++++++--------
 drivers/vfio/pci/vfio_pci_core.c           |   2 +-
 drivers/video/aperture.c                   |  29 ++++
 include/drm/drm_aperture.h                 |   2 +
 include/linux/aperture.h                   |   7 +
 include/linux/vgaarb.h                     |  35 ++---
 11 files changed, 184 insertions(+), 89 deletions(-)
  

Comments

Lyude Paul June 21, 2023, 10:11 p.m. UTC | #1
For the nouveau bits:

Reviewed-by: Lyude Paul <lyude@redhat.com>

On Tue, 2023-06-13 at 03:25 +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> The vga_is_firmware_default() function is arch-dependent, it's probably
> wrong if we simply remove the arch guard. As the VRAM BAR which contains
> firmware framebuffer may move, while the lfb_base and lfb_size members of
> the screen_info does not change accordingly. In short, it should take the
> re-allocation of the PCI BAR into consideration.
> 
> With the observation that device drivers or video aperture helpers may
> have better knowledge about which PCI bar contains the firmware fb,
> 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 not suitable to
> make such an optimization since it is loaded too early.
> 
> 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. Also honor
> the comment: "Clients have two callback mechanisms they can use"
> 
> Sui Jingfeng (8):
>   PCI/VGA: Use unsigned type for the io_state variable
>   PCI/VGA: Deal only with VGA class devices
>   PCI/VGA: Tidy up the code and comment format
>   PCI/VGA: Replace full MIT license text with SPDX identifier
>   video/aperture: Add a helper to detect if an aperture contains
>     firmware FB
>   PCI/VGA: Introduce is_boot_device function callback to
>     vga_client_register
>   drm/amdgpu: Implement the is_boot_device callback function
>   drm/radeon: Implement the is_boot_device callback function
> 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  12 +-
>  drivers/gpu/drm/drm_aperture.c             |  16 +++
>  drivers/gpu/drm/i915/display/intel_vga.c   |   3 +-
>  drivers/gpu/drm/nouveau/nouveau_vga.c      |   2 +-
>  drivers/gpu/drm/radeon/radeon_device.c     |  12 +-
>  drivers/pci/vgaarb.c                       | 153 +++++++++++++--------
>  drivers/vfio/pci/vfio_pci_core.c           |   2 +-
>  drivers/video/aperture.c                   |  29 ++++
>  include/drm/drm_aperture.h                 |   2 +
>  include/linux/aperture.h                   |   7 +
>  include/linux/vgaarb.h                     |  35 ++---
>  11 files changed, 184 insertions(+), 89 deletions(-)
>
  
Sui Jingfeng June 22, 2023, 4:46 a.m. UTC | #2
Hi

On 2023/6/22 06:11, Lyude Paul wrote:
> For the nouveau bits:
>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
Thanks a lot
> On Tue, 2023-06-13 at 03:25 +0800, Sui Jingfeng wrote:
>> From: Sui Jingfeng <suijingfeng@loongson.cn>
>>
>> The vga_is_firmware_default() function is arch-dependent, it's probably
>> wrong if we simply remove the arch guard. As the VRAM BAR which contains
>> firmware framebuffer may move, while the lfb_base and lfb_size members of
>> the screen_info does not change accordingly. In short, it should take the
>> re-allocation of the PCI BAR into consideration.
>>
>> With the observation that device drivers or video aperture helpers may
>> have better knowledge about which PCI bar contains the firmware fb,
>> 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 not suitable to
>> make such an optimization since it is loaded too early.
>>
>> 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. Also honor
>> the comment: "Clients have two callback mechanisms they can use"
>>
>> Sui Jingfeng (8):
>>    PCI/VGA: Use unsigned type for the io_state variable
>>    PCI/VGA: Deal only with VGA class devices
>>    PCI/VGA: Tidy up the code and comment format
>>    PCI/VGA: Replace full MIT license text with SPDX identifier
>>    video/aperture: Add a helper to detect if an aperture contains
>>      firmware FB
>>    PCI/VGA: Introduce is_boot_device function callback to
>>      vga_client_register
>>    drm/amdgpu: Implement the is_boot_device callback function
>>    drm/radeon: Implement the is_boot_device callback function
>>
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  12 +-
>>   drivers/gpu/drm/drm_aperture.c             |  16 +++
>>   drivers/gpu/drm/i915/display/intel_vga.c   |   3 +-
>>   drivers/gpu/drm/nouveau/nouveau_vga.c      |   2 +-
>>   drivers/gpu/drm/radeon/radeon_device.c     |  12 +-
>>   drivers/pci/vgaarb.c                       | 153 +++++++++++++--------
>>   drivers/vfio/pci/vfio_pci_core.c           |   2 +-
>>   drivers/video/aperture.c                   |  29 ++++
>>   include/drm/drm_aperture.h                 |   2 +
>>   include/linux/aperture.h                   |   7 +
>>   include/linux/vgaarb.h                     |  35 ++---
>>   11 files changed, 184 insertions(+), 89 deletions(-)
>>