Re: [PATCH v3 2/2] drm: rcar-du: Clip planes to screen boundaries
Hi Laurent,
Thankyou for the patch.
On 26/11/17 11:30, Laurent Pinchart wrote:
> Unlike the KMS API, the hardware doesn't support planes exceeding the
> screen boundaries or planes being located fully off-screen. We need to
> clip plane coordinates to support the use case.
>
> Fortunately the DRM core offers a drm_atomic_helper_check_plane_state()
> helper that valides the scaling factor and clips the plane coordinates.
s/valides/validates/
> Use it to implement the plane atomic check and use the clipped source
> and destination rectangles from the plane state instead of the unclipped
> source and CRTC coordinates to configure the device.
>
> Signed-off-by: Laurent Pinchart
Aside from the spelling error above - I can't find a fault here. Maybe next
time :-)
Reviewed-by: Kieran Bingham
> ---
> Changes since v2:
>
> - Actually use the clipped source and destination rectangles
> - Use drm_crtc_state::mode instead of drm_crtc_state::adjusted_mode
> where applicable
> - Removed spurious semicolon
> - Rebased on top of latest drm+drm-misc
> ---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +-
> drivers/gpu/drm/rcar-du/rcar_du_plane.c | 50
> -
> drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 42 ++-
> 3 files changed, 62 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index b492063a6e1f..5685d5af6998 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -319,7 +319,8 @@ static void rcar_du_crtc_update_planes(struct
> rcar_du_crtc *rcrtc)
> struct rcar_du_plane *plane = &rcrtc->group->planes[i];
> unsigned int j;
>
> - if (plane->plane.state->crtc != &rcrtc->crtc)
> + if (plane->plane.state->crtc != &rcrtc->crtc ||
> + !plane->plane.state->visible)
> continue;
>
> /* Insert the plane in the sorted planes array. */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> index 4f076c364f25..4a3d16cf3ed6 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> @@ -332,8 +332,8 @@ static void rcar_du_plane_write(struct rcar_du_group
> *rgrp,
> static void rcar_du_plane_setup_scanout(struct rcar_du_group *rgrp,
> const struct rcar_du_plane_state *state)
> {
> - unsigned int src_x = state->state.src_x >> 16;
> - unsigned int src_y = state->state.src_y >> 16;
> + unsigned int src_x = state->state.src.x1 >> 16;
> + unsigned int src_y = state->state.src.y1 >> 16;
> unsigned int index = state->hwindex;
> unsigned int pitch;
> bool interlaced;
> @@ -357,7 +357,7 @@ static void rcar_du_plane_setup_scanout(struct
> rcar_du_group *rgrp,
> dma[i] = gem->paddr + fb->offsets[i];
> }
> } else {
> - pitch = state->state.src_w >> 16;
> + pitch = drm_rect_width(&state->state.src) >> 16;
> dma[0] = 0;
> dma[1] = 0;
> }
> @@ -521,6 +521,7 @@ static void rcar_du_plane_setup_format(struct
> rcar_du_group *rgrp,
> const struct rcar_du_plane_state *state)
> {
> struct rcar_du_device *rcdu = rgrp->dev;
> + const struct drm_rect *dst = &state->state.dst;
>
> if (rcdu->info->gen < 3)
> rcar_du_plane_setup_format_gen2(rgrp, index, state);
> @@ -528,10 +529,10 @@ static void rcar_du_plane_setup_format(struct
> rcar_du_group *rgrp,
> rcar_du_plane_setup_format_gen3(rgrp, index, state);
>
> /* Destination position and size */
> - rcar_du_plane_write(rgrp, index, PnDSXR, state->state.crtc_w);
> - rcar_du_plane_write(rgrp, index, PnDSYR, state->state.crtc_h);
> - rcar_du_plane_write(rgrp, index, PnDPXR, state->state.crtc_x);
> - rcar_du_plane_write(rgrp, index, PnDPYR, state->state.crtc_y);
> + rcar_du_plane_write(rgrp, index, PnDSXR, drm_rect_width(dst));
> + rcar_du_plane_write(rgrp, index, PnDSYR, drm_rect_height(dst));
> + rcar_du_plane_write(rgrp, index, PnDPXR, dst->x1);
> + rcar_du_plane_write(rgrp, index, PnDPYR, dst->y1);
>
> if (rcdu->info->gen < 3) {
> /* Wrap-around and blinking, disabled */
> @@ -570,16 +571,39 @@ int __rcar_du_plane_atomic_check(struct drm_plane
> *plane,
>const struct rcar_du_format_info **format)
> {
> struct drm_device *dev = plane->dev;
> + struct drm_crtc_state *crtc_state;
> + struct drm_rect clip;
> + int ret;
>
> - if (!state->fb || !state->crtc) {
> + if (!state->crtc) {
> + /*
> + * The visible field is not reset by the DRM core but only
> + * updated by drm_plane_helper_check_state(), se
[PATCH v3 2/2] drm: rcar-du: Clip planes to screen boundaries
Unlike the KMS API, the hardware doesn't support planes exceeding the
screen boundaries or planes being located fully off-screen. We need to
clip plane coordinates to support the use case.
Fortunately the DRM core offers a drm_atomic_helper_check_plane_state()
helper that valides the scaling factor and clips the plane coordinates.
Use it to implement the plane atomic check and use the clipped source
and destination rectangles from the plane state instead of the unclipped
source and CRTC coordinates to configure the device.
Signed-off-by: Laurent Pinchart
---
Changes since v2:
- Actually use the clipped source and destination rectangles
- Use drm_crtc_state::mode instead of drm_crtc_state::adjusted_mode
where applicable
- Removed spurious semicolon
- Rebased on top of latest drm+drm-misc
---
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 +-
drivers/gpu/drm/rcar-du/rcar_du_plane.c | 50 -
drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 42 ++-
3 files changed, 62 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index b492063a6e1f..5685d5af6998 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -319,7 +319,8 @@ static void rcar_du_crtc_update_planes(struct rcar_du_crtc
*rcrtc)
struct rcar_du_plane *plane = &rcrtc->group->planes[i];
unsigned int j;
- if (plane->plane.state->crtc != &rcrtc->crtc)
+ if (plane->plane.state->crtc != &rcrtc->crtc ||
+ !plane->plane.state->visible)
continue;
/* Insert the plane in the sorted planes array. */
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index 4f076c364f25..4a3d16cf3ed6 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -332,8 +332,8 @@ static void rcar_du_plane_write(struct rcar_du_group *rgrp,
static void rcar_du_plane_setup_scanout(struct rcar_du_group *rgrp,
const struct rcar_du_plane_state *state)
{
- unsigned int src_x = state->state.src_x >> 16;
- unsigned int src_y = state->state.src_y >> 16;
+ unsigned int src_x = state->state.src.x1 >> 16;
+ unsigned int src_y = state->state.src.y1 >> 16;
unsigned int index = state->hwindex;
unsigned int pitch;
bool interlaced;
@@ -357,7 +357,7 @@ static void rcar_du_plane_setup_scanout(struct
rcar_du_group *rgrp,
dma[i] = gem->paddr + fb->offsets[i];
}
} else {
- pitch = state->state.src_w >> 16;
+ pitch = drm_rect_width(&state->state.src) >> 16;
dma[0] = 0;
dma[1] = 0;
}
@@ -521,6 +521,7 @@ static void rcar_du_plane_setup_format(struct rcar_du_group
*rgrp,
const struct rcar_du_plane_state *state)
{
struct rcar_du_device *rcdu = rgrp->dev;
+ const struct drm_rect *dst = &state->state.dst;
if (rcdu->info->gen < 3)
rcar_du_plane_setup_format_gen2(rgrp, index, state);
@@ -528,10 +529,10 @@ static void rcar_du_plane_setup_format(struct
rcar_du_group *rgrp,
rcar_du_plane_setup_format_gen3(rgrp, index, state);
/* Destination position and size */
- rcar_du_plane_write(rgrp, index, PnDSXR, state->state.crtc_w);
- rcar_du_plane_write(rgrp, index, PnDSYR, state->state.crtc_h);
- rcar_du_plane_write(rgrp, index, PnDPXR, state->state.crtc_x);
- rcar_du_plane_write(rgrp, index, PnDPYR, state->state.crtc_y);
+ rcar_du_plane_write(rgrp, index, PnDSXR, drm_rect_width(dst));
+ rcar_du_plane_write(rgrp, index, PnDSYR, drm_rect_height(dst));
+ rcar_du_plane_write(rgrp, index, PnDPXR, dst->x1);
+ rcar_du_plane_write(rgrp, index, PnDPYR, dst->y1);
if (rcdu->info->gen < 3) {
/* Wrap-around and blinking, disabled */
@@ -570,16 +571,39 @@ int __rcar_du_plane_atomic_check(struct drm_plane *plane,
const struct rcar_du_format_info **format)
{
struct drm_device *dev = plane->dev;
+ struct drm_crtc_state *crtc_state;
+ struct drm_rect clip;
+ int ret;
- if (!state->fb || !state->crtc) {
+ if (!state->crtc) {
+ /*
+* The visible field is not reset by the DRM core but only
+* updated by drm_plane_helper_check_state(), set it manually.
+*/
+ state->visible = false;
*format = NULL;
return 0;
}
- if (state->src_w >> 16 != state->crtc_w ||
- state->src_h >> 16 != state->crtc_h) {
- dev_dbg(dev->dev, "%s: scaling not supported\n", __func__);
- ret
