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,

sorry for the late reply. I'm fairly busy.

Am 28.04.26 um 14:53 schrieb 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.

But why do you second-guess the results from ->detect?  Th call to 
->detect already established that there's a display connected.  If 
->get_modes cannot find useful modes, then don't treat it as a BMC now. 
Return 0 and let DRM choose some same defaults.  These 3 low-res modes 
act as a safe base line. Reporting high-res BMC modes might accidentally 
fry someone's monitor.

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

The upstream Linux kernel is not a product, but a shared commodity. It 
needs to work in everyone's best interest.

Best regards
Thomas

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

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