Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1693981 > unrolled thread

[PATCH] [RESEND] gpu: ipu-v3: add DRM dependency

Back to article view | Back to linux.kernel

Contents

  [PATCH] [RESEND] gpu: ipu-v3: add DRM dependency Arnd Bergmann <arnd@arndb.de> - 2017-07-21 23:00 +0200
    Re: [PATCH] [RESEND] gpu: ipu-v3: add DRM dependency Philipp Zabel <p.zabel@pengutronix.de> - 2017-07-24 10:10 +0200
      Re: [PATCH] [RESEND] gpu: ipu-v3: add DRM dependency Arnd Bergmann <arnd@arndb.de> - 2017-07-25 09:40 +0200
        Re: [PATCH] [RESEND] gpu: ipu-v3: add DRM dependency Philipp Zabel <p.zabel@pengutronix.de> - 2017-07-25 10:10 +0200
          Re: [PATCH] [RESEND] gpu: ipu-v3: add DRM dependency Arnd Bergmann <arnd@arndb.de> - 2017-07-25 13:40 +0200
            Re: [PATCH] [RESEND] gpu: ipu-v3: add DRM dependency Arnd Bergmann <arnd@arndb.de> - 2017-07-25 14:00 +0200
            Re: [PATCH] [RESEND] gpu: ipu-v3: add DRM dependency Philipp Zabel <p.zabel@pengutronix.de> - 2017-07-25 14:00 +0200

#1693981 — [PATCH] [RESEND] gpu: ipu-v3: add DRM dependency

The new PRE/PRG driver code causes a link failure when DRM is disabled:

drivers/gpu/ipu-v3/ipu-pre.o: In function `ipu_pre_configure':
ipu-pre.c:(.text.ipu_pre_configure+0x18): undefined reference to `drm_format_info'
drivers/gpu/ipu-v3/ipu-prg.o: In function `ipu_prg_format_supported':
ipu-prg.c:(.text.ipu_prg_format_supported+0x8): undefined reference to `drm_format_info'

Adding a Kconfig dependency on DRM means we don't run into this problem
any more. This might not be the best solution though, as the ipu seems
to have been intentionally kept separate from DRM in the past.

Fixes: ea9c260514c1 ("gpu: ipu-v3: add driver for Prefetch Resolve Gasket")
Link: https://patchwork.kernel.org/patch/9636665/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
Originally sent on March 21, but got no reply for it. Resending unchanged
as it is still needed in v4.13-rc1
---
 drivers/gpu/ipu-v3/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/ipu-v3/Kconfig b/drivers/gpu/ipu-v3/Kconfig
index 08766c6e7856..f8b200801408 100644
--- a/drivers/gpu/ipu-v3/Kconfig
+++ b/drivers/gpu/ipu-v3/Kconfig
@@ -1,6 +1,7 @@
 config IMX_IPUV3_CORE
 	tristate "IPUv3 core support"
 	depends on SOC_IMX5 || SOC_IMX6Q || ARCH_MULTIPLATFORM
+	depends on DRM
 	select GENERIC_IRQ_CHIP
 	help
 	  Choose this if you have a i.MX5/6 system and want to use the Image
-- 
2.9.0

[toc] | [next] | [standalone]

#1694526

Hi Arnd,

On Fri, 2017-07-21 at 22:56 +0200, Arnd Bergmann wrote:
> The new PRE/PRG driver code causes a link failure when DRM is disabled:
> 
> drivers/gpu/ipu-v3/ipu-pre.o: In function `ipu_pre_configure':
> ipu-pre.c:(.text.ipu_pre_configure+0x18): undefined reference to `drm_format_info'
> drivers/gpu/ipu-v3/ipu-prg.o: In function `ipu_prg_format_supported':
> ipu-prg.c:(.text.ipu_prg_format_supported+0x8): undefined reference to `drm_format_info'
> 
> Adding a Kconfig dependency on DRM means we don't run into this problem
> any more. This might not be the best solution though, as the ipu seems
> to have been intentionally kept separate from DRM in the past.
> 
> Fixes: ea9c260514c1 ("gpu: ipu-v3: add driver for Prefetch Resolve Gasket")
> Link: https://patchwork.kernel.org/patch/9636665/
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> Originally sent on March 21, but got no reply for it. Resending unchanged
> as it is still needed in v4.13-rc1

thank you for fix and for the resend. I have the original patch in my
inbox, I'm sorry I overlooked it.

I would still like to keep the ipu-v3 driver buildable without DRM
enabled. For now, I have applied your patch as is.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1695440

On Mon, Jul 24, 2017 at 10:05 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> On Fri, 2017-07-21 at 22:56 +0200, Arnd Bergmann wrote:
>> The new PRE/PRG driver code causes a link failure when DRM is disabled:
>>
>> drivers/gpu/ipu-v3/ipu-pre.o: In function `ipu_pre_configure':
>> ipu-pre.c:(.text.ipu_pre_configure+0x18): undefined reference to `drm_format_info'
>> drivers/gpu/ipu-v3/ipu-prg.o: In function ` ':
>> ipu-prg.c:(.text.ipu_prg_format_supported+0x8): undefined reference to `drm_format_info'
>>
>> Adding a Kconfig dependency on DRM means we don't run into this problem
>> any more. This might not be the best solution though, as the ipu seems
>> to have been intentionally kept separate from DRM in the past.
>>
>> Fixes: ea9c260514c1 ("gpu: ipu-v3: add driver for Prefetch Resolve Gasket")
>> Link: https://patchwork.kernel.org/patch/9636665/
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> Originally sent on March 21, but got no reply for it. Resending unchanged
>> as it is still needed in v4.13-rc1
>
> thank you for fix and for the resend. I have the original patch in my
> inbox, I'm sorry I overlooked it.
>
> I would still like to keep the ipu-v3 driver buildable without DRM
> enabled. For now, I have applied your patch as is.

Ok, thanks!

I'm pretty sure we can find a way to solve it so that you don't depend
on DRM, but we should discuss what that would look like.

Are you mainly interested in being able to build-test without DRM,
or do you actually want the ipu_pre_configure() and
ipu_prg_format_supported() functions to work correctly in that case?

If you only need build-testing, you could have a simple wrapper like

const struct drm_format_info *ipu_format_info(u32 format)
{
         static const struct drm_format_info invalid = {};

         if (!IS_REACHABLE(CONFIG_DRM))
                  return &invalid;

         return drm_format_info(format);
}

The more useful way to solve it would be more work, we could for
instance move (parts of) drivers/gpu/drm/drm_fourcc.c into
lib/fourcc.c and have it built whenever at least DRM or IPU_v3
are enabled.

         Arnd

[toc] | [prev] | [next] | [standalone]

#1695469

On Tue, 2017-07-25 at 09:33 +0200, Arnd Bergmann wrote:
> On Mon, Jul 24, 2017 at 10:05 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > On Fri, 2017-07-21 at 22:56 +0200, Arnd Bergmann wrote:
> >> The new PRE/PRG driver code causes a link failure when DRM is disabled:
> >>
> >> drivers/gpu/ipu-v3/ipu-pre.o: In function `ipu_pre_configure':
> >> ipu-pre.c:(.text.ipu_pre_configure+0x18): undefined reference to `drm_format_info'
> >> drivers/gpu/ipu-v3/ipu-prg.o: In function ` ':
> >> ipu-prg.c:(.text.ipu_prg_format_supported+0x8): undefined reference to `drm_format_info'
> >>
> >> Adding a Kconfig dependency on DRM means we don't run into this problem
> >> any more. This might not be the best solution though, as the ipu seems
> >> to have been intentionally kept separate from DRM in the past.
> >>
> >> Fixes: ea9c260514c1 ("gpu: ipu-v3: add driver for Prefetch Resolve Gasket")
> >> Link: https://patchwork.kernel.org/patch/9636665/
> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >> ---
> >> Originally sent on March 21, but got no reply for it. Resending unchanged
> >> as it is still needed in v4.13-rc1
> >
> > thank you for fix and for the resend. I have the original patch in my
> > inbox, I'm sorry I overlooked it.
> >
> > I would still like to keep the ipu-v3 driver buildable without DRM
> > enabled. For now, I have applied your patch as is.
> 
> Ok, thanks!
> 
> I'm pretty sure we can find a way to solve it so that you don't depend
> on DRM, but we should discuss what that would look like.
> 
> Are you mainly interested in being able to build-test without DRM,

Both that, and I'd like to keep the ability to disable DRM for devices
that only need the video capture parts of the IPUv3 to work. An example
would be a network connected camera without display or need for GPU
processing.

> or do you actually want the ipu_pre_configure() and
> ipu_prg_format_supported() functions to work correctly in that case?

In both cases the PRE and PRG don't have to be usable, as their only
function is pixel data prefetching for the display path.

> If you only need build-testing, you could have a simple wrapper like
> 
> const struct drm_format_info *ipu_format_info(u32 format)
> {
>          static const struct drm_format_info invalid = {};
> 
>          if (!IS_REACHABLE(CONFIG_DRM))
>                   return &invalid;
> 
>          return drm_format_info(format);
> }

That should work fine. Both ipu_prg_format_supported and
ipu_prg_channel_configure are only ever called by DRM code.

> The more useful way to solve it would be more work, we could for
> instance move (parts of) drivers/gpu/drm/drm_fourcc.c into
> lib/fourcc.c and have it built whenever at least DRM or IPU_v3
> are enabled.

I don't think this is necessary, at least for this case.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1695631

On Tue, Jul 25, 2017 at 10:03 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> On Tue, 2017-07-25 at 09:33 +0200, Arnd Bergmann wrote:
>> On Mon, Jul 24, 2017 at 10:05 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> > On Fri, 2017-07-21 at 22:56 +0200, Arnd Bergmann wrote:

>> If you only need build-testing, you could have a simple wrapper like
>>
>> const struct drm_format_info *ipu_format_info(u32 format)
>> {
>>          static const struct drm_format_info invalid = {};
>>
>>          if (!IS_REACHABLE(CONFIG_DRM))
>>                   return &invalid;
>>
>>          return drm_format_info(format);
>> }
>
> That should work fine. Both ipu_prg_format_supported and
> ipu_prg_channel_configure are only ever called by DRM code.

Ok, in that case, I think we should just make the compilation
of PRE and PRG conditional and leave them out when DRM
is not built-in (be aware of the case of DRM=m, IPU=y).

        Arnd

[toc] | [prev] | [next] | [standalone]

#1695652

On Tue, Jul 25, 2017 at 1:52 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> On Tue, 2017-07-25 at 13:35 +0200, Arnd Bergmann wrote:
>> On Tue, Jul 25, 2017 at 10:03 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> > On Tue, 2017-07-25 at 09:33 +0200, Arnd Bergmann wrote:
>> >> On Mon, Jul 24, 2017 at 10:05 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> >> > On Fri, 2017-07-21 at 22:56 +0200, Arnd Bergmann wrote:
>>
>> >> If you only need build-testing, you could have a simple wrapper like
>> >>
>> >> const struct drm_format_info *ipu_format_info(u32 format)
>> >> {
>> >>          static const struct drm_format_info invalid = {};
>> >>
>> >>          if (!IS_REACHABLE(CONFIG_DRM))
>> >>                   return &invalid;
>> >>
>> >>          return drm_format_info(format);
>> >> }
>> >
>> > That should work fine. Both ipu_prg_format_supported and
>> > ipu_prg_channel_configure are only ever called by DRM code.
>>
>> Ok, in that case, I think we should just make the compilation
>> of PRE and PRG conditional and leave them out when DRM
>> is not built-in (be aware of the case of DRM=m, IPU=y).
>
> Maybe we should just:
>
> config IMX_IPUV3_CORE
>         depends on DRM || !DRM # if DRM=m, this can't be 'y'

Yes, that would solve that case. I see now that the DRM=n
case is already handled by 30310c835f3e ("gpu: ipu-v3: don't
depend on DRM being enabled"), so that is probably sufficient.

I've added that change to my randconfig tree now and will give
it some more testing. If you don't hear back from me, please assume
that it works, and send that version upstream.

      Arnd

[toc] | [prev] | [next] | [standalone]

#1695653

On Tue, 2017-07-25 at 13:35 +0200, Arnd Bergmann wrote:
> On Tue, Jul 25, 2017 at 10:03 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > On Tue, 2017-07-25 at 09:33 +0200, Arnd Bergmann wrote:
> >> On Mon, Jul 24, 2017 at 10:05 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> >> > On Fri, 2017-07-21 at 22:56 +0200, Arnd Bergmann wrote:
> 
> >> If you only need build-testing, you could have a simple wrapper like
> >>
> >> const struct drm_format_info *ipu_format_info(u32 format)
> >> {
> >>          static const struct drm_format_info invalid = {};
> >>
> >>          if (!IS_REACHABLE(CONFIG_DRM))
> >>                   return &invalid;
> >>
> >>          return drm_format_info(format);
> >> }
> >
> > That should work fine. Both ipu_prg_format_supported and
> > ipu_prg_channel_configure are only ever called by DRM code.
> 
> Ok, in that case, I think we should just make the compilation
> of PRE and PRG conditional and leave them out when DRM
> is not built-in (be aware of the case of DRM=m, IPU=y).

Maybe we should just:

config IMX_IPUV3_CORE
	depends on DRM || !DRM # if DRM=m, this can't be 'y'

regards
Philipp

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web