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

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

 



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/

> > 在 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

-- 
Regards,

Laurent Pinchart



[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