* [PATCH v4 0/1] Fix errors when reporting rotation capability @ 2023-11-24 10:00 Hsiao Chien Sung 2023-11-24 10:00 ` [PATCH v4 1/1] drm/mediatek: " Hsiao Chien Sung 0 siblings, 1 reply; 6+ messages in thread From: Hsiao Chien Sung @ 2023-11-24 10:00 UTC (permalink / raw) To: Chun-Kuang Hu, AngeloGioacchino Del Regno Cc: Philipp Zabel, David Airlie, Daniel Vetter, Matthias Brugger, Sean Paul, CK Hu, dri-devel, linux-mediatek, linux-arm-kernel, linux-kernel, Hsiao Chien Sung This commit is based on 20231024130048.14749-1-shawn.sung@mediatek.com. This bug was found when running IGT tests. For CRTCs that doesn't support rotation should still return DRM_MODE_ROTATE_0, otherwise the test will fail to restart. Returns the hardware capabilities accordingly. Changes in v4: - Remove rotation property from the driver data since OVL rotation property for all chips are the same Changes in v3: - Return 0 (not support) if rotation capabilities is not defined in the driver data. Changes in v2: - Restore DRM_MODE_ROTATE_180 (reflect x + reflect y = rotate 180) - Define supported rotations in the driver data Hsiao Chien Sung (1): drm/mediatek: Fix errors when reporting rotation capability 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(-) -- 2.39.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 1/1] drm/mediatek: Fix errors when reporting rotation capability 2023-11-24 10:00 [PATCH v4 0/1] Fix errors when reporting rotation capability Hsiao Chien Sung @ 2023-11-24 10:00 ` Hsiao Chien Sung 2023-12-19 2:35 ` CK Hu (胡俊光) 0 siblings, 1 reply; 6+ messages in thread From: Hsiao Chien Sung @ 2023-11-24 10:00 UTC (permalink / raw) To: Chun-Kuang Hu, AngeloGioacchino Del Regno Cc: Philipp Zabel, David Airlie, Daniel Vetter, Matthias Brugger, Sean Paul, CK Hu, dri-devel, linux-mediatek, linux-arm-kernel, linux-kernel, Hsiao Chien Sung 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; /* * 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) { err = drm_plane_create_rotation_property(plane, DRM_MODE_ROTATE_0, supported_rotations); -- 2.39.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/1] drm/mediatek: Fix errors when reporting rotation capability 2023-11-24 10:00 ` [PATCH v4 1/1] drm/mediatek: " Hsiao Chien Sung @ 2023-12-19 2:35 ` CK Hu (胡俊光) 2023-12-19 3:45 ` Shawn Sung (宋孝謙) 0 siblings, 1 reply; 6+ messages in thread From: CK Hu (胡俊光) @ 2023-12-19 2:35 UTC (permalink / raw) To: Shawn Sung (宋孝謙), chunkuang.hu, angelogioacchino.delregno Cc: linux-mediatek, linux-kernel, daniel, p.zabel, seanpaul, dri-devel, airlied, linux-arm-kernel, matthias.bgg 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. > > /* > * 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? [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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/1] drm/mediatek: Fix errors when reporting rotation capability 2023-12-19 2:35 ` CK Hu (胡俊光) @ 2023-12-19 3:45 ` Shawn Sung (宋孝謙) 2023-12-19 5:24 ` CK Hu (胡俊光) 0 siblings, 1 reply; 6+ messages in thread From: Shawn Sung (宋孝謙) @ 2023-12-19 3:45 UTC (permalink / raw) To: CK Hu (胡俊光), chunkuang.hu, angelogioacchino.delregno Cc: linux-kernel, linux-mediatek, daniel, p.zabel, seanpaul, dri-devel, airlied, linux-arm-kernel, matthias.bgg 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/1] drm/mediatek: Fix errors when reporting rotation capability 2023-12-19 3:45 ` Shawn Sung (宋孝謙) @ 2023-12-19 5:24 ` CK Hu (胡俊光) 2023-12-19 5:59 ` Shawn Sung (宋孝謙) 0 siblings, 1 reply; 6+ messages in thread From: CK Hu (胡俊光) @ 2023-12-19 5:24 UTC (permalink / raw) To: Shawn Sung (宋孝謙), chunkuang.hu, angelogioacchino.delregno Cc: linux-kernel, linux-mediatek, daniel, p.zabel, seanpaul, dri-devel, airlied, linux-arm-kernel, matthias.bgg Hi, Hsiao-chien: On Tue, 2023-12-19 at 03:45 +0000, Shawn Sung (宋孝謙) wrote: > 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"? OK. But I don't know why do this, so describe more about this. > > > > > > > > > /* > > > * 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. OK, so, when supported_rotations == NULL (mtk_disp_rdma), it should also create rotation property. But I'm confused that if IGT assume that all drm driver should have rotation property, why drm core does not add this property for all drm driver? Regards, CK > > > > > > > [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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/1] drm/mediatek: Fix errors when reporting rotation capability 2023-12-19 5:24 ` CK Hu (胡俊光) @ 2023-12-19 5:59 ` Shawn Sung (宋孝謙) 0 siblings, 0 replies; 6+ messages in thread From: Shawn Sung (宋孝謙) @ 2023-12-19 5:59 UTC (permalink / raw) To: CK Hu (胡俊光), chunkuang.hu, angelogioacchino.delregno Cc: linux-kernel, linux-mediatek, daniel, p.zabel, seanpaul, dri-devel, airlied, linux-arm-kernel, matthias.bgg Hi CK, On Tue, 2023-12-19 at 05:24 +0000, CK Hu (胡俊光) wrote: > Hi, Hsiao-chien: > > On Tue, 2023-12-19 at 03:45 +0000, Shawn Sung (宋孝謙) wrote: > > 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"? > > OK. But I don't know why do this, so describe more about this. > Got it, thanks. Will add more information to the commit message. > > > > > > > > > > > > > /* > > > > * 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. > > OK, so, when supported_rotations == NULL (mtk_disp_rdma), it should > also create rotation property. But I'm confused that if IGT assume > that > all drm driver should have rotation property, why drm core does not > add > this property for all drm driver? > After checking the report, only "graphics.IgtKms.kms_flip_unstable" has such a constraint. However, since IGT version is still keep updating, there are not guarantees about this behavior. I did search for "DRM_MODE_ROTATE_0" in the kernel to check if there is any rule or even suggestion about setting this property, but the flag is rarely even mentioned. Maybe we could also report this issue to IGT team instead of changing the behavior of our driver. > Regards, > CK > > > > > > > > > > > > [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_MO > > > > DE_ROTAT > > > > E_0, > > > > suppor > > > > ted_rota > > > > tions); > > > > -- > > > > 2.39.2 > > > > > > > > Thanks, > > Shawn ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-19 5:59 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-11-24 10:00 [PATCH v4 0/1] Fix errors when reporting rotation capability Hsiao Chien Sung 2023-11-24 10:00 ` [PATCH v4 1/1] drm/mediatek: " Hsiao Chien Sung 2023-12-19 2:35 ` CK Hu (胡俊光) 2023-12-19 3:45 ` Shawn Sung (宋孝謙) 2023-12-19 5:24 ` CK Hu (胡俊光) 2023-12-19 5:59 ` Shawn Sung (宋孝謙)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®