Re: [PATCH v6 00/10] Make PCI's devres API more consistent
On Fri, Apr 26, 2024 at 10:07:02AM +0200, Philipp Stanner wrote: > On Wed, 2024-04-24 at 15:12 -0500, Bjorn Helgaas wrote: > > On Mon, Apr 08, 2024 at 10:44:12AM +0200, Philipp Stanner wrote: > > > ... > > > PCI's devres API suffers several weaknesses: > > > > > > 1. There are functions prefixed with pcim_. Those are always > > > managed > > > counterparts to never-managed functions prefixed with pci_ – or > > > so one > > > would like to think. There are some apparently unmanaged > > > functions > > > (all region-request / release functions, and pci_intx()) which > > > suddenly become managed once the user has initialized the device > > > with > > > pcim_enable_device() instead of pci_enable_device(). This > > > "sometimes > > > yes, sometimes no" nature of those functions is confusing and > > > therefore bug-provoking. In fact, it has already caused a bug in > > > DRM. > > > The last patch in this series fixes that bug. > > > 2. iomappings: Instead of giving each mapping its own callback, the > > > existing API uses a statically allocated struct tracking one > > > mapping > > > per bar. This is not extensible. Especially, you can't create > > > _ranged_ managed mappings that way, which many drivers want. > > > 3. Managed request functions only exist as "plural versions" with a > > > bit-mask as a parameter. That's quite over-engineered > > > considering > > > that each user only ever mapps one, maybe two bars. > > > > > > This series: > > > - add a set of new "singular" devres functions that use devres the > > > way > > > its intended, with one callback per resource. > > > - deprecates the existing iomap-table mechanism. > > > - deprecates the hybrid nature of pci_ functions. > > > - preserves backwards compatibility so that drivers using the > > > existing > > > API won't notice any changes. > > > - adds documentation, especially some warning users about the > > > complicated nature of PCI's devres. > > > > There's a lot of good work here; thanks for working on it. > > Thanks! > Good to get some more feedback from you > > > > > > Philipp Stanner (10): > > > PCI: Add new set of devres functions > > > > This first patch adds some infrastructure and several new exported > > interfaces: > > > > void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar, > > const char *name) > > void pcim_iounmap_region(struct pci_dev *pdev, int bar) > > int pcim_request_region(struct pci_dev *pdev, int bar, const char > > *name) > > void pcim_release_region(struct pci_dev *pdev, int bar) > > void __iomem *pcim_iomap_range(struct pci_dev *pdev, int bar, > > void __iomem *pcim_iomap_region_range(struct pci_dev *pdev, int > > bar, > > void pcim_iounmap_region_range(struct pci_dev *pdev, int bar, > > > > > PCI: Deprecate iomap-table functions > > > > This adds a little bit of infrastructure (add/remove to > > legacy_table), > > reimplements these existing interfaces: > > > > void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned > > long maxlen) > > void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr) > > int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char > > *name) > > int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask, > > void pcim_iounmap_regions(struct pci_dev *pdev, int mask) > > > > and adds a couple new exported interfaces: > > > > void pcim_release_all_regions(struct pci_dev *pdev) > > int pcim_request_all_regions(struct pci_dev *pdev, const char > > *name) > > > > There's a lot going on in these two patches, so they're hard to > > review. I think it would be easier if you could do the fixes to > > existing interfaces first, > > I agree that the patches can be further split into smaller chunks to > make them more atomic and easier to review. I can do that. > > BUT I'd need some more details about what you mean by "do the fixes > first" – which fixes? > The later patches at least in part rely on the new better functions > being available. > > > followed by adding new things, maybe > > something like separate patches that: > > > > - Add pcim_addr_devres_alloc(), pcim_addr_devres_free(), > > pcim_addr_devres_clear(). > > > > - Ad
Re: [PATCH v6 03/10] PCI: Warn users about complicated devres nature
On Mon, Apr 08, 2024 at 10:44:15AM +0200, Philipp Stanner wrote: > The PCI region-request functions become managed functions when > pcim_enable_device() has been called previously instead of > pci_enable_device(). > > This has already caused bugs by confusing users, who came to believe > that all pci functions, such as pci_iomap_range(), suddenly are managed > that way. Since you mention a bug, it'd be nice to include a URL or commit if you have one. > This is not the case. > > Add comments to the relevant functions' docstrings that warn users about > this behavior. > @@ -3914,6 +3916,13 @@ EXPORT_SYMBOL(pci_release_region); > * > * Returns 0 on success, or %EBUSY on error. A warning > * message is also printed on failure. > + * > + * NOTE: > + * This is a "hybrid" function: Its normally unmanaged, but becomes managed > + * when pcim_enable_device() has been called in advance. > + * This hybrid feature is DEPRECATED! If you need to implement a new pci > + * function that does automatic cleanup, write a new pcim_* function that > uses > + * devres directly. This note makes sense on exported functions, but I think it's overkill on static ones like this. s/Its normally/It's normally/ s/new pci/new PCI/ Rewrap into one paragraph (or separate by blank line). Applies to all the hunks of this patch. > static int __pci_request_region(struct pci_dev *pdev, int bar, > const char *res_name, int exclusive)
Re: [PATCH v6 04/10] PCI: Make devres region requests consistent
On Mon, Apr 08, 2024 at 10:44:16AM +0200, Philipp Stanner wrote:
> Now that pure managed region request functions are available, the
> implementation of the hybrid-functions which are only sometimes managed
> can be made more consistent and readable by wrapping those
> always-managed functions.
>
> Implement a new pcim_ function for exclusively requested regions.
> Have the pci_request / release functions call their pcim_ counterparts.
> Remove the now surplus region_mask from struct pci_devres.
This looks like two patches; could they be separated?
- Convert __pci_request_region() etc to the new pcim model
- Add pcim_request_region_exclusive()
IORESOURCE_EXCLUSIVE was added by e8de1481fd71 ("resource: allow MMIO
exclusivity for device drivers") in 2008 to help debug an e1000e
problem. In the 16 years since, there's only been one new PCI-related
use (ne_pci_probe()), and we don't add a user of
pcim_request_region_exclusive() in this series, so I would defer it
until somebody wants it.
Bjorn
Re: [PATCH v6 00/10] Make PCI's devres API more consistent
On Mon, Apr 08, 2024 at 10:44:12AM +0200, Philipp Stanner wrote: > ... > PCI's devres API suffers several weaknesses: > > 1. There are functions prefixed with pcim_. Those are always managed >counterparts to never-managed functions prefixed with pci_ – or so one >would like to think. There are some apparently unmanaged functions >(all region-request / release functions, and pci_intx()) which >suddenly become managed once the user has initialized the device with >pcim_enable_device() instead of pci_enable_device(). This "sometimes >yes, sometimes no" nature of those functions is confusing and >therefore bug-provoking. In fact, it has already caused a bug in DRM. >The last patch in this series fixes that bug. > 2. iomappings: Instead of giving each mapping its own callback, the >existing API uses a statically allocated struct tracking one mapping >per bar. This is not extensible. Especially, you can't create >_ranged_ managed mappings that way, which many drivers want. > 3. Managed request functions only exist as "plural versions" with a >bit-mask as a parameter. That's quite over-engineered considering >that each user only ever mapps one, maybe two bars. > > This series: > - add a set of new "singular" devres functions that use devres the way > its intended, with one callback per resource. > - deprecates the existing iomap-table mechanism. > - deprecates the hybrid nature of pci_ functions. > - preserves backwards compatibility so that drivers using the existing > API won't notice any changes. > - adds documentation, especially some warning users about the > complicated nature of PCI's devres. There's a lot of good work here; thanks for working on it. > Philipp Stanner (10): > PCI: Add new set of devres functions This first patch adds some infrastructure and several new exported interfaces: void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar, const char *name) void pcim_iounmap_region(struct pci_dev *pdev, int bar) int pcim_request_region(struct pci_dev *pdev, int bar, const char *name) void pcim_release_region(struct pci_dev *pdev, int bar) void __iomem *pcim_iomap_range(struct pci_dev *pdev, int bar, void __iomem *pcim_iomap_region_range(struct pci_dev *pdev, int bar, void pcim_iounmap_region_range(struct pci_dev *pdev, int bar, > PCI: Deprecate iomap-table functions This adds a little bit of infrastructure (add/remove to legacy_table), reimplements these existing interfaces: void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned long maxlen) void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr) int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char *name) int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask, void pcim_iounmap_regions(struct pci_dev *pdev, int mask) and adds a couple new exported interfaces: void pcim_release_all_regions(struct pci_dev *pdev) int pcim_request_all_regions(struct pci_dev *pdev, const char *name) There's a lot going on in these two patches, so they're hard to review. I think it would be easier if you could do the fixes to existing interfaces first, followed by adding new things, maybe something like separate patches that: - Add pcim_addr_devres_alloc(), pcim_addr_devres_free(), pcim_addr_devres_clear(). - Add pcim_add_mapping_to_legacy_table(), pcim_remove_mapping_from_legacy_table(), pcim_remove_bar_from_legacy_table(). - Reimplement pcim_iomap(), pcim_iomap_regions(), pcim_iounmap(). - Add new interfaces like pcim_iomap_region(), pcim_request_region(), etc. AFAICS, except for pcim_iomap_range() (used by vbox), these new interfaces have no users outside drivers/pci, so ... we might defer adding them, or at least defer exposing them via include/linux/pci.h, until we have users for them. > PCI: Warn users about complicated devres nature > PCI: Make devres region requests consistent > PCI: Move dev-enabled status bit to struct pci_dev > PCI: Move pinned status bit to struct pci_dev > PCI: Give pcim_set_mwi() its own devres callback > PCI: Give pci(m)_intx its own devres callback > PCI: Remove legacy pcim_release() > drm/vboxvideo: fix mapping leaks > > drivers/gpu/drm/vboxvideo/vbox_main.c | 20 +- > drivers/pci/devres.c | 1011 + > drivers/pci/iomap.c | 18 + > drivers/pci/pci.c | 123 ++- > drivers/pci/pci.h | 21 +- > include/linux/pci.h | 18 +- > 6 files changed, 999 insertions(+), 212 deletions(-) > > -- > 2.44.0 >
Re: [PATCH v7 00/37] Device Tree support for SH7751 based board
On Thu, Apr 04, 2024 at 01:59:25PM +0900, Yoshinori Sato wrote: > This is an updated version of something I wrote about 7 years ago. > Minimum support for R2D-plus and LANDISK. > I think R2D-1 will work if you add AX88796 to dts. > And board-specific functions and SCI's SPI functions are not supported. My comments/questions from https://lore.kernel.org/r/20231120181600.GA205977@bhelgaas https://lore.kernel.org/r/20231016172742.GA1215127@bhelgaas still apply.
Re: [PATCH v4 10/10] drm/vboxvideo: fix mapping leaks
On Fri, Mar 01, 2024 at 12:29:58PM +0100, Philipp Stanner wrote:
> When the PCI devres API was introduced to this driver, it was wrongly
> assumed that initializing the device with pcim_enable_device() instead
> of pci_enable_device() will make all PCI functions managed.
>
> This is wrong and was caused by the quite confusing PCI devres API in
> which some, but not all, functions become managed that way.
>
> The function pci_iomap_range() is never managed.
>
> Replace pci_iomap_range() with the actually managed function
> pcim_iomap_range().
>
> CC: # v5.10+
This is marked for stable but depends on the preceding patches in this
series, which are not marked for stable.
The rest of this series might be picked up automatically for stable,
but I personally wouldn't suggest backporting it because it's quite a
lot of change and I don't think it fits per
Documentation/process/stable-kernel-rules.rst.
So I think the best way to fix the vboxvideo leaks would be to fix
them independently of this series, then include as a separate patch a
conversion to the new pcim_iomap_range() in this series (or possibly
for the next merge window to avoid merge conflicts).
> Fixes: 8558de401b5f ("drm/vboxvideo: use managed pci functions")
> Signed-off-by: Philipp Stanner
> ---
> drivers/gpu/drm/vboxvideo/vbox_main.c | 20 +---
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_main.c
> b/drivers/gpu/drm/vboxvideo/vbox_main.c
> index 42c2d8a99509..d4ade9325401 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_main.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_main.c
> @@ -42,12 +42,11 @@ static int vbox_accel_init(struct vbox_private *vbox)
> /* Take a command buffer for each screen from the end of usable VRAM. */
> vbox->available_vram_size -= vbox->num_crtcs * VBVA_MIN_BUFFER_SIZE;
>
> - vbox->vbva_buffers = pci_iomap_range(pdev, 0,
> - vbox->available_vram_size,
> - vbox->num_crtcs *
> - VBVA_MIN_BUFFER_SIZE);
> - if (!vbox->vbva_buffers)
> - return -ENOMEM;
> + vbox->vbva_buffers = pcim_iomap_range(
> + pdev, 0, vbox->available_vram_size,
> + vbox->num_crtcs * VBVA_MIN_BUFFER_SIZE);
> + if (IS_ERR(vbox->vbva_buffers))
> + return PTR_ERR(vbox->vbva_buffers);
>
> for (i = 0; i < vbox->num_crtcs; ++i) {
> vbva_setup_buffer_context(>vbva_info[i],
> @@ -116,11 +115,10 @@ int vbox_hw_init(struct vbox_private *vbox)
> DRM_INFO("VRAM %08x\n", vbox->full_vram_size);
>
> /* Map guest-heap at end of vram */
> - vbox->guest_heap =
> - pci_iomap_range(pdev, 0, GUEST_HEAP_OFFSET(vbox),
> - GUEST_HEAP_SIZE);
> - if (!vbox->guest_heap)
> - return -ENOMEM;
> + vbox->guest_heap = pcim_iomap_range(pdev, 0,
> + GUEST_HEAP_OFFSET(vbox), GUEST_HEAP_SIZE);
> + if (IS_ERR(vbox->guest_heap))
> + return PTR_ERR(vbox->guest_heap);
>
> /* Create guest-heap mem-pool use 2^4 = 16 byte chunks */
> vbox->guest_pool = devm_gen_pool_create(vbox->ddev.dev, 4, -1,
> --
> 2.43.0
>
Re: [PATCH v4 00/10] Make PCI's devres API more consistent
On Mon, Mar 11, 2024 at 12:45:15PM +0100, Philipp Stanner wrote: > Gentle ping because the Merge Window opened 8-) Thanks; I'll look at this for v6.10. It's too late to add significant changes for the v6.9 merge window since it's already open. Bjorn > On Fri, 2024-03-01 at 12:29 +0100, Philipp Stanner wrote: > > This v4 now can (hopefully) be applied to linux-next, but not to > > mainline/master. > > > > Changes in v4: > > - Rebase against linux-next > > > > Changes in v3: > > - Use the term "PCI devres API" at some forgotten places. > > - Fix more grammar errors in patch #3. > > - Remove the comment advising to call (the outdated) pcim_intx() in > > pci.c > > - Rename __pcim_request_region_range() flags-field "exclusive" to > > "req_flags", since this is what the int actually represents. > > - Remove the call to pcim_region_request() from patch #10. (Hans) > > > > Changes in v2: > > - Make commit head lines congruent with PCI's style (Bjorn) > > - Add missing error checks for devm_add_action(). (Andy) > > - Repair the "Returns: " marks for docu generation (Andy) > > - Initialize the addr_devres struct with memset(). (Andy) > > - Make pcim_intx() a PCI-internal function so that new drivers > > won't > > be encouraged to use the outdated pci_intx() mechanism. > > (Andy / Philipp) > > - Fix grammar and spelling (Bjorn) > > - Be more precise on why pcim_iomap_table() is problematic (Bjorn) > > - Provide the actual structs' and functions' names in the commit > > messages (Bjorn) > > - Remove redundant variable initializers (Andy) > > - Regroup PM bitfield members in struct pci_dev (Andy) > > - Make pcim_intx() visible only for the PCI subsystem so that > > new > > drivers won't use this outdated API (Andy, Myself) > > - Add a NOTE to pcim_iomap() to warn about this function being > > the onee > > xception that does just return NULL. > > - Consistently use the term "PCI devres API"; also in Patch #10 > > (Bjorn) > > > > > > ¡Hola! > > > > PCI's devres API suffers several weaknesses: > > > > 1. There are functions prefixed with pcim_. Those are always managed > > counterparts to never-managed functions prefixed with pci_ – or so > > one > > would like to think. There are some apparently unmanaged functions > > (all region-request / release functions, and pci_intx()) which > > suddenly become managed once the user has initialized the device > > with > > pcim_enable_device() instead of pci_enable_device(). This > > "sometimes > > yes, sometimes no" nature of those functions is confusing and > > therefore bug-provoking. In fact, it has already caused a bug in > > DRM. > > The last patch in this series fixes that bug. > > 2. iomappings: Instead of giving each mapping its own callback, the > > existing API uses a statically allocated struct tracking one > > mapping > > per bar. This is not extensible. Especially, you can't create > > _ranged_ managed mappings that way, which many drivers want. > > 3. Managed request functions only exist as "plural versions" with a > > bit-mask as a parameter. That's quite over-engineered considering > > that each user only ever mapps one, maybe two bars. > > > > This series: > > - add a set of new "singular" devres functions that use devres the > > way > > its intended, with one callback per resource. > > - deprecates the existing iomap-table mechanism. > > - deprecates the hybrid nature of pci_ functions. > > - preserves backwards compatibility so that drivers using the > > existing > > API won't notice any changes. > > - adds documentation, especially some warning users about the > > complicated nature of PCI's devres. > > > > > > Note that this series is based on my "unify pci_iounmap"-series from > > a > > few weeks ago. [1] > > > > I tested this on a x86 VM with a simple pci test-device with two > > regions. Operates and reserves resources as intended on my system. > > Kasan and kmemleak didn't find any problems. > > > > I believe this series cleans the API up as much as possible without > > having to port all existing drivers to the new API. Especially, I > > think > > that this implementation is easy to extend if the need for new > > managed > > functions arises :) > > > > Greetings, > > P. > > > > Philipp Stanner (10): > > PCI: Add new set of devres functions > > PCI: Deprecate iomap-table functions > > PCI: Warn users about complicated devres nature > > PCI: Make devres region requests consistent > > PCI: Move dev-enabled status bit to struct pci_dev > > PCI: Move pinned status bit to struct pci_dev > > PCI: Give pcim_set_mwi() its own devres callback > > PCI: Give pci(m)_intx its own devres callback > > PCI: Remove legacy pcim_release() > > drm/vboxvideo: fix mapping leaks > > > > drivers/gpu/drm/vboxvideo/vbox_main.c | 20 +- > > drivers/pci/devres.c | 1013 +-- > > -- > >
Re: [PATCH v3 00/10] Make PCI's devres API more consistent
On Thu, Feb 29, 2024 at 09:31:20AM +0100, Philipp Stanner wrote: > @Bjorn: > Hey Bjorn, are we good with this series? Any more wishes or > suggestions? Sorry, haven't had a chance to go through it yet. FWIW, I just tried to apply these on top of pci/devres, but it failed here: Applying: PCI: Add new set of devres functions Applying: PCI: Deprecate iomap-table functions Applying: PCI: Warn users about complicated devres nature Applying: PCI: Make devres region requests consistent Applying: PCI: Move dev-enabled status bit to struct pci_dev error: patch failed: drivers/pci/pci.h:811 error: drivers/pci/pci.h: patch does not apply Patch failed at 0005 PCI: Move dev-enabled status bit to struct pci_dev Haven't investigated, so maybe it's some trivial easily fixed thing. Bjorn
[PATCH] drm/fsl-dcu: remove unnecessary NULL checks
From: Bjorn Helgaas
The power management callbacks are never called unless .probe() has already
returned success, which means it has set drvdata to a non-NULL pointer, so
"dev" can never be NULL in the other callbacks.
Remove the unnecessary checks.
Signed-off-by: Bjorn Helgaas
---
drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
index ab6c0c6cd0e2..ca23a2ca16bb 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -170,9 +170,6 @@ static int fsl_dcu_drm_pm_suspend(struct device *dev)
struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
int ret;
- if (!fsl_dev)
- return 0;
-
disable_irq(fsl_dev->irq);
ret = drm_mode_config_helper_suspend(fsl_dev->drm);
@@ -191,9 +188,6 @@ static int fsl_dcu_drm_pm_resume(struct device *dev)
struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
int ret;
- if (!fsl_dev)
- return 0;
-
ret = clk_prepare_enable(fsl_dev->clk);
if (ret < 0) {
dev_err(dev, "failed to enable dcu clk\n");
--
2.34.1
[PATCH] drm/amdgpu: remove misleading amdgpu_pmops_runtime_idle() comment
From: Bjorn Helgaas
After 4020c2280233 ("drm/amdgpu: don't runtime suspend if there are
displays attached (v3)"), "ret" is unconditionally set later before being
used, so there's point in initializing it and the associated comment is no
longer meaningful.
Remove the comment and the unnecessary initialization.
Signed-off-by: Bjorn Helgaas
---
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index cc69005f5b46..68416e2a9130 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2744,8 +2744,7 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
{
struct drm_device *drm_dev = dev_get_drvdata(dev);
struct amdgpu_device *adev = drm_to_adev(drm_dev);
- /* we don't want the main rpm_idle to call suspend - we want to
autosuspend */
- int ret = 1;
+ int ret;
if (adev->pm.rpm_mode == AMDGPU_RUNPM_NONE) {
pm_runtime_forbid(dev);
--
2.34.1
Re: [PATCH 3/3] drm/amdgpu: wire up the can_remove() callback
[+cc Bartosz]
On Fri, Feb 02, 2024 at 05:25:56PM -0500, Hamza Mahfooz wrote:
> Removing an amdgpu device that still has user space references allocated
> to it causes undefined behaviour. So, implement amdgpu_pci_can_remove()
> and disallow devices that still have files allocated to them from being
> unbound.
Maybe this would help for things that are completely built-in or
soldered down, but nothing can prevent a user from physically pulling
a card or cable, so I don't think this is a generic solution to the
problem of dangling user space references.
Maybe Bartosz's recent LPC talk is relevant:
https://lpc.events/event/17/contributions/1627/
> Cc: sta...@vger.kernel.org
> Signed-off-by: Hamza Mahfooz
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 17 +
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index cc69005f5b46..cfa64f3c5be5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2323,6 +2323,22 @@ static int amdgpu_pci_probe(struct pci_dev *pdev,
> return ret;
> }
>
> +static bool amdgpu_pci_can_remove(struct pci_dev *pdev)
> +{
> + struct drm_device *dev = pci_get_drvdata(pdev);
> +
> + mutex_lock(>filelist_mutex);
> +
> + if (!list_empty(>filelist)) {
> + mutex_unlock(>filelist_mutex);
> + return false;
> + }
> +
> + mutex_unlock(>filelist_mutex);
> +
> + return true;
> +}
> +
> static void
> amdgpu_pci_remove(struct pci_dev *pdev)
> {
> @@ -2929,6 +2945,7 @@ static struct pci_driver amdgpu_kms_pci_driver = {
> .name = DRIVER_NAME,
> .id_table = pciidlist,
> .probe = amdgpu_pci_probe,
> + .can_remove = amdgpu_pci_can_remove,
> .remove = amdgpu_pci_remove,
> .shutdown = amdgpu_pci_shutdown,
> .driver.pm = _pm_ops,
> --
> 2.43.0
>
Re: [PATCH v4 1/3] pm: runtime: Simplify pm_runtime_get_if_active() usage
On Tue, Jan 23, 2024 at 08:44:04PM +, Sakari Ailus wrote: > On Tue, Jan 23, 2024 at 11:24:23AM -0600, Bjorn Helgaas wrote: > ... > > - I don't know whether it's feasible, but it would be nice if the > > intel_pm_runtime_pm.c rework could be done in one shot instead of > > being split between patches 1/3 and 2/3. > > > > Maybe it could be a preliminary patch that uses the existing > > if_active/if_in_use interfaces, followed by the trivial if_active > > updates in this patch. I think that would make the history easier > > to read than having the transitory pm_runtime_get_conditional() in > > the middle. > > I think I'd merge the two patches. The second patch is fairly small, after > all, and both deal with largely the same code. I'm not sure which two patches you mean, but the fact that two patches deal with largely the same code is not necessarily an argument for merging them. From a reviewing perspective, it's nice if a patch like 1/3, where it's largely mechanical and easy to review, is separated from patches that make more substantive changes. That's why I think it'd be nice if the "interesting" intel_pm_runtime_pm.c changes were all in the same patch, and ideally, if that patch *only* touched intel_pm_runtime_pm.c. Bjorn
Re: [PATCH v4 1/3] pm: runtime: Simplify pm_runtime_get_if_active() usage
On Tue, Jan 23, 2024 at 11:56:42AM +0200, Sakari Ailus wrote: > There are two ways to opportunistically increment a device's runtime PM > usage count, calling either pm_runtime_get_if_active() or > pm_runtime_get_if_in_use(). The former has an argument to tell whether to > ignore the usage count or not, and the latter simply calls the former with > ign_usage_count set to false. The other users that want to ignore the > usage_count will have to explitly set that argument to true which is a bit > cumbersome. > > To make this function more practical to use, remove the ign_usage_count > argument from the function. The main implementation is renamed as > pm_runtime_get_conditional(). > > Signed-off-by: Sakari Ailus > Reviewed-by: Alex Elder # drivers/net/ipa/ipa_smp2p.c > Reviewed-by: Laurent Pinchart > Acked-by: Takashi Iwai # sound/ > Reviewed-by: Jacek Lawrynowicz # > drivers/accel/ivpu/ > Acked-by: Rodrigo Vivi # drivers/gpu/drm/i915/ > Reviewed-by: Rodrigo Vivi Acked-by: Bjorn Helgaas # drivers/pci/ - Previous PM history uses "PM: " in the subject lines (not "pm: "). - I don't know whether it's feasible, but it would be nice if the intel_pm_runtime_pm.c rework could be done in one shot instead of being split between patches 1/3 and 2/3. Maybe it could be a preliminary patch that uses the existing if_active/if_in_use interfaces, followed by the trivial if_active updates in this patch. I think that would make the history easier to read than having the transitory pm_runtime_get_conditional() in the middle. - Similarly, it would be nice if pm_runtime_get_conditional() never had to be published in pm_runtime.h, instead of being temporarily added there by this patch and then immediately made private by 2/3. Maybe that's not practical, I dunno. Bjorn
Re: [PATCH v3 1/2] pm: runtime: Simplify pm_runtime_get_if_active() usage
On Mon, Jan 22, 2024 at 01:41:21PM +0200, Sakari Ailus wrote: > There are two ways to opportunistically increment a device's runtime PM > usage count, calling either pm_runtime_get_if_active() or > pm_runtime_get_if_in_use(). The former has an argument to tell whether to > ignore the usage count or not, and the latter simply calls the former with > ign_usage_count set to false. The other users that want to ignore the > usage_count will have to explitly set that argument to true which is a bit > cumbersome. s/explitly/explicitly/ > To make this function more practical to use, remove the ign_usage_count > argument from the function. The main implementation is renamed as > pm_runtime_get_conditional(). > > Signed-off-by: Sakari Ailus > Reviewed-by: Alex Elder # drivers/net/ipa/ipa_smp2p.c > Reviewed-by: Laurent Pinchart > Acked-by: Takashi Iwai # sound/ > Reviewed-by: Jacek Lawrynowicz # > drivers/accel/ivpu/ > Acked-by: Rodrigo Vivi # drivers/gpu/drm/i915/ > Reviewed-by: Rodrigo Vivi Acked-by: Bjorn Helgaas # drivers/pci/ > -EXPORT_SYMBOL_GPL(pm_runtime_get_if_active); > +EXPORT_SYMBOL_GPL(pm_runtime_get_conditional); If pm_runtime_get_conditional() is exported, shouldn't it also be documented in Documentation/power/runtime_pm.rst? But I'm dubious about exporting it because __intel_runtime_pm_get_if_active() is the only caller, and you end up with the same pattern there that we have before this series in the PM core. Why can't intel_runtime_pm.c be updated to use pm_runtime_get_if_active() or pm_runtime_get_if_in_use() directly, and make pm_runtime_get_conditional() static? > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -246,7 +246,7 @@ static intel_wakeref_t > __intel_runtime_pm_get_if_active(struct intel_runtime_pm >* function, since the power state is undefined. This applies >* atm to the late/early system suspend/resume handlers. >*/ > - if (pm_runtime_get_if_active(rpm->kdev, ignore_usecount) <= 0) > + if (pm_runtime_get_conditional(rpm->kdev, ignore_usecount) <= 0) > return 0; > } Bjorn
Re: [PATCH 01/10] pci: add new set of devres functions
On Wed, Jan 17, 2024 at 09:54:47AM +0100, Philipp Stanner wrote: > On Tue, 2024-01-16 at 12:44 -0600, Bjorn Helgaas wrote: > > On Mon, Jan 15, 2024 at 03:46:12PM +0100, Philipp Stanner wrote: > > > PCI's devres API is not extensible to ranged mappings and has > > > bug-provoking features. Improve that by providing better > > > alternatives. > > > > I guess "ranged mappings" means a mapping that doesn't cover an > > entire BAR? Maybe there's a way to clarify? > > That's what it's supposed to mean, yes. We could give it the longer > title "mappings smaller than the whole BAR" or something, I guess. "partial BAR mappings"? > > > to the creation of a set of "pural functions" such as s/pural/plural/ (I missed this before). > > > c) The iomap-table mechanism is over-engineered, > > > complicated and > > > can by definition not perform bounds checks, thus, > > > provoking > > > memory faults: pcim_iomap_table(pdev)[42] > > > > Not sure what "pcim_iomap_table(pdev)[42]" means. > > That function currently is implemented with this prototype: > void __iomem * const *pcim_iomap_table(struct pci_dev *pdev); > > And apparently, it's intended to index directly over the function. And > that's how at least part of the users use it indeed. > > Here in drivers/crypto/inside-secure/safexcel.c, L.1919 for example: > > priv->base = pcim_iomap_table(pdev)[0]; > > I've never seen something that wonderful in C ever before, so it's not > surprising that you weren't sure what I mean > > pcim_iomap_table() can not and does not perform any bounds check. If > you do > > void __iomem *mappy_map_mapface = pcim_iomap_table(pdev)[42]; > > then it will just return random garbage, or it faults. No -EINVAL or > anything. You won't even get NULL. > > That's why this function must die. No argument except that this example only makes sense after one looks up the prototype and connects the dots. Bjorn
Re: [PATCH 01/10] pci: add new set of devres functions
On Mon, Jan 15, 2024 at 03:46:12PM +0100, Philipp Stanner wrote: > PCI's devres API is not extensible to ranged mappings and has > bug-provoking features. Improve that by providing better alternatives. I guess "ranged mappings" means a mapping that doesn't cover an entire BAR? Maybe there's a way to clarify? > When the original devres API for PCI was implemented, priority was given > to the creation of a set of "pural functions" such as > pcim_request_regions(). These functions have bit masks as parameters to > specify which BARs shall get mapped. Most users, however, only use those > to mapp 1-3 BARs. > A complete set of "singular functions" does not exist. s/mapp/map/ Rewrap to fill 75 columns or add blank lines between paragraphs. Also below. > As functions mapping / requesting multiple BARs at once have (almost) no > mechanism in C to return the resources to the caller of the plural > function, the devres API utilizes the iomap-table administrated by the > function pcim_iomap_table(). > > The entire PCI devres implementation was strongly tied to that table > which only allows for mapping whole, complete BARs, as the BAR's index > is used as table index. Consequently, it's not possible to, e.g., have a > pcim_iomap_range() function with that mechanism. > > An additional problem is that pci-devres has been ipmlemented in a sort > of "hybrid-mode": Some unmanaged functions have managed counterparts > (e.g.: pci_iomap() <-> pcim_iomap()), making their managed nature > obvious to the programmer. However, the region-request functions in > pci.c, prefixed with pci_, behave either managed or unmanaged, depending > on whether pci_enable_device() or pcim_enable_device() has been called > in advance. s/ipmlemented/implemented/ > This hybrid API is confusing and should be more cleanly separated by > providing always-managed functions prefixed with pcim_. > > Thus, the existing devres API is not desirable because: > a) The vast majority of the users of the plural functions only > ever sets a single bit in the bit mask, consequently making > them singular functions anyways. > b) There is no mechanism to request / iomap only part of a BAR. > c) The iomap-table mechanism is over-engineered, complicated and > can by definition not perform bounds checks, thus, provoking > memory faults: pcim_iomap_table(pdev)[42] Not sure what "pcim_iomap_table(pdev)[42]" means. > d) region-request functions being sometimes managed and > sometimes not is bug-provoking. Indent with spaces (not tabs) so it still looks good when "git log" adds spaces to indent. > + * Legacy struct storing addresses to whole mapped bars. s/bar/BAR/ (several places). > + /* A region spaning an entire bar. */ > + PCIM_ADDR_DEVRES_TYPE_REGION, > + > + /* A region spaning an entire bar, and a mapping for that whole bar. */ > + PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING, > + > + /* > + * A mapping within a bar, either spaning the whole bar or just a range. > + * Without a requested region. s/spaning/spanning/ (several places). > + if (start == 0 || len == 0) /* that's an unused BAR. */ s/that/That/ > + /* > + * Ranged mappings don't get added to the legacy-table, since the table > + * only ever keeps track of whole BARs. > + */ > + Spurious blank line. > + devres_add(>dev, res); > + return mapping; > +} > +EXPORT_SYMBOL(pcim_iomap_range); Bjorn
Re: [PATCH 07/10] pci: devres: give mwi its own callback
On Mon, Jan 15, 2024 at 03:46:18PM +0100, Philipp Stanner wrote:
> managing mwi with devres can easily be done with its own callback,
> without the necessity to store any state about it in a device-related
> struct.
s/managing/Managing/
s/mwi/MWI/ since this is an initialism. Also in subject (but would be
even better if it could mention an actual function name).
> Remove the mwi state from the devres-struct.
> Make the devres-mwi functions set a separate devres-callback.
Wrap to fill 75 columns (or add blank lines if you intend multiple
paragraphs).
s/devres-struct/struct pci_devres/ to make this greppable.
s/the devres-mwi functions/pcim_set_mwi()/ similarly.
> Signed-off-by: Philipp Stanner
> ---
> drivers/pci/devres.c | 23 +++
> drivers/pci/pci.h| 1 -
> 2 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/devres.c b/drivers/pci/devres.c
> index de8cf6f87dd7..911c2037d9fd 100644
> --- a/drivers/pci/devres.c
> +++ b/drivers/pci/devres.c
> @@ -378,24 +378,26 @@ void __iomem *devm_pci_remap_cfg_resource(struct device
> *dev,
> }
> EXPORT_SYMBOL(devm_pci_remap_cfg_resource);
>
> +static void __pcim_clear_mwi(void *pdev_raw)
> +{
> + struct pci_dev *pdev = pdev_raw;
> +
> + pci_clear_mwi(pdev);
> +}
> +
> /**
> * pcim_set_mwi - a device-managed pci_set_mwi()
> - * @dev: the PCI device for which MWI is enabled
> + * @pdev: the PCI device for which MWI is enabled
> *
> * Managed pci_set_mwi().
> *
> * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
> */
> -int pcim_set_mwi(struct pci_dev *dev)
> +int pcim_set_mwi(struct pci_dev *pdev)
> {
> - struct pci_devres *dr;
> -
> - dr = find_pci_dr(dev);
> - if (!dr)
> - return -ENOMEM;
> + devm_add_action(>dev, __pcim_clear_mwi, pdev);
>
> - dr->mwi = 1;
> - return pci_set_mwi(dev);
> + return pci_set_mwi(pdev);
> }
> EXPORT_SYMBOL(pcim_set_mwi);
>
> @@ -405,9 +407,6 @@ static void pcim_release(struct device *gendev, void *res)
> struct pci_dev *dev = to_pci_dev(gendev);
> struct pci_devres *this = res;
>
> - if (this->mwi)
> - pci_clear_mwi(dev);
> -
> if (this->restore_intx)
> pci_intx(dev, this->orig_intx);
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 3d9908a69ebf..667bfdd61d5e 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -811,7 +811,6 @@ static inline pci_power_t mid_pci_get_power_state(struct
> pci_dev *pdev)
> struct pci_devres {
> unsigned int orig_intx:1;
> unsigned int restore_intx:1;
> - unsigned int mwi:1;
> };
>
> struct pci_devres *find_pci_dr(struct pci_dev *pdev);
> --
> 2.43.0
>
Re: [PATCH 00/10] Make PCI's devres API more consistent
On Mon, Jan 15, 2024 at 03:46:11PM +0100, Philipp Stanner wrote: > ... > pci: add new set of devres functions > pci: deprecate iomap-table functions > pci: warn users about complicated devres nature > pci: devres: make devres region requests consistent > pci: move enabled status bit to pci_dev struct > pci: move pinned status bit to pci_dev struct > pci: devres: give mwi its own callback > pci: devres: give pci(m)_intx its own callback > pci: devres: remove legacy pcim_release() > drm/vboxvideo: fix mapping leaks If/when you update these, take a look at the drivers/pci/ subject line history and capitalize these to match. We haven't really used the "devres" prefix in drivers/pci. The de facto convention is: - "PCI/AER:" for major features defined by the PCIe base spec (AER, DPC, VGA, ASPM, PM, etc). - "PCI: iproc:" for controller drivers (iproc, dwc, qcom, mvebu, etc). - "PCI:" for everything else
Re: [PATCH v2 0/9] Improvements to pcie_bandwidth_available() for eGPUs
On Fri, Nov 03, 2023 at 02:07:49PM -0500, Mario Limonciello wrote: > Downstream drivers are getting the wrong values from > pcie_bandwidth_available() which is causing problems for performance > of eGPUs. > > This series overhauls Thunderbolt related device detection and uses > the changes to change the behavior of pcie_bandwidth_available(). > > NOTE: This series is currently based on top of v6.6 + this change that > will be merged for 6.7: > Link: https://patchwork.freedesktop.org/patch/564738/ Thanks, Mario, I'll look at this soon after v6.7-rc1 (probably Nov 12), so the amdgpu patch should be in mainline by then. > v1->v2: > * Rename is_thunderbolt > * Look for _DSD instead of link > * Drop pci_is_thunderbolt_attached() from all drivers > * Adjust links > * Adjust commit messages > * Add quirk for Tiger Lake > > Mario Limonciello (9): > drm/nouveau: Switch from pci_is_thunderbolt_attached() to > dev_is_removable() > drm/radeon: Switch from pci_is_thunderbolt_attached() to > dev_is_removable() > PCI: Drop pci_is_thunderbolt_attached() > PCI: Move the `PCI_CLASS_SERIAL_USB_USB4` definition to common header > PCI: pciehp: Move check for is_thunderbolt into a quirk > PCI: Rename is_thunderbolt to is_tunneled > PCI: ACPI: Detect PCIe root ports that are used for tunneling > PCI: Exclude PCIe ports used for tunneling in > pcie_bandwidth_available() > PCI: Add a quirk to mark 0x8086 : 0x9a23 as supporting PCIe tunneling > > drivers/gpu/drm/nouveau/nouveau_vga.c | 6 +- > drivers/gpu/drm/radeon/radeon_device.c | 4 +- > drivers/gpu/drm/radeon/radeon_kms.c| 2 +- > drivers/pci/hotplug/pciehp_hpc.c | 6 +- > drivers/pci/pci-acpi.c | 16 ++ > drivers/pci/pci.c | 76 +- > drivers/pci/probe.c| 2 +- > drivers/pci/quirks.c | 31 +++ > drivers/platform/x86/apple-gmux.c | 2 +- > drivers/thunderbolt/nhi.h | 2 - > include/linux/pci.h| 25 + > include/linux/pci_ids.h| 1 + > 12 files changed, 109 insertions(+), 64 deletions(-) > > -- > 2.34.1 >
Re: [-next 0/5] Add the pci_is_vga() helper and use it
On Wed, Aug 30, 2023 at 07:15:27PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> The PCI code and ID assignment specification defined four types of
> display controllers for the display base class(03h), and the devices
> with 0x00h sub-class code are VGA devices. VGA devices with programming
> interface 0x00 is VGA-compatible, VGA devices with programming interface
> 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
> is defined to provide backward compatibility for devices that were built
> before the class code field was defined. Thus, PCI(e) device with the
> PCI_CLASS_NOT_DEFINED_VGA class code should also be handled as the normal
> VGA-compatible devices.
>
> Compared with the "if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)" code,
> the newly implemented pci_is_vga() is shorter and straightforward. So it
> is more easy to use. It is designed as a inline function, the more common
> case "if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA))" is put before the
> less common case "if ((pdev->class >> 8) == PCI_CLASS_NOT_DEFINED_VGA)",
> so there should no performance penalty.
>
> Sui Jingfeng (5):
> PCI: Add the pci_is_vga() helper
> PCI/VGA: Deal with VGA devices
> PCI/sysfs: Use pci_is_vga() helper
> drm/virgpu: Switch to pci_is_vga()
> drm/qxl: Switch to pci_is_vga()
>
> drivers/gpu/drm/qxl/qxl_drv.c| 11 +++
> drivers/gpu/drm/virtio/virtgpu_drv.c | 2 +-
> drivers/pci/pci-sysfs.c | 6 +++---
> drivers/pci/vgaarb.c | 19 +--
> include/linux/pci.h | 27 +++
> 5 files changed, 43 insertions(+), 22 deletions(-)
I applied these on pci/vga for v6.7, thanks!
Please take a look at
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=vga
and let me know if I broke anything because I changed a few things;
interdiff below:
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 522708938563..252ee29fd697 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1551,9 +1551,8 @@ static umode_t pci_dev_attrs_are_visible(struct kobject
*kobj,
struct device *dev = kobj_to_dev(kobj);
struct pci_dev *pdev = to_pci_dev(dev);
- if (a == _attr_boot_vga.attr)
- if (pci_is_vga(pdev))
- return a->mode;
+ if (a == _attr_boot_vga.attr && pci_is_vga(pdev))
+ return a->mode;
return 0;
}
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index ef8fe685de67..feca96fc4255 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -1499,6 +1499,7 @@ static int pci_notify(struct notifier_block *nb, unsigned
long action,
vgaarb_dbg(dev, "%s\n", __func__);
+ /* Only deal with VGA class devices */
if (!pci_is_vga(pdev))
return 0;
@@ -1536,8 +1537,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)
@@ -1546,11 +1547,13 @@ static int __init vga_arb_device_init(void)
bus_register_notifier(_bus_type, _notifier);
/* Add all VGA class PCI devices by default */
- do {
- pdev = pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
PCI_ANY_ID, pdev);
+ pdev = NULL;
+ while ((pdev =
+ pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
+ PCI_ANY_ID, pdev)) != NULL) {
if (pci_is_vga(pdev))
vga_arbiter_add_pci_device(pdev);
- } while (pdev);
+ }
pr_info("loaded\n");
return rc;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 53e24e31c27e..7bab234391cb 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -714,23 +714,20 @@ static inline bool pci_is_bridge(struct pci_dev *dev)
}
/**
- * The PCI code and ID assignment specification defined four types of
- * display controllers for the display base class(03h), and the devices
- * with 0x00h sub-class code are VGA devices. VGA devices with programming
- * interface 0x00 is VGA-compatible, VGA devices with programming interface
- * 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
- * is defined to provide backward compatibility for devices that were built
- * before the class code field was defined. This means that it belong to the
- * VGA devices category also.
+ * pci_is_vga - check if the PCI device is a VGA device
*
- * Returns:
- * true if the PCI device is a VGA device, false otherwise.
+ * The PCI Code and ID Assignment spec, r1.15, secs 1.4 and 1.1, define
+ * VGA Base Class and Sub-Classes:
+ *
+ * 03 00 PCI_CLASS_DISPLAY_VGA VGA-compatible or 8514-compatible
+ * 00 01 PCI_CLASS_NOT_DEFINED_VGA VGA-compatible (before Class Code)
+ *
+ * Return true if
Re: [-next 1/5] PCI: Add the pci_is_vga() helper
On Wed, Aug 30, 2023 at 07:15:28PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> The PCI code and ID assignment specification defined four types of
> display controllers for the display base class(03h), and the devices
> with 0x00h sub-class code are VGA devices. VGA devices with programming
I can update this with the spec details (PCI Code and Assignment spec
r1.15, secs 1.1 and 1.4).
> interface 0x00 is VGA-compatible, VGA devices with programming interface
> 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
> is defined to provide backward compatibility for devices that were built
> before the class code field was defined. Hence, introduce the pci_is_vga()
> helper, let it handle the details for us. It returns true if the PCI(e)
> device being tested belongs to the VGA devices category.
>
> Cc: "Maciej W. Rozycki"
> Signed-off-by: Sui Jingfeng
> ---
> include/linux/pci.h | 27 +++
> 1 file changed, 27 insertions(+)
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index cf6e0b057752..ace727001911 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -713,6 +713,33 @@ static inline bool pci_is_bridge(struct pci_dev *dev)
> dev->hdr_type == PCI_HEADER_TYPE_CARDBUS;
> }
>
> +/**
> + * The PCI code and ID assignment specification defined four types of
> + * display controllers for the display base class(03h), and the devices
> + * with 0x00h sub-class code are VGA devices. VGA devices with programming
> + * interface 0x00 is VGA-compatible, VGA devices with programming interface
> + * 0x01 are 8514-compatible controllers. Besides, PCI_CLASS_NOT_DEFINED_VGA
> + * is defined to provide backward compatibility for devices that were built
> + * before the class code field was defined. This means that it belong to the
> + * VGA devices category also.
> + *
> + * Returns:
> + * true if the PCI device is a VGA device, false otherwise.
> + */
> +static inline bool pci_is_vga(struct pci_dev *pdev)
> +{
> + if (!pdev)
> + return false;
> +
> + if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> + return true;
> +
> + if ((pdev->class >> 8) == PCI_CLASS_NOT_DEFINED_VGA)
> + return true;
Are you seeing a problem that will be fixed by this series, i.e., a
PCI_CLASS_NOT_DEFINED_VGA device that we currently don't handle
correctly?
I think this makes sense per the spec, but there's always a risk of
breaking something, so it's nice if the change actually *fixes*
something to make that risk worthwhile.
> + return false;
> +}
> +
> #define for_each_pci_bridge(dev, bus)\
> list_for_each_entry(dev, >devices, bus_list) \
> if (!pci_is_bridge(dev)) {} else
> --
> 2.34.1
>
Re: [-next 4/5] drm/virgpu: Switch to pci_is_vga()
On Thu, Oct 05, 2023 at 04:57:14PM -0500, Bjorn Helgaas wrote:
> In subject: "drm/virtio" to match previous history.
>
> On Wed, Aug 30, 2023 at 07:15:31PM +0800, Sui Jingfeng wrote:
> > From: Sui Jingfeng
> >
> > Should be no functional change, just for cleanup purpose.
> >
> > Cc: David Airlie
> > Cc: Gerd Hoffmann
> > Cc: Gurchetan Singh
> > Cc: Chia-I Wu
> > Cc: Daniel Vetter
> > Signed-off-by: Sui Jingfeng
> > ---
> > drivers/gpu/drm/virtio/virtgpu_drv.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
> > b/drivers/gpu/drm/virtio/virtgpu_drv.c
> > index add075681e18..3a368304475a 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> > @@ -51,7 +51,7 @@ static int virtio_gpu_pci_quirk(struct drm_device *dev)
> > {
> > struct pci_dev *pdev = to_pci_dev(dev->dev);
> > const char *pname = dev_name(>dev);
> > - bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
> > + bool vga = pci_is_vga(pdev);
>
> This *is* a functional change: Previously "vga" was only true for
> PCI_CLASS_DISPLAY_VGA (0x0300). Now it will be true for both
> PCI_CLASS_DISPLAY_VGA (0x0300) and PCI_CLASS_DISPLAY_OTHER (0x0380).
Oops, sorry, my mistake here. I meant PCI_CLASS_NOT_DEFINED_VGA, not
PCI_CLASS_DISPLAY_OTHER. pci_is_vga() is true for either of:
PCI_CLASS_DISPLAY_VGA 0x0300
PCI_CLASS_NOT_DEFINED_VGA 0x0001
(PCI_CLASS_NOT_DEFINED_VGA is defined in the PCI Code and Assignment
spec r1.15, sec 1.1; PCI_CLASS_DISPLAY_VGA is sec 1.4.)
> Is that desirable? I can't tell. Maybe the GPU folks will chime in.
>
> > int ret;
> >
> > DRM_INFO("pci: %s detected at %s\n",
> > --
> > 2.34.1
> >
Re: [-next 4/5] drm/virgpu: Switch to pci_is_vga()
In subject: "drm/virtio" to match previous history.
On Wed, Aug 30, 2023 at 07:15:31PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> Should be no functional change, just for cleanup purpose.
>
> Cc: David Airlie
> Cc: Gerd Hoffmann
> Cc: Gurchetan Singh
> Cc: Chia-I Wu
> Cc: Daniel Vetter
> Signed-off-by: Sui Jingfeng
> ---
> drivers/gpu/drm/virtio/virtgpu_drv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
> b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index add075681e18..3a368304475a 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -51,7 +51,7 @@ static int virtio_gpu_pci_quirk(struct drm_device *dev)
> {
> struct pci_dev *pdev = to_pci_dev(dev->dev);
> const char *pname = dev_name(>dev);
> - bool vga = (pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA;
> + bool vga = pci_is_vga(pdev);
This *is* a functional change: Previously "vga" was only true for
PCI_CLASS_DISPLAY_VGA (0x0300). Now it will be true for both
PCI_CLASS_DISPLAY_VGA (0x0300) and PCI_CLASS_DISPLAY_OTHER (0x0380).
Is that desirable? I can't tell. Maybe the GPU folks will chime in.
> int ret;
>
> DRM_INFO("pci: %s detected at %s\n",
> --
> 2.34.1
>
Re: [PATCH 0/5] Add the pci_get_base_class() helper and use it
On Fri, Aug 25, 2023 at 02:27:09PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> There is no function that can be used to get all PCI(e) devices in a
> system by matching against its the PCI base class code only, while keep
> the sub-class code and the programming interface ignored. Therefore, add
> the pci_get_base_class() function to suit the need.
>
> For example, if an application want to process all PCI(e) display devices
> in a system, it can achieve such goal by writing the code as following:
>
> pdev = NULL;
> do {
> pdev = pci_get_base_class(PCI_BASE_CLASS_DISPLAY, pdev);
> if (!pdev)
> break;
>
> do_something_for_pci_display_device(pdev);
> } while (1);
>
> Sui Jingfeng (5):
> PCI: Add the pci_get_base_class() helper
> ALSA: hda/intel: Use pci_get_base_class() to reduce duplicated code
> drm/nouveau: Use pci_get_base_class() to reduce duplicated code
> drm/amdgpu: Use pci_get_base_class() to reduce duplicated code
> drm/radeon: Use pci_get_base_class() to reduce duplicated code
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 11 +++--
> drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c | 20 ---
> drivers/gpu/drm/nouveau/nouveau_acpi.c | 11 +++--
> drivers/gpu/drm/radeon/radeon_bios.c | 20 ---
> drivers/pci/search.c | 31
> include/linux/pci.h | 5
> sound/pci/hda/hda_intel.c| 16
> 7 files changed, 59 insertions(+), 55 deletions(-)
Applied to pci/enumeration for v6.7, thanks.
Re: [PATCH v2 00/11] Fix typos, comments and copyright
On Wed, Aug 09, 2023 at 06:34:01AM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> v1:
> * Various improve.
> v2:
> * More fixes, optimizations and improvements.
>
> Sui Jingfeng (11):
> PCI/VGA: Use unsigned type for the io_state variable
> PCI: Add the pci_get_class_masked() helper
> PCI/VGA: Deal with VGA class devices
I dropped these two patches, at least for now. There's no other use
of pci_get_class_masked(), and the VGA use seems to be mostly
optimization and possibly some behavior change that isn't 100% clear
yet. If it's important, we can look at it again later.
> PCI/VGA: Drop the inline in the vga_update_device_decodes() function.
> PCI/VGA: Move the new_state assignment out of the loop
> PCI/VGA: Fix two typos in the comments of pci_notify()
> PCI/VGA: vga_client_register() return -ENODEV on failure, not -1
> PCI/VGA: Fix a typo to the comment of vga_default
> PCI/VGA: Fix a typo to the comments in vga_str_to_iostate() function
> PCI/VGA: Tidy up the code and comment format
> PCI/VGA: Replace full MIT license text with SPDX identifier
>
> drivers/pci/search.c | 30 ++
> drivers/pci/vgaarb.c | 233 +
> include/linux/pci.h| 7 ++
> include/linux/vgaarb.h | 27 +
> 4 files changed, 185 insertions(+), 112 deletions(-)
I applied the rest of the patches on pci/vga for v6.5.
I updated the commit logs and tweaked some of the patches.
I squashed all the typo fixes together and added several more that I
had done previously but not posted. The diff between your original v2
posting and the current pci/vga branch is attached. Most of it is
additional typo fixes, but if you look closely you'll see:
- The omission of the pci_get_class_masked() patches (in
vga_arbiter_add_pci_device(), pci_notify(), vga_arb_device_init())
- The tweaks I did to:
vga_update_device_decodes()
vga_client_register()
vga_arbiter_notify_clients()
I dropped the Reviewed-bys from the typo fixes because I didn't want
to extend them to everything that got squashed together. Happy to add
them back if anybody wants to look again.
The branch is at
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=vga
Bjorn
diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index f1c15aea868b..b4c138a6ec02 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -334,36 +334,6 @@ struct pci_dev *pci_get_device(unsigned int vendor,
unsigned int device,
}
EXPORT_SYMBOL(pci_get_device);
-/**
- * pci_get_class_masked - begin or continue searching for a PCI device by
class and mask
- * @class: search for a PCI device with this class designation
- * @from: Previous PCI device found in search, or %NULL for new search.
- *
- * Iterates through the list of known PCI devices. If a PCI device is
- * found with a matching @class, the reference count to the device is
- * incremented and a pointer to its device structure is returned.
- * Otherwise, %NULL is returned.
- * A new search is initiated by passing %NULL as the @from argument.
- * Otherwise if @from is not %NULL, searches continue from next device
- * on the global list. The reference count for @from is always decremented
- * if it is not %NULL.
- */
-struct pci_dev *pci_get_class_masked(unsigned int class, unsigned int mask,
-struct pci_dev *from)
-{
- struct pci_device_id id = {
- .vendor = PCI_ANY_ID,
- .device = PCI_ANY_ID,
- .subvendor = PCI_ANY_ID,
- .subdevice = PCI_ANY_ID,
- .class_mask = mask,
- .class = class,
- };
-
- return pci_get_dev_by_id(, from);
-}
-EXPORT_SYMBOL(pci_get_class_masked);
-
/**
* pci_get_class - begin or continue searching for a PCI device by class
* @class: search for a PCI device with this class designation
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index a2f6e0e6b634..5e6b1eb54c64 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: MIT
/*
- * vgaarb.c: Implements the VGA arbitration. For details refer to
+ * vgaarb.c: Implements VGA arbitration. For details refer to
* Documentation/gpu/vgaarbiter.rst
*
* (C) Copyright 2005 Benjamin Herrenschmidt
@@ -42,9 +42,9 @@ static void vga_arbiter_notify_clients(void);
struct vga_device {
struct list_head list;
struct pci_dev *pdev;
- unsigned int decodes; /* what does it decodes */
- unsigned int owns; /* what does it owns */
- unsigned int locks; /* what does it locks */
+ unsigned int decodes; /* what it decodes */
+ unsigned int owns; /* what it owns */
+ unsigned int locks; /* what it locks */
unsigned int io_lock_cnt; /* legacy IO lock count */
unsigned int mem_lock_cnt; /* legacy MEM lock count */
Re: [PATCH v5 06/11] drm/radeon: Use RMW accessors for changing LNKCTL
On Fri, Aug 18, 2023 at 04:12:57PM +, Deucher, Alexander wrote:
> > -Original Message-
> > From: Ilpo Järvinen
> > Sent: Monday, July 17, 2023 8:05 AM
> > To: linux-...@vger.kernel.org; Bjorn Helgaas ; Lorenzo
> > Pieralisi ; Rob Herring ;
> > Krzysztof Wilczyński ; Emmanuel Grumbach
> > ; Rafael J . Wysocki ;
> > Heiner Kallweit ; Lukas Wunner ;
> > Andy Shevchenko ; Deucher, Alexander
> > ; Koenig, Christian
> > ; Pan, Xinhui ; David
> > Airlie ; Daniel Vetter ; amd-
> > g...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-
> > ker...@vger.kernel.org
> > Cc: Dean Luick ; Jonas Dreßler
> > ; Ilpo Järvinen ;
> > sta...@vger.kernel.org
> > Subject: [PATCH v5 06/11] drm/radeon: Use RMW accessors for changing
> > LNKCTL
> >
> > Don't assume that only the driver would be accessing LNKCTL. ASPM policy
> > changes can trigger write to LNKCTL outside of driver's control.
> > And in the case of upstream bridge, the driver does not even own the device
> > it's changing the registers for.
> >
> > Use RMW capability accessors which do proper locking to avoid losing
> > concurrent updates to the register value.
> >
> > Fixes: 8a7cd27679d0 ("drm/radeon/cik: add support for pcie gen1/2/3
> > switching")
> > Fixes: b9d305dfb66c ("drm/radeon: implement pcie gen2/3 support for SI")
> > Suggested-by: Lukas Wunner
> > Signed-off-by: Ilpo Järvinen
> > Cc: sta...@vger.kernel.org
>
> For this and the amdgpu patch:
> Acked-by: Alex Deucher
> I'm not sure if this is stable material however. Is there some issue today?
Added your ack, thanks! I dropped the stable tag on the whole series.
Bjorn
Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware_default() less arch-dependent
On Fri, Aug 18, 2023 at 12:09:29PM +0800, suijingfeng wrote:
> On 2023/8/18 06:08, Bjorn Helgaas wrote:
> > On Wed, Aug 16, 2023 at 06:05:27AM +0800, Sui Jingfeng wrote:
> > > Currently, the vga_is_firmware_default() function only works on x86 and
> > > ia64, it is a no-op on ARM, ARM64, PPC, RISC-V, etc. This patch completes
> > > the implementation for the rest of the architectures. The added code tries
> > > to identify the PCI(e) VGA device that owns the firmware framebuffer
> > > before PCI resource reallocation happens.
> >
> > As far as I can tell, this is basically identical to the existing
> > vga_is_firmware_default(), except that this patch funs that code as a
> > header fixup, so it happens before any PCI BAR reallocations happen.
>
> Yes, what you said is right in overall.
> But I think I should mention a few tiny points that make a difference.
>
> 1) My version is *less arch-dependent*
Of course. If we make the patch simple and the commit log simple by
removing extraneous details, this will all be obvious.
> 2) My version focus on the address in ranges, weaken the size parameter.
>
> Which make the code easy to read and follow the canonical convention to
> express the address range. while the vga_is_firmware_default() is not.
Whether it's start/size or start/end is a trivial question. We don't
need to waste time on it now.
> 3) A tiny change make a big difference.
>
> The original vga_is_firmware_default() only works with the assumption
> that the PCI resource reallocation won't happens. While I see no clue
> that why this is true even on X86 and IA64. The original patch[1] not
> mention this assumption explicitly.
> [1] 86fd887b7fe3 ('vgaarb: Don't default exclusively to first video device
> with mem+io')
>
> > That sounds like a good idea, because this is all based on the
> > framebuffer in screen_info, and screen_info was initialized before PCI
> > enumeration, and it certainly doesn't account for any BAR changes done
> > by the PCI core.
>
> Yes.
>
> > So why would we keep vga_is_firmware_default() at all? If the header
> > fixup has already identified the firmware framebuffer, it seems
> > pointless to look again later.
>
> It need another patch to do the cleanup work, while my patch just
> add code to solve the real problem. It focus on provide a solution
> for the architectures which have a decent way set up the
> screen_info. Other things except that is secondary.
I don't want both mechanisms when only one of them is useful. PCI BAR
reassignment is completely fine, and keeping the assumption in
vga_is_firmware_default() that we can compare reassigned BAR values to
the pre-reassignment screen_info range is a trap that we should
remove.
Bjorn
Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware_default() less arch-dependent
On Fri, Aug 18, 2023 at 09:48:46AM +0800, suijingfeng wrote:
> On 2023/8/18 06:08, Bjorn Helgaas wrote:
> > > + if (resource_type(res) != IORESOURCE_MEM)
> > > + continue;
> > > +
> > > + if (!res->start || !res->end)
> > > + continue;
> > > +
> > > + if (res->start <= fb_start && fb_end <= res->end) {
> > > + pdev_boot_vga = pdev;
> > > +
> > > + vgaarb_info(>dev,
> > > + "BAR %d contains firmware FB\n", i);
> > Print the BAR with %pR and include the framebuffer region from
> > screen_info in the same format.
>
> I do remember that you already told me to do this in V3, sorry for not
> replying to you at V3. Most of the time, what you tell me is right.But here,
> I think I need to explain. Because doing it that way will make the code line
> too long,and it will exceed 80 characters in the column if we print too
> much.
> I believe that the vgaarb_info() at here is already the most compact and
> simplest form. Printing the BAR with %pR is not absolute necessary, because
> we can get the additional information by: $ lspci | grep VGA
>
> $ dmesg | grep 05:00.0
> $ dmesg | grep :03:00.0
> $ dmesg | grep PCI
Fair enough. The BAR info is already there. But I don't think the
screen_info framebuffer data is in the dmesg log anywhere, and I think
that would be useful.
It's fine if dmesg lines or kernel printk lines exceed 80 columns.
> Actually, I only add something that is absolute necessary.
> Printing BAR with %pR and/or Printing the framebuffer region
> is consider to only for *debugging* purpose.
I think printing the framebuffer region is important because it makes
it clear *why* we're selecting the device as the default VGA device.
It's more than just debugging; it helps make the system more
transparent and more understandable.
Bjorn
Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware_default() less arch-dependent
On Wed, Aug 16, 2023 at 06:05:27AM +0800, Sui Jingfeng wrote: > Currently, the vga_is_firmware_default() function only works on x86 and > ia64, it is a no-op on ARM, ARM64, PPC, RISC-V, etc. This patch completes > the implementation for the rest of the architectures. The added code tries > to identify the PCI(e) VGA device that owns the firmware framebuffer > before PCI resource reallocation happens. As far as I can tell, this is basically identical to the existing vga_is_firmware_default(), except that this patch funs that code as a header fixup, so it happens before any PCI BAR reallocations happen. That sounds like a good idea, because this is all based on the framebuffer in screen_info, and screen_info was initialized before PCI enumeration, and it certainly doesn't account for any BAR changes done by the PCI core. So why would we keep vga_is_firmware_default() at all? If the header fixup has already identified the firmware framebuffer, it seems pointless to look again later. > This patch makes the vga_is_firmware_default() function works on whatever > arch that has UEFI GOP support. But we make it available only on platforms > where PCI resource relocation happens. if the provided method proves to be > effective and reliable, it can be expanded to other arch easily. > > v2: > * Fix test robot warnnings and fix typos > > v3: > * Fix linkage problems if the global screen_info is not exported > > v4: > * Handle linkage problems by hiding behind of CONFIG_SYSFB, > * Drop side-effects and simplify. The v2, v3, v4 changelog is nice, but we don't need it in the commit log itself, where it will become part of the git history. It should go in a cover letter or after the "---" marker: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.0#n678 > Since only one GPU could owns the firmware fb in normal case, things > are almost done once we determine the boot VGA selected by firmware. > By using the DECLARE_PCI_FIXUP_CLASS_HEADER(), we also ensure that we > could identify the primary display (if there do have one) before PCI > resource reallocation happen. > > The method provided in this patch will be effective as long as the target > platform has a way set up the kernel's screen_info. For the machines with > UEFI firmware, the screen_info is typically set up by the UEFI stub with > respect to the UEFI GOP protocol. Since the UEFI stub executes in the > context of the decompressor, and it cannot access the kernel's screen_info > directly. Hence, the efi stub copies the screen_info provided by firmware > to kernel's counterpart. Therefore, we handle linkage problems by using > the CONFIG_EFI guard. Since when CONFIG_EFI is set, the kernel's > screen_info is guaranteed to be static linkable for arm, arm64, riscv. > V4 of this patch handle linkage problems by hiding behind of CONFIG_SYSFB, > since sysfb reference the global screen_info as well. > > This patch is tested on: > > 1) LS3A5000+LS7A1000 platform with three video cards > > $ lspci | grep VGA > > 00:06.1 VGA compatible controller: Loongson Technology LLC DC (Display > Controller) (rev 01) > 03:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] > Caicos XT [Radeon HD 7470/8470 / R5 235/310 OEM] > 07:00.0 VGA compatible controller: S3 Graphics Ltd. Device 9070 (rev 01) > 08:00.0 VGA compatible controller: S3 Graphics Ltd. Device 9070 (rev 01) > > Before apply this patch: > > [0.361748] pci :00:06.1: vgaarb: setting as boot VGA device > [0.361751] pci :00:06.1: vgaarb: bridge control possible > [0.361753] pci :00:06.1: vgaarb: VGA device added: > decodes=io+mem,owns=io+mem,locks=none > [0.361763] pci :03:00.0: vgaarb: bridge control possible > [0.361765] pci :03:00.0: vgaarb: VGA device added: > decodes=io+mem,owns=none,locks=none > [0.361771] pci :07:00.0: vgaarb: bridge control possible > [0.361773] pci :07:00.0: vgaarb: VGA device added: > decodes=io+mem,owns=none,locks=none > [0.361777] pci :08:00.0: vgaarb: bridge control possible > [0.361779] pci :08:00.0: vgaarb: VGA device added: > decodes=io+mem,owns=none,locks=none > [0.361781] vgaarb: loaded > [0.367838] pci :00:06.1: Overriding boot device as 1002:6778 > [0.367841] pci :00:06.1: Overriding boot device as 5333:9070 > [0.367843] pci :00:06.1: Overriding boot device as 5333:9070 > > After apply this patch: > > [0.357780] pci :03:00.0: vgaarb: BAR 0 contains firmware FB > [0.361726] pci :00:06.1: vgaarb: setting as boot VGA device > [0.361729] pci :00:06.1: vgaarb: bridge control possible > [0.361731] pci :00:06.1: vgaarb: VGA device added: > decodes=io+mem,owns=io+mem,locks=none > [0.361741] pci :03:00.0: vgaarb: setting as boot VGA device > (overriding previous) > [0.361743] pci :03:00.0: vgaarb: bridge control
Re: [PATCH] PCI/VGA: Make the vga_is_firmware_default() less arch-independent
On Tue, Aug 08, 2023 at 10:58:59PM +0800, Sui Jingfeng wrote:
> Currently, the vga_is_firmware_default() function works only on x86 and
> IA64 architectures, but it is a no-op on ARM64, PPC, RISC-V, etc. This
> patch completes the implementation by tracking the firmware framebuffer's
> address range. The added code is trying to identify the VRAM aperture that
> contains the firmware framebuffer. Once found, related information about
> the VRAM aperture will be tracked.
>
> Note that we need to identify the VRAM aperture before it get moved. We
> achieve this by using DECLARE_PCI_FIXUP_CLASS_HEADER(), which ensures that
> vga_arb_firmware_fb_addr_tracker() gets called before PCI resource
> allocation. Once we found the VRAM aperture that contains firmware fb, we
> are able to monitor the address changes of it. If the VRAM aperture of the
> primary GPU do moved, we will update our cached firmware framebuffer's
> address range accordingly. This approach overcomes the VRAM bar relocation
> issue successfully. Hence, this patch make the vga_is_firmware_default()
> function works on whatever arch that has UEFI GOP support, including x86
> and IA64. But, at the first step, we make it available only on platforms
> which PCI resource relocation do happens. Once provided method proved to
> be effective and reliable, it can be expanded to other arch easily.
I think this patch tries to solve two problems, and it should be split
into two patches:
1) Identify firmware framebuffer on arches other than x86 and ia64
2) Deal with VGA devices where the PCI core has moved the BAR
containing the framebuffer
For x86 and ia64, vga_is_firmware_default() currently gets the
framebuffer base and size from screen_info. Whenever
vga_arbiter_add_pci_device() adds a VGA device, we check to see if it
has a BAR containing the framebuffer.
It looks like this patch retains that for x86 and ia64, but only if
CONFIG_EFI=y. I think CONFIG_EFI is optional for both x86 and ia64,
so it looks like this will break systems where CONFIG_X86=y and
CONFIG_EFI is not set.
> This patch is tested on
> 1) LS3A5000+LS7A2000 and LS3A5000+LS7A1000 platform.
> 2) Intel i3-8100 CPU + H110 D4L motherboard with triple video cards:
>
> $ lspci | grep VGA
>
> Intel Corporation CoffeeLake-S GT2 [UHD Graphics 630]
> Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere [Radeon RX 470] (rev cf)
> ASPEED Technology, Inc. ASPEED Graphics Family (rev 52)
>
> Note that on x86, in order to testing the new approach this patch provided,
> we remove the vga_arb_get_fb_range_from_screen_info() call in
> vga_is_firmware_default() function, as following.
>
> -#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> - ret = vga_arb_get_fb_range_from_screen_info(_start, _end);
> -#else
> ret = vga_arb_get_fb_range_from_tracker(_start, _end);
> -#endif
>
> It is just that we don't observe the case which VRAM Bar of VGA compatible
> controller moves, so there just no need to unify it. But on LoongArch,
> the VRAM Bar of AMDGPU do moves.
>
> v2:
> * Fix test robot warnnings and fix typos
>
> v3:
> * Fix linkage problems if the global screen_info is not exported
This doesn't build on x86:
$ git checkout -b wip/sui-vga-default-arch-independent v6.5-rc1
$ b4 am 20230808145859.1590625-1-suijingf...@loongson.cn
$ git am
./20230808_suijingfeng_pci_vga_make_the_vga_is_firmware_default_less_arch_independent.mbx
$ make drivers/pci/vgaarb.o
CC drivers/pci/vgaarb.o
drivers/pci/vgaarb.c:114:13: error: ‘vga_arb_get_fb_range_from_tracker’
defined but not used [-Werror=unused-function]
114 | static bool vga_arb_get_fb_range_from_tracker(resource_size_t *start,
| ^
> Signed-off-by: Sui Jingfeng
> ---
> drivers/pci/vgaarb.c | 154 ++-
> 1 file changed, 139 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 5a696078b382..e0919a70af3e 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -61,6 +61,92 @@ static bool vga_arbiter_used;
> static DEFINE_SPINLOCK(vga_lock);
> static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
>
> +static struct firmware_fb_tracker {
> + /* The PCI(e) device who owns the firmware framebuffer */
> + struct pci_dev *pdev;
> + /* The index of the VRAM Bar */
> + unsigned int bar;
> + /* Firmware fb's offset from the VRAM aperture start */
> + resource_size_t offset;
> + /* The firmware fb's size, in bytes */
> + resource_size_t size;
> +
> + /* Firmware fb's address range, suffer from change */
> + resource_size_t start;
> + resource_size_t end;
It's redundant to save start, size, and end. Start and end should be
enough, and maybe you could use a struct resource for that.
It's not clear to me why you need to save the start/end/etc anyway.
All we need to know is which pci_dev is the firmware device. Doesn't
Re: [PATCH] PCI/VGA: Make the vga_is_firmware_default() arch-independent
On Fri, Aug 04, 2023 at 11:11:12AM +0800, suijingfeng wrote: > On 2023/8/3 20:25, kernel test robot wrote: > > Hi Sui, > > > > kernel test robot noticed the following build errors: > > > > [auto build test ERROR on pci/next] > > [also build test ERROR on pci/for-linus linus/master v6.5-rc4 next-20230803] > > [If your patch is applied to the wrong git tree, kindly drop us a note. > > And when submitting patch, we suggest to use '--base' as documented in > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > > > url: > > https://github.com/intel-lab-lkp/linux/commits/Sui-Jingfeng/PCI-VGA-Make-the-vga_is_firmware_default-arch-independent/20230803-161838 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next > > patch link: > > https://lore.kernel.org/r/20230803081758.968742-1-suijingfeng%40loongson.cn > > patch subject: [PATCH] PCI/VGA: Make the vga_is_firmware_default() > > arch-independent > > config: arm64-randconfig-r026-20230731 > > (https://download.01.org/0day-ci/archive/20230803/202308032022.yizngbbk-...@intel.com/config) > > compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git > > 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a) > > reproduce: > > (https://download.01.org/0day-ci/archive/20230803/202308032022.yizngbbk-...@intel.com/reproduce) > > > > 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/202308032022.yizngbbk-...@intel.com/ > > > > All errors (new ones prefixed by >>): > > > > > > ld.lld: error: undefined symbol: screen_info > > >>> referenced by vgaarb.c:86 (drivers/pci/vgaarb.c:86) > > >>> > > drivers/pci/vgaarb.o:(vga_arb_firmware_fb_addr_tracker) in archive vmlinux.a > > >>> referenced by vgaarb.c:86 (drivers/pci/vgaarb.c:86) > > >>> > > drivers/pci/vgaarb.o:(vga_arb_firmware_fb_addr_tracker) in archive vmlinux.a > > >>> referenced by vgaarb.c:88 (drivers/pci/vgaarb.c:88) > > >>> > > drivers/pci/vgaarb.o:(vga_arb_firmware_fb_addr_tracker) in archive vmlinux.a > > >>> referenced 3 more times > > > This is a more like arch-specific problem, It will be pain at many places on > platforms > that do not export the screen_info symbol. Not only here. > > I have already explained that screen_info is arch-dependent many times, but > no one cares about me. > By using (looking at) screen_info, vgaarb gets infected, and becomes > arch-dependent as well. > vgaarb deals with VGA class (pdev->class == 0x0300XX) devices only, This > makes it device-dependent. > Hence, It only works correctly for a small set of PCIe devices on x86. This build error report is from an automated service; there's nothing personal about it and the automated service isn't going to respond to you. The build issue is just something that will have to be resolved before we can consider merging the patch. Any explanation needs to go in the commit logs for the relevant patches. > arch-dependent, device-dependent, subsystem-dependent (part of it rely on > ACPI) and > loading order dependent, those dependent itself are the problems. > It results in various undefined (uncertain) behaviors on non-x86 > architectures. > > Even on x86, some platform choose to relay on the firmware to solve the > multiple GPU coexist problem. > so it is also firmware-dependent. > > This patch solves part of the above problems listed, target at the *device > level*, as early as possible. > while they still a few problems could be only solved at the *driver level*. > For an example, The display controller in Intel N2000 and d2000 series don't > has a dedicated VRAM bar. > they use the "stolen memory", which is carve out by somebody (either bios or > kernel?). > >
Re: [PATCH 4/6] PCI/VGA: Move the new_state assignment out the loop
On Mon, Jul 24, 2023 at 09:02:14PM +0800, suijingfeng wrote:
> PING, please !
Don't worry, these are not forgotten. Your other series seems more
important, so that's what I've been paying attention to.
> On 2023/7/11 21:43, Sui Jingfeng wrote:
> > From: Sui Jingfeng
> >
> > In the vga_arbiter_notify_clients() function, the value of the 'new_state'
> > variable will be 'false' on systems that have more than one PCI VGA card.
> > Its value will be 'true' on systems that have one or no PCI VGA compatible
> > card. In other words, its value is not relevant to the iteration, so move
> > the assignment () out of the loop.
> >
> > For a system with multiple video cards, this patch saves the redundant
> > assignment.
> >
> > Signed-off-by: Sui Jingfeng
> > ---
> > drivers/pci/vgaarb.c | 16 +++-
> > 1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> > index 668139f7c247..4c448c758bab 100644
> > --- a/drivers/pci/vgaarb.c
> > +++ b/drivers/pci/vgaarb.c
> > @@ -1468,22 +1468,20 @@ static void vga_arbiter_notify_clients(void)
> > {
> > struct vga_device *vgadev;
> > unsigned long flags;
> > - uint32_t new_decodes;
> > - bool new_state;
> > + bool state;
> > if (!vga_arbiter_used)
> > return;
> > + state = (vga_count > 1) ? false : true;
> > +
> > spin_lock_irqsave(_lock, flags);
> > list_for_each_entry(vgadev, _list, list) {
> > - if (vga_count > 1)
> > - new_state = false;
> > - else
> > - new_state = true;
> > if (vgadev->set_decode) {
> > - new_decodes = vgadev->set_decode(vgadev->pdev,
> > -new_state);
> > - vga_update_device_decodes(vgadev, new_decodes);
> > + unsigned int decodes;
> > +
> > + decodes = vgadev->set_decode(vgadev->pdev, state);
> > + vga_update_device_decodes(vgadev, decodes);
> > }
> > }
> > spin_unlock_irqrestore(_lock, flags);
>
Re: [PATCH 2/6] PCI/VGA: Deal with PCI VGA compatible devices only
On Sat, Jul 22, 2023 at 04:11:07PM +0800, suijingfeng wrote: > ... > In the future, we may want to expand VGAARB to deal all PCI display class > devices, with another patch. > > if (pdev->class >> 16 == PCI_BASE_CLASS_DISPLAY) > > // accept > > else > > // return immediately. > > > Then, we will have a more good chance to clarify the programmer interface. I would prefer not to expand vgaarb to deal with all display devices unless they actually need the legacy resources like [pci 0xa-0xb]. But maybe the consumer of these interfaces relies on vgaarb even for devices that don't need those resources? If so, please mention examples of where they depend on vgaarb. I expect the vgaarb interfaces are used by things that need to emulate the option ROM to initialize the device. If the device has a programming interface other than b, the option ROM should not be using the [pci 0xa-0xb] resource, so vgaarb should not be needed. Bjorn
Re: [PATCH v3 0/9] PCI/VGA: Improve the default VGA device selection
On Mon, Jul 24, 2023 at 08:47:48PM +0800, suijingfeng wrote: > On 2023/7/20 03:32, Bjorn Helgaas wrote: > > "drm/loongson: Add an implement for ..." also solves a problem, but it > > lacks a commit log, so I don't know what the problem is. > > I have already telling you one yeas ago. The patch itself must be self-contained, including a commit log that justifies the change. You may have told me a year ago, but that doesn't help somebody looking at these changes next year. Bjorn
Re: [PATCH v3 4/9] PCI/VGA: Improve the default VGA device selection
On Mon, Jul 24, 2023 at 08:16:18PM +0800, suijingfeng wrote: > On 2023/7/20 03:32, Bjorn Helgaas wrote: > > > 2) It does not take the PCI Bar may get relocated into consideration. > > > 3) It is not effective for the PCI device without a dedicated VRAM Bar. > > > 4) It is device-agnostic, thus it has to waste the effort to iterate all > > > of the PCI Bar to find the VRAM aperture. > > > 5) It has invented lots of methods to determine which one is the default > > > boot device, but this is still a policy because it doesn't give the > > > user a choice to override. > > I don't think we need a list of*potential* problems. We need an > > example of the specific problem this will solve, i.e., what currently > > does not work? > > > This version do allow the arbitration service works on non-x86 arch, > which also allow me remove a arch-specific workaround. > I will give more detail at the next version. Yes. This part I think we want. > But I want to provide one more drawback of vgaarb here: > > (6) It does not works for non VGA-compatible PCI(e) display controllers. > > Because, currently, vgaarb deal with PCI VGA compatible devices only. > > See another my patch set [1] for more elaborate discussion. > > It also ignore PCI_CLASS_NOT_DEFINED_VGA as Maciej puts it[2]. > > While my approach do not required the display controller to be > VGA-compatible to enjoy the arbitration service. I think vgaarb is really only for dealing with the problem of the legacy VGA address space routing. For example, there may be VGA devices that require the [pci 0xa-0xb] range but they don't describe that via a BAR. There may also be VGA option ROMs that depend on that range so they can initialize the device. The [pci 0xa-0xb] range can only be routed to one device at a time, and vgaarb is what takes care of that by manipulating the VGA Enable bits in bridges. I don't think we should extend vgaarb to deal with non-VGA GPUs in general, i.e., I don't think it should be concerned with devices and option ROMs that do not require the [pci 0xa-0xb] range. I think a strict reading of the PCI Class Code spec would be that only devices with Programming Interface b can depend on that legacy range. If that's what vgaarb currently enforces, great. If it currently deals with more than just b devices, and there's some value in restricting it to only b, we could try that, but I would suggest doing that in a tiny patch all by itself. Then if we trip over a problem, it's easy to bisect and revert it. > [1] https://patchwork.freedesktop.org/patch/546690/?series=120548=1 > > [2] https://lkml.org/lkml/2023/6/18/315 >
Re: [PATCH v5 05/11] drm/amdgpu: Use RMW accessors for changing LNKCTL
On Mon, Jul 17, 2023 at 03:04:57PM +0300, Ilpo Järvinen wrote:
> Don't assume that only the driver would be accessing LNKCTL. ASPM
> policy changes can trigger write to LNKCTL outside of driver's control.
> And in the case of upstream bridge, the driver does not even own the
> device it's changing the registers for.
>
> Use RMW capability accessors which do proper locking to avoid losing
> concurrent updates to the register value.
>
> Fixes: a2e73f56fa62 ("drm/amdgpu: Add support for CIK parts")
> Fixes: 62a37553414a ("drm/amdgpu: add si implementation v10")
> Suggested-by: Lukas Wunner
> Signed-off-by: Ilpo Järvinen
> Cc: sta...@vger.kernel.org
Do we have any reports of problems that are fixed by this patch (or by
others in the series)? If not, I'm not sure it really fits the usual
stable kernel criteria:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?id=v6.4
> ---
> drivers/gpu/drm/amd/amdgpu/cik.c | 36 +---
> drivers/gpu/drm/amd/amdgpu/si.c | 36 +---
> 2 files changed, 20 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c
> b/drivers/gpu/drm/amd/amdgpu/cik.c
> index 5641cf05d856..e63abdf52b6c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
> @@ -1574,17 +1574,8 @@ static void cik_pcie_gen3_enable(struct amdgpu_device
> *adev)
> u16 bridge_cfg2, gpu_cfg2;
> u32 max_lw, current_lw, tmp;
>
> - pcie_capability_read_word(root, PCI_EXP_LNKCTL,
> - _cfg);
> - pcie_capability_read_word(adev->pdev, PCI_EXP_LNKCTL,
> - _cfg);
> -
> - tmp16 = bridge_cfg | PCI_EXP_LNKCTL_HAWD;
> - pcie_capability_write_word(root, PCI_EXP_LNKCTL, tmp16);
> -
> - tmp16 = gpu_cfg | PCI_EXP_LNKCTL_HAWD;
> - pcie_capability_write_word(adev->pdev, PCI_EXP_LNKCTL,
> -tmp16);
> + pcie_capability_set_word(root, PCI_EXP_LNKCTL,
> PCI_EXP_LNKCTL_HAWD);
> + pcie_capability_set_word(adev->pdev, PCI_EXP_LNKCTL,
> PCI_EXP_LNKCTL_HAWD);
>
> tmp = RREG32_PCIE(ixPCIE_LC_STATUS1);
> max_lw = (tmp &
> PCIE_LC_STATUS1__LC_DETECTED_LINK_WIDTH_MASK) >>
> @@ -1637,21 +1628,14 @@ static void cik_pcie_gen3_enable(struct amdgpu_device
> *adev)
> msleep(100);
>
> /* linkctl */
> - pcie_capability_read_word(root, PCI_EXP_LNKCTL,
> - );
> - tmp16 &= ~PCI_EXP_LNKCTL_HAWD;
> - tmp16 |= (bridge_cfg & PCI_EXP_LNKCTL_HAWD);
> - pcie_capability_write_word(root, PCI_EXP_LNKCTL,
> -tmp16);
> -
> - pcie_capability_read_word(adev->pdev,
> - PCI_EXP_LNKCTL,
> - );
> - tmp16 &= ~PCI_EXP_LNKCTL_HAWD;
> - tmp16 |= (gpu_cfg & PCI_EXP_LNKCTL_HAWD);
> - pcie_capability_write_word(adev->pdev,
> -PCI_EXP_LNKCTL,
> -tmp16);
> + pcie_capability_clear_and_set_word(root,
> PCI_EXP_LNKCTL,
> +
> PCI_EXP_LNKCTL_HAWD,
> +bridge_cfg &
> +
> PCI_EXP_LNKCTL_HAWD);
> + pcie_capability_clear_and_set_word(adev->pdev,
> PCI_EXP_LNKCTL,
> +
> PCI_EXP_LNKCTL_HAWD,
> +gpu_cfg &
> +
> PCI_EXP_LNKCTL_HAWD);
Wow, there's a lot of pointless-looking work going on here:
set root PCI_EXP_LNKCTL_HAWD
set GPU PCI_EXP_LNKCTL_HAWD
for (i = 0; i < 10; i++) {
read root PCI_EXP_LNKCTL
read GPU PCI_EXP_LNKCTL
clear root PCI_EXP_LNKCTL_HAWD
if (root PCI_EXP_LNKCTL_HAWD was set)
set root PCI_EXP_LNKCTL_HAWD
clear GPU PCI_EXP_LNKCTL_HAWD
if (GPU PCI_EXP_LNKCTL_HAWD was set)
set GPU PCI_EXP_LNKCTL_HAWD
}
If it really *is* pointless, it would be nice to clean it up, but that
wouldn't be material for this patch, so what you
Re: [PATCH v3 1/9] video/aperture: Add a helper to detect if an aperture contains firmware FB
On Wed, Jul 12, 2023 at 12:43:02AM +0800, Sui Jingfeng wrote: > From: Sui Jingfeng > > This patch adds the aperture_contain_firmware_fb() function to do the > determination. Unfortunately, due to the fact that the apertures list > will be freed dynamically, the location and size information of the > firmware FB will be lost after dedicated drivers call > aperture_remove_conflicting_devices(), > aperture_remove_conflicting_pci_devices() or > aperture_remove_all_conflicting_devices() functions > We solve this problem by introducing two static variables that record the > firmware framebuffer's start addrness and end addrness. It assumes that the > system has only one active firmware framebuffer driver at a time. We don't > use the global structure screen_info here, because PCI resources may get > reallocated (the VRAM BAR could be moved) during the kernel boot stage. s/addrness/address/ (twice)
Re: [Intel-gfx] [PATCH v3 3/9] PCI/VGA: Switch to aperture_contain_firmware_fb_nonreloc()
[+cc linux-pci; I don't apply or ack PCI patches unless they appear there]
On Wed, Jul 12, 2023 at 12:43:04AM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> The observation behind this is that we should avoid accessing the global
> screen_info directly. Call the aperture_contain_firmware_fb_nonreloc()
> function to implement the detection of whether an aperture contains the
> firmware FB.
Because it's better to access the global screen_info from
aperture_contain_firmware_fb_nonreloc()? The reasoning here is not
super clear to me.
> This patch helps to decouple the determination from the implementation.
> Or, in other words, we intend to make the determination opaque to the
> caller. The determination may choose to be arch-dependent or
> arch-independent. But vgaarb, as a consumer of the determination,
> shouldn't care how the does determination is implemented.
"how the determination ..." (drop the "does")
Are you saying that aperture_contain_firmware_fb_nonreloc() might be
arch-dependent? Are there multiple callers? Or does this just move
code from one place to a more appropriate place?
> Signed-off-by: Sui Jingfeng
> ---
> drivers/pci/vgaarb.c | 19 ---
> 1 file changed, 4 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index bf96e085751d..953daf731b2c 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -14,6 +14,7 @@
> #define vgaarb_info(dev, fmt, arg...)dev_info(dev, "vgaarb: " fmt,
> ##arg)
> #define vgaarb_err(dev, fmt, arg...) dev_err(dev, "vgaarb: " fmt, ##arg)
>
> +#include
> #include
> #include
> #include
> @@ -26,7 +27,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -558,20 +558,11 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
> }
> EXPORT_SYMBOL(vga_put);
>
> +/* Select the device owning the boot framebuffer if there is one */
> static bool vga_is_firmware_default(struct pci_dev *pdev)
> {
> #if defined(CONFIG_X86) || defined(CONFIG_IA64)
> - u64 base = screen_info.lfb_base;
> - u64 size = screen_info.lfb_size;
> struct resource *r;
> - u64 limit;
> -
> - /* Select the device owning the boot framebuffer if there is one */
> -
> - if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
> - base |= (u64)screen_info.ext_lfb_base << 32;
> -
> - limit = base + size;
>
> /* Does firmware framebuffer belong to us? */
> pci_dev_for_each_resource(pdev, r) {
> @@ -581,10 +572,8 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
> if (!r->start || !r->end)
> continue;
>
> - if (base < r->start || limit >= r->end)
> - continue;
> -
> - return true;
> + if (aperture_contain_firmware_fb_nonreloc(r->start, r->end))
> + return true;
> }
> #endif
> return false;
> --
> 2.25.1
>
Re: [PATCH v3 0/9] PCI/VGA: Improve the default VGA device selection
[+cc linux-pci] On Wed, Jul 12, 2023 at 12:43:01AM +0800, Sui Jingfeng wrote: > From: Sui Jingfeng > > Currently, the default VGA device selection is not perfect. Potential > problems are: > > 1) This function is a no-op on non-x86 architectures. > 2) It does not take the PCI Bar may get relocated into consideration. > 3) It is not effective for the PCI device without a dedicated VRAM Bar. > 4) It is device-agnostic, thus it has to waste the effort to iterate all >of the PCI Bar to find the VRAM aperture. > 5) It has invented lots of methods to determine which one is the default >boot device on a multiple video card coexistence system. But this is >still a policy because it doesn't give the user a choice to override. > > With the observation that device drivers or video aperture helpers may > have better knowledge about which PCI bar contains the firmware FB, > > This patch tries to solve the above problems by introducing a function > callback to the vga_client_register() function interface. DRM device > drivers for the PCI device need to register the is_boot_device() function > callback during the driver loading time. Once the driver binds the device > successfully, VRAARB will call back to the driver. This gives the device > drivers a chance to provide accurate boot device identification. Which in > turn unlock the abitration service to non-x86 architectures. A device > driver can also pass a NULL pointer to keep the original behavior. I skimmed all these patches, but the only one that seems to mention an actual problem being solved, i.e., something that doesn't work for an end user, is "drm/ast: Register as a vga client ..." Maybe "drm/loongson: Add an implement for ..." also solves a problem, but it lacks a commit log, so I don't know what the problem is. In the future, can you please cc: linux-pci with the entire series? In this posting, only the patches that touched drivers/pci/vgaarb.c went to linux-pci, but those aren't enough to make sense of the series as a whole. > video/aperture: Add a helper to detect if an aperture contains > firmware FB > video/aperture: Add a helper for determining if an unmoved aperture > contain FB > PCI/VGA: Switch to aperture_contain_firmware_fb_nonreloc() Since this subject includes the function name (which is nice!), it would also be helpful if the "Add a helper ..." subject included the same function name. > PCI/VGA: Improve the default VGA device selection If you can make this subject any more specific, that would be useful. There's more to say about that patch, so I'll respond there. > drm/amdgpu: Implement the is_primary_gpu callback of > vga_client_register() > drm/radeon: Add an implement for the is_primary_gpu function callback > drm/i915: Add an implement for the is_primary_gpu hook > drm/ast: Register as a vga client to vgaarb by calling > vga_client_register() > drm/loongson: Add an implement for the is_primary_gpu function > callback There's unnecessary variation in the subject lines (and the commit logs) of these patches. If they all do the same thing but in different drivers, it's useful if the patches all *look* the same. You might be able to write these subjects as "Implement .is_primary_gpu() callback" for brevity. Bjorn
Re: [PATCH v3 4/9] PCI/VGA: Improve the default VGA device selection
[+cc linux-pci (please cc in the future since the bulk of this patch
is in drivers/pci/)]
On Wed, Jul 12, 2023 at 12:43:05AM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> Currently, the strategy of selecting the default boot on a multiple video
> card coexistence system is not perfect. Potential problems are:
>
> 1) This function is a no-op on non-x86 architectures.
Which function in particular is a no-op for non-x86?
> 2) It does not take the PCI Bar may get relocated into consideration.
> 3) It is not effective for the PCI device without a dedicated VRAM Bar.
> 4) It is device-agnostic, thus it has to waste the effort to iterate all
>of the PCI Bar to find the VRAM aperture.
> 5) It has invented lots of methods to determine which one is the default
>boot device, but this is still a policy because it doesn't give the
>user a choice to override.
I don't think we need a list of *potential* problems. We need an
example of the specific problem this will solve, i.e., what currently
does not work?
The drm/ast and maybe drm/loongson patches are the only ones that use
the new callback, so I assume there are real problems with those
drivers.
CONFIG_DRM_AST is a tristate. We're talking about identifying the
boot-time console device. So if CONFIG_DRM_AST=m, I guess we don't
get the benefit of the new callback unless the module gets loaded?
> Also honor the comment: "Clients have *TWO* callback mechanisms they
> can use"
This refers to the existing vga_client_register() function comment:
* vga_client_register - register or unregister a VGA arbitration client
* @pdev: pci device of the VGA client
* @set_decode: vga decode change callback
*
* Clients have two callback mechanisms they can use.
*
* @set_decode callback: If a client can disable its GPU VGA resource, it
* will get a callback from this to set the encode/decode state.
and the fact that struct vga_device currently only contains *one*
callback function pointer:
unsigned int (*set_decode)(struct pci_dev *pdev, bool decode);
Adding the .is_primary_gpu() callback does mean there will now be two
callbacks, as the comment says, but I think it's just confusing to
mention this in the commit log, so I would just remove it.
> @@ -1509,13 +1543,24 @@ static int pci_notify(struct notifier_block *nb,
> unsigned long action,
>* cases of hotplugable vga cards.
>*/
>
> - if (action == BUS_NOTIFY_ADD_DEVICE)
> + switch (action) {
> + case BUS_NOTIFY_ADD_DEVICE:
> notify = vga_arbiter_add_pci_device(pdev);
> - else if (action == BUS_NOTIFY_DEL_DEVICE)
> + if (notify)
> + vga_arbiter_notify_clients();
> + break;
> + case BUS_NOTIFY_DEL_DEVICE:
> notify = vga_arbiter_del_pci_device(pdev);
> + if (notify)
> + vga_arbiter_notify_clients();
> + break;
> + case BUS_NOTIFY_BOUND_DRIVER:
> + vga_arbiter_do_arbitration(pdev);
> + break;
> + default:
> + break;
> + }
Changing from if/else to switch makes the patch bigger than necessary
for no real benefit and obscures what is really changing.
Bjorn
Re: [PATCH 2/6] PCI/VGA: Deal with PCI VGA compatible devices only
On Tue, Jul 11, 2023 at 09:43:50PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> Currently, vgaarb only cares about PCI VGA-compatible class devices.
>
> While vga_arbiter_del_pci_device() gets called unbalanced when some PCI
> device is about to be removed. This happens even during the boot process.
The previous code calls vga_arbiter_add_pci_device() for every device
(every device present at boot and also every hot-added device). It
only allocates a vga_device if pdev->class is 0x0300XX.
It calls vga_arbiter_del_pci_device() for every device removal. It
does nothing unless it finds a vga_device.
This seems symmetric and reasonable to me. Did you observe a problem
with it?
> Another reason is that the vga_arb_device_init() function is not efficient.
> Since we only care about VGA-compatible devices (pdev->class == 0x03),
> We could filter the unqualified devices out in the vga_arb_device_init()
> function. While the current implementation is to search all PCI devices
> in a system, this is not necessary.
Optimization is fine, but the most important thing here is to be clear
about what functional change this patch makes. As I mentioned at [1],
if this patch affects the class codes accepted, please make that clear
here.
> Reviewed-by: Mario Limonciello
> Signed-off-by: Sui Jingfeng
I do not see Mario's Reviewed-by on the list. I do see Mario's
Reviewed-by [2] for a previous version, but that version added this in
pci_notify():
+ if (pdev->class != PCI_CLASS_DISPLAY_VGA << 8)
+ return 0;
while this version adds:
+ if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
+ return 0;
It's OK to carry a review to future versions if there are
insignificant changes, but this is a functional change that seems
significant to me. The first matches only 0x03, while the second
discards the low eight bits so it matches 0x0300XX.
[1] https://lore.kernel.org/r/20230718231400.GA496927@bhelgaas
[2] https://lore.kernel.org/all/5b6fdf65-b354-94a9-f883-be820157e...@amd.com/
> ---
> drivers/pci/vgaarb.c | 25 +
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index c1bc6c983932..021116ed61cb 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -754,10 +754,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) {
> @@ -1502,6 +1498,10 @@ static int pci_notify(struct notifier_block *nb,
> unsigned long action,
>
> vgaarb_dbg(dev, "%s\n", __func__);
>
> + /* Deal with VGA compatible devices only */
> + 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
>* cases of hotplugable vga cards. */
> @@ -1534,8 +1534,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)
> @@ -1543,13 +1543,14 @@ static int __init vga_arb_device_init(void)
>
> bus_register_notifier(_bus_type, _notifier);
>
> - /* 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)
> - vga_arbiter_add_pci_device(pdev);
> + /*
> + * We add all PCI VGA compatible devices in the arbiter by default
> + */
> + do {
> + pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev);
> + if (pdev)
> + vga_arbiter_add_pci_device(pdev);
> + } while (pdev);
>
> pr_info("loaded\n");
> return rc;
> --
> 2.25.1
>
Re: [PATCH 2/4] PCI/VGA: Deal only with PCI VGA class devices
On Tue, Jul 18, 2023 at 06:14:00PM -0500, Bjorn Helgaas wrote: > On Fri, Jun 30, 2023 at 06:17:29PM +0800, Sui Jingfeng wrote: > > From: Sui Jingfeng > > > > VGAARB should only care about PCI VGA class devices (pdev->class == 0x0300) > > since only those devices might have VGA routed to them. > > This is not actually a question of whether VGA addresses (mem > 0xa-0xb and io 0x3b0-0x3bb, 0x3c0-0x3df) might be *routed* to > the device because that routing is controlled by the bridge VGA Enable > bit, not by a device Class Code. > > I think the important question here is what devices will *respond* to > those VGA addresses. The VGA arbiter works by managing bridge VGA > Enable bits, so if we know a device doesn't respond to the VGA > addresses, there's no point in adding a vga_device for it. Sorry, I see that I replied to an old version of this patch. I'll go look at this series instead: https://lore.kernel.org/r/20230711134354.755966-1-sui.jingf...@linux.dev Bjorn
Re: [PATCH 2/4] PCI/VGA: Deal only with PCI VGA class devices
On Fri, Jun 30, 2023 at 06:17:29PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> VGAARB should only care about PCI VGA class devices (pdev->class == 0x0300)
> since only those devices might have VGA routed to them.
This is not actually a question of whether VGA addresses (mem
0xa-0xb and io 0x3b0-0x3bb, 0x3c0-0x3df) might be *routed* to
the device because that routing is controlled by the bridge VGA Enable
bit, not by a device Class Code.
I think the important question here is what devices will *respond* to
those VGA addresses. The VGA arbiter works by managing bridge VGA
Enable bits, so if we know a device doesn't respond to the VGA
addresses, there's no point in adding a vga_device for it.
> PCI_CLASS_DISPLAY_3D and PCI_CLASS_DISPLAY_OTHER are used to annotate the
> render-only GPU. Render-only GPUs shouldn't decode the fixed VGA address.
> For example, nvidia render-only GPU typically has 0x0380 as its PCI class.
>
> A render-only GPU cannot be used to display something on the screen.
> Hence, it should not be the default boot device in normal cases.
Can you make the commit log say specifically what changes with this
patch? Is the idea that we previously added GPUs with Class Codes
like 0x0380, and after this patch we will only add GPUs that exactly
match 0x0300?
It doesn't *look* like that's the case because
vga_arbiter_add_pci_device() previously had:
if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
return false;
This ignores the programming interface (the low byte) but still
matches only base class 0x03 and subclass 0x00, so it shouldn't add a
0x0380 GPU.
This patch matches the entire 24-bit dev->class (base class, subclass,
and programming interface) against PCI_CLASS_DISPLAY_VGA << 8, so I
*think* this only accepts programming interface 0x00.
That might be OK, since the "PCI Code and ID Assignment" spec, r1.15,
sec 1.4, only mentions 0x0300 programming interface 0x00 as decoding
the legacy VGA addresses. But it is something the commit log should
be clear about.
> Cc: Bjorn Helgaas
> Reviewed-by: Mario Limonciello
> 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 c1bc6c983932..22a505e877dc 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -754,10 +754,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) {
> @@ -1500,7 +1496,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 != PCI_CLASS_DISPLAY_VGA << 8)
> + 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
> @@ -1510,6 +1508,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;
> @@ -1534,8 +1534,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: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function callback to vga_client_register
On Fri, Jun 30, 2023 at 10:14:11AM +0800, suijingfeng wrote: > On 2023/6/30 01:44, Limonciello, Mario wrote: > > > On 2023/6/29 23:54, Bjorn Helgaas wrote: > > > > On Thu, Jun 22, 2023 at 01:08:15PM +0800, Sui Jingfeng wrote: > > > > 4) Right now we're in the middle of the v6.5 merge window, so new > > > > content, e.g., this series, is too late for v6.5. Most > > > > maintainers, including me, wait to merge new content until the > > > > merge window closes and a new -rc1 is tagged. This merge window > > > > should close on July 9, and people will start merging content for > > > > v6.6, typically based on v6.5-rc1. > > > > > > Would you will merge all of the patches in this series (e.g. including > > > the patch for drm/amdgpu(7/8) and drm/radeon(8/8)) ? > > > > > > Or just part of them? The bulk of this series is drivers/pci changes, so typically I would merge all the patches after getting Acked-by tags from the other subsystems (DRM and VFIO). > Is it possible to merge the PCI/VGA part as fast as possible, > especially the PATCH-0006 PCI/VGA: Introduce is_boot_device function > callback to vga_client_register We're in the middle of the v6.5 merge window, so it's too late to add things to v6.5-rc1. The most likely path for new material like this would be to queue it for v6.6, which means I would merge it after v6.5-rc1 is tagged (that tag will probably happen on July 9). It would then be in -next until the v6.6 merge window opens (likely in September), when it would be merged into Linus' tree. If the series fixes a regression or other major defect, it's *possible* to merge things earlier, so they appear in v6.5. But this series doesn't seem to fall in that category, so I think v6.6 is a more realistic target. Merging for v6.6 would include both the PCI parts and the DRM parts at the same time, so hopefully that addresses your dependency concerns. I suggest that you wait until v6.5-rc1, rebase your patches so they apply cleanly on that tag, collect all the Reviewed-by and Acked-by tags, include them in your commit logs, and then repost them. Bjorn
Re: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function callback to vga_client_register
On Thu, Jun 22, 2023 at 01:08:15PM +0800, Sui Jingfeng wrote: > Hi, > > > A nouveau developer(Lyude) from redhat send me a R-B, > > Thanks for the developers of nouveau project. > > > Please allow me add a link[1] here. > > > [1] > https://lore.kernel.org/all/0afadc69f99a36bc9d03ecf54ff25859dbc10e28.ca...@redhat.com/ 1) Thanks for this. If you post another version of this series, please pick up Lyude's Reviewed-by and include it in the relevant patches (as long as you haven't made significant changes to the code Lyude reviewed). Whoever applies this should automatically pick up Reviewed-by/Ack/etc that are replies to the version being applied, but they won't go through previous revisions to find them. 2) Please mention the commit to which the series applies. I tried to apply this on v6.4-rc1, but it doesn't apply cleanly. 3) Thanks for including cover letters in your postings. Please include a little changelog in the cover letter so we know what changed between v6 and v7, etc. 4) Right now we're in the middle of the v6.5 merge window, so new content, e.g., this series, is too late for v6.5. Most maintainers, including me, wait to merge new content until the merge window closes and a new -rc1 is tagged. This merge window should close on July 9, and people will start merging content for v6.6, typically based on v6.5-rc1. Bjorn
Re: [PATCH v2] PCI: Add dummy implement for pci_clear_master() function
On Tue, Jun 20, 2023 at 06:06:00AM -0500, Bjorn Helgaas wrote: > On Tue, Jun 20, 2023 at 12:04:40PM +0800, Sui Jingfeng wrote: > > Where is the formal(unstream) PCI git branch where we could see the latest > > patch ? > > Here's the "misc" branch: > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=misc > > And here's the "next" branch that will be merged for v6.5, which > includes "misc" and other things: > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=next I forgot to mention: in case you need to find other git branches, most subsystems list this in the MAINTAINERS file, e.g., PCI SUBSYSTEM ... T: git git://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git
Re: [PATCH v2] PCI: Add dummy implement for pci_clear_master() function
On Tue, Jun 20, 2023 at 12:04:40PM +0800, Sui Jingfeng wrote: > Where is the formal(unstream) PCI git branch where we could see the latest > patch ? Here's the "misc" branch: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=misc And here's the "next" branch that will be merged for v6.5, which includes "misc" and other things: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=next
Re: [PATCH v8 6/8] drm/etnaviv: add driver support for the PCI devices
On Sat, Jun 10, 2023 at 02:07:58AM +0800, Sui Jingfeng wrote:
> On 2023/6/10 01:52, Bjorn Helgaas wrote:
> > On Fri, Jun 09, 2023 at 09:37:02AM +0800, Sui Jingfeng wrote:
> > > On 2023/6/9 01:32, Bjorn Helgaas wrote:
> > > > On Wed, Jun 07, 2023 at 06:55:49PM +0800, Sui Jingfeng wrote:
> > > > > From: Sui Jingfeng
> > > > >
> > > > > This patch adds PCI driver support on top of what we already have.
> > > > > Take
> > > > > the GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI
> > > > > device
> > > > > driver. There is only one GPU core for the GC1000 in the LS7A1000 and
> > > > > LS2K1000. Therefore, component frameworks can be avoided.
> > > > > + {PCI_VENDOR_ID_LOONGSON, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > > > > 0},
> > > > > + {PCI_VENDOR_ID_LOONGSON, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> > > > > 0},
> > > > PCI_VDEVICE()
> > > This make it impossible to hook device-specific data in the future.
> > >
> > > But currently there no device specific data associated with the
> > > 0x7a05 and 0x7a15,
> > >
> > > so it's acceptable for now. Thanks.
> > Haha, ISTR having this conversation before, sorry for repeating it.
> >
> > Indeed, it's fine as-is. But PCI_VDEVICE() actually *does* allow for
> > vendor-specific data because it doesn't include the data element,
> > which defaults to zero if you don't specify it.
> >
> > So for example, drivers/net/ethernet/realtek/r8169_main.c has this:
> >
> >{ PCI_VDEVICE(REALTEK, 0x8129) },
> >{ PCI_VDEVICE(REALTEK, 0x8136), RTL_CFG_NO_GBIT },
> >
> > where 0x8129 has no driver_data (it defaults to zero), but 0x8136
> > does.
>
> PCI_VDEVICE macro end with two zero. (I thought it was three)
No worries, I thought the same thing the first five times I read it :)
Re: [PATCH v8 6/8] drm/etnaviv: add driver support for the PCI devices
On Fri, Jun 09, 2023 at 09:37:02AM +0800, Sui Jingfeng wrote:
> On 2023/6/9 01:32, Bjorn Helgaas wrote:
> > On Wed, Jun 07, 2023 at 06:55:49PM +0800, Sui Jingfeng wrote:
> > > From: Sui Jingfeng
> > >
> > > This patch adds PCI driver support on top of what we already have. Take
> > > the GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device
> > > driver. There is only one GPU core for the GC1000 in the LS7A1000 and
> > > LS2K1000. Therefore, component frameworks can be avoided.
> > > + {PCI_VENDOR_ID_LOONGSON, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> > > + {PCI_VENDOR_ID_LOONGSON, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> >
> > PCI_VDEVICE()
>
> This make it impossible to hook device-specific data in the future.
>
> But currently there no device specific data associated with the
> 0x7a05 and 0x7a15,
>
> so it's acceptable for now. Thanks.
Haha, ISTR having this conversation before, sorry for repeating it.
Indeed, it's fine as-is. But PCI_VDEVICE() actually *does* allow for
vendor-specific data because it doesn't include the data element,
which defaults to zero if you don't specify it.
So for example, drivers/net/ethernet/realtek/r8169_main.c has this:
{ PCI_VDEVICE(REALTEK, 0x8129) },
{ PCI_VDEVICE(REALTEK, 0x8136), RTL_CFG_NO_GBIT },
where 0x8129 has no driver_data (it defaults to zero), but 0x8136
does.
Bjorn
Re: [Intel-gfx] [PATCH v3 4/4] PCI/VGA: introduce is_boot_device function callback to vga_client_register
On Fri, Jun 09, 2023 at 10:27:39AM +0800, Sui Jingfeng wrote: > On 2023/6/9 03:19, Bjorn Helgaas wrote: > > 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. > > This is chicken and egg question. > > If you could help get this merge first, I will show you the first user. > > > I'm not sure why the device driver should know whether its device is > > the default boot device. > > It's not that the device driver should know, > > but it's about that the device driver has the right to override. > > Device driver may have better approach to identify the default boot > device. The way we usually handle this is to include the new callback in the same series as the first user of it. That has two benefits: (1) everybody can review the whole picture and possibly suggest different approaches, and (2) when we merge the infrastructure, we also merge a user of it at the same time, so the whole thing can be tested and we don't end up with unused code. Bjorn
Re: [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: [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: [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: [PATCH v8 6/8] drm/etnaviv: add driver support for the PCI devices
On Wed, Jun 07, 2023 at 06:55:49PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> This patch adds PCI driver support on top of what we already have. Take
> the GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device
> driver. There is only one GPU core for the GC1000 in the LS7A1000 and
> LS2K1000. Therefore, component frameworks can be avoided.
> +#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
> +#include "etnaviv_pci_drv.h"
> +#endif
With trivial stubs for etnaviv_register_pci_driver() and
etnaviv_unregister_pci_driver(), I think you could get rid of all
these #ifdefs.
> +void etnaviv_drm_unbind(struct device *dev, bool component)
> {
> struct etnaviv_drm_private *priv = etna_private_ptr;
> struct drm_device *drm = priv->drm;
> @@ -746,6 +750,12 @@ static int __init etnaviv_init(void)
> if (ret != 0)
> goto unregister_gpu_driver;
>
> +#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
> + ret = etnaviv_register_pci_driver();
> + if (ret != 0)
> + goto unregister_platform_driver;
> +#endif
> +
> /*
>* If the DT contains at least one available GPU device, instantiate
>* the DRM platform device.
> @@ -763,7 +773,7 @@ static int __init etnaviv_init(void)
> break;
> }
>
> - return 0;
> + return ret;
>
> unregister_platform_driver:
> platform_driver_unregister(_platform_driver);
> @@ -778,6 +788,10 @@ static void __exit etnaviv_exit(void)
> etnaviv_destroy_platform_device(_platform_device);
> platform_driver_unregister(_platform_driver);
> platform_driver_unregister(_gpu_driver);
> +
> +#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
> + etnaviv_unregister_pci_driver();
> +#endif
> +static const struct pci_device_id etnaviv_pci_id_lists[] = {
> + {PCI_VENDOR_ID_LOONGSON, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
> + {PCI_VENDOR_ID_LOONGSON, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
PCI_VDEVICE()
Bjorn
Re: [PATCH v8 1/8] drm/etnaviv: add a dedicated function to register an irq handler
On Wed, Jun 07, 2023 at 06:55:44PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> Because getting IRQ from a device is platform-dependent, PCI devices have
> different methods for getting an IRQ. This patch is a preparation patch to
> extend the driver for the PCI device support.
>
> Cc: Lucas Stach
> Cc: Christian Gmeiner
> Cc: Philipp Zabel
> Cc: Bjorn Helgaas
> Cc: Daniel Vetter
> Signed-off-by: Sui Jingfeng
> ---
> drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 34 ---
> 1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index de8c9894967c..b9c12d3145a2 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -1817,6 +1817,29 @@ static const struct of_device_id etnaviv_gpu_match[] =
> {
> };
> MODULE_DEVICE_TABLE(of, etnaviv_gpu_match);
>
> +static int etnaviv_gpu_register_irq(struct etnaviv_gpu *gpu, int irq)
> +{
> + struct device *dev = gpu->dev;
> + int err;
> +
> + if (irq < 0) {
> + dev_err(dev, "failed to get irq: %d\n", irq);
Isn't this message redundant because platform_get_irq() already
emitted a message for this case?
> + return irq;
> + }
> +
> + err = devm_request_irq(dev, irq, irq_handler, 0, dev_name(dev), gpu);
> + if (err) {
> + dev_err(dev, "failed to request irq %u: %d\n", irq, err);
> + return err;
> + }
> +
> + gpu->irq = irq;
> +
> + dev_info(dev, "irq(%d) handler registered\n", irq);
> +
> + return 0;
> +}
> +
> static int etnaviv_gpu_platform_probe(struct platform_device *pdev)
> {
> struct device *dev = >dev;
> @@ -1837,16 +1860,9 @@ static int etnaviv_gpu_platform_probe(struct
> platform_device *pdev)
> return PTR_ERR(gpu->mmio);
>
> /* Get Interrupt: */
> - gpu->irq = platform_get_irq(pdev, 0);
> - if (gpu->irq < 0)
> - return gpu->irq;
> -
> - err = devm_request_irq(>dev, gpu->irq, irq_handler, 0,
> -dev_name(gpu->dev), gpu);
> - if (err) {
> - dev_err(dev, "failed to request IRQ%u: %d\n", gpu->irq, err);
> + err = etnaviv_gpu_register_irq(gpu, platform_get_irq(pdev, 0));
> + if (err)
> return err;
> - }
>
> /* Get Clocks: */
> gpu->clk_reg = devm_clk_get_optional(>dev, "reg");
> --
> 2.25.1
>
Re: [Intel-gfx] [PATCH v2 1/2] vgaarb: various coding style and comments fix
Match the subject line style:
$ git log --oneline drivers/pci/vgaarb.c
f321c35feaee PCI/VGA: Replace full MIT license text with SPDX identifier
d5109fe4d1ec PCI/VGA: Use unsigned format string to print lock counts
4e6c91847a7f PCI/VGA: Log bridge control messages when adding devices
dc593fd48abb PCI/VGA: Remove empty vga_arb_device_card_gone()
...
Subject line should be a summary of the commit log, not just "various
style fixes". This one needs to say something about
vga_str_to_iostate().
On Mon, Jun 05, 2023 at 04:58:30AM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> To keep consistent with vga_iostate_to_str() function, the third argument
> of vga_str_to_iostate() function should be 'unsigned int *'.
>
> Signed-off-by: Sui Jingfeng
> ---
> drivers/pci/vgaarb.c | 29 +++--
> include/linux/vgaarb.h | 8 +++-
> 2 files changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 5a696078b382..e40e6e5e5f03 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -61,7 +61,6 @@ static bool vga_arbiter_used;
> static DEFINE_SPINLOCK(vga_lock);
> static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
>
> -
> static const char *vga_iostate_to_str(unsigned int iostate)
> {
> /* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */
> @@ -77,10 +76,12 @@ static const char *vga_iostate_to_str(unsigned int
> iostate)
> return "none";
> }
>
> -static int vga_str_to_iostate(char *buf, int str_size, int *io_state)
> +static int vga_str_to_iostate(char *buf, int str_size, unsigned int
> *io_state)
> {
> - /* we could in theory hand out locks on IO and mem
> - * separately to userspace but it can cause deadlocks */
> + /*
> + * we could in theory hand out locks on IO and mem
> + * separately to userspace but it can cause deadlocks
> + */
Omit all the comment formatting changes. They are distractions from the
vga_str_to_iostate() parameter change.
I think this patch should be the single line change to the
vga_str_to_iostate() prototype so it matches the callers.
If you want to do the other comment formatting changes, they're fine,
but they should be all together in a separate patch that clearly
doesn't change the generated code.
Bjorn
Re: [PATCH v7 7/7] drm/etnaviv: add support for the dma coherent device
On Wed, Jun 07, 2023 at 02:43:27AM +0800, Sui Jingfeng wrote: > On 2023/6/7 00:56, Bjorn Helgaas wrote: > > On Sat, Jun 03, 2023 at 06:59:43PM +0800, Sui Jingfeng wrote: > > > From: Sui Jingfeng > > > > > > Loongson CPUs maintain cache coherency by hardware, which means that the > > > data in the CPU cache is identical to the data in main system memory. As > > > for the peripheral device, most of Loongson chips chose to define the > > > peripherals as DMA coherent by default, device drivers do not need to > > > maintain the coherency between a processor and an I/O device manually. > > ... > > I guess the only way to discover this coherency attribute is via the > > DT "vivante,gc" property? Seems a little weird but I'm really not a > > DT person. > > I'm not sure it is *only*, but it is very convenient to achieve such a thing > with DT. I don't know if this is a property of the PCI device, or a property of the system as a whole. I asked because PCI devices should be self-describing (the Device/Vendor ID should be enough to identify the correct driver, and the driver should know how to learn anything else it needs to know about the device from PCI config space) and should not require extra DT properties. But if this is a CPU or system property, you probably have to use a firmware interface like DT or ACPI. > > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c > > > @@ -8,6 +8,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > > It looks like this #include might not be needed? > > No, the dev_is_dma_coherent() function is declared and defined in > dma-map-ops.h. if remove it, gcc will complain: > > drivers/gpu/drm/etnaviv/etnaviv_drv.c: In function > ‘etnaviv_is_dma_coherent’: > drivers/gpu/drm/etnaviv/etnaviv_drv.c:56:14: error: implicit declaration of > function ‘dev_is_dma_coherent’; did you mean ‘etnaviv_is_dma_coherent’? > [-Werror=implicit-function-declaration] > 56 | coherent = dev_is_dma_coherent(dev); > | ^~~ Of course, but that warning is for etnaviv_drv.c, not for etnaviv_gpu.c. So etnaviv_drv.c needs to include dma-map-ops.h, but I don't think etnaviv_gpu.c does. I removed this #include from etnaviv_gpu.c and it still built without errors. > > You're only adding a > > new reference to priv->dma_coherent, which looks like it was added to > > etnaviv_drv.h.
Re: [PATCH v7 7/7] drm/etnaviv: add support for the dma coherent device
On Sat, Jun 03, 2023 at 06:59:43PM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng
>
> Loongson CPUs maintain cache coherency by hardware, which means that the
> data in the CPU cache is identical to the data in main system memory. As
> for the peripheral device, most of Loongson chips chose to define the
> peripherals as DMA coherent by default, device drivers do not need to
> maintain the coherency between a processor and an I/O device manually.
>
> There are exceptions, for LS2K1000 SoC, part of peripheral device can be
> configured as dma non-coherent. But there is no released version of such
> firmware exist in the market. Peripherals of older ls2k1000 is also DMA
> non-conherent, but they are nearly outdated. So, those are trivial cases.
s/dma/DMA/
s/non-conherent/non-coherent/
s/ls2k1000/LS2K1000/
I guess when you say these are "trivial cases," you mean you don't
care about supporting those devices?
> Nevertheless, kernel space still need to do probe work, because vivante GPU
> IP has been integrated into various platform. Hence, this patch add runtime
> detection code to probe if a specific gpu is DMA coherent, If the answer is
> yes, we are going to utilize such features. On Loongson platfform, When a
> buffer is accesed by both the GPU and the CPU, The driver should prefer
> ETNA_BO_CACHED over ETNA_BO_WC.
s/gpu/GPU/
s/platfform/platform/
s/accesed/accessed/
I guess the only way to discover this coherency attribute is via the
DT "vivante,gc" property? Seems a little weird but I'm really not a
DT person.
> This patch also add a new parameter: etnaviv_param_gpu_coherent, which
> allow userspace to know if such a feature is available. Because
> write-combined BO is still preferred in some case, especially where don't
> need CPU read, for example, uploading shader bin.
> ...
> +static struct device_node *etnaviv_of_first_available_node(void)
> +{
> + struct device_node *core_node;
> +
> + for_each_compatible_node(core_node, NULL, "vivante,gc") {
> + if (!of_device_is_available(core_node))
> + continue;
> +
> + return core_node;
> + }
> +
> + return NULL;
Seems like this would be simpler as:
for_each_compatible_node(core_node, NULL, "vivante,gc") {
if (of_device_is_available(core_node))
return core_node;
}
return NULL;
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -8,6 +8,7 @@
> #include
> #include
> #include
> +#include
It looks like this #include might not be needed? You're only adding a
new reference to priv->dma_coherent, which looks like it was added to
etnaviv_drv.h.
> #include
> #include
> #include
> @@ -164,6 +165,10 @@ int etnaviv_gpu_get_param(struct etnaviv_gpu *gpu, u32
> param, u64 *value)
> *value = gpu->identity.eco_id;
> break;
>
> + case ETNAVIV_PARAM_GPU_COHERENT:
> + *value = priv->dma_coherent;
> + break;
> +
> default:
> DBG("%s: invalid param: %u", dev_name(gpu->dev), param);
> return -EINVAL;
> @@ -1861,7 +1866,7 @@ static int etnaviv_gpu_register_irq(struct etnaviv_gpu
> *gpu, int irq)
>
> gpu->irq = irq;
>
> - dev_info(dev, "IRQ handler registered, irq = %d\n", irq);
> + dev_info(dev, "irq(%d) handler registered\n", irq);
Looks possibly unnecessary, or at least unrelated to this patch.
> return 0;
> }
Re: [PATCH v2] PCI: Add dummy implement for pci_clear_master() function
On Thu, Jun 01, 2023 at 01:44:52AM +0800, Sui Jingfeng wrote: > Can you receive my email? > > I reply several email to you, but Thunderbird told me that may mail is > returned. > > Maybe you could read the content at lore. I do receive your email (at least some; obviously if there are things I don't receive I wouldn't know). I read most Linux email via lei [1], so if it shows up on lore, I should see it. Bjorn [1] https://people.kernel.org/monsieuricon/lore-lei-part-1-getting-started
Re: [PATCH v2] PCI: Add dummy implement for pci_clear_master() function
On Wed, May 31, 2023 at 06:27:44PM +0800, Sui Jingfeng wrote:
> As some arch(m68k for example) doesn't have config_pci enabled, drivers[1]
> call pci_clear_master() without config_pci guard can not pass compile test.
>
>drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c:
>In function 'etnaviv_gpu_pci_fini':
> >> drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c:32:9:
>error: implicit declaration of function 'pci_clear_master';
>did you mean 'pci_set_master'? [-Werror=implicit-function-declaration]
> 32 | pci_clear_master(pdev);
> | ^~~~
> | pci_set_master
>cc1: some warnings being treated as errors
>
> [1] https://patchwork.freedesktop.org/patch/539977/?series=118522=1
>
> V2:
> * Adjust commit log style to meet the convention and add Fixes tag
>
> Fixes: 6a479079c072 ("PCI: Add pci_clear_master() as opposite of
> pci_set_master()")
> Reported-by: kernel test robot
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202305301659.4guslavl-...@intel.com/
> Reviewed-by: Geert Uytterhoeven
> Signed-off-by: Sui Jingfeng
Applied to pci/misc for v6.5 as follows:
Author: Sui Jingfeng
Date: Wed May 31 18:27:44 2023 +0800
PCI: Add pci_clear_master() stub for non-CONFIG_PCI
Add a pci_clear_master() stub when CONFIG_PCI is not set so drivers that
support both PCI and platform devices don't need #ifdefs or extra Kconfig
symbols for the PCI parts.
[bhelgaas: commit log]
Fixes: 6a479079c072 ("PCI: Add pci_clear_master() as opposite of
pci_set_master()")
Link:
https://lore.kernel.org/r/20230531102744.2354313-1-suijingf...@loongson.cn
Signed-off-by: Sui Jingfeng
Signed-off-by: Bjorn Helgaas
Reviewed-by: Geert Uytterhoeven
> ---
> include/linux/pci.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d0c19ff0c958..71c85380676c 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1904,6 +1904,7 @@ static inline int pci_dev_present(const struct
> pci_device_id *ids)
> #define pci_dev_put(dev) do { } while (0)
>
> static inline void pci_set_master(struct pci_dev *dev) { }
> +static inline void pci_clear_master(struct pci_dev *dev) { }
> static inline int pci_enable_device(struct pci_dev *dev) { return -EIO; }
> static inline void pci_disable_device(struct pci_dev *dev) { }
> static inline int pcim_enable_device(struct pci_dev *pdev) { return -EIO; }
> --
> 2.25.1
>
Re: [PATCH] pci/vgaarb: make vga_is_firmware_default() arch independent
On Mon, May 29, 2023 at 11:45:04PM +0800, Sui Jingfeng wrote:
> The vga_is_firmware_default() function will work on non-x86 architectures
> as long as the arch has UEFI GOP support, which passes the firmware
> framebuffer base address and size.
>
> This patch makes the vga_is_firmware_default() function arch-independent.
> This could help the VGAARB subsystem make the right choice for multiple
> GPU systems. Usually an integrated one and a discrete one for desktop
> computers. Depending on the firmware framebuffer being put into which
> GPU's VRAM, VGAARB could inherit the firmware's choice, which in turn,
> is the exact choice of the user.
Is there a system that needs this change? If so, the commit log
should mention it.
It's definitely nice to remove #ifdefs, but it's better if we have an
actual reason and some testing of another arch that makes use of this.
Also, take a look at the git history and match the subject line and
commit log style (prefix, capitalization, imperative voice).
> Signed-off-by: Sui Jingfeng
> ---
> drivers/pci/vgaarb.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 5a696078b382..f81b6c54e327 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -61,7 +61,6 @@ static bool vga_arbiter_used;
> static DEFINE_SPINLOCK(vga_lock);
> static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
>
> -
> static const char *vga_iostate_to_str(unsigned int iostate)
> {
> /* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */
> @@ -545,7 +544,6 @@ EXPORT_SYMBOL(vga_put);
>
> static bool vga_is_firmware_default(struct pci_dev *pdev)
> {
> -#if defined(CONFIG_X86) || defined(CONFIG_IA64)
> u64 base = screen_info.lfb_base;
> u64 size = screen_info.lfb_size;
> struct resource *r;
> @@ -571,7 +569,7 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
>
> return true;
> }
> -#endif
> +
> return false;
> }
>
> @@ -865,8 +863,7 @@ static bool vga_arbiter_del_pci_device(struct pci_dev
> *pdev)
> }
>
> /* this is called with the lock */
> -static inline void vga_update_device_decodes(struct vga_device *vgadev,
> - int new_decodes)
> +static void vga_update_device_decodes(struct vga_device *vgadev, int
> new_decodes)
I don't mind removing the "inline" here, but it shouldn't be combined
with the rest of the patch. When it's combined, I can't tell whether
there's a reason we need this change or if it's just a cleanup.
> {
> struct device *dev = >pdev->dev;
> int old_decodes, decodes_removed, decodes_unlocked;
> --
> 2.25.1
>
Re: [PATCH v6 5/6] drm/etnaviv: add driver support for the PCI devices
On Wed, May 31, 2023 at 12:06:42AM +0800, Sui Jingfeng wrote:
> This patch adds PCI driver support on top of what already have. Take the
> GC1000 in LS7A1000/LS2K1000 as the first instance of the PCI device driver.
> There is only one GPU core for the GC1000 in the LS7A1000 and LS2K1000.
> Therefore, component frameworks can be avoided. Because we want to bind the
> DRM driver service to the PCI driver manually.
> + * Loongson Mips and LoongArch CPU(ls3a5000, ls3a4000, ls2k1000la)
> + * maintain cache coherency by hardware
> + */
> + if (IS_ENABLED(CONFIG_CPU_LOONGSON64) || IS_ENABLED(CONFIG_LOONGARCH))
> + priv->has_cached_coherent = true;
This looks like something that should be a runtime check, not a
compile-time check.
If it's possible to build a single kernel image that runs on Loongson
MIPS or LoongArch CPU and, in addition, runs on other platforms, you
cannot assume that all the others maintain this cache coherency.
> +static struct etnaviv_drm_private *etna_private_s;
A static pointer looks wrong because it probably limits you to a
single instance of something.
> @@ -727,6 +756,12 @@ static int __init etnaviv_init(void)
> if (ret != 0)
> goto unregister_gpu_driver;
>
> +#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
> + ret = pci_register_driver(_pci_driver);
> +#endif
> + if (ret != 0)
> + goto unregister_platform_driver;
Why is this outside the #ifdef? If CONFIG_DRM_ETNAVIV_PCI_DRIVER is
not set, you already tested "ret != 0" above and will never take this
goto.
> +static int etnaviv_gpu_plat_drv_init(struct etnaviv_gpu *gpu, bool component)
> +{
> + struct device *dev = gpu->dev;
> + struct platform_device *pdev = to_platform_device(dev);
> + int err;
> +
> + /* Map registers: */
> + gpu->mmio = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(gpu->mmio))
> + return PTR_ERR(gpu->mmio);
> +
> + if (component) {
> + err = component_add(dev, _ops);
> + if (err < 0) {
> + dev_err(dev, "failed to register component: %d\n", err);
> + return err;
> + }
> + }
> +
> + return 0;
> +}
All this platform driver rearrangement looks like it should be a
separate patch so adding PCI support only adds PCI-related stuff.
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c
> @@ -0,0 +1,87 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include
> +
> +#include "etnaviv_drv.h"
> +#include "etnaviv_gpu.h"
> +#include "etnaviv_pci_drv.h"
> +
> +enum etnaviv_pci_gpu_family {
> + GC1000_IN_LS7A1000 = 0,
> + GC1000_IN_LS2K1000 = 1,
Seems unused; why is this here?
> +static int etnaviv_pci_probe(struct pci_dev *pdev,
> + const struct pci_device_id *ent)
> +{
> + struct device *dev = >dev;
> + int ret;
> +
> + ret = pcim_enable_device(pdev);
> + if (ret) {
> + dev_err(>dev, "failed to enable\n");
Use "dev", no need for ">dev" since you already looked it up
above. Also below for dma_set_mask_and_coherent().
> + return ret;
> + }
> +
> + pci_set_master(pdev);
> +
> + ret = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(32));
> +static const struct pci_device_id etnaviv_pci_id_lists[] = {
> + {0x0014, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0, GC1000_IN_LS7A1000},
> + {0x0014, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0, GC1000_IN_LS2K1000},
> + {0, 0, 0, 0, 0, 0, 0}
Should probably use PCI_DEVICE_DATA(). Use PCI_VENDOR_ID_LOONGSON.
Only "{ }" required to terminate.
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __ETNAVIV_PCI_DRV_H__
> +#define __ETNAVIV_PCI_DRV_H__
> +
> +#include
This #include isn't required by this file.
> +#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
> +extern struct pci_driver etnaviv_pci_driver;
> +#endif
> +
> +#endif
Re: [PATCH v6 6/6] drm/etnaviv: allow usperspace create cached coherent bo
s/usperspace/userspace/ (in subject) On Wed, May 31, 2023 at 12:06:43AM +0800, Sui Jingfeng wrote: > cached system RAM is coherent on loongson CPUs, and the GPU and DC allways > snoop the CPU's cache. write-combine caching property is not suitiable for > us. s/allways/always/ s/suitiable/suitable/
Re: [PATCH] PCI: Add ASPEED vendor ID
[+cc Damien, linux-ide]
On Thu, Apr 20, 2023 at 09:08:48AM +0200, Thomas Zimmermann wrote:
> Am 19.04.23 um 20:37 schrieb Bjorn Helgaas:
> > On Wed, Apr 19, 2023 at 09:00:15AM +0200, Thomas Zimmermann wrote:
> > > Am 19.04.23 um 00:57 schrieb Patrick McLean:
> > > > Currently the ASPEED PCI vendor ID is defined in
> > > > drivers/gpu/drm/ast/ast_drv.c, move that to include/linux/pci_ids.h
> > > > with all the rest of the PCI vendor ID definitions. Rename the
> > > > definition
> > > > to follow the format that the other definitions follow.
> > >
> > > Thanks a lot. Can you please also move and rename the PCI device ids? [1]
> >
> > Generally we move things to pci_ids.h only when they are shared
> > between multiple drivers. This is mostly to make backports easier.
> >
> > PCI_VENDOR_ID_ASPEED is (or will be) used in both ast_drv.c and
> > libata-core.c, so it qualifies.
> >
> > It doesn't look like PCI_CHIP_AST2000 and PCI_CHIP_AST2100 would
> > qualify since they're only used in ast_drv.c and ast_main.c, which are
> > part of the same driver.
>
> Ok, I see. Can I take the patch into DRM trees?
The first time around I got two patches [2]. This time I only got
this patch, but IIUC there are still two patches in play here:
- This one, which moves PCI_VENDOR_ID_ASPEED to pci_ids.h, and
- The libata-core one that adds a use in ata_dev_config_ncq()
Those should go together via the same tree. I supplied my ack to
indicate that I'm not going to merge anything myself, and I expect
whoever merges the libata patch to also merge this one.
If for some reason the libata-core patch doesn't happen, then this
patch shouldn't happen either, because there would no longer be any
sharing between drivers that would justify a pci_ids.h addition.
Bjorn
[2] https://lore.kernel.org/r/20230418011720.3900090-1-chutz...@gentoo.org
> > > [1]
> > > https://elixir.bootlin.com/linux/v6.2/source/drivers/gpu/drm/ast/ast_drv.h#L52
> > > > Signed-off-by: Patrick McLean
> > > > ---
> > > >drivers/gpu/drm/ast/ast_drv.c | 4 +---
> > > >include/linux/pci_ids.h | 2 ++
> > > >2 files changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/ast/ast_drv.c
> > > > b/drivers/gpu/drm/ast/ast_drv.c
> > > > index d78852c7cf5b..232e797793b6 100644
> > > > --- a/drivers/gpu/drm/ast/ast_drv.c
> > > > +++ b/drivers/gpu/drm/ast/ast_drv.c
> > > > @@ -70,12 +70,10 @@ static const struct drm_driver ast_driver = {
> > > > * PCI driver
> > > > */
> > > > -#define PCI_VENDOR_ASPEED 0x1a03
> > > > -
> > > >#define AST_VGA_DEVICE(id, info) { \
> > > > .class = PCI_BASE_CLASS_DISPLAY << 16, \
> > > > .class_mask = 0xff, \
> > > > - .vendor = PCI_VENDOR_ASPEED,\
> > > > + .vendor = PCI_VENDOR_ID_ASPEED, \
> > > > .device = id, \
> > > > .subvendor = PCI_ANY_ID,\
> > > > .subdevice = PCI_ANY_ID,\
> > > > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> > > > index 45c3d62e616d..40e04e88ca5a 100644
> > > > --- a/include/linux/pci_ids.h
> > > > +++ b/include/linux/pci_ids.h
> > > > @@ -2553,6 +2553,8 @@
> > > >#define PCI_DEVICE_ID_NETRONOME_NFP3800_VF 0x3803
> > > >#define PCI_DEVICE_ID_NETRONOME_NFP6000_VF 0x6003
> > > > +#define PCI_VENDOR_ID_ASPEED 0x1a03
> > > > +
> > > >#define PCI_VENDOR_ID_QMI0x1a32
> > > >#define PCI_VENDOR_ID_AZWAVE 0x1a3b
> > >
> > > --
> > > Thomas Zimmermann
> > > Graphics Driver Developer
> > > SUSE Software Solutions Germany GmbH
> > > Maxfeldstr. 5, 90409 Nürnberg, Germany
> > > (HRB 36809, AG Nürnberg)
> > > Geschäftsführer: Ivo Totev
> >
> >
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
Re: [PATCH] PCI: Add ASPEED vendor ID
On Tue, Apr 18, 2023 at 03:57:57PM -0700, Patrick McLean wrote:
> Currently the ASPEED PCI vendor ID is defined in
> drivers/gpu/drm/ast/ast_drv.c, move that to include/linux/pci_ids.h
> with all the rest of the PCI vendor ID definitions. Rename the definition
> to follow the format that the other definitions follow.
>
> Signed-off-by: Patrick McLean
Acked-by: Bjorn Helgaas
But please include this patch in the series that adds the use in
libata-core, as in your original posting, so we can see *why* we're
moving this to pci_ids.h. That also makes it easier to make sure
those patches go together.
> ---
> drivers/gpu/drm/ast/ast_drv.c | 4 +---
> include/linux/pci_ids.h | 2 ++
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index d78852c7cf5b..232e797793b6 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -70,12 +70,10 @@ static const struct drm_driver ast_driver = {
> * PCI driver
> */
>
> -#define PCI_VENDOR_ASPEED 0x1a03
> -
> #define AST_VGA_DEVICE(id, info) { \
> .class = PCI_BASE_CLASS_DISPLAY << 16, \
> .class_mask = 0xff, \
> - .vendor = PCI_VENDOR_ASPEED,\
> + .vendor = PCI_VENDOR_ID_ASPEED, \
> .device = id, \
> .subvendor = PCI_ANY_ID,\
> .subdevice = PCI_ANY_ID,\
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 45c3d62e616d..40e04e88ca5a 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2553,6 +2553,8 @@
> #define PCI_DEVICE_ID_NETRONOME_NFP3800_VF 0x3803
> #define PCI_DEVICE_ID_NETRONOME_NFP6000_VF 0x6003
>
> +#define PCI_VENDOR_ID_ASPEED 0x1a03
> +
> #define PCI_VENDOR_ID_QMI0x1a32
>
> #define PCI_VENDOR_ID_AZWAVE 0x1a3b
> --
> 2.40.0
>
Re: [PATCH] PCI: Add ASPEED vendor ID
On Wed, Apr 19, 2023 at 09:00:15AM +0200, Thomas Zimmermann wrote:
> Am 19.04.23 um 00:57 schrieb Patrick McLean:
> > Currently the ASPEED PCI vendor ID is defined in
> > drivers/gpu/drm/ast/ast_drv.c, move that to include/linux/pci_ids.h
> > with all the rest of the PCI vendor ID definitions. Rename the definition
> > to follow the format that the other definitions follow.
>
> Thanks a lot. Can you please also move and rename the PCI device ids? [1]
Generally we move things to pci_ids.h only when they are shared
between multiple drivers. This is mostly to make backports easier.
PCI_VENDOR_ID_ASPEED is (or will be) used in both ast_drv.c and
libata-core.c, so it qualifies.
It doesn't look like PCI_CHIP_AST2000 and PCI_CHIP_AST2100 would
qualify since they're only used in ast_drv.c and ast_main.c, which are
part of the same driver.
> [1]
> https://elixir.bootlin.com/linux/v6.2/source/drivers/gpu/drm/ast/ast_drv.h#L52
>
> >
> > Signed-off-by: Patrick McLean
> > ---
> > drivers/gpu/drm/ast/ast_drv.c | 4 +---
> > include/linux/pci_ids.h | 2 ++
> > 2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> > index d78852c7cf5b..232e797793b6 100644
> > --- a/drivers/gpu/drm/ast/ast_drv.c
> > +++ b/drivers/gpu/drm/ast/ast_drv.c
> > @@ -70,12 +70,10 @@ static const struct drm_driver ast_driver = {
> >* PCI driver
> >*/
> > -#define PCI_VENDOR_ASPEED 0x1a03
> > -
> > #define AST_VGA_DEVICE(id, info) {\
> > .class = PCI_BASE_CLASS_DISPLAY << 16, \
> > .class_mask = 0xff, \
> > - .vendor = PCI_VENDOR_ASPEED,\
> > + .vendor = PCI_VENDOR_ID_ASPEED, \
> > .device = id, \
> > .subvendor = PCI_ANY_ID,\
> > .subdevice = PCI_ANY_ID,\
> > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> > index 45c3d62e616d..40e04e88ca5a 100644
> > --- a/include/linux/pci_ids.h
> > +++ b/include/linux/pci_ids.h
> > @@ -2553,6 +2553,8 @@
> > #define PCI_DEVICE_ID_NETRONOME_NFP3800_VF0x3803
> > #define PCI_DEVICE_ID_NETRONOME_NFP6000_VF0x6003
> > +#define PCI_VENDOR_ID_ASPEED 0x1a03
> > +
> > #define PCI_VENDOR_ID_QMI 0x1a32
> > #define PCI_VENDOR_ID_AZWAVE 0x1a3b
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev
Re: [PATCH 1/2] gpu: Move ASPEED vendor ID definition to pci_ids.h
On Tue, Apr 18, 2023 at 04:14:03PM -0500, Bjorn Helgaas wrote: > Most subject lines for pci_ids.h look like this: > > PCI: Add ASPEED vendor ID > > On Mon, Apr 17, 2023 at 06:17:19PM -0700, Patrick McLean wrote: > > Currently the ASPEED PCI vendor ID is defined in > > drivers/gpu/drm/ast/ast_drv.c, > > move that to include/linux/pci_ids.h with all the rest of the PCI vendor ID > > definitions. Rename the definition to follow the format that the other > > definitions follow. > > > > Signed-off-by: Patrick McLean > > Given the subject line and file placement (below) updates, > > Acked-by: Bjorn Helgaas Oh, at the same time, would you mind rewrapping at least this commit log so it fits in 75 columns to "git log" doesn't overflow an 80 column terminal? Bjorn
Re: [PATCH 1/2] gpu: Move ASPEED vendor ID definition to pci_ids.h
Most subject lines for pci_ids.h look like this:
PCI: Add ASPEED vendor ID
On Mon, Apr 17, 2023 at 06:17:19PM -0700, Patrick McLean wrote:
> Currently the ASPEED PCI vendor ID is defined in
> drivers/gpu/drm/ast/ast_drv.c,
> move that to include/linux/pci_ids.h with all the rest of the PCI vendor ID
> definitions. Rename the definition to follow the format that the other
> definitions follow.
>
> Signed-off-by: Patrick McLean
Given the subject line and file placement (below) updates,
Acked-by: Bjorn Helgaas
> ---
> drivers/gpu/drm/ast/ast_drv.c | 4 +---
> include/linux/pci_ids.h | 2 ++
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index d78852c7cf5b..232e797793b6 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -70,12 +70,10 @@ static const struct drm_driver ast_driver = {
> * PCI driver
> */
>
> -#define PCI_VENDOR_ASPEED 0x1a03
> -
> #define AST_VGA_DEVICE(id, info) { \
> .class = PCI_BASE_CLASS_DISPLAY << 16, \
> .class_mask = 0xff, \
> - .vendor = PCI_VENDOR_ASPEED,\
> + .vendor = PCI_VENDOR_ID_ASPEED, \
> .device = id, \
> .subvendor = PCI_ANY_ID,\
> .subdevice = PCI_ANY_ID,\
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 45c3d62e616d..6634741aea80 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -815,6 +815,8 @@
> #define PCI_VENDOR_ID_ASUSTEK0x1043
> #define PCI_DEVICE_ID_ASUSTEK_0675 0x0675
>
> +#define PCI_VENDOR_ID_ASPEED 0x1a03
This looks like a random placement. Please keep sorted by numeric
vendor ID. I'll make the comment at the top a little more specific.
> #define PCI_VENDOR_ID_DPT0x1044
> #define PCI_DEVICE_ID_DPT0xa400
>
> --
> 2.40.0
>
Re: [PATCH] accel/ivpu: Remove D3hot delay for Meteorlake
On Fri, Mar 31, 2023 at 01:40:27PM +0200, Stanislaw Gruszka wrote:
> From: Karol Wachowski
>
> VPU on MTL has hardware optimizations and does not require 10ms
> D0 - D3hot transition delay imposed by PCI specification.
PCIe r6.0, sec 5.9.
> The delay removal is traditionally done by adding PCI ID to
> quirk_remove_dhot_delay() in drivers/pci/quirks.c . But since
quirk_remove_d3hot_delay()
> we do not need that optimization before driver probe and we
> can better specify in the ivpu driver on what (future) hardware
> use the optimization, we do not use quirk_remove_dhot_delay()
Again.
> for that.
>
> Signed-off-by: Karol Wachowski
> Signed-off-by: Stanislaw Gruszka
> ---
> drivers/accel/ivpu/ivpu_drv.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
> index 3be4a5a2b07a..cf9925c0a8ad 100644
> --- a/drivers/accel/ivpu/ivpu_drv.c
> +++ b/drivers/accel/ivpu/ivpu_drv.c
> @@ -442,6 +442,10 @@ static int ivpu_pci_init(struct ivpu_device *vdev)
> /* Clear any pending errors */
> pcie_capability_clear_word(pdev, PCI_EXP_DEVSTA, 0x3f);
>
> + /* VPU MTL does not require PCI spec 10m D3hot delay */
> + if (ivpu_is_mtl(vdev))
> + pdev->d3hot_delay = 0;
d3hot_delay is used after a D0->D3hot transition, after a D3hot->D0
transition, and after the D0->D3hot and D3hot->D0 transitions in
pci_pm_reset().
I assume this device can tolerate removing *all* of those delays,
right?
> ret = pcim_enable_device(pdev);
> if (ret) {
> ivpu_err(vdev, "Failed to enable PCI device: %d\n", ret);
> --
> 2.25.1
>
[PATCH] habanalabs: Drop redundant pci_enable_pcie_error_reporting()
From: Bjorn Helgaas
pci_enable_pcie_error_reporting() enables the device to send ERR_*
Messages. Since f26e58bf6f54 ("PCI/AER: Enable error reporting when AER is
native"), the PCI core does this for all devices during enumeration, so the
driver doesn't need to do it itself.
Remove the redundant pci_enable_pcie_error_reporting() call from the
driver. Also remove the corresponding pci_disable_pcie_error_reporting()
from the driver .remove() path.
Note that this only controls ERR_* Messages from the device. An ERR_*
Message may cause the Root Port to generate an interrupt, depending on the
AER Root Error Command register managed by the AER service driver.
Signed-off-by: Bjorn Helgaas
---
drivers/accel/habanalabs/common/habanalabs_drv.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/accel/habanalabs/common/habanalabs_drv.c
b/drivers/accel/habanalabs/common/habanalabs_drv.c
index 03dae57dc838..26f65aa21079 100644
--- a/drivers/accel/habanalabs/common/habanalabs_drv.c
+++ b/drivers/accel/habanalabs/common/habanalabs_drv.c
@@ -12,7 +12,6 @@
#include "../include/hw_ip/pci/pci_general.h"
#include
-#include
#include
#define CREATE_TRACE_POINTS
@@ -550,8 +549,6 @@ static int hl_pci_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
pci_set_drvdata(pdev, hdev);
- pci_enable_pcie_error_reporting(pdev);
-
rc = hl_device_init(hdev, hl_class);
if (rc) {
dev_err(>dev, "Fatal error during habanalabs device
init\n");
@@ -562,7 +559,6 @@ static int hl_pci_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
return 0;
disable_device:
- pci_disable_pcie_error_reporting(pdev);
pci_set_drvdata(pdev, NULL);
destroy_hdev(hdev);
@@ -585,7 +581,6 @@ static void hl_pci_remove(struct pci_dev *pdev)
return;
hl_device_fini(hdev);
- pci_disable_pcie_error_reporting(pdev);
pci_set_drvdata(pdev, NULL);
destroy_hdev(hdev);
}
--
2.25.1
[PATCH] drm/amdgpu: Drop redundant pci_enable_pcie_error_reporting()
From: Bjorn Helgaas
pci_enable_pcie_error_reporting() enables the device to send ERR_*
Messages. Since f26e58bf6f54 ("PCI/AER: Enable error reporting when AER is
native"), the PCI core does this for all devices during enumeration, so the
driver doesn't need to do it itself.
Remove the redundant pci_enable_pcie_error_reporting() call from the
driver.
Note that this only controls ERR_* Messages from the device. An ERR_*
Message may cause the Root Port to generate an interrupt, depending on the
AER Root Error Command register managed by the AER service driver.
Signed-off-by: Bjorn Helgaas
---
drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 -
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 --
2 files changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 164141bc8b4a..208cebb40232 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -50,7 +50,6 @@
#include
#include
#include
-#include
#include
#include
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index c4a4e2fe6681..a5151e83a3f7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3773,8 +3773,6 @@ int amdgpu_device_init(struct amdgpu_device *adev,
}
}
- pci_enable_pcie_error_reporting(adev->pdev);
-
/* Post card if necessary */
if (amdgpu_device_need_post(adev)) {
if (!adev->bios) {
--
2.25.1
Re: [PATCH] Revert "fbdev: Remove conflicting devices on PCI bus"
On Mon, Feb 06, 2023 at 06:59:40AM +1000, Dave Airlie wrote:
> On Sat, 4 Feb 2023 at 09:09, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas
> >
> > This reverts commit 145eed48de278007f646b908fd70ac59d24ed81a.
> >
> > Zeno Davatz reported that 145eed48de27 ("fbdev: Remove conflicting devices
> > on PCI bus") caused a console hang. The machine was actually still usable
> > via ssh, etc., but there was no activity on the console.
> >
> > Reverting 145eed48de27 for the nvidiafb on that system fixed the problem.
> >
> > Revert 145eed48de27 ("fbdev: Remove conflicting devices on PCI bus") since
> > we don't know what caused the problem.
>
> Why is the user using nvidiafb?
I don't know, and of course, it really doesn't matter; we shouldn't
regress a user's experience, and there's no hint to the user of where
to look for a resolution.
Thanks for working out a better fix!
Bjorn
Re: [PATCH] Revert "fbdev: Remove conflicting devices on PCI bus"
On Sat, Feb 04, 2023 at 09:50:18AM +0100, Lukas Wunner wrote:
> On Fri, Feb 03, 2023 at 05:09:09PM -0600, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas
> >
> > This reverts commit 145eed48de278007f646b908fd70ac59d24ed81a.
> >
> > Zeno Davatz reported that 145eed48de27 ("fbdev: Remove conflicting devices
> > on PCI bus") caused a console hang. The machine was actually still usable
> > via ssh, etc., but there was no activity on the console.
> >
> > Reverting 145eed48de27 for the nvidiafb on that system fixed the problem.
> >
> > Revert 145eed48de27 ("fbdev: Remove conflicting devices on PCI bus") since
> > we don't know what caused the problem.
> >
> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=216859
>
> Shouldn't that rather be:
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216859
> Fixes: 145eed48de27 ("fbdev: Remove conflicting devices on PCI bus")
> Cc: sta...@vger.kernel.org # v6.1+
Yes, of course, thank you, Lukas!
[PATCH] Revert "fbdev: Remove conflicting devices on PCI bus"
From: Bjorn Helgaas
This reverts commit 145eed48de278007f646b908fd70ac59d24ed81a.
Zeno Davatz reported that 145eed48de27 ("fbdev: Remove conflicting devices
on PCI bus") caused a console hang. The machine was actually still usable
via ssh, etc., but there was no activity on the console.
Reverting 145eed48de27 for the nvidiafb on that system fixed the problem.
Revert 145eed48de27 ("fbdev: Remove conflicting devices on PCI bus") since
we don't know what caused the problem.
Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=216859
Reported-by: Zeno Davatz
Tested-by: Zeno Davatz
Signed-off-by: Bjorn Helgaas
Cc: Helge Deller
Cc: Thomas Zimmermann
Cc: Javier Martinez Canillas
Cc: linux-fb...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/video/fbdev/arkfb.c | 5 -
drivers/video/fbdev/asiliantfb.c | 5 -
drivers/video/fbdev/aty/aty128fb.c | 5 -
drivers/video/fbdev/aty/atyfb_base.c | 7 +--
drivers/video/fbdev/carminefb.c | 5 -
drivers/video/fbdev/chipsfb.c| 7 +--
drivers/video/fbdev/cirrusfb.c | 5 -
drivers/video/fbdev/cyber2000fb.c| 5 -
drivers/video/fbdev/geode/gx1fb_core.c | 5 -
drivers/video/fbdev/geode/gxfb_core.c| 5 -
drivers/video/fbdev/geode/lxfb_core.c| 5 -
drivers/video/fbdev/gxt4500.c| 5 -
drivers/video/fbdev/i740fb.c | 5 -
drivers/video/fbdev/i810/i810_main.c | 5 -
drivers/video/fbdev/imsttfb.c| 8 +---
drivers/video/fbdev/intelfb/intelfbdrv.c | 5 -
drivers/video/fbdev/kyro/fbdev.c | 5 -
drivers/video/fbdev/matrox/matroxfb_base.c | 5 -
drivers/video/fbdev/mb862xx/mb862xxfbdrv.c | 5 -
drivers/video/fbdev/neofb.c | 5 -
drivers/video/fbdev/nvidia/nvidia.c | 7 +--
drivers/video/fbdev/pm2fb.c | 5 -
drivers/video/fbdev/pm3fb.c | 5 -
drivers/video/fbdev/pvr2fb.c | 5 -
drivers/video/fbdev/riva/fbdev.c | 5 -
drivers/video/fbdev/s3fb.c | 5 -
drivers/video/fbdev/savage/savagefb_driver.c | 5 -
drivers/video/fbdev/sis/sis_main.c | 5 -
drivers/video/fbdev/skeletonfb.c | 8
drivers/video/fbdev/sm712fb.c| 5 -
drivers/video/fbdev/sstfb.c | 5 -
drivers/video/fbdev/sunxvr2500.c | 5 -
drivers/video/fbdev/sunxvr500.c | 5 -
drivers/video/fbdev/tdfxfb.c | 5 -
drivers/video/fbdev/tgafb.c | 7 ---
drivers/video/fbdev/tridentfb.c | 5 -
drivers/video/fbdev/vermilion/vermilion.c| 7 +--
drivers/video/fbdev/via/via-core.c | 5 -
drivers/video/fbdev/vt8623fb.c | 5 -
39 files changed, 5 insertions(+), 206 deletions(-)
diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c
index 60a96fdb5dd8..41b9117c55bb 100644
--- a/drivers/video/fbdev/arkfb.c
+++ b/drivers/video/fbdev/arkfb.c
@@ -11,7 +11,6 @@
* Code is based on s3fb
*/
-#include
#include
#include
#include
@@ -949,10 +948,6 @@ static int ark_pci_probe(struct pci_dev *dev, const struct
pci_device_id *id)
int rc;
u8 regval;
- rc = aperture_remove_conflicting_pci_devices(dev, "arkfb");
- if (rc < 0)
- return rc;
-
/* Ignore secondary VGA device because there is no VGA arbitration */
if (! svga_primary_device(dev)) {
dev_info(&(dev->dev), "ignoring secondary device\n");
diff --git a/drivers/video/fbdev/asiliantfb.c b/drivers/video/fbdev/asiliantfb.c
index 8383468f5577..4a98383eb274 100644
--- a/drivers/video/fbdev/asiliantfb.c
+++ b/drivers/video/fbdev/asiliantfb.c
@@ -29,7 +29,6 @@
* more details.
*/
-#include
#include
#include
#include
@@ -546,10 +545,6 @@ static int asiliantfb_pci_init(struct pci_dev *dp,
struct fb_info *p;
int err;
- err = aperture_remove_conflicting_pci_devices(dp, "asiliantfb");
- if (err)
- return err;
-
if ((dp->resource[0].flags & IORESOURCE_MEM) == 0)
return -ENODEV;
addr = pci_resource_start(dp, 0);
diff --git a/drivers/video/fbdev/aty/aty128fb.c
b/drivers/video/fbdev/aty/aty128fb.c
index dd31b9d7d337..a5cb33feaf4a 100644
--- a/drivers/video/fbdev/aty/aty128fb.c
+++ b/drivers/video/fbdev/aty/aty128fb.c
@@ -47,7 +47,6 @@
*/
-#include
#include
#include
#include
@@ -2056,10 +2055,6 @@ static int aty128_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
void __iomem *bios = NULL;
#endif
- err = aperture_remove_conflicting_pci_devices(pdev, "aty1
Re: [REGRESSION] [Bug 216859] New: PCI bridge to bus boot hang at enumeration
[+cc Geert]
On Thu, Jan 26, 2023 at 06:11:24AM -0600, Bjorn Helgaas wrote:
> On Thu, Jan 12, 2023 at 02:08:19PM -0600, Bjorn Helgaas wrote:
> > On Wed, Dec 28, 2022 at 06:02:48AM -0600, Bjorn Helgaas wrote:
> > > On Wed, Dec 28, 2022 at 08:37:52AM +, bugzilla-dae...@kernel.org
> > > wrote:
> > > > https://bugzilla.kernel.org/show_bug.cgi?id=216859
> > >
> > > >Summary: PCI bridge to bus boot hang at enumeration
> > > > Kernel Version: 6.1-rc1
> > > > ...
> > >
> > > > With Kernel 6.1-rc1 the enumeration process stopped working for me,
> > > > see attachments.
> > > >
> > > > The enumeration works fine with Kernel 6.0 and below.
> > > >
> > > > Same problem still exists with v6.1. and v6.2.-rc1
>
> This is a regression between v6.0 and v6.1-rc1. Console output during
> boot freezes after nvidiafb deactivates the VGA console.
>
> It was a lot of work for Zeno, but we finally isolated this console
> hang to 145eed48de27 ("fbdev: Remove conflicting devices on PCI bus").
>
> The system actually does continue to boot and is accessible via ssh,
> but the console appears hung, at least for output. More details in
> the bugzilla starting at
> https://bugzilla.kernel.org/show_bug.cgi?id=216859#c47 .
145eed48de27 ("fbdev: Remove conflicting devices on PCI bus") doesn't
say what the benefit is, or what would break if we reverted it.
Does anybody have any clues? It would be nice to resolve this
regression before v6.2, which will probably be released 2/12 or 2/19.
Bjorn
Re: [REGRESSION] [Bug 216859] New: PCI bridge to bus boot hang at enumeration
[+cc folks from 145eed48de27 and framebuffer folks, regression list]
On Thu, Jan 12, 2023 at 02:08:19PM -0600, Bjorn Helgaas wrote:
> On Wed, Dec 28, 2022 at 06:02:48AM -0600, Bjorn Helgaas wrote:
> > On Wed, Dec 28, 2022 at 08:37:52AM +, bugzilla-dae...@kernel.org wrote:
> > > https://bugzilla.kernel.org/show_bug.cgi?id=216859
> >
> > >Summary: PCI bridge to bus boot hang at enumeration
> > > Kernel Version: 6.1-rc1
> > > ...
> >
> > > With Kernel 6.1-rc1 the enumeration process stopped working for me,
> > > see attachments.
> > >
> > > The enumeration works fine with Kernel 6.0 and below.
> > >
> > > Same problem still exists with v6.1. and v6.2.-rc1
This is a regression between v6.0 and v6.1-rc1. Console output during
boot freezes after nvidiafb deactivates the VGA console.
It was a lot of work for Zeno, but we finally isolated this console
hang to 145eed48de27 ("fbdev: Remove conflicting devices on PCI bus").
The system actually does continue to boot and is accessible via ssh,
but the console appears hung, at least for output. More details in
the bugzilla starting at
https://bugzilla.kernel.org/show_bug.cgi?id=216859#c47 .
Bjorn
Re: [PATCH v2 1/1] Docs/subsystem-apis: Remove '[The ]Linux' prefixes from titles of listed documents
On Sun, Jan 22, 2023 at 06:48:34PM +, SeongJae Park wrote: > Some documents that listed on subsystem-apis have 'Linux' or 'The Linux' > title prefixes. It's duplicated information, and makes finding the > document of interest with human eyes not easy. Remove the prefixes from > the titles. > > Signed-off-by: SeongJae Park PCI/index.rst change is fine with me: Acked-by: Bjorn Helgaas > --- > Changes from v1 > (https://lore.kernel.org/lkml/20230114194741.115855-1...@kernel.org/) > - Drop second patch (will post later for each subsystem) > > Documentation/PCI/index.rst| 6 +++--- > Documentation/cpu-freq/index.rst | 6 +++--- > Documentation/crypto/index.rst | 6 +++--- > Documentation/driver-api/index.rst | 6 +++--- > Documentation/gpu/index.rst| 6 +++--- > Documentation/hwmon/index.rst | 6 +++--- > Documentation/input/index.rst | 6 +++--- > Documentation/mm/index.rst | 6 +++--- > Documentation/peci/index.rst | 6 +++--- > Documentation/scheduler/index.rst | 6 +++--- > Documentation/scsi/index.rst | 6 +++--- > Documentation/sound/index.rst | 6 +++--- > Documentation/virt/index.rst | 6 +++--- > Documentation/watchdog/index.rst | 6 +++--- > 14 files changed, 42 insertions(+), 42 deletions(-) > > diff --git a/Documentation/PCI/index.rst b/Documentation/PCI/index.rst > index c17c87af1968..e73f84aebde3 100644 > --- a/Documentation/PCI/index.rst > +++ b/Documentation/PCI/index.rst > @@ -1,8 +1,8 @@ > .. SPDX-License-Identifier: GPL-2.0 > > -=== > -Linux PCI Bus Subsystem > -=== > += > +PCI Bus Subsystem > += > > .. toctree:: > :maxdepth: 2
Re: [Bug 216950] New: DisplayPort USB-C hub issue
On Wed, Jan 18, 2023 at 07:49:38PM +, bugzilla-dae...@kernel.org wrote: > https://bugzilla.kernel.org/show_bug.cgi?id=216950 >Summary: DisplayPort USB-C hub issue > Kernel Version: 6.2.0-rc3 > ... > Created attachment 303624 > --> https://bugzilla.kernel.org/attachment.cgi?id=303624=edit > dmidecode + kern.log > > I have HP 845 EliteBook G7 (Ryzen) and before 6.2.0-rc3 installation > there was no issue with my laptop and HP docking station via USB-C. > > This kernel display behaves weirdly. LCD monitor (connected to > docking station via DisplayPort) wakes up only once after reboot. > Second docking connection ends up with blank external screen and > only restart helps. > > Display manager detects extra screen and even newly launched apps > land there. > > I didn't have such issues with original distro kernel > 5.16.15-76051615-generic This report (which looks like a regression between 5.16.15-76051615-generic and 6.2.0-rc3) was raised against PCI, but I don't see PCI issues in the logs. Any DRM ideas? Bjorn
Re: [PATCH v2 0/8] agp: Convert to generic power management
On Tue, Oct 25, 2022 at 03:38:44PM -0500, Bjorn Helgaas wrote: > From: Bjorn Helgaas > > Vaibhav converted several AGP drivers from legacy PCI power management to > generic power management [1]. This series converts the rest of them. > > v1 posted at [2]. > > Changes from v1 to v2: > - Convert from SIMPLE_DEV_PM_OPS() (which is deprecated) to > DEFINE_SIMPLE_DEV_PM_OPS() and remove __maybe_unused annotations. > > [1] > https://lore.kernel.org/all/20210112080924.1038907-1-vaibhavgupt...@gmail.com/#t > [2] https://lore.kernel.org/all/20220607034340.307318-1-helg...@kernel.org/ > > Bjorn Helgaas (8): > agp/efficeon: Convert to generic power management > agp/intel: Convert to generic power management > agp/amd-k7: Convert to generic power management > agp/ati: Convert to generic power management > agp/nvidia: Convert to generic power management > agp/amd64: Update to DEFINE_SIMPLE_DEV_PM_OPS() > agp/sis: Update to DEFINE_SIMPLE_DEV_PM_OPS() > agp/via: Update to DEFINE_SIMPLE_DEV_PM_OPS() > > drivers/char/agp/amd-k7-agp.c | 24 > drivers/char/agp/amd64-agp.c| 6 ++ > drivers/char/agp/ati-agp.c | 22 -- > drivers/char/agp/efficeon-agp.c | 16 > drivers/char/agp/intel-agp.c| 11 +-- > drivers/char/agp/nvidia-agp.c | 24 > drivers/char/agp/sis-agp.c | 7 ++- > drivers/char/agp/via-agp.c | 6 ++ > 8 files changed, 27 insertions(+), 89 deletions(-) Applied with Dave's ack to pci/pm-agp for v6.2.
Re: [PATCH v2 0/8] agp: Convert to generic power management
On Wed, Oct 26, 2022 at 08:17:47AM +1000, Dave Airlie wrote: > On Wed, 26 Oct 2022 at 06:39, Bjorn Helgaas wrote: > > > > From: Bjorn Helgaas > > > > Vaibhav converted several AGP drivers from legacy PCI power management to > > generic power management [1]. This series converts the rest of them. > > Do you want to merge through the PCI tree? > > Acked-by: Dave Airlie Sure, will be happy to. Thanks! > > v1 posted at [2]. > > > > Changes from v1 to v2: > > - Convert from SIMPLE_DEV_PM_OPS() (which is deprecated) to > > DEFINE_SIMPLE_DEV_PM_OPS() and remove __maybe_unused annotations. > > > > [1] > > https://lore.kernel.org/all/20210112080924.1038907-1-vaibhavgupt...@gmail.com/#t > > [2] https://lore.kernel.org/all/20220607034340.307318-1-helg...@kernel.org/ > > > > Bjorn Helgaas (8): > > agp/efficeon: Convert to generic power management > > agp/intel: Convert to generic power management > > agp/amd-k7: Convert to generic power management > > agp/ati: Convert to generic power management > > agp/nvidia: Convert to generic power management > > agp/amd64: Update to DEFINE_SIMPLE_DEV_PM_OPS() > > agp/sis: Update to DEFINE_SIMPLE_DEV_PM_OPS() > > agp/via: Update to DEFINE_SIMPLE_DEV_PM_OPS() > > > > drivers/char/agp/amd-k7-agp.c | 24 > > drivers/char/agp/amd64-agp.c| 6 ++ > > drivers/char/agp/ati-agp.c | 22 -- > > drivers/char/agp/efficeon-agp.c | 16 > > drivers/char/agp/intel-agp.c| 11 +-- > > drivers/char/agp/nvidia-agp.c | 24 > > drivers/char/agp/sis-agp.c | 7 ++- > > drivers/char/agp/via-agp.c | 6 ++ > > 8 files changed, 27 insertions(+), 89 deletions(-) > > > > -- > > 2.25.1 > >
[PATCH v2 4/8] agp/ati: Convert to generic power management
From: Bjorn Helgaas
Convert agpgart-ati from legacy PCI power management to the generic power
management framework.
Previously agpgart-ati used legacy PCI power management, and
agp_ati_suspend() and agp_ati_resume() were responsible for both
device-specific things and generic PCI things like saving and restoring
config space and managing power state:
agp_ati_suspend
pci_save_state <-- generic PCI
pci_set_power_state(PCI_D3hot) <-- generic PCI
agp_ati_resume
pci_set_power_state(PCI_D0)<-- generic PCI
pci_restore_state <-- generic PCI
ati_configure <-- device-specific
With generic power management, the PCI bus PM methods do the generic PCI
things, and the driver needs only the device-specific part, i.e.,
suspend_devices_and_enter
dpm_suspend_start(PMSG_SUSPEND)
pci_pm_suspend # PCI bus .suspend() method
agp_ati_suspend<-- not needed at all; removed
suspend_enter
dpm_suspend_noirq(PMSG_SUSPEND)
pci_pm_suspend_noirq # PCI bus .suspend_noirq() method
pci_save_state <-- generic PCI
pci_prepare_to_sleep <-- generic PCI
pci_set_power_state
...
dpm_resume_end(PMSG_RESUME)
pci_pm_resume# PCI bus .resume() method
pci_restore_standard_config
pci_set_power_state(PCI_D0) <-- generic PCI
pci_restore_state<-- generic PCI
agp_ati_resume # driver->pm->resume
ati_configure<-- device-specific
Based on 0aeddbd0cb07 ("via-agp: convert to generic power management") by
Vaibhav Gupta .
Signed-off-by: Bjorn Helgaas
---
drivers/char/agp/ati-agp.c | 22 --
1 file changed, 4 insertions(+), 18 deletions(-)
diff --git a/drivers/char/agp/ati-agp.c b/drivers/char/agp/ati-agp.c
index 6f5530482d83..3c1fce48aabe 100644
--- a/drivers/char/agp/ati-agp.c
+++ b/drivers/char/agp/ati-agp.c
@@ -238,23 +238,10 @@ static int ati_configure(void)
}
-#ifdef CONFIG_PM
-static int agp_ati_suspend(struct pci_dev *dev, pm_message_t state)
+static int agp_ati_resume(struct device *dev)
{
- pci_save_state(dev);
- pci_set_power_state(dev, PCI_D3hot);
-
- return 0;
-}
-
-static int agp_ati_resume(struct pci_dev *dev)
-{
- pci_set_power_state(dev, PCI_D0);
- pci_restore_state(dev);
-
return ati_configure();
}
-#endif
/*
*Since we don't need contiguous memory we just try
@@ -559,15 +546,14 @@ static const struct pci_device_id agp_ati_pci_table[] = {
MODULE_DEVICE_TABLE(pci, agp_ati_pci_table);
+static DEFINE_SIMPLE_DEV_PM_OPS(agp_ati_pm_ops, NULL, agp_ati_resume);
+
static struct pci_driver agp_ati_pci_driver = {
.name = "agpgart-ati",
.id_table = agp_ati_pci_table,
.probe = agp_ati_probe,
.remove = agp_ati_remove,
-#ifdef CONFIG_PM
- .suspend= agp_ati_suspend,
- .resume = agp_ati_resume,
-#endif
+ .driver.pm = _ati_pm_ops,
};
static int __init agp_ati_init(void)
--
2.25.1
[PATCH v2 8/8] agp/via: Update to DEFINE_SIMPLE_DEV_PM_OPS()
From: Bjorn Helgaas
As of 1a3c7bb08826 ("PM: core: Add new *_PM_OPS macros, deprecate old
ones"), SIMPLE_DEV_PM_OPS() is deprecated in favor of
DEFINE_SIMPLE_DEV_PM_OPS(), which has the advantage that the PM callbacks
don't need to be wrapped with #ifdef CONFIG_PM or tagged with
__maybe_unused.
Convert to DEFINE_SIMPLE_DEV_PM_OPS(). No functional change intended.
Signed-off-by: Bjorn Helgaas
---
drivers/char/agp/via-agp.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/char/agp/via-agp.c b/drivers/char/agp/via-agp.c
index b2f484f527fb..bc5140af2dcb 100644
--- a/drivers/char/agp/via-agp.c
+++ b/drivers/char/agp/via-agp.c
@@ -489,9 +489,7 @@ static void agp_via_remove(struct pci_dev *pdev)
agp_put_bridge(bridge);
}
-#define agp_via_suspend NULL
-
-static int __maybe_unused agp_via_resume(struct device *dev)
+static int agp_via_resume(struct device *dev)
{
struct agp_bridge_data *bridge = dev_get_drvdata(dev);
@@ -551,7 +549,7 @@ static const struct pci_device_id agp_via_pci_table[] = {
MODULE_DEVICE_TABLE(pci, agp_via_pci_table);
-static SIMPLE_DEV_PM_OPS(agp_via_pm_ops, agp_via_suspend, agp_via_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(agp_via_pm_ops, NULL, agp_via_resume);
static struct pci_driver agp_via_pci_driver = {
.name = "agpgart-via",
--
2.25.1
[PATCH v2 6/8] agp/amd64: Update to DEFINE_SIMPLE_DEV_PM_OPS()
From: Bjorn Helgaas
As of 1a3c7bb08826 ("PM: core: Add new *_PM_OPS macros, deprecate old
ones"), SIMPLE_DEV_PM_OPS() is deprecated in favor of
DEFINE_SIMPLE_DEV_PM_OPS(), which has the advantage that the PM callbacks
don't need to be wrapped with #ifdef CONFIG_PM or tagged with
__maybe_unused.
Convert to DEFINE_SIMPLE_DEV_PM_OPS(). No functional change intended.
Signed-off-by: Bjorn Helgaas
---
drivers/char/agp/amd64-agp.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
index 84a4aa9312cf..ce8651436609 100644
--- a/drivers/char/agp/amd64-agp.c
+++ b/drivers/char/agp/amd64-agp.c
@@ -588,9 +588,7 @@ static void agp_amd64_remove(struct pci_dev *pdev)
agp_bridges_found--;
}
-#define agp_amd64_suspend NULL
-
-static int __maybe_unused agp_amd64_resume(struct device *dev)
+static int agp_amd64_resume(struct device *dev)
{
struct pci_dev *pdev = to_pci_dev(dev);
@@ -727,7 +725,7 @@ static const struct pci_device_id
agp_amd64_pci_promisc_table[] = {
{ }
};
-static SIMPLE_DEV_PM_OPS(agp_amd64_pm_ops, agp_amd64_suspend,
agp_amd64_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(agp_amd64_pm_ops, NULL, agp_amd64_resume);
static struct pci_driver agp_amd64_pci_driver = {
.name = "agpgart-amd64",
--
2.25.1
[PATCH v2 2/8] agp/intel: Convert to generic power management
From: Bjorn Helgaas
Convert agpgart-intel from legacy PCI power management to the generic power
management framework.
Previously agpgart-intel used legacy PCI power management, and
agp_intel_resume() was responsible for both device-specific things and
generic PCI things like saving and restoring config space and managing
power state.
In this case, agp_intel_suspend() was empty, and agp_intel_resume()
already did only device-specific things, so simply convert it to take a
struct device * instead of a struct pci_dev *.
Based on 0aeddbd0cb07 ("via-agp: convert to generic power management") by
Vaibhav Gupta .
Signed-off-by: Bjorn Helgaas
---
drivers/char/agp/intel-agp.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/char/agp/intel-agp.c b/drivers/char/agp/intel-agp.c
index 9e4f27a6cb5a..c518b3a9db04 100644
--- a/drivers/char/agp/intel-agp.c
+++ b/drivers/char/agp/intel-agp.c
@@ -817,16 +817,15 @@ static void agp_intel_remove(struct pci_dev *pdev)
agp_put_bridge(bridge);
}
-#ifdef CONFIG_PM
-static int agp_intel_resume(struct pci_dev *pdev)
+static int agp_intel_resume(struct device *dev)
{
+ struct pci_dev *pdev = to_pci_dev(dev);
struct agp_bridge_data *bridge = pci_get_drvdata(pdev);
bridge->driver->configure();
return 0;
}
-#endif
static const struct pci_device_id agp_intel_pci_table[] = {
#define ID(x) \
@@ -895,14 +894,14 @@ static const struct pci_device_id agp_intel_pci_table[] =
{
MODULE_DEVICE_TABLE(pci, agp_intel_pci_table);
+static DEFINE_SIMPLE_DEV_PM_OPS(agp_intel_pm_ops, NULL, agp_intel_resume);
+
static struct pci_driver agp_intel_pci_driver = {
.name = "agpgart-intel",
.id_table = agp_intel_pci_table,
.probe = agp_intel_probe,
.remove = agp_intel_remove,
-#ifdef CONFIG_PM
- .resume = agp_intel_resume,
-#endif
+ .driver.pm = _intel_pm_ops,
};
static int __init agp_intel_init(void)
--
2.25.1
[PATCH v2 0/8] agp: Convert to generic power management
From: Bjorn Helgaas Vaibhav converted several AGP drivers from legacy PCI power management to generic power management [1]. This series converts the rest of them. v1 posted at [2]. Changes from v1 to v2: - Convert from SIMPLE_DEV_PM_OPS() (which is deprecated) to DEFINE_SIMPLE_DEV_PM_OPS() and remove __maybe_unused annotations. [1] https://lore.kernel.org/all/20210112080924.1038907-1-vaibhavgupt...@gmail.com/#t [2] https://lore.kernel.org/all/20220607034340.307318-1-helg...@kernel.org/ Bjorn Helgaas (8): agp/efficeon: Convert to generic power management agp/intel: Convert to generic power management agp/amd-k7: Convert to generic power management agp/ati: Convert to generic power management agp/nvidia: Convert to generic power management agp/amd64: Update to DEFINE_SIMPLE_DEV_PM_OPS() agp/sis: Update to DEFINE_SIMPLE_DEV_PM_OPS() agp/via: Update to DEFINE_SIMPLE_DEV_PM_OPS() drivers/char/agp/amd-k7-agp.c | 24 drivers/char/agp/amd64-agp.c| 6 ++ drivers/char/agp/ati-agp.c | 22 -- drivers/char/agp/efficeon-agp.c | 16 drivers/char/agp/intel-agp.c| 11 +-- drivers/char/agp/nvidia-agp.c | 24 drivers/char/agp/sis-agp.c | 7 ++- drivers/char/agp/via-agp.c | 6 ++ 8 files changed, 27 insertions(+), 89 deletions(-) -- 2.25.1
[PATCH v2 7/8] agp/sis: Update to DEFINE_SIMPLE_DEV_PM_OPS()
From: Bjorn Helgaas
As of 1a3c7bb08826 ("PM: core: Add new *_PM_OPS macros, deprecate old
ones"), SIMPLE_DEV_PM_OPS() is deprecated in favor of
DEFINE_SIMPLE_DEV_PM_OPS(), which has the advantage that the PM callbacks
don't need to be wrapped with #ifdef CONFIG_PM or tagged with
__maybe_unused.
Convert to DEFINE_SIMPLE_DEV_PM_OPS(). No functional change intended.
Signed-off-by: Bjorn Helgaas
---
drivers/char/agp/sis-agp.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/char/agp/sis-agp.c b/drivers/char/agp/sis-agp.c
index f8a02f4bef1b..484bb101c53b 100644
--- a/drivers/char/agp/sis-agp.c
+++ b/drivers/char/agp/sis-agp.c
@@ -217,10 +217,7 @@ static void agp_sis_remove(struct pci_dev *pdev)
agp_put_bridge(bridge);
}
-#define agp_sis_suspend NULL
-
-static int __maybe_unused agp_sis_resume(
- __attribute__((unused)) struct device *dev)
+static int agp_sis_resume(__attribute__((unused)) struct device *dev)
{
return sis_driver.configure();
}
@@ -407,7 +404,7 @@ static const struct pci_device_id agp_sis_pci_table[] = {
MODULE_DEVICE_TABLE(pci, agp_sis_pci_table);
-static SIMPLE_DEV_PM_OPS(agp_sis_pm_ops, agp_sis_suspend, agp_sis_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(agp_sis_pm_ops, NULL, agp_sis_resume);
static struct pci_driver agp_sis_pci_driver = {
.name = "agpgart-sis",
--
2.25.1
[PATCH v2 5/8] agp/nvidia: Convert to generic power management
From: Bjorn Helgaas
Convert agpgart-nvidia from legacy PCI power management to the generic
power management framework.
Previously agpgart-nvidia used legacy PCI power management, and
agp_nvidia_suspend() and agp_nvidia_resume() were responsible for both
device-specific things and generic PCI things:
agp_nvidia_suspend
pci_save_state <-- generic PCI
pci_set_power_state(PCI_D3hot) <-- generic PCI
agp_nvidia_resume
pci_set_power_state(PCI_D0)<-- generic PCI
pci_restore_state <-- generic PCI
nvidia_configure <-- device-specific
Convert to generic power management where the PCI bus PM methods do the
generic PCI things, and the driver needs only the device-specific part,
i.e.,
suspend_devices_and_enter
dpm_suspend_start(PMSG_SUSPEND)
pci_pm_suspend # PCI bus .suspend() method
agp_nvidia_suspend <-- not needed at all; removed
suspend_enter
dpm_suspend_noirq(PMSG_SUSPEND)
pci_pm_suspend_noirq # PCI bus .suspend_noirq() method
pci_save_state <-- generic PCI
pci_prepare_to_sleep <-- generic PCI
pci_set_power_state
...
dpm_resume_end(PMSG_RESUME)
pci_pm_resume# PCI bus .resume() method
pci_restore_standard_config
pci_set_power_state(PCI_D0) <-- generic PCI
pci_restore_state<-- generic PCI
agp_nvidia_resume # driver->pm->resume
nvidia_configure <-- device-specific
Based on 0aeddbd0cb07 ("via-agp: convert to generic power management") by
Vaibhav Gupta .
Signed-off-by: Bjorn Helgaas
---
drivers/char/agp/nvidia-agp.c | 24
1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/drivers/char/agp/nvidia-agp.c b/drivers/char/agp/nvidia-agp.c
index 826dbd06f6bb..dbcbc06cc202 100644
--- a/drivers/char/agp/nvidia-agp.c
+++ b/drivers/char/agp/nvidia-agp.c
@@ -404,28 +404,13 @@ static void agp_nvidia_remove(struct pci_dev *pdev)
agp_put_bridge(bridge);
}
-#ifdef CONFIG_PM
-static int agp_nvidia_suspend(struct pci_dev *pdev, pm_message_t state)
+static int agp_nvidia_resume(struct device *dev)
{
- pci_save_state(pdev);
- pci_set_power_state(pdev, PCI_D3hot);
-
- return 0;
-}
-
-static int agp_nvidia_resume(struct pci_dev *pdev)
-{
- /* set power state 0 and restore PCI space */
- pci_set_power_state(pdev, PCI_D0);
- pci_restore_state(pdev);
-
/* reconfigure AGP hardware again */
nvidia_configure();
return 0;
}
-#endif
-
static const struct pci_device_id agp_nvidia_pci_table[] = {
{
@@ -449,15 +434,14 @@ static const struct pci_device_id agp_nvidia_pci_table[]
= {
MODULE_DEVICE_TABLE(pci, agp_nvidia_pci_table);
+static DEFINE_SIMPLE_DEV_PM_OPS(agp_nvidia_pm_ops, NULL, agp_nvidia_resume);
+
static struct pci_driver agp_nvidia_pci_driver = {
.name = "agpgart-nvidia",
.id_table = agp_nvidia_pci_table,
.probe = agp_nvidia_probe,
.remove = agp_nvidia_remove,
-#ifdef CONFIG_PM
- .suspend= agp_nvidia_suspend,
- .resume = agp_nvidia_resume,
-#endif
+ .driver.pm = _nvidia_pm_ops,
};
static int __init agp_nvidia_init(void)
--
2.25.1
[PATCH v2 3/8] agp/amd-k7: Convert to generic power management
From: Bjorn Helgaas
Convert agpgart-amdk7 from legacy PCI power management to the generic power
management framework.
Previously agpgart-amdk7 used legacy PCI power management, and
agp_amdk7_suspend() and agp_amdk7_resume() were responsible for both
device-specific things and generic PCI things like saving and restoring
config space and managing power state:
agp_amdk7_suspend
pci_save_state <-- generic PCI
pci_set_power_state<-- generic PCI
agp_amdk7_resume
pci_set_power_state(PCI_D0)<-- generic PCI
pci_restore_state <-- generic PCI
amd_irongate_driver.configure <-- device-specific
Convert to generic power management where the PCI bus PM methods do the
generic PCI things, and the driver needs only the device-specific part,
i.e.,
suspend_devices_and_enter
dpm_suspend_start(PMSG_SUSPEND)
pci_pm_suspend # PCI bus .suspend() method
agp_amdk7_suspend <-- not needed at all; removed
suspend_enter
dpm_suspend_noirq(PMSG_SUSPEND)
pci_pm_suspend_noirq # PCI bus .suspend_noirq() method
pci_save_state <-- generic PCI
pci_prepare_to_sleep <-- generic PCI
pci_set_power_state
...
dpm_resume_end(PMSG_RESUME)
pci_pm_resume# PCI bus .resume() method
pci_restore_standard_config
pci_set_power_state(PCI_D0) <-- generic PCI
pci_restore_state<-- generic PCI
agp_amdk7_resume # driver->pm->resume
amd_irongate_driver.configure<-- device-specific
Based on 0aeddbd0cb07 ("via-agp: convert to generic power management") by
Vaibhav Gupta .
Signed-off-by: Bjorn Helgaas
---
drivers/char/agp/amd-k7-agp.c | 24
1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/drivers/char/agp/amd-k7-agp.c b/drivers/char/agp/amd-k7-agp.c
index 2b2095542816..55397ba765d2 100644
--- a/drivers/char/agp/amd-k7-agp.c
+++ b/drivers/char/agp/amd-k7-agp.c
@@ -488,26 +488,11 @@ static void agp_amdk7_remove(struct pci_dev *pdev)
agp_put_bridge(bridge);
}
-#ifdef CONFIG_PM
-
-static int agp_amdk7_suspend(struct pci_dev *pdev, pm_message_t state)
+static int agp_amdk7_resume(struct device *dev)
{
- pci_save_state(pdev);
- pci_set_power_state(pdev, pci_choose_state(pdev, state));
-
- return 0;
-}
-
-static int agp_amdk7_resume(struct pci_dev *pdev)
-{
- pci_set_power_state(pdev, PCI_D0);
- pci_restore_state(pdev);
-
return amd_irongate_driver.configure();
}
-#endif /* CONFIG_PM */
-
/* must be the same order as name table above */
static const struct pci_device_id agp_amdk7_pci_table[] = {
{
@@ -539,15 +524,14 @@ static const struct pci_device_id agp_amdk7_pci_table[] =
{
MODULE_DEVICE_TABLE(pci, agp_amdk7_pci_table);
+static DEFINE_SIMPLE_DEV_PM_OPS(agp_amdk7_pm_ops, NULL, agp_amdk7_resume);
+
static struct pci_driver agp_amdk7_pci_driver = {
.name = "agpgart-amdk7",
.id_table = agp_amdk7_pci_table,
.probe = agp_amdk7_probe,
.remove = agp_amdk7_remove,
-#ifdef CONFIG_PM
- .suspend= agp_amdk7_suspend,
- .resume = agp_amdk7_resume,
-#endif
+ .driver.pm = _amdk7_pm_ops,
};
static int __init agp_amdk7_init(void)
--
2.25.1
[PATCH v2 1/8] agp/efficeon: Convert to generic power management
From: Bjorn Helgaas
Convert agpgart-efficeon from legacy PCI power management to the generic
power management framework.
Previously agpgart-efficeon used legacy PCI power management, which means
agp_efficeon_suspend() and agp_efficeon_resume() were responsible for both
device-specific things and generic PCI things like saving and restoring
config space and managing power state.
In this case, agp_efficeon_suspend() was empty, and agp_efficeon_resume()
already did only device-specific things, so simply convert it to take a
struct device * instead of a struct pci_dev *.
Based on 0aeddbd0cb07 ("via-agp: convert to generic power management") by
Vaibhav Gupta .
Signed-off-by: Bjorn Helgaas
---
drivers/char/agp/efficeon-agp.c | 16
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/char/agp/efficeon-agp.c b/drivers/char/agp/efficeon-agp.c
index c53f0f9ef5b0..f28d42319269 100644
--- a/drivers/char/agp/efficeon-agp.c
+++ b/drivers/char/agp/efficeon-agp.c
@@ -412,18 +412,11 @@ static void agp_efficeon_remove(struct pci_dev *pdev)
agp_put_bridge(bridge);
}
-#ifdef CONFIG_PM
-static int agp_efficeon_suspend(struct pci_dev *dev, pm_message_t state)
-{
- return 0;
-}
-
-static int agp_efficeon_resume(struct pci_dev *pdev)
+static int agp_efficeon_resume(struct device *dev)
{
printk(KERN_DEBUG PFX "agp_efficeon_resume()\n");
return efficeon_configure();
}
-#endif
static const struct pci_device_id agp_efficeon_pci_table[] = {
{
@@ -437,6 +430,8 @@ static const struct pci_device_id agp_efficeon_pci_table[]
= {
{ }
};
+static DEFINE_SIMPLE_DEV_PM_OPS(agp_efficeon_pm_ops, NULL,
agp_efficeon_resume);
+
MODULE_DEVICE_TABLE(pci, agp_efficeon_pci_table);
static struct pci_driver agp_efficeon_pci_driver = {
@@ -444,10 +439,7 @@ static struct pci_driver agp_efficeon_pci_driver = {
.id_table = agp_efficeon_pci_table,
.probe = agp_efficeon_probe,
.remove = agp_efficeon_remove,
-#ifdef CONFIG_PM
- .suspend= agp_efficeon_suspend,
- .resume = agp_efficeon_resume,
-#endif
+ .driver.pm = _efficeon_pm_ops,
};
static int __init agp_efficeon_init(void)
--
2.25.1
Re: [PATCH v2 2/2] pci_ids: Add the various Microsoft PCI device IDs
Please follow the PCI subject line conventions. Discover it with
"git log --oneline include/linux/pci_ids.h".
On Fri, Sep 09, 2022 at 11:50:25AM -0700, Easwar Hariharan wrote:
> From: Easwar Hariharan
>
Needs a commit log, even if it is nothing more than the subject line.
Also read the top of include/linux/pci_ids.h, because it looks like
some of these are only used in one driver and hence do not need to be
in pci_ids.h.
> Signed-off-by: Easwar Hariharan
> ---
> drivers/gpu/drm/hyperv/hyperv_drm_drv.c | 2 +-
> drivers/net/ethernet/microsoft/mana/gdma.h | 3 ---
> drivers/net/ethernet/microsoft/mana/gdma_main.c | 6 +++---
> drivers/video/fbdev/hyperv_fb.c | 4 ++--
> include/linux/pci_ids.h | 4 +++-
> 5 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> index f84d397..24c2def 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> @@ -51,7 +51,7 @@ static void hyperv_pci_remove(struct pci_dev *pdev)
> static const struct pci_device_id hyperv_pci_tbl[] = {
> {
> .vendor = PCI_VENDOR_ID_MICROSOFT,
> - .device = PCI_DEVICE_ID_HYPERV_VIDEO,
> + .device = PCI_DEVICE_ID_MICROSOFT_HYPERV_VIDEO,
> },
> { /* end of list */ }
> };
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma.h
> b/drivers/net/ethernet/microsoft/mana/gdma.h
> index 4a6efe6..9d3a9f7 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma.h
> +++ b/drivers/net/ethernet/microsoft/mana/gdma.h
> @@ -476,9 +476,6 @@ struct gdma_eqe {
>
> #define GDMA_SRIOV_REG_CFG_BASE_OFF 0x108
>
> -#define MANA_PF_DEVICE_ID 0x00B9
> -#define MANA_VF_DEVICE_ID 0x00BA
> -
> struct gdma_posted_wqe_info {
> u32 wqe_size_in_bu;
> };
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 00d8198..18cf168 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -1333,7 +1333,7 @@ static void mana_gd_cleanup(struct pci_dev *pdev)
>
> static bool mana_is_pf(unsigned short dev_id)
> {
> - return dev_id == MANA_PF_DEVICE_ID;
> + return dev_id == PCI_DEVICE_ID_MICROSOFT_MANA_PF;
> }
>
> static int mana_gd_probe(struct pci_dev *pdev, const struct pci_device_id
> *ent)
> @@ -1466,8 +1466,8 @@ static void mana_gd_shutdown(struct pci_dev *pdev)
> }
>
> static const struct pci_device_id mana_id_table[] = {
> - { PCI_DEVICE(PCI_VENDOR_ID_MICROSOFT, MANA_PF_DEVICE_ID) },
> - { PCI_DEVICE(PCI_VENDOR_ID_MICROSOFT, MANA_VF_DEVICE_ID) },
> + { PCI_DEVICE(PCI_VENDOR_ID_MICROSOFT, PCI_DEVICE_ID_MICROSOFT_MANA_PF)
> },
> + { PCI_DEVICE(PCI_VENDOR_ID_MICROSOFT, PCI_DEVICE_ID_MICROSOFT_MANA_VF)
> },
> { }
> };
>
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index b58b445..118e244 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -997,7 +997,7 @@ static int hvfb_getmem(struct hv_device *hdev, struct
> fb_info *info)
>
> if (!gen2vm) {
> pdev = pci_get_device(PCI_VENDOR_ID_MICROSOFT,
> - PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
> + PCI_DEVICE_ID_MICROSOFT_HYPERV_VIDEO, NULL);
> if (!pdev) {
> pr_err("Unable to find PCI Hyper-V video\n");
> return -ENODEV;
> @@ -1311,7 +1311,7 @@ static int hvfb_resume(struct hv_device *hdev)
> static const struct pci_device_id pci_stub_id_table[] = {
> {
> .vendor = PCI_VENDOR_ID_MICROSOFT,
> - .device = PCI_DEVICE_ID_HYPERV_VIDEO,
> + .device = PCI_DEVICE_ID_MICROSOFT_HYPERV_VIDEO,
> },
> { /* end of list */ }
> };
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 15b49e6..fe3517f 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2080,7 +2080,9 @@
> #define PCI_DEVICE_ID_VT1724 0x1724
>
> #define PCI_VENDOR_ID_MICROSOFT 0x1414
> -#define PCI_DEVICE_ID_HYPERV_VIDEO 0x5353
> +#define PCI_DEVICE_ID_MICROSOFT_HYPERV_VIDEO 0x5353
> +#define PCI_DEVICE_ID_MICROSOFT_MANA_PF 0x00B9
> +#define PCI_DEVICE_ID_MICROSOFT_MANA_VF 0x00BA
>
> #define PCI_VENDOR_ID_OXSEMI 0x1415
> #define PCI_DEVICE_ID_OXSEMI_12PCI8400x8403
> --
> 1.8.3.1
>
Re: [PATCH v3 1/3] PCI: Move PCI_VENDOR_ID_MICROSOFT/PCI_DEVICE_ID_HYPERV_VIDEO definitions to pci_ids.h
On Sat, Aug 27, 2022 at 03:03:43PM +0200, Vitaly Kuznetsov wrote: > There are already three places in kernel which define PCI_VENDOR_ID_MICROSOFT > and two for PCI_DEVICE_ID_HYPERV_VIDEO and there's a need to use these > from core Vmbus code. Move the defines where they belong. It's a minor annoyance that the above is 81 characters long when "git log" adds its 4-character indent, so it wraps in a default terminal. It'd be nice if we could settle on a conventional spelling of "Vmbus", too. "Vmbus" looks to be in the minority: $ git grep Vmbus | wc -l; git grep VMbus | wc -l; git grep VMBus | wc -l 4 82 62 FWIW, one published microsoft.com doc uses "VMBus": https://docs.microsoft.com/en-us/virtualization/hyper-v-on-windows/reference/hyper-v-architecture
Re: [PATCH v2 1/3] PCI: Move PCI_VENDOR_ID_MICROSOFT/PCI_DEVICE_ID_HYPERV_VIDEO definitions to pci_ids.h
On Thu, Aug 25, 2022 at 11:00:22AM +0200, Vitaly Kuznetsov wrote:
> There are already three places in kernel which define PCI_VENDOR_ID_MICROSOFT
> and two for PCI_DEVICE_ID_HYPERV_VIDEO and there's a need to use these
> from core Vmbus code. Move the defines where they belong.
>
> No functional change.
>
> Signed-off-by: Vitaly Kuznetsov
Acked-by: Bjorn Helgaas# pci_ids.h
> ---
> drivers/gpu/drm/hyperv/hyperv_drm_drv.c | 3 ---
> drivers/net/ethernet/microsoft/mana/gdma_main.c | 4
> drivers/video/fbdev/hyperv_fb.c | 4
> include/linux/pci_ids.h | 3 +++
> 4 files changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> index 6d11e7938c83..40888e36f91a 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> @@ -23,9 +23,6 @@
> #define DRIVER_MAJOR 1
> #define DRIVER_MINOR 0
>
> -#define PCI_VENDOR_ID_MICROSOFT 0x1414
> -#define PCI_DEVICE_ID_HYPERV_VIDEO 0x5353
> -
> DEFINE_DRM_GEM_FOPS(hv_fops);
>
> static struct drm_driver hyperv_driver = {
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 5f9240182351..00d8198072ae 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -1465,10 +1465,6 @@ static void mana_gd_shutdown(struct pci_dev *pdev)
> pci_disable_device(pdev);
> }
>
> -#ifndef PCI_VENDOR_ID_MICROSOFT
> -#define PCI_VENDOR_ID_MICROSOFT 0x1414
> -#endif
> -
> static const struct pci_device_id mana_id_table[] = {
> { PCI_DEVICE(PCI_VENDOR_ID_MICROSOFT, MANA_PF_DEVICE_ID) },
> { PCI_DEVICE(PCI_VENDOR_ID_MICROSOFT, MANA_VF_DEVICE_ID) },
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index 886c564787f1..b58b445bb529 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -74,10 +74,6 @@
> #define SYNTHVID_DEPTH_WIN8 32
> #define SYNTHVID_FB_SIZE_WIN8 (8 * 1024 * 1024)
>
> -#define PCI_VENDOR_ID_MICROSOFT 0x1414
> -#define PCI_DEVICE_ID_HYPERV_VIDEO 0x5353
> -
> -
> enum pipe_msg_type {
> PIPE_MSG_INVALID,
> PIPE_MSG_DATA,
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 6feade66efdb..15b49e655ce3 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2079,6 +2079,9 @@
> #define PCI_DEVICE_ID_ICE_1712 0x1712
> #define PCI_DEVICE_ID_VT1724 0x1724
>
> +#define PCI_VENDOR_ID_MICROSOFT 0x1414
> +#define PCI_DEVICE_ID_HYPERV_VIDEO 0x5353
> +
> #define PCI_VENDOR_ID_OXSEMI 0x1415
> #define PCI_DEVICE_ID_OXSEMI_12PCI8400x8403
> #define PCI_DEVICE_ID_OXSEMI_PCIe840 0xC000
> --
> 2.37.1
>
Re: [PATCH 0/2] video: fbdev: Convert from PCI to generic power management
On Wed, Jun 08, 2022 at 06:26:34PM +0200, Daniel Vetter wrote: > On Tue, Jun 07, 2022 at 06:11:10PM -0500, Bjorn Helgaas wrote: > > From: Bjorn Helgaas > > > > PCI-specific power management (pci_driver.suspend and pci_driver.resume) is > > deprecated. If drivers implement power management, they should use the > > generic power management framework, not the PCI-specific hooks. > > > > No fbdev drivers actually implement PCI power management, but there are a > > cirrusfb has some commented-out references to it and skeletonfb has > > examples of it. Remove these. > > Is this holding up some cleanup on your side and so would be easier to > merge these through the pci tree? If so > > Acked-by: Daniel Vetter > > for merging through your tree. Otherwise I guess Helge will get around to > pile them up for 5.20 (or 6.0) eventually. No particular rush and nothing depending on these. I added your ack to my local branch and if nothing happens for a while, I'll merge them via my tree. Bjorn
[PATCH 2/2] video: fbdev: skeletonfb: Convert to generic power management
From: Bjorn Helgaas
PCI-specific power management (pci_driver.suspend and pci_driver.resume) is
deprecated. If drivers implement power management, they should use the
generic power management framework, not the PCI-specific hooks.
Convert the sample code to use the generic power management framework.
Signed-off-by: Bjorn Helgaas
---
drivers/video/fbdev/skeletonfb.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/video/fbdev/skeletonfb.c b/drivers/video/fbdev/skeletonfb.c
index bcacfb6934fa..70ba78450837 100644
--- a/drivers/video/fbdev/skeletonfb.c
+++ b/drivers/video/fbdev/skeletonfb.c
@@ -838,9 +838,9 @@ static void xxxfb_remove(struct pci_dev *dev)
*
* See Documentation/driver-api/pm/devices.rst for more information
*/
-static int xxxfb_suspend(struct pci_dev *dev, pm_message_t msg)
+static int xxxfb_suspend(struct device *dev)
{
- struct fb_info *info = pci_get_drvdata(dev);
+ struct fb_info *info = dev_get_drvdata(dev);
struct xxxfb_par *par = info->par;
/* suspend here */
@@ -853,9 +853,9 @@ static int xxxfb_suspend(struct pci_dev *dev, pm_message_t
msg)
*
* See Documentation/driver-api/pm/devices.rst for more information
*/
-static int xxxfb_resume(struct pci_dev *dev)
+static int xxxfb_resume(struct device *dev)
{
- struct fb_info *info = pci_get_drvdata(dev);
+ struct fb_info *info = dev_get_drvdata(dev);
struct xxxfb_par *par = info->par;
/* resume here */
@@ -873,14 +873,15 @@ static const struct pci_device_id xxxfb_id_table[] = {
{ 0, }
};
+static SIMPLE_DEV_PM_OPS(xxxfb_pm_ops, xxxfb_suspend, xxxfb_resume);
+
/* For PCI drivers */
static struct pci_driver xxxfb_driver = {
.name = "xxxfb",
.id_table = xxxfb_id_table,
.probe =xxxfb_probe,
.remove = xxxfb_remove,
- .suspend = xxxfb_suspend, /* optional but recommended */
- .resume = xxxfb_resume, /* optional but recommended */
+ .driver.pm =xxxfb_pm_ops, /* optional but recommended */
};
MODULE_DEVICE_TABLE(pci, xxxfb_id_table);
--
2.25.1
[PATCH 1/2] video: fbdev: cirrusfb: Remove useless reference to PCI power management
From: Bjorn Helgaas
PCI-specific power management (pci_driver.suspend and pci_driver.resume) is
deprecated. The cirrusfb driver has never implemented power management at
all, but if it ever does, it should use the generic power management
framework, not the PCI-specific hooks.
Remove the commented-out references to the PCI-specific power management
hooks.
Signed-off-by: Bjorn Helgaas
---
drivers/video/fbdev/cirrusfb.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/video/fbdev/cirrusfb.c b/drivers/video/fbdev/cirrusfb.c
index 3d47c347b897..51e072c03e1c 100644
--- a/drivers/video/fbdev/cirrusfb.c
+++ b/drivers/video/fbdev/cirrusfb.c
@@ -2184,12 +2184,6 @@ static struct pci_driver cirrusfb_pci_driver = {
.id_table = cirrusfb_pci_table,
.probe = cirrusfb_pci_register,
.remove = cirrusfb_pci_unregister,
-#ifdef CONFIG_PM
-#if 0
- .suspend= cirrusfb_pci_suspend,
- .resume = cirrusfb_pci_resume,
-#endif
-#endif
};
#endif /* CONFIG_PCI */
--
2.25.1
[PATCH 0/2] video: fbdev: Convert from PCI to generic power management
From: Bjorn Helgaas PCI-specific power management (pci_driver.suspend and pci_driver.resume) is deprecated. If drivers implement power management, they should use the generic power management framework, not the PCI-specific hooks. No fbdev drivers actually implement PCI power management, but there are a cirrusfb has some commented-out references to it and skeletonfb has examples of it. Remove these. Bjorn Helgaas (2): video: fbdev: cirrusfb: Remove useless reference to PCI power management video: fbdev: skeletonfb: Convert to generic power management drivers/video/fbdev/cirrusfb.c | 6 -- drivers/video/fbdev/skeletonfb.c | 13 +++-- 2 files changed, 7 insertions(+), 12 deletions(-) -- 2.25.1
Re: [Bug 215958] New: thunderbolt3 egpu cannot disconnect cleanly
On Sun, May 8, 2022 at 3:29 PM wrote: > > https://bugzilla.kernel.org/show_bug.cgi?id=215958 > > Bug ID: 215958 >Summary: thunderbolt3 egpu cannot disconnect cleanly >Product: Drivers >Version: 2.5 > Kernel Version: 5.17.0-1003-oem #3-Ubuntu SMP PREEMPT > Hardware: All > OS: Linux > Tree: Mainline > Status: NEW > Severity: normal > Priority: P1 > Component: PCI > Assignee: drivers_...@kernel-bugs.osdl.org > Reporter: r087...@yahoo.it > Regression: No I assume this is not a regression, right? If it is a regression, what previous kernel worked correctly? > I have an external egpu (Radeon 6600 RX) connected through thunderbolt3 to my > Thinkpad X1 carbon 6th Gen.. When I disconnect the thunderbolt3 cable I get > the > following error in dmesg: > > [21874.194994] amdgpu :0c:00.0: amdgpu: SMU: response:0x for > index:18 param:0x0005 message:TransferTableSmu2Dram? > [21874.195006] amdgpu :0c:00.0: amdgpu: Failed to export SMU metrics > table! > [21874.195123] amdgpu :0c:00.0: amdgpu: SMU: response:0x for > index:18 param:0x0005 message:TransferTableSmu2Dram? > [21874.195129] amdgpu :0c:00.0: amdgpu: Failed to export SMU metrics > table! > [21874.195271] amdgpu :0c:00.0: amdgpu: SMU: response:0x for > index:18 param:0x0005 message:TransferTableSmu2Dram? > [21874.195276] amdgpu :0c:00.0: amdgpu: Failed to export SMU metrics > table! > [21874.195406] amdgpu :0c:00.0: amdgpu: SMU: response:0x for > index:18 param:0x0005 message:TransferTableSmu2Dram? > [21874.195411] amdgpu :0c:00.0: amdgpu: Failed to export SMU metrics > table! > [21874.195544] amdgpu :0c:00.0: amdgpu: SMU: response:0x for > index:51 param:0x message:GetPptLimit? > [21874.195550] amdgpu :0c:00.0: amdgpu: > [smu_v11_0_get_current_power_limit] > get PPT limit failed! > [21874.195582] amdgpu :0c:00.0: amdgpu: SMU: response:0x for > index:18 param:0x0005 message:TransferTableSmu2Dram? > [21874.195587] amdgpu :0c:00.0: amdgpu: Failed to export SMU metrics > table! > [21874.227454] amdgpu :0c:00.0: amdgpu: SMU: response:0x for > index:18 param:0x0005 message:TransferTableSmu2Dram? > [21874.227463] amdgpu :0c:00.0: amdgpu: Failed to export SMU metrics > table! > [21874.227532] amdgpu :0c:00.0: amdgpu: SMU: response:0x for > index:18 param:0x0005 message:TransferTableSmu2Dram? > [21874.227536] amdgpu :0c:00.0: amdgpu: Failed to export SMU metrics > table! > [21874.227618] amdgpu :0c:00.0: amdgpu: SMU: response:0x for > index:18 param:0x0005 message:TransferTableSmu2Dram? > [21874.227621] amdgpu :0c:00.0: amdgpu: Failed to export SMU metrics > table! > [21874.227700] amdgpu :0c:00.0: amdgpu: SMU: response:0x for > index:18 param:0x0005 message:TransferTableSmu2Dram? > [21874.227703] amdgpu :0c:00.0: amdgpu: Failed to export SMU metrics > table! > [21874.227784] amdgpu :0c:00.0: amdgpu: > [smu_v11_0_get_current_power_limit] > get PPT limit failed! > [21874.227804] amdgpu :0c:00.0: amdgpu: Failed to export SMU metrics > table! > [21874.514661] snd_hda_codec_hdmi hdaudioC1D0: Unable to sync register > 0x2f0d00. -5 > [21874.568360] amdgpu :0c:00.0: amdgpu: Failed to switch to AC mode! > [21874.599292] amdgpu :0c:00.0: amdgpu: Failed to switch to AC mode! > [21874.718562] amdgpu :0c:00.0: amdgpu: amdgpu: finishing device. > [21878.722376] amdgpu: cp queue pipe 4 queue 0 preemption failed > [21878.722422] amdgpu :0c:00.0: amdgpu: Failed to disable gfxoff! > [21879.134918] amdgpu :0c:00.0: [drm:amdgpu_ring_test_helper [amdgpu]] > *ERROR* ring kiq_2.1.0 test failed (-110) > [21879.135144] [drm:gfx_v10_0_hw_fini [amdgpu]] *ERROR* KGQ disable failed > [21879.338158] amdgpu :0c:00.0: [drm:amdgpu_ring_test_helper [amdgpu]] > *ERROR* ring kiq_2.1.0 test failed (-110) > [21879.338402] [drm:gfx_v10_0_hw_fini [amdgpu]] *ERROR* KCQ disable failed > [21879.543318] [drm:gfx_v10_0_cp_gfx_enable.isra.0 [amdgpu]] *ERROR* failed to > halt cp gfx > [21879.544216] __smu_cmn_reg_print_error: 5 callbacks suppressed > [21879.544220] amdgpu :0c:00.0: amdgpu: SMU: response:0x for > index:7 param:0x message:DisableAllSmuFeatures? > [21879.544226] amdgpu :0c:00.0: amdgpu: Failed to disable smu features. > [21879.544230] amdgpu :0c:00.0: amdgpu: Fail to disable dpm features! > [21879.544238] [drm] free PSP TMR buffer The above looks like what amdgpu would see when the GPU is no longer accessible (writes are dropped and reads return 0x). It's possible amdgpu could notice this and shut down more gracefully, but I don't think it's the main problem here and it probably wouldn't force you to reboot. > [21880.455935] i915 :00:02.0: vgaarb:
RX 5500 XT: PCIe link speed stuck at Gen1 2.5GT/s by default
See https://gitlab.freedesktop.org/drm/amd/-/issues/1447 This report was against DRM, but I'm mentioning it here because we've been talking about some link speed init issues lately, and AFAICT the gitlab reports don't show up anywhere in lore. Robert Hancock reported: > I'm using a RX 5500 XT card on an Asus PRIME H270-PRO motherboard, > Intel i5-7500 CPU, with kernel 5.10.9 under Fedora 33. I noticed > that in Linux, "lspci -vv" always showed the GPU PCIe link running > at 2.5GT/s link speed and never seemed to change regardless of the > application being run, while in Windows, GPU-Z shows the link > running at the max supported 8GT/s speed when under graphical load. > > It seems like the driver thinks that 2.5GT/s is the max allowable > speed, based on the pp_dpm_pcie file: > > > cd > /sys/devices/pci:00/:00:01.0/:01:00.0/:02:00.0/:03:00.0/ > > cat pp_dpm_pcie > 0: 2.5GT/s, x8 81Mhz * > 1: 2.5GT/s, x8 619Mhz * > > I'm assuming that something is going wrong with the PCIe link speed > detection in the driver. Using the "amdgpu.pcie_gen_cap=0x70007" > kernel command line option seems to result in the driver detecting > the proper 8GT/s maximum speed. > > lspci -vv output from booting without overriding the speed is > attached.
Re: [PATCH v9 00/11] vgaarb: Rework default VGA device selection
On Fri, Feb 25, 2022 at 04:15:23PM -0600, Bjorn Helgaas wrote: > On Thu, Feb 24, 2022 at 04:47:42PM -0600, Bjorn Helgaas wrote: > > From: Bjorn Helgaas > > > > Current default VGA device selection fails in some cases because part of it > > is done in the vga_arb_device_init() subsys_initcall, and some arches > > enumerate PCI devices in pcibios_init(), which runs *after* that. > > > > The big change from the v8 posting is that this moves vgaarb.c from > > drivers/gpu/vga to drivers/pci because it really has nothing to do with > > GPUs or DRM. > > I provisionally applied this to pci/vga and put it into -next just > to get a little runtime on it. > > But I'd prefer not to unilaterally yank this out of drivers/gpu > without a consensus from the GPU folks that this is the right thing to > do. > > Any thoughts? If it seems OK to you, I think patch 1/11 (the move > itself) is all you would need to look at, although of course I would > still be grateful for any review and feedback on the rest. > > After it's in drivers/pci, all the blame for any issues would come my > way. Ping? This has been in -next since the Feb 28 tree, and I plan to ask Linus to include it during the v5.18 merge window (which will probably open Mar 13) unless somebody objects. Bjorn
Re: [PATCH] vgaarbiter: fix vgaarbiter doc build break
On Tue, Mar 01, 2022 at 11:29:09AM -0800, t...@redhat.com wrote:
> From: Tom Rix
>
> make htmldocs fails with
> Error: Cannot open file ./drivers/gpu/vga/vgaarb.c
>
> The location of the file changed
> drivers/gpu/vga/vgaarb.c -> drivers/pci/vgaarb.c
> So update the docs with the new location.
>
> Fixes: d6e1898bfa5b ("PCI/VGA: Move vgaarb to drivers/pci")
> Signed-off-by: Tom Rix
Fixed, thanks!
> ---
> Documentation/gpu/vgaarbiter.rst | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/gpu/vgaarbiter.rst
> b/Documentation/gpu/vgaarbiter.rst
> index 339ed5fecd2e4..bde3c0afb0590 100644
> --- a/Documentation/gpu/vgaarbiter.rst
> +++ b/Documentation/gpu/vgaarbiter.rst
> @@ -100,7 +100,7 @@ In-kernel interface
> .. kernel-doc:: include/linux/vgaarb.h
> :internal:
>
> -.. kernel-doc:: drivers/gpu/vga/vgaarb.c
> +.. kernel-doc:: drivers/pci/vgaarb.c
> :export:
>
> libpciaccess
> --
> 2.26.3
>
