[PATCH 00/23] OMAP DRM fixes and improvements

[PATCH 00/23] OMAP DRM fixes and improvements

[Laurent Pinchart]

Hello,

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

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

The rest of the patches are mostly dead code removal and small
simplifications.

Tomi, do you have pending omapdrm patches for v4.7 ? If so (and if time
permits) would you like to get this series merged through your tree ?

Laurent Pinchart (23):
  drm: omapdrm: fb: Limit number of planes per framebuffer to two
  drm: omapdrm: fb: Don't store format BPP for each plane
  drm: omapdrm: fb: Store number of planes in format structure
  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: Use atomic state instead of local device state
  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: 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
  drm: omapdrm: panel-lgphilips-lb035q02: Remove unused backlight GPIO
  drm: omapdrm: Remove unused omap_framebuffer_bo function
  drm: omapdrm: Remove unused omap_gem_tiled_size function
  drm: omapdrm: Remove buffer synchronization support

 .../omapdrm/displays/panel-lgphilips-lb035q02.c    |  19 --
 drivers/gpu/drm/omapdrm/dss/dispc.c                |   1 -
 drivers/gpu/drm/omapdrm/dss/output.c               |   6 +
 drivers/gpu/drm/omapdrm/omap_crtc.c                | 119 ++++++-----
 drivers/gpu/drm/omapdrm/omap_drv.c                 |  49 -----
 drivers/gpu/drm/omapdrm/omap_drv.h                 |  58 +-----
 drivers/gpu/drm/omapdrm/omap_fb.c                  | 161 ++++++++-------
 drivers/gpu/drm/omapdrm/omap_gem.c                 | 227 ---------------------
 drivers/gpu/drm/omapdrm/omap_irq.c                 | 227 +++++++++++----------
 drivers/gpu/drm/omapdrm/omap_plane.c               |  24 ---
 include/uapi/drm/omap_drm.h                        |  26 ---
 11 files changed, 274 insertions(+), 643 deletions(-)

-- 
Regards,

Laurent Pinchart

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

[PATCH 01/23] 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>
---
 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 610962396eb0..a35e1e052907 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -34,7 +34,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;
 };
 
@@ -88,7 +88,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;
 };
-- 
2.7.3

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

[PATCH 02/23] drm: omapdrm: fb: Don't store format BPP for each plane

[Laurent Pinchart]

The number of bits per pixel is identical for all planes, don't store
multiple copies.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index a35e1e052907..2cc9f15fe439 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -31,33 +31,31 @@
 struct format {
 	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];
+	int stride_bpp;           	/* this times width is stride */
+	int sub_y[2];			/* sub-sample in y dimension (per plane) */
 	bool yuv;
 };
 
 static const struct format 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,   2, { 1, 0 }, false }, /* RGB16-565 */
+	{ OMAP_DSS_COLOR_RGB12U,      DRM_FORMAT_RGBX4444, 2, { 1, 0 }, false }, /* RGB12x-4444 */
+	{ OMAP_DSS_COLOR_RGBX16,      DRM_FORMAT_XRGB4444, 2, { 1, 0 }, false }, /* xRGB12-4444 */
+	{ OMAP_DSS_COLOR_RGBA16,      DRM_FORMAT_RGBA4444, 2, { 1, 0 }, false }, /* RGBA12-4444 */
+	{ OMAP_DSS_COLOR_ARGB16,      DRM_FORMAT_ARGB4444, 2, { 1, 0 }, false }, /* ARGB16-4444 */
+	{ OMAP_DSS_COLOR_XRGB16_1555, DRM_FORMAT_XRGB1555, 2, { 1, 0 }, false }, /* xRGB15-1555 */
+	{ OMAP_DSS_COLOR_ARGB16_1555, DRM_FORMAT_ARGB1555, 2, { 1, 0 }, false }, /* ARGB16-1555 */
 	/* 24bpp RGB: */
-	{ OMAP_DSS_COLOR_RGB24P,      DRM_FORMAT_RGB888,   {{3, 1}}, false }, /* RGB24-888 */
+	{ OMAP_DSS_COLOR_RGB24P,      DRM_FORMAT_RGB888,   3, { 1, 0 }, false }, /* 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, 4, { 1, 0 }, false }, /* RGBx24-8888 */
+	{ OMAP_DSS_COLOR_RGB24U,      DRM_FORMAT_XRGB8888, 4, { 1, 0 }, false }, /* xRGB24-8888 */
+	{ OMAP_DSS_COLOR_RGBA32,      DRM_FORMAT_RGBA8888, 4, { 1, 0 }, false }, /* RGBA32-8888 */
+	{ OMAP_DSS_COLOR_ARGB32,      DRM_FORMAT_ARGB8888, 4, { 1, 0 }, false }, /* 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,     1, { 1, 2 }, true },
+	{ OMAP_DSS_COLOR_YUV2,        DRM_FORMAT_YUYV,     2, { 1, 0 }, true },
+	{ OMAP_DSS_COLOR_UYVY,        DRM_FORMAT_UYVY,     2, { 1, 0 }, true },
 };
 
 /* convert from overlay's pixel formats bitmask to an array of fourcc's */
@@ -138,9 +136,8 @@ static uint32_t get_linear_addr(struct plane *plane,
 {
 	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->stride_bpp
+	       + y * plane->pitch / format->sub_y[n];
 
 	return plane->paddr + offset;
 }
@@ -237,7 +234,7 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 	}
 
 	/* convert to pixels: */
-	info->screen_width /= format->planes[0].stride_bpp;
+	info->screen_width /= format->stride_bpp;
 
 	if (format->dss_format == OMAP_DSS_COLOR_NV12) {
 		plane = &omap_fb->planes[1];
@@ -433,22 +430,22 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 		struct plane *plane = &omap_fb->planes[i];
 		int size, pitch = mode_cmd->pitches[i];
 
-		if (pitch < (mode_cmd->width * format->planes[i].stride_bpp)) {
+		if (pitch < (mode_cmd->width * format->stride_bpp)) {
 			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->stride_bpp);
 			ret = -EINVAL;
 			goto fail;
 		}
 
-		if (pitch % format->planes[i].stride_bpp != 0) {
+		if (pitch % format->stride_bpp != 0) {
 			dev_err(dev->dev,
 				"buffer pitch (%d bytes) is not a multiple of pixel size (%d bytes)\n",
-				pitch, format->planes[i].stride_bpp);
+				pitch, format->stride_bpp);
 			ret = -EINVAL;
 			goto fail;
 		}
 
-		size = pitch * mode_cmd->height / format->planes[i].sub_y;
+		size = pitch * mode_cmd->height / format->sub_y[i];
 
 		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",
-- 
2.7.3

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

[PATCH 03/23] drm: omapdrm: fb: Store number of planes in format structure

[Laurent Pinchart]

This replaces calls to drm_format_num_planes() by a simple field access
every time we need the number of planes for the frame buffer.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 2cc9f15fe439..ca7726e4d23e 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -31,6 +31,7 @@
 struct format {
 	enum omap_color_mode dss_format;
 	uint32_t pixel_format;
+	unsigned int num_planes;
 	int stride_bpp;           	/* this times width is stride */
 	int sub_y[2];			/* sub-sample in y dimension (per plane) */
 	bool yuv;
@@ -38,24 +39,24 @@ struct format {
 
 static const struct format formats[] = {
 	/* 16bpp [A]RGB: */
-	{ OMAP_DSS_COLOR_RGB16,       DRM_FORMAT_RGB565,   2, { 1, 0 }, false }, /* RGB16-565 */
-	{ OMAP_DSS_COLOR_RGB12U,      DRM_FORMAT_RGBX4444, 2, { 1, 0 }, false }, /* RGB12x-4444 */
-	{ OMAP_DSS_COLOR_RGBX16,      DRM_FORMAT_XRGB4444, 2, { 1, 0 }, false }, /* xRGB12-4444 */
-	{ OMAP_DSS_COLOR_RGBA16,      DRM_FORMAT_RGBA4444, 2, { 1, 0 }, false }, /* RGBA12-4444 */
-	{ OMAP_DSS_COLOR_ARGB16,      DRM_FORMAT_ARGB4444, 2, { 1, 0 }, false }, /* ARGB16-4444 */
-	{ OMAP_DSS_COLOR_XRGB16_1555, DRM_FORMAT_XRGB1555, 2, { 1, 0 }, false }, /* xRGB15-1555 */
-	{ OMAP_DSS_COLOR_ARGB16_1555, DRM_FORMAT_ARGB1555, 2, { 1, 0 }, false }, /* ARGB16-1555 */
+	{ OMAP_DSS_COLOR_RGB16,       DRM_FORMAT_RGB565,   1, 2, { 1, 0 }, false }, /* RGB16-565 */
+	{ OMAP_DSS_COLOR_RGB12U,      DRM_FORMAT_RGBX4444, 1, 2, { 1, 0 }, false }, /* RGB12x-4444 */
+	{ OMAP_DSS_COLOR_RGBX16,      DRM_FORMAT_XRGB4444, 1, 2, { 1, 0 }, false }, /* xRGB12-4444 */
+	{ OMAP_DSS_COLOR_RGBA16,      DRM_FORMAT_RGBA4444, 1, 2, { 1, 0 }, false }, /* RGBA12-4444 */
+	{ OMAP_DSS_COLOR_ARGB16,      DRM_FORMAT_ARGB4444, 1, 2, { 1, 0 }, false }, /* ARGB16-4444 */
+	{ OMAP_DSS_COLOR_XRGB16_1555, DRM_FORMAT_XRGB1555, 1, 2, { 1, 0 }, false }, /* xRGB15-1555 */
+	{ OMAP_DSS_COLOR_ARGB16_1555, DRM_FORMAT_ARGB1555, 1, 2, { 1, 0 }, false }, /* ARGB16-1555 */
 	/* 24bpp RGB: */
-	{ OMAP_DSS_COLOR_RGB24P,      DRM_FORMAT_RGB888,   3, { 1, 0 }, false }, /* RGB24-888 */
+	{ OMAP_DSS_COLOR_RGB24P,      DRM_FORMAT_RGB888,   1, 3, { 1, 0 }, false }, /* RGB24-888 */
 	/* 32bpp [A]RGB: */
-	{ OMAP_DSS_COLOR_RGBX32,      DRM_FORMAT_RGBX8888, 4, { 1, 0 }, false }, /* RGBx24-8888 */
-	{ OMAP_DSS_COLOR_RGB24U,      DRM_FORMAT_XRGB8888, 4, { 1, 0 }, false }, /* xRGB24-8888 */
-	{ OMAP_DSS_COLOR_RGBA32,      DRM_FORMAT_RGBA8888, 4, { 1, 0 }, false }, /* RGBA32-8888 */
-	{ OMAP_DSS_COLOR_ARGB32,      DRM_FORMAT_ARGB8888, 4, { 1, 0 }, false }, /* ARGB32-8888 */
+	{ OMAP_DSS_COLOR_RGBX32,      DRM_FORMAT_RGBX8888, 1, 4, { 1, 0 }, false }, /* RGBx24-8888 */
+	{ OMAP_DSS_COLOR_RGB24U,      DRM_FORMAT_XRGB8888, 1, 4, { 1, 0 }, false }, /* xRGB24-8888 */
+	{ OMAP_DSS_COLOR_RGBA32,      DRM_FORMAT_RGBA8888, 1, 4, { 1, 0 }, false }, /* RGBA32-8888 */
+	{ OMAP_DSS_COLOR_ARGB32,      DRM_FORMAT_ARGB8888, 1, 4, { 1, 0 }, false }, /* ARGB32-8888 */
 	/* YUV: */
-	{ OMAP_DSS_COLOR_NV12,        DRM_FORMAT_NV12,     1, { 1, 2 }, true },
-	{ OMAP_DSS_COLOR_YUV2,        DRM_FORMAT_YUYV,     2, { 1, 0 }, true },
-	{ OMAP_DSS_COLOR_UYVY,        DRM_FORMAT_UYVY,     2, { 1, 0 }, true },
+	{ OMAP_DSS_COLOR_NV12,        DRM_FORMAT_NV12,     2, 1, { 1, 2 }, true },
+	{ OMAP_DSS_COLOR_YUV2,        DRM_FORMAT_YUYV,     1, 2, { 1, 0 }, true },
+	{ OMAP_DSS_COLOR_UYVY,        DRM_FORMAT_UYVY,     1, 2, { 1, 0 }, true },
 };
 
 /* convert from overlay's pixel formats bitmask to an array of fourcc's */
@@ -103,13 +104,13 @@ static int omap_framebuffer_create_handle(struct drm_framebuffer *fb,
 static void omap_framebuffer_destroy(struct drm_framebuffer *fb)
 {
 	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
-	int i, n = drm_format_num_planes(fb->pixel_format);
+	int i;
 
 	DBG("destroy: FB ID: %d (%p)", fb->base.id, fb);
 
 	drm_framebuffer_cleanup(fb);
 
-	for (i = 0; i < n; i++) {
+	for (i = 0; i < omap_fb->format->num_planes; i++) {
 		struct plane *plane = &omap_fb->planes[i];
 		if (plane->bo)
 			drm_gem_object_unreference_unlocked(plane->bo);
@@ -255,7 +256,7 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 int omap_framebuffer_pin(struct drm_framebuffer *fb)
 {
 	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
-	int ret, i, n = drm_format_num_planes(fb->pixel_format);
+	int ret, i;
 
 	mutex_lock(&omap_fb->lock);
 
@@ -265,7 +266,7 @@ int omap_framebuffer_pin(struct drm_framebuffer *fb)
 		return 0;
 	}
 
-	for (i = 0; i < n; i++) {
+	for (i = 0; i < omap_fb->format->num_planes; i++) {
 		struct plane *plane = &omap_fb->planes[i];
 		ret = omap_gem_get_paddr(plane->bo, &plane->paddr, true);
 		if (ret)
@@ -295,7 +296,7 @@ fail:
 void omap_framebuffer_unpin(struct drm_framebuffer *fb)
 {
 	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
-	int i, n = drm_format_num_planes(fb->pixel_format);
+	int i;
 
 	mutex_lock(&omap_fb->lock);
 
@@ -306,7 +307,7 @@ void omap_framebuffer_unpin(struct drm_framebuffer *fb)
 		return;
 	}
 
-	for (i = 0; i < n; i++) {
+	for (i = 0; i < omap_fb->format->num_planes; i++) {
 		struct plane *plane = &omap_fb->planes[i];
 		omap_gem_put_paddr(plane->bo);
 		plane->paddr = 0;
@@ -318,8 +319,10 @@ void omap_framebuffer_unpin(struct drm_framebuffer *fb)
 struct drm_gem_object *omap_framebuffer_bo(struct drm_framebuffer *fb, int p)
 {
 	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
-	if (p >= drm_format_num_planes(fb->pixel_format))
+
+	if (p >= omap_fb->format->num_planes)
 		return NULL;
+
 	return omap_fb->planes[p].bo;
 }
 
@@ -354,12 +357,12 @@ struct drm_connector *omap_framebuffer_get_next_connector(
 void omap_framebuffer_describe(struct drm_framebuffer *fb, struct seq_file *m)
 {
 	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
-	int i, n = drm_format_num_planes(fb->pixel_format);
+	int i;
 
 	seq_printf(m, "fb: %dx%d@%4.4s\n", fb->width, fb->height,
 			(char *)&fb->pixel_format);
 
-	for (i = 0; i < n; i++) {
+	for (i = 0; i < omap_fb->format->num_planes; i++) {
 		struct plane *plane = &omap_fb->planes[i];
 		seq_printf(m, "   %d: offset=%d pitch=%d, obj: ",
 				i, plane->offset, plane->pitch);
@@ -396,7 +399,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 	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);
+	int ret, i;
 
 	DBG("create framebuffer: dev=%p, mode_cmd=%p (%dx%d@%4.4s)",
 			dev, mode_cmd, mode_cmd->width, mode_cmd->height,
@@ -426,7 +429,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 	omap_fb->format = 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];
 
-- 
2.7.3

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

[PATCH 04/23] 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>
---
 drivers/gpu/drm/omapdrm/omap_drv.h | 25 -------------------------
 drivers/gpu/drm/omapdrm/omap_fb.c  | 30 +++++++++++++++++++-----------
 2 files changed, 19 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 0fbe17d0ec6f..ed15b2dbe47a 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -254,29 +254,4 @@ static inline int align_pitch(int pitch, int width, int bpp)
 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_device *dev,
-		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(dev, 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 ca7726e4d23e..015b6a50c581 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -374,22 +374,30 @@ 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(dev, 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(dev, 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;
 	}
+
+	fb = omap_framebuffer_init(dev, mode_cmd, bos);
+	if (IS_ERR(fb))
+		goto error;
+
+	return fb;
+
+error:
+	while (--i > 0)
+		drm_gem_object_unreference_unlocked(bos[i]);
+
 	return fb;
 }
 
-- 
2.7.3

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

[PATCH 05/23] drm: omapdrm: fb: Simplify mode command checks when creating framebuffer

[Laurent Pinchart]

Checks can be simplified based on the requirement that pitches must be
identical for all planes.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 015b6a50c581..8629ba6ff9d7 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -407,6 +407,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 	struct omap_framebuffer *omap_fb = NULL;
 	struct drm_framebuffer *fb = NULL;
 	const struct format *format = NULL;
+	unsigned int pitch = mode_cmd->pitches[0];
 	int ret, i;
 
 	DBG("create framebuffer: dev=%p, mode_cmd=%p (%dx%d@%4.4s)",
@@ -437,38 +438,38 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 	omap_fb->format = format;
 	mutex_init(&omap_fb->lock);
 
-	for (i = 0; i < format->num_planes; i++) {
-		struct plane *plane = &omap_fb->planes[i];
-		int size, pitch = mode_cmd->pitches[i];
+	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->stride_bpp)) {
-			dev_err(dev->dev, "provided buffer pitch is too small! %d < %d\n",
-					pitch, mode_cmd->width * format->stride_bpp);
-			ret = -EINVAL;
-			goto fail;
-		}
+	if (pitch < mode_cmd->width * format->stride_bpp) {
+		dev_err(dev->dev,
+			"provided buffer pitch is too small! %u < %u\n",
+			pitch, mode_cmd->width * format->stride_bpp);
+		ret = -EINVAL;
+		goto fail;
+	}
 
-		if (pitch % format->stride_bpp != 0) {
-			dev_err(dev->dev,
-				"buffer pitch (%d bytes) is not a multiple of pixel size (%d bytes)\n",
-				pitch, format->stride_bpp);
-			ret = -EINVAL;
-			goto fail;
-		}
+	if (pitch % format->stride_bpp != 0) {
+		dev_err(dev->dev,
+			"buffer pitch (%u bytes) is not a multiple of pixel size (%u bytes)\n",
+			pitch, format->stride_bpp);
+		ret = -EINVAL;
+		goto fail;
+	}
+
+	for (i = 0; i < format->num_planes; i++) {
+		struct plane *plane = &omap_fb->planes[i];
+		unsigned int size;
 
 		size = pitch * mode_cmd->height / format->sub_y[i];
 
 		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]) {
 			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;
 		}
-- 
2.7.3

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

[PATCH 06/23] 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>
---
 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 8629ba6ff9d7..2edf86ab1fe1 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -422,8 +422,8 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 	}
 
 	if (!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;
 	}
@@ -439,13 +439,13 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 	mutex_init(&omap_fb->lock);
 
 	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->stride_bpp) {
-		dev_err(dev->dev,
+		dev_dbg(dev->dev,
 			"provided buffer pitch is too small! %u < %u\n",
 			pitch, mode_cmd->width * format->stride_bpp);
 		ret = -EINVAL;
@@ -453,7 +453,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 	}
 
 	if (pitch % format->stride_bpp != 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->stride_bpp);
 		ret = -EINVAL;
@@ -467,7 +467,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 		size = pitch * mode_cmd->height / format->sub_y[i];
 
 		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;
-- 
2.7.3

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

[PATCH 07/23] 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>
---
 drivers/gpu/drm/omapdrm/omap_drv.h   |  2 +-
 drivers/gpu/drm/omapdrm/omap_irq.c   | 49 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/omapdrm/omap_plane.c | 24 ------------------
 3 files changed, 47 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index ed15b2dbe47a..4ac629053ab1 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -103,7 +103,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..eb7133698d07 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,47 @@ 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(uint32_t 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;
+
+	if (!(irqstatus & mask))
+		return;
+
+	if (!__ratelimit(&_rs))
+		return;
+
+	drm_err("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 +241,8 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
 			drm_handle_vblank(dev, id);
 	}
 
+	omap_irq_fifo_underflow(irqstatus);
+
 	spin_lock_irqsave(&list_lock, flags);
 	list_for_each_entry_safe(handler, n, &priv->irq_list, node) {
 		if (handler->irqmask & irqstatus) {
@@ -233,6 +271,11 @@ int omap_drm_irq_install(struct drm_device *dev)
 
 	INIT_LIST_HEAD(&priv->irq_list);
 
+	priv->irq_mask = DISPC_IRQ_GFX_FIFO_UNDERFLOW
+		       | DISPC_IRQ_VID1_FIFO_UNDERFLOW
+		       | DISPC_IRQ_VID2_FIFO_UNDERFLOW
+		       | DISPC_IRQ_VID3_FIFO_UNDERFLOW;
+
 	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 93ee538a99f5..7b7d74072579 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;
 }
-- 
2.7.3

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

[PATCH 08/23] 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>
---
 drivers/gpu/drm/omapdrm/omap_crtc.c | 12 ++----------
 drivers/gpu/drm/omapdrm/omap_drv.h  |  1 +
 drivers/gpu/drm/omapdrm/omap_irq.c  |  9 +++++++++
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 075f2bb44867..6359d7933b93 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);
 
@@ -520,11 +517,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 4ac629053ab1..2d8fbdb2d39f 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -156,6 +156,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 eb7133698d07..a90e093f5f42 100644
--- a/drivers/gpu/drm/omapdrm/omap_irq.c
+++ b/drivers/gpu/drm/omapdrm/omap_irq.c
@@ -236,9 +236,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(irqstatus);
@@ -267,6 +271,8 @@ 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 i;
 	int ret;
 
 	INIT_LIST_HEAD(&priv->irq_list);
@@ -276,6 +282,9 @@ int omap_drm_irq_install(struct drm_device *dev)
 		       | DISPC_IRQ_VID2_FIFO_UNDERFLOW
 		       | DISPC_IRQ_VID3_FIFO_UNDERFLOW;
 
+	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();
-- 
2.7.3

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

[PATCH 09/23] 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>
---
 drivers/gpu/drm/omapdrm/omap_drv.h |  1 -
 drivers/gpu/drm/omapdrm/omap_irq.c | 29 +++++++++++------------------
 2 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 2d8fbdb2d39f..17dd3b98fc1a 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -104,7 +104,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 a90e093f5f42..499da6e2c5a4 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)
 {
@@ -219,6 +213,14 @@ static void omap_irq_fifo_underflow(uint32_t irqstatus)
 	pr_cont("(0x%08x)\n", irqstatus);
 }
 
+static void omap_irq_error_handler(uint32_t irqstatus)
+{
+	if (!(irqstatus & DISPC_IRQ_OCP_ERR))
+		return;
+
+	DRM_ERROR("errors: %08x\n", irqstatus);
+}
+
 static irqreturn_t omap_irq_handler(int irq, void *arg)
 {
 	struct drm_device *dev = (struct drm_device *) arg;
@@ -245,6 +247,7 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
 			omap_crtc_error_irq(crtc, irqstatus);
 	}
 
+	omap_irq_error_handler(irqstatus);
 	omap_irq_fifo_underflow(irqstatus);
 
 	spin_lock_irqsave(&list_lock, flags);
@@ -270,14 +273,14 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
 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 i;
 	int ret;
 
 	INIT_LIST_HEAD(&priv->irq_list);
 
-	priv->irq_mask = DISPC_IRQ_GFX_FIFO_UNDERFLOW
+	priv->irq_mask = DISPC_IRQ_OCP_ERR
+		       | DISPC_IRQ_GFX_FIFO_UNDERFLOW
 		       | DISPC_IRQ_VID1_FIFO_UNDERFLOW
 		       | DISPC_IRQ_VID2_FIFO_UNDERFLOW
 		       | DISPC_IRQ_VID3_FIFO_UNDERFLOW;
@@ -293,16 +296,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;
-- 
2.7.3

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

[PATCH 10/23] drm: omapdrm: Use atomic state instead of local device state

[Laurent Pinchart]

Instead of conditioning planes update based on the hardware device
state, use the CRTC state stored in the atomic state. This reduces the
dependency from the DRM layer to the DSS layer.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 6359d7933b93..4c56d6a68390 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -381,18 +381,23 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
 
 	WARN_ON(omap_crtc->vblank_irq.registered);
 
-	if (dispc_mgr_is_enabled(omap_crtc->channel)) {
+	/*
+	 * Only flush the CRTC if it is currently active. CRTCs that require a
+	 * mode set are disabled prior plane updates and enabled afterwards.
+	 * They are thus not active, regardless of what their state report.
+	 */
+	if (!crtc->state->active || drm_atomic_crtc_needs_modeset(crtc->state))
+		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,
-- 
2.7.3

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

[PATCH 11/23] 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 f83608b69e68..f18a6911923a 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -2887,7 +2887,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 829232ad8c81..e6f67d76ec95 100644
--- a/drivers/gpu/drm/omapdrm/dss/output.c
+++ b/drivers/gpu/drm/omapdrm/dss/output.c
@@ -218,12 +218,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 4c56d6a68390..35d59c385d38 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -139,9 +139,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
-- 
2.7.3

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

[PATCH 12/23] 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 35d59c385d38..f723db1096b1 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -42,6 +42,7 @@ struct omap_crtc {
 
 	bool pending;
 	wait_queue_head_t pending_wait;
+	struct drm_pending_vblank_event *event;
 };
 
 /* -----------------------------------------------------------------------------
@@ -257,11 +258,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;
@@ -367,12 +372,23 @@ static void omap_crtc_mode_set_nofb(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);
 
-- 
2.7.3

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

[PATCH 13/23] 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 f723db1096b1..ee744ab6608c 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -68,6 +68,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);
@@ -77,7 +90,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));
 }
 
@@ -294,6 +307,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;
@@ -302,10 +316,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);
@@ -337,10 +351,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);
 
@@ -404,10 +418,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);
-- 
2.7.3

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

[PATCH 14/23] 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 ee744ab6608c..3265cd990acf 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 pending;
@@ -302,25 +300,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);
 
@@ -338,8 +335,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);
@@ -351,14 +346,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)
@@ -406,8 +400,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);
-
 	/*
 	 * Only flush the CRTC if it is currently active. CRTCs that require a
 	 * mode set are disabled prior plane updates and enabled afterwards.
@@ -418,13 +410,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,
@@ -546,9 +539,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 17dd3b98fc1a..d5f27d117b6a 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -156,6 +156,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 499da6e2c5a4..b2e3fd78f7e0 100644
--- a/drivers/gpu/drm/omapdrm/omap_irq.c
+++ b/drivers/gpu/drm/omapdrm/omap_irq.c
@@ -240,8 +240,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);
-- 
2.7.3

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

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

[Laurent Pinchart]

The functions are not used outside of their compilation unit, make them
static.

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

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index d5f27d117b6a..c8c0e207c823 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -59,7 +59,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:
@@ -128,10 +128,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 b2e3fd78f7e0..45062a17efdf 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();
 }
 
@@ -95,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);
@@ -256,7 +243,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);
 		}
 	}
-- 
2.7.3

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

[PATCH 16/23] 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 45062a17efdf..0b28db014569 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 {
-- 
2.7.3

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

[PATCH 17/23] 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 3265cd990acf..e077d36016c2 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -47,13 +47,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 c8c0e207c823..29f2cc2ce555 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -248,7 +248,6 @@ static inline int align_pitch(int pitch, int width, int bpp)
 }
 
 /* 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 0b28db014569..f7c507cc104b 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
@@ -223,7 +228,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);
 		}
-- 
2.7.3

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

[PATCH 18/23] 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 29f2cc2ce555..e3a4a09d8f92 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -49,19 +49,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;
@@ -102,8 +89,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 f7c507cc104b..8a5616a5936b 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)
@@ -213,7 +192,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;
@@ -241,12 +220,9 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
 	omap_irq_fifo_underflow(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);
 
@@ -267,7 +243,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
 		       | DISPC_IRQ_GFX_FIFO_UNDERFLOW
-- 
2.7.3

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

[PATCH 19/23] 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 | 38 ++++++++++++++++++++------------------
 2 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index e3a4a09d8f92..e30b77c42f63 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -89,7 +89,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 8a5616a5936b..feeaaf78a7c7 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(uint32_t irqstatus)
@@ -219,12 +220,12 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
 	omap_irq_error_handler(irqstatus);
 	omap_irq_fifo_underflow(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;
 }
@@ -243,6 +244,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
-- 
2.7.3

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

[PATCH 20/23] drm: omapdrm: panel-lgphilips-lb035q02: Remove unused backlight GPIO

[Laurent Pinchart]

The backlight GPIO was supported with platform data only. Now that the
driver only supports DT, the backlight GPIO is never initialized. Remove
it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../drm/omapdrm/displays/panel-lgphilips-lb035q02.c   | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/displays/panel-lgphilips-lb035q02.c b/drivers/gpu/drm/omapdrm/displays/panel-lgphilips-lb035q02.c
index 458f77bc473d..9b52ccf22764 100644
--- a/drivers/gpu/drm/omapdrm/displays/panel-lgphilips-lb035q02.c
+++ b/drivers/gpu/drm/omapdrm/displays/panel-lgphilips-lb035q02.c
@@ -50,9 +50,6 @@ struct panel_drv_data {
 
 	struct omap_video_timings videomode;
 
-	/* used for non-DT boot, to be removed */
-	int backlight_gpio;
-
 	struct gpio_desc *enable_gpio;
 };
 
@@ -170,9 +167,6 @@ static int lb035q02_enable(struct omap_dss_device *dssdev)
 	if (ddata->enable_gpio)
 		gpiod_set_value_cansleep(ddata->enable_gpio, 1);
 
-	if (gpio_is_valid(ddata->backlight_gpio))
-		gpio_set_value_cansleep(ddata->backlight_gpio, 1);
-
 	dssdev->state = OMAP_DSS_DISPLAY_ACTIVE;
 
 	return 0;
@@ -189,9 +183,6 @@ static void lb035q02_disable(struct omap_dss_device *dssdev)
 	if (ddata->enable_gpio)
 		gpiod_set_value_cansleep(ddata->enable_gpio, 0);
 
-	if (gpio_is_valid(ddata->backlight_gpio))
-		gpio_set_value_cansleep(ddata->backlight_gpio, 0);
-
 	in->ops.dpi->disable(in);
 
 	dssdev->state = OMAP_DSS_DISPLAY_DISABLED;
@@ -255,8 +246,6 @@ static int lb035q02_probe_of(struct spi_device *spi)
 
 	ddata->enable_gpio = gpio;
 
-	ddata->backlight_gpio = -ENOENT;
-
 	in = omapdss_of_find_source_for_first_ep(node);
 	if (IS_ERR(in)) {
 		dev_err(&spi->dev, "failed to find video source\n");
@@ -289,13 +278,6 @@ static int lb035q02_panel_spi_probe(struct spi_device *spi)
 	if (r)
 		return r;
 
-	if (gpio_is_valid(ddata->backlight_gpio)) {
-		r = devm_gpio_request_one(&spi->dev, ddata->backlight_gpio,
-				GPIOF_OUT_INIT_LOW, "panel backlight");
-		if (r)
-			goto err_gpio;
-	}
-
 	ddata->videomode = lb035q02_timings;
 
 	dssdev = &ddata->dssdev;
@@ -315,7 +297,6 @@ static int lb035q02_panel_spi_probe(struct spi_device *spi)
 	return 0;
 
 err_reg:
-err_gpio:
 	omap_dss_put_device(ddata->in);
 	return r;
 }
-- 
2.7.3

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

[PATCH 21/23] drm: omapdrm: Remove unused omap_framebuffer_bo function

[Laurent Pinchart]

The function is never used, remove it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.h |  1 -
 drivers/gpu/drm/omapdrm/omap_fb.c  | 10 ----------
 2 files changed, 11 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index e30b77c42f63..bcb9520315ab 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -168,7 +168,6 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
 		struct drm_file *file, const struct drm_mode_fb_cmd2 *mode_cmd);
 struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos);
-struct drm_gem_object *omap_framebuffer_bo(struct drm_framebuffer *fb, int p);
 int omap_framebuffer_pin(struct drm_framebuffer *fb);
 void omap_framebuffer_unpin(struct drm_framebuffer *fb);
 void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 2edf86ab1fe1..ec2a7cd1790e 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -316,16 +316,6 @@ void omap_framebuffer_unpin(struct drm_framebuffer *fb)
 	mutex_unlock(&omap_fb->lock);
 }
 
-struct drm_gem_object *omap_framebuffer_bo(struct drm_framebuffer *fb, int p)
-{
-	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
-
-	if (p >= omap_fb->format->num_planes)
-		return NULL;
-
-	return omap_fb->planes[p].bo;
-}
-
 /* iterate thru all the connectors, returning ones that are attached
  * to the same fb..
  */
-- 
2.7.3

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

[PATCH 22/23] drm: omapdrm: Remove unused omap_gem_tiled_size function

[Laurent Pinchart]

The function is never used, remove it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.h |  1 -
 drivers/gpu/drm/omapdrm/omap_gem.c | 12 ------------
 2 files changed, 13 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index bcb9520315ab..779628f9ed7d 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -215,7 +215,6 @@ int omap_gem_rotated_paddr(struct drm_gem_object *obj, uint32_t orient,
 		int x, int y, dma_addr_t *paddr);
 uint64_t omap_gem_mmap_offset(struct drm_gem_object *obj);
 size_t omap_gem_mmap_size(struct drm_gem_object *obj);
-int omap_gem_tiled_size(struct drm_gem_object *obj, uint16_t *w, uint16_t *h);
 int omap_gem_tiled_stride(struct drm_gem_object *obj, uint32_t orient);
 
 struct dma_buf *omap_gem_prime_export(struct drm_device *dev,
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 907154f5b67c..9022cbbf26fb 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -382,18 +382,6 @@ size_t omap_gem_mmap_size(struct drm_gem_object *obj)
 	return size;
 }
 
-/* get tiled size, returns -EINVAL if not tiled buffer */
-int omap_gem_tiled_size(struct drm_gem_object *obj, uint16_t *w, uint16_t *h)
-{
-	struct omap_gem_object *omap_obj = to_omap_bo(obj);
-	if (omap_obj->flags & OMAP_BO_TILED) {
-		*w = omap_obj->width;
-		*h = omap_obj->height;
-		return 0;
-	}
-	return -EINVAL;
-}
-
 /* -----------------------------------------------------------------------------
  * Fault Handling
  */
-- 
2.7.3

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

[PATCH 23/23] drm: omapdrm: Remove buffer synchronization support

[Laurent Pinchart]

The omapdrm driver uses a custom API to synchronize with the SGX GPU.
This is unusable as such in the mainline kernel as the API is only
partially implemented and requires additional out-of-tree patches.
Furthermore, as no SGX driver is available in the mainline kernel, the
API can't be considered as a stable mainline API.

Remove buffer synchronization support to prepare for its replacement by
an implementation based on standard fences and reservation objects.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c |  49 ---------
 drivers/gpu/drm/omapdrm/omap_drv.h |   5 -
 drivers/gpu/drm/omapdrm/omap_gem.c | 215 -------------------------------------
 include/uapi/drm/omap_drm.h        |  26 -----
 4 files changed, 295 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 80398a684cae..68f434ad9696 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -552,53 +552,6 @@ static int ioctl_gem_new(struct drm_device *dev, void *data,
 				   &args->handle);
 }
 
-static int ioctl_gem_cpu_prep(struct drm_device *dev, void *data,
-		struct drm_file *file_priv)
-{
-	struct drm_omap_gem_cpu_prep *args = data;
-	struct drm_gem_object *obj;
-	int ret;
-
-	VERB("%p:%p: handle=%d, op=%x", dev, file_priv, args->handle, args->op);
-
-	obj = drm_gem_object_lookup(dev, file_priv, args->handle);
-	if (!obj)
-		return -ENOENT;
-
-	ret = omap_gem_op_sync(obj, args->op);
-
-	if (!ret)
-		ret = omap_gem_op_start(obj, args->op);
-
-	drm_gem_object_unreference_unlocked(obj);
-
-	return ret;
-}
-
-static int ioctl_gem_cpu_fini(struct drm_device *dev, void *data,
-		struct drm_file *file_priv)
-{
-	struct drm_omap_gem_cpu_fini *args = data;
-	struct drm_gem_object *obj;
-	int ret;
-
-	VERB("%p:%p: handle=%d", dev, file_priv, args->handle);
-
-	obj = drm_gem_object_lookup(dev, file_priv, args->handle);
-	if (!obj)
-		return -ENOENT;
-
-	/* XXX flushy, flushy */
-	ret = 0;
-
-	if (!ret)
-		ret = omap_gem_op_finish(obj, args->op);
-
-	drm_gem_object_unreference_unlocked(obj);
-
-	return ret;
-}
-
 static int ioctl_gem_info(struct drm_device *dev, void *data,
 		struct drm_file *file_priv)
 {
@@ -624,8 +577,6 @@ static const struct drm_ioctl_desc ioctls[DRM_COMMAND_END - DRM_COMMAND_BASE] =
 	DRM_IOCTL_DEF_DRV(OMAP_GET_PARAM, ioctl_get_param, DRM_AUTH),
 	DRM_IOCTL_DEF_DRV(OMAP_SET_PARAM, ioctl_set_param, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF_DRV(OMAP_GEM_NEW, ioctl_gem_new, DRM_AUTH),
-	DRM_IOCTL_DEF_DRV(OMAP_GEM_CPU_PREP, ioctl_gem_cpu_prep, DRM_AUTH),
-	DRM_IOCTL_DEF_DRV(OMAP_GEM_CPU_FINI, ioctl_gem_cpu_fini, DRM_AUTH),
 	DRM_IOCTL_DEF_DRV(OMAP_GEM_INFO, ioctl_gem_info, DRM_AUTH),
 };
 
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 779628f9ed7d..2aa198bec662 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -195,11 +195,6 @@ int omap_gem_mmap(struct file *filp, struct vm_area_struct *vma);
 int omap_gem_mmap_obj(struct drm_gem_object *obj,
 		struct vm_area_struct *vma);
 int omap_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf);
-int omap_gem_op_start(struct drm_gem_object *obj, enum omap_gem_op op);
-int omap_gem_op_finish(struct drm_gem_object *obj, enum omap_gem_op op);
-int omap_gem_op_sync(struct drm_gem_object *obj, enum omap_gem_op op);
-int omap_gem_op_async(struct drm_gem_object *obj, enum omap_gem_op op,
-		void (*fxn)(void *arg), void *arg);
 int omap_gem_roll(struct drm_gem_object *obj, uint32_t roll);
 void omap_gem_cpu_sync(struct drm_gem_object *obj, int pgoff);
 void omap_gem_dma_sync(struct drm_gem_object *obj,
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 9022cbbf26fb..d5a577037e94 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -100,19 +100,6 @@ struct omap_gem_object {
 	 * Virtual address, if mapped.
 	 */
 	void *vaddr;
-
-	/**
-	 * sync-object allocated on demand (if needed)
-	 *
-	 * Per-buffer sync-object for tracking pending and completed hw/dma
-	 * read and write operations.
-	 */
-	struct {
-		uint32_t write_pending;
-		uint32_t write_complete;
-		uint32_t read_pending;
-		uint32_t read_complete;
-	} *sync;
 };
 
 #define to_omap_bo(x) container_of(x, struct omap_gem_object, base)
@@ -1071,206 +1058,6 @@ void omap_gem_describe_objects(struct list_head *list, struct seq_file *m)
 #endif
 
 /* -----------------------------------------------------------------------------
- * Buffer Synchronization
- */
-
-static DEFINE_SPINLOCK(sync_lock);
-
-struct omap_gem_sync_waiter {
-	struct list_head list;
-	struct omap_gem_object *omap_obj;
-	enum omap_gem_op op;
-	uint32_t read_target, write_target;
-	/* notify called w/ sync_lock held */
-	void (*notify)(void *arg);
-	void *arg;
-};
-
-/* list of omap_gem_sync_waiter.. the notify fxn gets called back when
- * the read and/or write target count is achieved which can call a user
- * callback (ex. to kick 3d and/or 2d), wakeup blocked task (prep for
- * cpu access), etc.
- */
-static LIST_HEAD(waiters);
-
-static inline bool is_waiting(struct omap_gem_sync_waiter *waiter)
-{
-	struct omap_gem_object *omap_obj = waiter->omap_obj;
-	if ((waiter->op & OMAP_GEM_READ) &&
-			(omap_obj->sync->write_complete < waiter->write_target))
-		return true;
-	if ((waiter->op & OMAP_GEM_WRITE) &&
-			(omap_obj->sync->read_complete < waiter->read_target))
-		return true;
-	return false;
-}
-
-/* macro for sync debug.. */
-#define SYNCDBG 0
-#define SYNC(fmt, ...) do { if (SYNCDBG) \
-		printk(KERN_ERR "%s:%d: "fmt"\n", \
-				__func__, __LINE__, ##__VA_ARGS__); \
-	} while (0)
-
-
-static void sync_op_update(void)
-{
-	struct omap_gem_sync_waiter *waiter, *n;
-	list_for_each_entry_safe(waiter, n, &waiters, list) {
-		if (!is_waiting(waiter)) {
-			list_del(&waiter->list);
-			SYNC("notify: %p", waiter);
-			waiter->notify(waiter->arg);
-			kfree(waiter);
-		}
-	}
-}
-
-static inline int sync_op(struct drm_gem_object *obj,
-		enum omap_gem_op op, bool start)
-{
-	struct omap_gem_object *omap_obj = to_omap_bo(obj);
-	int ret = 0;
-
-	spin_lock(&sync_lock);
-
-	if (!omap_obj->sync) {
-		omap_obj->sync = kzalloc(sizeof(*omap_obj->sync), GFP_ATOMIC);
-		if (!omap_obj->sync) {
-			ret = -ENOMEM;
-			goto unlock;
-		}
-	}
-
-	if (start) {
-		if (op & OMAP_GEM_READ)
-			omap_obj->sync->read_pending++;
-		if (op & OMAP_GEM_WRITE)
-			omap_obj->sync->write_pending++;
-	} else {
-		if (op & OMAP_GEM_READ)
-			omap_obj->sync->read_complete++;
-		if (op & OMAP_GEM_WRITE)
-			omap_obj->sync->write_complete++;
-		sync_op_update();
-	}
-
-unlock:
-	spin_unlock(&sync_lock);
-
-	return ret;
-}
-
-/* mark the start of read and/or write operation */
-int omap_gem_op_start(struct drm_gem_object *obj, enum omap_gem_op op)
-{
-	return sync_op(obj, op, true);
-}
-
-int omap_gem_op_finish(struct drm_gem_object *obj, enum omap_gem_op op)
-{
-	return sync_op(obj, op, false);
-}
-
-static DECLARE_WAIT_QUEUE_HEAD(sync_event);
-
-static void sync_notify(void *arg)
-{
-	struct task_struct **waiter_task = arg;
-	*waiter_task = NULL;
-	wake_up_all(&sync_event);
-}
-
-int omap_gem_op_sync(struct drm_gem_object *obj, enum omap_gem_op op)
-{
-	struct omap_gem_object *omap_obj = to_omap_bo(obj);
-	int ret = 0;
-	if (omap_obj->sync) {
-		struct task_struct *waiter_task = current;
-		struct omap_gem_sync_waiter *waiter =
-				kzalloc(sizeof(*waiter), GFP_KERNEL);
-
-		if (!waiter)
-			return -ENOMEM;
-
-		waiter->omap_obj = omap_obj;
-		waiter->op = op;
-		waiter->read_target = omap_obj->sync->read_pending;
-		waiter->write_target = omap_obj->sync->write_pending;
-		waiter->notify = sync_notify;
-		waiter->arg = &waiter_task;
-
-		spin_lock(&sync_lock);
-		if (is_waiting(waiter)) {
-			SYNC("waited: %p", waiter);
-			list_add_tail(&waiter->list, &waiters);
-			spin_unlock(&sync_lock);
-			ret = wait_event_interruptible(sync_event,
-					(waiter_task == NULL));
-			spin_lock(&sync_lock);
-			if (waiter_task) {
-				SYNC("interrupted: %p", waiter);
-				/* we were interrupted */
-				list_del(&waiter->list);
-				waiter_task = NULL;
-			} else {
-				/* freed in sync_op_update() */
-				waiter = NULL;
-			}
-		}
-		spin_unlock(&sync_lock);
-		kfree(waiter);
-	}
-	return ret;
-}
-
-/* call fxn(arg), either synchronously or asynchronously if the op
- * is currently blocked..  fxn() can be called from any context
- *
- * (TODO for now fxn is called back from whichever context calls
- * omap_gem_op_finish().. but this could be better defined later
- * if needed)
- *
- * TODO more code in common w/ _sync()..
- */
-int omap_gem_op_async(struct drm_gem_object *obj, enum omap_gem_op op,
-		void (*fxn)(void *arg), void *arg)
-{
-	struct omap_gem_object *omap_obj = to_omap_bo(obj);
-	if (omap_obj->sync) {
-		struct omap_gem_sync_waiter *waiter =
-				kzalloc(sizeof(*waiter), GFP_ATOMIC);
-
-		if (!waiter)
-			return -ENOMEM;
-
-		waiter->omap_obj = omap_obj;
-		waiter->op = op;
-		waiter->read_target = omap_obj->sync->read_pending;
-		waiter->write_target = omap_obj->sync->write_pending;
-		waiter->notify = fxn;
-		waiter->arg = arg;
-
-		spin_lock(&sync_lock);
-		if (is_waiting(waiter)) {
-			SYNC("waited: %p", waiter);
-			list_add_tail(&waiter->list, &waiters);
-			spin_unlock(&sync_lock);
-			return 0;
-		}
-
-		spin_unlock(&sync_lock);
-
-		kfree(waiter);
-	}
-
-	/* no waiting.. */
-	fxn(arg);
-
-	return 0;
-}
-
-/* -----------------------------------------------------------------------------
  * Constructor & Destructor
  */
 
@@ -1309,8 +1096,6 @@ void omap_gem_free_object(struct drm_gem_object *obj)
 		drm_prime_gem_destroy(obj, omap_obj->sgt);
 	}
 
-	kfree(omap_obj->sync);
-
 	drm_gem_object_release(obj);
 
 	kfree(omap_obj);
diff --git a/include/uapi/drm/omap_drm.h b/include/uapi/drm/omap_drm.h
index 38a3bd847e15..c34114bc0e51 100644
--- a/include/uapi/drm/omap_drm.h
+++ b/include/uapi/drm/omap_drm.h
@@ -63,28 +63,6 @@ struct drm_omap_gem_new {
 	uint32_t __pad;
 };
 
-/* mask of operations: */
-enum omap_gem_op {
-	OMAP_GEM_READ = 0x01,
-	OMAP_GEM_WRITE = 0x02,
-};
-
-struct drm_omap_gem_cpu_prep {
-	uint32_t handle;		/* buffer handle (in) */
-	uint32_t op;			/* mask of omap_gem_op (in) */
-};
-
-struct drm_omap_gem_cpu_fini {
-	uint32_t handle;		/* buffer handle (in) */
-	uint32_t op;			/* mask of omap_gem_op (in) */
-	/* TODO maybe here we pass down info about what regions are touched
-	 * by sw so we can be clever about cache ops?  For now a placeholder,
-	 * set to zero and we just do full buffer flush..
-	 */
-	uint32_t nregions;
-	uint32_t __pad;
-};
-
 struct drm_omap_gem_info {
 	uint32_t handle;		/* buffer handle (in) */
 	uint32_t pad;
@@ -102,16 +80,12 @@ struct drm_omap_gem_info {
 #define DRM_OMAP_GET_PARAM		0x00
 #define DRM_OMAP_SET_PARAM		0x01
 #define DRM_OMAP_GEM_NEW		0x03
-#define DRM_OMAP_GEM_CPU_PREP		0x04
-#define DRM_OMAP_GEM_CPU_FINI		0x05
 #define DRM_OMAP_GEM_INFO		0x06
 #define DRM_OMAP_NUM_IOCTLS		0x07
 
 #define DRM_IOCTL_OMAP_GET_PARAM	DRM_IOWR(DRM_COMMAND_BASE + DRM_OMAP_GET_PARAM, struct drm_omap_param)
 #define DRM_IOCTL_OMAP_SET_PARAM	DRM_IOW (DRM_COMMAND_BASE + DRM_OMAP_SET_PARAM, struct drm_omap_param)
 #define DRM_IOCTL_OMAP_GEM_NEW		DRM_IOWR(DRM_COMMAND_BASE + DRM_OMAP_GEM_NEW, struct drm_omap_gem_new)
-#define DRM_IOCTL_OMAP_GEM_CPU_PREP	DRM_IOW (DRM_COMMAND_BASE + DRM_OMAP_GEM_CPU_PREP, struct drm_omap_gem_cpu_prep)
-#define DRM_IOCTL_OMAP_GEM_CPU_FINI	DRM_IOW (DRM_COMMAND_BASE + DRM_OMAP_GEM_CPU_FINI, struct drm_omap_gem_cpu_fini)
 #define DRM_IOCTL_OMAP_GEM_INFO		DRM_IOWR(DRM_COMMAND_BASE + DRM_OMAP_GEM_INFO, struct drm_omap_gem_info)
 
 #endif /* __OMAP_DRM_H__ */
-- 
2.7.3

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

Re: [PATCH 02/23] drm: omapdrm: fb: Don't store format BPP for each plane

[Tomi Valkeinen]

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

Hi Laurent,

On 26/04/16 23:35, Laurent Pinchart wrote:
> The number of bits per pixel is identical for all planes, don't store
> multiple copies.

That's not true, as with NV12, Y has 8 bits per pixel and UV has 16 bits
per pixel. But as the subsampling is precalculated into the stride_bpp
(is it bytes or bits? bpp always confuses me =), the 'stride_bpp' ends
up being same for both planes.

To be honest, I'd rather go into more complex struct than simpler one.
The current one is already confusing, I think, and your version is too.
The main issue is that the sub_x is encoded into the stride_bpp. In
kmsxx I used this format:

{ PixelFormat::NV12, { 2, { { 8, 1, 1, }, { 8, 2, 2 } }, } },

The first number is the number of planes, and for each plane, bitspp,
xsub and ysub. It's more verbose, but (I think) easier to understand.

 Tomi


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

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

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

Re: [PATCH 02/23] drm: omapdrm: fb: Don't store format BPP for each plane

[Rob Clark]

On Mon, May 2, 2016 at 11:43 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi Laurent,
>
> On 26/04/16 23:35, Laurent Pinchart wrote:
>> The number of bits per pixel is identical for all planes, don't store
>> multiple copies.
>
> That's not true, as with NV12, Y has 8 bits per pixel and UV has 16 bits
> per pixel. But as the subsampling is precalculated into the stride_bpp
> (is it bytes or bits? bpp always confuses me =), the 'stride_bpp' ends
> up being same for both planes.
>
> To be honest, I'd rather go into more complex struct than simpler one.
> The current one is already confusing, I think, and your version is too.
> The main issue is that the sub_x is encoded into the stride_bpp. In
> kmsxx I used this format:
>
> { PixelFormat::NV12, { 2, { { 8, 1, 1, }, { 8, 2, 2 } }, } },
>
> The first number is the number of planes, and for each plane, bitspp,
> xsub and ysub. It's more verbose, but (I think) easier to understand.

fwiw, I guess a lot of data from that table could these days be
replaced w/ some of the drm format helpers
(drm_format_num_planes()/drm_format_plane_cpp()/drm_format_{horz,vert}_chroma_subsampling()/etc)

BR,
-R

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

Re: [PATCH 02/23] drm: omapdrm: fb: Don't store format BPP for each plane

[Tomi Valkeinen]

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


On 03/05/16 00:01, Rob Clark wrote:
> On Mon, May 2, 2016 at 11:43 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> Hi Laurent,
>>
>> On 26/04/16 23:35, Laurent Pinchart wrote:
>>> The number of bits per pixel is identical for all planes, don't store
>>> multiple copies.
>>
>> That's not true, as with NV12, Y has 8 bits per pixel and UV has 16 bits
>> per pixel. But as the subsampling is precalculated into the stride_bpp
>> (is it bytes or bits? bpp always confuses me =), the 'stride_bpp' ends
>> up being same for both planes.
>>
>> To be honest, I'd rather go into more complex struct than simpler one.
>> The current one is already confusing, I think, and your version is too.
>> The main issue is that the sub_x is encoded into the stride_bpp. In
>> kmsxx I used this format:
>>
>> { PixelFormat::NV12, { 2, { { 8, 1, 1, }, { 8, 2, 2 } }, } },
>>
>> The first number is the number of planes, and for each plane, bitspp,
>> xsub and ysub. It's more verbose, but (I think) easier to understand.
> 
> fwiw, I guess a lot of data from that table could these days be
> replaced w/ some of the drm format helpers
> (drm_format_num_planes()/drm_format_plane_cpp()/drm_format_{horz,vert}_chroma_subsampling()/etc)

Indeed, I think we can remove all but the DRM -> DSS format mapping.

Btw, what is "cpp" short from?

 Tomi


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

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

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

Re: [PATCH 02/23] drm: omapdrm: fb: Don't store format BPP for each plane

[Ville Syrjälä]

On Tue, May 03, 2016 at 09:08:06AM +0300, Tomi Valkeinen wrote:
> 
> On 03/05/16 00:01, Rob Clark wrote:
> > On Mon, May 2, 2016 at 11:43 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >> Hi Laurent,
> >>
> >> On 26/04/16 23:35, Laurent Pinchart wrote:
> >>> The number of bits per pixel is identical for all planes, don't store
> >>> multiple copies.
> >>
> >> That's not true, as with NV12, Y has 8 bits per pixel and UV has 16 bits
> >> per pixel. But as the subsampling is precalculated into the stride_bpp
> >> (is it bytes or bits? bpp always confuses me =), the 'stride_bpp' ends
> >> up being same for both planes.
> >>
> >> To be honest, I'd rather go into more complex struct than simpler one.
> >> The current one is already confusing, I think, and your version is too.
> >> The main issue is that the sub_x is encoded into the stride_bpp. In
> >> kmsxx I used this format:
> >>
> >> { PixelFormat::NV12, { 2, { { 8, 1, 1, }, { 8, 2, 2 } }, } },
> >>
> >> The first number is the number of planes, and for each plane, bitspp,
> >> xsub and ysub. It's more verbose, but (I think) easier to understand.
> > 
> > fwiw, I guess a lot of data from that table could these days be
> > replaced w/ some of the drm format helpers
> > (drm_format_num_planes()/drm_format_plane_cpp()/drm_format_{horz,vert}_chroma_subsampling()/etc)
> 
> Indeed, I think we can remove all but the DRM -> DSS format mapping.
> 
> Btw, what is "cpp" short from?

Bytes per pixel. The 'c' is silent :P

PS.
Actually it's "chars per pixel"

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/23] drm: omapdrm: fb: Simplify objects lookup when creating framebuffer

[Tomi Valkeinen]

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



On 26/04/16 23:35, Laurent Pinchart wrote:
> 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>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.h | 25 -------------------------
>  drivers/gpu/drm/omapdrm/omap_fb.c  | 30 +++++++++++++++++++-----------
>  2 files changed, 19 insertions(+), 36 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 06/23] drm: omapdrm: fb: Turn framebuffer creation error messages into debug

[Tomi Valkeinen]

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


On 26/04/16 23:35, Laurent Pinchart wrote:
> 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>
> ---
>  drivers/gpu/drm/omapdrm/omap_fb.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 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 07/23] drm: omapdrm: Handle FIFO underflow IRQs internally

[Tomi Valkeinen]

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

On 26/04/16 23:35, 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>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.h   |  2 +-
>  drivers/gpu/drm/omapdrm/omap_irq.c   | 49 +++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/omapdrm/omap_plane.c | 24 ------------------
>  3 files changed, 47 insertions(+), 28 deletions(-)

> @@ -233,6 +271,11 @@ int omap_drm_irq_install(struct drm_device *dev)
>  
>  	INIT_LIST_HEAD(&priv->irq_list);
>  
> +	priv->irq_mask = DISPC_IRQ_GFX_FIFO_UNDERFLOW
> +		       | DISPC_IRQ_VID1_FIFO_UNDERFLOW
> +		       | DISPC_IRQ_VID2_FIFO_UNDERFLOW
> +		       | DISPC_IRQ_VID3_FIFO_UNDERFLOW;

Not all VID overlays exist on all DSS versions, so we shouldn't register
irqs that don't exist on the HW.

Also, I do like it that we deal with crtc or plane interrupts in
omap_crtc or omap_plane. Would similar approach here work as you use in
the following patches, i.e. just call underflow-handler func in
omap_plane.c directly, instead of using the registration mechanism?

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

[Tomi Valkeinen]

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

On 26/04/16 23:35, Laurent Pinchart wrote:
> Checks can be simplified based on the requirement that pitches must be
> identical for all planes.

Your code also presumes there are only 1 or 2 planes, I think that
should be mentioned too.

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_fb.c | 51 ++++++++++++++++++++-------------------
>  1 file changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
> index 015b6a50c581..8629ba6ff9d7 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -407,6 +407,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>  	struct omap_framebuffer *omap_fb = NULL;
>  	struct drm_framebuffer *fb = NULL;
>  	const struct format *format = NULL;
> +	unsigned int pitch = mode_cmd->pitches[0];
>  	int ret, i;
>  
>  	DBG("create framebuffer: dev=%p, mode_cmd=%p (%dx%d@%4.4s)",
> @@ -437,38 +438,38 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>  	omap_fb->format = format;
>  	mutex_init(&omap_fb->lock);
>  
> -	for (i = 0; i < format->num_planes; i++) {
> -		struct plane *plane = &omap_fb->planes[i];
> -		int size, pitch = mode_cmd->pitches[i];
> +	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->stride_bpp)) {
> -			dev_err(dev->dev, "provided buffer pitch is too small! %d < %d\n",
> -					pitch, mode_cmd->width * format->stride_bpp);
> -			ret = -EINVAL;
> -			goto fail;
> -		}
> +	if (pitch < mode_cmd->width * format->stride_bpp) {
> +		dev_err(dev->dev,
> +			"provided buffer pitch is too small! %u < %u\n",
> +			pitch, mode_cmd->width * format->stride_bpp);
> +		ret = -EINVAL;
> +		goto fail;
> +	}
>  
> -		if (pitch % format->stride_bpp != 0) {
> -			dev_err(dev->dev,
> -				"buffer pitch (%d bytes) is not a multiple of pixel size (%d bytes)\n",
> -				pitch, format->stride_bpp);
> -			ret = -EINVAL;
> -			goto fail;
> -		}
> +	if (pitch % format->stride_bpp != 0) {
> +		dev_err(dev->dev,
> +			"buffer pitch (%u bytes) is not a multiple of pixel size (%u bytes)\n",
> +			pitch, format->stride_bpp);
> +		ret = -EINVAL;
> +		goto fail;
> +	}
> +
> +	for (i = 0; i < format->num_planes; i++) {
> +		struct plane *plane = &omap_fb->planes[i];
> +		unsigned int size;
>  
>  		size = pitch * mode_cmd->height / format->sub_y[i];
>  
>  		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]) {
>  			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;
>  		}

So, hmm... I think the current code is actually not right, even if it
works right: I think the DSS's requirement is actually that the width in
pixels is the same between planes, not stride.

At the moment the only two plane format, NV12, has the same pixel size
for both planes, but an upcoming DSS might have a format that has a
separate 8 byte A plane. I don't know if that ever realizes or if we
want to support the mode, but after thinking about this, it makes more
sense that the pixel width has to be the same between planes, not the
byte width.

 Tomi


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

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

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

Re: [PATCH 02/23] drm: omapdrm: fb: Don't store format BPP for each plane

[Laurent Pinchart]

Hi Tomi,

On Monday 02 May 2016 18:43:15 Tomi Valkeinen wrote:
> Hi Laurent,
> 
> On 26/04/16 23:35, Laurent Pinchart wrote:
> > The number of bits per pixel is identical for all planes, don't store
> > multiple copies.
> 
> That's not true, as with NV12, Y has 8 bits per pixel and UV has 16 bits
> per pixel. But as the subsampling is precalculated into the stride_bpp
> (is it bytes or bits? bpp always confuses me =), the 'stride_bpp' ends
> up being same for both planes.

It's true if you average it over a full line. I can clarify this in the commit 
message.

> To be honest, I'd rather go into more complex struct than simpler one.
> The current one is already confusing, I think, and your version is too.
> The main issue is that the sub_x is encoded into the stride_bpp. In
> kmsxx I used this format:
> 
> { PixelFormat::NV12, { 2, { { 8, 1, 1, }, { 8, 2, 2 } }, } },
> 
> The first number is the number of planes, and for each plane, bitspp,
> xsub and ysub. It's more verbose, but (I think) easier to understand.

I'm fine with that assuming we'd have a use for it :-) Shouldn't we aim for a 
simpler structure to save memory by only storing the information we need, 
compared to a more verbose structure that duplicates information or stores 
data that we don't need ?

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 02/23] drm: omapdrm: fb: Don't store format BPP for each plane

[Laurent Pinchart]

Hi Rob,

On Monday 02 May 2016 17:01:23 Rob Clark wrote:
> On Mon, May 2, 2016 at 11:43 AM, Tomi Valkeinen wrote:
> > On 26/04/16 23:35, Laurent Pinchart wrote:
> >> The number of bits per pixel is identical for all planes, don't store
> >> multiple copies.
> > 
> > That's not true, as with NV12, Y has 8 bits per pixel and UV has 16 bits
> > per pixel. But as the subsampling is precalculated into the stride_bpp
> > (is it bytes or bits? bpp always confuses me =), the 'stride_bpp' ends
> > up being same for both planes.
> > 
> > To be honest, I'd rather go into more complex struct than simpler one.
> > The current one is already confusing, I think, and your version is too.
> > The main issue is that the sub_x is encoded into the stride_bpp. In
> > kmsxx I used this format:
> > 
> > { PixelFormat::NV12, { 2, { { 8, 1, 1, }, { 8, 2, 2 } }, } },
> > 
> > The first number is the number of planes, and for each plane, bitspp,
> > xsub and ysub. It's more verbose, but (I think) easier to understand.
> 
> fwiw, I guess a lot of data from that table could these days be
> replaced w/ some of the drm format helpers
> (drm_format_num_planes()/drm_format_plane_cpp()/drm_format_{horz,vert}_chrom
> a_subsampling()/etc)

I don't like those helpers as they're inefficient. Drivers often need to know 
multiple pieces of information about a format, and the API forces look-ups for 
every piece of information needed.

Would it make sense to add a drm_format_info() function that returns a pointer 
to a data structure that describes the format, and reimplement the existing 
helpers on top of that ?

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 02/23] drm: omapdrm: fb: Don't store format BPP for each plane

[Tomi Valkeinen]

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

On 09/05/16 23:57, Laurent Pinchart wrote:

>> fwiw, I guess a lot of data from that table could these days be
>> replaced w/ some of the drm format helpers
>> (drm_format_num_planes()/drm_format_plane_cpp()/drm_format_{horz,vert}_chrom
>> a_subsampling()/etc)
> 
> I don't like those helpers as they're inefficient. Drivers often need to know 
> multiple pieces of information about a format, and the API forces look-ups for 
> every piece of information needed.
> 
> Would it make sense to add a drm_format_info() function that returns a pointer 
> to a data structure that describes the format, and reimplement the existing 
> helpers on top of that ?

I do like that idea.

A table implementation might possibly be faster even with the current
API. Those big switch-cases compile into a big pile of
if-else-if-else-ifs, whereas a for loop would compile into just a few
instructions. But just guessing here =).

In theory, the table could be sorted, making the lookup much faster, but
I'm not sure if that's worth the effort.

 Tomi


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

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

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

Re: [PATCH 02/23] drm: omapdrm: fb: Don't store format BPP for each plane

[Tomi Valkeinen]

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

On 09/05/16 23:55, Laurent Pinchart wrote:

>> To be honest, I'd rather go into more complex struct than simpler one.
>> The current one is already confusing, I think, and your version is too.
>> The main issue is that the sub_x is encoded into the stride_bpp. In
>> kmsxx I used this format:
>>
>> { PixelFormat::NV12, { 2, { { 8, 1, 1, }, { 8, 2, 2 } }, } },
>>
>> The first number is the number of planes, and for each plane, bitspp,
>> xsub and ysub. It's more verbose, but (I think) easier to understand.
> 
> I'm fine with that assuming we'd have a use for it :-) Shouldn't we aim for a 
> simpler structure to save memory by only storing the information we need, 
> compared to a more verbose structure that duplicates information or stores 
> data that we don't need ?

Well, my take is: if we save a few bytes by obfuscating the code, it's
not worth it. "Obfuscating" is, of course, relative, but a "stride_bpp"
is confusing to me. If it's a common term, I'm fine with using it and I
just need to learn it =).

Also, I like tables like these to be somewhat generic, so that if and
when we get new color formats, the whole table doesn't need to be rewritten.

That said, the kmsxx format above is not super clear either, as the
bitspp for the second plane is 8, i.e. it also has the sub_x encoded...
I don't remember why I made it like that.

So, in the end, all I know is that describing pixel formats well is not
easy =).

 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 20/23] drm: omapdrm: panel-lgphilips-lb035q02: Remove unused backlight GPIO

[Tomi Valkeinen]

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



On 26/04/16 23:35, Laurent Pinchart wrote:
> The backlight GPIO was supported with platform data only. Now that the
> driver only supports DT, the backlight GPIO is never initialized. Remove
> it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../drm/omapdrm/displays/panel-lgphilips-lb035q02.c   | 19 -------------------
>  1 file changed, 19 deletions(-)

Thanks, I've picked this one up for 4.8.

 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 22/23] drm: omapdrm: Remove unused omap_gem_tiled_size function

[Tomi Valkeinen]

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


On 26/04/16 23:35, Laurent Pinchart wrote:
> The function is never used, remove it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.h |  1 -
>  drivers/gpu/drm/omapdrm/omap_gem.c | 12 ------------
>  2 files changed, 13 deletions(-)

Thanks, I've picked this one up for 4.8.

 Tomi

Ps. I really don't like it if the patch description continues the text
in the subject... The description should be independent of the subject.

 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 21/23] drm: omapdrm: Remove unused omap_framebuffer_bo function

[Tomi Valkeinen]

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

On 26/04/16 23:35, Laurent Pinchart wrote:
> The function is never used, remove it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.h |  1 -
>  drivers/gpu/drm/omapdrm/omap_fb.c  | 10 ----------
>  2 files changed, 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
> index e30b77c42f63..bcb9520315ab 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -168,7 +168,6 @@ struct drm_framebuffer *omap_framebuffer_create(struct drm_device *dev,
>  		struct drm_file *file, const struct drm_mode_fb_cmd2 *mode_cmd);
>  struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
>  		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos);
> -struct drm_gem_object *omap_framebuffer_bo(struct drm_framebuffer *fb, int p);
>  int omap_framebuffer_pin(struct drm_framebuffer *fb);
>  void omap_framebuffer_unpin(struct drm_framebuffer *fb);
>  void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
> diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
> index 2edf86ab1fe1..ec2a7cd1790e 100644
> --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> @@ -316,16 +316,6 @@ void omap_framebuffer_unpin(struct drm_framebuffer *fb)
>  	mutex_unlock(&omap_fb->lock);
>  }
>  
> -struct drm_gem_object *omap_framebuffer_bo(struct drm_framebuffer *fb, int p)
> -{
> -	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
> -
> -	if (p >= omap_fb->format->num_planes)
> -		return NULL;
> -
> -	return omap_fb->planes[p].bo;
> -}
> -
>  /* iterate thru all the connectors, returning ones that are attached
>   * to the same fb..
>   */
> 

I would have picked this up, but it doesn't apply without the previous
patches in the series. The conflict is trivial, though. Do you want me
to resolve it and apply, or do you want to keep this in your series?

 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 08/23] drm: omapdrm: Handle CRTC error IRQs directly

[Tomi Valkeinen]

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


On 26/04/16 23:35, Laurent Pinchart wrote:
> 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>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 12 ++----------
>  drivers/gpu/drm/omapdrm/omap_drv.h  |  1 +
>  drivers/gpu/drm/omapdrm/omap_irq.c  |  9 +++++++++
>  3 files changed, 12 insertions(+), 10 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 09/23] drm: omapdrm: Handle OCP error IRQ directly

[Tomi Valkeinen]

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

On 26/04/16 23:35, 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>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.h |  1 -
>  drivers/gpu/drm/omapdrm/omap_irq.c | 29 +++++++++++------------------
>  2 files changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
> index 2d8fbdb2d39f..17dd3b98fc1a 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -104,7 +104,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 a90e093f5f42..499da6e2c5a4 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)
>  {
> @@ -219,6 +213,14 @@ static void omap_irq_fifo_underflow(uint32_t irqstatus)
>  	pr_cont("(0x%08x)\n", irqstatus);
>  }
>  
> +static void omap_irq_error_handler(uint32_t irqstatus)

I think the function should mention "ocp_error".

> +{
> +	if (!(irqstatus & DISPC_IRQ_OCP_ERR))
> +		return;
> +
> +	DRM_ERROR("errors: %08x\n", irqstatus);

Now we have a separate print for OCP error, so we could instead of
printing hex numbers, print "OCP error".

> +}
> +
>  static irqreturn_t omap_irq_handler(int irq, void *arg)
>  {
>  	struct drm_device *dev = (struct drm_device *) arg;
> @@ -245,6 +247,7 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
>  			omap_crtc_error_irq(crtc, irqstatus);
>  	}
>  
> +	omap_irq_error_handler(irqstatus);
>  	omap_irq_fifo_underflow(irqstatus);
>  
>  	spin_lock_irqsave(&list_lock, flags);
> @@ -270,14 +273,14 @@ static irqreturn_t omap_irq_handler(int irq, void *arg)
>  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 i;
>  	int ret;
>  
>  	INIT_LIST_HEAD(&priv->irq_list);
>  
> -	priv->irq_mask = DISPC_IRQ_GFX_FIFO_UNDERFLOW
> +	priv->irq_mask = DISPC_IRQ_OCP_ERR
> +		       | DISPC_IRQ_GFX_FIFO_UNDERFLOW
>  		       | DISPC_IRQ_VID1_FIFO_UNDERFLOW
>  		       | DISPC_IRQ_VID2_FIFO_UNDERFLOW
>  		       | DISPC_IRQ_VID3_FIFO_UNDERFLOW;
> @@ -293,16 +296,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);

This makes me wonder is the previous patch correct... It doesn't ignore
the SYNC_LOST_DIGIT. Oh, but is this ignore only for the error handler
that only prints. Ah, confusing =).

 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 10/23] drm: omapdrm: Use atomic state instead of local device state

[Tomi Valkeinen]

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


On 26/04/16 23:35, Laurent Pinchart wrote:
> Instead of conditioning planes update based on the hardware device
> state, use the CRTC state stored in the atomic state. This reduces the
> dependency from the DRM layer to the DSS layer.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 6359d7933b93..4c56d6a68390 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -381,18 +381,23 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
>  
>  	WARN_ON(omap_crtc->vblank_irq.registered);
>  
> -	if (dispc_mgr_is_enabled(omap_crtc->channel)) {
> +	/*
> +	 * Only flush the CRTC if it is currently active. CRTCs that require a
> +	 * mode set are disabled prior plane updates and enabled afterwards.
> +	 * They are thus not active, regardless of what their state report.
> +	 */
> +	if (!crtc->state->active || drm_atomic_crtc_needs_modeset(crtc->state))
> +		return;

If the DRM core doesn't track whether a CRTC HW is enabled at the
moment, maybe omapdrm should? I guess the above works, but that if()
makes me a bit uneasy, as it's not really obvious, and the logic behind
it could possibly change later...

A "if (crtc->is_hw_enabled)" would be much more readable.

 Tomi


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

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

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

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

[Tomi Valkeinen]

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


On 26/04/16 23:35, 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.

Shouldn't omapdrm know if the CRTC is enabled or not, and avoid
double-enable/disable by just looking at its internal state?

If so, we could remove dispc_mgr_is_enabled() call as you do, and add a
WARN_ON() to omapdss if the mgr is already enabled/disabled to catch
bugs in omapdrm.

 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 10/23] drm: omapdrm: Use atomic state instead of local device state

[Daniel Vetter]

On Tue, May 10, 2016 at 04:24:11PM +0300, Tomi Valkeinen wrote:
> 
> On 26/04/16 23:35, Laurent Pinchart wrote:
> > Instead of conditioning planes update based on the hardware device
> > state, use the CRTC state stored in the atomic state. This reduces the
> > dependency from the DRM layer to the DSS layer.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/omapdrm/omap_crtc.c | 23 ++++++++++++++---------
> >  1 file changed, 14 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> > index 6359d7933b93..4c56d6a68390 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> > @@ -381,18 +381,23 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
> >  
> >  	WARN_ON(omap_crtc->vblank_irq.registered);
> >  
> > -	if (dispc_mgr_is_enabled(omap_crtc->channel)) {
> > +	/*
> > +	 * Only flush the CRTC if it is currently active. CRTCs that require a
> > +	 * mode set are disabled prior plane updates and enabled afterwards.
> > +	 * They are thus not active, regardless of what their state report.
> > +	 */
> > +	if (!crtc->state->active || drm_atomic_crtc_needs_modeset(crtc->state))
> > +		return;
> 
> If the DRM core doesn't track whether a CRTC HW is enabled at the
> moment, maybe omapdrm should? I guess the above works, but that if()
> makes me a bit uneasy, as it's not really obvious, and the logic behind
> it could possibly change later...
> 
> A "if (crtc->is_hw_enabled)" would be much more readable.

Look at the active_only paramater of the planes_commit helper. That should
do exactly what you want.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Tue, May 10, 2016 at 04:28:22PM +0300, Tomi Valkeinen wrote:
> 
> On 26/04/16 23:35, 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.
> 
> Shouldn't omapdrm know if the CRTC is enabled or not, and avoid
> double-enable/disable by just looking at its internal state?
> 
> If so, we could remove dispc_mgr_is_enabled() call as you do, and add a
> WARN_ON() to omapdss if the mgr is already enabled/disabled to catch
> bugs in omapdrm.

Yeah, atomic helpers guarantees you that it'll never screw this up and
disable/enable twice. The only tricky bit to consider is boot-up, where
the firmware might have left something enabled, but by default the reset
helpers assume everything is off, and just reset sw state to all off.

Either patch up that state to match (you only need to set very few things,
no need to recover all the details about the mode), or manually shut
things down. But you need to make sure that at driver load sw matches hw
state.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 17/23] drm: omapdrm: Make pipe2vbl function static

[Tomi Valkeinen]

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


On 26/04/16 23:35, 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 3265cd990acf..e077d36016c2 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -47,13 +47,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 c8c0e207c823..29f2cc2ce555 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -248,7 +248,6 @@ static inline int align_pitch(int pitch, int width, int bpp)
>  }
>  
>  /* 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 0b28db014569..f7c507cc104b 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
> @@ -223,7 +228,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);
>  		}

You move pipe2vbl() to omap_irq.c, and then you change omap_irq to not
use 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 15/23] drm: omapdrm: Don't expose the omap_irq_(un)register() functions

[Tomi Valkeinen]

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

On 26/04/16 23:35, Laurent Pinchart wrote:
> The functions are not used outside of their compilation unit, make them
> static.

The patch doesn't seem to match the description. If I'm not mistaken,
the patch is doing: remove __omap_irq_* funcs, remove parameter from the
irq callback, make funcs static.

Combining everything into one patch makes it difficult to review,
especially if the description doesn't explain what's done.

 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 23/23] drm: omapdrm: Remove buffer synchronization support

[Tomi Valkeinen]

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

On 26/04/16 23:35, Laurent Pinchart wrote:
> The omapdrm driver uses a custom API to synchronize with the SGX GPU.
> This is unusable as such in the mainline kernel as the API is only
> partially implemented and requires additional out-of-tree patches.
> Furthermore, as no SGX driver is available in the mainline kernel, the
> API can't be considered as a stable mainline API.
> 
> Remove buffer synchronization support to prepare for its replacement by
> an implementation based on standard fences and reservation objects.

I thought one thing the OMAP_GEM_CPU_PREP/FINI ioctls were supposed to
do it cache flushing, if the buffer is OMAP_BO_CACHED. I don't think
that works, though, which might be considered as an omapdrm bug.

Also, if an app is using the prep/fini ioctls at the moment, even if it
doesn't exactly require flushing (or any kind of synchronization), after
this patch the app might fail. So even if we remove all the code behind,
perhaps we should leave no-op ioctls.

 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 07/23] drm: omapdrm: Handle FIFO underflow IRQs internally

[Laurent Pinchart]

Hi Tomi,

On Monday 09 May 2016 17:42:43 Tomi Valkeinen wrote:
> On 26/04/16 23:35, 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>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_drv.h   |  2 +-
> >  drivers/gpu/drm/omapdrm/omap_irq.c   | 49 ++++++++++++++++++++++++++++---
> >  drivers/gpu/drm/omapdrm/omap_plane.c | 24 ------------------
> >  3 files changed, 47 insertions(+), 28 deletions(-)
> > 
> > @@ -233,6 +271,11 @@ int omap_drm_irq_install(struct drm_device *dev)
> > 
> >  	INIT_LIST_HEAD(&priv->irq_list);
> > 
> > +	priv->irq_mask = DISPC_IRQ_GFX_FIFO_UNDERFLOW
> > +		       | DISPC_IRQ_VID1_FIFO_UNDERFLOW
> > +		       | DISPC_IRQ_VID2_FIFO_UNDERFLOW
> > +		       | DISPC_IRQ_VID3_FIFO_UNDERFLOW;
> 
> Not all VID overlays exist on all DSS versions, so we shouldn't register
> irqs that don't exist on the HW.

Good point, I'll fix that.

> Also, I do like it that we deal with crtc or plane interrupts in
> omap_crtc or omap_plane. Would similar approach here work as you use in
> the following patches, i.e. just call underflow-handler func in
> omap_plane.c directly, instead of using the registration mechanism?

I can do that, but given that all we do is just printing error messages, it 
sounds a bit overkill. I propose moving FIFO underflow IRQ handling to the 
CRTC and plane code later when/if we need to perform more work in the 
handlers.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 05/23] drm: omapdrm: fb: Simplify mode command checks when creating framebuffer

[Laurent Pinchart]

Hi Tomi,

On Monday 09 May 2016 18:15:10 Tomi Valkeinen wrote:
> On 26/04/16 23:35, Laurent Pinchart wrote:
> > Checks can be simplified based on the requirement that pitches must be
> > identical for all planes.
> 
> Your code also presumes there are only 1 or 2 planes, I think that
> should be mentioned too.

I'll update the commit message and add a comment to the code.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_fb.c | 51 +++++++++++++++++-----------------
> >  1 file changed, 26 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c
> > b/drivers/gpu/drm/omapdrm/omap_fb.c index 015b6a50c581..8629ba6ff9d7
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> > @@ -407,6 +407,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct
> > drm_device *dev,> 
> >  	struct omap_framebuffer *omap_fb = NULL;
> >  	struct drm_framebuffer *fb = NULL;
> >  	const struct format *format = NULL;
> > +	unsigned int pitch = mode_cmd->pitches[0];
> >  	int ret, i;
> >  	
> >  	DBG("create framebuffer: dev=%p, mode_cmd=%p (%dx%d@%4.4s)",
> > @@ -437,38 +438,38 @@ struct drm_framebuffer *omap_framebuffer_init(struct
> > drm_device *dev,> 
> >  	omap_fb->format = format;
> >  	mutex_init(&omap_fb->lock);
> > 
> > -	for (i = 0; i < format->num_planes; i++) {
> > -		struct plane *plane = &omap_fb->planes[i];
> > -		int size, pitch = mode_cmd->pitches[i];
> > +	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->stride_bpp)) {
> > -			dev_err(dev->dev, "provided buffer pitch is too small! %d < 
%d\n",
> > -					pitch, mode_cmd->width * format->stride_bpp);
> > -			ret = -EINVAL;
> > -			goto fail;
> > -		}
> > +	if (pitch < mode_cmd->width * format->stride_bpp) {
> > +		dev_err(dev->dev,
> > +			"provided buffer pitch is too small! %u < %u\n",
> > +			pitch, mode_cmd->width * format->stride_bpp);
> > +		ret = -EINVAL;
> > +		goto fail;
> > +	}
> > 
> > -		if (pitch % format->stride_bpp != 0) {
> > -			dev_err(dev->dev,
> > -				"buffer pitch (%d bytes) is not a multiple of pixel size (%d
> > bytes)\n", -				pitch, format->stride_bpp);
> > -			ret = -EINVAL;
> > -			goto fail;
> > -		}
> > +	if (pitch % format->stride_bpp != 0) {
> > +		dev_err(dev->dev,
> > +			"buffer pitch (%u bytes) is not a multiple of pixel size (%u
> > bytes)\n",
> > +			pitch, format->stride_bpp);
> > +		ret = -EINVAL;
> > +		goto fail;
> > +	}
> > +
> > +	for (i = 0; i < format->num_planes; i++) {
> > +		struct plane *plane = &omap_fb->planes[i];
> > +		unsigned int size;
> > 
> >  		size = pitch * mode_cmd->height / format->sub_y[i];
> >  		
> >  		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]) {
> > 
> >  			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;
> >  		
> >  		}
> 
> So, hmm... I think the current code is actually not right, even if it
> works right: I think the DSS's requirement is actually that the width in
> pixels is the same between planes, not stride.
> 
> At the moment the only two plane format, NV12, has the same pixel size
> for both planes, but an upcoming DSS might have a format that has a
> separate 8 byte A plane. I don't know if that ever realizes or if we
> want to support the mode, but after thinking about this, it makes more
> sense that the pixel width has to be the same between planes, not the
> byte width.

Given that the code is currently wrong anyway, how about implementing correct 
generic support for formats with more than two planes when hardware supporting 
those formats appear ?

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 21/23] drm: omapdrm: Remove unused omap_framebuffer_bo function

[Laurent Pinchart]

Hi Tomi,

On Tuesday 10 May 2016 14:00:51 Tomi Valkeinen wrote:
> On 26/04/16 23:35, Laurent Pinchart wrote:
> > The function is never used, remove it.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_drv.h |  1 -
> >  drivers/gpu/drm/omapdrm/omap_fb.c  | 10 ----------
> >  2 files changed, 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h
> > b/drivers/gpu/drm/omapdrm/omap_drv.h index e30b77c42f63..bcb9520315ab
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> > +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> > @@ -168,7 +168,6 @@ struct drm_framebuffer *omap_framebuffer_create(struct
> > drm_device *dev,
> >  		struct drm_file *file, const struct drm_mode_fb_cmd2 *mode_cmd);
> >  struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
> >  		const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object
> >  		**bos);
> > 
> > -struct drm_gem_object *omap_framebuffer_bo(struct drm_framebuffer *fb,
> > int p);
> >  int omap_framebuffer_pin(struct drm_framebuffer *fb);
> >  void omap_framebuffer_unpin(struct drm_framebuffer *fb);
> >  void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
> > diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c
> > b/drivers/gpu/drm/omapdrm/omap_fb.c index 2edf86ab1fe1..ec2a7cd1790e
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_fb.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_fb.c
> > @@ -316,16 +316,6 @@ void omap_framebuffer_unpin(struct drm_framebuffer
> > *fb)
> >  	mutex_unlock(&omap_fb->lock);
> >  }
> > 
> > -struct drm_gem_object *omap_framebuffer_bo(struct drm_framebuffer *fb,
> > int p)
> > -{
> > -	struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb);
> > -
> > -	if (p >= omap_fb->format->num_planes)
> > -		return NULL;
> > -
> > -	return omap_fb->planes[p].bo;
> > -}
> > -
> >  /* iterate thru all the connectors, returning ones that are attached
> >   * to the same fb..
> >   */
> 
> I would have picked this up, but it doesn't apply without the previous
> patches in the series. The conflict is trivial, though. Do you want me
> to resolve it and apply, or do you want to keep this in your series?

Please do as is most convenient for you. I'll rebase my series on top of your 
tree anyway.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 09/23] drm: omapdrm: Handle OCP error IRQ directly

[Laurent Pinchart]

Hi Tomi,

On Tuesday 10 May 2016 16:10:59 Tomi Valkeinen wrote:
> On 26/04/16 23:35, 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>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_drv.h |  1 -
> >  drivers/gpu/drm/omapdrm/omap_irq.c | 29 +++++++++++------------------
> >  2 files changed, 11 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h
> > b/drivers/gpu/drm/omapdrm/omap_drv.h index 2d8fbdb2d39f..17dd3b98fc1a
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> > +++ b/drivers/gpu/drm/omapdrm/omap_drv.h

[snip]

> > diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c
> > b/drivers/gpu/drm/omapdrm/omap_irq.c index a90e093f5f42..499da6e2c5a4
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_irq.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_irq.c

[snip]

> > @@ -219,6 +213,14 @@ static void omap_irq_fifo_underflow(uint32_t
> > irqstatus)> 
> >  	pr_cont("(0x%08x)\n", irqstatus);
> >  }
> > 
> > +static void omap_irq_error_handler(uint32_t irqstatus)
> 
> I think the function should mention "ocp_error".
> 
> > +{
> > +	if (!(irqstatus & DISPC_IRQ_OCP_ERR))
> > +		return;
> > +
> > +	DRM_ERROR("errors: %08x\n", irqstatus);
> 
> Now we have a separate print for OCP error, so we could instead of
> printing hex numbers, print "OCP error".

I'll fix those two issues in v2.

> > +}

[snip]

> > @@ -293,16 +296,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);
> 
> This makes me wonder is the previous patch correct... It doesn't ignore
> the SYNC_LOST_DIGIT. Oh, but is this ignore only for the error handler
> that only prints. Ah, confusing =).

The good news is that this patch removes the confusing code :-)

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 17/23] drm: omapdrm: Make pipe2vbl function static

[Laurent Pinchart]

Hi Tomi,

On Wednesday 11 May 2016 14:01:26 Tomi Valkeinen wrote:
> On 26/04/16 23:35, 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(-)

[snip]

> > diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c
> > b/drivers/gpu/drm/omapdrm/omap_irq.c index 0b28db014569..f7c507cc104b
> > 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
> > @@ -223,7 +228,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);
> >  		}
> 
> You move pipe2vbl() to omap_irq.c, and then you change omap_irq to not
> use pipe2vbl()?

I do here because we already have the channel number. The pipe2vbl function is 
still used in two other locations in this file.

-- 
Regards,

Laurent Pinchart

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

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

[Laurent Pinchart]

Hi Tomi,

On Wednesday 11 May 2016 14:05:48 Tomi Valkeinen wrote:
> On 26/04/16 23:35, Laurent Pinchart wrote:
> > The functions are not used outside of their compilation unit, make them
> > static.
> 
> The patch doesn't seem to match the description. If I'm not mistaken,
> the patch is doing: remove __omap_irq_* funcs, remove parameter from the
> irq callback, make funcs static.
>
> Combining everything into one patch makes it difficult to review,
> especially if the description doesn't explain what's done.

The purpose of the patch is to stop exposing the omap_irq_(un)register() 
functions (as you describe by "make funcs static"). As further cleanup it then 
merges __omap_irq_(un)register() with omap_irq_(un)register(), and simplifies 
the IRQ handler API by removing an unused parameter.

I'll split this in two patches, one that stops exposing functions and merges 
them, and another one that removes the parameter.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 10/23] drm: omapdrm: Use atomic state instead of local device state

[Laurent Pinchart]

Hi Tomi and Daniel,

On Wednesday 11 May 2016 09:37:56 Daniel Vetter wrote:
> On Tue, May 10, 2016 at 04:24:11PM +0300, Tomi Valkeinen wrote:
> > On 26/04/16 23:35, Laurent Pinchart wrote:
> >> Instead of conditioning planes update based on the hardware device
> >> state, use the CRTC state stored in the atomic state. This reduces the
> >> dependency from the DRM layer to the DSS layer.
> >> 
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/omapdrm/omap_crtc.c | 23 ++++++++++++++---------
> >>  1 file changed, 14 insertions(+), 9 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 6359d7933b93..4c56d6a68390
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >> @@ -381,18 +381,23 @@ static void omap_crtc_atomic_flush(struct drm_crtc
> >> *crtc,
> >>  	WARN_ON(omap_crtc->vblank_irq.registered);
> >> 
> >> -	if (dispc_mgr_is_enabled(omap_crtc->channel)) {
> >> +	/*
> >> +	 * Only flush the CRTC if it is currently active. CRTCs that require a
> >> +	 * mode set are disabled prior plane updates and enabled afterwards.
> >> +	 * They are thus not active, regardless of what their state report.
> >> +	 */
> >> +	if (!crtc->state->active ||
> >> drm_atomic_crtc_needs_modeset(crtc->state))
> >> +		return;
> > 
> > If the DRM core doesn't track whether a CRTC HW is enabled at the
> > moment, maybe omapdrm should? I guess the above works, but that if()
> > makes me a bit uneasy, as it's not really obvious, and the logic behind
> > it could possibly change later...
> > 
> > A "if (crtc->is_hw_enabled)" would be much more readable.

The whole point of this patch is to remove local state and rely on DRM core 
state, so I'd like to avoid that if possible.

> Look at the active_only paramater of the planes_commit helper. That should
> do exactly what you want.

The drm_atomic_helper_commit_planes() helper has this check

        if (active_only && !crtc->state->active)
                continue;

        funcs->atomic_flush(crtc, old_crtc_state);

I could thus remove the !crtc->state->active check, but the 
drm_atomic_crtc_needs_modeset() check would need to stay, wouldn't it ? When 
CRTCs go through a modeset they are disabled prior to plane updates, but their 
state active status can still be true. Or should that be fixed in the core ?

-- 
Regards,

Laurent Pinchart

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

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

[Laurent Pinchart]

Hi Daniel,

On Wednesday 11 May 2016 09:40:14 Daniel Vetter wrote:
> On Tue, May 10, 2016 at 04:28:22PM +0300, Tomi Valkeinen wrote:
> > On 26/04/16 23:35, 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.
> > 
> > Shouldn't omapdrm know if the CRTC is enabled or not, and avoid
> > double-enable/disable by just looking at its internal state?
> > 
> > If so, we could remove dispc_mgr_is_enabled() call as you do, and add a
> > WARN_ON() to omapdss if the mgr is already enabled/disabled to catch
> > bugs in omapdrm.
> 
> Yeah, atomic helpers guarantees you that it'll never screw this up and
> disable/enable twice.

It's more complex than that. The omapdrm driver has a complicated call stack, 
going through omapdss subdrivers that make verification difficult at the 
moment. The CRTCs are not enabled/disabled directly in response to the DRM 
core's calls to the CRTC enable and disable operations, but through the 
omapdss driver from the encoder helper enable and disable operations. That's 
something I'd like to clean up, but I'd rather not mess with it right now.

> The only tricky bit to consider is boot-up, where the firmware might have
> left something enabled, but by default the reset helpers assume everything
> is off, and just reset sw state to all off.
> 
> Either patch up that state to match (you only need to set very few things,
> no need to recover all the details about the mode), or manually shut
> things down. But you need to make sure that at driver load sw matches hw
> state.
> -Daniel

-- 
Regards,

Laurent Pinchart

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

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

[Laurent Pinchart]

Hi Tomi,

On Tuesday 10 May 2016 16:28:22 Tomi Valkeinen wrote:
> On 26/04/16 23:35, 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.
> 
> Shouldn't omapdrm know if the CRTC is enabled or not, and avoid
> double-enable/disable by just looking at its internal state?

Ideally yes, and more than that, we shouldn't look at any omapdrm-specific 
state for the CRTC, but only at the CRTC core state. However, given the driver 
design that enables/disables CRTCs through a complicated call stack starting 
from the encoder enable/disable functions instead of in the CRTC 
enable/disable handlers, this is hard to track at the moment. I'd like to 
clean this mess up, but that will be a separate (and likely quite large) patch 
series.

> If so, we could remove dispc_mgr_is_enabled() call as you do, and add a
> WARN_ON() to omapdss if the mgr is already enabled/disabled to catch
> bugs in omapdrm.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 02/23] drm: omapdrm: fb: Don't store format BPP for each plane

[Laurent Pinchart]

Hi Tomi, Rob and Daniel,

On Tuesday 10 May 2016 10:14:17 Tomi Valkeinen wrote:
> On 09/05/16 23:57, Laurent Pinchart wrote:
> >> fwiw, I guess a lot of data from that table could these days be
> >> replaced w/ some of the drm format helpers
> >> (drm_format_num_planes()/drm_format_plane_cpp()/drm_format_{horz,vert}_ch
> >> rom a_subsampling()/etc)
> > 
> > I don't like those helpers as they're inefficient. Drivers often need to
> > know multiple pieces of information about a format, and the API forces
> > look-ups for every piece of information needed.
> > 
> > Would it make sense to add a drm_format_info() function that returns a
> > pointer to a data structure that describes the format, and reimplement
> > the existing helpers on top of that ?
> 
> I do like that idea.

Rob and Daniel, what do you think ?

> A table implementation might possibly be faster even with the current
> API. Those big switch-cases compile into a big pile of
> if-else-if-else-ifs, whereas a for loop would compile into just a few
> instructions. But just guessing here =).
> 
> In theory, the table could be sorted, making the lookup much faster, but
> I'm not sure if that's worth the effort.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 02/23] drm: omapdrm: fb: Don't store format BPP for each plane

[Daniel Vetter]

On Mon, Jun 06, 2016 at 04:39:49AM +0300, Laurent Pinchart wrote:
> Hi Tomi, Rob and Daniel,
> 
> On Tuesday 10 May 2016 10:14:17 Tomi Valkeinen wrote:
> > On 09/05/16 23:57, Laurent Pinchart wrote:
> > >> fwiw, I guess a lot of data from that table could these days be
> > >> replaced w/ some of the drm format helpers
> > >> (drm_format_num_planes()/drm_format_plane_cpp()/drm_format_{horz,vert}_ch
> > >> rom a_subsampling()/etc)
> > > 
> > > I don't like those helpers as they're inefficient. Drivers often need to
> > > know multiple pieces of information about a format, and the API forces
> > > look-ups for every piece of information needed.
> > > 
> > > Would it make sense to add a drm_format_info() function that returns a
> > > pointer to a data structure that describes the format, and reimplement
> > > the existing helpers on top of that ?
> > 
> > I do like that idea.
> 
> Rob and Daniel, what do you think ?

Seems like a bikeshed, but then I don't mind bikesheds too much either.
Meh.
-Daniel

> 
> > A table implementation might possibly be faster even with the current
> > API. Those big switch-cases compile into a big pile of
> > if-else-if-else-ifs, whereas a for loop would compile into just a few
> > instructions. But just guessing here =).
> > 
> > In theory, the table could be sorted, making the lookup much faster, but
> > I'm not sure if that's worth the effort.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/23] drm: omapdrm: Use atomic state instead of local device state

[Tomi Valkeinen]

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

On 06/06/16 04:14, Laurent Pinchart wrote:

>>> If the DRM core doesn't track whether a CRTC HW is enabled at the
>>> moment, maybe omapdrm should? I guess the above works, but that if()
>>> makes me a bit uneasy, as it's not really obvious, and the logic behind
>>> it could possibly change later...
>>>
>>> A "if (crtc->is_hw_enabled)" would be much more readable.
> 
> The whole point of this patch is to remove local state and rely on DRM core 
> state, so I'd like to avoid that if possible.

Yep, but if DRM core doesn't give that information...

Using drm_atomic_crtc_needs_modeset() to check if a crtc is enabled at a
particular point in the commit sequence feels a bit risky to me.

You do explain it in the comment, so it's not that bad, but I'd still
rather see an 'if (is-the-hw-enabled-or-not)' than looking at seemingly
unrelated information, and deducing from that if the hw is enabled or not.

 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 07/23] drm: omapdrm: Handle FIFO underflow IRQs internally

[Tomi Valkeinen]

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

On 06/06/16 02:21, Laurent Pinchart wrote:

>> Also, I do like it that we deal with crtc or plane interrupts in
>> omap_crtc or omap_plane. Would similar approach here work as you use in
>> the following patches, i.e. just call underflow-handler func in
>> omap_plane.c directly, instead of using the registration mechanism?
> 
> I can do that, but given that all we do is just printing error messages, it 
> sounds a bit overkill. I propose moving FIFO underflow IRQ handling to the 
> CRTC and plane code later when/if we need to perform more work in the 
> handlers.

What's the overkill? Isn't it just making the function public, and
calling that from omap_irq?

And it's true your patch just prints an error message, but
omap_irq_fifo_underflow() is still 35 lines, so it's not a one-liner.

I don't feel strongly about this, though =).

 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 07/23] drm: omapdrm: Handle FIFO underflow IRQs internally

[Laurent Pinchart]

Hi Tomi,

On Monday 06 Jun 2016 13:50:13 Tomi Valkeinen wrote:
> On 06/06/16 02:21, Laurent Pinchart wrote:
> >> Also, I do like it that we deal with crtc or plane interrupts in
> >> omap_crtc or omap_plane. Would similar approach here work as you use in
> >> the following patches, i.e. just call underflow-handler func in
> >> omap_plane.c directly, instead of using the registration mechanism?
> > 
> > I can do that, but given that all we do is just printing error messages,
> > it sounds a bit overkill. I propose moving FIFO underflow IRQ handling to
> > the CRTC and plane code later when/if we need to perform more work in the
> > handlers.
> 
> What's the overkill? Isn't it just making the function public, and
> calling that from omap_irq?

To turn the IRQ handler into a plane function, we would have to look up the 
plane corresponding to the overflow source, call the overflow IRQ handler with 
that plane as a parameter, and then convert the plane into an IRQ source name 
to be printed. I don't think that's worth it.

> And it's true your patch just prints an error message, but
> omap_irq_fifo_underflow() is still 35 lines, so it's not a one-liner.
> 
> I don't feel strongly about this, though =).

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 10/23] drm: omapdrm: Use atomic state instead of local device state

[Laurent Pinchart]

Hi Tomi,

On Monday 06 Jun 2016 13:37:13 Tomi Valkeinen wrote:
> On 06/06/16 04:14, Laurent Pinchart wrote:
> >>> If the DRM core doesn't track whether a CRTC HW is enabled at the
> >>> moment, maybe omapdrm should? I guess the above works, but that if()
> >>> makes me a bit uneasy, as it's not really obvious, and the logic behind
> >>> it could possibly change later...
> >>> 
> >>> A "if (crtc->is_hw_enabled)" would be much more readable.
> > 
> > The whole point of this patch is to remove local state and rely on DRM
> > core state, so I'd like to avoid that if possible.
> 
> Yep, but if DRM core doesn't give that information...
> 
> Using drm_atomic_crtc_needs_modeset() to check if a crtc is enabled at a
> particular point in the commit sequence feels a bit risky to me.
> 
> You do explain it in the comment, so it's not that bad, but I'd still
> rather see an 'if (is-the-hw-enabled-or-not)' than looking at seemingly
> unrelated information, and deducing from that if the hw is enabled or not.

I agree, but I'd like that "is-the-hw-enabled-or-not" state to be provided by 
the DRM core, not the omapdrm driver. I'll see how we can get there, but I'd 
rather do that separately from this patch series as it will require very 
careful design and review.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 10/23] drm: omapdrm: Use atomic state instead of local device state

[Laurent Pinchart]

Hi Daniel,

On Monday 06 Jun 2016 04:14:59 Laurent Pinchart wrote:
> On Wednesday 11 May 2016 09:37:56 Daniel Vetter wrote:
> > On Tue, May 10, 2016 at 04:24:11PM +0300, Tomi Valkeinen wrote:
> >> On 26/04/16 23:35, Laurent Pinchart wrote:
> >>> Instead of conditioning planes update based on the hardware device
> >>> state, use the CRTC state stored in the atomic state. This reduces the
> >>> dependency from the DRM layer to the DSS layer.
> >>> 
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> ---
> >>> 
> >>>  drivers/gpu/drm/omapdrm/omap_crtc.c | 23 ++++++++++++++---------
> >>>  1 file changed, 14 insertions(+), 9 deletions(-)
> >>> 
> >>> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>> b/drivers/gpu/drm/omapdrm/omap_crtc.c index 6359d7933b93..4c56d6a68390
> >>> 100644
> >>> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> >>> @@ -381,18 +381,23 @@ static void omap_crtc_atomic_flush(struct
> >>> drm_crtc *crtc,
> >>> 
> >>>  	WARN_ON(omap_crtc->vblank_irq.registered);
> >>> 
> >>> -	if (dispc_mgr_is_enabled(omap_crtc->channel)) {
> >>> +	/*
> >>> +	 * Only flush the CRTC if it is currently active. CRTCs that require
> >>> a
> >>> +	 * mode set are disabled prior plane updates and enabled afterwards.
> >>> +	 * They are thus not active, regardless of what their state report.
> >>> +	 */
> >>> +	if (!crtc->state->active ||
> >>> drm_atomic_crtc_needs_modeset(crtc->state))
> >>> +		return;
> >> 
> >> If the DRM core doesn't track whether a CRTC HW is enabled at the
> >> moment, maybe omapdrm should? I guess the above works, but that if()
> >> makes me a bit uneasy, as it's not really obvious, and the logic behind
> >> it could possibly change later...
> >> 
> >> A "if (crtc->is_hw_enabled)" would be much more readable.
> 
> The whole point of this patch is to remove local state and rely on DRM core
> state, so I'd like to avoid that if possible.
> 
> > Look at the active_only paramater of the planes_commit helper. That should
> > do exactly what you want.
> 
> The drm_atomic_helper_commit_planes() helper has this check
> 
>         if (active_only && !crtc->state->active)
>                 continue;
> 
>         funcs->atomic_flush(crtc, old_crtc_state);
> 
> I could thus remove the !crtc->state->active check, but the
> drm_atomic_crtc_needs_modeset() check would need to stay, wouldn't it ? When
> CRTCs go through a modeset they are disabled prior to plane updates, but
> their state active status can still be true. Or should that be fixed in the
> core ?

Any comment on this ? Knowing whether the CRTC hardware is enabled is helpful 
to implement the flush() function. Would it make sense to add a new "is 
hardware enabled" flag to the CRTC structure (or possibly the CRTC state 
structure, but given that there is by definition a single instance of the 
hardware state the CRTC structure would make more sense) ?

-- 
Regards,

Laurent Pinchart

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