[i-g-t,v3,12/21] lib/igt_vc4: Add helper for checking SAND tiling support on plane

Submitted by Paul Kocialkowski on Jan. 11, 2019, 9:05 a.m.

Details

Message ID 20190111090532.19235-13-paul.kocialkowski@bootlin.com
State New
Series "Chamelium VC4 plane fuzzy testing, with SAND and T-tiled mode"
Headers show

Commit Message

Paul Kocialkowski Jan. 11, 2019, 9:05 a.m.
This introduces a convenience helper for checking whether a plane
supports SAND tiling for a given format.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 lib/igt_vc4.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Patch hide | download patch | download mbox

diff --git a/lib/igt_vc4.h b/lib/igt_vc4.h
index 9118ae193f60..cb980541a61f 100644
--- a/lib/igt_vc4.h
+++ b/lib/igt_vc4.h
@@ -33,6 +33,30 @@  static inline bool igt_vc4_plane_supports_t_tiling(igt_plane_t *plane,
 					DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED);
 }
 
+static inline bool igt_vc4_plane_supports_sand_tiling(igt_plane_t *plane,
+						      uint32_t format,
+						      size_t column_width_bytes)
+{
+	uint64_t modifier;
+
+	switch (column_width_bytes) {
+	case 32:
+		modifier = DRM_FORMAT_MOD_BROADCOM_SAND32;
+		break;
+	case 64:
+		modifier = DRM_FORMAT_MOD_BROADCOM_SAND64;
+		break;
+	case 128:
+		modifier = DRM_FORMAT_MOD_BROADCOM_SAND128;
+		break;
+	case 256:
+		modifier = DRM_FORMAT_MOD_BROADCOM_SAND256;
+		break;
+	}
+
+	return igt_plane_has_format_mod(plane, format, modifier);
+}
+
 uint32_t igt_vc4_get_cleared_bo(int fd, size_t size, uint32_t clearval);
 int igt_vc4_create_bo(int fd, size_t size);
 void *igt_vc4_mmap_bo(int fd, uint32_t handle, uint32_t size, unsigned prot);

Comments

Maxime Ripard Jan. 11, 2019, 3:15 p.m.
On Fri, Jan 11, 2019 at 10:05:23AM +0100, Paul Kocialkowski wrote:
> This introduces a convenience helper for checking whether a plane
> supports SAND tiling for a given format.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  lib/igt_vc4.h | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/lib/igt_vc4.h b/lib/igt_vc4.h
> index 9118ae193f60..cb980541a61f 100644
> --- a/lib/igt_vc4.h
> +++ b/lib/igt_vc4.h
> @@ -33,6 +33,30 @@ static inline bool igt_vc4_plane_supports_t_tiling(igt_plane_t *plane,
>  					DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED);
>  }
>  
> +static inline bool igt_vc4_plane_supports_sand_tiling(igt_plane_t *plane,
> +						      uint32_t format,
> +						      size_t column_width_bytes)
> +{
> +	uint64_t modifier;
> +
> +	switch (column_width_bytes) {
> +	case 32:
> +		modifier = DRM_FORMAT_MOD_BROADCOM_SAND32;
> +		break;
> +	case 64:
> +		modifier = DRM_FORMAT_MOD_BROADCOM_SAND64;
> +		break;
> +	case 128:
> +		modifier = DRM_FORMAT_MOD_BROADCOM_SAND128;
> +		break;
> +	case 256:
> +		modifier = DRM_FORMAT_MOD_BROADCOM_SAND256;
> +		break;
> +	}
> +
> +	return igt_plane_has_format_mod(plane, format, modifier);
> +}
> +

I'm not quite sure what that function is supposed to be doing, or
rather, why do you need the column_width_bytes parameter.

If that function was just about checking whether the sand tiling is
supported, then you can just return an or'd igt_plane_has_format_mod
of all the modifiers supported, right?

Maxime
Paul Kocialkowski Jan. 14, 2019, 3:01 p.m.
Hi,

On Fri, 2019-01-11 at 16:15 +0100, Maxime Ripard wrote:
> On Fri, Jan 11, 2019 at 10:05:23AM +0100, Paul Kocialkowski wrote:
> > This introduces a convenience helper for checking whether a plane
> > supports SAND tiling for a given format.
> > 
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  lib/igt_vc4.h | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/lib/igt_vc4.h b/lib/igt_vc4.h
> > index 9118ae193f60..cb980541a61f 100644
> > --- a/lib/igt_vc4.h
> > +++ b/lib/igt_vc4.h
> > @@ -33,6 +33,30 @@ static inline bool igt_vc4_plane_supports_t_tiling(igt_plane_t *plane,
> >  					DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED);
> >  }
> >  
> > +static inline bool igt_vc4_plane_supports_sand_tiling(igt_plane_t *plane,
> > +						      uint32_t format,
> > +						      size_t column_width_bytes)
> > +{
> > +	uint64_t modifier;
> > +
> > +	switch (column_width_bytes) {
> > +	case 32:
> > +		modifier = DRM_FORMAT_MOD_BROADCOM_SAND32;
> > +		break;
> > +	case 64:
> > +		modifier = DRM_FORMAT_MOD_BROADCOM_SAND64;
> > +		break;
> > +	case 128:
> > +		modifier = DRM_FORMAT_MOD_BROADCOM_SAND128;
> > +		break;
> > +	case 256:
> > +		modifier = DRM_FORMAT_MOD_BROADCOM_SAND256;
> > +		break;
> > +	}
> > +
> > +	return igt_plane_has_format_mod(plane, format, modifier);
> > +}
> > +
> 
> I'm not quite sure what that function is supposed to be doing, or
> rather, why do you need the column_width_bytes parameter.
> 
> If that function was just about checking whether the sand tiling is
> supported, then you can just return an or'd igt_plane_has_format_mod
> of all the modifiers supported, right?

Well, some variants of SAND are only valid for a reduced set of
formats. IIRC, SAND256 only applies to NV12 while others apply to other
YUV formats as well.

So I believe it's relevant to distinguish between the different SAND
tilings when checking for support.

However, the function could be simplified by simply passing the
modifier, extracting the base modifier (without column height, that is
coded in the modifier) and calling igt_plane_has_format_mod.

Cheers,

Paul
Maxime Ripard Jan. 15, 2019, 8:25 a.m.
On Mon, Jan 14, 2019 at 04:01:05PM +0100, Paul Kocialkowski wrote:
> Hi,
> 
> On Fri, 2019-01-11 at 16:15 +0100, Maxime Ripard wrote:
> > On Fri, Jan 11, 2019 at 10:05:23AM +0100, Paul Kocialkowski wrote:
> > > This introduces a convenience helper for checking whether a plane
> > > supports SAND tiling for a given format.
> > > 
> > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > ---
> > >  lib/igt_vc4.h | 24 ++++++++++++++++++++++++
> > >  1 file changed, 24 insertions(+)
> > > 
> > > diff --git a/lib/igt_vc4.h b/lib/igt_vc4.h
> > > index 9118ae193f60..cb980541a61f 100644
> > > --- a/lib/igt_vc4.h
> > > +++ b/lib/igt_vc4.h
> > > @@ -33,6 +33,30 @@ static inline bool igt_vc4_plane_supports_t_tiling(igt_plane_t *plane,
> > >  					DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED);
> > >  }
> > >  
> > > +static inline bool igt_vc4_plane_supports_sand_tiling(igt_plane_t *plane,
> > > +						      uint32_t format,
> > > +						      size_t column_width_bytes)
> > > +{
> > > +	uint64_t modifier;
> > > +
> > > +	switch (column_width_bytes) {
> > > +	case 32:
> > > +		modifier = DRM_FORMAT_MOD_BROADCOM_SAND32;
> > > +		break;
> > > +	case 64:
> > > +		modifier = DRM_FORMAT_MOD_BROADCOM_SAND64;
> > > +		break;
> > > +	case 128:
> > > +		modifier = DRM_FORMAT_MOD_BROADCOM_SAND128;
> > > +		break;
> > > +	case 256:
> > > +		modifier = DRM_FORMAT_MOD_BROADCOM_SAND256;
> > > +		break;
> > > +	}
> > > +
> > > +	return igt_plane_has_format_mod(plane, format, modifier);
> > > +}
> > > +
> > 
> > I'm not quite sure what that function is supposed to be doing, or
> > rather, why do you need the column_width_bytes parameter.
> > 
> > If that function was just about checking whether the sand tiling is
> > supported, then you can just return an or'd igt_plane_has_format_mod
> > of all the modifiers supported, right?
> 
> Well, some variants of SAND are only valid for a reduced set of
> formats. IIRC, SAND256 only applies to NV12 while others apply to other
> YUV formats as well.
>
> So I believe it's relevant to distinguish between the different SAND
> tilings when checking for support.

Ok

> However, the function could be simplified by simply passing the
> modifier, extracting the base modifier (without column height, that is
> coded in the modifier) and calling igt_plane_has_format_mod.

Yeah, that sounds much better.

Thanks!
Maxime
Lyude Paul Jan. 15, 2019, 9:54 p.m.
On Fri, 2019-01-11 at 10:05 +0100, Paul Kocialkowski wrote:
> This introduces a convenience helper for checking whether a plane
> supports SAND tiling for a given format.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  lib/igt_vc4.h | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/lib/igt_vc4.h b/lib/igt_vc4.h
> index 9118ae193f60..cb980541a61f 100644
> --- a/lib/igt_vc4.h
> +++ b/lib/igt_vc4.h
> @@ -33,6 +33,30 @@ static inline bool
> igt_vc4_plane_supports_t_tiling(igt_plane_t *plane,
>  					DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED);
>  }
>  
> +static inline bool igt_vc4_plane_supports_sand_tiling(igt_plane_t *plane,
> +						      uint32_t format,
> +						      size_t
> column_width_bytes)
> +{
> +	uint64_t modifier;
> +
> +	switch (column_width_bytes) {
> +	case 32:
> +		modifier = DRM_FORMAT_MOD_BROADCOM_SAND32;
> +		break;
> +	case 64:
> +		modifier = DRM_FORMAT_MOD_BROADCOM_SAND64;
> +		break;
> +	case 128:
> +		modifier = DRM_FORMAT_MOD_BROADCOM_SAND128;
> +		break;
> +	case 256:
> +		modifier = DRM_FORMAT_MOD_BROADCOM_SAND256;
> +		break;
Optional bikeshed: Maybe add a default: case with an igt_assert()?

Either way:

Reviewed-by: Lyude Paul <lyude@redhat.com>
> +	}
> +
> +	return igt_plane_has_format_mod(plane, format, modifier);
> +}
> +
>  uint32_t igt_vc4_get_cleared_bo(int fd, size_t size, uint32_t clearval);
>  int igt_vc4_create_bo(int fd, size_t size);
>  void *igt_vc4_mmap_bo(int fd, uint32_t handle, uint32_t size, unsigned
> prot);