[V3,09/29] drm/i915: deprecate pci_get_bus_and_slot()

Message ID 1511801886-6753-10-git-send-email-okaya@codeaurora.org
State Not Applicable
Delegated to: Bjorn Helgaas
Headers show
Series
  • PCI: deprecate pci_get_bus_and_slot()
Related show

Commit Message

Sinan Kaya Nov. 27, 2017, 4:57 p.m.
pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as
where a PCI device is present. This restricts the device drivers to be
reused for other domain numbers.

Getting ready to remove pci_get_bus_and_slot() function in favor of
pci_get_domain_bus_and_slot().

Extract the domain number from drm_device and pass it into
pci_get_domain_bus_and_slot() function.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/gpu/drm/i915/i915_drv.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Sinan Kaya Nov. 27, 2017, 6:50 p.m. | #1
+dri-devel@lists.freedesktop.org

On 11/27/2017 11:57 AM, Sinan Kaya wrote:
> pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as
> where a PCI device is present. This restricts the device drivers to be
> reused for other domain numbers.
> 
> Getting ready to remove pci_get_bus_and_slot() function in favor of
> pci_get_domain_bus_and_slot().
> 
> Extract the domain number from drm_device and pass it into
> pci_get_domain_bus_and_slot() function.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 9f45cfe..5a8cb79 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -419,7 +419,10 @@ static int i915_getparam(struct drm_device *dev, void *data,
>  
>  static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
>  {
> -	dev_priv->bridge_dev = pci_get_bus_and_slot(0, PCI_DEVFN(0, 0));
> +	int domain = pci_domain_nr(dev_priv->drm.pdev->bus);
> +
> +	dev_priv->bridge_dev =
> +		pci_get_domain_bus_and_slot(domain, 0, PCI_DEVFN(0, 0));
>  	if (!dev_priv->bridge_dev) {
>  		DRM_ERROR("bridge device not found\n");
>  		return -1;
>
Ville Syrjala Nov. 28, 2017, 3:30 p.m. | #2
On Mon, Nov 27, 2017 at 11:57:46AM -0500, Sinan Kaya wrote:
> pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as
> where a PCI device is present. This restricts the device drivers to be
> reused for other domain numbers.
> 
> Getting ready to remove pci_get_bus_and_slot() function in favor of
> pci_get_domain_bus_and_slot().
> 
> Extract the domain number from drm_device and pass it into
> pci_get_domain_bus_and_slot() function.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 9f45cfe..5a8cb79 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -419,7 +419,10 @@ static int i915_getparam(struct drm_device *dev, void *data,
>  
>  static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
>  {
> -	dev_priv->bridge_dev = pci_get_bus_and_slot(0, PCI_DEVFN(0, 0));
> +	int domain = pci_domain_nr(dev_priv->drm.pdev->bus);
> +
> +	dev_priv->bridge_dev =
> +		pci_get_domain_bus_and_slot(domain, 0, PCI_DEVFN(0, 0));

Maybe just pci_get_slot(pdev->bus, PCI_DEVFN(0, 0)) ?

I guess if we want to be pedantic we could go for:

bus = pci_find_host_bridge(pdev->bus)->bus;
pci_get_slot(bus, PCI_DEVFN(0, 0))

but I think the GPU should always be on the root bus, so the simpler
form should be fine.

>  	if (!dev_priv->bridge_dev) {
>  		DRM_ERROR("bridge device not found\n");
>  		return -1;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Sinan Kaya Nov. 28, 2017, 4:29 p.m. | #3
On 11/28/2017 10:30 AM, Ville Syrjälä wrote:
>> +	dev_priv->bridge_dev =
>> +		pci_get_domain_bus_and_slot(domain, 0, PCI_DEVFN(0, 0));
> Maybe just pci_get_slot(pdev->bus, PCI_DEVFN(0, 0)) ?
> 
> I guess if we want to be pedantic we could go for:
> 
> bus = pci_find_host_bridge(pdev->bus)->bus;
> pci_get_slot(bus, PCI_DEVFN(0, 0))
> 
> but I think the GPU should always be on the root bus, so the simpler
> form should be fine.
> 

All three of these should be correct. 

I'll use pci_get_slot(pdev->bus, PCI_DEVFN(0, 0)) as you suggested.
Sinan Kaya Dec. 3, 2017, 7:29 p.m. | #4
On 11/28/2017 11:29 AM, Sinan Kaya wrote:
> On 11/28/2017 10:30 AM, Ville Syrjälä wrote:
>>> +	dev_priv->bridge_dev =
>>> +		pci_get_domain_bus_and_slot(domain, 0, PCI_DEVFN(0, 0));
>> Maybe just pci_get_slot(pdev->bus, PCI_DEVFN(0, 0)) ?
>>
>> I guess if we want to be pedantic we could go for:
>>
>> bus = pci_find_host_bridge(pdev->bus)->bus;
>> pci_get_slot(bus, PCI_DEVFN(0, 0))
>>
>> but I think the GPU should always be on the root bus, so the simpler
>> form should be fine.
>>
> 
> All three of these should be correct. 
> 
> I'll use pci_get_slot(pdev->bus, PCI_DEVFN(0, 0)) as you suggested.
> 

Now that I think about this more, I think my version is a simpler change
and does not introduce "new features" by assuming GPU and host to be
on the same bus similar to the original code. 

Original code could have used pci_get_slot() too. Since all of them are
correct, mine is slightly more correct; I'd like to keep mine.
Joonas Lahtinen Dec. 12, 2017, 2:04 p.m. | #5
Hi,

I sent this individual i915 patch to our CI, and it is passing on all platforms:

https://patchwork.freedesktop.org/series/34822/

Is it ok if I merge this to drm-tip already?

Regards, Joonas

On Mon, 2017-11-27 at 13:50 -0500, Sinan Kaya wrote:
> +dri-devel@lists.freedesktop.org
> 
> On 11/27/2017 11:57 AM, Sinan Kaya wrote:
> > pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as
> > where a PCI device is present. This restricts the device drivers to be
> > reused for other domain numbers.
> > 
> > Getting ready to remove pci_get_bus_and_slot() function in favor of
> > pci_get_domain_bus_and_slot().
> > 
> > Extract the domain number from drm_device and pass it into
> > pci_get_domain_bus_and_slot() function.
> > 
> > Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 9f45cfe..5a8cb79 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -419,7 +419,10 @@ static int i915_getparam(struct drm_device *dev, void *data,
> >  
> >  static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
> >  {
> > -	dev_priv->bridge_dev = pci_get_bus_and_slot(0, PCI_DEVFN(0, 0));
> > +	int domain = pci_domain_nr(dev_priv->drm.pdev->bus);
> > +
> > +	dev_priv->bridge_dev =
> > +		pci_get_domain_bus_and_slot(domain, 0, PCI_DEVFN(0, 0));
> >  	if (!dev_priv->bridge_dev) {
> >  		DRM_ERROR("bridge device not found\n");
> >  		return -1;
> > 
> 
>
Sinan Kaya Dec. 13, 2017, 12:07 a.m. | #6
On 12/12/2017 9:04 AM, Joonas Lahtinen wrote:
> Hi,
> 
> I sent this individual i915 patch to our CI, and it is passing on all platforms:
> 
> https://patchwork.freedesktop.org/series/34822/
> 
> Is it ok if I merge this to drm-tip already?

As long as you have this change in your tree, it should be safe.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/include/linux/pci.h?id=7912af5c835bd86f2b0347a480e0f40e2fab30d0


> 
> Regards, Joonas
>
Joonas Lahtinen Dec. 13, 2017, 8:01 a.m. | #7
On Tue, 2017-12-12 at 19:07 -0500, Sinan Kaya wrote:
> On 12/12/2017 9:04 AM, Joonas Lahtinen wrote:
> > Hi,
> > 
> > I sent this individual i915 patch to our CI, and it is passing on
> > all platforms:
> > 
> > https://patchwork.freedesktop.org/series/34822/
> > 
> > Is it ok if I merge this to drm-tip already?
> 
> As long as you have this change in your tree, it should be safe.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/include/linux/pci.h?id=7912af5c835bd86f2b0347a480e0f40e2fab30d0
> 

We don't yet.

Rodrigo, can you please pull the above patch in once we get a
backmerge?

Regards, Joonas
Bjorn Helgaas Feb. 16, 2018, 2:54 p.m. | #8
On Mon, Nov 27, 2017 at 11:57:46AM -0500, Sinan Kaya wrote:
> pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as
> where a PCI device is present. This restricts the device drivers to be
> reused for other domain numbers.
> 
> Getting ready to remove pci_get_bus_and_slot() function in favor of
> pci_get_domain_bus_and_slot().
> 
> Extract the domain number from drm_device and pass it into
> pci_get_domain_bus_and_slot() function.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>

I don't know what happened to this, and it didn't make it into
v4.16-rc1.  I applied it to pci/deprecate-get-bus-and-slot for v4.17
along with the patch that actually removes pci_get_bus_and_slot().

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 9f45cfe..5a8cb79 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -419,7 +419,10 @@ static int i915_getparam(struct drm_device *dev, void *data,
>  
>  static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
>  {
> -	dev_priv->bridge_dev = pci_get_bus_and_slot(0, PCI_DEVFN(0, 0));
> +	int domain = pci_domain_nr(dev_priv->drm.pdev->bus);
> +
> +	dev_priv->bridge_dev =
> +		pci_get_domain_bus_and_slot(domain, 0, PCI_DEVFN(0, 0));
>  	if (!dev_priv->bridge_dev) {
>  		DRM_ERROR("bridge device not found\n");
>  		return -1;
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Jani Nikula Feb. 19, 2018, 9:34 a.m. | #9
On Fri, 16 Feb 2018, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Mon, Nov 27, 2017 at 11:57:46AM -0500, Sinan Kaya wrote:
>> pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as
>> where a PCI device is present. This restricts the device drivers to be
>> reused for other domain numbers.
>> 
>> Getting ready to remove pci_get_bus_and_slot() function in favor of
>> pci_get_domain_bus_and_slot().
>> 
>> Extract the domain number from drm_device and pass it into
>> pci_get_domain_bus_and_slot() function.
>> 
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>
> I don't know what happened to this, and it didn't make it into
> v4.16-rc1.  I applied it to pci/deprecate-get-bus-and-slot for v4.17
> along with the patch that actually removes pci_get_bus_and_slot().

It fell between the cracks as we couldn't apply it before getting a
backmerge on the dependency. Sorry about that.

Ack for merging through your tree.

Thanks,
Jani.


>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 9f45cfe..5a8cb79 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -419,7 +419,10 @@ static int i915_getparam(struct drm_device *dev, void *data,
>>  
>>  static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
>>  {
>> -	dev_priv->bridge_dev = pci_get_bus_and_slot(0, PCI_DEVFN(0, 0));
>> +	int domain = pci_domain_nr(dev_priv->drm.pdev->bus);
>> +
>> +	dev_priv->bridge_dev =
>> +		pci_get_domain_bus_and_slot(domain, 0, PCI_DEVFN(0, 0));
>>  	if (!dev_priv->bridge_dev) {
>>  		DRM_ERROR("bridge device not found\n");
>>  		return -1;
>> -- 
>> 1.9.1
>> 
>> 
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Joonas Lahtinen Feb. 19, 2018, 9:47 a.m. | #10
Quoting Jani Nikula (2018-02-19 11:34:34)
> On Fri, 16 Feb 2018, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Mon, Nov 27, 2017 at 11:57:46AM -0500, Sinan Kaya wrote:
> >> pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as
> >> where a PCI device is present. This restricts the device drivers to be
> >> reused for other domain numbers.
> >> 
> >> Getting ready to remove pci_get_bus_and_slot() function in favor of
> >> pci_get_domain_bus_and_slot().
> >> 
> >> Extract the domain number from drm_device and pass it into
> >> pci_get_domain_bus_and_slot() function.
> >> 
> >> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> >
> > I don't know what happened to this, and it didn't make it into
> > v4.16-rc1.  I applied it to pci/deprecate-get-bus-and-slot for v4.17
> > along with the patch that actually removes pci_get_bus_and_slot().
> 
> It fell between the cracks as we couldn't apply it before getting a
> backmerge on the dependency. Sorry about that.
> 
> Ack for merging through your tree.

I just retested the patch and it still passes CI. We also now have the
dependency in our tree through the backmerge, so I can send this for the
next drm-next pull request. Either way suits me.

Regards, Joonas

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 9f45cfe..5a8cb79 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -419,7 +419,10 @@  static int i915_getparam(struct drm_device *dev, void *data,
 
 static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
 {
-	dev_priv->bridge_dev = pci_get_bus_and_slot(0, PCI_DEVFN(0, 0));
+	int domain = pci_domain_nr(dev_priv->drm.pdev->bus);
+
+	dev_priv->bridge_dev =
+		pci_get_domain_bus_and_slot(domain, 0, PCI_DEVFN(0, 0));
 	if (!dev_priv->bridge_dev) {
 		DRM_ERROR("bridge device not found\n");
 		return -1;