| Message ID | 20180503112217.37292-3-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 03, 2018 at 01:22:14PM +0200, Maarten Lankhorst wrote: > Instead of relying on a scale which may increase rounding errors, > clip src by doing: src * (dst - clip) / dst and rounding the result > away from 1, so the new coordinates get closer to 1. We won't need > to fix up with a magic macro afterwards, because our scaling factor > will never go to the other side of 1. > > Changes since v1: > - Adjust dst immediately, else drm_rect_width/height on dst gives bogus > results. > Change since v2: > - Get rid of macros and use 64-bits math. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 2 +- > drivers/gpu/drm/drm_rect.c | 45 ++++++++++++++++++++--------- > drivers/gpu/drm/i915/intel_sprite.c | 2 +- > include/drm/drm_rect.h | 3 +- > 4 files changed, 35 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 9cb2209f6fc8..130da5195f3b 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -766,7 +766,7 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, > if (crtc_state->enable) > drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2); > > - plane_state->visible = drm_rect_clip_scaled(src, dst, &clip, hscale, vscale); > + plane_state->visible = drm_rect_clip_scaled(src, dst, &clip); > > drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation); > > diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c > index 4735526297aa..f477063f18ea 100644 > --- a/drivers/gpu/drm/drm_rect.c > +++ b/drivers/gpu/drm/drm_rect.c > @@ -50,13 +50,21 @@ 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) > +{ > + u64 newsrc = mul_u32_u32(src, dst - clip); 'newsrc' feels slightly misleading. It would be a decent name for the final return value of the function, but for this intermediate value 'tmp' or something equally non specific might be better. > + > + if (src < (dst << 16)) > + return DIV_ROUND_UP_ULL(newsrc, dst); > + else > + return DIV_ROUND_DOWN_ULL(newsrc, dst); I think we could use a comment here to explain the choice of rounding direction. Eg. /* * Round toward 1.0 when clipping so that we don't accidentally * change upscaling to downscaling or vice versa. */ ? > +} > + > /** > * drm_rect_clip_scaled - perform a scaled clip operation > * @src: source window rectangle > * @dst: destination window rectangle > * @clip: clip rectangle > - * @hscale: horizontal scaling factor > - * @vscale: vertical scaling factor > * > * Clip rectangle @dst by rectangle @clip. Clip rectangle @src by the > * same amounts multiplied by @hscale and @vscale. > @@ -66,33 +74,44 @@ EXPORT_SYMBOL(drm_rect_intersect); > * %false otherwise > */ > bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, > - const struct drm_rect *clip, > - int hscale, int vscale) > + const struct drm_rect *clip) > { > int diff; > > 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); > + u32 new_src_w = clip_scaled(drm_rect_width(src), > + drm_rect_width(dst), diff); Could have precomputed the src/dst width/height upfront maybe. Oh, I guess you can't since your clip_scaled() uses the dst width/height for more than the scaling factor. If it just computed diff*src/dst like the original code does then precomputing would work just fine. Your way feels a bit more complicated to me, but looks like it should work. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > + > + src->x1 = clamp_t(int64_t, src->x2 - new_src_w, INT_MIN, INT_MAX); > + dst->x1 = clip->x1; > } > 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); > + 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); > + dst->y1 = clip->y1; > } > 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); > + 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); > + dst->x2 = clip->x2; > } > 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); > + 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); > + dst->y2 = clip->y2; > } > > - return drm_rect_intersect(dst, clip); > + return drm_rect_visible(dst); > } > EXPORT_SYMBOL(drm_rect_clip_scaled); > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index aa1dfaa692b9..44d7aca222b0 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -1008,7 +1008,7 @@ intel_check_sprite_plane(struct intel_plane *plane, > drm_mode_get_hv_timing(&crtc_state->base.mode, > &clip.x2, &clip.y2); > > - state->base.visible = drm_rect_clip_scaled(src, dst, &clip, hscale, vscale); > + state->base.visible = drm_rect_clip_scaled(src, dst, &clip); > > crtc_x = dst->x1; > crtc_y = dst->y1; > diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h > index 44bc122b9ee0..6c54544a4be7 100644 > --- a/include/drm/drm_rect.h > +++ b/include/drm/drm_rect.h > @@ -175,8 +175,7 @@ static inline bool drm_rect_equals(const struct drm_rect *r1, > > bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip); > bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, > - const struct drm_rect *clip, > - int hscale, int vscale); > + const struct drm_rect *clip); > int drm_rect_calc_hscale(const struct drm_rect *src, > const struct drm_rect *dst, > int min_hscale, int max_hscale); > -- > 2.17.0
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 9cb2209f6fc8..130da5195f3b 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -766,7 +766,7 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state, if (crtc_state->enable) drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2); - plane_state->visible = drm_rect_clip_scaled(src, dst, &clip, hscale, vscale); + plane_state->visible = drm_rect_clip_scaled(src, dst, &clip); drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, rotation); diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c index 4735526297aa..f477063f18ea 100644 --- a/drivers/gpu/drm/drm_rect.c +++ b/drivers/gpu/drm/drm_rect.c @@ -50,13 +50,21 @@ 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) +{ + u64 newsrc = mul_u32_u32(src, dst - clip); + + if (src < (dst << 16)) + return DIV_ROUND_UP_ULL(newsrc, dst); + else + return DIV_ROUND_DOWN_ULL(newsrc, dst); +} + /** * drm_rect_clip_scaled - perform a scaled clip operation * @src: source window rectangle * @dst: destination window rectangle * @clip: clip rectangle - * @hscale: horizontal scaling factor - * @vscale: vertical scaling factor * * Clip rectangle @dst by rectangle @clip. Clip rectangle @src by the * same amounts multiplied by @hscale and @vscale. @@ -66,33 +74,44 @@ EXPORT_SYMBOL(drm_rect_intersect); * %false otherwise */ bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, - const struct drm_rect *clip, - int hscale, int vscale) + const struct drm_rect *clip) { int diff; 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); + 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); + dst->x1 = clip->x1; } 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); + 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); + dst->y1 = clip->y1; } 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); + 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); + dst->x2 = clip->x2; } 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); + 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); + dst->y2 = clip->y2; } - return drm_rect_intersect(dst, clip); + return drm_rect_visible(dst); } EXPORT_SYMBOL(drm_rect_clip_scaled); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index aa1dfaa692b9..44d7aca222b0 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1008,7 +1008,7 @@ intel_check_sprite_plane(struct intel_plane *plane, drm_mode_get_hv_timing(&crtc_state->base.mode, &clip.x2, &clip.y2); - state->base.visible = drm_rect_clip_scaled(src, dst, &clip, hscale, vscale); + state->base.visible = drm_rect_clip_scaled(src, dst, &clip); crtc_x = dst->x1; crtc_y = dst->y1; diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h index 44bc122b9ee0..6c54544a4be7 100644 --- a/include/drm/drm_rect.h +++ b/include/drm/drm_rect.h @@ -175,8 +175,7 @@ static inline bool drm_rect_equals(const struct drm_rect *r1, bool drm_rect_intersect(struct drm_rect *r, const struct drm_rect *clip); bool drm_rect_clip_scaled(struct drm_rect *src, struct drm_rect *dst, - const struct drm_rect *clip, - int hscale, int vscale); + const struct drm_rect *clip); int drm_rect_calc_hscale(const struct drm_rect *src, const struct drm_rect *dst, int min_hscale, int max_hscale);
Instead of relying on a scale which may increase rounding errors, clip src by doing: src * (dst - clip) / dst and rounding the result away from 1, so the new coordinates get closer to 1. We won't need to fix up with a magic macro afterwards, because our scaling factor will never go to the other side of 1. Changes since v1: - Adjust dst immediately, else drm_rect_width/height on dst gives bogus results. Change since v2: - Get rid of macros and use 64-bits math. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/drm_atomic_helper.c | 2 +- drivers/gpu/drm/drm_rect.c | 45 ++++++++++++++++++++--------- drivers/gpu/drm/i915/intel_sprite.c | 2 +- include/drm/drm_rect.h | 3 +- 4 files changed, 35 insertions(+), 17 deletions(-)