[PATCH 1/2] drm/amd/display: Send vblank and user events at vsartup for DCN

[Li, Sun peng (Leo)]

On 2019-11-05 11:15 a.m., Kazlauskas, Nicholas wrote:
> On 2019-11-05 10:34 a.m., sunpeng.li at amd.com wrote:
>> From: Leo Li <sunpeng.li at amd.com>
>>
>> [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 at amd.com>
>> ---
>>   .../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
>>
>