2019-01-08 12:34:04

by Peter Rosin

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

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 <[email protected]>
---
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



2019-01-09 10:36:41

by Daniel Vetter

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

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 <[email protected]>
> ---
> 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2019-01-09 11:40:48

by Boris Brezillon

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

On Wed, 9 Jan 2019 11:12:24 +0100
Daniel Vetter <[email protected]> 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 <[email protected]>
> > ---
> > 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().

2019-01-09 11:52:00

by Peter Rosin

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

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 <[email protected]>
>> ---
>> 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

2019-01-09 12:23:24

by Boris Brezillon

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

On Wed, 9 Jan 2019 11:37:19 +0000
Peter Rosin <[email protected]> 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 <[email protected]>
> >> ---
> >> 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.