Hi Kieran.
在 2021/7/28 下午8:24, Kieran Bingham 写道:
Hi Jackie
On 28/07/2021 13:21, Jackie Liu wrote:
Hi Kieran.
在 2021/7/28 下午8:13, Kieran Bingham 写道:
Hi Jackie / Laurent,
On 28/07/2021 12:30, Laurent Pinchart wrote:
On Wed, Jul 28, 2021 at 12:09:34PM +0100, Kieran Bingham wrote:
Hi Jackie,
On 28/07/2021 10:57, Jackie Liu wrote:
Hi Kieran.
How about this.
diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig index b47e74421e34..14cf3e6415d7 100644 --- a/drivers/gpu/drm/rcar-du/Kconfig +++ b/drivers/gpu/drm/rcar-du/Kconfig @@ -1,7 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 config DRM_RCAR_DU tristate "DRM Support for R-Car Display Unit" - depends on DRM && OF + depends on DRM && OF && m depends on ARM || ARM64 depends on ARCH_RENESAS || COMPILE_TEST imply DRM_RCAR_CMM
Of course, this is not a good way, in fact, as long as rcar-du built-in, cmm must also be built-in, otherwise an error will be reported.
Correct, ideally we should enforce that if the RCAR_DU is built in (y), then the CMM can only be y/n and not m.
I thought that the depends on DRM_RCAR_DU should do that, but it appears it doesn't enforce the built-in rule to match...
Do you have a good way?
Kconfig-language.rst says:
Note: If the combination of FOO=y and BAR=m causes a link error, you can guard the function call with IS_REACHABLE()::
foo_init() { if (IS_REACHABLE(CONFIG_BAZ)) baz_register(&foo); ... }
But that seems redundant, so I suspect we could/should change the drivers/gpu/drm/rcar-du/rcar_cmm.h from:
#if IS_ENABLED(CONFIG_DRM_RCAR_CMM) to #if IS_REACHABLE(CONFIG_DRM_RCAR_CMM)
...
Seems odd that we might allow the module to be compiled at all if it won't be reachable and that we can't enforce that at the KConfig level - but at least IS_REACHABLE would prevent the linker error..
This has been discussed before:
https://lore.kernel.org/dri-devel/20200408202711.1198966-6-arnd@arndb.de/
Arnd suggested this as a solution which looks like it solves the overall issue (locally to cmm) with the current restrictions we have...
In that case, a Makefile trick could also work, doing
ifdef CONFIG_DRM_RCAR_CMM obj-$(CONFIG_DRM_RCAR_DU) += rcar-cmm.o endif
Thereby making the cmm module have the same state (y or m) as the du module whenever the option is enabled.
That seems like a reasonable solution to me until someone comes up with a solution in KConfig.
Arnd's suggestion it's much better than me.
But I still propose another solution, but it doesn’t seem very clever.
====== diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index ea7e39d03545..23d4f0e1823b 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -512,7 +512,9 @@ static void rcar_du_cmm_setup(struct drm_crtc *crtc) if (drm_lut) cmm_config.lut.table = (struct drm_color_lut *)drm_lut->data;
+#if !(IS_MODULE(CONFIG_DRM_RCAR_CMM) && IS_BUILTIN(CONFIG_DRM_RCAR_DU))
I think you could replace those lines with
if (IS_REACHABLE(CONFIG_DRM_RCAR_CMM))
But I think Arnd's solution is cleaner overall and doesn't require modifying the driver code.
Cool!!!
If you decide to apply this patch, you can add my tag:
Reviewed-by: Jackie Liu liuyun01@kylinos.cn
Thanks. -- Jackie Liu
rcar_cmm_setup(rcrtc->cmm, &cmm_config); +#endif }
/*
@@ -660,8 +662,10 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc) if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) rcar_du_vsp_disable(rcrtc);
+#if !(IS_MODULE(CONFIG_DRM_RCAR_CMM) && IS_BUILTIN(CONFIG_DRM_RCAR_DU)) if (rcrtc->cmm) rcar_cmm_disable(rcrtc->cmm); +#endif
/* * Select switch sync mode. This stops display operation and configures @@ -719,8 +723,10 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc, struct rcar_du_crtc_state *rstate = to_rcar_crtc_state(crtc->state); struct rcar_du_device *rcdu = rcrtc->dev;
+#if !(IS_MODULE(CONFIG_DRM_RCAR_CMM) && IS_BUILTIN(CONFIG_DRM_RCAR_DU)) if (rcrtc->cmm) rcar_cmm_enable(rcrtc->cmm); +#endif rcar_du_crtc_get(rcrtc);
/* diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index fdb8a0d127ad..b6a10fa7aaa4 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -726,7 +726,12 @@ static int rcar_du_cmm_init(struct rcar_du_device *rcdu) * -ENODEV is used to report that the CMM config option is * disabled: return 0 and let the DU continue probing. */ - ret = rcar_cmm_init(pdev); + ret = +#if !(IS_MODULE(CONFIG_DRM_RCAR_CMM) && IS_BUILTIN(CONFIG_DRM_RCAR_DU)) + rcar_cmm_init(pdev); +#else + -ENODEV; if (ret) { platform_device_put(pdev); return ret == -ENODEV ? 0 : ret;
It doesn’t look good, but it seems to solve the problem.
-- Jackie Liu
-- Kieran
在 2021/7/28 下午4:58, Kieran Bingham 写道: > On 28/07/2021 09:42, Jackie Liu wrote: >> From: Jackie Liu liuyun01@kylinos.cn >> >> After the patch 78b6bb1d24db ("drm: rcar-du: crtc: Control CMM >> operations"), >> the cmm module must be included in the crtc. We cannot remove this >> configuration option separately. Therefore, simply linking them >> together >> is the best solution, otherwise some errors will be reported. >> > > Yikes, I'm sure we've had plenty of problems with the config > options on > this module. The churn alone makes me wonder if it should just be > part > of the overall module to simplify things... but... > >> rcar_du_crtc.c:(.text+0x194): undefined reference to >> `rcar_cmm_setup' >> rcar_du_crtc.c:(.text+0x11bc): undefined reference to >> `rcar_cmm_enable' >> rcar_du_crtc.c:(.text+0x1604): undefined reference to >> `rcar_cmm_disable' >> rcar_du_kms.c:(.text+0x768): undefined reference to >> `rcar_cmm_init' > > Those are guarded by #if IS_ENABLED in the header. > > So from that - perhaps we can assume that the config attempted > here was > a built-in DU - but a module CMM which wouldn't be supported? > > > >> Fixes: 78b6bb1d24db ("rm: rcar-du: crtc: Control CMM operations") > > That fixes tag is missing a 'd', but that's trivial. > > >> Reported-by: kernel-bot k2ci@kylinos.cn >> Signed-off-by: Jackie Liu liuyun01@kylinos.cn >> --- >> drivers/gpu/drm/rcar-du/Kconfig | 8 -------- >> drivers/gpu/drm/rcar-du/Makefile | 2 +- >> 2 files changed, 1 insertion(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/rcar-du/Kconfig >> b/drivers/gpu/drm/rcar-du/Kconfig >> index b47e74421e34..bc71ad2a472f 100644 >> --- a/drivers/gpu/drm/rcar-du/Kconfig >> +++ b/drivers/gpu/drm/rcar-du/Kconfig >> @@ -4,7 +4,6 @@ config DRM_RCAR_DU >> depends on DRM && OF >> depends on ARM || ARM64 >> depends on ARCH_RENESAS || COMPILE_TEST >> - imply DRM_RCAR_CMM >> imply DRM_RCAR_LVDS >> select DRM_KMS_HELPER >> select DRM_KMS_CMA_HELPER >> @@ -14,13 +13,6 @@ config DRM_RCAR_DU >> Choose this option if you have an R-Car chipset. >> If M is selected the module will be called rcar-du-drm. >> -config DRM_RCAR_CMM >> - tristate "R-Car DU Color Management Module (CMM) Support" >> - depends on DRM && OF >> - depends on DRM_RCAR_DU >> - help >> - Enable support for R-Car Color Management Module (CMM). >> - > > I suspect the issue lies somewhere in this config that the CMM is not > being enforced to be a built in when the DU is built in ? > > >> config DRM_RCAR_DW_HDMI >> tristate "R-Car Gen3 and RZ/G2 DU HDMI Encoder Support" >> depends on DRM && OF >> diff --git a/drivers/gpu/drm/rcar-du/Makefile >> b/drivers/gpu/drm/rcar-du/Makefile >> index 4d1187ccc3e5..76ff2e15bc2d 100644 >> --- a/drivers/gpu/drm/rcar-du/Makefile >> +++ b/drivers/gpu/drm/rcar-du/Makefile >> @@ -5,6 +5,7 @@ rcar-du-drm-y := rcar_du_crtc.o \ >> rcar_du_group.o \ >> rcar_du_kms.o \ >> rcar_du_plane.o \ >> + rcar_cmm.o >> rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += rcar_du_of.o \ >> rcar_du_of_lvds_r8a7790.dtb.o \ >> @@ -15,7 +16,6 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS) += >> rcar_du_of.o \ >> rcar-du-drm-$(CONFIG_DRM_RCAR_VSP) += rcar_du_vsp.o >> rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o >> -obj-$(CONFIG_DRM_RCAR_CMM) += rcar_cmm.o >> obj-$(CONFIG_DRM_RCAR_DU) += rcar-du-drm.o >> obj-$(CONFIG_DRM_RCAR_DW_HDMI) += rcar_dw_hdmi.o >> obj-$(CONFIG_DRM_RCAR_LVDS) += rcar_lvds.o
Content-type: Text/plain
No virus found Checked by Hillstone Network AntiVirus