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

[Rob Clark]

On Sat, Oct 15, 2016 at 5:39 AM, Archit Taneja  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 
>> ---
>> 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, [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())'

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
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Archit Taneja]

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 
---
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, [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.

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
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Ville Syrjälä]

On Thu, Oct 13, 2016 at 12:48:46PM -0400, 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 
> ---
> 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);
> +}

And what if the plane is larger than the screen size while covering it
fully?

> +
>  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, [0].state->base))
> + base++;
> +
> + /* 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);
> -- 
> 2.7.4
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Archit Taneja]

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.


The patch looks good. I couldn't test the problematic scenario since I
don't have an easy way to reproduce it, but it doesn't break regular
usage (where base layer is full screen).

Reviewed-by: Archit Taneja 



Signed-off-by: Rob Clark 
---
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, [0].state->base))
+   base++;
+
+   /* 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
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno