[PATCH 1/7] drm/msm: dpu: Remove impossible checks

[PATCH 1/7] drm/msm: dpu: Remove impossible checks

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

This patch removes some checks which are impossible to hit. As a result,
we can move some of the local var assignments into the declarations.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 46de0de466ff..0f428f66b951 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1080,30 +1080,13 @@ static int dpu_plane_sspp_atomic_check(struct drm_plane *plane,
 		struct drm_plane_state *state)
 {
 	int ret = 0;
-	struct dpu_plane *pdpu;
-	struct dpu_plane_state *pstate;
+	struct dpu_plane *pdpu = to_dpu_plane(plane);
 	const struct dpu_format *fmt;
 	struct drm_rect src, dst, fb_rect = { 0 };
 	uint32_t max_upscale = 1, max_downscale = 1;
 	uint32_t min_src_size, max_linewidth;
 	int hscale = 1, vscale = 1;
 
-	if (!plane || !state) {
-		DPU_ERROR("invalid arg(s), plane %d state %d\n",
-				plane != 0, state != 0);
-		ret = -EINVAL;
-		goto exit;
-	}
-
-	pdpu = to_dpu_plane(plane);
-	pstate = to_dpu_plane_state(state);
-
-	if (!pdpu->pipe_sblk) {
-		DPU_ERROR_PLANE(pdpu, "invalid catalog\n");
-		ret = -EINVAL;
-		goto exit;
-	}
-
 	src.x1 = state->src_x >> 16;
 	src.y1 = state->src_y >> 16;
 	src.x2 = src.x1 + (state->src_w >> 16);
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 2/7] drm/msm: dpu: Move atomic_check_plane_state() call to atomic_check

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

src/dst rects are checked in both atomic_check and atomic_update, with
the more comprehensive check occurring in atomic_update, which is
backwards. So consolodate the checks in atomic_check.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 56 +++++++----------------
 1 file changed, 16 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 0f428f66b951..853902ac4fb9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1079,13 +1079,26 @@ static bool dpu_plane_validate_src(struct drm_rect *src,
 static int dpu_plane_sspp_atomic_check(struct drm_plane *plane,
 		struct drm_plane_state *state)
 {
-	int ret = 0;
+	int ret = 0, min_scale;
 	struct dpu_plane *pdpu = to_dpu_plane(plane);
+	const struct drm_crtc_state *crtc_state = NULL;
 	const struct dpu_format *fmt;
 	struct drm_rect src, dst, fb_rect = { 0 };
-	uint32_t max_upscale = 1, max_downscale = 1;
 	uint32_t min_src_size, max_linewidth;
-	int hscale = 1, vscale = 1;
+
+	if (state->crtc)
+		crtc_state = state->crtc->state;
+
+	min_scale = FRAC_16_16(1, pdpu->pipe_sblk->maxdwnscale);
+	ret = drm_atomic_helper_check_plane_state(state, crtc_state, min_scale,
+					  pdpu->pipe_sblk->maxupscale << 16,
+					  true, true);
+	if (ret) {
+		DPU_ERROR_PLANE(pdpu, "Check plane state failed (%d)\n", ret);
+		return ret;
+	}
+	if (!state->visible)
+		return 0;
 
 	src.x1 = state->src_x >> 16;
 	src.y1 = state->src_y >> 16;
@@ -1099,25 +1112,6 @@ static int dpu_plane_sspp_atomic_check(struct drm_plane *plane,
 
 	max_linewidth = pdpu->pipe_sblk->common->maxlinewidth;
 
-	if (pdpu->features & DPU_SSPP_SCALER) {
-		max_downscale = pdpu->pipe_sblk->maxdwnscale;
-		max_upscale = pdpu->pipe_sblk->maxupscale;
-	}
-	if (drm_rect_width(&src) < drm_rect_width(&dst))
-		hscale = drm_rect_calc_hscale(&src, &dst, 1, max_upscale);
-	else
-		hscale = drm_rect_calc_hscale(&dst, &src, 1, max_downscale);
-	if (drm_rect_height(&src) < drm_rect_height(&dst))
-		vscale = drm_rect_calc_vscale(&src, &dst, 1, max_upscale);
-	else
-		vscale = drm_rect_calc_vscale(&dst, &src, 1, max_downscale);
-
-	DPU_DEBUG_PLANE(pdpu, "check %d -> %d\n",
-		dpu_plane_enabled(plane->state), dpu_plane_enabled(state));
-
-	if (!dpu_plane_enabled(state))
-		goto exit;
-
 	fmt = to_dpu_format(msm_framebuffer_format(state->fb));
 
 	min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
@@ -1156,16 +1150,8 @@ static int dpu_plane_sspp_atomic_check(struct drm_plane *plane,
 		DPU_ERROR_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
 				DRM_RECT_ARG(&src), max_linewidth);
 		ret = -E2BIG;
-
-	/* check scaler capability */
-	} else if (hscale < 0 || vscale < 0) {
-		DPU_ERROR_PLANE(pdpu, "invalid scaling requested src="
-				DRM_RECT_FMT " dst=" DRM_RECT_FMT "\n",
-				DRM_RECT_ARG(&src), DRM_RECT_ARG(&dst));
-		ret = -E2BIG;
 	}
 
-exit:
 	return ret;
 }
 
@@ -1237,7 +1223,6 @@ static int dpu_plane_sspp_atomic_update(struct drm_plane *plane,
 	const struct dpu_format *fmt;
 	struct drm_crtc *crtc;
 	struct drm_framebuffer *fb;
-	int ret, min_scale;
 
 	if (!plane) {
 		DPU_ERROR("invalid plane\n");
@@ -1276,15 +1261,6 @@ static int dpu_plane_sspp_atomic_update(struct drm_plane *plane,
 	pdpu->is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT);
 	_dpu_plane_set_qos_ctrl(plane, false, DPU_PLANE_QOS_PANIC_CTRL);
 
-	min_scale = FRAC_16_16(1, pdpu->pipe_sblk->maxdwnscale);
-	ret = drm_atomic_helper_check_plane_state(state, crtc->state, min_scale,
-					  pdpu->pipe_sblk->maxupscale << 16,
-					  true, false);
-	if (ret) {
-		DPU_ERROR_PLANE(pdpu, "Check plane state failed (%d)\n", ret);
-		return ret;
-	}
-
 	DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u " DRM_RECT_FMT
 			", %4.4s ubwc %d\n", fb->base.id, DRM_RECT_FP_ARG(&state->src),
 			crtc->base.id, DRM_RECT_ARG(&state->dst),
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 3/7] drm/msm: dpu: Consolodate atomic_check functions()

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

dpu_plane_atomic_check() is a very thin wrapper around
dpu_plane_sspp_atomic_check(). All it does is a NULL-check of state->fb,
which is already done by drm_atomic_helper_check_plane_state(). Further,
the helper sets state->visible = false when this is true. So remove
dpu_plane_atomic_check() and just use dpu_plane_sspp_atomic_check()
directly.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 853902ac4fb9..7e894c8f69c3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1076,8 +1076,8 @@ static bool dpu_plane_validate_src(struct drm_rect *src,
 		drm_rect_equals(fb_rect, src);
 }
 
-static int dpu_plane_sspp_atomic_check(struct drm_plane *plane,
-		struct drm_plane_state *state)
+static int dpu_plane_atomic_check(struct drm_plane *plane,
+				  struct drm_plane_state *state)
 {
 	int ret = 0, min_scale;
 	struct dpu_plane *pdpu = to_dpu_plane(plane);
@@ -1155,17 +1155,6 @@ static int dpu_plane_sspp_atomic_check(struct drm_plane *plane,
 	return ret;
 }
 
-static int dpu_plane_atomic_check(struct drm_plane *plane,
-		struct drm_plane_state *state)
-{
-	if (!state->fb)
-		return 0;
-
-	DPU_DEBUG_PLANE(to_dpu_plane(plane), "\n");
-
-	return dpu_plane_sspp_atomic_check(plane, state);
-}
-
 void dpu_plane_flush(struct drm_plane *plane)
 {
 	struct dpu_plane *pdpu;
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 4/7] drm/msm: dpu: Remove dpu_plane_sspp_enabled()

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

It's doing the same thing dpu_plane_enabled() is.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 7e894c8f69c3..fd3c7cb9e5b8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -138,11 +138,6 @@ static bool dpu_plane_enabled(struct drm_plane_state *state)
 	return state && state->fb && state->crtc;
 }
 
-static bool dpu_plane_sspp_enabled(struct drm_plane_state *state)
-{
-	return state && state->crtc;
-}
-
 /**
  * _dpu_plane_calc_fill_level - calculate fill level of the given source format
  * @plane:		Pointer to drm plane
@@ -1397,7 +1392,7 @@ static void dpu_plane_atomic_update(struct drm_plane *plane,
 
 	DPU_DEBUG_PLANE(pdpu, "\n");
 
-	if (!dpu_plane_sspp_enabled(state)) {
+	if (!dpu_plane_enabled(state)) {
 		_dpu_plane_atomic_disable(plane, old_state);
 	} else {
 		int ret;
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 5/7] drm/msm: dpu: Remove dpu_plane_enabled()

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

plane->state->visible encompasses all of these checks and more, so we
can just check visible.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index fd3c7cb9e5b8..10f32cf6f239 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -133,11 +133,6 @@ static struct dpu_kms *_dpu_plane_get_kms(struct drm_plane *plane)
 	return to_dpu_kms(priv->kms);
 }
 
-static bool dpu_plane_enabled(struct drm_plane_state *state)
-{
-	return state && state->fb && state->crtc;
-}
-
 /**
  * _dpu_plane_calc_fill_level - calculate fill level of the given source format
  * @plane:		Pointer to drm plane
@@ -163,7 +158,7 @@ static inline int _dpu_plane_calc_fill_level(struct drm_plane *plane,
 	fixed_buff_size = pdpu->pipe_sblk->common->pixel_ram_size;
 
 	list_for_each_entry(tmp, &pdpu->mplane_list, mplane_list) {
-		if (!dpu_plane_enabled(tmp->base.state))
+		if (!tmp->base.state->visible)
 			continue;
 		DPU_DEBUG("plane%d/%d src_width:%d/%d\n",
 				pdpu->base.base.id, tmp->base.base.id,
@@ -1392,7 +1387,7 @@ static void dpu_plane_atomic_update(struct drm_plane *plane,
 
 	DPU_DEBUG_PLANE(pdpu, "\n");
 
-	if (!dpu_plane_enabled(state)) {
+	if (!state->visible) {
 		_dpu_plane_atomic_disable(plane, old_state);
 	} else {
 		int ret;
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 6/7] drm/msm: dpu: Make dpu_plane_sspp_atomic_update() void

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

All of the checks in dpu_plane_sspp_atomic_update() are impossible, so
remove them and make the function void. This removes the need to error
check in dpu_plane_atomic_update(). Additionally, remove impossible checks
in dpu_plane_atomic_update().

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 71 +++++------------------
 1 file changed, 14 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 10f32cf6f239..fa338cf4d0eb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1191,45 +1191,17 @@ void dpu_plane_set_error(struct drm_plane *plane, bool error)
 	pdpu->is_error = error;
 }
 
-static int dpu_plane_sspp_atomic_update(struct drm_plane *plane,
-				struct drm_plane_state *old_state)
+static void dpu_plane_sspp_atomic_update(struct drm_plane *plane,
+					 struct drm_plane_state *old_state)
 {
-	uint32_t nplanes, src_flags;
-	struct dpu_plane *pdpu;
-	struct drm_plane_state *state;
-	struct dpu_plane_state *pstate;
-	struct dpu_plane_state *old_pstate;
-	const struct dpu_format *fmt;
-	struct drm_crtc *crtc;
-	struct drm_framebuffer *fb;
-
-	if (!plane) {
-		DPU_ERROR("invalid plane\n");
-		return -EINVAL;
-	} else if (!plane->state) {
-		DPU_ERROR("invalid plane state\n");
-		return -EINVAL;
-	} else if (!old_state) {
-		DPU_ERROR("invalid old state\n");
-		return -EINVAL;
-	}
-
-	pdpu = to_dpu_plane(plane);
-	state = plane->state;
-
-	pstate = to_dpu_plane_state(state);
-
-	old_pstate = to_dpu_plane_state(old_state);
-
-	crtc = state->crtc;
-	fb = state->fb;
-	if (!crtc || !fb) {
-		DPU_ERROR_PLANE(pdpu, "invalid crtc %d or fb %d\n",
-				crtc != 0, fb != 0);
-		return -EINVAL;
-	}
-	fmt = to_dpu_format(msm_framebuffer_format(fb));
-	nplanes = fmt->num_planes;
+	uint32_t src_flags;
+	struct dpu_plane *pdpu = to_dpu_plane(plane);
+	struct drm_plane_state *state = plane->state;
+	struct dpu_plane_state *pstate = to_dpu_plane_state(state);
+	struct drm_crtc *crtc = state->crtc;
+	struct drm_framebuffer *fb = state->fb;
+	const struct dpu_format *fmt =
+		to_dpu_format(msm_framebuffer_format(fb));
 
 	memset(&(pdpu->pipe_cfg), 0, sizeof(struct dpu_hw_pipe_cfg));
 
@@ -1260,7 +1232,7 @@ static int dpu_plane_sspp_atomic_update(struct drm_plane *plane,
 	/* override for color fill */
 	if (pdpu->color_fill & DPU_PLANE_COLOR_FILL_FLAG) {
 		/* skip remaining processing on color fill */
-		return 0;
+		return;
 	}
 
 	if (pdpu->pipe_hw->ops.setup_rects) {
@@ -1331,7 +1303,6 @@ static int dpu_plane_sspp_atomic_update(struct drm_plane *plane,
 	}
 
 	_dpu_plane_set_qos_remap(plane);
-	return 0;
 }
 
 static void _dpu_plane_atomic_disable(struct drm_plane *plane,
@@ -1370,31 +1341,17 @@ static void _dpu_plane_atomic_disable(struct drm_plane *plane,
 static void dpu_plane_atomic_update(struct drm_plane *plane,
 				struct drm_plane_state *old_state)
 {
-	struct dpu_plane *pdpu;
-	struct drm_plane_state *state;
-
-	if (!plane) {
-		DPU_ERROR("invalid plane\n");
-		return;
-	} else if (!plane->state) {
-		DPU_ERROR("invalid plane state\n");
-		return;
-	}
+	struct dpu_plane *pdpu = to_dpu_plane(plane);
+	struct drm_plane_state *state = plane->state;
 
-	pdpu = to_dpu_plane(plane);
 	pdpu->is_error = false;
-	state = plane->state;
 
 	DPU_DEBUG_PLANE(pdpu, "\n");
 
 	if (!state->visible) {
 		_dpu_plane_atomic_disable(plane, old_state);
 	} else {
-		int ret;
-
-		ret = dpu_plane_sspp_atomic_update(plane, old_state);
-		/* atomic_check should have ensured that this doesn't fail */
-		WARN_ON(ret < 0);
+		dpu_plane_sspp_atomic_update(plane, old_state);
 	}
 }
 
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 7/7] drm/msm: dpu: Don't continue after error in atomic_check

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

There's no benefit in falling out of the if, just return directly.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index fa338cf4d0eb..43e0d27f5074 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1112,13 +1112,13 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
 		 | BIT(DPU_SSPP_CSC_10BIT))))) {
 		DPU_ERROR_PLANE(pdpu,
 				"plane doesn't have scaler/csc for yuv\n");
-		ret = -EINVAL;
+		return -EINVAL;
 
 	/* check src bounds */
 	} else if (!dpu_plane_validate_src(&src, &fb_rect, min_src_size)) {
 		DPU_ERROR_PLANE(pdpu, "invalid source " DRM_RECT_FMT "\n",
 				DRM_RECT_ARG(&src));
-		ret = -E2BIG;
+		return -E2BIG;
 
 	/* valid yuv image */
 	} else if (DPU_FORMAT_IS_YUV(fmt) &&
@@ -1127,22 +1127,22 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
 		    drm_rect_height(&src) & 0x1)) {
 		DPU_ERROR_PLANE(pdpu, "invalid yuv source " DRM_RECT_FMT "\n",
 				DRM_RECT_ARG(&src));
-		ret = -EINVAL;
+		return -EINVAL;
 
 	/* min dst support */
 	} else if (drm_rect_width(&dst) < 0x1 || drm_rect_height(&dst) < 0x1) {
 		DPU_ERROR_PLANE(pdpu, "invalid dest rect " DRM_RECT_FMT "\n",
 				DRM_RECT_ARG(&dst));
-		ret = -EINVAL;
+		return -EINVAL;
 
 	/* check decimated source width */
 	} else if (drm_rect_width(&src) > max_linewidth) {
 		DPU_ERROR_PLANE(pdpu, "invalid src " DRM_RECT_FMT " line:%u\n",
 				DRM_RECT_ARG(&src), max_linewidth);
-		ret = -E2BIG;
+		return -E2BIG;
 	}
 
-	return ret;
+	return 0;
 }
 
 void dpu_plane_flush(struct drm_plane *plane)
-- 
Sean Paul, Software Engineer, Google / Chromium OS

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 2/7] drm/msm: dpu: Move atomic_check_plane_state() call to atomic_check

[Jeykumar Sankaran]

On 2018-09-11 13:15, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> src/dst rects are checked in both atomic_check and atomic_update, with
> the more comprehensive check occurring in atomic_update, which is
> backwards. So consolodate the checks in atomic_check.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 56 +++++++----------------
>  1 file changed, 16 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 0f428f66b951..853902ac4fb9 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -1079,13 +1079,26 @@ static bool dpu_plane_validate_src(struct 
> drm_rect
> *src,
>  static int dpu_plane_sspp_atomic_check(struct drm_plane *plane,
>  		struct drm_plane_state *state)
>  {
> -	int ret = 0;
> +	int ret = 0, min_scale;
>  	struct dpu_plane *pdpu = to_dpu_plane(plane);
> +	const struct drm_crtc_state *crtc_state = NULL;
>  	const struct dpu_format *fmt;
>  	struct drm_rect src, dst, fb_rect = { 0 };
> -	uint32_t max_upscale = 1, max_downscale = 1;
>  	uint32_t min_src_size, max_linewidth;
> -	int hscale = 1, vscale = 1;
> +
> +	if (state->crtc)
> +		crtc_state = state->crtc->state;
Isn't the crtc_state retrieved here the old crtc state? Since the plane 
check is called before swap, the CRTC state
for this commit should be retrieved from global atomic state 
state->state using drm_atomic_get_existing_crtc_state.

Jeykumar S.
> +
> +	min_scale = FRAC_16_16(1, pdpu->pipe_sblk->maxdwnscale);
> +	ret = drm_atomic_helper_check_plane_state(state, crtc_state,
> min_scale,
> +					  pdpu->pipe_sblk->maxupscale <<
> 16,
> +					  true, true);
> +	if (ret) {
> +		DPU_ERROR_PLANE(pdpu, "Check plane state failed (%d)\n",
> ret);
> +		return ret;
> +	}
> +	if (!state->visible)
> +		return 0;
> 
>  	src.x1 = state->src_x >> 16;
>  	src.y1 = state->src_y >> 16;
> @@ -1099,25 +1112,6 @@ static int dpu_plane_sspp_atomic_check(struct
> drm_plane *plane,
> 
>  	max_linewidth = pdpu->pipe_sblk->common->maxlinewidth;
> 
> -	if (pdpu->features & DPU_SSPP_SCALER) {
> -		max_downscale = pdpu->pipe_sblk->maxdwnscale;
> -		max_upscale = pdpu->pipe_sblk->maxupscale;
> -	}
> -	if (drm_rect_width(&src) < drm_rect_width(&dst))
> -		hscale = drm_rect_calc_hscale(&src, &dst, 1, max_upscale);
> -	else
> -		hscale = drm_rect_calc_hscale(&dst, &src, 1,
> max_downscale);
> -	if (drm_rect_height(&src) < drm_rect_height(&dst))
> -		vscale = drm_rect_calc_vscale(&src, &dst, 1, max_upscale);
> -	else
> -		vscale = drm_rect_calc_vscale(&dst, &src, 1,
> max_downscale);
> -
> -	DPU_DEBUG_PLANE(pdpu, "check %d -> %d\n",
> -		dpu_plane_enabled(plane->state),
> dpu_plane_enabled(state));
> -
> -	if (!dpu_plane_enabled(state))
> -		goto exit;
> -
>  	fmt = to_dpu_format(msm_framebuffer_format(state->fb));
> 
>  	min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
> @@ -1156,16 +1150,8 @@ static int dpu_plane_sspp_atomic_check(struct
> drm_plane *plane,
>  		DPU_ERROR_PLANE(pdpu, "invalid src " DRM_RECT_FMT "
> line:%u\n",
>  				DRM_RECT_ARG(&src), max_linewidth);
>  		ret = -E2BIG;
> -
> -	/* check scaler capability */
> -	} else if (hscale < 0 || vscale < 0) {
> -		DPU_ERROR_PLANE(pdpu, "invalid scaling requested src="
> -				DRM_RECT_FMT " dst=" DRM_RECT_FMT "\n",
> -				DRM_RECT_ARG(&src), DRM_RECT_ARG(&dst));
> -		ret = -E2BIG;
>  	}
> 
> -exit:
>  	return ret;
>  }
> 
> @@ -1237,7 +1223,6 @@ static int dpu_plane_sspp_atomic_update(struct
> drm_plane *plane,
>  	const struct dpu_format *fmt;
>  	struct drm_crtc *crtc;
>  	struct drm_framebuffer *fb;
> -	int ret, min_scale;
> 
>  	if (!plane) {
>  		DPU_ERROR("invalid plane\n");
> @@ -1276,15 +1261,6 @@ static int dpu_plane_sspp_atomic_update(struct
> drm_plane *plane,
>  	pdpu->is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT);
>  	_dpu_plane_set_qos_ctrl(plane, false, DPU_PLANE_QOS_PANIC_CTRL);
> 
> -	min_scale = FRAC_16_16(1, pdpu->pipe_sblk->maxdwnscale);
> -	ret = drm_atomic_helper_check_plane_state(state, crtc->state,
> min_scale,
> -					  pdpu->pipe_sblk->maxupscale <<
> 16,
> -					  true, false);
> -	if (ret) {
> -		DPU_ERROR_PLANE(pdpu, "Check plane state failed (%d)\n",
> ret);
> -		return ret;
> -	}
> -
>  	DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u "
> DRM_RECT_FMT
>  			", %4.4s ubwc %d\n", fb->base.id,
> DRM_RECT_FP_ARG(&state->src),
>  			crtc->base.id, DRM_RECT_ARG(&state->dst),

-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 3/7] drm/msm: dpu: Consolodate atomic_check functions()

[Jeykumar Sankaran]

On 2018-09-11 13:15, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> dpu_plane_atomic_check() is a very thin wrapper around
> dpu_plane_sspp_atomic_check(). All it does is a NULL-check of 
> state->fb,
> which is already done by drm_atomic_helper_check_plane_state(). 
> Further,
> the helper sets state->visible = false when this is true. So remove
> dpu_plane_atomic_check() and just use dpu_plane_sspp_atomic_check()
> directly.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 853902ac4fb9..7e894c8f69c3 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -1076,8 +1076,8 @@ static bool dpu_plane_validate_src(struct 
> drm_rect
> *src,
>  		drm_rect_equals(fb_rect, src);
>  }
> 
> -static int dpu_plane_sspp_atomic_check(struct drm_plane *plane,
> -		struct drm_plane_state *state)
> +static int dpu_plane_atomic_check(struct drm_plane *plane,
> +				  struct drm_plane_state *state)
>  {
>  	int ret = 0, min_scale;
>  	struct dpu_plane *pdpu = to_dpu_plane(plane);
> @@ -1155,17 +1155,6 @@ static int dpu_plane_sspp_atomic_check(struct
> drm_plane *plane,
>  	return ret;
>  }
> 
> -static int dpu_plane_atomic_check(struct drm_plane *plane,
> -		struct drm_plane_state *state)
> -{
> -	if (!state->fb)
> -		return 0;
> -
> -	DPU_DEBUG_PLANE(to_dpu_plane(plane), "\n");
> -
> -	return dpu_plane_sspp_atomic_check(plane, state);
> -}
> -
>  void dpu_plane_flush(struct drm_plane *plane)
>  {
>  	struct dpu_plane *pdpu;

Subject line typo:
%s/Consolodate/Consolidate

Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.org>

-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 2/7] drm/msm: dpu: Move atomic_check_plane_state() call to atomic_check

[Sean Paul]

On Tue, Sep 11, 2018 at 03:44:48PM -0700, Jeykumar Sankaran wrote:
> On 2018-09-11 13:15, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > src/dst rects are checked in both atomic_check and atomic_update, with
> > the more comprehensive check occurring in atomic_update, which is
> > backwards. So consolodate the checks in atomic_check.
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 56 +++++++----------------
> >  1 file changed, 16 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > index 0f428f66b951..853902ac4fb9 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > @@ -1079,13 +1079,26 @@ static bool dpu_plane_validate_src(struct
> > drm_rect
> > *src,
> >  static int dpu_plane_sspp_atomic_check(struct drm_plane *plane,
> >  		struct drm_plane_state *state)
> >  {
> > -	int ret = 0;
> > +	int ret = 0, min_scale;
> >  	struct dpu_plane *pdpu = to_dpu_plane(plane);
> > +	const struct drm_crtc_state *crtc_state = NULL;
> >  	const struct dpu_format *fmt;
> >  	struct drm_rect src, dst, fb_rect = { 0 };
> > -	uint32_t max_upscale = 1, max_downscale = 1;
> >  	uint32_t min_src_size, max_linewidth;
> > -	int hscale = 1, vscale = 1;
> > +
> > +	if (state->crtc)
> > +		crtc_state = state->crtc->state;
> Isn't the crtc_state retrieved here the old crtc state? Since the plane
> check is called before swap, the CRTC state
> for this commit should be retrieved from global atomic state state->state
> using drm_atomic_get_existing_crtc_state.

Yep, nice catch. I'll send a v2 using drm_atomic_get_new_crtc_state.

Sean

> 
> Jeykumar S.
> > +
> > +	min_scale = FRAC_16_16(1, pdpu->pipe_sblk->maxdwnscale);
> > +	ret = drm_atomic_helper_check_plane_state(state, crtc_state,
> > min_scale,
> > +					  pdpu->pipe_sblk->maxupscale <<
> > 16,
> > +					  true, true);
> > +	if (ret) {
> > +		DPU_ERROR_PLANE(pdpu, "Check plane state failed (%d)\n",
> > ret);
> > +		return ret;
> > +	}
> > +	if (!state->visible)
> > +		return 0;
> > 
> >  	src.x1 = state->src_x >> 16;
> >  	src.y1 = state->src_y >> 16;
> > @@ -1099,25 +1112,6 @@ static int dpu_plane_sspp_atomic_check(struct
> > drm_plane *plane,
> > 
> >  	max_linewidth = pdpu->pipe_sblk->common->maxlinewidth;
> > 
> > -	if (pdpu->features & DPU_SSPP_SCALER) {
> > -		max_downscale = pdpu->pipe_sblk->maxdwnscale;
> > -		max_upscale = pdpu->pipe_sblk->maxupscale;
> > -	}
> > -	if (drm_rect_width(&src) < drm_rect_width(&dst))
> > -		hscale = drm_rect_calc_hscale(&src, &dst, 1, max_upscale);
> > -	else
> > -		hscale = drm_rect_calc_hscale(&dst, &src, 1,
> > max_downscale);
> > -	if (drm_rect_height(&src) < drm_rect_height(&dst))
> > -		vscale = drm_rect_calc_vscale(&src, &dst, 1, max_upscale);
> > -	else
> > -		vscale = drm_rect_calc_vscale(&dst, &src, 1,
> > max_downscale);
> > -
> > -	DPU_DEBUG_PLANE(pdpu, "check %d -> %d\n",
> > -		dpu_plane_enabled(plane->state),
> > dpu_plane_enabled(state));
> > -
> > -	if (!dpu_plane_enabled(state))
> > -		goto exit;
> > -
> >  	fmt = to_dpu_format(msm_framebuffer_format(state->fb));
> > 
> >  	min_src_size = DPU_FORMAT_IS_YUV(fmt) ? 2 : 1;
> > @@ -1156,16 +1150,8 @@ static int dpu_plane_sspp_atomic_check(struct
> > drm_plane *plane,
> >  		DPU_ERROR_PLANE(pdpu, "invalid src " DRM_RECT_FMT "
> > line:%u\n",
> >  				DRM_RECT_ARG(&src), max_linewidth);
> >  		ret = -E2BIG;
> > -
> > -	/* check scaler capability */
> > -	} else if (hscale < 0 || vscale < 0) {
> > -		DPU_ERROR_PLANE(pdpu, "invalid scaling requested src="
> > -				DRM_RECT_FMT " dst=" DRM_RECT_FMT "\n",
> > -				DRM_RECT_ARG(&src), DRM_RECT_ARG(&dst));
> > -		ret = -E2BIG;
> >  	}
> > 
> > -exit:
> >  	return ret;
> >  }
> > 
> > @@ -1237,7 +1223,6 @@ static int dpu_plane_sspp_atomic_update(struct
> > drm_plane *plane,
> >  	const struct dpu_format *fmt;
> >  	struct drm_crtc *crtc;
> >  	struct drm_framebuffer *fb;
> > -	int ret, min_scale;
> > 
> >  	if (!plane) {
> >  		DPU_ERROR("invalid plane\n");
> > @@ -1276,15 +1261,6 @@ static int dpu_plane_sspp_atomic_update(struct
> > drm_plane *plane,
> >  	pdpu->is_rt_pipe = (dpu_crtc_get_client_type(crtc) != NRT_CLIENT);
> >  	_dpu_plane_set_qos_ctrl(plane, false, DPU_PLANE_QOS_PANIC_CTRL);
> > 
> > -	min_scale = FRAC_16_16(1, pdpu->pipe_sblk->maxdwnscale);
> > -	ret = drm_atomic_helper_check_plane_state(state, crtc->state,
> > min_scale,
> > -					  pdpu->pipe_sblk->maxupscale <<
> > 16,
> > -					  true, false);
> > -	if (ret) {
> > -		DPU_ERROR_PLANE(pdpu, "Check plane state failed (%d)\n",
> > ret);
> > -		return ret;
> > -	}
> > -
> >  	DPU_DEBUG_PLANE(pdpu, "FB[%u] " DRM_RECT_FP_FMT "->crtc%u "
> > DRM_RECT_FMT
> >  			", %4.4s ubwc %d\n", fb->base.id,
> > DRM_RECT_FP_ARG(&state->src),
> >  			crtc->base.id, DRM_RECT_ARG(&state->dst),
> 
> -- 
> Jeykumar S

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno