Re: [PATCH for drm-misc-fixes v5 2/4] drm/hisilicon/hibmc: fix no showing when no connectors connected

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 28.04.26 um 05:58 schrieb Yongbang Shi:
[...]
>> There's a problem with the overall logic here: if the physical status is 'connected' and the helper could not retrieve
>> any modes, the helper should return 0 here. Then the DRM helpers do the right thing with setting up a few modes or an
>> EDID override as a fallback. See [2]. For example, on my broken test system, I'd be able to provide my display's EDID
>> and get the correct output.
>>
>> [2] https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/drm_probe_helper.c#L436
>>
>> Any code below is BMC setup and should run in an else branch, just like in ast.
>>
> The `drm_edid_override_connector_update` interface is used to retrieve an overridden EDID from debugfs or from firmware:
>    * Debugfs: Requires the user to manually perform file operations to configure it;
>    * Firmware: Requires the distribution’s operating system or the user to place the EDID binary file in the
>                `/lib/firmware/` directory, and the `drm.edid_firmware` parameter must be specified in the GRUB boot
>                parameters;
>
> The modification method you provided allows for display even when the EDID cannot be retrieved. But both of these
> methods require cooperation from the user or the distribution's operating system, making implementation relatively
> difficult.

Yes, it's the established way for users to override the EDID.

>
> Our use case requires that the KVM's display functionality remain intact even when no VGA or DisplayPort monitor is
> connected. I believe setting `drm_set_preferred_mode` (1024x768) as the default resolution when no monitor is present
> would be a more universal solution. Similarly, on your test system, this approach would ensure basic display functionality.

If no modes could be detected on the VGA and no EDID has been provided 
by the user, DRM helpers will install a set of default modes that are 
suitable for graphics cards. See [1]. The modes installed for the KVM 
are not all compatible with VGA.  If you rely on DRM helpers, you'll 
always get correct modes for VGA and KVM.

Generally speaking, the decision of VGA-vs-KVM should be in ->detect. 
Having phys_state/phys_status does this very well. The ->get_modes 
function should then use whatever has been detected.

[1] 
https://elixir.bootlin.com/linux/v7.0.1/source/drivers/gpu/drm/drm_probe_helper.c#L653

Best regards
Thomas

>
>>>    +    drm_edid_connector_update(connector, NULL);
>>>        count = drm_add_modes_noedid(connector,
>>>                         connector->dev->mode_config.max_width,
>>>                         connector->dev->mode_config.max_height);
>>>        drm_set_preferred_mode(connector, 1024, 768);
>>>    -out:
>>> -    drm_edid_free(drm_edid);
>>> -
>>>        return count;
>>>    }
>>>    @@ -57,10 +50,32 @@ static void hibmc_connector_destroy(struct drm_connector *connector)
>>>        drm_connector_cleanup(connector);
>>>    }
>>>    +static int hibmc_vdac_detect(struct drm_connector *connector,
>>> +                 struct drm_modeset_acquire_ctx *ctx,
>>> +                 bool force)
>>> +{
>>> +    struct hibmc_drm_private *priv = to_hibmc_drm_private(connector->dev);
>>> +    int state = drm_connector_helper_detect_from_ddc(connector, ctx,
>>> +                             force);
>> 'state' -> 'status'
>>
> Okay.
>
>>> +    struct hibmc_vdac *vdac = to_hibmc_vdac(connector);
>>> +
>>> +    if (priv->dp.phys_state == connector_status_connected)
>>> +        return vdac->phys_state = state;
>> Please only one statement per line. First assign, then return.
>>
> Yes, Sorry about that. According to the Linux kernel coding guidelines, this line of code should be split.
>
>
> Thanks,
> Yongbang.
>
>>> +
>>> +    if (state != vdac->phys_state)
>>> +        ++connector->epoch_counter;
>>> +    vdac->phys_state = state;
>>> +
>>> +    /* If both the DP and VDAC physical states are disconnected,
>>> +     * the "connected" status is returned to support KVM display.
>>> +     */
>>> +    return connector_status_connected;
>> I haven't tried, but I think this should also resolve the problems on my test systems. Great, thanks a lot! I might just
>> get default resolutions for now, but that's OK.
>>
>> Best regards
>> Thomas
>>
>>> +}
>>> +
>>>    static const struct drm_connector_helper_funcs
>>>        hibmc_connector_helper_funcs = {
>>>        .get_modes = hibmc_connector_get_modes,
>>> -    .detect_ctx = drm_connector_helper_detect_from_ddc,
>>> +    .detect_ctx = hibmc_vdac_detect,
>>>    };
>>>      static const struct drm_connector_funcs hibmc_connector_funcs = {
>>> @@ -130,6 +145,8 @@ int hibmc_vdac_init(struct hibmc_drm_private *priv)
>>>          connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
>>>    +    vdac->phys_state = connector_status_connected;
>>> +
>>>        return 0;
>>>      err:

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)