From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Philipp Zabel <p.zabel@pengutronix.de>
Cc: kernel@pengutronix.de, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 10/10] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates
Date: Wed, 19 Oct 2016 13:31:33 +0300 [thread overview]
Message-ID: <20161019103133.GG4329@intel.com> (raw)
In-Reply-To: <1476872477-29493-10-git-send-email-p.zabel@pengutronix.de>
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
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2016-10-19 10:31 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-19 10:21 [PATCH 01/10] drm/imx: ipuv3-plane: make sure x/y offsets are even in case of chroma subsampling Philipp Zabel
2016-10-19 10:21 ` [PATCH 02/10] drm/imx: ipuv3-plane: disable local alpha for planes without alpha channel Philipp Zabel
2016-10-19 10:21 ` [PATCH 03/10] drm/imx: ipuv3-plane: request modeset if plane offsets changed Philipp Zabel
2016-10-19 10:21 ` [PATCH 04/10] drm/imx: ipuv3-plane: merge ipu_plane_atomic_set_base into atomic_update Philipp Zabel
2016-10-19 10:21 ` [PATCH 05/10] drm/imx: ipuv3-plane: let drm_plane_state_to_ubo/vbo handle chroma subsampling other than 4:2:0 Philipp Zabel
2016-10-20 6:19 ` Ying Liu
2016-10-19 10:21 ` [PATCH 06/10] gpu: ipu-cpmem: remove unused ipu_cpmem_set_yuv_planar function Philipp Zabel
2016-10-20 6:20 ` Ying Liu
2016-10-19 10:21 ` [PATCH 07/10] gpu: ipu-v3: add YUV 4:4:4 support Philipp Zabel
2016-10-19 10:21 ` [PATCH 08/10] drm/imx: ipuv3-plane: add support for YUV 4:2:2 and 4:4:4, NV12, and NV16 formats Philipp Zabel
2016-10-20 6:21 ` Ying Liu
2016-10-19 10:21 ` [PATCH 09/10] gpu: ipu-v3: initially clear all interrupts Philipp Zabel
2016-10-19 10:21 ` [PATCH 10/10] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates Philipp Zabel
2016-10-19 10:31 ` Ville Syrjälä [this message]
2016-10-19 10:46 ` Philipp Zabel
2016-10-20 7:53 ` Ying Liu
2016-10-20 8:41 ` Ville Syrjälä
2016-10-20 8:51 ` Ying Liu
2016-10-20 13:20 ` Ville Syrjälä
2016-10-20 13:29 ` Philipp Zabel
2016-10-21 5:45 ` Ying Liu
2016-10-21 8:18 ` Philipp Zabel
2016-10-21 8:49 ` Ying Liu
2016-10-24 11:50 ` Philipp Zabel
2016-10-25 2:59 ` Ying Liu
2016-10-20 6:18 ` [PATCH 01/10] drm/imx: ipuv3-plane: make sure x/y offsets are even in case of chroma subsampling Ying Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161019103133.GG4329@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=kernel@pengutronix.de \
--cc=p.zabel@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox