| Message ID | 20221226225246.1.I15dff7bb5a0e485c862eae61a69096caf12ef29f@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: Check source height is > 0 | expand |
Is there a better place for this check higher up the intel specific atomic-check? (so the check won't be skl specific - i notice that intel_adjusted_rate is also called by ilk_foo as well and non-backend-specific functions). Else, perhaps intel_adjusted_rate should add a check + WARN? (if we are trying to propagate this slowly across HW). ...alan On Mon, 2022-12-26 at 22:53 -0700, Drew Davenport wrote: > The error message suggests that the height of the src rect must be at > least 1. Reject source with height of 0. > > Signed-off-by: Drew Davenport <ddavenport@chromium.org> > > --- > I was investigating some divide-by-zero crash reports on ChromeOS which > pointed to the intel_adjusted_rate function. Further prodding showed > that I could reproduce this in a simple test program if I made src_h > some value less than 1 but greater than 0. > > This seemed to be a sensible place to check that the source height is at > least 1. I tried to repro this issue on an amd device I had on hand, and > the configuration was rejected. > > Would it make sense to add a check that source dimensions are at least 1 > somewhere in core, like in drm_atomic_plane_check? Or is that a valid > use case on some devices, and thus any such check should be done on a > per-driver basis? > > Thanks. > > drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c > index 4b79c2d2d6177..9b172a1e90deb 100644 > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state) > u32 offset; > int ret; > > - if (w > max_width || w < min_width || h > max_height) { > + if (w > max_width || w < min_width || h > max_height || h < 1) { > drm_dbg_kms(&dev_priv->drm, > "requested Y/RGB source size %dx%d outside limits (min: %dx1 max: %dx%d)\n", > w, h, min_width, max_width, max_height); > -- > 2.39.0.314.g84b9a713c41-goog >
Hi Drew,
this is good find. I went looking where the problem is in and saw what
you probably also saw earlier.
I was wondering if diff below would be better fix? I assume this would
end up with einval or erange in your case but code flow otherwise would
stay as is while fixing all future callers for same issue:
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,
What do you think? I'll in any case come up with some test for this in igt.
/Juha-Pekka
On 27.12.2022 7.53, Drew Davenport wrote:
> The error message suggests that the height of the src rect must be at
> least 1. Reject source with height of 0.
>
> Signed-off-by: Drew Davenport <ddavenport@chromium.org>
>
> ---
> I was investigating some divide-by-zero crash reports on ChromeOS which
> pointed to the intel_adjusted_rate function. Further prodding showed
> that I could reproduce this in a simple test program if I made src_h
> some value less than 1 but greater than 0.
>
> This seemed to be a sensible place to check that the source height is at
> least 1. I tried to repro this issue on an amd device I had on hand, and
> the configuration was rejected.
>
> Would it make sense to add a check that source dimensions are at least 1
> somewhere in core, like in drm_atomic_plane_check? Or is that a valid
> use case on some devices, and thus any such check should be done on a
> per-driver basis?
>
> Thanks.
>
> drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 4b79c2d2d6177..9b172a1e90deb 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
> u32 offset;
> int ret;
>
> - if (w > max_width || w < min_width || h > max_height) {
> + if (w > max_width || w < min_width || h > max_height || h < 1) {
> drm_dbg_kms(&dev_priv->drm,
> "requested Y/RGB source size %dx%d outside limits (min: %dx1 max: %dx%d)\n",
> w, h, min_width, max_width, max_height);
On Tue, Jan 03, 2023 at 12:42:43PM +0200, Juha-Pekka Heikkila wrote: > Hi Drew, Hi Juha-Pekka, sorry for the late response since I was on vacation. > > this is good find. I went looking where the problem is in and saw what you > probably also saw earlier. > > I was wondering if diff below would be better fix? I assume this would end > up with einval or erange in your case but code flow otherwise would stay as > is while fixing all future callers for same issue: Yes, the function you identify below is where I encountered divide-by-zero errors. If width/height less than 1 is a legitimate use case, then your proposed solution looks like it would be better. It should have no risk of regression in userspace either, which is nice. I tested your patch, and as expected I did not hit the divide-by-zero error, though the test commit was rejected due to a check further along inside skl_update_scaler. Perhaps there is some other configuration which would pass the test commit with a width/height less than 1, but I didn't dig much further. > > 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, > > > What do you think? I'll in any case come up with some test for this in igt. I see that you've posted your fix to the list already. Adding a test to cover this in IGT also sounds great. Thanks! Breadcrumbs to Juha-Pekka's patch for anyone following this thread: https://patchwork.freedesktop.org/series/112396/ > > /Juha-Pekka > > On 27.12.2022 7.53, Drew Davenport wrote: > > The error message suggests that the height of the src rect must be at > > least 1. Reject source with height of 0. > > > > Signed-off-by: Drew Davenport <ddavenport@chromium.org> > > > > --- > > I was investigating some divide-by-zero crash reports on ChromeOS which > > pointed to the intel_adjusted_rate function. Further prodding showed > > that I could reproduce this in a simple test program if I made src_h > > some value less than 1 but greater than 0. > > > > This seemed to be a sensible place to check that the source height is at > > least 1. I tried to repro this issue on an amd device I had on hand, and > > the configuration was rejected. > > > > Would it make sense to add a check that source dimensions are at least 1 > > somewhere in core, like in drm_atomic_plane_check? Or is that a valid > > use case on some devices, and thus any such check should be done on a > > per-driver basis? > > > > Thanks. > > > > drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c > > index 4b79c2d2d6177..9b172a1e90deb 100644 > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > > @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state) > > u32 offset; > > int ret; > > - if (w > max_width || w < min_width || h > max_height) { > > + if (w > max_width || w < min_width || h > max_height || h < 1) { > > drm_dbg_kms(&dev_priv->drm, > > "requested Y/RGB source size %dx%d outside limits (min: %dx1 max: %dx%d)\n", > > w, h, min_width, max_width, max_height); >
On Tue, Dec 27, 2022 at 05:55:17PM +0000, Teres Alexis, Alan Previn wrote: > Is there a better place for this check higher up the intel specific atomic-check? (so the check won't be skl specific - i notice that intel_adjusted_rate is also called by > ilk_foo as well and non-backend-specific functions). Else, perhaps intel_adjusted_rate should add a check + WARN? (if we are trying to propagate this slowly across HW). Would intel_plane_atomic_check_with_state be a more appropriate place to check that the src width and height are at least 1? This is where skl_plane_check and other HW's foo_plane_check functions are called from. I don't think that the potential divide-by-zero will be hit in the case where intel_adjusted_rate is called from ilk_pipe_pixel_rate because the src rect will not have a fractional part to it in this case. I'm assuming that something earlier on would catch and reject a src with zero width/height. Drew > > > ...alan > > On Mon, 2022-12-26 at 22:53 -0700, Drew Davenport wrote: > > The error message suggests that the height of the src rect must be at > > least 1. Reject source with height of 0. > > > > Signed-off-by: Drew Davenport <ddavenport@chromium.org> > > > > --- > > I was investigating some divide-by-zero crash reports on ChromeOS which > > pointed to the intel_adjusted_rate function. Further prodding showed > > that I could reproduce this in a simple test program if I made src_h > > some value less than 1 but greater than 0. > > > > This seemed to be a sensible place to check that the source height is at > > least 1. I tried to repro this issue on an amd device I had on hand, and > > the configuration was rejected. > > > > Would it make sense to add a check that source dimensions are at least 1 > > somewhere in core, like in drm_atomic_plane_check? Or is that a valid > > use case on some devices, and thus any such check should be done on a > > per-driver basis? > > > > Thanks. > > > > drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c > > index 4b79c2d2d6177..9b172a1e90deb 100644 > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > > @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state) > > u32 offset; > > int ret; > > > > - if (w > max_width || w < min_width || h > max_height) { > > + if (w > max_width || w < min_width || h > max_height || h < 1) { > > drm_dbg_kms(&dev_priv->drm, > > "requested Y/RGB source size %dx%d outside limits (min: %dx1 max: %dx%d)\n", > > w, h, min_width, max_width, max_height); > > -- > > 2.39.0.314.g84b9a713c41-goog > > >
On Mon, Dec 26, 2022 at 10:53:24PM -0700, Drew Davenport wrote: > The error message suggests that the height of the src rect must be at > least 1. Reject source with height of 0. > > Signed-off-by: Drew Davenport <ddavenport@chromium.org> > > --- > I was investigating some divide-by-zero crash reports on ChromeOS which > pointed to the intel_adjusted_rate function. Further prodding showed > that I could reproduce this in a simple test program if I made src_h > some value less than 1 but greater than 0. > > This seemed to be a sensible place to check that the source height is at > least 1. I tried to repro this issue on an amd device I had on hand, and > the configuration was rejected. > > Would it make sense to add a check that source dimensions are at least 1 > somewhere in core, like in drm_atomic_plane_check? Or is that a valid > use case on some devices, and thus any such check should be done on a > per-driver basis? > > Thanks. > > drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c > index 4b79c2d2d6177..9b172a1e90deb 100644 > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state) > u32 offset; > int ret; > > - if (w > max_width || w < min_width || h > max_height) { > + if (w > max_width || w < min_width || h > max_height || h < 1) { I liked this one best so pushed to drm-intel-next with cc:stable. Thanks. In the future we might want to move some of these checks to an earlier spot to make sure we don't hit any other weird issues in some other code, but for the moment I think this will do. > drm_dbg_kms(&dev_priv->drm, > "requested Y/RGB source size %dx%d outside limits (min: %dx1 max: %dx%d)\n", > w, h, min_width, max_width, max_height); > -- > 2.39.0.314.g84b9a713c41-goog
On 12.1.2023 20.28, Ville Syrjälä wrote: > On Mon, Dec 26, 2022 at 10:53:24PM -0700, Drew Davenport wrote: >> The error message suggests that the height of the src rect must be at >> least 1. Reject source with height of 0. >> >> Signed-off-by: Drew Davenport <ddavenport@chromium.org> >> >> --- >> I was investigating some divide-by-zero crash reports on ChromeOS which >> pointed to the intel_adjusted_rate function. Further prodding showed >> that I could reproduce this in a simple test program if I made src_h >> some value less than 1 but greater than 0. >> >> This seemed to be a sensible place to check that the source height is at >> least 1. I tried to repro this issue on an amd device I had on hand, and >> the configuration was rejected. >> >> Would it make sense to add a check that source dimensions are at least 1 >> somewhere in core, like in drm_atomic_plane_check? Or is that a valid >> use case on some devices, and thus any such check should be done on a >> per-driver basis? >> >> Thanks. >> >> drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c >> index 4b79c2d2d6177..9b172a1e90deb 100644 >> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c >> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c >> @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state) >> u32 offset; >> int ret; >> >> - if (w > max_width || w < min_width || h > max_height) { >> + if (w > max_width || w < min_width || h > max_height || h < 1) { > > I liked this one best so pushed to drm-intel-next with cc:stable. Thanks. > > In the future we might want to move some of these checks to an earlier > spot to make sure we don't hit any other weird issues in some other > code, but for the moment I think this will do. > Look ok to me. Tests which I had written to try different ways to cause this issue are now returning einval as expected. I'll polish my igt test for this issue and send it out bit later. /Juha-pekka >> drm_dbg_kms(&dev_priv->drm, >> "requested Y/RGB source size %dx%d outside limits (min: %dx1 max: %dx%d)\n", >> w, h, min_width, max_width, max_height); >> -- >> 2.39.0.314.g84b9a713c41-goog >
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c index 4b79c2d2d6177..9b172a1e90deb 100644 --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c @@ -1627,7 +1627,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state) u32 offset; int ret; - if (w > max_width || w < min_width || h > max_height) { + if (w > max_width || w < min_width || h > max_height || h < 1) { drm_dbg_kms(&dev_priv->drm, "requested Y/RGB source size %dx%d outside limits (min: %dx1 max: %dx%d)\n", w, h, min_width, max_width, max_height);
The error message suggests that the height of the src rect must be at least 1. Reject source with height of 0. Signed-off-by: Drew Davenport <ddavenport@chromium.org> --- I was investigating some divide-by-zero crash reports on ChromeOS which pointed to the intel_adjusted_rate function. Further prodding showed that I could reproduce this in a simple test program if I made src_h some value less than 1 but greater than 0. This seemed to be a sensible place to check that the source height is at least 1. I tried to repro this issue on an amd device I had on hand, and the configuration was rejected. Would it make sense to add a check that source dimensions are at least 1 somewhere in core, like in drm_atomic_plane_check? Or is that a valid use case on some devices, and thus any such check should be done on a per-driver basis? Thanks. drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)