diff mbox series

drm/i915: Remove unneeded double drm_rect_visible call in check_overlay_dst

Message ID 79c92cfa-cf5a-4a23-8a93-11c1af7432fc@ancud.ru
State New
Headers
Series drm/i915: Remove unneeded double drm_rect_visible call in check_overlay_dst |

Commit Message

Nikita Kiryushin Feb. 28, 2024, 6:32 p.m. UTC 
  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 <kiryushin@ancud.ru>
---
  drivers/gpu/drm/i915/display/intel_overlay.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Ville Syrjälä Feb. 29, 2024, 12:30 p.m. UTC | #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 <kiryushin@ancud.ru>
> ---
>   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
Nikita Kiryushin March 1, 2024, 6:56 p.m. UTC | #2 
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)?
Ville Syrjälä March 4, 2024, 11:11 a.m. UTC | #3 
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)?
diff mbox series

Patch

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