| Message ID | 20230203182132.1307834-16-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dpu: wide planes support | expand |
On 2/3/2023 10:21 AM, 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> > --- > 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 b485234eefb2..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 1b3033b15bfa..5aabf9694a53 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; > + But I am not sure if clearing the multirect belongs here and now I want to clarify one thing about https://patchwork.freedesktop.org/patch/521354/?series=99909&rev=4 which was R-bed in the v1 and carried fwd since then. So prior to that change, we were only clearing the multirects of the planes that were staged to the crtc and we were getting those from the crtc state. But now we are clearing the multirect of all the planes. Dont we have to keep that in the crtc_atomic_check() since we do that on all the planes attached to a certain CRTC. In that case shouldnt we keep this in the crtc_atomic_check() and bring back pipe_staged[] without the multirect and source split cases ofcourse. > + 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; > + } > + I agree that this check belongs to the plane_atomic_check(). > 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 04/02/2023 00:44, Abhinav Kumar wrote: > > > On 2/3/2023 10:21 AM, 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> >> --- >> 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 b485234eefb2..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 1b3033b15bfa..5aabf9694a53 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; >> + > > But I am not sure if clearing the multirect belongs here and now I want > to clarify one thing about > https://patchwork.freedesktop.org/patch/521354/?series=99909&rev=4 which > was R-bed in the v1 and carried fwd since then. > > So prior to that change, we were only clearing the multirects of the > planes that were staged to the crtc and we were getting those from the > crtc state. But now we are clearing the multirect of all the planes. > > Dont we have to keep that in the crtc_atomic_check() since we do that on > all the planes attached to a certain CRTC. > > In that case shouldnt we keep this in the crtc_atomic_check() and bring > back pipe_staged[] without the multirect and source split cases ofcourse. What for? In other words, what would be the difference? > >> + 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; >> + } >> + > > I agree that this check belongs to the plane_atomic_check(). > >> 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
Hi Dmitry Sorry for the late response on this one. On 2/3/2023 2:55 PM, Dmitry Baryshkov wrote: > On 04/02/2023 00:44, Abhinav Kumar wrote: >> >> >> On 2/3/2023 10:21 AM, 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> >>> --- >>> 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 b485234eefb2..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 1b3033b15bfa..5aabf9694a53 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; >>> + >> >> But I am not sure if clearing the multirect belongs here and now I >> want to clarify one thing about >> https://patchwork.freedesktop.org/patch/521354/?series=99909&rev=4 >> which was R-bed in the v1 and carried fwd since then. >> >> So prior to that change, we were only clearing the multirects of the >> planes that were staged to the crtc and we were getting those from the >> crtc state. But now we are clearing the multirect of all the planes. >> >> Dont we have to keep that in the crtc_atomic_check() since we do that >> on all the planes attached to a certain CRTC. >> >> In that case shouldnt we keep this in the crtc_atomic_check() and >> bring back pipe_staged[] without the multirect and source split cases >> ofcourse. > > What for? In other words, what would be the difference? > So, please correct my understanding here. drm_plane's atomic_check() will be called for all the planes which are getting updated in this atomic commit using for_each_oldnew_plane_in_state() and drm_crtc's atomic_check() will be called for all the CRTC's in this atomic update using for_each_new_crtc_in_state(). If the plane is not connected to any CRTC, why do we need to clear the multirect pstates. OR in that case would atomic_commit not even be called if the plane is not connected to any CRTC? One case i can think of is the disable commit where the no planes will be connected to the CRTC so in that case, before this change we would explicitly clear out all the planes connected to the CRTC but now with this change is there a possibility that only if the plane state changed we would clear it out? >> >>> + 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; >>> + } >>> + >> >> I agree that this check belongs to the plane_atomic_check(). >> >>> 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 15/02/2023 01:25, Abhinav Kumar wrote: > Hi Dmitry > > Sorry for the late response on this one. > > On 2/3/2023 2:55 PM, Dmitry Baryshkov wrote: >> On 04/02/2023 00:44, Abhinav Kumar wrote: >>> >>> >>> On 2/3/2023 10:21 AM, 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> >>>> --- >>>> 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 b485234eefb2..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 1b3033b15bfa..5aabf9694a53 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; >>>> + >>> >>> But I am not sure if clearing the multirect belongs here and now I >>> want to clarify one thing about >>> https://patchwork.freedesktop.org/patch/521354/?series=99909&rev=4 >>> which was R-bed in the v1 and carried fwd since then. >>> >>> So prior to that change, we were only clearing the multirects of the >>> planes that were staged to the crtc and we were getting those from >>> the crtc state. But now we are clearing the multirect of all the planes. >>> >>> Dont we have to keep that in the crtc_atomic_check() since we do that >>> on all the planes attached to a certain CRTC. >>> >>> In that case shouldnt we keep this in the crtc_atomic_check() and >>> bring back pipe_staged[] without the multirect and source split cases >>> ofcourse. >> >> What for? In other words, what would be the difference? >> > > So, please correct my understanding here. drm_plane's atomic_check() > will be called for all the planes which are getting updated in this > atomic commit using for_each_oldnew_plane_in_state() and drm_crtc's > atomic_check() will be called for all the CRTC's in this atomic update > using for_each_new_crtc_in_state() > > If the plane is not connected to any CRTC, why do we need to clear the > multirect pstates. If the plane is not connected to any CRTC, then we just don't care what is there in the multirect state, so we might clear it as well. > > OR in that case would atomic_commit not even be called if the plane is > not connected to any CRTC? > > One case i can think of is the disable commit where the no planes will > be connected to the CRTC so in that case, before this change we would > explicitly clear out all the planes connected to the CRTC but now with > this change is there a possibility that only if the plane state changed > we would clear it out? Ah. Maybe I understand your point. I think drm_atomic_add_affected_planes() will ensure that all planes attached to CRTCs are also a part of the atomic state. Regarding the change itself. Think about encapsulation. CRTC should not care about plane's multirect state. It is a plane implementation detail. As we delve upon a path of using rect1 and then even using different SSPPs for the plane, these implementation details will change (mostly) behind CRTC's back. > > >>> >>>> + 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; >>>> + } >>>> + >>> >>> I agree that this check belongs to the plane_atomic_check(). >>> >>>> 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 15/02/2023 02:08, Dmitry Baryshkov wrote: > On 15/02/2023 01:25, Abhinav Kumar wrote: >> Hi Dmitry >> >> Sorry for the late response on this one. >> >> On 2/3/2023 2:55 PM, Dmitry Baryshkov wrote: >>> On 04/02/2023 00:44, Abhinav Kumar wrote: >>>> >>>> >>>> On 2/3/2023 10:21 AM, 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> >>>>> --- >>>>> 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 b485234eefb2..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 1b3033b15bfa..5aabf9694a53 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; >>>>> + >>>> >>>> But I am not sure if clearing the multirect belongs here and now I >>>> want to clarify one thing about >>>> https://patchwork.freedesktop.org/patch/521354/?series=99909&rev=4 >>>> which was R-bed in the v1 and carried fwd since then. >>>> >>>> So prior to that change, we were only clearing the multirects of the >>>> planes that were staged to the crtc and we were getting those from >>>> the crtc state. But now we are clearing the multirect of all the >>>> planes. >>>> >>>> Dont we have to keep that in the crtc_atomic_check() since we do >>>> that on all the planes attached to a certain CRTC. >>>> >>>> In that case shouldnt we keep this in the crtc_atomic_check() and >>>> bring back pipe_staged[] without the multirect and source split >>>> cases ofcourse. >>> >>> What for? In other words, what would be the difference? >>> >> >> So, please correct my understanding here. drm_plane's atomic_check() >> will be called for all the planes which are getting updated in this >> atomic commit using for_each_oldnew_plane_in_state() and drm_crtc's >> atomic_check() will be called for all the CRTC's in this atomic update >> using for_each_new_crtc_in_state() > >> If the plane is not connected to any CRTC, why do we need to clear the >> multirect pstates. > > If the plane is not connected to any CRTC, then we just don't care what > is there in the multirect state, so we might clear it as well. > >> >> OR in that case would atomic_commit not even be called if the plane is >> not connected to any CRTC? >> >> One case i can think of is the disable commit where the no planes will >> be connected to the CRTC so in that case, before this change we would >> explicitly clear out all the planes connected to the CRTC but now with >> this change is there a possibility that only if the plane state >> changed we would clear it out? > > Ah. Maybe I understand your point. I think > drm_atomic_add_affected_planes() will ensure that all planes attached to > CRTCs are also a part of the atomic state. Checked, it works as expected. But on the other hand, this pointed me to a possible issue in dpu_plane_atomic_disable. Probably we should drop the multirect setup there. > > Regarding the change itself. Think about encapsulation. CRTC should not > care about plane's multirect state. It is a plane implementation detail. > As we delve upon a path of using rect1 and then even using different > SSPPs for the plane, these implementation details will change (mostly) > behind CRTC's back. > >> >> >>>> >>>>> + 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; >>>>> + } >>>>> + >>>> >>>> I agree that this check belongs to the plane_atomic_check(). >>>> >>>>> 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 >>> >
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index b485234eefb2..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 1b3033b15bfa..5aabf9694a53 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(-)