diff mbox series

drm/mediatek: Declare Z Position for all planes

Message ID 20240718082507.216764-1-angelogioacchino.delregno@collabora.com (mailing list archive)
State New
Headers show
Series drm/mediatek: Declare Z Position for all planes | expand

Commit Message

AngeloGioacchino Del Regno July 18, 2024, 8:25 a.m. UTC
MediaTek SoCs support multiple planes, one of which is the primary
and all the others are overlays (and CURSOR is the last overlay).

In all currently supported SoCs, the Z order of the overlays can't
be changed with any fast muxing action, and can only be changed by
swapping the contents of the entire register set of one overlay
with the other to internally reorder the layer properties, which
is indeed feasible, but probably more expensive than desired.

Declare the Z position for all planes with an immutable property
at least for now, so that the userspace can take its decisions
accordingly.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_crtc.c  |  2 +-
 drivers/gpu/drm/mediatek/mtk_plane.c | 18 +++++++++++++++++-
 drivers/gpu/drm/mediatek/mtk_plane.h |  3 +--
 3 files changed, 19 insertions(+), 4 deletions(-)

Comments

Fei Shao July 18, 2024, 9:50 a.m. UTC | #1
On Thu, Jul 18, 2024 at 4:25 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> MediaTek SoCs support multiple planes, one of which is the primary
> and all the others are overlays (and CURSOR is the last overlay).
>
> In all currently supported SoCs, the Z order of the overlays can't
> be changed with any fast muxing action, and can only be changed by
> swapping the contents of the entire register set of one overlay
> with the other to internally reorder the layer properties, which
> is indeed feasible, but probably more expensive than desired.
>
> Declare the Z position for all planes with an immutable property
> at least for now, so that the userspace can take its decisions
> accordingly.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Fei Shao <fshao@chromium.org>
> ---
>  drivers/gpu/drm/mediatek/mtk_crtc.c  |  2 +-
>  drivers/gpu/drm/mediatek/mtk_plane.c | 18 +++++++++++++++++-
>  drivers/gpu/drm/mediatek/mtk_plane.h |  3 +--
>  3 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
> index 072b2fdae87b..327214721b4d 100644
> --- a/drivers/gpu/drm/mediatek/mtk_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_crtc.c
> @@ -874,7 +874,7 @@ static int mtk_crtc_init_comp_planes(struct drm_device *drm_dev,
>                                 mtk_crtc_plane_type(mtk_crtc->layer_nr, num_planes),
>                                 mtk_ddp_comp_supported_rotations(comp),
>                                 mtk_ddp_comp_get_formats(comp),
> -                               mtk_ddp_comp_get_num_formats(comp));
> +                               mtk_ddp_comp_get_num_formats(comp), i);
>                 if (ret)
>                         return ret;
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_plane.c b/drivers/gpu/drm/mediatek/mtk_plane.c
> index 5bf757a3ef20..7d2cb4e0fafa 100644
> --- a/drivers/gpu/drm/mediatek/mtk_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_plane.c
> @@ -321,7 +321,7 @@ static const struct drm_plane_helper_funcs mtk_plane_helper_funcs = {
>  int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>                    unsigned long possible_crtcs, enum drm_plane_type type,
>                    unsigned int supported_rotations, const u32 *formats,
> -                  size_t num_formats)
> +                  size_t num_formats, unsigned int plane_idx)
>  {
>         int err;
>
> @@ -338,6 +338,22 @@ int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>                 return err;
>         }
>
> +       /*
> +        * The hardware does not support repositioning planes by muxing: their
> +        * Z-position is infact fixed and the only way to change the actual
> +        * order is to swap the contents of the entire register set of one
> +        * overlay with another, which may be more expensive than desired.
> +        *
> +        * With no repositioning, the caller of this function guarantees that
> +        * the plane_idx is correct. This means that, for example, the PRIMARY
> +        * plane fed to this function will always have plane_idx zero.
> +        */
> +       err = drm_plane_create_zpos_immutable_property(plane, plane_idx);
> +       if (err) {
> +               DRM_ERROR("Failed to create zpos property for plane %u\n", plane_idx);
> +               return err;
> +       }
> +
>         if (supported_rotations) {
>                 err = drm_plane_create_rotation_property(plane,
>                                                          DRM_MODE_ROTATE_0,
> diff --git a/drivers/gpu/drm/mediatek/mtk_plane.h b/drivers/gpu/drm/mediatek/mtk_plane.h
> index 231bb7aac947..5b177eac67b7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_plane.h
> +++ b/drivers/gpu/drm/mediatek/mtk_plane.h
> @@ -49,6 +49,5 @@ to_mtk_plane_state(struct drm_plane_state *state)
>  int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>                    unsigned long possible_crtcs, enum drm_plane_type type,
>                    unsigned int supported_rotations, const u32 *formats,
> -                  size_t num_formats);
> -
> +                  size_t num_formats, unsigned int plane_idx);
>  #endif
> --
> 2.45.2
>
>
Daniel Stone July 18, 2024, 11:15 a.m. UTC | #2
Hi,

On Thu, 18 Jul 2024 at 09:25, AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
> MediaTek SoCs support multiple planes, one of which is the primary
> and all the others are overlays (and CURSOR is the last overlay).
>
> In all currently supported SoCs, the Z order of the overlays can't
> be changed with any fast muxing action, and can only be changed by
> swapping the contents of the entire register set of one overlay
> with the other to internally reorder the layer properties, which
> is indeed feasible, but probably more expensive than desired.
>
> Declare the Z position for all planes with an immutable property
> at least for now, so that the userspace can take its decisions
> accordingly.

Thanks a lot for this fix!

If I understand your middle paragraph correctly, please don't ever do
that though. I think what you're suggesting by 'swapping the contents
of the entire register set' is:
* plane ID 40 (normally) controls OVL1
* plane ID 41 controls OVL2
* userspace configures planes 40 & 41 with a zpos suggesting that 41
should be below 40
* the DRM driver 'swaps the contents of the entire register set' by
making plane 40 control OVL2 instead and plane 41 control OVL1 instead

If so - please no. Just declare an immutable zpos, because that
actually matches the hardware, and then leave userspace to configure
the planes in a way which makes sense. Looking at the zpos property is
already required in order to handle overlapping overlay planes, and
any userspace which supports overlays (including Weston) already looks
at zpos, handles immutable properties, and will dtrt.

Anyway, this is:
Acked-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel
AngeloGioacchino Del Regno July 18, 2024, 11:19 a.m. UTC | #3
Il 18/07/24 13:15, Daniel Stone ha scritto:
> Hi,
> 
> On Thu, 18 Jul 2024 at 09:25, AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>> MediaTek SoCs support multiple planes, one of which is the primary
>> and all the others are overlays (and CURSOR is the last overlay).
>>
>> In all currently supported SoCs, the Z order of the overlays can't
>> be changed with any fast muxing action, and can only be changed by
>> swapping the contents of the entire register set of one overlay
>> with the other to internally reorder the layer properties, which
>> is indeed feasible, but probably more expensive than desired.
>>
>> Declare the Z position for all planes with an immutable property
>> at least for now, so that the userspace can take its decisions
>> accordingly.
> 
> Thanks a lot for this fix!
> 
> If I understand your middle paragraph correctly, please don't ever do
> that though. I think what you're suggesting by 'swapping the contents
> of the entire register set' is:
> * plane ID 40 (normally) controls OVL1
> * plane ID 41 controls OVL2
> * userspace configures planes 40 & 41 with a zpos suggesting that 41
> should be below 40
> * the DRM driver 'swaps the contents of the entire register set' by
> making plane 40 control OVL2 instead and plane 41 control OVL1 instead

Yeah, that's exactly what I mean.

> 
> If so - please no. Just declare an immutable zpos, because that
> actually matches the hardware, and then leave userspace to configure
> the planes in a way which makes sense. 

I had a hunch... and that's a perfect confirmation - thank you!

> Looking at the zpos property is
> already required in order to handle overlapping overlay planes, and
> any userspace which supports overlays (including Weston) already looks
> at zpos, handles immutable properties, and will dtrt.
> 
> Anyway, this is:
> Acked-by: Daniel Stone <daniels@collabora.com>

Thumbs up! :-)

Cheers!

> 
> Cheers,
> Daniel
CK Hu (胡俊光) Aug. 12, 2024, 1:37 a.m. UTC | #4
Hi, Angelo:

On Thu, 2024-07-18 at 10:25 +0200, AngeloGioacchino Del Regno wrote:
> MediaTek SoCs support multiple planes, one of which is the primary
> and all the others are overlays (and CURSOR is the last overlay).
> 
> In all currently supported SoCs, the Z order of the overlays can't
> be changed with any fast muxing action, and can only be changed by
> swapping the contents of the entire register set of one overlay
> with the other to internally reorder the layer properties, which
> is indeed feasible, but probably more expensive than desired.
> 
> Declare the Z position for all planes with an immutable property
> at least for now, so that the userspace can take its decisions
> accordingly.

Reviewed-by: CK Hu <ck.hu@mediatek.com>

> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_crtc.c  |  2 +-
>  drivers/gpu/drm/mediatek/mtk_plane.c | 18 +++++++++++++++++-
>  drivers/gpu/drm/mediatek/mtk_plane.h |  3 +--
>  3 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
> index 072b2fdae87b..327214721b4d 100644
> --- a/drivers/gpu/drm/mediatek/mtk_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_crtc.c
> @@ -874,7 +874,7 @@ static int mtk_crtc_init_comp_planes(struct drm_device *drm_dev,
>  				mtk_crtc_plane_type(mtk_crtc->layer_nr, num_planes),
>  				mtk_ddp_comp_supported_rotations(comp),
>  				mtk_ddp_comp_get_formats(comp),
> -				mtk_ddp_comp_get_num_formats(comp));
> +				mtk_ddp_comp_get_num_formats(comp), i);
>  		if (ret)
>  			return ret;
>  
> diff --git a/drivers/gpu/drm/mediatek/mtk_plane.c b/drivers/gpu/drm/mediatek/mtk_plane.c
> index 5bf757a3ef20..7d2cb4e0fafa 100644
> --- a/drivers/gpu/drm/mediatek/mtk_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_plane.c
> @@ -321,7 +321,7 @@ static const struct drm_plane_helper_funcs mtk_plane_helper_funcs = {
>  int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  		   unsigned long possible_crtcs, enum drm_plane_type type,
>  		   unsigned int supported_rotations, const u32 *formats,
> -		   size_t num_formats)
> +		   size_t num_formats, unsigned int plane_idx)
>  {
>  	int err;
>  
> @@ -338,6 +338,22 @@ int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  		return err;
>  	}
>  
> +	/*
> +	 * The hardware does not support repositioning planes by muxing: their
> +	 * Z-position is infact fixed and the only way to change the actual
> +	 * order is to swap the contents of the entire register set of one
> +	 * overlay with another, which may be more expensive than desired.
> +	 *
> +	 * With no repositioning, the caller of this function guarantees that
> +	 * the plane_idx is correct. This means that, for example, the PRIMARY
> +	 * plane fed to this function will always have plane_idx zero.
> +	 */
> +	err = drm_plane_create_zpos_immutable_property(plane, plane_idx);
> +	if (err) {
> +		DRM_ERROR("Failed to create zpos property for plane %u\n", plane_idx);
> +		return err;
> +	}
> +
>  	if (supported_rotations) {
>  		err = drm_plane_create_rotation_property(plane,
>  							 DRM_MODE_ROTATE_0,
> diff --git a/drivers/gpu/drm/mediatek/mtk_plane.h b/drivers/gpu/drm/mediatek/mtk_plane.h
> index 231bb7aac947..5b177eac67b7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_plane.h
> +++ b/drivers/gpu/drm/mediatek/mtk_plane.h
> @@ -49,6 +49,5 @@ to_mtk_plane_state(struct drm_plane_state *state)
>  int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  		   unsigned long possible_crtcs, enum drm_plane_type type,
>  		   unsigned int supported_rotations, const u32 *formats,
> -		   size_t num_formats);
> -
> +		   size_t num_formats, unsigned int plane_idx);
>  #endif
Chun-Kuang Hu Aug. 22, 2024, 2:19 p.m. UTC | #5
Hi, Angelo:

AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> 於
2024年7月18日 週四 下午4:25寫道:
>
> MediaTek SoCs support multiple planes, one of which is the primary
> and all the others are overlays (and CURSOR is the last overlay).
>
> In all currently supported SoCs, the Z order of the overlays can't
> be changed with any fast muxing action, and can only be changed by
> swapping the contents of the entire register set of one overlay
> with the other to internally reorder the layer properties, which
> is indeed feasible, but probably more expensive than desired.
>
> Declare the Z position for all planes with an immutable property
> at least for now, so that the userspace can take its decisions
> accordingly.

Applied to mediatek-drm-next [1], thanks.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/chunkuang.hu/linux.git/log/?h=mediatek-drm-next

Regards,
Chun-Kuang.

>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_crtc.c  |  2 +-
>  drivers/gpu/drm/mediatek/mtk_plane.c | 18 +++++++++++++++++-
>  drivers/gpu/drm/mediatek/mtk_plane.h |  3 +--
>  3 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
> index 072b2fdae87b..327214721b4d 100644
> --- a/drivers/gpu/drm/mediatek/mtk_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_crtc.c
> @@ -874,7 +874,7 @@ static int mtk_crtc_init_comp_planes(struct drm_device *drm_dev,
>                                 mtk_crtc_plane_type(mtk_crtc->layer_nr, num_planes),
>                                 mtk_ddp_comp_supported_rotations(comp),
>                                 mtk_ddp_comp_get_formats(comp),
> -                               mtk_ddp_comp_get_num_formats(comp));
> +                               mtk_ddp_comp_get_num_formats(comp), i);
>                 if (ret)
>                         return ret;
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_plane.c b/drivers/gpu/drm/mediatek/mtk_plane.c
> index 5bf757a3ef20..7d2cb4e0fafa 100644
> --- a/drivers/gpu/drm/mediatek/mtk_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_plane.c
> @@ -321,7 +321,7 @@ static const struct drm_plane_helper_funcs mtk_plane_helper_funcs = {
>  int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>                    unsigned long possible_crtcs, enum drm_plane_type type,
>                    unsigned int supported_rotations, const u32 *formats,
> -                  size_t num_formats)
> +                  size_t num_formats, unsigned int plane_idx)
>  {
>         int err;
>
> @@ -338,6 +338,22 @@ int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>                 return err;
>         }
>
> +       /*
> +        * The hardware does not support repositioning planes by muxing: their
> +        * Z-position is infact fixed and the only way to change the actual
> +        * order is to swap the contents of the entire register set of one
> +        * overlay with another, which may be more expensive than desired.
> +        *
> +        * With no repositioning, the caller of this function guarantees that
> +        * the plane_idx is correct. This means that, for example, the PRIMARY
> +        * plane fed to this function will always have plane_idx zero.
> +        */
> +       err = drm_plane_create_zpos_immutable_property(plane, plane_idx);
> +       if (err) {
> +               DRM_ERROR("Failed to create zpos property for plane %u\n", plane_idx);
> +               return err;
> +       }
> +
>         if (supported_rotations) {
>                 err = drm_plane_create_rotation_property(plane,
>                                                          DRM_MODE_ROTATE_0,
> diff --git a/drivers/gpu/drm/mediatek/mtk_plane.h b/drivers/gpu/drm/mediatek/mtk_plane.h
> index 231bb7aac947..5b177eac67b7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_plane.h
> +++ b/drivers/gpu/drm/mediatek/mtk_plane.h
> @@ -49,6 +49,5 @@ to_mtk_plane_state(struct drm_plane_state *state)
>  int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
>                    unsigned long possible_crtcs, enum drm_plane_type type,
>                    unsigned int supported_rotations, const u32 *formats,
> -                  size_t num_formats);
> -
> +                  size_t num_formats, unsigned int plane_idx);
>  #endif
> --
> 2.45.2
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
index 072b2fdae87b..327214721b4d 100644
--- a/drivers/gpu/drm/mediatek/mtk_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_crtc.c
@@ -874,7 +874,7 @@  static int mtk_crtc_init_comp_planes(struct drm_device *drm_dev,
 				mtk_crtc_plane_type(mtk_crtc->layer_nr, num_planes),
 				mtk_ddp_comp_supported_rotations(comp),
 				mtk_ddp_comp_get_formats(comp),
-				mtk_ddp_comp_get_num_formats(comp));
+				mtk_ddp_comp_get_num_formats(comp), i);
 		if (ret)
 			return ret;
 
diff --git a/drivers/gpu/drm/mediatek/mtk_plane.c b/drivers/gpu/drm/mediatek/mtk_plane.c
index 5bf757a3ef20..7d2cb4e0fafa 100644
--- a/drivers/gpu/drm/mediatek/mtk_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_plane.c
@@ -321,7 +321,7 @@  static const struct drm_plane_helper_funcs mtk_plane_helper_funcs = {
 int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		   unsigned long possible_crtcs, enum drm_plane_type type,
 		   unsigned int supported_rotations, const u32 *formats,
-		   size_t num_formats)
+		   size_t num_formats, unsigned int plane_idx)
 {
 	int err;
 
@@ -338,6 +338,22 @@  int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		return err;
 	}
 
+	/*
+	 * The hardware does not support repositioning planes by muxing: their
+	 * Z-position is infact fixed and the only way to change the actual
+	 * order is to swap the contents of the entire register set of one
+	 * overlay with another, which may be more expensive than desired.
+	 *
+	 * With no repositioning, the caller of this function guarantees that
+	 * the plane_idx is correct. This means that, for example, the PRIMARY
+	 * plane fed to this function will always have plane_idx zero.
+	 */
+	err = drm_plane_create_zpos_immutable_property(plane, plane_idx);
+	if (err) {
+		DRM_ERROR("Failed to create zpos property for plane %u\n", plane_idx);
+		return err;
+	}
+
 	if (supported_rotations) {
 		err = drm_plane_create_rotation_property(plane,
 							 DRM_MODE_ROTATE_0,
diff --git a/drivers/gpu/drm/mediatek/mtk_plane.h b/drivers/gpu/drm/mediatek/mtk_plane.h
index 231bb7aac947..5b177eac67b7 100644
--- a/drivers/gpu/drm/mediatek/mtk_plane.h
+++ b/drivers/gpu/drm/mediatek/mtk_plane.h
@@ -49,6 +49,5 @@  to_mtk_plane_state(struct drm_plane_state *state)
 int mtk_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		   unsigned long possible_crtcs, enum drm_plane_type type,
 		   unsigned int supported_rotations, const u32 *formats,
-		   size_t num_formats);
-
+		   size_t num_formats, unsigned int plane_idx);
 #endif