[PATCH 0/3] Add some adjustment for VCN 4.0.3 and 5.0.1

[PATCH 0/3] Add some adjustment for VCN 4.0.3 and 5.0.1

[Rodrigo Siqueira]

While I was looking into the ring buffer interface, I noticed some minor
issues in the VCN 4.0.3 and 5.0.1. This series has some code
improvements in those two components.

Thanks

Rodrigo Siqueira (3):
  drm/amdgpu/vcn: Use the correct irq for the ring initialization
  drm/amdgpu/vcn: Remove unnecessary check
  drm/amdgpu/vcn: Ensure that sysfs reset run in the fini

 drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c | 8 ++------
 drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c | 4 ++--
 2 files changed, 4 insertions(+), 8 deletions(-)

-- 
2.47.2

[PATCH 1/3] drm/amdgpu/vcn: Use the correct irq for the ring initialization

[Rodrigo Siqueira]

In the loop that iterates over the different VCN instances from VCN
4.0.3, the same irq source has been passed for different instances.
This commit addresses the issue by adding the missing index to the array
access for the IRQ.

Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
---
 drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
index a63a1e3435ab..018a526a8801 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
@@ -212,7 +212,7 @@ static int vcn_v4_0_3_sw_init(struct amdgpu_ip_block *ip_block)
 
 		ring->vm_hub = AMDGPU_MMHUB0(adev->vcn.inst[i].aid_id);
 		sprintf(ring->name, "vcn_unified_%d", adev->vcn.inst[i].aid_id);
-		r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst->irq, 0,
+		r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst[i].irq, 0,
 				     AMDGPU_RING_PRIO_DEFAULT,
 				     &adev->vcn.inst[i].sched_score);
 		if (r)
-- 
2.47.2

[PATCH 2/3] drm/amdgpu/vcn: Remove unnecessary check

[Rodrigo Siqueira]

The function amdgpu_vcn_sysfs_reset_mask_init already returns 0, which
makes the check of the result unnecessary in the vcn_v4_0_3_sw_init().
Just return the amdgpu_vcn_sysfs_reset_mask_init directly.

Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
---
 drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
index 018a526a8801..168c394deeba 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
@@ -242,11 +242,7 @@ static int vcn_v4_0_3_sw_init(struct amdgpu_ip_block *ip_block)
 	if (r)
 		return r;
 
-	r = amdgpu_vcn_sysfs_reset_mask_init(adev);
-	if (r)
-		return r;
-
-	return 0;
+	return amdgpu_vcn_sysfs_reset_mask_init(adev);
 }
 
 /**
-- 
2.47.2

[PATCH 3/3] drm/amdgpu/vcn: Ensure that sysfs reset run in the fini

[Rodrigo Siqueira]

The function amdgpu_vcn_sysfs_reset_mask_fini() in the
vcn_v5_0_1_sw_fini() is invoked at the end of the function, after
amdgpu_vcn_sw_fini(). This can be a problem if amdgpu_vcn_sw_fini()
returns early, since the VCN reset sysfs interface will not be removed.
This commit addresses the issue by moving
amdgpu_vcn_sysfs_reset_mask_fini() above amdgpu_vcn_sw_fini(), aligning
the fini code with vcn_v4_0_3_sw_fini().

Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
---
 drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c b/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c
index 7cb21e2b4eb0..3b7372861032 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c
@@ -245,14 +245,14 @@ static int vcn_v5_0_1_sw_fini(struct amdgpu_ip_block *ip_block)
 			return r;
 	}
 
+	amdgpu_vcn_sysfs_reset_mask_fini(adev);
+
 	for (i = 0; i < adev->vcn.num_vcn_inst; i++) {
 		r = amdgpu_vcn_sw_fini(adev, i);
 		if (r)
 			return r;
 	}
 
-	amdgpu_vcn_sysfs_reset_mask_fini(adev);
-
 	return 0;
 }
 
-- 
2.47.2

Re: [PATCH 3/3] drm/amdgpu/vcn: Ensure that sysfs reset run in the fini

[Christian König]

On 16.08.25 18:27, Rodrigo Siqueira wrote:
> The function amdgpu_vcn_sysfs_reset_mask_fini() in the
> vcn_v5_0_1_sw_fini() is invoked at the end of the function, after
> amdgpu_vcn_sw_fini(). This can be a problem if amdgpu_vcn_sw_fini()
> returns early, since the VCN reset sysfs interface will not be removed.
> This commit addresses the issue by moving
> amdgpu_vcn_sysfs_reset_mask_fini() above amdgpu_vcn_sw_fini(), aligning
> the fini code with vcn_v4_0_3_sw_fini().
> 
> Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c b/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c
> index 7cb21e2b4eb0..3b7372861032 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c
> @@ -245,14 +245,14 @@ static int vcn_v5_0_1_sw_fini(struct amdgpu_ip_block *ip_block)
>  			return r;
>  	}
>  
> +	amdgpu_vcn_sysfs_reset_mask_fini(adev);
> +
>  	for (i = 0; i < adev->vcn.num_vcn_inst; i++) {
>  		r = amdgpu_vcn_sw_fini(adev, i);
>  		if (r)
>  			return r;
>  	}
>  
> -	amdgpu_vcn_sysfs_reset_mask_fini(adev);
> -
>  	return 0;
>  }
>  

Re: [PATCH 1/3] drm/amdgpu/vcn: Use the correct irq for the ring initialization

[Alex Deucher]

On Sat, Aug 16, 2025 at 12:28 PM Rodrigo Siqueira <siqueira@igalia.com> wrote:
>
> In the loop that iterates over the different VCN instances from VCN
> 4.0.3, the same irq source has been passed for different instances.
> This commit addresses the issue by adding the missing index to the array
> access for the IRQ.

This is on purpose.  There are no per instance source ids on 4.0.3.
The IH packets on this chip have a separate field to differentiate the
instances.

Alex

>
> Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
> index a63a1e3435ab..018a526a8801 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
> @@ -212,7 +212,7 @@ static int vcn_v4_0_3_sw_init(struct amdgpu_ip_block *ip_block)
>
>                 ring->vm_hub = AMDGPU_MMHUB0(adev->vcn.inst[i].aid_id);
>                 sprintf(ring->name, "vcn_unified_%d", adev->vcn.inst[i].aid_id);
> -               r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst->irq, 0,
> +               r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst[i].irq, 0,
>                                      AMDGPU_RING_PRIO_DEFAULT,
>                                      &adev->vcn.inst[i].sched_score);
>                 if (r)
> --
> 2.47.2
>

Re: [PATCH 3/3] drm/amdgpu/vcn: Ensure that sysfs reset run in the fini

[Alex Deucher]

On Sat, Aug 16, 2025 at 12:28 PM Rodrigo Siqueira <siqueira@igalia.com> wrote:
>
> The function amdgpu_vcn_sysfs_reset_mask_fini() in the
> vcn_v5_0_1_sw_fini() is invoked at the end of the function, after
> amdgpu_vcn_sw_fini(). This can be a problem if amdgpu_vcn_sw_fini()
> returns early, since the VCN reset sysfs interface will not be removed.
> This commit addresses the issue by moving
> amdgpu_vcn_sysfs_reset_mask_fini() above amdgpu_vcn_sw_fini(), aligning
> the fini code with vcn_v4_0_3_sw_fini().
>
> Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c b/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c
> index 7cb21e2b4eb0..3b7372861032 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c
> @@ -245,14 +245,14 @@ static int vcn_v5_0_1_sw_fini(struct amdgpu_ip_block *ip_block)
>                         return r;
>         }
>
> +       amdgpu_vcn_sysfs_reset_mask_fini(adev);
> +
>         for (i = 0; i < adev->vcn.num_vcn_inst; i++) {
>                 r = amdgpu_vcn_sw_fini(adev, i);

This function always returns success.  I'd suggested changing it to a
void and dropping the early return.

Alex

>                 if (r)
>                         return r;
>         }
>
> -       amdgpu_vcn_sysfs_reset_mask_fini(adev);
> -
>         return 0;
>  }
>
> --
> 2.47.2
>

Re: [PATCH 2/3] drm/amdgpu/vcn: Remove unnecessary check

[Alex Deucher]

Applied.  Thanks!

Alex

On Sat, Aug 16, 2025 at 12:28 PM Rodrigo Siqueira <siqueira@igalia.com> wrote:
>
> The function amdgpu_vcn_sysfs_reset_mask_init already returns 0, which
> makes the check of the result unnecessary in the vcn_v4_0_3_sw_init().
> Just return the amdgpu_vcn_sysfs_reset_mask_init directly.
>
> Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
> index 018a526a8801..168c394deeba 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
> @@ -242,11 +242,7 @@ static int vcn_v4_0_3_sw_init(struct amdgpu_ip_block *ip_block)
>         if (r)
>                 return r;
>
> -       r = amdgpu_vcn_sysfs_reset_mask_init(adev);
> -       if (r)
> -               return r;
> -
> -       return 0;
> +       return amdgpu_vcn_sysfs_reset_mask_init(adev);
>  }
>
>  /**
> --
> 2.47.2
>

Re: [PATCH 1/3] drm/amdgpu/vcn: Use the correct irq for the ring initialization

[Rodrigo Siqueira]

On 08/18, Alex Deucher wrote:
> On Sat, Aug 16, 2025 at 12:28 PM Rodrigo Siqueira <siqueira@igalia.com> wrote:
> >
> > In the loop that iterates over the different VCN instances from VCN
> > 4.0.3, the same irq source has been passed for different instances.
> > This commit addresses the issue by adding the missing index to the array
> > access for the IRQ.
> 
> This is on purpose.  There are no per instance source ids on 4.0.3.
> The IH packets on this chip have a separate field to differentiate the
> instances.

Thanks for the explanation. Is this rationale also valid for VCN 1.0 and
VCN 2.0?

Also, do you think it is worth making this hardware difference more
evident in the code with something like this:

-               r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst[i].irq, 0,
+
+               // There are no per-instance source IDs on 4.0.3, the IH
+               // packets use a separate field to differentiate instances.
+               r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst[0].irq, 0,

Thanks

> 
> Alex
> 
> >
> > Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
> > index a63a1e3435ab..018a526a8801 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
> > @@ -212,7 +212,7 @@ static int vcn_v4_0_3_sw_init(struct amdgpu_ip_block *ip_block)
> >
> >                 ring->vm_hub = AMDGPU_MMHUB0(adev->vcn.inst[i].aid_id);
> >                 sprintf(ring->name, "vcn_unified_%d", adev->vcn.inst[i].aid_id);
> > -               r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst->irq, 0,
> > +               r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst[i].irq, 0,
> >                                      AMDGPU_RING_PRIO_DEFAULT,
> >                                      &adev->vcn.inst[i].sched_score);
> >                 if (r)
> > --
> > 2.47.2
> >

-- 
Rodrigo Siqueira

Re: [PATCH 3/3] drm/amdgpu/vcn: Ensure that sysfs reset run in the fini

[Rodrigo Siqueira]

On 08/18, Alex Deucher wrote:
> On Sat, Aug 16, 2025 at 12:28 PM Rodrigo Siqueira <siqueira@igalia.com> wrote:
> >
> > The function amdgpu_vcn_sysfs_reset_mask_fini() in the
> > vcn_v5_0_1_sw_fini() is invoked at the end of the function, after
> > amdgpu_vcn_sw_fini(). This can be a problem if amdgpu_vcn_sw_fini()
> > returns early, since the VCN reset sysfs interface will not be removed.
> > This commit addresses the issue by moving
> > amdgpu_vcn_sysfs_reset_mask_fini() above amdgpu_vcn_sw_fini(), aligning
> > the fini code with vcn_v4_0_3_sw_fini().
> >
> > Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c b/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c
> > index 7cb21e2b4eb0..3b7372861032 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v5_0_1.c
> > @@ -245,14 +245,14 @@ static int vcn_v5_0_1_sw_fini(struct amdgpu_ip_block *ip_block)
> >                         return r;
> >         }
> >
> > +       amdgpu_vcn_sysfs_reset_mask_fini(adev);
> > +
> >         for (i = 0; i < adev->vcn.num_vcn_inst; i++) {
> >                 r = amdgpu_vcn_sw_fini(adev, i);
> 
> This function always returns success.  I'd suggested changing it to a
> void and dropping the early return.

I'll do that in the V2.

Thanks

> 
> Alex
> 
> >                 if (r)
> >                         return r;
> >         }
> >
> > -       amdgpu_vcn_sysfs_reset_mask_fini(adev);
> > -
> >         return 0;
> >  }
> >
> > --
> > 2.47.2
> >

-- 
Rodrigo Siqueira

Re: [PATCH 1/3] drm/amdgpu/vcn: Use the correct irq for the ring initialization

[Alex Deucher]

On Wed, Aug 20, 2025 at 6:09 PM Rodrigo Siqueira <siqueira@igalia.com> wrote:
>
> On 08/18, Alex Deucher wrote:
> > On Sat, Aug 16, 2025 at 12:28 PM Rodrigo Siqueira <siqueira@igalia.com> wrote:
> > >
> > > In the loop that iterates over the different VCN instances from VCN
> > > 4.0.3, the same irq source has been passed for different instances.
> > > This commit addresses the issue by adding the missing index to the array
> > > access for the IRQ.
> >
> > This is on purpose.  There are no per instance source ids on 4.0.3.
> > The IH packets on this chip have a separate field to differentiate the
> > instances.
>
> Thanks for the explanation. Is this rationale also valid for VCN 1.0 and
> VCN 2.0?

VCN 1.0 and 2.0 only exist as single instances.  There are no chips
with multiple instances of them.

>
> Also, do you think it is worth making this hardware difference more
> evident in the code with something like this:
>
> -               r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst[i].irq, 0,
> +
> +               // There are no per-instance source IDs on 4.0.3, the IH
> +               // packets use a separate field to differentiate instances.

If you think it's helpful.  You may want to clarify that this comment
refers to the irqs ("per-instance irq source IDs").  Also, please use
C style comments.

Alex

> +               r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst[0].irq, 0,
>
> Thanks
>
> >
> > Alex
> >
> > >
> > > Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
> > > ---
> > >  drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
> > > index a63a1e3435ab..018a526a8801 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0_3.c
> > > @@ -212,7 +212,7 @@ static int vcn_v4_0_3_sw_init(struct amdgpu_ip_block *ip_block)
> > >
> > >                 ring->vm_hub = AMDGPU_MMHUB0(adev->vcn.inst[i].aid_id);
> > >                 sprintf(ring->name, "vcn_unified_%d", adev->vcn.inst[i].aid_id);
> > > -               r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst->irq, 0,
> > > +               r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.inst[i].irq, 0,
> > >                                      AMDGPU_RING_PRIO_DEFAULT,
> > >                                      &adev->vcn.inst[i].sched_score);
> > >                 if (r)
> > > --
> > > 2.47.2
> > >
>
> --
> Rodrigo Siqueira