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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> <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.


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
>>> 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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux