| Message ID | 20180426082821.11129-2-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 26, 2018 at 10:28:20AM +0200, Maarten Lankhorst wrote: > No matter how you perform the clip adjustments, a small > error may push the scaling factor to the other side of > 0x10000. Solve this with a macro that will fixup the > scale to 0x10000 if we accidentally wrap to the other side. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> I think this and the previous patch are perfect candidates for in-kernel selftests. Can I volunteer you to get those started for the kms side? We already have a drm_mm selftest, but I think splitting things a bit might be useful. Or we rename that one and just stuff all the kms tests in there, dunno. -Daniel > --- > drivers/gpu/drm/drm_rect.c | 65 +++++++++++++++++++++++++++----------- > 1 file changed, 47 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c > index b179c7c73cc5..71b6b7f5d58f 100644 > --- a/drivers/gpu/drm/drm_rect.c > +++ b/drivers/gpu/drm/drm_rect.c > @@ -50,6 +50,24 @@ bool drm_rect_intersect(struct drm_rect *r1, const struct drm_rect *r2) > } > EXPORT_SYMBOL(drm_rect_intersect); > > +static int drm_calc_scale(int src, int dst) > +{ > + int scale = 0; > + > + if (WARN_ON(src < 0 || dst < 0)) > + return -EINVAL; > + > + if (dst == 0) > + return 0; > + > + if (src > (dst << 16)) > + return DIV_ROUND_UP(src, dst); > + else > + scale = src / dst; > + > + return scale; > +} > + > /** > * drm_rect_clip_scaled - perform a scaled clip operation > * @src: source window rectangle > @@ -71,49 +89,60 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, > { > int diff; > > + /* > + * When scale is near 0x10000 rounding errors may cause the scaling > + * factor to the other side. Some hardware may support > + * upsampling, but not downsampling, and that would break when > + * rounding. > + */ > +#define FIXUP(oldscale, fn, m, second) do { \ > + if (oldscale != 1 << 16) { \ > + int newscale = drm_calc_scale(fn(src), fn(dst)); \ > + \ > + if (newscale < 0) \ > + return false; \ > + \ > + if ((oldscale < 0x10000) != (newscale < 0x10000)) { \ > + if (!second) \ > + src->m##1 = src->m##2 - ((fn(dst) - diff) << 16); \ > + else \ > + src->m##2 = src->m##1 + ((fn(dst) - diff) << 16); \ > + } \ > + } \ > + } while (0) > + > diff = clip->x1 - dst->x1; > if (diff > 0) { > int64_t tmp = src->x1 + (int64_t) diff * hscale; > src->x1 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX); > + FIXUP(hscale, drm_rect_width, x, 0); > } > + > diff = clip->y1 - dst->y1; > if (diff > 0) { > int64_t tmp = src->y1 + (int64_t) diff * vscale; > src->y1 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX); > + FIXUP(vscale, drm_rect_height, y, 0); > } > + > diff = dst->x2 - clip->x2; > if (diff > 0) { > int64_t tmp = src->x2 - (int64_t) diff * hscale; > src->x2 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX); > + FIXUP(hscale, drm_rect_width, x, 1); > } > diff = dst->y2 - clip->y2; > if (diff > 0) { > int64_t tmp = src->y2 - (int64_t) diff * vscale; > src->y2 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX); > + FIXUP(vscale, drm_rect_height, y, 1); > } > +#undef FIXUP > > return drm_rect_intersect(dst, clip); > } > EXPORT_SYMBOL(drm_rect_clip_scaled); > > -static int drm_calc_scale(int src, int dst) > -{ > - int scale = 0; > - > - if (WARN_ON(src < 0 || dst < 0)) > - return -EINVAL; > - > - if (dst == 0) > - return 0; > - > - if (src > (dst << 16)) > - return DIV_ROUND_UP(src, dst); > - else > - scale = src / dst; > - > - return scale; > -} > - > /** > * drm_rect_calc_hscale - calculate the horizontal scaling factor > * @src: source window rectangle > -- > 2.17.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Apr 26, 2018 at 10:28:20AM +0200, Maarten Lankhorst wrote: > No matter how you perform the clip adjustments, a small > error may push the scaling factor to the other side of > 0x10000. Solve this with a macro that will fixup the > scale to 0x10000 if we accidentally wrap to the other side. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/drm_rect.c | 65 +++++++++++++++++++++++++++----------- > 1 file changed, 47 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c > index b179c7c73cc5..71b6b7f5d58f 100644 > --- a/drivers/gpu/drm/drm_rect.c > +++ b/drivers/gpu/drm/drm_rect.c > @@ -50,6 +50,24 @@ bool drm_rect_intersect(struct drm_rect *r1, const struct drm_rect *r2) > } > EXPORT_SYMBOL(drm_rect_intersect); > > +static int drm_calc_scale(int src, int dst) > +{ > + int scale = 0; > + > + if (WARN_ON(src < 0 || dst < 0)) > + return -EINVAL; > + > + if (dst == 0) > + return 0; > + > + if (src > (dst << 16)) > + return DIV_ROUND_UP(src, dst); > + else > + scale = src / dst; > + > + return scale; > +} > + > /** > * drm_rect_clip_scaled - perform a scaled clip operation > * @src: source window rectangle > @@ -71,49 +89,60 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, > { > int diff; > > + /* > + * When scale is near 0x10000 rounding errors may cause the scaling > + * factor to the other side. Some hardware may support > + * upsampling, but not downsampling, and that would break when > + * rounding. > + */ > +#define FIXUP(oldscale, fn, m, second) do { \ > + if (oldscale != 1 << 16) { \ > + int newscale = drm_calc_scale(fn(src), fn(dst)); \ > + \ > + if (newscale < 0) \ > + return false; \ > + \ > + if ((oldscale < 0x10000) != (newscale < 0x10000)) { \ > + if (!second) \ > + src->m##1 = src->m##2 - ((fn(dst) - diff) << 16); \ > + else \ > + src->m##2 = src->m##1 + ((fn(dst) - diff) << 16); \ This is starting to get very messy. Also are we sure the x2/y2 can't go past the initial value here? It's starting to feel like we should do the clip with the rounding the orher way, which should guarantee we never flip between up and down scaling by accident. And then just do a post-clip check for final scale factor with the pessimistic rounding direction to make sure we stay within the limits. > + } \ > + } \ > + } while (0) > + > diff = clip->x1 - dst->x1; > if (diff > 0) { > int64_t tmp = src->x1 + (int64_t) diff * hscale; > src->x1 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX); > + FIXUP(hscale, drm_rect_width, x, 0); > } > + > diff = clip->y1 - dst->y1; > if (diff > 0) { > int64_t tmp = src->y1 + (int64_t) diff * vscale; > src->y1 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX); > + FIXUP(vscale, drm_rect_height, y, 0); > } > + > diff = dst->x2 - clip->x2; > if (diff > 0) { > int64_t tmp = src->x2 - (int64_t) diff * hscale; > src->x2 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX); > + FIXUP(hscale, drm_rect_width, x, 1); > } > diff = dst->y2 - clip->y2; > if (diff > 0) { > int64_t tmp = src->y2 - (int64_t) diff * vscale; > src->y2 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX); > + FIXUP(vscale, drm_rect_height, y, 1); > } > +#undef FIXUP > > return drm_rect_intersect(dst, clip); > } > EXPORT_SYMBOL(drm_rect_clip_scaled); > > -static int drm_calc_scale(int src, int dst) > -{ > - int scale = 0; > - > - if (WARN_ON(src < 0 || dst < 0)) > - return -EINVAL; > - > - if (dst == 0) > - return 0; > - > - if (src > (dst << 16)) > - return DIV_ROUND_UP(src, dst); > - else > - scale = src / dst; > - > - return scale; > -} > - > /** > * drm_rect_calc_hscale - calculate the horizontal scaling factor > * @src: source window rectangle > -- > 2.17.0
diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c index b179c7c73cc5..71b6b7f5d58f 100644 --- a/drivers/gpu/drm/drm_rect.c +++ b/drivers/gpu/drm/drm_rect.c @@ -50,6 +50,24 @@ bool drm_rect_intersect(struct drm_rect *r1, const struct drm_rect *r2) } EXPORT_SYMBOL(drm_rect_intersect); +static int drm_calc_scale(int src, int dst) +{ + int scale = 0; + + if (WARN_ON(src < 0 || dst < 0)) + return -EINVAL; + + if (dst == 0) + return 0; + + if (src > (dst << 16)) + return DIV_ROUND_UP(src, dst); + else + scale = src / dst; + + return scale; +} + /** * drm_rect_clip_scaled - perform a scaled clip operation * @src: source window rectangle @@ -71,49 +89,60 @@ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, { int diff; + /* + * When scale is near 0x10000 rounding errors may cause the scaling + * factor to the other side. Some hardware may support + * upsampling, but not downsampling, and that would break when + * rounding. + */ +#define FIXUP(oldscale, fn, m, second) do { \ + if (oldscale != 1 << 16) { \ + int newscale = drm_calc_scale(fn(src), fn(dst)); \ + \ + if (newscale < 0) \ + return false; \ + \ + if ((oldscale < 0x10000) != (newscale < 0x10000)) { \ + if (!second) \ + src->m##1 = src->m##2 - ((fn(dst) - diff) << 16); \ + else \ + src->m##2 = src->m##1 + ((fn(dst) - diff) << 16); \ + } \ + } \ + } while (0) + diff = clip->x1 - dst->x1; if (diff > 0) { int64_t tmp = src->x1 + (int64_t) diff * hscale; src->x1 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX); + FIXUP(hscale, drm_rect_width, x, 0); } + diff = clip->y1 - dst->y1; if (diff > 0) { int64_t tmp = src->y1 + (int64_t) diff * vscale; src->y1 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX); + FIXUP(vscale, drm_rect_height, y, 0); } + diff = dst->x2 - clip->x2; if (diff > 0) { int64_t tmp = src->x2 - (int64_t) diff * hscale; src->x2 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX); + FIXUP(hscale, drm_rect_width, x, 1); } diff = dst->y2 - clip->y2; if (diff > 0) { int64_t tmp = src->y2 - (int64_t) diff * vscale; src->y2 = clamp_t(int64_t, tmp, INT_MIN, INT_MAX); + FIXUP(vscale, drm_rect_height, y, 1); } +#undef FIXUP return drm_rect_intersect(dst, clip); } EXPORT_SYMBOL(drm_rect_clip_scaled); -static int drm_calc_scale(int src, int dst) -{ - int scale = 0; - - if (WARN_ON(src < 0 || dst < 0)) - return -EINVAL; - - if (dst == 0) - return 0; - - if (src > (dst << 16)) - return DIV_ROUND_UP(src, dst); - else - scale = src / dst; - - return scale; -} - /** * drm_rect_calc_hscale - calculate the horizontal scaling factor * @src: source window rectangle
No matter how you perform the clip adjustments, a small error may push the scaling factor to the other side of 0x10000. Solve this with a macro that will fixup the scale to 0x10000 if we accidentally wrap to the other side. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/drm_rect.c | 65 +++++++++++++++++++++++++++----------- 1 file changed, 47 insertions(+), 18 deletions(-)