| Message ID | 20180920164924.225847-5-bzwang@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | drm/msm/dpu: Clean up dpu code | expand |
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
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
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
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)
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(-)