[v2,1/2] drm/cma-helper: Add multi buffer support for cma fbdev

Commit Message

Maxime Ripard Feb. 2, 2017, 10:31 a.m. UTC

From: Xinliang Liu <[e-mail]>

This patch add a config to support to create multi buffer for cma fbdev.
Such as double buffer and triple buffer.

Cma fbdev is convient to add a legency fbdev. And still many Android
devices use fbdev now and at least double buffer is needed for these
Android devices, so that a buffer flip can be operated. It will need
some time for Android device vendors to abondon legency fbdev. So multi
buffer for fbdev is needed.

Signed-off-by: Xinliang Liu <[e-mail]>
[[e-mail]: Picking patch from
                     https://lkml.org/lkml/2015/9/14/188]
Signed-off-by: Stefan Christ <[e-mail]>
Signed-off-by: Maxime Ripard <[e-mail]>
---
 drivers/gpu/drm/Kconfig             | 8 ++++++++
 drivers/gpu/drm/drm_fb_cma_helper.c | 8 +++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

Comments

Daniel Vetter Feb. 9, 2017, 5:04 p.m. UTC | #1

On Thu, Feb 02, 2017 at 11:31:56AM +0100, Maxime Ripard wrote:
> From: Xinliang Liu <[e-mail]>
> 
> This patch add a config to support to create multi buffer for cma fbdev.
> Such as double buffer and triple buffer.
> 
> Cma fbdev is convient to add a legency fbdev. And still many Android
> devices use fbdev now and at least double buffer is needed for these
> Android devices, so that a buffer flip can be operated. It will need
> some time for Android device vendors to abondon legency fbdev. So multi
> buffer for fbdev is needed.
> 
> Signed-off-by: Xinliang Liu <[e-mail]>
> [[e-mail]: Picking patch from
>                      https://lkml.org/lkml/2015/9/14/188]
> Signed-off-by: Stefan Christ <[e-mail]>
> Signed-off-by: Maxime Ripard <[e-mail]>
> ---
>  drivers/gpu/drm/Kconfig             | 8 ++++++++
>  drivers/gpu/drm/drm_fb_cma_helper.c | 8 +++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index ebfe8404c25f..2ca9bb26a4e4 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -121,6 +121,14 @@ config DRM_KMS_CMA_HELPER
>  	help
>  	  Choose this if you need the KMS CMA helper functions
>  
> +config DRM_CMA_FBDEV_BUFFER_NUM
> +	int "Cma Fbdev Buffer Number"
> +	depends on DRM_KMS_CMA_HELPER
> +	default 1
> +	help
> +	  Defines the buffer number of cma fbdev.  Default is one buffer.
> +	  For double buffer please set to 2 and 3 for triple buffer.
> +
>  source "drivers/gpu/drm/i2c/Kconfig"
>  
>  source "drivers/gpu/drm/arm/Kconfig"
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> index 81b3558302b5..e3f8b9e720a0 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -411,6 +411,12 @@ static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
>  	kfree(fbi->fbops);
>  }
>  
> +static int fbdev_num_buffers = CONFIG_DRM_CMA_FBDEV_BUFFER_NUM;
> +module_param(fbdev_num_buffers, int, 0444);
> +MODULE_PARM_DESC(fbdev_num_buffers,
> +		 "Number of frame buffers to allocate [default="
> +		 __MODULE_STRING(CONFIG_DRM_CMA_FBDEV_BUFFER_NUM) "]");

Pure bikshed: Should this be an overallocation %? 200 for
double-buffering, 100 as the default?

Slightly less bikesheddy: Can't we do this in the core helpers somehow?
I'd be nice if this is not cma specific. If it's not possible, I'd at
least move the symbol to drm_fb_helper.c, and add some kernel-doc around
it. That allows any other non-cma driver to at least implement support for
this. That also has the benefit of featuring it more prominently, in our
docs.

Besides these bikesheds/question, looks all reasonable to me. If you can
get some more acks would be great, but will merge anyway.
-Daniel

> +
>  /*
>   * For use in a (struct drm_fb_helper_funcs *)->fb_probe callback function that
>   * needs custom struct drm_framebuffer_funcs, like dirty() for deferred_io use.
> @@ -437,7 +443,7 @@ int drm_fbdev_cma_create_with_funcs(struct drm_fb_helper *helper,
>  	bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
>  
>  	mode_cmd.width = sizes->surface_width;
> -	mode_cmd.height = sizes->surface_height;
> +	mode_cmd.height = sizes->surface_height * fbdev_num_buffers;
>  	mode_cmd.pitches[0] = sizes->surface_width * bytes_per_pixel;
>  	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
>  		sizes->surface_depth);
> -- 
> git-series 0.8.11
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Maxime Ripard Feb. 10, 2017, 3:27 p.m. UTC | #2

Hi Daniel,

On Thu, Feb 09, 2017 at 06:04:38PM +0100, Daniel Vetter wrote:
> On Thu, Feb 02, 2017 at 11:31:56AM +0100, Maxime Ripard wrote:
> > From: Xinliang Liu <[e-mail]>
> > 
> > This patch add a config to support to create multi buffer for cma fbdev.
> > Such as double buffer and triple buffer.
> > 
> > Cma fbdev is convient to add a legency fbdev. And still many Android
> > devices use fbdev now and at least double buffer is needed for these
> > Android devices, so that a buffer flip can be operated. It will need
> > some time for Android device vendors to abondon legency fbdev. So multi
> > buffer for fbdev is needed.
> > 
> > Signed-off-by: Xinliang Liu <[e-mail]>
> > [[e-mail]: Picking patch from
> >                      https://lkml.org/lkml/2015/9/14/188]
> > Signed-off-by: Stefan Christ <[e-mail]>
> > Signed-off-by: Maxime Ripard <[e-mail]>
> > ---
> >  drivers/gpu/drm/Kconfig             | 8 ++++++++
> >  drivers/gpu/drm/drm_fb_cma_helper.c | 8 +++++++-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index ebfe8404c25f..2ca9bb26a4e4 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -121,6 +121,14 @@ config DRM_KMS_CMA_HELPER
> >  	help
> >  	  Choose this if you need the KMS CMA helper functions
> >  
> > +config DRM_CMA_FBDEV_BUFFER_NUM
> > +	int "Cma Fbdev Buffer Number"
> > +	depends on DRM_KMS_CMA_HELPER
> > +	default 1
> > +	help
> > +	  Defines the buffer number of cma fbdev.  Default is one buffer.
> > +	  For double buffer please set to 2 and 3 for triple buffer.
> > +
> >  source "drivers/gpu/drm/i2c/Kconfig"
> >  
> >  source "drivers/gpu/drm/arm/Kconfig"
> > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
> > index 81b3558302b5..e3f8b9e720a0 100644
> > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > @@ -411,6 +411,12 @@ static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
> >  	kfree(fbi->fbops);
> >  }
> >  
> > +static int fbdev_num_buffers = CONFIG_DRM_CMA_FBDEV_BUFFER_NUM;
> > +module_param(fbdev_num_buffers, int, 0444);
> > +MODULE_PARM_DESC(fbdev_num_buffers,
> > +		 "Number of frame buffers to allocate [default="
> > +		 __MODULE_STRING(CONFIG_DRM_CMA_FBDEV_BUFFER_NUM) "]");
> 
> Pure bikshed: Should this be an overallocation %? 200 for
> double-buffering, 100 as the default?

I'm not against it, but would it make sense to allocate something like
120% of a buffer?

> Slightly less bikesheddy: Can't we do this in the core helpers somehow?

I guess that would move it to drm_fb_helper_single_fb_probe then? It
makes sense.

> I'd be nice if this is not cma specific. If it's not possible, I'd at
> least move the symbol to drm_fb_helper.c, and add some kernel-doc around
> it. That allows any other non-cma driver to at least implement support for
> this. That also has the benefit of featuring it more prominently, in our
> docs.

I'll move it and add the kerneldoc.

Thanks!
Maxime

Laurent Pinchart Feb. 12, 2017, 12:28 p.m. UTC | #3

Hi Maxime,

Thank you for the patch.

On Thursday 02 Feb 2017 11:31:56 Maxime Ripard wrote:
> From: Xinliang Liu <[e-mail]>
> 
> This patch add a config to support to create multi buffer for cma fbdev.
> Such as double buffer and triple buffer.
> 
> Cma fbdev is convient to add a legency fbdev. And still many Android
> devices use fbdev now and at least double buffer is needed for these
> Android devices, so that a buffer flip can be operated. It will need
> some time for Android device vendors to abondon legency fbdev. So multi
> buffer for fbdev is needed.

How exactly do we expect Android to move away from fbdev if we add features to 
the fbdev compat layer ? I'd much rather make it clear to them that fbdev is a 
thing from the past and that they'd better migrate now.

> Signed-off-by: Xinliang Liu <[e-mail]>
> [[e-mail]: Picking patch from
>                      https://lkml.org/lkml/2015/9/14/188]
> Signed-off-by: Stefan Christ <[e-mail]>
> Signed-off-by: Maxime Ripard <[e-mail]>
> ---
>  drivers/gpu/drm/Kconfig             | 8 ++++++++
>  drivers/gpu/drm/drm_fb_cma_helper.c | 8 +++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index ebfe8404c25f..2ca9bb26a4e4 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -121,6 +121,14 @@ config DRM_KMS_CMA_HELPER
>  	help
>  	  Choose this if you need the KMS CMA helper functions
> 
> +config DRM_CMA_FBDEV_BUFFER_NUM
> +	int "Cma Fbdev Buffer Number"
> +	depends on DRM_KMS_CMA_HELPER
> +	default 1
> +	help
> +	  Defines the buffer number of cma fbdev.  Default is one buffer.
> +	  For double buffer please set to 2 and 3 for triple buffer.
> +
>  source "drivers/gpu/drm/i2c/Kconfig"
> 
>  source "drivers/gpu/drm/arm/Kconfig"
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> b/drivers/gpu/drm/drm_fb_cma_helper.c index 81b3558302b5..e3f8b9e720a0
> 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -411,6 +411,12 @@ static void drm_fbdev_cma_defio_fini(struct fb_info
> *fbi) kfree(fbi->fbops);
>  }
> 
> +static int fbdev_num_buffers = CONFIG_DRM_CMA_FBDEV_BUFFER_NUM;
> +module_param(fbdev_num_buffers, int, 0444);
> +MODULE_PARM_DESC(fbdev_num_buffers,
> +		 "Number of frame buffers to allocate [default="
> +		 __MODULE_STRING(CONFIG_DRM_CMA_FBDEV_BUFFER_NUM) "]");
> +
>  /*
>   * For use in a (struct drm_fb_helper_funcs *)->fb_probe callback function
> that * needs custom struct drm_framebuffer_funcs, like dirty() for
> deferred_io use. @@ -437,7 +443,7 @@ int
> drm_fbdev_cma_create_with_funcs(struct drm_fb_helper *helper,
> bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
> 
>  	mode_cmd.width = sizes->surface_width;
> -	mode_cmd.height = sizes->surface_height;
> +	mode_cmd.height = sizes->surface_height * fbdev_num_buffers;
>  	mode_cmd.pitches[0] = sizes->surface_width * bytes_per_pixel;
>  	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
>  		sizes->surface_depth);

Maxime Ripard Feb. 13, 2017, 10:54 a.m. UTC | #4

Hi Laurent,

On Sun, Feb 12, 2017 at 02:28:11PM +0200, Laurent Pinchart wrote:
> Hi Maxime,
> 
> Thank you for the patch.
> 
> On Thursday 02 Feb 2017 11:31:56 Maxime Ripard wrote:
> > From: Xinliang Liu <[e-mail]>
> > 
> > This patch add a config to support to create multi buffer for cma fbdev.
> > Such as double buffer and triple buffer.
> > 
> > Cma fbdev is convient to add a legency fbdev. And still many Android
> > devices use fbdev now and at least double buffer is needed for these
> > Android devices, so that a buffer flip can be operated. It will need
> > some time for Android device vendors to abondon legency fbdev. So multi
> > buffer for fbdev is needed.
> 
> How exactly do we expect Android to move away from fbdev if we add features to 
> the fbdev compat layer ? I'd much rather make it clear to them that fbdev is a 
> thing from the past and that they'd better migrate now.

If your point is that merging this patch will slow down the Android
move away from fbdev, I disagree with that (obviously).

I don't care at all about Android on my platform of choice, but don't
see how that merging this patch will change anything.

Let's be honest, Android trees typically have thousands of patches on
top of mainline. Do you think a simple, 15 LoC, patch will make any
difference to vendors? If they want to stay on fbdev and have that
feature, they'll just merge this patch, done.

However, what I do see is that three different people/organisations
have now expressed interest in that feature, on three different
SoCs. If that patch needed a significant rework of the fbdev layer,
then yes, I might agree that it's not worth it. But in this case, it's
pretty trivial.

The only people you're "punishing" here with that kind of concern are
the people who actually play fair and want not to have any patches and
everything upstream.

I guess a much better strategy would be to provide an incentive to
moving to KMS. And I truely think there's one already, so it's just a
matter of time before people switch over. Fbdev emulation or not.

Ma

Daniel Stone Feb. 13, 2017, 11:20 a.m. UTC | #5

Hi Maxime,

On 13 February 2017 at 10:54, Maxime Ripard
<[e-mail]> wrote:
> On Sun, Feb 12, 2017 at 02:28:11PM +0200, Laurent Pinchart wrote:
>> On Thursday 02 Feb 2017 11:31:56 Maxime Ripard wrote:
>> > This patch add a config to support to create multi buffer for cma fbdev.
>> > Such as double buffer and triple buffer.
>> >
>> > Cma fbdev is convient to add a legency fbdev. And still many Android
>> > devices use fbdev now and at least double buffer is needed for these
>> > Android devices, so that a buffer flip can be operated. It will need
>> > some time for Android device vendors to abondon legency fbdev. So multi
>> > buffer for fbdev is needed.
>>
>> How exactly do we expect Android to move away from fbdev if we add features to
>> the fbdev compat layer ? I'd much rather make it clear to them that fbdev is a
>> thing from the past and that they'd better migrate now.
>
> If your point is that merging this patch will slow down the Android
> move away from fbdev, I disagree with that (obviously).
>
> I don't care at all about Android on my platform of choice, but don't
> see how that merging this patch will change anything.
>
> Let's be honest, Android trees typically have thousands of patches on
> top of mainline. Do you think a simple, 15 LoC, patch will make any
> difference to vendors? If they want to stay on fbdev and have that
> feature, they'll just merge this patch, done.

So, in that case, why not just let them do that? They'd already have
to add patches to use this, surely; we don't have anything in mainline
kernels which allows people to actually use this larger allocation.
Apart from software mmap() and using panning to do flips, but I'm
taking it as a given that people shipping Android on their devices
aren't using software rendering.

> However, what I do see is that three different people/organisations
> have now expressed interest in that feature, on three different
> SoCs. If that patch needed a significant rework of the fbdev layer,
> then yes, I might agree that it's not worth it. But in this case, it's
> pretty trivial.
>
> The only people you're "punishing" here with that kind of concern are
> the people who actually play fair and want not to have any patches and
> everything upstream.

I would hazard a guess that most users of this have out-of-tree GPU drivers.

> I guess a much better strategy would be to provide an incentive to
> moving to KMS. And I truely think there's one already, so it's just a
> matter of time before people switch over. Fbdev emulation or not.

The concern makes sense, but on the other hand, fbdev is deprecated:
no new drivers, and no new features.

Cheers,
Daniel

Daniel Vetter Feb. 14, 2017, 8:09 p.m. UTC | #6

On Mon, Feb 13, 2017 at 11:20:51AM +0000, Daniel Stone wrote:
> Hi Maxime,
> 
> On 13 February 2017 at 10:54, Maxime Ripard
> <[e-mail]> wrote:
> > On Sun, Feb 12, 2017 at 02:28:11PM +0200, Laurent Pinchart wrote:
> >> On Thursday 02 Feb 2017 11:31:56 Maxime Ripard wrote:
> >> > This patch add a config to support to create multi buffer for cma fbdev.
> >> > Such as double buffer and triple buffer.
> >> >
> >> > Cma fbdev is convient to add a legency fbdev. And still many Android
> >> > devices use fbdev now and at least double buffer is needed for these
> >> > Android devices, so that a buffer flip can be operated. It will need
> >> > some time for Android device vendors to abondon legency fbdev. So multi
> >> > buffer for fbdev is needed.
> >>
> >> How exactly do we expect Android to move away from fbdev if we add features to
> >> the fbdev compat layer ? I'd much rather make it clear to them that fbdev is a
> >> thing from the past and that they'd better migrate now.
> >
> > If your point is that merging this patch will slow down the Android
> > move away from fbdev, I disagree with that (obviously).
> >
> > I don't care at all about Android on my platform of choice, but don't
> > see how that merging this patch will change anything.
> >
> > Let's be honest, Android trees typically have thousands of patches on
> > top of mainline. Do you think a simple, 15 LoC, patch will make any
> > difference to vendors? If they want to stay on fbdev and have that
> > feature, they'll just merge this patch, done.
> 
> So, in that case, why not just let them do that? They'd already have
> to add patches to use this, surely; we don't have anything in mainline
> kernels which allows people to actually use this larger allocation.
> Apart from software mmap() and using panning to do flips, but I'm
> taking it as a given that people shipping Android on their devices
> aren't using software rendering.

I think we need to make a distinction between fbdev the subsystem in the
kernel, and fbdev the uabi:

- fbdev the subsystem is completely dead in upstream. I think we have full
  agreement on that.
- fbdev the uabi isn't, and if we can get more users from fbdev based
  drivers to kms/atomic drivers by adding fairly simple stuff like this,
  I'm all for it.

Which means: Yes, I fully plan to merge this, it makes sense. It even
_helps_ by making fbdev-the-subsystem even deader. Making live hard for
out-of-tree folks or folks with shit userspace doesn't make sense, at
least if the only benefit for us is that we'll feel pure about our
intentions :-)

Cheers, Daniel

Laurent Pinchart Feb. 14, 2017, 9:25 p.m. UTC | #7

Hi Daniel,

On Tuesday 14 Feb 2017 21:09:51 Daniel Vetter wrote:
> On Mon, Feb 13, 2017 at 11:20:51AM +0000, Daniel Stone wrote:
> > On 13 February 2017 at 10:54, Maxime Ripard wrote:
> >> On Sun, Feb 12, 2017 at 02:28:11PM +0200, Laurent Pinchart wrote:
> >>> On Thursday 02 Feb 2017 11:31:56 Maxime Ripard wrote:
> >>>> This patch add a config to support to create multi buffer for cma
> >>>> fbdev. Such as double buffer and triple buffer.
> >>>> 
> >>>> Cma fbdev is convient to add a legency fbdev. And still many Android
> >>>> devices use fbdev now and at least double buffer is needed for these
> >>>> Android devices, so that a buffer flip can be operated. It will need
> >>>> some time for Android device vendors to abondon legency fbdev. So
> >>>> multi buffer for fbdev is needed.
> >>> 
> >>> How exactly do we expect Android to move away from fbdev if we add
> >>> features to the fbdev compat layer ? I'd much rather make it clear to
> >>> them that fbdev is a thing from the past and that they'd better
> >>> migrate now.
> >> 
> >> If your point is that merging this patch will slow down the Android
> >> move away from fbdev, I disagree with that (obviously).
> >> 
> >> I don't care at all about Android on my platform of choice, but don't
> >> see how that merging this patch will change anything.
> >> 
> >> Let's be honest, Android trees typically have thousands of patches on
> >> top of mainline. Do you think a simple, 15 LoC, patch will make any
> >> difference to vendors? If they want to stay on fbdev and have that
> >> feature, they'll just merge this patch, done.
> > 
> > So, in that case, why not just let them do that? They'd already have
> > to add patches to use this, surely; we don't have anything in mainline
> > kernels which allows people to actually use this larger allocation.
> > Apart from software mmap() and using panning to do flips, but I'm
> > taking it as a given that people shipping Android on their devices
> > aren't using software rendering.
> 
> I think we need to make a distinction between fbdev the subsystem in the
> kernel, and fbdev the uabi:
> 
> - fbdev the subsystem is completely dead in upstream. I think we have full
>   agreement on that.
> - fbdev the uabi isn't, and if we can get more users from fbdev based
>   drivers to kms/atomic drivers by adding fairly simple stuff like this,
>   I'm all for it.

The real question, in my opinion, is how to get more users for the DRM/KMS 
userspace API, to help killing the fbdev API. What's the incentive for 
userspace to migrate if we tell them that we're going to support the fbdev API 
forever, and will even go through the trouble of extending the supported 
feature set ? I have a customer who wouldn't have migrated their userspace to 
DRM/KMS if these two patches had been merged a few years ago. I'd rather 
*reduce* the supported feature set over time until we can finally switch fbdev 
off.

> Which means: Yes, I fully plan to merge this, it makes sense. It even
> _helps_ by making fbdev-the-subsystem even deader. Making live hard for
> out-of-tree folks or folks with shit userspace doesn't make sense, at
> least if the only benefit for us is that we'll feel pure about our
> intentions :-)

Maxime Ripard Feb. 15, 2017, 12:38 p.m. UTC | #8

Hi,

On Mon, Feb 13, 2017 at 11:20:51AM +0000, Daniel Stone wrote:
> Hi Maxime,
> 
> On 13 February 2017 at 10:54, Maxime Ripard
> <[e-mail]> wrote:
> > On Sun, Feb 12, 2017 at 02:28:11PM +0200, Laurent Pinchart wrote:
> >> On Thursday 02 Feb 2017 11:31:56 Maxime Ripard wrote:
> >> > This patch add a config to support to create multi buffer for cma fbdev.
> >> > Such as double buffer and triple buffer.
> >> >
> >> > Cma fbdev is convient to add a legency fbdev. And still many Android
> >> > devices use fbdev now and at least double buffer is needed for these
> >> > Android devices, so that a buffer flip can be operated. It will need
> >> > some time for Android device vendors to abondon legency fbdev. So multi
> >> > buffer for fbdev is needed.
> >>
> >> How exactly do we expect Android to move away from fbdev if we add features to
> >> the fbdev compat layer ? I'd much rather make it clear to them that fbdev is a
> >> thing from the past and that they'd better migrate now.
> >
> > If your point is that merging this patch will slow down the Android
> > move away from fbdev, I disagree with that (obviously).
> >
> > I don't care at all about Android on my platform of choice, but don't
> > see how that merging this patch will change anything.
> >
> > Let's be honest, Android trees typically have thousands of patches on
> > top of mainline. Do you think a simple, 15 LoC, patch will make any
> > difference to vendors? If they want to stay on fbdev and have that
> > feature, they'll just merge this patch, done.
> 
> So, in that case, why not just let them do that? They'd already have
> to add patches to use this, surely; we don't have anything in mainline
> kernels which allows people to actually use this larger allocation.
> Apart from software mmap() and using panning to do flips, but I'm
> taking it as a given that people shipping Android on their devices
> aren't using software rendering.

My point was that you're not doing it more difficult for people not
willing to contribute upstream, you're just making it more difficult
for people who want to contribute.

The whole argument to engage vendors upstream is that we sell them
that eventually they will be able to just use whatever kernel release
is on kernel.org or in their distro of choice.

If those people depend on a feature that is entirely rejected
upstream, then they'll have to carry that patch in their tree,
creating a BSP in the process. And that reduces greatly the strength
of the "you should contribute" argument, making them less involved.

> > However, what I do see is that three different people/organisations
> > have now expressed interest in that feature, on three different
> > SoCs. If that patch needed a significant rework of the fbdev layer,
> > then yes, I might agree that it's not worth it. But in this case, it's
> > pretty trivial.
> >
> > The only people you're "punishing" here with that kind of concern are
> > the people who actually play fair and want not to have any patches and
> > everything upstream.
> 
> I would hazard a guess that most users of this have out-of-tree GPU
> drivers.

Out of tree GPU drivers, that can be distributed separately from the
kernel, just like any out of tree module can. This doesn't require any
kernel patches at all.

Maxime

Maxime Ripard Feb. 15, 2017, 12:51 p.m. UTC | #9

Hi,

On Tue, Feb 14, 2017 at 11:25:08PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Tuesday 14 Feb 2017 21:09:51 Daniel Vetter wrote:
> > On Mon, Feb 13, 2017 at 11:20:51AM +0000, Daniel Stone wrote:
> > > On 13 February 2017 at 10:54, Maxime Ripard wrote:
> > >> On Sun, Feb 12, 2017 at 02:28:11PM +0200, Laurent Pinchart wrote:
> > >>> On Thursday 02 Feb 2017 11:31:56 Maxime Ripard wrote:
> > >>>> This patch add a config to support to create multi buffer for cma
> > >>>> fbdev. Such as double buffer and triple buffer.
> > >>>> 
> > >>>> Cma fbdev is convient to add a legency fbdev. And still many Android
> > >>>> devices use fbdev now and at least double buffer is needed for these
> > >>>> Android devices, so that a buffer flip can be operated. It will need
> > >>>> some time for Android device vendors to abondon legency fbdev. So
> > >>>> multi buffer for fbdev is needed.
> > >>> 
> > >>> How exactly do we expect Android to move away from fbdev if we add
> > >>> features to the fbdev compat layer ? I'd much rather make it clear to
> > >>> them that fbdev is a thing from the past and that they'd better
> > >>> migrate now.
> > >> 
> > >> If your point is that merging this patch will slow down the Android
> > >> move away from fbdev, I disagree with that (obviously).
> > >> 
> > >> I don't care at all about Android on my platform of choice, but don't
> > >> see how that merging this patch will change anything.
> > >> 
> > >> Let's be honest, Android trees typically have thousands of patches on
> > >> top of mainline. Do you think a simple, 15 LoC, patch will make any
> > >> difference to vendors? If they want to stay on fbdev and have that
> > >> feature, they'll just merge this patch, done.
> > > 
> > > So, in that case, why not just let them do that? They'd already have
> > > to add patches to use this, surely; we don't have anything in mainline
> > > kernels which allows people to actually use this larger allocation.
> > > Apart from software mmap() and using panning to do flips, but I'm
> > > taking it as a given that people shipping Android on their devices
> > > aren't using software rendering.
> > 
> > I think we need to make a distinction between fbdev the subsystem in the
> > kernel, and fbdev the uabi:
> > 
> > - fbdev the subsystem is completely dead in upstream. I think we have full
> >   agreement on that.
> > - fbdev the uabi isn't, and if we can get more users from fbdev based
> >   drivers to kms/atomic drivers by adding fairly simple stuff like this,
> >   I'm all for it.
>
> The real question, in my opinion, is how to get more users for the DRM/KMS 
> userspace API, to help killing the fbdev API. What's the incentive for 
> userspace to migrate if we tell them that we're going to support the fbdev API 
> forever, and will even go through the trouble of extending the supported 
> feature set ? I have a customer who wouldn't have migrated their userspace to 
> DRM/KMS if these two patches had been merged a few years ago.

If those patches are not in, then I can see three ways to support old
/ deficient userspaces:
  1) Carry those patches out of tree
  2) Write an fbdev driver for the display engine
  3) Rewrite the userspace components

While 3. would arguably be the best option, this isn't always one,
unfortunately.

And as a community, I think 1 and 2 are not very good for us. 1. will
drive away vendors from our community, undermining the effort we've
been doing for a few years. And 2 will result in a driver we really
don't want to merge (so useless), and even if it would out of tree,
that would make it one less system / board / SoC *with* DRM/KMS APIs,
reducing the interest of switching for application developpers.

If we really want to make people switch to DRM / KMS, we have to make
it ubiquitous. And if we want to make it ubiquitous, we really want to
have a transition period where people will have both APIs, supported
in a decent enough way.

And then, that's a win for everyone, because in the process you get
fbdev (booo!) and KMS (yay!), allowing for people to switch over, and
eventually kill the emulation entirely. But it's far too early for
that. I mean, we don't even have an fbv replacement yet...

Maxime

Laurent Pinchart Feb. 17, 2017, 11:23 a.m. UTC | #10

Hi Maxime,

On Wednesday 15 Feb 2017 13:38:44 Maxime Ripard wrote:
> On Mon, Feb 13, 2017 at 11:20:51AM +0000, Daniel Stone wrote:
> > On 13 February 2017 at 10:54, Maxime Ripard wrote:
> >> On Sun, Feb 12, 2017 at 02:28:11PM +0200, Laurent Pinchart wrote:
> >>> On Thursday 02 Feb 2017 11:31:56 Maxime Ripard wrote:
> >>>> This patch add a config to support to create multi buffer for cma
> >>>> fbdev. Such as double buffer and triple buffer.
> >>>> 
> >>>> Cma fbdev is convient to add a legency fbdev. And still many Android
> >>>> devices use fbdev now and at least double buffer is needed for these
> >>>> Android devices, so that a buffer flip can be operated. It will need
> >>>> some time for Android device vendors to abondon legency fbdev. So
> >>>> multi buffer for fbdev is needed.
> >>> 
> >>> How exactly do we expect Android to move away from fbdev if we add
> >>> features to the fbdev compat layer ? I'd much rather make it clear to
> >>> them that fbdev is a thing from the past and that they'd better
> >>> migrate now.
> >> 
> >> If your point is that merging this patch will slow down the Android
> >> move away from fbdev, I disagree with that (obviously).
> >> 
> >> I don't care at all about Android on my platform of choice, but don't
> >> see how that merging this patch will change anything.
> >> 
> >> Let's be honest, Android trees typically have thousands of patches on
> >> top of mainline. Do you think a simple, 15 LoC, patch will make any
> >> difference to vendors? If they want to stay on fbdev and have that
> >> feature, they'll just merge this patch, done.
> > 
> > So, in that case, why not just let them do that? They'd already have
> > to add patches to use this, surely; we don't have anything in mainline
> > kernels which allows people to actually use this larger allocation.
> > Apart from software mmap() and using panning to do flips, but I'm
> > taking it as a given that people shipping Android on their devices
> > aren't using software rendering.
> 
> My point was that you're not doing it more difficult for people not
> willing to contribute upstream, you're just making it more difficult
> for people who want to contribute.
> 
> The whole argument to engage vendors upstream is that we sell them
> that eventually they will be able to just use whatever kernel release
> is on kernel.org or in their distro of choice.
> 
> If those people depend on a feature that is entirely rejected
> upstream, then they'll have to carry that patch in their tree,
> creating a BSP in the process. And that reduces greatly the strength
> of the "you should contribute" argument, making them less involved.

No, they should not carry an out-of-tree patch, they should not use that 
feature in the first place. fbdev is a dead-end, Linux has clearly decided to 
move to DRM/KMS. Any vendor who wants to keep using fbdev is shooting 
themselves in the foot, as the more they depend on fbdev, the more painful it 
will be to switch later when they will have no choice. Switching sooner than 
later is the best decision, and I'd argue that by making it easier to stay on 
fbdev we would actually hurt those vendors in the longer term.

> >> However, what I do see is that three different people/organisations
> >> have now expressed interest in that feature, on three different
> >> SoCs. If that patch needed a significant rework of the fbdev layer,
> >> then yes, I might agree that it's not worth it. But in this case, it's
> >> pretty trivial.
> >> 
> >> The only people you're "punishing" here with that kind of concern are
> >> the people who actually play fair and want not to have any patches and
> >> everything upstream.
> > 
> > I would hazard a guess that most users of this have out-of-tree GPU
> > drivers.
> 
> Out of tree GPU drivers, that can be distributed separately from the
> kernel, just like any out of tree module can. This doesn't require any
> kernel patches at all.

That might be true in some cases, but usually those GPU drivers require heavy 
patching of at least the display controller driver.

Laurent Pinchart Feb. 17, 2017, 11:30 a.m. UTC | #11

Hi Maxime,

On Wednesday 15 Feb 2017 13:51:29 Maxime Ripard wrote:
> On Tue, Feb 14, 2017 at 11:25:08PM +0200, Laurent Pinchart wrote:
> > On Tuesday 14 Feb 2017 21:09:51 Daniel Vetter wrote:
> >> On Mon, Feb 13, 2017 at 11:20:51AM +0000, Daniel Stone wrote:
> >>> On 13 February 2017 at 10:54, Maxime Ripard wrote:
> >>>> On Sun, Feb 12, 2017 at 02:28:11PM +0200, Laurent Pinchart wrote:
> >>>>> On Thursday 02 Feb 2017 11:31:56 Maxime Ripard wrote:
> >>>>>> This patch add a config to support to create multi buffer for cma
> >>>>>> fbdev. Such as double buffer and triple buffer.
> >>>>>> 
> >>>>>> Cma fbdev is convient to add a legency fbdev. And still many
> >>>>>> Android devices use fbdev now and at least double buffer is needed
> >>>>>> for these Android devices, so that a buffer flip can be operated. It
> >>>>>> will need some time for Android device vendors to abondon legency
> >>>>>> fbdev. So multi buffer for fbdev is needed.
> >>>>> 
> >>>>> How exactly do we expect Android to move away from fbdev if we add
> >>>>> features to the fbdev compat layer ? I'd much rather make it clear
> >>>>> to them that fbdev is a thing from the past and that they'd better
> >>>>> migrate now.
> >>>> 
> >>>> If your point is that merging this patch will slow down the Android
> >>>> move away from fbdev, I disagree with that (obviously).
> >>>> 
> >>>> I don't care at all about Android on my platform of choice, but don't
> >>>> see how that merging this patch will change anything.
> >>>> 
> >>>> Let's be honest, Android trees typically have thousands of patches on
> >>>> top of mainline. Do you think a simple, 15 LoC, patch will make any
> >>>> difference to vendors? If they want to stay on fbdev and have that
> >>>> feature, they'll just merge this patch, done.
> >>> 
> >>> So, in that case, why not just let them do that? They'd already have
> >>> to add patches to use this, surely; we don't have anything in mainline
> >>> kernels which allows people to actually use this larger allocation.
> >>> Apart from software mmap() and using panning to do flips, but I'm
> >>> taking it as a given that people shipping Android on their devices
> >>> aren't using software rendering.
> >> 
> >> I think we need to make a distinction between fbdev the subsystem in the
> >> kernel, and fbdev the uabi:
> >> 
> >> - fbdev the subsystem is completely dead in upstream. I think we have
> >>   full agreement on that.
> >> 
> >> - fbdev the uabi isn't, and if we can get more users from fbdev based
> >>   drivers to kms/atomic drivers by adding fairly simple stuff like this,
> >>   I'm all for it.
> > 
> > The real question, in my opinion, is how to get more users for the DRM/KMS
> > userspace API, to help killing the fbdev API. What's the incentive for
> > userspace to migrate if we tell them that we're going to support the fbdev
> > API forever, and will even go through the trouble of extending the
> > supported feature set ? I have a customer who wouldn't have migrated
> > their userspace to DRM/KMS if these two patches had been merged a few
> > years ago.
> 
> If those patches are not in, then I can see three ways to support old
> / deficient userspaces:
>   1) Carry those patches out of tree
>   2) Write an fbdev driver for the display engine
>   3) Rewrite the userspace components
> 
> While 3. would arguably be the best option, this isn't always one,
> unfortunately.

I agree that it's not a solution that can be deployed overnight, but it's 
clearly what we all (as in kernel community and system vendors) need to head 
towards.

> And as a community, I think 1 and 2 are not very good for us. 1. will
> drive away vendors from our community, undermining the effort we've
> been doing for a few years. And 2 will result in a driver we really
> don't want to merge (so useless), and even if it would out of tree,
> that would make it one less system / board / SoC *with* DRM/KMS APIs,
> reducing the interest of switching for application developpers.
> 
> If we really want to make people switch to DRM / KMS, we have to make
> it ubiquitous. And if we want to make it ubiquitous, we really want to
> have a transition period where people will have both APIs, supported
> in a decent enough way.

Haven't we had such a grace period already, until the fbdev subsystem stopped 
accepting new drivers ? It has hardly been an overnight switch.

> And then, that's a win for everyone, because in the process you get
> fbdev (booo!) and KMS (yay!), allowing for people to switch over, and
> eventually kill the emulation entirely. But it's far too early for
> that. I mean, we don't even have an fbv replacement yet...

We're talking about http://s-tech.elsat.net.pl/fbv/, whose latest release 
dates from 2011 ? :-)

https://github.com/tomba/kmsxx/blob/master/utils/kmsview.cpp

It won't be hard to add support for BMP, GIF, JPG or PNG.

Patch

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index ebfe8404c25f..2ca9bb26a4e4 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -121,6 +121,14 @@  config DRM_KMS_CMA_HELPER
 	help
 	  Choose this if you need the KMS CMA helper functions
 
+config DRM_CMA_FBDEV_BUFFER_NUM
+	int "Cma Fbdev Buffer Number"
+	depends on DRM_KMS_CMA_HELPER
+	default 1
+	help
+	  Defines the buffer number of cma fbdev.  Default is one buffer.
+	  For double buffer please set to 2 and 3 for triple buffer.
+
 source "drivers/gpu/drm/i2c/Kconfig"
 
 source "drivers/gpu/drm/arm/Kconfig"
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c
index 81b3558302b5..e3f8b9e720a0 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -411,6 +411,12 @@  static void drm_fbdev_cma_defio_fini(struct fb_info *fbi)
 	kfree(fbi->fbops);
 }
 
+static int fbdev_num_buffers = CONFIG_DRM_CMA_FBDEV_BUFFER_NUM;
+module_param(fbdev_num_buffers, int, 0444);
+MODULE_PARM_DESC(fbdev_num_buffers,
+		 "Number of frame buffers to allocate [default="
+		 __MODULE_STRING(CONFIG_DRM_CMA_FBDEV_BUFFER_NUM) "]");
+
 /*
  * For use in a (struct drm_fb_helper_funcs *)->fb_probe callback function that
  * needs custom struct drm_framebuffer_funcs, like dirty() for deferred_io use.
@@ -437,7 +443,7 @@  int drm_fbdev_cma_create_with_funcs(struct drm_fb_helper *helper,
 	bytes_per_pixel = DIV_ROUND_UP(sizes->surface_bpp, 8);
 
 	mode_cmd.width = sizes->surface_width;
-	mode_cmd.height = sizes->surface_height;
+	mode_cmd.height = sizes->surface_height * fbdev_num_buffers;
 	mode_cmd.pitches[0] = sizes->surface_width * bytes_per_pixel;
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
 		sizes->surface_depth);