| Message ID | 1476872477-29493-10-git-send-email-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 19, 2016 at 12:21:17PM +0200, Philipp Zabel wrote: > Use drm_plane_helper_check_state to clip raw user coordinates to crtc > bounds. This checks for full plane coverage and scaling already, so > we can drop some custom checks. Use the clipped coordinates everywhere. > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/gpu/drm/imx/ipuv3-plane.c | 78 ++++++++++++++++++--------------------- > 1 file changed, 36 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c > index 6a97e396..f0ce899 100644 > --- a/drivers/gpu/drm/imx/ipuv3-plane.c > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c > @@ -70,8 +70,8 @@ drm_plane_state_to_eba(struct drm_plane_state *state) > { > struct drm_framebuffer *fb = state->fb; > struct drm_gem_cma_object *cma_obj; > - int x = state->src_x >> 16; > - int y = state->src_y >> 16; > + int x = state->src.x1 >> 16; > + int y = state->src.y1 >> 16; > > cma_obj = drm_fb_cma_get_gem_obj(fb, 0); > BUG_ON(!cma_obj); > @@ -86,8 +86,8 @@ drm_plane_state_to_ubo(struct drm_plane_state *state) > struct drm_framebuffer *fb = state->fb; > struct drm_gem_cma_object *cma_obj; > unsigned long eba = drm_plane_state_to_eba(state); > - int x = state->src_x >> 16; > - int y = state->src_y >> 16; > + int x = state->src.x1 >> 16; > + int y = state->src.y1 >> 16; > > cma_obj = drm_fb_cma_get_gem_obj(fb, 1); > BUG_ON(!cma_obj); > @@ -105,8 +105,8 @@ drm_plane_state_to_vbo(struct drm_plane_state *state) > struct drm_framebuffer *fb = state->fb; > struct drm_gem_cma_object *cma_obj; > unsigned long eba = drm_plane_state_to_eba(state); > - int x = state->src_x >> 16; > - int y = state->src_y >> 16; > + int x = state->src.x1 >> 16; > + int y = state->src.y1 >> 16; > > cma_obj = drm_fb_cma_get_gem_obj(fb, 2); > BUG_ON(!cma_obj); > @@ -218,7 +218,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > struct drm_framebuffer *fb = state->fb; > struct drm_framebuffer *old_fb = old_state->fb; > unsigned long eba, ubo, vbo, old_ubo, old_vbo; > + bool can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY); > + struct drm_rect clip; > int hsub, vsub; > + int ret; > > /* Ok to disable */ > if (!fb) > @@ -232,44 +235,35 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > if (WARN_ON(!crtc_state)) > return -EINVAL; > > + clip.x1 = 0; > + clip.y1 = 0; > + clip.x2 = crtc_state->adjusted_mode.hdisplay; > + clip.y2 = crtc_state->adjusted_mode.vdisplay; > + ret = drm_plane_helper_check_state(state, &clip, > + DRM_PLANE_HELPER_NO_SCALING, > + DRM_PLANE_HELPER_NO_SCALING, > + can_position, true); > + if (ret) > + return ret; > + > /* CRTC should be enabled */ > if (!crtc_state->enable) > return -EINVAL; > > - /* no scaling */ > - if (state->src_w >> 16 != state->crtc_w || > - state->src_h >> 16 != state->crtc_h) > - return -EINVAL; > - > switch (plane->type) { > case DRM_PLANE_TYPE_PRIMARY: > - /* full plane doesn't support partial off screen */ > - if (state->crtc_x || state->crtc_y || > - state->crtc_w != crtc_state->adjusted_mode.hdisplay || > - state->crtc_h != crtc_state->adjusted_mode.vdisplay) > - return -EINVAL; > - > /* full plane minimum width is 13 pixels */ > - if (state->crtc_w < 13) > + if (drm_rect_width(&state->dst) < 13) > return -EINVAL; > break; > case DRM_PLANE_TYPE_OVERLAY: > - if (state->crtc_x < 0 || state->crtc_y < 0) > - return -EINVAL; > - > - if (state->crtc_x + state->crtc_w > > - crtc_state->adjusted_mode.hdisplay) > - return -EINVAL; > - if (state->crtc_y + state->crtc_h > > - crtc_state->adjusted_mode.vdisplay) > - return -EINVAL; > break; > default: > - dev_warn(dev, "Unsupported plane type\n"); > + dev_warn(dev, "Unsupported plane type %d\n", plane->type); > return -EINVAL; > } > > - if (state->crtc_h < 2) > + if (drm_rect_height(&state->dst) < 2) > return -EINVAL; > > /* > @@ -279,9 +273,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > * callback. The planes will be reenabled in plane's ->atomic_update > * callback. > */ > - if (old_fb && (state->src_w != old_state->src_w || > - state->src_h != old_state->src_h || > - fb->pixel_format != old_fb->pixel_format)) > + if (old_fb && (!drm_rect_equals(&state->dst, &old_state->dst) || I presume you don't want to force a modeset for moving a plane around. Using drm_rect_equals() here would end up doing that. Otherwise it all looks quite decent to me. > + fb->pixel_format != old_fb->pixel_format)) > crtc_state->mode_changed = true; > > eba = drm_plane_state_to_eba(state); > @@ -350,8 +343,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > */ > hsub = drm_format_horz_chroma_subsampling(fb->pixel_format); > vsub = drm_format_vert_chroma_subsampling(fb->pixel_format); > - if (((state->src_x >> 16) & (hsub - 1)) || > - ((state->src_y >> 16) & (vsub - 1))) > + if (((state->src.x1 >> 16) & (hsub - 1)) || > + ((state->src.y1 >> 16) & (vsub - 1))) > return -EINVAL; > } > > @@ -395,8 +388,8 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, > ics = ipu_drm_fourcc_to_colorspace(state->fb->pixel_format); > ipu_dp_setup_channel(ipu_plane->dp, ics, > IPUV3_COLORSPACE_UNKNOWN); > - ipu_dp_set_window_pos(ipu_plane->dp, state->crtc_x, > - state->crtc_y); > + ipu_dp_set_window_pos(ipu_plane->dp, > + state->dst.x1, state->dst.y1); > /* Enable local alpha on partial plane */ > switch (state->fb->pixel_format) { > case DRM_FORMAT_ARGB1555: > @@ -416,11 +409,12 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, > } > } > > - ipu_dmfc_config_wait4eot(ipu_plane->dmfc, state->crtc_w); > + ipu_dmfc_config_wait4eot(ipu_plane->dmfc, drm_rect_width(&state->dst)); > > ipu_cpmem_zero(ipu_plane->ipu_ch); > - ipu_cpmem_set_resolution(ipu_plane->ipu_ch, state->src_w >> 16, > - state->src_h >> 16); > + ipu_cpmem_set_resolution(ipu_plane->ipu_ch, > + drm_rect_width(&state->src) >> 16, > + drm_rect_height(&state->src) >> 16); > ipu_cpmem_set_fmt(ipu_plane->ipu_ch, state->fb->pixel_format); > ipu_cpmem_set_high_priority(ipu_plane->ipu_ch); > ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1); > @@ -444,7 +438,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, > > dev_dbg(ipu_plane->base.dev->dev, > "phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo, > - state->src_x >> 16, state->src_y >> 16); > + state->src.x1 >> 16, state->src.y1 >> 16); > break; > case DRM_FORMAT_NV12: > case DRM_FORMAT_NV16: > @@ -455,11 +449,11 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, > > dev_dbg(ipu_plane->base.dev->dev, > "phy = %lu %lu, x = %d, y = %d", eba, ubo, > - state->src_x >> 16, state->src_y >> 16); > + state->src.x1 >> 16, state->src.y1 >> 16); > break; > default: > dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d", > - eba, state->src_x >> 16, state->src_y >> 16); > + eba, state->src.x1 >> 16, state->src.y1 >> 16); > break; > } > ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba); > -- > 2.9.3
Am Mittwoch, den 19.10.2016, 13:31 +0300 schrieb Ville Syrjälä: > On Wed, Oct 19, 2016 at 12:21:17PM +0200, Philipp Zabel wrote: > > Use drm_plane_helper_check_state to clip raw user coordinates to crtc > > bounds. This checks for full plane coverage and scaling already, so > > we can drop some custom checks. Use the clipped coordinates everywhere. > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > drivers/gpu/drm/imx/ipuv3-plane.c | 78 ++++++++++++++++++--------------------- > > 1 file changed, 36 insertions(+), 42 deletions(-) > > > > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c > > index 6a97e396..f0ce899 100644 > > --- a/drivers/gpu/drm/imx/ipuv3-plane.c > > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c > > @@ -70,8 +70,8 @@ drm_plane_state_to_eba(struct drm_plane_state *state) > > { > > struct drm_framebuffer *fb = state->fb; > > struct drm_gem_cma_object *cma_obj; > > - int x = state->src_x >> 16; > > - int y = state->src_y >> 16; > > + int x = state->src.x1 >> 16; > > + int y = state->src.y1 >> 16; > > > > cma_obj = drm_fb_cma_get_gem_obj(fb, 0); > > BUG_ON(!cma_obj); > > @@ -86,8 +86,8 @@ drm_plane_state_to_ubo(struct drm_plane_state *state) > > struct drm_framebuffer *fb = state->fb; > > struct drm_gem_cma_object *cma_obj; > > unsigned long eba = drm_plane_state_to_eba(state); > > - int x = state->src_x >> 16; > > - int y = state->src_y >> 16; > > + int x = state->src.x1 >> 16; > > + int y = state->src.y1 >> 16; > > > > cma_obj = drm_fb_cma_get_gem_obj(fb, 1); > > BUG_ON(!cma_obj); > > @@ -105,8 +105,8 @@ drm_plane_state_to_vbo(struct drm_plane_state *state) > > struct drm_framebuffer *fb = state->fb; > > struct drm_gem_cma_object *cma_obj; > > unsigned long eba = drm_plane_state_to_eba(state); > > - int x = state->src_x >> 16; > > - int y = state->src_y >> 16; > > + int x = state->src.x1 >> 16; > > + int y = state->src.y1 >> 16; > > > > cma_obj = drm_fb_cma_get_gem_obj(fb, 2); > > BUG_ON(!cma_obj); > > @@ -218,7 +218,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > > struct drm_framebuffer *fb = state->fb; > > struct drm_framebuffer *old_fb = old_state->fb; > > unsigned long eba, ubo, vbo, old_ubo, old_vbo; > > + bool can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY); > > + struct drm_rect clip; > > int hsub, vsub; > > + int ret; > > > > /* Ok to disable */ > > if (!fb) > > @@ -232,44 +235,35 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > > if (WARN_ON(!crtc_state)) > > return -EINVAL; > > > > + clip.x1 = 0; > > + clip.y1 = 0; > > + clip.x2 = crtc_state->adjusted_mode.hdisplay; > > + clip.y2 = crtc_state->adjusted_mode.vdisplay; > > + ret = drm_plane_helper_check_state(state, &clip, > > + DRM_PLANE_HELPER_NO_SCALING, > > + DRM_PLANE_HELPER_NO_SCALING, > > + can_position, true); > > + if (ret) > > + return ret; > > + > > /* CRTC should be enabled */ > > if (!crtc_state->enable) > > return -EINVAL; > > > > - /* no scaling */ > > - if (state->src_w >> 16 != state->crtc_w || > > - state->src_h >> 16 != state->crtc_h) > > - return -EINVAL; > > - > > switch (plane->type) { > > case DRM_PLANE_TYPE_PRIMARY: > > - /* full plane doesn't support partial off screen */ > > - if (state->crtc_x || state->crtc_y || > > - state->crtc_w != crtc_state->adjusted_mode.hdisplay || > > - state->crtc_h != crtc_state->adjusted_mode.vdisplay) > > - return -EINVAL; > > - > > /* full plane minimum width is 13 pixels */ > > - if (state->crtc_w < 13) > > + if (drm_rect_width(&state->dst) < 13) > > return -EINVAL; > > break; > > case DRM_PLANE_TYPE_OVERLAY: > > - if (state->crtc_x < 0 || state->crtc_y < 0) > > - return -EINVAL; > > - > > - if (state->crtc_x + state->crtc_w > > > - crtc_state->adjusted_mode.hdisplay) > > - return -EINVAL; > > - if (state->crtc_y + state->crtc_h > > > - crtc_state->adjusted_mode.vdisplay) > > - return -EINVAL; > > break; > > default: > > - dev_warn(dev, "Unsupported plane type\n"); > > + dev_warn(dev, "Unsupported plane type %d\n", plane->type); > > return -EINVAL; > > } > > > > - if (state->crtc_h < 2) > > + if (drm_rect_height(&state->dst) < 2) > > return -EINVAL; > > > > /* > > @@ -279,9 +273,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > > * callback. The planes will be reenabled in plane's ->atomic_update > > * callback. > > */ > > - if (old_fb && (state->src_w != old_state->src_w || > > - state->src_h != old_state->src_h || > > - fb->pixel_format != old_fb->pixel_format)) > > + if (old_fb && (!drm_rect_equals(&state->dst, &old_state->dst) || > > I presume you don't want to force a modeset for moving a plane around. > Using drm_rect_equals() here would end up doing that. > > Otherwise it all looks quite decent to me. Thanks, I'll use drm_rect_width / drm_rect_height as I should have. regards Philipp
On Wed, Oct 19, 2016 at 6:21 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Use drm_plane_helper_check_state to clip raw user coordinates to crtc > bounds. This checks for full plane coverage and scaling already, so > we can drop some custom checks. Use the clipped coordinates everywhere. > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/gpu/drm/imx/ipuv3-plane.c | 78 ++++++++++++++++++--------------------- > 1 file changed, 36 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c > index 6a97e396..f0ce899 100644 > --- a/drivers/gpu/drm/imx/ipuv3-plane.c > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c > @@ -70,8 +70,8 @@ drm_plane_state_to_eba(struct drm_plane_state *state) > { > struct drm_framebuffer *fb = state->fb; > struct drm_gem_cma_object *cma_obj; > - int x = state->src_x >> 16; > - int y = state->src_y >> 16; > + int x = state->src.x1 >> 16; > + int y = state->src.y1 >> 16; > > cma_obj = drm_fb_cma_get_gem_obj(fb, 0); > BUG_ON(!cma_obj); > @@ -86,8 +86,8 @@ drm_plane_state_to_ubo(struct drm_plane_state *state) > struct drm_framebuffer *fb = state->fb; > struct drm_gem_cma_object *cma_obj; > unsigned long eba = drm_plane_state_to_eba(state); > - int x = state->src_x >> 16; > - int y = state->src_y >> 16; > + int x = state->src.x1 >> 16; > + int y = state->src.y1 >> 16; > > cma_obj = drm_fb_cma_get_gem_obj(fb, 1); > BUG_ON(!cma_obj); > @@ -105,8 +105,8 @@ drm_plane_state_to_vbo(struct drm_plane_state *state) > struct drm_framebuffer *fb = state->fb; > struct drm_gem_cma_object *cma_obj; > unsigned long eba = drm_plane_state_to_eba(state); > - int x = state->src_x >> 16; > - int y = state->src_y >> 16; > + int x = state->src.x1 >> 16; > + int y = state->src.y1 >> 16; > > cma_obj = drm_fb_cma_get_gem_obj(fb, 2); > BUG_ON(!cma_obj); > @@ -218,7 +218,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > struct drm_framebuffer *fb = state->fb; > struct drm_framebuffer *old_fb = old_state->fb; > unsigned long eba, ubo, vbo, old_ubo, old_vbo; > + bool can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY); > + struct drm_rect clip; > int hsub, vsub; > + int ret; > > /* Ok to disable */ > if (!fb) > @@ -232,44 +235,35 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > if (WARN_ON(!crtc_state)) > return -EINVAL; > > + clip.x1 = 0; > + clip.y1 = 0; > + clip.x2 = crtc_state->adjusted_mode.hdisplay; > + clip.y2 = crtc_state->adjusted_mode.vdisplay; > + ret = drm_plane_helper_check_state(state, &clip, > + DRM_PLANE_HELPER_NO_SCALING, > + DRM_PLANE_HELPER_NO_SCALING, > + can_position, true); > + if (ret) > + return ret; Does the clip thing potentially change the user's request by force? For example, the user request an unreasonable big resolution. > + > /* CRTC should be enabled */ > if (!crtc_state->enable) > return -EINVAL; > > - /* no scaling */ > - if (state->src_w >> 16 != state->crtc_w || > - state->src_h >> 16 != state->crtc_h) > - return -EINVAL; > - > switch (plane->type) { > case DRM_PLANE_TYPE_PRIMARY: > - /* full plane doesn't support partial off screen */ > - if (state->crtc_x || state->crtc_y || > - state->crtc_w != crtc_state->adjusted_mode.hdisplay || > - state->crtc_h != crtc_state->adjusted_mode.vdisplay) > - return -EINVAL; > - > /* full plane minimum width is 13 pixels */ > - if (state->crtc_w < 13) > + if (drm_rect_width(&state->dst) < 13) > return -EINVAL; > break; > case DRM_PLANE_TYPE_OVERLAY: > - if (state->crtc_x < 0 || state->crtc_y < 0) > - return -EINVAL; How does drm_plane_helper_check_state() cover this check? > - > - if (state->crtc_x + state->crtc_w > > - crtc_state->adjusted_mode.hdisplay) > - return -EINVAL; > - if (state->crtc_y + state->crtc_h > > - crtc_state->adjusted_mode.vdisplay) > - return -EINVAL; And these? Regards, Liu Ying > break; > default: > - dev_warn(dev, "Unsupported plane type\n"); > + dev_warn(dev, "Unsupported plane type %d\n", plane->type); > return -EINVAL; > } > > - if (state->crtc_h < 2) > + if (drm_rect_height(&state->dst) < 2) > return -EINVAL; > > /* > @@ -279,9 +273,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > * callback. The planes will be reenabled in plane's ->atomic_update > * callback. > */ > - if (old_fb && (state->src_w != old_state->src_w || > - state->src_h != old_state->src_h || > - fb->pixel_format != old_fb->pixel_format)) > + if (old_fb && (!drm_rect_equals(&state->dst, &old_state->dst) || > + fb->pixel_format != old_fb->pixel_format)) > crtc_state->mode_changed = true; > > eba = drm_plane_state_to_eba(state); > @@ -350,8 +343,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > */ > hsub = drm_format_horz_chroma_subsampling(fb->pixel_format); > vsub = drm_format_vert_chroma_subsampling(fb->pixel_format); > - if (((state->src_x >> 16) & (hsub - 1)) || > - ((state->src_y >> 16) & (vsub - 1))) > + if (((state->src.x1 >> 16) & (hsub - 1)) || > + ((state->src.y1 >> 16) & (vsub - 1))) > return -EINVAL; > } > > @@ -395,8 +388,8 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, > ics = ipu_drm_fourcc_to_colorspace(state->fb->pixel_format); > ipu_dp_setup_channel(ipu_plane->dp, ics, > IPUV3_COLORSPACE_UNKNOWN); > - ipu_dp_set_window_pos(ipu_plane->dp, state->crtc_x, > - state->crtc_y); > + ipu_dp_set_window_pos(ipu_plane->dp, > + state->dst.x1, state->dst.y1); > /* Enable local alpha on partial plane */ > switch (state->fb->pixel_format) { > case DRM_FORMAT_ARGB1555: > @@ -416,11 +409,12 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, > } > } > > - ipu_dmfc_config_wait4eot(ipu_plane->dmfc, state->crtc_w); > + ipu_dmfc_config_wait4eot(ipu_plane->dmfc, drm_rect_width(&state->dst)); > > ipu_cpmem_zero(ipu_plane->ipu_ch); > - ipu_cpmem_set_resolution(ipu_plane->ipu_ch, state->src_w >> 16, > - state->src_h >> 16); > + ipu_cpmem_set_resolution(ipu_plane->ipu_ch, > + drm_rect_width(&state->src) >> 16, > + drm_rect_height(&state->src) >> 16); > ipu_cpmem_set_fmt(ipu_plane->ipu_ch, state->fb->pixel_format); > ipu_cpmem_set_high_priority(ipu_plane->ipu_ch); > ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1); > @@ -444,7 +438,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, > > dev_dbg(ipu_plane->base.dev->dev, > "phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo, > - state->src_x >> 16, state->src_y >> 16); > + state->src.x1 >> 16, state->src.y1 >> 16); > break; > case DRM_FORMAT_NV12: > case DRM_FORMAT_NV16: > @@ -455,11 +449,11 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, > > dev_dbg(ipu_plane->base.dev->dev, > "phy = %lu %lu, x = %d, y = %d", eba, ubo, > - state->src_x >> 16, state->src_y >> 16); > + state->src.x1 >> 16, state->src.y1 >> 16); > break; > default: > dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d", > - eba, state->src_x >> 16, state->src_y >> 16); > + eba, state->src.x1 >> 16, state->src.y1 >> 16); > break; > } > ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba); > -- > 2.9.3 >
On Thu, Oct 20, 2016 at 03:53:46PM +0800, Ying Liu wrote: > On Wed, Oct 19, 2016 at 6:21 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Use drm_plane_helper_check_state to clip raw user coordinates to crtc > > bounds. This checks for full plane coverage and scaling already, so > > we can drop some custom checks. Use the clipped coordinates everywhere. > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > drivers/gpu/drm/imx/ipuv3-plane.c | 78 ++++++++++++++++++--------------------- > > 1 file changed, 36 insertions(+), 42 deletions(-) > > > > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c > > index 6a97e396..f0ce899 100644 > > --- a/drivers/gpu/drm/imx/ipuv3-plane.c > > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c > > @@ -70,8 +70,8 @@ drm_plane_state_to_eba(struct drm_plane_state *state) > > { > > struct drm_framebuffer *fb = state->fb; > > struct drm_gem_cma_object *cma_obj; > > - int x = state->src_x >> 16; > > - int y = state->src_y >> 16; > > + int x = state->src.x1 >> 16; > > + int y = state->src.y1 >> 16; > > > > cma_obj = drm_fb_cma_get_gem_obj(fb, 0); > > BUG_ON(!cma_obj); > > @@ -86,8 +86,8 @@ drm_plane_state_to_ubo(struct drm_plane_state *state) > > struct drm_framebuffer *fb = state->fb; > > struct drm_gem_cma_object *cma_obj; > > unsigned long eba = drm_plane_state_to_eba(state); > > - int x = state->src_x >> 16; > > - int y = state->src_y >> 16; > > + int x = state->src.x1 >> 16; > > + int y = state->src.y1 >> 16; > > > > cma_obj = drm_fb_cma_get_gem_obj(fb, 1); > > BUG_ON(!cma_obj); > > @@ -105,8 +105,8 @@ drm_plane_state_to_vbo(struct drm_plane_state *state) > > struct drm_framebuffer *fb = state->fb; > > struct drm_gem_cma_object *cma_obj; > > unsigned long eba = drm_plane_state_to_eba(state); > > - int x = state->src_x >> 16; > > - int y = state->src_y >> 16; > > + int x = state->src.x1 >> 16; > > + int y = state->src.y1 >> 16; > > > > cma_obj = drm_fb_cma_get_gem_obj(fb, 2); > > BUG_ON(!cma_obj); > > @@ -218,7 +218,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > > struct drm_framebuffer *fb = state->fb; > > struct drm_framebuffer *old_fb = old_state->fb; > > unsigned long eba, ubo, vbo, old_ubo, old_vbo; > > + bool can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY); > > + struct drm_rect clip; > > int hsub, vsub; > > + int ret; > > > > /* Ok to disable */ > > if (!fb) > > @@ -232,44 +235,35 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > > if (WARN_ON(!crtc_state)) > > return -EINVAL; > > > > + clip.x1 = 0; > > + clip.y1 = 0; > > + clip.x2 = crtc_state->adjusted_mode.hdisplay; > > + clip.y2 = crtc_state->adjusted_mode.vdisplay; > > + ret = drm_plane_helper_check_state(state, &clip, > > + DRM_PLANE_HELPER_NO_SCALING, > > + DRM_PLANE_HELPER_NO_SCALING, > > + can_position, true); > > + if (ret) > > + return ret; > > Does the clip thing potentially change the user's request by force? > For example, the user request an unreasonable big resolution. The user is allowed to ask for destination coordinates extending outside the crtc dimensions. This will chop off the parts that aren't visible, and it will chop off the corresponding areas of the source as well. > > > + > > /* CRTC should be enabled */ > > if (!crtc_state->enable) > > return -EINVAL; > > > > - /* no scaling */ > > - if (state->src_w >> 16 != state->crtc_w || > > - state->src_h >> 16 != state->crtc_h) > > - return -EINVAL; > > - > > switch (plane->type) { > > case DRM_PLANE_TYPE_PRIMARY: > > - /* full plane doesn't support partial off screen */ > > - if (state->crtc_x || state->crtc_y || > > - state->crtc_w != crtc_state->adjusted_mode.hdisplay || > > - state->crtc_h != crtc_state->adjusted_mode.vdisplay) > > - return -EINVAL; > > - > > /* full plane minimum width is 13 pixels */ > > - if (state->crtc_w < 13) > > + if (drm_rect_width(&state->dst) < 13) > > return -EINVAL; > > break; > > case DRM_PLANE_TYPE_OVERLAY: > > - if (state->crtc_x < 0 || state->crtc_y < 0) > > - return -EINVAL; > > How does drm_plane_helper_check_state() cover this check? > > > - > > - if (state->crtc_x + state->crtc_w > > > - crtc_state->adjusted_mode.hdisplay) > > - return -EINVAL; > > - if (state->crtc_y + state->crtc_h > > > - crtc_state->adjusted_mode.vdisplay) > > - return -EINVAL; > > And these? > > Regards, > Liu Ying > > > break; > > default: > > - dev_warn(dev, "Unsupported plane type\n"); > > + dev_warn(dev, "Unsupported plane type %d\n", plane->type); > > return -EINVAL; > > } > > > > - if (state->crtc_h < 2) > > + if (drm_rect_height(&state->dst) < 2) > > return -EINVAL; > > > > /* > > @@ -279,9 +273,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > > * callback. The planes will be reenabled in plane's ->atomic_update > > * callback. > > */ > > - if (old_fb && (state->src_w != old_state->src_w || > > - state->src_h != old_state->src_h || > > - fb->pixel_format != old_fb->pixel_format)) > > + if (old_fb && (!drm_rect_equals(&state->dst, &old_state->dst) || > > + fb->pixel_format != old_fb->pixel_format)) > > crtc_state->mode_changed = true; > > > > eba = drm_plane_state_to_eba(state); > > @@ -350,8 +343,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > > */ > > hsub = drm_format_horz_chroma_subsampling(fb->pixel_format); > > vsub = drm_format_vert_chroma_subsampling(fb->pixel_format); > > - if (((state->src_x >> 16) & (hsub - 1)) || > > - ((state->src_y >> 16) & (vsub - 1))) > > + if (((state->src.x1 >> 16) & (hsub - 1)) || > > + ((state->src.y1 >> 16) & (vsub - 1))) > > return -EINVAL; > > } > > > > @@ -395,8 +388,8 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, > > ics = ipu_drm_fourcc_to_colorspace(state->fb->pixel_format); > > ipu_dp_setup_channel(ipu_plane->dp, ics, > > IPUV3_COLORSPACE_UNKNOWN); > > - ipu_dp_set_window_pos(ipu_plane->dp, state->crtc_x, > > - state->crtc_y); > > + ipu_dp_set_window_pos(ipu_plane->dp, > > + state->dst.x1, state->dst.y1); > > /* Enable local alpha on partial plane */ > > switch (state->fb->pixel_format) { > > case DRM_FORMAT_ARGB1555: > > @@ -416,11 +409,12 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, > > } > > } > > > > - ipu_dmfc_config_wait4eot(ipu_plane->dmfc, state->crtc_w); > > + ipu_dmfc_config_wait4eot(ipu_plane->dmfc, drm_rect_width(&state->dst)); > > > > ipu_cpmem_zero(ipu_plane->ipu_ch); > > - ipu_cpmem_set_resolution(ipu_plane->ipu_ch, state->src_w >> 16, > > - state->src_h >> 16); > > + ipu_cpmem_set_resolution(ipu_plane->ipu_ch, > > + drm_rect_width(&state->src) >> 16, > > + drm_rect_height(&state->src) >> 16); > > ipu_cpmem_set_fmt(ipu_plane->ipu_ch, state->fb->pixel_format); > > ipu_cpmem_set_high_priority(ipu_plane->ipu_ch); > > ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1); > > @@ -444,7 +438,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, > > > > dev_dbg(ipu_plane->base.dev->dev, > > "phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo, > > - state->src_x >> 16, state->src_y >> 16); > > + state->src.x1 >> 16, state->src.y1 >> 16); > > break; > > case DRM_FORMAT_NV12: > > case DRM_FORMAT_NV16: > > @@ -455,11 +449,11 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, > > > > dev_dbg(ipu_plane->base.dev->dev, > > "phy = %lu %lu, x = %d, y = %d", eba, ubo, > > - state->src_x >> 16, state->src_y >> 16); > > + state->src.x1 >> 16, state->src.y1 >> 16); > > break; > > default: > > dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d", > > - eba, state->src_x >> 16, state->src_y >> 16); > > + eba, state->src.x1 >> 16, state->src.y1 >> 16); > > break; > > } > > ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba); > > -- > > 2.9.3 > >
On Thu, Oct 20, 2016 at 4:41 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Oct 20, 2016 at 03:53:46PM +0800, Ying Liu wrote: >> On Wed, Oct 19, 2016 at 6:21 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: >> > Use drm_plane_helper_check_state to clip raw user coordinates to crtc >> > bounds. This checks for full plane coverage and scaling already, so >> > we can drop some custom checks. Use the clipped coordinates everywhere. >> > >> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> >> > --- >> > drivers/gpu/drm/imx/ipuv3-plane.c | 78 ++++++++++++++++++--------------------- >> > 1 file changed, 36 insertions(+), 42 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c >> > index 6a97e396..f0ce899 100644 >> > --- a/drivers/gpu/drm/imx/ipuv3-plane.c >> > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c >> > @@ -70,8 +70,8 @@ drm_plane_state_to_eba(struct drm_plane_state *state) >> > { >> > struct drm_framebuffer *fb = state->fb; >> > struct drm_gem_cma_object *cma_obj; >> > - int x = state->src_x >> 16; >> > - int y = state->src_y >> 16; >> > + int x = state->src.x1 >> 16; >> > + int y = state->src.y1 >> 16; >> > >> > cma_obj = drm_fb_cma_get_gem_obj(fb, 0); >> > BUG_ON(!cma_obj); >> > @@ -86,8 +86,8 @@ drm_plane_state_to_ubo(struct drm_plane_state *state) >> > struct drm_framebuffer *fb = state->fb; >> > struct drm_gem_cma_object *cma_obj; >> > unsigned long eba = drm_plane_state_to_eba(state); >> > - int x = state->src_x >> 16; >> > - int y = state->src_y >> 16; >> > + int x = state->src.x1 >> 16; >> > + int y = state->src.y1 >> 16; >> > >> > cma_obj = drm_fb_cma_get_gem_obj(fb, 1); >> > BUG_ON(!cma_obj); >> > @@ -105,8 +105,8 @@ drm_plane_state_to_vbo(struct drm_plane_state *state) >> > struct drm_framebuffer *fb = state->fb; >> > struct drm_gem_cma_object *cma_obj; >> > unsigned long eba = drm_plane_state_to_eba(state); >> > - int x = state->src_x >> 16; >> > - int y = state->src_y >> 16; >> > + int x = state->src.x1 >> 16; >> > + int y = state->src.y1 >> 16; >> > >> > cma_obj = drm_fb_cma_get_gem_obj(fb, 2); >> > BUG_ON(!cma_obj); >> > @@ -218,7 +218,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, >> > struct drm_framebuffer *fb = state->fb; >> > struct drm_framebuffer *old_fb = old_state->fb; >> > unsigned long eba, ubo, vbo, old_ubo, old_vbo; >> > + bool can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY); >> > + struct drm_rect clip; >> > int hsub, vsub; >> > + int ret; >> > >> > /* Ok to disable */ >> > if (!fb) >> > @@ -232,44 +235,35 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, >> > if (WARN_ON(!crtc_state)) >> > return -EINVAL; >> > >> > + clip.x1 = 0; >> > + clip.y1 = 0; >> > + clip.x2 = crtc_state->adjusted_mode.hdisplay; >> > + clip.y2 = crtc_state->adjusted_mode.vdisplay; >> > + ret = drm_plane_helper_check_state(state, &clip, >> > + DRM_PLANE_HELPER_NO_SCALING, >> > + DRM_PLANE_HELPER_NO_SCALING, >> > + can_position, true); >> > + if (ret) >> > + return ret; >> >> Does the clip thing potentially change the user's request by force? >> For example, the user request an unreasonable big resolution. > > The user is allowed to ask for destination coordinates extending outside > the crtc dimensions. This will chop off the parts that aren't visible, > and it will chop off the corresponding areas of the source as well. How about returning -EINVAL in this case which stands for an atomic check failure? > >> >> > + >> > /* CRTC should be enabled */ >> > if (!crtc_state->enable) >> > return -EINVAL; >> > >> > - /* no scaling */ >> > - if (state->src_w >> 16 != state->crtc_w || >> > - state->src_h >> 16 != state->crtc_h) >> > - return -EINVAL; >> > - >> > switch (plane->type) { >> > case DRM_PLANE_TYPE_PRIMARY: >> > - /* full plane doesn't support partial off screen */ >> > - if (state->crtc_x || state->crtc_y || >> > - state->crtc_w != crtc_state->adjusted_mode.hdisplay || >> > - state->crtc_h != crtc_state->adjusted_mode.vdisplay) >> > - return -EINVAL; >> > - >> > /* full plane minimum width is 13 pixels */ >> > - if (state->crtc_w < 13) >> > + if (drm_rect_width(&state->dst) < 13) >> > return -EINVAL; >> > break; >> > case DRM_PLANE_TYPE_OVERLAY: >> > - if (state->crtc_x < 0 || state->crtc_y < 0) >> > - return -EINVAL; >> >> How does drm_plane_helper_check_state() cover this check? >> >> > - >> > - if (state->crtc_x + state->crtc_w > >> > - crtc_state->adjusted_mode.hdisplay) >> > - return -EINVAL; >> > - if (state->crtc_y + state->crtc_h > >> > - crtc_state->adjusted_mode.vdisplay) >> > - return -EINVAL; >> >> And these? >> >> Regards, >> Liu Ying >> >> > break; >> > default: >> > - dev_warn(dev, "Unsupported plane type\n"); >> > + dev_warn(dev, "Unsupported plane type %d\n", plane->type); >> > return -EINVAL; >> > } >> > >> > - if (state->crtc_h < 2) >> > + if (drm_rect_height(&state->dst) < 2) >> > return -EINVAL; >> > >> > /* >> > @@ -279,9 +273,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, >> > * callback. The planes will be reenabled in plane's ->atomic_update >> > * callback. >> > */ >> > - if (old_fb && (state->src_w != old_state->src_w || >> > - state->src_h != old_state->src_h || >> > - fb->pixel_format != old_fb->pixel_format)) >> > + if (old_fb && (!drm_rect_equals(&state->dst, &old_state->dst) || >> > + fb->pixel_format != old_fb->pixel_format)) >> > crtc_state->mode_changed = true; >> > >> > eba = drm_plane_state_to_eba(state); >> > @@ -350,8 +343,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, >> > */ >> > hsub = drm_format_horz_chroma_subsampling(fb->pixel_format); >> > vsub = drm_format_vert_chroma_subsampling(fb->pixel_format); >> > - if (((state->src_x >> 16) & (hsub - 1)) || >> > - ((state->src_y >> 16) & (vsub - 1))) >> > + if (((state->src.x1 >> 16) & (hsub - 1)) || >> > + ((state->src.y1 >> 16) & (vsub - 1))) >> > return -EINVAL; >> > } >> > >> > @@ -395,8 +388,8 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, >> > ics = ipu_drm_fourcc_to_colorspace(state->fb->pixel_format); >> > ipu_dp_setup_channel(ipu_plane->dp, ics, >> > IPUV3_COLORSPACE_UNKNOWN); >> > - ipu_dp_set_window_pos(ipu_plane->dp, state->crtc_x, >> > - state->crtc_y); >> > + ipu_dp_set_window_pos(ipu_plane->dp, >> > + state->dst.x1, state->dst.y1); >> > /* Enable local alpha on partial plane */ >> > switch (state->fb->pixel_format) { >> > case DRM_FORMAT_ARGB1555: >> > @@ -416,11 +409,12 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, >> > } >> > } >> > >> > - ipu_dmfc_config_wait4eot(ipu_plane->dmfc, state->crtc_w); >> > + ipu_dmfc_config_wait4eot(ipu_plane->dmfc, drm_rect_width(&state->dst)); >> > >> > ipu_cpmem_zero(ipu_plane->ipu_ch); >> > - ipu_cpmem_set_resolution(ipu_plane->ipu_ch, state->src_w >> 16, >> > - state->src_h >> 16); >> > + ipu_cpmem_set_resolution(ipu_plane->ipu_ch, >> > + drm_rect_width(&state->src) >> 16, >> > + drm_rect_height(&state->src) >> 16); >> > ipu_cpmem_set_fmt(ipu_plane->ipu_ch, state->fb->pixel_format); >> > ipu_cpmem_set_high_priority(ipu_plane->ipu_ch); >> > ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1); >> > @@ -444,7 +438,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, >> > >> > dev_dbg(ipu_plane->base.dev->dev, >> > "phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo, >> > - state->src_x >> 16, state->src_y >> 16); >> > + state->src.x1 >> 16, state->src.y1 >> 16); >> > break; >> > case DRM_FORMAT_NV12: >> > case DRM_FORMAT_NV16: >> > @@ -455,11 +449,11 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, >> > >> > dev_dbg(ipu_plane->base.dev->dev, >> > "phy = %lu %lu, x = %d, y = %d", eba, ubo, >> > - state->src_x >> 16, state->src_y >> 16); >> > + state->src.x1 >> 16, state->src.y1 >> 16); >> > break; >> > default: >> > dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d", >> > - eba, state->src_x >> 16, state->src_y >> 16); >> > + eba, state->src.x1 >> 16, state->src.y1 >> 16); >> > break; >> > } >> > ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba); >> > -- >> > 2.9.3 >> > > > -- > Ville Syrjälä > Intel OTC
On Thu, Oct 20, 2016 at 04:51:30PM +0800, Ying Liu wrote: > On Thu, Oct 20, 2016 at 4:41 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > On Thu, Oct 20, 2016 at 03:53:46PM +0800, Ying Liu wrote: > >> On Wed, Oct 19, 2016 at 6:21 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > >> > Use drm_plane_helper_check_state to clip raw user coordinates to crtc > >> > bounds. This checks for full plane coverage and scaling already, so > >> > we can drop some custom checks. Use the clipped coordinates everywhere. > >> > > >> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > >> > --- > >> > drivers/gpu/drm/imx/ipuv3-plane.c | 78 ++++++++++++++++++--------------------- > >> > 1 file changed, 36 insertions(+), 42 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c > >> > index 6a97e396..f0ce899 100644 > >> > --- a/drivers/gpu/drm/imx/ipuv3-plane.c > >> > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c > >> > @@ -70,8 +70,8 @@ drm_plane_state_to_eba(struct drm_plane_state *state) > >> > { > >> > struct drm_framebuffer *fb = state->fb; > >> > struct drm_gem_cma_object *cma_obj; > >> > - int x = state->src_x >> 16; > >> > - int y = state->src_y >> 16; > >> > + int x = state->src.x1 >> 16; > >> > + int y = state->src.y1 >> 16; > >> > > >> > cma_obj = drm_fb_cma_get_gem_obj(fb, 0); > >> > BUG_ON(!cma_obj); > >> > @@ -86,8 +86,8 @@ drm_plane_state_to_ubo(struct drm_plane_state *state) > >> > struct drm_framebuffer *fb = state->fb; > >> > struct drm_gem_cma_object *cma_obj; > >> > unsigned long eba = drm_plane_state_to_eba(state); > >> > - int x = state->src_x >> 16; > >> > - int y = state->src_y >> 16; > >> > + int x = state->src.x1 >> 16; > >> > + int y = state->src.y1 >> 16; > >> > > >> > cma_obj = drm_fb_cma_get_gem_obj(fb, 1); > >> > BUG_ON(!cma_obj); > >> > @@ -105,8 +105,8 @@ drm_plane_state_to_vbo(struct drm_plane_state *state) > >> > struct drm_framebuffer *fb = state->fb; > >> > struct drm_gem_cma_object *cma_obj; > >> > unsigned long eba = drm_plane_state_to_eba(state); > >> > - int x = state->src_x >> 16; > >> > - int y = state->src_y >> 16; > >> > + int x = state->src.x1 >> 16; > >> > + int y = state->src.y1 >> 16; > >> > > >> > cma_obj = drm_fb_cma_get_gem_obj(fb, 2); > >> > BUG_ON(!cma_obj); > >> > @@ -218,7 +218,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > >> > struct drm_framebuffer *fb = state->fb; > >> > struct drm_framebuffer *old_fb = old_state->fb; > >> > unsigned long eba, ubo, vbo, old_ubo, old_vbo; > >> > + bool can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY); > >> > + struct drm_rect clip; > >> > int hsub, vsub; > >> > + int ret; > >> > > >> > /* Ok to disable */ > >> > if (!fb) > >> > @@ -232,44 +235,35 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > >> > if (WARN_ON(!crtc_state)) > >> > return -EINVAL; > >> > > >> > + clip.x1 = 0; > >> > + clip.y1 = 0; > >> > + clip.x2 = crtc_state->adjusted_mode.hdisplay; > >> > + clip.y2 = crtc_state->adjusted_mode.vdisplay; > >> > + ret = drm_plane_helper_check_state(state, &clip, > >> > + DRM_PLANE_HELPER_NO_SCALING, > >> > + DRM_PLANE_HELPER_NO_SCALING, > >> > + can_position, true); > >> > + if (ret) > >> > + return ret; > >> > >> Does the clip thing potentially change the user's request by force? > >> For example, the user request an unreasonable big resolution. > > > > The user is allowed to ask for destination coordinates extending outside > > the crtc dimensions. This will chop off the parts that aren't visible, > > and it will chop off the corresponding areas of the source as well. > > How about returning -EINVAL in this case which stands for > an atomic check failure? That's not the expected behaviour. > > > > >> > >> > + > >> > /* CRTC should be enabled */ > >> > if (!crtc_state->enable) > >> > return -EINVAL; > >> > > >> > - /* no scaling */ > >> > - if (state->src_w >> 16 != state->crtc_w || > >> > - state->src_h >> 16 != state->crtc_h) > >> > - return -EINVAL; > >> > - > >> > switch (plane->type) { > >> > case DRM_PLANE_TYPE_PRIMARY: > >> > - /* full plane doesn't support partial off screen */ > >> > - if (state->crtc_x || state->crtc_y || > >> > - state->crtc_w != crtc_state->adjusted_mode.hdisplay || > >> > - state->crtc_h != crtc_state->adjusted_mode.vdisplay) > >> > - return -EINVAL; > >> > - > >> > /* full plane minimum width is 13 pixels */ > >> > - if (state->crtc_w < 13) > >> > + if (drm_rect_width(&state->dst) < 13) > >> > return -EINVAL; > >> > break; > >> > case DRM_PLANE_TYPE_OVERLAY: > >> > - if (state->crtc_x < 0 || state->crtc_y < 0) > >> > - return -EINVAL; > >> > >> How does drm_plane_helper_check_state() cover this check? > >> > >> > - > >> > - if (state->crtc_x + state->crtc_w > > >> > - crtc_state->adjusted_mode.hdisplay) > >> > - return -EINVAL; > >> > - if (state->crtc_y + state->crtc_h > > >> > - crtc_state->adjusted_mode.vdisplay) > >> > - return -EINVAL; > >> > >> And these? > >> > >> Regards, > >> Liu Ying > >> > >> > break; > >> > default: > >> > - dev_warn(dev, "Unsupported plane type\n"); > >> > + dev_warn(dev, "Unsupported plane type %d\n", plane->type); > >> > return -EINVAL; > >> > } > >> > > >> > - if (state->crtc_h < 2) > >> > + if (drm_rect_height(&state->dst) < 2) > >> > return -EINVAL; > >> > > >> > /* > >> > @@ -279,9 +273,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > >> > * callback. The planes will be reenabled in plane's ->atomic_update > >> > * callback. > >> > */ > >> > - if (old_fb && (state->src_w != old_state->src_w || > >> > - state->src_h != old_state->src_h || > >> > - fb->pixel_format != old_fb->pixel_format)) > >> > + if (old_fb && (!drm_rect_equals(&state->dst, &old_state->dst) || > >> > + fb->pixel_format != old_fb->pixel_format)) > >> > crtc_state->mode_changed = true; > >> > > >> > eba = drm_plane_state_to_eba(state); > >> > @@ -350,8 +343,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > >> > */ > >> > hsub = drm_format_horz_chroma_subsampling(fb->pixel_format); > >> > vsub = drm_format_vert_chroma_subsampling(fb->pixel_format); > >> > - if (((state->src_x >> 16) & (hsub - 1)) || > >> > - ((state->src_y >> 16) & (vsub - 1))) > >> > + if (((state->src.x1 >> 16) & (hsub - 1)) || > >> > + ((state->src.y1 >> 16) & (vsub - 1))) > >> > return -EINVAL; > >> > } > >> > > >> > @@ -395,8 +388,8 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, > >> > ics = ipu_drm_fourcc_to_colorspace(state->fb->pixel_format); > >> > ipu_dp_setup_channel(ipu_plane->dp, ics, > >> > IPUV3_COLORSPACE_UNKNOWN); > >> > - ipu_dp_set_window_pos(ipu_plane->dp, state->crtc_x, > >> > - state->crtc_y); > >> > + ipu_dp_set_window_pos(ipu_plane->dp, > >> > + state->dst.x1, state->dst.y1); > >> > /* Enable local alpha on partial plane */ > >> > switch (state->fb->pixel_format) { > >> > case DRM_FORMAT_ARGB1555: > >> > @@ -416,11 +409,12 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, > >> > } > >> > } > >> > > >> > - ipu_dmfc_config_wait4eot(ipu_plane->dmfc, state->crtc_w); > >> > + ipu_dmfc_config_wait4eot(ipu_plane->dmfc, drm_rect_width(&state->dst)); > >> > > >> > ipu_cpmem_zero(ipu_plane->ipu_ch); > >> > - ipu_cpmem_set_resolution(ipu_plane->ipu_ch, state->src_w >> 16, > >> > - state->src_h >> 16); > >> > + ipu_cpmem_set_resolution(ipu_plane->ipu_ch, > >> > + drm_rect_width(&state->src) >> 16, > >> > + drm_rect_height(&state->src) >> 16); > >> > ipu_cpmem_set_fmt(ipu_plane->ipu_ch, state->fb->pixel_format); > >> > ipu_cpmem_set_high_priority(ipu_plane->ipu_ch); > >> > ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1); > >> > @@ -444,7 +438,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, > >> > > >> > dev_dbg(ipu_plane->base.dev->dev, > >> > "phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo, > >> > - state->src_x >> 16, state->src_y >> 16); > >> > + state->src.x1 >> 16, state->src.y1 >> 16); > >> > break; > >> > case DRM_FORMAT_NV12: > >> > case DRM_FORMAT_NV16: > >> > @@ -455,11 +449,11 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, > >> > > >> > dev_dbg(ipu_plane->base.dev->dev, > >> > "phy = %lu %lu, x = %d, y = %d", eba, ubo, > >> > - state->src_x >> 16, state->src_y >> 16); > >> > + state->src.x1 >> 16, state->src.y1 >> 16); > >> > break; > >> > default: > >> > dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d", > >> > - eba, state->src_x >> 16, state->src_y >> 16); > >> > + eba, state->src.x1 >> 16, state->src.y1 >> 16); > >> > break; > >> > } > >> > ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba); > >> > -- > >> > 2.9.3 > >> > > > > > -- > > Ville Syrjälä > > Intel OTC
Am Donnerstag, den 20.10.2016, 16:51 +0800 schrieb Ying Liu: > >> Does the clip thing potentially change the user's request by force? > >> For example, the user request an unreasonable big resolution. > > > > The user is allowed to ask for destination coordinates extending outside > > the crtc dimensions. This will chop off the parts that aren't visible, > > and it will chop off the corresponding areas of the source as well. > > How about returning -EINVAL in this case which stands for > an atomic check failure? Say the user requests to display a 640x480+0,0 source framebuffer at destination offset -320,0 on a 320x240 screen, unscaled. The expectation would be to see the upper right quarter of the framebuffer on the screen, at least if the hardware was actually able to position overlays partially offscreen. If we can also fulfill that expectation by clipping the source rectangle to 320,240+320,0 and changing the destination rectangle to 320x240+0,0, why should -EINVAL be returned? regards Philipp
On Thu, Oct 20, 2016 at 9:29 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Am Donnerstag, den 20.10.2016, 16:51 +0800 schrieb Ying Liu: >> >> Does the clip thing potentially change the user's request by force? >> >> For example, the user request an unreasonable big resolution. >> > >> > The user is allowed to ask for destination coordinates extending outside >> > the crtc dimensions. This will chop off the parts that aren't visible, >> > and it will chop off the corresponding areas of the source as well. >> >> How about returning -EINVAL in this case which stands for >> an atomic check failure? > > Say the user requests to display a 640x480+0,0 source framebuffer at > destination offset -320,0 on a 320x240 screen, unscaled. The expectation > would be to see the upper right quarter of the framebuffer on the > screen, at least if the hardware was actually able to position overlays > partially offscreen. > If we can also fulfill that expectation by clipping the source rectangle > to 320,240+320,0 and changing the destination rectangle to 320x240+0,0, > why should -EINVAL be returned? Well, IIUC, there are two kinds of clipping. 1) Clipping a rectangle from a fb according to src_x/y and src_w/h. 2) Clipping done by drm_plane_helper_check_state(), which potentially changes src/dst->x1/2 and src/dst->y1/2(in other words, src_x/y, src_w/h and crtc_x/y/w/h, though not directly). 1) is fine, no problem. I doubt 2) is wrong as the users' original request could be changed. That's why I mentioned returning -EINVAL. Moreover, before and after applying the patch, I think the ->atomic_check behavior consistency is broken. For example, negative crtc_x or crtc_y for overlay are changed from unacceptable to potentially acceptable just because 2) may change their equivalent dst_>x/y1. Regards, Liu Ying > > regards > Philipp >
Am Freitag, den 21.10.2016, 13:45 +0800 schrieb Ying Liu: > On Thu, Oct 20, 2016 at 9:29 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Am Donnerstag, den 20.10.2016, 16:51 +0800 schrieb Ying Liu: > >> >> Does the clip thing potentially change the user's request by force? > >> >> For example, the user request an unreasonable big resolution. > >> > > >> > The user is allowed to ask for destination coordinates extending outside > >> > the crtc dimensions. This will chop off the parts that aren't visible, > >> > and it will chop off the corresponding areas of the source as well. > >> > >> How about returning -EINVAL in this case which stands for > >> an atomic check failure? > > > > Say the user requests to display a 640x480+0,0 source framebuffer at > > destination offset -320,0 on a 320x240 screen, unscaled. The expectation > > would be to see the upper right quarter of the framebuffer on the > > screen, at least if the hardware was actually able to position overlays > > partially offscreen. > > If we can also fulfill that expectation by clipping the source rectangle > > to 320,240+320,0 and changing the destination rectangle to 320x240+0,0, > > why should -EINVAL be returned? > > Well, IIUC, there are two kinds of clipping. > 1) Clipping a rectangle from a fb according to src_x/y and src_w/h. > 2) Clipping done by drm_plane_helper_check_state(), which potentially > changes src/dst->x1/2 and src/dst->y1/2(in other words, src_x/y, > src_w/h and crtc_x/y/w/h, though not directly). > > 1) is fine, no problem. > I doubt 2) is wrong as the users' original request could be changed. > That's why I mentioned returning -EINVAL. > > Moreover, before and after applying the patch, I think the > ->atomic_check behavior consistency is broken. For example, > negative crtc_x or crtc_y for overlay are changed from unacceptable > to potentially acceptable just because 2) may change their equivalent > dst_>x/y1. I fail to see what's wrong with 2) as long as we can keep the observable behaviour exactly the same as if the user request was unchanged. regards Philipp
On Fri, Oct 21, 2016 at 4:18 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Am Freitag, den 21.10.2016, 13:45 +0800 schrieb Ying Liu: >> On Thu, Oct 20, 2016 at 9:29 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: >> > Am Donnerstag, den 20.10.2016, 16:51 +0800 schrieb Ying Liu: >> >> >> Does the clip thing potentially change the user's request by force? >> >> >> For example, the user request an unreasonable big resolution. >> >> > >> >> > The user is allowed to ask for destination coordinates extending outside >> >> > the crtc dimensions. This will chop off the parts that aren't visible, >> >> > and it will chop off the corresponding areas of the source as well. >> >> >> >> How about returning -EINVAL in this case which stands for >> >> an atomic check failure? >> > >> > Say the user requests to display a 640x480+0,0 source framebuffer at >> > destination offset -320,0 on a 320x240 screen, unscaled. The expectation >> > would be to see the upper right quarter of the framebuffer on the >> > screen, at least if the hardware was actually able to position overlays >> > partially offscreen. >> > If we can also fulfill that expectation by clipping the source rectangle >> > to 320,240+320,0 and changing the destination rectangle to 320x240+0,0, >> > why should -EINVAL be returned? >> >> Well, IIUC, there are two kinds of clipping. >> 1) Clipping a rectangle from a fb according to src_x/y and src_w/h. >> 2) Clipping done by drm_plane_helper_check_state(), which potentially >> changes src/dst->x1/2 and src/dst->y1/2(in other words, src_x/y, >> src_w/h and crtc_x/y/w/h, though not directly). >> >> 1) is fine, no problem. >> I doubt 2) is wrong as the users' original request could be changed. >> That's why I mentioned returning -EINVAL. >> >> Moreover, before and after applying the patch, I think the >> ->atomic_check behavior consistency is broken. For example, >> negative crtc_x or crtc_y for overlay are changed from unacceptable >> to potentially acceptable just because 2) may change their equivalent >> dst_>x/y1. > > I fail to see what's wrong with 2) as long as we can keep the observable > behaviour exactly the same as if the user request was unchanged. It seems the behavior could change - negative crtc_x or crtc_y for overlay make ->atomic_check return -EINVAL before(overlay hw state machine has nothing changed), and potentially successful after(overlay hw state machine changes). Regards, Liu Ying > > regards > Philipp >
Am Freitag, den 21.10.2016, 16:49 +0800 schrieb Ying Liu: > On Fri, Oct 21, 2016 at 4:18 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Am Freitag, den 21.10.2016, 13:45 +0800 schrieb Ying Liu: > >> On Thu, Oct 20, 2016 at 9:29 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > >> > Am Donnerstag, den 20.10.2016, 16:51 +0800 schrieb Ying Liu: > >> >> >> Does the clip thing potentially change the user's request by force? > >> >> >> For example, the user request an unreasonable big resolution. > >> >> > > >> >> > The user is allowed to ask for destination coordinates extending outside > >> >> > the crtc dimensions. This will chop off the parts that aren't visible, > >> >> > and it will chop off the corresponding areas of the source as well. > >> >> > >> >> How about returning -EINVAL in this case which stands for > >> >> an atomic check failure? > >> > > >> > Say the user requests to display a 640x480+0,0 source framebuffer at > >> > destination offset -320,0 on a 320x240 screen, unscaled. The expectation > >> > would be to see the upper right quarter of the framebuffer on the > >> > screen, at least if the hardware was actually able to position overlays > >> > partially offscreen. > >> > If we can also fulfill that expectation by clipping the source rectangle > >> > to 320,240+320,0 and changing the destination rectangle to 320x240+0,0, > >> > why should -EINVAL be returned? > >> > >> Well, IIUC, there are two kinds of clipping. > >> 1) Clipping a rectangle from a fb according to src_x/y and src_w/h. > >> 2) Clipping done by drm_plane_helper_check_state(), which potentially > >> changes src/dst->x1/2 and src/dst->y1/2(in other words, src_x/y, > >> src_w/h and crtc_x/y/w/h, though not directly). > >> > >> 1) is fine, no problem. > >> I doubt 2) is wrong as the users' original request could be changed. > >> That's why I mentioned returning -EINVAL. > >> > >> Moreover, before and after applying the patch, I think the > >> ->atomic_check behavior consistency is broken. For example, > >> negative crtc_x or crtc_y for overlay are changed from unacceptable > >> to potentially acceptable just because 2) may change their equivalent > >> dst_>x/y1. > > > > I fail to see what's wrong with 2) as long as we can keep the observable > > behaviour exactly the same as if the user request was unchanged. > > It seems the behavior could change - negative crtc_x or crtc_y for > overlay make ->atomic_check return -EINVAL before(overlay hw state > machine has nothing changed), and potentially successful after(overlay > hw state machine changes). That in itself doesn't seem so bad. One thing we can't do though is 'position' at any negative crtc_x/y due to the fact that when clipping the src.x1/y1 still must be even for chroma subsampled pixel formats and the x1 still must result in scanline start addresses aligned to 8-byte boundaries. So for 32-bit framebuffer depth negative x offsets must be even, and for 16-bit framebuffer depth only negative x offsets that are a multiple of 4 are possible. regards Philipp
On Mon, Oct 24, 2016 at 7:50 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > Am Freitag, den 21.10.2016, 16:49 +0800 schrieb Ying Liu: >> On Fri, Oct 21, 2016 at 4:18 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: >> > Am Freitag, den 21.10.2016, 13:45 +0800 schrieb Ying Liu: >> >> On Thu, Oct 20, 2016 at 9:29 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: >> >> > Am Donnerstag, den 20.10.2016, 16:51 +0800 schrieb Ying Liu: >> >> >> >> Does the clip thing potentially change the user's request by force? >> >> >> >> For example, the user request an unreasonable big resolution. >> >> >> > >> >> >> > The user is allowed to ask for destination coordinates extending outside >> >> >> > the crtc dimensions. This will chop off the parts that aren't visible, >> >> >> > and it will chop off the corresponding areas of the source as well. >> >> >> >> >> >> How about returning -EINVAL in this case which stands for >> >> >> an atomic check failure? >> >> > >> >> > Say the user requests to display a 640x480+0,0 source framebuffer at >> >> > destination offset -320,0 on a 320x240 screen, unscaled. The expectation >> >> > would be to see the upper right quarter of the framebuffer on the >> >> > screen, at least if the hardware was actually able to position overlays >> >> > partially offscreen. >> >> > If we can also fulfill that expectation by clipping the source rectangle >> >> > to 320,240+320,0 and changing the destination rectangle to 320x240+0,0, >> >> > why should -EINVAL be returned? >> >> >> >> Well, IIUC, there are two kinds of clipping. >> >> 1) Clipping a rectangle from a fb according to src_x/y and src_w/h. >> >> 2) Clipping done by drm_plane_helper_check_state(), which potentially >> >> changes src/dst->x1/2 and src/dst->y1/2(in other words, src_x/y, >> >> src_w/h and crtc_x/y/w/h, though not directly). >> >> >> >> 1) is fine, no problem. >> >> I doubt 2) is wrong as the users' original request could be changed. >> >> That's why I mentioned returning -EINVAL. >> >> >> >> Moreover, before and after applying the patch, I think the >> >> ->atomic_check behavior consistency is broken. For example, >> >> negative crtc_x or crtc_y for overlay are changed from unacceptable >> >> to potentially acceptable just because 2) may change their equivalent >> >> dst_>x/y1. >> > >> > I fail to see what's wrong with 2) as long as we can keep the observable >> > behaviour exactly the same as if the user request was unchanged. >> >> It seems the behavior could change - negative crtc_x or crtc_y for >> overlay make ->atomic_check return -EINVAL before(overlay hw state >> machine has nothing changed), and potentially successful after(overlay >> hw state machine changes). > > That in itself doesn't seem so bad. One thing we can't do though is > 'position' at any negative crtc_x/y due to the fact that when clipping > the src.x1/y1 still must be even for chroma subsampled pixel formats and > the x1 still must result in scanline start addresses aligned to 8-byte > boundaries. So for 32-bit framebuffer depth negative x offsets must be > even, and for 16-bit framebuffer depth only negative x offsets that are > a multiple of 4 are possible. I think the alignment requirements from HW can be guaranteed by proper ->atomic_check implementation. The main concern about this patch is still the clipping 2) itself. Besides the negative crtc_x/y example, another one is that 'modetest -P 24:1280x800' may run successfully on the i.MX6Q SabreSD board with the 1024x768 LVDS primary display. IMO, this may confuse the userspace. Note that this test case cannot pass atomic check before applying this patch. Regards, Liu Ying > > regards > Philipp >
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index 6a97e396..f0ce899 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -70,8 +70,8 @@ drm_plane_state_to_eba(struct drm_plane_state *state) { struct drm_framebuffer *fb = state->fb; struct drm_gem_cma_object *cma_obj; - int x = state->src_x >> 16; - int y = state->src_y >> 16; + int x = state->src.x1 >> 16; + int y = state->src.y1 >> 16; cma_obj = drm_fb_cma_get_gem_obj(fb, 0); BUG_ON(!cma_obj); @@ -86,8 +86,8 @@ drm_plane_state_to_ubo(struct drm_plane_state *state) struct drm_framebuffer *fb = state->fb; struct drm_gem_cma_object *cma_obj; unsigned long eba = drm_plane_state_to_eba(state); - int x = state->src_x >> 16; - int y = state->src_y >> 16; + int x = state->src.x1 >> 16; + int y = state->src.y1 >> 16; cma_obj = drm_fb_cma_get_gem_obj(fb, 1); BUG_ON(!cma_obj); @@ -105,8 +105,8 @@ drm_plane_state_to_vbo(struct drm_plane_state *state) struct drm_framebuffer *fb = state->fb; struct drm_gem_cma_object *cma_obj; unsigned long eba = drm_plane_state_to_eba(state); - int x = state->src_x >> 16; - int y = state->src_y >> 16; + int x = state->src.x1 >> 16; + int y = state->src.y1 >> 16; cma_obj = drm_fb_cma_get_gem_obj(fb, 2); BUG_ON(!cma_obj); @@ -218,7 +218,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, struct drm_framebuffer *fb = state->fb; struct drm_framebuffer *old_fb = old_state->fb; unsigned long eba, ubo, vbo, old_ubo, old_vbo; + bool can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY); + struct drm_rect clip; int hsub, vsub; + int ret; /* Ok to disable */ if (!fb) @@ -232,44 +235,35 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, if (WARN_ON(!crtc_state)) return -EINVAL; + clip.x1 = 0; + clip.y1 = 0; + clip.x2 = crtc_state->adjusted_mode.hdisplay; + clip.y2 = crtc_state->adjusted_mode.vdisplay; + ret = drm_plane_helper_check_state(state, &clip, + DRM_PLANE_HELPER_NO_SCALING, + DRM_PLANE_HELPER_NO_SCALING, + can_position, true); + if (ret) + return ret; + /* CRTC should be enabled */ if (!crtc_state->enable) return -EINVAL; - /* no scaling */ - if (state->src_w >> 16 != state->crtc_w || - state->src_h >> 16 != state->crtc_h) - return -EINVAL; - switch (plane->type) { case DRM_PLANE_TYPE_PRIMARY: - /* full plane doesn't support partial off screen */ - if (state->crtc_x || state->crtc_y || - state->crtc_w != crtc_state->adjusted_mode.hdisplay || - state->crtc_h != crtc_state->adjusted_mode.vdisplay) - return -EINVAL; - /* full plane minimum width is 13 pixels */ - if (state->crtc_w < 13) + if (drm_rect_width(&state->dst) < 13) return -EINVAL; break; case DRM_PLANE_TYPE_OVERLAY: - if (state->crtc_x < 0 || state->crtc_y < 0) - return -EINVAL; - - if (state->crtc_x + state->crtc_w > - crtc_state->adjusted_mode.hdisplay) - return -EINVAL; - if (state->crtc_y + state->crtc_h > - crtc_state->adjusted_mode.vdisplay) - return -EINVAL; break; default: - dev_warn(dev, "Unsupported plane type\n"); + dev_warn(dev, "Unsupported plane type %d\n", plane->type); return -EINVAL; } - if (state->crtc_h < 2) + if (drm_rect_height(&state->dst) < 2) return -EINVAL; /* @@ -279,9 +273,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, * callback. The planes will be reenabled in plane's ->atomic_update * callback. */ - if (old_fb && (state->src_w != old_state->src_w || - state->src_h != old_state->src_h || - fb->pixel_format != old_fb->pixel_format)) + if (old_fb && (!drm_rect_equals(&state->dst, &old_state->dst) || + fb->pixel_format != old_fb->pixel_format)) crtc_state->mode_changed = true; eba = drm_plane_state_to_eba(state); @@ -350,8 +343,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, */ hsub = drm_format_horz_chroma_subsampling(fb->pixel_format); vsub = drm_format_vert_chroma_subsampling(fb->pixel_format); - if (((state->src_x >> 16) & (hsub - 1)) || - ((state->src_y >> 16) & (vsub - 1))) + if (((state->src.x1 >> 16) & (hsub - 1)) || + ((state->src.y1 >> 16) & (vsub - 1))) return -EINVAL; } @@ -395,8 +388,8 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, ics = ipu_drm_fourcc_to_colorspace(state->fb->pixel_format); ipu_dp_setup_channel(ipu_plane->dp, ics, IPUV3_COLORSPACE_UNKNOWN); - ipu_dp_set_window_pos(ipu_plane->dp, state->crtc_x, - state->crtc_y); + ipu_dp_set_window_pos(ipu_plane->dp, + state->dst.x1, state->dst.y1); /* Enable local alpha on partial plane */ switch (state->fb->pixel_format) { case DRM_FORMAT_ARGB1555: @@ -416,11 +409,12 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, } } - ipu_dmfc_config_wait4eot(ipu_plane->dmfc, state->crtc_w); + ipu_dmfc_config_wait4eot(ipu_plane->dmfc, drm_rect_width(&state->dst)); ipu_cpmem_zero(ipu_plane->ipu_ch); - ipu_cpmem_set_resolution(ipu_plane->ipu_ch, state->src_w >> 16, - state->src_h >> 16); + ipu_cpmem_set_resolution(ipu_plane->ipu_ch, + drm_rect_width(&state->src) >> 16, + drm_rect_height(&state->src) >> 16); ipu_cpmem_set_fmt(ipu_plane->ipu_ch, state->fb->pixel_format); ipu_cpmem_set_high_priority(ipu_plane->ipu_ch); ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1); @@ -444,7 +438,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, dev_dbg(ipu_plane->base.dev->dev, "phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo, - state->src_x >> 16, state->src_y >> 16); + state->src.x1 >> 16, state->src.y1 >> 16); break; case DRM_FORMAT_NV12: case DRM_FORMAT_NV16: @@ -455,11 +449,11 @@ static void ipu_plane_atomic_update(struct drm_plane *plane, dev_dbg(ipu_plane->base.dev->dev, "phy = %lu %lu, x = %d, y = %d", eba, ubo, - state->src_x >> 16, state->src_y >> 16); + state->src.x1 >> 16, state->src.y1 >> 16); break; default: dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d", - eba, state->src_x >> 16, state->src_y >> 16); + eba, state->src.x1 >> 16, state->src.y1 >> 16); break; } ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
Use drm_plane_helper_check_state to clip raw user coordinates to crtc bounds. This checks for full plane coverage and scaling already, so we can drop some custom checks. Use the clipped coordinates everywhere. Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/gpu/drm/imx/ipuv3-plane.c | 78 ++++++++++++++++++--------------------- 1 file changed, 36 insertions(+), 42 deletions(-)