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

["Shawn Sung (宋孝謙)" <Shawn.Sung@mediatek.com>]

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@collabora.com>
> > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com>
> > ---
> >  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