[PATCH v3 00/20] OMAP DRM fixes and improvements

[PATCH v3 00/20] OMAP DRM fixes and improvements

[Laurent Pinchart]

Hello,

Here's the third version of my current stack of pending patches for the
omapdrm driver.

All comments received for v2 have been considered and patches updated where
applicable. Changes since v2 include

- Rebased on top of Dave's latest drm-next branch
- Replaced "drm: omapdrm: Use atomic state instead of local device state"
  (09/20) with "drm: omapdrm: Replace DSS manager state check with omapdrm
  CRTC state" (09/20)

Individual changelogs are available in the patches.

The most notable change in this series is still the rework of the IRQ handling
code (patches 06/20 to 19/20) that, beside simplifying the code, ensures that
the vblank count and timestamp get updated properly in order to be reported to
userspace.

The series is based on top of the "[PATCH v4 00/14] Centralize format
information" patches sent to the dri-devel mailing list.

Laurent Pinchart (20):
  drm: omapdrm: fb: Limit number of planes per framebuffer to two
  drm: omapdrm: fb: Use format information provided by the DRM core
  drm: omapdrm: fb: Simplify objects lookup when creating framebuffer
  drm: omapdrm: fb: Simplify mode command checks when creating
    framebuffer
  drm: omapdrm: fb: Turn framebuffer creation error messages into debug
  drm: omapdrm: Handle FIFO underflow IRQs internally
  drm: omapdrm: Handle CRTC error IRQs directly
  drm: omapdrm: Handle OCP error IRQ directly
  drm: omapdrm: Replace DSS manager state check with omapdrm CRTC state
  drm: omapdrm: Only commit planes on active CRTCs
  drm: omapdrm: Check DSS manager state in the enable/disable helpers
  drm: omapdrm: Prevent processing the same event multiple times
  drm: omapdrm: Use a spinlock to protect the CRTC pending flag
  drm: omapdrm: Keep vblank interrupt enabled while CRTC is active
  drm: omapdrm: Don't expose the omap_irq_(un)register() functions
  drm: omapdrm: Remove unused parameter from omap_drm_irq handler
  drm: omapdrm: Don't call DISPC power handling in IRQ wait functions
  drm: omapdrm: Make pipe2vbl function static
  drm: omapdrm: Simplify IRQ wait implementation
  drm: omapdrm: Remove global variables

 drivers/gpu/drm/omapdrm/dss/dispc.c  |   1 -
 drivers/gpu/drm/omapdrm/dss/output.c |   6 +
 drivers/gpu/drm/omapdrm/omap_crtc.c  | 126 +++++++++---------
 drivers/gpu/drm/omapdrm/omap_drv.c   |   7 +-
 drivers/gpu/drm/omapdrm/omap_drv.h   |  51 +-------
 drivers/gpu/drm/omapdrm/omap_fb.c    | 170 ++++++++++++------------
 drivers/gpu/drm/omapdrm/omap_irq.c   | 242 +++++++++++++++++++----------------
 drivers/gpu/drm/omapdrm/omap_plane.c |  24 ----
 8 files changed, 310 insertions(+), 317 deletions(-)

-- 
Regards,

Laurent Pinchart

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

[PATCH v3 01/20] drm: omapdrm: fb: Limit number of planes per framebuffer to two

[Laurent Pinchart]

The only multi-planar format supported by the driver is NV12, there will
thus never be more than two planes per framebuffer.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 5f3337f1e9aa..7646df33f9a1 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -36,7 +36,7 @@ struct format {
 	struct {
 		int stride_bpp;           /* this times width is stride */
 		int sub_y;                /* sub-sample in y dimension */
-	} planes[4];
+	} planes[2];
 	bool yuv;
 };
 
@@ -90,7 +90,7 @@ struct omap_framebuffer {
 	struct drm_framebuffer base;
 	int pin_count;
 	const struct format *format;
-	struct plane planes[4];
+	struct plane planes[2];
 	/* lock for pinning (pin_count and planes.paddr) */
 	struct mutex lock;
 };
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 02/20] drm: omapdrm: fb: Use format information provided by the DRM core

[Laurent Pinchart]

The driver stores in a custom structure named format several pieces of
information about the format that are available in the DRM core. Remove
them and get the information from the DRM core instead.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_fb.c | 91 ++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 7646df33f9a1..3cd627f49e5d 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -29,37 +29,30 @@
  * framebuffer funcs
  */
 
-/* per-format info: */
-struct format {
+/* DSS to DRM formats mapping */
+static const struct {
 	enum omap_color_mode dss_format;
 	uint32_t pixel_format;
-	struct {
-		int stride_bpp;           /* this times width is stride */
-		int sub_y;                /* sub-sample in y dimension */
-	} planes[2];
-	bool yuv;
-};
-
-static const struct format formats[] = {
+} formats[] = {
 	/* 16bpp [A]RGB: */
-	{ OMAP_DSS_COLOR_RGB16,       DRM_FORMAT_RGB565,   {{2, 1}}, false }, /* RGB16-565 */
-	{ OMAP_DSS_COLOR_RGB12U,      DRM_FORMAT_RGBX4444, {{2, 1}}, false }, /* RGB12x-4444 */
-	{ OMAP_DSS_COLOR_RGBX16,      DRM_FORMAT_XRGB4444, {{2, 1}}, false }, /* xRGB12-4444 */
-	{ OMAP_DSS_COLOR_RGBA16,      DRM_FORMAT_RGBA4444, {{2, 1}}, false }, /* RGBA12-4444 */
-	{ OMAP_DSS_COLOR_ARGB16,      DRM_FORMAT_ARGB4444, {{2, 1}}, false }, /* ARGB16-4444 */
-	{ OMAP_DSS_COLOR_XRGB16_1555, DRM_FORMAT_XRGB1555, {{2, 1}}, false }, /* xRGB15-1555 */
-	{ OMAP_DSS_COLOR_ARGB16_1555, DRM_FORMAT_ARGB1555, {{2, 1}}, false }, /* ARGB16-1555 */
+	{ OMAP_DSS_COLOR_RGB16,       DRM_FORMAT_RGB565 },   /* RGB16-565 */
+	{ OMAP_DSS_COLOR_RGB12U,      DRM_FORMAT_RGBX4444 }, /* RGB12x-4444 */
+	{ OMAP_DSS_COLOR_RGBX16,      DRM_FORMAT_XRGB4444 }, /* xRGB12-4444 */
+	{ OMAP_DSS_COLOR_RGBA16,      DRM_FORMAT_RGBA4444 }, /* RGBA12-4444 */
+	{ OMAP_DSS_COLOR_ARGB16,      DRM_FORMAT_ARGB4444 }, /* ARGB16-4444 */
+	{ OMAP_DSS_COLOR_XRGB16_1555, DRM_FORMAT_XRGB1555 }, /* xRGB15-1555 */
+	{ OMAP_DSS_COLOR_ARGB16_1555, DRM_FORMAT_ARGB1555 }, /* ARGB16-1555 */
 	/* 24bpp RGB: */
-	{ OMAP_DSS_COLOR_RGB24P,      DRM_FORMAT_RGB888,   {{3, 1}}, false }, /* RGB24-888 */
+	{ OMAP_DSS_COLOR_RGB24P,      DRM_FORMAT_RGB888 },   /* RGB24-888 */
 	/* 32bpp [A]RGB: */
-	{ OMAP_DSS_COLOR_RGBX32,      DRM_FORMAT_RGBX8888, {{4, 1}}, false }, /* RGBx24-8888 */
-	{ OMAP_DSS_COLOR_RGB24U,      DRM_FORMAT_XRGB8888, {{4, 1}}, false }, /* xRGB24-8888 */
-	{ OMAP_DSS_COLOR_RGBA32,      DRM_FORMAT_RGBA8888, {{4, 1}}, false }, /* RGBA32-8888 */
-	{ OMAP_DSS_COLOR_ARGB32,      DRM_FORMAT_ARGB8888, {{4, 1}}, false }, /* ARGB32-8888 */
+	{ OMAP_DSS_COLOR_RGBX32,      DRM_FORMAT_RGBX8888 }, /* RGBx24-8888 */
+	{ OMAP_DSS_COLOR_RGB24U,      DRM_FORMAT_XRGB8888 }, /* xRGB24-8888 */
+	{ OMAP_DSS_COLOR_RGBA32,      DRM_FORMAT_RGBA8888 }, /* RGBA32-8888 */
+	{ OMAP_DSS_COLOR_ARGB32,      DRM_FORMAT_ARGB8888 }, /* ARGB32-8888 */
 	/* YUV: */
-	{ OMAP_DSS_COLOR_NV12,        DRM_FORMAT_NV12,     {{1, 1}, {1, 2}}, true },
-	{ OMAP_DSS_COLOR_YUV2,        DRM_FORMAT_YUYV,     {{2, 1}}, true },
-	{ OMAP_DSS_COLOR_UYVY,        DRM_FORMAT_UYVY,     {{2, 1}}, true },
+	{ OMAP_DSS_COLOR_NV12,        DRM_FORMAT_NV12 },
+	{ OMAP_DSS_COLOR_YUV2,        DRM_FORMAT_YUYV },
+	{ OMAP_DSS_COLOR_UYVY,        DRM_FORMAT_UYVY },
 };
 
 /* convert from overlay's pixel formats bitmask to an array of fourcc's */
@@ -89,7 +82,8 @@ struct plane {
 struct omap_framebuffer {
 	struct drm_framebuffer base;
 	int pin_count;
-	const struct format *format;
+	const struct drm_format_info *format;
+	enum omap_color_mode dss_format;
 	struct plane planes[2];
 	/* lock for pinning (pin_count and planes.paddr) */
 	struct mutex lock;
@@ -128,13 +122,13 @@ static const struct drm_framebuffer_funcs omap_framebuffer_funcs = {
 };
 
 static uint32_t get_linear_addr(struct plane *plane,
-		const struct format *format, int n, int x, int y)
+		const struct drm_format_info *format, int n, int x, int y)
 {
 	uint32_t offset;
 
-	offset = plane->offset +
-			(x * format->planes[n].stride_bpp) +
-			(y * plane->pitch / format->planes[n].sub_y);
+	offset = plane->offset
+	       + (x * format->cpp[n] / (n == 1 ? 1 : format->hsub))
+	       + (y * plane->pitch / (n == 1 ? 1 : format->vsub));
 
 	return plane->paddr + offset;
 }
@@ -153,11 +147,11 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 		struct omap_drm_window *win, struct omap_overlay_info *info)
 {
 	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
-	const struct format *format = omap_fb->format;
+	const struct drm_format_info *format = omap_fb->format;
 	struct plane *plane = &omap_fb->planes[0];
 	uint32_t x, y, orient = 0;
 
-	info->color_mode = format->dss_format;
+	info->color_mode = omap_fb->dss_format;
 
 	info->pos_x      = win->crtc_x;
 	info->pos_y      = win->crtc_y;
@@ -231,9 +225,9 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 	}
 
 	/* convert to pixels: */
-	info->screen_width /= format->planes[0].stride_bpp;
+	info->screen_width /= format->cpp[0];
 
-	if (format->dss_format == OMAP_DSS_COLOR_NV12) {
+	if (omap_fb->dss_format == OMAP_DSS_COLOR_NV12) {
 		plane = &omap_fb->planes[1];
 
 		if (info->rotation_type == OMAP_DSS_ROT_TILER) {
@@ -382,23 +376,26 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
 struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos)
 {
+	const struct drm_format_info *format = NULL;
 	struct omap_framebuffer *omap_fb = NULL;
 	struct drm_framebuffer *fb = NULL;
-	const struct format *format = NULL;
-	int ret, i, n = drm_format_num_planes(mode_cmd->pixel_format);
+	enum omap_color_mode dss_format = 0;
+	int ret, i;
 
 	DBG("create framebuffer: dev=%p, mode_cmd=%p (%dx%d@%4.4s)",
 			dev, mode_cmd, mode_cmd->width, mode_cmd->height,
 			(char *)&mode_cmd->pixel_format);
 
+	format = drm_format_info(mode_cmd->pixel_format);
+
 	for (i = 0; i < ARRAY_SIZE(formats); i++) {
 		if (formats[i].pixel_format == mode_cmd->pixel_format) {
-			format = &formats[i];
+			dss_format = formats[i].dss_format;
 			break;
 		}
 	}
 
-	if (!format) {
+	if (!format || !dss_format) {
 		dev_err(dev->dev, "unsupported pixel format: %4.4s\n",
 				(char *)&mode_cmd->pixel_format);
 		ret = -EINVAL;
@@ -413,28 +410,32 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 
 	fb = &omap_fb->base;
 	omap_fb->format = format;
+	omap_fb->dss_format = dss_format;
 	mutex_init(&omap_fb->lock);
 
-	for (i = 0; i < n; i++) {
+	for (i = 0; i < format->num_planes; i++) {
 		struct plane *plane = &omap_fb->planes[i];
-		int size, pitch = mode_cmd->pitches[i];
+		unsigned int pitch = mode_cmd->pitches[i];
+		unsigned int hsub = i == 0 ? 1 : format->hsub;
+		unsigned int vsub = i == 0 ? 1 : format->vsub;
+		unsigned int size;
 
-		if (pitch < (mode_cmd->width * format->planes[i].stride_bpp)) {
+		if (pitch < mode_cmd->width * format->cpp[i] / hsub) {
 			dev_err(dev->dev, "provided buffer pitch is too small! %d < %d\n",
-					pitch, mode_cmd->width * format->planes[i].stride_bpp);
+				pitch, mode_cmd->width * format->cpp[i] / hsub);
 			ret = -EINVAL;
 			goto fail;
 		}
 
-		if (pitch % format->planes[i].stride_bpp != 0) {
+		if (pitch % format->cpp[i] != 0) {
 			dev_err(dev->dev,
 				"buffer pitch (%d bytes) is not a multiple of pixel size (%d bytes)\n",
-				pitch, format->planes[i].stride_bpp);
+				pitch, format->cpp[i]);
 			ret = -EINVAL;
 			goto fail;
 		}
 
-		size = pitch * mode_cmd->height / format->planes[i].sub_y;
+		size = pitch * mode_cmd->height / vsub;
 
 		if (size > (omap_gem_mmap_size(bos[i]) - mode_cmd->offsets[i])) {
 			dev_err(dev->dev, "provided buffer object is too small! %d < %d\n",
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 03/20] drm: omapdrm: fb: Simplify objects lookup when creating framebuffer

[Laurent Pinchart]

Merge the single-user objects_lookup inline function into its caller,
allowing reuse of the error code path.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.h | 25 -------------------------
 drivers/gpu/drm/omapdrm/omap_fb.c  | 29 ++++++++++++++++++-----------
 2 files changed, 18 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index dcc30a98b9d4..897c5656908b 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -240,29 +240,4 @@ struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
 uint32_t pipe2vbl(struct drm_crtc *crtc);
 struct omap_dss_device *omap_encoder_get_dssdev(struct drm_encoder *encoder);
 
-/* should these be made into common util helpers?
- */
-
-static inline int objects_lookup(
-		struct drm_file *filp, uint32_t pixel_format,
-		struct drm_gem_object **bos, const uint32_t *handles)
-{
-	int i, n = drm_format_num_planes(pixel_format);
-
-	for (i = 0; i < n; i++) {
-		bos[i] = drm_gem_object_lookup(filp, handles[i]);
-		if (!bos[i])
-			goto fail;
-
-	}
-
-	return 0;
-
-fail:
-	while (--i > 0)
-		drm_gem_object_unreference_unlocked(bos[i]);
-
-	return -ENOENT;
-}
-
 #endif /* __OMAP_DRV_H__ */
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 3cd627f49e5d..705901bcdfb1 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -354,22 +354,29 @@ void omap_framebuffer_describe(struct drm_framebuffer *fb, struct seq_file *m)
 struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
 		struct drm_file *file, const struct drm_mode_fb_cmd2 *mode_cmd)
 {
+	unsigned int num_planes = drm_format_num_planes(mode_cmd->pixel_format);
 	struct drm_gem_object *bos[4];
 	struct drm_framebuffer *fb;
-	int ret;
+	int i;
 
-	ret = objects_lookup(file, mode_cmd->pixel_format,
-			bos, mode_cmd->handles);
-	if (ret)
-		return ERR_PTR(ret);
+	for (i = 0; i < num_planes; i++) {
+		bos[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
+		if (!bos[i]) {
+			fb = ERR_PTR(-ENOENT);
+			goto error;
+		}
+	}
 
 	fb = omap_framebuffer_init(dev, mode_cmd, bos);
-	if (IS_ERR(fb)) {
-		int i, n = drm_format_num_planes(mode_cmd->pixel_format);
-		for (i = 0; i < n; i++)
-			drm_gem_object_unreference_unlocked(bos[i]);
-		return fb;
-	}
+	if (IS_ERR(fb))
+		goto error;
+
+	return fb;
+
+error:
+	while (--i > 0)
+		drm_gem_object_unreference_unlocked(bos[i]);
+
 	return fb;
 }
 
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 04/20] drm: omapdrm: fb: Simplify mode command checks when creating framebuffer

[Laurent Pinchart]

The hardware requires all planes to have an identical pitch in number of
pixels. Given that all supported formats use the same number of bytes
per pixel in all planes, framebuffer creation checks can be simplified.
The implementations assumes that no format use more than two planes
which is true with the existing hardware.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Clarify commit message and mention explicitly in the code that only
  one and two planes formats are supported.
- Rebase on top of the switch to drm_format_info()
---
 drivers/gpu/drm/omapdrm/omap_fb.c | 58 +++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 705901bcdfb1..903d61dd910c 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -387,6 +387,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 	struct omap_framebuffer *omap_fb = NULL;
 	struct drm_framebuffer *fb = NULL;
 	enum omap_color_mode dss_format = 0;
+	unsigned int pitch = mode_cmd->pitches[0];
 	int ret, i;
 
 	DBG("create framebuffer: dev=%p, mode_cmd=%p (%dx%d@%4.4s)",
@@ -420,41 +421,44 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 	omap_fb->dss_format = dss_format;
 	mutex_init(&omap_fb->lock);
 
+	/*
+	 * The code below assumes that no format use more than two planes, and
+	 * that the two planes of multiplane formats need the same number of
+	 * bytes per pixel.
+	 */
+	if (format->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
+		dev_err(dev->dev, "pitches differ between planes 0 and 1\n");
+		ret = -EINVAL;
+		goto fail;
+	}
+
+	if (pitch < mode_cmd->width * format->cpp[0]) {
+		dev_err(dev->dev,
+			"provided buffer pitch is too small! %u < %u\n",
+			pitch, mode_cmd->width * format->cpp[0]);
+		ret = -EINVAL;
+		goto fail;
+	}
+
+	if (pitch % format->cpp[0]) {
+		dev_err(dev->dev,
+			"buffer pitch (%u bytes) is not a multiple of pixel size (%u bytes)\n",
+			pitch, format->cpp[0]);
+		ret = -EINVAL;
+		goto fail;
+	}
+
 	for (i = 0; i < format->num_planes; i++) {
 		struct plane *plane = &omap_fb->planes[i];
-		unsigned int pitch = mode_cmd->pitches[i];
-		unsigned int hsub = i == 0 ? 1 : format->hsub;
 		unsigned int vsub = i == 0 ? 1 : format->vsub;
 		unsigned int size;
 
-		if (pitch < mode_cmd->width * format->cpp[i] / hsub) {
-			dev_err(dev->dev, "provided buffer pitch is too small! %d < %d\n",
-				pitch, mode_cmd->width * format->cpp[i] / hsub);
-			ret = -EINVAL;
-			goto fail;
-		}
-
-		if (pitch % format->cpp[i] != 0) {
-			dev_err(dev->dev,
-				"buffer pitch (%d bytes) is not a multiple of pixel size (%d bytes)\n",
-				pitch, format->cpp[i]);
-			ret = -EINVAL;
-			goto fail;
-		}
-
 		size = pitch * mode_cmd->height / vsub;
 
-		if (size > (omap_gem_mmap_size(bos[i]) - mode_cmd->offsets[i])) {
-			dev_err(dev->dev, "provided buffer object is too small! %d < %d\n",
-					bos[i]->size - mode_cmd->offsets[i], size);
-			ret = -EINVAL;
-			goto fail;
-		}
-
-		if (i > 0 && pitch != mode_cmd->pitches[i - 1]) {
+		if (size > omap_gem_mmap_size(bos[i]) - mode_cmd->offsets[i]) {
 			dev_err(dev->dev,
-				"pitches are not the same between framebuffer planes %d != %d\n",
-				pitch, mode_cmd->pitches[i - 1]);
+				"provided buffer object is too small! %d < %d\n",
+				bos[i]->size - mode_cmd->offsets[i], size);
 			ret = -EINVAL;
 			goto fail;
 		}
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 05/20] drm: omapdrm: fb: Turn framebuffer creation error messages into debug

[Laurent Pinchart]

Don't print userspace parameters validation failures as error messages
to avoid giving userspace the ability to flood the kernel log.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 903d61dd910c..691f5c9f73f5 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -404,8 +404,8 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 	}
 
 	if (!format || !dss_format) {
-		dev_err(dev->dev, "unsupported pixel format: %4.4s\n",
-				(char *)&mode_cmd->pixel_format);
+		dev_dbg(dev->dev, "unsupported pixel format: %4.4s\n",
+			(char *)&mode_cmd->pixel_format);
 		ret = -EINVAL;
 		goto fail;
 	}
@@ -427,13 +427,13 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 	 * bytes per pixel.
 	 */
 	if (format->num_planes == 2 && pitch != mode_cmd->pitches[1]) {
-		dev_err(dev->dev, "pitches differ between planes 0 and 1\n");
+		dev_dbg(dev->dev, "pitches differ between planes 0 and 1\n");
 		ret = -EINVAL;
 		goto fail;
 	}
 
 	if (pitch < mode_cmd->width * format->cpp[0]) {
-		dev_err(dev->dev,
+		dev_dbg(dev->dev,
 			"provided buffer pitch is too small! %u < %u\n",
 			pitch, mode_cmd->width * format->cpp[0]);
 		ret = -EINVAL;
@@ -441,7 +441,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 	}
 
 	if (pitch % format->cpp[0]) {
-		dev_err(dev->dev,
+		dev_dbg(dev->dev,
 			"buffer pitch (%u bytes) is not a multiple of pixel size (%u bytes)\n",
 			pitch, format->cpp[0]);
 		ret = -EINVAL;
@@ -456,7 +456,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 		size = pitch * mode_cmd->height / vsub;
 
 		if (size > omap_gem_mmap_size(bos[i]) - mode_cmd->offsets[i]) {
-			dev_err(dev->dev,
+			dev_dbg(dev->dev,
 				"provided buffer object is too small! %d < %d\n",
 				bos[i]->size - mode_cmd->offsets[i], size);
 			ret = -EINVAL;
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 06/20] drm: omapdrm: Handle FIFO underflow IRQs internally

[Laurent Pinchart]

As the FIFO underflow IRQ handler just prints an error message to the
kernel log, simplify the code by not registering one IRQ handler per
plane but print the messages directly from the main IRQ handler.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Only register error IRQs that exist on the HW
---
 drivers/gpu/drm/omapdrm/omap_drv.c   |  4 +--
 drivers/gpu/drm/omapdrm/omap_drv.h   |  2 +-
 drivers/gpu/drm/omapdrm/omap_irq.c   | 66 ++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/omapdrm/omap_plane.c | 24 -------------
 4 files changed, 66 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index e1cfba51cff6..23e12362c4b5 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -321,8 +321,6 @@ static int omap_modeset_init(struct drm_device *dev)
 
 	drm_mode_config_init(dev);
 
-	omap_drm_irq_install(dev);
-
 	ret = omap_modeset_init_properties(dev);
 	if (ret < 0)
 		return ret;
@@ -491,6 +489,8 @@ static int omap_modeset_init(struct drm_device *dev)
 
 	drm_mode_config_reset(dev);
 
+	omap_drm_irq_install(dev);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 897c5656908b..6bc1588e1c9e 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -102,7 +102,7 @@ struct omap_drm_private {
 
 	/* irq handling: */
 	struct list_head irq_list;    /* list of omap_drm_irq */
-	uint32_t vblank_mask;         /* irq bits set for userspace vblank */
+	uint32_t irq_mask;		/* enabled irqs in addition to irq_list */
 	struct omap_drm_irq error_handler;
 
 	/* atomic commit */
diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
index 60e1e8016708..23045f528a93 100644
--- a/drivers/gpu/drm/omapdrm/omap_irq.c
+++ b/drivers/gpu/drm/omapdrm/omap_irq.c
@@ -32,7 +32,7 @@ static void omap_irq_update(struct drm_device *dev)
 {
 	struct omap_drm_private *priv = dev->dev_private;
 	struct omap_drm_irq *irq;
-	uint32_t irqmask = priv->vblank_mask;
+	uint32_t irqmask = priv->irq_mask;
 
 	assert_spin_locked(&list_lock);
 
@@ -153,7 +153,7 @@ int omap_irq_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	DBG("dev=%p, crtc=%u", dev, pipe);
 
 	spin_lock_irqsave(&list_lock, flags);
-	priv->vblank_mask |= pipe2vbl(crtc);
+	priv->irq_mask |= pipe2vbl(crtc);
 	omap_irq_update(dev);
 	spin_unlock_irqrestore(&list_lock, flags);
 
@@ -178,11 +178,52 @@ void omap_irq_disable_vblank(struct drm_device *dev, unsigned int pipe)
 	DBG("dev=%p, crtc=%u", dev, pipe);
 
 	spin_lock_irqsave(&list_lock, flags);
-	priv->vblank_mask &= ~pipe2vbl(crtc);
+	priv->irq_mask &= ~pipe2vbl(crtc);
 	omap_irq_update(dev);
 	spin_unlock_irqrestore(&list_lock, flags);
 }
 
+static void omap_irq_fifo_underflow(struct omap_drm_private *priv,
+				    u32 irqstatus)
+{
+	static DEFINE_RATELIMIT_STATE(_rs, DEFAULT_RATELIMIT_INTERVAL,
+				      DEFAULT_RATELIMIT_BURST);
+	static const struct {
+		const char *name;
+		u32 mask;
+	} sources[] = {
+		{ "gfx", DISPC_IRQ_GFX_FIFO_UNDERFLOW },
+		{ "vid1", DISPC_IRQ_VID1_FIFO_UNDERFLOW },
+		{ "vid2", DISPC_IRQ_VID2_FIFO_UNDERFLOW },
+		{ "vid3", DISPC_IRQ_VID3_FIFO_UNDERFLOW },
+	};
+
+	const u32 mask = DISPC_IRQ_GFX_FIFO_UNDERFLOW
+		       | DISPC_IRQ_VID1_FIFO_UNDERFLOW
+		       | DISPC_IRQ_VID2_FIFO_UNDERFLOW
+		       | DISPC_IRQ_VID3_FIFO_UNDERFLOW;
+	unsigned int i;
+
+	spin_lock(&list_lock);
+	irqstatus &= priv->irq_mask & mask;
+	spin_unlock(&list_lock);
+
+	if (!irqstatus)
+		return;
+
+	if (!__ratelimit(&_rs))
+		return;
+
+	DRM_ERROR("FIFO underflow on ");
+
+	for (i = 0; i < ARRAY_SIZE(sources); ++i) {
+		if (sources[i].mask & irqstatus)
+			pr_cont("%s ", sources[i].name);
+	}
+
+	pr_cont("(0x%08x)\n", irqstatus);
+}
+
 static irqreturn_t omap_irq_handler(int irq, void *arg)
 {
 	struct drm_device *dev = (struct drm_device *) arg;
@@ -205,6 +246,8 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
 			drm_handle_vblank(dev, id);
 	}
 
+	omap_irq_fifo_underflow(priv, irqstatus);
+
 	spin_lock_irqsave(&list_lock, flags);
 	list_for_each_entry_safe(handler, n, &priv->irq_list, node) {
 		if (handler->irqmask & irqstatus) {
@@ -218,6 +261,13 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+static const u32 error_irqs[] = {
+	[OMAP_DSS_GFX] = DISPC_IRQ_GFX_FIFO_UNDERFLOW,
+	[OMAP_DSS_VIDEO1] = DISPC_IRQ_VID1_FIFO_UNDERFLOW,
+	[OMAP_DSS_VIDEO2] = DISPC_IRQ_VID2_FIFO_UNDERFLOW,
+	[OMAP_DSS_VIDEO3] = DISPC_IRQ_VID3_FIFO_UNDERFLOW,
+};
+
 /*
  * We need a special version, instead of just using drm_irq_install(),
  * because we need to register the irq via omapdss.  Once omapdss and
@@ -229,10 +279,20 @@ int omap_drm_irq_install(struct drm_device *dev)
 {
 	struct omap_drm_private *priv = dev->dev_private;
 	struct omap_drm_irq *error_handler = &priv->error_handler;
+	unsigned int max_planes;
+	unsigned int i;
 	int ret;
 
 	INIT_LIST_HEAD(&priv->irq_list);
 
+	priv->irq_mask = 0;
+
+	max_planes = min(ARRAY_SIZE(priv->planes), ARRAY_SIZE(error_irqs));
+	for (i = 0; i < max_planes; ++i) {
+		if (priv->planes[i])
+			priv->irq_mask |= error_irqs[i];
+	}
+
 	dispc_runtime_get();
 	dispc_clear_irqstatus(0xffffffff);
 	dispc_runtime_put();
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
index 66ac8c40db26..7852272b4fe7 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -43,8 +43,6 @@ struct omap_plane {
 
 	uint32_t nformats;
 	uint32_t formats[32];
-
-	struct omap_drm_irq error_irq;
 };
 
 struct omap_plane_state {
@@ -200,8 +198,6 @@ static void omap_plane_destroy(struct drm_plane *plane)
 
 	DBG("%s", omap_plane->name);
 
-	omap_irq_unregister(plane->dev, &omap_plane->error_irq);
-
 	drm_plane_cleanup(plane);
 
 	kfree(omap_plane);
@@ -320,14 +316,6 @@ static const struct drm_plane_funcs omap_plane_funcs = {
 	.atomic_get_property = omap_plane_atomic_get_property,
 };
 
-static void omap_plane_error_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
-{
-	struct omap_plane *omap_plane =
-			container_of(irq, struct omap_plane, error_irq);
-	DRM_ERROR_RATELIMITED("%s: errors: %08x\n", omap_plane->name,
-		irqstatus);
-}
-
 static const char *plane_names[] = {
 	[OMAP_DSS_GFX] = "gfx",
 	[OMAP_DSS_VIDEO1] = "vid1",
@@ -335,13 +323,6 @@ static const char *plane_names[] = {
 	[OMAP_DSS_VIDEO3] = "vid3",
 };
 
-static const uint32_t error_irqs[] = {
-	[OMAP_DSS_GFX] = DISPC_IRQ_GFX_FIFO_UNDERFLOW,
-	[OMAP_DSS_VIDEO1] = DISPC_IRQ_VID1_FIFO_UNDERFLOW,
-	[OMAP_DSS_VIDEO2] = DISPC_IRQ_VID2_FIFO_UNDERFLOW,
-	[OMAP_DSS_VIDEO3] = DISPC_IRQ_VID3_FIFO_UNDERFLOW,
-};
-
 /* initialize plane */
 struct drm_plane *omap_plane_init(struct drm_device *dev,
 		int id, enum drm_plane_type type)
@@ -365,10 +346,6 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
 
 	plane = &omap_plane->base;
 
-	omap_plane->error_irq.irqmask = error_irqs[id];
-	omap_plane->error_irq.irq = omap_plane_error_irq;
-	omap_irq_register(dev, &omap_plane->error_irq);
-
 	ret = drm_universal_plane_init(dev, plane, (1 << priv->num_crtcs) - 1,
 				       &omap_plane_funcs, omap_plane->formats,
 				       omap_plane->nformats, type, NULL);
@@ -382,7 +359,6 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
 	return plane;
 
 error:
-	omap_irq_unregister(plane->dev, &omap_plane->error_irq);
 	kfree(omap_plane);
 	return NULL;
 }
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 07/20] drm: omapdrm: Handle CRTC error IRQs directly

[Laurent Pinchart]

Instead of going through a complicated registration mechanism, just
expose the CRTC error IRQ function and call it directly from the main
IRQ handler.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 12 ++----------
 drivers/gpu/drm/omapdrm/omap_drv.h  |  1 +
 drivers/gpu/drm/omapdrm/omap_irq.c  |  8 ++++++++
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 180f644e861e..cdcfda31043e 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -37,7 +37,6 @@ struct omap_crtc {
 	struct omap_video_timings timings;
 
 	struct omap_drm_irq vblank_irq;
-	struct omap_drm_irq error_irq;
 
 	bool ignore_digit_sync_lost;
 
@@ -275,10 +274,9 @@ static void omap_crtc_complete_page_flip(struct drm_crtc *crtc)
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 }
 
-static void omap_crtc_error_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
+void omap_crtc_error_irq(struct drm_crtc *crtc, uint32_t irqstatus)
 {
-	struct omap_crtc *omap_crtc =
-			container_of(irq, struct omap_crtc, error_irq);
+	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
 
 	if (omap_crtc->ignore_digit_sync_lost) {
 		irqstatus &= ~DISPC_IRQ_SYNC_LOST_DIGIT;
@@ -325,7 +323,6 @@ static void omap_crtc_destroy(struct drm_crtc *crtc)
 	DBG("%s", omap_crtc->name);
 
 	WARN_ON(omap_crtc->vblank_irq.registered);
-	omap_irq_unregister(crtc->dev, &omap_crtc->error_irq);
 
 	drm_crtc_cleanup(crtc);
 
@@ -562,11 +559,6 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 	omap_crtc->vblank_irq.irqmask = pipe2vbl(crtc);
 	omap_crtc->vblank_irq.irq = omap_crtc_vblank_irq;
 
-	omap_crtc->error_irq.irqmask =
-			dispc_mgr_get_sync_lost_irq(channel);
-	omap_crtc->error_irq.irq = omap_crtc_error_irq;
-	omap_irq_register(dev, &omap_crtc->error_irq);
-
 	ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL,
 					&omap_crtc_funcs, NULL);
 	if (ret < 0) {
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 6bc1588e1c9e..7a1d8384147c 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -155,6 +155,7 @@ void omap_crtc_pre_uninit(void);
 struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 		struct drm_plane *plane, enum omap_channel channel, int id);
 int omap_crtc_wait_pending(struct drm_crtc *crtc);
+void omap_crtc_error_irq(struct drm_crtc *crtc, uint32_t irqstatus);
 
 struct drm_plane *omap_plane_init(struct drm_device *dev,
 		int id, enum drm_plane_type type);
diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
index 23045f528a93..f0f110faf1e6 100644
--- a/drivers/gpu/drm/omapdrm/omap_irq.c
+++ b/drivers/gpu/drm/omapdrm/omap_irq.c
@@ -241,9 +241,13 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
 
 	for (id = 0; id < priv->num_crtcs; id++) {
 		struct drm_crtc *crtc = priv->crtcs[id];
+		enum omap_channel channel = omap_crtc_channel(crtc);
 
 		if (irqstatus & pipe2vbl(crtc))
 			drm_handle_vblank(dev, id);
+
+		if (irqstatus & dispc_mgr_get_sync_lost_irq(channel))
+			omap_crtc_error_irq(crtc, irqstatus);
 	}
 
 	omap_irq_fifo_underflow(priv, irqstatus);
@@ -279,6 +283,7 @@ int omap_drm_irq_install(struct drm_device *dev)
 {
 	struct omap_drm_private *priv = dev->dev_private;
 	struct omap_drm_irq *error_handler = &priv->error_handler;
+	unsigned int num_mgrs = dss_feat_get_num_mgrs();
 	unsigned int max_planes;
 	unsigned int i;
 	int ret;
@@ -293,6 +298,9 @@ int omap_drm_irq_install(struct drm_device *dev)
 			priv->irq_mask |= error_irqs[i];
 	}
 
+	for (i = 0; i < num_mgrs; ++i)
+		priv->irq_mask |= dispc_mgr_get_sync_lost_irq(i);
+
 	dispc_runtime_get();
 	dispc_clear_irqstatus(0xffffffff);
 	dispc_runtime_put();
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 08/20] drm: omapdrm: Handle OCP error IRQ directly

[Laurent Pinchart]

Instead of going through a complicated registration mechanism, just
call the OCP error IRQ handler directly from the main IRQ handler.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Rename IRQ handler to omap_irq_ocp_error_handler()
- Replace hex error value with "OCP error" message
---
 drivers/gpu/drm/omapdrm/omap_drv.h |  1 -
 drivers/gpu/drm/omapdrm/omap_irq.c | 28 ++++++++++------------------
 2 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 7a1d8384147c..05375bc40091 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -103,7 +103,6 @@ struct omap_drm_private {
 	/* irq handling: */
 	struct list_head irq_list;    /* list of omap_drm_irq */
 	uint32_t irq_mask;		/* enabled irqs in addition to irq_list */
-	struct omap_drm_irq error_handler;
 
 	/* atomic commit */
 	struct {
diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
index f0f110faf1e6..257c1f3c76f3 100644
--- a/drivers/gpu/drm/omapdrm/omap_irq.c
+++ b/drivers/gpu/drm/omapdrm/omap_irq.c
@@ -21,12 +21,6 @@
 
 static DEFINE_SPINLOCK(list_lock);
 
-static void omap_irq_error_handler(struct omap_drm_irq *irq,
-		uint32_t irqstatus)
-{
-	DRM_ERROR("errors: %08x\n", irqstatus);
-}
-
 /* call with list_lock and dispc runtime held */
 static void omap_irq_update(struct drm_device *dev)
 {
@@ -224,6 +218,14 @@ static void omap_irq_fifo_underflow(struct omap_drm_private *priv,
 	pr_cont("(0x%08x)\n", irqstatus);
 }
 
+static void omap_irq_ocp_error_handler(u32 irqstatus)
+{
+	if (!(irqstatus & DISPC_IRQ_OCP_ERR))
+		return;
+
+	DRM_ERROR("OCP error\n");
+}
+
 static irqreturn_t omap_irq_handler(int irq, void *arg)
 {
 	struct drm_device *dev = (struct drm_device *) arg;
@@ -250,6 +252,7 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
 			omap_crtc_error_irq(crtc, irqstatus);
 	}
 
+	omap_irq_ocp_error_handler(irqstatus);
 	omap_irq_fifo_underflow(priv, irqstatus);
 
 	spin_lock_irqsave(&list_lock, flags);
@@ -282,7 +285,6 @@ static const u32 error_irqs[] = {
 int omap_drm_irq_install(struct drm_device *dev)
 {
 	struct omap_drm_private *priv = dev->dev_private;
-	struct omap_drm_irq *error_handler = &priv->error_handler;
 	unsigned int num_mgrs = dss_feat_get_num_mgrs();
 	unsigned int max_planes;
 	unsigned int i;
@@ -290,7 +292,7 @@ int omap_drm_irq_install(struct drm_device *dev)
 
 	INIT_LIST_HEAD(&priv->irq_list);
 
-	priv->irq_mask = 0;
+	priv->irq_mask = DISPC_IRQ_OCP_ERR;
 
 	max_planes = min(ARRAY_SIZE(priv->planes), ARRAY_SIZE(error_irqs));
 	for (i = 0; i < max_planes; ++i) {
@@ -309,16 +311,6 @@ int omap_drm_irq_install(struct drm_device *dev)
 	if (ret < 0)
 		return ret;
 
-	error_handler->irq = omap_irq_error_handler;
-	error_handler->irqmask = DISPC_IRQ_OCP_ERR;
-
-	/* for now ignore DISPC_IRQ_SYNC_LOST_DIGIT.. really I think
-	 * we just need to ignore it while enabling tv-out
-	 */
-	error_handler->irqmask &= ~DISPC_IRQ_SYNC_LOST_DIGIT;
-
-	omap_irq_register(dev, error_handler);
-
 	dev->irq_enabled = true;
 
 	return 0;
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 09/20] drm: omapdrm: Replace DSS manager state check with omapdrm CRTC state

[Laurent Pinchart]

Instead of conditioning planes update based on the DD manager state, use
the enabled field newly added to the omap_crtc structure. This reduces
the dependency from the DRM layer to the DSS layer.

The enabled field is a transitory measure, the implementation should use
the CRTC atomic state instead. However, given that CRTCs are currently
not enabled/disabled through their .enable() and .disable() operations
but through a convoluted code paths starting at the associated encoder
operations, there is not clear guarantee that the atomic state always
matches the hardware state. This will be refactored later, at which
point the enabled field will be removed.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v2:

- Use enabled field in struct omap_crtc instead of CRTC atomic state
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index cdcfda31043e..f41a638c8d65 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -40,6 +40,7 @@ struct omap_crtc {
 
 	bool ignore_digit_sync_lost;
 
+	bool enabled;
 	bool pending;
 	wait_queue_head_t pending_wait;
 };
@@ -136,6 +137,7 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable)
 
 	if (omap_crtc_output[channel]->output_type == OMAP_DISPLAY_TYPE_HDMI) {
 		dispc_mgr_enable(channel, enable);
+		omap_crtc->enabled = enable;
 		return;
 	}
 
@@ -172,6 +174,7 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable)
 	}
 
 	dispc_mgr_enable(channel, enable);
+	omap_crtc->enabled = enable;
 
 	ret = omap_irq_wait(dev, wait, msecs_to_jiffies(100));
 	if (ret) {
@@ -421,18 +424,19 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
 		dispc_mgr_set_gamma(omap_crtc->channel, lut, length);
 	}
 
-	if (dispc_mgr_is_enabled(omap_crtc->channel)) {
+	/* Only flush the CRTC if it is currently enabled. */
+	if (!omap_crtc->enabled)
+		return;
 
-		DBG("%s: GO", omap_crtc->name);
+	DBG("%s: GO", omap_crtc->name);
 
-		rmb();
-		WARN_ON(omap_crtc->pending);
-		omap_crtc->pending = true;
-		wmb();
+	rmb();
+	WARN_ON(omap_crtc->pending);
+	omap_crtc->pending = true;
+	wmb();
 
-		dispc_mgr_go(omap_crtc->channel);
-		omap_irq_register(crtc->dev, &omap_crtc->vblank_irq);
-	}
+	dispc_mgr_go(omap_crtc->channel);
+	omap_irq_register(crtc->dev, &omap_crtc->vblank_irq);
 }
 
 static bool omap_crtc_is_plane_prop(struct drm_device *dev,
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 10/20] drm: omapdrm: Only commit planes on active CRTCs

[Laurent Pinchart]

The DRM core supports skipping plane update for inactive CRTCs for
hardware that don't need it or can't cope with it. That's our case, so
use the DRM core infrastructure instead of reinventing it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 8 +++++++-
 drivers/gpu/drm/omapdrm/omap_drv.c  | 3 ++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index f41a638c8d65..4b7e16786e1e 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -424,7 +424,13 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
 		dispc_mgr_set_gamma(omap_crtc->channel, lut, length);
 	}
 
-	/* Only flush the CRTC if it is currently enabled. */
+	/*
+	 * Only flush the CRTC if it is currently enabled. CRTCs that require a
+	 * mode set are disabled prior plane updates and enabled afterwards.
+	 * They are thus not active (regardless of what their CRTC core state
+	 * reports) and the DRM core could thus call this function even though
+	 * the CRTC is currently disabled. Do nothing in that case.
+	 */
 	if (!omap_crtc->enabled)
 		return;
 
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 23e12362c4b5..113f7a22c41c 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -96,7 +96,8 @@ static void omap_atomic_complete(struct omap_atomic_state_commit *commit)
 	dispc_runtime_get();
 
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
-	drm_atomic_helper_commit_planes(dev, old_state, 0);
+	drm_atomic_helper_commit_planes(dev, old_state,
+					DRM_PLANE_COMMIT_ACTIVE_ONLY);
 	drm_atomic_helper_commit_modeset_enables(dev, old_state);
 
 	omap_atomic_wait_for_completion(dev, old_state);
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 11/20] drm: omapdrm: Check DSS manager state in the enable/disable helpers

[Laurent Pinchart]

The omapdrm DSS manager enable/disable operations check the DSS manager
state to avoid double enabling/disabling. Move that code to the DSS
manager to decrease the dependency of the DRM layer to the DSS layer.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/dss/dispc.c  | 1 -
 drivers/gpu/drm/omapdrm/dss/output.c | 6 ++++++
 drivers/gpu/drm/omapdrm/omap_crtc.c  | 3 ---
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 535240fba671..ab150bf21dd8 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -2911,7 +2911,6 @@ bool dispc_mgr_is_enabled(enum omap_channel channel)
 {
 	return !!mgr_fld_read(channel, DISPC_MGR_FLD_ENABLE);
 }
-EXPORT_SYMBOL(dispc_mgr_is_enabled);
 
 void dispc_wb_enable(bool enable)
 {
diff --git a/drivers/gpu/drm/omapdrm/dss/output.c b/drivers/gpu/drm/omapdrm/dss/output.c
index 24f859488201..f0be621895fa 100644
--- a/drivers/gpu/drm/omapdrm/dss/output.c
+++ b/drivers/gpu/drm/omapdrm/dss/output.c
@@ -217,12 +217,18 @@ EXPORT_SYMBOL(dss_mgr_set_lcd_config);
 
 int dss_mgr_enable(enum omap_channel channel)
 {
+	if (dispc_mgr_is_enabled(channel))
+		return 0;
+
 	return dss_mgr_ops->enable(channel);
 }
 EXPORT_SYMBOL(dss_mgr_enable);
 
 void dss_mgr_disable(enum omap_channel channel)
 {
+	if (!dispc_mgr_is_enabled(channel))
+		return;
+
 	dss_mgr_ops->disable(channel);
 }
 EXPORT_SYMBOL(dss_mgr_disable);
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 4b7e16786e1e..a0c26592fc69 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -141,9 +141,6 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable)
 		return;
 	}
 
-	if (dispc_mgr_is_enabled(channel) == enable)
-		return;
-
 	if (omap_crtc->channel == OMAP_DSS_CHANNEL_DIGIT) {
 		/*
 		 * Digit output produces some sync lost interrupts during the
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 12/20] drm: omapdrm: Prevent processing the same event multiple times

[Laurent Pinchart]

The vblank interrupt is disabled after one occurrence, preventing the
atomic update event from being processed twice. However, this also
prevents the software frame counter from being updated correctly that
would require vblank interrupts to be kept enabled while the CRTC is
active.

In preparation for vblank interrupt fixes, make sure that the atomic
update event will be processed once only when the vblank interrupt will
be kept enabled.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index a0c26592fc69..8fef6558197b 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -43,6 +43,7 @@ struct omap_crtc {
 	bool enabled;
 	bool pending;
 	wait_queue_head_t pending_wait;
+	struct drm_pending_vblank_event *event;
 };
 
 /* -----------------------------------------------------------------------------
@@ -260,11 +261,15 @@ static const struct dss_mgr_ops mgr_ops = {
 
 static void omap_crtc_complete_page_flip(struct drm_crtc *crtc)
 {
+	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
 	struct drm_pending_vblank_event *event;
 	struct drm_device *dev = crtc->dev;
 	unsigned long flags;
 
-	event = crtc->state->event;
+	spin_lock_irqsave(&dev->event_lock, flags);
+	event = omap_crtc->event;
+	omap_crtc->event = NULL;
+	spin_unlock_irqrestore(&dev->event_lock, flags);
 
 	if (!event)
 		return;
@@ -384,12 +389,23 @@ static int omap_crtc_atomic_check(struct drm_crtc *crtc,
 }
 
 static void omap_crtc_atomic_begin(struct drm_crtc *crtc,
-                                  struct drm_crtc_state *old_crtc_state)
+				   struct drm_crtc_state *old_crtc_state)
 {
+	struct drm_pending_vblank_event *event = crtc->state->event;
+	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
+	unsigned long flags;
+
+	if (event) {
+		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
+
+		spin_lock_irqsave(&crtc->dev->event_lock, flags);
+		omap_crtc->event = event;
+		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
+	}
 }
 
 static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
-                                  struct drm_crtc_state *old_crtc_state)
+				   struct drm_crtc_state *old_crtc_state)
 {
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
 
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 13/20] drm: omapdrm: Use a spinlock to protect the CRTC pending flag

[Laurent Pinchart]

The flag will need to be accessed atomically in the vblank interrupt
handler, memory barriers won't be enough.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 8fef6558197b..ac0ec851865c 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -69,6 +69,19 @@ enum omap_channel omap_crtc_channel(struct drm_crtc *crtc)
 	return omap_crtc->channel;
 }
 
+static bool omap_crtc_is_pending(struct drm_crtc *crtc)
+{
+	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
+	unsigned long flags;
+	bool pending;
+
+	spin_lock_irqsave(&crtc->dev->event_lock, flags);
+	pending = omap_crtc->pending;
+	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
+
+	return pending;
+}
+
 int omap_crtc_wait_pending(struct drm_crtc *crtc)
 {
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
@@ -78,7 +91,7 @@ int omap_crtc_wait_pending(struct drm_crtc *crtc)
 	 * a single frame refresh even on slower displays.
 	 */
 	return wait_event_timeout(omap_crtc->pending_wait,
-				  !omap_crtc->pending,
+				  !omap_crtc_is_pending(crtc),
 				  msecs_to_jiffies(250));
 }
 
@@ -297,6 +310,7 @@ static void omap_crtc_vblank_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
 	struct omap_crtc *omap_crtc =
 			container_of(irq, struct omap_crtc, vblank_irq);
 	struct drm_device *dev = omap_crtc->base.dev;
+	struct drm_crtc *crtc = &omap_crtc->base;
 
 	if (dispc_mgr_go_busy(omap_crtc->channel))
 		return;
@@ -305,10 +319,10 @@ static void omap_crtc_vblank_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
 
 	__omap_irq_unregister(dev, &omap_crtc->vblank_irq);
 
-	rmb();
+	spin_lock(&crtc->dev->event_lock);
 	WARN_ON(!omap_crtc->pending);
 	omap_crtc->pending = false;
-	wmb();
+	spin_unlock(&crtc->dev->event_lock);
 
 	/* wake up userspace */
 	omap_crtc_complete_page_flip(&omap_crtc->base);
@@ -340,10 +354,10 @@ static void omap_crtc_enable(struct drm_crtc *crtc)
 
 	DBG("%s", omap_crtc->name);
 
-	rmb();
+	spin_lock_irq(&crtc->dev->event_lock);
 	WARN_ON(omap_crtc->pending);
 	omap_crtc->pending = true;
-	wmb();
+	spin_unlock_irq(&crtc->dev->event_lock);
 
 	omap_irq_register(crtc->dev, &omap_crtc->vblank_irq);
 
@@ -449,10 +463,10 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
 
 	DBG("%s: GO", omap_crtc->name);
 
-	rmb();
+	spin_lock_irq(&crtc->dev->event_lock);
 	WARN_ON(omap_crtc->pending);
 	omap_crtc->pending = true;
-	wmb();
+	spin_unlock_irq(&crtc->dev->event_lock);
 
 	dispc_mgr_go(omap_crtc->channel);
 	omap_irq_register(crtc->dev, &omap_crtc->vblank_irq);
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 14/20] drm: omapdrm: Keep vblank interrupt enabled while CRTC is active

[Laurent Pinchart]

Instead of going through a complicated private IRQ registration
mechanism, handle the vblank interrupt activation with the standard
drm_crtc_vblank_get() and drm_crtc_vblank_put() mechanism. This will let
the DRM core keep the vblank interrupt enabled as long as needed to
update the frame counter.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 38 ++++++++++++++-----------------------
 drivers/gpu/drm/omapdrm/omap_drv.h  |  1 +
 drivers/gpu/drm/omapdrm/omap_irq.c  |  4 +++-
 3 files changed, 18 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index ac0ec851865c..6983a889d811 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -36,8 +36,6 @@ struct omap_crtc {
 
 	struct omap_video_timings timings;
 
-	struct omap_drm_irq vblank_irq;
-
 	bool ignore_digit_sync_lost;
 
 	bool enabled;
@@ -305,25 +303,24 @@ void omap_crtc_error_irq(struct drm_crtc *crtc, uint32_t irqstatus)
 	DRM_ERROR_RATELIMITED("%s: errors: %08x\n", omap_crtc->name, irqstatus);
 }
 
-static void omap_crtc_vblank_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
+void omap_crtc_vblank_irq(struct drm_crtc *crtc)
 {
-	struct omap_crtc *omap_crtc =
-			container_of(irq, struct omap_crtc, vblank_irq);
-	struct drm_device *dev = omap_crtc->base.dev;
-	struct drm_crtc *crtc = &omap_crtc->base;
+	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
+	bool pending;
 
 	if (dispc_mgr_go_busy(omap_crtc->channel))
 		return;
 
 	DBG("%s: apply done", omap_crtc->name);
 
-	__omap_irq_unregister(dev, &omap_crtc->vblank_irq);
-
 	spin_lock(&crtc->dev->event_lock);
-	WARN_ON(!omap_crtc->pending);
+	pending = omap_crtc->pending;
 	omap_crtc->pending = false;
 	spin_unlock(&crtc->dev->event_lock);
 
+	if (pending)
+		drm_crtc_vblank_put(crtc);
+
 	/* wake up userspace */
 	omap_crtc_complete_page_flip(&omap_crtc->base);
 
@@ -341,8 +338,6 @@ static void omap_crtc_destroy(struct drm_crtc *crtc)
 
 	DBG("%s", omap_crtc->name);
 
-	WARN_ON(omap_crtc->vblank_irq.registered);
-
 	drm_crtc_cleanup(crtc);
 
 	kfree(omap_crtc);
@@ -354,14 +349,13 @@ static void omap_crtc_enable(struct drm_crtc *crtc)
 
 	DBG("%s", omap_crtc->name);
 
+	drm_crtc_vblank_on(crtc);
+	WARN_ON(drm_crtc_vblank_get(crtc) != 0);
+
 	spin_lock_irq(&crtc->dev->event_lock);
 	WARN_ON(omap_crtc->pending);
 	omap_crtc->pending = true;
 	spin_unlock_irq(&crtc->dev->event_lock);
-
-	omap_irq_register(crtc->dev, &omap_crtc->vblank_irq);
-
-	drm_crtc_vblank_on(crtc);
 }
 
 static void omap_crtc_disable(struct drm_crtc *crtc)
@@ -423,8 +417,6 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
 {
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
 
-	WARN_ON(omap_crtc->vblank_irq.registered);
-
 	if (crtc->state->color_mgmt_changed) {
 		struct drm_color_lut *lut = NULL;
 		uint length = 0;
@@ -463,13 +455,14 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
 
 	DBG("%s: GO", omap_crtc->name);
 
+	dispc_mgr_go(omap_crtc->channel);
+
+	WARN_ON(drm_crtc_vblank_get(crtc) != 0);
+
 	spin_lock_irq(&crtc->dev->event_lock);
 	WARN_ON(omap_crtc->pending);
 	omap_crtc->pending = true;
 	spin_unlock_irq(&crtc->dev->event_lock);
-
-	dispc_mgr_go(omap_crtc->channel);
-	omap_irq_register(crtc->dev, &omap_crtc->vblank_irq);
 }
 
 static bool omap_crtc_is_plane_prop(struct drm_device *dev,
@@ -593,9 +586,6 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 	omap_crtc->channel = channel;
 	omap_crtc->name = channel_names[channel];
 
-	omap_crtc->vblank_irq.irqmask = pipe2vbl(crtc);
-	omap_crtc->vblank_irq.irq = omap_crtc_vblank_irq;
-
 	ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL,
 					&omap_crtc_funcs, NULL);
 	if (ret < 0) {
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 05375bc40091..c549f943ebff 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -155,6 +155,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
 		struct drm_plane *plane, enum omap_channel channel, int id);
 int omap_crtc_wait_pending(struct drm_crtc *crtc);
 void omap_crtc_error_irq(struct drm_crtc *crtc, uint32_t irqstatus);
+void omap_crtc_vblank_irq(struct drm_crtc *crtc);
 
 struct drm_plane *omap_plane_init(struct drm_device *dev,
 		int id, enum drm_plane_type type);
diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
index 257c1f3c76f3..dd018d7a1e91 100644
--- a/drivers/gpu/drm/omapdrm/omap_irq.c
+++ b/drivers/gpu/drm/omapdrm/omap_irq.c
@@ -245,8 +245,10 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
 		struct drm_crtc *crtc = priv->crtcs[id];
 		enum omap_channel channel = omap_crtc_channel(crtc);
 
-		if (irqstatus & pipe2vbl(crtc))
+		if (irqstatus & pipe2vbl(crtc)) {
 			drm_handle_vblank(dev, id);
+			omap_crtc_vblank_irq(crtc);
+		}
 
 		if (irqstatus & dispc_mgr_get_sync_lost_irq(channel))
 			omap_crtc_error_irq(crtc, irqstatus);
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 15/20] drm: omapdrm: Don't expose the omap_irq_(un)register() functions

[Laurent Pinchart]

The IRQ registration functions are not used outside of their compilation
unit, make them static. As the __omap_irq_(un)register() functions are
only called by their omap_irq_(un)register() counterparts, merge them
together.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Split the omap_drm_irq irqstatus parameter removal change out
---
 drivers/gpu/drm/omapdrm/omap_drv.h |  4 ----
 drivers/gpu/drm/omapdrm/omap_irq.c | 23 +++++------------------
 2 files changed, 5 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index c549f943ebff..ce9bb54bf03e 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -127,10 +127,6 @@ int omap_gem_resume(struct device *dev);
 
 int omap_irq_enable_vblank(struct drm_device *dev, unsigned int pipe);
 void omap_irq_disable_vblank(struct drm_device *dev, unsigned int pipe);
-void __omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq);
-void __omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq);
-void omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq);
-void omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq);
 void omap_drm_irq_uninstall(struct drm_device *dev);
 int omap_drm_irq_install(struct drm_device *dev);
 
diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
index dd018d7a1e91..b324c45ce01e 100644
--- a/drivers/gpu/drm/omapdrm/omap_irq.c
+++ b/drivers/gpu/drm/omapdrm/omap_irq.c
@@ -39,11 +39,12 @@ static void omap_irq_update(struct drm_device *dev)
 	dispc_read_irqenable();        /* flush posted write */
 }
 
-void __omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq)
+static void omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq)
 {
 	struct omap_drm_private *priv = dev->dev_private;
 	unsigned long flags;
 
+	dispc_runtime_get();
 	spin_lock_irqsave(&list_lock, flags);
 
 	if (!WARN_ON(irq->registered)) {
@@ -53,21 +54,15 @@ void __omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq)
 	}
 
 	spin_unlock_irqrestore(&list_lock, flags);
-}
-
-void omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq)
-{
-	dispc_runtime_get();
-
-	__omap_irq_register(dev, irq);
-
 	dispc_runtime_put();
 }
 
-void __omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq)
+static void omap_irq_unregister(struct drm_device *dev,
+				struct omap_drm_irq *irq)
 {
 	unsigned long flags;
 
+	dispc_runtime_get();
 	spin_lock_irqsave(&list_lock, flags);
 
 	if (!WARN_ON(!irq->registered)) {
@@ -77,14 +72,6 @@ void __omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq)
 	}
 
 	spin_unlock_irqrestore(&list_lock, flags);
-}
-
-void omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq)
-{
-	dispc_runtime_get();
-
-	__omap_irq_unregister(dev, irq);
-
 	dispc_runtime_put();
 }
 
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 16/20] drm: omapdrm: Remove unused parameter from omap_drm_irq handler

[Laurent Pinchart]

The only omap_drm_irq handler doesn't use the irqstatus parameter passed
to the function. Remove it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- New patch
---
 drivers/gpu/drm/omapdrm/omap_drv.h | 2 +-
 drivers/gpu/drm/omapdrm/omap_irq.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index ce9bb54bf03e..8aa8e45098a2 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -58,7 +58,7 @@ struct omap_drm_irq {
 	struct list_head node;
 	uint32_t irqmask;
 	bool registered;
-	void (*irq)(struct omap_drm_irq *irq, uint32_t irqstatus);
+	void (*irq)(struct omap_drm_irq *irq);
 };
 
 /* For KMS code that needs to wait for a certain # of IRQs:
diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
index b324c45ce01e..df4a83a6f2df 100644
--- a/drivers/gpu/drm/omapdrm/omap_irq.c
+++ b/drivers/gpu/drm/omapdrm/omap_irq.c
@@ -82,7 +82,7 @@ struct omap_irq_wait {
 
 static DECLARE_WAIT_QUEUE_HEAD(wait_event);
 
-static void wait_irq(struct omap_drm_irq *irq, uint32_t irqstatus)
+static void wait_irq(struct omap_drm_irq *irq)
 {
 	struct omap_irq_wait *wait =
 			container_of(irq, struct omap_irq_wait, irq);
@@ -248,7 +248,7 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
 	list_for_each_entry_safe(handler, n, &priv->irq_list, node) {
 		if (handler->irqmask & irqstatus) {
 			spin_unlock_irqrestore(&list_lock, flags);
-			handler->irq(handler, handler->irqmask & irqstatus);
+			handler->irq(handler);
 			spin_lock_irqsave(&list_lock, flags);
 		}
 	}
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 17/20] drm: omapdrm: Don't call DISPC power handling in IRQ wait functions

[Laurent Pinchart]

The IRQ wait functions are called from the DSS enable and disable
operations only, where the DISPC is guaranteed to be enabled. There's no
need for manual DISPC power management there.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_irq.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
index df4a83a6f2df..27225492c81b 100644
--- a/drivers/gpu/drm/omapdrm/omap_irq.c
+++ b/drivers/gpu/drm/omapdrm/omap_irq.c
@@ -44,7 +44,6 @@ static void omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq)
 	struct omap_drm_private *priv = dev->dev_private;
 	unsigned long flags;
 
-	dispc_runtime_get();
 	spin_lock_irqsave(&list_lock, flags);
 
 	if (!WARN_ON(irq->registered)) {
@@ -54,7 +53,6 @@ static void omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq)
 	}
 
 	spin_unlock_irqrestore(&list_lock, flags);
-	dispc_runtime_put();
 }
 
 static void omap_irq_unregister(struct drm_device *dev,
@@ -62,7 +60,6 @@ static void omap_irq_unregister(struct drm_device *dev,
 {
 	unsigned long flags;
 
-	dispc_runtime_get();
 	spin_lock_irqsave(&list_lock, flags);
 
 	if (!WARN_ON(!irq->registered)) {
@@ -72,7 +69,6 @@ static void omap_irq_unregister(struct drm_device *dev,
 	}
 
 	spin_unlock_irqrestore(&list_lock, flags);
-	dispc_runtime_put();
 }
 
 struct omap_irq_wait {
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 18/20] drm: omapdrm: Make pipe2vbl function static

[Laurent Pinchart]

The function is only used in omap_irq.c, move it there and make it
static.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 7 -------
 drivers/gpu/drm/omapdrm/omap_drv.h  | 1 -
 drivers/gpu/drm/omapdrm/omap_irq.c  | 7 ++++++-
 3 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 6983a889d811..fb01357721fe 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -48,13 +48,6 @@ struct omap_crtc {
  * Helper Functions
  */
 
-uint32_t pipe2vbl(struct drm_crtc *crtc)
-{
-	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
-
-	return dispc_mgr_get_vsync_irq(omap_crtc->channel);
-}
-
 struct omap_video_timings *omap_crtc_timings(struct drm_crtc *crtc)
 {
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 8aa8e45098a2..4fee7b94eefb 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -234,7 +234,6 @@ struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
 		struct dma_buf *buffer);
 
 /* map crtc to vblank mask */
-uint32_t pipe2vbl(struct drm_crtc *crtc);
 struct omap_dss_device *omap_encoder_get_dssdev(struct drm_encoder *encoder);
 
 #endif /* __OMAP_DRV_H__ */
diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
index 27225492c81b..28ff7b73a853 100644
--- a/drivers/gpu/drm/omapdrm/omap_irq.c
+++ b/drivers/gpu/drm/omapdrm/omap_irq.c
@@ -108,6 +108,11 @@ int omap_irq_wait(struct drm_device *dev, struct omap_irq_wait *wait,
 	return 0;
 }
 
+static uint32_t pipe2vbl(struct drm_crtc *crtc)
+{
+	return dispc_mgr_get_vsync_irq(omap_crtc_channel(crtc));
+}
+
 /**
  * enable_vblank - enable vblank interrupt events
  * @dev: DRM device
@@ -228,7 +233,7 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
 		struct drm_crtc *crtc = priv->crtcs[id];
 		enum omap_channel channel = omap_crtc_channel(crtc);
 
-		if (irqstatus & pipe2vbl(crtc)) {
+		if (irqstatus & dispc_mgr_get_vsync_irq(channel)) {
 			drm_handle_vblank(dev, id);
 			omap_crtc_vblank_irq(crtc);
 		}
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 19/20] drm: omapdrm: Simplify IRQ wait implementation

[Laurent Pinchart]

Now that the IRQ list is used for IRQ wait only we can merge
omap_drm_irq and omap_irq_wait and simplify the implementation.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.h | 17 +------
 drivers/gpu/drm/omapdrm/omap_irq.c | 94 ++++++++++++++------------------------
 2 files changed, 37 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 4fee7b94eefb..9baae072fe88 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -48,19 +48,6 @@ struct omap_drm_window {
 	uint32_t src_w, src_h;
 };
 
-/* For transiently registering for different DSS irqs that various parts
- * of the KMS code need during setup/configuration.  We these are not
- * necessarily the same as what drm_vblank_get/put() are requesting, and
- * the hysteresis in drm_vblank_put() is not necessarily desirable for
- * internal housekeeping related irq usage.
- */
-struct omap_drm_irq {
-	struct list_head node;
-	uint32_t irqmask;
-	bool registered;
-	void (*irq)(struct omap_drm_irq *irq);
-};
-
 /* For KMS code that needs to wait for a certain # of IRQs:
  */
 struct omap_irq_wait;
@@ -101,8 +88,8 @@ struct omap_drm_private {
 	struct drm_property *zorder_prop;
 
 	/* irq handling: */
-	struct list_head irq_list;    /* list of omap_drm_irq */
-	uint32_t irq_mask;		/* enabled irqs in addition to irq_list */
+	struct list_head wait_list;     /* list of omap_irq_wait */
+	uint32_t irq_mask;		/* enabled irqs in addition to wait_list */
 
 	/* atomic commit */
 	struct {
diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
index 28ff7b73a853..c93e28be8ff7 100644
--- a/drivers/gpu/drm/omapdrm/omap_irq.c
+++ b/drivers/gpu/drm/omapdrm/omap_irq.c
@@ -21,17 +21,23 @@
 
 static DEFINE_SPINLOCK(list_lock);
 
+struct omap_irq_wait {
+	struct list_head node;
+	uint32_t irqmask;
+	int count;
+};
+
 /* call with list_lock and dispc runtime held */
 static void omap_irq_update(struct drm_device *dev)
 {
 	struct omap_drm_private *priv = dev->dev_private;
-	struct omap_drm_irq *irq;
+	struct omap_irq_wait *wait;
 	uint32_t irqmask = priv->irq_mask;
 
 	assert_spin_locked(&list_lock);
 
-	list_for_each_entry(irq, &priv->irq_list, node)
-		irqmask |= irq->irqmask;
+	list_for_each_entry(wait, &priv->wait_list, node)
+		irqmask |= wait->irqmask;
 
 	DBG("irqmask=%08x", irqmask);
 
@@ -39,61 +45,29 @@ static void omap_irq_update(struct drm_device *dev)
 	dispc_read_irqenable();        /* flush posted write */
 }
 
-static void omap_irq_register(struct drm_device *dev, struct omap_drm_irq *irq)
-{
-	struct omap_drm_private *priv = dev->dev_private;
-	unsigned long flags;
-
-	spin_lock_irqsave(&list_lock, flags);
-
-	if (!WARN_ON(irq->registered)) {
-		irq->registered = true;
-		list_add(&irq->node, &priv->irq_list);
-		omap_irq_update(dev);
-	}
-
-	spin_unlock_irqrestore(&list_lock, flags);
-}
-
-static void omap_irq_unregister(struct drm_device *dev,
-				struct omap_drm_irq *irq)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&list_lock, flags);
-
-	if (!WARN_ON(!irq->registered)) {
-		irq->registered = false;
-		list_del(&irq->node);
-		omap_irq_update(dev);
-	}
-
-	spin_unlock_irqrestore(&list_lock, flags);
-}
-
-struct omap_irq_wait {
-	struct omap_drm_irq irq;
-	int count;
-};
-
 static DECLARE_WAIT_QUEUE_HEAD(wait_event);
 
-static void wait_irq(struct omap_drm_irq *irq)
+static void omap_irq_wait_irq(struct omap_irq_wait *wait)
 {
-	struct omap_irq_wait *wait =
-			container_of(irq, struct omap_irq_wait, irq);
 	wait->count--;
-	wake_up_all(&wait_event);
+	wake_up(&wait_event);
 }
 
 struct omap_irq_wait * omap_irq_wait_init(struct drm_device *dev,
 		uint32_t irqmask, int count)
 {
+	struct omap_drm_private *priv = dev->dev_private;
 	struct omap_irq_wait *wait = kzalloc(sizeof(*wait), GFP_KERNEL);
-	wait->irq.irq = wait_irq;
-	wait->irq.irqmask = irqmask;
+	unsigned long flags;
+
+	wait->irqmask = irqmask;
 	wait->count = count;
-	omap_irq_register(dev, &wait->irq);
+
+	spin_lock_irqsave(&list_lock, flags);
+	list_add(&wait->node, &priv->wait_list);
+	omap_irq_update(dev);
+	spin_unlock_irqrestore(&list_lock, flags);
+
 	return wait;
 }
 
@@ -101,11 +75,16 @@ int omap_irq_wait(struct drm_device *dev, struct omap_irq_wait *wait,
 		unsigned long timeout)
 {
 	int ret = wait_event_timeout(wait_event, (wait->count <= 0), timeout);
-	omap_irq_unregister(dev, &wait->irq);
+	unsigned long flags;
+
+	spin_lock_irqsave(&list_lock, flags);
+	list_del(&wait->node);
+	omap_irq_update(dev);
+	spin_unlock_irqrestore(&list_lock, flags);
+
 	kfree(wait);
-	if (ret == 0)
-		return -1;
-	return 0;
+
+	return ret == 0 ? -1 : 0;
 }
 
 static uint32_t pipe2vbl(struct drm_crtc *crtc)
@@ -218,7 +197,7 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
 {
 	struct drm_device *dev = (struct drm_device *) arg;
 	struct omap_drm_private *priv = dev->dev_private;
-	struct omap_drm_irq *handler, *n;
+	struct omap_irq_wait *wait, *n;
 	unsigned long flags;
 	unsigned int id;
 	u32 irqstatus;
@@ -246,12 +225,9 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
 	omap_irq_fifo_underflow(priv, irqstatus);
 
 	spin_lock_irqsave(&list_lock, flags);
-	list_for_each_entry_safe(handler, n, &priv->irq_list, node) {
-		if (handler->irqmask & irqstatus) {
-			spin_unlock_irqrestore(&list_lock, flags);
-			handler->irq(handler);
-			spin_lock_irqsave(&list_lock, flags);
-		}
+	list_for_each_entry_safe(wait, n, &priv->wait_list, node) {
+		if (wait->irqmask & irqstatus)
+			omap_irq_wait_irq(wait);
 	}
 	spin_unlock_irqrestore(&list_lock, flags);
 
@@ -280,7 +256,7 @@ int omap_drm_irq_install(struct drm_device *dev)
 	unsigned int i;
 	int ret;
 
-	INIT_LIST_HEAD(&priv->irq_list);
+	INIT_LIST_HEAD(&priv->wait_list);
 
 	priv->irq_mask = DISPC_IRQ_OCP_ERR;
 
-- 
Regards,

Laurent Pinchart

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

[PATCH v3 20/20] drm: omapdrm: Remove global variables

[Laurent Pinchart]

Move the list of pending IRQ wait instances to the omap_drm_private
structure and the wait queue head to the IRQ wait structure.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.h |  3 ++-
 drivers/gpu/drm/omapdrm/omap_irq.c | 42 ++++++++++++++++++++------------------
 2 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 9baae072fe88..5f278a74f4e9 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -88,7 +88,8 @@ struct omap_drm_private {
 	struct drm_property *zorder_prop;
 
 	/* irq handling: */
-	struct list_head wait_list;     /* list of omap_irq_wait */
+	spinlock_t wait_lock;		/* protects the wait_list */
+	struct list_head wait_list;	/* list of omap_irq_wait */
 	uint32_t irq_mask;		/* enabled irqs in addition to wait_list */
 
 	/* atomic commit */
diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
index c93e28be8ff7..4bde4fc16a66 100644
--- a/drivers/gpu/drm/omapdrm/omap_irq.c
+++ b/drivers/gpu/drm/omapdrm/omap_irq.c
@@ -19,22 +19,21 @@
 
 #include "omap_drv.h"
 
-static DEFINE_SPINLOCK(list_lock);
-
 struct omap_irq_wait {
 	struct list_head node;
+	wait_queue_head_t wq;
 	uint32_t irqmask;
 	int count;
 };
 
-/* call with list_lock and dispc runtime held */
+/* call with wait_lock and dispc runtime held */
 static void omap_irq_update(struct drm_device *dev)
 {
 	struct omap_drm_private *priv = dev->dev_private;
 	struct omap_irq_wait *wait;
 	uint32_t irqmask = priv->irq_mask;
 
-	assert_spin_locked(&list_lock);
+	assert_spin_locked(&priv->wait_lock);
 
 	list_for_each_entry(wait, &priv->wait_list, node)
 		irqmask |= wait->irqmask;
@@ -45,12 +44,10 @@ static void omap_irq_update(struct drm_device *dev)
 	dispc_read_irqenable();        /* flush posted write */
 }
 
-static DECLARE_WAIT_QUEUE_HEAD(wait_event);
-
 static void omap_irq_wait_irq(struct omap_irq_wait *wait)
 {
 	wait->count--;
-	wake_up(&wait_event);
+	wake_up(&wait->wq);
 }
 
 struct omap_irq_wait * omap_irq_wait_init(struct drm_device *dev,
@@ -60,13 +57,14 @@ struct omap_irq_wait * omap_irq_wait_init(struct drm_device *dev,
 	struct omap_irq_wait *wait = kzalloc(sizeof(*wait), GFP_KERNEL);
 	unsigned long flags;
 
+	init_waitqueue_head(&wait->wq);
 	wait->irqmask = irqmask;
 	wait->count = count;
 
-	spin_lock_irqsave(&list_lock, flags);
+	spin_lock_irqsave(&priv->wait_lock, flags);
 	list_add(&wait->node, &priv->wait_list);
 	omap_irq_update(dev);
-	spin_unlock_irqrestore(&list_lock, flags);
+	spin_unlock_irqrestore(&priv->wait_lock, flags);
 
 	return wait;
 }
@@ -74,13 +72,16 @@ struct omap_irq_wait * omap_irq_wait_init(struct drm_device *dev,
 int omap_irq_wait(struct drm_device *dev, struct omap_irq_wait *wait,
 		unsigned long timeout)
 {
-	int ret = wait_event_timeout(wait_event, (wait->count <= 0), timeout);
+	struct omap_drm_private *priv = dev->dev_private;
 	unsigned long flags;
+	int ret;
+
+	ret = wait_event_timeout(wait->wq, (wait->count <= 0), timeout);
 
-	spin_lock_irqsave(&list_lock, flags);
+	spin_lock_irqsave(&priv->wait_lock, flags);
 	list_del(&wait->node);
 	omap_irq_update(dev);
-	spin_unlock_irqrestore(&list_lock, flags);
+	spin_unlock_irqrestore(&priv->wait_lock, flags);
 
 	kfree(wait);
 
@@ -113,10 +114,10 @@ int omap_irq_enable_vblank(struct drm_device *dev, unsigned int pipe)
 
 	DBG("dev=%p, crtc=%u", dev, pipe);
 
-	spin_lock_irqsave(&list_lock, flags);
+	spin_lock_irqsave(&priv->wait_lock, flags);
 	priv->irq_mask |= pipe2vbl(crtc);
 	omap_irq_update(dev);
-	spin_unlock_irqrestore(&list_lock, flags);
+	spin_unlock_irqrestore(&priv->wait_lock, flags);
 
 	return 0;
 }
@@ -138,10 +139,10 @@ void omap_irq_disable_vblank(struct drm_device *dev, unsigned int pipe)
 
 	DBG("dev=%p, crtc=%u", dev, pipe);
 
-	spin_lock_irqsave(&list_lock, flags);
+	spin_lock_irqsave(&priv->wait_lock, flags);
 	priv->irq_mask &= ~pipe2vbl(crtc);
 	omap_irq_update(dev);
-	spin_unlock_irqrestore(&list_lock, flags);
+	spin_unlock_irqrestore(&priv->wait_lock, flags);
 }
 
 static void omap_irq_fifo_underflow(struct omap_drm_private *priv,
@@ -165,9 +166,9 @@ static void omap_irq_fifo_underflow(struct omap_drm_private *priv,
 		       | DISPC_IRQ_VID3_FIFO_UNDERFLOW;
 	unsigned int i;
 
-	spin_lock(&list_lock);
+	spin_lock(&priv->wait_lock);
 	irqstatus &= priv->irq_mask & mask;
-	spin_unlock(&list_lock);
+	spin_unlock(&priv->wait_lock);
 
 	if (!irqstatus)
 		return;
@@ -224,12 +225,12 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
 	omap_irq_ocp_error_handler(irqstatus);
 	omap_irq_fifo_underflow(priv, irqstatus);
 
-	spin_lock_irqsave(&list_lock, flags);
+	spin_lock_irqsave(&priv->wait_lock, flags);
 	list_for_each_entry_safe(wait, n, &priv->wait_list, node) {
 		if (wait->irqmask & irqstatus)
 			omap_irq_wait_irq(wait);
 	}
-	spin_unlock_irqrestore(&list_lock, flags);
+	spin_unlock_irqrestore(&priv->wait_lock, flags);
 
 	return IRQ_HANDLED;
 }
@@ -256,6 +257,7 @@ int omap_drm_irq_install(struct drm_device *dev)
 	unsigned int i;
 	int ret;
 
+	spin_lock_init(&priv->wait_lock);
 	INIT_LIST_HEAD(&priv->wait_list);
 
 	priv->irq_mask = DISPC_IRQ_OCP_ERR;
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 02/20] drm: omapdrm: fb: Use format information provided by the DRM core

[Tomi Valkeinen]

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

On 19/09/16 15:27, Laurent Pinchart wrote:
> The driver stores in a custom structure named format several pieces of
> information about the format that are available in the DRM core. Remove
> them and get the information from the DRM core instead.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---


> @@ -128,13 +122,13 @@ static const struct drm_framebuffer_funcs omap_framebuffer_funcs = {
>  };
>  
>  static uint32_t get_linear_addr(struct plane *plane,
> -		const struct format *format, int n, int x, int y)
> +		const struct drm_format_info *format, int n, int x, int y)
>  {
>  	uint32_t offset;
>  
> -	offset = plane->offset +
> -			(x * format->planes[n].stride_bpp) +
> -			(y * plane->pitch / format->planes[n].sub_y);
> +	offset = plane->offset
> +	       + (x * format->cpp[n] / (n == 1 ? 1 : format->hsub))
> +	       + (y * plane->pitch / (n == 1 ? 1 : format->vsub));

n is the plane number? Shouldn't the above be (n == 0 ? 1 : format->hsub)?

> @@ -413,28 +410,32 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>  
>  	fb = &omap_fb->base;
>  	omap_fb->format = format;
> +	omap_fb->dss_format = dss_format;
>  	mutex_init(&omap_fb->lock);
>  
> -	for (i = 0; i < n; i++) {
> +	for (i = 0; i < format->num_planes; i++) {
>  		struct plane *plane = &omap_fb->planes[i];
> -		int size, pitch = mode_cmd->pitches[i];
> +		unsigned int pitch = mode_cmd->pitches[i];
> +		unsigned int hsub = i == 0 ? 1 : format->hsub;
> +		unsigned int vsub = i == 0 ? 1 : format->vsub;

This makes me wonder... Will all drivers do something like the above?
It's a bit laborious way to get the pixel subsampling factor, and I
presume something like above is quite common so that all the
calculations can be more generic (and not specific to a UV plane).

 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 v3 04/20] drm: omapdrm: fb: Simplify mode command checks when creating framebuffer

[Tomi Valkeinen]

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

On 19/09/16 15:27, Laurent Pinchart wrote:
> The hardware requires all planes to have an identical pitch in number of
> pixels. Given that all supported formats use the same number of bytes
> per pixel in all planes, framebuffer creation checks can be simplified.
> The implementations assumes that no format use more than two planes
> which is true with the existing hardware.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Clarify commit message and mention explicitly in the code that only
>   one and two planes formats are supported.
> - Rebase on top of the switch to drm_format_info()
> ---
>  drivers/gpu/drm/omapdrm/omap_fb.c | 58 +++++++++++++++++++++------------------
>  1 file changed, 31 insertions(+), 27 deletions(-)

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 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 v3 06/20] drm: omapdrm: Handle FIFO underflow IRQs internally

[Tomi Valkeinen]

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

On 19/09/16 15:27, Laurent Pinchart wrote:
> As the FIFO underflow IRQ handler just prints an error message to the
> kernel log, simplify the code by not registering one IRQ handler per
> plane but print the messages directly from the main IRQ handler.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Only register error IRQs that exist on the HW
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c   |  4 +--
>  drivers/gpu/drm/omapdrm/omap_drv.h   |  2 +-
>  drivers/gpu/drm/omapdrm/omap_irq.c   | 66 ++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/omapdrm/omap_plane.c | 24 -------------
>  4 files changed, 66 insertions(+), 30 deletions(-)

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 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 v3 06/20] drm: omapdrm: Handle FIFO underflow IRQs internally

[Tomi Valkeinen]

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

On 19/09/16 15:27, Laurent Pinchart wrote:
> As the FIFO underflow IRQ handler just prints an error message to the
> kernel log, simplify the code by not registering one IRQ handler per
> plane but print the messages directly from the main IRQ handler.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Only register error IRQs that exist on the HW
> ---


> +static const u32 error_irqs[] = {
> +	[OMAP_DSS_GFX] = DISPC_IRQ_GFX_FIFO_UNDERFLOW,
> +	[OMAP_DSS_VIDEO1] = DISPC_IRQ_VID1_FIFO_UNDERFLOW,
> +	[OMAP_DSS_VIDEO2] = DISPC_IRQ_VID2_FIFO_UNDERFLOW,
> +	[OMAP_DSS_VIDEO3] = DISPC_IRQ_VID3_FIFO_UNDERFLOW,
> +};

Actually, one comment: "error_irqs" is perhaps not a good name for this.
"omap_underflow_irqs"?

Or actually, maybe it would make sense to move this into dispc, and have
a similar func to dispc_mgr_get_sync_lost_irq().

 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 v3 08/20] drm: omapdrm: Handle OCP error IRQ directly

[Tomi Valkeinen]

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

On 19/09/16 15:27, Laurent Pinchart wrote:
> Instead of going through a complicated registration mechanism, just
> call the OCP error IRQ handler directly from the main IRQ handler.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
> 
> - Rename IRQ handler to omap_irq_ocp_error_handler()
> - Replace hex error value with "OCP error" message
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.h |  1 -
>  drivers/gpu/drm/omapdrm/omap_irq.c | 28 ++++++++++------------------
>  2 files changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
> index 7a1d8384147c..05375bc40091 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -103,7 +103,6 @@ struct omap_drm_private {
>  	/* irq handling: */
>  	struct list_head irq_list;    /* list of omap_drm_irq */
>  	uint32_t irq_mask;		/* enabled irqs in addition to irq_list */
> -	struct omap_drm_irq error_handler;
>  
>  	/* atomic commit */
>  	struct {
> diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
> index f0f110faf1e6..257c1f3c76f3 100644
> --- a/drivers/gpu/drm/omapdrm/omap_irq.c
> +++ b/drivers/gpu/drm/omapdrm/omap_irq.c
> @@ -21,12 +21,6 @@
>  
>  static DEFINE_SPINLOCK(list_lock);
>  
> -static void omap_irq_error_handler(struct omap_drm_irq *irq,
> -		uint32_t irqstatus)
> -{
> -	DRM_ERROR("errors: %08x\n", irqstatus);
> -}
> -
>  /* call with list_lock and dispc runtime held */
>  static void omap_irq_update(struct drm_device *dev)
>  {
> @@ -224,6 +218,14 @@ static void omap_irq_fifo_underflow(struct omap_drm_private *priv,
>  	pr_cont("(0x%08x)\n", irqstatus);
>  }
>  
> +static void omap_irq_ocp_error_handler(u32 irqstatus)
> +{
> +	if (!(irqstatus & DISPC_IRQ_OCP_ERR))
> +		return;
> +
> +	DRM_ERROR("OCP error\n");
> +}
> +
>  static irqreturn_t omap_irq_handler(int irq, void *arg)
>  {
>  	struct drm_device *dev = (struct drm_device *) arg;
> @@ -250,6 +252,7 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
>  			omap_crtc_error_irq(crtc, irqstatus);
>  	}
>  
> +	omap_irq_ocp_error_handler(irqstatus);
>  	omap_irq_fifo_underflow(priv, irqstatus);
>  
>  	spin_lock_irqsave(&list_lock, flags);
> @@ -282,7 +285,6 @@ static const u32 error_irqs[] = {
>  int omap_drm_irq_install(struct drm_device *dev)
>  {
>  	struct omap_drm_private *priv = dev->dev_private;
> -	struct omap_drm_irq *error_handler = &priv->error_handler;
>  	unsigned int num_mgrs = dss_feat_get_num_mgrs();
>  	unsigned int max_planes;
>  	unsigned int i;
> @@ -290,7 +292,7 @@ int omap_drm_irq_install(struct drm_device *dev)
>  
>  	INIT_LIST_HEAD(&priv->irq_list);
>  
> -	priv->irq_mask = 0;
> +	priv->irq_mask = DISPC_IRQ_OCP_ERR;
>  
>  	max_planes = min(ARRAY_SIZE(priv->planes), ARRAY_SIZE(error_irqs));
>  	for (i = 0; i < max_planes; ++i) {
> @@ -309,16 +311,6 @@ int omap_drm_irq_install(struct drm_device *dev)
>  	if (ret < 0)
>  		return ret;
>  
> -	error_handler->irq = omap_irq_error_handler;
> -	error_handler->irqmask = DISPC_IRQ_OCP_ERR;
> -
> -	/* for now ignore DISPC_IRQ_SYNC_LOST_DIGIT.. really I think
> -	 * we just need to ignore it while enabling tv-out
> -	 */
> -	error_handler->irqmask &= ~DISPC_IRQ_SYNC_LOST_DIGIT;
> -
> -	omap_irq_register(dev, error_handler);
> -
>  	dev->irq_enabled = true;
>  
>  	return 0;
> 

The removal of DISPC_IRQ_SYNC_LOST_DIGIT code above feels a bit out of
place for this patch.

 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 v3 09/20] drm: omapdrm: Replace DSS manager state check with omapdrm CRTC state

[Tomi Valkeinen]

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

On 19/09/16 15:27, Laurent Pinchart wrote:
> Instead of conditioning planes update based on the DD manager state, use

s/DD/DSS/

Maybe "manager HW state" to highlight that the status is read from a HW
register.

> the enabled field newly added to the omap_crtc structure. This reduces
> the dependency from the DRM layer to the DSS layer.
> 
> The enabled field is a transitory measure, the implementation should use
> the CRTC atomic state instead. However, given that CRTCs are currently
> not enabled/disabled through their .enable() and .disable() operations
> but through a convoluted code paths starting at the associated encoder
> operations, there is not clear guarantee that the atomic state always
> matches the hardware state. This will be refactored later, at which
> point the enabled field will be removed.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v2:
> 
> - Use enabled field in struct omap_crtc instead of CRTC atomic state
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index cdcfda31043e..f41a638c8d65 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -40,6 +40,7 @@ struct omap_crtc {
>  
>  	bool ignore_digit_sync_lost;
>  
> +	bool enabled;
>  	bool pending;
>  	wait_queue_head_t pending_wait;
>  };
> @@ -136,6 +137,7 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable)
>  
>  	if (omap_crtc_output[channel]->output_type == OMAP_DISPLAY_TYPE_HDMI) {
>  		dispc_mgr_enable(channel, enable);
> +		omap_crtc->enabled = enable;
>  		return;
>  	}
>  
> @@ -172,6 +174,7 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable)
>  	}
>  
>  	dispc_mgr_enable(channel, enable);
> +	omap_crtc->enabled = enable;

omap_crtc_set_enabled() is only called from omap_crtc_dss_enable() and
omap_crtc_dss_disable(). It's probably a bit cleaner to set the
omap_crtc->enabled in those functions.

Btw, omap_crtc_set_enabled() has a comment:

/* Called only from the encoder enable/disable and suspend/resume
handlers. */

which doesn't seem to hold true...

 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 v3 10/20] drm: omapdrm: Only commit planes on active CRTCs

[Tomi Valkeinen]

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

On 19/09/16 15:27, Laurent Pinchart wrote:
> The DRM core supports skipping plane update for inactive CRTCs for
> hardware that don't need it or can't cope with it. That's our case, so
> use the DRM core infrastructure instead of reinventing it.

I don't follow this desc. What is omapdrm reinventing? At least this
patch does not remove any of the "reinvention".

What does DRM_PLANE_COMMIT_ACTIVE_ONLY do? Skips plane HW configuration
for planes on crtcs that are disabled? The plane HW config will still be
done when the crtc is about to be enabled, right?

 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 v3 11/20] drm: omapdrm: Check DSS manager state in the enable/disable helpers

[Tomi Valkeinen]

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

On 19/09/16 15:27, Laurent Pinchart wrote:
> The omapdrm DSS manager enable/disable operations check the DSS manager
> state to avoid double enabling/disabling. Move that code to the DSS
> manager to decrease the dependency of the DRM layer to the DSS layer.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dispc.c  | 1 -
>  drivers/gpu/drm/omapdrm/dss/output.c | 6 ++++++
>  drivers/gpu/drm/omapdrm/omap_crtc.c  | 3 ---
>  3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c
> index 535240fba671..ab150bf21dd8 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> @@ -2911,7 +2911,6 @@ bool dispc_mgr_is_enabled(enum omap_channel channel)
>  {
>  	return !!mgr_fld_read(channel, DISPC_MGR_FLD_ENABLE);
>  }
> -EXPORT_SYMBOL(dispc_mgr_is_enabled);
>  
>  void dispc_wb_enable(bool enable)
>  {
> diff --git a/drivers/gpu/drm/omapdrm/dss/output.c b/drivers/gpu/drm/omapdrm/dss/output.c
> index 24f859488201..f0be621895fa 100644
> --- a/drivers/gpu/drm/omapdrm/dss/output.c
> +++ b/drivers/gpu/drm/omapdrm/dss/output.c
> @@ -217,12 +217,18 @@ EXPORT_SYMBOL(dss_mgr_set_lcd_config);
>  
>  int dss_mgr_enable(enum omap_channel channel)
>  {
> +	if (dispc_mgr_is_enabled(channel))
> +		return 0;
> +
>  	return dss_mgr_ops->enable(channel);
>  }
>  EXPORT_SYMBOL(dss_mgr_enable);
>  
>  void dss_mgr_disable(enum omap_channel channel)
>  {
> +	if (!dispc_mgr_is_enabled(channel))
> +		return;
> +
>  	dss_mgr_ops->disable(channel);
>  }
>  EXPORT_SYMBOL(dss_mgr_disable);
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 4b7e16786e1e..a0c26592fc69 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -141,9 +141,6 @@ static void omap_crtc_set_enabled(struct drm_crtc *crtc, bool enable)
>  		return;
>  	}
>  
> -	if (dispc_mgr_is_enabled(channel) == enable)
> -		return;
> -
>  	if (omap_crtc->channel == OMAP_DSS_CHANNEL_DIGIT) {
>  		/*
>  		 * Digit output produces some sync lost interrupts during the
> 

With this change omap_crtc_set_enabled() will do extra work if the
output is already enabled/disabled, and, if I'm not mistaken, will do
omap_irq_wait() there which might lead to issues.

If you remove the check, then I think the driver should make sure that
omap_crtc_set_enabled() is not called if the output is already
enabled/disabled. Maybe that can be done in
omap_crtc_dss_enable/disable, using the new enabled flag.

 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 v3 12/20] drm: omapdrm: Prevent processing the same event multiple times

[Tomi Valkeinen]

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

On 19/09/16 15:27, Laurent Pinchart wrote:
> The vblank interrupt is disabled after one occurrence, preventing the
> atomic update event from being processed twice. However, this also
> prevents the software frame counter from being updated correctly that
> would require vblank interrupts to be kept enabled while the CRTC is
> active.
> 
> In preparation for vblank interrupt fixes, make sure that the atomic
> update event will be processed once only when the vblank interrupt will
> be kept enabled.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index a0c26592fc69..8fef6558197b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -43,6 +43,7 @@ struct omap_crtc {
>  	bool enabled;
>  	bool pending;
>  	wait_queue_head_t pending_wait;
> +	struct drm_pending_vblank_event *event;
>  };
>  
>  /* -----------------------------------------------------------------------------
> @@ -260,11 +261,15 @@ static const struct dss_mgr_ops mgr_ops = {
>  
>  static void omap_crtc_complete_page_flip(struct drm_crtc *crtc)
>  {
> +	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>  	struct drm_pending_vblank_event *event;
>  	struct drm_device *dev = crtc->dev;
>  	unsigned long flags;
>  
> -	event = crtc->state->event;
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	event = omap_crtc->event;
> +	omap_crtc->event = NULL;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
>  
>  	if (!event)
>  		return;
> @@ -384,12 +389,23 @@ static int omap_crtc_atomic_check(struct drm_crtc *crtc,
>  }
>  
>  static void omap_crtc_atomic_begin(struct drm_crtc *crtc,
> -                                  struct drm_crtc_state *old_crtc_state)
> +				   struct drm_crtc_state *old_crtc_state)
>  {
> +	struct drm_pending_vblank_event *event = crtc->state->event;
> +	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> +	unsigned long flags;
> +
> +	if (event) {
> +		WARN_ON(drm_crtc_vblank_get(crtc) != 0);

Where's the matching put?

 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 v3 13/20] drm: omapdrm: Use a spinlock to protect the CRTC pending flag

[Tomi Valkeinen]

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

On 19/09/16 15:27, Laurent Pinchart wrote:
> The flag will need to be accessed atomically in the vblank interrupt
> handler, memory barriers won't be enough.
> 

Please write the descriptions so that they don't depend on the subject.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 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 v3 12/20] drm: omapdrm: Prevent processing the same event multiple times

[Daniel Kurtz]

Hi Laurent,

On Mon, Sep 19, 2016 at 8:27 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> The vblank interrupt is disabled after one occurrence, preventing the
> atomic update event from being processed twice. However, this also
> prevents the software frame counter from being updated correctly that
> would require vblank interrupts to be kept enabled while the CRTC is
> active.
>
> In preparation for vblank interrupt fixes, make sure that the atomic
> update event will be processed once only when the vblank interrupt will
> be kept enabled.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index a0c26592fc69..8fef6558197b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -43,6 +43,7 @@ struct omap_crtc {
>         bool enabled;
>         bool pending;
>         wait_queue_head_t pending_wait;
> +       struct drm_pending_vblank_event *event;
>  };
>
>  /* -----------------------------------------------------------------------------
> @@ -260,11 +261,15 @@ static const struct dss_mgr_ops mgr_ops = {
>
>  static void omap_crtc_complete_page_flip(struct drm_crtc *crtc)
>  {
> +       struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>         struct drm_pending_vblank_event *event;
>         struct drm_device *dev = crtc->dev;
>         unsigned long flags;
>
> -       event = crtc->state->event;
> +       spin_lock_irqsave(&dev->event_lock, flags);
> +       event = omap_crtc->event;
> +       omap_crtc->event = NULL;
> +       spin_unlock_irqrestore(&dev->event_lock, flags);

Perhaps just hold the lock until after drm_crtc_send_vblank_event(crtc, event)?

>
>         if (!event)
>                 return;
> @@ -384,12 +389,23 @@ static int omap_crtc_atomic_check(struct drm_crtc *crtc,
>  }
>
>  static void omap_crtc_atomic_begin(struct drm_crtc *crtc,
> -                                  struct drm_crtc_state *old_crtc_state)
> +                                  struct drm_crtc_state *old_crtc_state)
>  {
> +       struct drm_pending_vblank_event *event = crtc->state->event;
> +       struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> +       unsigned long flags;
> +
> +       if (event) {
> +               WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> +
> +               spin_lock_irqsave(&crtc->dev->event_lock, flags);
> +               omap_crtc->event = event;
> +               spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> +       }

AFAICT, the only reason to use the spin_lock_irqsave here is if there
is a possibility that the vblank irq can run in parallel to
atomic_begin().

If this is the case, then setting omap_crtc->event here in
atomic_begin() may not work exactly as you wish.
It sets up a race: the vblank irq may call
omap_crtc_complete_page_flip and send the event between here and when
the "GO" bit is set in atomic_flush(), and thus, it may send the event
too early, on the wrong vblank.

By the way, it looks like there is the same race, but just much
shorter, with the "omap_crtc->pending" flag.  Since it is set in
atomic_flush() before hitting the go bit - there is a tiny window
where an irq could arrive and see pending = true for the wrong vblank.

I'm not familiar enough with this driver, however, to tell if it is
really possible to get an irq here or not.

-Dan

>  }
>
>  static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
> -                                  struct drm_crtc_state *old_crtc_state)
> +                                  struct drm_crtc_state *old_crtc_state)
>  {
>         struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 12/20] drm: omapdrm: Prevent processing the same event multiple times

[Laurent Pinchart]

Hi Daniel,

On Tuesday 27 Sep 2016 23:05:20 Daniel Kurtz wrote:
> On Mon, Sep 19, 2016 at 8:27 PM, Laurent Pinchart wrote:
> > The vblank interrupt is disabled after one occurrence, preventing the
> > atomic update event from being processed twice. However, this also
> > prevents the software frame counter from being updated correctly that
> > would require vblank interrupts to be kept enabled while the CRTC is
> > active.
> > 
> > In preparation for vblank interrupt fixes, make sure that the atomic
> > update event will be processed once only when the vblank interrupt will
> > be kept enabled.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_crtc.c | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > b/drivers/gpu/drm/omapdrm/omap_crtc.c index a0c26592fc69..8fef6558197b
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> > @@ -43,6 +43,7 @@ struct omap_crtc {
> >         bool enabled;
> >         bool pending;
> >         wait_queue_head_t pending_wait;
> > +       struct drm_pending_vblank_event *event;
> >  };
> >  
> >  /* ---------------------------------------------------------------------
> > @@ -260,11 +261,15 @@ static const struct dss_mgr_ops mgr_ops = {
> > 
> >  static void omap_crtc_complete_page_flip(struct drm_crtc *crtc)
> >  {
> > +       struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> >         struct drm_pending_vblank_event *event;
> >         struct drm_device *dev = crtc->dev;
> >         unsigned long flags;
> > 
> > -       event = crtc->state->event;
> > +       spin_lock_irqsave(&dev->event_lock, flags);
> > +       event = omap_crtc->event;
> > +       omap_crtc->event = NULL;
> > +       spin_unlock_irqrestore(&dev->event_lock, flags);
> 
> Perhaps just hold the lock until after drm_crtc_send_vblank_event(crtc,
> event)?

Good point, I'll do that.

> >         if (!event)
> >                 return;
> > 
> > @@ -384,12 +389,23 @@ static int omap_crtc_atomic_check(struct drm_crtc
> > *crtc,
> >  }
> >  
> >  static void omap_crtc_atomic_begin(struct drm_crtc *crtc,
> > -                                  struct drm_crtc_state *old_crtc_state)
> > +                                  struct drm_crtc_state *old_crtc_state)
> >  {
> > +       struct drm_pending_vblank_event *event = crtc->state->event;
> > +       struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> > +       unsigned long flags;
> > +
> > +       if (event) {
> > +               WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> > +
> > +               spin_lock_irqsave(&crtc->dev->event_lock, flags);
> > +               omap_crtc->event = event;
> > +               spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > +       }
> 
> AFAICT, the only reason to use the spin_lock_irqsave here is if there
> is a possibility that the vblank irq can run in parallel to
> atomic_begin().
> 
> If this is the case, then setting omap_crtc->event here in
> atomic_begin() may not work exactly as you wish.
> It sets up a race: the vblank irq may call
> omap_crtc_complete_page_flip and send the event between here and when
> the "GO" bit is set in atomic_flush(), and thus, it may send the event
> too early, on the wrong vblank.
> 
> By the way, it looks like there is the same race, but just much
> shorter, with the "omap_crtc->pending" flag.  Since it is set in
> atomic_flush() before hitting the go bit - there is a tiny window
> where an irq could arrive and see pending = true for the wrong vblank.

This should be fixed in patch 14/20 ("drm: omapdrm: Keep vblank interrupt 
enabled while CRTC is active") in this series.

> I'm not familiar enough with this driver, however, to tell if it is
> really possible to get an irq here or not.

It shouldn't be as the driver only enables interrupts after setting the GO 
bit. However, that logic is changed in the aforementioned patch, which will 
introduce the race condition you described. I'll move the code to the atomic 
flush handler in this patch to fix it.

> >  }
> >  
> >  static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
> > -                                  struct drm_crtc_state *old_crtc_state)
> > +                                  struct drm_crtc_state *old_crtc_state)
> >  {
> >         struct omap_crtc *omap_crtc = to_omap_crtc(crtc);

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 12/20] drm: omapdrm: Prevent processing the same event multiple times

[Laurent Pinchart]

Hi Tomi,

On Tuesday 27 Sep 2016 15:24:47 Tomi Valkeinen wrote:
> On 19/09/16 15:27, Laurent Pinchart wrote:
> > The vblank interrupt is disabled after one occurrence, preventing the
> > atomic update event from being processed twice. However, this also
> > prevents the software frame counter from being updated correctly that
> > would require vblank interrupts to be kept enabled while the CRTC is
> > active.
> > 
> > In preparation for vblank interrupt fixes, make sure that the atomic
> > update event will be processed once only when the vblank interrupt will
> > be kept enabled.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_crtc.c | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > b/drivers/gpu/drm/omapdrm/omap_crtc.c index a0c26592fc69..8fef6558197b
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> > @@ -43,6 +43,7 @@ struct omap_crtc {
> >  	bool enabled;
> >  	bool pending;
> >  	wait_queue_head_t pending_wait;
> > +	struct drm_pending_vblank_event *event;
> >  };
> >  
> >  /* ----------------------------------------------------------------------
> > @@ -260,11 +261,15 @@ static const struct dss_mgr_ops mgr_ops = {
> > 
> >  static void omap_crtc_complete_page_flip(struct drm_crtc *crtc)
> >  {
> > +	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> >  	struct drm_pending_vblank_event *event;
> >  	struct drm_device *dev = crtc->dev;
> >  	unsigned long flags;
> > 
> > -	event = crtc->state->event;
> > +	spin_lock_irqsave(&dev->event_lock, flags);
> > +	event = omap_crtc->event;
> > +	omap_crtc->event = NULL;
> > +	spin_unlock_irqrestore(&dev->event_lock, flags);
> > 
> >  	if (!event)
> >  		return;
> > @@ -384,12 +389,23 @@ static int omap_crtc_atomic_check(struct drm_crtc
> > *crtc,
> >  }
> >  
> >  static void omap_crtc_atomic_begin(struct drm_crtc *crtc,
> > -                                  struct drm_crtc_state *old_crtc_state)
> > +				   struct drm_crtc_state *old_crtc_state)
> >  {
> > +	struct drm_pending_vblank_event *event = crtc->state->event;
> > +	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> > +	unsigned long flags;
> > +
> > +	if (event) {
> > +		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> 
> Where's the matching put?

It's missing, or rather the get shouldn't be added here. The vblank get/put 
calls are added by patch 14/20 in this series. I'll remove the call from here.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 18/20] drm: omapdrm: Make pipe2vbl function static

[Tomi Valkeinen]

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


On 19/09/16 15:27, Laurent Pinchart wrote:
> The function is only used in omap_irq.c, move it there and make it
> static.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 7 -------
>  drivers/gpu/drm/omapdrm/omap_drv.h  | 1 -
>  drivers/gpu/drm/omapdrm/omap_irq.c  | 7 ++++++-
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 6983a889d811..fb01357721fe 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -48,13 +48,6 @@ struct omap_crtc {
>   * Helper Functions
>   */
>  
> -uint32_t pipe2vbl(struct drm_crtc *crtc)
> -{
> -	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> -
> -	return dispc_mgr_get_vsync_irq(omap_crtc->channel);
> -}
> -
>  struct omap_video_timings *omap_crtc_timings(struct drm_crtc *crtc)
>  {
>  	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
> index 8aa8e45098a2..4fee7b94eefb 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -234,7 +234,6 @@ struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
>  		struct dma_buf *buffer);
>  
>  /* map crtc to vblank mask */
> -uint32_t pipe2vbl(struct drm_crtc *crtc);
>  struct omap_dss_device *omap_encoder_get_dssdev(struct drm_encoder *encoder);
>  
>  #endif /* __OMAP_DRV_H__ */
> diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c
> index 27225492c81b..28ff7b73a853 100644
> --- a/drivers/gpu/drm/omapdrm/omap_irq.c
> +++ b/drivers/gpu/drm/omapdrm/omap_irq.c
> @@ -108,6 +108,11 @@ int omap_irq_wait(struct drm_device *dev, struct omap_irq_wait *wait,
>  	return 0;
>  }
>  
> +static uint32_t pipe2vbl(struct drm_crtc *crtc)
> +{
> +	return dispc_mgr_get_vsync_irq(omap_crtc_channel(crtc));
> +}
> +
>  /**
>   * enable_vblank - enable vblank interrupt events
>   * @dev: DRM device
> @@ -228,7 +233,7 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
>  		struct drm_crtc *crtc = priv->crtcs[id];
>  		enum omap_channel channel = omap_crtc_channel(crtc);
>  
> -		if (irqstatus & pipe2vbl(crtc)) {
> +		if (irqstatus & dispc_mgr_get_vsync_irq(channel)) {
>  			drm_handle_vblank(dev, id);
>  			omap_crtc_vblank_irq(crtc);
>  		}
> 

Did you look at the code you have above =).

I think the bottom change is a good one. Just remove pipe2vbl().

 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 v3 02/20] drm: omapdrm: fb: Use format information provided by the DRM core

[Laurent Pinchart]

Hi Tomi,

On Tuesday 20 Sep 2016 15:47:57 Tomi Valkeinen wrote:
> On 19/09/16 15:27, Laurent Pinchart wrote:
> > The driver stores in a custom structure named format several pieces of
> > information about the format that are available in the DRM core. Remove
> > them and get the information from the DRM core instead.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> > 
> > @@ -128,13 +122,13 @@ static const struct drm_framebuffer_funcs
> > omap_framebuffer_funcs = {
> >  };
> >  
> >  static uint32_t get_linear_addr(struct plane *plane,
> > -		const struct format *format, int n, int x, int y)
> > +		const struct drm_format_info *format, int n, int x, int y)
> >  {
> >  	uint32_t offset;
> > 
> > -	offset = plane->offset +
> > -			(x * format->planes[n].stride_bpp) +
> > -			(y * plane->pitch / format->planes[n].sub_y);
> > +	offset = plane->offset
> > +	       + (x * format->cpp[n] / (n == 1 ? 1 : format->hsub))
> > +	       + (y * plane->pitch / (n == 1 ? 1 : format->vsub));
> 
> n is the plane number? Shouldn't the above be (n == 0 ? 1 : format->hsub)?

Oops. I'll fix this.

> > @@ -413,28 +410,32 @@ struct drm_framebuffer *omap_framebuffer_init(struct
> > drm_device *dev,
> > 
> >  	fb = &omap_fb->base;
> >  	omap_fb->format = format;
> > +	omap_fb->dss_format = dss_format;
> > 
> >  	mutex_init(&omap_fb->lock);
> > 
> > -	for (i = 0; i < n; i++) {
> > +	for (i = 0; i < format->num_planes; i++) {
> >  		struct plane *plane = &omap_fb->planes[i];
> > -		int size, pitch = mode_cmd->pitches[i];
> > +		unsigned int pitch = mode_cmd->pitches[i];
> > +		unsigned int hsub = i == 0 ? 1 : format->hsub;
> > +		unsigned int vsub = i == 0 ? 1 : format->vsub;
> 
> This makes me wonder... Will all drivers do something like the above?
> It's a bit laborious way to get the pixel subsampling factor, and I
> presume something like above is quite common so that all the
> calculations can be more generic (and not specific to a UV plane).

Helper functions could be useful. That would require auditing drivers to 
locate common code patterns, and I'm afraid I don't have time to do so at the 
moment. Patches are of course welcome ;-)

Some of this code can also be removed as the DRM core performs checks in 
framebuffer_check() too. I'll add a patch to fix that.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 06/20] drm: omapdrm: Handle FIFO underflow IRQs internally

[Laurent Pinchart]

Hi Tomi,

On Tuesday 20 Sep 2016 16:27:22 Tomi Valkeinen wrote:
> On 19/09/16 15:27, Laurent Pinchart wrote:
> > As the FIFO underflow IRQ handler just prints an error message to the
> > kernel log, simplify the code by not registering one IRQ handler per
> > plane but print the messages directly from the main IRQ handler.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Only register error IRQs that exist on the HW
> > ---
> > 
> > 
> > +static const u32 error_irqs[] = {
> > +	[OMAP_DSS_GFX] = DISPC_IRQ_GFX_FIFO_UNDERFLOW,
> > +	[OMAP_DSS_VIDEO1] = DISPC_IRQ_VID1_FIFO_UNDERFLOW,
> > +	[OMAP_DSS_VIDEO2] = DISPC_IRQ_VID2_FIFO_UNDERFLOW,
> > +	[OMAP_DSS_VIDEO3] = DISPC_IRQ_VID3_FIFO_UNDERFLOW,
> > +};
> 
> Actually, one comment: "error_irqs" is perhaps not a good name for this.
> "omap_underflow_irqs"?

That sounds good to me, I'll rename it.

> Or actually, maybe it would make sense to move this into dispc, and have
> a similar func to dispc_mgr_get_sync_lost_irq().

I'm certainly not a big fan of the omapdrm/omapdss split, but I think that's a 
more general problem. I'll likely need to reorganize that when trying to move 
panel and encoder drivers to drm_panel and drm_bridge. I propose revisiting 
the topic then.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 08/20] drm: omapdrm: Handle OCP error IRQ directly

[Laurent Pinchart]

Hi Tomi,

On Tuesday 20 Sep 2016 16:34:37 Tomi Valkeinen wrote:
> On 19/09/16 15:27, Laurent Pinchart wrote:
> > Instead of going through a complicated registration mechanism, just
> > call the OCP error IRQ handler directly from the main IRQ handler.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Rename IRQ handler to omap_irq_ocp_error_handler()
> > - Replace hex error value with "OCP error" message
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_drv.h |  1 -
> >  drivers/gpu/drm/omapdrm/omap_irq.c | 28 ++++++++++------------------
> >  2 files changed, 10 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h
> > b/drivers/gpu/drm/omapdrm/omap_drv.h index 7a1d8384147c..05375bc40091
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> > +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> > @@ -103,7 +103,6 @@ struct omap_drm_private {
> > 
> >  	/* irq handling: */
> >  	struct list_head irq_list;    /* list of omap_drm_irq */
> >  	uint32_t irq_mask;		/* enabled irqs in addition to 
irq_list */
> > 
> > -	struct omap_drm_irq error_handler;
> > 
> >  	/* atomic commit */
> >  	struct {
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c
> > b/drivers/gpu/drm/omapdrm/omap_irq.c index f0f110faf1e6..257c1f3c76f3
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_irq.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_irq.c
> > @@ -21,12 +21,6 @@
> > 
> >  static DEFINE_SPINLOCK(list_lock);
> > 
> > -static void omap_irq_error_handler(struct omap_drm_irq *irq,
> > -		uint32_t irqstatus)
> > -{
> > -	DRM_ERROR("errors: %08x\n", irqstatus);
> > -}
> > -
> > 
> >  /* call with list_lock and dispc runtime held */
> >  static void omap_irq_update(struct drm_device *dev)
> >  {
> > 
> > @@ -224,6 +218,14 @@ static void omap_irq_fifo_underflow(struct
> > omap_drm_private *priv,> 
> >  	pr_cont("(0x%08x)\n", irqstatus);
> >  
> >  }
> > 
> > +static void omap_irq_ocp_error_handler(u32 irqstatus)
> > +{
> > +	if (!(irqstatus & DISPC_IRQ_OCP_ERR))
> > +		return;
> > +
> > +	DRM_ERROR("OCP error\n");
> > +}
> > +
> > 
> >  static irqreturn_t omap_irq_handler(int irq, void *arg)
> >  {
> >  
> >  	struct drm_device *dev = (struct drm_device *) arg;
> > 
> > @@ -250,6 +252,7 @@ static irqreturn_t omap_irq_handler(int irq, void
> > *arg)
> > 
> >  			omap_crtc_error_irq(crtc, irqstatus);
> >  	
> >  	}
> > 
> > +	omap_irq_ocp_error_handler(irqstatus);
> > 
> >  	omap_irq_fifo_underflow(priv, irqstatus);
> >  	
> >  	spin_lock_irqsave(&list_lock, flags);
> > 
> > @@ -282,7 +285,6 @@ static const u32 error_irqs[] = {
> > 
> >  int omap_drm_irq_install(struct drm_device *dev)
> >  {
> >  
> >  	struct omap_drm_private *priv = dev->dev_private;
> > 
> > -	struct omap_drm_irq *error_handler = &priv->error_handler;
> > 
> >  	unsigned int num_mgrs = dss_feat_get_num_mgrs();
> >  	unsigned int max_planes;
> >  	unsigned int i;
> > 
> > @@ -290,7 +292,7 @@ int omap_drm_irq_install(struct drm_device *dev)
> > 
> >  	INIT_LIST_HEAD(&priv->irq_list);
> > 
> > -	priv->irq_mask = 0;
> > +	priv->irq_mask = DISPC_IRQ_OCP_ERR;
> > 
> >  	max_planes = min(ARRAY_SIZE(priv->planes), ARRAY_SIZE(error_irqs));
> >  	for (i = 0; i < max_planes; ++i) {
> > 
> > @@ -309,16 +311,6 @@ int omap_drm_irq_install(struct drm_device *dev)
> > 
> >  	if (ret < 0)
> >  	
> >  		return ret;
> > 
> > -	error_handler->irq = omap_irq_error_handler;
> > -	error_handler->irqmask = DISPC_IRQ_OCP_ERR;
> > -
> > -	/* for now ignore DISPC_IRQ_SYNC_LOST_DIGIT.. really I think
> > -	 * we just need to ignore it while enabling tv-out
> > -	 */
> > -	error_handler->irqmask &= ~DISPC_IRQ_SYNC_LOST_DIGIT;
> > -
> > -	omap_irq_register(dev, error_handler);
> > -
> > 
> >  	dev->irq_enabled = true;
> >  	
> >  	return 0;
> 
> The removal of DISPC_IRQ_SYNC_LOST_DIGIT code above feels a bit out of
> place for this patch.

It's a no-op already, as irqmask is set to DISPC_IRQ_OCP_ERR (bit 9), and 
immediately after that bit 15 is masked out. I could split the removal out, 
but I think it wouldn't be very useful.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 09/20] drm: omapdrm: Replace DSS manager state check with omapdrm CRTC state

[Laurent Pinchart]

Hi Tomi,

On Tuesday 20 Sep 2016 16:44:21 Tomi Valkeinen wrote:
> On 19/09/16 15:27, Laurent Pinchart wrote:
> > Instead of conditioning planes update based on the DD manager state, use
> 
> s/DD/DSS/
> 
> Maybe "manager HW state" to highlight that the status is read from a HW
> register.

I'll change that.

> > the enabled field newly added to the omap_crtc structure. This reduces
> > the dependency from the DRM layer to the DSS layer.
> > 
> > The enabled field is a transitory measure, the implementation should use
> > the CRTC atomic state instead. However, given that CRTCs are currently
> > not enabled/disabled through their .enable() and .disable() operations
> > but through a convoluted code paths starting at the associated encoder
> > operations, there is not clear guarantee that the atomic state always
> > matches the hardware state. This will be refactored later, at which
> > point the enabled field will be removed.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v2:
> > 
> > - Use enabled field in struct omap_crtc instead of CRTC atomic state
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_crtc.c | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > b/drivers/gpu/drm/omapdrm/omap_crtc.c index cdcfda31043e..f41a638c8d65
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> > @@ -40,6 +40,7 @@ struct omap_crtc {
> > 
> >  	bool ignore_digit_sync_lost;
> > 
> > +	bool enabled;
> >  	bool pending;
> >  	wait_queue_head_t pending_wait;
> >  };
> > @@ -136,6 +137,7 @@ static void omap_crtc_set_enabled(struct drm_crtc
> > *crtc, bool enable)
> > 
> >  	if (omap_crtc_output[channel]->output_type == OMAP_DISPLAY_TYPE_HDMI)
> >  	{
> >  		dispc_mgr_enable(channel, enable);
> > +		omap_crtc->enabled = enable;
> >  		return;
> >  	}
> > 
> > @@ -172,6 +174,7 @@ static void omap_crtc_set_enabled(struct drm_crtc
> > *crtc, bool enable)
> >  	}
> >  	
> >  	dispc_mgr_enable(channel, enable);
> > +	omap_crtc->enabled = enable;
> 
> omap_crtc_set_enabled() is only called from omap_crtc_dss_enable() and
> omap_crtc_dss_disable(). It's probably a bit cleaner to set the
> omap_crtc->enabled in those functions.

I like keeping the line close to the dispc_mgr_enable() call. If you insist I 
can move it, it doesn't make a big difference. I hope to refactor all that as 
part of the panel and encoder rework anyway.

> Btw, omap_crtc_set_enabled() has a comment:
> 
> /* Called only from the encoder enable/disable and suspend/resume
> handlers. */
> 
> which doesn't seem to hold true...

omap_crtc_dss_(enable|disable) are only called from the encoder drivers, I 
think that's what the comment tries to capture.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 10/20] drm: omapdrm: Only commit planes on active CRTCs

[Laurent Pinchart]

Hi Tomi,

On Tuesday 20 Sep 2016 16:51:11 Tomi Valkeinen wrote:
> On 19/09/16 15:27, Laurent Pinchart wrote:
> > The DRM core supports skipping plane update for inactive CRTCs for
> > hardware that don't need it or can't cope with it. That's our case, so
> > use the DRM core infrastructure instead of reinventing it.
> 
> I don't follow this desc. What is omapdrm reinventing? At least this
> patch does not remove any of the "reinvention".

There used to be one, but it got removed when I rebased the patch series. I'll 
reword the commit message.

> What does DRM_PLANE_COMMIT_ACTIVE_ONLY do? Skips plane HW configuration
> for planes on crtcs that are disabled? The plane HW config will still be
> done when the crtc is about to be enabled, right?

It skips plane update (atomic_begin, atomic_disable, atomic_flush) for 
disabled CRTCs. The CRTC .begin() operation is still called for those CRTCs, 
only plane update is skipped.

Now that I wrote that, I'm not quite sure this change is right. It looks like 
disabling a plane is shadowed, and without an atomic_flush call the GO bit 
will never be set. However, the problem predates this patch, as the GO bit 
will only be set if dispc_mgr_is_enabled() returns true, which shouldn't be 
the case for disabled CRTCs.

How is this supposed to work, how is plane disable supposed to be synchronized 
at the hardware level ?

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 11/20] drm: omapdrm: Check DSS manager state in the enable/disable helpers

[Laurent Pinchart]

Hi Tomi,

On Tuesday 20 Sep 2016 16:57:59 Tomi Valkeinen wrote:
> On 19/09/16 15:27, Laurent Pinchart wrote:
> > The omapdrm DSS manager enable/disable operations check the DSS manager
> > state to avoid double enabling/disabling. Move that code to the DSS
> > manager to decrease the dependency of the DRM layer to the DSS layer.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/dss/dispc.c  | 1 -
> >  drivers/gpu/drm/omapdrm/dss/output.c | 6 ++++++
> >  drivers/gpu/drm/omapdrm/omap_crtc.c  | 3 ---
> >  3 files changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> > b/drivers/gpu/drm/omapdrm/dss/dispc.c index 535240fba671..ab150bf21dd8
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> > +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> > @@ -2911,7 +2911,6 @@ bool dispc_mgr_is_enabled(enum omap_channel channel)
> >  {
> >  	return !!mgr_fld_read(channel, DISPC_MGR_FLD_ENABLE);
> >  }
> > -EXPORT_SYMBOL(dispc_mgr_is_enabled);
> > 
> >  void dispc_wb_enable(bool enable)
> >  {
> > diff --git a/drivers/gpu/drm/omapdrm/dss/output.c
> > b/drivers/gpu/drm/omapdrm/dss/output.c index 24f859488201..f0be621895fa
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/dss/output.c
> > +++ b/drivers/gpu/drm/omapdrm/dss/output.c
> > @@ -217,12 +217,18 @@ EXPORT_SYMBOL(dss_mgr_set_lcd_config);
> > 
> >  int dss_mgr_enable(enum omap_channel channel)
> >  {
> > +	if (dispc_mgr_is_enabled(channel))
> > +		return 0;
> > +
> >  	return dss_mgr_ops->enable(channel);
> >  }
> >  EXPORT_SYMBOL(dss_mgr_enable);
> >  
> >  void dss_mgr_disable(enum omap_channel channel)
> >  {
> > +	if (!dispc_mgr_is_enabled(channel))
> > +		return;
> > +
> >  	dss_mgr_ops->disable(channel);
> >  }
> >  EXPORT_SYMBOL(dss_mgr_disable);
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > b/drivers/gpu/drm/omapdrm/omap_crtc.c index 4b7e16786e1e..a0c26592fc69
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> > @@ -141,9 +141,6 @@ static void omap_crtc_set_enabled(struct drm_crtc
> > *crtc, bool enable)
> >  		return;
> >  	}
> > 
> > -	if (dispc_mgr_is_enabled(channel) == enable)
> > -		return;
> > -
> >  	if (omap_crtc->channel == OMAP_DSS_CHANNEL_DIGIT) {
> >  		/*
> >  		 * Digit output produces some sync lost interrupts during the
> 
> With this change omap_crtc_set_enabled() will do extra work if the
> output is already enabled/disabled, and, if I'm not mistaken, will do
> omap_irq_wait() there which might lead to issues.

That's correct, but the DRM core nowadays should really guarantee that CRTCs 
won't be enabled or disabled multiple times. 

> If you remove the check, then I think the driver should make sure that
> omap_crtc_set_enabled() is not called if the output is already
> enabled/disabled. Maybe that can be done in
> omap_crtc_dss_enable/disable, using the new enabled flag.

The situation isn't supposed to happen, would you be more comfortable still 
checking in the enable/disable handlers ? If so I propose using a WARN_ON() to 
catch those impossible situations.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 18/20] drm: omapdrm: Make pipe2vbl function static

[Laurent Pinchart]

Hi Tomi,

On Monday 12 Dec 2016 12:41:11 Tomi Valkeinen wrote:
> On 19/09/16 15:27, Laurent Pinchart wrote:
> > The function is only used in omap_irq.c, move it there and make it
> > static.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_crtc.c | 7 -------
> >  drivers/gpu/drm/omapdrm/omap_drv.h  | 1 -
> >  drivers/gpu/drm/omapdrm/omap_irq.c  | 7 ++++++-
> >  3 files changed, 6 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > b/drivers/gpu/drm/omapdrm/omap_crtc.c index 6983a889d811..fb01357721fe
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> > @@ -48,13 +48,6 @@ struct omap_crtc {
> >   * Helper Functions
> >   */
> > 
> > -uint32_t pipe2vbl(struct drm_crtc *crtc)
> > -{
> > -	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> > -
> > -	return dispc_mgr_get_vsync_irq(omap_crtc->channel);
> > -}
> > -
> >  struct omap_video_timings *omap_crtc_timings(struct drm_crtc *crtc)
> >  {
> >  	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h
> > b/drivers/gpu/drm/omapdrm/omap_drv.h index 8aa8e45098a2..4fee7b94eefb
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> > +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> > @@ -234,7 +234,6 @@ struct drm_gem_object *omap_gem_prime_import(struct
> > drm_device *dev,
> >  		struct dma_buf *buffer);
> >  
> >  /* map crtc to vblank mask */
> > -uint32_t pipe2vbl(struct drm_crtc *crtc);
> > 
> >  struct omap_dss_device *omap_encoder_get_dssdev(struct drm_encoder
> >  *encoder);
> >  
> >  #endif /* __OMAP_DRV_H__ */
> > diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c
> > b/drivers/gpu/drm/omapdrm/omap_irq.c index 27225492c81b..28ff7b73a853
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_irq.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_irq.c
> > @@ -108,6 +108,11 @@ int omap_irq_wait(struct drm_device *dev, struct
> > omap_irq_wait *wait,
> >  	return 0;
> >  }
> > 
> > +static uint32_t pipe2vbl(struct drm_crtc *crtc)
> > +{
> > +	return dispc_mgr_get_vsync_irq(omap_crtc_channel(crtc));
> > +}
> > +
> >  /**
> >   * enable_vblank - enable vblank interrupt events
> >   * @dev: DRM device
> > @@ -228,7 +233,7 @@ static irqreturn_t omap_irq_handler(int irq, void
> > *arg)
> >  		struct drm_crtc *crtc = priv->crtcs[id];
> >  		enum omap_channel channel = omap_crtc_channel(crtc);
> > 
> > -		if (irqstatus & pipe2vbl(crtc)) {
> > +		if (irqstatus & dispc_mgr_get_vsync_irq(channel)) {
> >  			drm_handle_vblank(dev, id);
> >  			omap_crtc_vblank_irq(crtc);
> >  		}
> 
> Did you look at the code you have above =).

I've added this change because the function already retrieved the channel 
number, so it's pointless to duplicate the operation.

> I think the bottom change is a good one. Just remove pipe2vbl().

The result fits in 80 columns, so I'm OK :-) I'll remove it.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 10/20] drm: omapdrm: Only commit planes on active CRTCs

[Tomi Valkeinen]

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

On 13/12/16 00:53, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Tuesday 20 Sep 2016 16:51:11 Tomi Valkeinen wrote:
>> On 19/09/16 15:27, Laurent Pinchart wrote:
>>> The DRM core supports skipping plane update for inactive CRTCs for
>>> hardware that don't need it or can't cope with it. That's our case, so
>>> use the DRM core infrastructure instead of reinventing it.
>>
>> I don't follow this desc. What is omapdrm reinventing? At least this
>> patch does not remove any of the "reinvention".
> 
> There used to be one, but it got removed when I rebased the patch series. I'll 
> reword the commit message.
> 
>> What does DRM_PLANE_COMMIT_ACTIVE_ONLY do? Skips plane HW configuration
>> for planes on crtcs that are disabled? The plane HW config will still be
>> done when the crtc is about to be enabled, right?
> 
> It skips plane update (atomic_begin, atomic_disable, atomic_flush) for 
> disabled CRTCs. The CRTC .begin() operation is still called for those CRTCs, 
> only plane update is skipped.

I didn't catch that. Skips atomic_begin, but .begin is still called?

> Now that I wrote that, I'm not quite sure this change is right. It looks like 
> disabling a plane is shadowed, and without an atomic_flush call the GO bit 
> will never be set. However, the problem predates this patch, as the GO bit 
> will only be set if dispc_mgr_is_enabled() returns true, which shouldn't be 
> the case for disabled CRTCs.
> 
> How is this supposed to work, how is plane disable supposed to be synchronized 
> at the hardware level ?

If a crtc is currently disabled (in the HW), all the new settings
(including plane enable/disable bit) can be programmed and when the crtc
is enabled, those settings are taken into use. GO bit is not necessary.

When the crtc is active, you need to set the GO bit to get the new
settings into use at next vblank.

If a crtc is to be disabled, we can just disable the crtc, GO bit is not
necessary.

 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 v3 11/20] drm: omapdrm: Check DSS manager state in the enable/disable helpers

[Tomi Valkeinen]

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

On 13/12/16 01:07, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Tuesday 20 Sep 2016 16:57:59 Tomi Valkeinen wrote:
>> On 19/09/16 15:27, Laurent Pinchart wrote:
>>> The omapdrm DSS manager enable/disable operations check the DSS manager
>>> state to avoid double enabling/disabling. Move that code to the DSS
>>> manager to decrease the dependency of the DRM layer to the DSS layer.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>>
>>>  drivers/gpu/drm/omapdrm/dss/dispc.c  | 1 -
>>>  drivers/gpu/drm/omapdrm/dss/output.c | 6 ++++++
>>>  drivers/gpu/drm/omapdrm/omap_crtc.c  | 3 ---
>>>  3 files changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
>>> b/drivers/gpu/drm/omapdrm/dss/dispc.c index 535240fba671..ab150bf21dd8
>>> 100644
>>> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
>>> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
>>> @@ -2911,7 +2911,6 @@ bool dispc_mgr_is_enabled(enum omap_channel channel)
>>>  {
>>>  	return !!mgr_fld_read(channel, DISPC_MGR_FLD_ENABLE);
>>>  }
>>> -EXPORT_SYMBOL(dispc_mgr_is_enabled);
>>>
>>>  void dispc_wb_enable(bool enable)
>>>  {
>>> diff --git a/drivers/gpu/drm/omapdrm/dss/output.c
>>> b/drivers/gpu/drm/omapdrm/dss/output.c index 24f859488201..f0be621895fa
>>> 100644
>>> --- a/drivers/gpu/drm/omapdrm/dss/output.c
>>> +++ b/drivers/gpu/drm/omapdrm/dss/output.c
>>> @@ -217,12 +217,18 @@ EXPORT_SYMBOL(dss_mgr_set_lcd_config);
>>>
>>>  int dss_mgr_enable(enum omap_channel channel)
>>>  {
>>> +	if (dispc_mgr_is_enabled(channel))
>>> +		return 0;
>>> +
>>>  	return dss_mgr_ops->enable(channel);
>>>  }

Looking at this again, I think this is not correct. The functions in
output.c are just trampolines basically. They just pass the calls to
omapdrm. Let's not add any logic there.

Also, the above could even cause a crash, as the HW is not necessarily
enabled before the ->enable() is called.

>>>  EXPORT_SYMBOL(dss_mgr_enable);
>>>  
>>>  void dss_mgr_disable(enum omap_channel channel)
>>>  {
>>> +	if (!dispc_mgr_is_enabled(channel))
>>> +		return;
>>> +
>>>  	dss_mgr_ops->disable(channel);
>>>  }
>>>  EXPORT_SYMBOL(dss_mgr_disable);
>>>
>>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
>>> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 4b7e16786e1e..a0c26592fc69
>>> 100644
>>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
>>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
>>> @@ -141,9 +141,6 @@ static void omap_crtc_set_enabled(struct drm_crtc
>>> *crtc, bool enable)
>>>  		return;
>>>  	}
>>>
>>> -	if (dispc_mgr_is_enabled(channel) == enable)
>>> -		return;
>>> -
>>>  	if (omap_crtc->channel == OMAP_DSS_CHANNEL_DIGIT) {
>>>  		/*
>>>  		 * Digit output produces some sync lost interrupts during the
>>
>> With this change omap_crtc_set_enabled() will do extra work if the
>> output is already enabled/disabled, and, if I'm not mistaken, will do
>> omap_irq_wait() there which might lead to issues.
> 
> That's correct, but the DRM core nowadays should really guarantee that CRTCs 
> won't be enabled or disabled multiple times. 

Ok. So we could probably add a dev_warn() there, as getting that state
wrong might cause issues that are difficult to debug. It would be best
to check the HW state, but if you want to remove the call to dispc, I
guess just checking omap_crtc->enabled is good enough.

The comment for omap_crtc_set_enabled talks about suspend/resume.
Perhaps at some point it was called from suspend/resume, even for crtcs
that were already disabled, which would explain the need for the check.

 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 v3 10/20] drm: omapdrm: Only commit planes on active CRTCs

[Laurent Pinchart]

Hi Tomi,

On Tuesday 13 Dec 2016 09:58:34 Tomi Valkeinen wrote:
> On 13/12/16 00:53, Laurent Pinchart wrote:
> > On Tuesday 20 Sep 2016 16:51:11 Tomi Valkeinen wrote:
> >> On 19/09/16 15:27, Laurent Pinchart wrote:
> >>> The DRM core supports skipping plane update for inactive CRTCs for
> >>> hardware that don't need it or can't cope with it. That's our case, so
> >>> use the DRM core infrastructure instead of reinventing it.
> >> 
> >> I don't follow this desc. What is omapdrm reinventing? At least this
> >> patch does not remove any of the "reinvention".
> > 
> > There used to be one, but it got removed when I rebased the patch series.
> > I'll reword the commit message.
> > 
> >> What does DRM_PLANE_COMMIT_ACTIVE_ONLY do? Skips plane HW configuration
> >> for planes on crtcs that are disabled? The plane HW config will still be
> >> done when the crtc is about to be enabled, right?
> > 
> > It skips plane update (atomic_begin, atomic_disable, atomic_flush) for
> > disabled CRTCs. The CRTC .begin() operation is still called for those
> > CRTCs, only plane update is skipped.
> 
> I didn't catch that. Skips atomic_begin, but .begin is still called?

Sorry, I meant .disable().

> > Now that I wrote that, I'm not quite sure this change is right. It looks
> > like disabling a plane is shadowed, and without an atomic_flush call the
> > GO bit will never be set. However, the problem predates this patch, as
> > the GO bit will only be set if dispc_mgr_is_enabled() returns true, which
> > shouldn't be the case for disabled CRTCs.
> > 
> > How is this supposed to work, how is plane disable supposed to be
> > synchronized at the hardware level ?
> 
> If a crtc is currently disabled (in the HW), all the new settings
> (including plane enable/disable bit) can be programmed and when the crtc
> is enabled, those settings are taken into use. GO bit is not necessary.
> 
> When the crtc is active, you need to set the GO bit to get the new
> settings into use at next vblank.
> 
> If a crtc is to be disabled, we can just disable the crtc, GO bit is not
> necessary.

Thanks for the clarification.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v3 11/20] drm: omapdrm: Check DSS manager state in the enable/disable helpers

[Laurent Pinchart]

Hi Tomi,

On Tuesday 13 Dec 2016 10:15:35 Tomi Valkeinen wrote:
> On 13/12/16 01:07, Laurent Pinchart wrote:
> > On Tuesday 20 Sep 2016 16:57:59 Tomi Valkeinen wrote:
> >> On 19/09/16 15:27, Laurent Pinchart wrote:
> >>> The omapdrm DSS manager enable/disable operations check the DSS manager
> >>> state to avoid double enabling/disabling. Move that code to the DSS
> >>> manager to decrease the dependency of the DRM layer to the DSS layer.
> >>> 
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> ---
> >>>  drivers/gpu/drm/omapdrm/dss/dispc.c  | 1 -
> >>>  drivers/gpu/drm/omapdrm/dss/output.c | 6 ++++++
> >>>  drivers/gpu/drm/omapdrm/omap_crtc.c  | 3 ---
> >>>  3 files changed, 6 insertions(+), 4 deletions(-)
> >>> 
> >>> diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
> >>> b/drivers/gpu/drm/omapdrm/dss/dispc.c index 535240fba671..ab150bf21dd8
> >>> 100644
> >>> --- a/drivers/gpu/drm/omapdrm/dss/dispc.c
> >>> +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
> >>> @@ -2911,7 +2911,6 @@ bool dispc_mgr_is_enabled(enum omap_channel
> >>> channel)
> >>>  {
> >>>  	return !!mgr_fld_read(channel, DISPC_MGR_FLD_ENABLE);
> >>>  }
> >>> -EXPORT_SYMBOL(dispc_mgr_is_enabled);
> >>> 
> >>>  void dispc_wb_enable(bool enable)
> >>>  {
> >>> diff --git a/drivers/gpu/drm/omapdrm/dss/output.c
> >>> b/drivers/gpu/drm/omapdrm/dss/output.c index 24f859488201..f0be621895fa
> >>> 100644
> >>> --- a/drivers/gpu/drm/omapdrm/dss/output.c
> >>> +++ b/drivers/gpu/drm/omapdrm/dss/output.c
> >>> @@ -217,12 +217,18 @@ EXPORT_SYMBOL(dss_mgr_set_lcd_config);
> >>>  int dss_mgr_enable(enum omap_channel channel)
> >>>  {
> >>> +	if (dispc_mgr_is_enabled(channel))
> >>> +		return 0;
> >>> +
> >>>  	return dss_mgr_ops->enable(channel);
> >>>  }
> 
> Looking at this again, I think this is not correct. The functions in
> output.c are just trampolines basically. They just pass the calls to
> omapdrm. Let's not add any logic there.

I can't wait to clean all this up and get rid of them :-)

> Also, the above could even cause a crash, as the HW is not necessarily
> enabled before the ->enable() is called.
> 
> >>>  EXPORT_SYMBOL(dss_mgr_enable);
> >>>  
> >>>  void dss_mgr_disable(enum omap_channel channel)
> >>>  {
> >>> +	if (!dispc_mgr_is_enabled(channel))
> >>> +		return;
> >>> +
> >>>  	dss_mgr_ops->disable(channel);
> >>>  }
> >>>  EXPORT_SYMBOL(dss_mgr_disable);
> >>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 4b7e16786e1e..a0c26592fc69
> >>> 100644
> >>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>> @@ -141,9 +141,6 @@ static void omap_crtc_set_enabled(struct drm_crtc
> >>> *crtc, bool enable)
> >>>  		return;
> >>>  	}
> >>> 
> >>> -	if (dispc_mgr_is_enabled(channel) == enable)
> >>> -		return;
> >>> -
> >>>  	if (omap_crtc->channel == OMAP_DSS_CHANNEL_DIGIT) {
> >>>  		/*
> >>>  		 * Digit output produces some sync lost interrupts during the
> >> 
> >> With this change omap_crtc_set_enabled() will do extra work if the
> >> output is already enabled/disabled, and, if I'm not mistaken, will do
> >> omap_irq_wait() there which might lead to issues.
> > 
> > That's correct, but the DRM core nowadays should really guarantee that
> > CRTCs won't be enabled or disabled multiple times.
> 
> Ok. So we could probably add a dev_warn() there, as getting that state
> wrong might cause issues that are difficult to debug. It would be best
> to check the HW state, but if you want to remove the call to dispc, I
> guess just checking omap_crtc->enabled is good enough.

I'll do that. I don't like reading back the hardware state when it's supposed 
to match the software state. If there's a mismatch then there's a bug, and if 
they always match, it's pointless.

> The comment for omap_crtc_set_enabled talks about suspend/resume.
> Perhaps at some point it was called from suspend/resume, even for crtcs
> that were already disabled, which would explain the need for the check.

-- 
Regards,

Laurent Pinchart

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