| Message ID | 20221001220342.5828-1-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm: rcar-du: Fix Kconfig dependency between RCAR_DU and RCAR_MIPI_DSI | expand |
Hi, On 02/10/2022 01:03, Laurent Pinchart wrote: > When the R-Car MIPI DSI driver was added, it was a standalone encoder > driver without any dependency to or from the R-Car DU driver. Commit > 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") then > added a direct call from the DU driver to the MIPI DSI driver, without > updating Kconfig to take the new dependency into account. Fix it the > same way that the LVDS encoder is handled. > > Fixes: 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/gpu/drm/rcar-du/Kconfig | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig > index c959e8c6be7d..fd2c2eaee26b 100644 > --- a/drivers/gpu/drm/rcar-du/Kconfig > +++ b/drivers/gpu/drm/rcar-du/Kconfig > @@ -44,12 +44,17 @@ config DRM_RCAR_LVDS > select OF_FLATTREE > select OF_OVERLAY > > +config DRM_RCAR_USE_MIPI_DSI > + bool "R-Car DU MIPI DSI Encoder Support" > + depends on DRM_BRIDGE && OF > + default DRM_RCAR_DU > + help > + Enable support for the R-Car Display Unit embedded MIPI DSI encoders. > + > config DRM_RCAR_MIPI_DSI > - tristate "R-Car DU MIPI DSI Encoder Support" > - depends on DRM && DRM_BRIDGE && OF > + def_tristate DRM_RCAR_DU > + depends on DRM_RCAR_USE_MIPI_DSI > select DRM_MIPI_DSI > - help > - Enable support for the R-Car Display Unit embedded MIPI DSI encoders. > > config DRM_RCAR_VSP > bool "R-Car DU VSP Compositor Support" if ARM > > base-commit: 7860d720a84c74b2761c6b7995392a798ab0a3cb Interesting dependency issue. Took me a while to understand it =). But is there a reason to not have "depends on DRM_RCAR_DU" for DRM_RCAR_USE_MIPI_DSI and DRM_RCAR_USE_LVDS? Now the menu items are available even if RCAR_DU is n. That's also the case for DRM_RCAR_DW_HDMI, but I'm not sure if that's supposed to be usable even without RCAR_DU. Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Tomi
Hi Tomi, On Mon, Oct 3, 2022 at 8:56 AM Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote: > On 02/10/2022 01:03, Laurent Pinchart wrote: > > When the R-Car MIPI DSI driver was added, it was a standalone encoder > > driver without any dependency to or from the R-Car DU driver. Commit > > 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") then > > added a direct call from the DU driver to the MIPI DSI driver, without > > updating Kconfig to take the new dependency into account. Fix it the > > same way that the LVDS encoder is handled. > > > > Fixes: 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") > > Reported-by: kernel test robot <lkp@intel.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > --- > > drivers/gpu/drm/rcar-du/Kconfig | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig > > index c959e8c6be7d..fd2c2eaee26b 100644 > > --- a/drivers/gpu/drm/rcar-du/Kconfig > > +++ b/drivers/gpu/drm/rcar-du/Kconfig > > @@ -44,12 +44,17 @@ config DRM_RCAR_LVDS > > select OF_FLATTREE > > select OF_OVERLAY > > > > +config DRM_RCAR_USE_MIPI_DSI > > + bool "R-Car DU MIPI DSI Encoder Support" > > + depends on DRM_BRIDGE && OF > > + default DRM_RCAR_DU > > + help > > + Enable support for the R-Car Display Unit embedded MIPI DSI encoders. > > + > > config DRM_RCAR_MIPI_DSI > > - tristate "R-Car DU MIPI DSI Encoder Support" > > - depends on DRM && DRM_BRIDGE && OF > > + def_tristate DRM_RCAR_DU > > + depends on DRM_RCAR_USE_MIPI_DSI > > select DRM_MIPI_DSI > > - help > > - Enable support for the R-Car Display Unit embedded MIPI DSI encoders. > > > > config DRM_RCAR_VSP > > bool "R-Car DU VSP Compositor Support" if ARM > > > > base-commit: 7860d720a84c74b2761c6b7995392a798ab0a3cb > > Interesting dependency issue. Took me a while to understand it =). > > But is there a reason to not have "depends on DRM_RCAR_DU" for > DRM_RCAR_USE_MIPI_DSI and DRM_RCAR_USE_LVDS? Now the menu items are > available even if RCAR_DU is n. That's also the case for > DRM_RCAR_DW_HDMI, but I'm not sure if that's supposed to be usable even > without RCAR_DU. See https://lore.kernel.org/linux-renesas-soc/cover.1639559338.git.geert+renesas@glider.be/ Didn't get to implement the suggested improvements yet... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 03/10/2022 10:01, Geert Uytterhoeven wrote: > Hi Tomi, > > On Mon, Oct 3, 2022 at 8:56 AM Tomi Valkeinen > <tomi.valkeinen@ideasonboard.com> wrote: >> On 02/10/2022 01:03, Laurent Pinchart wrote: >>> When the R-Car MIPI DSI driver was added, it was a standalone encoder >>> driver without any dependency to or from the R-Car DU driver. Commit >>> 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") then >>> added a direct call from the DU driver to the MIPI DSI driver, without >>> updating Kconfig to take the new dependency into account. Fix it the >>> same way that the LVDS encoder is handled. >>> >>> Fixes: 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") >>> Reported-by: kernel test robot <lkp@intel.com> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >>> --- >>> drivers/gpu/drm/rcar-du/Kconfig | 13 +++++++++---- >>> 1 file changed, 9 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig >>> index c959e8c6be7d..fd2c2eaee26b 100644 >>> --- a/drivers/gpu/drm/rcar-du/Kconfig >>> +++ b/drivers/gpu/drm/rcar-du/Kconfig >>> @@ -44,12 +44,17 @@ config DRM_RCAR_LVDS >>> select OF_FLATTREE >>> select OF_OVERLAY >>> >>> +config DRM_RCAR_USE_MIPI_DSI >>> + bool "R-Car DU MIPI DSI Encoder Support" >>> + depends on DRM_BRIDGE && OF >>> + default DRM_RCAR_DU >>> + help >>> + Enable support for the R-Car Display Unit embedded MIPI DSI encoders. >>> + >>> config DRM_RCAR_MIPI_DSI >>> - tristate "R-Car DU MIPI DSI Encoder Support" >>> - depends on DRM && DRM_BRIDGE && OF >>> + def_tristate DRM_RCAR_DU >>> + depends on DRM_RCAR_USE_MIPI_DSI >>> select DRM_MIPI_DSI >>> - help >>> - Enable support for the R-Car Display Unit embedded MIPI DSI encoders. >>> >>> config DRM_RCAR_VSP >>> bool "R-Car DU VSP Compositor Support" if ARM >>> >>> base-commit: 7860d720a84c74b2761c6b7995392a798ab0a3cb >> >> Interesting dependency issue. Took me a while to understand it =). >> >> But is there a reason to not have "depends on DRM_RCAR_DU" for >> DRM_RCAR_USE_MIPI_DSI and DRM_RCAR_USE_LVDS? Now the menu items are >> available even if RCAR_DU is n. That's also the case for >> DRM_RCAR_DW_HDMI, but I'm not sure if that's supposed to be usable even >> without RCAR_DU. > > See > https://lore.kernel.org/linux-renesas-soc/cover.1639559338.git.geert+renesas@glider.be/ > > Didn't get to implement the suggested improvements yet... Ok, looks good. But I started to wonder, for LVDS and DSI (maybe for HDMI too), what's the point of modules here... If RCAR_DU, LVDS and DSI are compiled as modules, you can load either LVDS or DSI module, but those won't do anything alone. So in practice you always need to load RCAR_DU module too. But if LVDS or DSI are enabled in the kconfig, you _have_ to load those before RCAR_DU. In other words, you can never, e.g. load RCAR_DU and LVDS, but not DSI, if all three are enabled. So why not just compile LVDS and DSI into the RCAR_DU module, simplifying the dependencies, removing this issue altogether? Tomi
Hi Tomi, On Mon, Oct 03, 2022 at 10:31:24AM +0300, Tomi Valkeinen wrote: > On 03/10/2022 10:01, Geert Uytterhoeven wrote: > > On Mon, Oct 3, 2022 at 8:56 AM Tomi Valkeinen wrote: > >> On 02/10/2022 01:03, Laurent Pinchart wrote: > >>> When the R-Car MIPI DSI driver was added, it was a standalone encoder > >>> driver without any dependency to or from the R-Car DU driver. Commit > >>> 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") then > >>> added a direct call from the DU driver to the MIPI DSI driver, without > >>> updating Kconfig to take the new dependency into account. Fix it the > >>> same way that the LVDS encoder is handled. > >>> > >>> Fixes: 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") > >>> Reported-by: kernel test robot <lkp@intel.com> > >>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > >>> --- > >>> drivers/gpu/drm/rcar-du/Kconfig | 13 +++++++++---- > >>> 1 file changed, 9 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig > >>> index c959e8c6be7d..fd2c2eaee26b 100644 > >>> --- a/drivers/gpu/drm/rcar-du/Kconfig > >>> +++ b/drivers/gpu/drm/rcar-du/Kconfig > >>> @@ -44,12 +44,17 @@ config DRM_RCAR_LVDS > >>> select OF_FLATTREE > >>> select OF_OVERLAY > >>> > >>> +config DRM_RCAR_USE_MIPI_DSI > >>> + bool "R-Car DU MIPI DSI Encoder Support" > >>> + depends on DRM_BRIDGE && OF > >>> + default DRM_RCAR_DU > >>> + help > >>> + Enable support for the R-Car Display Unit embedded MIPI DSI encoders. > >>> + > >>> config DRM_RCAR_MIPI_DSI > >>> - tristate "R-Car DU MIPI DSI Encoder Support" > >>> - depends on DRM && DRM_BRIDGE && OF > >>> + def_tristate DRM_RCAR_DU > >>> + depends on DRM_RCAR_USE_MIPI_DSI > >>> select DRM_MIPI_DSI > >>> - help > >>> - Enable support for the R-Car Display Unit embedded MIPI DSI encoders. > >>> > >>> config DRM_RCAR_VSP > >>> bool "R-Car DU VSP Compositor Support" if ARM > >>> > >>> base-commit: 7860d720a84c74b2761c6b7995392a798ab0a3cb > >> > >> Interesting dependency issue. Took me a while to understand it =). > >> > >> But is there a reason to not have "depends on DRM_RCAR_DU" for > >> DRM_RCAR_USE_MIPI_DSI and DRM_RCAR_USE_LVDS? Now the menu items are > >> available even if RCAR_DU is n. That's also the case for > >> DRM_RCAR_DW_HDMI, but I'm not sure if that's supposed to be usable even > >> without RCAR_DU. > > > > See > > https://lore.kernel.org/linux-renesas-soc/cover.1639559338.git.geert+renesas@glider.be/ > > > > Didn't get to implement the suggested improvements yet... > > Ok, looks good. > > But I started to wonder, for LVDS and DSI (maybe for HDMI too), what's > the point of modules here... > > If RCAR_DU, LVDS and DSI are compiled as modules, you can load either > LVDS or DSI module, but those won't do anything alone. So in practice > you always need to load RCAR_DU module too. But if LVDS or DSI are > enabled in the kconfig, you _have_ to load those before RCAR_DU. > > In other words, you can never, e.g. load RCAR_DU and LVDS, but not DSI, > if all three are enabled. > > So why not just compile LVDS and DSI into the RCAR_DU module, > simplifying the dependencies, removing this issue altogether? Patches are welcome :-) I'd do that for v6.2 though, not as a v6.1 fix.
Hi Laurent, On Sun, Oct 2, 2022 at 12:06 AM Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > When the R-Car MIPI DSI driver was added, it was a standalone encoder > driver without any dependency to or from the R-Car DU driver. Commit > 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") then > added a direct call from the DU driver to the MIPI DSI driver, without > updating Kconfig to take the new dependency into account. Fix it the > same way that the LVDS encoder is handled. > > Fixes: 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Thanks for your patch, which is now commit a830a15678593948 ("drm: rcar-du: Fix Kconfig dependency between RCAR_DU and RCAR_MIPI_DSI") in v6.1-rc5. > --- a/drivers/gpu/drm/rcar-du/Kconfig > +++ b/drivers/gpu/drm/rcar-du/Kconfig > @@ -44,12 +44,17 @@ config DRM_RCAR_LVDS > select OF_FLATTREE > select OF_OVERLAY > > +config DRM_RCAR_USE_MIPI_DSI > + bool "R-Car DU MIPI DSI Encoder Support" > + depends on DRM_BRIDGE && OF > + default DRM_RCAR_DU This means this driver is now enabled by default on systems that do not have the MIPI DSI Encoder (e.g. R-Car Gen2), and that we should probably disable it explicitly in shmobile_defconfig. Is that intentional? > + help > + Enable support for the R-Car Display Unit embedded MIPI DSI encoders. > + > config DRM_RCAR_MIPI_DSI > - tristate "R-Car DU MIPI DSI Encoder Support" > - depends on DRM && DRM_BRIDGE && OF > + def_tristate DRM_RCAR_DU > + depends on DRM_RCAR_USE_MIPI_DSI > select DRM_MIPI_DSI > - help > - Enable support for the R-Car Display Unit embedded MIPI DSI encoders. > > config DRM_RCAR_VSP > bool "R-Car DU VSP Compositor Support" if ARM Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On Mon, Nov 14, 2022 at 10:05:58AM +0100, Geert Uytterhoeven wrote: > On Sun, Oct 2, 2022 at 12:06 AM Laurent Pinchart wrote: > > When the R-Car MIPI DSI driver was added, it was a standalone encoder > > driver without any dependency to or from the R-Car DU driver. Commit > > 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") then > > added a direct call from the DU driver to the MIPI DSI driver, without > > updating Kconfig to take the new dependency into account. Fix it the > > same way that the LVDS encoder is handled. > > > > Fixes: 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") > > Reported-by: kernel test robot <lkp@intel.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > Thanks for your patch, which is now commit a830a15678593948 > ("drm: rcar-du: Fix Kconfig dependency between RCAR_DU > and RCAR_MIPI_DSI") in v6.1-rc5. > > > --- a/drivers/gpu/drm/rcar-du/Kconfig > > +++ b/drivers/gpu/drm/rcar-du/Kconfig > > @@ -44,12 +44,17 @@ config DRM_RCAR_LVDS > > select OF_FLATTREE > > select OF_OVERLAY > > > > +config DRM_RCAR_USE_MIPI_DSI > > + bool "R-Car DU MIPI DSI Encoder Support" > > + depends on DRM_BRIDGE && OF > > + default DRM_RCAR_DU > > This means this driver is now enabled by default on systems that do not > have the MIPI DSI Encoder (e.g. R-Car Gen2), and that we should probably > disable it explicitly in shmobile_defconfig. Is that intentional? I don't think so, no. Would you like to send a patch ? If so, it should enable the option in relevant defconfig files. > > + help > > + Enable support for the R-Car Display Unit embedded MIPI DSI encoders. > > + > > config DRM_RCAR_MIPI_DSI > > - tristate "R-Car DU MIPI DSI Encoder Support" > > - depends on DRM && DRM_BRIDGE && OF > > + def_tristate DRM_RCAR_DU > > + depends on DRM_RCAR_USE_MIPI_DSI > > select DRM_MIPI_DSI > > - help > > - Enable support for the R-Car Display Unit embedded MIPI DSI encoders. > > > > config DRM_RCAR_VSP > > bool "R-Car DU VSP Compositor Support" if ARM
Hi Laurent, On Mon, Nov 14, 2022 at 3:22 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Mon, Nov 14, 2022 at 10:05:58AM +0100, Geert Uytterhoeven wrote: > > On Sun, Oct 2, 2022 at 12:06 AM Laurent Pinchart wrote: > > > When the R-Car MIPI DSI driver was added, it was a standalone encoder > > > driver without any dependency to or from the R-Car DU driver. Commit > > > 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") then > > > added a direct call from the DU driver to the MIPI DSI driver, without > > > updating Kconfig to take the new dependency into account. Fix it the > > > same way that the LVDS encoder is handled. > > > > > > Fixes: 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") > > > Reported-by: kernel test robot <lkp@intel.com> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > > > Thanks for your patch, which is now commit a830a15678593948 > > ("drm: rcar-du: Fix Kconfig dependency between RCAR_DU > > and RCAR_MIPI_DSI") in v6.1-rc5. > > > > > --- a/drivers/gpu/drm/rcar-du/Kconfig > > > +++ b/drivers/gpu/drm/rcar-du/Kconfig > > > @@ -44,12 +44,17 @@ config DRM_RCAR_LVDS > > > select OF_FLATTREE > > > select OF_OVERLAY > > > > > > +config DRM_RCAR_USE_MIPI_DSI > > > + bool "R-Car DU MIPI DSI Encoder Support" > > > + depends on DRM_BRIDGE && OF > > > + default DRM_RCAR_DU > > > > This means this driver is now enabled by default on systems that do not > > have the MIPI DSI Encoder (e.g. R-Car Gen2), and that we should probably > > disable it explicitly in shmobile_defconfig. Is that intentional? > > I don't think so, no. Would you like to send a patch ? If so, it should > enable the option in relevant defconfig files. You mean just drop the "default DRM_RCAR_DU" here? I'm wondering if we can solve this in a consistent way. Currently we have: config DRM_RCAR_USE_CMM default DRM_RCAR_DU config DRM_RCAR_DW_HDMI default n config DRM_RCAR_USE_LVDS default DRM_RCAR_DU config DRM_RCAR_MIPI_DSI default n config DRM_RCAR_VSP default y if ARM64 HDMI is only used on R-Car Gen3 parts MIPI_DSI is only used on R-Car Gen4 parts LVDS is used on R-Car Gen2 and Gen3 parts Thoughts? > > > + help > > > + Enable support for the R-Car Display Unit embedded MIPI DSI encoders. > > > + > > > config DRM_RCAR_MIPI_DSI > > > - tristate "R-Car DU MIPI DSI Encoder Support" > > > - depends on DRM && DRM_BRIDGE && OF > > > + def_tristate DRM_RCAR_DU > > > + depends on DRM_RCAR_USE_MIPI_DSI > > > select DRM_MIPI_DSI > > > - help > > > - Enable support for the R-Car Display Unit embedded MIPI DSI encoders. > > > > > > config DRM_RCAR_VSP > > > bool "R-Car DU VSP Compositor Support" if ARM Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig index c959e8c6be7d..fd2c2eaee26b 100644 --- a/drivers/gpu/drm/rcar-du/Kconfig +++ b/drivers/gpu/drm/rcar-du/Kconfig @@ -44,12 +44,17 @@ config DRM_RCAR_LVDS select OF_FLATTREE select OF_OVERLAY +config DRM_RCAR_USE_MIPI_DSI + bool "R-Car DU MIPI DSI Encoder Support" + depends on DRM_BRIDGE && OF + default DRM_RCAR_DU + help + Enable support for the R-Car Display Unit embedded MIPI DSI encoders. + config DRM_RCAR_MIPI_DSI - tristate "R-Car DU MIPI DSI Encoder Support" - depends on DRM && DRM_BRIDGE && OF + def_tristate DRM_RCAR_DU + depends on DRM_RCAR_USE_MIPI_DSI select DRM_MIPI_DSI - help - Enable support for the R-Car Display Unit embedded MIPI DSI encoders. config DRM_RCAR_VSP bool "R-Car DU VSP Compositor Support" if ARM
When the R-Car MIPI DSI driver was added, it was a standalone encoder driver without any dependency to or from the R-Car DU driver. Commit 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") then added a direct call from the DU driver to the MIPI DSI driver, without updating Kconfig to take the new dependency into account. Fix it the same way that the LVDS encoder is handled. Fixes: 957fe62d7d15 ("drm: rcar-du: Fix DSI enable & disable sequence") Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/gpu/drm/rcar-du/Kconfig | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) base-commit: 7860d720a84c74b2761c6b7995392a798ab0a3cb