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

["Kazlauskas, Nicholas" <Nicholas.Kazlauskas-5C7GfCeVMHo@public.gmane.org>]

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@amd.com wrote:
>>> From: Leo Li <sunpeng.li@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@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

Sounds fine to me then.

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx