| Message ID | 20210705012115.4179824-17-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,01/22] drm/msm/dpu: move LUT levels out of QOS config | expand |
On 2021-07-04 18:21, Dmitry Baryshkov wrote: > Stop limiting zpos property values, we use normalized_zpos anyway. And > nothing stops userspace from assigning several planes to a single zpos > (it is a userspace bug, but the kernel is forgiving about it). Userspace assigning several planes to a single zpos was intended to identify cases where src split can be used. Downstream does not use normalized zpos, hence it did not come across as a bug but mostly as a way to identify when usermode needs src split to be enabled based on the composition strategy. We can talk about that more in the rest of the patches of this series. For this one, I only have a couple of questions: 1) Across different vendors, some have gone with limiting the zpos and some have gone with the max, so is there an issue with sticking with the max_blend_stages limit? 2) If there is no hard reason to make this change, I think its better to keep it the way it is. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 11 +---------- > 1 file changed, 1 insertion(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > index 8ed7b8f0db69..3850f2714bf3 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > @@ -44,7 +44,6 @@ > #define DPU_NAME_SIZE 12 > > #define DPU_PLANE_COLOR_FILL_FLAG BIT(31) > -#define DPU_ZPOS_MAX 255 > > /* multirect rect index */ > enum { > @@ -1374,7 +1373,6 @@ struct drm_plane *dpu_plane_init(struct > drm_device *dev, > struct dpu_plane *pdpu; > struct msm_drm_private *priv = dev->dev_private; > struct dpu_kms *kms = to_dpu_kms(priv->kms); > - int zpos_max = DPU_ZPOS_MAX; > uint32_t num_formats; > int ret = -EINVAL; > > @@ -1412,14 +1410,7 @@ struct drm_plane *dpu_plane_init(struct > drm_device *dev, > > pdpu->catalog = kms->catalog; > > - if (kms->catalog->mixer_count && > - kms->catalog->mixer[0].sblk->maxblendstages) { > - zpos_max = kms->catalog->mixer[0].sblk->maxblendstages - 1; > - if (zpos_max > DPU_STAGE_MAX - DPU_STAGE_0 - 1) > - zpos_max = DPU_STAGE_MAX - DPU_STAGE_0 - 1; > - } > - > - ret = drm_plane_create_zpos_property(plane, 0, 0, zpos_max); > + ret = drm_plane_create_zpos_property(plane, 0, 0, 255); > if (ret) > DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
On Tue, 9 Nov 2021 at 23:15, <abhinavk@codeaurora.org> wrote: > > On 2021-07-04 18:21, Dmitry Baryshkov wrote: > > Stop limiting zpos property values, we use normalized_zpos anyway. And > > nothing stops userspace from assigning several planes to a single zpos > > (it is a userspace bug, but the kernel is forgiving about it). > > Userspace assigning several planes to a single zpos was intended to > identify > cases where src split can be used. Downstream does not use normalized > zpos, > hence it did not come across as a bug but mostly as a way to identify > when > usermode needs src split to be enabled based on the composition > strategy. > > We can talk about that more in the rest of the patches of this series. > > For this one, I only have a couple of questions: > > 1) Across different vendors, some have gone with limiting the zpos and > some have gone with > the max, so is there an issue with sticking with the max_blend_stages > limit? > > 2) If there is no hard reason to make this change, I think its better to > keep it the way it is. Short answer to both questions: we want to have more planes than the max_blend_stages. So we should remove the limit. Consider this to be a unification with MDP5, which uses 255 here. > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 11 +---------- > > 1 file changed, 1 insertion(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > index 8ed7b8f0db69..3850f2714bf3 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > > @@ -44,7 +44,6 @@ > > #define DPU_NAME_SIZE 12 > > > > #define DPU_PLANE_COLOR_FILL_FLAG BIT(31) > > -#define DPU_ZPOS_MAX 255 > > > > /* multirect rect index */ > > enum { > > @@ -1374,7 +1373,6 @@ struct drm_plane *dpu_plane_init(struct > > drm_device *dev, > > struct dpu_plane *pdpu; > > struct msm_drm_private *priv = dev->dev_private; > > struct dpu_kms *kms = to_dpu_kms(priv->kms); > > - int zpos_max = DPU_ZPOS_MAX; > > uint32_t num_formats; > > int ret = -EINVAL; > > > > @@ -1412,14 +1410,7 @@ struct drm_plane *dpu_plane_init(struct > > drm_device *dev, > > > > pdpu->catalog = kms->catalog; > > > > - if (kms->catalog->mixer_count && > > - kms->catalog->mixer[0].sblk->maxblendstages) { > > - zpos_max = kms->catalog->mixer[0].sblk->maxblendstages - 1; > > - if (zpos_max > DPU_STAGE_MAX - DPU_STAGE_0 - 1) > > - zpos_max = DPU_STAGE_MAX - DPU_STAGE_0 - 1; > > - } > > - > > - ret = drm_plane_create_zpos_property(plane, 0, 0, zpos_max); > > + ret = drm_plane_create_zpos_property(plane, 0, 0, 255); > > if (ret) > > DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
On 2021-11-09 12:21, Dmitry Baryshkov wrote: > On Tue, 9 Nov 2021 at 23:15, <abhinavk@codeaurora.org> wrote: >> >> On 2021-07-04 18:21, Dmitry Baryshkov wrote: >> > Stop limiting zpos property values, we use normalized_zpos anyway. And >> > nothing stops userspace from assigning several planes to a single zpos >> > (it is a userspace bug, but the kernel is forgiving about it). >> >> Userspace assigning several planes to a single zpos was intended to >> identify >> cases where src split can be used. Downstream does not use normalized >> zpos, >> hence it did not come across as a bug but mostly as a way to identify >> when >> usermode needs src split to be enabled based on the composition >> strategy. >> >> We can talk about that more in the rest of the patches of this series. >> >> For this one, I only have a couple of questions: >> >> 1) Across different vendors, some have gone with limiting the zpos and >> some have gone with >> the max, so is there an issue with sticking with the max_blend_stages >> limit? >> >> 2) If there is no hard reason to make this change, I think its better >> to >> keep it the way it is. > > Short answer to both questions: we want to have more planes than the > max_blend_stages. So we should remove the limit. > > Consider this to be a unification with MDP5, which uses 255 here. Alright, one minor comment below. Also, what you have mentioned now "wanting to have more planes than blend stages" should goto the commit text and the userspace bug part can be omitted as its technically isnt a bug but just the way we intended it to be. > >> >> > >> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> > --- >> > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 11 +---------- >> > 1 file changed, 1 insertion(+), 10 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> > index 8ed7b8f0db69..3850f2714bf3 100644 >> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c >> > @@ -44,7 +44,6 @@ >> > #define DPU_NAME_SIZE 12 >> > >> > #define DPU_PLANE_COLOR_FILL_FLAG BIT(31) >> > -#define DPU_ZPOS_MAX 255 instead of getting rid of this, you can use this macro below where 255 is hardcoded. >> > >> > /* multirect rect index */ >> > enum { >> > @@ -1374,7 +1373,6 @@ struct drm_plane *dpu_plane_init(struct >> > drm_device *dev, >> > struct dpu_plane *pdpu; >> > struct msm_drm_private *priv = dev->dev_private; >> > struct dpu_kms *kms = to_dpu_kms(priv->kms); >> > - int zpos_max = DPU_ZPOS_MAX; >> > uint32_t num_formats; >> > int ret = -EINVAL; >> > >> > @@ -1412,14 +1410,7 @@ struct drm_plane *dpu_plane_init(struct >> > drm_device *dev, >> > >> > pdpu->catalog = kms->catalog; >> > >> > - if (kms->catalog->mixer_count && >> > - kms->catalog->mixer[0].sblk->maxblendstages) { >> > - zpos_max = kms->catalog->mixer[0].sblk->maxblendstages - 1; >> > - if (zpos_max > DPU_STAGE_MAX - DPU_STAGE_0 - 1) >> > - zpos_max = DPU_STAGE_MAX - DPU_STAGE_0 - 1; >> > - } >> > - >> > - ret = drm_plane_create_zpos_property(plane, 0, 0, zpos_max); >> > + ret = drm_plane_create_zpos_property(plane, 0, 0, 255); >> > if (ret) >> > DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
On Wed, 10 Nov 2021 at 04:35, <abhinavk@codeaurora.org> wrote: > > On 2021-11-09 12:21, Dmitry Baryshkov wrote: > > On Tue, 9 Nov 2021 at 23:15, <abhinavk@codeaurora.org> wrote: > >> > >> On 2021-07-04 18:21, Dmitry Baryshkov wrote: > >> > Stop limiting zpos property values, we use normalized_zpos anyway. And > >> > nothing stops userspace from assigning several planes to a single zpos > >> > (it is a userspace bug, but the kernel is forgiving about it). > >> > >> Userspace assigning several planes to a single zpos was intended to > >> identify > >> cases where src split can be used. Downstream does not use normalized > >> zpos, > >> hence it did not come across as a bug but mostly as a way to identify > >> when > >> usermode needs src split to be enabled based on the composition > >> strategy. > >> > >> We can talk about that more in the rest of the patches of this series. > >> > >> For this one, I only have a couple of questions: > >> > >> 1) Across different vendors, some have gone with limiting the zpos and > >> some have gone with > >> the max, so is there an issue with sticking with the max_blend_stages > >> limit? > >> > >> 2) If there is no hard reason to make this change, I think its better > >> to > >> keep it the way it is. > > > > Short answer to both questions: we want to have more planes than the > > max_blend_stages. So we should remove the limit. > > > > Consider this to be a unification with MDP5, which uses 255 here. > > Alright, one minor comment below. > > Also, what you have mentioned now "wanting to have more planes than > blend stages" > should goto the commit text and the userspace bug part can be omitted as > its technically > isnt a bug but just the way we intended it to be. It still is a bug. See https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#plane-abstraction, "zpos" description: 'User-space may set mutable zpos properties so that multiple active planes on the same CRTC have identical zpos values. This is a user-space bug...' > > > > > >> > >> > > >> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >> > --- > >> > drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 11 +---------- > >> > 1 file changed, 1 insertion(+), 10 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > >> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > >> > index 8ed7b8f0db69..3850f2714bf3 100644 > >> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > >> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c > >> > @@ -44,7 +44,6 @@ > >> > #define DPU_NAME_SIZE 12 > >> > > >> > #define DPU_PLANE_COLOR_FILL_FLAG BIT(31) > >> > -#define DPU_ZPOS_MAX 255 > instead of getting rid of this, you can use this macro below where 255 > is hardcoded. Ack > >> > > >> > /* multirect rect index */ > >> > enum { > >> > @@ -1374,7 +1373,6 @@ struct drm_plane *dpu_plane_init(struct > >> > drm_device *dev, > >> > struct dpu_plane *pdpu; > >> > struct msm_drm_private *priv = dev->dev_private; > >> > struct dpu_kms *kms = to_dpu_kms(priv->kms); > >> > - int zpos_max = DPU_ZPOS_MAX; > >> > uint32_t num_formats; > >> > int ret = -EINVAL; > >> > > >> > @@ -1412,14 +1410,7 @@ struct drm_plane *dpu_plane_init(struct > >> > drm_device *dev, > >> > > >> > pdpu->catalog = kms->catalog; > >> > > >> > - if (kms->catalog->mixer_count && > >> > - kms->catalog->mixer[0].sblk->maxblendstages) { > >> > - zpos_max = kms->catalog->mixer[0].sblk->maxblendstages - 1; > >> > - if (zpos_max > DPU_STAGE_MAX - DPU_STAGE_0 - 1) > >> > - zpos_max = DPU_STAGE_MAX - DPU_STAGE_0 - 1; > >> > - } > >> > - > >> > - ret = drm_plane_create_zpos_property(plane, 0, 0, zpos_max); > >> > + ret = drm_plane_create_zpos_property(plane, 0, 0, 255); > >> > if (ret) > >> > DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index 8ed7b8f0db69..3850f2714bf3 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -44,7 +44,6 @@ #define DPU_NAME_SIZE 12 #define DPU_PLANE_COLOR_FILL_FLAG BIT(31) -#define DPU_ZPOS_MAX 255 /* multirect rect index */ enum { @@ -1374,7 +1373,6 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev, struct dpu_plane *pdpu; struct msm_drm_private *priv = dev->dev_private; struct dpu_kms *kms = to_dpu_kms(priv->kms); - int zpos_max = DPU_ZPOS_MAX; uint32_t num_formats; int ret = -EINVAL; @@ -1412,14 +1410,7 @@ struct drm_plane *dpu_plane_init(struct drm_device *dev, pdpu->catalog = kms->catalog; - if (kms->catalog->mixer_count && - kms->catalog->mixer[0].sblk->maxblendstages) { - zpos_max = kms->catalog->mixer[0].sblk->maxblendstages - 1; - if (zpos_max > DPU_STAGE_MAX - DPU_STAGE_0 - 1) - zpos_max = DPU_STAGE_MAX - DPU_STAGE_0 - 1; - } - - ret = drm_plane_create_zpos_property(plane, 0, 0, zpos_max); + ret = drm_plane_create_zpos_property(plane, 0, 0, 255); if (ret) DPU_ERROR("failed to install zpos property, rc = %d\n", ret);
Stop limiting zpos property values, we use normalized_zpos anyway. And nothing stops userspace from assigning several planes to a single zpos (it is a userspace bug, but the kernel is forgiving about it). Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)