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@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.
Cool!!!
If you decide to apply this patch, you can add my tag:
Reviewed-by: Jackie Liu <liuyun01@xxxxxxxxxx>
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@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
>>>>
Content-type: Text/plain
No virus found
Checked by Hillstone Network AntiVirus
