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
>
>> +
>> + 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
