On Wed, 10 Nov 2021 at 04:35, <abhinavk@xxxxxxxxxxxxxx> wrote: > > On 2021-11-09 12:21, Dmitry Baryshkov wrote: > > On Tue, 9 Nov 2021 at 23:15, <abhinavk@xxxxxxxxxxxxxx> 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@xxxxxxxxxx> > >> > --- > >> > 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); -- With best wishes Dmitry
