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
>
