Ok, fixed all the style issues and ran checkpatch over the patches. Thanks.
On Tue, Mar 19, 2019 at 2:32 PM Kazlauskas, Nicholas
<Nicholas.Kazlauskas@xxxxxxx> 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 <mario.kleiner.de@xxxxxxxxx>
>
> 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
> > amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> >
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
