| Message ID | 20180924174555.11494-1-bzwang@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | None | expand |
On Mon, Sep 24, 2018 at 01:45:55PM -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. > > v3: change _dpu_plane_get_aspace to return a struct > *msm_gem_address_space instead passing in a pointer of the same > type to edit. Thats the stuff... :) > Signed-off-by: Bruce Wang <bzwang@chromium.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 35 ++++------------------- > 1 file changed, 6 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > index 924e321a5ac4..02c27240d38f 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > @@ -431,23 +431,12 @@ 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( > - struct dpu_plane *pdpu, > - struct dpu_plane_state *pstate, > - struct msm_gem_address_space **aspace) > +static inline struct msm_gem_address_space *_dpu_plane_get_aspace( > + struct dpu_plane *pdpu) > { > - 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; > + return kms->base.aspace; > } > > static inline void _dpu_plane_set_scanout(struct drm_plane *plane, > @@ -456,15 +445,9 @@ static inline void _dpu_plane_set_scanout(struct drm_plane *plane, > struct drm_framebuffer *fb) > { > struct dpu_plane *pdpu = to_dpu_plane(plane); > - struct msm_gem_address_space *aspace = NULL; > + struct msm_gem_address_space *aspace = _dpu_plane_get_aspace(pdpu); > int ret; > > - ret = _dpu_plane_get_aspace(pdpu, pstate, &aspace); > - if (ret) { > - DPU_ERROR_PLANE(pdpu, "Failed to get aspace %d\n", ret); > - return; > - } > - > ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout); > if (ret == -EAGAIN) > DPU_DEBUG_PLANE(pdpu, "not updating same src addrs\n"); > @@ -816,7 +799,7 @@ static int dpu_plane_prepare_fb(struct drm_plane *plane, > struct drm_gem_object *obj; > struct msm_gem_object *msm_obj; > struct dma_fence *fence; > - struct msm_gem_address_space *aspace; > + struct msm_gem_address_space *aspace = _dpu_plane_get_aspace(pdpu); > int ret; > > if (!new_state->fb) > @@ -824,12 +807,6 @@ 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; > - } > - > /* cache aspace */ > pstate->aspace = aspace; Nit - aspace is only used to assign pstate->aspace so you can skip the middleman and assign it directly. That said it appears that outside of this function pstate->aspace is only used in dpu_plane_cleanup_fb and I'm not sure why we couldn't just use _dpu_plane_get_aspace() at that point and save storing the aspace in pstate. But don't let that derail an otherwise great patch. Reviewed-by: Jordan Crouse <jcrouse@codeaurora.org>
On 2018-09-24 10:45, 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. > Are the subject and text still appropriate for the patch? The function is returning a ptr to msm_gem_address_space. > v3: change _dpu_plane_get_aspace to return a struct > *msm_gem_address_space instead passing in a pointer of the same > type to edit. > > Signed-off-by: Bruce Wang <bzwang@chromium.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 35 ++++------------------- > 1 file changed, 6 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > index 924e321a5ac4..02c27240d38f 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > @@ -431,23 +431,12 @@ 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( > - struct dpu_plane *pdpu, > - struct dpu_plane_state *pstate, > - struct msm_gem_address_space **aspace) > +static inline struct msm_gem_address_space *_dpu_plane_get_aspace( > + struct dpu_plane *pdpu) > { > - 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; > + return kms->base.aspace; > } > > static inline void _dpu_plane_set_scanout(struct drm_plane *plane, > @@ -456,15 +445,9 @@ static inline void _dpu_plane_set_scanout(struct > drm_plane *plane, > struct drm_framebuffer *fb) > { > struct dpu_plane *pdpu = to_dpu_plane(plane); > - struct msm_gem_address_space *aspace = NULL; > + struct msm_gem_address_space *aspace = > _dpu_plane_get_aspace(pdpu); > int ret; > > - ret = _dpu_plane_get_aspace(pdpu, pstate, &aspace); > - if (ret) { > - DPU_ERROR_PLANE(pdpu, "Failed to get aspace %d\n", ret); > - return; > - } > - > ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout); > if (ret == -EAGAIN) > DPU_DEBUG_PLANE(pdpu, "not updating same src addrs\n"); > @@ -816,7 +799,7 @@ static int dpu_plane_prepare_fb(struct drm_plane > *plane, > struct drm_gem_object *obj; > struct msm_gem_object *msm_obj; > struct dma_fence *fence; > - struct msm_gem_address_space *aspace; > + struct msm_gem_address_space *aspace = > _dpu_plane_get_aspace(pdpu); > int ret; > > if (!new_state->fb) > @@ -824,12 +807,6 @@ 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; > - } > - > /* cache aspace */ > pstate->aspace = aspace;
On Mon, Sep 24, 2018 at 2:01 PM Jeykumar Sankaran <jsanka@codeaurora.org> wrote: > > On 2018-09-24 10:45, 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. > > > Are the subject and text still appropriate for the patch? The function > is returning > a ptr to msm_gem_address_space. > Good point! Will fix > > v3: change _dpu_plane_get_aspace to return a struct > > *msm_gem_address_space instead passing in a pointer of the same > > type to edit. > > > > Signed-off-by: Bruce Wang <bzwang@chromium.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 35 ++++------------------- > > 1 file changed, 6 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > index 924e321a5ac4..02c27240d38f 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > @@ -431,23 +431,12 @@ 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( > > - struct dpu_plane *pdpu, > > - struct dpu_plane_state *pstate, > > - struct msm_gem_address_space **aspace) > > +static inline struct msm_gem_address_space *_dpu_plane_get_aspace( > > + struct dpu_plane *pdpu) > > { > > - 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; > > + return kms->base.aspace; > > } > > > > static inline void _dpu_plane_set_scanout(struct drm_plane *plane, > > @@ -456,15 +445,9 @@ static inline void _dpu_plane_set_scanout(struct > > drm_plane *plane, > > struct drm_framebuffer *fb) > > { > > struct dpu_plane *pdpu = to_dpu_plane(plane); > > - struct msm_gem_address_space *aspace = NULL; > > + struct msm_gem_address_space *aspace = > > _dpu_plane_get_aspace(pdpu); > > int ret; > > > > - ret = _dpu_plane_get_aspace(pdpu, pstate, &aspace); > > - if (ret) { > > - DPU_ERROR_PLANE(pdpu, "Failed to get aspace %d\n", ret); > > - return; > > - } > > - > > ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout); > > if (ret == -EAGAIN) > > DPU_DEBUG_PLANE(pdpu, "not updating same src addrs\n"); > > @@ -816,7 +799,7 @@ static int dpu_plane_prepare_fb(struct drm_plane > > *plane, > > struct drm_gem_object *obj; > > struct msm_gem_object *msm_obj; > > struct dma_fence *fence; > > - struct msm_gem_address_space *aspace; > > + struct msm_gem_address_space *aspace = > > _dpu_plane_get_aspace(pdpu); > > int ret; > > > > if (!new_state->fb) > > @@ -824,12 +807,6 @@ 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; > > - } > > - > > /* cache aspace */ > > pstate->aspace = aspace; > > -- > Jeykumar S
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index 924e321a5ac4..02c27240d38f 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -431,23 +431,12 @@ 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( - struct dpu_plane *pdpu, - struct dpu_plane_state *pstate, - struct msm_gem_address_space **aspace) +static inline struct msm_gem_address_space *_dpu_plane_get_aspace( + struct dpu_plane *pdpu) { - 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; + return kms->base.aspace; } static inline void _dpu_plane_set_scanout(struct drm_plane *plane, @@ -456,15 +445,9 @@ static inline void _dpu_plane_set_scanout(struct drm_plane *plane, struct drm_framebuffer *fb) { struct dpu_plane *pdpu = to_dpu_plane(plane); - struct msm_gem_address_space *aspace = NULL; + struct msm_gem_address_space *aspace = _dpu_plane_get_aspace(pdpu); int ret; - ret = _dpu_plane_get_aspace(pdpu, pstate, &aspace); - if (ret) { - DPU_ERROR_PLANE(pdpu, "Failed to get aspace %d\n", ret); - return; - } - ret = dpu_format_populate_layout(aspace, fb, &pipe_cfg->layout); if (ret == -EAGAIN) DPU_DEBUG_PLANE(pdpu, "not updating same src addrs\n"); @@ -816,7 +799,7 @@ static int dpu_plane_prepare_fb(struct drm_plane *plane, struct drm_gem_object *obj; struct msm_gem_object *msm_obj; struct dma_fence *fence; - struct msm_gem_address_space *aspace; + struct msm_gem_address_space *aspace = _dpu_plane_get_aspace(pdpu); int ret; if (!new_state->fb) @@ -824,12 +807,6 @@ 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; - } - /* cache aspace */ pstate->aspace = aspace;
Remove unneeded checks from _dpu_plane_get_aspace. The function no longer needs to return anything so it is changed to void. v3: change _dpu_plane_get_aspace to return a struct *msm_gem_address_space instead passing in a pointer of the same type to edit. Signed-off-by: Bruce Wang <bzwang@chromium.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 35 ++++------------------- 1 file changed, 6 insertions(+), 29 deletions(-)