| Message ID | 20230310005704.1332368-17-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dpu: wide planes support | expand |
On 3/9/2023 4:56 PM, Dmitry Baryshkov wrote: > Move plane state updates from dpu_crtc_atomic_check() to the function > where they belong: to dpu_plane_atomic_check(). > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Can you please elaborate a bit on this comment? https://patchwork.freedesktop.org/patch/521356/?series=99909&rev=4#comment_949772 Is something still missing? I dont see the multirects being cleared in dpu_plane_atomic_disable() before or even now. > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 18 +----------------- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 18 ++++++++++-------- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 6 ------ > 3 files changed, 11 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index 0260c4d6ded7..bd09bb319a58 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -1129,7 +1129,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > crtc); > struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); > struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); > - struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); > > const struct drm_plane_state *pstate; > struct drm_plane *plane; > @@ -1161,11 +1160,10 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > crtc_rect.x2 = mode->hdisplay; > crtc_rect.y2 = mode->vdisplay; > > - /* get plane state for all drm planes associated with crtc state */ > + /* FIXME: move this to dpu_plane_atomic_check? */ > drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) { > struct dpu_plane_state *dpu_pstate = to_dpu_plane_state(pstate); > struct drm_rect dst, clip = crtc_rect; > - int stage; > > if (IS_ERR_OR_NULL(pstate)) { > rc = PTR_ERR(pstate); > @@ -1179,8 +1177,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > > dpu_pstate->needs_dirtyfb = needs_dirtyfb; > > - dpu_plane_clear_multirect(pstate); > - > dst = drm_plane_state_dest(pstate); > if (!drm_rect_intersect(&clip, &dst)) { > DPU_ERROR("invalid vertical/horizontal destination\n"); > @@ -1189,18 +1185,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > DRM_RECT_ARG(&dst)); > return -E2BIG; > } > - > - /* verify stage setting before using it */ > - stage = DPU_STAGE_0 + pstate->normalized_zpos; > - if (stage >= dpu_kms->catalog->caps->max_mixer_blendstages) { > - DPU_ERROR("> %d plane stages assigned\n", > - dpu_kms->catalog->caps->max_mixer_blendstages - DPU_STAGE_0); > - return -EINVAL; > - } > - > - to_dpu_plane_state(pstate)->stage = stage; > - DRM_DEBUG_ATOMIC("%s: stage %d\n", dpu_crtc->name, stage); > - > } > > atomic_inc(&_dpu_crtc_get_kms(crtc)->bandwidth_ref); > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > index ce01a602cbc9..3fba63fbbd78 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > @@ -733,14 +733,6 @@ static int _dpu_plane_color_fill(struct dpu_plane *pdpu, > return 0; > } > > -void dpu_plane_clear_multirect(const struct drm_plane_state *drm_state) > -{ > - struct dpu_plane_state *pstate = to_dpu_plane_state(drm_state); > - > - pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO; > - pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE; > -} > - > int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane) > { > struct dpu_plane_state *pstate[R_MAX]; > @@ -994,6 +986,16 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, > if (!new_plane_state->visible) > return 0; > > + pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO; > + pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE; > + > + pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos; > + if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) { > + DPU_ERROR("> %d plane stages assigned\n", > + pdpu->catalog->caps->max_mixer_blendstages - DPU_STAGE_0); > + return -EINVAL; > + } > + > src.x1 = new_plane_state->src_x >> 16; > src.y1 = new_plane_state->src_y >> 16; > src.x2 = src.x1 + (new_plane_state->src_w >> 16); > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h > index 228db401e905..a08b0539513b 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h > @@ -88,12 +88,6 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev, > */ > int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane); > > -/** > - * dpu_plane_clear_multirect - clear multirect bits for the given pipe > - * @drm_state: Pointer to DRM plane state > - */ > -void dpu_plane_clear_multirect(const struct drm_plane_state *drm_state); > - > /** > * dpu_plane_color_fill - enables color fill on plane > * @plane: Pointer to DRM plane object
On 14/03/2023 06:02, Abhinav Kumar wrote: > > > On 3/9/2023 4:56 PM, Dmitry Baryshkov wrote: >> Move plane state updates from dpu_crtc_atomic_check() to the function >> where they belong: to dpu_plane_atomic_check(). >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Can you please elaborate a bit on this comment? > > https://patchwork.freedesktop.org/patch/521356/?series=99909&rev=4#comment_949772 > > Is something still missing? I dont see the multirects being cleared in > dpu_plane_atomic_disable() before or even now. It is done in the last patch, where it belongs. There is no need to clear multirect setup until we support RECT_1. > >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 18 +----------------- >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 18 ++++++++++-------- >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 6 ------ >> 3 files changed, 11 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> index 0260c4d6ded7..bd09bb319a58 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> @@ -1129,7 +1129,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc >> *crtc, >> crtc); >> struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); >> struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); >> - struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); >> const struct drm_plane_state *pstate; >> struct drm_plane *plane; >> @@ -1161,11 +1160,10 @@ static int dpu_crtc_atomic_check(struct >> drm_crtc *crtc, >> crtc_rect.x2 = mode->hdisplay; >> crtc_rect.y2 = mode->vdisplay; >> - /* get plane state for all drm planes associated with crtc state */ >> + /* FIXME: move this to dpu_plane_atomic_check? */ >> drm_atomic_crtc_state_for_each_plane_state(plane, pstate, >> crtc_state) { >> struct dpu_plane_state *dpu_pstate = >> to_dpu_plane_state(pstate); >> struct drm_rect dst, clip = crtc_rect; >> - int stage; >> if (IS_ERR_OR_NULL(pstate)) { >> rc = PTR_ERR(pstate); >> @@ -1179,8 +1177,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc >> *crtc, >> dpu_pstate->needs_dirtyfb = needs_dirtyfb; >> - dpu_plane_clear_multirect(pstate); >> - >> dst = drm_plane_state_dest(pstate); >> if (!drm_rect_intersect(&clip, &dst)) { >> DPU_ERROR("invalid vertical/horizontal destination\n"); >> @@ -1189,18 +1185,6 @@ static int dpu_crtc_atomic_check(struct >> drm_crtc *crtc, >> DRM_RECT_ARG(&dst)); >> return -E2BIG; >> } >> - >> - /* verify stage setting before using it */ >> - stage = DPU_STAGE_0 + pstate->normalized_zpos; >> - if (stage >= dpu_kms->catalog->caps->max_mixer_blendstages) { >> - DPU_ERROR("> %d plane stages assigned\n", >> - dpu_kms->catalog->caps->max_mixer_blendstages - >> DPU_STAGE_0); >> - return -EINVAL; >> - } >> - >> - to_dpu_plane_state(pstate)->stage = stage; >> - DRM_DEBUG_ATOMIC("%s: stage %d\n", dpu_crtc->name, stage); >> - >> } >> atomic_inc(&_dpu_crtc_get_kms(crtc)->bandwidth_ref); >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> index ce01a602cbc9..3fba63fbbd78 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> @@ -733,14 +733,6 @@ static int _dpu_plane_color_fill(struct dpu_plane >> *pdpu, >> return 0; >> } >> -void dpu_plane_clear_multirect(const struct drm_plane_state *drm_state) >> -{ >> - struct dpu_plane_state *pstate = to_dpu_plane_state(drm_state); >> - >> - pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO; >> - pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE; >> -} >> - >> int dpu_plane_validate_multirect_v2(struct >> dpu_multirect_plane_states *plane) >> { >> struct dpu_plane_state *pstate[R_MAX]; >> @@ -994,6 +986,16 @@ static int dpu_plane_atomic_check(struct >> drm_plane *plane, >> if (!new_plane_state->visible) >> return 0; >> + pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO; >> + pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE; >> + >> + pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos; >> + if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) { >> + DPU_ERROR("> %d plane stages assigned\n", >> + pdpu->catalog->caps->max_mixer_blendstages - DPU_STAGE_0); >> + return -EINVAL; >> + } >> + >> src.x1 = new_plane_state->src_x >> 16; >> src.y1 = new_plane_state->src_y >> 16; >> src.x2 = src.x1 + (new_plane_state->src_w >> 16); >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h >> index 228db401e905..a08b0539513b 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h >> @@ -88,12 +88,6 @@ struct drm_plane *dpu_plane_init(struct drm_device >> *dev, >> */ >> int dpu_plane_validate_multirect_v2(struct >> dpu_multirect_plane_states *plane); >> -/** >> - * dpu_plane_clear_multirect - clear multirect bits for the given pipe >> - * @drm_state: Pointer to DRM plane state >> - */ >> -void dpu_plane_clear_multirect(const struct drm_plane_state *drm_state); >> - >> /** >> * dpu_plane_color_fill - enables color fill on plane >> * @plane: Pointer to DRM plane object
On 3/13/2023 9:14 PM, Dmitry Baryshkov wrote: > On 14/03/2023 06:02, Abhinav Kumar wrote: >> >> >> On 3/9/2023 4:56 PM, Dmitry Baryshkov wrote: >>> Move plane state updates from dpu_crtc_atomic_check() to the function >>> where they belong: to dpu_plane_atomic_check(). >>> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> >> Can you please elaborate a bit on this comment? >> >> https://patchwork.freedesktop.org/patch/521356/?series=99909&rev=4#comment_949772 >> >> >> Is something still missing? I dont see the multirects being cleared in >> dpu_plane_atomic_disable() before or even now. > > It is done in the last patch, where it belongs. There is no need to > clear multirect setup until we support RECT_1. > Yup, checked the relevant patch (it wasnt the last). Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 0260c4d6ded7..bd09bb319a58 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1129,7 +1129,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, crtc); struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); - struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); const struct drm_plane_state *pstate; struct drm_plane *plane; @@ -1161,11 +1160,10 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, crtc_rect.x2 = mode->hdisplay; crtc_rect.y2 = mode->vdisplay; - /* get plane state for all drm planes associated with crtc state */ + /* FIXME: move this to dpu_plane_atomic_check? */ drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) { struct dpu_plane_state *dpu_pstate = to_dpu_plane_state(pstate); struct drm_rect dst, clip = crtc_rect; - int stage; if (IS_ERR_OR_NULL(pstate)) { rc = PTR_ERR(pstate); @@ -1179,8 +1177,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, dpu_pstate->needs_dirtyfb = needs_dirtyfb; - dpu_plane_clear_multirect(pstate); - dst = drm_plane_state_dest(pstate); if (!drm_rect_intersect(&clip, &dst)) { DPU_ERROR("invalid vertical/horizontal destination\n"); @@ -1189,18 +1185,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, DRM_RECT_ARG(&dst)); return -E2BIG; } - - /* verify stage setting before using it */ - stage = DPU_STAGE_0 + pstate->normalized_zpos; - if (stage >= dpu_kms->catalog->caps->max_mixer_blendstages) { - DPU_ERROR("> %d plane stages assigned\n", - dpu_kms->catalog->caps->max_mixer_blendstages - DPU_STAGE_0); - return -EINVAL; - } - - to_dpu_plane_state(pstate)->stage = stage; - DRM_DEBUG_ATOMIC("%s: stage %d\n", dpu_crtc->name, stage); - } atomic_inc(&_dpu_crtc_get_kms(crtc)->bandwidth_ref); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index ce01a602cbc9..3fba63fbbd78 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -733,14 +733,6 @@ static int _dpu_plane_color_fill(struct dpu_plane *pdpu, return 0; } -void dpu_plane_clear_multirect(const struct drm_plane_state *drm_state) -{ - struct dpu_plane_state *pstate = to_dpu_plane_state(drm_state); - - pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO; - pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE; -} - int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane) { struct dpu_plane_state *pstate[R_MAX]; @@ -994,6 +986,16 @@ static int dpu_plane_atomic_check(struct drm_plane *plane, if (!new_plane_state->visible) return 0; + pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO; + pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE; + + pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos; + if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) { + DPU_ERROR("> %d plane stages assigned\n", + pdpu->catalog->caps->max_mixer_blendstages - DPU_STAGE_0); + return -EINVAL; + } + src.x1 = new_plane_state->src_x >> 16; src.y1 = new_plane_state->src_y >> 16; src.x2 = src.x1 + (new_plane_state->src_w >> 16); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h index 228db401e905..a08b0539513b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h @@ -88,12 +88,6 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev, */ int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane); -/** - * dpu_plane_clear_multirect - clear multirect bits for the given pipe - * @drm_state: Pointer to DRM plane state - */ -void dpu_plane_clear_multirect(const struct drm_plane_state *drm_state); - /** * dpu_plane_color_fill - enables color fill on plane * @plane: Pointer to DRM plane object
Move plane state updates from dpu_crtc_atomic_check() to the function where they belong: to dpu_plane_atomic_check(). Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 18 +----------------- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 18 ++++++++++-------- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 6 ------ 3 files changed, 11 insertions(+), 31 deletions(-)