Re: [PATCH] drm/msm/mdp5: handle non-fullscreen base plane case

[Archit Taneja <architt@codeaurora.org>]

On 10/15/2016 5:02 PM, Rob Clark wrote:
> On Sat, Oct 15, 2016 at 5:39 AM, Archit Taneja <architt@codeaurora.org> wrote:
>>
>> On 10/13/2016 10:18 PM, Rob Clark wrote:
>>>
>>> If the bottom-most layer is not fullscreen, we need to use the BASE
>>> mixer stage for solid fill (ie. MDP5_CTL_BLEND_OP_FLAG_BORDER_OUT).  The
>>> blend_setup() code pretty much handled this already, we just had to
>>> figure this out in _atomic_check() and assign the stages appropriately.
>>>
>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>> ---
>>> TODO mdp4 might need similar treatment?
>>>
>>>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 44
>>> ++++++++++++++++++++------------
>>>  1 file changed, 27 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
>>> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
>>> index fa2be7c..e42f62d 100644
>>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
>>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
>>> @@ -223,11 +223,6 @@ static void blend_setup(struct drm_crtc *crtc)
>>>                 plane_cnt++;
>>>         }
>>>
>>> -       /*
>>> -       * If there is no base layer, enable border color.
>>> -       * Although it's not possbile in current blend logic,
>>> -       * put it here as a reminder.
>>> -       */
>>>         if (!pstates[STAGE_BASE] && plane_cnt) {
>>>                 ctl_blend_flags |= MDP5_CTL_BLEND_OP_FLAG_BORDER_OUT;
>>>                 DBG("Border Color is enabled");
>>> @@ -365,6 +360,15 @@ static int pstate_cmp(const void *a, const void *b)
>>>         return pa->state->zpos - pb->state->zpos;
>>>  }
>>>
>>> +/* is there a helper for this? */
>>> +static bool is_fullscreen(struct drm_crtc_state *cstate,
>>> +               struct drm_plane_state *pstate)
>>> +{
>>> +       return (pstate->crtc_x == 0) && (pstate->crtc_y == 0) &&
>>> +               (pstate->crtc_w == cstate->mode.hdisplay) &&
>>> +               (pstate->crtc_h == cstate->mode.vdisplay);
>>> +}
>>> +
>>>  static int mdp5_crtc_atomic_check(struct drm_crtc *crtc,
>>>                 struct drm_crtc_state *state)
>>>  {
>>> @@ -375,21 +379,11 @@ static int mdp5_crtc_atomic_check(struct drm_crtc
>>> *crtc,
>>>         struct plane_state pstates[STAGE_MAX + 1];
>>>         const struct mdp5_cfg_hw *hw_cfg;
>>>         const struct drm_plane_state *pstate;
>>> -       int cnt = 0, i;
>>> +       int cnt = 0, base = 0, i;
>>>
>>>         DBG("%s: check", mdp5_crtc->name);
>>>
>>> -       /* verify that there are not too many planes attached to crtc
>>> -        * and that we don't have conflicting mixer stages:
>>> -        */
>>> -       hw_cfg = mdp5_cfg_get_hw_config(mdp5_kms->cfg);
>>>         drm_atomic_crtc_state_for_each_plane_state(plane, pstate, state) {
>>> -               if (cnt >= (hw_cfg->lm.nb_stages)) {
>>> -                       dev_err(dev->dev, "too many planes!\n");
>>> -                       return -EINVAL;
>>> -               }
>>> -
>>> -
>>>                 pstates[cnt].plane = plane;
>>>                 pstates[cnt].state = to_mdp5_plane_state(pstate);
>>>
>>> @@ -399,8 +393,24 @@ static int mdp5_crtc_atomic_check(struct drm_crtc
>>> *crtc,
>>>         /* assign a stage based on sorted zpos property */
>>>         sort(pstates, cnt, sizeof(pstates[0]), pstate_cmp, NULL);
>>>
>>> +       /* if the bottom-most layer is not fullscreen, we need to use
>>> +        * it for solid-color:
>>> +        */
>>> +       if (!is_fullscreen(state, &pstates[0].state->base))
>>> +               base++;
>>> +
>>
>>
>> I get a crash here when fbcon is enabled and there are no connectors
>> connected. We're trying to refer pstates[0] when there is no plane
>> connected to the crtc. I guess we could bail out much earlier if cnt
>> is 0.
>
> yeah, I hit that last night with no fbcon and killing the UI.. I've
> already fixed it up locally with an 'if (pstates[0].state &&
> !is_fullscreen())'

Okay, I guess we should probably memset pstates array to 0 in case the
stack memory has some older non NULL stuff in it.

Thanks,
Archit

>
> BR,
> -R
>
>> Archit
>>
>>> +       /* verify that there are not too many planes attached to crtc
>>> +        * and that we don't have conflicting mixer stages:
>>> +        */
>>> +       hw_cfg = mdp5_cfg_get_hw_config(mdp5_kms->cfg);
>>> +
>>> +       if ((cnt + base) >= hw_cfg->lm.nb_stages) {
>>> +               dev_err(dev->dev, "too many planes!\n");
>>> +               return -EINVAL;
>>> +       }
>>> +
>>>         for (i = 0; i < cnt; i++) {
>>> -               pstates[i].state->stage = STAGE_BASE + i;
>>> +               pstates[i].state->stage = STAGE_BASE + i + base;
>>>                 DBG("%s: assign pipe %s on stage=%d", mdp5_crtc->name,
>>>
>>> pipe2name(mdp5_plane_pipe(pstates[i].plane)),
>>>                                 pstates[i].state->stage);
>>>
>>
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel