Re: [PATCH 2/2 v2] drm/amd/display: Disable VUpdate interrupt for DCN hardware

["Li, Sun peng (Leo)" <Sunpeng.Li-5C7GfCeVMHo@public.gmane.org>]

On 2019-11-05 11:16 a.m., Kazlauskas, Nicholas wrote:
> On 2019-11-05 10:58 a.m., sunpeng.li@amd.com wrote:
>> From: Leo Li <sunpeng.li@amd.com>
>>
>> [Why]
>>
>> On DCN hardware, the crtc_high_irq handler makes vupdate_high_irq
>> handler redundant.
>>
>> All the vupdate handler does is handle vblank events, and update vrr
>> for DCE hw (excluding VEGA, more on that later). As far as usermode is
>> concerned. vstartup happens close enough to vupdate on DCN that it can
>> be considered the "same". Handling vblank and updating vrr at vstartup
>> effectively replaces vupdate on DCN.
>>
>> Vega is a bit special. Like DCN, the VRR registers on Vega are
>> double-buffered, and swapped at vupdate. But Unlike DCN, it lacks a
>> vstartup interrupt. This means we can't quite remove the vupdate handler
>> for it, since delayerd user events due to vrr are sent off there.
>>
>> [How]
>>
>> Remove registration of VUpdate interrupt handler for DCN. Disable
>> vupdate interrupt if asic family DCN, enable otherwise.
>>
>> Signed-off-by: Leo Li <sunpeng.li@amd.com>
>> ---
>>
>> v2: Don't exclude vega when enabling vupdate interrupts
>>
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 34 +++----------------
>>   1 file changed, 4 insertions(+), 30 deletions(-)
>>
>> 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 256a23a0ec28..3664af3b41a1 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -2241,34 +2241,6 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>>   				dm_dcn_crtc_high_irq, c_irq_params);
>>   	}
>>
>> -	/* Use VUPDATE_NO_LOCK interrupt on DCN, which seems to correspond to
>> -	 * the regular VUPDATE interrupt on DCE. We want DC_IRQ_SOURCE_VUPDATEx
>> -	 * to trigger at end of each vblank, regardless of state of the lock,
>> -	 * matching DCE behaviour.
>> -	 */
>> -	for (i = DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT;
>> -	     i <= DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT + adev->mode_info.num_crtc - 1;
>> -	     i++) {
>> -		r = amdgpu_irq_add_id(adev, SOC15_IH_CLIENTID_DCE, i, &adev->vupdate_irq);
>> -
>> -		if (r) {
>> -			DRM_ERROR("Failed to add vupdate irq id!\n");
>> -			return r;
>> -		}
>> -
>> -		int_params.int_context = INTERRUPT_HIGH_IRQ_CONTEXT;
>> -		int_params.irq_source =
>> -			dc_interrupt_to_irq_source(dc, i, 0);
>> -
>> -		c_irq_params = &adev->dm.vupdate_params[int_params.irq_source - DC_IRQ_SOURCE_VUPDATE1];
>> -
>> -		c_irq_params->adev = adev;
>> -		c_irq_params->irq_src = int_params.irq_source;
>> -
>> -		amdgpu_dm_irq_register_interrupt(adev, &int_params,
>> -				dm_vupdate_high_irq, c_irq_params);
>> -	}
>> -
>>   	/* Use GRPH_PFLIP interrupt */
>>   	for (i = DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT;
>>   			i <= DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT + adev->mode_info.num_crtc - 1;
>> @@ -4266,7 +4238,7 @@ static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
>>   	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
>>   	int rc = 0;
>>
>> -	if (enable) {
>> +	if (enable && adev->family <= AMDGPU_FAMILY_AI) {
>>   		/* vblank irq on -> Only need vupdate irq in vrr mode */
>>   		if (amdgpu_dm_vrr_active(acrtc_state))
>>   			rc = dm_set_vupdate_irq(crtc, true);
>> @@ -6243,6 +6215,7 @@ static void pre_update_freesync_state_on_stream(
>>   static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>>   					    struct dm_crtc_state *new_state)
>>   {
>> +	struct amdgpu_device *adev = old_state->base.crtc->dev->dev_private;
>>   	bool old_vrr_active = amdgpu_dm_vrr_active(old_state);
>>   	bool new_vrr_active = amdgpu_dm_vrr_active(new_state);
>>
>> @@ -6255,7 +6228,8 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>>   		 * We also need vupdate irq for the actual core vblank handling
>>   		 * at end of vblank.
>>   		 */
>> -		dm_set_vupdate_irq(new_state->base.crtc, true);
>> +		if (adev->family <= AMDGPU_FAMILY_AI)
>> +			dm_set_vupdate_irq(new_state->base.crtc, true);
> 
> Can we move the if into dm_set_vupdate_irq directly? We're setting it to 
> false even when we don't have it with this patch.
> 
> Nicholas Kazlauskas

Good point, don't know why I didn't do that in the first place :)

Thanks,
Leo

> 
>>   		drm_crtc_vblank_get(new_state->base.crtc);
>>   		DRM_DEBUG_DRIVER("%s: crtc=%u VRR off->on: Get vblank ref\n",
>>   				 __func__, new_state->base.crtc->base.id);
>> --
>> 2.23.0
>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx