From: Ville Syrjälä ville.syrjala@linux.intel.com
Limit the scaled clip to only clip at most dst_w/h pixels. This avoids the problem with clip_scaled() not being able to return negative values. Since new_src_w/h is now properly bounded we can remove the clamp()s.
Cc: Benjamin Gaignard benjamin.gaignard@st.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_rect.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c index b8363aaa9032..7762b6e9278d 100644 --- a/drivers/gpu/drm/drm_rect.c +++ b/drivers/gpu/drm/drm_rect.c @@ -54,7 +54,12 @@ EXPORT_SYMBOL(drm_rect_intersect);
static u32 clip_scaled(u32 src, u32 dst, u32 clip) { - u64 tmp = mul_u32_u32(src, dst - clip); + u64 tmp; + + /* Only clip what we have. Keeps the result bounded as well. */ + clip = min(clip, dst); + + tmp = mul_u32_u32(src, dst - clip);
/* * Round toward 1.0 when clipping so that we don't accidentally @@ -89,7 +94,7 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, u32 new_src_w = clip_scaled(drm_rect_width(src), drm_rect_width(dst), diff);
- src->x1 = clamp_t(int64_t, src->x2 - new_src_w, INT_MIN, INT_MAX); + src->x1 = src->x2 - new_src_w; dst->x1 = clip->x1; } diff = clip->y1 - dst->y1; @@ -97,7 +102,7 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, u32 new_src_h = clip_scaled(drm_rect_height(src), drm_rect_height(dst), diff);
- src->y1 = clamp_t(int64_t, src->y2 - new_src_h, INT_MIN, INT_MAX); + src->y1 = src->y2 - new_src_h; dst->y1 = clip->y1; } diff = dst->x2 - clip->x2; @@ -105,7 +110,7 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, u32 new_src_w = clip_scaled(drm_rect_width(src), drm_rect_width(dst), diff);
- src->x2 = clamp_t(int64_t, src->x1 + new_src_w, INT_MIN, INT_MAX); + src->x2 = src->x1 + new_src_w; dst->x2 = clip->x2; } diff = dst->y2 - clip->y2; @@ -113,7 +118,7 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, u32 new_src_h = clip_scaled(drm_rect_height(src), drm_rect_height(dst), diff);
- src->y2 = clamp_t(int64_t, src->y1 + new_src_h, INT_MIN, INT_MAX); + src->y2 = src->y1 + new_src_h; dst->y2 = clip->y2; }
From: Ville Syrjälä ville.syrjala@linux.intel.com
Now that we've constrained the clipped source rectangle such that it can't have negative dimensions doing the same for the dst rectangle seems appropriate. Should at least result in the clipped src and dst rectangles being a bit more consistent with each other.
Cc: Benjamin Gaignard benjamin.gaignard@st.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_rect.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c index 7762b6e9278d..229325fcf333 100644 --- a/drivers/gpu/drm/drm_rect.c +++ b/drivers/gpu/drm/drm_rect.c @@ -52,14 +52,14 @@ bool drm_rect_intersect(struct drm_rect *r1, const struct drm_rect *r2) } EXPORT_SYMBOL(drm_rect_intersect);
-static u32 clip_scaled(u32 src, u32 dst, u32 clip) +static u32 clip_scaled(int src, int dst, int *clip) { u64 tmp;
/* Only clip what we have. Keeps the result bounded as well. */ - clip = min(clip, dst); + *clip = min(*clip, dst);
- tmp = mul_u32_u32(src, dst - clip); + tmp = mul_u32_u32(src, dst - *clip);
/* * Round toward 1.0 when clipping so that we don't accidentally @@ -92,34 +92,34 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, diff = clip->x1 - dst->x1; if (diff > 0) { u32 new_src_w = clip_scaled(drm_rect_width(src), - drm_rect_width(dst), diff); + drm_rect_width(dst), &diff);
src->x1 = src->x2 - new_src_w; - dst->x1 = clip->x1; + dst->x1 += diff; } diff = clip->y1 - dst->y1; if (diff > 0) { u32 new_src_h = clip_scaled(drm_rect_height(src), - drm_rect_height(dst), diff); + drm_rect_height(dst), &diff);
src->y1 = src->y2 - new_src_h; - dst->y1 = clip->y1; + dst->y1 += diff; } diff = dst->x2 - clip->x2; if (diff > 0) { u32 new_src_w = clip_scaled(drm_rect_width(src), - drm_rect_width(dst), diff); + drm_rect_width(dst), &diff);
src->x2 = src->x1 + new_src_w; - dst->x2 = clip->x2; + dst->x2 -= diff; } diff = dst->y2 - clip->y2; if (diff > 0) { u32 new_src_h = clip_scaled(drm_rect_height(src), - drm_rect_height(dst), diff); + drm_rect_height(dst), &diff);
src->y2 = src->y1 + new_src_h; - dst->y2 = clip->y2; + dst->y2 -= diff; }
return drm_rect_visible(dst);
On Wed, Nov 20, 2019 at 06:25:12PM +0200, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Now that we've constrained the clipped source rectangle such that it can't have negative dimensions doing the same for the dst rectangle seems appropriate. Should at least result in the clipped src and dst rectangles being a bit more consistent with each other.
Cc: Benjamin Gaignard benjamin.gaignard@st.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
selftests for this stuff? Looks like the prime example, write testcase proving code is busted, fix it, everyone celebrate? -Daniel
drivers/gpu/drm/drm_rect.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c index 7762b6e9278d..229325fcf333 100644 --- a/drivers/gpu/drm/drm_rect.c +++ b/drivers/gpu/drm/drm_rect.c @@ -52,14 +52,14 @@ bool drm_rect_intersect(struct drm_rect *r1, const struct drm_rect *r2) } EXPORT_SYMBOL(drm_rect_intersect);
-static u32 clip_scaled(u32 src, u32 dst, u32 clip) +static u32 clip_scaled(int src, int dst, int *clip) { u64 tmp;
/* Only clip what we have. Keeps the result bounded as well. */
- clip = min(clip, dst);
- *clip = min(*clip, dst);
- tmp = mul_u32_u32(src, dst - clip);
tmp = mul_u32_u32(src, dst - *clip);
/*
- Round toward 1.0 when clipping so that we don't accidentally
@@ -92,34 +92,34 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, diff = clip->x1 - dst->x1; if (diff > 0) { u32 new_src_w = clip_scaled(drm_rect_width(src),
drm_rect_width(dst), diff);
drm_rect_width(dst), &diff);src->x1 = src->x2 - new_src_w;
dst->x1 = clip->x1;
dst->x1 += diff;
drm_rect_height(dst), diff);
drm_rect_height(dst), &diff);src->y1 = src->y2 - new_src_h;
dst->y1 = clip->y1;
dst->y1 += diff;
drm_rect_width(dst), diff);
drm_rect_width(dst), &diff);src->x2 = src->x1 + new_src_w;
dst->x2 = clip->x2;
dst->x2 -= diff;
drm_rect_height(dst), diff);
drm_rect_height(dst), &diff);src->y2 = src->y1 + new_src_h;
dst->y2 = clip->y2;
dst->y2 -= diff;}
return drm_rect_visible(dst);
-- 2.23.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Nov 20, 2019 at 05:43:40PM +0100, Daniel Vetter wrote:
On Wed, Nov 20, 2019 at 06:25:12PM +0200, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Now that we've constrained the clipped source rectangle such that it can't have negative dimensions doing the same for the dst rectangle seems appropriate. Should at least result in the clipped src and dst rectangles being a bit more consistent with each other.
Cc: Benjamin Gaignard benjamin.gaignard@st.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
selftests for this stuff? Looks like the prime example, write testcase proving code is busted, fix it, everyone celebrate?
Yeah, seems like a good idea. Though I'll have to figure out if it's actually broken or not ;)
Hmm. Ouch. There's seems to be a div by zero lurking in there if dst_w/h == 0. I wonder why nothing has hit that.
-Daniel
drivers/gpu/drm/drm_rect.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c index 7762b6e9278d..229325fcf333 100644 --- a/drivers/gpu/drm/drm_rect.c +++ b/drivers/gpu/drm/drm_rect.c @@ -52,14 +52,14 @@ bool drm_rect_intersect(struct drm_rect *r1, const struct drm_rect *r2) } EXPORT_SYMBOL(drm_rect_intersect);
-static u32 clip_scaled(u32 src, u32 dst, u32 clip) +static u32 clip_scaled(int src, int dst, int *clip) { u64 tmp;
/* Only clip what we have. Keeps the result bounded as well. */
- clip = min(clip, dst);
- *clip = min(*clip, dst);
- tmp = mul_u32_u32(src, dst - clip);
tmp = mul_u32_u32(src, dst - *clip);
/*
- Round toward 1.0 when clipping so that we don't accidentally
@@ -92,34 +92,34 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, diff = clip->x1 - dst->x1; if (diff > 0) { u32 new_src_w = clip_scaled(drm_rect_width(src),
drm_rect_width(dst), diff);
drm_rect_width(dst), &diff);src->x1 = src->x2 - new_src_w;
dst->x1 = clip->x1;
dst->x1 += diff;
drm_rect_height(dst), diff);
drm_rect_height(dst), &diff);src->y1 = src->y2 - new_src_h;
dst->y1 = clip->y1;
dst->y1 += diff;
drm_rect_width(dst), diff);
drm_rect_width(dst), &diff);src->x2 = src->x1 + new_src_w;
dst->x2 = clip->x2;
dst->x2 -= diff;
drm_rect_height(dst), diff);
drm_rect_height(dst), &diff);src->y2 = src->y1 + new_src_h;
dst->y2 = clip->y2;
dst->y2 -= diff;}
return drm_rect_visible(dst);
-- 2.23.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On 11/20/19 6:11 PM, Ville Syrjälä wrote:
On Wed, Nov 20, 2019 at 05:43:40PM +0100, Daniel Vetter wrote:
On Wed, Nov 20, 2019 at 06:25:12PM +0200, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Now that we've constrained the clipped source rectangle such that it can't have negative dimensions doing the same for the dst rectangle seems appropriate. Should at least result in the clipped src and dst rectangles being a bit more consistent with each other.
Cc: Benjamin Gaignard benjamin.gaignard@st.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
selftests for this stuff? Looks like the prime example, write testcase proving code is busted, fix it, everyone celebrate?
Yeah, seems like a good idea. Though I'll have to figure out if it's actually broken or not ;)
Hmm. Ouch. There's seems to be a div by zero lurking in there if dst_w/h == 0. I wonder why nothing has hit that.
At least W=1 warnings have disappear with these patches ;-)
Benjamin
-Daniel
drivers/gpu/drm/drm_rect.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c index 7762b6e9278d..229325fcf333 100644 --- a/drivers/gpu/drm/drm_rect.c +++ b/drivers/gpu/drm/drm_rect.c @@ -52,14 +52,14 @@ bool drm_rect_intersect(struct drm_rect *r1, const struct drm_rect *r2) } EXPORT_SYMBOL(drm_rect_intersect);
-static u32 clip_scaled(u32 src, u32 dst, u32 clip) +static u32 clip_scaled(int src, int dst, int *clip) { u64 tmp;
/* Only clip what we have. Keeps the result bounded as well. */
- clip = min(clip, dst);
- *clip = min(*clip, dst);
- tmp = mul_u32_u32(src, dst - clip);
tmp = mul_u32_u32(src, dst - *clip);
/*
- Round toward 1.0 when clipping so that we don't accidentally
@@ -92,34 +92,34 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, diff = clip->x1 - dst->x1; if (diff > 0) { u32 new_src_w = clip_scaled(drm_rect_width(src),
drm_rect_width(dst), diff);
drm_rect_width(dst), &diff);src->x1 = src->x2 - new_src_w;
dst->x1 = clip->x1;
dst->x1 += diff;
drm_rect_height(dst), diff);
drm_rect_height(dst), &diff);src->y1 = src->y2 - new_src_h;
dst->y1 = clip->y1;
dst->y1 += diff;
drm_rect_width(dst), diff);
drm_rect_width(dst), &diff);src->x2 = src->x1 + new_src_w;
dst->x2 = clip->x2;
dst->x2 -= diff;
drm_rect_height(dst), diff);
drm_rect_height(dst), &diff);src->y2 = src->y1 + new_src_h;
dst->y2 = clip->y2;
dst->y2 -= diff;}
return drm_rect_visible(dst);
-- 2.23.0
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Wed, Nov 20, 2019 at 07:11:38PM +0200, Ville Syrjälä wrote:
On Wed, Nov 20, 2019 at 05:43:40PM +0100, Daniel Vetter wrote:
On Wed, Nov 20, 2019 at 06:25:12PM +0200, Ville Syrjala wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Now that we've constrained the clipped source rectangle such that it can't have negative dimensions doing the same for the dst rectangle seems appropriate. Should at least result in the clipped src and dst rectangles being a bit more consistent with each other.
Cc: Benjamin Gaignard benjamin.gaignard@st.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
selftests for this stuff? Looks like the prime example, write testcase proving code is busted, fix it, everyone celebrate?
Yeah, seems like a good idea. Though I'll have to figure out if it's actually broken or not ;)
I *think* the only problem is that the clip can result in a visible source rectangle when this happens. The dst rectangle will still be correctly invisible so hopefully not a big deal. But I guess we might as well fix it, and I can do a selftest which makes sure both src and dst come out invisible.
Hmm. Ouch. There's seems to be a div by zero lurking in there if dst_w/h == 0. I wonder why nothing has hit that.
Definitely real. I'll fix it and toss in a selftest.
dri-devel@lists.freedesktop.org
-
Benjamin GAIGNARD -
Daniel Vetter -
Ville Syrjala -
Ville Syrjälä