[PATCH] drm/exynos: Fix build breakage on !DRM_EXYNOS_FIMD

[PATCH] drm/exynos: Fix build breakage on !DRM_EXYNOS_FIMD

[Krzysztof Kozlowski]

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(+)

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_ */
-- 
2.1.4

[PATCH] drm/exynos: Fix build breakage on !DRM_EXYNOS_FIMD

[Inki Dae]

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_ */
> 

[PATCH] drm/exynos: Fix build breakage on !DRM_EXYNOS_FIMD

[Daniel Stone]

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

[PATCH] drm/exynos: Fix build breakage on !DRM_EXYNOS_FIMD

[Krzysztof Kozlowski]

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

[PATCH] drm/exynos: Fix build breakage on !DRM_EXYNOS_FIMD

[Andrzej Hajda]

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
> 

[PATCH] drm/exynos: Fix build breakage on !DRM_EXYNOS_FIMD

[Krzysztof Kozlowski]

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

[PATCH] drm/exynos: Fix build breakage on !DRM_EXYNOS_FIMD

[Inki Dae]

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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] drm/exynos: Fix build breakage on !DRM_EXYNOS_FIMD

[Inki Dae]

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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel