On 2019-11-05 1:32 p.m., Li, Sun peng (Leo) wrote:
>
>
> On 2019-11-05 11:15 a.m., Kazlauskas, Nicholas wrote:
>> On 2019-11-05 10:34 a.m., sunpeng.li@xxxxxxx wrote:
>>> From: Leo Li <sunpeng.li@xxxxxxx>
>>>
>>> [Why]
>>>
>>> For DCN hardware, the crtc_high_irq handler is assigned to the vstartup
>>> interrupt. This is different from DCE, which has it assigned to vblank
>>> start.
>>>
>>> We'd like to send vblank and user events at vstartup because:
>>>
>>> * It happens close enough to vupdate - the point of no return for HW.
>>>
>>> * It is programmed as lines relative to vblank end - i.e. it is not in
>>> the variable portion when VRR is enabled. We should signal user
>>> events here.
>>>
>>> * The pflip interrupt responsible for sending user events today only
>>> fires if the DCH HUBP component is not clock gated. In situations
>>> where planes are disabled - but the CRTC is enabled - user events won't
>>> be sent out, leading to flip done timeouts.
>>>
>>> Consequently, this makes vupdate on DCN hardware redundant. It will be
>>> removed in the next change.
>>>
>>> [How]
>>>
>>> Add a DCN-specific crtc_high_irq handler, and hook it to the VStartup
>>> signal. Inside the DCN handler, we send off user events if the pflip
>>> handler hasn't already done so.
>>>
>>> Signed-off-by: Leo Li <sunpeng.li@xxxxxxx>
>>> ---
>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 65 ++++++++++++++++++-
>>> 1 file changed, 64 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index 00017b91c91a..256a23a0ec28 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -485,6 +485,69 @@ static void dm_crtc_high_irq(void *interrupt_params)
>>> }
>>> }
>>>
>>> +
>>> +/**
>>> + * dm_dcn_crtc_high_irq() - Handles VStartup interrupt for DCN generation ASICs
>>> + * @interrupt params - interrupt parameters
>>> + *
>>> + * Notify DRM's vblank event handler at VSTARTUP
>>> + *
>>> + * Unlike DCE hardware, we trigger the handler at VSTARTUP. at which:
>>> + * * We are close enough to VUPDATE - the point of no return for hw
>>> + * * We are in the fixed portion of variable front porch when vrr is enabled
>>> + * * We are before VUPDATE, where double-buffered vrr registers are swapped
>>> + *
>>> + * It is therefore the correct place to signal vblank, send user flip events,
>>> + * and update VRR.
>>> + */
>>> +static void dm_dcn_crtc_high_irq(void *interrupt_params)
>>> +{
>>> + struct common_irq_params *irq_params = interrupt_params;
>>> + struct amdgpu_device *adev = irq_params->adev;
>>> + struct amdgpu_crtc *acrtc;
>>> + struct dm_crtc_state *acrtc_state;
>>> + unsigned long flags;
>>> +
>>> + acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
>>> +
>>> + if (!acrtc)
>>> + return;
>>> +
>>> + acrtc_state = to_dm_crtc_state(acrtc->base.state);
>>> +
>>> + DRM_DEBUG_DRIVER("crtc:%d, vupdate-vrr:%d\n", acrtc->crtc_id,
>>> + amdgpu_dm_vrr_active(acrtc_state));
>>> +
>>> + amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
>>> + drm_crtc_handle_vblank(&acrtc->base);
>>
>> Shouldn't this be the other way around? Don't we want the CRC sent back
>> to userspace to have the updated vblank counter?
>>
>> This is how it worked before at least.
>>
>> Other than that, this patch looks fine to me.
>>
>> Nicholas Kazlauskas
>
>
> Looks like we're doing a crtc_accurate_vblank_count() inside the crc handler,
> so I don't think order matters here.
>
> Leo
Sounds fine to me then.
Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
Nicholas Kazlauskas
>
>>
>>> +
>>> + spin_lock_irqsave(&adev->ddev->event_lock, flags);
>>> +
>>> + if (acrtc_state->vrr_params.supported &&
>>> + acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
>>> + mod_freesync_handle_v_update(
>>> + adev->dm.freesync_module,
>>> + acrtc_state->stream,
>>> + &acrtc_state->vrr_params);
>>> +
>>> + dc_stream_adjust_vmin_vmax(
>>> + adev->dm.dc,
>>> + acrtc_state->stream,
>>> + &acrtc_state->vrr_params.adjust);
>>> + }
>>> +
>>> + if (acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED) {
>>> + if (acrtc->event) {
>>> + drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
>>> + acrtc->event = NULL;
>>> + drm_crtc_vblank_put(&acrtc->base);
>>> + }
>>> + acrtc->pflip_status = AMDGPU_FLIP_NONE;
>>> + }
>>> +
>>> + spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
>>> +}
>>> +
>>> static int dm_set_clockgating_state(void *handle,
>>> enum amd_clockgating_state state)
>>> {
>>> @@ -2175,7 +2238,7 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>>> c_irq_params->irq_src = int_params.irq_source;
>>>
>>> amdgpu_dm_irq_register_interrupt(adev, &int_params,
>>> - dm_crtc_high_irq, c_irq_params);
>>> + dm_dcn_crtc_high_irq, c_irq_params);
>>> }
>>>
>>> /* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
>>> --
>>> 2.23.0
>>>
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
