From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Date: Thu, 01 Feb 2018 13:19:07 +0000 Subject: Re: [PATCH] drm/fb-helper: Scale back depth to supported maximum Message-Id: <20180201131907.GO5453@intel.com> List-Id: References: <20180201130446.6165-1-linus.walleij@linaro.org> In-Reply-To: <20180201130446.6165-1-linus.walleij@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: Linus Walleij Cc: linux-fbdev@vger.kernel.org, David Lechner , Bartlomiej Zolnierkiewicz , dri-devel@lists.freedesktop.org, Dave Airlie , linux-arm-kernel@lists.infradead.org On Thu, Feb 01, 2018 at 02:04:46PM +0100, Linus Walleij wrote: > The following happened when migrating an old fbdev driver to DRM: >=20 > The Integrator/CP PL111 supports 16BPP but only ARGB1555/ABGR1555 > or XRGB1555/XBGR1555 i.e. the maximum depth is 15. >=20 > This makes the initialization of the framebuffer fail since > the code in drm_fb_helper_single_fb_probe() assigns the same value > to sizes.surface_bpp and sizes.surface_depth. I.e. it simply assumes > a 1-to-1 mapping between BPP and depth, which is true in most cases > but typically not for this hardware. >=20 > To support the odd case of a driver supporting 16BPP with only 15 > bits of depth, this patch will make the code loop over the formats > supported on the primary plane and cap the depth to the maximum > supported. >=20 > On the PL110 Integrator, this makes drm_mode_legacy_fb_format() > select DRM_FORMAT_XRGB1555 which is acceptable for this driver, and > thus we get framebuffer, penguin and console on the Integrator/CP. >=20 > Signed-off-by: Linus Walleij > --- > drivers/gpu/drm/drm_fb_helper.c | 40 +++++++++++++++++++++++++++++++++++= ++++- > 1 file changed, 39 insertions(+), 1 deletion(-) >=20 > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_hel= per.c > index e56166334455..5076f9103740 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1720,6 +1720,8 @@ static int drm_fb_helper_single_fb_probe(struct drm= _fb_helper *fb_helper, > int i; > struct drm_fb_helper_surface_size sizes; > int gamma_size =3D 0; > + struct drm_plane *plane; > + int best_depth =3D 0; > =20 > memset(&sizes, 0, sizeof(struct drm_fb_helper_surface_size)); > sizes.surface_depth =3D 24; > @@ -1727,7 +1729,10 @@ static int drm_fb_helper_single_fb_probe(struct dr= m_fb_helper *fb_helper, > sizes.fb_width =3D (u32)-1; > sizes.fb_height =3D (u32)-1; > =20 > - /* if driver picks 8 or 16 by default use that for both depth/bpp */ > + /* > + * If driver picks 8 or 16 by default use that for both depth/bpp > + * to begin with > + */ > if (preferred_bpp !=3D sizes.surface_bpp) > sizes.surface_depth =3D sizes.surface_bpp =3D preferred_bpp; > =20 > @@ -1762,6 +1767,39 @@ static int drm_fb_helper_single_fb_probe(struct dr= m_fb_helper *fb_helper, > } > } > =20 > + /* > + * If we run into a situation where, for example, the primary plane > + * supports RGBA5551 (16 bpp, depth 15) but not RGB565 (16 bpp, depth > + * 16) we need to scale down the depth of the sizes we request. > + */ > + drm_for_each_plane(plane, fb_helper->dev) { > + /* Only check the primary plane */ > + if (plane->type !=3D DRM_PLANE_TYPE_PRIMARY) > + continue; I think this should look at crtc->primary for each of the crtcs managed by the fb_helper. Also this probably shouldn't look at YUV formats at all? I do wonder if instead we should just have the driver specify the pixel format explicitly instead of trying to guess based on bpp? > + > + for (i =3D 0; i < plane->format_count; i++) { > + const struct drm_format_info *fmt; > + > + fmt =3D drm_format_info(plane->format_types[i]); > + /* We found a perfect fit, great */ > + if (fmt->depth =3D sizes.surface_depth) > + break; > + > + /* Skip depths above what we're looking for */ > + if (fmt->depth > sizes.surface_depth) > + continue; > + > + /* Best depth found so far */ > + if (fmt->depth > best_depth) > + best_depth =3D fmt->depth; > + } > + } > + if (sizes.surface_depth !=3D best_depth) { > + DRM_DEBUG("requested bpp %d, scaled depth down to %d", > + sizes.surface_bpp, best_depth); > + sizes.surface_depth =3D best_depth; > + } > + > crtc_count =3D 0; > for (i =3D 0; i < fb_helper->crtc_count; i++) { > struct drm_display_mode *desired_mode; > --=20 > 2.14.3 >=20 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel --=20 Ville Syrj=E4l=E4 Intel OTC From mboxrd@z Thu Jan 1 00:00:00 1970 From: ville.syrjala@linux.intel.com (Ville =?iso-8859-1?Q?Syrj=E4l=E4?=) Date: Thu, 1 Feb 2018 15:19:07 +0200 Subject: [PATCH] drm/fb-helper: Scale back depth to supported maximum In-Reply-To: <20180201130446.6165-1-linus.walleij@linaro.org> References: <20180201130446.6165-1-linus.walleij@linaro.org> Message-ID: <20180201131907.GO5453@intel.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, Feb 01, 2018 at 02:04:46PM +0100, Linus Walleij wrote: > The following happened when migrating an old fbdev driver to DRM: > > The Integrator/CP PL111 supports 16BPP but only ARGB1555/ABGR1555 > or XRGB1555/XBGR1555 i.e. the maximum depth is 15. > > This makes the initialization of the framebuffer fail since > the code in drm_fb_helper_single_fb_probe() assigns the same value > to sizes.surface_bpp and sizes.surface_depth. I.e. it simply assumes > a 1-to-1 mapping between BPP and depth, which is true in most cases > but typically not for this hardware. > > To support the odd case of a driver supporting 16BPP with only 15 > bits of depth, this patch will make the code loop over the formats > supported on the primary plane and cap the depth to the maximum > supported. > > On the PL110 Integrator, this makes drm_mode_legacy_fb_format() > select DRM_FORMAT_XRGB1555 which is acceptable for this driver, and > thus we get framebuffer, penguin and console on the Integrator/CP. > > Signed-off-by: Linus Walleij > --- > drivers/gpu/drm/drm_fb_helper.c | 40 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 39 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index e56166334455..5076f9103740 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1720,6 +1720,8 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, > int i; > struct drm_fb_helper_surface_size sizes; > int gamma_size = 0; > + struct drm_plane *plane; > + int best_depth = 0; > > memset(&sizes, 0, sizeof(struct drm_fb_helper_surface_size)); > sizes.surface_depth = 24; > @@ -1727,7 +1729,10 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, > sizes.fb_width = (u32)-1; > sizes.fb_height = (u32)-1; > > - /* if driver picks 8 or 16 by default use that for both depth/bpp */ > + /* > + * If driver picks 8 or 16 by default use that for both depth/bpp > + * to begin with > + */ > if (preferred_bpp != sizes.surface_bpp) > sizes.surface_depth = sizes.surface_bpp = preferred_bpp; > > @@ -1762,6 +1767,39 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, > } > } > > + /* > + * If we run into a situation where, for example, the primary plane > + * supports RGBA5551 (16 bpp, depth 15) but not RGB565 (16 bpp, depth > + * 16) we need to scale down the depth of the sizes we request. > + */ > + drm_for_each_plane(plane, fb_helper->dev) { > + /* Only check the primary plane */ > + if (plane->type != DRM_PLANE_TYPE_PRIMARY) > + continue; I think this should look at crtc->primary for each of the crtcs managed by the fb_helper. Also this probably shouldn't look at YUV formats at all? I do wonder if instead we should just have the driver specify the pixel format explicitly instead of trying to guess based on bpp? > + > + for (i = 0; i < plane->format_count; i++) { > + const struct drm_format_info *fmt; > + > + fmt = drm_format_info(plane->format_types[i]); > + /* We found a perfect fit, great */ > + if (fmt->depth == sizes.surface_depth) > + break; > + > + /* Skip depths above what we're looking for */ > + if (fmt->depth > sizes.surface_depth) > + continue; > + > + /* Best depth found so far */ > + if (fmt->depth > best_depth) > + best_depth = fmt->depth; > + } > + } > + if (sizes.surface_depth != best_depth) { > + DRM_DEBUG("requested bpp %d, scaled depth down to %d", > + sizes.surface_bpp, best_depth); > + sizes.surface_depth = best_depth; > + } > + > crtc_count = 0; > for (i = 0; i < fb_helper->crtc_count; i++) { > struct drm_display_mode *desired_mode; > -- > 2.14.3 > > _______________________________________________ > dri-devel mailing list > dri-devel at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrj?l? Intel OTC From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/fb-helper: Scale back depth to supported maximum Date: Thu, 1 Feb 2018 15:19:07 +0200 Message-ID: <20180201131907.GO5453@intel.com> References: <20180201130446.6165-1-linus.walleij@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTPS id D76936E6E3 for ; Thu, 1 Feb 2018 13:19:12 +0000 (UTC) Content-Disposition: inline In-Reply-To: <20180201130446.6165-1-linus.walleij@linaro.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Linus Walleij Cc: linux-fbdev@vger.kernel.org, David Lechner , Bartlomiej Zolnierkiewicz , dri-devel@lists.freedesktop.org, Dave Airlie , linux-arm-kernel@lists.infradead.org List-Id: dri-devel@lists.freedesktop.org T24gVGh1LCBGZWIgMDEsIDIwMTggYXQgMDI6MDQ6NDZQTSArMDEwMCwgTGludXMgV2FsbGVpaiB3 cm90ZToKPiBUaGUgZm9sbG93aW5nIGhhcHBlbmVkIHdoZW4gbWlncmF0aW5nIGFuIG9sZCBmYmRl diBkcml2ZXIgdG8gRFJNOgo+IAo+IFRoZSBJbnRlZ3JhdG9yL0NQIFBMMTExIHN1cHBvcnRzIDE2 QlBQIGJ1dCBvbmx5IEFSR0IxNTU1L0FCR1IxNTU1Cj4gb3IgWFJHQjE1NTUvWEJHUjE1NTUgaS5l LiB0aGUgbWF4aW11bSBkZXB0aCBpcyAxNS4KPiAKPiBUaGlzIG1ha2VzIHRoZSBpbml0aWFsaXph dGlvbiBvZiB0aGUgZnJhbWVidWZmZXIgZmFpbCBzaW5jZQo+IHRoZSBjb2RlIGluIGRybV9mYl9o ZWxwZXJfc2luZ2xlX2ZiX3Byb2JlKCkgYXNzaWducyB0aGUgc2FtZSB2YWx1ZQo+IHRvIHNpemVz LnN1cmZhY2VfYnBwIGFuZCBzaXplcy5zdXJmYWNlX2RlcHRoLiBJLmUuIGl0IHNpbXBseSBhc3N1 bWVzCj4gYSAxLXRvLTEgbWFwcGluZyBiZXR3ZWVuIEJQUCBhbmQgZGVwdGgsIHdoaWNoIGlzIHRy dWUgaW4gbW9zdCBjYXNlcwo+IGJ1dCB0eXBpY2FsbHkgbm90IGZvciB0aGlzIGhhcmR3YXJlLgo+ IAo+IFRvIHN1cHBvcnQgdGhlIG9kZCBjYXNlIG9mIGEgZHJpdmVyIHN1cHBvcnRpbmcgMTZCUFAg d2l0aCBvbmx5IDE1Cj4gYml0cyBvZiBkZXB0aCwgdGhpcyBwYXRjaCB3aWxsIG1ha2UgdGhlIGNv ZGUgbG9vcCBvdmVyIHRoZSBmb3JtYXRzCj4gc3VwcG9ydGVkIG9uIHRoZSBwcmltYXJ5IHBsYW5l IGFuZCBjYXAgdGhlIGRlcHRoIHRvIHRoZSBtYXhpbXVtCj4gc3VwcG9ydGVkLgo+IAo+IE9uIHRo ZSBQTDExMCBJbnRlZ3JhdG9yLCB0aGlzIG1ha2VzIGRybV9tb2RlX2xlZ2FjeV9mYl9mb3JtYXQo KQo+IHNlbGVjdCBEUk1fRk9STUFUX1hSR0IxNTU1IHdoaWNoIGlzIGFjY2VwdGFibGUgZm9yIHRo aXMgZHJpdmVyLCBhbmQKPiB0aHVzIHdlIGdldCBmcmFtZWJ1ZmZlciwgcGVuZ3VpbiBhbmQgY29u c29sZSBvbiB0aGUgSW50ZWdyYXRvci9DUC4KPiAKPiBTaWduZWQtb2ZmLWJ5OiBMaW51cyBXYWxs ZWlqIDxsaW51cy53YWxsZWlqQGxpbmFyby5vcmc+Cj4gLS0tCj4gIGRyaXZlcnMvZ3B1L2RybS9k cm1fZmJfaGVscGVyLmMgfCA0MCArKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysr KystCj4gIDEgZmlsZSBjaGFuZ2VkLCAzOSBpbnNlcnRpb25zKCspLCAxIGRlbGV0aW9uKC0pCj4g Cj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9kcm1fZmJfaGVscGVyLmMgYi9kcml2ZXJz L2dwdS9kcm0vZHJtX2ZiX2hlbHBlci5jCj4gaW5kZXggZTU2MTY2MzM0NDU1Li41MDc2ZjkxMDM3 NDAgMTAwNjQ0Cj4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2RybV9mYl9oZWxwZXIuYwo+ICsrKyBi L2RyaXZlcnMvZ3B1L2RybS9kcm1fZmJfaGVscGVyLmMKPiBAQCAtMTcyMCw2ICsxNzIwLDggQEAg c3RhdGljIGludCBkcm1fZmJfaGVscGVyX3NpbmdsZV9mYl9wcm9iZShzdHJ1Y3QgZHJtX2ZiX2hl bHBlciAqZmJfaGVscGVyLAo+ICAJaW50IGk7Cj4gIAlzdHJ1Y3QgZHJtX2ZiX2hlbHBlcl9zdXJm YWNlX3NpemUgc2l6ZXM7Cj4gIAlpbnQgZ2FtbWFfc2l6ZSA9IDA7Cj4gKwlzdHJ1Y3QgZHJtX3Bs YW5lICpwbGFuZTsKPiArCWludCBiZXN0X2RlcHRoID0gMDsKPiAgCj4gIAltZW1zZXQoJnNpemVz LCAwLCBzaXplb2Yoc3RydWN0IGRybV9mYl9oZWxwZXJfc3VyZmFjZV9zaXplKSk7Cj4gIAlzaXpl cy5zdXJmYWNlX2RlcHRoID0gMjQ7Cj4gQEAgLTE3MjcsNyArMTcyOSwxMCBAQCBzdGF0aWMgaW50 IGRybV9mYl9oZWxwZXJfc2luZ2xlX2ZiX3Byb2JlKHN0cnVjdCBkcm1fZmJfaGVscGVyICpmYl9o ZWxwZXIsCj4gIAlzaXplcy5mYl93aWR0aCA9ICh1MzIpLTE7Cj4gIAlzaXplcy5mYl9oZWlnaHQg PSAodTMyKS0xOwo+ICAKPiAtCS8qIGlmIGRyaXZlciBwaWNrcyA4IG9yIDE2IGJ5IGRlZmF1bHQg dXNlIHRoYXQgZm9yIGJvdGggZGVwdGgvYnBwICovCj4gKwkvKgo+ICsJICogSWYgZHJpdmVyIHBp Y2tzIDggb3IgMTYgYnkgZGVmYXVsdCB1c2UgdGhhdCBmb3IgYm90aCBkZXB0aC9icHAKPiArCSAq IHRvIGJlZ2luIHdpdGgKPiArCSAqLwo+ICAJaWYgKHByZWZlcnJlZF9icHAgIT0gc2l6ZXMuc3Vy ZmFjZV9icHApCj4gIAkJc2l6ZXMuc3VyZmFjZV9kZXB0aCA9IHNpemVzLnN1cmZhY2VfYnBwID0g cHJlZmVycmVkX2JwcDsKPiAgCj4gQEAgLTE3NjIsNiArMTc2NywzOSBAQCBzdGF0aWMgaW50IGRy bV9mYl9oZWxwZXJfc2luZ2xlX2ZiX3Byb2JlKHN0cnVjdCBkcm1fZmJfaGVscGVyICpmYl9oZWxw ZXIsCj4gIAkJfQo+ICAJfQo+ICAKPiArCS8qCj4gKwkgKiBJZiB3ZSBydW4gaW50byBhIHNpdHVh dGlvbiB3aGVyZSwgZm9yIGV4YW1wbGUsIHRoZSBwcmltYXJ5IHBsYW5lCj4gKwkgKiBzdXBwb3J0 cyBSR0JBNTU1MSAoMTYgYnBwLCBkZXB0aCAxNSkgYnV0IG5vdCBSR0I1NjUgKDE2IGJwcCwgZGVw dGgKPiArCSAqIDE2KSB3ZSBuZWVkIHRvIHNjYWxlIGRvd24gdGhlIGRlcHRoIG9mIHRoZSBzaXpl cyB3ZSByZXF1ZXN0Lgo+ICsJICovCj4gKwlkcm1fZm9yX2VhY2hfcGxhbmUocGxhbmUsIGZiX2hl bHBlci0+ZGV2KSB7Cj4gKwkJLyogT25seSBjaGVjayB0aGUgcHJpbWFyeSBwbGFuZSAqLwo+ICsJ CWlmIChwbGFuZS0+dHlwZSAhPSBEUk1fUExBTkVfVFlQRV9QUklNQVJZKQo+ICsJCQljb250aW51 ZTsKCkkgdGhpbmsgdGhpcyBzaG91bGQgbG9vayBhdCBjcnRjLT5wcmltYXJ5IGZvciBlYWNoIG9m IHRoZSBjcnRjcyBtYW5hZ2VkCmJ5IHRoZSBmYl9oZWxwZXIuCgpBbHNvIHRoaXMgcHJvYmFibHkg c2hvdWxkbid0IGxvb2sgYXQgWVVWIGZvcm1hdHMgYXQgYWxsPwoKSSBkbyB3b25kZXIgaWYgaW5z dGVhZCB3ZSBzaG91bGQganVzdCBoYXZlIHRoZSBkcml2ZXIgc3BlY2lmeSB0aGUKcGl4ZWwgZm9y bWF0IGV4cGxpY2l0bHkgaW5zdGVhZCBvZiB0cnlpbmcgdG8gZ3Vlc3MgYmFzZWQgb24gYnBwPwoK PiArCj4gKwkJZm9yIChpID0gMDsgaSA8IHBsYW5lLT5mb3JtYXRfY291bnQ7IGkrKykgewo+ICsJ CQljb25zdCBzdHJ1Y3QgZHJtX2Zvcm1hdF9pbmZvICpmbXQ7Cj4gKwo+ICsJCQlmbXQgPSBkcm1f Zm9ybWF0X2luZm8ocGxhbmUtPmZvcm1hdF90eXBlc1tpXSk7Cj4gKwkJCS8qIFdlIGZvdW5kIGEg cGVyZmVjdCBmaXQsIGdyZWF0ICovCj4gKwkJCWlmIChmbXQtPmRlcHRoID09IHNpemVzLnN1cmZh Y2VfZGVwdGgpCj4gKwkJCQlicmVhazsKPiArCj4gKwkJCS8qIFNraXAgZGVwdGhzIGFib3ZlIHdo YXQgd2UncmUgbG9va2luZyBmb3IgKi8KPiArCQkJaWYgKGZtdC0+ZGVwdGggPiBzaXplcy5zdXJm YWNlX2RlcHRoKQo+ICsJCQkJY29udGludWU7Cj4gKwo+ICsJCQkvKiBCZXN0IGRlcHRoIGZvdW5k IHNvIGZhciAqLwo+ICsJCQlpZiAoZm10LT5kZXB0aCA+IGJlc3RfZGVwdGgpCj4gKwkJCQliZXN0 X2RlcHRoID0gZm10LT5kZXB0aDsKPiArCQl9Cj4gKwl9Cj4gKwlpZiAoc2l6ZXMuc3VyZmFjZV9k ZXB0aCAhPSBiZXN0X2RlcHRoKSB7Cj4gKwkJRFJNX0RFQlVHKCJyZXF1ZXN0ZWQgYnBwICVkLCBz Y2FsZWQgZGVwdGggZG93biB0byAlZCIsCj4gKwkJCSAgc2l6ZXMuc3VyZmFjZV9icHAsIGJlc3Rf ZGVwdGgpOwo+ICsJCXNpemVzLnN1cmZhY2VfZGVwdGggPSBiZXN0X2RlcHRoOwo+ICsJfQo+ICsK PiAgCWNydGNfY291bnQgPSAwOwo+ICAJZm9yIChpID0gMDsgaSA8IGZiX2hlbHBlci0+Y3J0Y19j b3VudDsgaSsrKSB7Cj4gIAkJc3RydWN0IGRybV9kaXNwbGF5X21vZGUgKmRlc2lyZWRfbW9kZTsK PiAtLSAKPiAyLjE0LjMKPiAKPiBfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fXwo+IGRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKPiBkcmktZGV2ZWxAbGlzdHMuZnJl ZWRlc2t0b3Aub3JnCj4gaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0 aW5mby9kcmktZGV2ZWwKCi0tIApWaWxsZSBTeXJqw6Rsw6QKSW50ZWwgT1RDCl9fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxp c3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNr dG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo=