Hi,
This is v2 of this series. Changes to v1:
* updated according to the comments * added fix for 24 bpp formats * added PAGE_ALIGN cleanup that I have missed
Tomi
Fabian Frederick (1): drm/omap: replace ALIGN(PAGE_SIZE) by PAGE_ALIGN
Tomi Valkeinen (6): drm/omap: return error if dma_alloc_writecombine fails drm/omap: check that plane is inside crtc drm/omap: increase DMM transaction timeout drm/omap: fix omap_framebuffer_unpin() error handling drm/omap: fix omap_gem_put_paddr() error handling drm/omap: fix align_pitch() for 24 bits per pixel
drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 2 +- drivers/gpu/drm/omapdrm/omap_drv.h | 6 +++--- drivers/gpu/drm/omapdrm/omap_fb.c | 16 ++++------------ drivers/gpu/drm/omapdrm/omap_fbdev.c | 2 +- drivers/gpu/drm/omapdrm/omap_gem.c | 26 ++++++++++++++------------ drivers/gpu/drm/omapdrm/omap_plane.c | 26 ++++++++++++++++++++++++++ 6 files changed, 49 insertions(+), 29 deletions(-)
On a platform with no TILER (e.g. omap3, am43xx), when the user wants to allocate buffer with flag OMAP_BO_SCANOUT, the buffer needs to be allocated with dma_alloc_writecombine. For some reason the driver does not return an error if that alloc fails, instead it continues without backing memory. This leads to errors later when the user tries to use the buffer.
This patch makes the driver return an error if dma_alloc_writecombine fails.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_gem.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 2ab77801cf5f..874eb6dcf94b 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -1378,11 +1378,7 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
omap_obj = kzalloc(sizeof(*omap_obj), GFP_KERNEL); if (!omap_obj) - goto fail; - - spin_lock(&priv->list_lock); - list_add(&omap_obj->mm_list, &priv->obj_list); - spin_unlock(&priv->list_lock); + return NULL;
obj = &omap_obj->base;
@@ -1392,11 +1388,19 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev, */ omap_obj->vaddr = dma_alloc_writecombine(dev->dev, size, &omap_obj->paddr, GFP_KERNEL); - if (omap_obj->vaddr) - flags |= OMAP_BO_DMA; + if (!omap_obj->vaddr) { + kfree(omap_obj);
+ return NULL; + } + + flags |= OMAP_BO_DMA; }
+ spin_lock(&priv->list_lock); + list_add(&omap_obj->mm_list, &priv->obj_list); + spin_unlock(&priv->list_lock); + omap_obj->flags = flags;
if (flags & OMAP_BO_TILED) {
DRM allows planes to be partially off-screen, but DSS hardware does not. This patch adds the necessary check to reject plane configs if the plane is not fully inside the crtc.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com --- drivers/gpu/drm/omapdrm/omap_plane.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index cfa8276c4deb..098904696a5c 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -17,6 +17,7 @@ * this program. If not, see http://www.gnu.org/licenses/. */
+#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_plane_helper.h>
@@ -153,9 +154,34 @@ static void omap_plane_atomic_disable(struct drm_plane *plane, dispc_ovl_enable(omap_plane->id, false); }
+static int omap_plane_atomic_check(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct drm_crtc_state *crtc_state; + + if (!state->crtc) + return 0; + + crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc); + if (IS_ERR(crtc_state)) + return PTR_ERR(crtc_state); + + if (state->crtc_x < 0 || state->crtc_y < 0) + return -EINVAL; + + if (state->crtc_x + state->crtc_w > crtc_state->adjusted_mode.hdisplay) + return -EINVAL; + + if (state->crtc_y + state->crtc_h > crtc_state->adjusted_mode.vdisplay) + return -EINVAL; + + return 0; +} + static const struct drm_plane_helper_funcs omap_plane_helper_funcs = { .prepare_fb = omap_plane_prepare_fb, .cleanup_fb = omap_plane_cleanup_fb, + .atomic_check = omap_plane_atomic_check, .atomic_update = omap_plane_atomic_update, .atomic_disable = omap_plane_atomic_disable, };
Hi Tomi,
Thank you for the patch.
On Thursday 02 July 2015 15:35:31 Tomi Valkeinen wrote:
DRM allows planes to be partially off-screen, but DSS hardware does not. This patch adds the necessary check to reject plane configs if the plane is not fully inside the crtc.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com
Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/omapdrm/omap_plane.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index cfa8276c4deb..098904696a5c 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -17,6 +17,7 @@
- this program. If not, see http://www.gnu.org/licenses/.
*/
+#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_plane_helper.h>
@@ -153,9 +154,34 @@ static void omap_plane_atomic_disable(struct drm_plane *plane, dispc_ovl_enable(omap_plane->id, false); }
+static int omap_plane_atomic_check(struct drm_plane *plane,
struct drm_plane_state *state)+{
- struct drm_crtc_state *crtc_state;
- if (!state->crtc)
return 0;- crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
- if (IS_ERR(crtc_state))
return PTR_ERR(crtc_state);- if (state->crtc_x < 0 || state->crtc_y < 0)
return -EINVAL;- if (state->crtc_x + state->crtc_w > crtc_state->adjusted_mode.hdisplay)
return -EINVAL;- if (state->crtc_y + state->crtc_h > crtc_state->adjusted_mode.vdisplay)
return -EINVAL;- return 0;
+}
static const struct drm_plane_helper_funcs omap_plane_helper_funcs = { .prepare_fb = omap_plane_prepare_fb, .cleanup_fb = omap_plane_cleanup_fb,
- .atomic_check = omap_plane_atomic_check, .atomic_update = omap_plane_atomic_update, .atomic_disable = omap_plane_atomic_disable,
};
The DMM driver uses a timeout of 1 ms to wait for DMM transaction to finish. While DMM should always finish the operation within that time, the timeout is rather strict. Small misbehavior of the system (e.g. an irq taking too long) could trigger the timeout.
As the DMM is a critical piece of code for display memory management, let's increase the timeout to 100 ms so that we are less likely to fail a memory allocation in case of system misbehaviors. 100 ms is just a guess of a reasonably large timeout. The HW should accomplish the task in less than 1 ms.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com --- drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c index f2daad8c3d96..7841970de48d 100644 --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c @@ -285,7 +285,7 @@ static int dmm_txn_commit(struct dmm_txn *txn, bool wait)
if (wait) { if (!wait_for_completion_timeout(&engine->compl, - msecs_to_jiffies(1))) { + msecs_to_jiffies(100))) { dev_err(dmm->dev, "timed out waiting for done\n"); ret = -ETIMEDOUT; }
omap_framebuffer_unpin() check the return value of omap_gem_put_paddr() and return immediately if omap_gem_put_paddr() fails.
This patch removes the check for the return value, and also removes the return value of omap_framebuffer_unpin(), because:
* Nothing checks the return value of omap_framebuffer_unpin(), and even something did check it, there's nothing the caller can do to handle the error.
* If a omap_gem_put_paddr() fails, the framebuffer's other planes will be left unreleased. So it's better to call omap_gem_put_paddr() for all the planes, even if one would fail.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_drv.h | 2 +- drivers/gpu/drm/omapdrm/omap_fb.c | 16 ++++------------ 2 files changed, 5 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index ae2df41f216f..2ef89c0c3006 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -177,7 +177,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev, 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); -int omap_framebuffer_unpin(struct drm_framebuffer *fb); +void omap_framebuffer_unpin(struct drm_framebuffer *fb); void omap_framebuffer_update_scanout(struct drm_framebuffer *fb, struct omap_drm_window *win, struct omap_overlay_info *info); struct drm_connector *omap_framebuffer_get_next_connector( diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c index 0b967e76df1a..51b1219af87f 100644 --- a/drivers/gpu/drm/omapdrm/omap_fb.c +++ b/drivers/gpu/drm/omapdrm/omap_fb.c @@ -287,10 +287,10 @@ fail: }
/* unpin, no longer being scanned out: */ -int omap_framebuffer_unpin(struct drm_framebuffer *fb) +void omap_framebuffer_unpin(struct drm_framebuffer *fb) { struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb); - int ret, i, n = drm_format_num_planes(fb->pixel_format); + int i, n = drm_format_num_planes(fb->pixel_format);
mutex_lock(&omap_fb->lock);
@@ -298,24 +298,16 @@ int omap_framebuffer_unpin(struct drm_framebuffer *fb)
if (omap_fb->pin_count > 0) { mutex_unlock(&omap_fb->lock); - return 0; + return; }
for (i = 0; i < n; i++) { struct plane *plane = &omap_fb->planes[i]; - ret = omap_gem_put_paddr(plane->bo); - if (ret) - goto fail; + omap_gem_put_paddr(plane->bo); plane->paddr = 0; }
mutex_unlock(&omap_fb->lock); - - return 0; - -fail: - mutex_unlock(&omap_fb->lock); - return ret; }
struct drm_gem_object *omap_framebuffer_bo(struct drm_framebuffer *fb, int p)
If tiler_unpin() call in omap_gem_put_paddr() fails, omap_gem_put_paddr() will immediately stop processing and return an error.
This patch remoes that error checking, and also removes omap_gem_put_paddr()'s return value, because:
* The caller of omap_gem_put_paddr() can do nothing if an error happens, so it's pointless to return an error value
* If tiler_unpin() fails, the GEM object will possibly be left in an undefined state, where the DMM mapping may have been removed, but the GEM object still thinks everything is as it should be, leading to crashes later.
* There's no point in returning an error from a "free" call, as the caller can do nothing about it. So it's better to clean up as much as possible.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_drv.h | 2 +- drivers/gpu/drm/omapdrm/omap_gem.c | 8 +++----- 2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 2ef89c0c3006..1ae8477e4289 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -211,7 +211,7 @@ void omap_gem_dma_sync(struct drm_gem_object *obj, enum dma_data_direction dir); int omap_gem_get_paddr(struct drm_gem_object *obj, dma_addr_t *paddr, bool remap); -int omap_gem_put_paddr(struct drm_gem_object *obj); +void omap_gem_put_paddr(struct drm_gem_object *obj); int omap_gem_get_pages(struct drm_gem_object *obj, struct page ***pages, bool remap); int omap_gem_put_pages(struct drm_gem_object *obj); diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 874eb6dcf94b..7ed08fdc4c42 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -808,10 +808,10 @@ fail: /* Release physical address, when DMA is no longer being performed.. this * could potentially unpin and unmap buffers from TILER */ -int omap_gem_put_paddr(struct drm_gem_object *obj) +void omap_gem_put_paddr(struct drm_gem_object *obj) { struct omap_gem_object *omap_obj = to_omap_bo(obj); - int ret = 0; + int ret;
mutex_lock(&obj->dev->struct_mutex); if (omap_obj->paddr_cnt > 0) { @@ -821,7 +821,6 @@ int omap_gem_put_paddr(struct drm_gem_object *obj) if (ret) { dev_err(obj->dev->dev, "could not unpin pages: %d\n", ret); - goto fail; } ret = tiler_release(omap_obj->block); if (ret) { @@ -832,9 +831,8 @@ int omap_gem_put_paddr(struct drm_gem_object *obj) omap_obj->block = NULL; } } -fail: + mutex_unlock(&obj->dev->struct_mutex); - return ret; }
/* Get rotated scanout address (only valid if already pinned), at the
align_pitch() uses ALIGN() to ensure the pitch is aligned to SGX's requirement of 8 pixels. However, ALIGN() expects the alignment value to be a power of two, which is not the case for 24 bits per pixels.
Use roundup() instead, which works for all alignments.
This fixes the error seen with 24 bits per pixel modes:
"buffer pitch (2176 bytes) is not a multiple of pixel size (3 bytes)"
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com --- drivers/gpu/drm/omapdrm/omap_drv.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 1ae8477e4289..12081e61d45a 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -236,7 +236,7 @@ static inline int align_pitch(int pitch, int width, int bpp) /* PVR needs alignment to 8 pixels.. right now that is the most * restrictive stride requirement.. */ - return ALIGN(pitch, 8 * bytespp); + return roundup(pitch, 8 * bytespp); }
/* map crtc to vblank mask */
From: Fabian Frederick fabf@skynet.be
use mm.h definition
Cc: David Airlie airlied@linux.ie Cc: Tomi Valkeinen tomi.valkeinen@ti.com Cc: dri-devel@lists.freedesktop.org Signed-off-by: Fabian Frederick fabf@skynet.be Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com --- drivers/gpu/drm/omapdrm/omap_fbdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c index 23b5a84389e3..720d16bce7e8 100644 --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c @@ -135,7 +135,7 @@ static int omap_fbdev_create(struct drm_fb_helper *helper, fbdev->ywrap_enabled = priv->has_dmm && ywrap_enabled; if (fbdev->ywrap_enabled) { /* need to align pitch to page size if using DMM scrolling */ - mode_cmd.pitches[0] = ALIGN(mode_cmd.pitches[0], PAGE_SIZE); + mode_cmd.pitches[0] = PAGE_ALIGN(mode_cmd.pitches[0]); }
/* allocate backing bo */
dri-devel@lists.freedesktop.org
-
Laurent Pinchart -
Tomi Valkeinen