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@xxxxxxxxxxxxxxx> > > Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> > > --- > > 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel