Message-ID: <7082c67a-a336-54fc-dd32-81b9b8c0a64b@suse.de>
Date:   Thu, 17 Mar 2022 20:15:38 +0100
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Arnd Bergmann <arnd@...nel.org>
Cc:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Thierry Reding <thierry.reding@...il.com>,
        Lukas Bulwahn <lukas.bulwahn@...il.com>,
        Kees Cook <keescook@...omium.org>,
        Arnd Bergmann <arnd@...db.de>,
        Jani Nikula <jani.nikula@...el.com>,
        Sam Ravnborg <sam@...nborg.org>,
        Javier Martinez Canillas <javierm@...hat.com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Douglas Anderson <dianders@...omium.org>,
        Deepak Rawat <drawat.floss@...il.com>,
        Noralf Trønnes <noralf@...nnes.org>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Alex Deucher <alexander.deucher@....com>,
        Dillon Min <dillon.minfei@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/panel: add CONFIG_DRM_KMS_HELPER dependencies

Hi Arnd

Am 16.03.22 um 21:59 schrieb Arnd Bergmann:
> On Wed, Mar 16, 2022 at 8:31 PM Thomas Zimmermann <tzimmermann@...e.de> wrote:
>> Am 16.03.22 um 20:12 schrieb Thomas Zimmermann:
>>>>
>>>> Adding a dependency in all drivers that select DRM_MIPI_DBI avoids
>>>> the problem for now, adding the dependency in DRM_MIPI_DBI as well
>>>> should help make it easier to figure out why it breaks if someone
>>>> forgets the dependency the next time.
>>>>
>>>>        tristate
>>>> -    depends on DRM
>>>> +    depends on DRM_KMS_HELPER
>>>
>>> This symbol cannot be selected by users, so it's maybe not a good idea
>>> to depend on it. In fact, I've had to remove such a statement because it
>>> created a cyclic dependency. [1]
> 
> I tried to explain above what I was thinking here: the added dependency
> is both a correct statement (DRM_MIPI_DBI depends on DRM_KMS_HELPER
> because it cannot be built without DRM_KMS_HELPER) and helpful as
> an indication what went wrong if we run into the same problem with a new
> driver, instead of the cryptic link failure you get something like
> 
> WARNING: unmet direct dependencies detected for DRM_MIPI_DBI
>    Depends on [m]: HAS_IOMEM [=y] && DRM_KMS_HELPER [=m]
>    Selected by [y]:
>    - DRM_PANEL_WIDECHIPS_WS2401 [=y] && HAS_IOMEM [=y] && DRM [=y] &&
> DRM_PANEL [=y] && SPI [=y] && GPIOLIB [=y] && BACKLIGHT_CLASS_DEVICE
> [=y]
>    Selected by [m]:
>    - TINYDRM_ILI9225 [=m] && HAS_IOMEM [=y] && DRM [=y] && SPI [=y]
> 
>> [1]
>> https://lore.kernel.org/dri-devel/20220315084559.23510-1-tzimmermann@suse.de/
> 
> 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. (?)

I just added my patch to drm-misc-fixes today and it should show up in 
upstream soon. Maybe just rebase your patch on top of it and if nothing 
breaks let's merge it as-is including the 'depends on'. You can add

Acked-by: Thomas Zimmermann <tzimmermann@...e.de>

in this case.

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.

One issue is that different submodules of DRM seem to use different 
logic for expressing such config dependencies. That's been an endless 
source of problems so far.

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

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.

Best regards
Thomas

> 
>         Arnd

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists