On 3/21/19 11:38 AM, Wentland, Harry wrote:
>
>
> On 2019-03-21 5:39 a.m., Mario Kleiner wrote:
>> On Wed, Mar 20, 2019 at 1:53 PM Kazlauskas, Nicholas
>> <nicholas.kazlaus...@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.kazlaus...@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.
Mapping the DC_IRQ_SOURCE_VUPDATE1...6 to the _NO_LOCK interrupt seems
fine to me then.
FWIW on the few displays I tested on Raven the event was usually sent
out rather late - near or after the end of the back porch. I don't think
we have to necessarily worry that it'll happen before vline 0 at least.
It's more about it happening too late I suppose. A vline 0 IRQ would
probably be ideal here I think, but this works for now.
>
>>>>
>>>> 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.wentl...@amd.com>
>
> I recommend trying the VLINE IRQ, though. Nick can you create a task for us
> to look into that?
>
> Harry
Sure.
Nicholas Kazlauskas
>
>> -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
>>>>>>
>>>>>
>>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
