[PATCH] drm/atmel-hlcdc: prevent divide by zero

[PATCH] drm/atmel-hlcdc: prevent divide by zero

[Peter Rosin]

While trying to temporarily hide a plane, one thing that was attempted
was to call (from libdrm)

	drmModeSetPlane(fd, plane_id, crtc_id, fb_id, 0,
			0, 0, 0, 0, 0, 0, 0, 0);

This call causes a pair of "Division by zero in kernel." messages. Kill
those messages, but preserve the current behaviour that also happen to
make the plane disappear with the above call.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Side note, when comparing with drm_atomic_helper_check_plane_state(), I get
the feeling that the src rect should be clipped together with the crtc rect
if/when clipping is needed. That function calls drm_rect_clip_scaled(), and
the equivalent does not seem to happen here. Should clipping be performed
on the src rect?

Cheers,
Peter

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
index 3cc489b870fe..1bdb30dc218c 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
@@ -675,10 +675,16 @@ static int atmel_hlcdc_plane_atomic_check(struct drm_plane *p,
 		state->crtc_y = 0;
 	}
 
-	patched_src_w = DIV_ROUND_CLOSEST(patched_crtc_w * state->src_w,
-					  state->crtc_w);
-	patched_src_h = DIV_ROUND_CLOSEST(patched_crtc_h * state->src_h,
-					  state->crtc_h);
+	if (state->crtc_w)
+		patched_src_w = DIV_ROUND_CLOSEST(patched_crtc_w * state->src_w,
+						  state->crtc_w);
+	else
+		patched_src_w = 0;
+	if (state->crtc_h)
+		patched_src_h = DIV_ROUND_CLOSEST(patched_crtc_h * state->src_h,
+						  state->crtc_h);
+	else
+		patched_src_h = 0;
 
 	hsub = drm_format_horz_chroma_subsampling(fb->format->format);
 	vsub = drm_format_vert_chroma_subsampling(fb->format->format);
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drm/atmel-hlcdc: prevent divide by zero

[Daniel Vetter]

On Tue, Jan 08, 2019 at 12:31:36PM +0000, Peter Rosin wrote:
> While trying to temporarily hide a plane, one thing that was attempted
> was to call (from libdrm)
> 
> 	drmModeSetPlane(fd, plane_id, crtc_id, fb_id, 0,
> 			0, 0, 0, 0, 0, 0, 0, 0);
> 
> This call causes a pair of "Division by zero in kernel." messages. Kill
> those messages, but preserve the current behaviour that also happen to
> make the plane disappear with the above call.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> Side note, when comparing with drm_atomic_helper_check_plane_state(), I get
> the feeling that the src rect should be clipped together with the crtc rect
> if/when clipping is needed. That function calls drm_rect_clip_scaled(), and
> the equivalent does not seem to happen here. Should clipping be performed
> on the src rect?

Any reasons atmel can't switch over to that helper? Would compute a nice
->visible state variable for you ...
-Daniel

> 
> Cheers,
> Peter
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> index 3cc489b870fe..1bdb30dc218c 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
> @@ -675,10 +675,16 @@ static int atmel_hlcdc_plane_atomic_check(struct drm_plane *p,
>  		state->crtc_y = 0;
>  	}
>  
> -	patched_src_w = DIV_ROUND_CLOSEST(patched_crtc_w * state->src_w,
> -					  state->crtc_w);
> -	patched_src_h = DIV_ROUND_CLOSEST(patched_crtc_h * state->src_h,
> -					  state->crtc_h);
> +	if (state->crtc_w)
> +		patched_src_w = DIV_ROUND_CLOSEST(patched_crtc_w * state->src_w,
> +						  state->crtc_w);
> +	else
> +		patched_src_w = 0;
> +	if (state->crtc_h)
> +		patched_src_h = DIV_ROUND_CLOSEST(patched_crtc_h * state->src_h,
> +						  state->crtc_h);
> +	else
> +		patched_src_h = 0;
>  
>  	hsub = drm_format_horz_chroma_subsampling(fb->format->format);
>  	vsub = drm_format_vert_chroma_subsampling(fb->format->format);
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drm/atmel-hlcdc: prevent divide by zero

[Peter Rosin]

On 2019-01-09 11:12, Daniel Vetter wrote:
> On Tue, Jan 08, 2019 at 12:31:36PM +0000, Peter Rosin wrote:
>> While trying to temporarily hide a plane, one thing that was attempted
>> was to call (from libdrm)
>>
>> 	drmModeSetPlane(fd, plane_id, crtc_id, fb_id, 0,
>> 			0, 0, 0, 0, 0, 0, 0, 0);
>>
>> This call causes a pair of "Division by zero in kernel." messages. Kill
>> those messages, but preserve the current behaviour that also happen to
>> make the plane disappear with the above call.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> Side note, when comparing with drm_atomic_helper_check_plane_state(), I get
>> the feeling that the src rect should be clipped together with the crtc rect
>> if/when clipping is needed. That function calls drm_rect_clip_scaled(), and
>> the equivalent does not seem to happen here. Should clipping be performed
>> on the src rect?
> 
> Any reasons atmel can't switch over to that helper? Would compute a nice
> ->visible state variable for you ...
> -Daniel

I have not researched that, and as stated above, I was not sure if that was the
correct approach to begin with. Boris, any insights? Does this code predate the
helper or something?

Maybe there's some register bit that hides a plane explicitly, matching the
->visible state variable? I haven't looked at that either...

Cheers,
Peter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drm/atmel-hlcdc: prevent divide by zero

[Boris Brezillon]

On Wed, 9 Jan 2019 11:37:19 +0000
Peter Rosin <peda@axentia.se> wrote:

> On 2019-01-09 11:12, Daniel Vetter wrote:
> > On Tue, Jan 08, 2019 at 12:31:36PM +0000, Peter Rosin wrote:  
> >> While trying to temporarily hide a plane, one thing that was attempted
> >> was to call (from libdrm)
> >>
> >> 	drmModeSetPlane(fd, plane_id, crtc_id, fb_id, 0,
> >> 			0, 0, 0, 0, 0, 0, 0, 0);
> >>
> >> This call causes a pair of "Division by zero in kernel." messages. Kill
> >> those messages, but preserve the current behaviour that also happen to
> >> make the plane disappear with the above call.
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 14 ++++++++++----
> >>  1 file changed, 10 insertions(+), 4 deletions(-)
> >>
> >> Side note, when comparing with drm_atomic_helper_check_plane_state(), I get
> >> the feeling that the src rect should be clipped together with the crtc rect
> >> if/when clipping is needed. That function calls drm_rect_clip_scaled(), and
> >> the equivalent does not seem to happen here. Should clipping be performed
> >> on the src rect?  
> > 
> > Any reasons atmel can't switch over to that helper? Would compute a nice  
> > ->visible state variable for you ...  
> > -Daniel  
> 
> I have not researched that, and as stated above, I was not sure if that was the
> correct approach to begin with. Boris, any insights?

You can look at vc4_plane.c if you want an example of how this helper
can be used to get clipped dimensions of the plane.

> Does this code predate the
> helper or something?

Yes, and I must admit I'm not actively maintaining the driver, so there
are probably a few other things we could simplify by using generic
helpers.

> 
> Maybe there's some register bit that hides a plane explicitly, matching the
> ->visible state variable? I haven't looked at that either...

I don't think so, but you can simply disable the plane when ->visible
is false.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] drm/atmel-hlcdc: prevent divide by zero

[Boris Brezillon]

On Wed, 9 Jan 2019 11:12:24 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Jan 08, 2019 at 12:31:36PM +0000, Peter Rosin wrote:
> > While trying to temporarily hide a plane, one thing that was attempted
> > was to call (from libdrm)
> > 
> > 	drmModeSetPlane(fd, plane_id, crtc_id, fb_id, 0,
> > 			0, 0, 0, 0, 0, 0, 0, 0);
> > 
> > This call causes a pair of "Division by zero in kernel." messages. Kill
> > those messages, but preserve the current behaviour that also happen to
> > make the plane disappear with the above call.
> > 
> > Signed-off-by: Peter Rosin <peda@axentia.se>
> > ---
> >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> > 
> > Side note, when comparing with drm_atomic_helper_check_plane_state(), I get
> > the feeling that the src rect should be clipped together with the crtc rect
> > if/when clipping is needed. That function calls drm_rect_clip_scaled(), and
> > the equivalent does not seem to happen here. Should clipping be performed
> > on the src rect?  
> 
> Any reasons atmel can't switch over to that helper? Would compute a nice
> ->visible state variable for you ...  

We should definitely use drm_atomic_helper_check_plane_state().

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel