From mboxrd@z Thu Jan 1 00:00:00 1970 From: boris.brezillon@bootlin.com (Boris Brezillon) Date: Wed, 28 Mar 2018 09:34:54 +0200 Subject: [PATCH] drm/atmel-hlcdc: add command line option to specify preferred depth In-Reply-To: <20180326073502.19259-1-peda@axentia.se> References: <20180326073502.19259-1-peda@axentia.se> Message-ID: <20180328093454.4149fa3b@bbrezillon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Peter, On Mon, 26 Mar 2018 09:35:02 +0200 Peter Rosin 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 at 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 at 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 > --- > 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=:[-] AFAIR, 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Subject: Re: [PATCH] drm/atmel-hlcdc: add command line option to specify preferred depth Date: Wed, 28 Mar 2018 09:34:54 +0200 Message-ID: <20180328093454.4149fa3b@bbrezillon> References: <20180326073502.19259-1-peda@axentia.se> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail.bootlin.com (mail.bootlin.com [62.4.15.54]) by gabe.freedesktop.org (Postfix) with ESMTP id AB22F89B20 for ; Wed, 28 Mar 2018 07:35:06 +0000 (UTC) In-Reply-To: <20180326073502.19259-1-peda@axentia.se> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Peter Rosin Cc: Egbert Eich , Boris Brezillon , Alexandre Belloni , David Airlie , Nicolas Ferre , Takashi Iwai , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org List-Id: dri-devel@lists.freedesktop.org SGkgUGV0ZXIsCgpPbiBNb24sIDI2IE1hciAyMDE4IDA5OjM1OjAyICswMjAwClBldGVyIFJvc2lu IDxwZWRhQGF4ZW50aWEuc2U+IHdyb3RlOgoKPiBJIGhhdmUgYW4gc2FtYTVkMzEtYmFzZWQgc3lz dGVtIHdpdGggNjRNQiBvZiBtZW1vcnkgYW5kIGEgMTkyMHgxMDgwCj4gTFZEUyBkaXNwbGF5IHdp cmVkIGZvciAxNi1icHAuIFdoZW4gSSBlbmFibGUgbGVnYWN5IGZiZGV2IHN1cHBvcnQsCj4gdGhl IGNvbnRpZ3VvdXMgbWVtb3J5IGFsbG9jYXRvciBpbnZhcmlhYmx5IGZhaWxzIHdpdGggdGhlIG9y ZGVyLTExCj4gYWxsb2NhdGlvbiBmb3IgYSAxOTIweDEwODBAMjQtYnBwIGJ1ZmZlciAofjZNQiku IEJ1dCB0aGlzIEhXIGNhbiBuZXZlcgo+IG1ha2UgYW55IGdvb2QgdXNlIG9mIFJHQjg4OCwgc28g dGhhdCBpcyBhIHdhc3RlZCBhdHRlbXB0IGFueXdheSB0aGF0Cj4gd291bGQgYWxzbyB3YXN0ZSBw cmVjaW91cyBtZW1vcnkgc2hvdWxkIGl0IHN1Y2NlZWQuCj4gCj4gU3VyZSwgSSBjb3VsZCByZXdy aXRlIHVzZXItc3BhY2UgdG8gZ28gZGlyZWN0bHkgdG8gS01TIGV0YywgYW5kIHRoYXQKPiBtYWtl cyB0aGUgKGF0dGVtcHRlZCkgb3JkZXItMTEgYWxsb2NhdGlvbiBnbyBhd2F5LCByZXBsYWNpbmcg aXQgd2l0aAo+IG9uZSBvcmRlci0xMCBhbGxvY2F0aW9uIHBlciBhcHBsaWNhdGlvbiByZXN0YXJ0 IGZvciBhIDE5MjB4MTA4MEAxNi1icHAKPiBidWZmZXIgKDw0TUIpLiBCdXQgYWZ0ZXIgYSBmZXcg cmVzdGFydHMsIG9yZGVyLTEwIGFsbG9jYXRpb25zIHN0YXJ0IHRvCj4gZmFpbCBhcyB3ZWxsLCB3 aGljaCBpcyBvbmx5IHRvIGJlIGV4cGVjdGVkIEFGQUlVLgo+IAo+IFNvLCBJJ2QgcmF0aGVyIG5v dCBjaGFuZ2UgdXNlci1zcGFjZSAod2hpY2ggd2FzIG9yaWdpbmFsbHkgd3JpdHRlbgo+IHRvIHRh cmdldCBhIHNtYWxsZXIgZGlzcGxheSkgc28gdGhhdCBJIGF0IHRoZSBzYW1lIHRpbWUgZ2V0IHRo ZQo+IGJlbmVmaXQgb2YgYW4gZWFybHkgcHJlLWFsbG9jYXRlZCBmYmRldiBmcmFtZS1idWZmZXIg dGhhdCBjYW4gYmUKPiByZXVzZWQgb3ZlciBhbmQgb3Zlci4gQnV0IHRvIGRvIHRoYXQgSSBuZWVk IHRvIHRlbGwgdGhlIGRyaXZlciB0aGF0Cj4gMTYtYnBwIGlzIHRoZSBwcmVmZXJyZWQgZGVwdGgu IEFkZCBhIG1vZHVsZSBwYXJhbWV0ZXIgdG8gZG8ganVzdCB0aGF0Lgo+IAo+IFNpZ25lZC1vZmYt Ynk6IFBldGVyIFJvc2luIDxwZWRhQGF4ZW50aWEuc2U+Cj4gLS0tCj4gIGRyaXZlcnMvZ3B1L2Ry bS9hdG1lbC1obGNkYy9hdG1lbF9obGNkY19kYy5jIHwgMTggKysrKysrKysrKysrKysrKystCj4g IDEgZmlsZSBjaGFuZ2VkLCAxNyBpbnNlcnRpb25zKCspLCAxIGRlbGV0aW9uKC0pCj4gCj4gSSBm b3VuZCBzb21lIGluc3BpcmF0aW9uIHJlZ2FyZGluZyBuYW1pbmcgYW5kIGltcGxlbWVudGF0aW9u IGhlcmU6Cj4gaHR0cHM6Ly9wYXRjaHdvcmsua2VybmVsLm9yZy9wYXRjaC85ODQ4NjMxLwo+IAo+ IEkgaGF2ZSBmb3VuZCBubyBmZWVkYmFjayBvbiB0aGF0IHBhdGNoIHRob3VnaCwgd2hpY2ggbWFr ZXMgbWUgd29uZGVyIGlmCj4gSSdtIHBlcmhhcHMgYmFya2luZyB1cCB0aGUgd3JvbmlnIHRyZWU/ CgpIbSwgaXNuJ3QgdGhhdCBzb21ldGhpbmcgeW91IGNhbiBhbHJlYWR5IG92ZXJsb2FkIHdpdGgg dGhlIHZpZGVvPQpwYXJhbWV0ZXI/CgoJdmlkZW89PG91dHB1dD46PHJlc29sdXRpb24+Wy08YnBw Pl0KCkFGQUlSLCA8YnBwPiBlbmNvZGVzIHRoZSBjb2xvciBkZXB0aCwgc28gd2hhdCBpcyB0aGUg YmVuZWZpdCBvZiBhZGRpbmcKdGhpcyBuZXcgcHJvcGVydHkgdG8gb3ZlcmxvYWQgdGhlIGRlZmF1 bHQgZGVwdGg/CgpNYXliZSBJJ20gd3JvbmcgYW5kIHRoZSBkZWZhdWx0IGRlcHRoIHBhcmFtIGlz IGFjdHVhbGx5IHVzZWZ1bCwgYnV0IGluCnRoaXMgY2FzZSB3ZSBzaG91bGQgcHJvYmFibHkgbWFr ZSBpdCBnZW5lcmljIHNpbmNlIG90aGVyIGRyaXZlcnMgc2VlbXMKdG8gbmVlZCBpdCB0b28sIGFu ZCB3ZSBtaWdodCB3YW50IHRvIGF0dGFjaCBpdCB0byBhIHNwZWNpZmljIGRpc3BsYXkKZW5naW5l IGluc3RhbmNlLgoKVGhhbmtzLAoKQm9yaXMKCj4gCj4gQ2hlZXJzLAo+IFBldGVyCj4gCj4gZGlm ZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9hdG1lbC1obGNkYy9hdG1lbF9obGNkY19kYy5jIGIv ZHJpdmVycy9ncHUvZHJtL2F0bWVsLWhsY2RjL2F0bWVsX2hsY2RjX2RjLmMKPiBpbmRleCBjMWVh NWMzNmIwMDYuLmYwMTQ4NjI3YzIyMSAxMDA2NDQKPiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0vYXRt ZWwtaGxjZGMvYXRtZWxfaGxjZGNfZGMuYwo+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9hdG1lbC1o bGNkYy9hdG1lbF9obGNkY19kYy5jCj4gQEAgLTI5LDYgKzI5LDExIEBACj4gIAo+ICAjZGVmaW5l IEFUTUVMX0hMQ0RDX0xBWUVSX0lSUVNfT0ZGU0VUCQk4Cj4gIAo+ICtzdGF0aWMgaW50IGF0bWVs X2hsY2RjX3ByZWZlcnJlZF9kZXB0aCBfX3JlYWRfbW9zdGx5Owo+ICsKPiArTU9EVUxFX1BBUk1f REVTQyhwcmVmZXJyZWRkZXB0aCwgIlNldCBwcmVmZXJyZWQgYnBwIik7Cj4gK21vZHVsZV9wYXJh bV9uYW1lZChwcmVmZXJyZWRkZXB0aCwgYXRtZWxfaGxjZGNfcHJlZmVycmVkX2RlcHRoLCBpbnQs IDA0MDApOwo+ICsKPiAgc3RhdGljIGNvbnN0IHN0cnVjdCBhdG1lbF9obGNkY19sYXllcl9kZXNj IGF0bWVsX2hsY2RjX2F0OTFzYW05bjEyX2xheWVyc1tdID0gewo+ICAJewo+ICAJCS5uYW1lID0g ImJhc2UiLAo+IEBAIC01OTAsNiArNTk1LDcgQEAgc3RhdGljIGludCBhdG1lbF9obGNkY19kY19t b2Rlc2V0X2luaXQoc3RydWN0IGRybV9kZXZpY2UgKmRldikKPiAgCWRldi0+bW9kZV9jb25maWcu bWluX2hlaWdodCA9IGRjLT5kZXNjLT5taW5faGVpZ2h0Owo+ICAJZGV2LT5tb2RlX2NvbmZpZy5t YXhfd2lkdGggPSBkYy0+ZGVzYy0+bWF4X3dpZHRoOwo+ICAJZGV2LT5tb2RlX2NvbmZpZy5tYXhf aGVpZ2h0ID0gZGMtPmRlc2MtPm1heF9oZWlnaHQ7Cj4gKwlkZXYtPm1vZGVfY29uZmlnLnByZWZl cnJlZF9kZXB0aCA9IDI0Owo+ICAJZGV2LT5tb2RlX2NvbmZpZy5mdW5jcyA9ICZtb2RlX2NvbmZp Z19mdW5jczsKPiAgCj4gIAlyZXR1cm4gMDsKPiBAQCAtNjU4LDcgKzY2NCw3IEBAIHN0YXRpYyBp bnQgYXRtZWxfaGxjZGNfZGNfbG9hZChzdHJ1Y3QgZHJtX2RldmljZSAqZGV2KQo+ICAKPiAgCXBs YXRmb3JtX3NldF9kcnZkYXRhKHBkZXYsIGRldik7Cj4gIAo+IC0JZHJtX2ZiX2NtYV9mYmRldl9p bml0KGRldiwgMjQsIDApOwo+ICsJZHJtX2ZiX2NtYV9mYmRldl9pbml0KGRldiwgYXRtZWxfaGxj ZGNfcHJlZmVycmVkX2RlcHRoLCAwKTsKPiAgCj4gIAlkcm1fa21zX2hlbHBlcl9wb2xsX2luaXQo ZGV2KTsKPiAgCj4gQEAgLTc1Niw2ICs3NjIsMTYgQEAgc3RhdGljIGludCBhdG1lbF9obGNkY19k Y19kcm1fcHJvYmUoc3RydWN0IHBsYXRmb3JtX2RldmljZSAqcGRldikKPiAgCXN0cnVjdCBkcm1f ZGV2aWNlICpkZGV2Owo+ICAJaW50IHJldDsKPiAgCj4gKwlzd2l0Y2ggKGF0bWVsX2hsY2RjX3By ZWZlcnJlZF9kZXB0aCkgewo+ICsJY2FzZSAwOiAvKiBkcml2ZXIgZGVmYXVsdCAqLwo+ICsJY2Fz ZSA4Ogo+ICsJY2FzZSAxNjoKPiArCWNhc2UgMjQ6Cj4gKwkJYnJlYWs7Cj4gKwlkZWZhdWx0Ogo+ ICsJCXJldHVybiAtRUlOVkFMOwo+ICsJfQo+ICsKPiAgCWRkZXYgPSBkcm1fZGV2X2FsbG9jKCZh dG1lbF9obGNkY19kY19kcml2ZXIsICZwZGV2LT5kZXYpOwo+ICAJaWYgKElTX0VSUihkZGV2KSkK PiAgCQlyZXR1cm4gUFRSX0VSUihkZGV2KTsKCgoKLS0gCkJvcmlzIEJyZXppbGxvbiwgQm9vdGxp biAoZm9ybWVybHkgRnJlZSBFbGVjdHJvbnMpCkVtYmVkZGVkIExpbnV4IGFuZCBLZXJuZWwgZW5n aW5lZXJpbmcKaHR0cHM6Ly9ib290bGluLmNvbQpfX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0 cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9s aXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753355AbeC1HfI (ORCPT ); Wed, 28 Mar 2018 03:35:08 -0400 Received: from mail.bootlin.com ([62.4.15.54]:49181 "EHLO mail.bootlin.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751932AbeC1HfG (ORCPT ); Wed, 28 Mar 2018 03:35:06 -0400 Date: Wed, 28 Mar 2018 09:34:54 +0200 From: Boris Brezillon To: Peter Rosin Cc: linux-kernel@vger.kernel.org, Boris Brezillon , David Airlie , Nicolas Ferre , Alexandre Belloni , Takashi Iwai , Egbert Eich , dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] drm/atmel-hlcdc: add command line option to specify preferred depth Message-ID: <20180328093454.4149fa3b@bbrezillon> In-Reply-To: <20180326073502.19259-1-peda@axentia.se> References: <20180326073502.19259-1-peda@axentia.se> X-Mailer: Claws Mail 3.15.0-dirty (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Peter, On Mon, 26 Mar 2018 09:35:02 +0200 Peter Rosin 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 > --- > 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=:[-] AFAIR, 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