* Re: [Intel-gfx] [PATCH 2/2] drm/rect: Keep the clipped dst rectangle in place
@ 2019-11-20 16:43 ` Daniel Vetter
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Vetter @ 2019-11-20 16:43 UTC (permalink / raw)
To: Ville Syrjala; +Cc: intel-gfx, Benjamin Gaignard, dri-devel
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Intel-gfx] [PATCH 2/2] drm/rect: Keep the clipped dst rectangle in place
@ 2019-11-20 16:43 ` Daniel Vetter
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Vetter @ 2019-11-20 16:43 UTC (permalink / raw)
To: Ville Syrjala; +Cc: intel-gfx, Benjamin Gaignard, dri-devel
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Intel-gfx] [PATCH 2/2] drm/rect: Keep the clipped dst rectangle in place
@ 2019-11-20 17:11 ` Ville Syrjälä
0 siblings, 0 replies; 18+ messages in thread
From: Ville Syrjälä @ 2019-11-20 17:11 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, Benjamin Gaignard, dri-devel
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
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Intel-gfx] [PATCH 2/2] drm/rect: Keep the clipped dst rectangle in place
@ 2019-11-20 17:11 ` Ville Syrjälä
0 siblings, 0 replies; 18+ messages in thread
From: Ville Syrjälä @ 2019-11-20 17:11 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, Benjamin Gaignard, dri-devel
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
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/2] drm/rect: Keep the clipped dst rectangle in place
@ 2019-11-20 19:55 ` Benjamin GAIGNARD
0 siblings, 0 replies; 18+ messages in thread
From: Benjamin GAIGNARD @ 2019-11-20 19:55 UTC (permalink / raw)
To: Ville Syrjälä, Daniel Vetter
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Intel-gfx] [PATCH 2/2] drm/rect: Keep the clipped dst rectangle in place
@ 2019-11-20 19:55 ` Benjamin GAIGNARD
0 siblings, 0 replies; 18+ messages in thread
From: Benjamin GAIGNARD @ 2019-11-20 19:55 UTC (permalink / raw)
To: Ville Syrjälä, Daniel Vetter
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Intel-gfx] [PATCH 2/2] drm/rect: Keep the clipped dst rectangle in place
@ 2019-11-20 19:55 ` Benjamin GAIGNARD
0 siblings, 0 replies; 18+ messages in thread
From: Benjamin GAIGNARD @ 2019-11-20 19:55 UTC (permalink / raw)
To: Ville Syrjälä, Daniel Vetter
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/rect: Keep the clipped dst rectangle in place
@ 2019-11-22 17:25 ` Ville Syrjälä
0 siblings, 0 replies; 18+ messages in thread
From: Ville Syrjälä @ 2019-11-22 17:25 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, Benjamin Gaignard, dri-devel
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.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Intel-gfx] [PATCH 2/2] drm/rect: Keep the clipped dst rectangle in place
@ 2019-11-22 17:25 ` Ville Syrjälä
0 siblings, 0 replies; 18+ messages in thread
From: Ville Syrjälä @ 2019-11-22 17:25 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, Benjamin Gaignard, dri-devel
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.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Intel-gfx] [PATCH 2/2] drm/rect: Keep the clipped dst rectangle in place
@ 2019-11-22 17:25 ` Ville Syrjälä
0 siblings, 0 replies; 18+ messages in thread
From: Ville Syrjälä @ 2019-11-22 17:25 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, Benjamin Gaignard, dri-devel
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.
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 18+ messages in thread