* Re: [PATCH v2] video: fbdev: controlfb: fix build for COMPILE_TEST=y && PPC_PMAC=y && PPC32=n
2020-04-29 10:48 ` [PATCH v2] video: fbdev: controlfb: fix build for COMPILE_TEST=y && PPC_PMAC=y && PPC32=n Bartlomiej Zolnierkiewicz
@ 2020-04-29 11:54 ` Sam Ravnborg
2020-04-29 12:06 ` Bartlomiej Zolnierkiewicz
2020-04-29 12:51 ` Christoph Hellwig
2020-04-29 19:28 ` Sam Ravnborg
2 siblings, 1 reply; 7+ messages in thread
From: Sam Ravnborg @ 2020-04-29 11:54 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz, Christoph Hellwig
Cc: Stephen Rothwell, linux-fbdev, kbuild test robot, Daniel Vetter,
linux-kernel, dri-devel
Hi Bartlomiej.
On Wed, Apr 29, 2020 at 12:48:24PM +0200, Bartlomiej Zolnierkiewicz wrote:
>
> powerpc allyesconfig fails like this:
>
> drivers/video/fbdev/controlfb.c: In function 'controlfb_mmap':
> drivers/video/fbdev/controlfb.c:756:23: error: implicit declaration of function 'pgprot_cached_wthru'; did you mean 'pgprot_cached'? [-Werror=implicit-function-declaration]
> 756 | vma->vm_page_prot = pgprot_cached_wthru(vma->vm_page_prot);
> | ^~~~~~~~~~~~~~~~~~~
> | pgprot_cached
> drivers/video/fbdev/controlfb.c:756:23: error: incompatible types when assigning to type 'pgprot_t' {aka 'struct <anonymous>'} from type 'int'
>
> Fix it by adding missing PPC32 dependency.
Is this really the right fix?
Short term I htink it is OK, but I think there should be a common way
to do the same for all archtectures so no conditional compilation is
needed. In other words the use of pgprot_cached_wthru looks like we
need a better abstraction.
Added Christoph to the mail as he has a good overview of the area.
Sam
>
> Fixes: a07a63b0e24d ("video: fbdev: controlfb: add COMPILE_TEST support")
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Reported-by: kbuild test robot <lkp@intel.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
> v2: fix implicit btext_update_display() function declaration error
>
> drivers/video/fbdev/controlfb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: b/drivers/video/fbdev/controlfb.c
> =================================> --- a/drivers/video/fbdev/controlfb.c
> +++ b/drivers/video/fbdev/controlfb.c
> @@ -55,7 +55,7 @@
> #include "macmodes.h"
> #include "controlfb.h"
>
> -#ifndef CONFIG_PPC_PMAC
> +#if !defined(CONFIG_PPC_PMAC) || !defined(CONFIG_PPC32)
> #define invalid_vram_cache(addr)
> #undef in_8
> #undef out_8
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] video: fbdev: controlfb: fix build for COMPILE_TEST=y && PPC_PMAC=y && PPC32=n
2020-04-29 11:54 ` Sam Ravnborg
@ 2020-04-29 12:06 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 7+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-04-29 12:06 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Stephen Rothwell, linux-fbdev, kbuild test robot, Daniel Vetter,
linux-kernel, dri-devel, Christoph Hellwig
On 4/29/20 1:54 PM, Sam Ravnborg wrote:
> Hi Bartlomiej.
>
> On Wed, Apr 29, 2020 at 12:48:24PM +0200, Bartlomiej Zolnierkiewicz wrote:
>>
>> powerpc allyesconfig fails like this:
>>
>> drivers/video/fbdev/controlfb.c: In function 'controlfb_mmap':
>> drivers/video/fbdev/controlfb.c:756:23: error: implicit declaration of function 'pgprot_cached_wthru'; did you mean 'pgprot_cached'? [-Werror=implicit-function-declaration]
>> 756 | vma->vm_page_prot = pgprot_cached_wthru(vma->vm_page_prot);
>> | ^~~~~~~~~~~~~~~~~~~
>> | pgprot_cached
>> drivers/video/fbdev/controlfb.c:756:23: error: incompatible types when assigning to type 'pgprot_t' {aka 'struct <anonymous>'} from type 'int'
>>
>> Fix it by adding missing PPC32 dependency.
>
> Is this really the right fix?
Yes, ifdef in the code should match driver dependencies in Kconfig:
config FB_CONTROL
bool "Apple \"control\" display support"
depends on (FB = y) && ((PPC_PMAC && PPC32) || COMPILE_TEST)
> Short term I htink it is OK, but I think there should be a common way
> to do the same for all archtectures so no conditional compilation is
> needed. In other words the use of pgprot_cached_wthru looks like we
> need a better abstraction.
This would be of course nice to have but won't be enough to remove
the ifdef in this particular driver.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
> Added Christoph to the mail as he has a good overview of the area.
>
> Sam
>
>
>>
>> Fixes: a07a63b0e24d ("video: fbdev: controlfb: add COMPILE_TEST support")
>> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
>> Reported-by: kbuild test robot <lkp@intel.com>
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>> ---
>> v2: fix implicit btext_update_display() function declaration error
>>
>> drivers/video/fbdev/controlfb.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Index: b/drivers/video/fbdev/controlfb.c
>> =================================>> --- a/drivers/video/fbdev/controlfb.c
>> +++ b/drivers/video/fbdev/controlfb.c
>> @@ -55,7 +55,7 @@
>> #include "macmodes.h"
>> #include "controlfb.h"
>>
>> -#ifndef CONFIG_PPC_PMAC
>> +#if !defined(CONFIG_PPC_PMAC) || !defined(CONFIG_PPC32)
>> #define invalid_vram_cache(addr)
>> #undef in_8
>> #undef out_8
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] video: fbdev: controlfb: fix build for COMPILE_TEST=y && PPC_PMAC=y && PPC32=n
2020-04-29 10:48 ` [PATCH v2] video: fbdev: controlfb: fix build for COMPILE_TEST=y && PPC_PMAC=y && PPC32=n Bartlomiej Zolnierkiewicz
2020-04-29 11:54 ` Sam Ravnborg
@ 2020-04-29 12:51 ` Christoph Hellwig
2020-04-29 13:14 ` Bartlomiej Zolnierkiewicz
2020-04-29 13:39 ` Sam Ravnborg
2020-04-29 19:28 ` Sam Ravnborg
2 siblings, 2 replies; 7+ messages in thread
From: Christoph Hellwig @ 2020-04-29 12:51 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Stephen Rothwell, kbuild test robot, Sam Ravnborg, Daniel Vetter,
linux-fbdev, dri-devel, linux-kernel
Why do we even bother allocing the driver to compile for !ppc32
given that it clearly needs ppc-specific infrastructure? The whole
idea of needing magic stubs for the COMPILE_TEST case seems rather
counterproduction.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] video: fbdev: controlfb: fix build for COMPILE_TEST=y && PPC_PMAC=y && PPC32=n
2020-04-29 12:51 ` Christoph Hellwig
@ 2020-04-29 13:14 ` Bartlomiej Zolnierkiewicz
2020-04-29 13:39 ` Sam Ravnborg
1 sibling, 0 replies; 7+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2020-04-29 13:14 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Stephen Rothwell, linux-fbdev, kbuild test robot, Daniel Vetter,
linux-kernel, dri-devel, Sam Ravnborg
On 4/29/20 2:51 PM, Christoph Hellwig wrote:
> Why do we even bother allocing the driver to compile for !ppc32
> given that it clearly needs ppc-specific infrastructure? The whole
> idea of needing magic stubs for the COMPILE_TEST case seems rather
> counterproduction.
Not a perfect solution but at the cost of 2 ifdefs it allows controlfb
driver to be compile tested on any arch.
Being able to compile test fbdev device drivers is really useful for
me and saves me a lot of time when doing fbdev maintainer duties.
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] video: fbdev: controlfb: fix build for COMPILE_TEST=y && PPC_PMAC=y && PPC32=n
2020-04-29 12:51 ` Christoph Hellwig
2020-04-29 13:14 ` Bartlomiej Zolnierkiewicz
@ 2020-04-29 13:39 ` Sam Ravnborg
1 sibling, 0 replies; 7+ messages in thread
From: Sam Ravnborg @ 2020-04-29 13:39 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Stephen Rothwell, linux-fbdev, kbuild test robot,
Bartlomiej Zolnierkiewicz, Daniel Vetter, linux-kernel, dri-devel
Hi Christoph
On Wed, Apr 29, 2020 at 05:51:01AM -0700, Christoph Hellwig wrote:
> Why do we even bother allocing the driver to compile for !ppc32
> given that it clearly needs ppc-specific infrastructure? The whole
> idea of needing magic stubs for the COMPILE_TEST case seems rather
> counterproduction.
All the usual good arguments.
If this driver only builds for 32bit powerpc then we will seldom
build it and every time we do some refactoring we risk introducing
build errros in this driver that is triggered much later.
So a few hacks are preferred to actually make it build.
But hacks should not paper over missing abstractions
in the general ioremap handlign and such.
I recall someone said the other day that drm folks had a tendency to
workaround rather than fixing this.
So this is "drm folks" reaching out and asking if this is a
case where we have a workaround and need a fix?
I will - after some testing - apply the fix from Bartlomiej.
But would like to know if this is a workarond or a fix.
Dropping COMPILE_TEST is not an option as explained above.
Sam
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] video: fbdev: controlfb: fix build for COMPILE_TEST=y && PPC_PMAC=y && PPC32=n
2020-04-29 10:48 ` [PATCH v2] video: fbdev: controlfb: fix build for COMPILE_TEST=y && PPC_PMAC=y && PPC32=n Bartlomiej Zolnierkiewicz
2020-04-29 11:54 ` Sam Ravnborg
2020-04-29 12:51 ` Christoph Hellwig
@ 2020-04-29 19:28 ` Sam Ravnborg
2 siblings, 0 replies; 7+ messages in thread
From: Sam Ravnborg @ 2020-04-29 19:28 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Stephen Rothwell, linux-fbdev, kbuild test robot, Daniel Vetter,
linux-kernel, dri-devel
Hi Bartlomiej
On Wed, Apr 29, 2020 at 12:48:24PM +0200, Bartlomiej Zolnierkiewicz wrote:
>
> powerpc allyesconfig fails like this:
>
> drivers/video/fbdev/controlfb.c: In function 'controlfb_mmap':
> drivers/video/fbdev/controlfb.c:756:23: error: implicit declaration of function 'pgprot_cached_wthru'; did you mean 'pgprot_cached'? [-Werror=implicit-function-declaration]
> 756 | vma->vm_page_prot = pgprot_cached_wthru(vma->vm_page_prot);
> | ^~~~~~~~~~~~~~~~~~~
> | pgprot_cached
> drivers/video/fbdev/controlfb.c:756:23: error: incompatible types when assigning to type 'pgprot_t' {aka 'struct <anonymous>'} from type 'int'
>
> Fix it by adding missing PPC32 dependency.
>
> Fixes: a07a63b0e24d ("video: fbdev: controlfb: add COMPILE_TEST support")
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Reported-by: kbuild test robot <lkp@intel.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
I've updated my build script to include a few popwerpc 32 bit configs.
Patch looked correct, and my builds was OK.
So applied to drm-misc-next and pushed out.
If my prodding of Christoph will result in anything only time will tell.
Sam
> ---
> v2: fix implicit btext_update_display() function declaration error
>
> drivers/video/fbdev/controlfb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: b/drivers/video/fbdev/controlfb.c
> =================================> --- a/drivers/video/fbdev/controlfb.c
> +++ b/drivers/video/fbdev/controlfb.c
> @@ -55,7 +55,7 @@
> #include "macmodes.h"
> #include "controlfb.h"
>
> -#ifndef CONFIG_PPC_PMAC
> +#if !defined(CONFIG_PPC_PMAC) || !defined(CONFIG_PPC32)
> #define invalid_vram_cache(addr)
> #undef in_8
> #undef out_8
^ permalink raw reply [flat|nested] 7+ messages in thread