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 06.05.26 um 10:56 schrieb Yongbang Shi:
>> Hi,
>>
>> sorry for the late reply. I'm fairly busy.
>>
> Thank you so much for taking the time to review our code. No worries at all—we'd really appreciate it if you could take
> a look whenever you have time.
>
>> 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.
>>
> Sorry, my mistake. My previous reply was not quite right. I'm a little confused about this situation. I focused on
> optimizing the KVM display when no monitor is connected, but when no monitor is connected, `vdac->phys_status` is set to
> `disconnected`. Following the AST implementation you recommended[1], it takes the `else` branch and sets a custom list.
>
> `vdac->phys_state == connected` and `drm_connector_helper_get_modes` returns 0, in this case, either the returned EDID
> does not match any available options, or there is an issue with the display. In such situations, it's good to let DRM
> choose some defaults.
>
> I will follow the AST implementation[1] to make the modification in v6.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/ast/ast_vga.c?h=v7.1-rc2#L31
>
>>> 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.
>>
> Yes, we'll keep that in mind; when developing community code, we should consider the overall architecture.
>
> Thanks,
> Yongbang.

Sounds good. Looking forward to the next revision.

Best regards
Thomas

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