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)