[PATCH 0/3] Bandwidth limitation on PL111, take 2

[PATCH 0/3] Bandwidth limitation on PL111, take 2

[Linus Walleij]

I had to add a hook into the simple KMS helper to be able
to gate off too high bandwidths on the PL111.

Let me know what you think.

This solution is inspired by the OMAPDRM commit a7631c4b9846
"drm/omap: Filter displays mode based on bandwidth limit"
by Peter Ujfalusi.

I'm not super-happy that we don't know or negotiate the
desired pixel format when the CRTC asks the driver whether
the mode is valid, I need to think more about this for the
future.

For now patch 2/2 just hammers that down to 16 or 32 BPP
depending on variant, so we get running code. But the
BPP is not used to negotiate formats wrt bandwidth, we
just fix that variable and work on the resolution
limitation.

Maybe the 16BPP systems are in such a minority that this
hard-coding is reasonable.

Linus Walleij (3):
  drm: simple_kms_helper: Add mode_valid() callback support
  drm/pl111: Make the default BPP a per-variant variable
  drm/pl111: Use max memory bandwidth for resolution

 drivers/gpu/drm/drm_simple_kms_helper.c | 15 +++++++++++++++
 drivers/gpu/drm/pl111/pl111_display.c   | 30 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/pl111/pl111_drm.h       |  3 +++
 drivers/gpu/drm/pl111/pl111_drv.c       | 10 +++++++++-
 drivers/gpu/drm/pl111/pl111_versatile.c |  2 ++
 include/drm/drm_simple_kms_helper.h     | 15 +++++++++++++++
 6 files changed, 74 insertions(+), 1 deletion(-)

-- 
2.14.3

[PATCH 0/3] Bandwidth limitation on PL111, take 2

[Linus Walleij]

I had to add a hook into the simple KMS helper to be able
to gate off too high bandwidths on the PL111.

Let me know what you think.

This solution is inspired by the OMAPDRM commit a7631c4b9846
"drm/omap: Filter displays mode based on bandwidth limit"
by Peter Ujfalusi.

I'm not super-happy that we don't know or negotiate the
desired pixel format when the CRTC asks the driver whether
the mode is valid, I need to think more about this for the
future.

For now patch 2/2 just hammers that down to 16 or 32 BPP
depending on variant, so we get running code. But the
BPP is not used to negotiate formats wrt bandwidth, we
just fix that variable and work on the resolution
limitation.

Maybe the 16BPP systems are in such a minority that this
hard-coding is reasonable.

Linus Walleij (3):
  drm: simple_kms_helper: Add mode_valid() callback support
  drm/pl111: Make the default BPP a per-variant variable
  drm/pl111: Use max memory bandwidth for resolution

 drivers/gpu/drm/drm_simple_kms_helper.c | 15 +++++++++++++++
 drivers/gpu/drm/pl111/pl111_display.c   | 30 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/pl111/pl111_drm.h       |  3 +++
 drivers/gpu/drm/pl111/pl111_drv.c       | 10 +++++++++-
 drivers/gpu/drm/pl111/pl111_versatile.c |  2 ++
 include/drm/drm_simple_kms_helper.h     | 15 +++++++++++++++
 6 files changed, 74 insertions(+), 1 deletion(-)

-- 
2.14.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm: simple_kms_helper: Add mode_valid() callback support

[Linus Walleij]

The PL111 needs to filter valid modes based on memory bandwidth.
I guess it is a pretty simple operation, so we can still claim
the DRM KMS helper pipeline is simple after adding this (optional)
vtable callback.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/drm_simple_kms_helper.c | 15 +++++++++++++++
 include/drm/drm_simple_kms_helper.h     | 15 +++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index dc9fd109de14..b7840cf34808 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -34,6 +34,20 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
 
+static enum drm_mode_status
+drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
+			       const struct drm_display_mode *mode)
+{
+	struct drm_simple_display_pipe *pipe;
+
+	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
+	if (!pipe->funcs || !pipe->funcs->mode_valid)
+		/* Anything goes */
+		return MODE_OK;
+
+	return pipe->funcs->mode_valid(crtc, mode);
+}
+
 static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
 				     struct drm_crtc_state *state)
 {
@@ -72,6 +86,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
+	.mode_valid = drm_simple_kms_crtc_mode_valid,
 	.atomic_check = drm_simple_kms_crtc_check,
 	.atomic_enable = drm_simple_kms_crtc_enable,
 	.atomic_disable = drm_simple_kms_crtc_disable,
diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
index 6d9adbb46293..ad74cb33c539 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -21,6 +21,21 @@ struct drm_simple_display_pipe;
  *                                        display pipeline
  */
 struct drm_simple_display_pipe_funcs {
+	/**
+	 * @mode_valid:
+	 *
+	 * This function is called to filter out valid modes from the
+	 * suggestions suggested by the bridge or display. This optional
+	 * hook is passed in when initializing the pipeline.
+	 *
+	 * RETURNS:
+	 *
+	 * MODE_OK if the mode is acceptable.
+	 * MODE_BAD if we need to try something else.
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
+					   const struct drm_display_mode *mode);
+
 	/**
 	 * @enable:
 	 *
-- 
2.14.3

[PATCH 1/3] drm: simple_kms_helper: Add mode_valid() callback support

[Linus Walleij]

The PL111 needs to filter valid modes based on memory bandwidth.
I guess it is a pretty simple operation, so we can still claim
the DRM KMS helper pipeline is simple after adding this (optional)
vtable callback.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/drm_simple_kms_helper.c | 15 +++++++++++++++
 include/drm/drm_simple_kms_helper.h     | 15 +++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index dc9fd109de14..b7840cf34808 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -34,6 +34,20 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
 
+static enum drm_mode_status
+drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
+			       const struct drm_display_mode *mode)
+{
+	struct drm_simple_display_pipe *pipe;
+
+	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
+	if (!pipe->funcs || !pipe->funcs->mode_valid)
+		/* Anything goes */
+		return MODE_OK;
+
+	return pipe->funcs->mode_valid(crtc, mode);
+}
+
 static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
 				     struct drm_crtc_state *state)
 {
@@ -72,6 +86,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
+	.mode_valid = drm_simple_kms_crtc_mode_valid,
 	.atomic_check = drm_simple_kms_crtc_check,
 	.atomic_enable = drm_simple_kms_crtc_enable,
 	.atomic_disable = drm_simple_kms_crtc_disable,
diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
index 6d9adbb46293..ad74cb33c539 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -21,6 +21,21 @@ struct drm_simple_display_pipe;
  *                                        display pipeline
  */
 struct drm_simple_display_pipe_funcs {
+	/**
+	 * @mode_valid:
+	 *
+	 * This function is called to filter out valid modes from the
+	 * suggestions suggested by the bridge or display. This optional
+	 * hook is passed in when initializing the pipeline.
+	 *
+	 * RETURNS:
+	 *
+	 * MODE_OK if the mode is acceptable.
+	 * MODE_BAD if we need to try something else.
+	 */
+	enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
+					   const struct drm_display_mode *mode);
+
 	/**
 	 * @enable:
 	 *
-- 
2.14.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/pl111: Make the default BPP a per-variant variable

[Linus Walleij]

The PL110, Integrator and Versatile boards strongly prefer to
use 16 BPP even if other modes are supported, both to keep down
memory consumption and also to easier find a good match to
supported resolutions with consideration taken to the memory
bandwidth of the platforms.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/pl111/pl111_drm.h       | 2 ++
 drivers/gpu/drm/pl111/pl111_drv.c       | 4 +++-
 drivers/gpu/drm/pl111/pl111_versatile.c | 2 ++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/pl111/pl111_drm.h b/drivers/gpu/drm/pl111/pl111_drm.h
index d74076c6b7ef..9f2d30b52e7a 100644
--- a/drivers/gpu/drm/pl111/pl111_drm.h
+++ b/drivers/gpu/drm/pl111/pl111_drm.h
@@ -43,6 +43,7 @@ struct drm_minor;
  * @broken_vblank: the vblank IRQ is broken on this variant
  * @formats: array of supported pixel formats on this variant
  * @nformats: the length of the array of supported pixel formats
+ * @fb_bpp: desired bits per pixel on the default framebuffer
  */
 struct pl111_variant_data {
 	const char *name;
@@ -52,6 +53,7 @@ struct pl111_variant_data {
 	bool broken_vblank;
 	const u32 *formats;
 	unsigned int nformats;
+	unsigned int fb_bpp;
 };
 
 struct pl111_drm_dev_private {
diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
index 221f3af02fb4..f5d5aa464ae2 100644
--- a/drivers/gpu/drm/pl111/pl111_drv.c
+++ b/drivers/gpu/drm/pl111/pl111_drv.c
@@ -191,7 +191,7 @@ static int pl111_modeset_init(struct drm_device *dev)
 
 	drm_mode_config_reset(dev);
 
-	priv->fbdev = drm_fbdev_cma_init(dev, 32,
+	priv->fbdev = drm_fbdev_cma_init(dev, priv->variant->fb_bpp,
 					 dev->mode_config.num_connector);
 	if (IS_ERR(priv->fbdev)) {
 		dev_err(dev->dev, "Failed to initialize CMA framebuffer\n");
@@ -354,6 +354,7 @@ static const struct pl111_variant_data pl110_variant = {
 	.is_pl110 = true,
 	.formats = pl110_pixel_formats,
 	.nformats = ARRAY_SIZE(pl110_pixel_formats),
+	.fb_bpp = 16,
 };
 
 /* RealView, Versatile Express etc use this modern variant */
@@ -378,6 +379,7 @@ static const struct pl111_variant_data pl111_variant = {
 	.name = "PL111",
 	.formats = pl111_pixel_formats,
 	.nformats = ARRAY_SIZE(pl111_pixel_formats),
+	.fb_bpp = 32,
 };
 
 static const struct amba_id pl111_id_table[] = {
diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
index 11024ad64181..9825f6d52788 100644
--- a/drivers/gpu/drm/pl111/pl111_versatile.c
+++ b/drivers/gpu/drm/pl111/pl111_versatile.c
@@ -241,6 +241,7 @@ static const struct pl111_variant_data pl110_integrator = {
 	.broken_vblank = true,
 	.formats = pl110_integrator_pixel_formats,
 	.nformats = ARRAY_SIZE(pl110_integrator_pixel_formats),
+	.fb_bpp = 16,
 };
 
 /*
@@ -253,6 +254,7 @@ static const struct pl111_variant_data pl110_versatile = {
 	.external_bgr = true,
 	.formats = pl110_versatile_pixel_formats,
 	.nformats = ARRAY_SIZE(pl110_versatile_pixel_formats),
+	.fb_bpp = 16,
 };
 
 int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
-- 
2.14.3

[PATCH 2/3] drm/pl111: Make the default BPP a per-variant variable

[Linus Walleij]

The PL110, Integrator and Versatile boards strongly prefer to
use 16 BPP even if other modes are supported, both to keep down
memory consumption and also to easier find a good match to
supported resolutions with consideration taken to the memory
bandwidth of the platforms.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/pl111/pl111_drm.h       | 2 ++
 drivers/gpu/drm/pl111/pl111_drv.c       | 4 +++-
 drivers/gpu/drm/pl111/pl111_versatile.c | 2 ++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/pl111/pl111_drm.h b/drivers/gpu/drm/pl111/pl111_drm.h
index d74076c6b7ef..9f2d30b52e7a 100644
--- a/drivers/gpu/drm/pl111/pl111_drm.h
+++ b/drivers/gpu/drm/pl111/pl111_drm.h
@@ -43,6 +43,7 @@ struct drm_minor;
  * @broken_vblank: the vblank IRQ is broken on this variant
  * @formats: array of supported pixel formats on this variant
  * @nformats: the length of the array of supported pixel formats
+ * @fb_bpp: desired bits per pixel on the default framebuffer
  */
 struct pl111_variant_data {
 	const char *name;
@@ -52,6 +53,7 @@ struct pl111_variant_data {
 	bool broken_vblank;
 	const u32 *formats;
 	unsigned int nformats;
+	unsigned int fb_bpp;
 };
 
 struct pl111_drm_dev_private {
diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
index 221f3af02fb4..f5d5aa464ae2 100644
--- a/drivers/gpu/drm/pl111/pl111_drv.c
+++ b/drivers/gpu/drm/pl111/pl111_drv.c
@@ -191,7 +191,7 @@ static int pl111_modeset_init(struct drm_device *dev)
 
 	drm_mode_config_reset(dev);
 
-	priv->fbdev = drm_fbdev_cma_init(dev, 32,
+	priv->fbdev = drm_fbdev_cma_init(dev, priv->variant->fb_bpp,
 					 dev->mode_config.num_connector);
 	if (IS_ERR(priv->fbdev)) {
 		dev_err(dev->dev, "Failed to initialize CMA framebuffer\n");
@@ -354,6 +354,7 @@ static const struct pl111_variant_data pl110_variant = {
 	.is_pl110 = true,
 	.formats = pl110_pixel_formats,
 	.nformats = ARRAY_SIZE(pl110_pixel_formats),
+	.fb_bpp = 16,
 };
 
 /* RealView, Versatile Express etc use this modern variant */
@@ -378,6 +379,7 @@ static const struct pl111_variant_data pl111_variant = {
 	.name = "PL111",
 	.formats = pl111_pixel_formats,
 	.nformats = ARRAY_SIZE(pl111_pixel_formats),
+	.fb_bpp = 32,
 };
 
 static const struct amba_id pl111_id_table[] = {
diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
index 11024ad64181..9825f6d52788 100644
--- a/drivers/gpu/drm/pl111/pl111_versatile.c
+++ b/drivers/gpu/drm/pl111/pl111_versatile.c
@@ -241,6 +241,7 @@ static const struct pl111_variant_data pl110_integrator = {
 	.broken_vblank = true,
 	.formats = pl110_integrator_pixel_formats,
 	.nformats = ARRAY_SIZE(pl110_integrator_pixel_formats),
+	.fb_bpp = 16,
 };
 
 /*
@@ -253,6 +254,7 @@ static const struct pl111_variant_data pl110_versatile = {
 	.external_bgr = true,
 	.formats = pl110_versatile_pixel_formats,
 	.nformats = ARRAY_SIZE(pl110_versatile_pixel_formats),
+	.fb_bpp = 16,
 };
 
 int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
-- 
2.14.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/pl111: Use max memory bandwidth for resolution

[Linus Walleij]

We were previously selecting 1024x768 and 32BPP as the default
set-up for the PL111 consumers.

This does not work on elder systems: the device tree bindings
support a property "max-memory-bandwidth" in bytes/second that
states that if you exceed this the memory bus will saturate.
The result is flickering and unstable images.

Parse the "max-memory-bandwidth" and respect it when
intializing the driver. On the RealView PB11MP, Versatile and
Integrator/CP we get a nice console as default with this code.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Exploit the new .mode_valid() callback we added to the
  simple KMS helper.
- Use the hardcoded bits per pixel per variant instead of
  trying to be heuristic about this setting for now.
---
 drivers/gpu/drm/pl111/pl111_display.c | 30 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/pl111/pl111_drm.h     |  1 +
 drivers/gpu/drm/pl111/pl111_drv.c     |  6 ++++++
 3 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
index d75923896609..a1ca9e1ffe15 100644
--- a/drivers/gpu/drm/pl111/pl111_display.c
+++ b/drivers/gpu/drm/pl111/pl111_display.c
@@ -50,6 +50,35 @@ irqreturn_t pl111_irq(int irq, void *data)
 	return status;
 }
 
+static enum drm_mode_status
+pl111_mode_valid(struct drm_crtc *crtc,
+		 const struct drm_display_mode *mode)
+{
+	struct drm_device *drm = crtc->dev;
+	struct pl111_drm_dev_private *priv = drm->dev_private;
+	u32 cpp = priv->variant->fb_bpp / 8;
+	u64 bw;
+
+	/*
+	 * We use the pixelclock to also account for interlaced modes, the
+	 * resulting bandwidth is in bytes per second.
+	 */
+	bw = mode->clock * 1000; /* In Hz */
+	bw = bw * mode->hdisplay * mode->vdisplay * cpp;
+	bw = div_u64(bw, mode->htotal * mode->vtotal);
+
+	if (bw > priv->memory_bw) {
+		DRM_DEBUG("%d x %d @ %d Hz, %d cpp, bw %llu too fast\n",
+			  mode->hdisplay, mode->vdisplay, mode->clock, cpp, bw);
+
+		return MODE_BAD;
+	}
+	DRM_DEBUG("%d x %d @ %d Hz, %d cpp, bw %llu bytes/s OK\n",
+		  mode->hdisplay, mode->vdisplay, mode->clock, cpp, bw);
+
+	return MODE_OK;
+}
+
 static int pl111_display_check(struct drm_simple_display_pipe *pipe,
 			       struct drm_plane_state *pstate,
 			       struct drm_crtc_state *cstate)
@@ -344,6 +373,7 @@ static int pl111_display_prepare_fb(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs pl111_display_funcs = {
+	.mode_valid = pl111_mode_valid,
 	.check = pl111_display_check,
 	.enable = pl111_display_enable,
 	.disable = pl111_display_disable,
diff --git a/drivers/gpu/drm/pl111/pl111_drm.h b/drivers/gpu/drm/pl111/pl111_drm.h
index 9f2d30b52e7a..7a3d7af1c8cf 100644
--- a/drivers/gpu/drm/pl111/pl111_drm.h
+++ b/drivers/gpu/drm/pl111/pl111_drm.h
@@ -66,6 +66,7 @@ struct pl111_drm_dev_private {
 	struct drm_fbdev_cma *fbdev;
 
 	void *regs;
+	u32 memory_bw;
 	u32 ienb;
 	u32 ctrl;
 	/* The pixel clock (a reference to our clock divider off of CLCDCLK). */
diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
index f5d5aa464ae2..0077059c897f 100644
--- a/drivers/gpu/drm/pl111/pl111_drv.c
+++ b/drivers/gpu/drm/pl111/pl111_drv.c
@@ -274,6 +274,12 @@ static int pl111_amba_probe(struct amba_device *amba_dev,
 	drm->dev_private = priv;
 	priv->variant = variant;
 
+	if (of_property_read_u32(dev->of_node, "max-memory-bandwidth",
+				 &priv->memory_bw)) {
+		dev_info(dev, "no max memory bandwidth specified, assume unlimited\n");
+		priv->memory_bw = 0;
+	}
+
 	/* The two variants swap this register */
 	if (variant->is_pl110) {
 		priv->ienb = CLCD_PL110_IENB;
-- 
2.14.3

[PATCH 3/3] drm/pl111: Use max memory bandwidth for resolution

[Linus Walleij]

We were previously selecting 1024x768 and 32BPP as the default
set-up for the PL111 consumers.

This does not work on elder systems: the device tree bindings
support a property "max-memory-bandwidth" in bytes/second that
states that if you exceed this the memory bus will saturate.
The result is flickering and unstable images.

Parse the "max-memory-bandwidth" and respect it when
intializing the driver. On the RealView PB11MP, Versatile and
Integrator/CP we get a nice console as default with this code.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Exploit the new .mode_valid() callback we added to the
  simple KMS helper.
- Use the hardcoded bits per pixel per variant instead of
  trying to be heuristic about this setting for now.
---
 drivers/gpu/drm/pl111/pl111_display.c | 30 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/pl111/pl111_drm.h     |  1 +
 drivers/gpu/drm/pl111/pl111_drv.c     |  6 ++++++
 3 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
index d75923896609..a1ca9e1ffe15 100644
--- a/drivers/gpu/drm/pl111/pl111_display.c
+++ b/drivers/gpu/drm/pl111/pl111_display.c
@@ -50,6 +50,35 @@ irqreturn_t pl111_irq(int irq, void *data)
 	return status;
 }
 
+static enum drm_mode_status
+pl111_mode_valid(struct drm_crtc *crtc,
+		 const struct drm_display_mode *mode)
+{
+	struct drm_device *drm = crtc->dev;
+	struct pl111_drm_dev_private *priv = drm->dev_private;
+	u32 cpp = priv->variant->fb_bpp / 8;
+	u64 bw;
+
+	/*
+	 * We use the pixelclock to also account for interlaced modes, the
+	 * resulting bandwidth is in bytes per second.
+	 */
+	bw = mode->clock * 1000; /* In Hz */
+	bw = bw * mode->hdisplay * mode->vdisplay * cpp;
+	bw = div_u64(bw, mode->htotal * mode->vtotal);
+
+	if (bw > priv->memory_bw) {
+		DRM_DEBUG("%d x %d @ %d Hz, %d cpp, bw %llu too fast\n",
+			  mode->hdisplay, mode->vdisplay, mode->clock, cpp, bw);
+
+		return MODE_BAD;
+	}
+	DRM_DEBUG("%d x %d @ %d Hz, %d cpp, bw %llu bytes/s OK\n",
+		  mode->hdisplay, mode->vdisplay, mode->clock, cpp, bw);
+
+	return MODE_OK;
+}
+
 static int pl111_display_check(struct drm_simple_display_pipe *pipe,
 			       struct drm_plane_state *pstate,
 			       struct drm_crtc_state *cstate)
@@ -344,6 +373,7 @@ static int pl111_display_prepare_fb(struct drm_simple_display_pipe *pipe,
 }
 
 static const struct drm_simple_display_pipe_funcs pl111_display_funcs = {
+	.mode_valid = pl111_mode_valid,
 	.check = pl111_display_check,
 	.enable = pl111_display_enable,
 	.disable = pl111_display_disable,
diff --git a/drivers/gpu/drm/pl111/pl111_drm.h b/drivers/gpu/drm/pl111/pl111_drm.h
index 9f2d30b52e7a..7a3d7af1c8cf 100644
--- a/drivers/gpu/drm/pl111/pl111_drm.h
+++ b/drivers/gpu/drm/pl111/pl111_drm.h
@@ -66,6 +66,7 @@ struct pl111_drm_dev_private {
 	struct drm_fbdev_cma *fbdev;
 
 	void *regs;
+	u32 memory_bw;
 	u32 ienb;
 	u32 ctrl;
 	/* The pixel clock (a reference to our clock divider off of CLCDCLK). */
diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
index f5d5aa464ae2..0077059c897f 100644
--- a/drivers/gpu/drm/pl111/pl111_drv.c
+++ b/drivers/gpu/drm/pl111/pl111_drv.c
@@ -274,6 +274,12 @@ static int pl111_amba_probe(struct amba_device *amba_dev,
 	drm->dev_private = priv;
 	priv->variant = variant;
 
+	if (of_property_read_u32(dev->of_node, "max-memory-bandwidth",
+				 &priv->memory_bw)) {
+		dev_info(dev, "no max memory bandwidth specified, assume unlimited\n");
+		priv->memory_bw = 0;
+	}
+
 	/* The two variants swap this register */
 	if (variant->is_pl110) {
 		priv->ienb = CLCD_PL110_IENB;
-- 
2.14.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm: simple_kms_helper: Add mode_valid() callback support

[Eric Anholt]

Linus Walleij <linus.walleij@linaro.org> writes:

> The PL111 needs to filter valid modes based on memory bandwidth.
> I guess it is a pretty simple operation, so we can still claim
> the DRM KMS helper pipeline is simple after adding this (optional)
> vtable callback.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpu/drm/drm_simple_kms_helper.c | 15 +++++++++++++++
>  include/drm/drm_simple_kms_helper.h     | 15 +++++++++++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index dc9fd109de14..b7840cf34808 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -34,6 +34,20 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
>  	.destroy = drm_encoder_cleanup,
>  };
>  
> +static enum drm_mode_status
> +drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
> +			       const struct drm_display_mode *mode)
> +{
> +	struct drm_simple_display_pipe *pipe;
> +
> +	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
> +	if (!pipe->funcs || !pipe->funcs->mode_valid)
> +		/* Anything goes */
> +		return MODE_OK;
> +
> +	return pipe->funcs->mode_valid(crtc, mode);
> +}
> +
>  static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
>  				     struct drm_crtc_state *state)
>  {
> @@ -72,6 +86,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc,
>  }
>  
>  static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
> +	.mode_valid = drm_simple_kms_crtc_mode_valid,
>  	.atomic_check = drm_simple_kms_crtc_check,
>  	.atomic_enable = drm_simple_kms_crtc_enable,
>  	.atomic_disable = drm_simple_kms_crtc_disable,
> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> index 6d9adbb46293..ad74cb33c539 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -21,6 +21,21 @@ struct drm_simple_display_pipe;
>   *                                        display pipeline
>   */
>  struct drm_simple_display_pipe_funcs {
> +	/**
> +	 * @mode_valid:
> +	 *
> +	 * This function is called to filter out valid modes from the
> +	 * suggestions suggested by the bridge or display. This optional
> +	 * hook is passed in when initializing the pipeline.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * MODE_OK if the mode is acceptable.
> +	 * MODE_BAD if we need to try something else.
> +	 */

I don't see why MODE_BAD would be the only valid error return from this
hook.  Can we just use the same RETURNS docs as other mode_valid methods?

Other than that,

Reviewed-by: Eric Anholt <eric@anholt.net>

> +	enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
> +					   const struct drm_display_mode *mode);
> +
>  	/**
>  	 * @enable:
>  	 *
> -- 
> 2.14.3
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180210/46e6f7bb/attachment.sig>

Re: [PATCH 1/3] drm: simple_kms_helper: Add mode_valid() callback support

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2965 bytes --]

Linus Walleij <linus.walleij@linaro.org> writes:

> The PL111 needs to filter valid modes based on memory bandwidth.
> I guess it is a pretty simple operation, so we can still claim
> the DRM KMS helper pipeline is simple after adding this (optional)
> vtable callback.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpu/drm/drm_simple_kms_helper.c | 15 +++++++++++++++
>  include/drm/drm_simple_kms_helper.h     | 15 +++++++++++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> index dc9fd109de14..b7840cf34808 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -34,6 +34,20 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
>  	.destroy = drm_encoder_cleanup,
>  };
>  
> +static enum drm_mode_status
> +drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
> +			       const struct drm_display_mode *mode)
> +{
> +	struct drm_simple_display_pipe *pipe;
> +
> +	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
> +	if (!pipe->funcs || !pipe->funcs->mode_valid)
> +		/* Anything goes */
> +		return MODE_OK;
> +
> +	return pipe->funcs->mode_valid(crtc, mode);
> +}
> +
>  static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
>  				     struct drm_crtc_state *state)
>  {
> @@ -72,6 +86,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc,
>  }
>  
>  static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
> +	.mode_valid = drm_simple_kms_crtc_mode_valid,
>  	.atomic_check = drm_simple_kms_crtc_check,
>  	.atomic_enable = drm_simple_kms_crtc_enable,
>  	.atomic_disable = drm_simple_kms_crtc_disable,
> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> index 6d9adbb46293..ad74cb33c539 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -21,6 +21,21 @@ struct drm_simple_display_pipe;
>   *                                        display pipeline
>   */
>  struct drm_simple_display_pipe_funcs {
> +	/**
> +	 * @mode_valid:
> +	 *
> +	 * This function is called to filter out valid modes from the
> +	 * suggestions suggested by the bridge or display. This optional
> +	 * hook is passed in when initializing the pipeline.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * MODE_OK if the mode is acceptable.
> +	 * MODE_BAD if we need to try something else.
> +	 */

I don't see why MODE_BAD would be the only valid error return from this
hook.  Can we just use the same RETURNS docs as other mode_valid methods?

Other than that,

Reviewed-by: Eric Anholt <eric@anholt.net>

> +	enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
> +					   const struct drm_display_mode *mode);
> +
>  	/**
>  	 * @enable:
>  	 *
> -- 
> 2.14.3

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/pl111: Make the default BPP a per-variant variable

[Eric Anholt]

Linus Walleij <linus.walleij@linaro.org> writes:

> The PL110, Integrator and Versatile boards strongly prefer to
> use 16 BPP even if other modes are supported, both to keep down
> memory consumption and also to easier find a good match to
> supported resolutions with consideration taken to the memory
> bandwidth of the platforms.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

This seems like a reasonable compromise in complexity to get the default
fb up.

Reviewed-by: Eric Anholt <eric@anholt.net>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180210/332a7256/attachment-0001.sig>

Re: [PATCH 2/3] drm/pl111: Make the default BPP a per-variant variable

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 519 bytes --]

Linus Walleij <linus.walleij@linaro.org> writes:

> The PL110, Integrator and Versatile boards strongly prefer to
> use 16 BPP even if other modes are supported, both to keep down
> memory consumption and also to easier find a good match to
> supported resolutions with consideration taken to the memory
> bandwidth of the platforms.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

This seems like a reasonable compromise in complexity to get the default
fb up.

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/pl111: Use max memory bandwidth for resolution

[Eric Anholt]

Linus Walleij <linus.walleij@linaro.org> writes:

> We were previously selecting 1024x768 and 32BPP as the default
> set-up for the PL111 consumers.
>
> This does not work on elder systems: the device tree bindings
> support a property "max-memory-bandwidth" in bytes/second that
> states that if you exceed this the memory bus will saturate.
> The result is flickering and unstable images.
>
> Parse the "max-memory-bandwidth" and respect it when
> intializing the driver. On the RealView PB11MP, Versatile and
> Integrator/CP we get a nice console as default with this code.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Exploit the new .mode_valid() callback we added to the
>   simple KMS helper.
> - Use the hardcoded bits per pixel per variant instead of
>   trying to be heuristic about this setting for now.
> ---
>  drivers/gpu/drm/pl111/pl111_display.c | 30 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/pl111/pl111_drm.h     |  1 +
>  drivers/gpu/drm/pl111/pl111_drv.c     |  6 ++++++
>  3 files changed, 37 insertions(+)
>
> diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
> index d75923896609..a1ca9e1ffe15 100644
> --- a/drivers/gpu/drm/pl111/pl111_display.c
> +++ b/drivers/gpu/drm/pl111/pl111_display.c
> @@ -50,6 +50,35 @@ irqreturn_t pl111_irq(int irq, void *data)
>  	return status;
>  }
>  
> +static enum drm_mode_status
> +pl111_mode_valid(struct drm_crtc *crtc,
> +		 const struct drm_display_mode *mode)
> +{
> +	struct drm_device *drm = crtc->dev;
> +	struct pl111_drm_dev_private *priv = drm->dev_private;
> +	u32 cpp = priv->variant->fb_bpp / 8;
> +	u64 bw;

Using the variant->fb_bpp for mode_valid checks here feels wrong to me
-- it means a larger mode wouldn't be considered valid on a
32bpp-preferred platform when 16bpp would make it work, and a 16bpp
platform will happily try to set a 32bpp mode that exceeds the
bandwidth.

On the other hand, if it makes things work most of the time I'm also
kind of OK with it.  Anyone else want to chime in here?

> +	/*
> +	 * We use the pixelclock to also account for interlaced modes, the
> +	 * resulting bandwidth is in bytes per second.
> +	 */
> +	bw = mode->clock * 1000; /* In Hz */
> +	bw = bw * mode->hdisplay * mode->vdisplay * cpp;
> +	bw = div_u64(bw, mode->htotal * mode->vtotal);
> +
> +	if (bw > priv->memory_bw) {
> +		DRM_DEBUG("%d x %d @ %d Hz, %d cpp, bw %llu too fast\n",
> +			  mode->hdisplay, mode->vdisplay, mode->clock, cpp, bw);
> +
> +		return MODE_BAD;
> +	}
> +	DRM_DEBUG("%d x %d @ %d Hz, %d cpp, bw %llu bytes/s OK\n",
> +		  mode->hdisplay, mode->vdisplay, mode->clock, cpp, bw);
> +
> +	return MODE_OK;
> +}

Maybe DRM_DEBUG_KMS for these?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180210/4df74e8d/attachment.sig>

Re: [PATCH 3/3] drm/pl111: Use max memory bandwidth for resolution

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2812 bytes --]

Linus Walleij <linus.walleij@linaro.org> writes:

> We were previously selecting 1024x768 and 32BPP as the default
> set-up for the PL111 consumers.
>
> This does not work on elder systems: the device tree bindings
> support a property "max-memory-bandwidth" in bytes/second that
> states that if you exceed this the memory bus will saturate.
> The result is flickering and unstable images.
>
> Parse the "max-memory-bandwidth" and respect it when
> intializing the driver. On the RealView PB11MP, Versatile and
> Integrator/CP we get a nice console as default with this code.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Exploit the new .mode_valid() callback we added to the
>   simple KMS helper.
> - Use the hardcoded bits per pixel per variant instead of
>   trying to be heuristic about this setting for now.
> ---
>  drivers/gpu/drm/pl111/pl111_display.c | 30 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/pl111/pl111_drm.h     |  1 +
>  drivers/gpu/drm/pl111/pl111_drv.c     |  6 ++++++
>  3 files changed, 37 insertions(+)
>
> diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
> index d75923896609..a1ca9e1ffe15 100644
> --- a/drivers/gpu/drm/pl111/pl111_display.c
> +++ b/drivers/gpu/drm/pl111/pl111_display.c
> @@ -50,6 +50,35 @@ irqreturn_t pl111_irq(int irq, void *data)
>  	return status;
>  }
>  
> +static enum drm_mode_status
> +pl111_mode_valid(struct drm_crtc *crtc,
> +		 const struct drm_display_mode *mode)
> +{
> +	struct drm_device *drm = crtc->dev;
> +	struct pl111_drm_dev_private *priv = drm->dev_private;
> +	u32 cpp = priv->variant->fb_bpp / 8;
> +	u64 bw;

Using the variant->fb_bpp for mode_valid checks here feels wrong to me
-- it means a larger mode wouldn't be considered valid on a
32bpp-preferred platform when 16bpp would make it work, and a 16bpp
platform will happily try to set a 32bpp mode that exceeds the
bandwidth.

On the other hand, if it makes things work most of the time I'm also
kind of OK with it.  Anyone else want to chime in here?

> +	/*
> +	 * We use the pixelclock to also account for interlaced modes, the
> +	 * resulting bandwidth is in bytes per second.
> +	 */
> +	bw = mode->clock * 1000; /* In Hz */
> +	bw = bw * mode->hdisplay * mode->vdisplay * cpp;
> +	bw = div_u64(bw, mode->htotal * mode->vtotal);
> +
> +	if (bw > priv->memory_bw) {
> +		DRM_DEBUG("%d x %d @ %d Hz, %d cpp, bw %llu too fast\n",
> +			  mode->hdisplay, mode->vdisplay, mode->clock, cpp, bw);
> +
> +		return MODE_BAD;
> +	}
> +	DRM_DEBUG("%d x %d @ %d Hz, %d cpp, bw %llu bytes/s OK\n",
> +		  mode->hdisplay, mode->vdisplay, mode->clock, cpp, bw);
> +
> +	return MODE_OK;
> +}

Maybe DRM_DEBUG_KMS for these?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/pl111: Use max memory bandwidth for resolution

[Linus Walleij]

On Sat, Feb 10, 2018 at 6:14 PM, Eric Anholt <eric@anholt.net> wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:

>> +static enum drm_mode_status
>> +pl111_mode_valid(struct drm_crtc *crtc,
>> +              const struct drm_display_mode *mode)
>> +{
>> +     struct drm_device *drm = crtc->dev;
>> +     struct pl111_drm_dev_private *priv = drm->dev_private;
>> +     u32 cpp = priv->variant->fb_bpp / 8;
>> +     u64 bw;
>
> Using the variant->fb_bpp for mode_valid checks here feels wrong to me
> -- it means a larger mode wouldn't be considered valid on a
> 32bpp-preferred platform when 16bpp would make it work, and a 16bpp
> platform will happily try to set a 32bpp mode that exceeds the
> bandwidth.

Yeah. So I have an additional patch (that I will send out with the
rest of the v2 patches) that actually go in and set ->fb_bpp to
16 on the RealViews to get some nice 1024x768 on these.

The other mode_valid() checks I've seen (well, OMAPDRM)
just assume 32bpp and goes on. I guess it is saved by not
supporting anything else.

> On the other hand, if it makes things work most of the time I'm also
> kind of OK with it.  Anyone else want to chime in here?

This makes things work but can be improved upon.

The core of the problem is that resolution is CRTC business
and seen as something limited by the monitor or
bridge/connector or so, then it calls this hook into the driver to
check that it can provide what the monitor/bridge/connector
suggests.

And then we have the opportunity to say we can't do such or
such resolution.

The pixel format on the other hand is seen as a display hardware
thing, so if I had an API here to call back into the driver and
restrict the formats to say 16bpp, I could confirm better
resolutions.

We can put that in place later and remove the ->fb_bpp if we
can chisel out the right API to set the pixel format.

I am working on another patch to set the preferred pixel format
instead of BPP when intiializing the framebuffer, so I will
investigate this at the same time, but I'd really like to build
on top of this code to avoid too many variables.

> Maybe DRM_DEBUG_KMS for these?

OK I fix :)

Yours,
Linus Walleij

Re: [PATCH 3/3] drm/pl111: Use max memory bandwidth for resolution

[Linus Walleij]

On Sat, Feb 10, 2018 at 6:14 PM, Eric Anholt <eric@anholt.net> wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:

>> +static enum drm_mode_status
>> +pl111_mode_valid(struct drm_crtc *crtc,
>> +              const struct drm_display_mode *mode)
>> +{
>> +     struct drm_device *drm = crtc->dev;
>> +     struct pl111_drm_dev_private *priv = drm->dev_private;
>> +     u32 cpp = priv->variant->fb_bpp / 8;
>> +     u64 bw;
>
> Using the variant->fb_bpp for mode_valid checks here feels wrong to me
> -- it means a larger mode wouldn't be considered valid on a
> 32bpp-preferred platform when 16bpp would make it work, and a 16bpp
> platform will happily try to set a 32bpp mode that exceeds the
> bandwidth.

Yeah. So I have an additional patch (that I will send out with the
rest of the v2 patches) that actually go in and set ->fb_bpp to
16 on the RealViews to get some nice 1024x768 on these.

The other mode_valid() checks I've seen (well, OMAPDRM)
just assume 32bpp and goes on. I guess it is saved by not
supporting anything else.

> On the other hand, if it makes things work most of the time I'm also
> kind of OK with it.  Anyone else want to chime in here?

This makes things work but can be improved upon.

The core of the problem is that resolution is CRTC business
and seen as something limited by the monitor or
bridge/connector or so, then it calls this hook into the driver to
check that it can provide what the monitor/bridge/connector
suggests.

And then we have the opportunity to say we can't do such or
such resolution.

The pixel format on the other hand is seen as a display hardware
thing, so if I had an API here to call back into the driver and
restrict the formats to say 16bpp, I could confirm better
resolutions.

We can put that in place later and remove the ->fb_bpp if we
can chisel out the right API to set the pixel format.

I am working on another patch to set the preferred pixel format
instead of BPP when intiializing the framebuffer, so I will
investigate this at the same time, but I'd really like to build
on top of this code to avoid too many variables.

> Maybe DRM_DEBUG_KMS for these?

OK I fix :)

Yours,
Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm: simple_kms_helper: Add mode_valid() callback support

[Daniel Vetter]

On Sat, Feb 10, 2018 at 05:08:34PM +0000, Eric Anholt wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:
> 
> > The PL111 needs to filter valid modes based on memory bandwidth.
> > I guess it is a pretty simple operation, so we can still claim
> > the DRM KMS helper pipeline is simple after adding this (optional)
> > vtable callback.
> >
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> >  drivers/gpu/drm/drm_simple_kms_helper.c | 15 +++++++++++++++
> >  include/drm/drm_simple_kms_helper.h     | 15 +++++++++++++++
> >  2 files changed, 30 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> > index dc9fd109de14..b7840cf34808 100644
> > --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > @@ -34,6 +34,20 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
> >  	.destroy = drm_encoder_cleanup,
> >  };
> >  
> > +static enum drm_mode_status
> > +drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
> > +			       const struct drm_display_mode *mode)
> > +{
> > +	struct drm_simple_display_pipe *pipe;
> > +
> > +	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
> > +	if (!pipe->funcs || !pipe->funcs->mode_valid)
> > +		/* Anything goes */
> > +		return MODE_OK;
> > +
> > +	return pipe->funcs->mode_valid(crtc, mode);
> > +}
> > +
> >  static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
> >  				     struct drm_crtc_state *state)
> >  {
> > @@ -72,6 +86,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc,
> >  }
> >  
> >  static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
> > +	.mode_valid = drm_simple_kms_crtc_mode_valid,
> >  	.atomic_check = drm_simple_kms_crtc_check,
> >  	.atomic_enable = drm_simple_kms_crtc_enable,
> >  	.atomic_disable = drm_simple_kms_crtc_disable,
> > diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> > index 6d9adbb46293..ad74cb33c539 100644
> > --- a/include/drm/drm_simple_kms_helper.h
> > +++ b/include/drm/drm_simple_kms_helper.h
> > @@ -21,6 +21,21 @@ struct drm_simple_display_pipe;
> >   *                                        display pipeline
> >   */
> >  struct drm_simple_display_pipe_funcs {
> > +	/**
> > +	 * @mode_valid:
> > +	 *
> > +	 * This function is called to filter out valid modes from the
> > +	 * suggestions suggested by the bridge or display. This optional
> > +	 * hook is passed in when initializing the pipeline.
> > +	 *
> > +	 * RETURNS:
> > +	 *
> > +	 * MODE_OK if the mode is acceptable.
> > +	 * MODE_BAD if we need to try something else.
> > +	 */
> 
> I don't see why MODE_BAD would be the only valid error return from this
> hook.  Can we just use the same RETURNS docs as other mode_valid methods?
> 
> Other than that,
> 
> Reviewed-by: Eric Anholt <eric@anholt.net>

Same comment (although note that except for dmesg noise we currently throw
them away), and I agree that mode_valid makes sense for simple drivers,
especially with all the refactoring we've done to call mode_valid both in
the connector probe and atomic_check paths. On the respun patch (I'm
behind on mails ...):

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
> > +	enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
> > +					   const struct drm_display_mode *mode);
> > +
> >  	/**
> >  	 * @enable:
> >  	 *
> > -- 
> > 2.14.3



> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


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

Re: [PATCH 1/3] drm: simple_kms_helper: Add mode_valid() callback support

[Daniel Vetter]

On Sat, Feb 10, 2018 at 05:08:34PM +0000, Eric Anholt wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:
> 
> > The PL111 needs to filter valid modes based on memory bandwidth.
> > I guess it is a pretty simple operation, so we can still claim
> > the DRM KMS helper pipeline is simple after adding this (optional)
> > vtable callback.
> >
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> >  drivers/gpu/drm/drm_simple_kms_helper.c | 15 +++++++++++++++
> >  include/drm/drm_simple_kms_helper.h     | 15 +++++++++++++++
> >  2 files changed, 30 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
> > index dc9fd109de14..b7840cf34808 100644
> > --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > @@ -34,6 +34,20 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = {
> >  	.destroy = drm_encoder_cleanup,
> >  };
> >  
> > +static enum drm_mode_status
> > +drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
> > +			       const struct drm_display_mode *mode)
> > +{
> > +	struct drm_simple_display_pipe *pipe;
> > +
> > +	pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
> > +	if (!pipe->funcs || !pipe->funcs->mode_valid)
> > +		/* Anything goes */
> > +		return MODE_OK;
> > +
> > +	return pipe->funcs->mode_valid(crtc, mode);
> > +}
> > +
> >  static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
> >  				     struct drm_crtc_state *state)
> >  {
> > @@ -72,6 +86,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc,
> >  }
> >  
> >  static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
> > +	.mode_valid = drm_simple_kms_crtc_mode_valid,
> >  	.atomic_check = drm_simple_kms_crtc_check,
> >  	.atomic_enable = drm_simple_kms_crtc_enable,
> >  	.atomic_disable = drm_simple_kms_crtc_disable,
> > diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> > index 6d9adbb46293..ad74cb33c539 100644
> > --- a/include/drm/drm_simple_kms_helper.h
> > +++ b/include/drm/drm_simple_kms_helper.h
> > @@ -21,6 +21,21 @@ struct drm_simple_display_pipe;
> >   *                                        display pipeline
> >   */
> >  struct drm_simple_display_pipe_funcs {
> > +	/**
> > +	 * @mode_valid:
> > +	 *
> > +	 * This function is called to filter out valid modes from the
> > +	 * suggestions suggested by the bridge or display. This optional
> > +	 * hook is passed in when initializing the pipeline.
> > +	 *
> > +	 * RETURNS:
> > +	 *
> > +	 * MODE_OK if the mode is acceptable.
> > +	 * MODE_BAD if we need to try something else.
> > +	 */
> 
> I don't see why MODE_BAD would be the only valid error return from this
> hook.  Can we just use the same RETURNS docs as other mode_valid methods?
> 
> Other than that,
> 
> Reviewed-by: Eric Anholt <eric@anholt.net>

Same comment (although note that except for dmesg noise we currently throw
them away), and I agree that mode_valid makes sense for simple drivers,
especially with all the refactoring we've done to call mode_valid both in
the connector probe and atomic_check paths. On the respun patch (I'm
behind on mails ...):

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
> > +	enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
> > +					   const struct drm_display_mode *mode);
> > +
> >  	/**
> >  	 * @enable:
> >  	 *
> > -- 
> > 2.14.3



> _______________________________________________
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/pl111: Use max memory bandwidth for resolution

[Eric Anholt]

Linus Walleij <linus.walleij@linaro.org> writes:

> We were previously selecting 1024x768 and 32BPP as the default
> set-up for the PL111 consumers.
>
> This does not work on elder systems: the device tree bindings
> support a property "max-memory-bandwidth" in bytes/second that
> states that if you exceed this the memory bus will saturate.
> The result is flickering and unstable images.
>
> Parse the "max-memory-bandwidth" and respect it when
> intializing the driver. On the RealView PB11MP, Versatile and
> Integrator/CP we get a nice console as default with this code.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Exploit the new .mode_valid() callback we added to the
>   simple KMS helper.
> - Use the hardcoded bits per pixel per variant instead of
>   trying to be heuristic about this setting for now.
> ---
>  drivers/gpu/drm/pl111/pl111_display.c | 30 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/pl111/pl111_drm.h     |  1 +
>  drivers/gpu/drm/pl111/pl111_drv.c     |  6 ++++++
>  3 files changed, 37 insertions(+)
>
> diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
> index d75923896609..a1ca9e1ffe15 100644
> --- a/drivers/gpu/drm/pl111/pl111_display.c
> +++ b/drivers/gpu/drm/pl111/pl111_display.c

> +	if (bw > priv->memory_bw) {
> +		DRM_DEBUG("%d x %d @ %d Hz, %d cpp, bw %llu too fast\n",
> +			  mode->hdisplay, mode->vdisplay, mode->clock, cpp, bw);
> +
> +		return MODE_BAD;
> +	}

Oh, hey, on platforms with no memory-bandwidth property, we should make
sure not to reject all modes :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180222/15494b96/attachment.sig>

Re: [PATCH 3/3] drm/pl111: Use max memory bandwidth for resolution

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1605 bytes --]

Linus Walleij <linus.walleij@linaro.org> writes:

> We were previously selecting 1024x768 and 32BPP as the default
> set-up for the PL111 consumers.
>
> This does not work on elder systems: the device tree bindings
> support a property "max-memory-bandwidth" in bytes/second that
> states that if you exceed this the memory bus will saturate.
> The result is flickering and unstable images.
>
> Parse the "max-memory-bandwidth" and respect it when
> intializing the driver. On the RealView PB11MP, Versatile and
> Integrator/CP we get a nice console as default with this code.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Exploit the new .mode_valid() callback we added to the
>   simple KMS helper.
> - Use the hardcoded bits per pixel per variant instead of
>   trying to be heuristic about this setting for now.
> ---
>  drivers/gpu/drm/pl111/pl111_display.c | 30 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/pl111/pl111_drm.h     |  1 +
>  drivers/gpu/drm/pl111/pl111_drv.c     |  6 ++++++
>  3 files changed, 37 insertions(+)
>
> diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
> index d75923896609..a1ca9e1ffe15 100644
> --- a/drivers/gpu/drm/pl111/pl111_display.c
> +++ b/drivers/gpu/drm/pl111/pl111_display.c

> +	if (bw > priv->memory_bw) {
> +		DRM_DEBUG("%d x %d @ %d Hz, %d cpp, bw %llu too fast\n",
> +			  mode->hdisplay, mode->vdisplay, mode->clock, cpp, bw);
> +
> +		return MODE_BAD;
> +	}

Oh, hey, on platforms with no memory-bandwidth property, we should make
sure not to reject all modes :)

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel