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

[Archit Taneja <architt-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On 10/15/2016 6:38 PM, Rob Clark wrote:
> On Sat, Oct 15, 2016 at 8:57 AM, Archit Taneja <architt@codeaurora.org> wrote:
>>
>>
>> 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.
>
> hmm, yeah, you are right.. I wonder how that wasn't already a
> problem..  I guess we at least always have an outgoing plane?  I guess
> changing the check to '(cnt > 0)' instead of 'pstates[0].state' would
> do the trick..
>
> fwiw, I pushed current version of this and the other patches to:
>
> https://github.com/freedreno/kernel-msm/commits/integration-linux-qcomlt-fences-2

thanks! I'll start using these patches too.

Archit

>
> BR,
> -R
>
>> 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

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