[3/4] drm/amd/display: In VRR mode, do DRM core vblank handling at end of vblank.

Commit Message

Mario Kleiner March 18, 2019, 5:19 p.m. UTC

In VRR mode, proper vblank/pageflip timestamps can only be computed
after the display scanout position has left front-porch. Therefore
delay calls to drm_crtc_handle_vblank(), and thereby calls to
drm_update_vblank_count() and pageflip event delivery, to after the
end of front-porch when in VRR mode.

We add a new vupdate irq, which triggers at the end of the vupdate
interval, ie. at the end of vblank, and calls the core vblank handler
function. The new irq handler is not executed in standard non-VRR
mode, so vblank handling for fixed refresh rate mode is identical
to the past implementation.

Signed-off-by: Mario Kleiner <[e-mail]>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h                |   1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 129 ++++++++++++++++++++-
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   9 ++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c  |  22 ++++
 .../amd/display/dc/irq/dce110/irq_service_dce110.c |   7 +-
 .../amd/display/dc/irq/dce120/irq_service_dce120.c |   7 +-
 .../amd/display/dc/irq/dce80/irq_service_dce80.c   |   6 +-
 .../amd/display/dc/irq/dcn10/irq_service_dcn10.c   |  40 +++++--
 8 files changed, 205 insertions(+), 16 deletions(-)

Comments

Paul Menzel March 19, 2019, 7:17 a.m. UTC | #1

Dear Mario,


On 18.03.19 18:19, Mario Kleiner wrote:
> In VRR mode, proper vblank/pageflip timestamps can only be computed
> after the display scanout position has left front-porch. Therefore
> delay calls to drm_crtc_handle_vblank(), and thereby calls to
> drm_update_vblank_count() and pageflip event delivery, to after the
> end of front-porch when in VRR mode.
> 
> We add a new vupdate irq, which triggers at the end of the vupdate
> interval, ie. at the end of vblank, and calls the core vblank handler
> function. The new irq handler is not executed in standard non-VRR
> mode, so vblank handling for fixed refresh rate mode is identical
> to the past implementation.
> 
> Signed-off-by: Mario Kleiner <[e-mail]>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h                |   1 +
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 129 ++++++++++++++++++++-
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   9 ++
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c  |  22 ++++
>   .../amd/display/dc/irq/dce110/irq_service_dce110.c |   7 +-
>   .../amd/display/dc/irq/dce120/irq_service_dce120.c |   7 +-
>   .../amd/display/dc/irq/dce80/irq_service_dce80.c   |   6 +-
>   .../amd/display/dc/irq/dcn10/irq_service_dcn10.c   |  40 +++++--
>   8 files changed, 205 insertions(+), 16 deletions(-)

[…]

`scripts/checkpatch.pl` shows two errors on your commit.

> +	/* Use VUPDATE interrupt */
> +	for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i+=2) {

ERROR: spaces required around that '+=' (ctx:VxV)
#107: FILE: drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:1490:
+	for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= 
VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i+=2) {
  	 
                    ^

[…]

>   static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
>   {
>   	enum dc_irq_source irq_source;
>   	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>   	struct amdgpu_device *adev = crtc->dev->dev_private;
> +	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
> +	int rc = 0;
> +
> +	if (enable) {
> +		/* vblank irq on -> Only need vupdate irq in vrr mode */
> +		if (amdgpu_dm_vrr_active(acrtc_state))
> +			rc = dm_set_vupdate_irq(crtc, true);
> +	}
> +	else {
> +		/* vblank irq off -> vupdate irq off */
> +		rc = dm_set_vupdate_irq(crtc, false);
> +	}

ERROR: else should follow close brace '}'
#198: FILE: drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:3346:
+	}
+	else {

Also:

WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#258: FILE: drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c:679:
+					   unsigned crtc_id,

[…]


Kind regards,

Paul

Kazlauskas, Nicholas March 19, 2019, 1:23 p.m. UTC | #2

On 3/18/19 1:19 PM, Mario Kleiner wrote:
> In VRR mode, proper vblank/pageflip timestamps can only be computed
> after the display scanout position has left front-porch. Therefore
> delay calls to drm_crtc_handle_vblank(), and thereby calls to
> drm_update_vblank_count() and pageflip event delivery, to after the
> end of front-porch when in VRR mode.
> 
> We add a new vupdate irq, which triggers at the end of the vupdate
> interval, ie. at the end of vblank, and calls the core vblank handler
> function. The new irq handler is not executed in standard non-VRR
> mode, so vblank handling for fixed refresh rate mode is identical
> to the past implementation.
> 
> Signed-off-by: Mario Kleiner <[e-mail]>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h                |   1 +
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 129 ++++++++++++++++++++-
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   9 ++
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c  |  22 ++++
>   .../amd/display/dc/irq/dce110/irq_service_dce110.c |   7 +-
>   .../amd/display/dc/irq/dce120/irq_service_dce120.c |   7 +-
>   .../amd/display/dc/irq/dce80/irq_service_dce80.c   |   6 +-
>   .../amd/display/dc/irq/dcn10/irq_service_dcn10.c   |  40 +++++--
>   8 files changed, 205 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index f88761a..64167dd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -827,6 +827,7 @@ struct amdgpu_device {
>   	/* For pre-DCE11. DCE11 and later are in "struct amdgpu_device->dm" */
>   	struct work_struct		hotplug_work;
>   	struct amdgpu_irq_src		crtc_irq;
> +	struct amdgpu_irq_src		vupdate_irq;
>   	struct amdgpu_irq_src		pageflip_irq;
>   	struct amdgpu_irq_src		hpd_irq;
>   
> 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 85e4f87..555d9e9f 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -315,6 +315,32 @@ static void dm_pflip_high_irq(void *interrupt_params)
>   	drm_crtc_vblank_put(&amdgpu_crtc->base);
>   }
>   
> +static void dm_vupdate_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;
> +
> +	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);
> +
> +	if (acrtc) {
> +		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));
> +
> +		/* Core vblank handling is done here after end of front-porch in
> +		 * vrr mode, as vblank timestamping will give valid results
> +		 * while now done after front-porch. This will also deliver
> +		 * page-flip completion events that have been queued to us
> +		 * if a pageflip happened inside front-porch.
> +		 */
> +		if (amdgpu_dm_vrr_active(acrtc_state))
> +			drm_crtc_handle_vblank(&acrtc->base)
I was thinking that 3 and 4 might have to be merged, but VRR pflip 
timestamping seems to be the same as it was before (off by a line or 
two) since it's not handled here yet. This seems to fix vblank events 
and timestamping at least.

> +	}
> +}
> +
>   static void dm_crtc_high_irq(void *interrupt_params)
>   {
>   	struct common_irq_params *irq_params = interrupt_params;
> @@ -325,11 +351,24 @@ static void dm_crtc_high_irq(void *interrupt_params)
>   	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
>   
>   	if (acrtc) {
> -		drm_crtc_handle_vblank(&acrtc->base);
> -		amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
> -
>   		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));
> +
> +		/* Core vblank handling at start of front-porch is only possible
> +		 * in non-vrr mode, as only there vblank timestamping will give
> +		 * valid results while done in front-porch. Otherwise defer it
> +		 * to dm_vupdate_high_irq after end of front-porch.
> +		 */
> +		if (!amdgpu_dm_vrr_active(acrtc_state))
> +			drm_crtc_handle_vblank(&acrtc->base);
> +
> +		/* Following stuff must happen at start of vblank, for crc
> +		 * computation and below-the-range btr support in vrr mode.
> +		 */
> +		amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
> +
>   		if (acrtc_state->stream &&
>   		    acrtc_state->vrr_params.supported &&
>   		    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
> @@ -1447,6 +1486,27 @@ static int dce110_register_irq_handlers(struct amdgpu_device *adev)
>   				dm_crtc_high_irq, c_irq_params);
>   	}
>   
> +	/* Use VUPDATE interrupt */
> +	for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i+=2) {
> +		r = amdgpu_irq_add_id(adev, client_id, 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 = VISLANDS30_IV_SRCID_D1_GRPH_PFLIP;
>   			i <= VISLANDS30_IV_SRCID_D6_GRPH_PFLIP; i += 2) {
> @@ -1532,6 +1592,34 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>   				dm_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;
> @@ -3226,12 +3314,42 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
>   	return &state->base;
>   }
>   
> +static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
> +{
> +	enum dc_irq_source irq_source;
> +	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> +	struct amdgpu_device *adev = crtc->dev->dev_private;
> +	int rc;
> +
> +	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
> +
> +	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
> +
> +	DRM_DEBUG_DRIVER("%s: crtc %d - vupdate irq %sabling: r=%d\n",
> +			 __func__, acrtc->crtc_id, enable ? "en" : "dis", rc);
> +	return rc;
> +}
>   
>   static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
>   {
>   	enum dc_irq_source irq_source;
>   	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>   	struct amdgpu_device *adev = crtc->dev->dev_private;
> +	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
> +	int rc = 0;
> +
> +	if (enable) {
> +		/* vblank irq on -> Only need vupdate irq in vrr mode */
> +		if (amdgpu_dm_vrr_active(acrtc_state))
> +			rc = dm_set_vupdate_irq(crtc, true);
> +	}
> +	else {

should be } else {

> +		/* vblank irq off -> vupdate irq off */
> +		rc = dm_set_vupdate_irq(crtc, false);
> +	}
> +
> +	if (rc)
> +		return rc;
>   
>   	irq_source = IRQ_TYPE_VBLANK + acrtc->otg_inst;
>   	return dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
> @@ -4741,7 +4859,11 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>   		 * While VRR is active, we must not disable vblank irq, as a
>   		 * reenable after disable would compute bogus vblank/pflip
>   		 * timestamps if it likely happened inside display front-porch.
> +		 *
> +		 * We also need vupdate irq for the actual core vblank handling
> +		 * at end of vblank.
>   		 */
> +		dm_set_vupdate_irq(new_state->base.crtc, true);
>   		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);
> @@ -4750,6 +4872,7 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>   		/* Transition VRR active -> inactive:
>   		 * Allow vblank irq disable again for fixed refresh rate.
>   		 */
> +		dm_set_vupdate_irq(new_state->base.crtc, false);
>   		drm_crtc_vblank_put(new_state->base.crtc);
>   		DRM_DEBUG_DRIVER("%s: crtc=%u VRR on->off: Drop vblank ref\n",
>   				 __func__, new_state->base.crtc->base.id);
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index f741ea3..0b35f84 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -182,6 +182,15 @@ struct amdgpu_display_manager {
>   	struct common_irq_params
>   	vblank_params[DC_IRQ_SOURCE_VBLANK6 - DC_IRQ_SOURCE_VBLANK1 + 1];
>   
> +	/**
> +	 * @vupdate_params:
> +	 *
> +	 * Vertical update IRQ parameters, passed to registered handlers when
> +	 * triggered.
> +	 */
> +	struct common_irq_params
> +	vupdate_params[DC_IRQ_SOURCE_VUPDATE6 - DC_IRQ_SOURCE_VUPDATE1 + 1];
> +
>   	spinlock_t irq_handler_list_table_lock;
>   
>   	struct backlight_device *backlight_dev;
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> index cd10f77..fee7837 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> @@ -674,11 +674,30 @@ static int amdgpu_dm_set_crtc_irq_state(struct amdgpu_device *adev,
>   		__func__);
>   }
>   
> +static int amdgpu_dm_set_vupdate_irq_state(struct amdgpu_device *adev,
> +					   struct amdgpu_irq_src *source,
> +					   unsigned crtc_id,

should be unsigned int crtc_id

> +					   enum amdgpu_interrupt_state state)
> +{
> +	return dm_irq_state(
> +		adev,
> +		source,
> +		crtc_id,
> +		state,
> +		IRQ_TYPE_VUPDATE,
> +		__func__);
> +}
> +
>   static const struct amdgpu_irq_src_funcs dm_crtc_irq_funcs = {
>   	.set = amdgpu_dm_set_crtc_irq_state,
>   	.process = amdgpu_dm_irq_handler,
>   };
>   
> +static const struct amdgpu_irq_src_funcs dm_vupdate_irq_funcs = {
> +	.set = amdgpu_dm_set_vupdate_irq_state,
> +	.process = amdgpu_dm_irq_handler,
> +};
> +
>   static const struct amdgpu_irq_src_funcs dm_pageflip_irq_funcs = {
>   	.set = amdgpu_dm_set_pflip_irq_state,
>   	.process = amdgpu_dm_irq_handler,
> @@ -695,6 +714,9 @@ void amdgpu_dm_set_irq_funcs(struct amdgpu_device *adev)
>   	adev->crtc_irq.num_types = adev->mode_info.num_crtc;
>   	adev->crtc_irq.funcs = &dm_crtc_irq_funcs;
>   
> +	adev->vupdate_irq.num_types = adev->mode_info.num_crtc;
> +	adev->vupdate_irq.funcs = &dm_vupdate_irq_funcs;
> +
>   	adev->pageflip_irq.num_types = adev->mode_info.num_crtc;
>   	adev->pageflip_irq.funcs = &dm_pageflip_irq_funcs;
>   
> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> index afe0876..86987f5 100644
> --- a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> @@ -81,6 +81,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>   	.ack = NULL
>   };
>   
> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> +	.set = NULL,
> +	.ack = NULL
> +};
> +
>   #define hpd_int_entry(reg_num)\
>   	[DC_IRQ_SOURCE_HPD1 + reg_num] = {\
>   		.enable_reg = mmHPD ## reg_num ## _DC_HPD_INT_CONTROL,\
> @@ -137,7 +142,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>   		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>   		.ack_value =\
>   		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> -		.funcs = &vblank_irq_info_funcs\
> +		.funcs = &vupdate_irq_info_funcs\
>   	}
>   
>   #define vblank_int_entry(reg_num)\
> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> index 1ea7256..750ba0a 100644
> --- a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> @@ -84,6 +84,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>   	.ack = NULL
>   };
>   
> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> +	.set = NULL,
> +	.ack = NULL
> +};
> +
>   #define BASE_INNER(seg) \
>   	DCE_BASE__INST0_SEG ## seg
>   
> @@ -140,7 +145,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>   		IRQ_REG_ENTRY(CRTC, reg_num,\
>   			CRTC_INTERRUPT_CONTROL, CRTC_V_UPDATE_INT_MSK,\
>   			CRTC_V_UPDATE_INT_STATUS, CRTC_V_UPDATE_INT_CLEAR),\
> -		.funcs = &vblank_irq_info_funcs\
> +		.funcs = &vupdate_irq_info_funcs\
>   	}
>   
>   #define vblank_int_entry(reg_num)\
> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> index 8a2066c..de218fe 100644
> --- a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> @@ -84,6 +84,10 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>   	.ack = NULL
>   };
>   
> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> +	.set = NULL,
> +	.ack = NULL
> +};
>   
>   #define hpd_int_entry(reg_num)\
>   	[DC_IRQ_SOURCE_INVALID + reg_num] = {\
> @@ -142,7 +146,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>   		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>   		.ack_value =\
>   		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> -		.funcs = &vblank_irq_info_funcs\
> +		.funcs = &vupdate_irq_info_funcs\
>   	}
>   
>   #define vblank_int_entry(reg_num)\
> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> index e04ae49..10ac6de 100644
> --- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> +++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> @@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
>   		return DC_IRQ_SOURCE_VBLANK5;
>   	case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
>   		return DC_IRQ_SOURCE_VBLANK6;
> +	case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> +		return DC_IRQ_SOURCE_VUPDATE1;
> +	case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> +		return DC_IRQ_SOURCE_VUPDATE2;
> +	case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> +		return DC_IRQ_SOURCE_VUPDATE3;
> +	case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> +		return DC_IRQ_SOURCE_VUPDATE4;
> +	case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> +		return DC_IRQ_SOURCE_VUPDATE5;
> +	case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> +		return DC_IRQ_SOURCE_VUPDATE6;
>   	case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
>   		return DC_IRQ_SOURCE_PFLIP1;
>   	case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
> @@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>   	.ack = NULL
>   };
>   
> +static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
> +	.set = NULL,
> +	.ack = NULL
> +};
> +
>   #define BASE_INNER(seg) \
>   	DCE_BASE__INST0_SEG ## seg
>   
> @@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>   		.funcs = &pflip_irq_info_funcs\
>   	}
>   
> -#define vupdate_int_entry(reg_num)\
> +/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
> + * of DCE's DC_IRQ_SOURCE_VUPDATEx.
> + */
> +#define vupdate_no_lock_int_entry(reg_num)\
>   	[DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
>   		IRQ_REG_ENTRY(OTG, reg_num,\
> -			OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
> -			OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
> -		.funcs = &vblank_irq_info_funcs\
> +			OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
> +			OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
> +		.funcs = &vupdate_no_lock_irq_info_funcs\
>   	}
>   
>   #define vblank_int_entry(reg_num)\
> @@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
>   	dc_underflow_int_entry(6),
>   	[DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
>   	[DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
> -	vupdate_int_entry(0),
> -	vupdate_int_entry(1),
> -	vupdate_int_entry(2),
> -	vupdate_int_entry(3),
> -	vupdate_int_entry(4),
> -	vupdate_int_entry(5),
> +	vupdate_no_lock_int_entry(0),
> +	vupdate_no_lock_int_entry(1),
> +	vupdate_no_lock_int_entry(2),
> +	vupdate_no_lock_int_entry(3),
> +	vupdate_no_lock_int_entry(4),
> +	vupdate_no_lock_int_entry(5),
>   	vblank_int_entry(0),
>   	vblank_int_entry(1),
>   	vblank_int_entry(2),
> 

Nicholas Kazlauskas

Kazlauskas, Nicholas March 19, 2019, 1:32 p.m. UTC | #3

On 3/19/19 9:23 AM, Kazlauskas, Nicholas wrote:
> On 3/18/19 1:19 PM, Mario Kleiner wrote:
>> In VRR mode, proper vblank/pageflip timestamps can only be computed
>> after the display scanout position has left front-porch. Therefore
>> delay calls to drm_crtc_handle_vblank(), and thereby calls to
>> drm_update_vblank_count() and pageflip event delivery, to after the
>> end of front-porch when in VRR mode.
>>
>> We add a new vupdate irq, which triggers at the end of the vupdate
>> interval, ie. at the end of vblank, and calls the core vblank handler
>> function. The new irq handler is not executed in standard non-VRR
>> mode, so vblank handling for fixed refresh rate mode is identical
>> to the past implementation.
>>
>> Signed-off-by: Mario Kleiner <[e-mail]>

Looks I lost some of my comments I wanted to send in my last email. Just 
a few nitpicks (including some things Paul mentioned).

Also meant to CC Harry on this as well.

>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu.h                |   1 +
>>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 129 ++++++++++++++++++++-
>>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   9 ++
>>    .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c  |  22 ++++
>>    .../amd/display/dc/irq/dce110/irq_service_dce110.c |   7 +-
>>    .../amd/display/dc/irq/dce120/irq_service_dce120.c |   7 +-
>>    .../amd/display/dc/irq/dce80/irq_service_dce80.c   |   6 +-
>>    .../amd/display/dc/irq/dcn10/irq_service_dcn10.c   |  40 +++++--
>>    8 files changed, 205 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index f88761a..64167dd 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -827,6 +827,7 @@ struct amdgpu_device {
>>    	/* For pre-DCE11. DCE11 and later are in "struct amdgpu_device->dm" */
>>    	struct work_struct		hotplug_work;
>>    	struct amdgpu_irq_src		crtc_irq;
>> +	struct amdgpu_irq_src		vupdate_irq;
>>    	struct amdgpu_irq_src		pageflip_irq;
>>    	struct amdgpu_irq_src		hpd_irq;
>>    
>> 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 85e4f87..555d9e9f 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -315,6 +315,32 @@ static void dm_pflip_high_irq(void *interrupt_params)
>>    	drm_crtc_vblank_put(&amdgpu_crtc->base);
>>    }
>>    
>> +static void dm_vupdate_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;
>> +
>> +	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);
>> +
>> +	if (acrtc) {
>> +		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));
>> +
>> +		/* Core vblank handling is done here after end of front-porch in
>> +		 * vrr mode, as vblank timestamping will give valid results
>> +		 * while now done after front-porch. This will also deliver
>> +		 * page-flip completion events that have been queued to us
>> +		 * if a pageflip happened inside front-porch.
>> +		 */
>> +		if (amdgpu_dm_vrr_active(acrtc_state))
>> +			drm_crtc_handle_vblank(&acrtc->base)
> I was thinking that 3 and 4 might have to be merged, but VRR pflip
> timestamping seems to be the same as it was before (off by a line or
> two) since it's not handled here yet. This seems to fix vblank events
> and timestamping at least.
> 
>> +	}
>> +}
>> +
>>    static void dm_crtc_high_irq(void *interrupt_params)
>>    {
>>    	struct common_irq_params *irq_params = interrupt_params;
>> @@ -325,11 +351,24 @@ static void dm_crtc_high_irq(void *interrupt_params)
>>    	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
>>    
>>    	if (acrtc) {
>> -		drm_crtc_handle_vblank(&acrtc->base);
>> -		amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
>> -
>>    		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));
>> +
>> +		/* Core vblank handling at start of front-porch is only possible
>> +		 * in non-vrr mode, as only there vblank timestamping will give
>> +		 * valid results while done in front-porch. Otherwise defer it
>> +		 * to dm_vupdate_high_irq after end of front-porch.
>> +		 */
>> +		if (!amdgpu_dm_vrr_active(acrtc_state))
>> +			drm_crtc_handle_vblank(&acrtc->base);
>> +
>> +		/* Following stuff must happen at start of vblank, for crc
>> +		 * computation and below-the-range btr support in vrr mode.
>> +		 */
>> +		amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
>> +
>>    		if (acrtc_state->stream &&
>>    		    acrtc_state->vrr_params.supported &&
>>    		    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
>> @@ -1447,6 +1486,27 @@ static int dce110_register_irq_handlers(struct amdgpu_device *adev)
>>    				dm_crtc_high_irq, c_irq_params);
>>    	}
>>    
>> +	/* Use VUPDATE interrupt */
>> +	for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i+=2) {
>> +		r = amdgpu_irq_add_id(adev, client_id, 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 = VISLANDS30_IV_SRCID_D1_GRPH_PFLIP;
>>    			i <= VISLANDS30_IV_SRCID_D6_GRPH_PFLIP; i += 2) {
>> @@ -1532,6 +1592,34 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>>    				dm_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;
>> @@ -3226,12 +3314,42 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
>>    	return &state->base;
>>    }
>>    
>> +static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
>> +{
>> +	enum dc_irq_source irq_source;
>> +	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>> +	struct amdgpu_device *adev = crtc->dev->dev_private;
>> +	int rc;
>> +
>> +	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
>> +
>> +	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
>> +
>> +	DRM_DEBUG_DRIVER("%s: crtc %d - vupdate irq %sabling: r=%d\n",
>> +			 __func__, acrtc->crtc_id, enable ? "en" : "dis", rc);

This extra __func__ is redundant here.

>> +	return rc;
>> +}
>>    
>>    static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
>>    {
>>    	enum dc_irq_source irq_source;
>>    	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>    	struct amdgpu_device *adev = crtc->dev->dev_private;
>> +	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
>> +	int rc = 0;
>> +
>> +	if (enable) {
>> +		/* vblank irq on -> Only need vupdate irq in vrr mode */
>> +		if (amdgpu_dm_vrr_active(acrtc_state))
>> +			rc = dm_set_vupdate_irq(crtc, true);
>> +	}
>> +	else {
> 
> should be } else {
> 
>> +		/* vblank irq off -> vupdate irq off */
>> +		rc = dm_set_vupdate_irq(crtc, false);
>> +	}
>> +
>> +	if (rc)
>> +		return rc;
>>    
>>    	irq_source = IRQ_TYPE_VBLANK + acrtc->otg_inst;
>>    	return dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
>> @@ -4741,7 +4859,11 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>>    		 * While VRR is active, we must not disable vblank irq, as a
>>    		 * reenable after disable would compute bogus vblank/pflip
>>    		 * timestamps if it likely happened inside display front-porch.
>> +		 *
>> +		 * We also need vupdate irq for the actual core vblank handling
>> +		 * at end of vblank.
>>    		 */
>> +		dm_set_vupdate_irq(new_state->base.crtc, true);
>>    		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);
>> @@ -4750,6 +4872,7 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>>    		/* Transition VRR active -> inactive:
>>    		 * Allow vblank irq disable again for fixed refresh rate.
>>    		 */
>> +		dm_set_vupdate_irq(new_state->base.crtc, false);
>>    		drm_crtc_vblank_put(new_state->base.crtc);
>>    		DRM_DEBUG_DRIVER("%s: crtc=%u VRR on->off: Drop vblank ref\n",
>>    				 __func__, new_state->base.crtc->base.id);
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> index f741ea3..0b35f84 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> @@ -182,6 +182,15 @@ struct amdgpu_display_manager {
>>    	struct common_irq_params
>>    	vblank_params[DC_IRQ_SOURCE_VBLANK6 - DC_IRQ_SOURCE_VBLANK1 + 1];
>>    
>> +	/**
>> +	 * @vupdate_params:
>> +	 *
>> +	 * Vertical update IRQ parameters, passed to registered handlers when
>> +	 * triggered.
>> +	 */
>> +	struct common_irq_params
>> +	vupdate_params[DC_IRQ_SOURCE_VUPDATE6 - DC_IRQ_SOURCE_VUPDATE1 + 1];
>> +
>>    	spinlock_t irq_handler_list_table_lock;
>>    
>>    	struct backlight_device *backlight_dev;
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
>> index cd10f77..fee7837 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
>> @@ -674,11 +674,30 @@ static int amdgpu_dm_set_crtc_irq_state(struct amdgpu_device *adev,
>>    		__func__);
>>    }
>>    
>> +static int amdgpu_dm_set_vupdate_irq_state(struct amdgpu_device *adev,
>> +					   struct amdgpu_irq_src *source,
>> +					   unsigned crtc_id,
> 
> should be unsigned int crtc_id
> 
>> +					   enum amdgpu_interrupt_state state)
>> +{
>> +	return dm_irq_state(
>> +		adev,
>> +		source,
>> +		crtc_id,
>> +		state,
>> +		IRQ_TYPE_VUPDATE,
>> +		__func__);
>> +}
>> +
>>    static const struct amdgpu_irq_src_funcs dm_crtc_irq_funcs = {
>>    	.set = amdgpu_dm_set_crtc_irq_state,
>>    	.process = amdgpu_dm_irq_handler,
>>    };
>>    
>> +static const struct amdgpu_irq_src_funcs dm_vupdate_irq_funcs = {
>> +	.set = amdgpu_dm_set_vupdate_irq_state,
>> +	.process = amdgpu_dm_irq_handler,
>> +};
>> +
>>    static const struct amdgpu_irq_src_funcs dm_pageflip_irq_funcs = {
>>    	.set = amdgpu_dm_set_pflip_irq_state,
>>    	.process = amdgpu_dm_irq_handler,
>> @@ -695,6 +714,9 @@ void amdgpu_dm_set_irq_funcs(struct amdgpu_device *adev)
>>    	adev->crtc_irq.num_types = adev->mode_info.num_crtc;
>>    	adev->crtc_irq.funcs = &dm_crtc_irq_funcs;
>>    
>> +	adev->vupdate_irq.num_types = adev->mode_info.num_crtc;
>> +	adev->vupdate_irq.funcs = &dm_vupdate_irq_funcs;
>> +
>>    	adev->pageflip_irq.num_types = adev->mode_info.num_crtc;
>>    	adev->pageflip_irq.funcs = &dm_pageflip_irq_funcs;
>>    
>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
>> index afe0876..86987f5 100644
>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
>> @@ -81,6 +81,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>    	.ack = NULL
>>    };
>>    
>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
>> +	.set = NULL,
>> +	.ack = NULL
>> +};
>> +
>>    #define hpd_int_entry(reg_num)\
>>    	[DC_IRQ_SOURCE_HPD1 + reg_num] = {\
>>    		.enable_reg = mmHPD ## reg_num ## _DC_HPD_INT_CONTROL,\
>> @@ -137,7 +142,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>    		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>>    		.ack_value =\
>>    		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>> -		.funcs = &vblank_irq_info_funcs\
>> +		.funcs = &vupdate_irq_info_funcs\
>>    	}
>>    
>>    #define vblank_int_entry(reg_num)\
>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
>> index 1ea7256..750ba0a 100644
>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
>> @@ -84,6 +84,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>    	.ack = NULL
>>    };
>>    
>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
>> +	.set = NULL,
>> +	.ack = NULL
>> +};
>> +
>>    #define BASE_INNER(seg) \
>>    	DCE_BASE__INST0_SEG ## seg
>>    
>> @@ -140,7 +145,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>    		IRQ_REG_ENTRY(CRTC, reg_num,\
>>    			CRTC_INTERRUPT_CONTROL, CRTC_V_UPDATE_INT_MSK,\
>>    			CRTC_V_UPDATE_INT_STATUS, CRTC_V_UPDATE_INT_CLEAR),\
>> -		.funcs = &vblank_irq_info_funcs\
>> +		.funcs = &vupdate_irq_info_funcs\
>>    	}
>>    
>>    #define vblank_int_entry(reg_num)\
>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
>> index 8a2066c..de218fe 100644
>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
>> @@ -84,6 +84,10 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>    	.ack = NULL
>>    };
>>    
>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
>> +	.set = NULL,
>> +	.ack = NULL
>> +};
>>    
>>    #define hpd_int_entry(reg_num)\
>>    	[DC_IRQ_SOURCE_INVALID + reg_num] = {\
>> @@ -142,7 +146,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>    		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>>    		.ack_value =\
>>    		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>> -		.funcs = &vblank_irq_info_funcs\
>> +		.funcs = &vupdate_irq_info_funcs\
>>    	}
>>    
>>    #define vblank_int_entry(reg_num)\
>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>> index e04ae49..10ac6de 100644
>> --- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>> @@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
>>    		return DC_IRQ_SOURCE_VBLANK5;
>>    	case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
>>    		return DC_IRQ_SOURCE_VBLANK6;
>> +	case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>> +		return DC_IRQ_SOURCE_VUPDATE1;
>> +	case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>> +		return DC_IRQ_SOURCE_VUPDATE2;
>> +	case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>> +		return DC_IRQ_SOURCE_VUPDATE3;
>> +	case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>> +		return DC_IRQ_SOURCE_VUPDATE4;
>> +	case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>> +		return DC_IRQ_SOURCE_VUPDATE5;
>> +	case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>> +		return DC_IRQ_SOURCE_VUPDATE6;

I'm not sure we necessarily want to reuse the same 
DC_IRQ_SOURCE_VUPDATE1...6 definitions here again when it's really 
V_UPDATE_NO_LOCK.

>>    	case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
>>    		return DC_IRQ_SOURCE_PFLIP1;
>>    	case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
>> @@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>    	.ack = NULL
>>    };
>>    
>> +static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
>> +	.set = NULL,
>> +	.ack = NULL
>> +};
>> +
>>    #define BASE_INNER(seg) \
>>    	DCE_BASE__INST0_SEG ## seg
>>    
>> @@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>    		.funcs = &pflip_irq_info_funcs\
>>    	}
>>    
>> -#define vupdate_int_entry(reg_num)\
>> +/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
>> + * of DCE's DC_IRQ_SOURCE_VUPDATEx.
>> + */
>> +#define vupdate_no_lock_int_entry(reg_num)\
>>    	[DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
>>    		IRQ_REG_ENTRY(OTG, reg_num,\
>> -			OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
>> -			OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
>> -		.funcs = &vblank_irq_info_funcs\
>> +			OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
>> +			OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
>> +		.funcs = &vupdate_no_lock_irq_info_funcs\
>>    	}
>>    
>>    #define vblank_int_entry(reg_num)\
>> @@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
>>    	dc_underflow_int_entry(6),
>>    	[DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
>>    	[DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
>> -	vupdate_int_entry(0),
>> -	vupdate_int_entry(1),
>> -	vupdate_int_entry(2),
>> -	vupdate_int_entry(3),
>> -	vupdate_int_entry(4),
>> -	vupdate_int_entry(5),

I don't think we should be necessarily dropping these, but I guess it 
could go either way since these IRQs technically aren't defined.

>> +	vupdate_no_lock_int_entry(0),
>> +	vupdate_no_lock_int_entry(1),
>> +	vupdate_no_lock_int_entry(2),
>> +	vupdate_no_lock_int_entry(3),
>> +	vupdate_no_lock_int_entry(4),
>> +	vupdate_no_lock_int_entry(5),
>>    	vblank_int_entry(0),
>>    	vblank_int_entry(1),
>>    	vblank_int_entry(2),
>>
> 
> Nicholas Kazlauskas
> _______________________________________________
> amd-gfx mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>

Mario Kleiner March 20, 2019, 7:51 a.m. UTC | #4

Ok, fixed all the style issues and ran checkpatch over the patches. Thanks.

On Tue, Mar 19, 2019 at 2:32 PM Kazlauskas, Nicholas
<[e-mail]> wrote:
>
> On 3/19/19 9:23 AM, Kazlauskas, Nicholas wrote:
> > On 3/18/19 1:19 PM, Mario Kleiner wrote:
> >> In VRR mode, proper vblank/pageflip timestamps can only be computed
> >> after the display scanout position has left front-porch. Therefore
> >> delay calls to drm_crtc_handle_vblank(), and thereby calls to
> >> drm_update_vblank_count() and pageflip event delivery, to after the
> >> end of front-porch when in VRR mode.
> >>
> >> We add a new vupdate irq, which triggers at the end of the vupdate
> >> interval, ie. at the end of vblank, and calls the core vblank handler
> >> function. The new irq handler is not executed in standard non-VRR
> >> mode, so vblank handling for fixed refresh rate mode is identical
> >> to the past implementation.
> >>
> >> Signed-off-by: Mario Kleiner <[e-mail]>
>
> Looks I lost some of my comments I wanted to send in my last email. Just
> a few nitpicks (including some things Paul mentioned).
>
> Also meant to CC Harry on this as well.
>
> >> ---
> >>    drivers/gpu/drm/amd/amdgpu/amdgpu.h                |   1 +
> >>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 129 ++++++++++++++++++++-
> >>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   9 ++
> >>    .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c  |  22 ++++
> >>    .../amd/display/dc/irq/dce110/irq_service_dce110.c |   7 +-
> >>    .../amd/display/dc/irq/dce120/irq_service_dce120.c |   7 +-
> >>    .../amd/display/dc/irq/dce80/irq_service_dce80.c   |   6 +-
> >>    .../amd/display/dc/irq/dcn10/irq_service_dcn10.c   |  40 +++++--
> >>    8 files changed, 205 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >> index f88761a..64167dd 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >> @@ -827,6 +827,7 @@ struct amdgpu_device {
> >>      /* For pre-DCE11. DCE11 and later are in "struct amdgpu_device->dm" */
> >>      struct work_struct              hotplug_work;
> >>      struct amdgpu_irq_src           crtc_irq;
> >> +    struct amdgpu_irq_src           vupdate_irq;
> >>      struct amdgpu_irq_src           pageflip_irq;
> >>      struct amdgpu_irq_src           hpd_irq;
> >>
> >> 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 85e4f87..555d9e9f 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> @@ -315,6 +315,32 @@ static void dm_pflip_high_irq(void *interrupt_params)
> >>      drm_crtc_vblank_put(&amdgpu_crtc->base);
> >>    }
> >>
> >> +static void dm_vupdate_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;
> >> +
> >> +    acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);
> >> +
> >> +    if (acrtc) {
> >> +            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));
> >> +
> >> +            /* Core vblank handling is done here after end of front-porch in
> >> +             * vrr mode, as vblank timestamping will give valid results
> >> +             * while now done after front-porch. This will also deliver
> >> +             * page-flip completion events that have been queued to us
> >> +             * if a pageflip happened inside front-porch.
> >> +             */
> >> +            if (amdgpu_dm_vrr_active(acrtc_state))
> >> +                    drm_crtc_handle_vblank(&acrtc->base)
> > I was thinking that 3 and 4 might have to be merged, but VRR pflip
> > timestamping seems to be the same as it was before (off by a line or
> > two) since it's not handled here yet. This seems to fix vblank events
> > and timestamping at least.
> >
> >> +    }
> >> +}
> >> +
> >>    static void dm_crtc_high_irq(void *interrupt_params)
> >>    {
> >>      struct common_irq_params *irq_params = interrupt_params;
> >> @@ -325,11 +351,24 @@ static void dm_crtc_high_irq(void *interrupt_params)
> >>      acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
> >>
> >>      if (acrtc) {
> >> -            drm_crtc_handle_vblank(&acrtc->base);
> >> -            amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
> >> -
> >>              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));
> >> +
> >> +            /* Core vblank handling at start of front-porch is only possible
> >> +             * in non-vrr mode, as only there vblank timestamping will give
> >> +             * valid results while done in front-porch. Otherwise defer it
> >> +             * to dm_vupdate_high_irq after end of front-porch.
> >> +             */
> >> +            if (!amdgpu_dm_vrr_active(acrtc_state))
> >> +                    drm_crtc_handle_vblank(&acrtc->base);
> >> +
> >> +            /* Following stuff must happen at start of vblank, for crc
> >> +             * computation and below-the-range btr support in vrr mode.
> >> +             */
> >> +            amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
> >> +
> >>              if (acrtc_state->stream &&
> >>                  acrtc_state->vrr_params.supported &&
> >>                  acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
> >> @@ -1447,6 +1486,27 @@ static int dce110_register_irq_handlers(struct amdgpu_device *adev)
> >>                              dm_crtc_high_irq, c_irq_params);
> >>      }
> >>
> >> +    /* Use VUPDATE interrupt */
> >> +    for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i+=2) {
> >> +            r = amdgpu_irq_add_id(adev, client_id, 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 = VISLANDS30_IV_SRCID_D1_GRPH_PFLIP;
> >>                      i <= VISLANDS30_IV_SRCID_D6_GRPH_PFLIP; i += 2) {
> >> @@ -1532,6 +1592,34 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
> >>                              dm_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;
> >> @@ -3226,12 +3314,42 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
> >>      return &state->base;
> >>    }
> >>
> >> +static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
> >> +{
> >> +    enum dc_irq_source irq_source;
> >> +    struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> >> +    struct amdgpu_device *adev = crtc->dev->dev_private;
> >> +    int rc;
> >> +
> >> +    irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
> >> +
> >> +    rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
> >> +
> >> +    DRM_DEBUG_DRIVER("%s: crtc %d - vupdate irq %sabling: r=%d\n",
> >> +                     __func__, acrtc->crtc_id, enable ? "en" : "dis", rc);
>
> This extra __func__ is redundant here.
>

Yep. I think maybe the whole debug message is redundant by now. Maybe drop it?

> >> +    return rc;
> >> +}
> >>
> >>    static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
> >>    {
> >>      enum dc_irq_source irq_source;
> >>      struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> >>      struct amdgpu_device *adev = crtc->dev->dev_private;
> >> +    struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
> >> +    int rc = 0;
> >> +
> >> +    if (enable) {
> >> +            /* vblank irq on -> Only need vupdate irq in vrr mode */
> >> +            if (amdgpu_dm_vrr_active(acrtc_state))
> >> +                    rc = dm_set_vupdate_irq(crtc, true);
> >> +    }
> >> +    else {
> >
> > should be } else {
> >
> >> +            /* vblank irq off -> vupdate irq off */
> >> +            rc = dm_set_vupdate_irq(crtc, false);
> >> +    }
> >> +
> >> +    if (rc)
> >> +            return rc;
> >>
> >>      irq_source = IRQ_TYPE_VBLANK + acrtc->otg_inst;
> >>      return dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
> >> @@ -4741,7 +4859,11 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
> >>               * While VRR is active, we must not disable vblank irq, as a
> >>               * reenable after disable would compute bogus vblank/pflip
> >>               * timestamps if it likely happened inside display front-porch.
> >> +             *
> >> +             * We also need vupdate irq for the actual core vblank handling
> >> +             * at end of vblank.
> >>               */
> >> +            dm_set_vupdate_irq(new_state->base.crtc, true);
> >>              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);
> >> @@ -4750,6 +4872,7 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
> >>              /* Transition VRR active -> inactive:
> >>               * Allow vblank irq disable again for fixed refresh rate.
> >>               */
> >> +            dm_set_vupdate_irq(new_state->base.crtc, false);
> >>              drm_crtc_vblank_put(new_state->base.crtc);
> >>              DRM_DEBUG_DRIVER("%s: crtc=%u VRR on->off: Drop vblank ref\n",
> >>                               __func__, new_state->base.crtc->base.id);
> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> >> index f741ea3..0b35f84 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> >> @@ -182,6 +182,15 @@ struct amdgpu_display_manager {
> >>      struct common_irq_params
> >>      vblank_params[DC_IRQ_SOURCE_VBLANK6 - DC_IRQ_SOURCE_VBLANK1 + 1];
> >>
> >> +    /**
> >> +     * @vupdate_params:
> >> +     *
> >> +     * Vertical update IRQ parameters, passed to registered handlers when
> >> +     * triggered.
> >> +     */
> >> +    struct common_irq_params
> >> +    vupdate_params[DC_IRQ_SOURCE_VUPDATE6 - DC_IRQ_SOURCE_VUPDATE1 + 1];
> >> +
> >>      spinlock_t irq_handler_list_table_lock;
> >>
> >>      struct backlight_device *backlight_dev;
> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> >> index cd10f77..fee7837 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> >> @@ -674,11 +674,30 @@ static int amdgpu_dm_set_crtc_irq_state(struct amdgpu_device *adev,
> >>              __func__);
> >>    }
> >>
> >> +static int amdgpu_dm_set_vupdate_irq_state(struct amdgpu_device *adev,
> >> +                                       struct amdgpu_irq_src *source,
> >> +                                       unsigned crtc_id,
> >
> > should be unsigned int crtc_id
> >
> >> +                                       enum amdgpu_interrupt_state state)
> >> +{
> >> +    return dm_irq_state(
> >> +            adev,
> >> +            source,
> >> +            crtc_id,
> >> +            state,
> >> +            IRQ_TYPE_VUPDATE,
> >> +            __func__);
> >> +}
> >> +
> >>    static const struct amdgpu_irq_src_funcs dm_crtc_irq_funcs = {
> >>      .set = amdgpu_dm_set_crtc_irq_state,
> >>      .process = amdgpu_dm_irq_handler,
> >>    };
> >>
> >> +static const struct amdgpu_irq_src_funcs dm_vupdate_irq_funcs = {
> >> +    .set = amdgpu_dm_set_vupdate_irq_state,
> >> +    .process = amdgpu_dm_irq_handler,
> >> +};
> >> +
> >>    static const struct amdgpu_irq_src_funcs dm_pageflip_irq_funcs = {
> >>      .set = amdgpu_dm_set_pflip_irq_state,
> >>      .process = amdgpu_dm_irq_handler,
> >> @@ -695,6 +714,9 @@ void amdgpu_dm_set_irq_funcs(struct amdgpu_device *adev)
> >>      adev->crtc_irq.num_types = adev->mode_info.num_crtc;
> >>      adev->crtc_irq.funcs = &dm_crtc_irq_funcs;
> >>
> >> +    adev->vupdate_irq.num_types = adev->mode_info.num_crtc;
> >> +    adev->vupdate_irq.funcs = &dm_vupdate_irq_funcs;
> >> +
> >>      adev->pageflip_irq.num_types = adev->mode_info.num_crtc;
> >>      adev->pageflip_irq.funcs = &dm_pageflip_irq_funcs;
> >>
> >> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> >> index afe0876..86987f5 100644
> >> --- a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> >> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> >> @@ -81,6 +81,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>      .ack = NULL
> >>    };
> >>
> >> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> >> +    .set = NULL,
> >> +    .ack = NULL
> >> +};
> >> +
> >>    #define hpd_int_entry(reg_num)\
> >>      [DC_IRQ_SOURCE_HPD1 + reg_num] = {\
> >>              .enable_reg = mmHPD ## reg_num ## _DC_HPD_INT_CONTROL,\
> >> @@ -137,7 +142,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>              CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> >>              .ack_value =\
> >>              CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> >> -            .funcs = &vblank_irq_info_funcs\
> >> +            .funcs = &vupdate_irq_info_funcs\
> >>      }
> >>
> >>    #define vblank_int_entry(reg_num)\
> >> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> >> index 1ea7256..750ba0a 100644
> >> --- a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> >> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> >> @@ -84,6 +84,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>      .ack = NULL
> >>    };
> >>
> >> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> >> +    .set = NULL,
> >> +    .ack = NULL
> >> +};
> >> +
> >>    #define BASE_INNER(seg) \
> >>      DCE_BASE__INST0_SEG ## seg
> >>
> >> @@ -140,7 +145,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>              IRQ_REG_ENTRY(CRTC, reg_num,\
> >>                      CRTC_INTERRUPT_CONTROL, CRTC_V_UPDATE_INT_MSK,\
> >>                      CRTC_V_UPDATE_INT_STATUS, CRTC_V_UPDATE_INT_CLEAR),\
> >> -            .funcs = &vblank_irq_info_funcs\
> >> +            .funcs = &vupdate_irq_info_funcs\
> >>      }
> >>
> >>    #define vblank_int_entry(reg_num)\
> >> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> >> index 8a2066c..de218fe 100644
> >> --- a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> >> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> >> @@ -84,6 +84,10 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>      .ack = NULL
> >>    };
> >>
> >> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> >> +    .set = NULL,
> >> +    .ack = NULL
> >> +};
> >>
> >>    #define hpd_int_entry(reg_num)\
> >>      [DC_IRQ_SOURCE_INVALID + reg_num] = {\
> >> @@ -142,7 +146,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>              CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> >>              .ack_value =\
> >>              CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> >> -            .funcs = &vblank_irq_info_funcs\
> >> +            .funcs = &vupdate_irq_info_funcs\
> >>      }
> >>
> >>    #define vblank_int_entry(reg_num)\
> >> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> >> index e04ae49..10ac6de 100644
> >> --- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> >> +++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> >> @@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
> >>              return DC_IRQ_SOURCE_VBLANK5;
> >>      case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
> >>              return DC_IRQ_SOURCE_VBLANK6;
> >> +    case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >> +            return DC_IRQ_SOURCE_VUPDATE1;
> >> +    case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >> +            return DC_IRQ_SOURCE_VUPDATE2;
> >> +    case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >> +            return DC_IRQ_SOURCE_VUPDATE3;
> >> +    case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >> +            return DC_IRQ_SOURCE_VUPDATE4;
> >> +    case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >> +            return DC_IRQ_SOURCE_VUPDATE5;
> >> +    case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >> +            return DC_IRQ_SOURCE_VUPDATE6;
>
> I'm not sure we necessarily want to reuse the same
> DC_IRQ_SOURCE_VUPDATE1...6 definitions here again when it's really
> V_UPDATE_NO_LOCK.
>

Hm. The problem is that if we use different ones for DCE and DCN we
need special-case handling in amdgpu_dm.c, e.g., in
amdgpu_dm_set_vupdate_irq_state() and dm_set_vupdate_irq() to detect
if it is caling DCE or DCN (how to detect this?) to select different
irq sources IRQ_TYPE_VUPDATE vs IRQ_TYPE_VUPDATE_NO_LOCK and such?
And definitions like "struct amdgpu_irq_src vupdate_irq;" should then
also exist twice as vupdate_irq and vupdate_irq_no_lock for correct
naming?

Or we'd name the IRQ source and all relevant data structures and
functions something like DC_IRQ_SOURCE_VBLANK_END1..6 to describe what
it signals instead of to what it corresponds in hardware? That would
be what was done with the regular DC_IRQ_SOURCE_VBLANK1..6. It signals
start of vblank, but the underlying hw interrupts are actually a
properly programmed VLINE0 irq on DCE and a VSTARTUP irq on DCN.

Of course this all assumes we need to use the NO_LOCK variant on DCN?
We haven't tested what the regular VUPDATE_IRQ does, because i don't
have a suitable test machine, and my use of the NO_LOCK variant was
just based on my interpretation of this little comment in the DCN
header file:

#define DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT 0x57 //
"OTG0 VUPDATE event without lock interrupt, VUPDATE is update event
for double buffered registers"

and the absence of any explanatory comment in the define of regular V_UPDATE:

#define DCN_1_0__SRCID__DC_D1_OTG_V_UPDATE 0x18 // D1 : OTG V_update
OTG1_IHC_V_UPDATE_INTERRUPT

I assumed the NO_LOCK means not affected by the state of any of the hw
locks, so regular V_UPDATE might be affected by them. You agreed with
that, but we never tested what regular V_UPDATE would do on DCN. Do we
actually know for sure from hw specs that it would not work, ie. not
unconditionally fire the IRQ at every end of VBLANK, as we need?


> >>      case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
> >>              return DC_IRQ_SOURCE_PFLIP1;
> >>      case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
> >> @@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>      .ack = NULL
> >>    };
> >>
> >> +static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
> >> +    .set = NULL,
> >> +    .ack = NULL
> >> +};
> >> +
> >>    #define BASE_INNER(seg) \
> >>      DCE_BASE__INST0_SEG ## seg
> >>
> >> @@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>              .funcs = &pflip_irq_info_funcs\
> >>      }
> >>
> >> -#define vupdate_int_entry(reg_num)\
> >> +/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
> >> + * of DCE's DC_IRQ_SOURCE_VUPDATEx.
> >> + */
> >> +#define vupdate_no_lock_int_entry(reg_num)\
> >>      [DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
> >>              IRQ_REG_ENTRY(OTG, reg_num,\
> >> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
> >> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
> >> -            .funcs = &vblank_irq_info_funcs\
> >> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
> >> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
> >> +            .funcs = &vupdate_no_lock_irq_info_funcs\
> >>      }
> >>
> >>    #define vblank_int_entry(reg_num)\
> >> @@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
> >>      dc_underflow_int_entry(6),
> >>      [DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
> >>      [DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
> >> -    vupdate_int_entry(0),
> >> -    vupdate_int_entry(1),
> >> -    vupdate_int_entry(2),
> >> -    vupdate_int_entry(3),
> >> -    vupdate_int_entry(4),
> >> -    vupdate_int_entry(5),
>
> I don't think we should be necessarily dropping these, but I guess it
> could go either way since these IRQs technically aren't defined.
>

We could keep both definitions around, original vupdate_int_entry +
the new vupdate_no_lock_int_entry.

-mario

> >> +    vupdate_no_lock_int_entry(0),
> >> +    vupdate_no_lock_int_entry(1),
> >> +    vupdate_no_lock_int_entry(2),
> >> +    vupdate_no_lock_int_entry(3),
> >> +    vupdate_no_lock_int_entry(4),
> >> +    vupdate_no_lock_int_entry(5),
> >>      vblank_int_entry(0),
> >>      vblank_int_entry(1),
> >>      vblank_int_entry(2),
> >>
> >
> > Nicholas Kazlauskas
> > _______________________________________________
> > amd-gfx mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> >
>

Kazlauskas, Nicholas March 20, 2019, 12:53 p.m. UTC | #5

On 3/20/19 3:51 AM, Mario Kleiner wrote:
> Ok, fixed all the style issues and ran checkpatch over the patches. Thanks.
> 
> On Tue, Mar 19, 2019 at 2:32 PM Kazlauskas, Nicholas
> <[e-mail]> wrote:
>>
>> On 3/19/19 9:23 AM, Kazlauskas, Nicholas wrote:
>>> On 3/18/19 1:19 PM, Mario Kleiner wrote:
>>>> In VRR mode, proper vblank/pageflip timestamps can only be computed
>>>> after the display scanout position has left front-porch. Therefore
>>>> delay calls to drm_crtc_handle_vblank(), and thereby calls to
>>>> drm_update_vblank_count() and pageflip event delivery, to after the
>>>> end of front-porch when in VRR mode.
>>>>
>>>> We add a new vupdate irq, which triggers at the end of the vupdate
>>>> interval, ie. at the end of vblank, and calls the core vblank handler
>>>> function. The new irq handler is not executed in standard non-VRR
>>>> mode, so vblank handling for fixed refresh rate mode is identical
>>>> to the past implementation.
>>>>
>>>> Signed-off-by: Mario Kleiner <[e-mail]>
>>
>> Looks I lost some of my comments I wanted to send in my last email. Just
>> a few nitpicks (including some things Paul mentioned).
>>
>> Also meant to CC Harry on this as well.
>>
>>>> ---
>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu.h                |   1 +
>>>>     drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 129 ++++++++++++++++++++-
>>>>     drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   9 ++
>>>>     .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c  |  22 ++++
>>>>     .../amd/display/dc/irq/dce110/irq_service_dce110.c |   7 +-
>>>>     .../amd/display/dc/irq/dce120/irq_service_dce120.c |   7 +-
>>>>     .../amd/display/dc/irq/dce80/irq_service_dce80.c   |   6 +-
>>>>     .../amd/display/dc/irq/dcn10/irq_service_dcn10.c   |  40 +++++--
>>>>     8 files changed, 205 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> index f88761a..64167dd 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>> @@ -827,6 +827,7 @@ struct amdgpu_device {
>>>>       /* For pre-DCE11. DCE11 and later are in "struct amdgpu_device->dm" */
>>>>       struct work_struct              hotplug_work;
>>>>       struct amdgpu_irq_src           crtc_irq;
>>>> +    struct amdgpu_irq_src           vupdate_irq;
>>>>       struct amdgpu_irq_src           pageflip_irq;
>>>>       struct amdgpu_irq_src           hpd_irq;
>>>>
>>>> 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 85e4f87..555d9e9f 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -315,6 +315,32 @@ static void dm_pflip_high_irq(void *interrupt_params)
>>>>       drm_crtc_vblank_put(&amdgpu_crtc->base);
>>>>     }
>>>>
>>>> +static void dm_vupdate_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;
>>>> +
>>>> +    acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);
>>>> +
>>>> +    if (acrtc) {
>>>> +            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));
>>>> +
>>>> +            /* Core vblank handling is done here after end of front-porch in
>>>> +             * vrr mode, as vblank timestamping will give valid results
>>>> +             * while now done after front-porch. This will also deliver
>>>> +             * page-flip completion events that have been queued to us
>>>> +             * if a pageflip happened inside front-porch.
>>>> +             */
>>>> +            if (amdgpu_dm_vrr_active(acrtc_state))
>>>> +                    drm_crtc_handle_vblank(&acrtc->base)
>>> I was thinking that 3 and 4 might have to be merged, but VRR pflip
>>> timestamping seems to be the same as it was before (off by a line or
>>> two) since it's not handled here yet. This seems to fix vblank events
>>> and timestamping at least.
>>>
>>>> +    }
>>>> +}
>>>> +
>>>>     static void dm_crtc_high_irq(void *interrupt_params)
>>>>     {
>>>>       struct common_irq_params *irq_params = interrupt_params;
>>>> @@ -325,11 +351,24 @@ static void dm_crtc_high_irq(void *interrupt_params)
>>>>       acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
>>>>
>>>>       if (acrtc) {
>>>> -            drm_crtc_handle_vblank(&acrtc->base);
>>>> -            amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
>>>> -
>>>>               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));
>>>> +
>>>> +            /* Core vblank handling at start of front-porch is only possible
>>>> +             * in non-vrr mode, as only there vblank timestamping will give
>>>> +             * valid results while done in front-porch. Otherwise defer it
>>>> +             * to dm_vupdate_high_irq after end of front-porch.
>>>> +             */
>>>> +            if (!amdgpu_dm_vrr_active(acrtc_state))
>>>> +                    drm_crtc_handle_vblank(&acrtc->base);
>>>> +
>>>> +            /* Following stuff must happen at start of vblank, for crc
>>>> +             * computation and below-the-range btr support in vrr mode.
>>>> +             */
>>>> +            amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
>>>> +
>>>>               if (acrtc_state->stream &&
>>>>                   acrtc_state->vrr_params.supported &&
>>>>                   acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
>>>> @@ -1447,6 +1486,27 @@ static int dce110_register_irq_handlers(struct amdgpu_device *adev)
>>>>                               dm_crtc_high_irq, c_irq_params);
>>>>       }
>>>>
>>>> +    /* Use VUPDATE interrupt */
>>>> +    for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i+=2) {
>>>> +            r = amdgpu_irq_add_id(adev, client_id, 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 = VISLANDS30_IV_SRCID_D1_GRPH_PFLIP;
>>>>                       i <= VISLANDS30_IV_SRCID_D6_GRPH_PFLIP; i += 2) {
>>>> @@ -1532,6 +1592,34 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
>>>>                               dm_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;
>>>> @@ -3226,12 +3314,42 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
>>>>       return &state->base;
>>>>     }
>>>>
>>>> +static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
>>>> +{
>>>> +    enum dc_irq_source irq_source;
>>>> +    struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>> +    struct amdgpu_device *adev = crtc->dev->dev_private;
>>>> +    int rc;
>>>> +
>>>> +    irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
>>>> +
>>>> +    rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
>>>> +
>>>> +    DRM_DEBUG_DRIVER("%s: crtc %d - vupdate irq %sabling: r=%d\n",
>>>> +                     __func__, acrtc->crtc_id, enable ? "en" : "dis", rc);
>>
>> This extra __func__ is redundant here.
>>
> 
> Yep. I think maybe the whole debug message is redundant by now. Maybe drop it?
> 
>>>> +    return rc;
>>>> +}
>>>>
>>>>     static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
>>>>     {
>>>>       enum dc_irq_source irq_source;
>>>>       struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>>       struct amdgpu_device *adev = crtc->dev->dev_private;
>>>> +    struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
>>>> +    int rc = 0;
>>>> +
>>>> +    if (enable) {
>>>> +            /* vblank irq on -> Only need vupdate irq in vrr mode */
>>>> +            if (amdgpu_dm_vrr_active(acrtc_state))
>>>> +                    rc = dm_set_vupdate_irq(crtc, true);
>>>> +    }
>>>> +    else {
>>>
>>> should be } else {
>>>
>>>> +            /* vblank irq off -> vupdate irq off */
>>>> +            rc = dm_set_vupdate_irq(crtc, false);
>>>> +    }
>>>> +
>>>> +    if (rc)
>>>> +            return rc;
>>>>
>>>>       irq_source = IRQ_TYPE_VBLANK + acrtc->otg_inst;
>>>>       return dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
>>>> @@ -4741,7 +4859,11 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>>>>                * While VRR is active, we must not disable vblank irq, as a
>>>>                * reenable after disable would compute bogus vblank/pflip
>>>>                * timestamps if it likely happened inside display front-porch.
>>>> +             *
>>>> +             * We also need vupdate irq for the actual core vblank handling
>>>> +             * at end of vblank.
>>>>                */
>>>> +            dm_set_vupdate_irq(new_state->base.crtc, true);
>>>>               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);
>>>> @@ -4750,6 +4872,7 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
>>>>               /* Transition VRR active -> inactive:
>>>>                * Allow vblank irq disable again for fixed refresh rate.
>>>>                */
>>>> +            dm_set_vupdate_irq(new_state->base.crtc, false);
>>>>               drm_crtc_vblank_put(new_state->base.crtc);
>>>>               DRM_DEBUG_DRIVER("%s: crtc=%u VRR on->off: Drop vblank ref\n",
>>>>                                __func__, new_state->base.crtc->base.id);
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>> index f741ea3..0b35f84 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>> @@ -182,6 +182,15 @@ struct amdgpu_display_manager {
>>>>       struct common_irq_params
>>>>       vblank_params[DC_IRQ_SOURCE_VBLANK6 - DC_IRQ_SOURCE_VBLANK1 + 1];
>>>>
>>>> +    /**
>>>> +     * @vupdate_params:
>>>> +     *
>>>> +     * Vertical update IRQ parameters, passed to registered handlers when
>>>> +     * triggered.
>>>> +     */
>>>> +    struct common_irq_params
>>>> +    vupdate_params[DC_IRQ_SOURCE_VUPDATE6 - DC_IRQ_SOURCE_VUPDATE1 + 1];
>>>> +
>>>>       spinlock_t irq_handler_list_table_lock;
>>>>
>>>>       struct backlight_device *backlight_dev;
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
>>>> index cd10f77..fee7837 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
>>>> @@ -674,11 +674,30 @@ static int amdgpu_dm_set_crtc_irq_state(struct amdgpu_device *adev,
>>>>               __func__);
>>>>     }
>>>>
>>>> +static int amdgpu_dm_set_vupdate_irq_state(struct amdgpu_device *adev,
>>>> +                                       struct amdgpu_irq_src *source,
>>>> +                                       unsigned crtc_id,
>>>
>>> should be unsigned int crtc_id
>>>
>>>> +                                       enum amdgpu_interrupt_state state)
>>>> +{
>>>> +    return dm_irq_state(
>>>> +            adev,
>>>> +            source,
>>>> +            crtc_id,
>>>> +            state,
>>>> +            IRQ_TYPE_VUPDATE,
>>>> +            __func__);
>>>> +}
>>>> +
>>>>     static const struct amdgpu_irq_src_funcs dm_crtc_irq_funcs = {
>>>>       .set = amdgpu_dm_set_crtc_irq_state,
>>>>       .process = amdgpu_dm_irq_handler,
>>>>     };
>>>>
>>>> +static const struct amdgpu_irq_src_funcs dm_vupdate_irq_funcs = {
>>>> +    .set = amdgpu_dm_set_vupdate_irq_state,
>>>> +    .process = amdgpu_dm_irq_handler,
>>>> +};
>>>> +
>>>>     static const struct amdgpu_irq_src_funcs dm_pageflip_irq_funcs = {
>>>>       .set = amdgpu_dm_set_pflip_irq_state,
>>>>       .process = amdgpu_dm_irq_handler,
>>>> @@ -695,6 +714,9 @@ void amdgpu_dm_set_irq_funcs(struct amdgpu_device *adev)
>>>>       adev->crtc_irq.num_types = adev->mode_info.num_crtc;
>>>>       adev->crtc_irq.funcs = &dm_crtc_irq_funcs;
>>>>
>>>> +    adev->vupdate_irq.num_types = adev->mode_info.num_crtc;
>>>> +    adev->vupdate_irq.funcs = &dm_vupdate_irq_funcs;
>>>> +
>>>>       adev->pageflip_irq.num_types = adev->mode_info.num_crtc;
>>>>       adev->pageflip_irq.funcs = &dm_pageflip_irq_funcs;
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
>>>> index afe0876..86987f5 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
>>>> @@ -81,6 +81,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>       .ack = NULL
>>>>     };
>>>>
>>>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
>>>> +    .set = NULL,
>>>> +    .ack = NULL
>>>> +};
>>>> +
>>>>     #define hpd_int_entry(reg_num)\
>>>>       [DC_IRQ_SOURCE_HPD1 + reg_num] = {\
>>>>               .enable_reg = mmHPD ## reg_num ## _DC_HPD_INT_CONTROL,\
>>>> @@ -137,7 +142,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>               CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>>>>               .ack_value =\
>>>>               CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>>>> -            .funcs = &vblank_irq_info_funcs\
>>>> +            .funcs = &vupdate_irq_info_funcs\
>>>>       }
>>>>
>>>>     #define vblank_int_entry(reg_num)\
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
>>>> index 1ea7256..750ba0a 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
>>>> @@ -84,6 +84,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>       .ack = NULL
>>>>     };
>>>>
>>>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
>>>> +    .set = NULL,
>>>> +    .ack = NULL
>>>> +};
>>>> +
>>>>     #define BASE_INNER(seg) \
>>>>       DCE_BASE__INST0_SEG ## seg
>>>>
>>>> @@ -140,7 +145,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>               IRQ_REG_ENTRY(CRTC, reg_num,\
>>>>                       CRTC_INTERRUPT_CONTROL, CRTC_V_UPDATE_INT_MSK,\
>>>>                       CRTC_V_UPDATE_INT_STATUS, CRTC_V_UPDATE_INT_CLEAR),\
>>>> -            .funcs = &vblank_irq_info_funcs\
>>>> +            .funcs = &vupdate_irq_info_funcs\
>>>>       }
>>>>
>>>>     #define vblank_int_entry(reg_num)\
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
>>>> index 8a2066c..de218fe 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
>>>> @@ -84,6 +84,10 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>       .ack = NULL
>>>>     };
>>>>
>>>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
>>>> +    .set = NULL,
>>>> +    .ack = NULL
>>>> +};
>>>>
>>>>     #define hpd_int_entry(reg_num)\
>>>>       [DC_IRQ_SOURCE_INVALID + reg_num] = {\
>>>> @@ -142,7 +146,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>               CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>>>>               .ack_value =\
>>>>               CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
>>>> -            .funcs = &vblank_irq_info_funcs\
>>>> +            .funcs = &vupdate_irq_info_funcs\
>>>>       }
>>>>
>>>>     #define vblank_int_entry(reg_num)\
>>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>> index e04ae49..10ac6de 100644
>>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>> @@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
>>>>               return DC_IRQ_SOURCE_VBLANK5;
>>>>       case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
>>>>               return DC_IRQ_SOURCE_VBLANK6;
>>>> +    case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>> +            return DC_IRQ_SOURCE_VUPDATE1;
>>>> +    case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>> +            return DC_IRQ_SOURCE_VUPDATE2;
>>>> +    case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>> +            return DC_IRQ_SOURCE_VUPDATE3;
>>>> +    case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>> +            return DC_IRQ_SOURCE_VUPDATE4;
>>>> +    case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>> +            return DC_IRQ_SOURCE_VUPDATE5;
>>>> +    case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>> +            return DC_IRQ_SOURCE_VUPDATE6;
>>
>> I'm not sure we necessarily want to reuse the same
>> DC_IRQ_SOURCE_VUPDATE1...6 definitions here again when it's really
>> V_UPDATE_NO_LOCK.
>>
> 
> Hm. The problem is that if we use different ones for DCE and DCN we
> need special-case handling in amdgpu_dm.c, e.g., in
> amdgpu_dm_set_vupdate_irq_state() and dm_set_vupdate_irq() to detect
> if it is caling DCE or DCN (how to detect this?) to select different
> irq sources IRQ_TYPE_VUPDATE vs IRQ_TYPE_VUPDATE_NO_LOCK and such?
> And definitions like "struct amdgpu_irq_src vupdate_irq;" should then
> also exist twice as vupdate_irq and vupdate_irq_no_lock for correct
> naming?
> 
> Or we'd name the IRQ source and all relevant data structures and
> functions something like DC_IRQ_SOURCE_VBLANK_END1..6 to describe what
> it signals instead of to what it corresponds in hardware? That would
> be what was done with the regular DC_IRQ_SOURCE_VBLANK1..6. It signals
> start of vblank, but the underlying hw interrupts are actually a
> properly programmed VLINE0 irq on DCE and a VSTARTUP irq on DCN.


Ah, I see what you mean. Maybe this is for the better to share the same 
names then. I'll defer this to Harry.

> 
> Of course this all assumes we need to use the NO_LOCK variant on DCN?
> We haven't tested what the regular VUPDATE_IRQ does, because i don't
> have a suitable test machine, and my use of the NO_LOCK variant was
> just based on my interpretation of this little comment in the DCN
> header file:
> 
> #define DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT 0x57 //
> "OTG0 VUPDATE event without lock interrupt, VUPDATE is update event
> for double buffered registers"
> 
> and the absence of any explanatory comment in the define of regular V_UPDATE:
> 
> #define DCN_1_0__SRCID__DC_D1_OTG_V_UPDATE 0x18 // D1 : OTG V_update
> OTG1_IHC_V_UPDATE_INTERRUPT
> 
> I assumed the NO_LOCK means not affected by the state of any of the hw
> locks, so regular V_UPDATE might be affected by them. You agreed with
> that, but we never tested what regular V_UPDATE would do on DCN. Do we
> actually know for sure from hw specs that it would not work, ie. not
> unconditionally fire the IRQ at every end of VBLANK, as we need?

FWIW, I did try your original patches with V_UPDATE. I don't know off 
the top of my head what specifically V_UPDATE does that V_UPDATE_NO_LOCK 
doesn't, but at the very least the HW won't be flipping anything to the 
screen if you're using the former. You'll end up with a static screen 
that looks like a system hang.

Nicholas Kazlauskas

> 
> 
>>>>       case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
>>>>               return DC_IRQ_SOURCE_PFLIP1;
>>>>       case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
>>>> @@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>       .ack = NULL
>>>>     };
>>>>
>>>> +static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
>>>> +    .set = NULL,
>>>> +    .ack = NULL
>>>> +};
>>>> +
>>>>     #define BASE_INNER(seg) \
>>>>       DCE_BASE__INST0_SEG ## seg
>>>>
>>>> @@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>               .funcs = &pflip_irq_info_funcs\
>>>>       }
>>>>
>>>> -#define vupdate_int_entry(reg_num)\
>>>> +/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
>>>> + * of DCE's DC_IRQ_SOURCE_VUPDATEx.
>>>> + */
>>>> +#define vupdate_no_lock_int_entry(reg_num)\
>>>>       [DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
>>>>               IRQ_REG_ENTRY(OTG, reg_num,\
>>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
>>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
>>>> -            .funcs = &vblank_irq_info_funcs\
>>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
>>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
>>>> +            .funcs = &vupdate_no_lock_irq_info_funcs\
>>>>       }
>>>>
>>>>     #define vblank_int_entry(reg_num)\
>>>> @@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
>>>>       dc_underflow_int_entry(6),
>>>>       [DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
>>>>       [DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
>>>> -    vupdate_int_entry(0),
>>>> -    vupdate_int_entry(1),
>>>> -    vupdate_int_entry(2),
>>>> -    vupdate_int_entry(3),
>>>> -    vupdate_int_entry(4),
>>>> -    vupdate_int_entry(5),
>>
>> I don't think we should be necessarily dropping these, but I guess it
>> could go either way since these IRQs technically aren't defined.
>>
> 
> We could keep both definitions around, original vupdate_int_entry +
> the new vupdate_no_lock_int_entry.
> 
> -mario
> 
>>>> +    vupdate_no_lock_int_entry(0),
>>>> +    vupdate_no_lock_int_entry(1),
>>>> +    vupdate_no_lock_int_entry(2),
>>>> +    vupdate_no_lock_int_entry(3),
>>>> +    vupdate_no_lock_int_entry(4),
>>>> +    vupdate_no_lock_int_entry(5),
>>>>       vblank_int_entry(0),
>>>>       vblank_int_entry(1),
>>>>       vblank_int_entry(2),
>>>>
>>>
>>> Nicholas Kazlauskas
>>> _______________________________________________
>>> amd-gfx mailing list
>>> [e-mail]
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>

Mario Kleiner March 21, 2019, 9:39 a.m. UTC | #6

On Wed, Mar 20, 2019 at 1:53 PM Kazlauskas, Nicholas
<[e-mail]> wrote:
>
> On 3/20/19 3:51 AM, Mario Kleiner wrote:
> > Ok, fixed all the style issues and ran checkpatch over the patches. Thanks.
> >
> > On Tue, Mar 19, 2019 at 2:32 PM Kazlauskas, Nicholas
> > <[e-mail]> wrote:
> >>
> >> On 3/19/19 9:23 AM, Kazlauskas, Nicholas wrote:
> >>> On 3/18/19 1:19 PM, Mario Kleiner wrote:
> >>>> In VRR mode, proper vblank/pageflip timestamps can only be computed
> >>>> after the display scanout position has left front-porch. Therefore
> >>>> delay calls to drm_crtc_handle_vblank(), and thereby calls to
> >>>> drm_update_vblank_count() and pageflip event delivery, to after the
> >>>> end of front-porch when in VRR mode.
> >>>>
> >>>> We add a new vupdate irq, which triggers at the end of the vupdate
> >>>> interval, ie. at the end of vblank, and calls the core vblank handler
> >>>> function. The new irq handler is not executed in standard non-VRR
> >>>> mode, so vblank handling for fixed refresh rate mode is identical
> >>>> to the past implementation.
> >>>>
> >>>> Signed-off-by: Mario Kleiner <[e-mail]>
> >>
> >> Looks I lost some of my comments I wanted to send in my last email. Just
> >> a few nitpicks (including some things Paul mentioned).
> >>
> >> Also meant to CC Harry on this as well.
> >>
> >>>> ---
> >>>>     drivers/gpu/drm/amd/amdgpu/amdgpu.h                |   1 +
> >>>>     drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 129 ++++++++++++++++++++-
> >>>>     drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   9 ++
> >>>>     .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c  |  22 ++++
> >>>>     .../amd/display/dc/irq/dce110/irq_service_dce110.c |   7 +-
> >>>>     .../amd/display/dc/irq/dce120/irq_service_dce120.c |   7 +-
> >>>>     .../amd/display/dc/irq/dce80/irq_service_dce80.c   |   6 +-
> >>>>     .../amd/display/dc/irq/dcn10/irq_service_dcn10.c   |  40 +++++--
> >>>>     8 files changed, 205 insertions(+), 16 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>> index f88761a..64167dd 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> >>>> @@ -827,6 +827,7 @@ struct amdgpu_device {
> >>>>       /* For pre-DCE11. DCE11 and later are in "struct amdgpu_device->dm" */
> >>>>       struct work_struct              hotplug_work;
> >>>>       struct amdgpu_irq_src           crtc_irq;
> >>>> +    struct amdgpu_irq_src           vupdate_irq;
> >>>>       struct amdgpu_irq_src           pageflip_irq;
> >>>>       struct amdgpu_irq_src           hpd_irq;
> >>>>
> >>>> 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 85e4f87..555d9e9f 100644
> >>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>> @@ -315,6 +315,32 @@ static void dm_pflip_high_irq(void *interrupt_params)
> >>>>       drm_crtc_vblank_put(&amdgpu_crtc->base);
> >>>>     }
> >>>>
> >>>> +static void dm_vupdate_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;
> >>>> +
> >>>> +    acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);
> >>>> +
> >>>> +    if (acrtc) {
> >>>> +            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));
> >>>> +
> >>>> +            /* Core vblank handling is done here after end of front-porch in
> >>>> +             * vrr mode, as vblank timestamping will give valid results
> >>>> +             * while now done after front-porch. This will also deliver
> >>>> +             * page-flip completion events that have been queued to us
> >>>> +             * if a pageflip happened inside front-porch.
> >>>> +             */
> >>>> +            if (amdgpu_dm_vrr_active(acrtc_state))
> >>>> +                    drm_crtc_handle_vblank(&acrtc->base)
> >>> I was thinking that 3 and 4 might have to be merged, but VRR pflip
> >>> timestamping seems to be the same as it was before (off by a line or
> >>> two) since it's not handled here yet. This seems to fix vblank events
> >>> and timestamping at least.
> >>>
> >>>> +    }
> >>>> +}
> >>>> +
> >>>>     static void dm_crtc_high_irq(void *interrupt_params)
> >>>>     {
> >>>>       struct common_irq_params *irq_params = interrupt_params;
> >>>> @@ -325,11 +351,24 @@ static void dm_crtc_high_irq(void *interrupt_params)
> >>>>       acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
> >>>>
> >>>>       if (acrtc) {
> >>>> -            drm_crtc_handle_vblank(&acrtc->base);
> >>>> -            amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
> >>>> -
> >>>>               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));
> >>>> +
> >>>> +            /* Core vblank handling at start of front-porch is only possible
> >>>> +             * in non-vrr mode, as only there vblank timestamping will give
> >>>> +             * valid results while done in front-porch. Otherwise defer it
> >>>> +             * to dm_vupdate_high_irq after end of front-porch.
> >>>> +             */
> >>>> +            if (!amdgpu_dm_vrr_active(acrtc_state))
> >>>> +                    drm_crtc_handle_vblank(&acrtc->base);
> >>>> +
> >>>> +            /* Following stuff must happen at start of vblank, for crc
> >>>> +             * computation and below-the-range btr support in vrr mode.
> >>>> +             */
> >>>> +            amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
> >>>> +
> >>>>               if (acrtc_state->stream &&
> >>>>                   acrtc_state->vrr_params.supported &&
> >>>>                   acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
> >>>> @@ -1447,6 +1486,27 @@ static int dce110_register_irq_handlers(struct amdgpu_device *adev)
> >>>>                               dm_crtc_high_irq, c_irq_params);
> >>>>       }
> >>>>
> >>>> +    /* Use VUPDATE interrupt */
> >>>> +    for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i+=2) {
> >>>> +            r = amdgpu_irq_add_id(adev, client_id, 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 = VISLANDS30_IV_SRCID_D1_GRPH_PFLIP;
> >>>>                       i <= VISLANDS30_IV_SRCID_D6_GRPH_PFLIP; i += 2) {
> >>>> @@ -1532,6 +1592,34 @@ static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
> >>>>                               dm_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;
> >>>> @@ -3226,12 +3314,42 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
> >>>>       return &state->base;
> >>>>     }
> >>>>
> >>>> +static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
> >>>> +{
> >>>> +    enum dc_irq_source irq_source;
> >>>> +    struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> >>>> +    struct amdgpu_device *adev = crtc->dev->dev_private;
> >>>> +    int rc;
> >>>> +
> >>>> +    irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
> >>>> +
> >>>> +    rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
> >>>> +
> >>>> +    DRM_DEBUG_DRIVER("%s: crtc %d - vupdate irq %sabling: r=%d\n",
> >>>> +                     __func__, acrtc->crtc_id, enable ? "en" : "dis", rc);
> >>
> >> This extra __func__ is redundant here.
> >>
> >
> > Yep. I think maybe the whole debug message is redundant by now. Maybe drop it?
> >
> >>>> +    return rc;
> >>>> +}
> >>>>
> >>>>     static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
> >>>>     {
> >>>>       enum dc_irq_source irq_source;
> >>>>       struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> >>>>       struct amdgpu_device *adev = crtc->dev->dev_private;
> >>>> +    struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
> >>>> +    int rc = 0;
> >>>> +
> >>>> +    if (enable) {
> >>>> +            /* vblank irq on -> Only need vupdate irq in vrr mode */
> >>>> +            if (amdgpu_dm_vrr_active(acrtc_state))
> >>>> +                    rc = dm_set_vupdate_irq(crtc, true);
> >>>> +    }
> >>>> +    else {
> >>>
> >>> should be } else {
> >>>
> >>>> +            /* vblank irq off -> vupdate irq off */
> >>>> +            rc = dm_set_vupdate_irq(crtc, false);
> >>>> +    }
> >>>> +
> >>>> +    if (rc)
> >>>> +            return rc;
> >>>>
> >>>>       irq_source = IRQ_TYPE_VBLANK + acrtc->otg_inst;
> >>>>       return dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
> >>>> @@ -4741,7 +4859,11 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
> >>>>                * While VRR is active, we must not disable vblank irq, as a
> >>>>                * reenable after disable would compute bogus vblank/pflip
> >>>>                * timestamps if it likely happened inside display front-porch.
> >>>> +             *
> >>>> +             * We also need vupdate irq for the actual core vblank handling
> >>>> +             * at end of vblank.
> >>>>                */
> >>>> +            dm_set_vupdate_irq(new_state->base.crtc, true);
> >>>>               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);
> >>>> @@ -4750,6 +4872,7 @@ static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
> >>>>               /* Transition VRR active -> inactive:
> >>>>                * Allow vblank irq disable again for fixed refresh rate.
> >>>>                */
> >>>> +            dm_set_vupdate_irq(new_state->base.crtc, false);
> >>>>               drm_crtc_vblank_put(new_state->base.crtc);
> >>>>               DRM_DEBUG_DRIVER("%s: crtc=%u VRR on->off: Drop vblank ref\n",
> >>>>                                __func__, new_state->base.crtc->base.id);
> >>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> >>>> index f741ea3..0b35f84 100644
> >>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> >>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> >>>> @@ -182,6 +182,15 @@ struct amdgpu_display_manager {
> >>>>       struct common_irq_params
> >>>>       vblank_params[DC_IRQ_SOURCE_VBLANK6 - DC_IRQ_SOURCE_VBLANK1 + 1];
> >>>>
> >>>> +    /**
> >>>> +     * @vupdate_params:
> >>>> +     *
> >>>> +     * Vertical update IRQ parameters, passed to registered handlers when
> >>>> +     * triggered.
> >>>> +     */
> >>>> +    struct common_irq_params
> >>>> +    vupdate_params[DC_IRQ_SOURCE_VUPDATE6 - DC_IRQ_SOURCE_VUPDATE1 + 1];
> >>>> +
> >>>>       spinlock_t irq_handler_list_table_lock;
> >>>>
> >>>>       struct backlight_device *backlight_dev;
> >>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> >>>> index cd10f77..fee7837 100644
> >>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> >>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> >>>> @@ -674,11 +674,30 @@ static int amdgpu_dm_set_crtc_irq_state(struct amdgpu_device *adev,
> >>>>               __func__);
> >>>>     }
> >>>>
> >>>> +static int amdgpu_dm_set_vupdate_irq_state(struct amdgpu_device *adev,
> >>>> +                                       struct amdgpu_irq_src *source,
> >>>> +                                       unsigned crtc_id,
> >>>
> >>> should be unsigned int crtc_id
> >>>
> >>>> +                                       enum amdgpu_interrupt_state state)
> >>>> +{
> >>>> +    return dm_irq_state(
> >>>> +            adev,
> >>>> +            source,
> >>>> +            crtc_id,
> >>>> +            state,
> >>>> +            IRQ_TYPE_VUPDATE,
> >>>> +            __func__);
> >>>> +}
> >>>> +
> >>>>     static const struct amdgpu_irq_src_funcs dm_crtc_irq_funcs = {
> >>>>       .set = amdgpu_dm_set_crtc_irq_state,
> >>>>       .process = amdgpu_dm_irq_handler,
> >>>>     };
> >>>>
> >>>> +static const struct amdgpu_irq_src_funcs dm_vupdate_irq_funcs = {
> >>>> +    .set = amdgpu_dm_set_vupdate_irq_state,
> >>>> +    .process = amdgpu_dm_irq_handler,
> >>>> +};
> >>>> +
> >>>>     static const struct amdgpu_irq_src_funcs dm_pageflip_irq_funcs = {
> >>>>       .set = amdgpu_dm_set_pflip_irq_state,
> >>>>       .process = amdgpu_dm_irq_handler,
> >>>> @@ -695,6 +714,9 @@ void amdgpu_dm_set_irq_funcs(struct amdgpu_device *adev)
> >>>>       adev->crtc_irq.num_types = adev->mode_info.num_crtc;
> >>>>       adev->crtc_irq.funcs = &dm_crtc_irq_funcs;
> >>>>
> >>>> +    adev->vupdate_irq.num_types = adev->mode_info.num_crtc;
> >>>> +    adev->vupdate_irq.funcs = &dm_vupdate_irq_funcs;
> >>>> +
> >>>>       adev->pageflip_irq.num_types = adev->mode_info.num_crtc;
> >>>>       adev->pageflip_irq.funcs = &dm_pageflip_irq_funcs;
> >>>>
> >>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> >>>> index afe0876..86987f5 100644
> >>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> >>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
> >>>> @@ -81,6 +81,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>>>       .ack = NULL
> >>>>     };
> >>>>
> >>>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> >>>> +    .set = NULL,
> >>>> +    .ack = NULL
> >>>> +};
> >>>> +
> >>>>     #define hpd_int_entry(reg_num)\
> >>>>       [DC_IRQ_SOURCE_HPD1 + reg_num] = {\
> >>>>               .enable_reg = mmHPD ## reg_num ## _DC_HPD_INT_CONTROL,\
> >>>> @@ -137,7 +142,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>>>               CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> >>>>               .ack_value =\
> >>>>               CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> >>>> -            .funcs = &vblank_irq_info_funcs\
> >>>> +            .funcs = &vupdate_irq_info_funcs\
> >>>>       }
> >>>>
> >>>>     #define vblank_int_entry(reg_num)\
> >>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> >>>> index 1ea7256..750ba0a 100644
> >>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> >>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
> >>>> @@ -84,6 +84,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>>>       .ack = NULL
> >>>>     };
> >>>>
> >>>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> >>>> +    .set = NULL,
> >>>> +    .ack = NULL
> >>>> +};
> >>>> +
> >>>>     #define BASE_INNER(seg) \
> >>>>       DCE_BASE__INST0_SEG ## seg
> >>>>
> >>>> @@ -140,7 +145,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>>>               IRQ_REG_ENTRY(CRTC, reg_num,\
> >>>>                       CRTC_INTERRUPT_CONTROL, CRTC_V_UPDATE_INT_MSK,\
> >>>>                       CRTC_V_UPDATE_INT_STATUS, CRTC_V_UPDATE_INT_CLEAR),\
> >>>> -            .funcs = &vblank_irq_info_funcs\
> >>>> +            .funcs = &vupdate_irq_info_funcs\
> >>>>       }
> >>>>
> >>>>     #define vblank_int_entry(reg_num)\
> >>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> >>>> index 8a2066c..de218fe 100644
> >>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> >>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
> >>>> @@ -84,6 +84,10 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>>>       .ack = NULL
> >>>>     };
> >>>>
> >>>> +static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
> >>>> +    .set = NULL,
> >>>> +    .ack = NULL
> >>>> +};
> >>>>
> >>>>     #define hpd_int_entry(reg_num)\
> >>>>       [DC_IRQ_SOURCE_INVALID + reg_num] = {\
> >>>> @@ -142,7 +146,7 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>>>               CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> >>>>               .ack_value =\
> >>>>               CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
> >>>> -            .funcs = &vblank_irq_info_funcs\
> >>>> +            .funcs = &vupdate_irq_info_funcs\
> >>>>       }
> >>>>
> >>>>     #define vblank_int_entry(reg_num)\
> >>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> >>>> index e04ae49..10ac6de 100644
> >>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> >>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
> >>>> @@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
> >>>>               return DC_IRQ_SOURCE_VBLANK5;
> >>>>       case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
> >>>>               return DC_IRQ_SOURCE_VBLANK6;
> >>>> +    case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >>>> +            return DC_IRQ_SOURCE_VUPDATE1;
> >>>> +    case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >>>> +            return DC_IRQ_SOURCE_VUPDATE2;
> >>>> +    case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >>>> +            return DC_IRQ_SOURCE_VUPDATE3;
> >>>> +    case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >>>> +            return DC_IRQ_SOURCE_VUPDATE4;
> >>>> +    case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >>>> +            return DC_IRQ_SOURCE_VUPDATE5;
> >>>> +    case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
> >>>> +            return DC_IRQ_SOURCE_VUPDATE6;
> >>
> >> I'm not sure we necessarily want to reuse the same
> >> DC_IRQ_SOURCE_VUPDATE1...6 definitions here again when it's really
> >> V_UPDATE_NO_LOCK.
> >>
> >
> > Hm. The problem is that if we use different ones for DCE and DCN we
> > need special-case handling in amdgpu_dm.c, e.g., in
> > amdgpu_dm_set_vupdate_irq_state() and dm_set_vupdate_irq() to detect
> > if it is caling DCE or DCN (how to detect this?) to select different
> > irq sources IRQ_TYPE_VUPDATE vs IRQ_TYPE_VUPDATE_NO_LOCK and such?
> > And definitions like "struct amdgpu_irq_src vupdate_irq;" should then
> > also exist twice as vupdate_irq and vupdate_irq_no_lock for correct
> > naming?
> >
> > Or we'd name the IRQ source and all relevant data structures and
> > functions something like DC_IRQ_SOURCE_VBLANK_END1..6 to describe what
> > it signals instead of to what it corresponds in hardware? That would
> > be what was done with the regular DC_IRQ_SOURCE_VBLANK1..6. It signals
> > start of vblank, but the underlying hw interrupts are actually a
> > properly programmed VLINE0 irq on DCE and a VSTARTUP irq on DCN.
>
>
> Ah, I see what you mean. Maybe this is for the better to share the same
> names then. I'll defer this to Harry.
>
> >
> > Of course this all assumes we need to use the NO_LOCK variant on DCN?
> > We haven't tested what the regular VUPDATE_IRQ does, because i don't
> > have a suitable test machine, and my use of the NO_LOCK variant was
> > just based on my interpretation of this little comment in the DCN
> > header file:
> >
> > #define DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT 0x57 //
> > "OTG0 VUPDATE event without lock interrupt, VUPDATE is update event
> > for double buffered registers"
> >
> > and the absence of any explanatory comment in the define of regular V_UPDATE:
> >
> > #define DCN_1_0__SRCID__DC_D1_OTG_V_UPDATE 0x18 // D1 : OTG V_update
> > OTG1_IHC_V_UPDATE_INTERRUPT
> >
> > I assumed the NO_LOCK means not affected by the state of any of the hw
> > locks, so regular V_UPDATE might be affected by them. You agreed with
> > that, but we never tested what regular V_UPDATE would do on DCN. Do we
> > actually know for sure from hw specs that it would not work, ie. not
> > unconditionally fire the IRQ at every end of VBLANK, as we need?
>
> FWIW, I did try your original patches with V_UPDATE. I don't know off
> the top of my head what specifically V_UPDATE does that V_UPDATE_NO_LOCK
> doesn't, but at the very least the HW won't be flipping anything to the
> screen if you're using the former. You'll end up with a static screen
> that looks like a system hang.
>

Yes, but that was because that iteration of the patch had a bug, where
i defined the irq sources / irq hw sources as _NO_LOCK variants as i
intended, but forgot to adapt the irq en/disable register macros. You
fixed my bug during your testing and so we know the _NO_LOCK variant
works perfectly for our purpose. But we never tesed if the standard
variant would have worked just as well.

I will test this later today, because since today i'm the proud owner
of a PC with Ryzen 5 2400G with Vega11 Raven / DCN-1.0 :). So i'll
give it a try. Would be good though if you could have a look at the
hardware docs i assume you have internally to verify what the V_UPDATE
irq hw source actually does?

-mario

> Nicholas Kazlauskas
>
> >
> >
> >>>>       case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
> >>>>               return DC_IRQ_SOURCE_PFLIP1;
> >>>>       case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
> >>>> @@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>>>       .ack = NULL
> >>>>     };
> >>>>
> >>>> +static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
> >>>> +    .set = NULL,
> >>>> +    .ack = NULL
> >>>> +};
> >>>> +
> >>>>     #define BASE_INNER(seg) \
> >>>>       DCE_BASE__INST0_SEG ## seg
> >>>>
> >>>> @@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
> >>>>               .funcs = &pflip_irq_info_funcs\
> >>>>       }
> >>>>
> >>>> -#define vupdate_int_entry(reg_num)\
> >>>> +/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
> >>>> + * of DCE's DC_IRQ_SOURCE_VUPDATEx.
> >>>> + */
> >>>> +#define vupdate_no_lock_int_entry(reg_num)\
> >>>>       [DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
> >>>>               IRQ_REG_ENTRY(OTG, reg_num,\
> >>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
> >>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
> >>>> -            .funcs = &vblank_irq_info_funcs\
> >>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
> >>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
> >>>> +            .funcs = &vupdate_no_lock_irq_info_funcs\
> >>>>       }
> >>>>
> >>>>     #define vblank_int_entry(reg_num)\
> >>>> @@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
> >>>>       dc_underflow_int_entry(6),
> >>>>       [DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
> >>>>       [DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
> >>>> -    vupdate_int_entry(0),
> >>>> -    vupdate_int_entry(1),
> >>>> -    vupdate_int_entry(2),
> >>>> -    vupdate_int_entry(3),
> >>>> -    vupdate_int_entry(4),
> >>>> -    vupdate_int_entry(5),
> >>
> >> I don't think we should be necessarily dropping these, but I guess it
> >> could go either way since these IRQs technically aren't defined.
> >>
> >
> > We could keep both definitions around, original vupdate_int_entry +
> > the new vupdate_no_lock_int_entry.
> >
> > -mario
> >
> >>>> +    vupdate_no_lock_int_entry(0),
> >>>> +    vupdate_no_lock_int_entry(1),
> >>>> +    vupdate_no_lock_int_entry(2),
> >>>> +    vupdate_no_lock_int_entry(3),
> >>>> +    vupdate_no_lock_int_entry(4),
> >>>> +    vupdate_no_lock_int_entry(5),
> >>>>       vblank_int_entry(0),
> >>>>       vblank_int_entry(1),
> >>>>       vblank_int_entry(2),
> >>>>
> >>>
> >>> Nicholas Kazlauskas
> >>> _______________________________________________
> >>> amd-gfx mailing list
> >>> [e-mail]
> >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> >>>
> >>
>

Harry Wentland March 21, 2019, 3:38 p.m. UTC | #7

On 2019-03-21 5:39 a.m., Mario Kleiner wrote:
> On Wed, Mar 20, 2019 at 1:53 PM Kazlauskas, Nicholas
> <[e-mail]> wrote:
>>
>> On 3/20/19 3:51 AM, Mario Kleiner wrote:
>>> Ok, fixed all the style issues and ran checkpatch over the patches. Thanks.
>>>
>>> On Tue, Mar 19, 2019 at 2:32 PM Kazlauskas, Nicholas
>>> <[e-mail]> wrote:
>>>>
>>>> On 3/19/19 9:23 AM, Kazlauskas, Nicholas wrote:
>>>>> On 3/18/19 1:19 PM, Mario Kleiner wrote:

...snip...

>>>>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>>>> index e04ae49..10ac6de 100644
>>>>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>>>> @@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
>>>>>>               return DC_IRQ_SOURCE_VBLANK5;
>>>>>>       case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
>>>>>>               return DC_IRQ_SOURCE_VBLANK6;
>>>>>> +    case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE1;
>>>>>> +    case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE2;
>>>>>> +    case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE3;
>>>>>> +    case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE4;
>>>>>> +    case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE5;
>>>>>> +    case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>> +            return DC_IRQ_SOURCE_VUPDATE6;
>>>>
>>>> I'm not sure we necessarily want to reuse the same
>>>> DC_IRQ_SOURCE_VUPDATE1...6 definitions here again when it's really
>>>> V_UPDATE_NO_LOCK.
>>>>
>>>
>>> Hm. The problem is that if we use different ones for DCE and DCN we
>>> need special-case handling in amdgpu_dm.c, e.g., in
>>> amdgpu_dm_set_vupdate_irq_state() and dm_set_vupdate_irq() to detect
>>> if it is caling DCE or DCN (how to detect this?) to select different
>>> irq sources IRQ_TYPE_VUPDATE vs IRQ_TYPE_VUPDATE_NO_LOCK and such?
>>> And definitions like "struct amdgpu_irq_src vupdate_irq;" should then
>>> also exist twice as vupdate_irq and vupdate_irq_no_lock for correct
>>> naming?
>>>
>>> Or we'd name the IRQ source and all relevant data structures and
>>> functions something like DC_IRQ_SOURCE_VBLANK_END1..6 to describe what
>>> it signals instead of to what it corresponds in hardware? That would
>>> be what was done with the regular DC_IRQ_SOURCE_VBLANK1..6. It signals
>>> start of vblank, but the underlying hw interrupts are actually a
>>> properly programmed VLINE0 irq on DCE and a VSTARTUP irq on DCN.
>>
>>
>> Ah, I see what you mean. Maybe this is for the better to share the same
>> names then. I'll defer this to Harry.
>>

I'd tend to agree with Mario. I think it's best to keep the same semantic for VUPDATE.

The regular VUPDATE has some interaction with the master lock. I imagine we want something that fires independently of it, which looks to be the _NO_LOCK version.

That said, both VSTARTUP and VUPDATE can occur before VSYNC on DCN. With most timings this probably won't occur but if our back porch is really small we'd see that happening. If we need to guarantee that this interrupt occurs no sooner than vline 0 we will need to register a vline IRQ.

As for VSTARTUP and VUPDATE, VSTARTUP is the IRQ that has an impact on the behavior of render clients as it's the deadline for frame submissions. After VSTARTUP any new framebuffer address update is postponed to the next frame (unless we do immediate flip).

So for now VUPDATE_NO_LOCK is probably fine and will improve our situation except for VRR displays with a really short back porch. Not sure those even exist.

>>>
>>> Of course this all assumes we need to use the NO_LOCK variant on DCN?
>>> We haven't tested what the regular VUPDATE_IRQ does, because i don't
>>> have a suitable test machine, and my use of the NO_LOCK variant was
>>> just based on my interpretation of this little comment in the DCN
>>> header file:
>>>
>>> #define DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT 0x57 //
>>> "OTG0 VUPDATE event without lock interrupt, VUPDATE is update event
>>> for double buffered registers"
>>>
>>> and the absence of any explanatory comment in the define of regular V_UPDATE:
>>>
>>> #define DCN_1_0__SRCID__DC_D1_OTG_V_UPDATE 0x18 // D1 : OTG V_update
>>> OTG1_IHC_V_UPDATE_INTERRUPT
>>>
>>> I assumed the NO_LOCK means not affected by the state of any of the hw
>>> locks, so regular V_UPDATE might be affected by them. You agreed with
>>> that, but we never tested what regular V_UPDATE would do on DCN. Do we
>>> actually know for sure from hw specs that it would not work, ie. not
>>> unconditionally fire the IRQ at every end of VBLANK, as we need?
>>
>> FWIW, I did try your original patches with V_UPDATE. I don't know off
>> the top of my head what specifically V_UPDATE does that V_UPDATE_NO_LOCK
>> doesn't, but at the very least the HW won't be flipping anything to the
>> screen if you're using the former. You'll end up with a static screen
>> that looks like a system hang.
>>
> 
> Yes, but that was because that iteration of the patch had a bug, where
> i defined the irq sources / irq hw sources as _NO_LOCK variants as i
> intended, but forgot to adapt the irq en/disable register macros. You
> fixed my bug during your testing and so we know the _NO_LOCK variant
> works perfectly for our purpose. But we never tesed if the standard
> variant would have worked just as well.
> 
> I will test this later today, because since today i'm the proud owner
> of a PC with Ryzen 5 2400G with Vega11 Raven / DCN-1.0 :). So i'll
> give it a try. Would be good though if you could have a look at the
> hardware docs i assume you have internally to verify what the V_UPDATE
> irq hw source actually does?
> 

I couldn't find much regarding the _NO_LOCK part in the HW docs, unfortunately. The other stuff is in my blurb up top.

I think this change is fine. With the comments from Nick and Paul addressed this is
Acked-by: Harry Wentland <[e-mail]>

I recommend trying the VLINE IRQ, though. Nick can you create a task for us to look into that?

Harry

> -mario
> 
>> Nicholas Kazlauskas
>>
>>>
>>>
>>>>>>       case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
>>>>>>               return DC_IRQ_SOURCE_PFLIP1;
>>>>>>       case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
>>>>>> @@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>>>       .ack = NULL
>>>>>>     };
>>>>>>
>>>>>> +static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
>>>>>> +    .set = NULL,
>>>>>> +    .ack = NULL
>>>>>> +};
>>>>>> +
>>>>>>     #define BASE_INNER(seg) \
>>>>>>       DCE_BASE__INST0_SEG ## seg
>>>>>>
>>>>>> @@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>>>               .funcs = &pflip_irq_info_funcs\
>>>>>>       }
>>>>>>
>>>>>> -#define vupdate_int_entry(reg_num)\
>>>>>> +/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
>>>>>> + * of DCE's DC_IRQ_SOURCE_VUPDATEx.
>>>>>> + */
>>>>>> +#define vupdate_no_lock_int_entry(reg_num)\
>>>>>>       [DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
>>>>>>               IRQ_REG_ENTRY(OTG, reg_num,\
>>>>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
>>>>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
>>>>>> -            .funcs = &vblank_irq_info_funcs\
>>>>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
>>>>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
>>>>>> +            .funcs = &vupdate_no_lock_irq_info_funcs\
>>>>>>       }
>>>>>>
>>>>>>     #define vblank_int_entry(reg_num)\
>>>>>> @@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
>>>>>>       dc_underflow_int_entry(6),
>>>>>>       [DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
>>>>>>       [DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
>>>>>> -    vupdate_int_entry(0),
>>>>>> -    vupdate_int_entry(1),
>>>>>> -    vupdate_int_entry(2),
>>>>>> -    vupdate_int_entry(3),
>>>>>> -    vupdate_int_entry(4),
>>>>>> -    vupdate_int_entry(5),
>>>>
>>>> I don't think we should be necessarily dropping these, but I guess it
>>>> could go either way since these IRQs technically aren't defined.
>>>>
>>>
>>> We could keep both definitions around, original vupdate_int_entry +
>>> the new vupdate_no_lock_int_entry.
>>>
>>> -mario
>>>
>>>>>> +    vupdate_no_lock_int_entry(0),
>>>>>> +    vupdate_no_lock_int_entry(1),
>>>>>> +    vupdate_no_lock_int_entry(2),
>>>>>> +    vupdate_no_lock_int_entry(3),
>>>>>> +    vupdate_no_lock_int_entry(4),
>>>>>> +    vupdate_no_lock_int_entry(5),
>>>>>>       vblank_int_entry(0),
>>>>>>       vblank_int_entry(1),
>>>>>>       vblank_int_entry(2),
>>>>>>
>>>>>
>>>>> Nicholas Kazlauskas
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> [e-mail]
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>
>>>>
>>

Kazlauskas, Nicholas March 21, 2019, 3:48 p.m. UTC | #8

On 3/21/19 11:38 AM, Wentland, Harry wrote:
> 
> 
> On 2019-03-21 5:39 a.m., Mario Kleiner wrote:
>> On Wed, Mar 20, 2019 at 1:53 PM Kazlauskas, Nicholas
>> <[e-mail]> wrote:
>>>
>>> On 3/20/19 3:51 AM, Mario Kleiner wrote:
>>>> Ok, fixed all the style issues and ran checkpatch over the patches. Thanks.
>>>>
>>>> On Tue, Mar 19, 2019 at 2:32 PM Kazlauskas, Nicholas
>>>> <[e-mail]> wrote:
>>>>>
>>>>> On 3/19/19 9:23 AM, Kazlauskas, Nicholas wrote:
>>>>>> On 3/18/19 1:19 PM, Mario Kleiner wrote:
> 
> ...snip...
> 
>>>>>>> diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>>>>> index e04ae49..10ac6de 100644
>>>>>>> --- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>>>>> +++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
>>>>>>> @@ -56,6 +56,18 @@ enum dc_irq_source to_dal_irq_source_dcn10(
>>>>>>>                return DC_IRQ_SOURCE_VBLANK5;
>>>>>>>        case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
>>>>>>>                return DC_IRQ_SOURCE_VBLANK6;
>>>>>>> +    case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>>> +            return DC_IRQ_SOURCE_VUPDATE1;
>>>>>>> +    case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>>> +            return DC_IRQ_SOURCE_VUPDATE2;
>>>>>>> +    case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>>> +            return DC_IRQ_SOURCE_VUPDATE3;
>>>>>>> +    case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>>> +            return DC_IRQ_SOURCE_VUPDATE4;
>>>>>>> +    case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>>> +            return DC_IRQ_SOURCE_VUPDATE5;
>>>>>>> +    case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
>>>>>>> +            return DC_IRQ_SOURCE_VUPDATE6;
>>>>>
>>>>> I'm not sure we necessarily want to reuse the same
>>>>> DC_IRQ_SOURCE_VUPDATE1...6 definitions here again when it's really
>>>>> V_UPDATE_NO_LOCK.
>>>>>
>>>>
>>>> Hm. The problem is that if we use different ones for DCE and DCN we
>>>> need special-case handling in amdgpu_dm.c, e.g., in
>>>> amdgpu_dm_set_vupdate_irq_state() and dm_set_vupdate_irq() to detect
>>>> if it is caling DCE or DCN (how to detect this?) to select different
>>>> irq sources IRQ_TYPE_VUPDATE vs IRQ_TYPE_VUPDATE_NO_LOCK and such?
>>>> And definitions like "struct amdgpu_irq_src vupdate_irq;" should then
>>>> also exist twice as vupdate_irq and vupdate_irq_no_lock for correct
>>>> naming?
>>>>
>>>> Or we'd name the IRQ source and all relevant data structures and
>>>> functions something like DC_IRQ_SOURCE_VBLANK_END1..6 to describe what
>>>> it signals instead of to what it corresponds in hardware? That would
>>>> be what was done with the regular DC_IRQ_SOURCE_VBLANK1..6. It signals
>>>> start of vblank, but the underlying hw interrupts are actually a
>>>> properly programmed VLINE0 irq on DCE and a VSTARTUP irq on DCN.
>>>
>>>
>>> Ah, I see what you mean. Maybe this is for the better to share the same
>>> names then. I'll defer this to Harry.
>>>
> 
> I'd tend to agree with Mario. I think it's best to keep the same semantic for VUPDATE.
> 
> The regular VUPDATE has some interaction with the master lock. I imagine we want something that fires independently of it, which looks to be the _NO_LOCK version.
> 
> That said, both VSTARTUP and VUPDATE can occur before VSYNC on DCN. With most timings this probably won't occur but if our back porch is really small we'd see that happening. If we need to guarantee that this interrupt occurs no sooner than vline 0 we will need to register a vline IRQ.
> 
> As for VSTARTUP and VUPDATE, VSTARTUP is the IRQ that has an impact on the behavior of render clients as it's the deadline for frame submissions. After VSTARTUP any new framebuffer address update is postponed to the next frame (unless we do immediate flip).
> 
> So for now VUPDATE_NO_LOCK is probably fine and will improve our situation except for VRR displays with a really short back porch. Not sure those even exist.
Mapping the DC_IRQ_SOURCE_VUPDATE1...6 to the _NO_LOCK interrupt seems 
fine to me then.

FWIW on the few displays I tested on Raven the event was usually sent 
out rather late - near or after the end of the back porch. I don't think 
we have to necessarily worry that it'll happen before vline 0 at least.

It's more about it happening too late I suppose. A vline 0 IRQ would 
probably be ideal here I think, but this works for now.

> 
>>>>
>>>> Of course this all assumes we need to use the NO_LOCK variant on DCN?
>>>> We haven't tested what the regular VUPDATE_IRQ does, because i don't
>>>> have a suitable test machine, and my use of the NO_LOCK variant was
>>>> just based on my interpretation of this little comment in the DCN
>>>> header file:
>>>>
>>>> #define DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT 0x57 //
>>>> "OTG0 VUPDATE event without lock interrupt, VUPDATE is update event
>>>> for double buffered registers"
>>>>
>>>> and the absence of any explanatory comment in the define of regular V_UPDATE:
>>>>
>>>> #define DCN_1_0__SRCID__DC_D1_OTG_V_UPDATE 0x18 // D1 : OTG V_update
>>>> OTG1_IHC_V_UPDATE_INTERRUPT
>>>>
>>>> I assumed the NO_LOCK means not affected by the state of any of the hw
>>>> locks, so regular V_UPDATE might be affected by them. You agreed with
>>>> that, but we never tested what regular V_UPDATE would do on DCN. Do we
>>>> actually know for sure from hw specs that it would not work, ie. not
>>>> unconditionally fire the IRQ at every end of VBLANK, as we need?
>>>
>>> FWIW, I did try your original patches with V_UPDATE. I don't know off
>>> the top of my head what specifically V_UPDATE does that V_UPDATE_NO_LOCK
>>> doesn't, but at the very least the HW won't be flipping anything to the
>>> screen if you're using the former. You'll end up with a static screen
>>> that looks like a system hang.
>>>
>>
>> Yes, but that was because that iteration of the patch had a bug, where
>> i defined the irq sources / irq hw sources as _NO_LOCK variants as i
>> intended, but forgot to adapt the irq en/disable register macros. You
>> fixed my bug during your testing and so we know the _NO_LOCK variant
>> works perfectly for our purpose. But we never tesed if the standard
>> variant would have worked just as well.
>>
>> I will test this later today, because since today i'm the proud owner
>> of a PC with Ryzen 5 2400G with Vega11 Raven / DCN-1.0 :). So i'll
>> give it a try. Would be good though if you could have a look at the
>> hardware docs i assume you have internally to verify what the V_UPDATE
>> irq hw source actually does?
>>
> 
> I couldn't find much regarding the _NO_LOCK part in the HW docs, unfortunately. The other stuff is in my blurb up top.
> 
> I think this change is fine. With the comments from Nick and Paul addressed this is
> Acked-by: Harry Wentland <[e-mail]>
> 
> I recommend trying the VLINE IRQ, though. Nick can you create a task for us to look into that?
> 
> Harry
Sure.

Nicholas Kazlauskas

> 
>> -mario
>>
>>> Nicholas Kazlauskas
>>>
>>>>
>>>>
>>>>>>>        case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
>>>>>>>                return DC_IRQ_SOURCE_PFLIP1;
>>>>>>>        case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
>>>>>>> @@ -153,6 +165,11 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>>>>        .ack = NULL
>>>>>>>      };
>>>>>>>
>>>>>>> +static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
>>>>>>> +    .set = NULL,
>>>>>>> +    .ack = NULL
>>>>>>> +};
>>>>>>> +
>>>>>>>      #define BASE_INNER(seg) \
>>>>>>>        DCE_BASE__INST0_SEG ## seg
>>>>>>>
>>>>>>> @@ -203,12 +220,15 @@ static const struct irq_source_info_funcs vblank_irq_info_funcs = {
>>>>>>>                .funcs = &pflip_irq_info_funcs\
>>>>>>>        }
>>>>>>>
>>>>>>> -#define vupdate_int_entry(reg_num)\
>>>>>>> +/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
>>>>>>> + * of DCE's DC_IRQ_SOURCE_VUPDATEx.
>>>>>>> + */
>>>>>>> +#define vupdate_no_lock_int_entry(reg_num)\
>>>>>>>        [DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
>>>>>>>                IRQ_REG_ENTRY(OTG, reg_num,\
>>>>>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
>>>>>>> -                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
>>>>>>> -            .funcs = &vblank_irq_info_funcs\
>>>>>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
>>>>>>> +                    OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
>>>>>>> +            .funcs = &vupdate_no_lock_irq_info_funcs\
>>>>>>>        }
>>>>>>>
>>>>>>>      #define vblank_int_entry(reg_num)\
>>>>>>> @@ -315,12 +335,12 @@ irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
>>>>>>>        dc_underflow_int_entry(6),
>>>>>>>        [DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
>>>>>>>        [DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
>>>>>>> -    vupdate_int_entry(0),
>>>>>>> -    vupdate_int_entry(1),
>>>>>>> -    vupdate_int_entry(2),
>>>>>>> -    vupdate_int_entry(3),
>>>>>>> -    vupdate_int_entry(4),
>>>>>>> -    vupdate_int_entry(5),
>>>>>
>>>>> I don't think we should be necessarily dropping these, but I guess it
>>>>> could go either way since these IRQs technically aren't defined.
>>>>>
>>>>
>>>> We could keep both definitions around, original vupdate_int_entry +
>>>> the new vupdate_no_lock_int_entry.
>>>>
>>>> -mario
>>>>
>>>>>>> +    vupdate_no_lock_int_entry(0),
>>>>>>> +    vupdate_no_lock_int_entry(1),
>>>>>>> +    vupdate_no_lock_int_entry(2),
>>>>>>> +    vupdate_no_lock_int_entry(3),
>>>>>>> +    vupdate_no_lock_int_entry(4),
>>>>>>> +    vupdate_no_lock_int_entry(5),
>>>>>>>        vblank_int_entry(0),
>>>>>>>        vblank_int_entry(1),
>>>>>>>        vblank_int_entry(2),
>>>>>>>
>>>>>>
>>>>>> Nicholas Kazlauskas
>>>>>> _______________________________________________
>>>>>> amd-gfx mailing list
>>>>>> [e-mail]
>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>>>
>>>>>
>>>

Patch

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index f88761a..64167dd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -827,6 +827,7 @@  struct amdgpu_device {
 	/* For pre-DCE11. DCE11 and later are in "struct amdgpu_device->dm" */
 	struct work_struct		hotplug_work;
 	struct amdgpu_irq_src		crtc_irq;
+	struct amdgpu_irq_src		vupdate_irq;
 	struct amdgpu_irq_src		pageflip_irq;
 	struct amdgpu_irq_src		hpd_irq;
 
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 85e4f87..555d9e9f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -315,6 +315,32 @@  static void dm_pflip_high_irq(void *interrupt_params)
 	drm_crtc_vblank_put(&amdgpu_crtc->base);
 }
 
+static void dm_vupdate_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;
+
+	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);
+
+	if (acrtc) {
+		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));
+
+		/* Core vblank handling is done here after end of front-porch in
+		 * vrr mode, as vblank timestamping will give valid results
+		 * while now done after front-porch. This will also deliver
+		 * page-flip completion events that have been queued to us
+		 * if a pageflip happened inside front-porch.
+		 */
+		if (amdgpu_dm_vrr_active(acrtc_state))
+			drm_crtc_handle_vblank(&acrtc->base);
+	}
+}
+
 static void dm_crtc_high_irq(void *interrupt_params)
 {
 	struct common_irq_params *irq_params = interrupt_params;
@@ -325,11 +351,24 @@  static void dm_crtc_high_irq(void *interrupt_params)
 	acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
 
 	if (acrtc) {
-		drm_crtc_handle_vblank(&acrtc->base);
-		amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
-
 		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));
+
+		/* Core vblank handling at start of front-porch is only possible
+		 * in non-vrr mode, as only there vblank timestamping will give
+		 * valid results while done in front-porch. Otherwise defer it
+		 * to dm_vupdate_high_irq after end of front-porch.
+		 */
+		if (!amdgpu_dm_vrr_active(acrtc_state))
+			drm_crtc_handle_vblank(&acrtc->base);
+
+		/* Following stuff must happen at start of vblank, for crc
+		 * computation and below-the-range btr support in vrr mode.
+		 */
+		amdgpu_dm_crtc_handle_crc_irq(&acrtc->base);
+
 		if (acrtc_state->stream &&
 		    acrtc_state->vrr_params.supported &&
 		    acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
@@ -1447,6 +1486,27 @@  static int dce110_register_irq_handlers(struct amdgpu_device *adev)
 				dm_crtc_high_irq, c_irq_params);
 	}
 
+	/* Use VUPDATE interrupt */
+	for (i = VISLANDS30_IV_SRCID_D1_V_UPDATE_INT; i <= VISLANDS30_IV_SRCID_D6_V_UPDATE_INT; i+=2) {
+		r = amdgpu_irq_add_id(adev, client_id, 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 = VISLANDS30_IV_SRCID_D1_GRPH_PFLIP;
 			i <= VISLANDS30_IV_SRCID_D6_GRPH_PFLIP; i += 2) {
@@ -1532,6 +1592,34 @@  static int dcn10_register_irq_handlers(struct amdgpu_device *adev)
 				dm_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;
@@ -3226,12 +3314,42 @@  dm_crtc_duplicate_state(struct drm_crtc *crtc)
 	return &state->base;
 }
 
+static inline int dm_set_vupdate_irq(struct drm_crtc *crtc, bool enable)
+{
+	enum dc_irq_source irq_source;
+	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
+	struct amdgpu_device *adev = crtc->dev->dev_private;
+	int rc;
+
+	irq_source = IRQ_TYPE_VUPDATE + acrtc->otg_inst;
+
+	rc = dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
+
+	DRM_DEBUG_DRIVER("%s: crtc %d - vupdate irq %sabling: r=%d\n",
+			 __func__, acrtc->crtc_id, enable ? "en" : "dis", rc);
+	return rc;
+}
 
 static inline int dm_set_vblank(struct drm_crtc *crtc, bool enable)
 {
 	enum dc_irq_source irq_source;
 	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 	struct amdgpu_device *adev = crtc->dev->dev_private;
+	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
+	int rc = 0;
+
+	if (enable) {
+		/* vblank irq on -> Only need vupdate irq in vrr mode */
+		if (amdgpu_dm_vrr_active(acrtc_state))
+			rc = dm_set_vupdate_irq(crtc, true);
+	}
+	else {
+		/* vblank irq off -> vupdate irq off */
+		rc = dm_set_vupdate_irq(crtc, false);
+	}
+
+	if (rc)
+		return rc;
 
 	irq_source = IRQ_TYPE_VBLANK + acrtc->otg_inst;
 	return dc_interrupt_set(adev->dm.dc, irq_source, enable) ? 0 : -EBUSY;
@@ -4741,7 +4859,11 @@  static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
 		 * While VRR is active, we must not disable vblank irq, as a
 		 * reenable after disable would compute bogus vblank/pflip
 		 * timestamps if it likely happened inside display front-porch.
+		 *
+		 * We also need vupdate irq for the actual core vblank handling
+		 * at end of vblank.
 		 */
+		dm_set_vupdate_irq(new_state->base.crtc, true);
 		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);
@@ -4750,6 +4872,7 @@  static void amdgpu_dm_handle_vrr_transition(struct dm_crtc_state *old_state,
 		/* Transition VRR active -> inactive:
 		 * Allow vblank irq disable again for fixed refresh rate.
 		 */
+		dm_set_vupdate_irq(new_state->base.crtc, false);
 		drm_crtc_vblank_put(new_state->base.crtc);
 		DRM_DEBUG_DRIVER("%s: crtc=%u VRR on->off: Drop vblank ref\n",
 				 __func__, new_state->base.crtc->base.id);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index f741ea3..0b35f84 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -182,6 +182,15 @@  struct amdgpu_display_manager {
 	struct common_irq_params
 	vblank_params[DC_IRQ_SOURCE_VBLANK6 - DC_IRQ_SOURCE_VBLANK1 + 1];
 
+	/**
+	 * @vupdate_params:
+	 *
+	 * Vertical update IRQ parameters, passed to registered handlers when
+	 * triggered.
+	 */
+	struct common_irq_params
+	vupdate_params[DC_IRQ_SOURCE_VUPDATE6 - DC_IRQ_SOURCE_VUPDATE1 + 1];
+
 	spinlock_t irq_handler_list_table_lock;
 
 	struct backlight_device *backlight_dev;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
index cd10f77..fee7837 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
@@ -674,11 +674,30 @@  static int amdgpu_dm_set_crtc_irq_state(struct amdgpu_device *adev,
 		__func__);
 }
 
+static int amdgpu_dm_set_vupdate_irq_state(struct amdgpu_device *adev,
+					   struct amdgpu_irq_src *source,
+					   unsigned crtc_id,
+					   enum amdgpu_interrupt_state state)
+{
+	return dm_irq_state(
+		adev,
+		source,
+		crtc_id,
+		state,
+		IRQ_TYPE_VUPDATE,
+		__func__);
+}
+
 static const struct amdgpu_irq_src_funcs dm_crtc_irq_funcs = {
 	.set = amdgpu_dm_set_crtc_irq_state,
 	.process = amdgpu_dm_irq_handler,
 };
 
+static const struct amdgpu_irq_src_funcs dm_vupdate_irq_funcs = {
+	.set = amdgpu_dm_set_vupdate_irq_state,
+	.process = amdgpu_dm_irq_handler,
+};
+
 static const struct amdgpu_irq_src_funcs dm_pageflip_irq_funcs = {
 	.set = amdgpu_dm_set_pflip_irq_state,
 	.process = amdgpu_dm_irq_handler,
@@ -695,6 +714,9 @@  void amdgpu_dm_set_irq_funcs(struct amdgpu_device *adev)
 	adev->crtc_irq.num_types = adev->mode_info.num_crtc;
 	adev->crtc_irq.funcs = &dm_crtc_irq_funcs;
 
+	adev->vupdate_irq.num_types = adev->mode_info.num_crtc;
+	adev->vupdate_irq.funcs = &dm_vupdate_irq_funcs;
+
 	adev->pageflip_irq.num_types = adev->mode_info.num_crtc;
 	adev->pageflip_irq.funcs = &dm_pageflip_irq_funcs;
 
diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
index afe0876..86987f5 100644
--- a/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
+++ b/drivers/gpu/drm/amd/display/dc/irq/dce110/irq_service_dce110.c
@@ -81,6 +81,11 @@  static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 	.ack = NULL
 };
 
+static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
+	.set = NULL,
+	.ack = NULL
+};
+
 #define hpd_int_entry(reg_num)\
 	[DC_IRQ_SOURCE_HPD1 + reg_num] = {\
 		.enable_reg = mmHPD ## reg_num ## _DC_HPD_INT_CONTROL,\
@@ -137,7 +142,7 @@  static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
 		.ack_value =\
 		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
-		.funcs = &vblank_irq_info_funcs\
+		.funcs = &vupdate_irq_info_funcs\
 	}
 
 #define vblank_int_entry(reg_num)\
diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
index 1ea7256..750ba0a 100644
--- a/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
+++ b/drivers/gpu/drm/amd/display/dc/irq/dce120/irq_service_dce120.c
@@ -84,6 +84,11 @@  static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 	.ack = NULL
 };
 
+static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
+	.set = NULL,
+	.ack = NULL
+};
+
 #define BASE_INNER(seg) \
 	DCE_BASE__INST0_SEG ## seg
 
@@ -140,7 +145,7 @@  static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 		IRQ_REG_ENTRY(CRTC, reg_num,\
 			CRTC_INTERRUPT_CONTROL, CRTC_V_UPDATE_INT_MSK,\
 			CRTC_V_UPDATE_INT_STATUS, CRTC_V_UPDATE_INT_CLEAR),\
-		.funcs = &vblank_irq_info_funcs\
+		.funcs = &vupdate_irq_info_funcs\
 	}
 
 #define vblank_int_entry(reg_num)\
diff --git a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
index 8a2066c..de218fe 100644
--- a/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
+++ b/drivers/gpu/drm/amd/display/dc/irq/dce80/irq_service_dce80.c
@@ -84,6 +84,10 @@  static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 	.ack = NULL
 };
 
+static const struct irq_source_info_funcs vupdate_irq_info_funcs = {
+	.set = NULL,
+	.ack = NULL
+};
 
 #define hpd_int_entry(reg_num)\
 	[DC_IRQ_SOURCE_INVALID + reg_num] = {\
@@ -142,7 +146,7 @@  static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
 		.ack_value =\
 		CRTC_V_UPDATE_INT_STATUS__CRTC_V_UPDATE_INT_CLEAR_MASK,\
-		.funcs = &vblank_irq_info_funcs\
+		.funcs = &vupdate_irq_info_funcs\
 	}
 
 #define vblank_int_entry(reg_num)\
diff --git a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
index e04ae49..10ac6de 100644
--- a/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
+++ b/drivers/gpu/drm/amd/display/dc/irq/dcn10/irq_service_dcn10.c
@@ -56,6 +56,18 @@  enum dc_irq_source to_dal_irq_source_dcn10(
 		return DC_IRQ_SOURCE_VBLANK5;
 	case DCN_1_0__SRCID__DC_D6_OTG_VSTARTUP:
 		return DC_IRQ_SOURCE_VBLANK6;
+	case DCN_1_0__SRCID__OTG0_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE1;
+	case DCN_1_0__SRCID__OTG1_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE2;
+	case DCN_1_0__SRCID__OTG2_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE3;
+	case DCN_1_0__SRCID__OTG3_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE4;
+	case DCN_1_0__SRCID__OTG4_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE5;
+	case DCN_1_0__SRCID__OTG5_IHC_V_UPDATE_NO_LOCK_INTERRUPT:
+		return DC_IRQ_SOURCE_VUPDATE6;
 	case DCN_1_0__SRCID__HUBP0_FLIP_INTERRUPT:
 		return DC_IRQ_SOURCE_PFLIP1;
 	case DCN_1_0__SRCID__HUBP1_FLIP_INTERRUPT:
@@ -153,6 +165,11 @@  static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 	.ack = NULL
 };
 
+static const struct irq_source_info_funcs vupdate_no_lock_irq_info_funcs = {
+	.set = NULL,
+	.ack = NULL
+};
+
 #define BASE_INNER(seg) \
 	DCE_BASE__INST0_SEG ## seg
 
@@ -203,12 +220,15 @@  static const struct irq_source_info_funcs vblank_irq_info_funcs = {
 		.funcs = &pflip_irq_info_funcs\
 	}
 
-#define vupdate_int_entry(reg_num)\
+/* vupdate_no_lock_int_entry maps to DC_IRQ_SOURCE_VUPDATEx, to match semantic
+ * of DCE's DC_IRQ_SOURCE_VUPDATEx.
+ */
+#define vupdate_no_lock_int_entry(reg_num)\
 	[DC_IRQ_SOURCE_VUPDATE1 + reg_num] = {\
 		IRQ_REG_ENTRY(OTG, reg_num,\
-			OTG_GLOBAL_SYNC_STATUS, VUPDATE_INT_EN,\
-			OTG_GLOBAL_SYNC_STATUS, VUPDATE_EVENT_CLEAR),\
-		.funcs = &vblank_irq_info_funcs\
+			OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_INT_EN,\
+			OTG_GLOBAL_SYNC_STATUS, VUPDATE_NO_LOCK_EVENT_CLEAR),\
+		.funcs = &vupdate_no_lock_irq_info_funcs\
 	}
 
 #define vblank_int_entry(reg_num)\
@@ -315,12 +335,12 @@  irq_source_info_dcn10[DAL_IRQ_SOURCES_NUMBER] = {
 	dc_underflow_int_entry(6),
 	[DC_IRQ_SOURCE_DMCU_SCP] = dummy_irq_entry(),
 	[DC_IRQ_SOURCE_VBIOS_SW] = dummy_irq_entry(),
-	vupdate_int_entry(0),
-	vupdate_int_entry(1),
-	vupdate_int_entry(2),
-	vupdate_int_entry(3),
-	vupdate_int_entry(4),
-	vupdate_int_entry(5),
+	vupdate_no_lock_int_entry(0),
+	vupdate_no_lock_int_entry(1),
+	vupdate_no_lock_int_entry(2),
+	vupdate_no_lock_int_entry(3),
+	vupdate_no_lock_int_entry(4),
+	vupdate_no_lock_int_entry(5),
 	vblank_int_entry(0),
 	vblank_int_entry(1),
 	vblank_int_entry(2),