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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxx/
>>>
>>
>>
>> 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.


>         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@xxxxxxxxxx>
>>>>>>>
>>>>>>> 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@xxxxxxxxxx>
>>>>>>> Signed-off-by: Jackie Liu <liuyun01@xxxxxxxxxx>
>>>>>>> ---
>>>>>>>    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
>>>



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux