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

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

[Thomas Zimmermann]

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)

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

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

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

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

[Thomas Zimmermann]

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)