Re: [PATCH 3/4] drm/amd/display: In VRR mode, do DRM core vblank handling at end of vblank.

["Wentland, Harry" <Harry.Wentland@amd.com>]

On 2019-03-21 5:39 a.m., Mario Kleiner wrote:
> On Wed, Mar 20, 2019 at 1:53 PM Kazlauskas, Nicholas
> <Nicholas.Kazlauskas@amd.com> wrote:
>>
>> On 3/20/19 3:51 AM, Mario Kleiner wrote:
>>> Ok, fixed all the style issues and ran checkpatch over the patches. Thanks.
>>>
>>> On Tue, Mar 19, 2019 at 2:32 PM Kazlauskas, Nicholas
>>> <Nicholas.Kazlauskas@amd.com> wrote:
>>>>
>>>> On 3/19/19 9:23 AM, Kazlauskas, Nicholas wrote:
>>>>> On 3/18/19 1:19 PM, Mario Kleiner wrote:

...snip...

>>>>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>>>> index e04ae49..10ac6de 100644
>>>>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>>>> @@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
>>>>>>               return DC_IRQ_SOURCE_VBLANK5;
>>>>>>       case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
>>>>>>               return DC_IRQ_SOURCE_VBLANK6;
>>>>>> +    case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE1;
>>>>>> +    case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE2;
>>>>>> +    case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE3;
>>>>>> +    case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE4;
>>>>>> +    case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE5;
>>>>>> +    case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE6;
>>>>
>>>> I'm not sure we necessarily want to reuse the same
>>>> DC_IRQ_SOURCE_VUPDATE1...6 definitions here again when it's really
>>>> V_UPDATE_NO_LOCK.
>>>>
>>>
>>> Hm. The problem is that if we use different ones for DCE and DCN we
>>> need special-case handling in amdgpu_dm.c, e.g., in
>>> amdgpu_dm_set_vupdate_irq_state() and dm_set_vupdate_irq() to detect
>>> if it is caling DCE or DCN (how to detect this?) to select different
>>> irq sources IRQ_TYPE_VUPDATE vs IRQ_TYPE_VUPDATE_NO_LOCK and such?
>>> And definitions like "struct amdgpu_irq_src vupdate_irq;" should then
>>> also exist twice as vupdate_irq and vupdate_irq_no_lock for correct
>>> naming?
>>>
>>> Or we'd name the IRQ source and all relevant data structures and
>>> functions something like DC_IRQ_SOURCE_VBLANK_END1..6 to describe what
>>> it signals instead of to what it corresponds in hardware? That would
>>> be what was done with the regular DC_IRQ_SOURCE_VBLANK1..6. It signals
>>> start of vblank, but the underlying hw interrupts are actually a
>>> properly programmed VLINE0 irq on DCE and a VSTARTUP irq on DCN.
>>
>>
>> Ah, I see what you mean. Maybe this is for the better to share the same
>> names then. I'll defer this to Harry.
>>

I'd tend to agree with Mario. I think it's best to keep the same semantic for VUPDATE.

The regular VUPDATE has some interaction with the master lock. I imagine we want something that fires independently of it, which looks to be the _NO_LOCK version.

That said, both VSTARTUP and VUPDATE can occur before VSYNC on DCN. With most timings this probably won't occur but if our back porch is really small we'd see that happening. If we need to guarantee that this interrupt occurs no sooner than vline 0 we will need to register a vline IRQ.

As for VSTARTUP and VUPDATE, VSTARTUP is the IRQ that has an impact on the behavior of render clients as it's the deadline for frame submissions. After VSTARTUP any new framebuffer address update is postponed to the next frame (unless we do immediate flip).

So for now VUPDATE_NO_LOCK is probably fine and will improve our situation except for VRR displays with a really short back porch. Not sure those even exist.

>>>
>>> Of course this all assumes we need to use the NO_LOCK variant on DCN?
>>> We haven't tested what the regular VUPDATE_IRQ does, because i don't
>>> have a suitable test machine, and my use of the NO_LOCK variant was
>>> just based on my interpretation of this little comment in the DCN
>>> header file:
>>>
>>> #define DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT 0x57 //
>>> "OTG0 VUPDATE event without lock interrupt, VUPDATE is update event
>>> for double buffered registers"
>>>
>>> and the absence of any explanatory comment in the define of regular V_UPDATE:
>>>
>>> #define DCN_1_0__SRCID__DC_D1_OTG_V_UPDATE 0x18 // D1 : OTG V_update
>>> OTG1_IHC_V_UPDATE_INTERRUPT
>>>
>>> I assumed the NO_LOCK means not affected by the state of any of the hw
>>> locks, so regular V_UPDATE might be affected by them. You agreed with
>>> that, but we never tested what regular V_UPDATE would do on DCN. Do we
>>> actually know for sure from hw specs that it would not work, ie. not
>>> unconditionally fire the IRQ at every end of VBLANK, as we need?
>>
>> FWIW, I did try your original patches with V_UPDATE. I don't know off
>> the top of my head what specifically V_UPDATE does that V_UPDATE_NO_LOCK
>> doesn't, but at the very least the HW won't be flipping anything to the
>> screen if you're using the former. You'll end up with a static screen
>> that looks like a system hang.
>>
> 
> Yes, but that was because that iteration of the patch had a bug, where
> i defined the irq sources / irq hw sources as _NO_LOCK variants as i
> intended, but forgot to adapt the irq en/disable register macros. You
> fixed my bug during your testing and so we know the _NO_LOCK variant
> works perfectly for our purpose. But we never tesed if the standard
> variant would have worked just as well.
> 
> I will test this later today, because since today i'm the proud owner
> of a PC with Ryzen 5 2400G with Vega11 Raven / DCN-1.0 :). So i'll
> give it a try. Would be good though if you could have a look at the
> hardware docs i assume you have internally to verify what the V_UPDATE
> irq hw source actually does?
> 

I couldn't find much regarding the _NO_LOCK part in the HW docs, unfortunately. The other stuff is in my blurb up top.

I think this change is fine. With the comments from Nick and Paul addressed this is
Acked-by: Harry Wentland <harry.wentland@amd.com>

I recommend trying the VLINE IRQ, though. Nick can you create a task for us to look into that?

Harry

> -mario
> 
>> Nicholas Kazlauskas
>>
>>>
>>>
>>>>>>       case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
>>>>>>               return DC_IRQ_SOURCE_PFLIP1;
>>>>>>       case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
>>>>>> @@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>>>       .ack = NULL
>>>>>>     };
>>>>>>
>>>>>> +static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
>>>>>> +    .set = NULL,
>>>>>> +    .ack = NULL
>>>>>> +};
>>>>>> +
>>>>>>     #define BASE_INNER(seg) \
>>>>>>       DCE_BASE__INST0_SEG ## seg
>>>>>>
>>>>>> @@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>>>               .funcs = &pflip_irq_info_funcs\
>>>>>>       }
>>>>>>
>>>>>> -#define vupdate_int_entry(reg_num)\
>>>>>> +/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
>>>>>> + * of DCE's DC_IRQ_SOURCE_VUPDATEx.
>>>>>> + */
>>>>>> +#define vupdate_no_lock_int_entry(reg_num)\
>>>>>>       [DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
>>>>>>               IRQ_REG_ENTRY(OTG, reg_num,\
>>>>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
>>>>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
>>>>>> -            .funcs = &vblank_irq_info_funcs\
>>>>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
>>>>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
>>>>>> +            .funcs = &vupdate_no_lock_irq_info_funcs\
>>>>>>       }
>>>>>>
>>>>>>     #define vblank_int_entry(reg_num)\
>>>>>> @@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
>>>>>>       dc_underflow_int_entry(6),
>>>>>>       [DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
>>>>>>       [DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
>>>>>> -    vupdate_int_entry(0),
>>>>>> -    vupdate_int_entry(1),
>>>>>> -    vupdate_int_entry(2),
>>>>>> -    vupdate_int_entry(3),
>>>>>> -    vupdate_int_entry(4),
>>>>>> -    vupdate_int_entry(5),
>>>>
>>>> I don't think we should be necessarily dropping these, but I guess it
>>>> could go either way since these IRQs technically aren't defined.
>>>>
>>>
>>> We could keep both definitions around, original vupdate_int_entry +
>>> the new vupdate_no_lock_int_entry.
>>>
>>> -mario
>>>
>>>>>> +    vupdate_no_lock_int_entry(0),
>>>>>> +    vupdate_no_lock_int_entry(1),
>>>>>> +    vupdate_no_lock_int_entry(2),
>>>>>> +    vupdate_no_lock_int_entry(3),
>>>>>> +    vupdate_no_lock_int_entry(4),
>>>>>> +    vupdate_no_lock_int_entry(5),
>>>>>>       vblank_int_entry(0),
>>>>>>       vblank_int_entry(1),
>>>>>>       vblank_int_entry(2),
>>>>>>
>>>>>
>>>>> Nicholas Kazlauskas
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>
>>>>
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel