Re: [PATCH v4 1/1] drm/mediatek: Fix errors when reporting rotation capability

From: Shawn Sung (宋孝謙)
Date: Mon Dec 18 2023 - 22:46:03 EST

Hi CK,

On Tue, 2023-12-19 at 02:35 +0000, CK Hu (胡俊光) wrote:
> Hi, Hsiao-chien:
>
> On Fri, 2023-11-24 at 18:00 +0800, Hsiao Chien Sung wrote:
> > Create rotation property according to the hardware capability.
> > Since currently OVL of all chips support same rotation,
> > no need to define it in the driver data.
> >
> > Fixes: 84d805753983 ("drm/mediatek: Support reflect-y plane
> > rotation")
> >
> > Reviewed-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@xxxxxxxxxxxxx>
> > Signed-off-by: Hsiao Chien Sung <shawn.sung@xxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/mediatek/mtk_disp_drv.h | 1 +
> > drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 18 ++++++--------
> > ----
> > .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 9 +++++++++
> > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 1 +
> > drivers/gpu/drm/mediatek/mtk_drm_plane.c | 2 +-
> > 5 files changed, 18 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > index 4d6e8b667bc3..c5afeb7c5527 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > @@ -127,6 +127,7 @@ void mtk_ovl_adaptor_register_vblank_cb(struct
> > device *dev, void (*vblank_cb)(vo
> > void mtk_ovl_adaptor_unregister_vblank_cb(struct device *dev);
> > void mtk_ovl_adaptor_enable_vblank(struct device *dev);
> > void mtk_ovl_adaptor_disable_vblank(struct device *dev);
> > +unsigned int mtk_ovl_adaptor_supported_rotations(struct device
> > *dev);
> > void mtk_ovl_adaptor_start(struct device *dev);
> > void mtk_ovl_adaptor_stop(struct device *dev);
> > unsigned int mtk_ovl_adaptor_layer_nr(struct device *dev);
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > index ecc38932fd44..319bbfde5cf9 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > @@ -415,6 +415,10 @@ unsigned int mtk_ovl_layer_nr(struct device
> > *dev)
> >
> > unsigned int mtk_ovl_supported_rotations(struct device *dev)
> > {
> > + /*
> > + * although currently OVL can only do reflection,
> > + * reflect x + reflect y = rotate 180
> > + */
> > return DRM_MODE_ROTATE_0 | DRM_MODE_ROTATE_180 |
> > DRM_MODE_REFLECT_X | DRM_MODE_REFLECT_Y;
> > }
> > @@ -423,27 +427,17 @@ int mtk_ovl_layer_check(struct device *dev,
> > unsigned int idx,
> > struct mtk_plane_state *mtk_state)
> > {
> > struct drm_plane_state *state = &mtk_state->base;
> > - unsigned int rotation = 0;
> >
> > - rotation = drm_rotation_simplify(state->rotation,
> > - DRM_MODE_ROTATE_0 |
> > - DRM_MODE_REFLECT_X |
> > - DRM_MODE_REFLECT_Y);
> > - rotation &= ~DRM_MODE_ROTATE_0;
> > -
> > - /* We can only do reflection, not rotation */
> > - if ((rotation & DRM_MODE_ROTATE_MASK) != 0)
> > + if (state->rotation & ~mtk_ovl_supported_rotations(dev))
> > return -EINVAL;
>
> The commit message of this patch is "Create rotation property
> according
> to the hardware capability". I think this modification is not related
> to create property, so separate this to another patch.

Since these modifications are all related to rotation property, or
should I change the title and commit message to "Modify roation
property for passing IGT"?

>
> >
> > /*
> > * TODO: Rotating/reflecting YUV buffers is not supported at
> > this time.
> > * Only RGB[AX] variants are supported.
> > */
> > - if (state->fb->format->is_yuv && rotation != 0)
> > + if (state->fb->format->is_yuv && (state->rotation &
> > ~DRM_MODE_ROTATE_0))
> > return -EINVAL;
> >
> > - state->rotation = rotation;
> > -
> > return 0;
> > }
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> > b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> > index 4398db9a6276..273c79d37bef 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> > @@ -383,6 +383,15 @@ void mtk_ovl_adaptor_register_vblank_cb(struct
> > device *dev, void (*vblank_cb)(vo
> > vblank_cb, vblank_cb_data);
> > }
> >
> > +unsigned int mtk_ovl_adaptor_supported_rotations(struct device
> > *dev)
> > +{
> > + /*
> > + * should still return DRM_MODE_ROTATE_0 if rotation is not
> > supported,
> > + * or IGT will fail.
> > + */
> > + return DRM_MODE_ROTATE_0;
> > +}
> > +
> > void mtk_ovl_adaptor_unregister_vblank_cb(struct device *dev)
> > {
> > struct mtk_disp_ovl_adaptor *ovl_adaptor =
> > dev_get_drvdata(dev);
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > index ffa4868b1222..206dd6f6f99e 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > @@ -422,6 +422,7 @@ static const struct mtk_ddp_comp_funcs
> > ddp_ovl_adaptor = {
> > .remove = mtk_ovl_adaptor_remove_comp,
> > .get_formats = mtk_ovl_adaptor_get_formats,
> > .get_num_formats = mtk_ovl_adaptor_get_num_formats,
> > + .supported_rotations = mtk_ovl_adaptor_supported_rotations,
> > };
> >
> > static const char * const mtk_ddp_comp_stem[MTK_DDP_COMP_TYPE_MAX]
> > =
> > {
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > index e2ec61b69618..894c39a38a58 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > @@ -344,7 +344,7 @@ int mtk_plane_init(struct drm_device *dev,
> > struct
> > drm_plane *plane,
> > return err;
> > }
> >
> > - if (supported_rotations & ~DRM_MODE_ROTATE_0) {
> > + if (supported_rotations) {
>
> Why need this modification?
> Before Sean's patch [1], only support rotate 0 and does not create
> property and it works. Why does it must create property for only
> support rotate 0?

Yes, as long as the user doesn't check rotation properties before
commiting the pitures, there will be no problem. But IGT somehow
checked this DRM_MODE_ROTATE_0 flag before executing the tests (not all
of them), and leads to failures in these test itmes.

>
>
> [1]
>
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/mediatek/mtk_drm_plane.c?h=v6.7-rc6&id=ef87d3e2dd251374c5c9fa3b6502aeff8fe29da9
>
> Regards,
> CK
>
> > err = drm_plane_create_rotation_property(plane,
> > DRM_MODE_ROTAT
> > E_0,
> > supported_rota
> > tions);
> > --
> > 2.39.2
> >

Thanks,
Shawn