| Message ID | 1430539706-2945-1-git-send-email-k.kozlowski.k@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2015? 05? 02? 13:08, Krzysztof Kozlowski wrote: > Selecting CONFIG_FB_S3C disables CONFIG_DRM_EXYNOS_FIMD leading to build > error: No, eDP has no any dependency of FIMD but DECON. Just add dependency code like below, config DRM_EXYNOS7_DECON bool "Exynos DRM DECON" - depends on DRM_EXYNOS + depends on DRM_EXYNOS && !FB_S3C > > drivers/built-in.o: In function `exynos_dp_dpms': > binder.c:(.text+0xd6a840): undefined reference to `fimd_dp_clock_enable' > binder.c:(.text+0xd6ab54): undefined reference to `fimd_dp_clock_enable' > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski.k@gmail.com> > --- > drivers/gpu/drm/exynos/exynos_drm_fimd.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.h b/drivers/gpu/drm/exynos/exynos_drm_fimd.h > index b4fcaa568456..db67f3d9786d 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.h > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.h > @@ -10,6 +10,10 @@ > #ifndef _EXYNOS_DRM_FIMD_H_ > #define _EXYNOS_DRM_FIMD_H_ > > +#ifdef CONFIG_DRM_EXYNOS_FIMD > extern void fimd_dp_clock_enable(struct exynos_drm_crtc *crtc, bool enable); > +#else > +static inline void fimd_dp_clock_enable(struct exynos_drm_crtc *crtc, bool enable) {}; > +#endif So above codes are unnecessary. It's really not good to add #ifdef ~ #endif. Thanks, Inki Dae > > #endif /* _EXYNOS_DRM_FIMD_H_ */ >
Hi, On 4 May 2015 at 08:43, Inki Dae <inki.dae@samsung.com> wrote: > On 2015? 05? 02? 13:08, Krzysztof Kozlowski wrote: >> Selecting CONFIG_FB_S3C disables CONFIG_DRM_EXYNOS_FIMD leading to build >> error: > > No, eDP has no any dependency of FIMD but DECON. Just add dependency > code like below, > > config DRM_EXYNOS7_DECON > bool "Exynos DRM DECON" > - depends on DRM_EXYNOS > + depends on DRM_EXYNOS && !FB_S3C But it does clearly and explicitly call fimd_dp_clock_enable from exynos_dp_powero{n,ff}. So the dependency you're proposing seems backwards: it's not an expression of the requirements of the current code (that FIMD DP code be available, i.e. CONFIG_DRM_EXYNOS_FIMD is selected), but an indirect expression of another dependency (CONFIG_FB_S3C disables CONFIG_DRM_EXYNOS_FIMD, so disable CONFIG_FB_S3C). Additionally, as the call comes from exynos_dp_core.c, which is built by CONFIG_DRM_EXYNOS_DP (an explicitly user-selectable option), why shouldn't the dependency be there? Ah, because the dependency on DP is for (DECON || FIMD), but as DECON doesn't provide fimd_dp_clock_enable(), it doesn't seem like it would compile if you selected DECON and not FIMD. So, for me, the cleanest solution would be config DRM_EXYNOS_DP gains a hard dependency on DRM_EXYNOS_FIMD, at least until it can be fixed to compile without FIMD. Cheers, Daniel
2015-05-04 20:34 GMT+09:00 Daniel Stone <daniel@fooishbar.org>: > Hi, > > On 4 May 2015 at 08:43, Inki Dae <inki.dae@samsung.com> wrote: >> On 2015? 05? 02? 13:08, Krzysztof Kozlowski wrote: >>> Selecting CONFIG_FB_S3C disables CONFIG_DRM_EXYNOS_FIMD leading to build >>> error: >> >> No, eDP has no any dependency of FIMD but DECON. Just add dependency >> code like below, >> >> config DRM_EXYNOS7_DECON >> bool "Exynos DRM DECON" >> - depends on DRM_EXYNOS >> + depends on DRM_EXYNOS && !FB_S3C Actually my commit message was not detailed enough. The FB_S3C here won't solve the issue because you may: 1, disable FIMD and FB_S3C, 2, enabke DECON and DP, and it won't compile. Currently the FIMD must be enabled if DRM_EXYNOS_DP is enabled. > > But it does clearly and explicitly call fimd_dp_clock_enable from > exynos_dp_powero{n,ff}. So the dependency you're proposing seems > backwards: it's not an expression of the requirements of the current > code (that FIMD DP code be available, i.e. CONFIG_DRM_EXYNOS_FIMD is > selected), but an indirect expression of another dependency > (CONFIG_FB_S3C disables CONFIG_DRM_EXYNOS_FIMD, so disable > CONFIG_FB_S3C). > > Additionally, as the call comes from exynos_dp_core.c, which is built > by CONFIG_DRM_EXYNOS_DP (an explicitly user-selectable option), why > shouldn't the dependency be there? Ah, because the dependency on DP is > for (DECON || FIMD), but as DECON doesn't provide > fimd_dp_clock_enable(), it doesn't seem like it would compile if you > selected DECON and not FIMD. > > So, for me, the cleanest solution would be config DRM_EXYNOS_DP gains > a hard dependency on DRM_EXYNOS_FIMD, at least until it can be fixed > to compile without FIMD. Right, you correctly pointed current dependencies. Still it looks little hacky because EXYNOS_DP may work with FIMD or DECON. It does not really need FIMD. Using ifdefs in headers is not uncommon - many core subsystems do this that way to provide stubs. Probably the cleanest way would be to provide by FIMD and DECON a common interface for DP for such operation, something like: struct exynos_drm_crtc { struct drm_crtc base; ... void (*clock_enable)(struct exynos_drm_crtc *crtc, bool enable) ); which, if non-NULL, will be called by exynos_dp_core.c: static void exynos_dp_poweron(struct exynos_dp_device *dp) { ... if (crtc->clock_enable) crtc->clock_enable(crtc, true); } What do you think? Best regards, Krzysztof
Hi, On 05/04/2015 02:43 PM, Krzysztof Kozlowski wrote: > 2015-05-04 20:34 GMT+09:00 Daniel Stone <daniel@fooishbar.org>: >> Hi, >> >> On 4 May 2015 at 08:43, Inki Dae <inki.dae@samsung.com> wrote: >>> On 2015? 05? 02? 13:08, Krzysztof Kozlowski wrote: >>>> Selecting CONFIG_FB_S3C disables CONFIG_DRM_EXYNOS_FIMD leading to build >>>> error: >>> >>> No, eDP has no any dependency of FIMD but DECON. Just add dependency >>> code like below, >>> >>> config DRM_EXYNOS7_DECON >>> bool "Exynos DRM DECON" >>> - depends on DRM_EXYNOS >>> + depends on DRM_EXYNOS && !FB_S3C > > Actually my commit message was not detailed enough. The FB_S3C here > won't solve the issue because you may: > 1, disable FIMD and FB_S3C, > 2, enabke DECON and DP, > and it won't compile. > > Currently the FIMD must be enabled if DRM_EXYNOS_DP is enabled. > >> >> But it does clearly and explicitly call fimd_dp_clock_enable from >> exynos_dp_powero{n,ff}. So the dependency you're proposing seems >> backwards: it's not an expression of the requirements of the current >> code (that FIMD DP code be available, i.e. CONFIG_DRM_EXYNOS_FIMD is >> selected), but an indirect expression of another dependency >> (CONFIG_FB_S3C disables CONFIG_DRM_EXYNOS_FIMD, so disable >> CONFIG_FB_S3C). >> >> Additionally, as the call comes from exynos_dp_core.c, which is built >> by CONFIG_DRM_EXYNOS_DP (an explicitly user-selectable option), why >> shouldn't the dependency be there? Ah, because the dependency on DP is >> for (DECON || FIMD), but as DECON doesn't provide >> fimd_dp_clock_enable(), it doesn't seem like it would compile if you >> selected DECON and not FIMD. >> >> So, for me, the cleanest solution would be config DRM_EXYNOS_DP gains >> a hard dependency on DRM_EXYNOS_FIMD, at least until it can be fixed >> to compile without FIMD. > > Right, you correctly pointed current dependencies. Still it looks little > hacky because EXYNOS_DP may work with FIMD or DECON. Are you sure? I have not seen any chipset having DECON and DP. In all chipsets known to me DP is always accompanied by FIMD. I guess it can change in the future, but for now hard dependency on FIMD seems to be OK - it just reflects hardware design. Of course this is just my humble opinion :) Regards Andrzej It does not really > need FIMD. Using ifdefs in headers is not uncommon - many core > subsystems do this that way to provide stubs. > > Probably the cleanest way would be to provide by FIMD and DECON a common > interface for DP for such operation, something like: > struct exynos_drm_crtc { > struct drm_crtc base; > ... > void (*clock_enable)(struct exynos_drm_crtc *crtc, bool enable) > ); > > which, if non-NULL, will be called by exynos_dp_core.c: > static void exynos_dp_poweron(struct exynos_dp_device *dp) > { > ... > if (crtc->clock_enable) > crtc->clock_enable(crtc, true); > } > > What do you think? > > Best regards, > Krzysztof >
W dniu 04.05.2015 o 22:15, Andrzej Hajda pisze: > Hi, > > > On 05/04/2015 02:43 PM, Krzysztof Kozlowski wrote: >> 2015-05-04 20:34 GMT+09:00 Daniel Stone <daniel@fooishbar.org>: >>> Hi, >>> >>> On 4 May 2015 at 08:43, Inki Dae <inki.dae@samsung.com> wrote: >>>> On 2015? 05? 02? 13:08, Krzysztof Kozlowski wrote: >>>>> Selecting CONFIG_FB_S3C disables CONFIG_DRM_EXYNOS_FIMD leading to build >>>>> error: >>>> >>>> No, eDP has no any dependency of FIMD but DECON. Just add dependency >>>> code like below, >>>> >>>> config DRM_EXYNOS7_DECON >>>> bool "Exynos DRM DECON" >>>> - depends on DRM_EXYNOS >>>> + depends on DRM_EXYNOS && !FB_S3C >> >> Actually my commit message was not detailed enough. The FB_S3C here >> won't solve the issue because you may: >> 1, disable FIMD and FB_S3C, >> 2, enabke DECON and DP, >> and it won't compile. >> >> Currently the FIMD must be enabled if DRM_EXYNOS_DP is enabled. >> >>> >>> But it does clearly and explicitly call fimd_dp_clock_enable from >>> exynos_dp_powero{n,ff}. So the dependency you're proposing seems >>> backwards: it's not an expression of the requirements of the current >>> code (that FIMD DP code be available, i.e. CONFIG_DRM_EXYNOS_FIMD is >>> selected), but an indirect expression of another dependency >>> (CONFIG_FB_S3C disables CONFIG_DRM_EXYNOS_FIMD, so disable >>> CONFIG_FB_S3C). >>> >>> Additionally, as the call comes from exynos_dp_core.c, which is built >>> by CONFIG_DRM_EXYNOS_DP (an explicitly user-selectable option), why >>> shouldn't the dependency be there? Ah, because the dependency on DP is >>> for (DECON || FIMD), but as DECON doesn't provide >>> fimd_dp_clock_enable(), it doesn't seem like it would compile if you >>> selected DECON and not FIMD. >>> >>> So, for me, the cleanest solution would be config DRM_EXYNOS_DP gains >>> a hard dependency on DRM_EXYNOS_FIMD, at least until it can be fixed >>> to compile without FIMD. >> >> Right, you correctly pointed current dependencies. Still it looks little >> hacky because EXYNOS_DP may work with FIMD or DECON. > > Are you sure? I have not seen any chipset having DECON and DP. In all > chipsets known to me DP is always accompanied by FIMD. I guess it can > change in the future, but for now hard dependency on FIMD seems to be OK > - it just reflects hardware design. > Of course this is just my humble opinion :) OK, so my next question would be: does DECON requires similar clock handling like FIMD on certain SoCs? In other words - does something like fimd_dp_clock_enable() have any sense in context of DECON? Best regards, Krzysztof
2015-05-04 20:34 GMT+09:00 Daniel Stone <daniel@fooishbar.org>: > Hi, > > On 4 May 2015 at 08:43, Inki Dae <inki.dae@samsung.com> wrote: >> On 2015? 05? 02? 13:08, Krzysztof Kozlowski wrote: >>> Selecting CONFIG_FB_S3C disables CONFIG_DRM_EXYNOS_FIMD leading to build >>> error: >> >> No, eDP has no any dependency of FIMD but DECON. Just add dependency >> code like below, >> >> config DRM_EXYNOS7_DECON >> bool "Exynos DRM DECON" >> - depends on DRM_EXYNOS >> + depends on DRM_EXYNOS && !FB_S3C > > But it does clearly and explicitly call fimd_dp_clock_enable from > exynos_dp_powero{n,ff}. So the dependency you're proposing seems > backwards: it's not an expression of the requirements of the current > code (that FIMD DP code be available, i.e. CONFIG_DRM_EXYNOS_FIMD is > selected), but an indirect expression of another dependency > (CONFIG_FB_S3C disables CONFIG_DRM_EXYNOS_FIMD, so disable > CONFIG_FB_S3C). > > Additionally, as the call comes from exynos_dp_core.c, which is built > by CONFIG_DRM_EXYNOS_DP (an explicitly user-selectable option), why > shouldn't the dependency be there? Ah, because the dependency on DP is > for (DECON || FIMD), but as DECON doesn't provide > fimd_dp_clock_enable(), it doesn't seem like it would compile if you Please know that the output data of DECON and FIMD can go to DSI or Panel directly not through eDP. This means that they - FIMD and eDP or DECON and eDP - have hardware dependency each other. DECON driver missed this - only one of Linux framebuffer and DRM KMS driver should be selected because Linux framebuffer and DRM KMS drivers control same hardware. So the reason that DECON driver should have dependency on eDP is not because dp core functions are called by FIMD driver. In addition, from hw point of view, eDP has depencency on DECON and FIMD because eDP cannot work itself without DECON or FIMD. Therefore, if DECON or FIMD is not selected, then eDP should also be disabled so that user cannot disable eDP with FIMD because FIMD driver calls eDP's functions directly. My missing point was that even though we add !S3C_FB to DECON Kconfig, we can disable eDP with FIMD so build error can still occur. The best way to resolve this issue would be to move the functions called by FIMD driver directly to Exynos specific KMS structure such as exynos crtc - eDP driver implements encoder which know crtc to FIMD or DECON. The directly call was not good way. Thanks, Inki Dae > selected DECON and not FIMD. > > So, for me, the cleanest solution would be config DRM_EXYNOS_DP gains > a hard dependency on DRM_EXYNOS_FIMD, at least until it can be fixed > to compile without FIMD. > > Cheers, > Daniel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
2015-05-04 21:43 GMT+09:00 Krzysztof Kozlowski <k.kozlowski.k@gmail.com>: > 2015-05-04 20:34 GMT+09:00 Daniel Stone <daniel@fooishbar.org>: >> Hi, >> >> On 4 May 2015 at 08:43, Inki Dae <inki.dae@samsung.com> wrote: >>> On 2015? 05? 02? 13:08, Krzysztof Kozlowski wrote: >>>> Selecting CONFIG_FB_S3C disables CONFIG_DRM_EXYNOS_FIMD leading to build >>>> error: >>> >>> No, eDP has no any dependency of FIMD but DECON. Just add dependency >>> code like below, >>> >>> config DRM_EXYNOS7_DECON >>> bool "Exynos DRM DECON" >>> - depends on DRM_EXYNOS >>> + depends on DRM_EXYNOS && !FB_S3C > > Actually my commit message was not detailed enough. The FB_S3C here > won't solve the issue because you may: > 1, disable FIMD and FB_S3C, > 2, enabke DECON and DP, > and it won't compile. > > Currently the FIMD must be enabled if DRM_EXYNOS_DP is enabled. > >> >> But it does clearly and explicitly call fimd_dp_clock_enable from >> exynos_dp_powero{n,ff}. So the dependency you're proposing seems >> backwards: it's not an expression of the requirements of the current >> code (that FIMD DP code be available, i.e. CONFIG_DRM_EXYNOS_FIMD is >> selected), but an indirect expression of another dependency >> (CONFIG_FB_S3C disables CONFIG_DRM_EXYNOS_FIMD, so disable >> CONFIG_FB_S3C). >> >> Additionally, as the call comes from exynos_dp_core.c, which is built >> by CONFIG_DRM_EXYNOS_DP (an explicitly user-selectable option), why >> shouldn't the dependency be there? Ah, because the dependency on DP is >> for (DECON || FIMD), but as DECON doesn't provide >> fimd_dp_clock_enable(), it doesn't seem like it would compile if you >> selected DECON and not FIMD. >> >> So, for me, the cleanest solution would be config DRM_EXYNOS_DP gains >> a hard dependency on DRM_EXYNOS_FIMD, at least until it can be fixed >> to compile without FIMD. > > Right, you correctly pointed current dependencies. Still it looks little > hacky because EXYNOS_DP may work with FIMD or DECON. It does not really > need FIMD. Using ifdefs in headers is not uncommon - many core > subsystems do this that way to provide stubs. > > Probably the cleanest way would be to provide by FIMD and DECON a common > interface for DP for such operation, something like: > struct exynos_drm_crtc { > struct drm_crtc base; > ... > void (*clock_enable)(struct exynos_drm_crtc *crtc, bool enable) > ); > > which, if non-NULL, will be called by exynos_dp_core.c: > static void exynos_dp_poweron(struct exynos_dp_device *dp) > { > ... > if (crtc->clock_enable) > crtc->clock_enable(crtc, true); > } > > What do you think? Good idea and you should have done so. the directly call was not good way. Anyway, with this, fimd driver can set fimd's callbacks to exynos_drm_crtc structure and eDP driver can call the callbacks through its encoder. But please, keep in mind that you should set the callbacks to Exynos specific structure - exynos_drm_crtc - because this hardware dependency is really specific to Exynos SoC. Thanks, Inki Dae > > Best regards, > Krzysztof > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.h b/drivers/gpu/drm/exynos/exynos_drm_fimd.h index b4fcaa568456..db67f3d9786d 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.h +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.h @@ -10,6 +10,10 @@ #ifndef _EXYNOS_DRM_FIMD_H_ #define _EXYNOS_DRM_FIMD_H_ +#ifdef CONFIG_DRM_EXYNOS_FIMD extern void fimd_dp_clock_enable(struct exynos_drm_crtc *crtc, bool enable); +#else +static inline void fimd_dp_clock_enable(struct exynos_drm_crtc *crtc, bool enable) {}; +#endif #endif /* _EXYNOS_DRM_FIMD_H_ */
Selecting CONFIG_FB_S3C disables CONFIG_DRM_EXYNOS_FIMD leading to build error: drivers/built-in.o: In function `exynos_dp_dpms': binder.c:(.text+0xd6a840): undefined reference to `fimd_dp_clock_enable' binder.c:(.text+0xd6ab54): undefined reference to `fimd_dp_clock_enable' Signed-off-by: Krzysztof Kozlowski <k.kozlowski.k@gmail.com> --- drivers/gpu/drm/exynos/exynos_drm_fimd.h | 4 ++++ 1 file changed, 4 insertions(+)