[PATCH] drm: rcar-du: crtc: force depends on cmm

Jackie Liu liu.yun at linux.dev
Wed Jul 28 12:21:40 UTC 2021


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))
         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 at 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 at kylinos.cn>
>>>>>> Signed-off-by: Jackie Liu <liuyun01 at 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
>>


More information about the dri-devel mailing list