| Message ID | 20230104124448.7418-1-juhapekka.heikkila@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: assume some pixelrate for src smaller than 1 | expand |
On Wed, 04 Jan 2023, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote: > intel_adjusted_rate() didn't take into account src rectangle > can be less than 1 in width or height. > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > --- > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > index 10e1fc9d0698..a9948e8d3543 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > @@ -144,7 +144,7 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src, > const struct drm_rect *dst, > unsigned int rate) > { > - unsigned int src_w, src_h, dst_w, dst_h; > + unsigned int src_w, src_h, dst_w, dst_h, dst_wh; > > src_w = drm_rect_width(src) >> 16; > src_h = drm_rect_height(src) >> 16; > @@ -155,8 +155,10 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src, > dst_w = min(src_w, dst_w); > dst_h = min(src_h, dst_h); > > - return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), > - dst_w * dst_h); > + /* in case src contained only fractional part */ > + dst_wh = max(dst_w * dst_h, (unsigned) 1); The options are to use 1U or max_t(), but please don't cast the parameters of max(). Side note, the explicit "unsigned int" is always preferred over the implicit "unsigned". BR, Jani. > + > + return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), dst_wh); > } > > unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
On Wed, Jan 04, 2023 at 02:44:48PM +0200, Juha-Pekka Heikkila wrote: > intel_adjusted_rate() didn't take into account src rectangle > can be less than 1 in width or height. This should not get called in those cases. What does the backtrace look like? > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > --- > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > index 10e1fc9d0698..a9948e8d3543 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > @@ -144,7 +144,7 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src, > const struct drm_rect *dst, > unsigned int rate) > { > - unsigned int src_w, src_h, dst_w, dst_h; > + unsigned int src_w, src_h, dst_w, dst_h, dst_wh; > > src_w = drm_rect_width(src) >> 16; > src_h = drm_rect_height(src) >> 16; > @@ -155,8 +155,10 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src, > dst_w = min(src_w, dst_w); > dst_h = min(src_h, dst_h); > > - return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), > - dst_w * dst_h); > + /* in case src contained only fractional part */ > + dst_wh = max(dst_w * dst_h, (unsigned) 1); > + > + return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), dst_wh); > } > > unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state, > -- > 2.37.3
On Wed, Jan 11, 2023 at 04:19:00PM +0200, Ville Syrjälä wrote: > On Wed, Jan 04, 2023 at 02:44:48PM +0200, Juha-Pekka Heikkila wrote: > > intel_adjusted_rate() didn't take into account src rectangle > > can be less than 1 in width or height. > > This should not get called in those cases. What does the > backtrace look like? In my repro of this issue, the backtrace looks as follows: [ 180.798331] RIP: 0010:intel_plane_pixel_rate+0x4a/0x53 [ 180.798336] Code: <snip long line> [ 180.798338] RSP: 0018:ffffb080ce4179b8 EFLAGS: 00010246 [ 180.798341] RAX: ffffffffffffffff RBX: ffff98cd22a24000 RCX: 0000000000000a00 [ 180.798343] RDX: 0000000000000000 RSI: ffff98cccbae7000 RDI: 0000000000000000 [ 180.798346] RBP: ffffb080ce4179b8 R08: 0000000000087780 R09: 0000000000000002 [ 180.798348] R10: 0000000000000a00 R11: 0000000000000000 R12: 0000000000000000 [ 180.798350] R13: ffff98cd0e495400 R14: ffff98ccc34e0000 R15: ffff98cccbae7000 [ 180.798352] FS: 00007b84119b5000(0000) GS:ffff98d02f900000(0000) knlGS:0000000000000000 [ 180.798354] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 180.798357] CR2: 00007ffc2d5e4080 CR3: 00000001042ee006 CR4: 0000000000770ee0 [ 180.798359] PKRU: 55555554 [ 180.798361] Call Trace: [ 180.798364] <TASK> [ 180.798366] intel_plane_atomic_check_with_state+0x1fd/0x6ea [ 180.798370] ? intel_plane_atomic_check+0x11b/0x145 [ 180.798373] intel_atomic_check_planes+0x263/0x7ce [ 180.798376] ? drm_atomic_helper_check_modeset+0x189/0x923 [ 180.798380] intel_atomic_check+0x14e4/0x184d [ 180.798382] ? intel_rps_mark_interactive+0x23/0x6a [ 180.798386] drm_atomic_check_only+0x3ec/0x98f [ 180.798391] drm_atomic_commit+0xa2/0x105 [ 180.798394] ? drm_atomic_set_fb_for_plane+0x96/0xa5 [ 180.798397] drm_atomic_helper_update_plane+0xdc/0x11f [ 180.798400] drm_mode_setplane+0x236/0x30c [ 180.798404] ? drm_any_plane_has_format+0x51/0x51 [ 180.798407] drm_ioctl_kernel+0xda/0x14d [ 180.798411] drm_ioctl+0x27e/0x3b4 [ 180.798414] ? drm_any_plane_has_format+0x51/0x51 [ 180.798418] __se_sys_ioctl+0x7a/0xbc [ 180.798421] do_syscall_64+0x55/0x9d [ 180.798424] ? exit_to_user_mode_prepare+0x3c/0x8b [ 180.798427] entry_SYSCALL_64_after_hwframe+0x61/0xcb If this function shouldn't be called in such a case, then perhaps I should revist my original attempt at fixing this in https://patchwork.freedesktop.org/patch/516060 by rejecting such a configuration? I'll respond to Alan on that thread. > > > > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > > --- > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > index 10e1fc9d0698..a9948e8d3543 100644 > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > @@ -144,7 +144,7 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src, > > const struct drm_rect *dst, > > unsigned int rate) > > { > > - unsigned int src_w, src_h, dst_w, dst_h; > > + unsigned int src_w, src_h, dst_w, dst_h, dst_wh; > > > > src_w = drm_rect_width(src) >> 16; > > src_h = drm_rect_height(src) >> 16; > > @@ -155,8 +155,10 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src, > > dst_w = min(src_w, dst_w); > > dst_h = min(src_h, dst_h); > > > > - return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), > > - dst_w * dst_h); > > + /* in case src contained only fractional part */ > > + dst_wh = max(dst_w * dst_h, (unsigned) 1); > > + > > + return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), dst_wh); > > } > > > > unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state, > > -- > > 2.37.3 > > -- > Ville Syrjälä > Intel
On Wed, Jan 11, 2023 at 11:28:51AM -0700, Drew Davenport wrote: > On Wed, Jan 11, 2023 at 04:19:00PM +0200, Ville Syrjälä wrote: > > On Wed, Jan 04, 2023 at 02:44:48PM +0200, Juha-Pekka Heikkila wrote: > > > intel_adjusted_rate() didn't take into account src rectangle > > > can be less than 1 in width or height. > > > > This should not get called in those cases. What does the > > backtrace look like? > > In my repro of this issue, the backtrace looks as follows: > > [ 180.798331] RIP: 0010:intel_plane_pixel_rate+0x4a/0x53 > [ 180.798336] Code: <snip long line> > [ 180.798338] RSP: 0018:ffffb080ce4179b8 EFLAGS: 00010246 > [ 180.798341] RAX: ffffffffffffffff RBX: ffff98cd22a24000 RCX: 0000000000000a00 > [ 180.798343] RDX: 0000000000000000 RSI: ffff98cccbae7000 RDI: 0000000000000000 > [ 180.798346] RBP: ffffb080ce4179b8 R08: 0000000000087780 R09: 0000000000000002 > [ 180.798348] R10: 0000000000000a00 R11: 0000000000000000 R12: 0000000000000000 > [ 180.798350] R13: ffff98cd0e495400 R14: ffff98ccc34e0000 R15: ffff98cccbae7000 > [ 180.798352] FS: 00007b84119b5000(0000) GS:ffff98d02f900000(0000) knlGS:0000000000000000 > [ 180.798354] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 180.798357] CR2: 00007ffc2d5e4080 CR3: 00000001042ee006 CR4: 0000000000770ee0 > [ 180.798359] PKRU: 55555554 > [ 180.798361] Call Trace: > [ 180.798364] <TASK> > [ 180.798366] intel_plane_atomic_check_with_state+0x1fd/0x6ea > [ 180.798370] ? intel_plane_atomic_check+0x11b/0x145 > [ 180.798373] intel_atomic_check_planes+0x263/0x7ce > [ 180.798376] ? drm_atomic_helper_check_modeset+0x189/0x923 > [ 180.798380] intel_atomic_check+0x14e4/0x184d > [ 180.798382] ? intel_rps_mark_interactive+0x23/0x6a > [ 180.798386] drm_atomic_check_only+0x3ec/0x98f > [ 180.798391] drm_atomic_commit+0xa2/0x105 > [ 180.798394] ? drm_atomic_set_fb_for_plane+0x96/0xa5 > [ 180.798397] drm_atomic_helper_update_plane+0xdc/0x11f > [ 180.798400] drm_mode_setplane+0x236/0x30c > [ 180.798404] ? drm_any_plane_has_format+0x51/0x51 > [ 180.798407] drm_ioctl_kernel+0xda/0x14d > [ 180.798411] drm_ioctl+0x27e/0x3b4 > [ 180.798414] ? drm_any_plane_has_format+0x51/0x51 > [ 180.798418] __se_sys_ioctl+0x7a/0xbc > [ 180.798421] do_syscall_64+0x55/0x9d > [ 180.798424] ? exit_to_user_mode_prepare+0x3c/0x8b > [ 180.798427] entry_SYSCALL_64_after_hwframe+0x61/0xcb > > If this function shouldn't be called in such a case, then perhaps > I should revist my original attempt at fixing this in > https://patchwork.freedesktop.org/patch/516060 by rejecting such a > configuration? I'm saying that this should be impossible already. At least I can't immediately see anything that could call this with an invisible plane. > > I'll respond to Alan on that thread. > > > > > > > > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 8 +++++--- > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > index 10e1fc9d0698..a9948e8d3543 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > @@ -144,7 +144,7 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src, > > > const struct drm_rect *dst, > > > unsigned int rate) > > > { > > > - unsigned int src_w, src_h, dst_w, dst_h; > > > + unsigned int src_w, src_h, dst_w, dst_h, dst_wh; > > > > > > src_w = drm_rect_width(src) >> 16; > > > src_h = drm_rect_height(src) >> 16; > > > @@ -155,8 +155,10 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src, > > > dst_w = min(src_w, dst_w); > > > dst_h = min(src_h, dst_h); > > > > > > - return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), > > > - dst_w * dst_h); > > > + /* in case src contained only fractional part */ > > > + dst_wh = max(dst_w * dst_h, (unsigned) 1); > > > + > > > + return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), dst_wh); > > > } > > > > > > unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state, > > > -- > > > 2.37.3 > > > > -- > > Ville Syrjälä > > Intel
On Wed, Jan 11, 2023 at 09:39:26PM +0200, Ville Syrjälä wrote: > On Wed, Jan 11, 2023 at 11:28:51AM -0700, Drew Davenport wrote: > > On Wed, Jan 11, 2023 at 04:19:00PM +0200, Ville Syrjälä wrote: > > > On Wed, Jan 04, 2023 at 02:44:48PM +0200, Juha-Pekka Heikkila wrote: > > > > intel_adjusted_rate() didn't take into account src rectangle > > > > can be less than 1 in width or height. > > > > > > This should not get called in those cases. What does the > > > backtrace look like? > > > > In my repro of this issue, the backtrace looks as follows: > > > > [ 180.798331] RIP: 0010:intel_plane_pixel_rate+0x4a/0x53 > > [ 180.798336] Code: <snip long line> > > [ 180.798338] RSP: 0018:ffffb080ce4179b8 EFLAGS: 00010246 > > [ 180.798341] RAX: ffffffffffffffff RBX: ffff98cd22a24000 RCX: 0000000000000a00 > > [ 180.798343] RDX: 0000000000000000 RSI: ffff98cccbae7000 RDI: 0000000000000000 > > [ 180.798346] RBP: ffffb080ce4179b8 R08: 0000000000087780 R09: 0000000000000002 > > [ 180.798348] R10: 0000000000000a00 R11: 0000000000000000 R12: 0000000000000000 > > [ 180.798350] R13: ffff98cd0e495400 R14: ffff98ccc34e0000 R15: ffff98cccbae7000 > > [ 180.798352] FS: 00007b84119b5000(0000) GS:ffff98d02f900000(0000) knlGS:0000000000000000 > > [ 180.798354] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > [ 180.798357] CR2: 00007ffc2d5e4080 CR3: 00000001042ee006 CR4: 0000000000770ee0 > > [ 180.798359] PKRU: 55555554 > > [ 180.798361] Call Trace: > > [ 180.798364] <TASK> > > [ 180.798366] intel_plane_atomic_check_with_state+0x1fd/0x6ea > > [ 180.798370] ? intel_plane_atomic_check+0x11b/0x145 > > [ 180.798373] intel_atomic_check_planes+0x263/0x7ce > > [ 180.798376] ? drm_atomic_helper_check_modeset+0x189/0x923 > > [ 180.798380] intel_atomic_check+0x14e4/0x184d > > [ 180.798382] ? intel_rps_mark_interactive+0x23/0x6a > > [ 180.798386] drm_atomic_check_only+0x3ec/0x98f > > [ 180.798391] drm_atomic_commit+0xa2/0x105 > > [ 180.798394] ? drm_atomic_set_fb_for_plane+0x96/0xa5 > > [ 180.798397] drm_atomic_helper_update_plane+0xdc/0x11f > > [ 180.798400] drm_mode_setplane+0x236/0x30c > > [ 180.798404] ? drm_any_plane_has_format+0x51/0x51 > > [ 180.798407] drm_ioctl_kernel+0xda/0x14d > > [ 180.798411] drm_ioctl+0x27e/0x3b4 > > [ 180.798414] ? drm_any_plane_has_format+0x51/0x51 > > [ 180.798418] __se_sys_ioctl+0x7a/0xbc > > [ 180.798421] do_syscall_64+0x55/0x9d > > [ 180.798424] ? exit_to_user_mode_prepare+0x3c/0x8b > > [ 180.798427] entry_SYSCALL_64_after_hwframe+0x61/0xcb > > > > If this function shouldn't be called in such a case, then perhaps > > I should revist my original attempt at fixing this in > > https://patchwork.freedesktop.org/patch/516060 by rejecting such a > > configuration? > > I'm saying that this should be impossible already. At least > I can't immediately see anything that could call this with > an invisible plane. In my repro case, I called drmModeSetPlane with the src_h parameter set to 65535 (so the largest 16.16 number that's less than one). This got through any existing checks on the height of the src rect, resulting in the divide-by-zero error in intel_plane_pixel_rate. While investigating this, I tried setting src_h to 0, but this configuration got rejected somewhere along the line before it got through the intel_plane_pixel_rate. > > > > > I'll respond to Alan on that thread. > > > > > > > > > > > > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_atomic_plane.c | 8 +++++--- > > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > index 10e1fc9d0698..a9948e8d3543 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > > @@ -144,7 +144,7 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src, > > > > const struct drm_rect *dst, > > > > unsigned int rate) > > > > { > > > > - unsigned int src_w, src_h, dst_w, dst_h; > > > > + unsigned int src_w, src_h, dst_w, dst_h, dst_wh; > > > > > > > > src_w = drm_rect_width(src) >> 16; > > > > src_h = drm_rect_height(src) >> 16; > > > > @@ -155,8 +155,10 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src, > > > > dst_w = min(src_w, dst_w); > > > > dst_h = min(src_h, dst_h); > > > > > > > > - return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), > > > > - dst_w * dst_h); > > > > + /* in case src contained only fractional part */ > > > > + dst_wh = max(dst_w * dst_h, (unsigned) 1); > > > > + > > > > + return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), dst_wh); > > > > } > > > > > > > > unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state, > > > > -- > > > > 2.37.3 > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
On 11.1.2023 22.09, Drew Davenport wrote: > On Wed, Jan 11, 2023 at 09:39:26PM +0200, Ville Syrjälä wrote: >> On Wed, Jan 11, 2023 at 11:28:51AM -0700, Drew Davenport wrote: >>> On Wed, Jan 11, 2023 at 04:19:00PM +0200, Ville Syrjälä wrote: >>>> On Wed, Jan 04, 2023 at 02:44:48PM +0200, Juha-Pekka Heikkila wrote: >>>>> intel_adjusted_rate() didn't take into account src rectangle >>>>> can be less than 1 in width or height. >>>> >>>> This should not get called in those cases. What does the >>>> backtrace look like? >>> >>> In my repro of this issue, the backtrace looks as follows: >>> >>> [ 180.798331] RIP: 0010:intel_plane_pixel_rate+0x4a/0x53 >>> [ 180.798336] Code: <snip long line> >>> [ 180.798338] RSP: 0018:ffffb080ce4179b8 EFLAGS: 00010246 >>> [ 180.798341] RAX: ffffffffffffffff RBX: ffff98cd22a24000 RCX: 0000000000000a00 >>> [ 180.798343] RDX: 0000000000000000 RSI: ffff98cccbae7000 RDI: 0000000000000000 >>> [ 180.798346] RBP: ffffb080ce4179b8 R08: 0000000000087780 R09: 0000000000000002 >>> [ 180.798348] R10: 0000000000000a00 R11: 0000000000000000 R12: 0000000000000000 >>> [ 180.798350] R13: ffff98cd0e495400 R14: ffff98ccc34e0000 R15: ffff98cccbae7000 >>> [ 180.798352] FS: 00007b84119b5000(0000) GS:ffff98d02f900000(0000) knlGS:0000000000000000 >>> [ 180.798354] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >>> [ 180.798357] CR2: 00007ffc2d5e4080 CR3: 00000001042ee006 CR4: 0000000000770ee0 >>> [ 180.798359] PKRU: 55555554 >>> [ 180.798361] Call Trace: >>> [ 180.798364] <TASK> >>> [ 180.798366] intel_plane_atomic_check_with_state+0x1fd/0x6ea >>> [ 180.798370] ? intel_plane_atomic_check+0x11b/0x145 >>> [ 180.798373] intel_atomic_check_planes+0x263/0x7ce >>> [ 180.798376] ? drm_atomic_helper_check_modeset+0x189/0x923 >>> [ 180.798380] intel_atomic_check+0x14e4/0x184d >>> [ 180.798382] ? intel_rps_mark_interactive+0x23/0x6a >>> [ 180.798386] drm_atomic_check_only+0x3ec/0x98f >>> [ 180.798391] drm_atomic_commit+0xa2/0x105 >>> [ 180.798394] ? drm_atomic_set_fb_for_plane+0x96/0xa5 >>> [ 180.798397] drm_atomic_helper_update_plane+0xdc/0x11f >>> [ 180.798400] drm_mode_setplane+0x236/0x30c >>> [ 180.798404] ? drm_any_plane_has_format+0x51/0x51 >>> [ 180.798407] drm_ioctl_kernel+0xda/0x14d >>> [ 180.798411] drm_ioctl+0x27e/0x3b4 >>> [ 180.798414] ? drm_any_plane_has_format+0x51/0x51 >>> [ 180.798418] __se_sys_ioctl+0x7a/0xbc >>> [ 180.798421] do_syscall_64+0x55/0x9d >>> [ 180.798424] ? exit_to_user_mode_prepare+0x3c/0x8b >>> [ 180.798427] entry_SYSCALL_64_after_hwframe+0x61/0xcb >>> >>> If this function shouldn't be called in such a case, then perhaps >>> I should revist my original attempt at fixing this in >>> https://patchwork.freedesktop.org/patch/516060 by rejecting such a >>> configuration? >> >> I'm saying that this should be impossible already. At least >> I can't immediately see anything that could call this with >> an invisible plane. > > In my repro case, I called drmModeSetPlane with the src_h parameter set > to 65535 (so the largest 16.16 number that's less than one). This got > through any existing checks on the height of the src rect, resulting in > the divide-by-zero error in intel_plane_pixel_rate. > > While investigating this, I tried setting src_h to 0, but this > configuration got rejected somewhere along the line before it got > through the intel_plane_pixel_rate. > Here's one of the igt tests I had come up when debugging this: --- #include "igt.h" igt_main { igt_subtest_f("testi") { int drm_fd = drm_open_driver_master(DRIVER_INTEL); igt_display_t display; igt_plane_t* plane; igt_output_t *output; igt_fb_t fb; kmstest_set_vt_graphics_mode(); igt_display_require(&display, drm_fd); igt_display_require_output(&display); output = igt_get_single_output_for_pipe(&display, PIPE_A); igt_output_set_pipe(output, PIPE_A); igt_display_commit_atomic(&display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); igt_create_pattern_fb(drm_fd, 256, 256, DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_NONE, &fb); plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); igt_plane_set_position(plane, 0, 0); igt_plane_set_fb(plane, &fb); igt_plane_set_prop_value(plane, IGT_PLANE_SRC_H, IGT_FIXED(0, 30)); igt_plane_set_size(plane, 256, 8); igt_display_commit_atomic(&display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL); } } --- This should give callstack ending same way as what Drew had seen. With my patch in place this test will die with einval coming from kernel from skl_update_scaler(..) where it doesn't pass range checks (and will note on dmesg about it) /Juha-Pekka >> >>> >>> I'll respond to Alan on that thread. >>> >>>> >>>>> >>>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> >>>>> --- >>>>> drivers/gpu/drm/i915/display/intel_atomic_plane.c | 8 +++++--- >>>>> 1 file changed, 5 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>>> index 10e1fc9d0698..a9948e8d3543 100644 >>>>> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>>> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c >>>>> @@ -144,7 +144,7 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src, >>>>> const struct drm_rect *dst, >>>>> unsigned int rate) >>>>> { >>>>> - unsigned int src_w, src_h, dst_w, dst_h; >>>>> + unsigned int src_w, src_h, dst_w, dst_h, dst_wh; >>>>> >>>>> src_w = drm_rect_width(src) >> 16; >>>>> src_h = drm_rect_height(src) >> 16; >>>>> @@ -155,8 +155,10 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src, >>>>> dst_w = min(src_w, dst_w); >>>>> dst_h = min(src_h, dst_h); >>>>> >>>>> - return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), >>>>> - dst_w * dst_h); >>>>> + /* in case src contained only fractional part */ >>>>> + dst_wh = max(dst_w * dst_h, (unsigned) 1); >>>>> + >>>>> + return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), dst_wh); >>>>> } >>>>> >>>>> unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state, >>>>> -- >>>>> 2.37.3 >>>> >>>> -- >>>> Ville Syrjälä >>>> Intel >> >> -- >> Ville Syrjälä >> Intel
diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c index 10e1fc9d0698..a9948e8d3543 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c @@ -144,7 +144,7 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src, const struct drm_rect *dst, unsigned int rate) { - unsigned int src_w, src_h, dst_w, dst_h; + unsigned int src_w, src_h, dst_w, dst_h, dst_wh; src_w = drm_rect_width(src) >> 16; src_h = drm_rect_height(src) >> 16; @@ -155,8 +155,10 @@ unsigned int intel_adjusted_rate(const struct drm_rect *src, dst_w = min(src_w, dst_w); dst_h = min(src_h, dst_h); - return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), - dst_w * dst_h); + /* in case src contained only fractional part */ + dst_wh = max(dst_w * dst_h, (unsigned) 1); + + return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h), dst_wh); } unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
intel_adjusted_rate() didn't take into account src rectangle can be less than 1 in width or height. Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> --- drivers/gpu/drm/i915/display/intel_atomic_plane.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)