Re: [RFC PATCH v2 1/3] drm/bridge: cadence: cdns-mhdp8546-core: Remove legacy support for connector initialisation in bridge

[Jayesh Choudhary <j-choudhary@ti.com>]

On 27/05/25 14:47, Tomi Valkeinen wrote:
> Hi,
> 
> On 27/05/2025 11:41, Jayesh Choudhary wrote:
>> Hello Tomi,
>>
>> On 27/05/25 13:08, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On 21/05/2025 10:32, Jayesh Choudhary wrote:
>>>> Now that we have DBANC framework, remove the connector initialisation
>>>> code
>>>> as that piece of code is not called if DRM_BRIDGE_ATTACH_NO_CONNECTOR
>>>> flag
>>>> is used. Only TI K3 platforms consume this driver and tidss (their
>>>> display
>>>> controller) has this flag set. So this legacy support can be dropped.
>>>>
>>>
>>> Why is the series RFC? Does it not work? Is there something here you're
>>> not comfortable with?
>>
>> These changes work without any issue.
>>
>> I was a little doubtful about the second patch so kept it as RFC.
> 
> You should explain why the series/patch is RFC, unless it's somehow
> obvious. In this series I didn't see any TODOs or open questions or
> anything.
> 

Oh okay. Got it. Will drop RFC prefix in v3.

>>>
>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>> ---
>>>>    .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 186 +++---------------
>>>>    1 file changed, 25 insertions(+), 161 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/
>>>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>> index b431e7efd1f0..66bd916c2fe9 100644
>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>> @@ -1444,56 +1444,6 @@ static const struct drm_edid
>>>> *cdns_mhdp_edid_read(struct cdns_mhdp_device *mhdp,
>>>>        return drm_edid_read_custom(connector,
>>>> cdns_mhdp_get_edid_block, mhdp);
>>>>    }
>>>>    -static int cdns_mhdp_get_modes(struct drm_connector *connector)
>>>> -{
>>>> -    struct cdns_mhdp_device *mhdp = connector_to_mhdp(connector);
>>>> -    const struct drm_edid *drm_edid;
>>>> -    int num_modes;
>>>> -
>>>> -    if (!mhdp->plugged)
>>>> -        return 0;
>>>> -
>>>> -    drm_edid = cdns_mhdp_edid_read(mhdp, connector);
>>>> -
>>>> -    drm_edid_connector_update(connector, drm_edid);
>>>> -
>>>> -    if (!drm_edid) {
>>>> -        dev_err(mhdp->dev, "Failed to read EDID\n");
>>>> -        return 0;
>>>> -    }
>>>> -
>>>> -    num_modes = drm_edid_connector_add_modes(connector);
>>>> -    drm_edid_free(drm_edid);
>>>> -
>>>> -    /*
>>>> -     * HACK: Warn about unsupported display formats until we deal
>>>> -     *       with them correctly.
>>>> -     */
>>>> -    if (connector->display_info.color_formats &&
>>>> -        !(connector->display_info.color_formats &
>>>> -          mhdp->display_fmt.color_format))
>>>> -        dev_warn(mhdp->dev,
>>>> -             "%s: No supported color_format found (0x%08x)\n",
>>>> -            __func__, connector->display_info.color_formats);
>>>> -
>>>> -    if (connector->display_info.bpc &&
>>>> -        connector->display_info.bpc < mhdp->display_fmt.bpc)
>>>> -        dev_warn(mhdp->dev, "%s: Display bpc only %d < %d\n",
>>>> -             __func__, connector->display_info.bpc,
>>>> -             mhdp->display_fmt.bpc);
>>>> -
>>>> -    return num_modes;
>>>> -}
>>>> -
>>>> -static int cdns_mhdp_connector_detect(struct drm_connector *conn,
>>>> -                      struct drm_modeset_acquire_ctx *ctx,
>>>> -                      bool force)
>>>> -{
>>>> -    struct cdns_mhdp_device *mhdp = connector_to_mhdp(conn);
>>>> -
>>>> -    return cdns_mhdp_detect(mhdp);
>>>> -}
>>>> -
>>>>    static u32 cdns_mhdp_get_bpp(struct cdns_mhdp_display_fmt *fmt)
>>>>    {
>>>>        u32 bpp;
>>>> @@ -1547,114 +1497,6 @@ bool cdns_mhdp_bandwidth_ok(struct
>>>> cdns_mhdp_device *mhdp,
>>>>        return true;
>>>>    }
>>>>    -static
>>>> -enum drm_mode_status cdns_mhdp_mode_valid(struct drm_connector *conn,
>>>> -                      const struct drm_display_mode *mode)
>>>> -{
>>>> -    struct cdns_mhdp_device *mhdp = connector_to_mhdp(conn);
>>>> -
>>>> -    mutex_lock(&mhdp->link_mutex);
>>>> -
>>>> -    if (!cdns_mhdp_bandwidth_ok(mhdp, mode, mhdp->link.num_lanes,
>>>> -                    mhdp->link.rate)) {
>>>> -        mutex_unlock(&mhdp->link_mutex);
>>>> -        return MODE_CLOCK_HIGH;
>>>> -    }
>>>> -
>>>> -    mutex_unlock(&mhdp->link_mutex);
>>>> -    return MODE_OK;
>>>> -}
>>>> -
>>>> -static int cdns_mhdp_connector_atomic_check(struct drm_connector *conn,
>>>> -                        struct drm_atomic_state *state)
>>>> -{
>>>> -    struct cdns_mhdp_device *mhdp = connector_to_mhdp(conn);
>>>> -    struct drm_connector_state *old_state, *new_state;
>>>> -    struct drm_crtc_state *crtc_state;
>>>> -    u64 old_cp, new_cp;
>>>> -
>>>> -    if (!mhdp->hdcp_supported)
>>>> -        return 0;
>>>> -
>>>> -    old_state = drm_atomic_get_old_connector_state(state, conn);
>>>> -    new_state = drm_atomic_get_new_connector_state(state, conn);
>>>> -    old_cp = old_state->content_protection;
>>>> -    new_cp = new_state->content_protection;
>>>> -
>>>> -    if (old_state->hdcp_content_type != new_state->hdcp_content_type &&
>>>> -        new_cp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
>>>> -        new_state->content_protection =
>>>> DRM_MODE_CONTENT_PROTECTION_DESIRED;
>>>> -        goto mode_changed;
>>>> -    }
>>>> -
>>>> -    if (!new_state->crtc) {
>>>> -        if (old_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED)
>>>> -            new_state->content_protection =
>>>> DRM_MODE_CONTENT_PROTECTION_DESIRED;
>>>> -        return 0;
>>>> -    }
>>>> -
>>>> -    if (old_cp == new_cp ||
>>>> -        (old_cp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
>>>> -         new_cp == DRM_MODE_CONTENT_PROTECTION_ENABLED))
>>>> -        return 0;
>>>> -
>>>> -mode_changed:
>>>> -    crtc_state = drm_atomic_get_new_crtc_state(state, new_state->crtc);
>>>> -    crtc_state->mode_changed = true;
>>>> -
>>>> -    return 0;
>>>> -}
>>>> -
>>>> -static const struct drm_connector_helper_funcs
>>>> cdns_mhdp_conn_helper_funcs = {
>>>> -    .detect_ctx = cdns_mhdp_connector_detect,
>>>> -    .get_modes = cdns_mhdp_get_modes,
>>>> -    .mode_valid = cdns_mhdp_mode_valid,
>>>> -    .atomic_check = cdns_mhdp_connector_atomic_check,
>>>> -};
>>>> -
>>>> -static const struct drm_connector_funcs cdns_mhdp_conn_funcs = {
>>>> -    .fill_modes = drm_helper_probe_single_connector_modes,
>>>> -    .atomic_duplicate_state =
>>>> drm_atomic_helper_connector_duplicate_state,
>>>> -    .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>>>> -    .reset = drm_atomic_helper_connector_reset,
>>>> -    .destroy = drm_connector_cleanup,
>>>> -};
>>>> -
>>>> -static int cdns_mhdp_connector_init(struct cdns_mhdp_device *mhdp)
>>>> -{
>>>> -    u32 bus_format = MEDIA_BUS_FMT_RGB121212_1X36;
>>>> -    struct drm_connector *conn = &mhdp->connector;
>>>> -    struct drm_bridge *bridge = &mhdp->bridge;
>>>> -    int ret;
>>>> -
>>>> -    conn->polled = DRM_CONNECTOR_POLL_HPD;
>>>> -
>>>> -    ret = drm_connector_init(bridge->dev, conn, &cdns_mhdp_conn_funcs,
>>>> -                 DRM_MODE_CONNECTOR_DisplayPort);
>>>> -    if (ret) {
>>>> -        dev_err(mhdp->dev, "Failed to initialize connector with
>>>> drm\n");
>>>> -        return ret;
>>>> -    }
>>>> -
>>>> -    drm_connector_helper_add(conn, &cdns_mhdp_conn_helper_funcs);
>>>> -
>>>> -    ret = drm_display_info_set_bus_formats(&conn->display_info,
>>>> -                           &bus_format, 1);
>>>> -    if (ret)
>>>> -        return ret;
>>>> -
>>>> -    ret = drm_connector_attach_encoder(conn, bridge->encoder);
>>>> -    if (ret) {
>>>> -        dev_err(mhdp->dev, "Failed to attach connector to encoder\n");
>>>> -        return ret;
>>>> -    }
>>>> -
>>>> -    if (mhdp->hdcp_supported)
>>>> -        ret = drm_connector_attach_content_protection_property(conn,
>>>> true);
>>>> -
>>>> -    return ret;
>>>> -}
>>>> -
>>>>    static int cdns_mhdp_attach(struct drm_bridge *bridge,
>>>>                    struct drm_encoder *encoder,
>>>>                    enum drm_bridge_attach_flags flags)
>>>> @@ -1671,9 +1513,11 @@ static int cdns_mhdp_attach(struct drm_bridge
>>>> *bridge,
>>>>            return ret;
>>>>          if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
>>>> -        ret = cdns_mhdp_connector_init(mhdp);
>>>> -        if (ret)
>>>> -            goto aux_unregister;
>>>> +        ret = -EINVAL;
>>>> +        dev_err(mhdp->dev,
>>>> +            "Connector initialisation not supported in bridge_attach
>>>> %d\n",
>>>> +            ret);
>>>> +        goto aux_unregister;
>>>>        }
>>>>          spin_lock(&mhdp->start_lock);
>>>> @@ -2158,6 +2002,25 @@ static const struct drm_edid
>>>> *cdns_mhdp_bridge_edid_read(struct drm_bridge *brid
>>>>        return cdns_mhdp_edid_read(mhdp, connector);
>>>>    }
>>>>    +static enum drm_mode_status
>>>> +cdns_mhdp_bridge_mode_valid(struct drm_bridge *bridge,
>>>> +                const struct drm_display_info *info,
>>>> +                const struct drm_display_mode *mode)
>>>> +{
>>>> +    struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
>>>> +
>>>> +    mutex_lock(&mhdp->link_mutex);
>>>> +
>>>> +    if (!cdns_mhdp_bandwidth_ok(mhdp, mode, mhdp->link.num_lanes,
>>>> +                    mhdp->link.rate)) {
>>>> +        mutex_unlock(&mhdp->link_mutex);
>>>> +        return MODE_CLOCK_HIGH;
>>>> +    }
>>>> +
>>>> +    mutex_unlock(&mhdp->link_mutex);
>>>> +    return MODE_OK;
>>>> +}
>>>> +
>>>>    static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
>>>>        .atomic_enable = cdns_mhdp_atomic_enable,
>>>>        .atomic_disable = cdns_mhdp_atomic_disable,
>>>> @@ -2172,6 +2035,7 @@ static const struct drm_bridge_funcs
>>>> cdns_mhdp_bridge_funcs = {
>>>>        .edid_read = cdns_mhdp_bridge_edid_read,
>>>>        .hpd_enable = cdns_mhdp_bridge_hpd_enable,
>>>>        .hpd_disable = cdns_mhdp_bridge_hpd_disable,
>>>> +    .mode_valid = cdns_mhdp_bridge_mode_valid,
>>>>    };
>>>>      static bool cdns_mhdp_detect_hpd(struct cdns_mhdp_device *mhdp,
>>>> bool *hpd_pulse)
>>>
>>> Why do you need to add bridge mode_valid() when removing the legacy
>>> non-DRM_BRIDGE_ATTACH_NO_CONNECTOR code?
>>
>> Okay. Then I will add the bridge mode_valid as a separate patch.
> 
> Well, it was a question =). But indeed my thought was that it sounded
> like material for a separate patch. Is it more of a fix for the
> DRM_BRIDGE_ATTACH_NO_CONNECTOR case?
> 
>   Tomi
> 

Yes this is a fix for DBANC case.
Without mode_valid, I can see a lot of modes getting propagated which
are not even supported by display monitor.
Since mode_valid was added to connector_funcs in the initial commit, I
will use that commit for "Fixes:" tag

Warm Regards,
Jayesh