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

[Yongbang Shi]

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

Yes, I understand what you mean. But there's another issue here; we've also tested the scenario where we directly return
0. However, we found that only three resolutions remain supported: 1024*768, 800*600 and 640*480. This is because the
resolution added via `drm_add_modes_noedid` in the helper has a maximum of 1024*768. This doesn't look good—there are
too few resolutions available for users to choose from in KVM.

With the current implementation, when `count == 0`, we set the resolution to the highest resolution supported by our
CRTC, allowing users more options.

Of course, this implementation has its drawbacks; it doesn't allow users to provide a custom resolution list via debugfs
using the override method when `count == 0`. But this method isn't very important for how our product is used.

Thanks,
Yongbang.

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