| Message ID | 20190108123129.20031-1-peda@axentia.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/atmel-hlcdc: prevent divide by zero | expand |
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
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
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().
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.
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);
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