[PATCH v3 15/27] drm/msm/dpu: move the rest of plane checks to dpu_plane_atomic_check()
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Fri Feb 3 22:55:44 UTC 2023
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 at 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
--
With best wishes
Dmitry
More information about the dri-devel
mailing list