Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies

Arnd Bergmann
2022-03-17T20:55:23Z

On Thu, Mar 17, 2022 at 8:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 16.03.22 um 21:59 schrieb Arnd Bergmann:
> > On Wed, Mar 16, 2022 at 8:31 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >
> > I was going for 'depends on' in the panel drivers because I saw the same being
> > done for other panel drivers, and mixing the two methods causes dependency
> > loops. I looked again now, and find that 'select DRM_KMS_HELPER' is more
> > common for other drivers, and makes sense here because it is generally
> > not user-selectable.
> >
> > The easiest replacement for my patch would then be to just use 'select
> > DRM_KMS_HELPER' from CONFIG_DRM_MIPI_DBI, which makes it
> > safer and more consistent with your change. If you like, I'll send an updated
> > version.
>
> MIPI DBI is another helper and select is not transitive IIRC. So drivers
> would still have to select KMS helpers as well. (?)

Not sure what you mean here: if a driver selects DRM_MIPI_DBI,
and DRM_MIPI_DBI selects DRM_KMS_HELPER, the leaf driver
does not need to select DRM_KMS_HELPER because it is already
selected. This is one of the major problems of overusing 'select' because
you end up unable to turn things off.

Maybe you are thinking of the case where DRM_MIPI_DBI depends
on DRM_KMS_HELPER, and something selects DRM_MIPI_DBI.
In this case, the dependency does /not/ get inherited by the leaf
driver, it needs a copy of the dependency or it triggers a warning,
which is what my patch intended.

> More generally, I think you're right about making DRM helper libraries
> using 'depends on' to link to other libraries. Drivers would at least
> know which config symbols to select. A number of config rules would have
> to be adapted to make that happen, I guess.

Generally speaking, a problem with DRM is that it uses way too
much 'select' to enforce other subsystems to be enabled, this is
what causes DRM to have more problems with incorrect or circular
dependencies, and the only way to avoid that is to be consistent
about the dependencies: each symbol should only be referenced
with either 'select' or 'depends on' but not both, and 'select' should
ideally only be used on hidden symbols.

> > One thing I'm not sure about is whether there is still use for ever having
> > CONFIG_DRM without CONFIG_DRM_KMS_HELPER if it gets selected
> > by almost every driver anyway. Is this actually a configuration that
> > users rely on, or should we just remove the symbol completely and
> > build the KMS helpers unconditionally?
>
> Best leave it as it is. i915 doesn't use it. And since it's a helper, it
> should not be lumped together with core DRM code simply for reasons of
> design.

Ok

> For DRM_KMS_HELPER itself, the mid-term plan is to move some of the code
> into other modules. KMS helpers used to contain all kind of helpers, but
> recently there's interest in reducing the minimum size of a built-in DRM
> with minimal driver support. So the non-essential stuff needs to go into
> modules for the more-sophisticated DRM drivers.

Right, that makes sense.

Arnd