[PATCH v1] drm/tegra: plane: Remove format-modifier checking

[PATCH v1] drm/tegra: plane: Remove format-modifier checking

[Dmitry Osipenko]

Tiling modifier can't be applied to YV12 video overlay because all tiling
modifiers are filtered out for multi-plane formats. AFAIK, all modifiers
should work with all of formats, hence the checking is incorrect and
simply not needed.

Fixes: e90124cb46bdb ("drm/tegra: plane: Support format modifiers")
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/drm/tegra/plane.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/gpu/drm/tegra/plane.c b/drivers/gpu/drm/tegra/plane.c
index d068e8aa3553..5a8a3387f5ee 100644
--- a/drivers/gpu/drm/tegra/plane.c
+++ b/drivers/gpu/drm/tegra/plane.c
@@ -72,21 +72,6 @@ static void tegra_plane_atomic_destroy_state(struct drm_plane *plane,
 	kfree(state);
 }
 
-static bool tegra_plane_format_mod_supported(struct drm_plane *plane,
-					     uint32_t format,
-					     uint64_t modifier)
-{
-	const struct drm_format_info *info = drm_format_info(format);
-
-	if (modifier == DRM_FORMAT_MOD_LINEAR)
-		return true;
-
-	if (info->num_planes == 1)
-		return true;
-
-	return false;
-}
-
 const struct drm_plane_funcs tegra_plane_funcs = {
 	.update_plane = drm_atomic_helper_update_plane,
 	.disable_plane = drm_atomic_helper_disable_plane,
@@ -94,7 +79,6 @@ const struct drm_plane_funcs tegra_plane_funcs = {
 	.reset = tegra_plane_reset,
 	.atomic_duplicate_state = tegra_plane_atomic_duplicate_state,
 	.atomic_destroy_state = tegra_plane_atomic_destroy_state,
-	.format_mod_supported = tegra_plane_format_mod_supported,
 };
 
 int tegra_plane_state_add(struct tegra_plane *plane,
-- 
2.20.1

Re: [PATCH v1] drm/tegra: plane: Remove format-modifier checking

[Dmitry Osipenko]

24.02.2019 18:34, Dmitry Osipenko пишет:
> Tiling modifier can't be applied to YV12 video overlay because all tiling
> modifiers are filtered out for multi-plane formats. AFAIK, all modifiers
> should work with all of formats, hence the checking is incorrect and
> simply not needed.
> 
> Fixes: e90124cb46bdb ("drm/tegra: plane: Support format modifiers")
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---

For the reference.. this patch needs a stable tag, I did the same mistake in [0]. Thierry, please let me know if you're okay with the change and if you could add the tag while applying the patch, thanks.

[0] https://lkml.org/lkml/2019/2/26/277

Re: [PATCH v1] drm/tegra: plane: Remove format-modifier checking

[Dmitry Osipenko]

27.02.2019 18:08, Dmitry Osipenko пишет:
> 24.02.2019 18:34, Dmitry Osipenko пишет:
>> Tiling modifier can't be applied to YV12 video overlay because all tiling
>> modifiers are filtered out for multi-plane formats. AFAIK, all modifiers
>> should work with all of formats, hence the checking is incorrect and
>> simply not needed.
>>
>> Fixes: e90124cb46bdb ("drm/tegra: plane: Support format modifiers")
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
> 
> For the reference.. this patch needs a stable tag, I did the same mistake in [0]. Thierry, please let me know if you're okay with the change and if you could add the tag while applying the patch, thanks.
> 
> [0] https://lkml.org/lkml/2019/2/26/277
> 

Thierry, please don't forget about this patch.

Re: [PATCH v1] drm/tegra: plane: Remove format-modifier checking

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3493 bytes --]

On Sun, Feb 24, 2019 at 06:34:05PM +0300, Dmitry Osipenko wrote:
> Tiling modifier can't be applied to YV12 video overlay because all tiling
> modifiers are filtered out for multi-plane formats. AFAIK, all modifiers
> should work with all of formats, hence the checking is incorrect and
> simply not needed.
> 
> Fixes: e90124cb46bdb ("drm/tegra: plane: Support format modifiers")
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/tegra/plane.c | 16 ----------------
>  1 file changed, 16 deletions(-)

I'm hesitant to apply this because we don't really have a good way to
test that this is actually the case. I vaguely recall that at least for
some of the block-linear formats supported on Tegra124 and later there
are additional restrictions on when they can be used.

There's also the problem that using these block-linear formats, and I
think this even applies to the TILED format on older Tegra SoCs, results
in higher bandwidth requirements.

Bandwidth requirements is something that we don't really concern
ourselves with, and that's bad enough as it is. I suspect that once we
blindly allow all format modifiers we could easily run into situations
where the display controllers underflow.

Now, regardless of which way you look at this, it boils down to testing.
We don't have a good way of testing various combinations of format
modifiers to verify that they work. You say yourself that "AFAIK, all
modifiers should work with all formats", but can you really know for
certain? Until we're able to properly test this, we really can't.

Given all of the above, I think it's better to be prudent and only allow
format/modifier combinations that we've actually tested. I'm not aware
of a good way to test planar formats, so we don't have a good way to get
the results that we need.

I'm all ears if you know of a good way to test this. It doesn't have to
be anything fully automated. Automated testing is especially difficult
to do for display because it usually needs visual inspection. But that's
okay, I'm willing to settle for something that we can roll into a script
and run manually after boot until we can find a way to automatically do
this type of test.

Thierry

> diff --git a/drivers/gpu/drm/tegra/plane.c b/drivers/gpu/drm/tegra/plane.c
> index d068e8aa3553..5a8a3387f5ee 100644
> --- a/drivers/gpu/drm/tegra/plane.c
> +++ b/drivers/gpu/drm/tegra/plane.c
> @@ -72,21 +72,6 @@ static void tegra_plane_atomic_destroy_state(struct drm_plane *plane,
>  	kfree(state);
>  }
>  
> -static bool tegra_plane_format_mod_supported(struct drm_plane *plane,
> -					     uint32_t format,
> -					     uint64_t modifier)
> -{
> -	const struct drm_format_info *info = drm_format_info(format);
> -
> -	if (modifier == DRM_FORMAT_MOD_LINEAR)
> -		return true;
> -
> -	if (info->num_planes == 1)
> -		return true;
> -
> -	return false;
> -}
> -
>  const struct drm_plane_funcs tegra_plane_funcs = {
>  	.update_plane = drm_atomic_helper_update_plane,
>  	.disable_plane = drm_atomic_helper_disable_plane,
> @@ -94,7 +79,6 @@ const struct drm_plane_funcs tegra_plane_funcs = {
>  	.reset = tegra_plane_reset,
>  	.atomic_duplicate_state = tegra_plane_atomic_duplicate_state,
>  	.atomic_destroy_state = tegra_plane_atomic_destroy_state,
> -	.format_mod_supported = tegra_plane_format_mod_supported,
>  };
>  
>  int tegra_plane_state_add(struct tegra_plane *plane,
> -- 
> 2.20.1
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1] drm/tegra: plane: Remove format-modifier checking

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3493 bytes --]

On Sun, Feb 24, 2019 at 06:34:05PM +0300, Dmitry Osipenko wrote:
> Tiling modifier can't be applied to YV12 video overlay because all tiling
> modifiers are filtered out for multi-plane formats. AFAIK, all modifiers
> should work with all of formats, hence the checking is incorrect and
> simply not needed.
> 
> Fixes: e90124cb46bdb ("drm/tegra: plane: Support format modifiers")
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/tegra/plane.c | 16 ----------------
>  1 file changed, 16 deletions(-)

I'm hesitant to apply this because we don't really have a good way to
test that this is actually the case. I vaguely recall that at least for
some of the block-linear formats supported on Tegra124 and later there
are additional restrictions on when they can be used.

There's also the problem that using these block-linear formats, and I
think this even applies to the TILED format on older Tegra SoCs, results
in higher bandwidth requirements.

Bandwidth requirements is something that we don't really concern
ourselves with, and that's bad enough as it is. I suspect that once we
blindly allow all format modifiers we could easily run into situations
where the display controllers underflow.

Now, regardless of which way you look at this, it boils down to testing.
We don't have a good way of testing various combinations of format
modifiers to verify that they work. You say yourself that "AFAIK, all
modifiers should work with all formats", but can you really know for
certain? Until we're able to properly test this, we really can't.

Given all of the above, I think it's better to be prudent and only allow
format/modifier combinations that we've actually tested. I'm not aware
of a good way to test planar formats, so we don't have a good way to get
the results that we need.

I'm all ears if you know of a good way to test this. It doesn't have to
be anything fully automated. Automated testing is especially difficult
to do for display because it usually needs visual inspection. But that's
okay, I'm willing to settle for something that we can roll into a script
and run manually after boot until we can find a way to automatically do
this type of test.

Thierry

> diff --git a/drivers/gpu/drm/tegra/plane.c b/drivers/gpu/drm/tegra/plane.c
> index d068e8aa3553..5a8a3387f5ee 100644
> --- a/drivers/gpu/drm/tegra/plane.c
> +++ b/drivers/gpu/drm/tegra/plane.c
> @@ -72,21 +72,6 @@ static void tegra_plane_atomic_destroy_state(struct drm_plane *plane,
>  	kfree(state);
>  }
>  
> -static bool tegra_plane_format_mod_supported(struct drm_plane *plane,
> -					     uint32_t format,
> -					     uint64_t modifier)
> -{
> -	const struct drm_format_info *info = drm_format_info(format);
> -
> -	if (modifier == DRM_FORMAT_MOD_LINEAR)
> -		return true;
> -
> -	if (info->num_planes == 1)
> -		return true;
> -
> -	return false;
> -}
> -
>  const struct drm_plane_funcs tegra_plane_funcs = {
>  	.update_plane = drm_atomic_helper_update_plane,
>  	.disable_plane = drm_atomic_helper_disable_plane,
> @@ -94,7 +79,6 @@ const struct drm_plane_funcs tegra_plane_funcs = {
>  	.reset = tegra_plane_reset,
>  	.atomic_duplicate_state = tegra_plane_atomic_duplicate_state,
>  	.atomic_destroy_state = tegra_plane_atomic_destroy_state,
> -	.format_mod_supported = tegra_plane_format_mod_supported,
>  };
>  
>  int tegra_plane_state_add(struct tegra_plane *plane,
> -- 
> 2.20.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]