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

[Boyuan Zhang]

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?

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
>