| Message ID | 20180423104922.6557-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 23, 2018 at 12:49:22PM +0200, Maarten Lankhorst wrote: > The rounding can cause a perfectly normal 16x16 src to full-screen > dst to be rounded down, even without clipping involved. Because of > this we should just remove the adjustment, as no other driver or plane > does it. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_sprite.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index a1d048af0261..203ca8b362a5 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -996,11 +996,6 @@ intel_check_sprite_plane(struct intel_plane *plane, > return vscale; > } > > - /* Make the source viewport size an exact multiple of the scaling factors. */ > - drm_rect_adjust_size(src, > - drm_rect_width(dst) * hscale - drm_rect_width(src), > - drm_rect_height(dst) * vscale - drm_rect_height(src)); > - This makes the scaling factor checks a slightly incorrect. Ie. we might exceed the max h/vscale a bit without realizing it. It's a shame the hardware doesn't let us actually program the scaling factors/increments and starting phases anymore :( Also we don't actually know how the hardware calculates that stuff (assuming it has such things internally), so I'm not actually sure how we should be checking the max limit so that we actually check what the hardware will use. Without a more detailed study of the hardware behaviour I'm thinking we should perhaps just check the final src vs. dst coordinates like so: if (src > dst*max) fail; as that would avoid the precision issues with the .16 scaling factors. Another option could be to round h/vscale up. That should guarantee that we never exceed the max. > drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, > state->base.rotation); > > -- > 2.17.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 23-04-18 om 16:30 schreef Ville Syrjälä: > On Mon, Apr 23, 2018 at 12:49:22PM +0200, Maarten Lankhorst wrote: >> The rounding can cause a perfectly normal 16x16 src to full-screen >> dst to be rounded down, even without clipping involved. Because of >> this we should just remove the adjustment, as no other driver or plane >> does it. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_sprite.c | 5 ----- >> 1 file changed, 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c >> index a1d048af0261..203ca8b362a5 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -996,11 +996,6 @@ intel_check_sprite_plane(struct intel_plane *plane, >> return vscale; >> } >> >> - /* Make the source viewport size an exact multiple of the scaling factors. */ >> - drm_rect_adjust_size(src, >> - drm_rect_width(dst) * hscale - drm_rect_width(src), >> - drm_rect_height(dst) * vscale - drm_rect_height(src)); >> - > This makes the scaling factor checks a slightly incorrect. Ie. we might > exceed the max h/vscale a bit without realizing it. It's a shame the > hardware doesn't let us actually program the scaling factors/increments > and starting phases anymore :( Also we don't actually know how the > hardware calculates that stuff (assuming it has such things internally), > so I'm not actually sure how we should be checking the max limit so > that we actually check what the hardware will use. > > Without a more detailed study of the hardware behaviour I'm thinking > we should perhaps just check the final src vs. dst coordinates like so: > if (src > dst*max) > fail; > as that would avoid the precision issues with the .16 scaling factors. > > Another option could be to round h/vscale up. That should guarantee > that we never exceed the max. Could we take a pessimistic view for both sides? Round up if scaling > 1, round down when scaling < 1? That way we should never be afraid of any limits.. ~Maarten
On Mon, Apr 23, 2018 at 04:39:54PM +0200, Maarten Lankhorst wrote: > Op 23-04-18 om 16:30 schreef Ville Syrjälä: > > On Mon, Apr 23, 2018 at 12:49:22PM +0200, Maarten Lankhorst wrote: > >> The rounding can cause a perfectly normal 16x16 src to full-screen > >> dst to be rounded down, even without clipping involved. Because of > >> this we should just remove the adjustment, as no other driver or plane > >> does it. > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_sprite.c | 5 ----- > >> 1 file changed, 5 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > >> index a1d048af0261..203ca8b362a5 100644 > >> --- a/drivers/gpu/drm/i915/intel_sprite.c > >> +++ b/drivers/gpu/drm/i915/intel_sprite.c > >> @@ -996,11 +996,6 @@ intel_check_sprite_plane(struct intel_plane *plane, > >> return vscale; > >> } > >> > >> - /* Make the source viewport size an exact multiple of the scaling factors. */ > >> - drm_rect_adjust_size(src, > >> - drm_rect_width(dst) * hscale - drm_rect_width(src), > >> - drm_rect_height(dst) * vscale - drm_rect_height(src)); > >> - > > This makes the scaling factor checks a slightly incorrect. Ie. we might > > exceed the max h/vscale a bit without realizing it. It's a shame the > > hardware doesn't let us actually program the scaling factors/increments > > and starting phases anymore :( Also we don't actually know how the > > hardware calculates that stuff (assuming it has such things internally), > > so I'm not actually sure how we should be checking the max limit so > > that we actually check what the hardware will use. > > > > Without a more detailed study of the hardware behaviour I'm thinking > > we should perhaps just check the final src vs. dst coordinates like so: > > if (src > dst*max) > > fail; > > as that would avoid the precision issues with the .16 scaling factors. > > > > Another option could be to round h/vscale up. That should guarantee > > that we never exceed the max. > Could we take a pessimistic view for both sides? Round up if scaling > 1, round down when scaling < 1? > That way we should never be afraid of any limits.. That does sound like a decent idea to me. At least I can't immediately think why it wouldn't work out.
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index a1d048af0261..203ca8b362a5 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -996,11 +996,6 @@ intel_check_sprite_plane(struct intel_plane *plane, return vscale; } - /* Make the source viewport size an exact multiple of the scaling factors. */ - drm_rect_adjust_size(src, - drm_rect_width(dst) * hscale - drm_rect_width(src), - drm_rect_height(dst) * vscale - drm_rect_height(src)); - drm_rect_rotate_inv(src, fb->width << 16, fb->height << 16, state->base.rotation);
The rounding can cause a perfectly normal 16x16 src to full-screen dst to be rounded down, even without clipping involved. Because of this we should just remove the adjustment, as no other driver or plane does it. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_sprite.c | 5 ----- 1 file changed, 5 deletions(-)