drm/amd & radeon deadcoding

[PATCH 2/4] drm/radeon: Remove unused radeon_doorbell_free

Posted by linux@treblig.org 8 months ago

From: "Dr. David Alan Gilbert" <linux@treblig.org>

radeon_doorbell_free() was added in 2013 by
commit 75efdee11b5d ("drm/radeon: implement simple doorbell page
allocator")
but never used.

Remove it.

Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org>
---
 drivers/gpu/drm/radeon/radeon.h        |  1 -
 drivers/gpu/drm/radeon/radeon_device.c | 14 --------------
 2 files changed, 15 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 8605c074d9f7..58111fdf520d 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -686,7 +686,6 @@ struct radeon_doorbell {
 };
 
 int radeon_doorbell_get(struct radeon_device *rdev, u32 *page);
-void radeon_doorbell_free(struct radeon_device *rdev, u32 doorbell);
 
 /*
  * IRQS.
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index bbd39348a7ab..4127ffb4bb6f 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -392,20 +392,6 @@ int radeon_doorbell_get(struct radeon_device *rdev, u32 *doorbell)
 	}
 }
 
-/**
- * radeon_doorbell_free - Free a doorbell entry
- *
- * @rdev: radeon_device pointer
- * @doorbell: doorbell index
- *
- * Free a doorbell allocated for use by the driver (all asics)
- */
-void radeon_doorbell_free(struct radeon_device *rdev, u32 doorbell)
-{
-	if (doorbell < rdev->doorbell.num_doorbells)
-		__clear_bit(doorbell, rdev->doorbell.used);
-}
-
 /*
  * radeon_wb_*()
  * Writeback is the method by which the GPU updates special pages
-- 
2.49.0

Re: [PATCH 2/4] drm/radeon: Remove unused radeon_doorbell_free

Posted by Christophe JAILLET 8 months ago

Le 18/04/2025 à 02:21, linux@treblig.org a écrit :
> From: "Dr. David Alan Gilbert" <linux@treblig.org>
> 
> radeon_doorbell_free() was added in 2013 by
> commit 75efdee11b5d ("drm/radeon: implement simple doorbell page
> allocator")
> but never used.

Hi,

I think than instead of being removed, it should be used in the error 
handling path of cik_init() and in cik_fini().

CJ

> 
> Remove it.
> 
> Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org>
> ---
>   drivers/gpu/drm/radeon/radeon.h        |  1 -
>   drivers/gpu/drm/radeon/radeon_device.c | 14 --------------
>   2 files changed, 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> index 8605c074d9f7..58111fdf520d 100644
> --- a/drivers/gpu/drm/radeon/radeon.h
> +++ b/drivers/gpu/drm/radeon/radeon.h
> @@ -686,7 +686,6 @@ struct radeon_doorbell {
>   };
>   
>   int radeon_doorbell_get(struct radeon_device *rdev, u32 *page);
> -void radeon_doorbell_free(struct radeon_device *rdev, u32 doorbell);
>   
>   /*
>    * IRQS.
> diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
> index bbd39348a7ab..4127ffb4bb6f 100644
> --- a/drivers/gpu/drm/radeon/radeon_device.c
> +++ b/drivers/gpu/drm/radeon/radeon_device.c
> @@ -392,20 +392,6 @@ int radeon_doorbell_get(struct radeon_device *rdev, u32 *doorbell)
>   	}
>   }
>   
> -/**
> - * radeon_doorbell_free - Free a doorbell entry
> - *
> - * @rdev: radeon_device pointer
> - * @doorbell: doorbell index
> - *
> - * Free a doorbell allocated for use by the driver (all asics)
> - */
> -void radeon_doorbell_free(struct radeon_device *rdev, u32 doorbell)
> -{
> -	if (doorbell < rdev->doorbell.num_doorbells)
> -		__clear_bit(doorbell, rdev->doorbell.used);
> -}
> -
>   /*
>    * radeon_wb_*()
>    * Writeback is the method by which the GPU updates special pages

Re: [PATCH 2/4] drm/radeon: Remove unused radeon_doorbell_free

Posted by Dr. David Alan Gilbert 7 months, 1 week ago

* Christophe JAILLET (christophe.jaillet@wanadoo.fr) wrote:
> Le 18/04/2025 à 02:21, linux@treblig.org a écrit :
> > From: "Dr. David Alan Gilbert" <linux@treblig.org>
> > 
> > radeon_doorbell_free() was added in 2013 by
> > commit 75efdee11b5d ("drm/radeon: implement simple doorbell page
> > allocator")
> > but never used.
> 
> Hi,
> 
> I think than instead of being removed, it should be used in the error
> handling path of cik_init() and in cik_fini().

OK, here's an RFC that builds; but if I understand correctly only
some devices run this code, and I'm not sure mine does;

Thoughts?

Dave

From 15b3830b4ee3eb819f86198dcbc596428f9ee0d0 Mon Sep 17 00:00:00 2001
From: "Dr. David Alan Gilbert" <linux@treblig.org>
Date: Sun, 11 May 2025 02:35:41 +0100
Subject: [RFC PATCH] drm/radeon/cik: Clean up doorbells

Free doorbells in the error paths of cik_init and in cik_fini.

Suggested-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org>
---
 drivers/gpu/drm/radeon/cik.c | 38 ++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
index 11a492f21157..3caa5a100f97 100644
--- a/drivers/gpu/drm/radeon/cik.c
+++ b/drivers/gpu/drm/radeon/cik.c
@@ -8548,7 +8548,7 @@ int cik_suspend(struct radeon_device *rdev)
  */
 int cik_init(struct radeon_device *rdev)
 {
-	struct radeon_ring *ring;
+	struct radeon_ring *ring, *ringCP1, *ringCP2;
 	int r;
 
 	/* Read BIOS */
@@ -8623,19 +8623,22 @@ int cik_init(struct radeon_device *rdev)
 	ring->ring_obj = NULL;
 	r600_ring_init(rdev, ring, 1024 * 1024);
 
-	ring = &rdev->ring[CAYMAN_RING_TYPE_CP1_INDEX];
-	ring->ring_obj = NULL;
-	r600_ring_init(rdev, ring, 1024 * 1024);
-	r = radeon_doorbell_get(rdev, &ring->doorbell_index);
+	ringCP1 = &rdev->ring[CAYMAN_RING_TYPE_CP1_INDEX];
+	ringCP2 = &rdev->ring[CAYMAN_RING_TYPE_CP2_INDEX];
+	ringCP1->ring_obj = NULL;
+	ringCP2->ring_obj = NULL;
+	ringCP1->doorbell_index = RADEON_MAX_DOORBELLS;
+	ringCP2->doorbell_index = RADEON_MAX_DOORBELLS;
+
+	r600_ring_init(rdev, ringCP1, 1024 * 1024);
+	r = radeon_doorbell_get(rdev, &ringCP1->doorbell_index);
 	if (r)
 		return r;
 
-	ring = &rdev->ring[CAYMAN_RING_TYPE_CP2_INDEX];
-	ring->ring_obj = NULL;
-	r600_ring_init(rdev, ring, 1024 * 1024);
-	r = radeon_doorbell_get(rdev, &ring->doorbell_index);
+	r600_ring_init(rdev, ringCP2, 1024 * 1024);
+	r = radeon_doorbell_get(rdev, &ringCP2->doorbell_index);
 	if (r)
-		return r;
+		goto out;
 
 	ring = &rdev->ring[R600_RING_TYPE_DMA_INDEX];
 	ring->ring_obj = NULL;
@@ -8653,7 +8656,7 @@ int cik_init(struct radeon_device *rdev)
 
 	r = r600_pcie_gart_init(rdev);
 	if (r)
-		return r;
+		goto out;
 
 	rdev->accel_working = true;
 	r = cik_startup(rdev);
@@ -8678,10 +8681,16 @@ int cik_init(struct radeon_device *rdev)
 	 */
 	if (!rdev->mc_fw && !(rdev->flags & RADEON_IS_IGP)) {
 		DRM_ERROR("radeon: MC ucode required for NI+.\n");
-		return -EINVAL;
+		r = -EINVAL;
+		goto out;
 	}
 
 	return 0;
+
+out:
+	radeon_doorbell_free(rdev, ringCP1->doorbell_index);
+	radeon_doorbell_free(rdev, ringCP2->doorbell_index);
+	return r;
 }
 
 /**
@@ -8695,6 +8704,7 @@ int cik_init(struct radeon_device *rdev)
  */
 void cik_fini(struct radeon_device *rdev)
 {
+	struct radeon_ring *ring;
 	radeon_pm_fini(rdev);
 	cik_cp_fini(rdev);
 	cik_sdma_fini(rdev);
@@ -8708,6 +8718,10 @@ void cik_fini(struct radeon_device *rdev)
 	radeon_ib_pool_fini(rdev);
 	radeon_irq_kms_fini(rdev);
 	uvd_v1_0_fini(rdev);
+	ring = &rdev->ring[CAYMAN_RING_TYPE_CP1_INDEX];
+	radeon_doorbell_free(rdev, ring->doorbell_index);
+	ring = &rdev->ring[CAYMAN_RING_TYPE_CP2_INDEX];
+	radeon_doorbell_free(rdev, ring->doorbell_index);
 	radeon_uvd_fini(rdev);
 	radeon_vce_fini(rdev);
 	cik_pcie_gart_fini(rdev);
-- 
2.49.0


-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 2/4] drm/radeon: Remove unused radeon_doorbell_free

Posted by Alex Deucher 7 months ago

On Sun, May 11, 2025 at 8:13 AM Dr. David Alan Gilbert
<linux@treblig.org> wrote:
>
> * Christophe JAILLET (christophe.jaillet@wanadoo.fr) wrote:
> > Le 18/04/2025 à 02:21, linux@treblig.org a écrit :
> > > From: "Dr. David Alan Gilbert" <linux@treblig.org>
> > >
> > > radeon_doorbell_free() was added in 2013 by
> > > commit 75efdee11b5d ("drm/radeon: implement simple doorbell page
> > > allocator")
> > > but never used.
> >
> > Hi,
> >
> > I think than instead of being removed, it should be used in the error
> > handling path of cik_init() and in cik_fini().
>
> OK, here's an RFC that builds; but if I understand correctly only
> some devices run this code, and I'm not sure mine does;
>
> Thoughts?
>
> Dave
>
> From 15b3830b4ee3eb819f86198dcbc596428f9ee0d0 Mon Sep 17 00:00:00 2001
> From: "Dr. David Alan Gilbert" <linux@treblig.org>
> Date: Sun, 11 May 2025 02:35:41 +0100
> Subject: [RFC PATCH] drm/radeon/cik: Clean up doorbells
>
> Free doorbells in the error paths of cik_init and in cik_fini.
>
> Suggested-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org>
> ---
>  drivers/gpu/drm/radeon/cik.c | 38 ++++++++++++++++++++++++------------
>  1 file changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
> index 11a492f21157..3caa5a100f97 100644
> --- a/drivers/gpu/drm/radeon/cik.c
> +++ b/drivers/gpu/drm/radeon/cik.c
> @@ -8548,7 +8548,7 @@ int cik_suspend(struct radeon_device *rdev)
>   */
>  int cik_init(struct radeon_device *rdev)
>  {
> -       struct radeon_ring *ring;
> +       struct radeon_ring *ring, *ringCP1, *ringCP2;

I'd prefer ring_cp1 and ring_cp2 for readability.

>         int r;
>
>         /* Read BIOS */
> @@ -8623,19 +8623,22 @@ int cik_init(struct radeon_device *rdev)
>         ring->ring_obj = NULL;
>         r600_ring_init(rdev, ring, 1024 * 1024);
>
> -       ring = &rdev->ring[CAYMAN_RING_TYPE_CP1_INDEX];
> -       ring->ring_obj = NULL;
> -       r600_ring_init(rdev, ring, 1024 * 1024);
> -       r = radeon_doorbell_get(rdev, &ring->doorbell_index);
> +       ringCP1 = &rdev->ring[CAYMAN_RING_TYPE_CP1_INDEX];
> +       ringCP2 = &rdev->ring[CAYMAN_RING_TYPE_CP2_INDEX];
> +       ringCP1->ring_obj = NULL;
> +       ringCP2->ring_obj = NULL;
> +       ringCP1->doorbell_index = RADEON_MAX_DOORBELLS;
> +       ringCP2->doorbell_index = RADEON_MAX_DOORBELLS;
> +
> +       r600_ring_init(rdev, ringCP1, 1024 * 1024);
> +       r = radeon_doorbell_get(rdev, &ringCP1->doorbell_index);
>         if (r)
>                 return r;
>
> -       ring = &rdev->ring[CAYMAN_RING_TYPE_CP2_INDEX];
> -       ring->ring_obj = NULL;
> -       r600_ring_init(rdev, ring, 1024 * 1024);
> -       r = radeon_doorbell_get(rdev, &ring->doorbell_index);
> +       r600_ring_init(rdev, ringCP2, 1024 * 1024);
> +       r = radeon_doorbell_get(rdev, &ringCP2->doorbell_index);
>         if (r)
> -               return r;
> +               goto out;
>
>         ring = &rdev->ring[R600_RING_TYPE_DMA_INDEX];
>         ring->ring_obj = NULL;
> @@ -8653,7 +8656,7 @@ int cik_init(struct radeon_device *rdev)
>
>         r = r600_pcie_gart_init(rdev);
>         if (r)
> -               return r;
> +               goto out;
>
>         rdev->accel_working = true;
>         r = cik_startup(rdev);

I think you can drop the doorbells in the error case for cik_startup()
as well since they won't be used in that case.

Alex

> @@ -8678,10 +8681,16 @@ int cik_init(struct radeon_device *rdev)
>          */
>         if (!rdev->mc_fw && !(rdev->flags & RADEON_IS_IGP)) {
>                 DRM_ERROR("radeon: MC ucode required for NI+.\n");
> -               return -EINVAL;
> +               r = -EINVAL;
> +               goto out;
>         }
>
>         return 0;
> +
> +out:
> +       radeon_doorbell_free(rdev, ringCP1->doorbell_index);
> +       radeon_doorbell_free(rdev, ringCP2->doorbell_index);
> +       return r;
>  }
>
>  /**
> @@ -8695,6 +8704,7 @@ int cik_init(struct radeon_device *rdev)
>   */
>  void cik_fini(struct radeon_device *rdev)
>  {
> +       struct radeon_ring *ring;
>         radeon_pm_fini(rdev);
>         cik_cp_fini(rdev);
>         cik_sdma_fini(rdev);
> @@ -8708,6 +8718,10 @@ void cik_fini(struct radeon_device *rdev)
>         radeon_ib_pool_fini(rdev);
>         radeon_irq_kms_fini(rdev);
>         uvd_v1_0_fini(rdev);
> +       ring = &rdev->ring[CAYMAN_RING_TYPE_CP1_INDEX];
> +       radeon_doorbell_free(rdev, ring->doorbell_index);
> +       ring = &rdev->ring[CAYMAN_RING_TYPE_CP2_INDEX];
> +       radeon_doorbell_free(rdev, ring->doorbell_index);
>         radeon_uvd_fini(rdev);
>         radeon_vce_fini(rdev);
>         cik_pcie_gart_fini(rdev);
> --
> 2.49.0
>
>
> --
>  -----Open up your eyes, open up your mind, open up your code -------
> / Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \
> \        dave @ treblig.org |                               | In Hex /
>  \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 2/4] drm/radeon: Remove unused radeon_doorbell_free

Posted by Dr. David Alan Gilbert 7 months ago

* Alex Deucher (alexdeucher@gmail.com) wrote:
> On Sun, May 11, 2025 at 8:13 AM Dr. David Alan Gilbert
> <linux@treblig.org> wrote:
> >
> > * Christophe JAILLET (christophe.jaillet@wanadoo.fr) wrote:
> > > Le 18/04/2025 à 02:21, linux@treblig.org a écrit :
> > > > From: "Dr. David Alan Gilbert" <linux@treblig.org>
> > > >
> > > > radeon_doorbell_free() was added in 2013 by
> > > > commit 75efdee11b5d ("drm/radeon: implement simple doorbell page
> > > > allocator")
> > > > but never used.
> > >
> > > Hi,
> > >
> > > I think than instead of being removed, it should be used in the error
> > > handling path of cik_init() and in cik_fini().
> >
> > OK, here's an RFC that builds; but if I understand correctly only
> > some devices run this code, and I'm not sure mine does;
> >
> > Thoughts?
> >
> > Dave
> >
> > From 15b3830b4ee3eb819f86198dcbc596428f9ee0d0 Mon Sep 17 00:00:00 2001
> > From: "Dr. David Alan Gilbert" <linux@treblig.org>
> > Date: Sun, 11 May 2025 02:35:41 +0100
> > Subject: [RFC PATCH] drm/radeon/cik: Clean up doorbells
> >
> > Free doorbells in the error paths of cik_init and in cik_fini.
> >
> > Suggested-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org>
> > ---
> >  drivers/gpu/drm/radeon/cik.c | 38 ++++++++++++++++++++++++------------
> >  1 file changed, 26 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
> > index 11a492f21157..3caa5a100f97 100644
> > --- a/drivers/gpu/drm/radeon/cik.c
> > +++ b/drivers/gpu/drm/radeon/cik.c
> > @@ -8548,7 +8548,7 @@ int cik_suspend(struct radeon_device *rdev)
> >   */
> >  int cik_init(struct radeon_device *rdev)
> >  {
> > -       struct radeon_ring *ring;
> > +       struct radeon_ring *ring, *ringCP1, *ringCP2;
> 
> I'd prefer ring_cp1 and ring_cp2 for readability.

OK.

> >         int r;
> >
> >         /* Read BIOS */
> > @@ -8623,19 +8623,22 @@ int cik_init(struct radeon_device *rdev)
> >         ring->ring_obj = NULL;
> >         r600_ring_init(rdev, ring, 1024 * 1024);
> >
> > -       ring = &rdev->ring[CAYMAN_RING_TYPE_CP1_INDEX];
> > -       ring->ring_obj = NULL;
> > -       r600_ring_init(rdev, ring, 1024 * 1024);
> > -       r = radeon_doorbell_get(rdev, &ring->doorbell_index);
> > +       ringCP1 = &rdev->ring[CAYMAN_RING_TYPE_CP1_INDEX];
> > +       ringCP2 = &rdev->ring[CAYMAN_RING_TYPE_CP2_INDEX];
> > +       ringCP1->ring_obj = NULL;
> > +       ringCP2->ring_obj = NULL;
> > +       ringCP1->doorbell_index = RADEON_MAX_DOORBELLS;
> > +       ringCP2->doorbell_index = RADEON_MAX_DOORBELLS;
> > +
> > +       r600_ring_init(rdev, ringCP1, 1024 * 1024);
> > +       r = radeon_doorbell_get(rdev, &ringCP1->doorbell_index);
> >         if (r)
> >                 return r;
> >
> > -       ring = &rdev->ring[CAYMAN_RING_TYPE_CP2_INDEX];
> > -       ring->ring_obj = NULL;
> > -       r600_ring_init(rdev, ring, 1024 * 1024);
> > -       r = radeon_doorbell_get(rdev, &ring->doorbell_index);
> > +       r600_ring_init(rdev, ringCP2, 1024 * 1024);
> > +       r = radeon_doorbell_get(rdev, &ringCP2->doorbell_index);
> >         if (r)
> > -               return r;
> > +               goto out;
> >
> >         ring = &rdev->ring[R600_RING_TYPE_DMA_INDEX];
> >         ring->ring_obj = NULL;
> > @@ -8653,7 +8656,7 @@ int cik_init(struct radeon_device *rdev)
> >
> >         r = r600_pcie_gart_init(rdev);
> >         if (r)
> > -               return r;
> > +               goto out;
> >
> >         rdev->accel_working = true;
> >         r = cik_startup(rdev);
> 
> I think you can drop the doorbells in the error case for cik_startup()
> as well since they won't be used in that case.

OK, can do that.

Thanks,

Dave

> Alex
> 
> > @@ -8678,10 +8681,16 @@ int cik_init(struct radeon_device *rdev)
> >          */
> >         if (!rdev->mc_fw && !(rdev->flags & RADEON_IS_IGP)) {
> >                 DRM_ERROR("radeon: MC ucode required for NI+.\n");
> > -               return -EINVAL;
> > +               r = -EINVAL;
> > +               goto out;
> >         }
> >
> >         return 0;
> > +
> > +out:
> > +       radeon_doorbell_free(rdev, ringCP1->doorbell_index);
> > +       radeon_doorbell_free(rdev, ringCP2->doorbell_index);
> > +       return r;
> >  }
> >
> >  /**
> > @@ -8695,6 +8704,7 @@ int cik_init(struct radeon_device *rdev)
> >   */
> >  void cik_fini(struct radeon_device *rdev)
> >  {
> > +       struct radeon_ring *ring;
> >         radeon_pm_fini(rdev);
> >         cik_cp_fini(rdev);
> >         cik_sdma_fini(rdev);
> > @@ -8708,6 +8718,10 @@ void cik_fini(struct radeon_device *rdev)
> >         radeon_ib_pool_fini(rdev);
> >         radeon_irq_kms_fini(rdev);
> >         uvd_v1_0_fini(rdev);
> > +       ring = &rdev->ring[CAYMAN_RING_TYPE_CP1_INDEX];
> > +       radeon_doorbell_free(rdev, ring->doorbell_index);
> > +       ring = &rdev->ring[CAYMAN_RING_TYPE_CP2_INDEX];
> > +       radeon_doorbell_free(rdev, ring->doorbell_index);
> >         radeon_uvd_fini(rdev);
> >         radeon_vce_fini(rdev);
> >         cik_pcie_gart_fini(rdev);
> > --
> > 2.49.0
> >
> >
> > --
> >  -----Open up your eyes, open up your mind, open up your code -------
> > / Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \
> > \        dave @ treblig.org |                               | In Hex /
> >  \ _________________________|_____ http://www.treblig.org   |_______/
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 2/4] drm/radeon: Remove unused radeon_doorbell_free

Posted by Dr. David Alan Gilbert 7 months ago

* Dr. David Alan Gilbert (linux@treblig.org) wrote:
> * Alex Deucher (alexdeucher@gmail.com) wrote:
> > On Sun, May 11, 2025 at 8:13 AM Dr. David Alan Gilbert
> > <linux@treblig.org> wrote:
> > >
> > > * Christophe JAILLET (christophe.jaillet@wanadoo.fr) wrote:
> > > > Le 18/04/2025 à 02:21, linux@treblig.org a écrit :
> > > > > From: "Dr. David Alan Gilbert" <linux@treblig.org>
> > > > >
> > > > > radeon_doorbell_free() was added in 2013 by
> > > > > commit 75efdee11b5d ("drm/radeon: implement simple doorbell page
> > > > > allocator")
> > > > > but never used.
> > > >
> > > > Hi,
> > > >
> > > > I think than instead of being removed, it should be used in the error
> > > > handling path of cik_init() and in cik_fini().
> > >
> > > OK, here's an RFC that builds; but if I understand correctly only
> > > some devices run this code, and I'm not sure mine does;
> > >
> > > Thoughts?
> > >
> > > Dave
> > >
> > > From 15b3830b4ee3eb819f86198dcbc596428f9ee0d0 Mon Sep 17 00:00:00 2001
> > > From: "Dr. David Alan Gilbert" <linux@treblig.org>
> > > Date: Sun, 11 May 2025 02:35:41 +0100
> > > Subject: [RFC PATCH] drm/radeon/cik: Clean up doorbells
> > >
> > > Free doorbells in the error paths of cik_init and in cik_fini.
> > >
> > > Suggested-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org>
> > > ---
> > >  drivers/gpu/drm/radeon/cik.c | 38 ++++++++++++++++++++++++------------
> > >  1 file changed, 26 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
> > > index 11a492f21157..3caa5a100f97 100644
> > > --- a/drivers/gpu/drm/radeon/cik.c
> > > +++ b/drivers/gpu/drm/radeon/cik.c
> > > @@ -8548,7 +8548,7 @@ int cik_suspend(struct radeon_device *rdev)
> > >   */
> > >  int cik_init(struct radeon_device *rdev)
> > >  {
> > > -       struct radeon_ring *ring;
> > > +       struct radeon_ring *ring, *ringCP1, *ringCP2;
> > 
> > I'd prefer ring_cp1 and ring_cp2 for readability.
> 
> OK.
> 
> > >         int r;
> > >
> > >         /* Read BIOS */
> > > @@ -8623,19 +8623,22 @@ int cik_init(struct radeon_device *rdev)
> > >         ring->ring_obj = NULL;
> > >         r600_ring_init(rdev, ring, 1024 * 1024);
> > >
> > > -       ring = &rdev->ring[CAYMAN_RING_TYPE_CP1_INDEX];
> > > -       ring->ring_obj = NULL;
> > > -       r600_ring_init(rdev, ring, 1024 * 1024);
> > > -       r = radeon_doorbell_get(rdev, &ring->doorbell_index);
> > > +       ringCP1 = &rdev->ring[CAYMAN_RING_TYPE_CP1_INDEX];
> > > +       ringCP2 = &rdev->ring[CAYMAN_RING_TYPE_CP2_INDEX];
> > > +       ringCP1->ring_obj = NULL;
> > > +       ringCP2->ring_obj = NULL;
> > > +       ringCP1->doorbell_index = RADEON_MAX_DOORBELLS;
> > > +       ringCP2->doorbell_index = RADEON_MAX_DOORBELLS;
> > > +
> > > +       r600_ring_init(rdev, ringCP1, 1024 * 1024);
> > > +       r = radeon_doorbell_get(rdev, &ringCP1->doorbell_index);
> > >         if (r)
> > >                 return r;
> > >
> > > -       ring = &rdev->ring[CAYMAN_RING_TYPE_CP2_INDEX];
> > > -       ring->ring_obj = NULL;
> > > -       r600_ring_init(rdev, ring, 1024 * 1024);
> > > -       r = radeon_doorbell_get(rdev, &ring->doorbell_index);
> > > +       r600_ring_init(rdev, ringCP2, 1024 * 1024);
> > > +       r = radeon_doorbell_get(rdev, &ringCP2->doorbell_index);
> > >         if (r)
> > > -               return r;
> > > +               goto out;
> > >
> > >         ring = &rdev->ring[R600_RING_TYPE_DMA_INDEX];
> > >         ring->ring_obj = NULL;
> > > @@ -8653,7 +8656,7 @@ int cik_init(struct radeon_device *rdev)
> > >
> > >         r = r600_pcie_gart_init(rdev);
> > >         if (r)
> > > -               return r;
> > > +               goto out;
> > >
> > >         rdev->accel_working = true;
> > >         r = cik_startup(rdev);
> > 
> > I think you can drop the doorbells in the error case for cik_startup()
> > as well since they won't be used in that case.
> 
> OK, can do that.

V1 sent as message 20250514011610.136607-1-linux@treblig.org 
Note this is still only build tested by me; so needs someone with
relevant hardware to give it a smoke test.

Dave

> Thanks,
> 
> Dave
> 
> > Alex
> > 
> > > @@ -8678,10 +8681,16 @@ int cik_init(struct radeon_device *rdev)
> > >          */
> > >         if (!rdev->mc_fw && !(rdev->flags & RADEON_IS_IGP)) {
> > >                 DRM_ERROR("radeon: MC ucode required for NI+.\n");
> > > -               return -EINVAL;
> > > +               r = -EINVAL;
> > > +               goto out;
> > >         }
> > >
> > >         return 0;
> > > +
> > > +out:
> > > +       radeon_doorbell_free(rdev, ringCP1->doorbell_index);
> > > +       radeon_doorbell_free(rdev, ringCP2->doorbell_index);
> > > +       return r;
> > >  }
> > >
> > >  /**
> > > @@ -8695,6 +8704,7 @@ int cik_init(struct radeon_device *rdev)
> > >   */
> > >  void cik_fini(struct radeon_device *rdev)
> > >  {
> > > +       struct radeon_ring *ring;
> > >         radeon_pm_fini(rdev);
> > >         cik_cp_fini(rdev);
> > >         cik_sdma_fini(rdev);
> > > @@ -8708,6 +8718,10 @@ void cik_fini(struct radeon_device *rdev)
> > >         radeon_ib_pool_fini(rdev);
> > >         radeon_irq_kms_fini(rdev);
> > >         uvd_v1_0_fini(rdev);
> > > +       ring = &rdev->ring[CAYMAN_RING_TYPE_CP1_INDEX];
> > > +       radeon_doorbell_free(rdev, ring->doorbell_index);
> > > +       ring = &rdev->ring[CAYMAN_RING_TYPE_CP2_INDEX];
> > > +       radeon_doorbell_free(rdev, ring->doorbell_index);
> > >         radeon_uvd_fini(rdev);
> > >         radeon_vce_fini(rdev);
> > >         cik_pcie_gart_fini(rdev);
> > > --
> > > 2.49.0
> > >
> > >
> > > --
> > >  -----Open up your eyes, open up your mind, open up your code -------
> > > / Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \
> > > \        dave @ treblig.org |                               | In Hex /
> > >  \ _________________________|_____ http://www.treblig.org   |_______/
> -- 
>  -----Open up your eyes, open up your mind, open up your code -------   
> / Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
> \        dave @ treblig.org |                               | In Hex /
>  \ _________________________|_____ http://www.treblig.org   |_______/
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 2/4] drm/radeon: Remove unused radeon_doorbell_free

Posted by Alex Deucher 7 months, 4 weeks ago

On Fri, Apr 18, 2025 at 2:18 AM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Le 18/04/2025 à 02:21, linux@treblig.org a écrit :
> > From: "Dr. David Alan Gilbert" <linux@treblig.org>
> >
> > radeon_doorbell_free() was added in 2013 by
> > commit 75efdee11b5d ("drm/radeon: implement simple doorbell page
> > allocator")
> > but never used.
>
> Hi,
>
> I think than instead of being removed, it should be used in the error
> handling path of cik_init() and in cik_fini().

Yes, ideally.  Care to make a patch to fix that?

Thanks,

Alex

>
> CJ
>
> >
> > Remove it.
> >
> > Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org>
> > ---
> >   drivers/gpu/drm/radeon/radeon.h        |  1 -
> >   drivers/gpu/drm/radeon/radeon_device.c | 14 --------------
> >   2 files changed, 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> > index 8605c074d9f7..58111fdf520d 100644
> > --- a/drivers/gpu/drm/radeon/radeon.h
> > +++ b/drivers/gpu/drm/radeon/radeon.h
> > @@ -686,7 +686,6 @@ struct radeon_doorbell {
> >   };
> >
> >   int radeon_doorbell_get(struct radeon_device *rdev, u32 *page);
> > -void radeon_doorbell_free(struct radeon_device *rdev, u32 doorbell);
> >
> >   /*
> >    * IRQS.
> > diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
> > index bbd39348a7ab..4127ffb4bb6f 100644
> > --- a/drivers/gpu/drm/radeon/radeon_device.c
> > +++ b/drivers/gpu/drm/radeon/radeon_device.c
> > @@ -392,20 +392,6 @@ int radeon_doorbell_get(struct radeon_device *rdev, u32 *doorbell)
> >       }
> >   }
> >
> > -/**
> > - * radeon_doorbell_free - Free a doorbell entry
> > - *
> > - * @rdev: radeon_device pointer
> > - * @doorbell: doorbell index
> > - *
> > - * Free a doorbell allocated for use by the driver (all asics)
> > - */
> > -void radeon_doorbell_free(struct radeon_device *rdev, u32 doorbell)
> > -{
> > -     if (doorbell < rdev->doorbell.num_doorbells)
> > -             __clear_bit(doorbell, rdev->doorbell.used);
> > -}
> > -
> >   /*
> >    * radeon_wb_*()
> >    * Writeback is the method by which the GPU updates special pages
>

Re: [PATCH 2/4] drm/radeon: Remove unused radeon_doorbell_free

Posted by Dr. David Alan Gilbert 7 months, 3 weeks ago

* Alex Deucher (alexdeucher@gmail.com) wrote:
> On Fri, Apr 18, 2025 at 2:18 AM Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> wrote:
> >
> > Le 18/04/2025 à 02:21, linux@treblig.org a écrit :
> > > From: "Dr. David Alan Gilbert" <linux@treblig.org>
> > >
> > > radeon_doorbell_free() was added in 2013 by
> > > commit 75efdee11b5d ("drm/radeon: implement simple doorbell page
> > > allocator")
> > > but never used.
> >
> > Hi,
> >
> > I think than instead of being removed, it should be used in the error
> > handling path of cik_init() and in cik_fini().
> 
> Yes, ideally.  Care to make a patch to fix that?

I can have a look at that.

Dave

> Thanks,
> 
> Alex
> 
> >
> > CJ
> >
> > >
> > > Remove it.
> > >
> > > Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org>
> > > ---
> > >   drivers/gpu/drm/radeon/radeon.h        |  1 -
> > >   drivers/gpu/drm/radeon/radeon_device.c | 14 --------------
> > >   2 files changed, 15 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> > > index 8605c074d9f7..58111fdf520d 100644
> > > --- a/drivers/gpu/drm/radeon/radeon.h
> > > +++ b/drivers/gpu/drm/radeon/radeon.h
> > > @@ -686,7 +686,6 @@ struct radeon_doorbell {
> > >   };
> > >
> > >   int radeon_doorbell_get(struct radeon_device *rdev, u32 *page);
> > > -void radeon_doorbell_free(struct radeon_device *rdev, u32 doorbell);
> > >
> > >   /*
> > >    * IRQS.
> > > diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
> > > index bbd39348a7ab..4127ffb4bb6f 100644
> > > --- a/drivers/gpu/drm/radeon/radeon_device.c
> > > +++ b/drivers/gpu/drm/radeon/radeon_device.c
> > > @@ -392,20 +392,6 @@ int radeon_doorbell_get(struct radeon_device *rdev, u32 *doorbell)
> > >       }
> > >   }
> > >
> > > -/**
> > > - * radeon_doorbell_free - Free a doorbell entry
> > > - *
> > > - * @rdev: radeon_device pointer
> > > - * @doorbell: doorbell index
> > > - *
> > > - * Free a doorbell allocated for use by the driver (all asics)
> > > - */
> > > -void radeon_doorbell_free(struct radeon_device *rdev, u32 doorbell)
> > > -{
> > > -     if (doorbell < rdev->doorbell.num_doorbells)
> > > -             __clear_bit(doorbell, rdev->doorbell.used);
> > > -}
> > > -
> > >   /*
> > >    * radeon_wb_*()
> > >    * Writeback is the method by which the GPU updates special pages
> >
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

[PATCH 1/4] drm/radeon/radeon_audio: Remove unused r600_hdmi_audio_workaround
[PATCH 2/4] drm/radeon: Remove unused radeon_doorbell_free
[PATCH 3/4] drm/radeon: Remove unused radeon_fence_wait_any
[PATCH 4/4] drm/amd/display: Remove unused *vbios_smu_set_dprefclk