check_overlay_dst for clipped is called 2 times: in drm_rect_intersect
and than directly. Change second call for check of drm_rect_intersect
result to save some time (in locked code section).
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 8d8b2dd3995f ("drm/i915: Make the PIPESRC rect relative to the
entire bigjoiner area")
Signed-off-by: Nikita Kiryushin <[email protected]>
---
drivers/gpu/drm/i915/display/intel_overlay.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c
b/drivers/gpu/drm/i915/display/intel_overlay.c
index 2b1392d5a902..1cda1c163a92 100644
--- a/drivers/gpu/drm/i915/display/intel_overlay.c
+++ b/drivers/gpu/drm/i915/display/intel_overlay.c
@@ -972,9 +972,8 @@ static int check_overlay_dst(struct intel_overlay
*overlay,
rec->dst_width, rec->dst_height);
clipped = req;
- drm_rect_intersect(&clipped, &crtc_state->pipe_src);
- if (!drm_rect_visible(&clipped) ||
+ if (!drm_rect_intersect(&clipped, &crtc_state->pipe_src) ||
!drm_rect_equals(&clipped, &req))
return -EINVAL;
-- 2.34.1
On Wed, Feb 28, 2024 at 09:32:47PM +0300, Nikita Kiryushin wrote:
>
> check_overlay_dst for clipped is called 2 times: in drm_rect_intersect
> and than directly. Change second call for check of drm_rect_intersect
> result to save some time (in locked code section).
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 8d8b2dd3995f ("drm/i915: Make the PIPESRC rect relative to the
> entire bigjoiner area")
> Signed-off-by: Nikita Kiryushin <[email protected]>
> ---
> drivers/gpu/drm/i915/display/intel_overlay.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c
> b/drivers/gpu/drm/i915/display/intel_overlay.c
> index 2b1392d5a902..1cda1c163a92 100644
> --- a/drivers/gpu/drm/i915/display/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
> @@ -972,9 +972,8 @@ static int check_overlay_dst(struct intel_overlay
> *overlay,
> rec->dst_width, rec->dst_height);
> clipped = req;
> - drm_rect_intersect(&clipped, &crtc_state->pipe_src);
> - if (!drm_rect_visible(&clipped) ||
> + if (!drm_rect_intersect(&clipped, &crtc_state->pipe_src) ||
I prefer the current way where we have no side effects in
the if statement.
> !drm_rect_equals(&clipped, &req))
> return -EINVAL;
> -- 2.34.1
--
Ville Syrj?l?
Intel
On 2/29/24 15:30, Ville Syrjälä wrote:
> I prefer the current way where we have no side effects in
> the if statement.
>
This seem like a valid concern from readability and maintainability
standpoint. My patch was aimed mostly at performance and maintainability
using tools: some more pedantic analyzers are sensitive to non-checked
return values (as of now, drm_rect_intersect is ignored).
Would it be a better idea to make an update to the patch with second
drm_rect_visible call changed to an appropriately named state flag set
with drm_rect_intersect result?
BTW, the original patch somehow got mangled while it made its way to the
patchwork: source list line in patch got broken, which permits the patch
from being applied (the original version did not have that line break).
Any ideas how to prevent this happening with the second version of patch
(in case the idea is viable)?
On Fri, Mar 01, 2024 at 09:56:41PM +0300, Nikita Kiryushin wrote:
> On 2/29/24 15:30, Ville Syrj?l? wrote:
> > I prefer the current way where we have no side effects in
> > the if statement.
> >
>
> This seem like a valid concern from readability and maintainability
> standpoint. My patch was aimed mostly at performance and maintainability
> using tools: some more pedantic analyzers are sensitive to non-checked
> return values (as of now, drm_rect_intersect is ignored).
>
> Would it be a better idea to make an update to the patch with second
> drm_rect_visible call changed to an appropriately named state flag set
> with drm_rect_intersect result?
I was thinking of maybe removing that drm_rect_visible() from
drm_rect_intersect() entirely, but looks like it's used fairly
extensively, so would require a bunch of work.
But now that I though about this I recalled that there was an earlier
patch trying to do exactly what you suggested in this patch. And looks
like there was a second version posted which I completely missed:
https://patchwork.freedesktop.org/series/115605/
While that does still have drm_rect_intersect() with its side effects
inside the if() I don't find it quite as objectionable since it's the
only thing in there. So it's a bit more obvious what is happening.
I've gone and merged that one.
Thanks for the patch regardless. At least I reminded me to look at the
earlier attempt ;)
>
> BTW, the original patch somehow got mangled while it made its way to the
> patchwork: source list line in patch got broken, which permits the patch
> from being applied (the original version did not have that line break).
> Any ideas how to prevent this happening with the second version of patch
> (in case the idea is viable)?
--
Ville Syrj?l?
Intel