| Message ID | 20191120162512.12484-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/rect: Keep the scaled clip bounded | expand |
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; > } > 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); > -- > 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; > > } > > 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); > > -- > > 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; >>> } >>> 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); >>> -- >>> 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.
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);