| Message ID | 20200206065951.213862-1-evanbenn@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mediatek: Ensure the cursor plane is on top of other overlays | expand |
Apologies if I have formatted this wrong / sent to the wrong place! The patch posted had an issue, another place in the driver was assuming planes[1] still referred to the cursor. Now search for the cursor before sending (as the plane cannot be hard-coded, some devices have different numbers of planes). Thanks Evan Benn On Thu, Feb 6, 2020 at 6:00 PM <evanbenn@google.com> wrote: > > From: Sean Paul <seanpaul@chromium.org> > > Currently the cursor is placed on the first overlay plane, which means > it will be at the bottom of the stack when the hw does the compositing > with anything other than primary plane. Since mtk doesn't support plane > zpos, change the cursor location to the top-most plane. > > Signed-off-by: Evan Benn <evanbenn@chromium.org> > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 29 +++++++++++++++++-------- > 1 file changed, 20 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > index 7b392d6c71cc..d4078c2089e0 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > @@ -658,10 +658,21 @@ static const struct drm_crtc_helper_funcs mtk_crtc_helper_funcs = { > > static int mtk_drm_crtc_init(struct drm_device *drm, > struct mtk_drm_crtc *mtk_crtc, > - struct drm_plane *primary, > - struct drm_plane *cursor, unsigned int pipe) > + unsigned int pipe) > { > - int ret; > + int i, ret; > + > + struct drm_plane *primary = NULL; > + struct drm_plane *cursor = NULL; > + > + for (i = 0; i < mtk_crtc->layer_nr; ++i) { > + if (!primary && mtk_crtc->planes[i].type == > + DRM_PLANE_TYPE_PRIMARY) > + primary = &mtk_crtc->planes[i]; > + if (!cursor && mtk_crtc->planes[i].type == > + DRM_PLANE_TYPE_CURSOR) > + cursor = &mtk_crtc->planes[i]; > + } > > ret = drm_crtc_init_with_planes(drm, &mtk_crtc->base, primary, cursor, > &mtk_crtc_funcs, NULL); > @@ -711,11 +722,12 @@ static int mtk_drm_crtc_num_comp_planes(struct mtk_drm_crtc *mtk_crtc, > } > > static inline > -enum drm_plane_type mtk_drm_crtc_plane_type(unsigned int plane_idx) > +enum drm_plane_type mtk_drm_crtc_plane_type(unsigned int plane_idx, > + unsigned int num_planes) > { > if (plane_idx == 0) > return DRM_PLANE_TYPE_PRIMARY; > - else if (plane_idx == 1) > + else if (plane_idx == (num_planes - 1)) > return DRM_PLANE_TYPE_CURSOR; > else > return DRM_PLANE_TYPE_OVERLAY; > @@ -734,7 +746,8 @@ static int mtk_drm_crtc_init_comp_planes(struct drm_device *drm_dev, > ret = mtk_plane_init(drm_dev, > &mtk_crtc->planes[mtk_crtc->layer_nr], > BIT(pipe), > - mtk_drm_crtc_plane_type(mtk_crtc->layer_nr), > + mtk_drm_crtc_plane_type(mtk_crtc->layer_nr, > + num_planes), > mtk_ddp_comp_supported_rotations(comp)); > if (ret) > return ret; > @@ -830,9 +843,7 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev, > return ret; > } > > - ret = mtk_drm_crtc_init(drm_dev, mtk_crtc, &mtk_crtc->planes[0], > - mtk_crtc->layer_nr > 1 ? &mtk_crtc->planes[1] : > - NULL, pipe); > + ret = mtk_drm_crtc_init(drm_dev, mtk_crtc, pipe); > if (ret < 0) > return ret; > > -- > 2.25.0.341.g760bfbb309-goog >
Hi, Evan: It looks like Sean's version has some problem. So this version is Reviewed-by: CK Hu <ck.hu@mediatek.com> On Thu, 2020-02-06 at 17:59 +1100, evanbenn@google.com wrote: > From: Sean Paul <seanpaul@chromium.org> > > Currently the cursor is placed on the first overlay plane, which means > it will be at the bottom of the stack when the hw does the compositing > with anything other than primary plane. Since mtk doesn't support plane > zpos, change the cursor location to the top-most plane. > > Signed-off-by: Evan Benn <evanbenn@chromium.org> > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 29 +++++++++++++++++-------- > 1 file changed, 20 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > index 7b392d6c71cc..d4078c2089e0 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > @@ -658,10 +658,21 @@ static const struct drm_crtc_helper_funcs mtk_crtc_helper_funcs = { > > static int mtk_drm_crtc_init(struct drm_device *drm, > struct mtk_drm_crtc *mtk_crtc, > - struct drm_plane *primary, > - struct drm_plane *cursor, unsigned int pipe) > + unsigned int pipe) > { > - int ret; > + int i, ret; > + > + struct drm_plane *primary = NULL; > + struct drm_plane *cursor = NULL; > + > + for (i = 0; i < mtk_crtc->layer_nr; ++i) { > + if (!primary && mtk_crtc->planes[i].type == > + DRM_PLANE_TYPE_PRIMARY) > + primary = &mtk_crtc->planes[i]; > + if (!cursor && mtk_crtc->planes[i].type == > + DRM_PLANE_TYPE_CURSOR) > + cursor = &mtk_crtc->planes[i]; > + } > > ret = drm_crtc_init_with_planes(drm, &mtk_crtc->base, primary, cursor, > &mtk_crtc_funcs, NULL); > @@ -711,11 +722,12 @@ static int mtk_drm_crtc_num_comp_planes(struct mtk_drm_crtc *mtk_crtc, > } > > static inline > -enum drm_plane_type mtk_drm_crtc_plane_type(unsigned int plane_idx) > +enum drm_plane_type mtk_drm_crtc_plane_type(unsigned int plane_idx, > + unsigned int num_planes) > { > if (plane_idx == 0) > return DRM_PLANE_TYPE_PRIMARY; > - else if (plane_idx == 1) > + else if (plane_idx == (num_planes - 1)) > return DRM_PLANE_TYPE_CURSOR; > else > return DRM_PLANE_TYPE_OVERLAY; > @@ -734,7 +746,8 @@ static int mtk_drm_crtc_init_comp_planes(struct drm_device *drm_dev, > ret = mtk_plane_init(drm_dev, > &mtk_crtc->planes[mtk_crtc->layer_nr], > BIT(pipe), > - mtk_drm_crtc_plane_type(mtk_crtc->layer_nr), > + mtk_drm_crtc_plane_type(mtk_crtc->layer_nr, > + num_planes), > mtk_ddp_comp_supported_rotations(comp)); > if (ret) > return ret; > @@ -830,9 +843,7 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev, > return ret; > } > > - ret = mtk_drm_crtc_init(drm_dev, mtk_crtc, &mtk_crtc->planes[0], > - mtk_crtc->layer_nr > 1 ? &mtk_crtc->planes[1] : > - NULL, pipe); > + ret = mtk_drm_crtc_init(drm_dev, mtk_crtc, pipe); > if (ret < 0) > return ret; >
On Thu, Feb 06, 2020 at 05:59:51PM +1100, evanbenn@google.com wrote: > From: Sean Paul <seanpaul@chromium.org> > > Currently the cursor is placed on the first overlay plane, which means > it will be at the bottom of the stack when the hw does the compositing > with anything other than primary plane. Since mtk doesn't support plane > zpos, change the cursor location to the top-most plane. > > Signed-off-by: Evan Benn <evanbenn@chromium.org> Hi Evan, Thanks for spotting the issue! I think this should probably be 2 patches, one to fix crtc init and then the cursor patch on top of that. We generally try to only do one thing per patch. A few other nits below.. > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 29 +++++++++++++++++-------- > 1 file changed, 20 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > index 7b392d6c71cc..d4078c2089e0 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c > @@ -658,10 +658,21 @@ static const struct drm_crtc_helper_funcs mtk_crtc_helper_funcs = { > > static int mtk_drm_crtc_init(struct drm_device *drm, > struct mtk_drm_crtc *mtk_crtc, > - struct drm_plane *primary, > - struct drm_plane *cursor, unsigned int pipe) > + unsigned int pipe) > { > - int ret; > + int i, ret; > + extra line > + struct drm_plane *primary = NULL; > + struct drm_plane *cursor = NULL; These should be on top of the int declaration > + > + for (i = 0; i < mtk_crtc->layer_nr; ++i) { We don't really do pre-increment in kernel for loops > + if (!primary && mtk_crtc->planes[i].type == > + DRM_PLANE_TYPE_PRIMARY) Line breaks should be around '&&': if (!primary && mtk_crtc->planes[i].type == DRM_PLANE_TYPE_PRIMARY) > + primary = &mtk_crtc->planes[i]; > + if (!cursor && mtk_crtc->planes[i].type == else if? > + DRM_PLANE_TYPE_CURSOR) > + cursor = &mtk_crtc->planes[i]; Since we can only have one primary and one cursor, the NULL checks on primary and cursor are unnecessary, you can just blindly assign them when you hit a plane of the right type. If the driver creates multiples the behavior is undefined anyways. > + } > > ret = drm_crtc_init_with_planes(drm, &mtk_crtc->base, primary, cursor, > &mtk_crtc_funcs, NULL); > @@ -711,11 +722,12 @@ static int mtk_drm_crtc_num_comp_planes(struct mtk_drm_crtc *mtk_crtc, > } > > static inline > -enum drm_plane_type mtk_drm_crtc_plane_type(unsigned int plane_idx) > +enum drm_plane_type mtk_drm_crtc_plane_type(unsigned int plane_idx, > + unsigned int num_planes) > { > if (plane_idx == 0) > return DRM_PLANE_TYPE_PRIMARY; > - else if (plane_idx == 1) > + else if (plane_idx == (num_planes - 1)) > return DRM_PLANE_TYPE_CURSOR; > else > return DRM_PLANE_TYPE_OVERLAY; > @@ -734,7 +746,8 @@ static int mtk_drm_crtc_init_comp_planes(struct drm_device *drm_dev, > ret = mtk_plane_init(drm_dev, > &mtk_crtc->planes[mtk_crtc->layer_nr], > BIT(pipe), > - mtk_drm_crtc_plane_type(mtk_crtc->layer_nr), > + mtk_drm_crtc_plane_type(mtk_crtc->layer_nr, > + num_planes), > mtk_ddp_comp_supported_rotations(comp)); > if (ret) > return ret; > @@ -830,9 +843,7 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev, > return ret; > } > > - ret = mtk_drm_crtc_init(drm_dev, mtk_crtc, &mtk_crtc->planes[0], > - mtk_crtc->layer_nr > 1 ? &mtk_crtc->planes[1] : > - NULL, pipe); > + ret = mtk_drm_crtc_init(drm_dev, mtk_crtc, pipe); > if (ret < 0) > return ret; > > -- > 2.25.0.341.g760bfbb309-goog >
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c index 7b392d6c71cc..d4078c2089e0 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c @@ -658,10 +658,21 @@ static const struct drm_crtc_helper_funcs mtk_crtc_helper_funcs = { static int mtk_drm_crtc_init(struct drm_device *drm, struct mtk_drm_crtc *mtk_crtc, - struct drm_plane *primary, - struct drm_plane *cursor, unsigned int pipe) + unsigned int pipe) { - int ret; + int i, ret; + + struct drm_plane *primary = NULL; + struct drm_plane *cursor = NULL; + + for (i = 0; i < mtk_crtc->layer_nr; ++i) { + if (!primary && mtk_crtc->planes[i].type == + DRM_PLANE_TYPE_PRIMARY) + primary = &mtk_crtc->planes[i]; + if (!cursor && mtk_crtc->planes[i].type == + DRM_PLANE_TYPE_CURSOR) + cursor = &mtk_crtc->planes[i]; + } ret = drm_crtc_init_with_planes(drm, &mtk_crtc->base, primary, cursor, &mtk_crtc_funcs, NULL); @@ -711,11 +722,12 @@ static int mtk_drm_crtc_num_comp_planes(struct mtk_drm_crtc *mtk_crtc, } static inline -enum drm_plane_type mtk_drm_crtc_plane_type(unsigned int plane_idx) +enum drm_plane_type mtk_drm_crtc_plane_type(unsigned int plane_idx, + unsigned int num_planes) { if (plane_idx == 0) return DRM_PLANE_TYPE_PRIMARY; - else if (plane_idx == 1) + else if (plane_idx == (num_planes - 1)) return DRM_PLANE_TYPE_CURSOR; else return DRM_PLANE_TYPE_OVERLAY; @@ -734,7 +746,8 @@ static int mtk_drm_crtc_init_comp_planes(struct drm_device *drm_dev, ret = mtk_plane_init(drm_dev, &mtk_crtc->planes[mtk_crtc->layer_nr], BIT(pipe), - mtk_drm_crtc_plane_type(mtk_crtc->layer_nr), + mtk_drm_crtc_plane_type(mtk_crtc->layer_nr, + num_planes), mtk_ddp_comp_supported_rotations(comp)); if (ret) return ret; @@ -830,9 +843,7 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev, return ret; } - ret = mtk_drm_crtc_init(drm_dev, mtk_crtc, &mtk_crtc->planes[0], - mtk_crtc->layer_nr > 1 ? &mtk_crtc->planes[1] : - NULL, pipe); + ret = mtk_drm_crtc_init(drm_dev, mtk_crtc, pipe); if (ret < 0) return ret;