From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932592AbdBWAtS (ORCPT ); Wed, 22 Feb 2017 19:49:18 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:32985 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754893AbdBWAtK (ORCPT ); Wed, 22 Feb 2017 19:49:10 -0500 Date: Wed, 22 Feb 2017 16:49:01 -0800 From: Maxime Ripard To: Stefan Lengfeld Cc: Daniel Vetter , David Airlie , Sean Paul , Jani Nikula , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH v3 1/2] drm/fb-helper: Add multi buffer support for cma fbdev Message-ID: <20170223004901.2e257pf5gy6v7736@lukather> References: <075ffb50cc16ab055b5d47b30163401bb356ab51.1487175046.git-series.maxime.ripard@free-electrons.com> <20170220170710.GA17643@sill.h.stcim.de> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="b2jfkctngsbhr6qf" Content-Disposition: inline In-Reply-To: <20170220170710.GA17643@sill.h.stcim.de> User-Agent: Mutt/1.6.2-neo (2016-08-21) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --b2jfkctngsbhr6qf Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Stefan, On Mon, Feb 20, 2017 at 06:07:10PM +0100, Stefan Lengfeld wrote: > Hi Maxime, >=20 > sorry, I have missed the discussion about the double buffering/virtual > surface size patch series two weeks ago. My comments about the patch are > inline: >=20 > On Wed, Feb 15, 2017 at 05:19:08PM +0100, Maxime Ripard wrote: > > From: Xinliang Liu >=20 > Mabye you should take the authorship here. Taking the credit and the > blame, because the patch was heavily modified by you and me. But I don't > really know what the offical police about that is. I don't usually know when to do that as well. But yeah, you're probably right. > >=20 > > This patch add a config to support to create multi buffer for cma fbdev. > > Such as double buffer and triple buffer. > >=20 > > 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. > >=20 > > Signed-off-by: Xinliang Liu > > [s.christ@phytec.de: Picking patch from > > https://lkml.org/lkml/2015/9/14/188] > > Signed-off-by: Stefan Christ > > Signed-off-by: Maxime Ripard >=20 > Reviewed-by: Stefan Lengfeld >=20 > My surname has changed from "Christ" to "Lengfeld" recently. So my > review tag contains the new name. Ack >=20 > > --- > > drivers/gpu/drm/Kconfig | 9 +++++++++ > > drivers/gpu/drm/drm_fb_helper.c | 10 ++++++++++ > > 2 files changed, 19 insertions(+), 0 deletions(-) > >=20 > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > > index ebfe8404c25f..700c8b8e57a9 100644 > > --- a/drivers/gpu/drm/Kconfig > > +++ b/drivers/gpu/drm/Kconfig > > @@ -84,6 +84,15 @@ config DRM_FBDEV_EMULATION > > =20 > > If in doubt, say "Y". > > =20 > > +config DRM_FBDEV_OVERALLOC > > + int "Overallocation of the fbdev buffer" > > + depends on DRM_FBDEV_EMULATION > > + default 100 > > + help > > + Defines the fbdev buffer overallocation in percent. Default > > + is 100. Typical values for double buffering will be 200, > > + triple buffering 300. > > + > > config DRM_LOAD_EDID_FIRMWARE > > bool "Allow to specify an EDID data set instead of probing for it" > > depends on DRM_KMS_HELPER > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_h= elper.c > > index e934b541feea..c6de87abaca8 100644 > > --- a/drivers/gpu/drm/drm_fb_helper.c > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > @@ -48,6 +48,12 @@ module_param_named(fbdev_emulation, drm_fbdev_emulat= ion, bool, 0600); > > MODULE_PARM_DESC(fbdev_emulation, > > "Enable legacy fbdev emulation [default=3Dtrue]"); > > =20 > > +static int drm_fbdev_overalloc =3D CONFIG_DRM_FBDEV_OVERALLOC; > > +module_param(drm_fbdev_overalloc, int, 0444); >=20 > Maybe the variable should be of type "uint" instead of "int". This would > rule out the negative numbers error case. Yep, I'll change it. > > +MODULE_PARM_DESC(drm_fbdev_overalloc, > > + "Overallocation of the fbdev buffer (%) [default=3D" > > + __MODULE_STRING(CONFIG_DRM_FBDEV_OVERALLOC) "]"); > > + > > static LIST_HEAD(kernel_fb_helper_list); > > static DEFINE_MUTEX(kernel_fb_helper_lock); > > =20 > > @@ -1573,6 +1579,10 @@ static int drm_fb_helper_single_fb_probe(struct = drm_fb_helper *fb_helper, > > sizes.fb_height =3D sizes.surface_height =3D 768; > > } > > =20 > > + /* Handle our overallocation */ > > + sizes.surface_height *=3D drm_fbdev_overalloc; > > + sizes.surface_height /=3D 100; > > + >=20 > The code can trigger an arithmetic overflow, but I think we can ignore > this error case here. >=20 > But there should be a check for drm_fbdev_overalloc not be smaller than > 100. If it is smaller, the variable drm_fbdev_overalloc should have the > default value "100". Otherwise the virtual surface height can be smaller > than the physical height. This could trigger a lot of errors in existing > code paths. That's a really good point, I'll change that. Thanks! Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --b2jfkctngsbhr6qf Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJYrjF9AAoJEBx+YmzsjxAgdcwQALPqvWMWSd7NpcCGD57Ch99O i28nzOou3+S2sImsgrTT60hdfhrjbtiEH4G1Dg4snEGmRWZ9mCdwXE4yKQS2kFmV Sy8lm8Ws8F4UHRgkZTCxgLkfF0WO2JIcQGSMPva+fKYbN9MG4nIejmJCUjcO53yn vc9UrkrcODEHFjNZxBbJRaCWAndK1ZZpi7GC1tA9GHo9RiWPu8oL/senm3I5wKGF 8sW615BpFR+vEUN78FXwNTlDwn1M3AnZvXGQLwULR2arB0wWziQ5SxsYPTIUvsKz TIfE2l1nvKYABFTXUw2q5l0Rp7IRFUhvUXVeb3hTXqin/BR1XCrexIxXgDhH2Cgr xiemmZXem1bgPzC9Lw+bOSlmbOvHEqPNxtdwVO0Y3UC3ZAdgtuL6wPAZgk7Hbkma Dfxg37LKcqyfVllA2qv60iF3kKVWyKtzF63ApOAU3ciIoF8Fvoo5BJElQSTSrMEc fx6SdcaE9yVrqY/PjqWk56cJYlLosuV2AbBJke6JDeti4ZfrhZUraHXQ1NGoLn8o g0yr4ajsmGj/cEmcCjS9J29MdsXZxr7MuPMk7q8nqpoAiQ2QhRYxOEUmlnNyMa63 awXqcE3+LSmsLQDk/7jEx6pgXP+vfFMFT0aHIVzteQEhLzwhK9zBjAPUxWRvj/gu UX7eJ2YdKkucsDp5NGn6 =m9SP -----END PGP SIGNATURE----- --b2jfkctngsbhr6qf--