[PATCH 3/5] drm/amdgpu: enable system interrupt for jrbc

[Boyuan Zhang]

On 2018-07-25 11:05 AM, Alex Deucher wrote:
> On Wed, Jul 25, 2018 at 10:01 AM, Boyuan Zhang <boyzhang at amd.com> wrote:
>>
>> On 2018-07-24 08:50 AM, Christian König wrote:
>>> Am 23.07.2018 um 21:53 schrieb Boyuan Zhang:
>>>>
>>>>
>>>> On 2018-07-19 02:51 PM, Alex Deucher wrote:
>>>>> On Wed, Jul 18, 2018 at 4:39 PM, <boyuan.zhang at amd.com> wrote:
>>>>>> From: Boyuan Zhang <boyuan.zhang at amd.com>
>>>>>>
>>>>>> Enable system interrupt for jrbc during engine starting time.
>>>>>>
>>>>>> Signed-off-by: Boyuan Zhang <boyuan.zhang at amd.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 8 +++++++-
>>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
>>>>>> index 4fccb21..22c1588 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
>>>>>> @@ -595,6 +595,7 @@ static int vcn_v1_0_start(struct amdgpu_device
>>>>>> *adev)
>>>>>>           struct amdgpu_ring *ring = &adev->vcn.ring_dec;
>>>>>>           uint32_t rb_bufsz, tmp;
>>>>>>           uint32_t lmi_swap_cntl;
>>>>>> +       uint32_t reg_temp;
>>>>>>           int i, j, r;
>>>>>>
>>>>>>           /* disable byte swapping */
>>>>>> @@ -700,6 +701,11 @@ static int vcn_v1_0_start(struct amdgpu_device
>>>>>> *adev)
>>>>>> (UVD_MASTINT_EN__VCPU_EN_MASK|UVD_MASTINT_EN__SYS_EN_MASK),
>>>>>> ~(UVD_MASTINT_EN__VCPU_EN_MASK|UVD_MASTINT_EN__SYS_EN_MASK));
>>>>>>
>>>>>> +       /* enable system interrupt for JRBC*/
>>>>>> +       reg_temp = RREG32(SOC15_REG_OFFSET(UVD, 0, mmUVD_SYS_INT_EN));
>>>>>> +       reg_temp |= UVD_SYS_INT_EN__UVD_JRBC_EN_MASK;
>>>>>> +       WREG32(SOC15_REG_OFFSET(UVD, 0, mmUVD_SYS_INT_EN), reg_temp);
>>>>>> +
>>>>> Shouldn't we move the setting of these interrupts into
>>>>> vcn_v1_0_set_interrupt_state()? Same for the mastint.  that way they
>>>>> will get enabled/disabled as part of the fence driver sequence I
>>>>> think.  Or do they need to happen in a specific sequence?
>>>>>
>>>>> Alex
>>>>
>>>> Hmm... at least for this JPEG specific case, interrupt won't be raised
>>>> during those times that we don't care about the interrupt. This is not like
>>>> other system component where interrupt might still be raised even if we
>>>> don't care about it. So my feeling is that whether we disable it at the
>>>> beginning and enable it later on, or we just enable it at the beginning
>>>> doesn't really matter in the practical sense.
>>>
>>> I agree with Alex here. While we currently don't use that much we would
>>> still like to be able to control interrupts and not just silently enable
>>> them all the time.
>>>
>>> Regards,
>>> Christian.
>>
>> Yes, I agree with you and Alex in terms of controlling interrupts. I did a
>> quick try from my side yesterday, it seems that
>> "vcn_v1_0_set_interrupt_state()" is not being called in fence driver
>> sequence, and not only for jpeg but for vcn in general. I saw that the
>> current "vcn_v1_0_set_interrupt_state()" just returns 0, is there any reason
>> that we didn't use this function before? Seems like this is on purpose to
>> me. Any information regarding to this?
> IIRC, on older versions of UVD there was no control bit for the UVD
> fence interrupt so it was always enabled.   Also, you may want to add
> an irq type so that you can pass that to amdgpu_ring_init() so we can
> enable/disable each irq source independently.
>
> Alex

As discussed, the codes will be leave as it for now. Will add TODO 
comment for future cleanup, and do more test on iqr_put/iqr_get to see 
the possibility of moving both master interrupt and system interrupt to 
set_interrupt call.

Thanks,
Boyuan

>
>> Regards,
>> Boyuan
>>
>>
>>>> Regards,
>>>> Boyuan
>>>>
>>>>>>           /* clear the bit 4 of VCN_STATUS */
>>>>>>           WREG32_P(SOC15_REG_OFFSET(UVD, 0, mmUVD_STATUS), 0,
>>>>>>                           ~(2 << UVD_STATUS__VCPU_REPORT__SHIFT));
>>>>>> @@ -1754,7 +1760,7 @@ static const struct amdgpu_irq_src_funcs
>>>>>> vcn_v1_0_irq_funcs = {
>>>>>>
>>>>>>    static void vcn_v1_0_set_irq_funcs(struct amdgpu_device *adev)
>>>>>>    {
>>>>>> -       adev->vcn.irq.num_types = adev->vcn.num_enc_rings + 1;
>>>>>> +       adev->vcn.irq.num_types = adev->vcn.num_enc_rings + 2;
>>>>>>           adev->vcn.irq.funcs = &vcn_v1_0_irq_funcs;
>>>>>>    }
>>>>>>
>>>>>> --
>>>>>> 2.7.4
>>>>>>
>>>>>> _______________________________________________
>>>>>> amd-gfx mailing list
>>>>>> amd-gfx at lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>