[PATCH 0/4] drm/amd & radeon deadcoding

[PATCH 0/4] drm/amd & radeon deadcoding

[linux]

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

Hi,
  Another set of deadcoding around amd/radeon GPU stuff.

Dave
Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org>


Dr. David Alan Gilbert (4):
  drm/radeon/radeon_audio: Remove unused r600_hdmi_audio_workaround
  drm/radeon: Remove unused radeon_doorbell_free
  drm/radeon: Remove unused radeon_fence_wait_any
  drm/amd/display: Remove unused *vbios_smu_set_dprefclk

 .../dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.c  | 14 -------
 .../dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.h  |  1 -
 .../dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.c   | 14 -------
 .../dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.h   |  1 -
 drivers/gpu/drm/radeon/r600_hdmi.c            | 22 ----------
 drivers/gpu/drm/radeon/radeon.h               |  4 --
 drivers/gpu/drm/radeon/radeon_asic.h          |  1 -
 drivers/gpu/drm/radeon/radeon_device.c        | 14 -------
 drivers/gpu/drm/radeon/radeon_fence.c         | 42 -------------------
 9 files changed, 113 deletions(-)

-- 
2.49.0

[PATCH 1/4] drm/radeon/radeon_audio: Remove unused r600_hdmi_audio_workaround

[linux]

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

The last use of r600_hdmi_audio_workaround() was removed by 2014's
commit 6e72376dcc66 ("radeon/audio: consolidate audio_mode_set()
functions")

Remove it.

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

diff --git a/drivers/gpu/drm/radeon/r600_hdmi.c b/drivers/gpu/drm/radeon/r600_hdmi.c
index 661f374f5f27..9758f3a9df75 100644
--- a/drivers/gpu/drm/radeon/r600_hdmi.c
+++ b/drivers/gpu/drm/radeon/r600_hdmi.c
@@ -290,28 +290,6 @@ int r600_hdmi_buffer_status_changed(struct drm_encoder *encoder)
 	return result;
 }
 
-/*
- * write the audio workaround status to the hardware
- */
-void r600_hdmi_audio_workaround(struct drm_encoder *encoder)
-{
-	struct drm_device *dev = encoder->dev;
-	struct radeon_device *rdev = dev->dev_private;
-	struct radeon_encoder *radeon_encoder = to_radeon_encoder(encoder);
-	struct radeon_encoder_atom_dig *dig = radeon_encoder->enc_priv;
-	uint32_t offset = dig->afmt->offset;
-	bool hdmi_audio_workaround = false; /* FIXME */
-	u32 value;
-
-	if (!hdmi_audio_workaround ||
-	    r600_hdmi_is_audio_buffer_filled(encoder))
-		value = 0; /* disable workaround */
-	else
-		value = HDMI0_AUDIO_TEST_EN; /* enable workaround */
-	WREG32_P(HDMI0_AUDIO_PACKET_CONTROL + offset,
-		 value, ~HDMI0_AUDIO_TEST_EN);
-}
-
 void r600_hdmi_audio_set_dto(struct radeon_device *rdev,
 			     struct radeon_crtc *crtc, unsigned int clock)
 {
diff --git a/drivers/gpu/drm/radeon/radeon_asic.h b/drivers/gpu/drm/radeon/radeon_asic.h
index 8f5e07834fcc..9e697f10f9ca 100644
--- a/drivers/gpu/drm/radeon/radeon_asic.h
+++ b/drivers/gpu/drm/radeon/radeon_asic.h
@@ -401,7 +401,6 @@ void r600_audio_set_dto(struct drm_encoder *encoder, u32 clock);
 void r600_hdmi_update_avi_infoframe(struct drm_encoder *encoder, void *buffer,
 				    size_t size);
 void r600_hdmi_update_ACR(struct drm_encoder *encoder, uint32_t clock);
-void r600_hdmi_audio_workaround(struct drm_encoder *encoder);
 int r600_hdmi_buffer_status_changed(struct drm_encoder *encoder);
 void r600_hdmi_update_audio_settings(struct drm_encoder *encoder);
 u32 r600_get_xclk(struct radeon_device *rdev);
-- 
2.49.0

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

[linux]

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

[PATCH 3/4] drm/radeon: Remove unused radeon_fence_wait_any

[linux]

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

radeon_fence_wait_any() last use was removed in 2023's
commit 254986e324ad ("drm/radeon: Use the drm suballocation manager
implementation.")

Remove it.

Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org>
---
 drivers/gpu/drm/radeon/radeon.h       |  3 --
 drivers/gpu/drm/radeon/radeon_fence.c | 42 ---------------------------
 2 files changed, 45 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 58111fdf520d..53f6378b6db6 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -394,9 +394,6 @@ long radeon_fence_wait_timeout(struct radeon_fence *fence, bool interruptible, l
 int radeon_fence_wait(struct radeon_fence *fence, bool interruptible);
 int radeon_fence_wait_next(struct radeon_device *rdev, int ring);
 int radeon_fence_wait_empty(struct radeon_device *rdev, int ring);
-int radeon_fence_wait_any(struct radeon_device *rdev,
-			  struct radeon_fence **fences,
-			  bool intr);
 struct radeon_fence *radeon_fence_ref(struct radeon_fence *fence);
 void radeon_fence_unref(struct radeon_fence **fence);
 unsigned radeon_fence_count_emitted(struct radeon_device *rdev, int ring);
diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c
index 8ff4f18b51a9..5b5b54e876d4 100644
--- a/drivers/gpu/drm/radeon/radeon_fence.c
+++ b/drivers/gpu/drm/radeon/radeon_fence.c
@@ -574,48 +574,6 @@ int radeon_fence_wait(struct radeon_fence *fence, bool intr)
 		return r;
 }
 
-/**
- * radeon_fence_wait_any - wait for a fence to signal on any ring
- *
- * @rdev: radeon device pointer
- * @fences: radeon fence object(s)
- * @intr: use interruptable sleep
- *
- * Wait for any requested fence to signal (all asics).  Fence
- * array is indexed by ring id.  @intr selects whether to use
- * interruptable (true) or non-interruptable (false) sleep when
- * waiting for the fences. Used by the suballocator.
- * Returns 0 if any fence has passed, error for all other cases.
- */
-int radeon_fence_wait_any(struct radeon_device *rdev,
-			  struct radeon_fence **fences,
-			  bool intr)
-{
-	uint64_t seq[RADEON_NUM_RINGS];
-	unsigned int i, num_rings = 0;
-	long r;
-
-	for (i = 0; i < RADEON_NUM_RINGS; ++i) {
-		seq[i] = 0;
-
-		if (!fences[i])
-			continue;
-
-		seq[i] = fences[i]->seq;
-		++num_rings;
-	}
-
-	/* nothing to wait for ? */
-	if (num_rings == 0)
-		return -ENOENT;
-
-	r = radeon_fence_wait_seq_timeout(rdev, seq, intr, MAX_SCHEDULE_TIMEOUT);
-	if (r < 0)
-		return r;
-
-	return 0;
-}
-
 /**
  * radeon_fence_wait_next - wait for the next fence to signal
  *
-- 
2.49.0

[PATCH 4/4] drm/amd/display: Remove unused *vbios_smu_set_dprefclk

[linux]

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

rn_vbios_smu_set_dprefclk() was added in 2019 by
commit 4edb6fc91878 ("drm/amd/display: Add Renoir clock manager")
rv1_vbios_smu_set_dprefclk() was also added in 2019 by
commit dc88b4a684d2 ("drm/amd/display: make clk mgr soc specific")

neither have been used.

Remove them.

Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org>
---
 .../dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.c       | 14 --------------
 .../dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.h       |  1 -
 .../dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.c        | 14 --------------
 .../dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.h        |  1 -
 4 files changed, 30 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.c
index 19897fa52e7e..d82a52319088 100644
--- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.c
+++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.c
@@ -142,17 +142,3 @@ int rv1_vbios_smu_set_dispclk(struct clk_mgr_internal *clk_mgr, int requested_di
 
 	return actual_dispclk_set_mhz * 1000;
 }
-
-int rv1_vbios_smu_set_dprefclk(struct clk_mgr_internal *clk_mgr)
-{
-	int actual_dprefclk_set_mhz = -1;
-
-	actual_dprefclk_set_mhz = rv1_vbios_smu_send_msg_with_param(
-			clk_mgr,
-			VBIOSSMC_MSG_SetDprefclkFreq,
-			khz_to_mhz_ceil(clk_mgr->base.dprefclk_khz));
-
-	/* TODO: add code for programing DP DTO, currently this is down by command table */
-
-	return actual_dprefclk_set_mhz * 1000;
-}
diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.h b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.h
index 083cb3158859..81d7c912549c 100644
--- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.h
+++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.h
@@ -27,6 +27,5 @@
 #define DAL_DC_DCN10_RV1_CLK_MGR_VBIOS_SMU_H_
 
 int rv1_vbios_smu_set_dispclk(struct clk_mgr_internal *clk_mgr, int requested_dispclk_khz);
-int rv1_vbios_smu_set_dprefclk(struct clk_mgr_internal *clk_mgr);
 
 #endif /* DAL_DC_DCN10_RV1_CLK_MGR_VBIOS_SMU_H_ */
diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.c
index 23b390245b5d..5a633333dbb5 100644
--- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.c
+++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.c
@@ -164,20 +164,6 @@ int rn_vbios_smu_set_dispclk(struct clk_mgr_internal *clk_mgr, int requested_dis
 	return actual_dispclk_set_mhz * 1000;
 }
 
-int rn_vbios_smu_set_dprefclk(struct clk_mgr_internal *clk_mgr)
-{
-	int actual_dprefclk_set_mhz = -1;
-
-	actual_dprefclk_set_mhz = rn_vbios_smu_send_msg_with_param(
-			clk_mgr,
-			VBIOSSMC_MSG_SetDprefclkFreq,
-			khz_to_mhz_ceil(clk_mgr->base.dprefclk_khz));
-
-	/* TODO: add code for programing DP DTO, currently this is down by command table */
-
-	return actual_dprefclk_set_mhz * 1000;
-}
-
 int rn_vbios_smu_set_hard_min_dcfclk(struct clk_mgr_internal *clk_mgr, int requested_dcfclk_khz)
 {
 	int actual_dcfclk_set_mhz = -1;
diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.h b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.h
index 1ce19d875358..f76fad87f0e1 100644
--- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.h
+++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.h
@@ -30,7 +30,6 @@ enum dcn_pwr_state;
 
 int rn_vbios_smu_get_smu_version(struct clk_mgr_internal *clk_mgr);
 int rn_vbios_smu_set_dispclk(struct clk_mgr_internal *clk_mgr, int requested_dispclk_khz);
-int rn_vbios_smu_set_dprefclk(struct clk_mgr_internal *clk_mgr);
 int rn_vbios_smu_set_hard_min_dcfclk(struct clk_mgr_internal *clk_mgr, int requested_dcfclk_khz);
 int rn_vbios_smu_set_min_deep_sleep_dcfclk(struct clk_mgr_internal *clk_mgr, int requested_min_ds_dcfclk_khz);
 void rn_vbios_smu_set_phyclk(struct clk_mgr_internal *clk_mgr, int requested_phyclk_khz);
-- 
2.49.0

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

[Christophe JAILLET]

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

[Alex Deucher]

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 4/4] drm/amd/display: Remove unused *vbios_smu_set_dprefclk

[Alex Deucher]

Applied patches 1, 3, 4.  Thanks!

On Thu, Apr 17, 2025 at 8:28 PM <linux@treblig.org> wrote:
>
> From: "Dr. David Alan Gilbert" <linux@treblig.org>
>
> rn_vbios_smu_set_dprefclk() was added in 2019 by
> commit 4edb6fc91878 ("drm/amd/display: Add Renoir clock manager")
> rv1_vbios_smu_set_dprefclk() was also added in 2019 by
> commit dc88b4a684d2 ("drm/amd/display: make clk mgr soc specific")
>
> neither have been used.
>
> Remove them.
>
> Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org>
> ---
>  .../dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.c       | 14 --------------
>  .../dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.h       |  1 -
>  .../dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.c        | 14 --------------
>  .../dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.h        |  1 -
>  4 files changed, 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.c
> index 19897fa52e7e..d82a52319088 100644
> --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.c
> +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.c
> @@ -142,17 +142,3 @@ int rv1_vbios_smu_set_dispclk(struct clk_mgr_internal *clk_mgr, int requested_di
>
>         return actual_dispclk_set_mhz * 1000;
>  }
> -
> -int rv1_vbios_smu_set_dprefclk(struct clk_mgr_internal *clk_mgr)
> -{
> -       int actual_dprefclk_set_mhz = -1;
> -
> -       actual_dprefclk_set_mhz = rv1_vbios_smu_send_msg_with_param(
> -                       clk_mgr,
> -                       VBIOSSMC_MSG_SetDprefclkFreq,
> -                       khz_to_mhz_ceil(clk_mgr->base.dprefclk_khz));
> -
> -       /* TODO: add code for programing DP DTO, currently this is down by command table */
> -
> -       return actual_dprefclk_set_mhz * 1000;
> -}
> diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.h b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.h
> index 083cb3158859..81d7c912549c 100644
> --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.h
> +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr_vbios_smu.h
> @@ -27,6 +27,5 @@
>  #define DAL_DC_DCN10_RV1_CLK_MGR_VBIOS_SMU_H_
>
>  int rv1_vbios_smu_set_dispclk(struct clk_mgr_internal *clk_mgr, int requested_dispclk_khz);
> -int rv1_vbios_smu_set_dprefclk(struct clk_mgr_internal *clk_mgr);
>
>  #endif /* DAL_DC_DCN10_RV1_CLK_MGR_VBIOS_SMU_H_ */
> diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.c
> index 23b390245b5d..5a633333dbb5 100644
> --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.c
> +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.c
> @@ -164,20 +164,6 @@ int rn_vbios_smu_set_dispclk(struct clk_mgr_internal *clk_mgr, int requested_dis
>         return actual_dispclk_set_mhz * 1000;
>  }
>
> -int rn_vbios_smu_set_dprefclk(struct clk_mgr_internal *clk_mgr)
> -{
> -       int actual_dprefclk_set_mhz = -1;
> -
> -       actual_dprefclk_set_mhz = rn_vbios_smu_send_msg_with_param(
> -                       clk_mgr,
> -                       VBIOSSMC_MSG_SetDprefclkFreq,
> -                       khz_to_mhz_ceil(clk_mgr->base.dprefclk_khz));
> -
> -       /* TODO: add code for programing DP DTO, currently this is down by command table */
> -
> -       return actual_dprefclk_set_mhz * 1000;
> -}
> -
>  int rn_vbios_smu_set_hard_min_dcfclk(struct clk_mgr_internal *clk_mgr, int requested_dcfclk_khz)
>  {
>         int actual_dcfclk_set_mhz = -1;
> diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.h b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.h
> index 1ce19d875358..f76fad87f0e1 100644
> --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.h
> +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn21/rn_clk_mgr_vbios_smu.h
> @@ -30,7 +30,6 @@ enum dcn_pwr_state;
>
>  int rn_vbios_smu_get_smu_version(struct clk_mgr_internal *clk_mgr);
>  int rn_vbios_smu_set_dispclk(struct clk_mgr_internal *clk_mgr, int requested_dispclk_khz);
> -int rn_vbios_smu_set_dprefclk(struct clk_mgr_internal *clk_mgr);
>  int rn_vbios_smu_set_hard_min_dcfclk(struct clk_mgr_internal *clk_mgr, int requested_dcfclk_khz);
>  int rn_vbios_smu_set_min_deep_sleep_dcfclk(struct clk_mgr_internal *clk_mgr, int requested_min_ds_dcfclk_khz);
>  void rn_vbios_smu_set_phyclk(struct clk_mgr_internal *clk_mgr, int requested_phyclk_khz);
> --
> 2.49.0
>

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

[Dr. David Alan Gilbert]

* 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   |_______/

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

[Dr. David Alan Gilbert]

* 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

[Alex Deucher]

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

[Dr. David Alan Gilbert]

* 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

[Dr. David Alan Gilbert]

* 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   |_______/