* [PATCH 1/5] drm/omap: return error if dma_alloc_writecombine fails
2015-06-18 10:10 [PATCH 0/5] drm/omap: various small fixes Tomi Valkeinen
@ 2015-06-18 10:10 ` Tomi Valkeinen
2015-06-18 14:27 ` Laurent Pinchart
2015-06-18 10:10 ` [PATCH 2/5] drm/omap: check that plane is inside crtc Tomi Valkeinen
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Tomi Valkeinen @ 2015-06-18 10:10 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel; +Cc: Tomi Valkeinen
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>
---
drivers/gpu/drm/omapdrm/omap_gem.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 2ab77801cf5f..51c5635aff62 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -1392,9 +1392,17 @@ 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) {
+ spin_lock(&priv->list_lock);
+ list_del(&omap_obj->mm_list);
+ spin_unlock(&priv->list_lock);
+ kfree(omap_obj);
+
+ return NULL;
+ }
+
+ flags |= OMAP_BO_DMA;
}
omap_obj->flags = flags;
--
2.1.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/5] drm/omap: return error if dma_alloc_writecombine fails
2015-06-18 10:10 ` [PATCH 1/5] drm/omap: return error if dma_alloc_writecombine fails Tomi Valkeinen
@ 2015-06-18 14:27 ` Laurent Pinchart
2015-06-23 8:18 ` Tomi Valkeinen
0 siblings, 1 reply; 13+ messages in thread
From: Laurent Pinchart @ 2015-06-18 14:27 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: dri-devel
Hi Tomi,
Thank you for the patch.
On Thursday 18 June 2015 13:10:35 Tomi Valkeinen wrote:
> 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>
> ---
> drivers/gpu/drm/omapdrm/omap_gem.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> b/drivers/gpu/drm/omapdrm/omap_gem.c index 2ab77801cf5f..51c5635aff62
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -1392,9 +1392,17 @@ 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) {
> + spin_lock(&priv->list_lock);
> + list_del(&omap_obj->mm_list);
> + spin_unlock(&priv->list_lock);
Wouldn't it be simpler to move the list_add after the "if ((flags &
OMAP_BO_SCANOUT) && !priv->has_dmm)" block ? You could then avoid the
list_del.
> + kfree(omap_obj);
> +
> + return NULL;
> + }
> +
> + flags |= OMAP_BO_DMA;
> }
>
> omap_obj->flags = flags;
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/5] drm/omap: return error if dma_alloc_writecombine fails
2015-06-18 14:27 ` Laurent Pinchart
@ 2015-06-23 8:18 ` Tomi Valkeinen
2015-06-23 19:43 ` Laurent Pinchart
0 siblings, 1 reply; 13+ messages in thread
From: Tomi Valkeinen @ 2015-06-23 8:18 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 3503 bytes --]
On 18/06/15 17:27, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Thursday 18 June 2015 13:10:35 Tomi Valkeinen wrote:
>> 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>
>> ---
>> drivers/gpu/drm/omapdrm/omap_gem.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
>> b/drivers/gpu/drm/omapdrm/omap_gem.c index 2ab77801cf5f..51c5635aff62
>> 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
>> @@ -1392,9 +1392,17 @@ 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) {
>> + spin_lock(&priv->list_lock);
>> + list_del(&omap_obj->mm_list);
>> + spin_unlock(&priv->list_lock);
>
> Wouldn't it be simpler to move the list_add after the "if ((flags &
> OMAP_BO_SCANOUT) && !priv->has_dmm)" block ? You could then avoid the
> list_del.
Thanks, it's cleaner that way:
commit 70a7c80ae9f787031fa3eebc70df024deadde09f (HEAD)
Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
Date: Tue Mar 17 15:31:11 2015 +0200
drm/omap: return error if dma_alloc_writecombine fails
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>
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) {
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/5] drm/omap: return error if dma_alloc_writecombine fails
2015-06-23 8:18 ` Tomi Valkeinen
@ 2015-06-23 19:43 ` Laurent Pinchart
0 siblings, 0 replies; 13+ messages in thread
From: Laurent Pinchart @ 2015-06-23 19:43 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: dri-devel
Hi Tomi,
On Tuesday 23 June 2015 11:18:16 Tomi Valkeinen wrote:
> On 18/06/15 17:27, Laurent Pinchart wrote:
> > On Thursday 18 June 2015 13:10:35 Tomi Valkeinen wrote:
> >> 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>
> >> ---
> >>
> >> drivers/gpu/drm/omapdrm/omap_gem.c | 12 ++++++++++--
> >> 1 file changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> >> b/drivers/gpu/drm/omapdrm/omap_gem.c index 2ab77801cf5f..51c5635aff62
> >> 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> >> @@ -1392,9 +1392,17 @@ 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) {
> >> + spin_lock(&priv->list_lock);
> >> + list_del(&omap_obj->mm_list);
> >> + spin_unlock(&priv->list_lock);
> >
> > Wouldn't it be simpler to move the list_add after the "if ((flags &
> > OMAP_BO_SCANOUT) && !priv->has_dmm)" block ? You could then avoid the
> > list_del.
>
> Thanks, it's cleaner that way:
>
> commit 70a7c80ae9f787031fa3eebc70df024deadde09f (HEAD)
> Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Date: Tue Mar 17 15:31:11 2015 +0200
>
> drm/omap: return error if dma_alloc_writecombine fails
>
> 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>
> 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) {
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/5] drm/omap: check that plane is inside crtc
2015-06-18 10:10 [PATCH 0/5] drm/omap: various small fixes Tomi Valkeinen
2015-06-18 10:10 ` [PATCH 1/5] drm/omap: return error if dma_alloc_writecombine fails Tomi Valkeinen
@ 2015-06-18 10:10 ` Tomi Valkeinen
2015-06-18 14:45 ` Laurent Pinchart
2015-06-18 10:10 ` [PATCH 3/5] drm/omap: increase DMM transaction timeout Tomi Valkeinen
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Tomi Valkeinen @ 2015-06-18 10:10 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel; +Cc: Tomi Valkeinen
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..53594a3b4e98 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 (!crtc_state)
+ return 0;
+
+ 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,
};
--
2.1.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 2/5] drm/omap: check that plane is inside crtc
2015-06-18 10:10 ` [PATCH 2/5] drm/omap: check that plane is inside crtc Tomi Valkeinen
@ 2015-06-18 14:45 ` Laurent Pinchart
2015-06-23 8:01 ` Tomi Valkeinen
0 siblings, 1 reply; 13+ messages in thread
From: Laurent Pinchart @ 2015-06-18 14:45 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: dri-devel
Hi Tomi,
Thank you for the patch.
On Thursday 18 June 2015 13:10:36 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>
> ---
> 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..53594a3b4e98
> 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 (!crtc_state)
> + return 0;
drm_atomic_get_crtc_state() returns an ERR_PTR on error. You should then
propagate the error to the caller:
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;
I wonder whether we couldn't clip the plane in software instead of failing.
This patch is fine though (except for the problem above), clipping can be
implemented in a separate patch.
> + 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,
> };
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/5] drm/omap: check that plane is inside crtc
2015-06-18 14:45 ` Laurent Pinchart
@ 2015-06-23 8:01 ` Tomi Valkeinen
0 siblings, 0 replies; 13+ messages in thread
From: Tomi Valkeinen @ 2015-06-23 8:01 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 2443 bytes --]
On 18/06/15 17:45, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Thursday 18 June 2015 13:10:36 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>
>> ---
>> 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..53594a3b4e98
>> 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 (!crtc_state)
>> + return 0;
>
> drm_atomic_get_crtc_state() returns an ERR_PTR on error. You should then
> propagate the error to the caller:
>
> crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
> if (IS_ERR(crtc_state))
> return PTR_ERR(crtc_state);
Thanks, I missed that. I've made the change.
>> + 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;
>
> I wonder whether we couldn't clip the plane in software instead of failing.
> This patch is fine though (except for the problem above), clipping can be
> implemented in a separate patch.
I had the same thought, but I didn't want to go that way yet. I have a
feeling that it could have corner cases that need to be taken care of,
and I just wanted to fix the current behavior.
Tomi
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/5] drm/omap: increase DMM transaction timeout
2015-06-18 10:10 [PATCH 0/5] drm/omap: various small fixes Tomi Valkeinen
2015-06-18 10:10 ` [PATCH 1/5] drm/omap: return error if dma_alloc_writecombine fails Tomi Valkeinen
2015-06-18 10:10 ` [PATCH 2/5] drm/omap: check that plane is inside crtc Tomi Valkeinen
@ 2015-06-18 10:10 ` Tomi Valkeinen
2015-06-18 10:10 ` [PATCH 4/5] drm/omap: fix omap_framebuffer_unpin() error handling Tomi Valkeinen
2015-06-18 10:10 ` [PATCH 5/5] drm/omap: fix omap_gem_put_paddr() " Tomi Valkeinen
4 siblings, 0 replies; 13+ messages in thread
From: Tomi Valkeinen @ 2015-06-18 10:10 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel; +Cc: Tomi Valkeinen
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;
}
--
2.1.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 4/5] drm/omap: fix omap_framebuffer_unpin() error handling
2015-06-18 10:10 [PATCH 0/5] drm/omap: various small fixes Tomi Valkeinen
` (2 preceding siblings ...)
2015-06-18 10:10 ` [PATCH 3/5] drm/omap: increase DMM transaction timeout Tomi Valkeinen
@ 2015-06-18 10:10 ` Tomi Valkeinen
2015-06-18 15:13 ` Laurent Pinchart
2015-06-18 10:10 ` [PATCH 5/5] drm/omap: fix omap_gem_put_paddr() " Tomi Valkeinen
4 siblings, 1 reply; 13+ messages in thread
From: Tomi Valkeinen @ 2015-06-18 10:10 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel; +Cc: Tomi Valkeinen
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>
---
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)
--
2.1.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 4/5] drm/omap: fix omap_framebuffer_unpin() error handling
2015-06-18 10:10 ` [PATCH 4/5] drm/omap: fix omap_framebuffer_unpin() error handling Tomi Valkeinen
@ 2015-06-18 15:13 ` Laurent Pinchart
0 siblings, 0 replies; 13+ messages in thread
From: Laurent Pinchart @ 2015-06-18 15:13 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: dri-devel
Hi Tomi,
Thank you for the patch.
On Thursday 18 June 2015 13:10:38 Tomi Valkeinen wrote:
> 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)
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 5/5] drm/omap: fix omap_gem_put_paddr() error handling
2015-06-18 10:10 [PATCH 0/5] drm/omap: various small fixes Tomi Valkeinen
` (3 preceding siblings ...)
2015-06-18 10:10 ` [PATCH 4/5] drm/omap: fix omap_framebuffer_unpin() error handling Tomi Valkeinen
@ 2015-06-18 10:10 ` Tomi Valkeinen
2015-06-18 15:14 ` Laurent Pinchart
4 siblings, 1 reply; 13+ messages in thread
From: Tomi Valkeinen @ 2015-06-18 10:10 UTC (permalink / raw)
To: Laurent Pinchart, dri-devel; +Cc: Tomi Valkeinen
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>
---
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 51c5635aff62..d01d40245298 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
--
2.1.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 5/5] drm/omap: fix omap_gem_put_paddr() error handling
2015-06-18 10:10 ` [PATCH 5/5] drm/omap: fix omap_gem_put_paddr() " Tomi Valkeinen
@ 2015-06-18 15:14 ` Laurent Pinchart
0 siblings, 0 replies; 13+ messages in thread
From: Laurent Pinchart @ 2015-06-18 15:14 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: dri-devel
Hi Tomi,
Thank you for the patch.
On Thursday 18 June 2015 13:10:39 Tomi Valkeinen wrote:
> 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 51c5635aff62..d01d40245298
> 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
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 13+ messages in thread