[PATCH v2 0/7] drm/msm/dpu: Clean up dpu code

[PATCH v2 0/7] drm/msm/dpu: Clean up dpu code

[Bruce Wang]

Removes uneeded checks and unused variables. Changes some
function that do not need return values to void.

Bruce Wang (7):
  drm/msm/dpu: Remove unneeded checks in dpu_plane.c
  drm/msm/dpu: Clean up plane atomic disable/update
  drm/msm/dpu: Remove unneeded checks in dpu_crtc.c
  drm/msm/dpu: Change _dpu_crtc_power_enable to void
  drm/msm/dpu: Change _dpu_crtc_vblank_enable_no_lock to void
  drm/msm/dpu: Make dpu_plane_danger_signal_ctrl void
  drm/msm/dpu: Make _dpu_plane_get_aspace void

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  | 247 +++-----------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 273 +++-------------------
 2 files changed, 68 insertions(+), 452 deletions(-)

-- 
2.19.0.444.g18242da7ef-goog

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

[PATCH v2 1/7] drm/msm/dpu: Remove unneeded checks in dpu_plane.c

[Bruce Wang]

Removes some checks from dpu_plane.c that will never result in an error.
Subsequent variable assignments become part of the initialization wherever
possible. Unused variables are removed.

Signed-off-by: Bruce Wang <bzwang@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 190 ++--------------------
 1 file changed, 17 insertions(+), 173 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 1ce76460d710..99887c804e4e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -123,13 +123,8 @@ struct dpu_plane {
 
 static struct dpu_kms *_dpu_plane_get_kms(struct drm_plane *plane)
 {
-	struct msm_drm_private *priv;
+	struct msm_drm_private *priv = plane->dev->dev_private;
 
-	if (!plane || !plane->dev)
-		return NULL;
-	priv = plane->dev->dev_private;
-	if (!priv)
-		return NULL;
 	return to_dpu_kms(priv->kms);
 }
 
@@ -148,7 +143,7 @@ static inline int _dpu_plane_calc_fill_level(struct drm_plane *plane,
 	u32 fixed_buff_size;
 	u32 total_fl;
 
-	if (!plane || !fmt || !plane->state || !src_width || !fmt->bpp) {
+	if (!fmt || !plane->state || !src_width || !fmt->bpp) {
 		DPU_ERROR("invalid arguments\n");
 		return 0;
 	}
@@ -229,26 +224,11 @@ static u64 _dpu_plane_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
 static void _dpu_plane_set_qos_lut(struct drm_plane *plane,
 		struct drm_framebuffer *fb)
 {
-	struct dpu_plane *pdpu;
+	struct dpu_plane *pdpu = to_dpu_plane(plane);
 	const struct dpu_format *fmt = NULL;
 	u64 qos_lut;
 	u32 total_fl = 0, lut_usage;
 
-	if (!plane || !fb) {
-		DPU_ERROR("invalid arguments plane %d fb %d\n",
-				plane != 0, fb != 0);
-		return;
-	}
-
-	pdpu = to_dpu_plane(plane);
-
-	if (!pdpu->pipe_hw || !pdpu->pipe_sblk || !pdpu->catalog) {
-		DPU_ERROR("invalid arguments\n");
-		return;
-	} else if (!pdpu->pipe_hw->ops.setup_creq_lut) {
-		return;
-	}
-
 	if (!pdpu->is_rt_pipe) {
 		lut_usage = DPU_QOS_LUT_USAGE_NRT;
 	} else {
@@ -290,24 +270,10 @@ static void _dpu_plane_set_qos_lut(struct drm_plane *plane,
 static void _dpu_plane_set_danger_lut(struct drm_plane *plane,
 		struct drm_framebuffer *fb)
 {
-	struct dpu_plane *pdpu;
+	struct dpu_plane *pdpu = to_dpu_plane(plane);
 	const struct dpu_format *fmt = NULL;
 	u32 danger_lut, safe_lut;
 
-	if (!plane || !fb) {
-		DPU_ERROR("invalid arguments\n");
-		return;
-	}
-
-	pdpu = to_dpu_plane(plane);
-
-	if (!pdpu->pipe_hw || !pdpu->pipe_sblk || !pdpu->catalog) {
-		DPU_ERROR("invalid arguments\n");
-		return;
-	} else if (!pdpu->pipe_hw->ops.setup_danger_safe_lut) {
-		return;
-	}
-
 	if (!pdpu->is_rt_pipe) {
 		danger_lut = pdpu->catalog->perf.danger_lut_tbl
 				[DPU_QOS_LUT_USAGE_NRT];
@@ -361,21 +327,7 @@ static void _dpu_plane_set_danger_lut(struct drm_plane *plane,
 static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
 	bool enable, u32 flags)
 {
-	struct dpu_plane *pdpu;
-
-	if (!plane) {
-		DPU_ERROR("invalid arguments\n");
-		return;
-	}
-
-	pdpu = to_dpu_plane(plane);
-
-	if (!pdpu->pipe_hw || !pdpu->pipe_sblk) {
-		DPU_ERROR("invalid arguments\n");
-		return;
-	} else if (!pdpu->pipe_hw->ops.setup_qos_ctrl) {
-		return;
-	}
+	struct dpu_plane *pdpu = to_dpu_plane(plane);
 
 	if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
 		pdpu->pipe_qos_cfg.creq_vblank = pdpu->pipe_sblk->creq_vblank;
@@ -450,29 +402,10 @@ int dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
 static void _dpu_plane_set_ot_limit(struct drm_plane *plane,
 		struct drm_crtc *crtc)
 {
-	struct dpu_plane *pdpu;
+	struct dpu_plane *pdpu = to_dpu_plane(plane);
 	struct dpu_vbif_set_ot_params ot_params;
-	struct msm_drm_private *priv;
-	struct dpu_kms *dpu_kms;
-
-	if (!plane || !plane->dev || !crtc) {
-		DPU_ERROR("invalid arguments plane %d crtc %d\n",
-				plane != 0, crtc != 0);
-		return;
-	}
-
-	priv = plane->dev->dev_private;
-	if (!priv || !priv->kms) {
-		DPU_ERROR("invalid KMS reference\n");
-		return;
-	}
-
-	dpu_kms = to_dpu_kms(priv->kms);
-	pdpu = to_dpu_plane(plane);
-	if (!pdpu->pipe_hw) {
-		DPU_ERROR("invalid pipe reference\n");
-		return;
-	}
+	struct msm_drm_private *priv = plane->dev->dev_private;
+	struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
 
 	memset(&ot_params, 0, sizeof(ot_params));
 	ot_params.xin_id = pdpu->pipe_hw->cap->xin_id;
@@ -494,28 +427,10 @@ static void _dpu_plane_set_ot_limit(struct drm_plane *plane,
  */
 static void _dpu_plane_set_qos_remap(struct drm_plane *plane)
 {
-	struct dpu_plane *pdpu;
+	struct dpu_plane *pdpu = to_dpu_plane(plane);
 	struct dpu_vbif_set_qos_params qos_params;
-	struct msm_drm_private *priv;
-	struct dpu_kms *dpu_kms;
-
-	if (!plane || !plane->dev) {
-		DPU_ERROR("invalid arguments\n");
-		return;
-	}
-
-	priv = plane->dev->dev_private;
-	if (!priv || !priv->kms) {
-		DPU_ERROR("invalid KMS reference\n");
-		return;
-	}
-
-	dpu_kms = to_dpu_kms(priv->kms);
-	pdpu = to_dpu_plane(plane);
-	if (!pdpu->pipe_hw) {
-		DPU_ERROR("invalid pipe reference\n");
-		return;
-	}
+	struct msm_drm_private *priv = plane->dev->dev_private;
+	struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
 
 	memset(&qos_params, 0, sizeof(qos_params));
 	qos_params.vbif_idx = VBIF_RT;
@@ -549,10 +464,6 @@ static int _dpu_plane_get_aspace(
 	}
 
 	kms = _dpu_plane_get_kms(&pdpu->base);
-	if (!kms) {
-		DPU_ERROR("invalid kms\n");
-		return -EINVAL;
-	}
 
 	*aspace = kms->base.aspace;
 
@@ -576,10 +487,6 @@ static inline void _dpu_plane_set_scanout(struct drm_plane *plane,
 	}
 
 	pdpu = to_dpu_plane(plane);
-	if (!pdpu->pipe_hw) {
-		DPU_ERROR_PLANE(pdpu, "invalid pipe_hw\n");
-		return;
-	}
 
 	ret = _dpu_plane_get_aspace(pdpu, pstate, &aspace);
 	if (ret) {
@@ -610,15 +517,6 @@ static void _dpu_plane_setup_scaler3(struct dpu_plane *pdpu,
 {
 	uint32_t i;
 
-	if (!pdpu || !pstate || !scale_cfg || !fmt || !chroma_subsmpl_h ||
-			!chroma_subsmpl_v) {
-		DPU_ERROR(
-			"pdpu %d pstate %d scale_cfg %d fmt %d smp_h %d smp_v %d\n",
-			!!pdpu, !!pstate, !!scale_cfg, !!fmt, chroma_subsmpl_h,
-			chroma_subsmpl_v);
-		return;
-	}
-
 	memset(scale_cfg, 0, sizeof(*scale_cfg));
 	memset(&pstate->pixel_ext, 0, sizeof(struct dpu_hw_pixel_ext));
 
@@ -722,17 +620,8 @@ static void _dpu_plane_setup_scaler(struct dpu_plane *pdpu,
 		struct dpu_plane_state *pstate,
 		const struct dpu_format *fmt, bool color_fill)
 {
-	struct dpu_hw_pixel_ext *pe;
 	uint32_t chroma_subsmpl_h, chroma_subsmpl_v;
 
-	if (!pdpu || !fmt || !pstate) {
-		DPU_ERROR("invalid arg(s), plane %d fmt %d state %d\n",
-				pdpu != 0, fmt != 0, pstate != 0);
-		return;
-	}
-
-	pe = &pstate->pixel_ext;
-
 	/* don't chroma subsample if decimating */
 	chroma_subsmpl_h =
 		drm_format_horz_chroma_subsampling(fmt->base.pixel_format);
@@ -760,21 +649,8 @@ static int _dpu_plane_color_fill(struct dpu_plane *pdpu,
 		uint32_t color, uint32_t alpha)
 {
 	const struct dpu_format *fmt;
-	const struct drm_plane *plane;
-	struct dpu_plane_state *pstate;
-
-	if (!pdpu || !pdpu->base.state) {
-		DPU_ERROR("invalid plane\n");
-		return -EINVAL;
-	}
-
-	if (!pdpu->pipe_hw) {
-		DPU_ERROR_PLANE(pdpu, "invalid plane h/w pointer\n");
-		return -EINVAL;
-	}
-
-	plane = &pdpu->base;
-	pstate = to_dpu_plane_state(plane->state);
+	const struct drm_plane *plane = &pdpu->base;
+	struct dpu_plane_state *pstate = to_dpu_plane_state(plane->state);
 
 	DPU_DEBUG_PLANE(pdpu, "\n");
 
@@ -825,12 +701,7 @@ static int _dpu_plane_color_fill(struct dpu_plane *pdpu,
 
 void dpu_plane_clear_multirect(const struct drm_plane_state *drm_state)
 {
-	struct dpu_plane_state *pstate;
-
-	if (!drm_state)
-		return;
-
-	pstate = to_dpu_plane_state(drm_state);
+	struct dpu_plane_state *pstate = to_dpu_plane_state(drm_state);
 
 	pstate->multirect_index = DPU_SSPP_RECT_SOLO;
 	pstate->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
@@ -961,15 +832,6 @@ int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane)
 void dpu_plane_get_ctl_flush(struct drm_plane *plane, struct dpu_hw_ctl *ctl,
 		u32 *flush_sspp)
 {
-	struct dpu_plane_state *pstate;
-
-	if (!plane || !flush_sspp) {
-		DPU_ERROR("invalid parameters\n");
-		return;
-	}
-
-	pstate = to_dpu_plane_state(plane->state);
-
 	*flush_sspp = ctl->ops.get_bitmask_sspp(ctl, dpu_plane_pipe(plane));
 }
 
@@ -1389,8 +1251,7 @@ static void dpu_plane_destroy(struct drm_plane *plane)
 		/* this will destroy the states as well */
 		drm_plane_cleanup(plane);
 
-		if (pdpu->pipe_hw)
-			dpu_hw_sspp_destroy(pdpu->pipe_hw);
+		dpu_hw_sspp_destroy(pdpu->pipe_hw);
 
 		kfree(pdpu);
 	}
@@ -1737,28 +1598,11 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
 	struct drm_plane *plane = NULL, *master_plane = NULL;
 	const struct dpu_format_extended *format_list;
 	struct dpu_plane *pdpu;
-	struct msm_drm_private *priv;
-	struct dpu_kms *kms;
+	struct msm_drm_private *priv = dev->dev_private;
+	struct dpu_kms *kms = to_dpu_kms(priv->kms);
 	int zpos_max = DPU_ZPOS_MAX;
 	int ret = -EINVAL;
 
-	if (!dev) {
-		DPU_ERROR("[%u]device is NULL\n", pipe);
-		goto exit;
-	}
-
-	priv = dev->dev_private;
-	if (!priv) {
-		DPU_ERROR("[%u]private data is NULL\n", pipe);
-		goto exit;
-	}
-
-	if (!priv->kms) {
-		DPU_ERROR("[%u]invalid KMS reference\n", pipe);
-		goto exit;
-	}
-	kms = to_dpu_kms(priv->kms);
-
 	if (!kms->catalog) {
 		DPU_ERROR("[%u]invalid catalog reference\n", pipe);
 		goto exit;
-- 
2.19.0.444.g18242da7ef-goog

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

[PATCH v2 2/7] drm/msm/dpu: Clean up plane atomic disable/update

[Bruce Wang]

Removes unnecessary checks from dpu_plane_atomic_disable, old_state
argument for both dpu_plane_atomic_disable and
dpu_plane_sspp_atomic_update is removed as it is no longer used.

Signed-off-by: Bruce Wang <bzwang@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 31 +++++------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 99887c804e4e..9a5d5afa53f2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1054,8 +1054,7 @@ void dpu_plane_set_error(struct drm_plane *plane, bool error)
 	pdpu->is_error = error;
 }
 
-static void 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)
 {
 	uint32_t src_flags;
 	struct dpu_plane *pdpu = to_dpu_plane(plane);
@@ -1168,27 +1167,11 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane,
 	_dpu_plane_set_qos_remap(plane);
 }
 
-static void _dpu_plane_atomic_disable(struct drm_plane *plane,
-				struct drm_plane_state *old_state)
+static void _dpu_plane_atomic_disable(struct drm_plane *plane)
 {
-	struct dpu_plane *pdpu;
-	struct drm_plane_state *state;
-	struct dpu_plane_state *pstate;
-
-	if (!plane) {
-		DPU_ERROR("invalid plane\n");
-		return;
-	} else if (!plane->state) {
-		DPU_ERROR("invalid plane state\n");
-		return;
-	} else if (!old_state) {
-		DPU_ERROR("invalid old state\n");
-		return;
-	}
-
-	pdpu = to_dpu_plane(plane);
-	state = plane->state;
-	pstate = to_dpu_plane_state(state);
+	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);
 
 	trace_dpu_plane_disable(DRMID(plane), is_dpu_plane_virtual(plane),
 				pstate->multirect_mode);
@@ -1212,9 +1195,9 @@ static void dpu_plane_atomic_update(struct drm_plane *plane,
 	DPU_DEBUG_PLANE(pdpu, "\n");
 
 	if (!state->visible) {
-		_dpu_plane_atomic_disable(plane, old_state);
+		_dpu_plane_atomic_disable(plane);
 	} else {
-		dpu_plane_sspp_atomic_update(plane, old_state);
+		dpu_plane_sspp_atomic_update(plane);
 	}
 }
 
-- 
2.19.0.444.g18242da7ef-goog

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

[PATCH v2 3/7] drm/msm/dpu: Remove unneeded checks in dpu_crtc.c

[Bruce Wang]

Removes impossible checks in dpu_crtc.c.
Variable assignments are moved up to be initializations where
possible. Some variables are no longer used, these are removed.

Signed-off-by: Bruce Wang <bzwang@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 170 +++--------------------
 1 file changed, 23 insertions(+), 147 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index a8f2dd7a37c7..a9adb16eac6f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -55,19 +55,8 @@ static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc_state *cstate,
 
 static inline struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc)
 {
-	struct msm_drm_private *priv;
-
-	if (!crtc || !crtc->dev || !crtc->dev->dev_private) {
-		DPU_ERROR("invalid crtc\n");
-		return NULL;
-	}
-	priv = crtc->dev->dev_private;
-	if (!priv || !priv->kms) {
-		DPU_ERROR("invalid kms\n");
-		return NULL;
-	}
-
-	return to_dpu_kms(priv->kms);
+	return to_dpu_kms(((struct msm_drm_private *) crtc->dev->dev_private)
+			->kms);
 }
 
 static inline int _dpu_crtc_power_enable(struct dpu_crtc *dpu_crtc, bool enable)
@@ -177,28 +166,17 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
 	struct drm_plane *plane;
 	struct drm_framebuffer *fb;
 	struct drm_plane_state *state;
-	struct dpu_crtc_state *cstate;
+	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
 	struct dpu_plane_state *pstate = NULL;
 	struct dpu_format *format;
-	struct dpu_hw_ctl *ctl;
-	struct dpu_hw_mixer *lm;
-	struct dpu_hw_stage_cfg *stage_cfg;
+	struct dpu_hw_ctl *ctl = mixer->lm_ctl;
+	struct dpu_hw_stage_cfg *stage_cfg = &dpu_crtc->stage_cfg;
 
 	u32 flush_mask;
 	uint32_t stage_idx, lm_idx;
 	int zpos_cnt[DPU_STAGE_MAX + 1] = { 0 };
 	bool bg_alpha_enable = false;
 
-	if (!dpu_crtc || !mixer) {
-		DPU_ERROR("invalid dpu_crtc or mixer\n");
-		return;
-	}
-
-	ctl = mixer->lm_ctl;
-	lm = mixer->hw_lm;
-	stage_cfg = &dpu_crtc->stage_cfg;
-	cstate = to_dpu_crtc_state(crtc->state);
-
 	drm_atomic_crtc_for_each_plane(plane, crtc) {
 		state = plane->state;
 		if (!state)
@@ -217,10 +195,6 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
 				state->fb ? state->fb->base.id : -1);
 
 		format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
-		if (!format) {
-			DPU_ERROR("invalid format\n");
-			return;
-		}
 
 		if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
 			bg_alpha_enable = true;
@@ -261,21 +235,13 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
  */
 static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
 {
-	struct dpu_crtc *dpu_crtc;
-	struct dpu_crtc_state *cstate;
-	struct dpu_crtc_mixer *mixer;
+	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
+	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
+	struct dpu_crtc_mixer *mixer = cstate->mixers;
 	struct dpu_hw_ctl *ctl;
 	struct dpu_hw_mixer *lm;
-
 	int i;
 
-	if (!crtc)
-		return;
-
-	dpu_crtc = to_dpu_crtc(crtc);
-	cstate = to_dpu_crtc_state(crtc->state);
-	mixer = cstate->mixers;
-
 	DPU_DEBUG("%s\n", dpu_crtc->name);
 
 	for (i = 0; i < cstate->num_mixers; i++) {
@@ -377,34 +343,13 @@ static void dpu_crtc_vblank_cb(void *data)
 
 static void dpu_crtc_frame_event_work(struct kthread_work *work)
 {
-	struct msm_drm_private *priv;
-	struct dpu_crtc_frame_event *fevent;
-	struct drm_crtc *crtc;
-	struct dpu_crtc *dpu_crtc;
-	struct dpu_kms *dpu_kms;
+	struct dpu_crtc_frame_event *fevent = container_of(work,
+			struct dpu_crtc_frame_event, work);
+	struct drm_crtc *crtc = fevent->crtc;
+	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
 	unsigned long flags;
 	bool frame_done = false;
 
-	if (!work) {
-		DPU_ERROR("invalid work handle\n");
-		return;
-	}
-
-	fevent = container_of(work, struct dpu_crtc_frame_event, work);
-	if (!fevent->crtc || !fevent->crtc->state) {
-		DPU_ERROR("invalid crtc\n");
-		return;
-	}
-
-	crtc = fevent->crtc;
-	dpu_crtc = to_dpu_crtc(crtc);
-
-	dpu_kms = _dpu_crtc_get_kms(crtc);
-	if (!dpu_kms) {
-		DPU_ERROR("invalid kms handle\n");
-		return;
-	}
-	priv = dpu_kms->dev->dev_private;
 	DPU_ATRACE_BEGIN("crtc_frame_event");
 
 	DRM_DEBUG_KMS("crtc%d event:%u ts:%lld\n", crtc->base.id, fevent->event,
@@ -470,11 +415,6 @@ static void dpu_crtc_frame_event_cb(void *data, u32 event)
 	unsigned long flags;
 	u32 crtc_id;
 
-	if (!crtc || !crtc->dev || !crtc->dev->dev_private) {
-		DPU_ERROR("invalid parameters\n");
-		return;
-	}
-
 	/* Nothing to do on idle event */
 	if (event & DPU_ENCODER_FRAME_EVENT_IDLE)
 		return;
@@ -583,23 +523,12 @@ static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
 static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,
 		struct drm_crtc_state *state)
 {
-	struct dpu_crtc *dpu_crtc;
-	struct dpu_crtc_state *cstate;
-	struct drm_display_mode *adj_mode;
-	u32 crtc_split_width;
+	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
+	struct dpu_crtc_state *cstate = to_dpu_crtc_state(state);
+	struct drm_display_mode *adj_mode = &state->adjusted_mode;
+	u32 crtc_split_width = _dpu_crtc_get_mixer_width(cstate, adj_mode);
 	int i;
 
-	if (!crtc || !state) {
-		DPU_ERROR("invalid args\n");
-		return;
-	}
-
-	dpu_crtc = to_dpu_crtc(crtc);
-	cstate = to_dpu_crtc_state(state);
-
-	adj_mode = &state->adjusted_mode;
-	crtc_split_width = _dpu_crtc_get_mixer_width(cstate, adj_mode);
-
 	for (i = 0; i < cstate->num_mixers; i++) {
 		struct drm_rect *r = &cstate->lm_bounds[i];
 		r->x1 = crtc_split_width * i;
@@ -693,11 +622,6 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc,
 	unsigned long flags;
 	struct dpu_crtc_state *cstate;
 
-	if (!crtc || !crtc->dev || !crtc->dev->dev_private) {
-		DPU_ERROR("invalid crtc\n");
-		return;
-	}
-
 	if (!crtc->state->enable) {
 		DPU_DEBUG("crtc%d -> enable %d, skip atomic_flush\n",
 				crtc->base.id, crtc->state->enable);
@@ -790,15 +714,9 @@ static void dpu_crtc_destroy_state(struct drm_crtc *crtc,
 
 static int _dpu_crtc_wait_for_frame_done(struct drm_crtc *crtc)
 {
-	struct dpu_crtc *dpu_crtc;
+	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
 	int ret, rc = 0;
 
-	if (!crtc) {
-		DPU_ERROR("invalid argument\n");
-		return -EINVAL;
-	}
-	dpu_crtc = to_dpu_crtc(crtc);
-
 	if (!atomic_read(&dpu_crtc->frame_pending)) {
 		DPU_DEBUG("no frames pending\n");
 		return 0;
@@ -819,29 +737,12 @@ static int _dpu_crtc_wait_for_frame_done(struct drm_crtc *crtc)
 void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 {
 	struct drm_encoder *encoder;
-	struct drm_device *dev;
-	struct dpu_crtc *dpu_crtc;
-	struct msm_drm_private *priv;
-	struct dpu_kms *dpu_kms;
-	struct dpu_crtc_state *cstate;
+	struct drm_device *dev = crtc->dev;
+	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
+	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
+	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
 	int ret;
 
-	if (!crtc) {
-		DPU_ERROR("invalid argument\n");
-		return;
-	}
-	dev = crtc->dev;
-	dpu_crtc = to_dpu_crtc(crtc);
-	dpu_kms = _dpu_crtc_get_kms(crtc);
-
-	if (!dpu_kms || !dpu_kms->dev || !dpu_kms->dev->dev_private) {
-		DPU_ERROR("invalid argument\n");
-		return;
-	}
-
-	priv = dpu_kms->dev->dev_private;
-	cstate = to_dpu_crtc_state(crtc->state);
-
 	/*
 	 * If no mixers has been allocated in dpu_crtc_atomic_check(),
 	 * it means we are trying to start a CRTC whose state is disabled:
@@ -969,24 +870,9 @@ static int _dpu_crtc_vblank_enable_no_lock(
  */
 static void _dpu_crtc_set_suspend(struct drm_crtc *crtc, bool enable)
 {
-	struct dpu_crtc *dpu_crtc;
-	struct msm_drm_private *priv;
-	struct dpu_kms *dpu_kms;
+	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
 	int ret = 0;
 
-	if (!crtc || !crtc->dev || !crtc->dev->dev_private) {
-		DPU_ERROR("invalid crtc\n");
-		return;
-	}
-	dpu_crtc = to_dpu_crtc(crtc);
-	priv = crtc->dev->dev_private;
-
-	if (!priv->kms) {
-		DPU_ERROR("invalid crtc kms\n");
-		return;
-	}
-	dpu_kms = to_dpu_kms(priv->kms);
-
 	DRM_DEBUG_KMS("crtc%d suspend = %d\n", crtc->base.id, enable);
 
 	mutex_lock(&dpu_crtc->crtc_lock);
@@ -1079,16 +965,8 @@ static void dpu_crtc_reset(struct drm_crtc *crtc)
 static void dpu_crtc_handle_power_event(u32 event_type, void *arg)
 {
 	struct drm_crtc *crtc = arg;
-	struct dpu_crtc *dpu_crtc;
+	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
 	struct drm_encoder *encoder;
-	struct dpu_crtc_state *cstate;
-
-	if (!crtc) {
-		DPU_ERROR("invalid crtc\n");
-		return;
-	}
-	dpu_crtc = to_dpu_crtc(crtc);
-	cstate = to_dpu_crtc_state(dpu_crtc->base.state);
 
 	mutex_lock(&dpu_crtc->crtc_lock);
 
@@ -1673,8 +1551,6 @@ static int _dpu_crtc_init_debugfs(struct drm_crtc *crtc)
 	dpu_crtc = to_dpu_crtc(crtc);
 
 	dpu_kms = _dpu_crtc_get_kms(crtc);
-	if (!dpu_kms)
-		return -EINVAL;
 
 	dpu_crtc->debugfs_root = debugfs_create_dir(dpu_crtc->name,
 			crtc->dev->primary->debugfs_root);
-- 
2.19.0.444.g18242da7ef-goog

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

[PATCH v2 4/7] drm/msm/dpu: Change _dpu_crtc_power_enable to void

[Bruce Wang]

All checks for _dpu_crtc_power_enable are not true, so the function
can never return an error code. All calls of the function have also
been changed so that they don't expect a return value.

Signed-off-by: Bruce Wang <bzwang@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 35 ++++--------------------
 1 file changed, 5 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index a9adb16eac6f..648d77c41c3e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -59,37 +59,16 @@ static inline struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc)
 			->kms);
 }
 
-static inline int _dpu_crtc_power_enable(struct dpu_crtc *dpu_crtc, bool enable)
+static inline void _dpu_crtc_power_enable(struct dpu_crtc *dpu_crtc, bool enable)
 {
-	struct drm_crtc *crtc;
-	struct msm_drm_private *priv;
-	struct dpu_kms *dpu_kms;
-
-	if (!dpu_crtc) {
-		DPU_ERROR("invalid dpu crtc\n");
-		return -EINVAL;
-	}
-
-	crtc = &dpu_crtc->base;
-	if (!crtc->dev || !crtc->dev->dev_private) {
-		DPU_ERROR("invalid drm device\n");
-		return -EINVAL;
-	}
-
-	priv = crtc->dev->dev_private;
-	if (!priv->kms) {
-		DPU_ERROR("invalid kms\n");
-		return -EINVAL;
-	}
-
-	dpu_kms = to_dpu_kms(priv->kms);
+	struct drm_crtc *crtc = &dpu_crtc->base;
+	struct msm_drm_private *priv = crtc->dev->dev_private;
+	struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
 
 	if (enable)
 		pm_runtime_get_sync(&dpu_kms->pdev->dev);
 	else
 		pm_runtime_put_sync(&dpu_kms->pdev->dev);
-
-	return 0;
 }
 
 static void dpu_crtc_destroy(struct drm_crtc *crtc)
@@ -822,14 +801,10 @@ static int _dpu_crtc_vblank_enable_no_lock(
 	dev = crtc->dev;
 
 	if (enable) {
-		int ret;
-
 		/* drop lock since power crtc cb may try to re-acquire lock */
 		mutex_unlock(&dpu_crtc->crtc_lock);
-		ret = _dpu_crtc_power_enable(dpu_crtc, true);
+		_dpu_crtc_power_enable(dpu_crtc, true);
 		mutex_lock(&dpu_crtc->crtc_lock);
-		if (ret)
-			return ret;
 
 		list_for_each_entry(enc, &dev->mode_config.encoder_list, head) {
 			if (enc->crtc != crtc)
-- 
2.19.0.444.g18242da7ef-goog

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

[PATCH v2 5/7] drm/msm/dpu: Change _dpu_crtc_vblank_enable_no_lock to void

[Bruce Wang]

Removes redundant tests for _dpu_crtc_vblank_enable_no_lock.
Function return type is now void and all function calls have
been changed accordingly.

Signed-off-by: Bruce Wang <bzwang@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 42 ++++--------------------
 1 file changed, 7 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 648d77c41c3e..0a2d52847655 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -782,24 +782,14 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
  * _dpu_crtc_vblank_enable_no_lock - update power resource and vblank request
  * @dpu_crtc: Pointer to dpu crtc structure
  * @enable: Whether to enable/disable vblanks
- *
- * @Return: error code
  */
-static int _dpu_crtc_vblank_enable_no_lock(
+static void _dpu_crtc_vblank_enable_no_lock(
 		struct dpu_crtc *dpu_crtc, bool enable)
 {
-	struct drm_device *dev;
-	struct drm_crtc *crtc;
+	struct drm_crtc *crtc = &dpu_crtc->base;
+	struct drm_device *dev = crtc->dev;
 	struct drm_encoder *enc;
 
-	if (!dpu_crtc) {
-		DPU_ERROR("invalid crtc\n");
-		return -EINVAL;
-	}
-
-	crtc = &dpu_crtc->base;
-	dev = crtc->dev;
-
 	if (enable) {
 		/* drop lock since power crtc cb may try to re-acquire lock */
 		mutex_unlock(&dpu_crtc->crtc_lock);
@@ -834,8 +824,6 @@ static int _dpu_crtc_vblank_enable_no_lock(
 		_dpu_crtc_power_enable(dpu_crtc, false);
 		mutex_lock(&dpu_crtc->crtc_lock);
 	}
-
-	return 0;
 }
 
 /**
@@ -846,7 +834,6 @@ static int _dpu_crtc_vblank_enable_no_lock(
 static void _dpu_crtc_set_suspend(struct drm_crtc *crtc, bool enable)
 {
 	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
-	int ret = 0;
 
 	DRM_DEBUG_KMS("crtc%d suspend = %d\n", crtc->base.id, enable);
 
@@ -861,10 +848,7 @@ static void _dpu_crtc_set_suspend(struct drm_crtc *crtc, bool enable)
 		DPU_DEBUG("crtc%d suspend already set to %d, ignoring update\n",
 				crtc->base.id, enable);
 	else if (dpu_crtc->enabled && dpu_crtc->vblank_requested) {
-		ret = _dpu_crtc_vblank_enable_no_lock(dpu_crtc, !enable);
-		if (ret)
-			DPU_ERROR("%s vblank enable failed: %d\n",
-					dpu_crtc->name, ret);
+		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, !enable);
 	}
 
 	dpu_crtc->suspend = enable;
@@ -965,7 +949,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
 	struct drm_display_mode *mode;
 	struct drm_encoder *encoder;
 	struct msm_drm_private *priv;
-	int ret;
 	unsigned long flags;
 
 	if (!crtc || !crtc->dev || !crtc->dev->dev_private || !crtc->state) {
@@ -996,10 +979,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
 	trace_dpu_crtc_disable(DRMID(crtc), false, dpu_crtc);
 	if (dpu_crtc->enabled && !dpu_crtc->suspend &&
 			dpu_crtc->vblank_requested) {
-		ret = _dpu_crtc_vblank_enable_no_lock(dpu_crtc, false);
-		if (ret)
-			DPU_ERROR("%s vblank enable failed: %d\n",
-					dpu_crtc->name, ret);
+		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, false);
 	}
 	dpu_crtc->enabled = false;
 
@@ -1045,7 +1025,6 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
 	struct dpu_crtc *dpu_crtc;
 	struct drm_encoder *encoder;
 	struct msm_drm_private *priv;
-	int ret;
 
 	if (!crtc || !crtc->dev || !crtc->dev->dev_private) {
 		DPU_ERROR("invalid crtc\n");
@@ -1067,10 +1046,7 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
 	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
 	if (!dpu_crtc->enabled && !dpu_crtc->suspend &&
 			dpu_crtc->vblank_requested) {
-		ret = _dpu_crtc_vblank_enable_no_lock(dpu_crtc, true);
-		if (ret)
-			DPU_ERROR("%s vblank enable failed: %d\n",
-					dpu_crtc->name, ret);
+		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, true);
 	}
 	dpu_crtc->enabled = true;
 
@@ -1325,7 +1301,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
 int dpu_crtc_vblank(struct drm_crtc *crtc, bool en)
 {
 	struct dpu_crtc *dpu_crtc;
-	int ret;
 
 	if (!crtc) {
 		DPU_ERROR("invalid crtc\n");
@@ -1336,10 +1311,7 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool en)
 	mutex_lock(&dpu_crtc->crtc_lock);
 	trace_dpu_crtc_vblank(DRMID(&dpu_crtc->base), en, dpu_crtc);
 	if (dpu_crtc->enabled && !dpu_crtc->suspend) {
-		ret = _dpu_crtc_vblank_enable_no_lock(dpu_crtc, en);
-		if (ret)
-			DPU_ERROR("%s vblank enable failed: %d\n",
-					dpu_crtc->name, ret);
+		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, en);
 	}
 	dpu_crtc->vblank_requested = en;
 	mutex_unlock(&dpu_crtc->crtc_lock);
-- 
2.19.0.444.g18242da7ef-goog

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

[PATCH v2 6/7] drm/msm/dpu: Make dpu_plane_danger_signal_ctrl void

[Bruce Wang]

Removed all impossible checks from the function, which eliminates
the need for a return value. This function is also never used
outside of dpu_plane.c, so the function is made static.

Signed-off-by: Bruce Wang <bzwang@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 27 +++++------------------
 1 file changed, 5 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 9a5d5afa53f2..f9e65acdf87e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -363,35 +363,18 @@ static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
 			&pdpu->pipe_qos_cfg);
 }
 
-int dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
+static void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
 {
-	struct dpu_plane *pdpu;
-	struct msm_drm_private *priv;
-	struct dpu_kms *dpu_kms;
-
-	if (!plane || !plane->dev) {
-		DPU_ERROR("invalid arguments\n");
-		return -EINVAL;
-	}
-
-	priv = plane->dev->dev_private;
-	if (!priv || !priv->kms) {
-		DPU_ERROR("invalid KMS reference\n");
-		return -EINVAL;
-	}
-
-	dpu_kms = to_dpu_kms(priv->kms);
-	pdpu = to_dpu_plane(plane);
+	struct dpu_plane *pdpu = to_dpu_plane(plane);
+	struct msm_drm_private *priv = plane->dev->dev_private;
+	struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
 
 	if (!pdpu->is_rt_pipe)
-		goto end;
+		return;
 
 	pm_runtime_get_sync(&dpu_kms->pdev->dev);
 	_dpu_plane_set_qos_ctrl(plane, enable, DPU_PLANE_QOS_PANIC_CTRL);
 	pm_runtime_put_sync(&dpu_kms->pdev->dev);
-
-end:
-	return 0;
 }
 
 /**
-- 
2.19.0.444.g18242da7ef-goog

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

[PATCH v2 7/7] drm/msm/dpu: Make _dpu_plane_get_aspace void

[Bruce Wang]

Remove unneeded checks from _dpu_plane_get_aspace. The function
no longer needs to return anything so it is changed to void.

Signed-off-by: Bruce Wang <bzwang@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 25 ++++-------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index f9e65acdf87e..59f019685658 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -434,23 +434,14 @@ static void _dpu_plane_set_qos_remap(struct drm_plane *plane)
 /**
  * _dpu_plane_get_aspace: gets the address space
  */
-static int _dpu_plane_get_aspace(
+static void _dpu_plane_get_aspace(
 		struct dpu_plane *pdpu,
 		struct dpu_plane_state *pstate,
 		struct msm_gem_address_space **aspace)
 {
-	struct dpu_kms *kms;
-
-	if (!pdpu || !pstate || !aspace) {
-		DPU_ERROR("invalid parameters\n");
-		return -EINVAL;
-	}
-
-	kms = _dpu_plane_get_kms(&pdpu->base);
+	struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
 
 	*aspace = kms->base.aspace;
-
-	return 0;
 }
 
 static inline void _dpu_plane_set_scanout(struct drm_plane *plane,
@@ -471,11 +462,7 @@ static inline void _dpu_plane_set_scanout(struct drm_plane *plane,
 
 	pdpu = to_dpu_plane(plane);
 
-	ret = _dpu_plane_get_aspace(pdpu, pstate, &aspace);
-	if (ret) {
-		DPU_ERROR_PLANE(pdpu, "Failed to get aspace %d\n", ret);
-		return;
-	}
+	_dpu_plane_get_aspace(pdpu, pstate, &aspace);
 
 	ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout);
 	if (ret == -EAGAIN)
@@ -836,11 +823,7 @@ static int dpu_plane_prepare_fb(struct drm_plane *plane,
 
 	DPU_DEBUG_PLANE(pdpu, "FB[%u]\n", fb->base.id);
 
-	ret = _dpu_plane_get_aspace(pdpu, pstate, &aspace);
-	if (ret) {
-		DPU_ERROR_PLANE(pdpu, "Failed to get aspace\n");
-		return ret;
-	}
+	_dpu_plane_get_aspace(pdpu, pstate, &aspace);
 
 	/* cache aspace */
 	pstate->aspace = aspace;
-- 
2.19.0.444.g18242da7ef-goog

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

Re: [PATCH v2 1/7] drm/msm/dpu: Remove unneeded checks in dpu_plane.c

[Jordan Crouse]

On Thu, Sep 20, 2018 at 12:49:18PM -0400, Bruce Wang wrote:
> Removes some checks from dpu_plane.c that will never result in an error.
> Subsequent variable assignments become part of the initialization wherever
> possible. Unused variables are removed.
> 
> Signed-off-by: Bruce Wang <bzwang@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 190 ++--------------------
>  1 file changed, 17 insertions(+), 173 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 1ce76460d710..99887c804e4e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -123,13 +123,8 @@ struct dpu_plane {
>  
>  static struct dpu_kms *_dpu_plane_get_kms(struct drm_plane *plane)
>  {
> -	struct msm_drm_private *priv;
> +	struct msm_drm_private *priv = plane->dev->dev_private;
>  
> -	if (!plane || !plane->dev)
> -		return NULL;
> -	priv = plane->dev->dev_private;
> -	if (!priv)
> -		return NULL;
>  	return to_dpu_kms(priv->kms);
>  }
>  
> @@ -148,7 +143,7 @@ static inline int _dpu_plane_calc_fill_level(struct drm_plane *plane,
>  	u32 fixed_buff_size;
>  	u32 total_fl;
>  
> -	if (!plane || !fmt || !plane->state || !src_width || !fmt->bpp) {
> +	if (!fmt || !plane->state || !src_width || !fmt->bpp) {

A lot of these second level if statements can also scrutinized and you can spend
a lot of time in those rabbit holes so I'm not sure if that is for this patch or
follow on patches.

My bigger concern is that we're storing and printing a log message for most of
these logic checks and those end up being redundant or (probably) impossible.
We should consider using a WARN_ON() in place of these "invalid arguments"
messages.

Again, I'm not sure if that is reasonable for this patch set (since anything
that cleans code is a +1 in my book) but something to keep in mind.

>  		DPU_ERROR("invalid arguments\n");
>  		return 0;
>  	}
> @@ -229,26 +224,11 @@ static u64 _dpu_plane_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
>  static void _dpu_plane_set_qos_lut(struct drm_plane *plane,
>  		struct drm_framebuffer *fb)
>  {
> -	struct dpu_plane *pdpu;
> +	struct dpu_plane *pdpu = to_dpu_plane(plane);
>  	const struct dpu_format *fmt = NULL;
>  	u64 qos_lut;
>  	u32 total_fl = 0, lut_usage;
>  
> -	if (!plane || !fb) {
> -		DPU_ERROR("invalid arguments plane %d fb %d\n",
> -				plane != 0, fb != 0);
> -		return;
> -	}
> -
> -	pdpu = to_dpu_plane(plane);
> -
> -	if (!pdpu->pipe_hw || !pdpu->pipe_sblk || !pdpu->catalog) {
> -		DPU_ERROR("invalid arguments\n");
> -		return;
> -	} else if (!pdpu->pipe_hw->ops.setup_creq_lut) {
> -		return;
> -	}
> -
>  	if (!pdpu->is_rt_pipe) {
>  		lut_usage = DPU_QOS_LUT_USAGE_NRT;
>  	} else {
> @@ -290,24 +270,10 @@ static void _dpu_plane_set_qos_lut(struct drm_plane *plane,
>  static void _dpu_plane_set_danger_lut(struct drm_plane *plane,
>  		struct drm_framebuffer *fb)
>  {
> -	struct dpu_plane *pdpu;
> +	struct dpu_plane *pdpu = to_dpu_plane(plane);
>  	const struct dpu_format *fmt = NULL;
>  	u32 danger_lut, safe_lut;
>  
> -	if (!plane || !fb) {
> -		DPU_ERROR("invalid arguments\n");
> -		return;
> -	}
> -
> -	pdpu = to_dpu_plane(plane);
> -
> -	if (!pdpu->pipe_hw || !pdpu->pipe_sblk || !pdpu->catalog) {
> -		DPU_ERROR("invalid arguments\n");
> -		return;
> -	} else if (!pdpu->pipe_hw->ops.setup_danger_safe_lut) {
> -		return;
> -	}
> -
>  	if (!pdpu->is_rt_pipe) {
>  		danger_lut = pdpu->catalog->perf.danger_lut_tbl
>  				[DPU_QOS_LUT_USAGE_NRT];
> @@ -361,21 +327,7 @@ static void _dpu_plane_set_danger_lut(struct drm_plane *plane,
>  static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
>  	bool enable, u32 flags)
>  {
> -	struct dpu_plane *pdpu;
> -
> -	if (!plane) {
> -		DPU_ERROR("invalid arguments\n");
> -		return;
> -	}
> -
> -	pdpu = to_dpu_plane(plane);
> -
> -	if (!pdpu->pipe_hw || !pdpu->pipe_sblk) {
> -		DPU_ERROR("invalid arguments\n");
> -		return;
> -	} else if (!pdpu->pipe_hw->ops.setup_qos_ctrl) {
> -		return;
> -	}
> +	struct dpu_plane *pdpu = to_dpu_plane(plane);
>  
>  	if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
>  		pdpu->pipe_qos_cfg.creq_vblank = pdpu->pipe_sblk->creq_vblank;
> @@ -450,29 +402,10 @@ int dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
>  static void _dpu_plane_set_ot_limit(struct drm_plane *plane,
>  		struct drm_crtc *crtc)
>  {
> -	struct dpu_plane *pdpu;
> +	struct dpu_plane *pdpu = to_dpu_plane(plane);
>  	struct dpu_vbif_set_ot_params ot_params;
> -	struct msm_drm_private *priv;
> -	struct dpu_kms *dpu_kms;
> -
> -	if (!plane || !plane->dev || !crtc) {
> -		DPU_ERROR("invalid arguments plane %d crtc %d\n",
> -				plane != 0, crtc != 0);
> -		return;
> -	}
> -
> -	priv = plane->dev->dev_private;
> -	if (!priv || !priv->kms) {
> -		DPU_ERROR("invalid KMS reference\n");
> -		return;
> -	}
> -
> -	dpu_kms = to_dpu_kms(priv->kms);
> -	pdpu = to_dpu_plane(plane);
> -	if (!pdpu->pipe_hw) {
> -		DPU_ERROR("invalid pipe reference\n");
> -		return;
> -	}
> +	struct msm_drm_private *priv = plane->dev->dev_private;
> +	struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);

This is the same as _dpu_plane_get_kms() - so you maybe get rid of priv too?

>  
>  	memset(&ot_params, 0, sizeof(ot_params));
>  	ot_params.xin_id = pdpu->pipe_hw->cap->xin_id;
> @@ -494,28 +427,10 @@ static void _dpu_plane_set_ot_limit(struct drm_plane *plane,
>   */
>  static void _dpu_plane_set_qos_remap(struct drm_plane *plane)
>  {
> -	struct dpu_plane *pdpu;
> +	struct dpu_plane *pdpu = to_dpu_plane(plane);
>  	struct dpu_vbif_set_qos_params qos_params;
> -	struct msm_drm_private *priv;
> -	struct dpu_kms *dpu_kms;
> -
> -	if (!plane || !plane->dev) {
> -		DPU_ERROR("invalid arguments\n");
> -		return;
> -	}
> -
> -	priv = plane->dev->dev_private;
> -	if (!priv || !priv->kms) {
> -		DPU_ERROR("invalid KMS reference\n");
> -		return;
> -	}
> -
> -	dpu_kms = to_dpu_kms(priv->kms);
> -	pdpu = to_dpu_plane(plane);
> -	if (!pdpu->pipe_hw) {
> -		DPU_ERROR("invalid pipe reference\n");
> -		return;
> -	}
> +	struct msm_drm_private *priv = plane->dev->dev_private;
> +	struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);

Same.

>  	memset(&qos_params, 0, sizeof(qos_params));
>  	qos_params.vbif_idx = VBIF_RT;
> @@ -549,10 +464,6 @@ static int _dpu_plane_get_aspace(
>  	}
>  
>  	kms = _dpu_plane_get_kms(&pdpu->base);
> -	if (!kms) {
> -		DPU_ERROR("invalid kms\n");
> -		return -EINVAL;
> -	}
>  
>  	*aspace = kms->base.aspace;
>  
> @@ -576,10 +487,6 @@ static inline void _dpu_plane_set_scanout(struct drm_plane *plane,
>  	}
>  
>  	pdpu = to_dpu_plane(plane);
> -	if (!pdpu->pipe_hw) {
> -		DPU_ERROR_PLANE(pdpu, "invalid pipe_hw\n");
> -		return;
> -	}
>  
>  	ret = _dpu_plane_get_aspace(pdpu, pstate, &aspace);
>  	if (ret) {
> @@ -610,15 +517,6 @@ static void _dpu_plane_setup_scaler3(struct dpu_plane *pdpu,
>  {
>  	uint32_t i;
>  
> -	if (!pdpu || !pstate || !scale_cfg || !fmt || !chroma_subsmpl_h ||
> -			!chroma_subsmpl_v) {
> -		DPU_ERROR(
> -			"pdpu %d pstate %d scale_cfg %d fmt %d smp_h %d smp_v %d\n",
> -			!!pdpu, !!pstate, !!scale_cfg, !!fmt, chroma_subsmpl_h,
> -			chroma_subsmpl_v);
> -		return;
> -	}
> -
>  	memset(scale_cfg, 0, sizeof(*scale_cfg));
>  	memset(&pstate->pixel_ext, 0, sizeof(struct dpu_hw_pixel_ext));
>  
> @@ -722,17 +620,8 @@ static void _dpu_plane_setup_scaler(struct dpu_plane *pdpu,
>  		struct dpu_plane_state *pstate,
>  		const struct dpu_format *fmt, bool color_fill)
>  {
> -	struct dpu_hw_pixel_ext *pe;
>  	uint32_t chroma_subsmpl_h, chroma_subsmpl_v;
>  
> -	if (!pdpu || !fmt || !pstate) {
> -		DPU_ERROR("invalid arg(s), plane %d fmt %d state %d\n",
> -				pdpu != 0, fmt != 0, pstate != 0);
> -		return;
> -	}
> -
> -	pe = &pstate->pixel_ext;
> -
>  	/* don't chroma subsample if decimating */
>  	chroma_subsmpl_h =
>  		drm_format_horz_chroma_subsampling(fmt->base.pixel_format);
> @@ -760,21 +649,8 @@ static int _dpu_plane_color_fill(struct dpu_plane *pdpu,
>  		uint32_t color, uint32_t alpha)
>  {
>  	const struct dpu_format *fmt;
> -	const struct drm_plane *plane;
> -	struct dpu_plane_state *pstate;
> -
> -	if (!pdpu || !pdpu->base.state) {
> -		DPU_ERROR("invalid plane\n");
> -		return -EINVAL;
> -	}
> -
> -	if (!pdpu->pipe_hw) {
> -		DPU_ERROR_PLANE(pdpu, "invalid plane h/w pointer\n");
> -		return -EINVAL;
> -	}
> -
> -	plane = &pdpu->base;
> -	pstate = to_dpu_plane_state(plane->state);
> +	const struct drm_plane *plane = &pdpu->base;
> +	struct dpu_plane_state *pstate = to_dpu_plane_state(plane->state);
>  
>  	DPU_DEBUG_PLANE(pdpu, "\n");
>  
> @@ -825,12 +701,7 @@ static int _dpu_plane_color_fill(struct dpu_plane *pdpu,
>  
>  void dpu_plane_clear_multirect(const struct drm_plane_state *drm_state)
>  {
> -	struct dpu_plane_state *pstate;
> -
> -	if (!drm_state)
> -		return;
> -
> -	pstate = to_dpu_plane_state(drm_state);
> +	struct dpu_plane_state *pstate = to_dpu_plane_state(drm_state);
>  
>  	pstate->multirect_index = DPU_SSPP_RECT_SOLO;
>  	pstate->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> @@ -961,15 +832,6 @@ int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane)
>  void dpu_plane_get_ctl_flush(struct drm_plane *plane, struct dpu_hw_ctl *ctl,
>  		u32 *flush_sspp)
>  {
> -	struct dpu_plane_state *pstate;
> -
> -	if (!plane || !flush_sspp) {
> -		DPU_ERROR("invalid parameters\n");
> -		return;
> -	}
> -
> -	pstate = to_dpu_plane_state(plane->state);
> -
>  	*flush_sspp = ctl->ops.get_bitmask_sspp(ctl, dpu_plane_pipe(plane));
>  }
>  
> @@ -1389,8 +1251,7 @@ static void dpu_plane_destroy(struct drm_plane *plane)
>  		/* this will destroy the states as well */
>  		drm_plane_cleanup(plane);
>  
> -		if (pdpu->pipe_hw)
> -			dpu_hw_sspp_destroy(pdpu->pipe_hw);
> +		dpu_hw_sspp_destroy(pdpu->pipe_hw);
>  
>  		kfree(pdpu);
>  	}
> @@ -1737,28 +1598,11 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
>  	struct drm_plane *plane = NULL, *master_plane = NULL;
>  	const struct dpu_format_extended *format_list;
>  	struct dpu_plane *pdpu;
> -	struct msm_drm_private *priv;
> -	struct dpu_kms *kms;
> +	struct msm_drm_private *priv = dev->dev_private;
> +	struct dpu_kms *kms = to_dpu_kms(priv->kms);
>  	int zpos_max = DPU_ZPOS_MAX;
>  	int ret = -EINVAL;
>  
> -	if (!dev) {
> -		DPU_ERROR("[%u]device is NULL\n", pipe);
> -		goto exit;
> -	}
> -
> -	priv = dev->dev_private;
> -	if (!priv) {
> -		DPU_ERROR("[%u]private data is NULL\n", pipe);
> -		goto exit;
> -	}
> -
> -	if (!priv->kms) {
> -		DPU_ERROR("[%u]invalid KMS reference\n", pipe);
> -		goto exit;
> -	}
> -	kms = to_dpu_kms(priv->kms);
> -
>  	if (!kms->catalog) {

From a cursory glance it seems that maybe kms->catalog is always valid but
that assumption should be double checked.

>  		DPU_ERROR("[%u]invalid catalog reference\n", pipe);
>  		goto exit;
> -- 
> 2.19.0.444.g18242da7ef-goog

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v2 4/7] drm/msm/dpu: Change _dpu_crtc_power_enable to void

[Jordan Crouse]

On Thu, Sep 20, 2018 at 12:49:21PM -0400, Bruce Wang wrote:
> All checks for _dpu_crtc_power_enable are not true, so the function
> can never return an error code. All calls of the function have also
> been changed so that they don't expect a return value.
> 
> Signed-off-by: Bruce Wang <bzwang@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 35 ++++--------------------
>  1 file changed, 5 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index a9adb16eac6f..648d77c41c3e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -59,37 +59,16 @@ static inline struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc)
>  			->kms);
>  }
>  
> -static inline int _dpu_crtc_power_enable(struct dpu_crtc *dpu_crtc, bool enable)
> +static inline void _dpu_crtc_power_enable(struct dpu_crtc *dpu_crtc, bool enable)

Since this is only two lines of code with the optimization below my personal
preference would be to turn this into two functions: _dpu_crtc_power_enable and
_dpu_crtc_power_disable. I find that to be more readable when walking through
the functional code.

>  {
> -	struct drm_crtc *crtc;
> -	struct msm_drm_private *priv;
> -	struct dpu_kms *dpu_kms;
> -
> -	if (!dpu_crtc) {
> -		DPU_ERROR("invalid dpu crtc\n");
> -		return -EINVAL;
> -	}
> -
> -	crtc = &dpu_crtc->base;
> -	if (!crtc->dev || !crtc->dev->dev_private) {
> -		DPU_ERROR("invalid drm device\n");
> -		return -EINVAL;
> -	}
> -
> -	priv = crtc->dev->dev_private;
> -	if (!priv->kms) {
> -		DPU_ERROR("invalid kms\n");
> -		return -EINVAL;
> -	}
> -
> -	dpu_kms = to_dpu_kms(priv->kms);
> +	struct drm_crtc *crtc = &dpu_crtc->base;
> +	struct msm_drm_private *priv = crtc->dev->dev_private;
> +	struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);

I think all three of these can be replaced by

struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(&dpu_crtc->base);

>  	if (enable)
>  		pm_runtime_get_sync(&dpu_kms->pdev->dev);
>  	else
>  		pm_runtime_put_sync(&dpu_kms->pdev->dev);
> -
> -	return 0;
>  }
>  
>  static void dpu_crtc_destroy(struct drm_crtc *crtc)
> @@ -822,14 +801,10 @@ static int _dpu_crtc_vblank_enable_no_lock(
>  	dev = crtc->dev;
>  
>  	if (enable) {
> -		int ret;
> -
>  		/* drop lock since power crtc cb may try to re-acquire lock */
>  		mutex_unlock(&dpu_crtc->crtc_lock);
> -		ret = _dpu_crtc_power_enable(dpu_crtc, true);
> +		_dpu_crtc_power_enable(dpu_crtc, true);
>  		mutex_lock(&dpu_crtc->crtc_lock);
> -		if (ret)
> -			return ret;
>  
>  		list_for_each_entry(enc, &dev->mode_config.encoder_list, head) {
>  			if (enc->crtc != crtc)
> -- 
> 2.19.0.444.g18242da7ef-goog
> 
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v2 6/7] drm/msm/dpu: Make dpu_plane_danger_signal_ctrl void

[Jordan Crouse]

On Thu, Sep 20, 2018 at 12:49:23PM -0400, Bruce Wang wrote:
> Removed all impossible checks from the function, which eliminates
> the need for a return value. This function is also never used
> outside of dpu_plane.c, so the function is made static.
> 
> Signed-off-by: Bruce Wang <bzwang@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 27 +++++------------------
>  1 file changed, 5 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 9a5d5afa53f2..f9e65acdf87e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -363,35 +363,18 @@ static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
>  			&pdpu->pipe_qos_cfg);
>  }
>  
> -int dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
> +static void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
>  {
> -	struct dpu_plane *pdpu;
> -	struct msm_drm_private *priv;
> -	struct dpu_kms *dpu_kms;
> -
> -	if (!plane || !plane->dev) {
> -		DPU_ERROR("invalid arguments\n");
> -		return -EINVAL;
> -	}
> -
> -	priv = plane->dev->dev_private;
> -	if (!priv || !priv->kms) {
> -		DPU_ERROR("invalid KMS reference\n");
> -		return -EINVAL;
> -	}
> -
> -	dpu_kms = to_dpu_kms(priv->kms);
> -	pdpu = to_dpu_plane(plane);
> +	struct dpu_plane *pdpu = to_dpu_plane(plane);
> +	struct msm_drm_private *priv = plane->dev->dev_private;
> +	struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);

I think _dpu_plane_get_kms() can be used here.

>  	if (!pdpu->is_rt_pipe)
> -		goto end;
> +		return;
>  
>  	pm_runtime_get_sync(&dpu_kms->pdev->dev);
>  	_dpu_plane_set_qos_ctrl(plane, enable, DPU_PLANE_QOS_PANIC_CTRL);
>  	pm_runtime_put_sync(&dpu_kms->pdev->dev);
> -
> -end:
> -	return 0;
>  }
>  
>  /**
> -- 
> 2.19.0.444.g18242da7ef-goog
> 
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH v2 1/7] drm/msm/dpu: Remove unneeded checks in dpu_plane.c

[Sean Paul]

On Thu, Sep 20, 2018 at 12:49:18PM -0400, Bruce Wang wrote:
> Removes some checks from dpu_plane.c that will never result in an error.
> Subsequent variable assignments become part of the initialization wherever
> possible. Unused variables are removed.
> 
> Signed-off-by: Bruce Wang <bzwang@chromium.org>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 190 ++--------------------
>  1 file changed, 17 insertions(+), 173 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 1ce76460d710..99887c804e4e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -123,13 +123,8 @@ struct dpu_plane {
>  
>  static struct dpu_kms *_dpu_plane_get_kms(struct drm_plane *plane)
>  {
> -	struct msm_drm_private *priv;
> +	struct msm_drm_private *priv = plane->dev->dev_private;
>  
> -	if (!plane || !plane->dev)
> -		return NULL;
> -	priv = plane->dev->dev_private;
> -	if (!priv)
> -		return NULL;
>  	return to_dpu_kms(priv->kms);
>  }
>  
> @@ -148,7 +143,7 @@ static inline int _dpu_plane_calc_fill_level(struct drm_plane *plane,
>  	u32 fixed_buff_size;
>  	u32 total_fl;
>  
> -	if (!plane || !fmt || !plane->state || !src_width || !fmt->bpp) {
> +	if (!fmt || !plane->state || !src_width || !fmt->bpp) {
>  		DPU_ERROR("invalid arguments\n");
>  		return 0;
>  	}
> @@ -229,26 +224,11 @@ static u64 _dpu_plane_get_qos_lut(const struct dpu_qos_lut_tbl *tbl,
>  static void _dpu_plane_set_qos_lut(struct drm_plane *plane,
>  		struct drm_framebuffer *fb)
>  {
> -	struct dpu_plane *pdpu;
> +	struct dpu_plane *pdpu = to_dpu_plane(plane);
>  	const struct dpu_format *fmt = NULL;
>  	u64 qos_lut;
>  	u32 total_fl = 0, lut_usage;
>  
> -	if (!plane || !fb) {
> -		DPU_ERROR("invalid arguments plane %d fb %d\n",
> -				plane != 0, fb != 0);
> -		return;
> -	}
> -
> -	pdpu = to_dpu_plane(plane);
> -
> -	if (!pdpu->pipe_hw || !pdpu->pipe_sblk || !pdpu->catalog) {
> -		DPU_ERROR("invalid arguments\n");
> -		return;
> -	} else if (!pdpu->pipe_hw->ops.setup_creq_lut) {
> -		return;
> -	}
> -
>  	if (!pdpu->is_rt_pipe) {
>  		lut_usage = DPU_QOS_LUT_USAGE_NRT;
>  	} else {
> @@ -290,24 +270,10 @@ static void _dpu_plane_set_qos_lut(struct drm_plane *plane,
>  static void _dpu_plane_set_danger_lut(struct drm_plane *plane,
>  		struct drm_framebuffer *fb)
>  {
> -	struct dpu_plane *pdpu;
> +	struct dpu_plane *pdpu = to_dpu_plane(plane);
>  	const struct dpu_format *fmt = NULL;
>  	u32 danger_lut, safe_lut;
>  
> -	if (!plane || !fb) {
> -		DPU_ERROR("invalid arguments\n");
> -		return;
> -	}
> -
> -	pdpu = to_dpu_plane(plane);
> -
> -	if (!pdpu->pipe_hw || !pdpu->pipe_sblk || !pdpu->catalog) {
> -		DPU_ERROR("invalid arguments\n");
> -		return;
> -	} else if (!pdpu->pipe_hw->ops.setup_danger_safe_lut) {
> -		return;
> -	}
> -
>  	if (!pdpu->is_rt_pipe) {
>  		danger_lut = pdpu->catalog->perf.danger_lut_tbl
>  				[DPU_QOS_LUT_USAGE_NRT];
> @@ -361,21 +327,7 @@ static void _dpu_plane_set_danger_lut(struct drm_plane *plane,
>  static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
>  	bool enable, u32 flags)
>  {
> -	struct dpu_plane *pdpu;
> -
> -	if (!plane) {
> -		DPU_ERROR("invalid arguments\n");
> -		return;
> -	}
> -
> -	pdpu = to_dpu_plane(plane);
> -
> -	if (!pdpu->pipe_hw || !pdpu->pipe_sblk) {
> -		DPU_ERROR("invalid arguments\n");
> -		return;
> -	} else if (!pdpu->pipe_hw->ops.setup_qos_ctrl) {
> -		return;
> -	}
> +	struct dpu_plane *pdpu = to_dpu_plane(plane);
>  
>  	if (flags & DPU_PLANE_QOS_VBLANK_CTRL) {
>  		pdpu->pipe_qos_cfg.creq_vblank = pdpu->pipe_sblk->creq_vblank;
> @@ -450,29 +402,10 @@ int dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
>  static void _dpu_plane_set_ot_limit(struct drm_plane *plane,
>  		struct drm_crtc *crtc)
>  {
> -	struct dpu_plane *pdpu;
> +	struct dpu_plane *pdpu = to_dpu_plane(plane);
>  	struct dpu_vbif_set_ot_params ot_params;
> -	struct msm_drm_private *priv;
> -	struct dpu_kms *dpu_kms;
> -
> -	if (!plane || !plane->dev || !crtc) {
> -		DPU_ERROR("invalid arguments plane %d crtc %d\n",
> -				plane != 0, crtc != 0);
> -		return;
> -	}
> -
> -	priv = plane->dev->dev_private;
> -	if (!priv || !priv->kms) {
> -		DPU_ERROR("invalid KMS reference\n");
> -		return;
> -	}
> -
> -	dpu_kms = to_dpu_kms(priv->kms);
> -	pdpu = to_dpu_plane(plane);
> -	if (!pdpu->pipe_hw) {
> -		DPU_ERROR("invalid pipe reference\n");
> -		return;
> -	}
> +	struct msm_drm_private *priv = plane->dev->dev_private;
> +	struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
>  
>  	memset(&ot_params, 0, sizeof(ot_params));
>  	ot_params.xin_id = pdpu->pipe_hw->cap->xin_id;
> @@ -494,28 +427,10 @@ static void _dpu_plane_set_ot_limit(struct drm_plane *plane,
>   */
>  static void _dpu_plane_set_qos_remap(struct drm_plane *plane)
>  {
> -	struct dpu_plane *pdpu;
> +	struct dpu_plane *pdpu = to_dpu_plane(plane);
>  	struct dpu_vbif_set_qos_params qos_params;
> -	struct msm_drm_private *priv;
> -	struct dpu_kms *dpu_kms;
> -
> -	if (!plane || !plane->dev) {
> -		DPU_ERROR("invalid arguments\n");
> -		return;
> -	}
> -
> -	priv = plane->dev->dev_private;
> -	if (!priv || !priv->kms) {
> -		DPU_ERROR("invalid KMS reference\n");
> -		return;
> -	}
> -
> -	dpu_kms = to_dpu_kms(priv->kms);
> -	pdpu = to_dpu_plane(plane);
> -	if (!pdpu->pipe_hw) {
> -		DPU_ERROR("invalid pipe reference\n");
> -		return;
> -	}
> +	struct msm_drm_private *priv = plane->dev->dev_private;
> +	struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
>  
>  	memset(&qos_params, 0, sizeof(qos_params));
>  	qos_params.vbif_idx = VBIF_RT;
> @@ -549,10 +464,6 @@ static int _dpu_plane_get_aspace(
>  	}
>  
>  	kms = _dpu_plane_get_kms(&pdpu->base);
> -	if (!kms) {
> -		DPU_ERROR("invalid kms\n");
> -		return -EINVAL;
> -	}
>  
>  	*aspace = kms->base.aspace;
>  
> @@ -576,10 +487,6 @@ static inline void _dpu_plane_set_scanout(struct drm_plane *plane,
>  	}
>  
>  	pdpu = to_dpu_plane(plane);
> -	if (!pdpu->pipe_hw) {
> -		DPU_ERROR_PLANE(pdpu, "invalid pipe_hw\n");
> -		return;
> -	}
>  
>  	ret = _dpu_plane_get_aspace(pdpu, pstate, &aspace);
>  	if (ret) {
> @@ -610,15 +517,6 @@ static void _dpu_plane_setup_scaler3(struct dpu_plane *pdpu,
>  {
>  	uint32_t i;
>  
> -	if (!pdpu || !pstate || !scale_cfg || !fmt || !chroma_subsmpl_h ||
> -			!chroma_subsmpl_v) {
> -		DPU_ERROR(
> -			"pdpu %d pstate %d scale_cfg %d fmt %d smp_h %d smp_v %d\n",
> -			!!pdpu, !!pstate, !!scale_cfg, !!fmt, chroma_subsmpl_h,
> -			chroma_subsmpl_v);
> -		return;
> -	}
> -
>  	memset(scale_cfg, 0, sizeof(*scale_cfg));
>  	memset(&pstate->pixel_ext, 0, sizeof(struct dpu_hw_pixel_ext));
>  
> @@ -722,17 +620,8 @@ static void _dpu_plane_setup_scaler(struct dpu_plane *pdpu,
>  		struct dpu_plane_state *pstate,
>  		const struct dpu_format *fmt, bool color_fill)
>  {
> -	struct dpu_hw_pixel_ext *pe;
>  	uint32_t chroma_subsmpl_h, chroma_subsmpl_v;
>  
> -	if (!pdpu || !fmt || !pstate) {
> -		DPU_ERROR("invalid arg(s), plane %d fmt %d state %d\n",
> -				pdpu != 0, fmt != 0, pstate != 0);
> -		return;
> -	}
> -
> -	pe = &pstate->pixel_ext;
> -
>  	/* don't chroma subsample if decimating */
>  	chroma_subsmpl_h =
>  		drm_format_horz_chroma_subsampling(fmt->base.pixel_format);
> @@ -760,21 +649,8 @@ static int _dpu_plane_color_fill(struct dpu_plane *pdpu,
>  		uint32_t color, uint32_t alpha)
>  {
>  	const struct dpu_format *fmt;
> -	const struct drm_plane *plane;
> -	struct dpu_plane_state *pstate;
> -
> -	if (!pdpu || !pdpu->base.state) {
> -		DPU_ERROR("invalid plane\n");
> -		return -EINVAL;
> -	}
> -
> -	if (!pdpu->pipe_hw) {
> -		DPU_ERROR_PLANE(pdpu, "invalid plane h/w pointer\n");
> -		return -EINVAL;
> -	}
> -
> -	plane = &pdpu->base;
> -	pstate = to_dpu_plane_state(plane->state);
> +	const struct drm_plane *plane = &pdpu->base;
> +	struct dpu_plane_state *pstate = to_dpu_plane_state(plane->state);
>  
>  	DPU_DEBUG_PLANE(pdpu, "\n");
>  
> @@ -825,12 +701,7 @@ static int _dpu_plane_color_fill(struct dpu_plane *pdpu,
>  
>  void dpu_plane_clear_multirect(const struct drm_plane_state *drm_state)
>  {
> -	struct dpu_plane_state *pstate;
> -
> -	if (!drm_state)
> -		return;
> -
> -	pstate = to_dpu_plane_state(drm_state);
> +	struct dpu_plane_state *pstate = to_dpu_plane_state(drm_state);
>  
>  	pstate->multirect_index = DPU_SSPP_RECT_SOLO;
>  	pstate->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
> @@ -961,15 +832,6 @@ int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane)
>  void dpu_plane_get_ctl_flush(struct drm_plane *plane, struct dpu_hw_ctl *ctl,
>  		u32 *flush_sspp)
>  {
> -	struct dpu_plane_state *pstate;
> -
> -	if (!plane || !flush_sspp) {
> -		DPU_ERROR("invalid parameters\n");
> -		return;
> -	}
> -
> -	pstate = to_dpu_plane_state(plane->state);
> -
>  	*flush_sspp = ctl->ops.get_bitmask_sspp(ctl, dpu_plane_pipe(plane));
>  }
>  
> @@ -1389,8 +1251,7 @@ static void dpu_plane_destroy(struct drm_plane *plane)
>  		/* this will destroy the states as well */
>  		drm_plane_cleanup(plane);
>  
> -		if (pdpu->pipe_hw)
> -			dpu_hw_sspp_destroy(pdpu->pipe_hw);
> +		dpu_hw_sspp_destroy(pdpu->pipe_hw);
>  
>  		kfree(pdpu);
>  	}
> @@ -1737,28 +1598,11 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
>  	struct drm_plane *plane = NULL, *master_plane = NULL;
>  	const struct dpu_format_extended *format_list;
>  	struct dpu_plane *pdpu;
> -	struct msm_drm_private *priv;
> -	struct dpu_kms *kms;
> +	struct msm_drm_private *priv = dev->dev_private;
> +	struct dpu_kms *kms = to_dpu_kms(priv->kms);
>  	int zpos_max = DPU_ZPOS_MAX;
>  	int ret = -EINVAL;
>  
> -	if (!dev) {
> -		DPU_ERROR("[%u]device is NULL\n", pipe);
> -		goto exit;
> -	}
> -
> -	priv = dev->dev_private;
> -	if (!priv) {
> -		DPU_ERROR("[%u]private data is NULL\n", pipe);
> -		goto exit;
> -	}
> -
> -	if (!priv->kms) {
> -		DPU_ERROR("[%u]invalid KMS reference\n", pipe);
> -		goto exit;
> -	}
> -	kms = to_dpu_kms(priv->kms);
> -
>  	if (!kms->catalog) {
>  		DPU_ERROR("[%u]invalid catalog reference\n", pipe);
>  		goto exit;
> -- 
> 2.19.0.444.g18242da7ef-goog
> 
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno

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

Re: [PATCH v2 2/7] drm/msm/dpu: Clean up plane atomic disable/update

[Sean Paul]

On Thu, Sep 20, 2018 at 12:49:19PM -0400, Bruce Wang wrote:
> Removes unnecessary checks from dpu_plane_atomic_disable, old_state
> argument for both dpu_plane_atomic_disable and
> dpu_plane_sspp_atomic_update is removed as it is no longer used.
> 
> Signed-off-by: Bruce Wang <bzwang@chromium.org>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 31 +++++------------------
>  1 file changed, 7 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 99887c804e4e..9a5d5afa53f2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -1054,8 +1054,7 @@ void dpu_plane_set_error(struct drm_plane *plane, bool error)
>  	pdpu->is_error = error;
>  }
>  
> -static void 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)
>  {
>  	uint32_t src_flags;
>  	struct dpu_plane *pdpu = to_dpu_plane(plane);
> @@ -1168,27 +1167,11 @@ static void dpu_plane_sspp_atomic_update(struct drm_plane *plane,
>  	_dpu_plane_set_qos_remap(plane);
>  }
>  
> -static void _dpu_plane_atomic_disable(struct drm_plane *plane,
> -				struct drm_plane_state *old_state)
> +static void _dpu_plane_atomic_disable(struct drm_plane *plane)
>  {
> -	struct dpu_plane *pdpu;
> -	struct drm_plane_state *state;
> -	struct dpu_plane_state *pstate;
> -
> -	if (!plane) {
> -		DPU_ERROR("invalid plane\n");
> -		return;
> -	} else if (!plane->state) {
> -		DPU_ERROR("invalid plane state\n");
> -		return;
> -	} else if (!old_state) {
> -		DPU_ERROR("invalid old state\n");
> -		return;
> -	}
> -
> -	pdpu = to_dpu_plane(plane);
> -	state = plane->state;
> -	pstate = to_dpu_plane_state(state);
> +	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);
>  
>  	trace_dpu_plane_disable(DRMID(plane), is_dpu_plane_virtual(plane),
>  				pstate->multirect_mode);
> @@ -1212,9 +1195,9 @@ static void dpu_plane_atomic_update(struct drm_plane *plane,
>  	DPU_DEBUG_PLANE(pdpu, "\n");
>  
>  	if (!state->visible) {
> -		_dpu_plane_atomic_disable(plane, old_state);
> +		_dpu_plane_atomic_disable(plane);
>  	} else {
> -		dpu_plane_sspp_atomic_update(plane, old_state);
> +		dpu_plane_sspp_atomic_update(plane);
>  	}
>  }
>  
> -- 
> 2.19.0.444.g18242da7ef-goog
> 

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

Re: [PATCH v2 3/7] drm/msm/dpu: Remove unneeded checks in dpu_crtc.c

[Sean Paul]

On Thu, Sep 20, 2018 at 12:49:20PM -0400, Bruce Wang wrote:
> Removes impossible checks in dpu_crtc.c.
> Variable assignments are moved up to be initializations where
> possible. Some variables are no longer used, these are removed.
> 
> Signed-off-by: Bruce Wang <bzwang@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 170 +++--------------------
>  1 file changed, 23 insertions(+), 147 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index a8f2dd7a37c7..a9adb16eac6f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -55,19 +55,8 @@ static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc_state *cstate,
>  
>  static inline struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc)
>  {
> -	struct msm_drm_private *priv;
> -
> -	if (!crtc || !crtc->dev || !crtc->dev->dev_private) {
> -		DPU_ERROR("invalid crtc\n");
> -		return NULL;
> -	}
> -	priv = crtc->dev->dev_private;
> -	if (!priv || !priv->kms) {
> -		DPU_ERROR("invalid kms\n");
> -		return NULL;
> -	}
> -
> -	return to_dpu_kms(priv->kms);
> +	return to_dpu_kms(((struct msm_drm_private *) crtc->dev->dev_private)
> +			->kms);

Hmm, I didn't realize the cast was needed in order to inline everything. In
light of that, I'd prefer going back to your v1.

With that fixed,

Reviewed-by: Sean Paul <seanpaul@chromium.org>


>  }
>  
>  static inline int _dpu_crtc_power_enable(struct dpu_crtc *dpu_crtc, bool enable)
> @@ -177,28 +166,17 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
>  	struct drm_plane *plane;
>  	struct drm_framebuffer *fb;
>  	struct drm_plane_state *state;
> -	struct dpu_crtc_state *cstate;
> +	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
>  	struct dpu_plane_state *pstate = NULL;
>  	struct dpu_format *format;
> -	struct dpu_hw_ctl *ctl;
> -	struct dpu_hw_mixer *lm;
> -	struct dpu_hw_stage_cfg *stage_cfg;
> +	struct dpu_hw_ctl *ctl = mixer->lm_ctl;
> +	struct dpu_hw_stage_cfg *stage_cfg = &dpu_crtc->stage_cfg;
>  
>  	u32 flush_mask;
>  	uint32_t stage_idx, lm_idx;
>  	int zpos_cnt[DPU_STAGE_MAX + 1] = { 0 };
>  	bool bg_alpha_enable = false;
>  
> -	if (!dpu_crtc || !mixer) {
> -		DPU_ERROR("invalid dpu_crtc or mixer\n");
> -		return;
> -	}
> -
> -	ctl = mixer->lm_ctl;
> -	lm = mixer->hw_lm;
> -	stage_cfg = &dpu_crtc->stage_cfg;
> -	cstate = to_dpu_crtc_state(crtc->state);
> -
>  	drm_atomic_crtc_for_each_plane(plane, crtc) {
>  		state = plane->state;
>  		if (!state)
> @@ -217,10 +195,6 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
>  				state->fb ? state->fb->base.id : -1);
>  
>  		format = to_dpu_format(msm_framebuffer_format(pstate->base.fb));
> -		if (!format) {
> -			DPU_ERROR("invalid format\n");
> -			return;
> -		}
>  
>  		if (pstate->stage == DPU_STAGE_BASE && format->alpha_enable)
>  			bg_alpha_enable = true;
> @@ -261,21 +235,13 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc,
>   */
>  static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
>  {
> -	struct dpu_crtc *dpu_crtc;
> -	struct dpu_crtc_state *cstate;
> -	struct dpu_crtc_mixer *mixer;
> +	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> +	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
> +	struct dpu_crtc_mixer *mixer = cstate->mixers;
>  	struct dpu_hw_ctl *ctl;
>  	struct dpu_hw_mixer *lm;
> -
>  	int i;
>  
> -	if (!crtc)
> -		return;
> -
> -	dpu_crtc = to_dpu_crtc(crtc);
> -	cstate = to_dpu_crtc_state(crtc->state);
> -	mixer = cstate->mixers;
> -
>  	DPU_DEBUG("%s\n", dpu_crtc->name);
>  
>  	for (i = 0; i < cstate->num_mixers; i++) {
> @@ -377,34 +343,13 @@ static void dpu_crtc_vblank_cb(void *data)
>  
>  static void dpu_crtc_frame_event_work(struct kthread_work *work)
>  {
> -	struct msm_drm_private *priv;
> -	struct dpu_crtc_frame_event *fevent;
> -	struct drm_crtc *crtc;
> -	struct dpu_crtc *dpu_crtc;
> -	struct dpu_kms *dpu_kms;
> +	struct dpu_crtc_frame_event *fevent = container_of(work,
> +			struct dpu_crtc_frame_event, work);
> +	struct drm_crtc *crtc = fevent->crtc;
> +	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>  	unsigned long flags;
>  	bool frame_done = false;
>  
> -	if (!work) {
> -		DPU_ERROR("invalid work handle\n");
> -		return;
> -	}
> -
> -	fevent = container_of(work, struct dpu_crtc_frame_event, work);
> -	if (!fevent->crtc || !fevent->crtc->state) {
> -		DPU_ERROR("invalid crtc\n");
> -		return;
> -	}
> -
> -	crtc = fevent->crtc;
> -	dpu_crtc = to_dpu_crtc(crtc);
> -
> -	dpu_kms = _dpu_crtc_get_kms(crtc);
> -	if (!dpu_kms) {
> -		DPU_ERROR("invalid kms handle\n");
> -		return;
> -	}
> -	priv = dpu_kms->dev->dev_private;
>  	DPU_ATRACE_BEGIN("crtc_frame_event");
>  
>  	DRM_DEBUG_KMS("crtc%d event:%u ts:%lld\n", crtc->base.id, fevent->event,
> @@ -470,11 +415,6 @@ static void dpu_crtc_frame_event_cb(void *data, u32 event)
>  	unsigned long flags;
>  	u32 crtc_id;
>  
> -	if (!crtc || !crtc->dev || !crtc->dev->dev_private) {
> -		DPU_ERROR("invalid parameters\n");
> -		return;
> -	}
> -
>  	/* Nothing to do on idle event */
>  	if (event & DPU_ENCODER_FRAME_EVENT_IDLE)
>  		return;
> @@ -583,23 +523,12 @@ static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
>  static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,
>  		struct drm_crtc_state *state)
>  {
> -	struct dpu_crtc *dpu_crtc;
> -	struct dpu_crtc_state *cstate;
> -	struct drm_display_mode *adj_mode;
> -	u32 crtc_split_width;
> +	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> +	struct dpu_crtc_state *cstate = to_dpu_crtc_state(state);
> +	struct drm_display_mode *adj_mode = &state->adjusted_mode;
> +	u32 crtc_split_width = _dpu_crtc_get_mixer_width(cstate, adj_mode);
>  	int i;
>  
> -	if (!crtc || !state) {
> -		DPU_ERROR("invalid args\n");
> -		return;
> -	}
> -
> -	dpu_crtc = to_dpu_crtc(crtc);
> -	cstate = to_dpu_crtc_state(state);
> -
> -	adj_mode = &state->adjusted_mode;
> -	crtc_split_width = _dpu_crtc_get_mixer_width(cstate, adj_mode);
> -
>  	for (i = 0; i < cstate->num_mixers; i++) {
>  		struct drm_rect *r = &cstate->lm_bounds[i];
>  		r->x1 = crtc_split_width * i;
> @@ -693,11 +622,6 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc,
>  	unsigned long flags;
>  	struct dpu_crtc_state *cstate;
>  
> -	if (!crtc || !crtc->dev || !crtc->dev->dev_private) {
> -		DPU_ERROR("invalid crtc\n");
> -		return;
> -	}
> -
>  	if (!crtc->state->enable) {
>  		DPU_DEBUG("crtc%d -> enable %d, skip atomic_flush\n",
>  				crtc->base.id, crtc->state->enable);
> @@ -790,15 +714,9 @@ static void dpu_crtc_destroy_state(struct drm_crtc *crtc,
>  
>  static int _dpu_crtc_wait_for_frame_done(struct drm_crtc *crtc)
>  {
> -	struct dpu_crtc *dpu_crtc;
> +	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>  	int ret, rc = 0;
>  
> -	if (!crtc) {
> -		DPU_ERROR("invalid argument\n");
> -		return -EINVAL;
> -	}
> -	dpu_crtc = to_dpu_crtc(crtc);
> -
>  	if (!atomic_read(&dpu_crtc->frame_pending)) {
>  		DPU_DEBUG("no frames pending\n");
>  		return 0;
> @@ -819,29 +737,12 @@ static int _dpu_crtc_wait_for_frame_done(struct drm_crtc *crtc)
>  void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
>  {
>  	struct drm_encoder *encoder;
> -	struct drm_device *dev;
> -	struct dpu_crtc *dpu_crtc;
> -	struct msm_drm_private *priv;
> -	struct dpu_kms *dpu_kms;
> -	struct dpu_crtc_state *cstate;
> +	struct drm_device *dev = crtc->dev;
> +	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> +	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
> +	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
>  	int ret;
>  
> -	if (!crtc) {
> -		DPU_ERROR("invalid argument\n");
> -		return;
> -	}
> -	dev = crtc->dev;
> -	dpu_crtc = to_dpu_crtc(crtc);
> -	dpu_kms = _dpu_crtc_get_kms(crtc);
> -
> -	if (!dpu_kms || !dpu_kms->dev || !dpu_kms->dev->dev_private) {
> -		DPU_ERROR("invalid argument\n");
> -		return;
> -	}
> -
> -	priv = dpu_kms->dev->dev_private;
> -	cstate = to_dpu_crtc_state(crtc->state);
> -
>  	/*
>  	 * If no mixers has been allocated in dpu_crtc_atomic_check(),
>  	 * it means we are trying to start a CRTC whose state is disabled:
> @@ -969,24 +870,9 @@ static int _dpu_crtc_vblank_enable_no_lock(
>   */
>  static void _dpu_crtc_set_suspend(struct drm_crtc *crtc, bool enable)
>  {
> -	struct dpu_crtc *dpu_crtc;
> -	struct msm_drm_private *priv;
> -	struct dpu_kms *dpu_kms;
> +	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>  	int ret = 0;
>  
> -	if (!crtc || !crtc->dev || !crtc->dev->dev_private) {
> -		DPU_ERROR("invalid crtc\n");
> -		return;
> -	}
> -	dpu_crtc = to_dpu_crtc(crtc);
> -	priv = crtc->dev->dev_private;
> -
> -	if (!priv->kms) {
> -		DPU_ERROR("invalid crtc kms\n");
> -		return;
> -	}
> -	dpu_kms = to_dpu_kms(priv->kms);
> -
>  	DRM_DEBUG_KMS("crtc%d suspend = %d\n", crtc->base.id, enable);
>  
>  	mutex_lock(&dpu_crtc->crtc_lock);
> @@ -1079,16 +965,8 @@ static void dpu_crtc_reset(struct drm_crtc *crtc)
>  static void dpu_crtc_handle_power_event(u32 event_type, void *arg)
>  {
>  	struct drm_crtc *crtc = arg;
> -	struct dpu_crtc *dpu_crtc;
> +	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>  	struct drm_encoder *encoder;
> -	struct dpu_crtc_state *cstate;
> -
> -	if (!crtc) {
> -		DPU_ERROR("invalid crtc\n");
> -		return;
> -	}
> -	dpu_crtc = to_dpu_crtc(crtc);
> -	cstate = to_dpu_crtc_state(dpu_crtc->base.state);
>  
>  	mutex_lock(&dpu_crtc->crtc_lock);
>  
> @@ -1673,8 +1551,6 @@ static int _dpu_crtc_init_debugfs(struct drm_crtc *crtc)
>  	dpu_crtc = to_dpu_crtc(crtc);
>  
>  	dpu_kms = _dpu_crtc_get_kms(crtc);
> -	if (!dpu_kms)
> -		return -EINVAL;
>  
>  	dpu_crtc->debugfs_root = debugfs_create_dir(dpu_crtc->name,
>  			crtc->dev->primary->debugfs_root);
> -- 
> 2.19.0.444.g18242da7ef-goog
> 
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno

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

Re: [PATCH v2 4/7] drm/msm/dpu: Change _dpu_crtc_power_enable to void

[Sean Paul]

On Thu, Sep 20, 2018 at 11:36:34AM -0600, Jordan Crouse wrote:
> On Thu, Sep 20, 2018 at 12:49:21PM -0400, Bruce Wang wrote:
> > All checks for _dpu_crtc_power_enable are not true, so the function
> > can never return an error code. All calls of the function have also
> > been changed so that they don't expect a return value.
> > 
> > Signed-off-by: Bruce Wang <bzwang@chromium.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 35 ++++--------------------
> >  1 file changed, 5 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index a9adb16eac6f..648d77c41c3e 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -59,37 +59,16 @@ static inline struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc)
> >  			->kms);
> >  }
> >  
> > -static inline int _dpu_crtc_power_enable(struct dpu_crtc *dpu_crtc, bool enable)
> > +static inline void _dpu_crtc_power_enable(struct dpu_crtc *dpu_crtc, bool enable)
> 
> Since this is only two lines of code with the optimization below my personal
> preference would be to turn this into two functions: _dpu_crtc_power_enable and
> _dpu_crtc_power_disable. I find that to be more readable when walking through
> the functional code.

...or just call the pm_runtime functions directly? If we can use
_dpu_crtc_get_kms() it shouldn't be too nasty.

Sean

> 
> >  {
> > -	struct drm_crtc *crtc;
> > -	struct msm_drm_private *priv;
> > -	struct dpu_kms *dpu_kms;
> > -
> > -	if (!dpu_crtc) {
> > -		DPU_ERROR("invalid dpu crtc\n");
> > -		return -EINVAL;
> > -	}
> > -
> > -	crtc = &dpu_crtc->base;
> > -	if (!crtc->dev || !crtc->dev->dev_private) {
> > -		DPU_ERROR("invalid drm device\n");
> > -		return -EINVAL;
> > -	}
> > -
> > -	priv = crtc->dev->dev_private;
> > -	if (!priv->kms) {
> > -		DPU_ERROR("invalid kms\n");
> > -		return -EINVAL;
> > -	}
> > -
> > -	dpu_kms = to_dpu_kms(priv->kms);
> > +	struct drm_crtc *crtc = &dpu_crtc->base;
> > +	struct msm_drm_private *priv = crtc->dev->dev_private;
> > +	struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
> 
> I think all three of these can be replaced by
> 
> struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(&dpu_crtc->base);
> 
> >  	if (enable)
> >  		pm_runtime_get_sync(&dpu_kms->pdev->dev);
> >  	else
> >  		pm_runtime_put_sync(&dpu_kms->pdev->dev);
> > -
> > -	return 0;
> >  }
> >  
> >  static void dpu_crtc_destroy(struct drm_crtc *crtc)
> > @@ -822,14 +801,10 @@ static int _dpu_crtc_vblank_enable_no_lock(
> >  	dev = crtc->dev;
> >  
> >  	if (enable) {
> > -		int ret;
> > -
> >  		/* drop lock since power crtc cb may try to re-acquire lock */
> >  		mutex_unlock(&dpu_crtc->crtc_lock);
> > -		ret = _dpu_crtc_power_enable(dpu_crtc, true);
> > +		_dpu_crtc_power_enable(dpu_crtc, true);
> >  		mutex_lock(&dpu_crtc->crtc_lock);
> > -		if (ret)
> > -			return ret;
> >  
> >  		list_for_each_entry(enc, &dev->mode_config.encoder_list, head) {
> >  			if (enc->crtc != crtc)
> > -- 
> > 2.19.0.444.g18242da7ef-goog
> > 
> > _______________________________________________
> > Freedreno mailing list
> > Freedreno@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/freedreno
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

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

Re: [PATCH v2 5/7] drm/msm/dpu: Change _dpu_crtc_vblank_enable_no_lock to void

[Sean Paul]

On Thu, Sep 20, 2018 at 12:49:22PM -0400, Bruce Wang wrote:
> Removes redundant tests for _dpu_crtc_vblank_enable_no_lock.
> Function return type is now void and all function calls have
> been changed accordingly.
> 
> Signed-off-by: Bruce Wang <bzwang@chromium.org>

Still

Reviewed-by: Sean Paul <seanpaul@chromium.org>


> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 42 ++++--------------------
>  1 file changed, 7 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 648d77c41c3e..0a2d52847655 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -782,24 +782,14 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
>   * _dpu_crtc_vblank_enable_no_lock - update power resource and vblank request
>   * @dpu_crtc: Pointer to dpu crtc structure
>   * @enable: Whether to enable/disable vblanks
> - *
> - * @Return: error code
>   */
> -static int _dpu_crtc_vblank_enable_no_lock(
> +static void _dpu_crtc_vblank_enable_no_lock(
>  		struct dpu_crtc *dpu_crtc, bool enable)
>  {
> -	struct drm_device *dev;
> -	struct drm_crtc *crtc;
> +	struct drm_crtc *crtc = &dpu_crtc->base;
> +	struct drm_device *dev = crtc->dev;
>  	struct drm_encoder *enc;
>  
> -	if (!dpu_crtc) {
> -		DPU_ERROR("invalid crtc\n");
> -		return -EINVAL;
> -	}
> -
> -	crtc = &dpu_crtc->base;
> -	dev = crtc->dev;
> -
>  	if (enable) {
>  		/* drop lock since power crtc cb may try to re-acquire lock */
>  		mutex_unlock(&dpu_crtc->crtc_lock);
> @@ -834,8 +824,6 @@ static int _dpu_crtc_vblank_enable_no_lock(
>  		_dpu_crtc_power_enable(dpu_crtc, false);
>  		mutex_lock(&dpu_crtc->crtc_lock);
>  	}
> -
> -	return 0;
>  }
>  
>  /**
> @@ -846,7 +834,6 @@ static int _dpu_crtc_vblank_enable_no_lock(
>  static void _dpu_crtc_set_suspend(struct drm_crtc *crtc, bool enable)
>  {
>  	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> -	int ret = 0;
>  
>  	DRM_DEBUG_KMS("crtc%d suspend = %d\n", crtc->base.id, enable);
>  
> @@ -861,10 +848,7 @@ static void _dpu_crtc_set_suspend(struct drm_crtc *crtc, bool enable)
>  		DPU_DEBUG("crtc%d suspend already set to %d, ignoring update\n",
>  				crtc->base.id, enable);
>  	else if (dpu_crtc->enabled && dpu_crtc->vblank_requested) {
> -		ret = _dpu_crtc_vblank_enable_no_lock(dpu_crtc, !enable);
> -		if (ret)
> -			DPU_ERROR("%s vblank enable failed: %d\n",
> -					dpu_crtc->name, ret);
> +		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, !enable);
>  	}
>  
>  	dpu_crtc->suspend = enable;
> @@ -965,7 +949,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
>  	struct drm_display_mode *mode;
>  	struct drm_encoder *encoder;
>  	struct msm_drm_private *priv;
> -	int ret;
>  	unsigned long flags;
>  
>  	if (!crtc || !crtc->dev || !crtc->dev->dev_private || !crtc->state) {
> @@ -996,10 +979,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
>  	trace_dpu_crtc_disable(DRMID(crtc), false, dpu_crtc);
>  	if (dpu_crtc->enabled && !dpu_crtc->suspend &&
>  			dpu_crtc->vblank_requested) {
> -		ret = _dpu_crtc_vblank_enable_no_lock(dpu_crtc, false);
> -		if (ret)
> -			DPU_ERROR("%s vblank enable failed: %d\n",
> -					dpu_crtc->name, ret);
> +		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, false);
>  	}
>  	dpu_crtc->enabled = false;
>  
> @@ -1045,7 +1025,6 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
>  	struct dpu_crtc *dpu_crtc;
>  	struct drm_encoder *encoder;
>  	struct msm_drm_private *priv;
> -	int ret;
>  
>  	if (!crtc || !crtc->dev || !crtc->dev->dev_private) {
>  		DPU_ERROR("invalid crtc\n");
> @@ -1067,10 +1046,7 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
>  	trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
>  	if (!dpu_crtc->enabled && !dpu_crtc->suspend &&
>  			dpu_crtc->vblank_requested) {
> -		ret = _dpu_crtc_vblank_enable_no_lock(dpu_crtc, true);
> -		if (ret)
> -			DPU_ERROR("%s vblank enable failed: %d\n",
> -					dpu_crtc->name, ret);
> +		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, true);
>  	}
>  	dpu_crtc->enabled = true;
>  
> @@ -1325,7 +1301,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>  int dpu_crtc_vblank(struct drm_crtc *crtc, bool en)
>  {
>  	struct dpu_crtc *dpu_crtc;
> -	int ret;
>  
>  	if (!crtc) {
>  		DPU_ERROR("invalid crtc\n");
> @@ -1336,10 +1311,7 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool en)
>  	mutex_lock(&dpu_crtc->crtc_lock);
>  	trace_dpu_crtc_vblank(DRMID(&dpu_crtc->base), en, dpu_crtc);
>  	if (dpu_crtc->enabled && !dpu_crtc->suspend) {
> -		ret = _dpu_crtc_vblank_enable_no_lock(dpu_crtc, en);
> -		if (ret)
> -			DPU_ERROR("%s vblank enable failed: %d\n",
> -					dpu_crtc->name, ret);
> +		_dpu_crtc_vblank_enable_no_lock(dpu_crtc, en);
>  	}
>  	dpu_crtc->vblank_requested = en;
>  	mutex_unlock(&dpu_crtc->crtc_lock);
> -- 
> 2.19.0.444.g18242da7ef-goog
> 
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno

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

Re: [PATCH v2 6/7] drm/msm/dpu: Make dpu_plane_danger_signal_ctrl void

[Sean Paul]

On Thu, Sep 20, 2018 at 11:38:54AM -0600, Jordan Crouse wrote:
> On Thu, Sep 20, 2018 at 12:49:23PM -0400, Bruce Wang wrote:
> > Removed all impossible checks from the function, which eliminates
> > the need for a return value. This function is also never used
> > outside of dpu_plane.c, so the function is made static.
> > 
> > Signed-off-by: Bruce Wang <bzwang@chromium.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 27 +++++------------------
> >  1 file changed, 5 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > index 9a5d5afa53f2..f9e65acdf87e 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > @@ -363,35 +363,18 @@ static void _dpu_plane_set_qos_ctrl(struct drm_plane *plane,
> >  			&pdpu->pipe_qos_cfg);
> >  }
> >  
> > -int dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
> > +static void dpu_plane_danger_signal_ctrl(struct drm_plane *plane, bool enable)
> >  {
> > -	struct dpu_plane *pdpu;
> > -	struct msm_drm_private *priv;
> > -	struct dpu_kms *dpu_kms;
> > -
> > -	if (!plane || !plane->dev) {
> > -		DPU_ERROR("invalid arguments\n");
> > -		return -EINVAL;
> > -	}
> > -
> > -	priv = plane->dev->dev_private;
> > -	if (!priv || !priv->kms) {
> > -		DPU_ERROR("invalid KMS reference\n");
> > -		return -EINVAL;
> > -	}
> > -
> > -	dpu_kms = to_dpu_kms(priv->kms);
> > -	pdpu = to_dpu_plane(plane);
> > +	struct dpu_plane *pdpu = to_dpu_plane(plane);
> > +	struct msm_drm_private *priv = plane->dev->dev_private;
> > +	struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
> 
> I think _dpu_plane_get_kms() can be used here.

Nice catch on the _dpu_blah_get_kms() funcs :-). With that change

Reviewed-by: Sean Paul <seanpaul@chromium.org>


> 
> >  	if (!pdpu->is_rt_pipe)
> > -		goto end;
> > +		return;
> >  
> >  	pm_runtime_get_sync(&dpu_kms->pdev->dev);
> >  	_dpu_plane_set_qos_ctrl(plane, enable, DPU_PLANE_QOS_PANIC_CTRL);
> >  	pm_runtime_put_sync(&dpu_kms->pdev->dev);
> > -
> > -end:
> > -	return 0;
> >  }
> >  
> >  /**
> > -- 
> > 2.19.0.444.g18242da7ef-goog
> > 
> > _______________________________________________
> > Freedreno mailing list
> > Freedreno@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/freedreno
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno

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

Re: [PATCH v2 7/7] drm/msm/dpu: Make _dpu_plane_get_aspace void

[Sean Paul]

On Thu, Sep 20, 2018 at 12:49:24PM -0400, Bruce Wang wrote:
> Remove unneeded checks from _dpu_plane_get_aspace. The function
> no longer needs to return anything so it is changed to void.
> 
> Signed-off-by: Bruce Wang <bzwang@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 25 ++++-------------------
>  1 file changed, 4 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index f9e65acdf87e..59f019685658 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -434,23 +434,14 @@ static void _dpu_plane_set_qos_remap(struct drm_plane *plane)
>  /**
>   * _dpu_plane_get_aspace: gets the address space
>   */
> -static int _dpu_plane_get_aspace(
> +static void _dpu_plane_get_aspace(
>  		struct dpu_plane *pdpu,
>  		struct dpu_plane_state *pstate,
>  		struct msm_gem_address_space **aspace)
>  {
> -	struct dpu_kms *kms;
> -
> -	if (!pdpu || !pstate || !aspace) {
> -		DPU_ERROR("invalid parameters\n");
> -		return -EINVAL;
> -	}
> -
> -	kms = _dpu_plane_get_kms(&pdpu->base);
> +	struct dpu_kms *kms = _dpu_plane_get_kms(&pdpu->base);
>  
>  	*aspace = kms->base.aspace;

I think we could make this better by one of two ways.

1- Just use aspace = _dpu_plane_get_kms(&pdpu->base)->base.aspace; at the
   callsites.

2- Make the return value struct msm_gem_address_space * and return the value
   from the function.

Sean

> -
> -	return 0;
>  }
>  
>  static inline void _dpu_plane_set_scanout(struct drm_plane *plane,
> @@ -471,11 +462,7 @@ static inline void _dpu_plane_set_scanout(struct drm_plane *plane,
>  
>  	pdpu = to_dpu_plane(plane);
>  
> -	ret = _dpu_plane_get_aspace(pdpu, pstate, &aspace);
> -	if (ret) {
> -		DPU_ERROR_PLANE(pdpu, "Failed to get aspace %d\n", ret);
> -		return;
> -	}
> +	_dpu_plane_get_aspace(pdpu, pstate, &aspace);
>  
>  	ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout);
>  	if (ret == -EAGAIN)
> @@ -836,11 +823,7 @@ static int dpu_plane_prepare_fb(struct drm_plane *plane,
>  
>  	DPU_DEBUG_PLANE(pdpu, "FB[%u]\n", fb->base.id);
>  
> -	ret = _dpu_plane_get_aspace(pdpu, pstate, &aspace);
> -	if (ret) {
> -		DPU_ERROR_PLANE(pdpu, "Failed to get aspace\n");
> -		return ret;
> -	}
> +	_dpu_plane_get_aspace(pdpu, pstate, &aspace);
>  
>  	/* cache aspace */
>  	pstate->aspace = aspace;
> -- 
> 2.19.0.444.g18242da7ef-goog
> 
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno

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

Re: [PATCH v2 4/7] drm/msm/dpu: Change _dpu_crtc_power_enable to void

[Bruce Wang]

On Fri, Sep 21, 2018 at 11:51 AM, Sean Paul <sean@poorly.run> wrote:
>
> On Thu, Sep 20, 2018 at 11:36:34AM -0600, Jordan Crouse wrote:
> > On Thu, Sep 20, 2018 at 12:49:21PM -0400, Bruce Wang wrote:
> > > All checks for _dpu_crtc_power_enable are not true, so the function
> > > can never return an error code. All calls of the function have also
> > > been changed so that they don't expect a return value.
> > >
> > > Signed-off-by: Bruce Wang <bzwang@chromium.org>
> > > ---
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 35 ++++--------------------
> > >  1 file changed, 5 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > index a9adb16eac6f..648d77c41c3e 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > @@ -59,37 +59,16 @@ static inline struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc)
> > >                     ->kms);
> > >  }
> > >
> > > -static inline int _dpu_crtc_power_enable(struct dpu_crtc *dpu_crtc, bool enable)
> > > +static inline void _dpu_crtc_power_enable(struct dpu_crtc *dpu_crtc, bool enable)
> >
> > Since this is only two lines of code with the optimization below my personal
> > preference would be to turn this into two functions: _dpu_crtc_power_enable and
> > _dpu_crtc_power_disable. I find that to be more readable when walking through
> > the functional code.
>
> ...or just call the pm_runtime functions directly? If we can use
> _dpu_crtc_get_kms() it shouldn't be too nasty.
>
> Sean
>

It feels a little more clear to me with the two separate
_dpu_crtc_power_enable and
_dpu_crtc_power_disable functions. Sending out another patch set with
that change.

> >
> > >  {
> > > -   struct drm_crtc *crtc;
> > > -   struct msm_drm_private *priv;
> > > -   struct dpu_kms *dpu_kms;
> > > -
> > > -   if (!dpu_crtc) {
> > > -           DPU_ERROR("invalid dpu crtc\n");
> > > -           return -EINVAL;
> > > -   }
> > > -
> > > -   crtc = &dpu_crtc->base;
> > > -   if (!crtc->dev || !crtc->dev->dev_private) {
> > > -           DPU_ERROR("invalid drm device\n");
> > > -           return -EINVAL;
> > > -   }
> > > -
> > > -   priv = crtc->dev->dev_private;
> > > -   if (!priv->kms) {
> > > -           DPU_ERROR("invalid kms\n");
> > > -           return -EINVAL;
> > > -   }
> > > -
> > > -   dpu_kms = to_dpu_kms(priv->kms);
> > > +   struct drm_crtc *crtc = &dpu_crtc->base;
> > > +   struct msm_drm_private *priv = crtc->dev->dev_private;
> > > +   struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
> >
> > I think all three of these can be replaced by
> >
> > struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(&dpu_crtc->base);
> >
> > >     if (enable)
> > >             pm_runtime_get_sync(&dpu_kms->pdev->dev);
> > >     else
> > >             pm_runtime_put_sync(&dpu_kms->pdev->dev);
> > > -
> > > -   return 0;
> > >  }
> > >
> > >  static void dpu_crtc_destroy(struct drm_crtc *crtc)
> > > @@ -822,14 +801,10 @@ static int _dpu_crtc_vblank_enable_no_lock(
> > >     dev = crtc->dev;
> > >
> > >     if (enable) {
> > > -           int ret;
> > > -
> > >             /* drop lock since power crtc cb may try to re-acquire lock */
> > >             mutex_unlock(&dpu_crtc->crtc_lock);
> > > -           ret = _dpu_crtc_power_enable(dpu_crtc, true);
> > > +           _dpu_crtc_power_enable(dpu_crtc, true);
> > >             mutex_lock(&dpu_crtc->crtc_lock);
> > > -           if (ret)
> > > -                   return ret;
> > >
> > >             list_for_each_entry(enc, &dev->mode_config.encoder_list, head) {
> > >                     if (enc->crtc != crtc)
> > > --
> > > 2.19.0.444.g18242da7ef-goog
> > >
> > > _______________________________________________
> > > Freedreno mailing list
> > > Freedreno@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/freedreno
> >
> > --
> > The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > a Linux Foundation Collaborative Project
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno