[PATCH] drm/atmel-hlcdc: add command line option to specify preferred depth

[PATCH] drm/atmel-hlcdc: add command line option to specify preferred depth

[Peter Rosin]

I have an sama5d31-based system with 64MB of memory and a 1920x1080
LVDS display wired for 16-bpp. When I enable legacy fbdev support,
the contiguous memory allocator invariably fails with the order-11
allocation for a 1920x1080@24-bpp buffer (~6MB). But this HW can never
make any good use of RGB888, so that is a wasted attempt anyway that
would also waste precious memory should it succeed.

Sure, I could rewrite user-space to go directly to KMS etc, and that
makes the (attempted) order-11 allocation go away, replacing it with
one order-10 allocation per application restart for a 1920x1080@16-bpp
buffer (<4MB). But after a few restarts, order-10 allocations start to
fail as well, which is only to be expected AFAIU.

So, I'd rather not change user-space (which was originally written
to target a smaller display) so that I at the same time get the
benefit of an early pre-allocated fbdev frame-buffer that can be
reused over and over. But to do that I need to tell the driver that
16-bpp is the preferred depth. Add a module parameter to do just that.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

I found some inspiration regarding naming and implementation here:
https://patchwork.kernel.org/patch/9848631/

I have found no feedback on that patch though, which makes me wonder if
I'm perhaps barking up the wronig tree?

Cheers,
Peter

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index c1ea5c36b006..f0148627c221 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -29,6 +29,11 @@
 
 #define ATMEL_HLCDC_LAYER_IRQS_OFFSET		8
 
+static int atmel_hlcdc_preferred_depth __read_mostly;
+
+MODULE_PARM_DESC(preferreddepth, "Set preferred bpp");
+module_param_named(preferreddepth, atmel_hlcdc_preferred_depth, int, 0400);
+
 static const struct atmel_hlcdc_layer_desc atmel_hlcdc_at91sam9n12_layers[] = {
 	{
 		.name = "base",
@@ -590,6 +595,7 @@ static int atmel_hlcdc_dc_modeset_init(struct drm_device *dev)
 	dev->mode_config.min_height = dc->desc->min_height;
 	dev->mode_config.max_width = dc->desc->max_width;
 	dev->mode_config.max_height = dc->desc->max_height;
+	dev->mode_config.preferred_depth = 24;
 	dev->mode_config.funcs = &mode_config_funcs;
 
 	return 0;
@@ -658,7 +664,7 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
 
 	platform_set_drvdata(pdev, dev);
 
-	drm_fb_cma_fbdev_init(dev, 24, 0);
+	drm_fb_cma_fbdev_init(dev, atmel_hlcdc_preferred_depth, 0);
 
 	drm_kms_helper_poll_init(dev);
 
@@ -756,6 +762,16 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
 	struct drm_device *ddev;
 	int ret;
 
+	switch (atmel_hlcdc_preferred_depth) {
+	case 0: /* driver default */
+	case 8:
+	case 16:
+	case 24:
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	ddev = drm_dev_alloc(&atmel_hlcdc_dc_driver, &pdev->dev);
 	if (IS_ERR(ddev))
 		return PTR_ERR(ddev);
-- 
2.11.0

Re: [PATCH] drm/atmel-hlcdc: add command line option to specify preferred depth

[Boris Brezillon]

Hi Peter,

On Mon, 26 Mar 2018 09:35:02 +0200
Peter Rosin <peda@axentia.se> wrote:

> I have an sama5d31-based system with 64MB of memory and a 1920x1080
> LVDS display wired for 16-bpp. When I enable legacy fbdev support,
> the contiguous memory allocator invariably fails with the order-11
> allocation for a 1920x1080@24-bpp buffer (~6MB). But this HW can never
> make any good use of RGB888, so that is a wasted attempt anyway that
> would also waste precious memory should it succeed.
> 
> Sure, I could rewrite user-space to go directly to KMS etc, and that
> makes the (attempted) order-11 allocation go away, replacing it with
> one order-10 allocation per application restart for a 1920x1080@16-bpp
> buffer (<4MB). But after a few restarts, order-10 allocations start to
> fail as well, which is only to be expected AFAIU.
> 
> So, I'd rather not change user-space (which was originally written
> to target a smaller display) so that I at the same time get the
> benefit of an early pre-allocated fbdev frame-buffer that can be
> reused over and over. But to do that I need to tell the driver that
> 16-bpp is the preferred depth. Add a module parameter to do just that.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> I found some inspiration regarding naming and implementation here:
> https://patchwork.kernel.org/patch/9848631/
> 
> I have found no feedback on that patch though, which makes me wonder if
> I'm perhaps barking up the wronig tree?

Hm, isn't that something you can already overload with the video=
parameter?

	video=<output>:<resolution>[-<bpp>]

AFAIR, <bpp> encodes the color depth, so what is the benefit of adding
this new property to overload the default depth?

Maybe I'm wrong and the default depth param is actually useful, but in
this case we should probably make it generic since other drivers seems
to need it too, and we might want to attach it to a specific display
engine instance.

Thanks,

Boris

> 
> Cheers,
> Peter
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index c1ea5c36b006..f0148627c221 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -29,6 +29,11 @@
>  
>  #define ATMEL_HLCDC_LAYER_IRQS_OFFSET		8
>  
> +static int atmel_hlcdc_preferred_depth __read_mostly;
> +
> +MODULE_PARM_DESC(preferreddepth, "Set preferred bpp");
> +module_param_named(preferreddepth, atmel_hlcdc_preferred_depth, int, 0400);
> +
>  static const struct atmel_hlcdc_layer_desc atmel_hlcdc_at91sam9n12_layers[] = {
>  	{
>  		.name = "base",
> @@ -590,6 +595,7 @@ static int atmel_hlcdc_dc_modeset_init(struct drm_device *dev)
>  	dev->mode_config.min_height = dc->desc->min_height;
>  	dev->mode_config.max_width = dc->desc->max_width;
>  	dev->mode_config.max_height = dc->desc->max_height;
> +	dev->mode_config.preferred_depth = 24;
>  	dev->mode_config.funcs = &mode_config_funcs;
>  
>  	return 0;
> @@ -658,7 +664,7 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
>  
>  	platform_set_drvdata(pdev, dev);
>  
> -	drm_fb_cma_fbdev_init(dev, 24, 0);
> +	drm_fb_cma_fbdev_init(dev, atmel_hlcdc_preferred_depth, 0);
>  
>  	drm_kms_helper_poll_init(dev);
>  
> @@ -756,6 +762,16 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
>  	struct drm_device *ddev;
>  	int ret;
>  
> +	switch (atmel_hlcdc_preferred_depth) {
> +	case 0: /* driver default */
> +	case 8:
> +	case 16:
> +	case 24:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
>  	ddev = drm_dev_alloc(&atmel_hlcdc_dc_driver, &pdev->dev);
>  	if (IS_ERR(ddev))
>  		return PTR_ERR(ddev);



-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] drm/atmel-hlcdc: add command line option to specify preferred depth

[Peter Rosin]

On 2018-03-28 09:34, Boris Brezillon wrote:
> Hi Peter,
> 
> On Mon, 26 Mar 2018 09:35:02 +0200
> Peter Rosin <peda@axentia.se> wrote:
> 
>> I have an sama5d31-based system with 64MB of memory and a 1920x1080
>> LVDS display wired for 16-bpp. When I enable legacy fbdev support,
>> the contiguous memory allocator invariably fails with the order-11
>> allocation for a 1920x1080@24-bpp buffer (~6MB). But this HW can never
>> make any good use of RGB888, so that is a wasted attempt anyway that
>> would also waste precious memory should it succeed.
>>
>> Sure, I could rewrite user-space to go directly to KMS etc, and that
>> makes the (attempted) order-11 allocation go away, replacing it with
>> one order-10 allocation per application restart for a 1920x1080@16-bpp
>> buffer (<4MB). But after a few restarts, order-10 allocations start to
>> fail as well, which is only to be expected AFAIU.
>>
>> So, I'd rather not change user-space (which was originally written
>> to target a smaller display) so that I at the same time get the
>> benefit of an early pre-allocated fbdev frame-buffer that can be
>> reused over and over. But to do that I need to tell the driver that
>> 16-bpp is the preferred depth. Add a module parameter to do just that.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> I found some inspiration regarding naming and implementation here:
>> https://patchwork.kernel.org/patch/9848631/
>>
>> I have found no feedback on that patch though, which makes me wonder if
>> I'm perhaps barking up the wronig tree?
> 
> Hm, isn't that something you can already overload with the video=
> parameter?
> 
> 	video=<output>:<resolution>[-<bpp>]

Heh, you are right...

> AFAIR, <bpp> encodes the color depth, so what is the benefit of adding
> this new property to overload the default depth?

...but hhhmmm, I think I will have to have to encode the display size
in the bootargs so I will have to use distinct barebox environments
depending on the panel, but that's no biggie.

Splendid, please throw away the patch!

Cheers,
Peter

> Maybe I'm wrong and the default depth param is actually useful, but in
> this case we should probably make it generic since other drivers seems
> to need it too, and we might want to attach it to a specific display
> engine instance.
> 
> Thanks,
> 
> Boris
> 
>>
>> Cheers,
>> Peter
>>
>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> index c1ea5c36b006..f0148627c221 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> @@ -29,6 +29,11 @@
>>  
>>  #define ATMEL_HLCDC_LAYER_IRQS_OFFSET		8
>>  
>> +static int atmel_hlcdc_preferred_depth __read_mostly;
>> +
>> +MODULE_PARM_DESC(preferreddepth, "Set preferred bpp");
>> +module_param_named(preferreddepth, atmel_hlcdc_preferred_depth, int, 0400);
>> +
>>  static const struct atmel_hlcdc_layer_desc atmel_hlcdc_at91sam9n12_layers[] = {
>>  	{
>>  		.name = "base",
>> @@ -590,6 +595,7 @@ static int atmel_hlcdc_dc_modeset_init(struct drm_device *dev)
>>  	dev->mode_config.min_height = dc->desc->min_height;
>>  	dev->mode_config.max_width = dc->desc->max_width;
>>  	dev->mode_config.max_height = dc->desc->max_height;
>> +	dev->mode_config.preferred_depth = 24;
>>  	dev->mode_config.funcs = &mode_config_funcs;
>>  
>>  	return 0;
>> @@ -658,7 +664,7 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
>>  
>>  	platform_set_drvdata(pdev, dev);
>>  
>> -	drm_fb_cma_fbdev_init(dev, 24, 0);
>> +	drm_fb_cma_fbdev_init(dev, atmel_hlcdc_preferred_depth, 0);
>>  
>>  	drm_kms_helper_poll_init(dev);
>>  
>> @@ -756,6 +762,16 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
>>  	struct drm_device *ddev;
>>  	int ret;
>>  
>> +	switch (atmel_hlcdc_preferred_depth) {
>> +	case 0: /* driver default */
>> +	case 8:
>> +	case 16:
>> +	case 24:
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>>  	ddev = drm_dev_alloc(&atmel_hlcdc_dc_driver, &pdev->dev);
>>  	if (IS_ERR(ddev))
>>  		return PTR_ERR(ddev);
> 
> 
> 

Re: [PATCH] drm/atmel-hlcdc: add command line option to specify preferred depth

[Daniel Vetter]

On Wed, Mar 28, 2018 at 09:34:54AM +0200, Boris Brezillon wrote:
> Hi Peter,
> 
> On Mon, 26 Mar 2018 09:35:02 +0200
> Peter Rosin <peda@axentia.se> wrote:
> 
> > I have an sama5d31-based system with 64MB of memory and a 1920x1080
> > LVDS display wired for 16-bpp. When I enable legacy fbdev support,
> > the contiguous memory allocator invariably fails with the order-11
> > allocation for a 1920x1080@24-bpp buffer (~6MB). But this HW can never
> > make any good use of RGB888, so that is a wasted attempt anyway that
> > would also waste precious memory should it succeed.
> > 
> > Sure, I could rewrite user-space to go directly to KMS etc, and that
> > makes the (attempted) order-11 allocation go away, replacing it with
> > one order-10 allocation per application restart for a 1920x1080@16-bpp
> > buffer (<4MB). But after a few restarts, order-10 allocations start to
> > fail as well, which is only to be expected AFAIU.
> > 
> > So, I'd rather not change user-space (which was originally written
> > to target a smaller display) so that I at the same time get the
> > benefit of an early pre-allocated fbdev frame-buffer that can be
> > reused over and over. But to do that I need to tell the driver that
> > 16-bpp is the preferred depth. Add a module parameter to do just that.
> > 
> > Signed-off-by: Peter Rosin <peda@axentia.se>
> > ---
> >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> > 
> > I found some inspiration regarding naming and implementation here:
> > https://patchwork.kernel.org/patch/9848631/
> > 
> > I have found no feedback on that patch though, which makes me wonder if
> > I'm perhaps barking up the wronig tree?
> 
> Hm, isn't that something you can already overload with the video=
> parameter?
> 
> 	video=<output>:<resolution>[-<bpp>]
> 
> AFAIR, <bpp> encodes the color depth, so what is the benefit of adding
> this new property to overload the default depth?
> 
> Maybe I'm wrong and the default depth param is actually useful, but in
> this case we should probably make it generic since other drivers seems
> to need it too, and we might want to attach it to a specific display
> engine instance.

I think for the drm's fbdev emulation we ignore the bpp ...

But yeah probably worth it to wire it up properly.
-Daniel
> 
> Thanks,
> 
> Boris
> 
> > 
> > Cheers,
> > Peter
> > 
> > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> > index c1ea5c36b006..f0148627c221 100644
> > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> > @@ -29,6 +29,11 @@
> >  
> >  #define ATMEL_HLCDC_LAYER_IRQS_OFFSET		8
> >  
> > +static int atmel_hlcdc_preferred_depth __read_mostly;
> > +
> > +MODULE_PARM_DESC(preferreddepth, "Set preferred bpp");
> > +module_param_named(preferreddepth, atmel_hlcdc_preferred_depth, int, 0400);
> > +
> >  static const struct atmel_hlcdc_layer_desc atmel_hlcdc_at91sam9n12_layers[] = {
> >  	{
> >  		.name = "base",
> > @@ -590,6 +595,7 @@ static int atmel_hlcdc_dc_modeset_init(struct drm_device *dev)
> >  	dev->mode_config.min_height = dc->desc->min_height;
> >  	dev->mode_config.max_width = dc->desc->max_width;
> >  	dev->mode_config.max_height = dc->desc->max_height;
> > +	dev->mode_config.preferred_depth = 24;
> >  	dev->mode_config.funcs = &mode_config_funcs;
> >  
> >  	return 0;
> > @@ -658,7 +664,7 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
> >  
> >  	platform_set_drvdata(pdev, dev);
> >  
> > -	drm_fb_cma_fbdev_init(dev, 24, 0);
> > +	drm_fb_cma_fbdev_init(dev, atmel_hlcdc_preferred_depth, 0);
> >  
> >  	drm_kms_helper_poll_init(dev);
> >  
> > @@ -756,6 +762,16 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
> >  	struct drm_device *ddev;
> >  	int ret;
> >  
> > +	switch (atmel_hlcdc_preferred_depth) {
> > +	case 0: /* driver default */
> > +	case 8:
> > +	case 16:
> > +	case 24:
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> >  	ddev = drm_dev_alloc(&atmel_hlcdc_dc_driver, &pdev->dev);
> >  	if (IS_ERR(ddev))
> >  		return PTR_ERR(ddev);
> 
> 
> 
> -- 
> Boris Brezillon, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/atmel-hlcdc: add command line option to specify preferred depth

[Boris Brezillon]

On Wed, 28 Mar 2018 14:22:36 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Mar 28, 2018 at 09:34:54AM +0200, Boris Brezillon wrote:
> > Hi Peter,
> > 
> > On Mon, 26 Mar 2018 09:35:02 +0200
> > Peter Rosin <peda@axentia.se> wrote:
> >   
> > > I have an sama5d31-based system with 64MB of memory and a 1920x1080
> > > LVDS display wired for 16-bpp. When I enable legacy fbdev support,
> > > the contiguous memory allocator invariably fails with the order-11
> > > allocation for a 1920x1080@24-bpp buffer (~6MB). But this HW can never
> > > make any good use of RGB888, so that is a wasted attempt anyway that
> > > would also waste precious memory should it succeed.
> > > 
> > > Sure, I could rewrite user-space to go directly to KMS etc, and that
> > > makes the (attempted) order-11 allocation go away, replacing it with
> > > one order-10 allocation per application restart for a 1920x1080@16-bpp
> > > buffer (<4MB). But after a few restarts, order-10 allocations start to
> > > fail as well, which is only to be expected AFAIU.
> > > 
> > > So, I'd rather not change user-space (which was originally written
> > > to target a smaller display) so that I at the same time get the
> > > benefit of an early pre-allocated fbdev frame-buffer that can be
> > > reused over and over. But to do that I need to tell the driver that
> > > 16-bpp is the preferred depth. Add a module parameter to do just that.
> > > 
> > > Signed-off-by: Peter Rosin <peda@axentia.se>
> > > ---
> > >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 18 +++++++++++++++++-
> > >  1 file changed, 17 insertions(+), 1 deletion(-)
> > > 
> > > I found some inspiration regarding naming and implementation here:
> > > https://patchwork.kernel.org/patch/9848631/
> > > 
> > > I have found no feedback on that patch though, which makes me wonder if
> > > I'm perhaps barking up the wronig tree?  
> > 
> > Hm, isn't that something you can already overload with the video=
> > parameter?
> > 
> > 	video=<output>:<resolution>[-<bpp>]
> > 
> > AFAIR, <bpp> encodes the color depth, so what is the benefit of adding
> > this new property to overload the default depth?
> > 
> > Maybe I'm wrong and the default depth param is actually useful, but in
> > this case we should probably make it generic since other drivers seems
> > to need it too, and we might want to attach it to a specific display
> > engine instance.  
> 
> I think for the drm's fbdev emulation we ignore the bpp ...

Nope, it's already parsed [1].

[1]https://elixir.bootlin.com/linux/v4.16-rc3/source/drivers/gpu/drm/drm_fb_helper.c#L1812
-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] drm/atmel-hlcdc: add command line option to specify preferred depth

[Daniel Vetter]

On Wed, Mar 28, 2018 at 02:25:12PM +0200, Boris Brezillon wrote:
> On Wed, 28 Mar 2018 14:22:36 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Wed, Mar 28, 2018 at 09:34:54AM +0200, Boris Brezillon wrote:
> > > Hi Peter,
> > > 
> > > On Mon, 26 Mar 2018 09:35:02 +0200
> > > Peter Rosin <peda@axentia.se> wrote:
> > >   
> > > > I have an sama5d31-based system with 64MB of memory and a 1920x1080
> > > > LVDS display wired for 16-bpp. When I enable legacy fbdev support,
> > > > the contiguous memory allocator invariably fails with the order-11
> > > > allocation for a 1920x1080@24-bpp buffer (~6MB). But this HW can never
> > > > make any good use of RGB888, so that is a wasted attempt anyway that
> > > > would also waste precious memory should it succeed.
> > > > 
> > > > Sure, I could rewrite user-space to go directly to KMS etc, and that
> > > > makes the (attempted) order-11 allocation go away, replacing it with
> > > > one order-10 allocation per application restart for a 1920x1080@16-bpp
> > > > buffer (<4MB). But after a few restarts, order-10 allocations start to
> > > > fail as well, which is only to be expected AFAIU.
> > > > 
> > > > So, I'd rather not change user-space (which was originally written
> > > > to target a smaller display) so that I at the same time get the
> > > > benefit of an early pre-allocated fbdev frame-buffer that can be
> > > > reused over and over. But to do that I need to tell the driver that
> > > > 16-bpp is the preferred depth. Add a module parameter to do just that.
> > > > 
> > > > Signed-off-by: Peter Rosin <peda@axentia.se>
> > > > ---
> > > >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 18 +++++++++++++++++-
> > > >  1 file changed, 17 insertions(+), 1 deletion(-)
> > > > 
> > > > I found some inspiration regarding naming and implementation here:
> > > > https://patchwork.kernel.org/patch/9848631/
> > > > 
> > > > I have found no feedback on that patch though, which makes me wonder if
> > > > I'm perhaps barking up the wronig tree?  
> > > 
> > > Hm, isn't that something you can already overload with the video=
> > > parameter?
> > > 
> > > 	video=<output>:<resolution>[-<bpp>]
> > > 
> > > AFAIR, <bpp> encodes the color depth, so what is the benefit of adding
> > > this new property to overload the default depth?
> > > 
> > > Maybe I'm wrong and the default depth param is actually useful, but in
> > > this case we should probably make it generic since other drivers seems
> > > to need it too, and we might want to attach it to a specific display
> > > engine instance.  
> > 
> > I think for the drm's fbdev emulation we ignore the bpp ...
> 
> Nope, it's already parsed [1].
> 
> [1]https://elixir.bootlin.com/linux/v4.16-rc3/source/drivers/gpu/drm/drm_fb_helper.c#L1812

Oh, totally missed this. I honestly wonder why we then have all the
various bpp module options in different drivers ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/atmel-hlcdc: add command line option to specify preferred depth

[Daniel Vetter]

On Wed, Mar 28, 2018 at 12:03:39PM +0200, Peter Rosin wrote:
> On 2018-03-28 09:34, Boris Brezillon wrote:
> > Hi Peter,
> > 
> > On Mon, 26 Mar 2018 09:35:02 +0200
> > Peter Rosin <peda@axentia.se> wrote:
> > 
> >> I have an sama5d31-based system with 64MB of memory and a 1920x1080
> >> LVDS display wired for 16-bpp. When I enable legacy fbdev support,
> >> the contiguous memory allocator invariably fails with the order-11
> >> allocation for a 1920x1080@24-bpp buffer (~6MB). But this HW can never
> >> make any good use of RGB888, so that is a wasted attempt anyway that
> >> would also waste precious memory should it succeed.
> >>
> >> Sure, I could rewrite user-space to go directly to KMS etc, and that
> >> makes the (attempted) order-11 allocation go away, replacing it with
> >> one order-10 allocation per application restart for a 1920x1080@16-bpp
> >> buffer (<4MB). But after a few restarts, order-10 allocations start to
> >> fail as well, which is only to be expected AFAIU.
> >>
> >> So, I'd rather not change user-space (which was originally written
> >> to target a smaller display) so that I at the same time get the
> >> benefit of an early pre-allocated fbdev frame-buffer that can be
> >> reused over and over. But to do that I need to tell the driver that
> >> 16-bpp is the preferred depth. Add a module parameter to do just that.
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 18 +++++++++++++++++-
> >>  1 file changed, 17 insertions(+), 1 deletion(-)
> >>
> >> I found some inspiration regarding naming and implementation here:
> >> https://patchwork.kernel.org/patch/9848631/
> >>
> >> I have found no feedback on that patch though, which makes me wonder if
> >> I'm perhaps barking up the wronig tree?
> > 
> > Hm, isn't that something you can already overload with the video=
> > parameter?
> > 
> > 	video=<output>:<resolution>[-<bpp>]
> 
> Heh, you are right...
> 
> > AFAIR, <bpp> encodes the color depth, so what is the benefit of adding
> > this new property to overload the default depth?
> 
> ...but hhhmmm, I think I will have to have to encode the display size
> in the bootargs so I will have to use distinct barebox environments
> depending on the panel, but that's no biggie.
> 
> Splendid, please throw away the patch!

I think we could fix the parsing to allow -bpp without the resolution ...
Not sure how to best do that though. Maybe state that 0x0-bpp means to not
change the resolution from the default?
-Daniel

> 
> Cheers,
> Peter
> 
> > Maybe I'm wrong and the default depth param is actually useful, but in
> > this case we should probably make it generic since other drivers seems
> > to need it too, and we might want to attach it to a specific display
> > engine instance.
> > 
> > Thanks,
> > 
> > Boris
> > 
> >>
> >> Cheers,
> >> Peter
> >>
> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> >> index c1ea5c36b006..f0148627c221 100644
> >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> >> @@ -29,6 +29,11 @@
> >>  
> >>  #define ATMEL_HLCDC_LAYER_IRQS_OFFSET		8
> >>  
> >> +static int atmel_hlcdc_preferred_depth __read_mostly;
> >> +
> >> +MODULE_PARM_DESC(preferreddepth, "Set preferred bpp");
> >> +module_param_named(preferreddepth, atmel_hlcdc_preferred_depth, int, 0400);
> >> +
> >>  static const struct atmel_hlcdc_layer_desc atmel_hlcdc_at91sam9n12_layers[] = {
> >>  	{
> >>  		.name = "base",
> >> @@ -590,6 +595,7 @@ static int atmel_hlcdc_dc_modeset_init(struct drm_device *dev)
> >>  	dev->mode_config.min_height = dc->desc->min_height;
> >>  	dev->mode_config.max_width = dc->desc->max_width;
> >>  	dev->mode_config.max_height = dc->desc->max_height;
> >> +	dev->mode_config.preferred_depth = 24;
> >>  	dev->mode_config.funcs = &mode_config_funcs;
> >>  
> >>  	return 0;
> >> @@ -658,7 +664,7 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
> >>  
> >>  	platform_set_drvdata(pdev, dev);
> >>  
> >> -	drm_fb_cma_fbdev_init(dev, 24, 0);
> >> +	drm_fb_cma_fbdev_init(dev, atmel_hlcdc_preferred_depth, 0);
> >>  
> >>  	drm_kms_helper_poll_init(dev);
> >>  
> >> @@ -756,6 +762,16 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
> >>  	struct drm_device *ddev;
> >>  	int ret;
> >>  
> >> +	switch (atmel_hlcdc_preferred_depth) {
> >> +	case 0: /* driver default */
> >> +	case 8:
> >> +	case 16:
> >> +	case 24:
> >> +		break;
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >> +
> >>  	ddev = drm_dev_alloc(&atmel_hlcdc_dc_driver, &pdev->dev);
> >>  	if (IS_ERR(ddev))
> >>  		return PTR_ERR(ddev);
> > 
> > 
> > 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/atmel-hlcdc: add command line option to specify preferred depth

[Peter Rosin]

On 2018-04-03 11:10, Daniel Vetter wrote:
> On Wed, Mar 28, 2018 at 12:03:39PM +0200, Peter Rosin wrote:
>> On 2018-03-28 09:34, Boris Brezillon wrote:
>>> Hi Peter,
>>>
>>> On Mon, 26 Mar 2018 09:35:02 +0200
>>> Peter Rosin <peda@axentia.se> wrote:
>>>
>>>> I have an sama5d31-based system with 64MB of memory and a 1920x1080
>>>> LVDS display wired for 16-bpp. When I enable legacy fbdev support,
>>>> the contiguous memory allocator invariably fails with the order-11
>>>> allocation for a 1920x1080@24-bpp buffer (~6MB). But this HW can never
>>>> make any good use of RGB888, so that is a wasted attempt anyway that
>>>> would also waste precious memory should it succeed.
>>>>
>>>> Sure, I could rewrite user-space to go directly to KMS etc, and that
>>>> makes the (attempted) order-11 allocation go away, replacing it with
>>>> one order-10 allocation per application restart for a 1920x1080@16-bpp
>>>> buffer (<4MB). But after a few restarts, order-10 allocations start to
>>>> fail as well, which is only to be expected AFAIU.
>>>>
>>>> So, I'd rather not change user-space (which was originally written
>>>> to target a smaller display) so that I at the same time get the
>>>> benefit of an early pre-allocated fbdev frame-buffer that can be
>>>> reused over and over. But to do that I need to tell the driver that
>>>> 16-bpp is the preferred depth. Add a module parameter to do just that.
>>>>
>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>> ---
>>>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 18 +++++++++++++++++-
>>>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>>>
>>>> I found some inspiration regarding naming and implementation here:
>>>> https://patchwork.kernel.org/patch/9848631/
>>>>
>>>> I have found no feedback on that patch though, which makes me wonder if
>>>> I'm perhaps barking up the wronig tree?
>>>
>>> Hm, isn't that something you can already overload with the video=
>>> parameter?
>>>
>>> 	video=<output>:<resolution>[-<bpp>]
>>
>> Heh, you are right...
>>
>>> AFAIR, <bpp> encodes the color depth, so what is the benefit of adding
>>> this new property to overload the default depth?
>>
>> ...but hhhmmm, I think I will have to have to encode the display size
>> in the bootargs so I will have to use distinct barebox environments
>> depending on the panel, but that's no biggie.
>>
>> Splendid, please throw away the patch!
> 
> I think we could fix the parsing to allow -bpp without the resolution ...
> Not sure how to best do that though. Maybe state that 0x0-bpp means to not
> change the resolution from the default?

That could be done, and I had a quick look but it's not immediately obvious
to me how that should be accomplished in a fairly localized manner. Since
this is no big deal, I will leave that as is and simply specify the screen
resolution along with the bpp in the bootargs.

But I do like the idea.

Cheers,
Peter