Re: [PATCH v2 1/7] drm/amdgpu: Per-instance init func for JPEG4_0_3

["Sundararaju, Sathishkumar" <sasundar@xxxxxxx>]

Hi Christian,


On 2/11/2025 12:52 AM, Christian König wrote:
> Am 10.02.25 um 15:25 schrieb Sathishkumar S:
> > Add helper functions to handle per-instance and per-core
> > initialization and deinitialization in JPEG4_0_3.
> >
> > Signed-off-by: Sathishkumar S <sathishkumar.sundararaju@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/jpeg_v4_0_3.c | 190 ++++++++++++-----------
> >   1 file changed, 98 insertions(+), 92 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/jpeg_v4_0_3.c 
> > b/drivers/gpu/drm/amd/amdgpu/jpeg_v4_0_3.c
> > index 2a97302a22d3..be0b3b4c8690 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/jpeg_v4_0_3.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/jpeg_v4_0_3.c
> > @@ -525,6 +525,75 @@ static void 
> > jpeg_v4_0_3_enable_clock_gating(struct amdgpu_device *adev, int inst
> >       WREG32_SOC15(JPEG, jpeg_inst, regJPEG_CGC_GATE, data);
> >   }
> >   +static void jpeg_v4_0_3_start_inst(struct amdgpu_device *adev, int 
> > inst)
> > +{
> > +    int jpeg_inst = GET_INST(JPEG, inst);
> > +
> > +    WREG32_SOC15(JPEG, jpeg_inst, regUVD_PGFSM_CONFIG,
> > +             1 << UVD_PGFSM_CONFIG__UVDJ_PWR_CONFIG__SHIFT);
> > +    SOC15_WAIT_ON_RREG
> > +        (JPEG, jpeg_inst, regUVD_PGFSM_STATUS,
> > +         UVD_PGFSM_STATUS__UVDJ_PWR_ON << 
> > UVD_PGFSM_STATUS__UVDJ_PWR_STATUS__SHIFT,
> > +         UVD_PGFSM_STATUS__UVDJ_PWR_STATUS_MASK);
>
> Please double check the coding style with checkpatch.pl this here 
> doesn't looks so well.
I did check the series with checkpatch.pl for 0 errors and 0 warnings, I 
can realign this to the usual way though, thank you.

Regards,
Sathish
> Regards,
> Christian.
>
> > +
> > +    /* disable anti hang mechanism */
> > +    WREG32_P(SOC15_REG_OFFSET(JPEG, jpeg_inst, 
> > regUVD_JPEG_POWER_STATUS),
> > +         0, ~UVD_JPEG_POWER_STATUS__JPEG_POWER_STATUS_MASK);
> > +
> > +    /* JPEG disable CGC */
> > +    jpeg_v4_0_3_disable_clock_gating(adev, inst);
> > +
> > +    /* MJPEG global tiling registers */
> > +    WREG32_SOC15(JPEG, jpeg_inst, regJPEG_DEC_GFX8_ADDR_CONFIG,
> > +             adev->gfx.config.gb_addr_config);
> > +    WREG32_SOC15(JPEG, jpeg_inst, regJPEG_DEC_GFX10_ADDR_CONFIG,
> > +             adev->gfx.config.gb_addr_config);
> > +
> > +    /* enable JMI channel */
> > +    WREG32_P(SOC15_REG_OFFSET(JPEG, jpeg_inst, regUVD_JMI_CNTL), 0,
> > +         ~UVD_JMI_CNTL__SOFT_RESET_MASK);
> > +}
> > +
> > +static void jpeg_v4_0_3_start_jrbc(struct amdgpu_ring *ring)
> > +{
> > +    struct amdgpu_device *adev = ring->adev;
> > +    int jpeg_inst = GET_INST(JPEG, ring->me);
> > +    int reg_offset = jpeg_v4_0_3_core_reg_offset(ring->pipe);
> > +
> > +    /* enable System Interrupt for JRBC */
> > +    WREG32_P(SOC15_REG_OFFSET(JPEG, jpeg_inst, regJPEG_SYS_INT_EN),
> > +         JPEG_SYS_INT_EN__DJRBC0_MASK << ring->pipe,
> > +         ~(JPEG_SYS_INT_EN__DJRBC0_MASK << ring->pipe));
> > +
> > +    WREG32_SOC15_OFFSET(JPEG, jpeg_inst,
> > +                regUVD_JMI0_UVD_LMI_JRBC_RB_VMID,
> > +                reg_offset, 0);
> > +    WREG32_SOC15_OFFSET(JPEG, jpeg_inst,
> > +                regUVD_JRBC0_UVD_JRBC_RB_CNTL,
> > +                reg_offset,
> > +                (0x00000001L | 0x00000002L));
> > +    WREG32_SOC15_OFFSET(JPEG, jpeg_inst,
> > +                regUVD_JMI0_UVD_LMI_JRBC_RB_64BIT_BAR_LOW,
> > +                reg_offset, lower_32_bits(ring->gpu_addr));
> > +    WREG32_SOC15_OFFSET(JPEG, jpeg_inst,
> > +                regUVD_JMI0_UVD_LMI_JRBC_RB_64BIT_BAR_HIGH,
> > +                reg_offset, upper_32_bits(ring->gpu_addr));
> > +    WREG32_SOC15_OFFSET(JPEG, jpeg_inst,
> > +                regUVD_JRBC0_UVD_JRBC_RB_RPTR,
> > +                reg_offset, 0);
> > +    WREG32_SOC15_OFFSET(JPEG, jpeg_inst,
> > +                regUVD_JRBC0_UVD_JRBC_RB_WPTR,
> > +                reg_offset, 0);
> > +    WREG32_SOC15_OFFSET(JPEG, jpeg_inst,
> > +                regUVD_JRBC0_UVD_JRBC_RB_CNTL,
> > +                reg_offset, 0x00000002L);
> > +    WREG32_SOC15_OFFSET(JPEG, jpeg_inst,
> > +                regUVD_JRBC0_UVD_JRBC_RB_SIZE,
> > +                reg_offset, ring->ring_size / 4);
> > +    ring->wptr = RREG32_SOC15_OFFSET(JPEG, jpeg_inst, 
> > regUVD_JRBC0_UVD_JRBC_RB_WPTR,
> > +                     reg_offset);
> > +}
> > +
> >   /**
> >    * jpeg_v4_0_3_start - start JPEG block
> >    *
> > @@ -535,84 +604,42 @@ static void 
> > jpeg_v4_0_3_enable_clock_gating(struct amdgpu_device *adev, int inst
> >   static int jpeg_v4_0_3_start(struct amdgpu_device *adev)
> >   {
> >       struct amdgpu_ring *ring;
> > -    int i, j, jpeg_inst;
> > +    int i, j;
> >         for (i = 0; i < adev->jpeg.num_jpeg_inst; ++i) {
> > -        jpeg_inst = GET_INST(JPEG, i);
> > -
> > -        WREG32_SOC15(JPEG, jpeg_inst, regUVD_PGFSM_CONFIG,
> > -                 1 << UVD_PGFSM_CONFIG__UVDJ_PWR_CONFIG__SHIFT);
> > -        SOC15_WAIT_ON_RREG(
> > -            JPEG, jpeg_inst, regUVD_PGFSM_STATUS,
> > -            UVD_PGFSM_STATUS__UVDJ_PWR_ON
> > -                << UVD_PGFSM_STATUS__UVDJ_PWR_STATUS__SHIFT,
> > -            UVD_PGFSM_STATUS__UVDJ_PWR_STATUS_MASK);
> > -
> > -        /* disable anti hang mechanism */
> > -        WREG32_P(SOC15_REG_OFFSET(JPEG, jpeg_inst,
> > -                      regUVD_JPEG_POWER_STATUS),
> > -             0, ~UVD_JPEG_POWER_STATUS__JPEG_POWER_STATUS_MASK);
> > -
> > -        /* JPEG disable CGC */
> > -        jpeg_v4_0_3_disable_clock_gating(adev, i);
> > -
> > -        /* MJPEG global tiling registers */
> > -        WREG32_SOC15(JPEG, jpeg_inst, regJPEG_DEC_GFX8_ADDR_CONFIG,
> > -                 adev->gfx.config.gb_addr_config);
> > -        WREG32_SOC15(JPEG, jpeg_inst, regJPEG_DEC_GFX10_ADDR_CONFIG,
> > -                 adev->gfx.config.gb_addr_config);
> > -
> > -        /* enable JMI channel */
> > -        WREG32_P(SOC15_REG_OFFSET(JPEG, jpeg_inst, regUVD_JMI_CNTL), 0,
> > -             ~UVD_JMI_CNTL__SOFT_RESET_MASK);
> > -
> > +        jpeg_v4_0_3_start_inst(adev, i);
> >           for (j = 0; j < adev->jpeg.num_jpeg_rings; ++j) {
> > -            int reg_offset = jpeg_v4_0_3_core_reg_offset(j);
> > -
> >               ring = &adev->jpeg.inst[i].ring_dec[j];
> > -
> > -            /* enable System Interrupt for JRBC */
> > -            WREG32_P(SOC15_REG_OFFSET(JPEG, jpeg_inst,
> > -                          regJPEG_SYS_INT_EN),
> > -                 JPEG_SYS_INT_EN__DJRBC0_MASK << j,
> > -                 ~(JPEG_SYS_INT_EN__DJRBC0_MASK << j));
> > -
> > -            WREG32_SOC15_OFFSET(JPEG, jpeg_inst,
> > -                        regUVD_JMI0_UVD_LMI_JRBC_RB_VMID,
> > -                        reg_offset, 0);
> > -            WREG32_SOC15_OFFSET(JPEG, jpeg_inst,
> > -                        regUVD_JRBC0_UVD_JRBC_RB_CNTL,
> > -                        reg_offset,
> > -                        (0x00000001L | 0x00000002L));
> > -            WREG32_SOC15_OFFSET(
> > -                JPEG, jpeg_inst,
> > -                regUVD_JMI0_UVD_LMI_JRBC_RB_64BIT_BAR_LOW,
> > -                reg_offset, lower_32_bits(ring->gpu_addr));
> > -            WREG32_SOC15_OFFSET(
> > -                JPEG, jpeg_inst,
> > -                regUVD_JMI0_UVD_LMI_JRBC_RB_64BIT_BAR_HIGH,
> > -                reg_offset, upper_32_bits(ring->gpu_addr));
> > -            WREG32_SOC15_OFFSET(JPEG, jpeg_inst,
> > -                        regUVD_JRBC0_UVD_JRBC_RB_RPTR,
> > -                        reg_offset, 0);
> > -            WREG32_SOC15_OFFSET(JPEG, jpeg_inst,
> > -                        regUVD_JRBC0_UVD_JRBC_RB_WPTR,
> > -                        reg_offset, 0);
> > -            WREG32_SOC15_OFFSET(JPEG, jpeg_inst,
> > -                        regUVD_JRBC0_UVD_JRBC_RB_CNTL,
> > -                        reg_offset, 0x00000002L);
> > -            WREG32_SOC15_OFFSET(JPEG, jpeg_inst,
> > -                        regUVD_JRBC0_UVD_JRBC_RB_SIZE,
> > -                        reg_offset, ring->ring_size / 4);
> > -            ring->wptr = RREG32_SOC15_OFFSET(
> > -                JPEG, jpeg_inst, regUVD_JRBC0_UVD_JRBC_RB_WPTR,
> > -                reg_offset);
> > +            jpeg_v4_0_3_start_jrbc(ring);
> >           }
> >       }
> >         return 0;
> >   }
> >   +static void jpeg_v4_0_3_stop_inst(struct amdgpu_device *adev, int 
> > inst)
> > +{
> > +    int jpeg_inst = GET_INST(JPEG, inst);
> > +    /* reset JMI */
> > +    WREG32_P(SOC15_REG_OFFSET(JPEG, jpeg_inst, regUVD_JMI_CNTL),
> > +         UVD_JMI_CNTL__SOFT_RESET_MASK,
> > +         ~UVD_JMI_CNTL__SOFT_RESET_MASK);
> > +
> > +    jpeg_v4_0_3_enable_clock_gating(adev, inst);
> > +
> > +    /* enable anti hang mechanism */
> > +    WREG32_P(SOC15_REG_OFFSET(JPEG, jpeg_inst, 
> > regUVD_JPEG_POWER_STATUS),
> > +         UVD_JPEG_POWER_STATUS__JPEG_POWER_STATUS_MASK,
> > +         ~UVD_JPEG_POWER_STATUS__JPEG_POWER_STATUS_MASK);
> > +
> > +    WREG32_SOC15(JPEG, jpeg_inst, regUVD_PGFSM_CONFIG,
> > +             2 << UVD_PGFSM_CONFIG__UVDJ_PWR_CONFIG__SHIFT);
> > +    SOC15_WAIT_ON_RREG
> > +        (JPEG, jpeg_inst, regUVD_PGFSM_STATUS,
> > +         UVD_PGFSM_STATUS__UVDJ_PWR_OFF << 
> > UVD_PGFSM_STATUS__UVDJ_PWR_STATUS__SHIFT,
> > +         UVD_PGFSM_STATUS__UVDJ_PWR_STATUS_MASK);
> > +}
> > +
> >   /**
> >    * jpeg_v4_0_3_stop - stop JPEG block
> >    *
> > @@ -622,31 +649,10 @@ static int jpeg_v4_0_3_start(struct 
> > amdgpu_device *adev)
> >    */
> >   static int jpeg_v4_0_3_stop(struct amdgpu_device *adev)
> >   {
> > -    int i, jpeg_inst;
> > +    int i;
> >   -    for (i = 0; i < adev->jpeg.num_jpeg_inst; ++i) {
> > -        jpeg_inst = GET_INST(JPEG, i);
> > -        /* reset JMI */
> > -        WREG32_P(SOC15_REG_OFFSET(JPEG, jpeg_inst, regUVD_JMI_CNTL),
> > -             UVD_JMI_CNTL__SOFT_RESET_MASK,
> > -             ~UVD_JMI_CNTL__SOFT_RESET_MASK);
> > -
> > -        jpeg_v4_0_3_enable_clock_gating(adev, i);
> > -
> > -        /* enable anti hang mechanism */
> > -        WREG32_P(SOC15_REG_OFFSET(JPEG, jpeg_inst,
> > -                      regUVD_JPEG_POWER_STATUS),
> > -             UVD_JPEG_POWER_STATUS__JPEG_POWER_STATUS_MASK,
> > -             ~UVD_JPEG_POWER_STATUS__JPEG_POWER_STATUS_MASK);
> > -
> > -        WREG32_SOC15(JPEG, jpeg_inst, regUVD_PGFSM_CONFIG,
> > -                 2 << UVD_PGFSM_CONFIG__UVDJ_PWR_CONFIG__SHIFT);
> > -        SOC15_WAIT_ON_RREG(
> > -            JPEG, jpeg_inst, regUVD_PGFSM_STATUS,
> > -            UVD_PGFSM_STATUS__UVDJ_PWR_OFF
> > -                << UVD_PGFSM_STATUS__UVDJ_PWR_STATUS__SHIFT,
> > -            UVD_PGFSM_STATUS__UVDJ_PWR_STATUS_MASK);
> > -    }
> > +    for (i = 0; i < adev->jpeg.num_jpeg_inst; ++i)
> > +        jpeg_v4_0_3_stop_inst(adev, i);
> >         return 0;
> >   }