On Wed, Sep 19, 2018 at 11:44:13AM -0400, Bruce Wang wrote:
> Removes additional impossible checks in dpu_plane.c and 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@xxxxxxxxxxxx>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 121 +++-------------------
> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 16 +--
The dpu_plane changes should probably get rolled into the dpu_plane removal
patch, then rename the subject of this patch "Remove impossible checks from
dpu_crtc".
> 2 files changed, 17 insertions(+), 120 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..017d16131dac 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -55,17 +55,7 @@ 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;
> - }
> + struct msm_drm_private *priv = crtc->dev->dev_private;
>
> return to_dpu_kms(priv->kms);
Might as well just slap the whole thing in the arg and save a local:
return to_dpu_kms(crtc->dev->dev_private->kms);
> }
> @@ -177,28 +167,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 +196,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 +236,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,7 +344,6 @@ 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;
> @@ -400,11 +366,6 @@ static void dpu_crtc_frame_event_work(struct kthread_work *work)
This is collapsed, but the if (!work) condition is also bogus.
The fevent null checks are also suspicious. Checking fevent->crtc->state
is downright dangerous since it's unlocked access. Fortunately it doesn't
look like the function is using state, so let's remove that check. Given that
you can remove a bunch more, most of these assignments can be folded up top.
> 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 +431,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 +539,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;
> @@ -819,29 +764,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 +897,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);
> @@ -1673,8 +1586,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);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index 7f6046166626..0a223ac94cfb 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);
> }
>
> @@ -514,10 +509,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;
>
> @@ -1690,11 +1681,6 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev,
> int zpos_max = DPU_ZPOS_MAX;
> int ret = -EINVAL;
>
> - if (!priv) {
> - DPU_ERROR("[%u]private data is NULL\n", pipe);
> - goto exit;
> - }
> -
> if (!priv->kms) {
This can go too
> DPU_ERROR("[%u]invalid KMS reference\n", pipe);
> goto exit;
> --
> 2.19.0.397.gdd90340f6a-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
