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 21.04.26 um 09:24 schrieb Yongbang Shi:
> [...]
>>> 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.
> 
> Right. It's a separate thing.
> 
> 
>>> 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?
> 
> Exactly. The poll helper will periodically run the detect helper to test if the display is still attached. This is
> required on connectors that do not auto-detect this, such as VGA.
> 
> The VGA standard assumes that the display would be connected while the computer was running. But everyone always
> connected and disconnected monitors at runtime, so we have to support that.
> 

Okay, this will further improve our hibmc-drm driver. The v5 patches are coming soon.

Thanks,
Yongbang.

> Best regards
> Thomas
> 
>>
>> 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:
>