On 2/13/2025 2:45 PM, Sundararaju, Sathishkumar wrote:
>
>
> 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.
>
Ok, that is fine if this provides more flexibility for future
functionalities.
Asked in particular based on the approach (that is to be) followed in
vcn. If that was the case, then this would be something like -
int jpeg_inst_start(amdgpu_jpeg_inst* inst)
{
power_up_inst();
for_each_core_in_inst(core, inst)
jpeg_core_start(core);
}
Thanks,
Lijo
> 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;
>>>>> }
>
