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

[Yongbang Shi <shiyongbang@huawei.com>]

> Hi
> 
> Am 20.04.26 um 16:00 schrieb Yongbang Shi:
>>> Hi
>>>
>>> Am 20.04.26 um 10:40 schrieb Yongbang Shi:
>>> [...]
>>>>   From the patch you contributed,，I understand that you are using an
>>>> RH1288H V3 server. I have confirmed with our chief hardware engineer
>>>> that the board design for this series of servers does not connect the
>>>> BMC chip’s DDC I2C to the VGA port.
>>> Thanks a lot for double-checking. That's the info I was missing. I suggest to build a workaround into the driver to
>>> filter out these boards and always return 'connected' on them. IIUC even with these new callbacks, the driver would not
>>> work as expected.
>>>
>> Our BMC chips can distinguish between generations, but since BMC chips of the same generation are used on many different
> 
> That's not really much of a problem. Just filter the older boards by some board identifier and handle it separately. We
> use that all over the kernel; including graphics drives. See for example DMI_MATCH. [1]
> 
Okay, we'll need to verify this with the BIOS and do some analysis. If necessary, we'll submit a patch as a feature
later on.

> [1] https://elixir.bootlin.com/linux/v7.0/A/ident/DMI_MATCH
> 
>> boards, filtering by board type or board id is not feasible. We can only determine whether DDC I2C is present by
>> comparing BMC chips of different generations. We have mandated that the board implementation must connect to DDC I2C on
>> the latest BMC chip.
>>
>>>> [1]https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/gpu/drm/hisilicon/hibmc/
>>>> hibmc_drm_vdac.c?h=v6.12.82#L60
>>>> [2]https://lore.kernel.org/all/20250320101455.2538835-10-shiyongbang@huawei.com/
>>>>
>>>>
>>>>>> Signed-off-by: Lin He <helin52@huawei.com>
>>>>>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>>>>>> ---
>>>>> [...]
>>>>>> +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);
>>>>>> +    struct hibmc_vdac *vdac = to_hibmc_vdac(connector);
>>>>>> +
>>>>>> +    vdac->phys_state = drm_connector_helper_detect_from_ddc(connector,
>>>>>> +                                ctx, force);
>>>>>> +
>>>>>> +    /* If the DP connectors are disconnected, the hibmc_vdac_detect
>>>>>> function
>>>>>> +     * must return a connected state to ensure KVM display
>>>>>> functionality.
>>>>>> +     * Additionally, for previous-generation products that may lack
>>>>>> hardware
>>>>>> +     * link support and thus cannot detect the monitor,
>>>>>> hibmc_vdac_detect
>>>>>> +     * should also return a connected state.
>>>>>> +     */
>>>>>> +    if (priv->dp.phys_state != connector_status_connected)
>>>>>> +        return connector_status_connected;
>>>>>> +
>>>>>> +    return vdac->phys_state;
>>>>> This will break user-space compositors if DP and VGA are connected at
>>>>> the same time. I know, because we had such logic in ast and mgag200.
>>>>>
>>>>> Today's compositors expect a single encoder-connector pair on each CRTC.
>>>>> That's what most contemporary hardware provides.  But the server
>>>>> chipsets usually come with one a single CRTC and multiple connectors
>>>>> attached to it.  Compositors fail to configure that. When we had this in
>>>>> ast and mgag200, Gnome would display garbage to the screen and mode-
>>>>> setting would fail. The bug report is at [1].
>>>>>
>>>>> In ast and mgag200, only one connector would be installed on a single
>>>>> system. So we could avoid the problem. It looks like that this is not an
>>>>> option with hibmc. VGA always seems to be present and DP seems optional.
>>>>> I would advice to only report one of the connectors as 'connected' if
>>>>> both are installed on a system.
>>>>>
>>>> According to our BMC chip specifications, both VGA and DisplayPort
>>>> outputs are required, but only one connection is allowed at a time,
>>>> which seems impractical.
>>> That's true.
>>>
>>> Yet it needs support form user space to make it work. It's not a kernel problem.
>>>
>>>> Do you have any additional details regarding this issue with the Mutter
>>>> synthesizer? For example, steps to reproduce the issue or more specific
>>>> alert logs. I looked at the issue you submitted to Mutter, which
>>>> describes the phenomenon as "a warning that the configuration is
>>>> incompatible", Could you clarify what this warning specifically refers
>>>> to—is it a message displayed on the screen, or is it a specific entry in
>>>> the logs?
>>> In ast, we used to have at least one connector for the physical output and another connector for the BMC/KVM. Running
>>> Gnome and opening the Settings app did not work correctly. One issue is that display configuration could not be changed
>>> in Gnome. That's the reported error of changes not being applicable.
>>>
>> Great, regarding hibmc-drm's support for DP, my initial idea was also to create separate connectors for DP, VGA, and
>> KVM. However, due to some non-technical reasons, that approach wasn't implemented. Since ast has implemented it this
>> way, we can also consider this solution.
> 
> I added a connector for the KVM to ast, but it turned out to be a mistake, as it triggered these user-space bugs. Hence
> the current design in ast. The KVM isn't really a connector. It's more of a bridge, but next to the CRTC instead of next
> to the connector; like this:
> 
>   Plane -> CRTC -> KVM-bridge -> encoders -> connectors
> 
> We have no good way of modelling this pipeline in DRM.
> 
> In the case of ast and mgag200, the BMC also cannot be disabled. For a real connector, one would expect that it can be
> disabled. This might also confuse compositors.
> 

Okay, got it.

> 
>>
>>> IIRC I've also seen distorted output if I really pushed it. But maybe that has meanwhile been mitigated.
>>>
>>> I've been told that other compositors also don't support this scenario well.
>>>
>>> I think you'd run into this case once someone connects the DP and VGA at the same time.
>>>
>> Okay, I understand. However, so far, we have thoroughly tested the scenario where both connector0 (VGA shared with KVM)
>> and connector1 (DP) are displaying at the same time, and we haven’t encountered any similar issues.
> 
> If this works for you, then let's keep it.  I'm just trying to help you avoid the pitfalls we have seen with other drivers.
> 
> 
>>
>> I will create a dedicated connector for the KVM and conduct the necessary tests to test for this scenario.
> 
> I think the design in the current patch is sound. It just needs to increment epoch_count if the physical status changes;
> and it would trigger these user-space bugs.  But we can only work around the latter.
> 

Okay, I understand, but there's one more thing I need to confirm: simply adding logic to increment `epoch_count` within
`vdac_detect` isn't enough, is it? We still need to enable the `connector.polled` mechanism for periodic detection, right?

like this:
hibmc_kms_init()
{
	...

	drm_kms_helper_poll_init(dev);
	
	...
}

a little change like this:
vdac_detect()
{
	phys_state = detect_from_hw()

	if (dp.phys_state == connected) {
		vdac->phys_state = phys_state;
		return vdac->phys_state;  /* return from here, epoch_counter will be update by drm framework */
	}
	
	if (phys_state != vdac->phys_state)
		++epoch_counter;
	
	vdac->phys_state = phys_state;

	return connector_status_connected;
}

Thanks,
Yongbang.

> Best regards
> Thomas
> 
> 
>>
>>>> Over the next few days, we will focus on testing relevant scenarios
>>>> while setting both the VGA and DP ports to `connected` to see if we
>>>> encounter similar issues or warnings.
>>>>
>>>>> You also need to increment the connector's epoch_counter if the physical
>>>>> status changed. Doing this will trigger DRM clients to re-read the
>>>>> connector state and reconfigure display.
>>>>>
>>>>> Something like that
>>>>>
>>>>> dp_detect()
>>>>> {
>>>>>       dp.phys_state = detect_from_hw()
>>>>>
>>>>>       if (dp.phys_state changed)
>>>>>           ++epoch_counter
>>>>>       return connected
>>>>> }
>>>>>
>>>>> vdac_detect()
>>>>> {
>>>>>       if (dp.phys_state == connected)
>>>>>           return disconnected
>>>>>
>>>> Only one option is displayed here, but it does not meet our specifications.
>>>>
>>>>>       vdac.phys_state = detect_from_hw()
>>>>>
>>>>>       if (vdac.phys_state changed)
>>>>>           ++epoch_counter
>>>>>       return connected
>>>>> }
>>>>>
>>>>> This handles the KVM in each connector's helper. The ast driver is
>>>>> probably the best reference for the current logic. See [2].
>>>>>
>>>> I looked at the implementations of `detect_ctx` in ATS and MGAG200;
>>>> these implementations generally always return `connected`.
>>>>
>>>> If my analysis is correct, the logic should be as follows:
>>>>     - If the `detect_ctx` function implemented by each vendor returns the
>>>>       actual hardware state, there is no need to modify `epoch_counter`;
>>>>       the DRM framework will execute `epoch_counter++` based on the return
>>>>       value of the `detect_ctx` function;
>>>>
>>>>     - If the `detect_ctx` function in a vendor’s code always returns
>>>>       `connected`, then `epoch_counter++` must be performed within the
>>>>       vendor’s `detect_ctx` function; the implementations in ATS and
>>>>       MGAG200 appear to be like this;
>>> Exactly. In ast/mgag200, we had the case that the VGA got disconnected. We'd still return connected, as the BMC/KVM was
>>> present. We now had to reconfigure for the KVM. That requires modifying epoch_counter. You can see at [1] that we only
>>> increment if the physical status changes. [1]  Same goes for re-connecting the VGA display.
>>>
>>> [1] https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/ast/ast_vga.c#L56
>>>
>>>>
>>>> Here is the pseudocode for our current logic. This logic is indeed a bit
>>>> hard to grasp, so I've added some more comments; it might be a bit
>>>> clearer than the patch:
>>>>
>>>> dp_detect()
>>>> {
>>>>      dp.phys_state = detect_from_hw()
>>>>           /* epoch_counter update by drm framework */
>>>>      return dp.phys_state;
>>>> }
>>>>
>>>> vdac_detect()
>>>> {
>>>>      /*
>>>>            * Scene 1: The driver runs on an older version of the BMC chip,
>>>>            *          and `dp.phys_state` is initialized to 0. To maintain
>>>>       *        consistency with the previous hibmc-drm driver
>>>>       *         logic, always return `connected`.
>>>>       * Scene 2: The driver runs on an new version of the BMC chip,
>>>>            *          and `dp.phys_state` is real status, witch is real
>>>>       *        `disconnect`. In this scenario, we must ensure that
>>>>       *        the VGA is `connected` to guarantee that the KVM
>>>>       *        displays properly.
>>>>       */
>>>>      if (dp.phys_state != connected)
>>>>          return connected;
>>>>
>>>>      /*
>>>>       * DP is connected: This driver is definitely running on the
>>>>       *        latest BMC chip, and the DDC I2C is definitely
>>>>       *        connected to the VGA port, so we can confirm whether
>>>>       *        the VGA is present by reading the EDID directly via
>>>>       *        I2C.
>>>>       */
>>>>      vdac.phys_state = detect_from_hw()
>>>>           /* epoch_counter update by drm framework */
>>>>      return vdac.phys_state;
>>> Let's assume the DP is disconnected. If VGA's physical status goes from connected to disconnected, user space will not
>>> know that it needs to reconfigure for the BMC.   (or vice versa)
>>>
>>> To give you some context from earlier discussions around the other drivers: We had the scenario that the user boots up
>>> without a connected VGA display. This would enable the BMC, which sometimes picks a large resolution; say >2k per
>>> dimension. That is problematic for KVM over networks. And if the user now plugs in a physical VGA display, they would be
>>> stuck with the large resolution on the framebuffer console. Maybe the VGA display wouldn't even support such a high-res
>>> mode.  So we set the default for the BMC to 1024x768. That is well manageable for all of today's VGA displays and does
>>> not impose too much overhead for KVM over networks.  As a drawback, it's a bit small on monitors and not nice to look
>>> at. So we modify epoch_counter whenever the physical status changes. This triggers compositors to reconfigure their
>>> screens for the new display.
>>>
>> Yes, this issue does exist. If we uses the `connector.polled` mechanism to monitor the VGA's presence in real time and
>> update the `epoch_counter`, which might resolve the issue. However, upon further consideration, I don't think this is
>> the optimal solution. If we create a separate connector for KVM and distinguish between older-generation BMC chips and
>> newer-generation BMC chips, the problem will be easily solved. In addition, DP and VGA, as well as VGA and KVM, will be
>> decoupled.
>>
>> A temporary solution:
>> /* called by poll work */
>> vdac_detect()
>> {
>>     /* Let’s examine this logic on its own to gain a better understanding. */
>>     if (older-generation)
>>         return connected;
>>
>>     vdac.phys_state = detect_from_hw();
>>     if (vdac.phys_state changed)
>>         ++epoch_counter;
>>
>>     if (dp.phys_state != connected) {
>>         return connected;
>>     }
>>
>>     return vdac.phys_state;
>> }
>>
>>
>> A more elegant solution:
>>        |--connector0(KVM): always connected, do not update epoch_counter.
>>        |
>> crtc--|--connector1(VGA): detect real time by `connector.polled`, update epoch_counter.
>>        |
>>        |--connector1(DP):  detect real time by hpd interrupt, update epoch_counter.
>>
>> kvm_detect()
>> {
>>     return connected;
>> }
>>
>> /* called by poll work */
>> vdac_detect()
>> {
>>     /* for older-generation */
>>     if (older-generation)
>>         return connected;
>>
>>     /* for newer-generation */
>>     vdac.phys_state = detect_from_hw();
>>
>>     /* epoch_counter update by drm framework */
>>     return vdac.phys_state;
>> }
>>
>> dp_detect()
>> {
>>     dp.phys_state = detect_from_hw();
>>
>>     /* epoch_counter update by drm framework */
>>     return dp.phys_state;
>> }
>>
>> Thanks,
>> Yongbang.
>>
>>> Best regards
>>> Thomas
>>>
>>>> }
>>>>
>>>> Sorry for the delay; it took me a little while to figure out how best to
>>>> explain this bug and our current implementation.
>>>>
>>>> Thanks,
>>>> Yongbang.
>>>>
>>>>> [1] https://gitlab.gnome.org/GNOME/mutter/-/issues/3858
>>>>> [2] https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/ast/
>>>>> ast_vga.c#L47
>>>>>
>>>>> 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:
>