[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
Wed Feb 15 00:08:13 UTC 2023
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 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?
>>
>
> 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
>>
--
With best wishes
Dmitry
More information about the dri-devel
mailing list