[PATCH 00/33] drm/omap: patches for v4.6

[PATCH 00/33] drm/omap: patches for v4.6

[Tomi Valkeinen]

Hi,

Here's a collection of patches for omapdrm. Some of them have been sent for
review at some point, most of them haven't.

There are two bigger features in the series: dmabuf import support and HDMI
interlace output support. Otherwise they are smaller improvements, fixes and
cleanups.

 Tomi

Jyri Sarha (1):
  drm/omap: drm_atomic_get_plane_state() may return ERR_PTR

Laurent Pinchart (3):
  drm/omap: gem: Clean up GEM objects memory flags
  drm/omap: gem: Refactor GEM object allocation
  drm/omap: gem: Implement dma_buf import

Manisha Agrawal (3):
  drm/omap: tpd12s015: remove platform data support
  drm/omap: tpd12s015: gpio descriptor API
  drm/omap: tpd12s015: CT_CP_HPD as optional gpio

Rob Clark (1):
  drm/omap: EBUSY status handling in omap_gem_fault()

Tomi Valkeinen (25):
  drm/omap: HDMI: change enable/disable to avoid sync-losts
  HACK: drm/omap: always use blocking DMM fill
  HACK: drm/omap: fix memory barrier bug in DMM driver
  drm/omap: add dmm_read() and dmm_write() wrappers
  drm/omap: partial workaround for DRA7 DMM errata i878
  drm/omap: add define for DISPC_IRQ_WBUNCOMPLETEERROR
  drm/omap: use dma_mapping_error in omap_gem_attach_pages
  drm/omap: use dma_mapping_error in omap_gem_dma_sync
  drm/omap: print an error if display enable fails
  drm/omap: remove support for ext mem & sync
  drm/omap: increase vblank wait timeout
  drm/omap: DISPC: support double-pixel mode
  drm/omap: support double-pixel
  drm/omap: HDMI: support double-pixel pixel clock
  drm/omap: HDMI: Fix HSW value
  drm/omap: HDMI: fix WP timings for ilace
  drm/omap: DISPC: Fix field order for HDMI
  drm/omap: HDMI5: Fix FC HSW value
  drm/omap: HDMI5: clean up timings copy
  drm/omap: HDMI5: Add interlace support
  drm/omap: HDMI5: allow interlace
  drm/omap: verify that display x-res is divisible by 8
  drm/omap: verify that fb plane pitches are the same
  drm/omap: fix crtc->plane property delegation
  drm/omap: check if rotation is supported before commit

 Documentation/devicetree/bindings/arm/omap/dmm.txt |   3 +-
 .../gpu/drm/omapdrm/displays/encoder-tpd12s015.c   | 118 +++-----
 drivers/gpu/drm/omapdrm/dss/dispc.c                |  20 ++
 drivers/gpu/drm/omapdrm/dss/dpi.c                  |   3 +
 drivers/gpu/drm/omapdrm/dss/hdmi4.c                |  23 +-
 drivers/gpu/drm/omapdrm/dss/hdmi5.c                |  27 +-
 drivers/gpu/drm/omapdrm/dss/hdmi5_core.c           |  42 ++-
 drivers/gpu/drm/omapdrm/dss/hdmi_wp.c              |  32 ++-
 drivers/gpu/drm/omapdrm/omap_connector.c           |   4 +
 drivers/gpu/drm/omapdrm/omap_crtc.c                |  65 +++--
 drivers/gpu/drm/omapdrm/omap_dmm_priv.h            |   8 +
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c           | 193 ++++++++++++-
 drivers/gpu/drm/omapdrm/omap_drv.h                 |   3 +
 drivers/gpu/drm/omapdrm/omap_encoder.c             |   6 +-
 drivers/gpu/drm/omapdrm/omap_fb.c                  |  16 ++
 drivers/gpu/drm/omapdrm/omap_gem.c                 | 311 +++++++++++++--------
 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c          |  53 +++-
 drivers/gpu/drm/omapdrm/omap_plane.c               |   6 +
 include/video/omap-panel-data.h                    |  15 -
 include/video/omapdss.h                            |   3 +
 20 files changed, 642 insertions(+), 309 deletions(-)

-- 
2.5.0

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

[PATCH 01/33] drm/omap: HDMI: change enable/disable to avoid sync-losts

[Tomi Valkeinen]

We occasionally see DISPC sync-lost errors when enabling and disabling
HDMI. Sometimes we get only a few, which get handled (ignored) by the
driver, but sometimes there's a flood of the errors which doesn't seem
to stop.

The HW team has root caused this to the order in which HDMI and DISPC
are enabled/disabled. Currently we enable HDMI first, and then DISPC,
and vice versa when disabling. HW team's suggestion is to do it the
other way around.

This patch changes the order, but this has two side effects as the pixel
clock is produced by HDMI, and the clock is not running when we
enable/disable DISPC:

* When enabling DISPC first, we don't get vertical sync events
* When disabling DISPC last, we don't get FRAMEDONE event

At the moment we use both of those to verify that DISPC has been
enabled/disabled properly. Thus this patch also needs to change the
omapdrm and omapdss which handle the DISPC side.

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

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
index 7103c659a534..b09ce9ee82fa 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
@@ -214,22 +214,22 @@ static int hdmi_power_on_full(struct omap_dss_device *dssdev)
 	/* tv size */
 	dss_mgr_set_timings(mgr, p);
 
-	r = hdmi_wp_video_start(&hdmi.wp);
-	if (r)
-		goto err_vid_enable;
-
 	r = dss_mgr_enable(mgr);
 	if (r)
 		goto err_mgr_enable;
 
+	r = hdmi_wp_video_start(&hdmi.wp);
+	if (r)
+		goto err_vid_enable;
+
 	hdmi_wp_set_irqenable(wp,
 		HDMI_IRQ_LINK_CONNECT | HDMI_IRQ_LINK_DISCONNECT);
 
 	return 0;
 
-err_mgr_enable:
-	hdmi_wp_video_stop(&hdmi.wp);
 err_vid_enable:
+	dss_mgr_disable(mgr);
+err_mgr_enable:
 	hdmi_wp_set_phy_pwr(&hdmi.wp, HDMI_PHYPWRCMD_OFF);
 err_phy_pwr:
 err_phy_cfg:
@@ -246,10 +246,10 @@ static void hdmi_power_off_full(struct omap_dss_device *dssdev)
 
 	hdmi_wp_clear_irqenable(&hdmi.wp, 0xffffffff);
 
-	dss_mgr_disable(mgr);
-
 	hdmi_wp_video_stop(&hdmi.wp);
 
+	dss_mgr_disable(mgr);
+
 	hdmi_wp_set_phy_pwr(&hdmi.wp, HDMI_PHYPWRCMD_OFF);
 
 	dss_pll_disable(&hdmi.pll.pll);
diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
index a955a2c4c061..4485a1c37bd8 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
@@ -231,22 +231,22 @@ static int hdmi_power_on_full(struct omap_dss_device *dssdev)
 	/* tv size */
 	dss_mgr_set_timings(mgr, p);
 
-	r = hdmi_wp_video_start(&hdmi.wp);
-	if (r)
-		goto err_vid_enable;
-
 	r = dss_mgr_enable(mgr);
 	if (r)
 		goto err_mgr_enable;
 
+	r = hdmi_wp_video_start(&hdmi.wp);
+	if (r)
+		goto err_vid_enable;
+
 	hdmi_wp_set_irqenable(&hdmi.wp,
 			HDMI_IRQ_LINK_CONNECT | HDMI_IRQ_LINK_DISCONNECT);
 
 	return 0;
 
-err_mgr_enable:
-	hdmi_wp_video_stop(&hdmi.wp);
 err_vid_enable:
+	dss_mgr_disable(mgr);
+err_mgr_enable:
 	hdmi_wp_set_phy_pwr(&hdmi.wp, HDMI_PHYPWRCMD_OFF);
 err_phy_pwr:
 err_phy_cfg:
@@ -263,10 +263,10 @@ static void hdmi_power_off_full(struct omap_dss_device *dssdev)
 
 	hdmi_wp_clear_irqenable(&hdmi.wp, 0xffffffff);
 
-	dss_mgr_disable(mgr);
-
 	hdmi_wp_video_stop(&hdmi.wp);
 
+	dss_mgr_disable(mgr);
+
 	hdmi_wp_set_phy_pwr(&hdmi.wp, HDMI_PHYPWRCMD_OFF);
 
 	dss_pll_disable(&hdmi.pll.pll);
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 2ed0754ed19e..7dd3d44a93e5 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -138,6 +138,11 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable)
 	u32 framedone_irq, vsync_irq;
 	int ret;
 
+	if (omap_crtc->mgr->output->output_type == OMAP_DISPLAY_TYPE_HDMI) {
+		dispc_mgr_enable(channel, enable);
+		return;
+	}
+
 	if (dispc_mgr_is_enabled(channel) == enable)
 		return;
 
-- 
2.5.0

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

[PATCH 02/33] HACK: drm/omap: always use blocking DMM fill

[Tomi Valkeinen]

The current driver uses non-blocking DMM fill when releasing memory.
This gives us a small performance increase as we don't have to wait for
the fill operation to finish.

However, the driver does not have any error handling for non-blocking
fill. In case of an error, the fill operation may silently fail, leading
to leaking DMM engines, which may eventually lead to deadlock if we run
out of DMM engines.

This patch makes the DMM driver always use blocking fills, so that we
can catch the errors. A more complex option would be to allow
non-blocking fills, and implement proper error handling, but that is
left for the future.

This patch is a HACK, as the proper fix is to either decide to always
use sync fills and remove all the async related code, or fix the async
code.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index dfebdc4aa0f2..80526dec7b2c 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -309,6 +309,12 @@ static int fill(struct tcm_area *area, struct page **pages,
 	struct tcm_area slice, area_s;
 	struct dmm_txn *txn;
 
+	/*
+	 * XXX async wait doesn't work, as it does not handle timeout errors
+	 * properly
+	 */
+	wait = true;
+
 	txn = dmm_txn_init(omap_dmm, area->tcm);
 	if (IS_ERR_OR_NULL(txn))
 		return -ENOMEM;
-- 
2.5.0

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

[PATCH 03/33] HACK: drm/omap: fix memory barrier bug in DMM driver

[Tomi Valkeinen]

A DMM timeout "timed out waiting for done" has been observed on DRA7
devices. The timeout happens rarely, and only when the system is under
heavy load.

Debugging showed that the timeout can be made to happen much more
frequently by optimizing the DMM driver, so that there's almost no code
between writing the last DMM descriptors to RAM, and writing to DMM
register which starts the DMM transaction.

The current theory is that a wmb() does not properly ensure that the
data written to RAM is observable by all the components in the system.

This DMM timeout has caused interesting (and rare) bugs as the error
handling was not functioning properly (the error handling has been fixed
in previous commits):

 * If a DMM timeout happened when a GEM buffer was being pinned for
   display on the screen, a timeout error would be shown, but the driver
   would continue programming DSS HW with broken buffer, leading to
   SYNCLOST floods and possible crashes.

 * If a DMM timeout happened when other user (say, video decoder) was
   pinning a GEM buffer, a timeout would be shown but if the user
   handled the error properly, no other issues followed.

 * If a DMM timeout happened when a GEM buffer was being released, the
   driver does not even notice the error, leading to crashes or hang
   later.

This patch adds wmb() and readl() calls after the last bit is written to
RAM, which should ensure that the execution proceeds only after the data
is actually in RAM, and thus observable by DMM.

This patch is a HACK, as a read-back should not be needed. Further study
is required to understand if DMM is somehow special case and read-back
is ok, or if DRA7's memory barriers do not work correctly.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index 80526dec7b2c..4e04f9487375 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -262,6 +262,17 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool wait)
 	}
 
 	txn->last_pat->next_pa = 0;
+	/* ensure that the written descriptors are visible to DMM */
+	wmb();
+
+	/*
+	 * NOTE: the wmb() above should be enough, but there seems to be a bug
+	 * in OMAP's memory barrier implementation, which in some rare cases may
+	 * cause the writes not to be observable after wmb().
+	 */
+
+	/* read back to ensure the data is in RAM */
+	readl(&txn->last_pat->next_pa);
 
 	/* write to PAT_DESCR to clear out any pending transaction */
 	writel(0x0, dmm->base + reg[PAT_DESCR][engine->id]);
-- 
2.5.0

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

[PATCH 04/33] drm/omap: add dmm_read() and dmm_write() wrappers

[Tomi Valkeinen]

This patch adds wrapper functions for readl() and writel(), dmm_read()
and dmm_write(), so that we can implement workaround for errata i878.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index 4e04f9487375..fe5260477b52 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -79,6 +79,16 @@ static const uint32_t reg[][4] = {
 			DMM_PAT_DESCR__2, DMM_PAT_DESCR__3},
 };
 
+static u32 dmm_read(struct dmm *dmm, u32 reg)
+{
+	return readl(dmm->base + reg);
+}
+
+static void dmm_write(struct dmm *dmm, u32 val, u32 reg)
+{
+	writel(val, dmm->base + reg);
+}
+
 /* simple allocator to grab next 16 byte aligned memory from txn */
 static void *alloc_dma(struct dmm_txn *txn, size_t sz, dma_addr_t *pa)
 {
@@ -108,7 +118,7 @@ static int wait_status(struct refill_engine *engine, uint32_t wait_mask)
 
 	i = DMM_FIXED_RETRY_COUNT;
 	while (true) {
-		r = readl(dmm->base + reg[PAT_STATUS][engine->id]);
+		r = dmm_read(dmm, reg[PAT_STATUS][engine->id]);
 		err = r & DMM_PATSTATUS_ERR;
 		if (err)
 			return -EFAULT;
@@ -140,11 +150,11 @@ static void release_engine(struct refill_engine *engine)
 static irqreturn_t omap_dmm_irq_handler(int irq, void *arg)
 {
 	struct dmm *dmm = arg;
-	uint32_t status = readl(dmm->base + DMM_PAT_IRQSTATUS);
+	uint32_t status = dmm_read(dmm, DMM_PAT_IRQSTATUS);
 	int i;
 
 	/* ack IRQ */
-	writel(status, dmm->base + DMM_PAT_IRQSTATUS);
+	dmm_write(dmm, status, DMM_PAT_IRQSTATUS);
 
 	for (i = 0; i < dmm->num_engines; i++) {
 		if (status & DMM_IRQSTAT_LST) {
@@ -275,7 +285,7 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool wait)
 	readl(&txn->last_pat->next_pa);
 
 	/* write to PAT_DESCR to clear out any pending transaction */
-	writel(0x0, dmm->base + reg[PAT_DESCR][engine->id]);
+	dmm_write(dmm, 0x0, reg[PAT_DESCR][engine->id]);
 
 	/* wait for engine ready: */
 	ret = wait_status(engine, DMM_PATSTATUS_READY);
@@ -291,8 +301,7 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool wait)
 	smp_mb();
 
 	/* kick reload */
-	writel(engine->refill_pa,
-		dmm->base + reg[PAT_DESCR][engine->id]);
+	dmm_write(dmm, engine->refill_pa, reg[PAT_DESCR][engine->id]);
 
 	if (wait) {
 		if (!wait_for_completion_timeout(&engine->compl,
@@ -659,7 +668,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 
 	omap_dmm->dev = &dev->dev;
 
-	hwinfo = readl(omap_dmm->base + DMM_PAT_HWINFO);
+	hwinfo = dmm_read(omap_dmm, DMM_PAT_HWINFO);
 	omap_dmm->num_engines = (hwinfo >> 24) & 0x1F;
 	omap_dmm->num_lut = (hwinfo >> 16) & 0x1F;
 	omap_dmm->container_width = 256;
@@ -668,7 +677,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 	atomic_set(&omap_dmm->engine_counter, omap_dmm->num_engines);
 
 	/* read out actual LUT width and height */
-	pat_geom = readl(omap_dmm->base + DMM_PAT_GEOMETRY);
+	pat_geom = dmm_read(omap_dmm, DMM_PAT_GEOMETRY);
 	omap_dmm->lut_width = ((pat_geom >> 16) & 0xF) << 5;
 	omap_dmm->lut_height = ((pat_geom >> 24) & 0xF) << 5;
 
@@ -678,12 +687,12 @@ static int omap_dmm_probe(struct platform_device *dev)
 		omap_dmm->num_lut++;
 
 	/* initialize DMM registers */
-	writel(0x88888888, omap_dmm->base + DMM_PAT_VIEW__0);
-	writel(0x88888888, omap_dmm->base + DMM_PAT_VIEW__1);
-	writel(0x80808080, omap_dmm->base + DMM_PAT_VIEW_MAP__0);
-	writel(0x80000000, omap_dmm->base + DMM_PAT_VIEW_MAP_BASE);
-	writel(0x88888888, omap_dmm->base + DMM_TILER_OR__0);
-	writel(0x88888888, omap_dmm->base + DMM_TILER_OR__1);
+	dmm_write(omap_dmm, 0x88888888, DMM_PAT_VIEW__0);
+	dmm_write(omap_dmm, 0x88888888, DMM_PAT_VIEW__1);
+	dmm_write(omap_dmm, 0x80808080, DMM_PAT_VIEW_MAP__0);
+	dmm_write(omap_dmm, 0x80000000, DMM_PAT_VIEW_MAP_BASE);
+	dmm_write(omap_dmm, 0x88888888, DMM_TILER_OR__0);
+	dmm_write(omap_dmm, 0x88888888, DMM_TILER_OR__1);
 
 	ret = request_irq(omap_dmm->irq, omap_dmm_irq_handler, IRQF_SHARED,
 				"omap_dmm_irq_handler", omap_dmm);
@@ -701,7 +710,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 	 * buffers for accelerated pan/scroll) and FILL_DSC<n> which
 	 * we just generally don't care about.
 	 */
-	writel(0x7e7e7e7e, omap_dmm->base + DMM_PAT_IRQENABLE_SET);
+	dmm_write(omap_dmm, 0x7e7e7e7e, DMM_PAT_IRQENABLE_SET);
 
 	omap_dmm->dummy_page = alloc_page(GFP_KERNEL | __GFP_DMA32);
 	if (!omap_dmm->dummy_page) {
-- 
2.5.0

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

[PATCH 05/33] drm/omap: partial workaround for DRA7 DMM errata i878

[Tomi Valkeinen]

Errata i878 says that MPU should not be used to access RAM and DMM at
the same time. As it's not possible to prevent MPU accessing RAM, we
need to access DMM via a proxy.

This patch changes DMM driver to access DMM registers via sDMA. Instead
of doing a normal readl/writel call to read/write a register, we use
sDMA to copy 4 bytes from/to the DMM registers.

This patch provides only a partial workaround for i878, as not only DMM
register reads/writes are affected, but also accesses to the DMM mapped
buffers (framebuffers, usually).

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 Documentation/devicetree/bindings/arm/omap/dmm.txt |   3 +-
 drivers/gpu/drm/omapdrm/omap_dmm_priv.h            |   8 ++
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c           | 141 ++++++++++++++++++++-
 3 files changed, 149 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/omap/dmm.txt b/Documentation/devicetree/bindings/arm/omap/dmm.txt
index 8bd6d0a238a8..e5fc2eb7f4da 100644
--- a/Documentation/devicetree/bindings/arm/omap/dmm.txt
+++ b/Documentation/devicetree/bindings/arm/omap/dmm.txt
@@ -8,7 +8,8 @@ translation for initiators which need contiguous dma bus addresses.
 
 Required properties:
 - compatible:	Should contain "ti,omap4-dmm" for OMAP4 family
-		Should contain "ti,omap5-dmm" for OMAP5 and DRA7x family
+		Should contain "ti,omap5-dmm" for OMAP5 family
+		Should contain "ti,dra7-dmm" for DRA7x family
 - reg:		Contains DMM register address range (base address and length)
 - interrupts:	Should contain an interrupt-specifier for DMM_IRQ.
 - ti,hwmods:	Name of the hwmod associated to DMM, which is typically "dmm"
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_priv.h b/drivers/gpu/drm/omapdrm/omap_dmm_priv.h
index 9f32a83ca507..4d292ba5bcca 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_priv.h
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_priv.h
@@ -155,10 +155,12 @@ struct refill_engine {
 
 struct dmm_platform_data {
 	uint32_t cpu_cache_flags;
+	bool errata_i878_wa;
 };
 
 struct dmm {
 	struct device *dev;
+	u32 phys_base;
 	void __iomem *base;
 	int irq;
 
@@ -189,6 +191,12 @@ struct dmm {
 	struct list_head alloc_head;
 
 	const struct dmm_platform_data *plat_data;
+
+	bool dmm_workaround;
+	spinlock_t wa_lock;
+	u32 *wa_dma_data;
+	dma_addr_t wa_dma_handle;
+	struct dma_chan *wa_dma_chan;
 };
 
 #endif
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index fe5260477b52..3ec5c6633585 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -19,6 +19,7 @@
 #include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
@@ -79,14 +80,124 @@ static const uint32_t reg[][4] = {
 			DMM_PAT_DESCR__2, DMM_PAT_DESCR__3},
 };
 
+static int dmm_dma_copy(struct dmm *dmm, u32 src, u32 dst)
+{
+	struct dma_device *dma_dev = dmm->wa_dma_chan->device;
+	struct dma_async_tx_descriptor *tx = NULL;
+	enum dma_status status;
+	dma_cookie_t cookie;
+
+	tx = dma_dev->device_prep_dma_memcpy(dmm->wa_dma_chan, dst, src, 4, 0);
+	if (!tx) {
+		dev_err(dmm->dev, "Failed to prepare DMA memcpy\n");
+		return -EIO;
+	}
+
+	cookie = tx->tx_submit(tx);
+	if (dma_submit_error(cookie)) {
+		dev_err(dmm->dev, "Failed to do DMA tx_submit\n");
+		return -EIO;
+	}
+
+	dma_async_issue_pending(dmm->wa_dma_chan);
+	status = dma_sync_wait(dmm->wa_dma_chan, cookie);
+	if (status != DMA_COMPLETE)
+		dev_err(dmm->dev, "i878 wa DMA copy failure\n");
+
+	dmaengine_terminate_all(dmm->wa_dma_chan);
+	return 0;
+}
+
+static u32 dmm_read_wa(struct dmm *dmm, u32 reg)
+{
+	u32 src, dst;
+	int r;
+
+	src = (u32)(dmm->phys_base + reg);
+	dst = (u32)dmm->wa_dma_handle;
+
+	r = dmm_dma_copy(dmm, src, dst);
+	if (r) {
+		dev_err(dmm->dev, "sDMA read transfer timeout\n");
+		return readl(dmm->base + reg);
+	}
+
+	return readl(dmm->wa_dma_data);
+}
+
+static void dmm_write_wa(struct dmm *dmm, u32 val, u32 reg)
+{
+	u32 src, dst;
+	int r;
+
+	writel(val, dmm->wa_dma_data);
+
+	src = (u32)dmm->wa_dma_handle;
+	dst = (u32)(dmm->phys_base + reg);
+
+	r = dmm_dma_copy(dmm, src, dst);
+	if (r) {
+		dev_err(dmm->dev, "sDMA write transfer timeout\n");
+		writel(val, dmm->base + reg);
+	}
+}
+
 static u32 dmm_read(struct dmm *dmm, u32 reg)
 {
-	return readl(dmm->base + reg);
+	if (dmm->dmm_workaround) {
+		u32 v;
+		unsigned long flags;
+
+		spin_lock_irqsave(&dmm->wa_lock, flags);
+		v = dmm_read_wa(dmm, reg);
+		spin_unlock_irqrestore(&dmm->wa_lock, flags);
+
+		return v;
+	} else {
+		return readl(dmm->base + reg);
+	}
 }
 
 static void dmm_write(struct dmm *dmm, u32 val, u32 reg)
 {
-	writel(val, dmm->base + reg);
+	if (dmm->dmm_workaround) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&dmm->wa_lock, flags);
+		dmm_write_wa(dmm, val, reg);
+		spin_unlock_irqrestore(&dmm->wa_lock, flags);
+	} else {
+		writel(val, dmm->base + reg);
+	}
+}
+
+static int dmm_workaround_init(struct dmm *dmm)
+{
+	dma_cap_mask_t mask;
+
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_MEMCPY, mask);
+
+	spin_lock_init(&dmm->wa_lock);
+
+	dmm->wa_dma_data = dma_alloc_coherent(dmm->dev, 4, &dmm->wa_dma_handle, GFP_KERNEL);
+	if (!dmm->wa_dma_data)
+		return -ENOMEM;
+
+	dmm->wa_dma_chan = dma_request_channel(mask, NULL, NULL);
+	if (!dmm->wa_dma_chan) {
+		dma_free_coherent(dmm->dev, 4, dmm->wa_dma_data, dmm->wa_dma_handle);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static void dmm_workaround_uninit(struct dmm *dmm)
+{
+	dma_release_channel(dmm->wa_dma_chan);
+
+	dma_free_coherent(dmm->dev, 4, dmm->wa_dma_data, dmm->wa_dma_handle);
 }
 
 /* simple allocator to grab next 16 byte aligned memory from txn */
@@ -606,6 +717,9 @@ static int omap_dmm_remove(struct platform_device *dev)
 		if (omap_dmm->dummy_page)
 			__free_page(omap_dmm->dummy_page);
 
+		if (omap_dmm->dmm_workaround)
+			dmm_workaround_uninit(omap_dmm);
+
 		if (omap_dmm->irq > 0)
 			free_irq(omap_dmm->irq, omap_dmm);
 
@@ -653,6 +767,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 		goto fail;
 	}
 
+	omap_dmm->phys_base = mem->start;
 	omap_dmm->base = ioremap(mem->start, SZ_2K);
 
 	if (!omap_dmm->base) {
@@ -668,6 +783,19 @@ static int omap_dmm_probe(struct platform_device *dev)
 
 	omap_dmm->dev = &dev->dev;
 
+	omap_dmm->dmm_workaround = omap_dmm->plat_data->errata_i878_wa;
+
+	if (omap_dmm->dmm_workaround) {
+		int r;
+		r = dmm_workaround_init(omap_dmm);
+		if (r) {
+			omap_dmm->dmm_workaround = false;
+			dev_err(&dev->dev, "failed to initialize work-around, WA not used\n");
+		} else {
+			dev_info(&dev->dev, "workaround for errata i878 in use\n");
+		}
+	}
+
 	hwinfo = dmm_read(omap_dmm, DMM_PAT_HWINFO);
 	omap_dmm->num_engines = (hwinfo >> 24) & 0x1F;
 	omap_dmm->num_lut = (hwinfo >> 16) & 0x1F;
@@ -1031,6 +1159,11 @@ static const struct dmm_platform_data dmm_omap5_platform_data = {
 	.cpu_cache_flags = OMAP_BO_UNCACHED,
 };
 
+static const struct dmm_platform_data dmm_dra7_platform_data = {
+	.cpu_cache_flags = OMAP_BO_UNCACHED,
+	.errata_i878_wa = true,
+};
+
 static const struct of_device_id dmm_of_match[] = {
 	{
 		.compatible = "ti,omap4-dmm",
@@ -1040,6 +1173,10 @@ static const struct of_device_id dmm_of_match[] = {
 		.compatible = "ti,omap5-dmm",
 		.data = &dmm_omap5_platform_data,
 	},
+	{
+		.compatible = "ti,dra7-dmm",
+		.data = &dmm_dra7_platform_data,
+	},
 	{},
 };
 #endif
-- 
2.5.0

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

[PATCH 06/33] drm/omap: drm_atomic_get_plane_state() may return ERR_PTR

[Tomi Valkeinen]

From: Jyri Sarha <jsarha@ti.com>

drm_atomic_get_plane_state() may return ERR_PTR. Handle
drm_atomic_get_plane_state() return values right in
omap_crtc_atomic_set_property().

Signed-off-by: Jyri Sarha <jsarha@ti.com>
Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 7dd3d44a93e5..f5b19d18fa8b 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -433,8 +433,8 @@ static int omap_crtc_atomic_set_property(struct drm_crtc *crtc,
 	 */
 
 	plane_state = drm_atomic_get_plane_state(state->state, plane);
-	if (!plane_state)
-		return -EINVAL;
+	if (IS_ERR(plane_state))
+		return PTR_ERR(plane_state);
 
 	return drm_atomic_plane_set_property(plane, plane_state, property, val);
 }
-- 
2.5.0

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

[PATCH 07/33] drm/omap: tpd12s015: remove platform data support

[Tomi Valkeinen]

From: Manisha Agrawal <manisha.agrawal@ti.com>

All devices using tpd12s015 driver are doing DT boot. No need of further
supporting the platform data. This patch removes support for platform
data.

Signed-off-by: Manisha Agrawal <manisha.agrawal@ti.com>
[tomi.valkeinen@ti.com: minor adjustments]
Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../gpu/drm/omapdrm/displays/encoder-tpd12s015.c   | 41 +++-------------------
 include/video/omap-panel-data.h                    | 15 --------
 2 files changed, 5 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c b/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c
index 990af6baeb0f..49fbad03a814 100644
--- a/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c
+++ b/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c
@@ -201,32 +201,6 @@ static const struct omapdss_hdmi_ops tpd_hdmi_ops = {
 	.set_hdmi_mode		= tpd_set_hdmi_mode,
 };
 
-static int tpd_probe_pdata(struct platform_device *pdev)
-{
-	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
-	struct encoder_tpd12s015_platform_data *pdata;
-	struct omap_dss_device *dssdev, *in;
-
-	pdata = dev_get_platdata(&pdev->dev);
-
-	ddata->ct_cp_hpd_gpio = pdata->ct_cp_hpd_gpio;
-	ddata->ls_oe_gpio = pdata->ls_oe_gpio;
-	ddata->hpd_gpio = pdata->hpd_gpio;
-
-	in = omap_dss_find_output(pdata->source);
-	if (in == NULL) {
-		dev_err(&pdev->dev, "Failed to find video source\n");
-		return -ENODEV;
-	}
-
-	ddata->in = in;
-
-	dssdev = &ddata->dssdev;
-	dssdev->name = pdata->name;
-
-	return 0;
-}
-
 static int tpd_probe_of(struct platform_device *pdev)
 {
 	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
@@ -282,17 +256,12 @@ static int tpd_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, ddata);
 
-	if (dev_get_platdata(&pdev->dev)) {
-		r = tpd_probe_pdata(pdev);
-		if (r)
-			return r;
-	} else if (pdev->dev.of_node) {
-		r = tpd_probe_of(pdev);
-		if (r)
-			return r;
-	} else {
+	if (!pdev->dev.of_node)
 		return -ENODEV;
-	}
+
+	r = tpd_probe_of(pdev);
+	if (r)
+		return r;
 
 	r = devm_gpio_request_one(&pdev->dev, ddata->ct_cp_hpd_gpio,
 			GPIOF_OUT_INIT_LOW, "hdmi_ct_cp_hpd");
diff --git a/include/video/omap-panel-data.h b/include/video/omap-panel-data.h
index 69279c013ac4..56830d1dc762 100644
--- a/include/video/omap-panel-data.h
+++ b/include/video/omap-panel-data.h
@@ -45,21 +45,6 @@ struct encoder_tfp410_platform_data {
 	int data_lines;
 };
 
-/**
- * encoder_tpd12s015 platform data
- * @name: name for this display entity
- * @ct_cp_hpd_gpio: CT_CP_HPD gpio number
- * @ls_oe_gpio: LS_OE gpio number
- * @hpd_gpio: HPD gpio number
- */
-struct encoder_tpd12s015_platform_data {
-	const char *name;
-	const char *source;
-
-	int ct_cp_hpd_gpio;
-	int ls_oe_gpio;
-	int hpd_gpio;
-};
 
 /**
  * connector_dvi platform data
-- 
2.5.0

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

[PATCH 08/33] drm/omap: tpd12s015: gpio descriptor API

[Tomi Valkeinen]

From: Manisha Agrawal <manisha.agrawal@ti.com>

Migrated the gpio APIs to descriptor-interface based.

Signed-off-by: Manisha Agrawal <manisha.agrawal@ti.com>
Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../gpu/drm/omapdrm/displays/encoder-tpd12s015.c   | 79 ++++++++--------------
 1 file changed, 28 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c b/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c
index 49fbad03a814..7fa80f5b4c6b 100644
--- a/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c
+++ b/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c
@@ -13,9 +13,8 @@
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/slab.h>
-#include <linux/gpio.h>
 #include <linux/platform_device.h>
-#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>
 
 #include <video/omapdss.h>
 #include <video/omap-panel-data.h>
@@ -24,9 +23,9 @@ struct panel_drv_data {
 	struct omap_dss_device dssdev;
 	struct omap_dss_device *in;
 
-	int ct_cp_hpd_gpio;
-	int ls_oe_gpio;
-	int hpd_gpio;
+	struct gpio_desc *ct_cp_hpd_gpio;
+	struct gpio_desc *ls_oe_gpio;
+	struct gpio_desc *hpd_gpio;
 
 	struct omap_video_timings timings;
 };
@@ -47,7 +46,7 @@ static int tpd_connect(struct omap_dss_device *dssdev,
 	dst->src = dssdev;
 	dssdev->dst = dst;
 
-	gpio_set_value_cansleep(ddata->ct_cp_hpd_gpio, 1);
+	gpiod_set_value_cansleep(ddata->ct_cp_hpd_gpio, 1);
 	/* DC-DC converter needs at max 300us to get to 90% of 5V */
 	udelay(300);
 
@@ -65,7 +64,7 @@ static void tpd_disconnect(struct omap_dss_device *dssdev,
 	if (dst != dssdev->dst)
 		return;
 
-	gpio_set_value_cansleep(ddata->ct_cp_hpd_gpio, 0);
+	gpiod_set_value_cansleep(ddata->ct_cp_hpd_gpio, 0);
 
 	dst->src = NULL;
 	dssdev->dst = NULL;
@@ -145,16 +144,14 @@ static int tpd_read_edid(struct omap_dss_device *dssdev,
 	struct omap_dss_device *in = ddata->in;
 	int r;
 
-	if (!gpio_get_value_cansleep(ddata->hpd_gpio))
+	if (!gpiod_get_value_cansleep(ddata->hpd_gpio))
 		return -ENODEV;
 
-	if (gpio_is_valid(ddata->ls_oe_gpio))
-		gpio_set_value_cansleep(ddata->ls_oe_gpio, 1);
+	gpiod_set_value_cansleep(ddata->ls_oe_gpio, 1);
 
 	r = in->ops.hdmi->read_edid(in, edid, len);
 
-	if (gpio_is_valid(ddata->ls_oe_gpio))
-		gpio_set_value_cansleep(ddata->ls_oe_gpio, 0);
+	gpiod_set_value_cansleep(ddata->ls_oe_gpio, 0);
 
 	return r;
 }
@@ -163,7 +160,7 @@ static bool tpd_detect(struct omap_dss_device *dssdev)
 {
 	struct panel_drv_data *ddata = to_panel_data(dssdev);
 
-	return gpio_get_value_cansleep(ddata->hpd_gpio);
+	return gpiod_get_value_cansleep(ddata->hpd_gpio);
 }
 
 static int tpd_set_infoframe(struct omap_dss_device *dssdev,
@@ -206,32 +203,6 @@ static int tpd_probe_of(struct platform_device *pdev)
 	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
 	struct device_node *node = pdev->dev.of_node;
 	struct omap_dss_device *in;
-	int gpio;
-
-	/* CT CP HPD GPIO */
-	gpio = of_get_gpio(node, 0);
-	if (!gpio_is_valid(gpio)) {
-		dev_err(&pdev->dev, "failed to parse CT CP HPD gpio\n");
-		return gpio;
-	}
-	ddata->ct_cp_hpd_gpio = gpio;
-
-	/* LS OE GPIO */
-	gpio = of_get_gpio(node, 1);
-	if (gpio_is_valid(gpio) || gpio == -ENOENT) {
-		ddata->ls_oe_gpio = gpio;
-	} else {
-		dev_err(&pdev->dev, "failed to parse LS OE gpio\n");
-		return gpio;
-	}
-
-	/* HPD GPIO */
-	gpio = of_get_gpio(node, 2);
-	if (!gpio_is_valid(gpio)) {
-		dev_err(&pdev->dev, "failed to parse HPD gpio\n");
-		return gpio;
-	}
-	ddata->hpd_gpio = gpio;
 
 	in = omapdss_of_find_source_for_first_ep(node);
 	if (IS_ERR(in)) {
@@ -249,6 +220,7 @@ static int tpd_probe(struct platform_device *pdev)
 	struct omap_dss_device *in, *dssdev;
 	struct panel_drv_data *ddata;
 	int r;
+	struct gpio_desc *gpio;
 
 	ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL);
 	if (!ddata)
@@ -263,23 +235,28 @@ static int tpd_probe(struct platform_device *pdev)
 	if (r)
 		return r;
 
-	r = devm_gpio_request_one(&pdev->dev, ddata->ct_cp_hpd_gpio,
-			GPIOF_OUT_INIT_LOW, "hdmi_ct_cp_hpd");
-	if (r)
+
+	gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0,
+		 GPIOD_OUT_LOW);
+	if (IS_ERR(gpio))
 		goto err_gpio;
 
-	if (gpio_is_valid(ddata->ls_oe_gpio)) {
-		r = devm_gpio_request_one(&pdev->dev, ddata->ls_oe_gpio,
-				GPIOF_OUT_INIT_LOW, "hdmi_ls_oe");
-		if (r)
-			goto err_gpio;
-	}
+	ddata->ct_cp_hpd_gpio = gpio;
 
-	r = devm_gpio_request_one(&pdev->dev, ddata->hpd_gpio,
-			GPIOF_DIR_IN, "hdmi_hpd");
-	if (r)
+	gpio = devm_gpiod_get_index_optional(&pdev->dev, NULL, 1,
+		 GPIOD_OUT_LOW);
+	if (IS_ERR(gpio))
 		goto err_gpio;
 
+	ddata->ls_oe_gpio = gpio;
+
+	gpio = devm_gpiod_get_index(&pdev->dev, NULL, 2,
+		GPIOD_IN);
+	if (IS_ERR(gpio))
+		goto err_gpio;
+
+	ddata->hpd_gpio = gpio;
+
 	dssdev = &ddata->dssdev;
 	dssdev->ops.hdmi = &tpd_hdmi_ops;
 	dssdev->dev = &pdev->dev;
-- 
2.5.0

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

[PATCH 09/33] drm/omap: tpd12s015: CT_CP_HPD as optional gpio

[Tomi Valkeinen]

From: Manisha Agrawal <manisha.agrawal@ti.com>

tpd12s015 HW has LS_OE, CT_CP_HPD and HPD gpios. Out of these gpios,
driver only handled LS_OE as optional. The CT_CP_HPD gpio should also
be treated as optional gpio as it is just a power saving feature. Some
boards hardwire this gpio to be always enable. In this patch, all access
to CT_CP_HPD gpio is made optional.

Signed-off-by: Manisha Agrawal <manisha.agrawal@ti.com>
Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c b/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c
index 7fa80f5b4c6b..916a89978387 100644
--- a/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c
+++ b/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c
@@ -236,7 +236,7 @@ static int tpd_probe(struct platform_device *pdev)
 		return r;
 
 
-	gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0,
+	gpio = devm_gpiod_get_index_optional(&pdev->dev, NULL, 0,
 		 GPIOD_OUT_LOW);
 	if (IS_ERR(gpio))
 		goto err_gpio;
-- 
2.5.0

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

[PATCH 10/33] drm/omap: add define for DISPC_IRQ_WBUNCOMPLETEERROR

[Tomi Valkeinen]

OMAP4+ DSS has WBUNCOMPLETEERROR irq, which was not defined in the irq
list. Add the define.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 include/video/omapdss.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/video/omapdss.h b/include/video/omapdss.h
index 295b41e20d8e..86f28a92281a 100644
--- a/include/video/omapdss.h
+++ b/include/video/omapdss.h
@@ -51,6 +51,7 @@
 #define DISPC_IRQ_FRAMEDONEWB		(1 << 23)
 #define DISPC_IRQ_FRAMEDONETV		(1 << 24)
 #define DISPC_IRQ_WBBUFFEROVERFLOW	(1 << 25)
+#define DISPC_IRQ_WBUNCOMPLETEERROR	(1 << 26)
 #define DISPC_IRQ_SYNC_LOST3		(1 << 27)
 #define DISPC_IRQ_VSYNC3		(1 << 28)
 #define DISPC_IRQ_ACBIAS_COUNT_STAT3	(1 << 29)
-- 
2.5.0

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

[PATCH 11/33] drm/omap: use dma_mapping_error in omap_gem_attach_pages

[Tomi Valkeinen]

omap_gem_attach_pages() calls dma_map_page() but does not check the
possible error with dma_mapping_error(). If DMA-API debugging is
enabled, the debug layer will give a warning if dma_mapping_error() has
not been used.

This patch adds proper error handling to omap_gem_attach_pages().

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

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 8495a1a4b617..7098190815f1 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -264,6 +264,18 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj)
 		for (i = 0; i < npages; i++) {
 			addrs[i] = dma_map_page(dev->dev, pages[i],
 					0, PAGE_SIZE, DMA_BIDIRECTIONAL);
+
+			if (dma_mapping_error(dev->dev, addrs[i])) {
+				dev_warn(dev->dev, "omap_gem_attach_pages: dma_map_page failed\n");
+
+				for (i = i - 1; i >= 0; --i) {
+					dma_unmap_page(dev->dev, addrs[i],
+						PAGE_SIZE, DMA_BIDIRECTIONAL);
+				}
+
+				ret = -ENOMEM;
+				goto free_addrs;
+			}
 		}
 	} else {
 		addrs = kzalloc(npages * sizeof(*addrs), GFP_KERNEL);
@@ -277,6 +289,8 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj)
 	omap_obj->pages = pages;
 
 	return 0;
+free_addrs:
+	kfree(addrs);
 
 free_pages:
 	drm_gem_put_pages(obj, pages, true, false);
-- 
2.5.0

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

[PATCH 12/33] drm/omap: use dma_mapping_error in omap_gem_dma_sync

[Tomi Valkeinen]

omap_gem_dma_sync() calls dma_map_page() but does not check the possible
error with dma_mapping_error(). If DMA-API debugging is enabled, the
debug layer will give a warning if dma_mapping_error() has not been
used.

This patch adds proper error handling to omap_gem_dma_sync().

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

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 7098190815f1..a60c30a59f7e 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -775,9 +775,18 @@ void omap_gem_dma_sync(struct drm_gem_object *obj,
 
 		for (i = 0; i < npages; i++) {
 			if (!omap_obj->addrs[i]) {
-				omap_obj->addrs[i] = dma_map_page(dev->dev, pages[i], 0,
+				dma_addr_t addr;
+
+				addr = dma_map_page(dev->dev, pages[i], 0,
 						PAGE_SIZE, DMA_BIDIRECTIONAL);
+
+				if (dma_mapping_error(dev->dev, addr)) {
+					dev_warn(dev->dev, "omap_gem_dma_sync: dma_map_page failed\n");
+					break;
+				}
+
 				dirty = true;
+				omap_obj->addrs[i] = addr;
 			}
 		}
 
-- 
2.5.0

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

[PATCH 13/33] drm/omap: print an error if display enable fails

[Tomi Valkeinen]

If the panel's enable fails, omap_encoder silently ignores the failure.
omapdrm should really handle the failure, but unfortunately the whole
encoder enable codepath is expected to always succeed.

So for now, catch the enable failure and print an error.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c
index 61714e9670ae..eb52b3e85d0c 100644
--- a/drivers/gpu/drm/omapdrm/omap_encoder.c
+++ b/drivers/gpu/drm/omapdrm/omap_encoder.c
@@ -139,11 +139,15 @@ static void omap_encoder_enable(struct drm_encoder *encoder)
 	struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
 	struct omap_dss_device *dssdev = omap_encoder->dssdev;
 	struct omap_dss_driver *dssdrv = dssdev->driver;
+	int r;
 
 	omap_encoder_update(encoder, omap_crtc_channel(encoder->crtc),
 			    omap_crtc_timings(encoder->crtc));
 
-	dssdrv->enable(dssdev);
+	r = dssdrv->enable(dssdev);
+	if (r)
+		dev_err(encoder->dev->dev, "Failed to enable display '%s'\n",
+			dssdev->name);
 }
 
 static int omap_encoder_atomic_check(struct drm_encoder *encoder,
-- 
2.5.0

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

[PATCH 14/33] drm/omap: gem: Clean up GEM objects memory flags

[Tomi Valkeinen]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

The driver assumes that only objects backed by shmem need to be mapped
through DMM. While this is true with the current code, the assumption
won't hold with dma_buf import support.

Condition the mapping based on whether the buffer has been allocated
using the DMA mapping API instead and clean up the flags to avoid having
to check both flags and GEM object filp field to decide how to process
buffers. Flags are not the authoritative source of information regarding
where the buffer memory comes from, and are renamed to make that
clearer.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_gem.c | 57 +++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index a60c30a59f7e..aa309eb82a97 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -31,9 +31,10 @@
  */
 
 /* note: we use upper 8 bits of flags for driver-internal flags: */
-#define OMAP_BO_DMA		0x01000000	/* actually is physically contiguous */
-#define OMAP_BO_EXT_SYNC	0x02000000	/* externally allocated sync object */
-#define OMAP_BO_EXT_MEM		0x04000000	/* externally allocated memory */
+#define OMAP_BO_MEM_DMA_API	0x01000000	/* memory allocated with the dma_alloc_* API */
+#define OMAP_BO_MEM_SHMEM	0x02000000	/* memory allocated through shmem backing */
+#define OMAP_BO_MEM_EXT		0x04000000	/* memory allocated externally */
+#define OMAP_BO_EXT_SYNC	0x10000000	/* externally allocated sync object */
 
 struct omap_gem_object {
 	struct drm_gem_object base;
@@ -49,16 +50,16 @@ struct omap_gem_object {
 	uint32_t roll;
 
 	/**
-	 * If buffer is allocated physically contiguous, the OMAP_BO_DMA flag
-	 * is set and the paddr is valid.  Also if the buffer is remapped in
-	 * TILER and paddr_cnt > 0, then paddr is valid.  But if you are using
-	 * the physical address and OMAP_BO_DMA is not set, then you should
-	 * be going thru omap_gem_{get,put}_paddr() to ensure the mapping is
-	 * not removed from under your feet.
+	 * If buffer is allocated physically contiguous, the OMAP_BO_MEM_DMA_API
+	 * flag is set and the paddr is valid.  Also if the buffer is remapped
+	 * in TILER and paddr_cnt > 0, then paddr is valid. But if you are using
+	 * the physical address and OMAP_BO_MEM_DMA_API is not set, then you
+	 * should be going thru omap_gem_{get,put}_paddr() to ensure the mapping
+	 * is not removed from under your feet.
 	 *
 	 * Note that OMAP_BO_SCANOUT is a hint from userspace that DMA capable
-	 * buffer is requested, but doesn't mean that it is.  Use the
-	 * OMAP_BO_DMA flag to determine if the buffer has a DMA capable
+	 * buffer is requested, but doesn't mean that it is. Use the
+	 * OMAP_BO_MEM_DMA_API flag to determine if the buffer has a DMA capable
 	 * physical address.
 	 */
 	dma_addr_t paddr;
@@ -166,18 +167,6 @@ static uint64_t mmap_offset(struct drm_gem_object *obj)
 	return drm_vma_node_offset_addr(&obj->vma_node);
 }
 
-/* GEM objects can either be allocated from contiguous memory (in which
- * case obj->filp==NULL), or w/ shmem backing (obj->filp!=NULL).  But non
- * contiguous buffers can be remapped in TILER/DMM if they need to be
- * contiguous... but we don't do this all the time to reduce pressure
- * on TILER/DMM space when we know at allocation time that the buffer
- * will need to be scanned out.
- */
-static inline bool is_shmem(struct drm_gem_object *obj)
-{
-	return obj->filp != NULL;
-}
-
 /* -----------------------------------------------------------------------------
  * Eviction
  */
@@ -306,7 +295,7 @@ static int get_pages(struct drm_gem_object *obj, struct page ***pages)
 	struct omap_gem_object *omap_obj = to_omap_bo(obj);
 	int ret = 0;
 
-	if (is_shmem(obj) && !omap_obj->pages) {
+	if ((omap_obj->flags & OMAP_BO_MEM_SHMEM) && !omap_obj->pages) {
 		ret = omap_gem_attach_pages(obj);
 		if (ret) {
 			dev_err(obj->dev->dev, "could not attach pages\n");
@@ -410,7 +399,7 @@ static int fault_1d(struct drm_gem_object *obj,
 		omap_gem_cpu_sync(obj, pgoff);
 		pfn = page_to_pfn(omap_obj->pages[pgoff]);
 	} else {
-		BUG_ON(!(omap_obj->flags & OMAP_BO_DMA));
+		BUG_ON(!(omap_obj->flags & OMAP_BO_MEM_DMA_API));
 		pfn = (omap_obj->paddr >> PAGE_SHIFT) + pgoff;
 	}
 
@@ -742,7 +731,8 @@ fail:
 static inline bool is_cached_coherent(struct drm_gem_object *obj)
 {
 	struct omap_gem_object *omap_obj = to_omap_bo(obj);
-	return is_shmem(obj) &&
+
+	return (omap_obj->flags & OMAP_BO_MEM_SHMEM) &&
 		((omap_obj->flags & OMAP_BO_CACHE_MASK) == OMAP_BO_CACHED);
 }
 
@@ -810,7 +800,8 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
 
 	mutex_lock(&obj->dev->struct_mutex);
 
-	if (remap && is_shmem(obj) && priv->has_dmm) {
+	if (!(omap_obj->flags & OMAP_BO_MEM_DMA_API) &&
+	    remap && priv->has_dmm) {
 		if (omap_obj->paddr_cnt == 0) {
 			struct page **pages;
 			uint32_t npages = obj->size >> PAGE_SHIFT;
@@ -857,7 +848,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
 		omap_obj->paddr_cnt++;
 
 		*paddr = omap_obj->paddr;
-	} else if (omap_obj->flags & OMAP_BO_DMA) {
+	} else if (omap_obj->flags & OMAP_BO_MEM_DMA_API) {
 		*paddr = omap_obj->paddr;
 	} else {
 		ret = -EINVAL;
@@ -1348,11 +1339,11 @@ void omap_gem_free_object(struct drm_gem_object *obj)
 	WARN_ON(omap_obj->paddr_cnt > 0);
 
 	/* don't free externally allocated backing memory */
-	if (!(omap_obj->flags & OMAP_BO_EXT_MEM)) {
+	if (!(omap_obj->flags & OMAP_BO_MEM_EXT)) {
 		if (omap_obj->pages)
 			omap_gem_detach_pages(obj);
 
-		if (!is_shmem(obj)) {
+		if (omap_obj->flags & OMAP_BO_MEM_DMA_API) {
 			dma_free_writecombine(dev->dev, obj->size,
 					omap_obj->vaddr, omap_obj->paddr);
 		} else if (omap_obj->vaddr) {
@@ -1426,7 +1417,7 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
 			return NULL;
 		}
 
-		flags |= OMAP_BO_DMA;
+		flags |= OMAP_BO_MEM_DMA_API;
 	}
 
 	spin_lock(&priv->list_lock);
@@ -1440,7 +1431,7 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
 		omap_obj->height = gsize.tiled.height;
 	}
 
-	if (flags & (OMAP_BO_DMA|OMAP_BO_EXT_MEM)) {
+	if (flags & (OMAP_BO_MEM_DMA_API | OMAP_BO_MEM_EXT)) {
 		drm_gem_private_object_init(dev, obj, size);
 	} else {
 		ret = drm_gem_object_init(dev, obj, size);
@@ -1449,6 +1440,8 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
 
 		mapping = file_inode(obj->filp)->i_mapping;
 		mapping_set_gfp_mask(mapping, GFP_USER | __GFP_DMA32);
+
+		omap_obj->flags |= OMAP_BO_MEM_SHMEM;
 	}
 
 	return obj;
-- 
2.5.0

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

[PATCH 15/33] drm/omap: gem: Refactor GEM object allocation

[Tomi Valkeinen]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Split the individual steps of GEM object allocation and initialization
clearly. This improves readability and prepares for dma_buf import
support.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_gem.c | 75 ++++++++++++++++++++++----------------
 1 file changed, 43 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index aa309eb82a97..f0647bcce3fc 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -1371,67 +1371,80 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
 	size_t size;
 	int ret;
 
+	/* Validate the flags and compute the memory and cache flags. */
 	if (flags & OMAP_BO_TILED) {
 		if (!priv->usergart) {
 			dev_err(dev->dev, "Tiled buffers require DMM\n");
 			return NULL;
 		}
 
-		/* tiled buffers are always shmem paged backed.. when they are
-		 * scanned out, they are remapped into DMM/TILER
+		/*
+		 * Tiled buffers are always shmem paged backed. When they are
+		 * scanned out, they are remapped into DMM/TILER.
 		 */
 		flags &= ~OMAP_BO_SCANOUT;
+		flags |= OMAP_BO_MEM_SHMEM;
 
-		/* currently don't allow cached buffers.. there is some caching
-		 * stuff that needs to be handled better
+		/*
+		 * Currently don't allow cached buffers. There is some caching
+		 * stuff that needs to be handled better.
 		 */
 		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),
-				&gsize.tiled.width, &gsize.tiled.height);
-
-		/* ...and calculate size based on aligned dimensions */
-		size = tiler_size(gem2fmt(flags),
-				gsize.tiled.width, gsize.tiled.height);
-	} else {
-		size = PAGE_ALIGN(gsize.bytes);
+	} else if ((flags & OMAP_BO_SCANOUT) && !priv->has_dmm) {
+		/*
+		 * Use contiguous memory if we don't have DMM to remap
+		 * discontiguous buffers.
+		 */
+		flags |= OMAP_BO_MEM_DMA_API;
+	} else if (!(flags & OMAP_BO_MEM_EXT)) {
+		/*
+		 * All other buffers not backed with external memory are
+		 * shmem-backed.
+		 */
+		flags |= OMAP_BO_MEM_SHMEM;
 	}
 
+	/* Allocate the initialize the OMAP GEM object. */
 	omap_obj = kzalloc(sizeof(*omap_obj), GFP_KERNEL);
 	if (!omap_obj)
 		return NULL;
 
 	obj = &omap_obj->base;
+	omap_obj->flags = flags;
 
-	if ((flags & OMAP_BO_SCANOUT) && !priv->has_dmm) {
-		/* attempt to allocate contiguous memory if we don't
-		 * have DMM for remappign discontiguous buffers
+	if (flags & OMAP_BO_TILED) {
+		/*
+		 * For tiled buffers align dimensions to slot boundaries and
+		 * calculate size based on aligned dimensions.
 		 */
-		omap_obj->vaddr =  dma_alloc_writecombine(dev->dev, size,
-				&omap_obj->paddr, GFP_KERNEL);
-		if (!omap_obj->vaddr) {
-			kfree(omap_obj);
+		tiler_align(gem2fmt(flags), &gsize.tiled.width,
+			    &gsize.tiled.height);
 
-			return NULL;
-		}
+		size = tiler_size(gem2fmt(flags), gsize.tiled.width,
+				  gsize.tiled.height);
 
-		flags |= OMAP_BO_MEM_DMA_API;
+		omap_obj->width = gsize.tiled.width;
+		omap_obj->height = gsize.tiled.height;
+	} else {
+		size = PAGE_ALIGN(gsize.bytes);
 	}
 
 	spin_lock(&priv->list_lock);
 	list_add(&omap_obj->mm_list, &priv->obj_list);
 	spin_unlock(&priv->list_lock);
 
-	omap_obj->flags = flags;
-
-	if (flags & OMAP_BO_TILED) {
-		omap_obj->width = gsize.tiled.width;
-		omap_obj->height = gsize.tiled.height;
+	/* Allocate memory if needed. */
+	if (flags & OMAP_BO_MEM_DMA_API) {
+		omap_obj->vaddr = dma_alloc_writecombine(dev->dev, size,
+							 &omap_obj->paddr,
+							 GFP_KERNEL);
+		if (!omap_obj->vaddr)
+			goto fail;
 	}
 
-	if (flags & (OMAP_BO_MEM_DMA_API | OMAP_BO_MEM_EXT)) {
+	/* Initialize the GEM object. */
+	if (!(flags & OMAP_BO_MEM_SHMEM)) {
 		drm_gem_private_object_init(dev, obj, size);
 	} else {
 		ret = drm_gem_object_init(dev, obj, size);
@@ -1440,8 +1453,6 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
 
 		mapping = file_inode(obj->filp)->i_mapping;
 		mapping_set_gfp_mask(mapping, GFP_USER | __GFP_DMA32);
-
-		omap_obj->flags |= OMAP_BO_MEM_SHMEM;
 	}
 
 	return obj;
-- 
2.5.0

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

[PATCH 16/33] drm/omap: gem: Implement dma_buf import

[Tomi Valkeinen]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

OMAP GEM objects backed by dma_buf reuse the current OMAP GEM object
support as much as possible. If the imported buffer is physically
contiguous its physical address will be used directly, reusing the
OMAP_BO_MEM_DMA_API code paths. Otherwise it will be mapped through the
TILER using a pages list created from the scatterlist instead of the
shmem backing storage.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.h        |   2 +
 drivers/gpu/drm/omapdrm/omap_gem.c        | 138 ++++++++++++++++++++++++------
 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c |  53 +++++++++---
 3 files changed, 159 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 9e0030731c37..c077367dcb1a 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -195,6 +195,8 @@ void omap_gem_deinit(struct drm_device *dev);
 
 struct drm_gem_object *omap_gem_new(struct drm_device *dev,
 		union omap_gem_size gsize, uint32_t flags);
+struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size,
+		struct sg_table *sgt);
 int omap_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 		union omap_gem_size gsize, uint32_t flags, uint32_t *handle);
 void omap_gem_free_object(struct drm_gem_object *obj);
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index f0647bcce3fc..db5dbdb8cd0a 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -34,6 +34,7 @@
 #define OMAP_BO_MEM_DMA_API	0x01000000	/* memory allocated with the dma_alloc_* API */
 #define OMAP_BO_MEM_SHMEM	0x02000000	/* memory allocated through shmem backing */
 #define OMAP_BO_MEM_EXT		0x04000000	/* memory allocated externally */
+#define OMAP_BO_MEM_DMABUF	0x08000000	/* memory imported from a dmabuf */
 #define OMAP_BO_EXT_SYNC	0x10000000	/* externally allocated sync object */
 
 struct omap_gem_object {
@@ -50,17 +51,25 @@ struct omap_gem_object {
 	uint32_t roll;
 
 	/**
-	 * If buffer is allocated physically contiguous, the OMAP_BO_MEM_DMA_API
-	 * flag is set and the paddr is valid.  Also if the buffer is remapped
-	 * in TILER and paddr_cnt > 0, then paddr is valid. But if you are using
-	 * the physical address and OMAP_BO_MEM_DMA_API is not set, then you
-	 * should be going thru omap_gem_{get,put}_paddr() to ensure the mapping
-	 * is not removed from under your feet.
+	 * paddr contains the buffer DMA address. It is valid for
 	 *
-	 * Note that OMAP_BO_SCANOUT is a hint from userspace that DMA capable
-	 * buffer is requested, but doesn't mean that it is. Use the
-	 * OMAP_BO_MEM_DMA_API flag to determine if the buffer has a DMA capable
-	 * physical address.
+	 * - buffers allocated through the DMA mapping API (with the
+	 *   OMAP_BO_MEM_DMA_API flag set)
+	 *
+	 * - buffers imported from dmabuf (with the OMAP_BO_MEM_DMABUF flag set)
+	 *   if they are physically contiguous (when sgt->orig_nents == 1)
+	 *
+	 * - buffers mapped through the TILER when paddr_cnt is not zero, in
+	 *   which case the DMA address points to the TILER aperture
+	 *
+	 * Physically contiguous buffers have their DMA address equal to the
+	 * physical address as we don't remap those buffers through the TILER.
+	 *
+	 * Buffers mapped to the TILER have their DMA address pointing to the
+	 * TILER aperture. As TILER mappings are refcounted (through paddr_cnt)
+	 * the DMA address must be accessed through omap_get_get_paddr() to
+	 * ensure that the mapping won't disappear unexpectedly. References must
+	 * be released with omap_gem_put_paddr().
 	 */
 	dma_addr_t paddr;
 
@@ -70,6 +79,12 @@ struct omap_gem_object {
 	uint32_t paddr_cnt;
 
 	/**
+	 * If the buffer has been imported from a dmabuf the OMAP_DB_DMABUF flag
+	 * is set and the sgt field is valid.
+	 */
+	struct sg_table *sgt;
+
+	/**
 	 * tiler block used when buffer is remapped in DMM/TILER.
 	 */
 	struct tiler_block *block;
@@ -167,6 +182,17 @@ static uint64_t mmap_offset(struct drm_gem_object *obj)
 	return drm_vma_node_offset_addr(&obj->vma_node);
 }
 
+static bool is_contiguous(struct omap_gem_object *omap_obj)
+{
+	if (omap_obj->flags & OMAP_BO_MEM_DMA_API)
+		return true;
+
+	if ((omap_obj->flags & OMAP_BO_MEM_DMABUF) && omap_obj->sgt->nents == 1)
+		return true;
+
+	return false;
+}
+
 /* -----------------------------------------------------------------------------
  * Eviction
  */
@@ -399,7 +425,7 @@ static int fault_1d(struct drm_gem_object *obj,
 		omap_gem_cpu_sync(obj, pgoff);
 		pfn = page_to_pfn(omap_obj->pages[pgoff]);
 	} else {
-		BUG_ON(!(omap_obj->flags & OMAP_BO_MEM_DMA_API));
+		BUG_ON(!is_contiguous(omap_obj));
 		pfn = (omap_obj->paddr >> PAGE_SHIFT) + pgoff;
 	}
 
@@ -800,8 +826,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
 
 	mutex_lock(&obj->dev->struct_mutex);
 
-	if (!(omap_obj->flags & OMAP_BO_MEM_DMA_API) &&
-	    remap && priv->has_dmm) {
+	if (!is_contiguous(omap_obj) && remap && priv->has_dmm) {
 		if (omap_obj->paddr_cnt == 0) {
 			struct page **pages;
 			uint32_t npages = obj->size >> PAGE_SHIFT;
@@ -848,7 +873,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
 		omap_obj->paddr_cnt++;
 
 		*paddr = omap_obj->paddr;
-	} else if (omap_obj->flags & OMAP_BO_MEM_DMA_API) {
+	} else if (is_contiguous(omap_obj)) {
 		*paddr = omap_obj->paddr;
 	} else {
 		ret = -EINVAL;
@@ -1316,9 +1341,6 @@ unlock:
  * Constructor & Destructor
  */
 
-/* don't call directly.. called from GEM core when it is time to actually
- * free the object..
- */
 void omap_gem_free_object(struct drm_gem_object *obj)
 {
 	struct drm_device *dev = obj->dev;
@@ -1340,14 +1362,20 @@ void omap_gem_free_object(struct drm_gem_object *obj)
 
 	/* don't free externally allocated backing memory */
 	if (!(omap_obj->flags & OMAP_BO_MEM_EXT)) {
-		if (omap_obj->pages)
-			omap_gem_detach_pages(obj);
+		if (omap_obj->pages) {
+			if (omap_obj->flags & OMAP_BO_MEM_DMABUF)
+				kfree(omap_obj->pages);
+			else
+				omap_gem_detach_pages(obj);
+		}
 
 		if (omap_obj->flags & OMAP_BO_MEM_DMA_API) {
 			dma_free_writecombine(dev->dev, obj->size,
 					omap_obj->vaddr, omap_obj->paddr);
 		} else if (omap_obj->vaddr) {
 			vunmap(omap_obj->vaddr);
+		} else if (obj->import_attach) {
+			drm_prime_gem_destroy(obj, omap_obj->sgt);
 		}
 	}
 
@@ -1393,14 +1421,15 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
 		flags |= tiler_get_cpu_cache_flags();
 	} else if ((flags & OMAP_BO_SCANOUT) && !priv->has_dmm) {
 		/*
-		 * Use contiguous memory if we don't have DMM to remap
-		 * discontiguous buffers.
+		 * OMAP_BO_SCANOUT hints that the buffer doesn't need to be
+		 * tiled. However, to lower the pressure on memory allocation,
+		 * use contiguous memory only if no TILER is available.
 		 */
 		flags |= OMAP_BO_MEM_DMA_API;
-	} else if (!(flags & OMAP_BO_MEM_EXT)) {
+	} else if (!(flags & (OMAP_BO_MEM_EXT | OMAP_BO_MEM_DMABUF))) {
 		/*
-		 * All other buffers not backed with external memory are
-		 * shmem-backed.
+		 * All other buffers not backed by external memory or dma_buf
+		 * are shmem-backed.
 		 */
 		flags |= OMAP_BO_MEM_SHMEM;
 	}
@@ -1462,6 +1491,67 @@ fail:
 	return NULL;
 }
 
+struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size,
+					   struct sg_table *sgt)
+{
+	struct omap_drm_private *priv = dev->dev_private;
+	struct omap_gem_object *omap_obj;
+	struct drm_gem_object *obj;
+	union omap_gem_size gsize;
+
+	/* Without a DMM only physically contiguous buffers can be supported. */
+	if (sgt->orig_nents != 1 && !priv->has_dmm)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&dev->struct_mutex);
+
+	gsize.bytes = PAGE_ALIGN(size);
+	obj = omap_gem_new(dev, gsize, OMAP_BO_MEM_DMABUF | OMAP_BO_WC);
+	if (!obj) {
+		obj = ERR_PTR(-ENOMEM);
+		goto done;
+	}
+
+	omap_obj = to_omap_bo(obj);
+	omap_obj->sgt = sgt;
+
+	if (sgt->orig_nents == 1) {
+		omap_obj->paddr = sg_dma_address(sgt->sgl);
+	} else {
+		/* Create pages list from sgt */
+		struct sg_page_iter iter;
+		struct page **pages;
+		unsigned int npages;
+		unsigned int i = 0;
+
+		npages = DIV_ROUND_UP(size, PAGE_SIZE);
+		pages = kcalloc(npages, sizeof(*pages), GFP_KERNEL);
+		if (!pages) {
+			omap_gem_free_object(obj);
+			obj = ERR_PTR(-ENOMEM);
+			goto done;
+		}
+
+		omap_obj->pages = pages;
+
+		for_each_sg_page(sgt->sgl, &iter, sgt->orig_nents, 0) {
+			pages[i++] = sg_page_iter_page(&iter);
+			if (i > npages)
+				break;
+		}
+
+		if (WARN_ON(i != npages)) {
+			omap_gem_free_object(obj);
+			obj = ERR_PTR(-ENOMEM);
+			goto done;
+		}
+	}
+
+done:
+	mutex_unlock(&dev->struct_mutex);
+	return obj;
+}
+
 /* convenience method to construct a GEM buffer object, and userspace handle */
 int omap_gem_new_handle(struct drm_device *dev, struct drm_file *file,
 		union omap_gem_size gsize, uint32_t flags, uint32_t *handle)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
index 27c297672076..19e426b698d3 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
@@ -21,6 +21,10 @@
 
 #include "omap_drv.h"
 
+/* -----------------------------------------------------------------------------
+ * DMABUF Export
+ */
+
 static struct sg_table *omap_gem_map_dma_buf(
 		struct dma_buf_attachment *attachment,
 		enum dma_data_direction dir)
@@ -178,15 +182,20 @@ struct dma_buf *omap_gem_prime_export(struct drm_device *dev,
 	return dma_buf_export(&exp_info);
 }
 
+/* -----------------------------------------------------------------------------
+ * DMABUF Import
+ */
+
 struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
-		struct dma_buf *buffer)
+					     struct dma_buf *dma_buf)
 {
+	struct dma_buf_attachment *attach;
 	struct drm_gem_object *obj;
+	struct sg_table *sgt;
+	int ret;
 
-	/* is this one of own objects? */
-	if (buffer->ops == &omap_dmabuf_ops) {
-		obj = buffer->priv;
-		/* is it from our device? */
+	if (dma_buf->ops == &omap_dmabuf_ops) {
+		obj = dma_buf->priv;
 		if (obj->dev == dev) {
 			/*
 			 * Importing dmabuf exported from out own gem increases
@@ -197,9 +206,33 @@ struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
 		}
 	}
 
-	/*
-	 * TODO add support for importing buffers from other devices..
-	 * for now we don't need this but would be nice to add eventually
-	 */
-	return ERR_PTR(-EINVAL);
+	attach = dma_buf_attach(dma_buf, dev->dev);
+	if (IS_ERR(attach))
+		return ERR_CAST(attach);
+
+	get_dma_buf(dma_buf);
+
+	sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
+	if (IS_ERR(sgt)) {
+		ret = PTR_ERR(sgt);
+		goto fail_detach;
+	}
+
+	obj = omap_gem_new_dmabuf(dev, dma_buf->size, sgt);
+	if (IS_ERR(obj)) {
+		ret = PTR_ERR(obj);
+		goto fail_unmap;
+	}
+
+	obj->import_attach = attach;
+
+	return obj;
+
+fail_unmap:
+	dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
+fail_detach:
+	dma_buf_detach(dma_buf, attach);
+	dma_buf_put(dma_buf);
+
+	return ERR_PTR(ret);
 }
-- 
2.5.0

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

[PATCH 17/33] drm/omap: remove support for ext mem & sync

[Tomi Valkeinen]

We no longer have the omapdrm plugin system for SGX, and we can thus
remove the support for external memory and sync objects from omap_gem.c.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index db5dbdb8cd0a..52e4a6c95058 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -33,9 +33,7 @@
 /* note: we use upper 8 bits of flags for driver-internal flags: */
 #define OMAP_BO_MEM_DMA_API	0x01000000	/* memory allocated with the dma_alloc_* API */
 #define OMAP_BO_MEM_SHMEM	0x02000000	/* memory allocated through shmem backing */
-#define OMAP_BO_MEM_EXT		0x04000000	/* memory allocated externally */
 #define OMAP_BO_MEM_DMABUF	0x08000000	/* memory imported from a dmabuf */
-#define OMAP_BO_EXT_SYNC	0x10000000	/* externally allocated sync object */
 
 struct omap_gem_object {
 	struct drm_gem_object base;
@@ -1177,20 +1175,6 @@ unlock:
 	return ret;
 }
 
-/* it is a bit lame to handle updates in this sort of polling way, but
- * in case of PVR, the GPU can directly update read/write complete
- * values, and not really tell us which ones it updated.. this also
- * means that sync_lock is not quite sufficient.  So we'll need to
- * do something a bit better when it comes time to add support for
- * separate 2d hw..
- */
-void omap_gem_op_update(void)
-{
-	spin_lock(&sync_lock);
-	sync_op_update();
-	spin_unlock(&sync_lock);
-}
-
 /* mark the start of read and/or write operation */
 int omap_gem_op_start(struct drm_gem_object *obj, enum omap_gem_op op)
 {
@@ -1300,43 +1284,6 @@ int omap_gem_op_async(struct drm_gem_object *obj, enum omap_gem_op op,
 	return 0;
 }
 
-/* special API so PVR can update the buffer to use a sync-object allocated
- * from it's sync-obj heap.  Only used for a newly allocated (from PVR's
- * perspective) sync-object, so we overwrite the new syncobj w/ values
- * from the already allocated syncobj (if there is one)
- */
-int omap_gem_set_sync_object(struct drm_gem_object *obj, void *syncobj)
-{
-	struct omap_gem_object *omap_obj = to_omap_bo(obj);
-	int ret = 0;
-
-	spin_lock(&sync_lock);
-
-	if ((omap_obj->flags & OMAP_BO_EXT_SYNC) && !syncobj) {
-		/* clearing a previously set syncobj */
-		syncobj = kmemdup(omap_obj->sync, sizeof(*omap_obj->sync),
-				  GFP_ATOMIC);
-		if (!syncobj) {
-			ret = -ENOMEM;
-			goto unlock;
-		}
-		omap_obj->flags &= ~OMAP_BO_EXT_SYNC;
-		omap_obj->sync = syncobj;
-	} else if (syncobj && !(omap_obj->flags & OMAP_BO_EXT_SYNC)) {
-		/* replacing an existing syncobj */
-		if (omap_obj->sync) {
-			memcpy(syncobj, omap_obj->sync, sizeof(*omap_obj->sync));
-			kfree(omap_obj->sync);
-		}
-		omap_obj->flags |= OMAP_BO_EXT_SYNC;
-		omap_obj->sync = syncobj;
-	}
-
-unlock:
-	spin_unlock(&sync_lock);
-	return ret;
-}
-
 /* -----------------------------------------------------------------------------
  * Constructor & Destructor
  */
@@ -1360,28 +1307,23 @@ void omap_gem_free_object(struct drm_gem_object *obj)
 	 */
 	WARN_ON(omap_obj->paddr_cnt > 0);
 
-	/* don't free externally allocated backing memory */
-	if (!(omap_obj->flags & OMAP_BO_MEM_EXT)) {
-		if (omap_obj->pages) {
-			if (omap_obj->flags & OMAP_BO_MEM_DMABUF)
-				kfree(omap_obj->pages);
-			else
-				omap_gem_detach_pages(obj);
-		}
+	if (omap_obj->pages) {
+		if (omap_obj->flags & OMAP_BO_MEM_DMABUF)
+			kfree(omap_obj->pages);
+		else
+			omap_gem_detach_pages(obj);
+	}
 
-		if (omap_obj->flags & OMAP_BO_MEM_DMA_API) {
-			dma_free_writecombine(dev->dev, obj->size,
-					omap_obj->vaddr, omap_obj->paddr);
-		} else if (omap_obj->vaddr) {
-			vunmap(omap_obj->vaddr);
-		} else if (obj->import_attach) {
-			drm_prime_gem_destroy(obj, omap_obj->sgt);
-		}
+	if (omap_obj->flags & OMAP_BO_MEM_DMA_API) {
+		dma_free_writecombine(dev->dev, obj->size,
+				omap_obj->vaddr, omap_obj->paddr);
+	} else if (omap_obj->vaddr) {
+		vunmap(omap_obj->vaddr);
+	} else if (obj->import_attach) {
+		drm_prime_gem_destroy(obj, omap_obj->sgt);
 	}
 
-	/* don't free externally allocated syncobj */
-	if (!(omap_obj->flags & OMAP_BO_EXT_SYNC))
-		kfree(omap_obj->sync);
+	kfree(omap_obj->sync);
 
 	drm_gem_object_release(obj);
 
@@ -1426,10 +1368,9 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
 		 * use contiguous memory only if no TILER is available.
 		 */
 		flags |= OMAP_BO_MEM_DMA_API;
-	} else if (!(flags & (OMAP_BO_MEM_EXT | OMAP_BO_MEM_DMABUF))) {
+	} else if (!(flags & OMAP_BO_MEM_DMABUF)) {
 		/*
-		 * All other buffers not backed by external memory or dma_buf
-		 * are shmem-backed.
+		 * All other buffers not backed by dma_buf are shmem-backed.
 		 */
 		flags |= OMAP_BO_MEM_SHMEM;
 	}
-- 
2.5.0

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

[PATCH 18/33] drm/omap: increase vblank wait timeout

[Tomi Valkeinen]

omap_crtc_wait_pending() waits until the config changes have been taken
into use, usually at next vblank. The wait-timeout used is 50ms, which
usually is enough, but in some rare cases not.

As time wait-timeout is just a safety measure for cases where something
is broken, we can just as well increase the timeout considerably.

This patch makes the timeout 250ms.

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 f5b19d18fa8b..86b77c4f299a 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -82,7 +82,7 @@ int omap_crtc_wait_pending(struct drm_crtc *crtc)
 
 	return wait_event_timeout(omap_crtc->pending_wait,
 				  !omap_crtc->pending,
-				  msecs_to_jiffies(50));
+				  msecs_to_jiffies(250));
 }
 
 /* -----------------------------------------------------------------------------
-- 
2.5.0

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

[PATCH 19/33] drm/omap: DISPC: support double-pixel mode

[Tomi Valkeinen]

We need double-pixel mode (pixel repetition) for interlace modes. This
patch adds the necessary support to DISPC to output double-pixel mode.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c | 8 ++++++++
 include/video/omapdss.h             | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 6b50476ec669..1e7f26985bda 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -104,6 +104,8 @@ struct dispc_features {
 	bool supports_sync_align:1;
 
 	bool has_writeback:1;
+
+	bool supports_double_pixel:1;
 };
 
 #define DISPC_MAX_NR_FIFOS 5
@@ -3287,6 +3289,10 @@ void dispc_mgr_set_timings(enum omap_channel channel,
 	} else {
 		if (t.interlace)
 			t.y_res /= 2;
+
+		if (dispc.feat->supports_double_pixel)
+			REG_FLD_MOD(DISPC_CONTROL, t.double_pixel ? 1 : 0,
+				19, 17);
 	}
 
 	dispc_mgr_set_size(channel, t.x_res, t.y_res);
@@ -3951,6 +3957,7 @@ static const struct dispc_features omap44xx_dispc_feats = {
 	.set_max_preload	=	true,
 	.supports_sync_align	=	true,
 	.has_writeback		=	true,
+	.supports_double_pixel	=	true,
 };
 
 static const struct dispc_features omap54xx_dispc_feats = {
@@ -3974,6 +3981,7 @@ static const struct dispc_features omap54xx_dispc_feats = {
 	.set_max_preload	=	true,
 	.supports_sync_align	=	true,
 	.has_writeback		=	true,
+	.supports_double_pixel	=	true,
 };
 
 static int dispc_init_features(struct platform_device *pdev)
diff --git a/include/video/omapdss.h b/include/video/omapdss.h
index 86f28a92281a..6c1a3e1b4d55 100644
--- a/include/video/omapdss.h
+++ b/include/video/omapdss.h
@@ -367,6 +367,8 @@ struct omap_video_timings {
 	enum omap_dss_signal_level de_level;
 	/* Pixel clock edges to drive HSYNC and VSYNC signals */
 	enum omap_dss_signal_edge sync_pclk_edge;
+
+	bool double_pixel;
 };
 
 /* Hardcoded timings for tv modes. Venc only uses these to
-- 
2.5.0

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

[PATCH 20/33] drm/omap: support double-pixel

[Tomi Valkeinen]

We need double-pixel mode (pixel repetition) for interlace modes. This
patch adds the necessary support to omapdrm to output double-pixel mode.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c
index 83f2a9177c14..ce2d67b6a8c7 100644
--- a/drivers/gpu/drm/omapdrm/omap_connector.c
+++ b/drivers/gpu/drm/omapdrm/omap_connector.c
@@ -63,6 +63,9 @@ void copy_timings_omap_to_drm(struct drm_display_mode *mode,
 	if (timings->interlace)
 		mode->flags |= DRM_MODE_FLAG_INTERLACE;
 
+	if (timings->double_pixel)
+		mode->flags |= DRM_MODE_FLAG_DBLCLK;
+
 	if (timings->hsync_level == OMAPDSS_SIG_ACTIVE_HIGH)
 		mode->flags |= DRM_MODE_FLAG_PHSYNC;
 	else
@@ -90,6 +93,7 @@ void copy_timings_drm_to_omap(struct omap_video_timings *timings,
 	timings->vbp = mode->vtotal - mode->vsync_end;
 
 	timings->interlace = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
+	timings->double_pixel = !!(mode->flags & DRM_MODE_FLAG_DBLCLK);
 
 	if (mode->flags & DRM_MODE_FLAG_PHSYNC)
 		timings->hsync_level = OMAPDSS_SIG_ACTIVE_HIGH;
-- 
2.5.0

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

[PATCH 21/33] drm/omap: HDMI: support double-pixel pixel clock

[Tomi Valkeinen]

We need double-pixel mode (pixel repetition) for interlace modes. This
patch adds the necessary support to HDMI to double the pixel clock when
double-pixel mode is used.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/hdmi4.c | 7 ++++++-
 drivers/gpu/drm/omapdrm/dss/hdmi5.c | 7 ++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
index b09ce9ee82fa..ddd6a331df39 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
@@ -168,6 +168,7 @@ static int hdmi_power_on_full(struct omap_dss_device *dssdev)
 	struct omap_overlay_manager *mgr = hdmi.output.manager;
 	struct hdmi_wp_data *wp = &hdmi.wp;
 	struct dss_pll_clock_info hdmi_cinfo = { 0 };
+	unsigned pc;
 
 	r = hdmi_power_on_core(dssdev);
 	if (r)
@@ -181,7 +182,11 @@ static int hdmi_power_on_full(struct omap_dss_device *dssdev)
 
 	DSSDBG("hdmi_power_on x_res= %d y_res = %d\n", p->x_res, p->y_res);
 
-	hdmi_pll_compute(&hdmi.pll, p->pixelclock, &hdmi_cinfo);
+	pc = p->pixelclock;
+	if (p->double_pixel)
+		pc *= 2;
+
+	hdmi_pll_compute(&hdmi.pll, pc, &hdmi_cinfo);
 
 	r = dss_pll_enable(&hdmi.pll.pll);
 	if (r) {
diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
index 4485a1c37bd8..34174ea85a54 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
@@ -184,6 +184,7 @@ static int hdmi_power_on_full(struct omap_dss_device *dssdev)
 	struct omap_video_timings *p;
 	struct omap_overlay_manager *mgr = hdmi.output.manager;
 	struct dss_pll_clock_info hdmi_cinfo = { 0 };
+	unsigned pc;
 
 	r = hdmi_power_on_core(dssdev);
 	if (r)
@@ -193,7 +194,11 @@ static int hdmi_power_on_full(struct omap_dss_device *dssdev)
 
 	DSSDBG("hdmi_power_on x_res= %d y_res = %d\n", p->x_res, p->y_res);
 
-	hdmi_pll_compute(&hdmi.pll, p->pixelclock, &hdmi_cinfo);
+	pc = p->pixelclock;
+	if (p->double_pixel)
+		pc *= 2;
+
+	hdmi_pll_compute(&hdmi.pll, pc, &hdmi_cinfo);
 
 	/* disable and clear irqs */
 	hdmi_wp_clear_irqenable(&hdmi.wp, 0xffffffff);
-- 
2.5.0

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

[PATCH 22/33] drm/omap: HDMI: Fix HSW value

[Tomi Valkeinen]

On OMAP4 and OMAP5 ES1.0 the HDMI_WP_VIDEO_TIMING_H:HSW field is
set directly to the HSW value. On later SoCs the field needs to be
programmed with the value of HSW-1.

Currently the driver always programs the field with the HSW value. Most
videomodes seem to work fine with that, but at least low resolution
interlaced modes don't work at all.

This patch fixes the HSW for OMAP5 ES2.0+ SoCs.

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

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi_wp.c b/drivers/gpu/drm/omapdrm/dss/hdmi_wp.c
index 7c544bc56fb5..48ffb39663c8 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi_wp.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi_wp.c
@@ -165,12 +165,24 @@ void hdmi_wp_video_config_timing(struct hdmi_wp_data *wp,
 {
 	u32 timing_h = 0;
 	u32 timing_v = 0;
+	bool hsw_minus_one = true;
 
 	DSSDBG("Enter hdmi_wp_video_config_timing\n");
 
+	/*
+	 * On OMAP4 and OMAP5 ES1 the HSW field is programmed as is. On OMAP5
+	 * ES2+ (including DRA7/AM5 SoCs) HSW field is programmed to hsw-1.
+	 * However, we don't support OMAP5 ES1 at all, so we can just check for
+	 * OMAP4 here.
+	 */
+	if (omapdss_get_version() == OMAPDSS_VER_OMAP4430_ES1 ||
+	    omapdss_get_version() == OMAPDSS_VER_OMAP4430_ES2 ||
+	    omapdss_get_version() == OMAPDSS_VER_OMAP4)
+		hsw_minus_one = false;
+
 	timing_h |= FLD_VAL(timings->hbp, 31, 20);
 	timing_h |= FLD_VAL(timings->hfp, 19, 8);
-	timing_h |= FLD_VAL(timings->hsw, 7, 0);
+	timing_h |= FLD_VAL(timings->hsw - (hsw_minus_one ? 1 : 0), 7, 0);
 	hdmi_write_reg(wp->base, HDMI_WP_VIDEO_TIMING_H, timing_h);
 
 	timing_v |= FLD_VAL(timings->vbp, 31, 20);
-- 
2.5.0

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

[PATCH 23/33] drm/omap: HDMI: fix WP timings for ilace

[Tomi Valkeinen]

The HDMI WP timings are not programmed correctly for interlace.

We need to halve the vertical timings when interlace is used, and double
the horizontal timings when pixel doubling is used.

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

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi_wp.c b/drivers/gpu/drm/omapdrm/dss/hdmi_wp.c
index 48ffb39663c8..5f4af41830b1 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi_wp.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi_wp.c
@@ -199,8 +199,6 @@ void hdmi_wp_init_vid_fmt_timings(struct hdmi_video_format *video_fmt,
 	video_fmt->packing_mode = HDMI_PACK_10b_RGB_YUV444;
 	video_fmt->y_res = param->timings.y_res;
 	video_fmt->x_res = param->timings.x_res;
-	if (param->timings.interlace)
-		video_fmt->y_res /= 2;
 
 	timings->hbp = param->timings.hbp;
 	timings->hfp = param->timings.hfp;
@@ -208,9 +206,25 @@ void hdmi_wp_init_vid_fmt_timings(struct hdmi_video_format *video_fmt,
 	timings->vbp = param->timings.vbp;
 	timings->vfp = param->timings.vfp;
 	timings->vsw = param->timings.vsw;
+
 	timings->vsync_level = param->timings.vsync_level;
 	timings->hsync_level = param->timings.hsync_level;
 	timings->interlace = param->timings.interlace;
+	timings->double_pixel = param->timings.double_pixel;
+
+	if (param->timings.interlace) {
+		video_fmt->y_res /= 2;
+		timings->vbp /= 2;
+		timings->vfp /= 2;
+		timings->vsw /= 2;
+	}
+
+	if (param->timings.double_pixel) {
+		video_fmt->x_res *= 2;
+		timings->hfp *= 2;
+		timings->hsw *= 2;
+		timings->hbp *= 2;
+	}
 }
 
 void hdmi_wp_audio_config_format(struct hdmi_wp_data *wp,
-- 
2.5.0

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

[PATCH 24/33] drm/omap: DISPC: Fix field order for HDMI

[Tomi Valkeinen]

Interlace field order is different between VENC and HDMI. The driver
currently sets the field order for VENC.

This patch adds the code to set the field order for HDMI.

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

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 1e7f26985bda..a4274dca384a 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -106,6 +106,13 @@ struct dispc_features {
 	bool has_writeback:1;
 
 	bool supports_double_pixel:1;
+
+	/*
+	 * Field order for VENC is different than HDMI. We should handle this in
+	 * some intelligent manner, but as the SoCs have either HDMI or VENC,
+	 * never both, we can just use this flag for now.
+	 */
+	bool reverse_ilace_field_order:1;
 };
 
 #define DISPC_MAX_NR_FIFOS 5
@@ -2749,6 +2756,9 @@ static int dispc_ovl_setup_common(enum omap_plane plane,
 
 	dispc_ovl_configure_burst_type(plane, rotation_type);
 
+	if (dispc.feat->reverse_ilace_field_order)
+		swap(offset0, offset1);
+
 	dispc_ovl_set_ba0(plane, paddr + offset0);
 	dispc_ovl_set_ba1(plane, paddr + offset1);
 
@@ -3958,6 +3968,7 @@ static const struct dispc_features omap44xx_dispc_feats = {
 	.supports_sync_align	=	true,
 	.has_writeback		=	true,
 	.supports_double_pixel	=	true,
+	.reverse_ilace_field_order =	true,
 };
 
 static const struct dispc_features omap54xx_dispc_feats = {
@@ -3982,6 +3993,7 @@ static const struct dispc_features omap54xx_dispc_feats = {
 	.supports_sync_align	=	true,
 	.has_writeback		=	true,
 	.supports_double_pixel	=	true,
+	.reverse_ilace_field_order =	true,
 };
 
 static int dispc_init_features(struct platform_device *pdev)
-- 
2.5.0

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

[PATCH 25/33] drm/omap: HDMI5: Fix FC HSW value

[Tomi Valkeinen]

For some reason the HDMI FC's HSW value is programmed to hsw-1. There's
no indication in the documentation that this would be correct, and no
other blanking value needs -1 either.

So remove the -1.

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

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c b/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
index 8ea531d2652c..ec9223e514fa 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
@@ -296,11 +296,11 @@ static void hdmi_core_init(struct hdmi_core_vid_config *video_cfg,
 	video_cfg->data_enable_pol = 1; /* It is always 1*/
 	video_cfg->v_fc_config.timings.hsync_level = cfg->timings.hsync_level;
 	video_cfg->v_fc_config.timings.x_res = cfg->timings.x_res;
-	video_cfg->v_fc_config.timings.hsw = cfg->timings.hsw - 1;
+	video_cfg->v_fc_config.timings.hsw = cfg->timings.hsw;
 	video_cfg->v_fc_config.timings.hbp = cfg->timings.hbp;
 	video_cfg->v_fc_config.timings.hfp = cfg->timings.hfp;
 	video_cfg->hblank = cfg->timings.hfp +
-				cfg->timings.hbp + cfg->timings.hsw - 1;
+				cfg->timings.hbp + cfg->timings.hsw;
 	video_cfg->v_fc_config.timings.vsync_level = cfg->timings.vsync_level;
 	video_cfg->v_fc_config.timings.y_res = cfg->timings.y_res;
 	video_cfg->v_fc_config.timings.vsw = cfg->timings.vsw;
-- 
2.5.0

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

[PATCH 26/33] drm/omap: HDMI5: clean up timings copy

[Tomi Valkeinen]

The HDMI driver copies the timing values one by one. Instead we can just
copy the whole struct.

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

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c b/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
index ec9223e514fa..947edb9d4275 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
@@ -292,25 +292,16 @@ static void hdmi_core_init(struct hdmi_core_vid_config *video_cfg,
 {
 	DSSDBG("hdmi_core_init\n");
 
+	video_cfg->v_fc_config.timings = cfg->timings;
+
 	/* video core */
 	video_cfg->data_enable_pol = 1; /* It is always 1*/
-	video_cfg->v_fc_config.timings.hsync_level = cfg->timings.hsync_level;
-	video_cfg->v_fc_config.timings.x_res = cfg->timings.x_res;
-	video_cfg->v_fc_config.timings.hsw = cfg->timings.hsw;
-	video_cfg->v_fc_config.timings.hbp = cfg->timings.hbp;
-	video_cfg->v_fc_config.timings.hfp = cfg->timings.hfp;
 	video_cfg->hblank = cfg->timings.hfp +
 				cfg->timings.hbp + cfg->timings.hsw;
-	video_cfg->v_fc_config.timings.vsync_level = cfg->timings.vsync_level;
-	video_cfg->v_fc_config.timings.y_res = cfg->timings.y_res;
-	video_cfg->v_fc_config.timings.vsw = cfg->timings.vsw;
-	video_cfg->v_fc_config.timings.vfp = cfg->timings.vfp;
-	video_cfg->v_fc_config.timings.vbp = cfg->timings.vbp;
 	video_cfg->vblank_osc = 0; /* Always 0 - need to confirm */
 	video_cfg->vblank = cfg->timings.vsw +
 				cfg->timings.vfp + cfg->timings.vbp;
 	video_cfg->v_fc_config.hdmi_dvi_mode = cfg->hdmi_dvi_mode;
-	video_cfg->v_fc_config.timings.interlace = cfg->timings.interlace;
 }
 
 /* DSS_HDMI_CORE_VIDEO_CONFIG */
-- 
2.5.0

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

[PATCH 27/33] drm/omap: HDMI5: Add interlace support

[Tomi Valkeinen]

Add the missing bits for interlace:

* Set VBLANK_OSC if the videomode's vblank is fractional
* Halve the vertical timings for interlace
* Double the horizontal timings for double-pixel mode
* Set FC_PRCONF properly for double-pixel mode

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

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c b/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
index 947edb9d4275..6a397520cae5 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi5_core.c
@@ -298,10 +298,30 @@ static void hdmi_core_init(struct hdmi_core_vid_config *video_cfg,
 	video_cfg->data_enable_pol = 1; /* It is always 1*/
 	video_cfg->hblank = cfg->timings.hfp +
 				cfg->timings.hbp + cfg->timings.hsw;
-	video_cfg->vblank_osc = 0; /* Always 0 - need to confirm */
+	video_cfg->vblank_osc = 0;
 	video_cfg->vblank = cfg->timings.vsw +
 				cfg->timings.vfp + cfg->timings.vbp;
 	video_cfg->v_fc_config.hdmi_dvi_mode = cfg->hdmi_dvi_mode;
+
+	if (cfg->timings.interlace) {
+		/* set vblank_osc if vblank is fractional */
+		if (video_cfg->vblank % 2 != 0)
+			video_cfg->vblank_osc = 1;
+
+		video_cfg->v_fc_config.timings.y_res /= 2;
+		video_cfg->vblank /= 2;
+		video_cfg->v_fc_config.timings.vfp /= 2;
+		video_cfg->v_fc_config.timings.vsw /= 2;
+		video_cfg->v_fc_config.timings.vbp /= 2;
+	}
+
+	if (cfg->timings.double_pixel) {
+		video_cfg->v_fc_config.timings.x_res *= 2;
+		video_cfg->hblank *= 2;
+		video_cfg->v_fc_config.timings.hfp *= 2;
+		video_cfg->v_fc_config.timings.hsw *= 2;
+		video_cfg->v_fc_config.timings.hbp *= 2;
+	}
 }
 
 /* DSS_HDMI_CORE_VIDEO_CONFIG */
@@ -368,6 +388,11 @@ static void hdmi_core_video_config(struct hdmi_core_data *core,
 	/* select DVI mode */
 	REG_FLD_MOD(base, HDMI_CORE_FC_INVIDCONF,
 			cfg->v_fc_config.hdmi_dvi_mode, 3, 3);
+
+	if (cfg->v_fc_config.timings.double_pixel)
+		REG_FLD_MOD(base, HDMI_CORE_FC_PRCONF, 2, 7, 4);
+	else
+		REG_FLD_MOD(base, HDMI_CORE_FC_PRCONF, 1, 7, 4);
 }
 
 static void hdmi_core_config_video_packetizer(struct hdmi_core_data *core)
-- 
2.5.0

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

[PATCH 28/33] drm/omap: HDMI5: allow interlace

[Tomi Valkeinen]

Now that interlace support has been added, we can remove the check that
prevents interlace.

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

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
index 34174ea85a54..b2dd4c9f20d5 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
@@ -284,10 +284,6 @@ static int hdmi_display_check_timing(struct omap_dss_device *dssdev,
 {
 	struct omap_dss_device *out = &hdmi.output;
 
-	/* TODO: proper interlace support */
-	if (timings->interlace)
-		return -EINVAL;
-
 	if (!dispc_mgr_timings_ok(out->dispc_channel, timings))
 		return -EINVAL;
 
-- 
2.5.0

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

[PATCH 29/33] drm/omap: verify that display x-res is divisible by 8

[Tomi Valkeinen]

DISPC requires the x resolution to be divisible by 8 when stall mode is
not used.

Add a check to the DPI driver to verify this.

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

diff --git a/drivers/gpu/drm/omapdrm/dss/dpi.c b/drivers/gpu/drm/omapdrm/dss/dpi.c
index 7953e6a52346..557cf3bdcc4e 100644
--- a/drivers/gpu/drm/omapdrm/dss/dpi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dpi.c
@@ -513,6 +513,9 @@ static int dpi_check_timings(struct omap_dss_device *dssdev,
 	struct dpi_clk_calc_ctx ctx;
 	bool ok;
 
+	if (timings->x_res % 8 != 0)
+		return -EINVAL;
+
 	if (mgr && !dispc_mgr_timings_ok(mgr->id, timings))
 		return -EINVAL;
 
-- 
2.5.0

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

[PATCH 30/33] drm/omap: verify that fb plane pitches are the same

[Tomi Valkeinen]

The DSS hardware uses the same ROW_INC value for both Y and UV planes
for NV12 format. This means that the pitches of the Y and UV planes have
to match. omapdrm doesn't check this at the moment, and this can lead
into a broken NV12 fb on the screen.

This patch adds the check.

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 ad202dfc1a49..481512db2656 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -449,6 +449,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 			goto fail;
 		}
 
+		if (i > 0 && pitch != mode_cmd->pitches[i - 1]) {
+			dev_err(dev->dev,
+				"pitches are not the same between framebuffer planes %d != %d\n",
+				pitch, mode_cmd->pitches[i - 1]);
+			ret = -EINVAL;
+			goto fail;
+		}
+
 		plane->bo     = bos[i];
 		plane->offset = mode_cmd->offsets[i];
 		plane->pitch  = pitch;
-- 
2.5.0

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

[PATCH 31/33] drm/omap: EBUSY status handling in omap_gem_fault()

[Tomi Valkeinen]

From: Rob Clark <robdclark@gmail.com>

Subsequent threads returning EBUSY from vm_insert_pfn() was not
handled correctly. As a result concurrent access from new threads
to mmapped data caused SIGBUS.

See e79e0fe380847493266fba557217e2773c61bd1b ("drm/i915: EBUSY status
handling added to i915_gem_fault()").

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

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 52e4a6c95058..7b1b3d8ded58 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -587,6 +587,11 @@ fail:
 	case 0:
 	case -ERESTARTSYS:
 	case -EINTR:
+	case -EBUSY:
+		/*
+		 * EBUSY is ok: this just means that another thread
+		 * already did the job.
+		 */
 		return VM_FAULT_NOPAGE;
 	case -ENOMEM:
 		return VM_FAULT_OOM;
-- 
2.5.0

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

[PATCH 32/33] drm/omap: fix crtc->plane property delegation

[Tomi Valkeinen]

Before universal planes we had to have plane specific properties for the
crtc too, as on the hardware level a crtc uses a plane. In other words,
e.g. 'zorder' property was added to both planes and crtcs, and
omap_crtc.c would delegate the property set/get to the primary plane.

However, the delegation was a bit too generic, delegating all property
set/get calls to planes. Thus it's possible to set, say, FB_ID, on a
crtc, which gets redirected to  the primary plane.

This is not standard, and shouldn't be allowed. To keep backward
compatibility, we still need to redirect the properties we supported
earlier for crtcs, namely 'zorder' and 'rotation'.

This patch redirects only the allowed properties from crtcs to planes.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 86b77c4f299a..280d80781635 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -419,24 +419,40 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
 	}
 }
 
+static bool omap_crtc_is_plane_prop(struct drm_device *dev,
+	struct drm_property *property)
+{
+	struct omap_drm_private *priv = dev->dev_private;
+
+	return	property == priv->zorder_prop ||
+		property == dev->mode_config.rotation_property;
+}
+
 static int omap_crtc_atomic_set_property(struct drm_crtc *crtc,
 					 struct drm_crtc_state *state,
 					 struct drm_property *property,
 					 uint64_t val)
 {
-	struct drm_plane_state *plane_state;
-	struct drm_plane *plane = crtc->primary;
+	struct drm_device *dev = crtc->dev;
 
-	/*
-	 * Delegate property set to the primary plane. Get the plane state and
-	 * set the property directly.
-	 */
+	if (omap_crtc_is_plane_prop(dev, property)) {
+		struct drm_plane_state *plane_state;
+		struct drm_plane *plane = crtc->primary;
+
+		/*
+		 * Delegate property set to the primary plane. Get the plane
+		 * state and set the property directly.
+		 */
 
-	plane_state = drm_atomic_get_plane_state(state->state, plane);
-	if (IS_ERR(plane_state))
-		return PTR_ERR(plane_state);
+		plane_state = drm_atomic_get_plane_state(state->state, plane);
+		if (IS_ERR(plane_state))
+			return PTR_ERR(plane_state);
 
-	return drm_atomic_plane_set_property(plane, plane_state, property, val);
+		return drm_atomic_plane_set_property(plane, plane_state,
+				property, val);
+	}
+
+	return -EINVAL;
 }
 
 static int omap_crtc_atomic_get_property(struct drm_crtc *crtc,
@@ -444,14 +460,20 @@ static int omap_crtc_atomic_get_property(struct drm_crtc *crtc,
 					 struct drm_property *property,
 					 uint64_t *val)
 {
-	/*
-	 * Delegate property get to the primary plane. The
-	 * drm_atomic_plane_get_property() function isn't exported, but can be
-	 * called through drm_object_property_get_value() as that will call
-	 * drm_atomic_get_property() for atomic drivers.
-	 */
-	return drm_object_property_get_value(&crtc->primary->base, property,
-					     val);
+	struct drm_device *dev = crtc->dev;
+
+	if (omap_crtc_is_plane_prop(dev, property)) {
+		/*
+		 * Delegate property get to the primary plane. The
+		 * drm_atomic_plane_get_property() function isn't exported, but
+		 * can be called through drm_object_property_get_value() as that
+		 * will call drm_atomic_get_property() for atomic drivers.
+		 */
+		return drm_object_property_get_value(&crtc->primary->base,
+				property, val);
+	}
+
+	return -EINVAL;
 }
 
 static const struct drm_crtc_funcs omap_crtc_funcs = {
-- 
2.5.0

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

[PATCH 33/33] drm/omap: check if rotation is supported before commit

[Tomi Valkeinen]

omapdrm is missing a check on the validity of the rotation property.
This leads to omapdrm possibly trying to use rotation on non-rotateable
framebuffer, which causes the overlay setup to fail.

This patch adds the necessary check to omap_plane_atomic_check().

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

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index c077367dcb1a..16c3eeeae668 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -189,6 +189,7 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 		struct omap_drm_window *win, struct omap_overlay_info *info);
 struct drm_connector *omap_framebuffer_get_next_connector(
 		struct drm_framebuffer *fb, struct drm_connector *from);
+bool omap_framebuffer_supports_rotation(struct drm_framebuffer *fb);
 
 void omap_gem_init(struct drm_device *dev);
 void omap_gem_deinit(struct drm_device *dev);
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 481512db2656..610962396eb0 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -145,6 +145,14 @@ static uint32_t get_linear_addr(struct plane *plane,
 	return plane->paddr + offset;
 }
 
+bool omap_framebuffer_supports_rotation(struct drm_framebuffer *fb)
+{
+	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
+	struct plane *plane = &omap_fb->planes[0];
+
+	return omap_gem_flags(plane->bo) & OMAP_BO_TILED;
+}
+
 /* update ovl info for scanout, handles cases of multi-planar fb's, etc.
  */
 void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index d75b197eff46..93ee538a99f5 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -177,6 +177,12 @@ static int omap_plane_atomic_check(struct drm_plane *plane,
 	if (state->crtc_y + state->crtc_h > crtc_state->adjusted_mode.vdisplay)
 		return -EINVAL;
 
+	if (state->fb) {
+		if (state->rotation != BIT(DRM_ROTATE_0) &&
+		    !omap_framebuffer_supports_rotation(state->fb))
+			return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
2.5.0

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

Re: [PATCH 01/33] drm/omap: HDMI: change enable/disable to avoid sync-losts

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:47:36 Tomi Valkeinen wrote:
> We occasionally see DISPC sync-lost errors when enabling and disabling
> HDMI. Sometimes we get only a few, which get handled (ignored) by the
> driver, but sometimes there's a flood of the errors which doesn't seem
> to stop.
> 
> The HW team has root caused this to the order in which HDMI and DISPC
> are enabled/disabled. Currently we enable HDMI first, and then DISPC,
> and vice versa when disabling. HW team's suggestion is to do it the
> other way around.
> 
> This patch changes the order, but this has two side effects as the pixel
> clock is produced by HDMI, and the clock is not running when we
> enable/disable DISPC:
> 
> * When enabling DISPC first, we don't get vertical sync events
> * When disabling DISPC last, we don't get FRAMEDONE event
> 
> At the moment we use both of those to verify that DISPC has been
> enabled/disabled properly. Thus this patch also needs to change the
> omapdrm and omapdss which handle the DISPC side.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/hdmi4.c | 16 ++++++++--------
>  drivers/gpu/drm/omapdrm/dss/hdmi5.c | 16 ++++++++--------
>  drivers/gpu/drm/omapdrm/omap_crtc.c |  5 +++++
>  3 files changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> b/drivers/gpu/drm/omapdrm/dss/hdmi4.c index 7103c659a534..b09ce9ee82fa
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> @@ -214,22 +214,22 @@ static int hdmi_power_on_full(struct omap_dss_device
> *dssdev) /* tv size */
>  	dss_mgr_set_timings(mgr, p);
> 
> -	r = hdmi_wp_video_start(&hdmi.wp);
> -	if (r)
> -		goto err_vid_enable;
> -
>  	r = dss_mgr_enable(mgr);
>  	if (r)
>  		goto err_mgr_enable;
> 
> +	r = hdmi_wp_video_start(&hdmi.wp);
> +	if (r)
> +		goto err_vid_enable;
> +
>  	hdmi_wp_set_irqenable(wp,
>  		HDMI_IRQ_LINK_CONNECT | HDMI_IRQ_LINK_DISCONNECT);
> 
>  	return 0;
> 
> -err_mgr_enable:
> -	hdmi_wp_video_stop(&hdmi.wp);
>  err_vid_enable:
> +	dss_mgr_disable(mgr);
> +err_mgr_enable:
>  	hdmi_wp_set_phy_pwr(&hdmi.wp, HDMI_PHYPWRCMD_OFF);
>  err_phy_pwr:
>  err_phy_cfg:
> @@ -246,10 +246,10 @@ static void hdmi_power_off_full(struct omap_dss_device
> *dssdev)
> 
>  	hdmi_wp_clear_irqenable(&hdmi.wp, 0xffffffff);
> 
> -	dss_mgr_disable(mgr);
> -
>  	hdmi_wp_video_stop(&hdmi.wp);
> 
> +	dss_mgr_disable(mgr);
> +
>  	hdmi_wp_set_phy_pwr(&hdmi.wp, HDMI_PHYPWRCMD_OFF);
> 
>  	dss_pll_disable(&hdmi.pll.pll);
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c
> b/drivers/gpu/drm/omapdrm/dss/hdmi5.c index a955a2c4c061..4485a1c37bd8
> 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
> @@ -231,22 +231,22 @@ static int hdmi_power_on_full(struct omap_dss_device
> *dssdev) /* tv size */
>  	dss_mgr_set_timings(mgr, p);
> 
> -	r = hdmi_wp_video_start(&hdmi.wp);
> -	if (r)
> -		goto err_vid_enable;
> -
>  	r = dss_mgr_enable(mgr);
>  	if (r)
>  		goto err_mgr_enable;
> 
> +	r = hdmi_wp_video_start(&hdmi.wp);
> +	if (r)
> +		goto err_vid_enable;
> +
>  	hdmi_wp_set_irqenable(&hdmi.wp,
>  			HDMI_IRQ_LINK_CONNECT | HDMI_IRQ_LINK_DISCONNECT);
> 
>  	return 0;
> 
> -err_mgr_enable:
> -	hdmi_wp_video_stop(&hdmi.wp);
>  err_vid_enable:
> +	dss_mgr_disable(mgr);
> +err_mgr_enable:
>  	hdmi_wp_set_phy_pwr(&hdmi.wp, HDMI_PHYPWRCMD_OFF);
>  err_phy_pwr:
>  err_phy_cfg:
> @@ -263,10 +263,10 @@ static void hdmi_power_off_full(struct omap_dss_device
> *dssdev)
> 
>  	hdmi_wp_clear_irqenable(&hdmi.wp, 0xffffffff);
> 
> -	dss_mgr_disable(mgr);
> -
>  	hdmi_wp_video_stop(&hdmi.wp);
> 
> +	dss_mgr_disable(mgr);
> +
>  	hdmi_wp_set_phy_pwr(&hdmi.wp, HDMI_PHYPWRCMD_OFF);
> 
>  	dss_pll_disable(&hdmi.pll.pll);
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 2ed0754ed19e..7dd3d44a93e5
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -138,6 +138,11 @@ static void omap_crtc_set_enabled(struct drm_crtc
> *crtc, bool enable) u32 framedone_irq, vsync_irq;
>  	int ret;
> 
> +	if (omap_crtc->mgr->output->output_type == OMAP_DISPLAY_TYPE_HDMI) {
> +		dispc_mgr_enable(channel, enable);
> +		return;
> +	}

This effectively bypasses the wait until the DISPC outputs the first vsync 
interrupt below. How does HDMI differ from other outputs in such a way to make 
the wait unneeded ?

>  	if (dispc_mgr_is_enabled(channel) == enable)
>  		return;

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 02/33] HACK: drm/omap: always use blocking DMM fill

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:47:37 Tomi Valkeinen wrote:
> The current driver uses non-blocking DMM fill when releasing memory.
> This gives us a small performance increase as we don't have to wait for
> the fill operation to finish.
> 
> However, the driver does not have any error handling for non-blocking
> fill. In case of an error, the fill operation may silently fail, leading
> to leaking DMM engines, which may eventually lead to deadlock if we run
> out of DMM engines.
> 
> This patch makes the DMM driver always use blocking fills, so that we
> can catch the errors. A more complex option would be to allow
> non-blocking fills, and implement proper error handling, but that is
> left for the future.
> 
> This patch is a HACK, as the proper fix is to either decide to always
> use sync fills and remove all the async related code, or fix the async
> code.

Could you capture this in the comment in the source code below ? I'd also 
replace XXXX with either FIXME or TODO.

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c index dfebdc4aa0f2..80526dec7b2c
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> @@ -309,6 +309,12 @@ static int fill(struct tcm_area *area, struct page
> **pages, struct tcm_area slice, area_s;
>  	struct dmm_txn *txn;
> 
> +	/*
> +	 * XXX async wait doesn't work, as it does not handle timeout errors
> +	 * properly
> +	 */
> +	wait = true;
> +
>  	txn = dmm_txn_init(omap_dmm, area->tcm);
>  	if (IS_ERR_OR_NULL(txn))
>  		return -ENOMEM;

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 01/33] drm/omap: HDMI: change enable/disable to avoid sync-losts

[Tomi Valkeinen]

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



On 23/02/16 12:22, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Friday 19 February 2016 11:47:36 Tomi Valkeinen wrote:
>> We occasionally see DISPC sync-lost errors when enabling and disabling
>> HDMI. Sometimes we get only a few, which get handled (ignored) by the
>> driver, but sometimes there's a flood of the errors which doesn't seem
>> to stop.
>>
>> The HW team has root caused this to the order in which HDMI and DISPC
>> are enabled/disabled. Currently we enable HDMI first, and then DISPC,
>> and vice versa when disabling. HW team's suggestion is to do it the
>> other way around.
>>
>> This patch changes the order, but this has two side effects as the pixel
>> clock is produced by HDMI, and the clock is not running when we
>> enable/disable DISPC:
>>
>> * When enabling DISPC first, we don't get vertical sync events
>> * When disabling DISPC last, we don't get FRAMEDONE event
>>
>> At the moment we use both of those to verify that DISPC has been
>> enabled/disabled properly. Thus this patch also needs to change the
>> omapdrm and omapdss which handle the DISPC side.

<snip>

>> +	if (omap_crtc->mgr->output->output_type == OMAP_DISPLAY_TYPE_HDMI) {
>> +		dispc_mgr_enable(channel, enable);
>> +		return;
>> +	}
> 
> This effectively bypasses the wait until the DISPC outputs the first vsync 
> interrupt below. How does HDMI differ from other outputs in such a way to make 
> the wait unneeded ?

There's to parts here. Enabling the output and disabling the output.

Enable:

We don't strictly need the wait after enable for any output. The output
works after setting the enable bit.

There are two reasons for the waiting:

1) A sanity check that the configuration is ok. If the config is broken
(which shouldn't happen, of course, as the driver should verify the
config), we won't see vsync. At the moment we only print an error in
that case.

2) OMAP_DSS_CHANNEL_DIGIT is a bit problematic. That channel is used for
analog tv-out (VENC) in older DSS versions, and for HDMI for more recent
ones. With VENC we always get a few sync lost errors when enabling the
output, so with the wait we can ignore those errors (this sync-lost
ignoring is only done for OMAP_DSS_CHANNEL_DIGIT).

We have seen similar sync losts with HDMI too, but apparently it is
possible to support HDMI without any sync losts. That's what this patch
is doing.

With this patch we lose both 1) and 2) above, but 1) is not strictly
needed and 2) shouldn't happen for HDMI after this patch.

We could implement 1) in the HDMI driver too, using the HDMI IP's VSYNC
interrupt, but I don't think it's really necessary.

Disable:

When disabling the output, we do want to wait until the DSS has finished
the work at the end of the frame. This is done in the
omap_crtc_set_enabled() function for all outputs, using FRAMEDONE
interrupt when available, or VSYNC if not.

For HDMI we can do it also in the HDMI driver. The HDMI IP has its own
FRAMEDONE interrupt, which we wait for in hdmi_wp_video_stop().

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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

Re: [PATCH 02/33] HACK: drm/omap: always use blocking DMM fill

[Tomi Valkeinen]

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



On 23/02/16 12:27, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Friday 19 February 2016 11:47:37 Tomi Valkeinen wrote:
>> The current driver uses non-blocking DMM fill when releasing memory.
>> This gives us a small performance increase as we don't have to wait for
>> the fill operation to finish.
>>
>> However, the driver does not have any error handling for non-blocking
>> fill. In case of an error, the fill operation may silently fail, leading
>> to leaking DMM engines, which may eventually lead to deadlock if we run
>> out of DMM engines.
>>
>> This patch makes the DMM driver always use blocking fills, so that we
>> can catch the errors. A more complex option would be to allow
>> non-blocking fills, and implement proper error handling, but that is
>> left for the future.
>>
>> This patch is a HACK, as the proper fix is to either decide to always
>> use sync fills and remove all the async related code, or fix the async
>> code.
> 
> Could you capture this in the comment in the source code below ? I'd also 
> replace XXXX with either FIXME or TODO.

Yes, the comment was a bit lacking. Here's an updated comment:

/*
 * FIXME
 *
 * Asynchronous fill does not work reliably, as the driver does not
 * handle errors in the async code paths. The fill operation may
 * silently fail, leading to leaking DMM engines, which may eventually
 * lead to deadlock if we run out of DMM engines.
 *
 * For now, always set 'wait' so that we only use sync fills. Async
 * fills should be fixed, or alternatively we could decide to only
 * support sync fills and so the whole async code path could be removed.
 */

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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

Re: [PATCH 03/33] HACK: drm/omap: fix memory barrier bug in DMM driver

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:47:38 Tomi Valkeinen wrote:
> A DMM timeout "timed out waiting for done" has been observed on DRA7
> devices. The timeout happens rarely, and only when the system is under
> heavy load.
> 
> Debugging showed that the timeout can be made to happen much more
> frequently by optimizing the DMM driver, so that there's almost no code
> between writing the last DMM descriptors to RAM, and writing to DMM
> register which starts the DMM transaction.
> 
> The current theory is that a wmb() does not properly ensure that the
> data written to RAM is observable by all the components in the system.
> 
> This DMM timeout has caused interesting (and rare) bugs as the error
> handling was not functioning properly (the error handling has been fixed
> in previous commits):
> 
>  * If a DMM timeout happened when a GEM buffer was being pinned for
>    display on the screen, a timeout error would be shown, but the driver
>    would continue programming DSS HW with broken buffer, leading to
>    SYNCLOST floods and possible crashes.
> 
>  * If a DMM timeout happened when other user (say, video decoder) was
>    pinning a GEM buffer, a timeout would be shown but if the user
>    handled the error properly, no other issues followed.
> 
>  * If a DMM timeout happened when a GEM buffer was being released, the
>    driver does not even notice the error, leading to crashes or hang
>    later.
> 
> This patch adds wmb() and readl() calls after the last bit is written to
> RAM, which should ensure that the execution proceeds only after the data
> is actually in RAM, and thus observable by DMM.
> 
> This patch is a HACK, as a read-back should not be needed. Further study
> is required to understand if DMM is somehow special case and read-back
> is ok, or if DRA7's memory barriers do not work correctly.

CONFIG_SOC_DRA7XX selects OMAP_INTERCONNECT and OMAP_INTERCONNECT_BARRIER, but 
dra7xx_map_io() doesn't call omap_barriers_init(). Could that be the root 
cause of the issue ? I don't have access to a DRA7xx system, would you be able 
to test that ?

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c index 80526dec7b2c..4e04f9487375
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> @@ -262,6 +262,17 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool
> wait) }
> 
>  	txn->last_pat->next_pa = 0;
> +	/* ensure that the written descriptors are visible to DMM */
> +	wmb();
> +
> +	/*
> +	 * NOTE: the wmb() above should be enough, but there seems to be a bug
> +	 * in OMAP's memory barrier implementation, which in some rare cases may
> +	 * cause the writes not to be observable after wmb().
> +	 */
> +
> +	/* read back to ensure the data is in RAM */
> +	readl(&txn->last_pat->next_pa);
> 
>  	/* write to PAT_DESCR to clear out any pending transaction */
>  	writel(0x0, dmm->base + reg[PAT_DESCR][engine->id]);

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 04/33] drm/omap: add dmm_read() and dmm_write() wrappers

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:47:39 Tomi Valkeinen wrote:
> This patch adds wrapper functions for readl() and writel(), dmm_read()
> and dmm_write(), so that we can implement workaround for errata i878.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 39 ++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c index 4e04f9487375..fe5260477b52
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> @@ -79,6 +79,16 @@ static const uint32_t reg[][4] = {
>  			DMM_PAT_DESCR__2, DMM_PAT_DESCR__3},
>  };
> 
> +static u32 dmm_read(struct dmm *dmm, u32 reg)
> +{
> +	return readl(dmm->base + reg);
> +}
> +
> +static void dmm_write(struct dmm *dmm, u32 val, u32 reg)
> +{
> +	writel(val, dmm->base + reg);
> +}
> +
>  /* simple allocator to grab next 16 byte aligned memory from txn */
>  static void *alloc_dma(struct dmm_txn *txn, size_t sz, dma_addr_t *pa)
>  {
> @@ -108,7 +118,7 @@ static int wait_status(struct refill_engine *engine,
> uint32_t wait_mask)
> 
>  	i = DMM_FIXED_RETRY_COUNT;
>  	while (true) {
> -		r = readl(dmm->base + reg[PAT_STATUS][engine->id]);
> +		r = dmm_read(dmm, reg[PAT_STATUS][engine->id]);
>  		err = r & DMM_PATSTATUS_ERR;
>  		if (err)
>  			return -EFAULT;
> @@ -140,11 +150,11 @@ static void release_engine(struct refill_engine
> *engine) static irqreturn_t omap_dmm_irq_handler(int irq, void *arg)
>  {
>  	struct dmm *dmm = arg;
> -	uint32_t status = readl(dmm->base + DMM_PAT_IRQSTATUS);
> +	uint32_t status = dmm_read(dmm, DMM_PAT_IRQSTATUS);
>  	int i;
> 
>  	/* ack IRQ */
> -	writel(status, dmm->base + DMM_PAT_IRQSTATUS);
> +	dmm_write(dmm, status, DMM_PAT_IRQSTATUS);
> 
>  	for (i = 0; i < dmm->num_engines; i++) {
>  		if (status & DMM_IRQSTAT_LST) {
> @@ -275,7 +285,7 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool
> wait) readl(&txn->last_pat->next_pa);
> 
>  	/* write to PAT_DESCR to clear out any pending transaction */
> -	writel(0x0, dmm->base + reg[PAT_DESCR][engine->id]);
> +	dmm_write(dmm, 0x0, reg[PAT_DESCR][engine->id]);
> 
>  	/* wait for engine ready: */
>  	ret = wait_status(engine, DMM_PATSTATUS_READY);
> @@ -291,8 +301,7 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool
> wait) smp_mb();
> 
>  	/* kick reload */
> -	writel(engine->refill_pa,
> -		dmm->base + reg[PAT_DESCR][engine->id]);
> +	dmm_write(dmm, engine->refill_pa, reg[PAT_DESCR][engine->id]);
> 
>  	if (wait) {
>  		if (!wait_for_completion_timeout(&engine->compl,
> @@ -659,7 +668,7 @@ static int omap_dmm_probe(struct platform_device *dev)
> 
>  	omap_dmm->dev = &dev->dev;
> 
> -	hwinfo = readl(omap_dmm->base + DMM_PAT_HWINFO);
> +	hwinfo = dmm_read(omap_dmm, DMM_PAT_HWINFO);
>  	omap_dmm->num_engines = (hwinfo >> 24) & 0x1F;
>  	omap_dmm->num_lut = (hwinfo >> 16) & 0x1F;
>  	omap_dmm->container_width = 256;
> @@ -668,7 +677,7 @@ static int omap_dmm_probe(struct platform_device *dev)
>  	atomic_set(&omap_dmm->engine_counter, omap_dmm->num_engines);
> 
>  	/* read out actual LUT width and height */
> -	pat_geom = readl(omap_dmm->base + DMM_PAT_GEOMETRY);
> +	pat_geom = dmm_read(omap_dmm, DMM_PAT_GEOMETRY);
>  	omap_dmm->lut_width = ((pat_geom >> 16) & 0xF) << 5;
>  	omap_dmm->lut_height = ((pat_geom >> 24) & 0xF) << 5;
> 
> @@ -678,12 +687,12 @@ static int omap_dmm_probe(struct platform_device *dev)
> omap_dmm->num_lut++;
> 
>  	/* initialize DMM registers */
> -	writel(0x88888888, omap_dmm->base + DMM_PAT_VIEW__0);
> -	writel(0x88888888, omap_dmm->base + DMM_PAT_VIEW__1);
> -	writel(0x80808080, omap_dmm->base + DMM_PAT_VIEW_MAP__0);
> -	writel(0x80000000, omap_dmm->base + DMM_PAT_VIEW_MAP_BASE);
> -	writel(0x88888888, omap_dmm->base + DMM_TILER_OR__0);
> -	writel(0x88888888, omap_dmm->base + DMM_TILER_OR__1);
> +	dmm_write(omap_dmm, 0x88888888, DMM_PAT_VIEW__0);
> +	dmm_write(omap_dmm, 0x88888888, DMM_PAT_VIEW__1);
> +	dmm_write(omap_dmm, 0x80808080, DMM_PAT_VIEW_MAP__0);
> +	dmm_write(omap_dmm, 0x80000000, DMM_PAT_VIEW_MAP_BASE);
> +	dmm_write(omap_dmm, 0x88888888, DMM_TILER_OR__0);
> +	dmm_write(omap_dmm, 0x88888888, DMM_TILER_OR__1);
> 
>  	ret = request_irq(omap_dmm->irq, omap_dmm_irq_handler, IRQF_SHARED,
>  				"omap_dmm_irq_handler", omap_dmm);
> @@ -701,7 +710,7 @@ static int omap_dmm_probe(struct platform_device *dev)
>  	 * buffers for accelerated pan/scroll) and FILL_DSC<n> which
>  	 * we just generally don't care about.
>  	 */
> -	writel(0x7e7e7e7e, omap_dmm->base + DMM_PAT_IRQENABLE_SET);
> +	dmm_write(omap_dmm, 0x7e7e7e7e, DMM_PAT_IRQENABLE_SET);
> 
>  	omap_dmm->dummy_page = alloc_page(GFP_KERNEL | __GFP_DMA32);
>  	if (!omap_dmm->dummy_page) {

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 05/33] drm/omap: partial workaround for DRA7 DMM errata i878

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:47:40 Tomi Valkeinen wrote:
> Errata i878 says that MPU should not be used to access RAM and DMM at
> the same time. As it's not possible to prevent MPU accessing RAM, we
> need to access DMM via a proxy.
> 
> This patch changes DMM driver to access DMM registers via sDMA. Instead
> of doing a normal readl/writel call to read/write a register, we use
> sDMA to copy 4 bytes from/to the DMM registers.
> 
> This patch provides only a partial workaround for i878, as not only DMM
> register reads/writes are affected, but also accesses to the DMM mapped
> buffers (framebuffers, usually).

Will this patch really improve the situation if the DMM mapping is accessed 
anyway ?

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  Documentation/devicetree/bindings/arm/omap/dmm.txt |   3 +-
>  drivers/gpu/drm/omapdrm/omap_dmm_priv.h            |   8 ++
>  drivers/gpu/drm/omapdrm/omap_dmm_tiler.c           | 141 +++++++++++++++++-
>  3 files changed, 149 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/omap/dmm.txt
> b/Documentation/devicetree/bindings/arm/omap/dmm.txt index
> 8bd6d0a238a8..e5fc2eb7f4da 100644
> --- a/Documentation/devicetree/bindings/arm/omap/dmm.txt
> +++ b/Documentation/devicetree/bindings/arm/omap/dmm.txt
> @@ -8,7 +8,8 @@ translation for initiators which need contiguous dma bus
> addresses.
> 
>  Required properties:
>  - compatible:	Should contain "ti,omap4-dmm" for OMAP4 family
> -		Should contain "ti,omap5-dmm" for OMAP5 and DRA7x family
> +		Should contain "ti,omap5-dmm" for OMAP5 family
> +		Should contain "ti,dra7-dmm" for DRA7x family

Isn't it DRA7xx instead of DRA7x ?

>  - reg:		Contains DMM register address range (base address and length)
>  - interrupts:	Should contain an interrupt-specifier for DMM_IRQ.
>  - ti,hwmods:	Name of the hwmod associated to DMM, which is typically 
"dmm"
> diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_priv.h
> b/drivers/gpu/drm/omapdrm/omap_dmm_priv.h index 9f32a83ca507..4d292ba5bcca
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_dmm_priv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_dmm_priv.h
> @@ -155,10 +155,12 @@ struct refill_engine {
> 
>  struct dmm_platform_data {
>  	uint32_t cpu_cache_flags;
> +	bool errata_i878_wa;
>  };
> 
>  struct dmm {
>  	struct device *dev;
> +	u32 phys_base;
>  	void __iomem *base;
>  	int irq;
> 
> @@ -189,6 +191,12 @@ struct dmm {
>  	struct list_head alloc_head;
> 
>  	const struct dmm_platform_data *plat_data;
> +
> +	bool dmm_workaround;
> +	spinlock_t wa_lock;
> +	u32 *wa_dma_data;
> +	dma_addr_t wa_dma_handle;
> +	struct dma_chan *wa_dma_chan;
>  };
> 
>  #endif
> diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c index fe5260477b52..3ec5c6633585
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> @@ -19,6 +19,7 @@
>  #include <linux/completion.h>
>  #include <linux/delay.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
>  #include <linux/errno.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
> @@ -79,14 +80,124 @@ static const uint32_t reg[][4] = {
>  			DMM_PAT_DESCR__2, DMM_PAT_DESCR__3},
>  };
> 
> +static int dmm_dma_copy(struct dmm *dmm, u32 src, u32 dst)

Anything wrong with using dma_addr_t ?

> +{
> +	struct dma_device *dma_dev = dmm->wa_dma_chan->device;
> +	struct dma_async_tx_descriptor *tx = NULL;

No need to initialize tx to NULL.

> +	enum dma_status status;
> +	dma_cookie_t cookie;
> +
> +	tx = dma_dev->device_prep_dma_memcpy(dmm->wa_dma_chan, dst, src, 4, 0);

Given that you're transferring between an I/O mapped register and system 
memory, I believe you should use dmaengine_prep_slave_single() instead, with a 
call to dmaengine_slave_config() to set the I/O mapped register physical 
address. You will also need to request a slave DMA channel instead of a memcpy 
DMA channel.

> +	if (!tx) {
> +		dev_err(dmm->dev, "Failed to prepare DMA memcpy\n");
> +		return -EIO;
> +	}
> +
> +	cookie = tx->tx_submit(tx);
> +	if (dma_submit_error(cookie)) {
> +		dev_err(dmm->dev, "Failed to do DMA tx_submit\n");
> +		return -EIO;
> +	}
> +
> +	dma_async_issue_pending(dmm->wa_dma_chan);
> +	status = dma_sync_wait(dmm->wa_dma_chan, cookie);
> +	if (status != DMA_COMPLETE)
> +		dev_err(dmm->dev, "i878 wa DMA copy failure\n");
> +
> +	dmaengine_terminate_all(dmm->wa_dma_chan);
> +	return 0;
> +}
> +
> +static u32 dmm_read_wa(struct dmm *dmm, u32 reg)
> +{
> +	u32 src, dst;
> +	int r;
> +
> +	src = (u32)(dmm->phys_base + reg);
> +	dst = (u32)dmm->wa_dma_handle;
> +
> +	r = dmm_dma_copy(dmm, src, dst);
> +	if (r) {
> +		dev_err(dmm->dev, "sDMA read transfer timeout\n");
> +		return readl(dmm->base + reg);
> +	}
> +
> +	return readl(dmm->wa_dma_data);

readl() has a memory barrier *after* the read, not before. Similarly writel() 
has its memory barrier *before* the write. I don't think that's what you want, 
shouldn't you use readl_relaxed() and writel_relaxed() with explicit barriers 
?

I'm also unsure whether readl and writel are the right primitives, as they 
target access to I/O memory, not system memory. Can't you use explicit 
barriers and direct access (return *dmm->wa_dma_data) ?

> +}
> +
> +static void dmm_write_wa(struct dmm *dmm, u32 val, u32 reg)
> +{
> +	u32 src, dst;
> +	int r;
> +
> +	writel(val, dmm->wa_dma_data);

Same comment here.

> +	src = (u32)dmm->wa_dma_handle;
> +	dst = (u32)(dmm->phys_base + reg);
> +
> +	r = dmm_dma_copy(dmm, src, dst);
> +	if (r) {
> +		dev_err(dmm->dev, "sDMA write transfer timeout\n");
> +		writel(val, dmm->base + reg);
> +	}
> +}
> +
>  static u32 dmm_read(struct dmm *dmm, u32 reg)
>  {
> -	return readl(dmm->base + reg);
> +	if (dmm->dmm_workaround) {
> +		u32 v;
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&dmm->wa_lock, flags);
> +		v = dmm_read_wa(dmm, reg);

dma_sync_wait() with a spinlock held doesn't seem like the best idea to me.

> +		spin_unlock_irqrestore(&dmm->wa_lock, flags);
> +
> +		return v;
> +	} else {
> +		return readl(dmm->base + reg);
> +	}
>  }
> 
>  static void dmm_write(struct dmm *dmm, u32 val, u32 reg)
>  {
> -	writel(val, dmm->base + reg);
> +	if (dmm->dmm_workaround) {
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&dmm->wa_lock, flags);
> +		dmm_write_wa(dmm, val, reg);

Ditto.

> +		spin_unlock_irqrestore(&dmm->wa_lock, flags);
> +	} else {
> +		writel(val, dmm->base + reg);
> +	}
> +}
> +
> +static int dmm_workaround_init(struct dmm *dmm)
> +{
> +	dma_cap_mask_t mask;
> +
> +	dma_cap_zero(mask);
> +	dma_cap_set(DMA_MEMCPY, mask);

Nitpicking, I'd move those two lines down right before the 
dma_request_channel() call, as they are part of the same logical block of 
code.

> +	spin_lock_init(&dmm->wa_lock);
> +
> +	dmm->wa_dma_data = dma_alloc_coherent(dmm->dev, 4, &dmm->wa_dma_handle,
> GFP_KERNEL);

Maybe a macro instead of 4 ?

> +	if (!dmm->wa_dma_data)
> +		return -ENOMEM;
> +
> +	dmm->wa_dma_chan = dma_request_channel(mask, NULL, NULL);
> +	if (!dmm->wa_dma_chan) {
> +		dma_free_coherent(dmm->dev, 4, dmm->wa_dma_data, dmm->wa_dma_handle);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static void dmm_workaround_uninit(struct dmm *dmm)
> +{
> +	dma_release_channel(dmm->wa_dma_chan);
> +
> +	dma_free_coherent(dmm->dev, 4, dmm->wa_dma_data, dmm->wa_dma_handle);
>  }
> 
>  /* simple allocator to grab next 16 byte aligned memory from txn */
> @@ -606,6 +717,9 @@ static int omap_dmm_remove(struct platform_device *dev)
>  		if (omap_dmm->dummy_page)
>  			__free_page(omap_dmm->dummy_page);
> 
> +		if (omap_dmm->dmm_workaround)
> +			dmm_workaround_uninit(omap_dmm);
> +
>  		if (omap_dmm->irq > 0)
>  			free_irq(omap_dmm->irq, omap_dmm);
> 
> @@ -653,6 +767,7 @@ static int omap_dmm_probe(struct platform_device *dev)
>  		goto fail;
>  	}
> 
> +	omap_dmm->phys_base = mem->start;
>  	omap_dmm->base = ioremap(mem->start, SZ_2K);
> 
>  	if (!omap_dmm->base) {
> @@ -668,6 +783,19 @@ static int omap_dmm_probe(struct platform_device *dev)
> 
>  	omap_dmm->dev = &dev->dev;
> 
> +	omap_dmm->dmm_workaround = omap_dmm->plat_data->errata_i878_wa;
> +
> +	if (omap_dmm->dmm_workaround) {

I'd test omap_dmm->plat_data->errata_i878_wa here, omap_dmm->dmm_workaround is 
initialized to false by kzalloc().

> +		int r;
> +		r = dmm_workaround_init(omap_dmm);

Can't you use the ret variable ? I know it's pre-initialized to -EFAULT, but 
it seems better to me to set it to explicit values in the three goto fail; 
error cases that would be impacted, especially given that -EFAULT is the wrong 
error code to return (and as it would touch that code, the 
platform_get_resource() + ioremap() calls can be optimized in a 
devm_ioremap_resource() in another cleanup patch).

> +		if (r) {
> +			omap_dmm->dmm_workaround = false;
> +			dev_err(&dev->dev, "failed to initialize work-around, WA not 
used\n");

As this is a non-fatal error, maybe dev_warn() ?

> +		} else {
> +			dev_info(&dev->dev, "workaround for errata i878 in use\n");
> +		}
> +	}
> +
>  	hwinfo = dmm_read(omap_dmm, DMM_PAT_HWINFO);
>  	omap_dmm->num_engines = (hwinfo >> 24) & 0x1F;
>  	omap_dmm->num_lut = (hwinfo >> 16) & 0x1F;
> @@ -1031,6 +1159,11 @@ static const struct dmm_platform_data
> dmm_omap5_platform_data = { .cpu_cache_flags = OMAP_BO_UNCACHED,
>  };
> 
> +static const struct dmm_platform_data dmm_dra7_platform_data = {
> +	.cpu_cache_flags = OMAP_BO_UNCACHED,
> +	.errata_i878_wa = true,
> +};
> +
>  static const struct of_device_id dmm_of_match[] = {
>  	{
>  		.compatible = "ti,omap4-dmm",
> @@ -1040,6 +1173,10 @@ static const struct of_device_id dmm_of_match[] = {
>  		.compatible = "ti,omap5-dmm",
>  		.data = &dmm_omap5_platform_data,
>  	},
> +	{
> +		.compatible = "ti,dra7-dmm",
> +		.data = &dmm_dra7_platform_data,
> +	},
>  	{},
>  };
>  #endif

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 06/33] drm/omap: drm_atomic_get_plane_state() may return ERR_PTR

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:47:41 Tomi Valkeinen wrote:
> From: Jyri Sarha <jsarha@ti.com>
> 
> drm_atomic_get_plane_state() may return ERR_PTR. Handle
> drm_atomic_get_plane_state() return values right in
> omap_crtc_atomic_set_property().
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 7dd3d44a93e5..f5b19d18fa8b
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -433,8 +433,8 @@ static int omap_crtc_atomic_set_property(struct drm_crtc
> *crtc, */
> 
>  	plane_state = drm_atomic_get_plane_state(state->state, plane);
> -	if (!plane_state)
> -		return -EINVAL;
> +	if (IS_ERR(plane_state))
> +		return PTR_ERR(plane_state);
> 
>  	return drm_atomic_plane_set_property(plane, plane_state, property, val);
>  }

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 10/33] drm/omap: add define for DISPC_IRQ_WBUNCOMPLETEERROR

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:47:45 Tomi Valkeinen wrote:
> OMAP4+ DSS has WBUNCOMPLETEERROR irq, which was not defined in the irq
> list. Add the define.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  include/video/omapdss.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/video/omapdss.h b/include/video/omapdss.h
> index 295b41e20d8e..86f28a92281a 100644
> --- a/include/video/omapdss.h
> +++ b/include/video/omapdss.h
> @@ -51,6 +51,7 @@
>  #define DISPC_IRQ_FRAMEDONEWB		(1 << 23)
>  #define DISPC_IRQ_FRAMEDONETV		(1 << 24)
>  #define DISPC_IRQ_WBBUFFEROVERFLOW	(1 << 25)
> +#define DISPC_IRQ_WBUNCOMPLETEERROR	(1 << 26)
>  #define DISPC_IRQ_SYNC_LOST3		(1 << 27)
>  #define DISPC_IRQ_VSYNC3		(1 << 28)
>  #define DISPC_IRQ_ACBIAS_COUNT_STAT3	(1 << 29)

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 11/33] drm/omap: use dma_mapping_error in omap_gem_attach_pages

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:47:46 Tomi Valkeinen wrote:
> omap_gem_attach_pages() calls dma_map_page() but does not check the
> possible error with dma_mapping_error(). If DMA-API debugging is
> enabled, the debug layer will give a warning if dma_mapping_error() has
> not been used.
> 
> This patch adds proper error handling to omap_gem_attach_pages().
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_gem.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> b/drivers/gpu/drm/omapdrm/omap_gem.c index 8495a1a4b617..7098190815f1
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -264,6 +264,18 @@ static int omap_gem_attach_pages(struct drm_gem_object
> *obj) for (i = 0; i < npages; i++) {
>  			addrs[i] = dma_map_page(dev->dev, pages[i],
>  					0, PAGE_SIZE, DMA_BIDIRECTIONAL);
> +
> +			if (dma_mapping_error(dev->dev, addrs[i])) {
> +				dev_warn(dev->dev, "omap_gem_attach_pages: dma_map_page 
failed\n");

Using dev_warn(dev->dev, "%s: failed to map page\n", __func__); and proper 
line breaks you'll have no trouble complying with the 80 characters per line 
limit :-)

> +
> +				for (i = i - 1; i >= 0; --i) {

Maybe i-- instead of i = i - 1 ?

> +					dma_unmap_page(dev->dev, addrs[i],
> +						PAGE_SIZE, DMA_BIDIRECTIONAL);
> +				}
> +
> +				ret = -ENOMEM;
> +				goto free_addrs;
> +			}
>  		}
>  	} else {
>  		addrs = kzalloc(npages * sizeof(*addrs), GFP_KERNEL);
> @@ -277,6 +289,8 @@ static int omap_gem_attach_pages(struct drm_gem_object
> *obj) omap_obj->pages = pages;
> 
>  	return 0;
> +free_addrs:
> +	kfree(addrs);
> 

I'd move this blank line before free_addrs.

>  free_pages:
>  	drm_gem_put_pages(obj, pages, true, false);

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 12/33] drm/omap: use dma_mapping_error in omap_gem_dma_sync

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:47:47 Tomi Valkeinen wrote:
> omap_gem_dma_sync() calls dma_map_page() but does not check the possible
> error with dma_mapping_error(). If DMA-API debugging is enabled, the
> debug layer will give a warning if dma_mapping_error() has not been
> used.
> 
> This patch adds proper error handling to omap_gem_dma_sync().
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_gem.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> b/drivers/gpu/drm/omapdrm/omap_gem.c index 7098190815f1..a60c30a59f7e
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -775,9 +775,18 @@ void omap_gem_dma_sync(struct drm_gem_object *obj,
> 
>  		for (i = 0; i < npages; i++) {
>  			if (!omap_obj->addrs[i]) {
> -				omap_obj->addrs[i] = dma_map_page(dev->dev, pages[i], 0,
> +				dma_addr_t addr;
> +
> +				addr = dma_map_page(dev->dev, pages[i], 0,
>  						PAGE_SIZE, DMA_BIDIRECTIONAL);
> +
> +				if (dma_mapping_error(dev->dev, addr)) {
> +					dev_warn(dev->dev, "omap_gem_dma_sync: dma_map_page 
failed\n");

Same comment as for the previous patch.

No need to unmap ?

> +					break;
> +				}
> +
>  				dirty = true;
> +				omap_obj->addrs[i] = addr;
>  			}
>  		}

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 13/33] drm/omap: print an error if display enable fails

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:47:48 Tomi Valkeinen wrote:
> If the panel's enable fails, omap_encoder silently ignores the failure.
> omapdrm should really handle the failure, but unfortunately the whole
> encoder enable codepath is expected to always succeed.
> 
> So for now, catch the enable failure and print an error.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_encoder.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c
> b/drivers/gpu/drm/omapdrm/omap_encoder.c index 61714e9670ae..eb52b3e85d0c
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_encoder.c
> +++ b/drivers/gpu/drm/omapdrm/omap_encoder.c
> @@ -139,11 +139,15 @@ static void omap_encoder_enable(struct drm_encoder
> *encoder) struct omap_encoder *omap_encoder = to_omap_encoder(encoder);
>  	struct omap_dss_device *dssdev = omap_encoder->dssdev;
>  	struct omap_dss_driver *dssdrv = dssdev->driver;
> +	int r;
> 
>  	omap_encoder_update(encoder, omap_crtc_channel(encoder->crtc),
>  			    omap_crtc_timings(encoder->crtc));
> 
> -	dssdrv->enable(dssdev);
> +	r = dssdrv->enable(dssdev);
> +	if (r)
> +		dev_err(encoder->dev->dev, "Failed to enable display '%s'\n",
> +			dssdev->name);

While at it I'd print the error code too.

>  }
> 
>  static int omap_encoder_atomic_check(struct drm_encoder *encoder,

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 17/33] drm/omap: remove support for ext mem & sync

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:47:52 Tomi Valkeinen wrote:
> We no longer have the omapdrm plugin system for SGX, and we can thus
> remove the support for external memory and sync objects from omap_gem.c.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_gem.c | 91 ++++++-----------------------------
>  1 file changed, 16 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> b/drivers/gpu/drm/omapdrm/omap_gem.c index db5dbdb8cd0a..52e4a6c95058
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -33,9 +33,7 @@
>  /* note: we use upper 8 bits of flags for driver-internal flags: */
>  #define OMAP_BO_MEM_DMA_API	0x01000000	/* memory allocated with the
>  dma_alloc_* API */
>  #define OMAP_BO_MEM_SHMEM	0x02000000	/* memory allocated through
>  shmem backing */
> -#define OMAP_BO_MEM_EXT		0x04000000	/* memory allocated externally */
>  #define OMAP_BO_MEM_DMABUF	0x08000000	/* memory imported from a dmabuf
> */
> -#define OMAP_BO_EXT_SYNC	0x10000000	/* externally allocated sync object
> */
> 
>  struct omap_gem_object {
>  	struct drm_gem_object base;
> @@ -1177,20 +1175,6 @@ unlock:
>  	return ret;
>  }
> 
> -/* it is a bit lame to handle updates in this sort of polling way, but
> - * in case of PVR, the GPU can directly update read/write complete
> - * values, and not really tell us which ones it updated.. this also
> - * means that sync_lock is not quite sufficient.  So we'll need to
> - * do something a bit better when it comes time to add support for
> - * separate 2d hw..
> - */
> -void omap_gem_op_update(void)

The function is still referenced from a comment above omap_gem_op_async().

> -{
> -	spin_lock(&sync_lock);
> -	sync_op_update();
> -	spin_unlock(&sync_lock);
> -}
> -
>  /* mark the start of read and/or write operation */
>  int omap_gem_op_start(struct drm_gem_object *obj, enum omap_gem_op op)
>  {
> @@ -1300,43 +1284,6 @@ int omap_gem_op_async(struct drm_gem_object *obj,
> enum omap_gem_op op, return 0;
>  }
> 
> -/* special API so PVR can update the buffer to use a sync-object allocated
> - * from it's sync-obj heap.  Only used for a newly allocated (from PVR's
> - * perspective) sync-object, so we overwrite the new syncobj w/ values
> - * from the already allocated syncobj (if there is one)
> - */
> -int omap_gem_set_sync_object(struct drm_gem_object *obj, void *syncobj)

And this one from a comment in the omap_gem_object() structure.

The rest looks good to me, after updating the comments,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> -{
> -	struct omap_gem_object *omap_obj = to_omap_bo(obj);
> -	int ret = 0;
> -
> -	spin_lock(&sync_lock);
> -
> -	if ((omap_obj->flags & OMAP_BO_EXT_SYNC) && !syncobj) {
> -		/* clearing a previously set syncobj */
> -		syncobj = kmemdup(omap_obj->sync, sizeof(*omap_obj->sync),
> -				  GFP_ATOMIC);
> -		if (!syncobj) {
> -			ret = -ENOMEM;
> -			goto unlock;
> -		}
> -		omap_obj->flags &= ~OMAP_BO_EXT_SYNC;
> -		omap_obj->sync = syncobj;
> -	} else if (syncobj && !(omap_obj->flags & OMAP_BO_EXT_SYNC)) {
> -		/* replacing an existing syncobj */
> -		if (omap_obj->sync) {
> -			memcpy(syncobj, omap_obj->sync, sizeof(*omap_obj->sync));
> -			kfree(omap_obj->sync);
> -		}
> -		omap_obj->flags |= OMAP_BO_EXT_SYNC;
> -		omap_obj->sync = syncobj;
> -	}
> -
> -unlock:
> -	spin_unlock(&sync_lock);
> -	return ret;
> -}
> -
>  /*
> ---------------------------------------------------------------------------
> -- * Constructor & Destructor
>   */
> @@ -1360,28 +1307,23 @@ void omap_gem_free_object(struct drm_gem_object
> *obj) */
>  	WARN_ON(omap_obj->paddr_cnt > 0);
> 
> -	/* don't free externally allocated backing memory */
> -	if (!(omap_obj->flags & OMAP_BO_MEM_EXT)) {
> -		if (omap_obj->pages) {
> -			if (omap_obj->flags & OMAP_BO_MEM_DMABUF)
> -				kfree(omap_obj->pages);
> -			else
> -				omap_gem_detach_pages(obj);
> -		}
> +	if (omap_obj->pages) {
> +		if (omap_obj->flags & OMAP_BO_MEM_DMABUF)
> +			kfree(omap_obj->pages);
> +		else
> +			omap_gem_detach_pages(obj);
> +	}
> 
> -		if (omap_obj->flags & OMAP_BO_MEM_DMA_API) {
> -			dma_free_writecombine(dev->dev, obj->size,
> -					omap_obj->vaddr, omap_obj->paddr);
> -		} else if (omap_obj->vaddr) {
> -			vunmap(omap_obj->vaddr);
> -		} else if (obj->import_attach) {
> -			drm_prime_gem_destroy(obj, omap_obj->sgt);
> -		}
> +	if (omap_obj->flags & OMAP_BO_MEM_DMA_API) {
> +		dma_free_writecombine(dev->dev, obj->size,
> +				omap_obj->vaddr, omap_obj->paddr);
> +	} else if (omap_obj->vaddr) {
> +		vunmap(omap_obj->vaddr);
> +	} else if (obj->import_attach) {
> +		drm_prime_gem_destroy(obj, omap_obj->sgt);
>  	}
> 
> -	/* don't free externally allocated syncobj */
> -	if (!(omap_obj->flags & OMAP_BO_EXT_SYNC))
> -		kfree(omap_obj->sync);
> +	kfree(omap_obj->sync);
> 
>  	drm_gem_object_release(obj);
> 
> @@ -1426,10 +1368,9 @@ struct drm_gem_object *omap_gem_new(struct drm_device
> *dev, * use contiguous memory only if no TILER is available.
>  		 */
>  		flags |= OMAP_BO_MEM_DMA_API;
> -	} else if (!(flags & (OMAP_BO_MEM_EXT | OMAP_BO_MEM_DMABUF))) {
> +	} else if (!(flags & OMAP_BO_MEM_DMABUF)) {
>  		/*
> -		 * All other buffers not backed by external memory or dma_buf
> -		 * are shmem-backed.
> +		 * All other buffers not backed by dma_buf are shmem-backed.
>  		 */
>  		flags |= OMAP_BO_MEM_SHMEM;
>  	}

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 18/33] drm/omap: increase vblank wait timeout

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:47:53 Tomi Valkeinen wrote:
> omap_crtc_wait_pending() waits until the config changes have been taken
> into use, usually at next vblank. The wait-timeout used is 50ms, which
> usually is enough, but in some rare cases not.
> 
> As time wait-timeout is just a safety measure for cases where something
> is broken, we can just as well increase the timeout considerably.

Would it make sense to capture this in a comment in the code ?

Apart from that,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> This patch makes the timeout 250ms.
> 
> 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 f5b19d18fa8b..86b77c4f299a
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -82,7 +82,7 @@ int omap_crtc_wait_pending(struct drm_crtc *crtc)
> 
>  	return wait_event_timeout(omap_crtc->pending_wait,
>  				  !omap_crtc->pending,
> -				  msecs_to_jiffies(50));
> +				  msecs_to_jiffies(250));
>  }
> 
>  /*
> ---------------------------------------------------------------------------
> --

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 30/33] drm/omap: verify that fb plane pitches are the same

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:48:05 Tomi Valkeinen wrote:
> The DSS hardware uses the same ROW_INC value for both Y and UV planes
> for NV12 format. This means that the pitches of the Y and UV planes have
> to match. omapdrm doesn't check this at the moment, and this can lead
> into a broken NV12 fb on the screen.
> 
> This patch adds the check.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

We can probably simplify the implementation further as most of the checks in 
the loop apply to the first place only. Would you like me to give it a go ?

> ---
>  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 ad202dfc1a49..481512db2656 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -449,6 +449,14 @@ struct drm_framebuffer *omap_framebuffer_init(struct
> drm_device *dev, goto fail;
>  		}
> 
> +		if (i > 0 && pitch != mode_cmd->pitches[i - 1]) {
> +			dev_err(dev->dev,
> +				"pitches are not the same between framebuffer planes %d != 
%d\n",
> +				pitch, mode_cmd->pitches[i - 1]);
> +			ret = -EINVAL;
> +			goto fail;
> +		}
> +
>  		plane->bo     = bos[i];
>  		plane->offset = mode_cmd->offsets[i];
>  		plane->pitch  = pitch;

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 32/33] drm/omap: fix crtc->plane property delegation

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:48:07 Tomi Valkeinen wrote:
> Before universal planes we had to have plane specific properties for the
> crtc too, as on the hardware level a crtc uses a plane. In other words,
> e.g. 'zorder' property was added to both planes and crtcs, and
> omap_crtc.c would delegate the property set/get to the primary plane.
> 
> However, the delegation was a bit too generic, delegating all property
> set/get calls to planes. Thus it's possible to set, say, FB_ID, on a
> crtc, which gets redirected to  the primary plane.
> 
> This is not standard, and shouldn't be allowed. To keep backward
> compatibility, we still need to redirect the properties we supported
> earlier for crtcs, namely 'zorder' and 'rotation'.
> 
> This patch redirects only the allowed properties from crtcs to planes.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 58 +++++++++++++++++++++++-----------
>  1 file changed, 40 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 86b77c4f299a..280d80781635
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -419,24 +419,40 @@ static void omap_crtc_atomic_flush(struct drm_crtc
> *crtc, }
>  }
> 
> +static bool omap_crtc_is_plane_prop(struct drm_device *dev,
> +	struct drm_property *property)
> +{
> +	struct omap_drm_private *priv = dev->dev_private;
> +
> +	return	property == priv->zorder_prop ||

s/return\t/return / ?

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Good catch.

> +		property == dev->mode_config.rotation_property;
> +}
> +
>  static int omap_crtc_atomic_set_property(struct drm_crtc *crtc,
>  					 struct drm_crtc_state *state,
>  					 struct drm_property *property,
>  					 uint64_t val)
>  {
> -	struct drm_plane_state *plane_state;
> -	struct drm_plane *plane = crtc->primary;
> +	struct drm_device *dev = crtc->dev;
> 
> -	/*
> -	 * Delegate property set to the primary plane. Get the plane state and
> -	 * set the property directly.
> -	 */
> +	if (omap_crtc_is_plane_prop(dev, property)) {
> +		struct drm_plane_state *plane_state;
> +		struct drm_plane *plane = crtc->primary;
> +
> +		/*
> +		 * Delegate property set to the primary plane. Get the plane
> +		 * state and set the property directly.
> +		 */
> 
> -	plane_state = drm_atomic_get_plane_state(state->state, plane);
> -	if (IS_ERR(plane_state))
> -		return PTR_ERR(plane_state);
> +		plane_state = drm_atomic_get_plane_state(state->state, plane);
> +		if (IS_ERR(plane_state))
> +			return PTR_ERR(plane_state);
> 
> -	return drm_atomic_plane_set_property(plane, plane_state, property, val);
> +		return drm_atomic_plane_set_property(plane, plane_state,
> +				property, val);
> +	}
> +
> +	return -EINVAL;
>  }
> 
>  static int omap_crtc_atomic_get_property(struct drm_crtc *crtc,
> @@ -444,14 +460,20 @@ static int omap_crtc_atomic_get_property(struct
> drm_crtc *crtc, struct drm_property *property,
>  					 uint64_t *val)
>  {
> -	/*
> -	 * Delegate property get to the primary plane. The
> -	 * drm_atomic_plane_get_property() function isn't exported, but can be
> -	 * called through drm_object_property_get_value() as that will call
> -	 * drm_atomic_get_property() for atomic drivers.
> -	 */
> -	return drm_object_property_get_value(&crtc->primary->base, property,
> -					     val);
> +	struct drm_device *dev = crtc->dev;
> +
> +	if (omap_crtc_is_plane_prop(dev, property)) {
> +		/*
> +		 * Delegate property get to the primary plane. The
> +		 * drm_atomic_plane_get_property() function isn't exported, but
> +		 * can be called through drm_object_property_get_value() as that
> +		 * will call drm_atomic_get_property() for atomic drivers.
> +		 */
> +		return drm_object_property_get_value(&crtc->primary->base,
> +				property, val);
> +	}
> +
> +	return -EINVAL;
>  }
> 
>  static const struct drm_crtc_funcs omap_crtc_funcs = {

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 33/33] drm/omap: check if rotation is supported before commit

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Friday 19 February 2016 11:48:08 Tomi Valkeinen wrote:
> omapdrm is missing a check on the validity of the rotation property.
> This leads to omapdrm possibly trying to use rotation on non-rotateable
> framebuffer, which causes the overlay setup to fail.
> 
> This patch adds the necessary check to omap_plane_atomic_check().
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/omapdrm/omap_drv.h   | 1 +
>  drivers/gpu/drm/omapdrm/omap_fb.c    | 8 ++++++++
>  drivers/gpu/drm/omapdrm/omap_plane.c | 6 ++++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h
> b/drivers/gpu/drm/omapdrm/omap_drv.h index c077367dcb1a..16c3eeeae668
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -189,6 +189,7 @@ void omap_framebuffer_update_scanout(struct
> drm_framebuffer *fb, struct omap_drm_window *win, struct omap_overlay_info
> *info);
>  struct drm_connector *omap_framebuffer_get_next_connector(
>  		struct drm_framebuffer *fb, struct drm_connector *from);
> +bool omap_framebuffer_supports_rotation(struct drm_framebuffer *fb);
> 
>  void omap_gem_init(struct drm_device *dev);
>  void omap_gem_deinit(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c
> b/drivers/gpu/drm/omapdrm/omap_fb.c index 481512db2656..610962396eb0 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -145,6 +145,14 @@ static uint32_t get_linear_addr(struct plane *plane,
>  	return plane->paddr + offset;
>  }
> 
> +bool omap_framebuffer_supports_rotation(struct drm_framebuffer *fb)
> +{
> +	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
> +	struct plane *plane = &omap_fb->planes[0];
> +
> +	return omap_gem_flags(plane->bo) & OMAP_BO_TILED;
> +}
> +
>  /* update ovl info for scanout, handles cases of multi-planar fb's, etc.
>   */
>  void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
> b/drivers/gpu/drm/omapdrm/omap_plane.c index d75b197eff46..93ee538a99f5
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> @@ -177,6 +177,12 @@ static int omap_plane_atomic_check(struct drm_plane
> *plane, if (state->crtc_y + state->crtc_h >
> crtc_state->adjusted_mode.vdisplay) return -EINVAL;
> 
> +	if (state->fb) {
> +		if (state->rotation != BIT(DRM_ROTATE_0) &&
> +		    !omap_framebuffer_supports_rotation(state->fb))
> +			return -EINVAL;
> +	}
> +
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 05/33] drm/omap: partial workaround for DRA7 DMM errata i878

[Tomi Valkeinen]

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

Hi,

On 23/02/16 23:57, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.

Thanks for review, good points. I'll drop this patch from the series for
now. The reason being, this issue is rather difficult to reproduce and
test, so any changes I do to patch this will take time to verify.

I'll address the comments later.

> On Friday 19 February 2016 11:47:40 Tomi Valkeinen wrote:
>> Errata i878 says that MPU should not be used to access RAM and DMM at
>> the same time. As it's not possible to prevent MPU accessing RAM, we
>> need to access DMM via a proxy.
>>
>> This patch changes DMM driver to access DMM registers via sDMA. Instead
>> of doing a normal readl/writel call to read/write a register, we use
>> sDMA to copy 4 bytes from/to the DMM registers.
>>
>> This patch provides only a partial workaround for i878, as not only DMM
>> register reads/writes are affected, but also accesses to the DMM mapped
>> buffers (framebuffers, usually).
> 
> Will this patch really improve the situation if the DMM mapping is accessed 
> anyway ?

Depends. For TI environments it helps, as we don't draw with the CPU. If
there's drawing done with CPU, the issue may still happen. I have never
seen it happen in normal situations, though, but I did manage to trigger
it with explicit stress testing.

Also, I don't have any good solution to the framebuffer part... Except
doing a memcpy, ruining the performance.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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

Re: [PATCH 17/33] drm/omap: remove support for ext mem & sync

[Tomi Valkeinen]

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



On 24/02/16 00:42, Laurent Pinchart wrote:

>> -void omap_gem_op_update(void)
> 
> The function is still referenced from a comment above omap_gem_op_async().

Right. I changed the comment to refer to omap_gem_op_finish().

>> -int omap_gem_set_sync_object(struct drm_gem_object *obj, void *syncobj)
> 
> And this one from a comment in the omap_gem_object() structure.

Yep. I think almost all of the comment can be just removed, as SGX no
longer uses this:

@@ -105,17 +105,7 @@ struct omap_gem_object {
         * sync-object allocated on demand (if needed)
         *
         * Per-buffer sync-object for tracking pending and completed hw/dma
-        * read and write operations.  The layout in memory is dictated by
-        * the SGX firmware, which uses this information to stall the command
-        * stream if a surface is not ready yet.
-        *
-        * Note that when buffer is used by SGX, the sync-object needs to be
-        * allocated from a special heap of sync-objects.  This way many sync
-        * objects can be packed in a page, and not waste GPU virtual address
-        * space.  Because of this we have to have a omap_gem_set_sync_object()
-        * API to allow replacement of the syncobj after it has (potentially)
-        * already been allocated.  A bit ugly but I haven't thought of a
-        * better alternative.
+        * read and write operations.
         */
        struct {
                uint32_t write_pending;

 Tomi



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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

Re: [PATCH 18/33] drm/omap: increase vblank wait timeout

[Tomi Valkeinen]

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



On 24/02/16 00:43, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Friday 19 February 2016 11:47:53 Tomi Valkeinen wrote:
>> omap_crtc_wait_pending() waits until the config changes have been taken
>> into use, usually at next vblank. The wait-timeout used is 50ms, which
>> usually is enough, but in some rare cases not.
>>
>> As time wait-timeout is just a safety measure for cases where something
>> is broken, we can just as well increase the timeout considerably.
> 
> Would it make sense to capture this in a comment in the code ?
> 
> Apart from that,
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I added a comment:

/*
 * timeout is set to a "sufficiently" high value, which should cover
 * a single frame refresh even on slower displays.
 */

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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

Re: [PATCH 03/33] HACK: drm/omap: fix memory barrier bug in DMM driver

[Tomi Valkeinen]

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

On 23/02/16 23:13, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Friday 19 February 2016 11:47:38 Tomi Valkeinen wrote:
>> A DMM timeout "timed out waiting for done" has been observed on DRA7
>> devices. The timeout happens rarely, and only when the system is under
>> heavy load.
>>
>> Debugging showed that the timeout can be made to happen much more
>> frequently by optimizing the DMM driver, so that there's almost no code
>> between writing the last DMM descriptors to RAM, and writing to DMM
>> register which starts the DMM transaction.
>>
>> The current theory is that a wmb() does not properly ensure that the
>> data written to RAM is observable by all the components in the system.
>>
>> This DMM timeout has caused interesting (and rare) bugs as the error
>> handling was not functioning properly (the error handling has been fixed
>> in previous commits):
>>
>>  * If a DMM timeout happened when a GEM buffer was being pinned for
>>    display on the screen, a timeout error would be shown, but the driver
>>    would continue programming DSS HW with broken buffer, leading to
>>    SYNCLOST floods and possible crashes.
>>
>>  * If a DMM timeout happened when other user (say, video decoder) was
>>    pinning a GEM buffer, a timeout would be shown but if the user
>>    handled the error properly, no other issues followed.
>>
>>  * If a DMM timeout happened when a GEM buffer was being released, the
>>    driver does not even notice the error, leading to crashes or hang
>>    later.
>>
>> This patch adds wmb() and readl() calls after the last bit is written to
>> RAM, which should ensure that the execution proceeds only after the data
>> is actually in RAM, and thus observable by DMM.
>>
>> This patch is a HACK, as a read-back should not be needed. Further study
>> is required to understand if DMM is somehow special case and read-back
>> is ok, or if DRA7's memory barriers do not work correctly.
> 
> CONFIG_SOC_DRA7XX selects OMAP_INTERCONNECT and OMAP_INTERCONNECT_BARRIER, but 
> dra7xx_map_io() doesn't call omap_barriers_init(). Could that be the root 
> cause of the issue ? I don't have access to a DRA7xx system, would you be able 
> to test that ?

No idea, but I did dig up discussions about this in my mailbox, and it
seems there's been some work done after I wrote this patch, in "Fix
OMAP4 barrier support" series last summer. I'm not sure if that's only
for OMAP4, though.

I'll drop this patch too from the series, and spend a bit more time on
it. This is again something that's a bit tricky to reproduce and test.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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

Re: [PATCH 11/33] drm/omap: use dma_mapping_error in omap_gem_attach_pages

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 2118 bytes --]

On 24/02/16 00:10, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Friday 19 February 2016 11:47:46 Tomi Valkeinen wrote:
>> omap_gem_attach_pages() calls dma_map_page() but does not check the
>> possible error with dma_mapping_error(). If DMA-API debugging is
>> enabled, the debug layer will give a warning if dma_mapping_error() has
>> not been used.
>>
>> This patch adds proper error handling to omap_gem_attach_pages().
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/omap_gem.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
>> b/drivers/gpu/drm/omapdrm/omap_gem.c index 8495a1a4b617..7098190815f1
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
>> @@ -264,6 +264,18 @@ static int omap_gem_attach_pages(struct drm_gem_object
>> *obj) for (i = 0; i < npages; i++) {
>>  			addrs[i] = dma_map_page(dev->dev, pages[i],
>>  					0, PAGE_SIZE, DMA_BIDIRECTIONAL);
>> +
>> +			if (dma_mapping_error(dev->dev, addrs[i])) {
>> +				dev_warn(dev->dev, "omap_gem_attach_pages: dma_map_page 
> failed\n");
> 
> Using dev_warn(dev->dev, "%s: failed to map page\n", __func__); and proper 
> line breaks you'll have no trouble complying with the 80 characters per line 
> limit :-)

Ok.

>> +
>> +				for (i = i - 1; i >= 0; --i) {
> 
> Maybe i-- instead of i = i - 1 ?

Hmm I don't know... I do like assignment in the initializer more than
i--. And why i--? Why not --i? =)

>> +					dma_unmap_page(dev->dev, addrs[i],
>> +						PAGE_SIZE, DMA_BIDIRECTIONAL);
>> +				}
>> +
>> +				ret = -ENOMEM;
>> +				goto free_addrs;
>> +			}
>>  		}
>>  	} else {
>>  		addrs = kzalloc(npages * sizeof(*addrs), GFP_KERNEL);
>> @@ -277,6 +289,8 @@ static int omap_gem_attach_pages(struct drm_gem_object
>> *obj) omap_obj->pages = pages;
>>
>>  	return 0;
>> +free_addrs:
>> +	kfree(addrs);
>>
> 
> I'd move this blank line before free_addrs.

Yes, that makes it cleaner.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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

Re: [PATCH 12/33] drm/omap: use dma_mapping_error in omap_gem_dma_sync

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1893 bytes --]



On 24/02/16 00:14, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Friday 19 February 2016 11:47:47 Tomi Valkeinen wrote:
>> omap_gem_dma_sync() calls dma_map_page() but does not check the possible
>> error with dma_mapping_error(). If DMA-API debugging is enabled, the
>> debug layer will give a warning if dma_mapping_error() has not been
>> used.
>>
>> This patch adds proper error handling to omap_gem_dma_sync().
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/omap_gem.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
>> b/drivers/gpu/drm/omapdrm/omap_gem.c index 7098190815f1..a60c30a59f7e
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
>> @@ -775,9 +775,18 @@ void omap_gem_dma_sync(struct drm_gem_object *obj,
>>
>>  		for (i = 0; i < npages; i++) {
>>  			if (!omap_obj->addrs[i]) {
>> -				omap_obj->addrs[i] = dma_map_page(dev->dev, pages[i], 0,
>> +				dma_addr_t addr;
>> +
>> +				addr = dma_map_page(dev->dev, pages[i], 0,
>>  						PAGE_SIZE, DMA_BIDIRECTIONAL);
>> +
>> +				if (dma_mapping_error(dev->dev, addr)) {
>> +					dev_warn(dev->dev, "omap_gem_dma_sync: dma_map_page 
> failed\n");
> 
> Same comment as for the previous patch.
> 
> No need to unmap ?

I don't know... Maybe, maybe not.

The function doesn't return any error, and the mapped pages are stored
in omap_obj->addrs[]. So, if the failure was temporary, next time we
have already mapped the pages that did succeed. If the failure was not
temporary, well, we don't pass any error anyway, so the pages have to be
unmapped somewhere in any case.

So possibly we could unmap, but I don't see us leaking anything even if
the dma_map_page fails.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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

Re: [PATCH 30/33] drm/omap: verify that fb plane pitches are the same

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 860 bytes --]


On 24/02/16 01:02, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Friday 19 February 2016 11:48:05 Tomi Valkeinen wrote:
>> The DSS hardware uses the same ROW_INC value for both Y and UV planes
>> for NV12 format. This means that the pitches of the Y and UV planes have
>> to match. omapdrm doesn't check this at the moment, and this can lead
>> into a broken NV12 fb on the screen.
>>
>> This patch adds the check.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> We can probably simplify the implementation further as most of the checks in 
> the loop apply to the first place only. Would you like me to give it a go ?

"place" is plane? I don't quite understand what you mean. The checks
apply to all planes.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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

Re: [PATCH 11/33] drm/omap: use dma_mapping_error in omap_gem_attach_pages

[Laurent Pinchart]

Hi Tomi,

On Thursday 25 February 2016 17:39:35 Tomi Valkeinen wrote:
> On 24/02/16 00:10, Laurent Pinchart wrote:
> > On Friday 19 February 2016 11:47:46 Tomi Valkeinen wrote:
> >> omap_gem_attach_pages() calls dma_map_page() but does not check the
> >> possible error with dma_mapping_error(). If DMA-API debugging is
> >> enabled, the debug layer will give a warning if dma_mapping_error() has
> >> not been used.
> >> 
> >> This patch adds proper error handling to omap_gem_attach_pages().
> >> 
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/omapdrm/omap_gem.c | 14 ++++++++++++++
> >>  1 file changed, 14 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> >> b/drivers/gpu/drm/omapdrm/omap_gem.c index 8495a1a4b617..7098190815f1
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> >> @@ -264,6 +264,18 @@ static int omap_gem_attach_pages(struct
> >> drm_gem_object *obj)
> >>  		for (i = 0; i < npages; i++) {
> >>  			addrs[i] = dma_map_page(dev->dev, pages[i],
> >>  					0, PAGE_SIZE, DMA_BIDIRECTIONAL);
> >> +
> >> +			if (dma_mapping_error(dev->dev, addrs[i])) {
> >> +				dev_warn(dev->dev, "omap_gem_attach_pages: dma_map_page
> > failed\n");
> > 
> > Using dev_warn(dev->dev, "%s: failed to map page\n", __func__); and proper
> > line breaks you'll have no trouble complying with the 80 characters per
> > line limit :-)
> 
> Ok.
> 
> >> +
> >> +				for (i = i - 1; i >= 0; --i) {
> > 
> > Maybe i-- instead of i = i - 1 ?
> 
> Hmm I don't know... I do like assignment in the initializer more than
> i--. And why i--? Why not --i? =)

--i is fine with me too ;-) Or maybe

	while (i--)
		dma_unmap_page(dev->dev, addrs[i],
					 PAGE_SIZE, DMA_BIDIRECTIONAL);

> >> +					dma_unmap_page(dev->dev, addrs[i],
> >> +						PAGE_SIZE, DMA_BIDIRECTIONAL);
> >> +				}
> >> +
> >> +				ret = -ENOMEM;
> >> +				goto free_addrs;
> >> +			}
> >> 
> >>  		}
> >>  	
> >>  	} else {
> >>  	
> >>  		addrs = kzalloc(npages * sizeof(*addrs), GFP_KERNEL);
> >> 
> >> @@ -277,6 +289,8 @@ static int omap_gem_attach_pages(struct
> >> drm_gem_object
> >> *obj) omap_obj->pages = pages;
> >> 
> >>  	return 0;
> >> 
> >> +free_addrs:
> >> +	kfree(addrs);
> > 
> > I'd move this blank line before free_addrs.
> 
> Yes, that makes it cleaner.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 12/33] drm/omap: use dma_mapping_error in omap_gem_dma_sync

[Laurent Pinchart]

Hi Tomi,

On Thursday 25 February 2016 17:45:48 Tomi Valkeinen wrote:
> On 24/02/16 00:14, Laurent Pinchart wrote:
> > On Friday 19 February 2016 11:47:47 Tomi Valkeinen wrote:
> >> omap_gem_dma_sync() calls dma_map_page() but does not check the possible
> >> error with dma_mapping_error(). If DMA-API debugging is enabled, the
> >> debug layer will give a warning if dma_mapping_error() has not been
> >> used.
> >> 
> >> This patch adds proper error handling to omap_gem_dma_sync().
> >> 
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/omapdrm/omap_gem.c | 11 ++++++++++-
> >>  1 file changed, 10 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> >> b/drivers/gpu/drm/omapdrm/omap_gem.c index 7098190815f1..a60c30a59f7e
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> >> @@ -775,9 +775,18 @@ void omap_gem_dma_sync(struct drm_gem_object *obj,
> >> 
> >>  		for (i = 0; i < npages; i++) {
> >>  			if (!omap_obj->addrs[i]) {
> >> -				omap_obj->addrs[i] = dma_map_page(dev->dev, pages[i], 0,
> >> +				dma_addr_t addr;
> >> +
> >> +				addr = dma_map_page(dev->dev, pages[i], 0,
> >>  						PAGE_SIZE, DMA_BIDIRECTIONAL);
> >> +
> >> +				if (dma_mapping_error(dev->dev, addr)) {
> >> +					dev_warn(dev->dev, "omap_gem_dma_sync: dma_map_page
> > failed\n");
> > 
> > Same comment as for the previous patch.
> > 
> > No need to unmap ?
> 
> I don't know... Maybe, maybe not.
> 
> The function doesn't return any error, and the mapped pages are stored
> in omap_obj->addrs[]. So, if the failure was temporary, next time we
> have already mapped the pages that did succeed. If the failure was not
> temporary, well, we don't pass any error anyway, so the pages have to be
> unmapped somewhere in any case.
> 
> So possibly we could unmap, but I don't see us leaking anything even if
> the dma_map_page fails.

OK. It's not a new issue anyway, so I'm fine with the patch (after fixing the 
dev_warn()).

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 30/33] drm/omap: verify that fb plane pitches are the same

[Laurent Pinchart]

Hi Tomi,

On Thursday 25 February 2016 17:56:57 Tomi Valkeinen wrote:
> On 24/02/16 01:02, Laurent Pinchart wrote:
> > On Friday 19 February 2016 11:48:05 Tomi Valkeinen wrote:
> >> The DSS hardware uses the same ROW_INC value for both Y and UV planes
> >> for NV12 format. This means that the pitches of the Y and UV planes have
> >> to match. omapdrm doesn't check this at the moment, and this can lead
> >> into a broken NV12 fb on the screen.
> >> 
> >> This patch adds the check.
> >> 
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > We can probably simplify the implementation further as most of the checks
> > in the loop apply to the first place only. Would you like me to give it a
> > go ?
>
> "place" is plane?

Yes, sorry.

> I don't quite understand what you mean. The checks apply to all planes.

My point is that if you add a check to ensure that the pitch is identical in 
all planes, the other tests on the pitch don't have to be repeated for every 
plane.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 11/33] drm/omap: use dma_mapping_error in omap_gem_attach_pages

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 692 bytes --]

On 26/02/16 10:52, Laurent Pinchart wrote:

>>>> +
>>>> +				for (i = i - 1; i >= 0; --i) {
>>>
>>> Maybe i-- instead of i = i - 1 ?
>>
>> Hmm I don't know... I do like assignment in the initializer more than
>> i--. And why i--? Why not --i? =)
> 
> --i is fine with me too ;-) Or maybe
> 
> 	while (i--)
> 		dma_unmap_page(dev->dev, addrs[i],
> 					 PAGE_SIZE, DMA_BIDIRECTIONAL);

Maybe it's just me, but I find it a bit difficult to decipher what
exactly goes on there.

I think the original 'if' is the most clear one: start from i-1, do
while i>=0, decrement by one. It's obvious with a quick glance, whereas
with the 'while' I need to stop and think.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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

Re: [PATCH 30/33] drm/omap: verify that fb plane pitches are the same

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 533 bytes --]



On 26/02/16 10:55, Laurent Pinchart wrote:

>> I don't quite understand what you mean. The checks apply to all planes.
> 
> My point is that if you add a check to ensure that the pitch is identical in 
> all planes, the other tests on the pitch don't have to be repeated for every 
> plane.

Hmm... The other tests also depend on the plane's bpp, so I'm not sure
if any of those can be skipped.

But I agree, the loop with the checks does look somewhat complex, and
there's probably room for improvement.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 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

Re: [PATCH 30/33] drm/omap: verify that fb plane pitches are the same

[Laurent Pinchart]

Hi Tomi,

On Friday 26 February 2016 11:12:21 Tomi Valkeinen wrote:
> On 26/02/16 10:55, Laurent Pinchart wrote:
> >> I don't quite understand what you mean. The checks apply to all planes.
> > 
> > My point is that if you add a check to ensure that the pitch is identical
> > in all planes, the other tests on the pitch don't have to be repeated for
> > every plane.
> 
> Hmm... The other tests also depend on the plane's bpp, so I'm not sure
> if any of those can be skipped.
> 
> But I agree, the loop with the checks does look somewhat complex, and
> there's probably room for improvement.

It doesn't have to be cleaned as part of this patch though, we can handle that 
later.

-- 
Regards,

Laurent Pinchart

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

Re: Re: [PATCH 05/33] drm/omap: partial workaround for DRA7 DMM errata i878

[Peter Ujfalusi]

Hi Laurent,


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 2016-02-23 23:57, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Friday 19 February 2016 11:47:40 Tomi Valkeinen wrote:
>> Errata i878 says that MPU should not be used to access RAM and DMM at
>> the same time. As it's not possible to prevent MPU accessing RAM, we
>> need to access DMM via a proxy.
>>
>> This patch changes DMM driver to access DMM registers via sDMA. Instead
>> of doing a normal readl/writel call to read/write a register, we use
>> sDMA to copy 4 bytes from/to the DMM registers.
>>
>> This patch provides only a partial workaround for i878, as not only DMM
>> register reads/writes are affected, but also accesses to the DMM mapped
>> buffers (framebuffers, usually).
> 
> Will this patch really improve the situation if the DMM mapping is accessed 
> anyway ?
> 
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  Documentation/devicetree/bindings/arm/omap/dmm.txt |   3 +-
>>  drivers/gpu/drm/omapdrm/omap_dmm_priv.h            |   8 ++
>>  drivers/gpu/drm/omapdrm/omap_dmm_tiler.c           | 141 +++++++++++++++++-
>>  3 files changed, 149 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/omap/dmm.txt
>> b/Documentation/devicetree/bindings/arm/omap/dmm.txt index
>> 8bd6d0a238a8..e5fc2eb7f4da 100644
>> --- a/Documentation/devicetree/bindings/arm/omap/dmm.txt
>> +++ b/Documentation/devicetree/bindings/arm/omap/dmm.txt
>> @@ -8,7 +8,8 @@ translation for initiators which need contiguous dma bus
>> addresses.
>>
>>  Required properties:
>>  - compatible:	Should contain "ti,omap4-dmm" for OMAP4 family
>> -		Should contain "ti,omap5-dmm" for OMAP5 and DRA7x family
>> +		Should contain "ti,omap5-dmm" for OMAP5 family
>> +		Should contain "ti,dra7-dmm" for DRA7x family
> 
> Isn't it DRA7xx instead of DRA7x ?
> 
>>  - reg:		Contains DMM register address range (base address and length)
>>  - interrupts:	Should contain an interrupt-specifier for DMM_IRQ.
>>  - ti,hwmods:	Name of the hwmod associated to DMM, which is typically 
> "dmm"
>> diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_priv.h
>> b/drivers/gpu/drm/omapdrm/omap_dmm_priv.h index 9f32a83ca507..4d292ba5bcca
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_dmm_priv.h
>> +++ b/drivers/gpu/drm/omapdrm/omap_dmm_priv.h
>> @@ -155,10 +155,12 @@ struct refill_engine {
>>
>>  struct dmm_platform_data {
>>  	uint32_t cpu_cache_flags;
>> +	bool errata_i878_wa;
>>  };
>>
>>  struct dmm {
>>  	struct device *dev;
>> +	u32 phys_base;
>>  	void __iomem *base;
>>  	int irq;
>>
>> @@ -189,6 +191,12 @@ struct dmm {
>>  	struct list_head alloc_head;
>>
>>  	const struct dmm_platform_data *plat_data;
>> +
>> +	bool dmm_workaround;
>> +	spinlock_t wa_lock;
>> +	u32 *wa_dma_data;
>> +	dma_addr_t wa_dma_handle;
>> +	struct dma_chan *wa_dma_chan;
>>  };
>>
>>  #endif
>> diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
>> b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c index fe5260477b52..3ec5c6633585
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
>> @@ -19,6 +19,7 @@
>>  #include <linux/completion.h>
>>  #include <linux/delay.h>
>>  #include <linux/dma-mapping.h>
>> +#include <linux/dmaengine.h>
>>  #include <linux/errno.h>
>>  #include <linux/init.h>
>>  #include <linux/interrupt.h>
>> @@ -79,14 +80,124 @@ static const uint32_t reg[][4] = {
>>  			DMM_PAT_DESCR__2, DMM_PAT_DESCR__3},
>>  };
>>
>> +static int dmm_dma_copy(struct dmm *dmm, u32 src, u32 dst)
> 
> Anything wrong with using dma_addr_t ?

Nothing wrong with the dma_addr_t.

>> +{
>> +	struct dma_device *dma_dev = dmm->wa_dma_chan->device;
>> +	struct dma_async_tx_descriptor *tx = NULL;
> 
> No need to initialize tx to NULL.

True.

>> +	enum dma_status status;
>> +	dma_cookie_t cookie;
>> +
>> +	tx = dma_dev->device_prep_dma_memcpy(dmm->wa_dma_chan, dst, src, 4, 0);
> 
> Given that you're transferring between an I/O mapped register and system 
> memory, I believe you should use dmaengine_prep_slave_single() instead, with a 
> call to dmaengine_slave_config() to set the I/O mapped register physical 
> address. You will also need to request a slave DMA channel instead of a memcpy 
> DMA channel.

No, slave channel can not be used in this case as a slave channel would
need hardware trigger to do the work and in this case we are not
implementing DMA support for dmm, but we are using the DMA to read and
write the registers due to the errata.

The DMA setup (sDMA for example) is almost identical in both case with
the exception that we don't have trigger configured for the memcpy type.

> 
>> +	if (!tx) {
>> +		dev_err(dmm->dev, "Failed to prepare DMA memcpy\n");
>> +		return -EIO;
>> +	}
>> +
>> +	cookie = tx->tx_submit(tx);
>> +	if (dma_submit_error(cookie)) {
>> +		dev_err(dmm->dev, "Failed to do DMA tx_submit\n");
>> +		return -EIO;
>> +	}
>> +
>> +	dma_async_issue_pending(dmm->wa_dma_chan);
>> +	status = dma_sync_wait(dmm->wa_dma_chan, cookie);
>> +	if (status != DMA_COMPLETE)
>> +		dev_err(dmm->dev, "i878 wa DMA copy failure\n");
>> +
>> +	dmaengine_terminate_all(dmm->wa_dma_chan);
>> +	return 0;
>> +}
>> +
>> +static u32 dmm_read_wa(struct dmm *dmm, u32 reg)
>> +{
>> +	u32 src, dst;
>> +	int r;
>> +
>> +	src = (u32)(dmm->phys_base + reg);
>> +	dst = (u32)dmm->wa_dma_handle;
>> +
>> +	r = dmm_dma_copy(dmm, src, dst);
>> +	if (r) {
>> +		dev_err(dmm->dev, "sDMA read transfer timeout\n");
>> +		return readl(dmm->base + reg);
>> +	}
>> +
>> +	return readl(dmm->wa_dma_data);
> 
> readl() has a memory barrier *after* the read, not before. Similarly writel() 
> has its memory barrier *before* the write. I don't think that's what you want, 
> shouldn't you use readl_relaxed() and writel_relaxed() with explicit barriers 
> ?
> 
> I'm also unsure whether readl and writel are the right primitives, as they 
> target access to I/O memory, not system memory. Can't you use explicit 
> barriers and direct access (return *dmm->wa_dma_data) ?

we would also need to mark it as volatile to make sure that we do reach
out for the data to RAM.

This is done by the readl/writel.

>> +}
>> +
>> +static void dmm_write_wa(struct dmm *dmm, u32 val, u32 reg)
>> +{
>> +	u32 src, dst;
>> +	int r;
>> +
>> +	writel(val, dmm->wa_dma_data);
> 
> Same comment here.


> 
>> +	src = (u32)dmm->wa_dma_handle;
>> +	dst = (u32)(dmm->phys_base + reg);
>> +
>> +	r = dmm_dma_copy(dmm, src, dst);
>> +	if (r) {
>> +		dev_err(dmm->dev, "sDMA write transfer timeout\n");
>> +		writel(val, dmm->base + reg);
>> +	}
>> +}
>> +
>>  static u32 dmm_read(struct dmm *dmm, u32 reg)
>>  {
>> -	return readl(dmm->base + reg);
>> +	if (dmm->dmm_workaround) {
>> +		u32 v;
>> +		unsigned long flags;
>> +
>> +		spin_lock_irqsave(&dmm->wa_lock, flags);
>> +		v = dmm_read_wa(dmm, reg);
> 
> dma_sync_wait() with a spinlock held doesn't seem like the best idea to me.

We are transferring 4 bytes in one go and the interrupts might be
disabled as well when we want to read registers. So if we would use
async DMA + callback + wait_for_completion_timeout() in dma_read/write
things might just fail on us.

With the dma_sync_wait() we are checking the tx_status of the transfer.
Since we have short transfer the DMA is already finished when we first
going to check the status. For sure we are not going to spin too long.

>> +		spin_unlock_irqrestore(&dmm->wa_lock, flags);
>> +
>> +		return v;
>> +	} else {
>> +		return readl(dmm->base + reg);
>> +	}
>>  }
>>
>>  static void dmm_write(struct dmm *dmm, u32 val, u32 reg)
>>  {
>> -	writel(val, dmm->base + reg);
>> +	if (dmm->dmm_workaround) {
>> +		unsigned long flags;
>> +
>> +		spin_lock_irqsave(&dmm->wa_lock, flags);
>> +		dmm_write_wa(dmm, val, reg);
> 
> Ditto.
> 
>> +		spin_unlock_irqrestore(&dmm->wa_lock, flags);
>> +	} else {
>> +		writel(val, dmm->base + reg);
>> +	}
>> +}
>> +
>> +static int dmm_workaround_init(struct dmm *dmm)
>> +{
>> +	dma_cap_mask_t mask;
>> +
>> +	dma_cap_zero(mask);
>> +	dma_cap_set(DMA_MEMCPY, mask);
> 
> Nitpicking, I'd move those two lines down right before the 
> dma_request_channel() call, as they are part of the same logical block of 
> code.

OK.

>> +	spin_lock_init(&dmm->wa_lock);
>> +
>> +	dmm->wa_dma_data = dma_alloc_coherent(dmm->dev, 4, &dmm->wa_dma_handle,
>> GFP_KERNEL);
> 
> Maybe a macro instead of 4 ?

sizeof(u32) ? The DMM registers are 32bits.

> 
>> +	if (!dmm->wa_dma_data)
>> +		return -ENOMEM;
>> +
>> +	dmm->wa_dma_chan = dma_request_channel(mask, NULL, NULL);
>> +	if (!dmm->wa_dma_chan) {
>> +		dma_free_coherent(dmm->dev, 4, dmm->wa_dma_data, dmm->wa_dma_handle);
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void dmm_workaround_uninit(struct dmm *dmm)
>> +{
>> +	dma_release_channel(dmm->wa_dma_chan);
>> +
>> +	dma_free_coherent(dmm->dev, 4, dmm->wa_dma_data, dmm->wa_dma_handle);
>>  }
>>
>>  /* simple allocator to grab next 16 byte aligned memory from txn */
>> @@ -606,6 +717,9 @@ static int omap_dmm_remove(struct platform_device *dev)
>>  		if (omap_dmm->dummy_page)
>>  			__free_page(omap_dmm->dummy_page);
>>
>> +		if (omap_dmm->dmm_workaround)
>> +			dmm_workaround_uninit(omap_dmm);
>> +
>>  		if (omap_dmm->irq > 0)
>>  			free_irq(omap_dmm->irq, omap_dmm);
>>
>> @@ -653,6 +767,7 @@ static int omap_dmm_probe(struct platform_device *dev)
>>  		goto fail;
>>  	}
>>
>> +	omap_dmm->phys_base = mem->start;
>>  	omap_dmm->base = ioremap(mem->start, SZ_2K);
>>
>>  	if (!omap_dmm->base) {
>> @@ -668,6 +783,19 @@ static int omap_dmm_probe(struct platform_device *dev)
>>
>>  	omap_dmm->dev = &dev->dev;
>>
>> +	omap_dmm->dmm_workaround = omap_dmm->plat_data->errata_i878_wa;
>> +
>> +	if (omap_dmm->dmm_workaround) {
> 
> I'd test omap_dmm->plat_data->errata_i878_wa here, omap_dmm->dmm_workaround is 
> initialized to false by kzalloc().
> 
>> +		int r;
>> +		r = dmm_workaround_init(omap_dmm);
> 
> Can't you use the ret variable ? I know it's pre-initialized to -EFAULT, but 
> it seems better to me to set it to explicit values in the three goto fail; 
> error cases that would be impacted, especially given that -EFAULT is the wrong 
> error code to return (and as it would touch that code, the 
> platform_get_resource() + ioremap() calls can be optimized in a 
> devm_ioremap_resource() in another cleanup patch).
> 
>> +		if (r) {
>> +			omap_dmm->dmm_workaround = false;
>> +			dev_err(&dev->dev, "failed to initialize work-around, WA not 
> used\n");
> 
> As this is a non-fatal error, maybe dev_warn() ?

Since it is not treated as fatal error, it might be simplier to

if (omap_dmm->plat_data->errata_i878_wa) {
	if (!dmm_workaround_init(omap_dmm))
		omap_dmm->dmm_workaround = true;
		dev_info(&dev->dev,
			 "workaround for errata i878 in use\n");
	} else {
		dev_warn(&dev->dev,
			 "failed to initialize work-around, WA not used\n");
	}
}

> 
>> +		} else {
>> +			dev_info(&dev->dev, "workaround for errata i878 in use\n");
>> +		}
>> +	}
>> +
>>  	hwinfo = dmm_read(omap_dmm, DMM_PAT_HWINFO);
>>  	omap_dmm->num_engines = (hwinfo >> 24) & 0x1F;
>>  	omap_dmm->num_lut = (hwinfo >> 16) & 0x1F;
>> @@ -1031,6 +1159,11 @@ static const struct dmm_platform_data
>> dmm_omap5_platform_data = { .cpu_cache_flags = OMAP_BO_UNCACHED,
>>  };
>>
>> +static const struct dmm_platform_data dmm_dra7_platform_data = {
>> +	.cpu_cache_flags = OMAP_BO_UNCACHED,
>> +	.errata_i878_wa = true,
>> +};
>> +
>>  static const struct of_device_id dmm_of_match[] = {
>>  	{
>>  		.compatible = "ti,omap4-dmm",
>> @@ -1040,6 +1173,10 @@ static const struct of_device_id dmm_of_match[] = {
>>  		.compatible = "ti,omap5-dmm",
>>  		.data = &dmm_omap5_platform_data,
>>  	},
>> +	{
>> +		.compatible = "ti,dra7-dmm",
>> +		.data = &dmm_dra7_platform_data,
>> +	},
>>  	{},
>>  };
>>  #endif
> 

- Péter

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