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

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

On 2/13/2025 1:35 PM, Lazar, Lijo wrote:
> On 2/13/2025 1:07 PM, Sundararaju, Sathishkumar wrote:
> > On 2/13/2025 12:16 PM, Lazar, Lijo wrote:
> > > On 2/13/2025 8:24 AM, Sathishkumar S wrote:
> > > > 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>
> > > > Acked-by: Christian König <christian.koenig@xxxxxxx>
> > > > Reviewed-by: Leo Liu <leo.liu@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..e355febd9b82 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);
> > > > +
> > > > +    /* 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) {
> > > It's better to move this inside the instance function. Instance takes
> > > care of all cores within the instance.
> > The separation in the helper functions was done to decouple core
> > specific configs away from instance
> > specific configs to have the degree of freedom to control them
> > independently without meddling with
> > each other, so having them done separately kind of helps to align better
> > with that choice.
> Functionally, is an instance considered started even if its cores are
> not initialized?

was that a question ? _start() does it.

The current design aligns better for the initialization and also future 
work planned, like per core reset
and full instance recovery, and this separation is a must for that, I do 
not want them together as it
hinders an axis of freedom in configuration and recovery process with 
validation at every step.

Regards,
Sathish

> Thanks,
> Lijo
>
> > Regards,
> > Sathish
> > > Thanks,
> > > Lijo
> > >
> > > > -            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;
> > > >    }