| Message ID | 20230203182132.1307834-22-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dpu: wide planes support | expand |
On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote: > Since the driver uses clipped src coordinates, there is no need to check > against the fb coordinates. Remove corresponding checks and inline > dpu_plane_validate_src(). > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Can you please explain how the clipping in drm_atomic_helper_check_plane_state() can allow us to remove checking the fb co-ordinates? The clipping is done using the mode parameters. So lets say the FB being used is smaller than the source buffer by an incorrect usermode setting. Then the src sspp shall try to fetch the full image of src rectangle size from a FB which isnt that big leading to a fault. How does the clipping avoid such a case? > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 30 ++++++++--------------- > 1 file changed, 10 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > index ecf5402ab61a..0986e740b978 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > @@ -894,25 +894,6 @@ static void dpu_plane_cleanup_fb(struct drm_plane *plane, > old_pstate->needs_dirtyfb); > } > > -static bool dpu_plane_validate_src(struct drm_rect *src, > - struct drm_rect *fb_rect, > - uint32_t min_src_size) > -{ > - /* Ensure fb size is supported */ > - if (drm_rect_width(fb_rect) > MAX_IMG_WIDTH || > - drm_rect_height(fb_rect) > MAX_IMG_HEIGHT) > - return false; > - > - /* Ensure src rect is above the minimum size */ > - if (drm_rect_width(src) < min_src_size || > - drm_rect_height(src) < min_src_size) > - return false; > - > - /* Ensure src is fully encapsulated in fb */ > - return drm_rect_intersect(fb_rect, src) && > - drm_rect_equals(fb_rect, src); > -} > - > static int dpu_plane_check_inline_rotation(struct dpu_plane *pdpu, > const struct dpu_sspp_sub_blks *sblk, > struct drm_rect src, const struct dpu_format *fmt) > @@ -998,6 +979,14 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > fb_rect.x2 = new_plane_state->fb->width; > fb_rect.y2 = new_plane_state->fb->height; > > + /* Ensure fb size is supported */ > + if (drm_rect_width(&fb_rect) > MAX_IMG_WIDTH || > + drm_rect_height(&fb_rect) > MAX_IMG_HEIGHT) { > + DPU_DEBUG_PLANE(pdpu, "invalid framebuffer " DRM_RECT_FMT "\n", > + DRM_RECT_ARG(&fb_rect)); > + return -E2BIG; > + } > + > max_linewidth = pdpu->catalog->caps->max_linewidth; > > fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); > @@ -1012,7 +1001,8 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > return -EINVAL; > > /* check src bounds */ > - } else if (!dpu_plane_validate_src(&pipe_cfg->src_rect, &fb_rect, min_src_size)) { > + } else if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || > + drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { > DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", > DRM_RECT_ARG(&pipe_cfg->src_rect)); > return -E2BIG;
On 07/02/2023 00:40, Abhinav Kumar wrote: > > > On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote: >> Since the driver uses clipped src coordinates, there is no need to check >> against the fb coordinates. Remove corresponding checks and inline >> dpu_plane_validate_src(). >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Can you please explain how the clipping in > drm_atomic_helper_check_plane_state() can allow us to remove checking > the fb co-ordinates? > > The clipping is done using the mode parameters. > > So lets say the FB being used is smaller than the source buffer by an > incorrect usermode setting. > > Then the src sspp shall try to fetch the full image of src rectangle > size from a FB which isnt that big leading to a fault. This case is checked by the drm_atomic_plane_check(). > > How does the clipping avoid such a case? clipping itself does not. However using clipped coordinates from plane_state->src ensures that they already were validated against the FB dimensions. I'll see if I can change the commit message to make it more obvious. > >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 30 ++++++++--------------- >> 1 file changed, 10 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> index ecf5402ab61a..0986e740b978 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> @@ -894,25 +894,6 @@ static void dpu_plane_cleanup_fb(struct drm_plane >> *plane, >> old_pstate->needs_dirtyfb); >> } >> -static bool dpu_plane_validate_src(struct drm_rect *src, >> - struct drm_rect *fb_rect, >> - uint32_t min_src_size) >> -{ >> - /* Ensure fb size is supported */ >> - if (drm_rect_width(fb_rect) > MAX_IMG_WIDTH || >> - drm_rect_height(fb_rect) > MAX_IMG_HEIGHT) >> - return false; >> - >> - /* Ensure src rect is above the minimum size */ >> - if (drm_rect_width(src) < min_src_size || >> - drm_rect_height(src) < min_src_size) >> - return false; >> - >> - /* Ensure src is fully encapsulated in fb */ >> - return drm_rect_intersect(fb_rect, src) && >> - drm_rect_equals(fb_rect, src); >> -} >> - >> static int dpu_plane_check_inline_rotation(struct dpu_plane *pdpu, >> const struct dpu_sspp_sub_blks *sblk, >> struct drm_rect src, const struct dpu_format >> *fmt) >> @@ -998,6 +979,14 @@ static int dpu_plane_atomic_check(struct >> drm_plane *plane, >> fb_rect.x2 = new_plane_state->fb->width; >> fb_rect.y2 = new_plane_state->fb->height; >> + /* Ensure fb size is supported */ >> + if (drm_rect_width(&fb_rect) > MAX_IMG_WIDTH || >> + drm_rect_height(&fb_rect) > MAX_IMG_HEIGHT) { >> + DPU_DEBUG_PLANE(pdpu, "invalid framebuffer " DRM_RECT_FMT "\n", >> + DRM_RECT_ARG(&fb_rect)); >> + return -E2BIG; >> + } >> + >> max_linewidth = pdpu->catalog->caps->max_linewidth; >> fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); >> @@ -1012,7 +1001,8 @@ static int dpu_plane_atomic_check(struct >> drm_plane *plane, >> return -EINVAL; >> /* check src bounds */ >> - } else if (!dpu_plane_validate_src(&pipe_cfg->src_rect, &fb_rect, >> min_src_size)) { >> + } else if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || >> + drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { >> DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", >> DRM_RECT_ARG(&pipe_cfg->src_rect)); >> return -E2BIG;
On 2/6/2023 4:27 PM, Dmitry Baryshkov wrote: > On 07/02/2023 00:40, Abhinav Kumar wrote: >> >> >> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote: >>> Since the driver uses clipped src coordinates, there is no need to check >>> against the fb coordinates. Remove corresponding checks and inline >>> dpu_plane_validate_src(). >>> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> >> Can you please explain how the clipping in >> drm_atomic_helper_check_plane_state() can allow us to remove checking >> the fb co-ordinates? >> >> The clipping is done using the mode parameters. >> >> So lets say the FB being used is smaller than the source buffer by an >> incorrect usermode setting. >> >> Then the src sspp shall try to fetch the full image of src rectangle >> size from a FB which isnt that big leading to a fault. > > This case is checked by the drm_atomic_plane_check(). > >> >> How does the clipping avoid such a case? > > clipping itself does not. However using clipped coordinates from > plane_state->src ensures that they already were validated against the FB > dimensions. I'll see if I can change the commit message to make it more > obvious. > Ah okay, yeah the commit text confused me a bit to look at the clipping code in drm_atomic_helper_check_plane_state(). Yes, please change it to point to the helper which addresses this. >> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 30 ++++++++--------------- >>> 1 file changed, 10 insertions(+), 20 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> index ecf5402ab61a..0986e740b978 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >>> @@ -894,25 +894,6 @@ static void dpu_plane_cleanup_fb(struct >>> drm_plane *plane, >>> old_pstate->needs_dirtyfb); >>> } >>> -static bool dpu_plane_validate_src(struct drm_rect *src, >>> - struct drm_rect *fb_rect, >>> - uint32_t min_src_size) >>> -{ >>> - /* Ensure fb size is supported */ >>> - if (drm_rect_width(fb_rect) > MAX_IMG_WIDTH || >>> - drm_rect_height(fb_rect) > MAX_IMG_HEIGHT) >>> - return false; >>> - >>> - /* Ensure src rect is above the minimum size */ >>> - if (drm_rect_width(src) < min_src_size || >>> - drm_rect_height(src) < min_src_size) >>> - return false; >>> - >>> - /* Ensure src is fully encapsulated in fb */ >>> - return drm_rect_intersect(fb_rect, src) && >>> - drm_rect_equals(fb_rect, src); >>> -} >>> - >>> static int dpu_plane_check_inline_rotation(struct dpu_plane *pdpu, >>> const struct dpu_sspp_sub_blks *sblk, >>> struct drm_rect src, const struct >>> dpu_format *fmt) >>> @@ -998,6 +979,14 @@ static int dpu_plane_atomic_check(struct >>> drm_plane *plane, >>> fb_rect.x2 = new_plane_state->fb->width; >>> fb_rect.y2 = new_plane_state->fb->height; >>> + /* Ensure fb size is supported */ >>> + if (drm_rect_width(&fb_rect) > MAX_IMG_WIDTH || >>> + drm_rect_height(&fb_rect) > MAX_IMG_HEIGHT) { >>> + DPU_DEBUG_PLANE(pdpu, "invalid framebuffer " DRM_RECT_FMT "\n", >>> + DRM_RECT_ARG(&fb_rect)); >>> + return -E2BIG; >>> + } >>> + >>> max_linewidth = pdpu->catalog->caps->max_linewidth; >>> fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); >>> @@ -1012,7 +1001,8 @@ static int dpu_plane_atomic_check(struct >>> drm_plane *plane, >>> return -EINVAL; >>> /* check src bounds */ >>> - } else if (!dpu_plane_validate_src(&pipe_cfg->src_rect, >>> &fb_rect, min_src_size)) { >>> + } else if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || >>> + drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { >>> DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", >>> DRM_RECT_ARG(&pipe_cfg->src_rect)); >>> return -E2BIG; >
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index ecf5402ab61a..0986e740b978 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -894,25 +894,6 @@ static void dpu_plane_cleanup_fb(struct drm_plane *plane, old_pstate->needs_dirtyfb); } -static bool dpu_plane_validate_src(struct drm_rect *src, - struct drm_rect *fb_rect, - uint32_t min_src_size) -{ - /* Ensure fb size is supported */ - if (drm_rect_width(fb_rect) > MAX_IMG_WIDTH || - drm_rect_height(fb_rect) > MAX_IMG_HEIGHT) - return false; - - /* Ensure src rect is above the minimum size */ - if (drm_rect_width(src) < min_src_size || - drm_rect_height(src) < min_src_size) - return false; - - /* Ensure src is fully encapsulated in fb */ - return drm_rect_intersect(fb_rect, src) && - drm_rect_equals(fb_rect, src); -} - static int dpu_plane_check_inline_rotation(struct dpu_plane *pdpu, const struct dpu_sspp_sub_blks *sblk, struct drm_rect src, const struct dpu_format *fmt) @@ -998,6 +979,14 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, fb_rect.x2 = new_plane_state->fb->width; fb_rect.y2 = new_plane_state->fb->height; + /* Ensure fb size is supported */ + if (drm_rect_width(&fb_rect) > MAX_IMG_WIDTH || + drm_rect_height(&fb_rect) > MAX_IMG_HEIGHT) { + DPU_DEBUG_PLANE(pdpu, "invalid framebuffer " DRM_RECT_FMT "\n", + DRM_RECT_ARG(&fb_rect)); + return -E2BIG; + } + max_linewidth = pdpu->catalog->caps->max_linewidth; fmt = to_dpu_format(msm_framebuffer_format(new_plane_state->fb)); @@ -1012,7 +1001,8 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, return -EINVAL; /* check src bounds */ - } else if (!dpu_plane_validate_src(&pipe_cfg->src_rect, &fb_rect, min_src_size)) { + } else if (drm_rect_width(&pipe_cfg->src_rect) < min_src_size || + drm_rect_height(&pipe_cfg->src_rect) < min_src_size) { DPU_DEBUG_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n", DRM_RECT_ARG(&pipe_cfg->src_rect)); return -E2BIG;
Since the driver uses clipped src coordinates, there is no need to check against the fb coordinates. Remove corresponding checks and inline dpu_plane_validate_src(). Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 30 ++++++++--------------- 1 file changed, 10 insertions(+), 20 deletions(-)