[PATCH 00/21] drm/omap: misc fixes/improvements

[PATCH 00/21] drm/omap: misc fixes/improvements

[Tomi Valkeinen]

Various small(ish) fixes and improvements for omapdrm.

Laurent, some of these may conflict with your work. I'm fine with dropping the
conflicting ones from this series, as long as the issue has been fixed in your
WIP branch. Or maybe some of these should be rebased on top of your work.

 Tomi

Tomi Valkeinen (21):
  drm/omap: fix encoder-crtc mapping
  drm/omap: page_flip: return -EBUSY if flip pending
  drm/omap: clear omap_obj->paddr in omap_gem_put_paddr()
  drm/omap: add pin refcounting to omap_framebuffer
  drm/omap: add a comment why locking is missing
  drm/omap: check CRTC color format earlier
  drm/omap: fix operation without fbdev
  drm/omap: fix error handling in omap_framebuffer_create()
  drm/omap: handle mismatching color format and buffer width
  drm/omap: fix TILER on OMAP5
  drm/omap: fix plane's channel selection
  drm/omap: tiler: fix race condition with engine->async
  drm/omap: remove dummy PM functions
  drm/omap: stop connector polling during suspend
  drm/omap: use DRM_ERROR_RATELIMITED() for error irqs
  drm/omap: fix race with error_irq
  drm/omap: only ignore DIGIT SYNC LOST for TV output
  drm/omap: do not use BUG_ON(!spin_is_locked(x))
  drm/omap: fix race condition with dev->obj_list
  drm/omap: fix race conditon in DMM
  drm/omap: keep ref to old_fb

 drivers/gpu/drm/omapdrm/omap_crtc.c      | 44 +++++++++++++++--------
 drivers/gpu/drm/omapdrm/omap_dmm_priv.h  |  8 ++++-
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 54 +++++++++++++++++++++++-----
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.h |  1 +
 drivers/gpu/drm/omapdrm/omap_drv.c       | 60 +++++++++++++++++---------------
 drivers/gpu/drm/omapdrm/omap_drv.h       |  3 ++
 drivers/gpu/drm/omapdrm/omap_fb.c        | 26 ++++++++++++--
 drivers/gpu/drm/omapdrm/omap_gem.c       | 10 ++++--
 drivers/gpu/drm/omapdrm/omap_irq.c       |  2 +-
 drivers/gpu/drm/omapdrm/omap_plane.c     | 28 +++++++++++++--
 10 files changed, 176 insertions(+), 60 deletions(-)

-- 
2.3.0

[PATCH 01/21] drm/omap: fix encoder-crtc mapping

[Tomi Valkeinen]

OMAP DSS hardware supports changing the output port to which an overlay
manager's video stream goes. For example, DPI video stream can come from
any of the four overlay managers on OMAP5.

However, as it's difficult to manage the change in the driver, the
omapdss driver does not support that at the moment, and has a hardcoded
overlay manager per output.

omapdrm, on the other hand, uses the hardware features to find out which
overlay manager to use for an output, which causes problems. For
example, on OMAP5, omapdrm tries to use DIGIT overlay manager for DPI
output, instead of the LCD3 required by the omapdss driver.

This patch changes the omapdrm to use the omapdss driver's hardcoded
overlay managers, which fixes the issue.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 8241ed9b353c..95e0356bf047 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -286,14 +286,13 @@ static int omap_modeset_init(struct drm_device *dev)
 		for (id = 0; id < priv->num_crtcs; id++) {
 			struct drm_crtc *crtc = priv->crtcs[id];
 			enum omap_channel crtc_channel;
-			enum omap_dss_output_id supported_outputs;
 
 			crtc_channel = omap_crtc_channel(crtc);
-			supported_outputs =
-				dss_feat_get_supported_outputs(crtc_channel);
 
-			if (supported_outputs & output->id)
+			if (output->dispc_channel == crtc_channel) {
 				encoder->possible_crtcs |= (1 << id);
+				break;
+			}
 		}
 
 		omap_dss_put_device(output);
-- 
2.3.0

[PATCH 02/21] drm/omap: page_flip: return -EBUSY if flip pending

[Tomi Valkeinen]

The DRM documentation says:

"If a page flip is already pending, the page_flip operation must return
-EBUSY."

Currently omapdrm returns -EINVAL instead. Fix omapdrm by returning
-EBUSY.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index b0566a1ca28f..af4b230b5269 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -361,7 +361,7 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc,
 	if (omap_crtc->old_fb) {
 		spin_unlock_irqrestore(&dev->event_lock, flags);
 		dev_err(dev->dev, "already a pending flip\n");
-		return -EINVAL;
+		return -EBUSY;
 	}
 
 	omap_crtc->event = event;
-- 
2.3.0

[PATCH 03/21] drm/omap: clear omap_obj->paddr in omap_gem_put_paddr()

[Tomi Valkeinen]

Clear omap_obj's paddr when unmapping the memory, so that it's easier to
catch bad use of the paddr.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_gem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index aeb91ed653c9..9b250c93b046 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -828,6 +828,7 @@ int omap_gem_put_paddr(struct drm_gem_object *obj)
 				dev_err(obj->dev->dev,
 					"could not release unmap: %d\n", ret);
 			}
+			omap_obj->paddr = 0;
 			omap_obj->block = NULL;
 		}
 	}
-- 
2.3.0

[PATCH 04/21] drm/omap: add pin refcounting to omap_framebuffer

[Tomi Valkeinen]

omap_framebuffer_pin() and omap_framebuffer_unpin() are currently
broken, as they cannot be called multiple times (i.e. pin, pin, unpin,
unpin), which is what happens in certain cases. This issue causes the
driver to possibly use 0 as an address for a displayed buffer, leading
to OCP error from DSS.

This patch fixes the issue by adding a simple pin_count, used to track
the number of pins.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_fb.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 2a5cacdc344b..58f2af32ede8 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -86,6 +86,7 @@ struct plane {
 
 struct omap_framebuffer {
 	struct drm_framebuffer base;
+	int pin_count;
 	const struct format *format;
 	struct plane planes[4];
 };
@@ -261,6 +262,11 @@ int omap_framebuffer_pin(struct drm_framebuffer *fb)
 	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
 	int ret, i, n = drm_format_num_planes(fb->pixel_format);
 
+	if (omap_fb->pin_count > 0) {
+		omap_fb->pin_count++;
+		return 0;
+	}
+
 	for (i = 0; i < n; i++) {
 		struct plane *plane = &omap_fb->planes[i];
 		ret = omap_gem_get_paddr(plane->bo, &plane->paddr, true);
@@ -269,6 +275,8 @@ int omap_framebuffer_pin(struct drm_framebuffer *fb)
 		omap_gem_dma_sync(plane->bo, DMA_TO_DEVICE);
 	}
 
+	omap_fb->pin_count++;
+
 	return 0;
 
 fail:
@@ -287,6 +295,11 @@ int omap_framebuffer_unpin(struct drm_framebuffer *fb)
 	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
 	int ret, i, n = drm_format_num_planes(fb->pixel_format);
 
+	omap_fb->pin_count--;
+
+	if (omap_fb->pin_count > 0)
+		return 0;
+
 	for (i = 0; i < n; i++) {
 		struct plane *plane = &omap_fb->planes[i];
 		ret = omap_gem_put_paddr(plane->bo);
-- 
2.3.0

[PATCH 05/21] drm/omap: add a comment why locking is missing

[Tomi Valkeinen]

unpin_worker() calls omap_framebuffer_unpin() without any locks, which
looks very suspicious. However, both pin and unpin are always called via
the driver's private workqueue, so the access is synchronized that way.

Add a comment to make this clear.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_plane.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index ee8e2b3a117e..1f4f2b866379 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -71,6 +71,10 @@ static void unpin_worker(struct drm_flip_work *work, void *val)
 			container_of(work, struct omap_plane, unpin_work);
 	struct drm_device *dev = omap_plane->base.dev;
 
+	/*
+	 * omap_framebuffer_pin/unpin are always called from priv->wq,
+	 * so there's no need for locking here.
+	 */
 	omap_framebuffer_unpin(val);
 	mutex_lock(&dev->mode_config.mutex);
 	drm_framebuffer_unreference(val);
-- 
2.3.0

[PATCH 06/21] drm/omap: check CRTC color format earlier

[Tomi Valkeinen]

When setting a color format to a DRM plane, the DRM core checks whether
the format is supported by the HW. However, it seems that when setting
the color format of a CRTC (i.e. a root plane), there's no checking
done. This causes omapdrm to configure omapdss with the bad color
format, which omapdss then rejects.

While the above is ok, the error message is a bit confusing, and the
configuring of omapdss happens asynchronously from the ioctl that does
the color format change.

This patch adds a color format check to omap_plane_mode_set() which
causes the ioctl to return an error for invalid color format. This means
that the userspace will get to know of the wrong setting, instead of the
current behavior where the error is not seen by the userspace.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_plane.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 1f4f2b866379..bedd6f7af0f1 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -207,6 +207,24 @@ int omap_plane_mode_set(struct drm_plane *plane,
 {
 	struct omap_plane *omap_plane = to_omap_plane(plane);
 	struct omap_drm_window *win = &omap_plane->win;
+	int i;
+
+	/*
+	 * Check whether this plane supports the fb pixel format.
+	 * I don't think this should really be needed, but it looks like the
+	 * drm core only checks the format for planes, not for the crtc. So
+	 * when setting the format for crtc, without this check we would
+	 * get an error later when trying to program the color format into the
+	 * HW.
+	 */
+	for (i = 0; i < plane->format_count; i++)
+		if (fb->pixel_format == plane->format_types[i])
+			break;
+	if (i == plane->format_count) {
+		DBG("Invalid pixel format %s",
+			      drm_get_format_name(fb->pixel_format));
+		return -EINVAL;
+	}
 
 	win->crtc_x = crtc_x;
 	win->crtc_y = crtc_y;
-- 
2.3.0

[PATCH 07/21] drm/omap: fix operation without fbdev

[Tomi Valkeinen]

omapdrm should work fine even if fbdev is missing. The current driver
crashes in that case, though, as it is missing checks for the fbdev.

Add the checks so that we don't free fbdev or restore fbdev mode when
there's no fbdev.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 95e0356bf047..87724fa818d0 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -518,7 +518,8 @@ static int dev_unload(struct drm_device *dev)
 
 	drm_kms_helper_poll_fini(dev);
 
-	omap_fbdev_free(dev);
+	if (priv->fbdev)
+		omap_fbdev_free(dev);
 
 	/* flush crtcs so the fbs get released */
 	for (i = 0; i < priv->num_crtcs; i++)
@@ -587,9 +588,11 @@ static void dev_lastclose(struct drm_device *dev)
 		}
 	}
 
-	ret = drm_fb_helper_restore_fbdev_mode_unlocked(priv->fbdev);
-	if (ret)
-		DBG("failed to restore crtc mode");
+	if (priv->fbdev) {
+		ret = drm_fb_helper_restore_fbdev_mode_unlocked(priv->fbdev);
+		if (ret)
+			DBG("failed to restore crtc mode");
+	}
 }
 
 static void dev_preclose(struct drm_device *dev, struct drm_file *file)
-- 
2.3.0

[PATCH 08/21] drm/omap: fix error handling in omap_framebuffer_create()

[Tomi Valkeinen]

When an error happens in omap_framebuffer_create(),
omap_framebuffer_create() calls omap_framebuffer_destroy() if the fb
struct has been allocated. However, that crashes, as
omap_framebuffer_destroy(), which calls drm_framebuffer_cleanup(),
should only be called after drm_framebuffer_init()

Fix this by just calling kfree() for the allocated fb when an error
happens.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_fb.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 58f2af32ede8..2975096abdf5 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -420,7 +420,7 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
 struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 		struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos)
 {
-	struct omap_framebuffer *omap_fb;
+	struct omap_framebuffer *omap_fb = NULL;
 	struct drm_framebuffer *fb = NULL;
 	const struct format *format = NULL;
 	int ret, i, n = drm_format_num_planes(mode_cmd->pixel_format);
@@ -491,8 +491,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 	return fb;
 
 fail:
-	if (fb)
-		omap_framebuffer_destroy(fb);
+	kfree(omap_fb);
 
 	return ERR_PTR(ret);
 }
-- 
2.3.0

[PATCH 09/21] drm/omap: handle mismatching color format and buffer width

[Tomi Valkeinen]

omapdrm doesn't check if the width of the framebuffer and the color
format's bits-per-pixel match.

For example, using a display with a width of 1280, and a buffer
allocated with using 32 bits per pixel (i.e. 1280*4 = 5120 bytes), with
a 24 bits per pixel color format, leads to the following mismatch:
5120/3 = 1706.666... bytes. This causes bad colors and a tilt on the
screen.

Add a check into omapdrm to return an error if the user tries to use
such a combination.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_fb.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 2975096abdf5..bf98580223d0 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 			goto fail;
 		}
 
+		if (mode_cmd->width % format->planes[i].stride_bpp != 0) {
+			dev_err(dev->dev,
+				"buffer width (%d) is not a multiple of pixel width (%d)\n",
+				mode_cmd->width, format->planes[i].stride_bpp);
+			ret = -EINVAL;
+			goto fail;
+		}
+
 		size = pitch * mode_cmd->height / format->planes[i].sub_y;
 
 		if (size > (omap_gem_mmap_size(bos[i]) - mode_cmd->offsets[i])) {
-- 
2.3.0

[PATCH 10/21] drm/omap: fix TILER on OMAP5

[Tomi Valkeinen]

On OMAP5 it is not possible to use TILER buffer with CPU when caching or
write-combining is used. Doing so leads to errors from the memory
manager.

However, on OMAP4, write-combining works fine.

This patch adds platform specific data for the TILER, and a function
tiler_get_cpu_cache_flags() which can be used to get the caching mode to
be used.

Note that without write-combining the use of the TILER buffer with CPU
is unusably slow. It's still good to have it operational for testing
purposes.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_dmm_priv.h  |  6 +++++
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 39 ++++++++++++++++++++++++++++++--
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.h |  1 +
 drivers/gpu/drm/omapdrm/omap_gem.c       |  4 ++--
 4 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_priv.h b/drivers/gpu/drm/omapdrm/omap_dmm_priv.h
index 58bcd6ae0255..d96660573076 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_priv.h
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_priv.h
@@ -153,6 +153,10 @@ struct refill_engine {
 	struct list_head idle_node;
 };
 
+struct dmm_platform_data {
+	uint32_t cpu_cache_flags;
+};
+
 struct dmm {
 	struct device *dev;
 	void __iomem *base;
@@ -183,6 +187,8 @@ struct dmm {
 
 	/* allocation list and lock */
 	struct list_head alloc_head;
+
+	const struct dmm_platform_data *plat_data;
 };
 
 #endif
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index 56c60552abba..ef3d14d9cfa1 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -39,6 +39,10 @@
 static struct tcm *containers[TILFMT_NFORMATS];
 static struct dmm *omap_dmm;
 
+#if defined(CONFIG_OF)
+static const struct of_device_id dmm_of_match[];
+#endif
+
 /* global spinlock for protecting lists */
 static DEFINE_SPINLOCK(list_lock);
 
@@ -529,6 +533,11 @@ size_t tiler_vsize(enum tiler_fmt fmt, uint16_t w, uint16_t h)
 	return round_up(geom[fmt].cpp * w, PAGE_SIZE) * h;
 }
 
+uint32_t tiler_get_cpu_cache_flags(void)
+{
+	return omap_dmm->plat_data->cpu_cache_flags;
+}
+
 bool dmm_is_available(void)
 {
 	return omap_dmm ? true : false;
@@ -592,6 +601,18 @@ static int omap_dmm_probe(struct platform_device *dev)
 
 	init_waitqueue_head(&omap_dmm->engine_queue);
 
+	if (dev->dev.of_node) {
+		const struct of_device_id *match;
+
+		match = of_match_node(dmm_of_match, dev->dev.of_node);
+		if (!match) {
+			dev_err(&dev->dev, "failed to find matching device node\n");
+			return -ENODEV;
+		}
+
+		omap_dmm->plat_data = match->data;
+	}
+
 	/* lookup hwmod data - base address and irq */
 	mem = platform_get_resource(dev, IORESOURCE_MEM, 0);
 	if (!mem) {
@@ -972,9 +993,23 @@ static const struct dev_pm_ops omap_dmm_pm_ops = {
 #endif
 
 #if defined(CONFIG_OF)
+static const struct dmm_platform_data dmm_omap4_platform_data = {
+	.cpu_cache_flags = OMAP_BO_WC,
+};
+
+static const struct dmm_platform_data dmm_omap5_platform_data = {
+	.cpu_cache_flags = OMAP_BO_UNCACHED,
+};
+
 static const struct of_device_id dmm_of_match[] = {
-	{ .compatible = "ti,omap4-dmm", },
-	{ .compatible = "ti,omap5-dmm", },
+	{
+		.compatible = "ti,omap4-dmm",
+		.data = &dmm_omap4_platform_data,
+	},
+	{
+		.compatible = "ti,omap5-dmm",
+		.data = &dmm_omap5_platform_data,
+	},
 	{},
 };
 #endif
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.h b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.h
index 4fdd61e54bd2..e83c78372db8 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.h
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.h
@@ -106,6 +106,7 @@ uint32_t tiler_stride(enum tiler_fmt fmt, uint32_t orient);
 size_t tiler_size(enum tiler_fmt fmt, uint16_t w, uint16_t h);
 size_t tiler_vsize(enum tiler_fmt fmt, uint16_t w, uint16_t h);
 void tiler_align(enum tiler_fmt fmt, uint16_t *w, uint16_t *h);
+uint32_t tiler_get_cpu_cache_flags(void);
 bool dmm_is_available(void);
 
 extern struct platform_driver omap_dmm_driver;
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 9b250c93b046..d37ee756a0b1 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -1359,8 +1359,8 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
 		/* currently don't allow cached buffers.. there is some caching
 		 * stuff that needs to be handled better
 		 */
-		flags &= ~(OMAP_BO_CACHED|OMAP_BO_UNCACHED);
-		flags |= OMAP_BO_WC;
+		flags &= ~(OMAP_BO_CACHED|OMAP_BO_WC|OMAP_BO_UNCACHED);
+		flags |= tiler_get_cpu_cache_flags();
 
 		/* align dimensions to slot boundaries... */
 		tiler_align(gem2fmt(flags),
-- 
2.3.0

[PATCH 11/21] drm/omap: fix plane's channel selection

[Tomi Valkeinen]

omap_plane_pre_apply() sets the plane's output channel too late, only
after the plane has already been otherwise configured and enabled. This
causes problems, as at the configuration stage we need to make decisions
based on the output channel.

This may lead to bad plane settings or failing to setup the plane.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_plane.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index bedd6f7af0f1..8e94694ba8a3 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -153,6 +153,8 @@ static void omap_plane_pre_apply(struct omap_drm_apply *apply)
 	ilace = false;
 	replication = false;
 
+	dispc_ovl_set_channel_out(omap_plane->id, channel);
+
 	/* and finally, update omapdss: */
 	ret = dispc_ovl_setup(omap_plane->id, info,
 			replication, omap_crtc_timings(crtc), false);
@@ -162,7 +164,6 @@ static void omap_plane_pre_apply(struct omap_drm_apply *apply)
 	}
 
 	dispc_ovl_enable(omap_plane->id, true);
-	dispc_ovl_set_channel_out(omap_plane->id, channel);
 }
 
 static void omap_plane_post_apply(struct omap_drm_apply *apply)
-- 
2.3.0

[PATCH 12/21] drm/omap: tiler: fix race condition with engine->async

[Tomi Valkeinen]

The tiler irq handler uses engine->async value, but the code that sets
engine->async and enables the interrupt does not have a barrier. This
may cause the irq handler to see the old value of engine->async, causing
memory corruption.

Reported-by: Harinarayan Bhatta <harinarayan@ti.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index ef3d14d9cfa1..c4563af55a82 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -273,6 +273,8 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool wait)
 
 	/* mark whether it is async to denote list management in IRQ handler */
 	engine->async = wait ? false : true;
+	/* verify that the irq handler sees the 'async' value */
+	smp_mb();
 
 	/* kick reload */
 	writel(engine->refill_pa,
-- 
2.3.0

[PATCH 13/21] drm/omap: remove dummy PM functions

[Tomi Valkeinen]

omapdrm has dummy functions for platform_device's
suspend/resume/shutdown. The functions don't do anything, and those
platform device functions are deprecated, so remove them from omapdrm.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 87724fa818d0..0ebd1315fff8 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -663,23 +663,6 @@ static struct drm_driver omap_drm_driver = {
 		.patchlevel = DRIVER_PATCHLEVEL,
 };
 
-static int pdev_suspend(struct platform_device *pDevice, pm_message_t state)
-{
-	DBG("");
-	return 0;
-}
-
-static int pdev_resume(struct platform_device *device)
-{
-	DBG("");
-	return 0;
-}
-
-static void pdev_shutdown(struct platform_device *device)
-{
-	DBG("");
-}
-
 static int pdev_probe(struct platform_device *device)
 {
 	int r;
@@ -726,9 +709,6 @@ static struct platform_driver pdev = {
 		},
 		.probe = pdev_probe,
 		.remove = pdev_remove,
-		.suspend = pdev_suspend,
-		.resume = pdev_resume,
-		.shutdown = pdev_shutdown,
 };
 
 static int __init omap_drm_init(void)
-- 
2.3.0

[PATCH 14/21] drm/omap: stop connector polling during suspend

[Tomi Valkeinen]

When not using proper hotplug detection, DRM polls periodically the
connectors to find out if a cable is connected. This polling can happen
at any time, even very late in the suspend process.

This causes a problem with omapdrm, when the poll happens during the
suspend process after GPIOs have been disabled, leading to a crash in
gpio_get().

This patch fixes the issue by adding suspend and resume hooks to
omapdrm, in which we disable and enable, respectively, the polling.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 0ebd1315fff8..d0b1aece8cc5 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -694,9 +694,28 @@ static int pdev_remove(struct platform_device *device)
 	return 0;
 }
 
+static int omap_drm_suspend(struct device *dev)
+{
+	struct drm_device *drm_dev = dev_get_drvdata(dev);
+
+	drm_kms_helper_poll_disable(drm_dev);
+
+	return 0;
+}
+
+static int omap_drm_resume(struct device *dev)
+{
+	struct drm_device *drm_dev = dev_get_drvdata(dev);
+
+	drm_kms_helper_poll_enable(drm_dev);
+
+	return omap_gem_resume(dev);
+}
+
 #ifdef CONFIG_PM
 static const struct dev_pm_ops omapdrm_pm_ops = {
-	.resume = omap_gem_resume,
+	.suspend = omap_drm_suspend,
+	.resume = omap_drm_resume,
 };
 #endif
 
-- 
2.3.0

[PATCH 15/21] drm/omap: use DRM_ERROR_RATELIMITED() for error irqs

[Tomi Valkeinen]

omapdrm uses normal DRM_ERROR() print when the HW reports an error. As
we sometimes may get a flood of errors, let's rather use
DRM_ERROR_RATELIMITED().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c  | 2 +-
 drivers/gpu/drm/omapdrm/omap_plane.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index af4b230b5269..832fb9e38612 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -429,7 +429,7 @@ static void omap_crtc_error_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
 	struct omap_crtc *omap_crtc =
 			container_of(irq, struct omap_crtc, error_irq);
 	struct drm_crtc *crtc = &omap_crtc->base;
-	DRM_ERROR("%s: errors: %08x\n", omap_crtc->name, irqstatus);
+	DRM_ERROR_RATELIMITED("%s: errors: %08x\n", omap_crtc->name, irqstatus);
 	/* avoid getting in a flood, unregister the irq until next vblank */
 	__omap_irq_unregister(crtc->dev, &omap_crtc->error_irq);
 }
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 8e94694ba8a3..dc666f6b2bf8 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -386,7 +386,8 @@ static void omap_plane_error_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
 {
 	struct omap_plane *omap_plane =
 			container_of(irq, struct omap_plane, error_irq);
-	DRM_ERROR("%s: errors: %08x\n", omap_plane->name, irqstatus);
+	DRM_ERROR_RATELIMITED("%s: errors: %08x\n", omap_plane->name,
+		irqstatus);
 }
 
 static const char *plane_names[] = {
-- 
2.3.0

[PATCH 16/21] drm/omap: fix race with error_irq

[Tomi Valkeinen]

omapdrm tries to avoid error floods by unregistering the error irq when
an error happens, and then registering the error irq again later.
However, the code is racy, as it sometimes tries to unregister the error
irq when it's already unregistered, leading to WARN().

Also, the code only registers the error irq again when something is done
on that particular output, i.e. if only TV is used to flip the buffers,
and LCD is showing a same buffer, an error on LCD will cause the LCD
error irq to be unregistered and never registered again.

To fix this, let's keep the error irqs always enabled and trust the
DRM_ERROR_RATELIMITED to limit the flood.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 832fb9e38612..2175b9b7c725 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -74,6 +74,8 @@ struct omap_crtc {
 	 * XXX maybe fold into apply_work??
 	 */
 	struct work_struct page_flip_work;
+
+	bool ignore_digit_sync_lost;
 };
 
 uint32_t pipe2vbl(struct drm_crtc *crtc)
@@ -428,10 +430,14 @@ static void omap_crtc_error_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
 {
 	struct omap_crtc *omap_crtc =
 			container_of(irq, struct omap_crtc, error_irq);
-	struct drm_crtc *crtc = &omap_crtc->base;
+
+	if (omap_crtc->ignore_digit_sync_lost) {
+		irqstatus &= ~DISPC_IRQ_SYNC_LOST_DIGIT;
+		if (!irqstatus)
+			return;
+	}
+
 	DRM_ERROR_RATELIMITED("%s: errors: %08x\n", omap_crtc->name, irqstatus);
-	/* avoid getting in a flood, unregister the irq until next vblank */
-	__omap_irq_unregister(crtc->dev, &omap_crtc->error_irq);
 }
 
 static void omap_crtc_apply_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
@@ -440,9 +446,6 @@ static void omap_crtc_apply_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
 			container_of(irq, struct omap_crtc, apply_irq);
 	struct drm_crtc *crtc = &omap_crtc->base;
 
-	if (!omap_crtc->error_irq.registered)
-		__omap_irq_register(crtc->dev, &omap_crtc->error_irq);
-
 	if (!dispc_mgr_go_busy(omap_crtc->channel)) {
 		struct omap_drm_private *priv =
 				crtc->dev->dev_private;
@@ -558,7 +561,7 @@ static void set_enabled(struct drm_crtc *crtc, bool enable)
 	 * Digit output produces some sync lost interrupts during the first
 	 * frame when enabling, so we need to ignore those.
 	 */
-	omap_irq_unregister(crtc->dev, &omap_crtc->error_irq);
+	omap_crtc->ignore_digit_sync_lost = true;
 
 	framedone_irq = dispc_mgr_get_framedone_irq(channel);
 	vsync_irq = dispc_mgr_get_vsync_irq(channel);
@@ -589,7 +592,9 @@ static void set_enabled(struct drm_crtc *crtc, bool enable)
 				omap_crtc->name, enable ? "enable" : "disable");
 	}
 
-	omap_irq_register(crtc->dev, &omap_crtc->error_irq);
+	omap_crtc->ignore_digit_sync_lost = false;
+	/* make sure the irq handler sees the value above */
+	mb();
 }
 
 static void omap_crtc_pre_apply(struct omap_drm_apply *apply)
-- 
2.3.0

[PATCH 17/21] drm/omap: only ignore DIGIT SYNC LOST for TV output

[Tomi Valkeinen]

We need to ignore DIGIT SYNC LOST error when enabling/disabling TV
output. The code does that, but it ignores the DIGI SYNC LOST when
enabling any output. Normally this does no harm, but it could make us
miss DIGIT SYNC LOST on some rare occasions.

Fix the code to only ignore DIGIT SYNC LOST when enabling/disabling TV.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 2175b9b7c725..4b06c039e9af 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -557,11 +557,13 @@ static void set_enabled(struct drm_crtc *crtc, bool enable)
 	if (dispc_mgr_is_enabled(channel) == enable)
 		return;
 
-	/*
-	 * Digit output produces some sync lost interrupts during the first
-	 * frame when enabling, so we need to ignore those.
-	 */
-	omap_crtc->ignore_digit_sync_lost = true;
+	if (omap_crtc->channel == OMAP_DSS_CHANNEL_DIGIT) {
+		/*
+		 * Digit output produces some sync lost interrupts during the
+		 * first frame when enabling, so we need to ignore those.
+		 */
+		omap_crtc->ignore_digit_sync_lost = true;
+	}
 
 	framedone_irq = dispc_mgr_get_framedone_irq(channel);
 	vsync_irq = dispc_mgr_get_vsync_irq(channel);
@@ -592,9 +594,11 @@ static void set_enabled(struct drm_crtc *crtc, bool enable)
 				omap_crtc->name, enable ? "enable" : "disable");
 	}
 
-	omap_crtc->ignore_digit_sync_lost = false;
-	/* make sure the irq handler sees the value above */
-	mb();
+	if (omap_crtc->channel == OMAP_DSS_CHANNEL_DIGIT) {
+		omap_crtc->ignore_digit_sync_lost = false;
+		/* make sure the irq handler sees the value above */
+		mb();
+	}
 }
 
 static void omap_crtc_pre_apply(struct omap_drm_apply *apply)
-- 
2.3.0

[PATCH 18/21] drm/omap: do not use BUG_ON(!spin_is_locked(x))

[Tomi Valkeinen]

spin_is_locked(x) returns always 0 on uniprocessor, triggering BUG() in
omapdrm.

Change it to use assert_spin_locked() to fix the issue.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
index f035d2bceae7..3eb097efc488 100644
--- a/drivers/gpu/drm/omapdrm/omap_irq.c
+++ b/drivers/gpu/drm/omapdrm/omap_irq.c
@@ -34,7 +34,7 @@ static void omap_irq_update(struct drm_device *dev)
 	struct omap_drm_irq *irq;
 	uint32_t irqmask = priv->vblank_mask;
 
-	BUG_ON(!spin_is_locked(&list_lock));
+	assert_spin_locked(&list_lock);
 
 	list_for_each_entry(irq, &priv->irq_list, node)
 		irqmask |= irq->irqmask;
-- 
2.3.0

[PATCH 19/21] drm/omap: fix race condition with dev->obj_list

[Tomi Valkeinen]

omap_gem_objects are added to dev->obj_list in omap_gem_new, and removed
in omap_gem_free_object. Unfortunately there's no locking for
dev->obj_list, which eventually leads to a crash:

WARNING: CPU: 1 PID: 1123 at lib/list_debug.c:59 __list_del_entry+0xa4/0xe0()
list_del corruption. prev->next should be e9281344, but was ea722b84

Add a spinlock to protect dev->obj_list.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 1 +
 drivers/gpu/drm/omapdrm/omap_drv.h | 3 +++
 drivers/gpu/drm/omapdrm/omap_gem.c | 5 +++++
 3 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index d0b1aece8cc5..2a85221e0604 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -479,6 +479,7 @@ static int dev_load(struct drm_device *dev, unsigned long flags)
 
 	priv->wq = alloc_ordered_workqueue("omapdrm", 0);
 
+	spin_lock_init(&priv->list_lock);
 	INIT_LIST_HEAD(&priv->obj_list);
 
 	omap_gem_init(dev);
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 60e47b33c801..dac9f8ef3b15 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -105,6 +105,9 @@ struct omap_drm_private {
 
 	struct workqueue_struct *wq;
 
+	/* lock for obj_list below */
+	spinlock_t list_lock;
+
 	/* list of GEM objects: */
 	struct list_head obj_list;
 
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index d37ee756a0b1..e9718b99a8a9 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -1273,13 +1273,16 @@ unlock:
 void omap_gem_free_object(struct drm_gem_object *obj)
 {
 	struct drm_device *dev = obj->dev;
+	struct omap_drm_private *priv = dev->dev_private;
 	struct omap_gem_object *omap_obj = to_omap_bo(obj);
 
 	evict(obj);
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
+	spin_lock(&priv->list_lock);
 	list_del(&omap_obj->mm_list);
+	spin_unlock(&priv->list_lock);
 
 	drm_gem_free_mmap_offset(obj);
 
@@ -1377,7 +1380,9 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
 	if (!omap_obj)
 		goto fail;
 
+	spin_lock(&priv->list_lock);
 	list_add(&omap_obj->mm_list, &priv->obj_list);
+	spin_unlock(&priv->list_lock);
 
 	obj = &omap_obj->base;
 
-- 
2.3.0

[PATCH 20/21] drm/omap: fix race conditon in DMM

[Tomi Valkeinen]

The omapdrm DMM code sometimes crashes with:

WARNING: CPU: 0 PID: 1235 at lib/list_debug.c:36 __list_add+0x8c/0xbc()
list_add double add: new=e9265368, prev=e90139c4, next=e9265368.

This is caused by the code calling release_engine() twice for the same
engine.

dmm_txn_commit(wait=true) call is supposed to wait until the DMM
transaction has been finished. And it does that, but it does not wait
for the irq handler to finish.

What happens is that the irq handler is triggered, and it either wakes
up the thread that called dmm_txn_commit(), or that thread never even
slept because the transaction was finished in the HW very quickly. That
thread then continues executing, even if the irq handler is not yet
finished, and a new transaction may be initiated. If that transaction is
async (i.e. wait=false), a 'async' flag is set to true. The original irq
handler, which has yet not finished, then sees the transaction as
'async', even if it was supposed to be 'sync'.

When that happens, the irq handler does an extra release_engine() call
because it thinks it need to release the engine, leading to the crash.

This patch fixes the issue by using completion to ensure that the irq
handler has finished before a dmm_txn_commit(wait=true) may continue.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_dmm_priv.h  |  2 +-
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 15 ++++++++-------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_priv.h b/drivers/gpu/drm/omapdrm/omap_dmm_priv.h
index d96660573076..9f32a83ca507 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_priv.h
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_priv.h
@@ -148,7 +148,7 @@ struct refill_engine {
 
 	bool async;
 
-	wait_queue_head_t wait_for_refill;
+	struct completion compl;
 
 	struct list_head idle_node;
 };
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index c4563af55a82..ccf1bf06ceb7 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -29,6 +29,7 @@
 #include <linux/mm.h>
 #include <linux/time.h>
 #include <linux/list.h>
+#include <linux/completion.h>
 
 #include "omap_dmm_tiler.h"
 #include "omap_dmm_priv.h"
@@ -146,10 +147,10 @@ static irqreturn_t omap_dmm_irq_handler(int irq, void *arg)
 
 	for (i = 0; i < dmm->num_engines; i++) {
 		if (status & DMM_IRQSTAT_LST) {
-			wake_up_interruptible(&dmm->engines[i].wait_for_refill);
-
 			if (dmm->engines[i].async)
 				release_engine(&dmm->engines[i]);
+
+			complete(&dmm->engines[i].compl);
 		}
 
 		status >>= 8;
@@ -273,7 +274,8 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool wait)
 
 	/* mark whether it is async to denote list management in IRQ handler */
 	engine->async = wait ? false : true;
-	/* verify that the irq handler sees the 'async' value */
+	reinit_completion(&engine->compl);
+	/* verify that the irq handler sees the 'async' and completion value */
 	smp_mb();
 
 	/* kick reload */
@@ -281,9 +283,8 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool wait)
 		dmm->base + reg[PAT_DESCR][engine->id]);
 
 	if (wait) {
-		if (wait_event_interruptible_timeout(engine->wait_for_refill,
-				wait_status(engine, DMM_PATSTATUS_READY) == 0,
-				msecs_to_jiffies(1)) <= 0) {
+		if (!wait_for_completion_timeout(&engine->compl,
+				msecs_to_jiffies(1))) {
 			dev_err(dmm->dev, "timed out waiting for done\n");
 			ret = -ETIMEDOUT;
 		}
@@ -719,7 +720,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 						(REFILL_BUFFER_SIZE * i);
 		omap_dmm->engines[i].refill_pa = omap_dmm->refill_pa +
 						(REFILL_BUFFER_SIZE * i);
-		init_waitqueue_head(&omap_dmm->engines[i].wait_for_refill);
+		init_completion(&omap_dmm->engines[i].compl);
 
 		list_add(&omap_dmm->engines[i].idle_node, &omap_dmm->idle_head);
 	}
-- 
2.3.0

[PATCH 21/21] drm/omap: keep ref to old_fb

[Tomi Valkeinen]

We store the fb being page-flipped to 'old_fb' field, but we don't
increase the ref count of the fb when doing that. While I am not
sure if it can cause problem in practice, it's still safer to keep a ref
when storing a pointer to a fb.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 4b06c039e9af..faa20fbc2ad9 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -301,6 +301,7 @@ static void vblank_cb(void *arg)
 	struct drm_device *dev = crtc->dev;
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
 	unsigned long flags;
+	struct drm_framebuffer *fb;
 
 	spin_lock_irqsave(&dev->event_lock, flags);
 
@@ -308,10 +309,15 @@ static void vblank_cb(void *arg)
 	if (omap_crtc->event)
 		drm_send_vblank_event(dev, omap_crtc->pipe, omap_crtc->event);
 
+	fb = omap_crtc->old_fb;
+
 	omap_crtc->event = NULL;
 	omap_crtc->old_fb = NULL;
 
 	spin_unlock_irqrestore(&dev->event_lock, flags);
+
+	if (fb)
+		drm_framebuffer_unreference(fb);
 }
 
 static void page_flip_worker(struct work_struct *work)
@@ -368,6 +374,7 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc,
 
 	omap_crtc->event = event;
 	omap_crtc->old_fb = primary->fb = fb;
+	drm_framebuffer_reference(omap_crtc->old_fb);
 
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 
-- 
2.3.0

Re: [PATCH 14/21] drm/omap: stop connector polling during suspend

[Grygorii.Strashko@linaro.org]

Hi Tomi,

On 02/26/2015 03:20 PM, Tomi Valkeinen wrote:
> When not using proper hotplug detection, DRM polls periodically the
> connectors to find out if a cable is connected. This polling can happen
> at any time, even very late in the suspend process.
> 
> This causes a problem with omapdrm, when the poll happens during the
> suspend process after GPIOs have been disabled, leading to a crash in
> gpio_get().
> 
> This patch fixes the issue by adding suspend and resume hooks to
> omapdrm, in which we disable and enable, respectively, the polling.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   drivers/gpu/drm/omapdrm/omap_drv.c | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 0ebd1315fff8..d0b1aece8cc5 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -694,9 +694,28 @@ static int pdev_remove(struct platform_device *device)
>   	return 0;
>   }
>   
> +static int omap_drm_suspend(struct device *dev)
> +{
> +	struct drm_device *drm_dev = dev_get_drvdata(dev);
> +
> +	drm_kms_helper_poll_disable(drm_dev);
> +
> +	return 0;
> +}
> +
> +static int omap_drm_resume(struct device *dev)
> +{
> +	struct drm_device *drm_dev = dev_get_drvdata(dev);
> +
> +	drm_kms_helper_poll_enable(drm_dev);
> +
> +	return omap_gem_resume(dev);
> +}
> +
>   #ifdef CONFIG_PM
>   static const struct dev_pm_ops omapdrm_pm_ops = {
> -	.resume = omap_gem_resume,
> +	.suspend = omap_drm_suspend,
> +	.resume = omap_drm_resume,
>   };
>   #endif
>   
> 

Could I ask you to update this patch as below, pls?

Regards,
-grygorii

===
drm/omap: add hibernation callbacks

Setting a dev_pm_ops suspend/resume pair but not a set of
hibernation functions means those pm functions will not be
called upon hibernation.
Fix this by using SET_SYSTEM_SLEEP_PM_OPS, which appropriately
assigns the suspend and hibernation handlers and move
omap_drm_suspend/omap_drm_resume under CONFIG_PM_SLEEP
to avoid build warnings.

Signed-off-by: Grygorii Strashko <Grygorii.Strashko@linaro.org>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 7cb1c8f..4a544e4 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -715,6 +715,7 @@ static int pdev_remove(struct platform_device *device)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
 static int omap_drm_suspend(struct device *dev)
 {
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
@@ -732,21 +733,15 @@ static int omap_drm_resume(struct device *dev)
 
 	return omap_gem_resume(dev);
 }
-
-#ifdef CONFIG_PM
-static const struct dev_pm_ops omapdrm_pm_ops = {
-	.suspend = omap_drm_suspend,
-	.resume = omap_drm_resume,
-};
 #endif
 
+static SIMPLE_DEV_PM_OPS(omapdrm_pm_ops, omap_drm_suspend, omap_drm_resume);
+
 static struct platform_driver pdev = {
 		.driver = {
 			.name = DRIVER_NAME,
 			.owner = THIS_MODULE,
-#ifdef CONFIG_PM
 			.pm = &omapdrm_pm_ops,
-#endif
 		},
 		.probe = pdev_probe,
 		.remove = pdev_remove,
-- 1.9.1 

-- 
regards,
-grygorii

Re: [PATCH 06/21] drm/omap: check CRTC color format earlier

[Daniel Vetter]

On Thu, Feb 26, 2015 at 03:20:14PM +0200, Tomi Valkeinen wrote:
> When setting a color format to a DRM plane, the DRM core checks whether
> the format is supported by the HW. However, it seems that when setting
> the color format of a CRTC (i.e. a root plane), there's no checking
> done. This causes omapdrm to configure omapdss with the bad color
> format, which omapdss then rejects.
> 
> While the above is ok, the error message is a bit confusing, and the
> configuring of omapdss happens asynchronously from the ioctl that does
> the color format change.
> 
> This patch adds a color format check to omap_plane_mode_set() which
> causes the ioctl to return an error for invalid color format. This means
> that the userspace will get to know of the wrong setting, instead of the
> current behavior where the error is not seen by the userspace.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_plane.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
> index 1f4f2b866379..bedd6f7af0f1 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -207,6 +207,24 @@ int omap_plane_mode_set(struct drm_plane *plane,
>  {
>  	struct omap_plane *omap_plane = to_omap_plane(plane);
>  	struct omap_drm_window *win = &omap_plane->win;
> +	int i;
> +
> +	/*
> +	 * Check whether this plane supports the fb pixel format.
> +	 * I don't think this should really be needed, but it looks like the
> +	 * drm core only checks the format for planes, not for the crtc. So
> +	 * when setting the format for crtc, without this check we would
> +	 * get an error later when trying to program the color format into the
> +	 * HW.
> +	 */

I think we should add a format check to the setcrtc ioctl if crtc->primary
is set. Atm the code is in __setplane_internal but could easily be shared
- there's already a copy in drm_atomi.c.

Omapdrm wouldn't benefit from this yet since it doesn't support universal
planes. But adding universal planes should be on your todo anyway ;-)
-Daniel

> +	for (i = 0; i < plane->format_count; i++)
> +		if (fb->pixel_format == plane->format_types[i])
> +			break;
> +	if (i == plane->format_count) {
> +		DBG("Invalid pixel format %s",
> +			      drm_get_format_name(fb->pixel_format));
> +		return -EINVAL;
> +	}
>  
>  	win->crtc_x = crtc_x;
>  	win->crtc_y = crtc_y;
> -- 
> 2.3.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/21] drm/omap: handle mismatching color format and buffer width

[Daniel Vetter]

On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote:
> omapdrm doesn't check if the width of the framebuffer and the color
> format's bits-per-pixel match.
> 
> For example, using a display with a width of 1280, and a buffer
> allocated with using 32 bits per pixel (i.e. 1280*4 = 5120 bytes), with
> a 24 bits per pixel color format, leads to the following mismatch:
> 5120/3 = 1706.666... bytes. This causes bad colors and a tilt on the
> screen.
> 
> Add a check into omapdrm to return an error if the user tries to use
> such a combination.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_fb.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
> index 2975096abdf5..bf98580223d0 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>  			goto fail;
>  		}
>  
> +		if (mode_cmd->width % format->planes[i].stride_bpp != 0) {

width is in pixels. No idea what you're trying to check here, but this
probably isn't it.

Also drm checks that things fit into the specified pitch (which is in
bytes), see the pichtes[i] < width * cpp check in framebuffer_check.

Cheers, Daniel
> +			dev_err(dev->dev,
> +				"buffer width (%d) is not a multiple of pixel width (%d)\n",
> +				mode_cmd->width, format->planes[i].stride_bpp);
> +			ret = -EINVAL;
> +			goto fail;
> +		}
> +
>  		size = pitch * mode_cmd->height / format->planes[i].sub_y;
>  
>  		if (size > (omap_gem_mmap_size(bos[i]) - mode_cmd->offsets[i])) {
> -- 
> 2.3.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 09/21] drm/omap: handle mismatching color format and buffer width

[Daniel Stone]

Hi,

On 27 February 2015 at 13:01, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote:
>> omapdrm doesn't check if the width of the framebuffer and the color

s/width/pitch/

>> format's bits-per-pixel match.

s/match/are compatible/

>> For example, using a display with a width of 1280, and a buffer
>> allocated with using 32 bits per pixel (i.e. 1280*4 = 5120 bytes), with

Might be clearer to say 'i.e. byte stride of ...', and also s/with using/for/.

>> a 24 bits per pixel color format, leads to the following mismatch:
>> 5120/3 = 1706.666... bytes. This causes bad colors and a tilt on the

s/bytes/pixels/

>> screen.
>>
>> Add a check into omapdrm to return an error if the user tries to use
>> such a combination.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/omap_fb.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
>> index 2975096abdf5..bf98580223d0 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
>> @@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>>                       goto fail;
>>               }
>>
>> +             if (mode_cmd->width % format->planes[i].stride_bpp != 0) {
>
> width is in pixels. No idea what you're trying to check here, but this
> probably isn't it.

stride_bpp is very misnamed: it is the bits per pixel for that plane,
and not stride at all. I think the check should in fact be be (pitch %
format->planes[i].stride_bpp), which would achieve the desired result,
i.e. that the stride can be expressed as an integer number of pixels.

> Also drm checks that things fit into the specified pitch (which is in
> bytes), see the pichtes[i] < width * cpp check in framebuffer_check.

This isn't that check. At some stages, OMAP IIRC requires pitch to be
specified in pixels rather than bytes, so this makes sure that's
possible to express.

Cheers,
Daniel

Re: [PATCH 09/21] drm/omap: handle mismatching color format and buffer width

[Daniel Vetter]

On Fri, Feb 27, 2015 at 02:40:20PM +0000, Daniel Stone wrote:
> On 27 February 2015 at 13:01, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote:
> >> omapdrm doesn't check if the width of the framebuffer and the color
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
> >> index 2975096abdf5..bf98580223d0 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> >> @@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >>                       goto fail;
> >>               }
> >>
> >> +             if (mode_cmd->width % format->planes[i].stride_bpp != 0) {
> >
> > width is in pixels. No idea what you're trying to check here, but this
> > probably isn't it.
> 
> stride_bpp is very misnamed: it is the bits per pixel for that plane,
> and not stride at all. I think the check should in fact be be (pitch %
> format->planes[i].stride_bpp), which would achieve the desired result,
> i.e. that the stride can be expressed as an integer number of pixels.

I meant that mode_cmd->width is in pixels and so totally not what you want
to check here. It probably should be mode_cmd->pitches[i].
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/21] drm/omap: handle mismatching color format and buffer width

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3486 bytes --]

On 27/02/15 16:40, Daniel Stone wrote:
> Hi,
> 
> On 27 February 2015 at 13:01, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote:
>>> omapdrm doesn't check if the width of the framebuffer and the color
> 
> s/width/pitch/
> 
>>> format's bits-per-pixel match.
> 
> s/match/are compatible/
> 
>>> For example, using a display with a width of 1280, and a buffer
>>> allocated with using 32 bits per pixel (i.e. 1280*4 = 5120 bytes), with
> 
> Might be clearer to say 'i.e. byte stride of ...', and also s/with using/for/.

Above you said pitch, here you say stride. They are the same thing, right?

>>> a 24 bits per pixel color format, leads to the following mismatch:
>>> 5120/3 = 1706.666... bytes. This causes bad colors and a tilt on the
> 
> s/bytes/pixels/
> 
>>> screen.
>>>
>>> Add a check into omapdrm to return an error if the user tries to use
>>> such a combination.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> ---
>>>  drivers/gpu/drm/omapdrm/omap_fb.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
>>> index 2975096abdf5..bf98580223d0 100644
>>> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
>>> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
>>> @@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>>>                       goto fail;
>>>               }
>>>
>>> +             if (mode_cmd->width % format->planes[i].stride_bpp != 0) {
>>
>> width is in pixels. No idea what you're trying to check here, but this
>> probably isn't it.

Yep, I don't know what I was smoking when writing the patch...

> stride_bpp is very misnamed: it is the bits per pixel for that plane,
> and not stride at all. I think the check should in fact be be (pitch %

I don't know why Rob named it like that. "The bpp of the stride"? Shrug.

> format->planes[i].stride_bpp), which would achieve the desired result,
> i.e. that the stride can be expressed as an integer number of pixels.

Yes, that looks correct.

>> Also drm checks that things fit into the specified pitch (which is in
>> bytes), see the pichtes[i] < width * cpp check in framebuffer_check.
> 
> This isn't that check. At some stages, OMAP IIRC requires pitch to be
> specified in pixels rather than bytes, so this makes sure that's
> possible to express.

Right, that's what this patch was trying to achieve.

However... After thinking about this and going through some of the DISPC
code, I think that's not a strict requirement. We do calculate all the
configs using pixels as units, so at the moment the stride has to be an
integer number of pixels. But the hardware actually takes the row-inc
and pix-inc as bytes.

That said, the HW supports features like rotation and whatnot, and it
was not clear with a quick study if there are corner cases where the
hardware also requires the stride to be an integer number of pixels.
Also, the HW documentation only talks about pixels in this context, even
if the final value written to the registers is in bytes. I don't know if
that's just to make the documentation simpler, or if there's some
reasoning to only use pixel units.

So I think for the time being I'll just fix this patch, and look at the
possibility of allowing any stride size in the future.

Thanks for the review!

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 06/21] drm/omap: check CRTC color format earlier

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2578 bytes --]

On 27/02/15 14:07, Daniel Vetter wrote:
> On Thu, Feb 26, 2015 at 03:20:14PM +0200, Tomi Valkeinen wrote:
>> When setting a color format to a DRM plane, the DRM core checks whether
>> the format is supported by the HW. However, it seems that when setting
>> the color format of a CRTC (i.e. a root plane), there's no checking
>> done. This causes omapdrm to configure omapdss with the bad color
>> format, which omapdss then rejects.
>>
>> While the above is ok, the error message is a bit confusing, and the
>> configuring of omapdss happens asynchronously from the ioctl that does
>> the color format change.
>>
>> This patch adds a color format check to omap_plane_mode_set() which
>> causes the ioctl to return an error for invalid color format. This means
>> that the userspace will get to know of the wrong setting, instead of the
>> current behavior where the error is not seen by the userspace.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/omap_plane.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
>> index 1f4f2b866379..bedd6f7af0f1 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
>> @@ -207,6 +207,24 @@ int omap_plane_mode_set(struct drm_plane *plane,
>>  {
>>  	struct omap_plane *omap_plane = to_omap_plane(plane);
>>  	struct omap_drm_window *win = &omap_plane->win;
>> +	int i;
>> +
>> +	/*
>> +	 * Check whether this plane supports the fb pixel format.
>> +	 * I don't think this should really be needed, but it looks like the
>> +	 * drm core only checks the format for planes, not for the crtc. So
>> +	 * when setting the format for crtc, without this check we would
>> +	 * get an error later when trying to program the color format into the
>> +	 * HW.
>> +	 */
> 
> I think we should add a format check to the setcrtc ioctl if crtc->primary
> is set. Atm the code is in __setplane_internal but could easily be shared
> - there's already a copy in drm_atomi.c.
> 
> Omapdrm wouldn't benefit from this yet since it doesn't support universal
> planes. But adding universal planes should be on your todo anyway ;-)

Laurent is working on universal planes and atomic modesetting for
omapdrm. In fact, I think universal planes is already done in his WIP
branch.

So, if this check is already done with universal planes (if I understood
correctly), I'm fine with dropping this patch.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 09/21] drm/omap: handle mismatching color format and buffer width

[Daniel Stone]

Hi,

On 2 March 2015 at 09:50, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 27/02/15 16:40, Daniel Stone wrote:
>> On 27 February 2015 at 13:01, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> On Thu, Feb 26, 2015 at 03:20:17PM +0200, Tomi Valkeinen wrote:
>>>> omapdrm doesn't check if the width of the framebuffer and the color
>>
>> s/width/pitch/
>>
>>>> format's bits-per-pixel match.
>>
>> s/match/are compatible/
>>
>>>> For example, using a display with a width of 1280, and a buffer
>>>> allocated with using 32 bits per pixel (i.e. 1280*4 = 5120 bytes), with
>>
>> Might be clearer to say 'i.e. byte stride of ...', and also s/with using/for/.
>
> Above you said pitch, here you say stride. They are the same thing, right?

Yeah, they're interchangeable. In theory, I think it's supposed to be
that pitch is in pixels and stride in bpp, but they're so often
interchanged that they've lost that distinction. Still, using one
consistently is always good - and since KMS uses pitch exclusively,
that might be a good one to settle on.

>> stride_bpp is very misnamed: it is the bits per pixel for that plane,
>> and not stride at all. I think the check should in fact be be (pitch %
>
> I don't know why Rob named it like that. "The bpp of the stride"? Shrug.

It's just the bpp of the pixel format; it's not at all associated with
the stride?

>> This isn't that check. At some stages, OMAP IIRC requires pitch to be
>> specified in pixels rather than bytes, so this makes sure that's
>> possible to express.
>
> Right, that's what this patch was trying to achieve.
>
> However... After thinking about this and going through some of the DISPC
> code, I think that's not a strict requirement. We do calculate all the
> configs using pixels as units, so at the moment the stride has to be an
> integer number of pixels. But the hardware actually takes the row-inc
> and pix-inc as bytes.
>
> That said, the HW supports features like rotation and whatnot, and it
> was not clear with a quick study if there are corner cases where the
> hardware also requires the stride to be an integer number of pixels.
> Also, the HW documentation only talks about pixels in this context, even
> if the final value written to the registers is in bytes. I don't know if
> that's just to make the documentation simpler, or if there's some
> reasoning to only use pixel units.

Indeed that was my impression from a quick look, but my OMAP is
extremely rusty these days, so wasn't quite sure ... :) Specifying
pixel units isn't totally unheard of, but bytes is definitely more
common.

Cheers,
Daniel

Re: [PATCH 09/21] drm/omap: handle mismatching color format and buffer width

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2297 bytes --]

On 02/03/15 12:22, Daniel Stone wrote:

>> I don't know why Rob named it like that. "The bpp of the stride"? Shrug.
> 
> It's just the bpp of the pixel format; it's not at all associated with
> the stride?

The comment says "this times width is stride", so I thought the naming comes
from that train of thought. But I agree, it's just bpp.

Here's updated patch:

From af0db526ae274d47488ec49577365909eb8f629d Mon Sep 17 00:00:00 2001
From: Tomi Valkeinen <tomi.valkeinen@ti.com>
Date: Thu, 25 Sep 2014 19:24:28 +0000
Subject: [PATCH] drm/omap: handle incompatible buffer stride and pixel size

omapdrm doesn't check if the pitch of the framebuffer and the color
format's bits-per-pixel are compatible. omapdss requires that the stride
of a buffer is an integer number of pixels

For example, when using modetest with a display that has x resolution of
1280, and using packed 24 RGB mode (3 bytes per pixel), modetest
allocates a buffer with a byte stride of 4 * 1280 = 5120. But 5120 / 3 =
1706.666... pixels, which causes wrong colors and a tilt on the screen.

Add a check into omapdrm to return an error if the user tries to use
such a combination.

Note: this is not a HW requirement at least for non-rotation use cases,
but a SW driver requirement. In the future we should study if also
rotation use cases are fine with any stride size, and if so, change the
driver to allow these strides.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_fb.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 2975096abdf5..e123b4dee670 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -463,6 +463,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 			goto fail;
 		}
 
+		if (pitch % format->planes[i].stride_bpp != 0) {
+			dev_err(dev->dev,
+				"buffer pitch (%d bytes) is not a multiple of pixel size (%d bytes)\n",
+				pitch, format->planes[i].stride_bpp);
+			ret = -EINVAL;
+			goto fail;
+		}
+
 		size = pitch * mode_cmd->height / format->planes[i].sub_y;
 
 		if (size > (omap_gem_mmap_size(bos[i]) - mode_cmd->offsets[i])) {
-- 
2.3.1




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 14/21] drm/omap: stop connector polling during suspend

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 271 bytes --]

On 26/02/15 15:57, Grygorii.Strashko@linaro.org wrote:

> Could I ask you to update this patch as below, pls?

Your changes look ok to me, but they are not related to my patch so I
don't see any reason to merge them. I'll pick your patch to my series.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 14/21] drm/omap: stop connector polling during suspend

[Grygorii Strashko]

On 03/02/2015 01:03 PM, Tomi Valkeinen wrote:
> On 26/02/15 15:57, Grygorii.Strashko@linaro.org wrote:
> 
>> Could I ask you to update this patch as below, pls?
> 
> Your changes look ok to me, but they are not related to my patch so I
> don't see any reason to merge them. I'll pick your patch to my series.

Sure. Thanks a lot. Ping me if anything will need to be done from my side.

Regards,
- grygorii

Re: [PATCH 06/21] drm/omap: check CRTC color format earlier

[Laurent Pinchart]

Hi Tomi,

On Monday 02 March 2015 11:55:48 Tomi Valkeinen wrote:
> On 27/02/15 14:07, Daniel Vetter wrote:
> > On Thu, Feb 26, 2015 at 03:20:14PM +0200, Tomi Valkeinen wrote:
> >> When setting a color format to a DRM plane, the DRM core checks whether
> >> the format is supported by the HW. However, it seems that when setting
> >> the color format of a CRTC (i.e. a root plane), there's no checking
> >> done. This causes omapdrm to configure omapdss with the bad color
> >> format, which omapdss then rejects.
> >> 
> >> While the above is ok, the error message is a bit confusing, and the
> >> configuring of omapdss happens asynchronously from the ioctl that does
> >> the color format change.
> >> 
> >> This patch adds a color format check to omap_plane_mode_set() which
> >> causes the ioctl to return an error for invalid color format. This means
> >> that the userspace will get to know of the wrong setting, instead of the
> >> current behavior where the error is not seen by the userspace.
> >> 
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/omapdrm/omap_plane.c | 18 ++++++++++++++++++
> >>  1 file changed, 18 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
> >> b/drivers/gpu/drm/omapdrm/omap_plane.c index 1f4f2b866379..bedd6f7af0f1
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> >> @@ -207,6 +207,24 @@ int omap_plane_mode_set(struct drm_plane *plane,
> >> 
> >>  {
> >>  
> >>  	struct omap_plane *omap_plane = to_omap_plane(plane);
> >>  	struct omap_drm_window *win = &omap_plane->win;
> >> 
> >> +	int i;
> >> +
> >> +	/*
> >> +	 * Check whether this plane supports the fb pixel format.
> >> +	 * I don't think this should really be needed, but it looks like the
> >> +	 * drm core only checks the format for planes, not for the crtc. So
> >> +	 * when setting the format for crtc, without this check we would
> >> +	 * get an error later when trying to program the color format into
> >> the
> >> +	 * HW.
> >> +	 */
> > 
> > I think we should add a format check to the setcrtc ioctl if crtc->primary
> > is set. Atm the code is in __setplane_internal but could easily be shared
> > - there's already a copy in drm_atomi.c.
> > 
> > Omapdrm wouldn't benefit from this yet since it doesn't support universal
> > planes. But adding universal planes should be on your todo anyway ;-)
> 
> Laurent is working on universal planes and atomic modesetting for
> omapdrm. In fact, I think universal planes is already done in his WIP
> branch.
> 
> So, if this check is already done with universal planes (if I understood
> correctly), I'm fine with dropping this patch.

Yes, I've implemented universal plane support. I'm currently rebasing your 
patches on top of my bug fixes (excluding the more intrusive atomic update 
rework). I'll push the resulting branch shortly and will send the patches for 
review.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 06/21] drm/omap: check CRTC color format earlier

[Laurent Pinchart]

Hi Daniel,

On Friday 27 February 2015 13:07:57 Daniel Vetter wrote:
> On Thu, Feb 26, 2015 at 03:20:14PM +0200, Tomi Valkeinen wrote:
> > When setting a color format to a DRM plane, the DRM core checks whether
> > the format is supported by the HW. However, it seems that when setting
> > the color format of a CRTC (i.e. a root plane), there's no checking
> > done. This causes omapdrm to configure omapdss with the bad color
> > format, which omapdss then rejects.
> > 
> > While the above is ok, the error message is a bit confusing, and the
> > configuring of omapdss happens asynchronously from the ioctl that does
> > the color format change.
> > 
> > This patch adds a color format check to omap_plane_mode_set() which
> > causes the ioctl to return an error for invalid color format. This means
> > that the userspace will get to know of the wrong setting, instead of the
> > current behavior where the error is not seen by the userspace.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_plane.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
> > b/drivers/gpu/drm/omapdrm/omap_plane.c index 1f4f2b866379..bedd6f7af0f1
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> > @@ -207,6 +207,24 @@ int omap_plane_mode_set(struct drm_plane *plane,
> > 
> >  {
> >  
> >  	struct omap_plane *omap_plane = to_omap_plane(plane);
> >  	struct omap_drm_window *win = &omap_plane->win;
> > 
> > +	int i;
> > +
> > +	/*
> > +	 * Check whether this plane supports the fb pixel format.
> > +	 * I don't think this should really be needed, but it looks like the
> > +	 * drm core only checks the format for planes, not for the crtc. So
> > +	 * when setting the format for crtc, without this check we would
> > +	 * get an error later when trying to program the color format into 
the
> > +	 * HW.
> > +	 */
> 
> I think we should add a format check to the setcrtc ioctl if crtc->primary
> is set. Atm the code is in __setplane_internal but could easily be shared
> - there's already a copy in drm_atomi.c.

I'll submit a patch.

> Omapdrm wouldn't benefit from this yet since it doesn't support universal
> planes. But adding universal planes should be on your todo anyway ;-)

Soon, soon :-)

> > +	for (i = 0; i < plane->format_count; i++)
> > +		if (fb->pixel_format == plane->format_types[i])
> > +			break;
> > +	if (i == plane->format_count) {
> > +		DBG("Invalid pixel format %s",
> > +			      drm_get_format_name(fb->pixel_format));
> > +		return -EINVAL;
> > +	}
> > 
> >  	win->crtc_x = crtc_x;
> >  	win->crtc_y = crtc_y;

-- 
Regards,

Laurent Pinchart

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