[PATCH 0/2] drm/tegra: handle implicit scanout modifiers

[PATCH 0/2] drm/tegra: handle implicit scanout modifiers

[Diogo Ivo]

Hello!

This patch series adds support for correctly displaying tiled
framebuffers when no modifiers are reported by userspace.

Patch 1 adds the sector_layout parameter to the SET/GET_TILING
IOCTLs so that userspace can set this field appropriately.

Patch 2 adds handling of the case where the buffer object
passed to create a framebuffer is allocated with non-linear
tiling but no modifier is reported.

Diogo Ivo (2):
  drm/tegra: add sector layout to SET/GET_TILING IOCTLs
  drm/tegra: add scanout support for implicit tiling parameters

 drivers/gpu/drm/tegra/drm.c  | 29 ++++++++++++++++++
 drivers/gpu/drm/tegra/fb.c   | 59 ++++++++++++++++++++++++++++++++++--
 include/uapi/drm/tegra_drm.h | 16 ++++++----
 3 files changed, 96 insertions(+), 8 deletions(-)

-- 
2.39.1

[PATCH 1/2] drm/tegra: add sector layout to SET/GET_TILING IOCTLs

[Diogo Ivo]

Commit 7b6f846785f4 ("drm/tegra: Support sector layout on Tegra194")
updated struct tegra_bo_tiling with a new field conveying information
about the sector layout of the buffer object. Update the
SET/GET_TILING IOCTLs with this field so that userspace can set it
appropriately.

Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
---
 drivers/gpu/drm/tegra/drm.c  | 29 +++++++++++++++++++++++++++++
 include/uapi/drm/tegra_drm.h | 16 ++++++++++------
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 6748ec1e0005..27afb7fa6259 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -612,6 +612,7 @@ static int tegra_gem_set_tiling(struct drm_device *drm, void *data,
 	enum tegra_bo_tiling_mode mode;
 	struct drm_gem_object *gem;
 	unsigned long value = 0;
+	enum tegra_bo_sector_layout layout;
 	struct tegra_bo *bo;
 
 	switch (args->mode) {
@@ -644,6 +645,19 @@ static int tegra_gem_set_tiling(struct drm_device *drm, void *data,
 		return -EINVAL;
 	}
 
+	switch (args->sector_layout) {
+	case DRM_TEGRA_GEM_SECTOR_LAYOUT_TEGRA:
+		layout = TEGRA_BO_SECTOR_LAYOUT_TEGRA;
+		break;
+
+	case DRM_TEGRA_GEM_SECTOR_LAYOUT_GPU:
+		layout = TEGRA_BO_SECTOR_LAYOUT_GPU;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
 	gem = drm_gem_object_lookup(file, args->handle);
 	if (!gem)
 		return -ENOENT;
@@ -652,6 +666,7 @@ static int tegra_gem_set_tiling(struct drm_device *drm, void *data,
 
 	bo->tiling.mode = mode;
 	bo->tiling.value = value;
+	bo->tiling.sector_layout = layout;
 
 	drm_gem_object_put(gem);
 
@@ -693,6 +708,20 @@ static int tegra_gem_get_tiling(struct drm_device *drm, void *data,
 		break;
 	}
 
+	switch (bo->tiling.sector_layout) {
+	case TEGRA_BO_SECTOR_LAYOUT_TEGRA:
+		args->sector_layout = DRM_TEGRA_GEM_SECTOR_LAYOUT_TEGRA;
+		break;
+
+	case TEGRA_BO_SECTOR_LAYOUT_GPU:
+		args->sector_layout = DRM_TEGRA_GEM_SECTOR_LAYOUT_GPU;
+		break;
+
+	default:
+		err = -EINVAL;
+		break;
+	}
+
 	drm_gem_object_put(gem);
 
 	return err;
diff --git a/include/uapi/drm/tegra_drm.h b/include/uapi/drm/tegra_drm.h
index 94cfc306d50a..811e21b1a60e 100644
--- a/include/uapi/drm/tegra_drm.h
+++ b/include/uapi/drm/tegra_drm.h
@@ -508,6 +508,9 @@ struct drm_tegra_submit {
 #define DRM_TEGRA_GEM_TILING_MODE_TILED 1
 #define DRM_TEGRA_GEM_TILING_MODE_BLOCK 2
 
+#define DRM_TEGRA_GEM_SECTOR_LAYOUT_TEGRA 0
+#define DRM_TEGRA_GEM_SECTOR_LAYOUT_GPU 1
+
 /**
  * struct drm_tegra_gem_set_tiling - parameters for the set tiling IOCTL
  */
@@ -543,11 +546,11 @@ struct drm_tegra_gem_set_tiling {
 	__u32 value;
 
 	/**
-	 * @pad:
+	 * @sector_layout:
 	 *
-	 * Structure padding that may be used in the future. Must be 0.
+	 * Specify low-level sector layout.
 	 */
-	__u32 pad;
+	__u32 sector_layout;
 };
 
 /**
@@ -578,11 +581,12 @@ struct drm_tegra_gem_get_tiling {
 	__u32 value;
 
 	/**
-	 * @pad:
+	 * @sector_layout:
 	 *
-	 * Structure padding that may be used in the future. Must be 0.
+	 * The sector layout parameter currently associated with the GEM object.
+	 * Set by the kernel upon successful completion of the IOCTL.
 	 */
-	__u32 pad;
+	__u32 sector_layout;
 };
 
 #define DRM_TEGRA_GEM_BOTTOM_UP		(1 << 0)
-- 
2.39.1

[PATCH 2/2] drm/tegra: add scanout support for implicit tiling parameters

[Diogo Ivo]

When importing buffers from the GPU to scan out, it may happen
that the buffer object has non-trivial tiling parameters, which
currently go by undetected. This leads to the framebuffer being
read and displayed in the wrong order. Explicitly check for this
case and reconstruct the adequate modifier so that the framebuffer
is correctly scanned out.

Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
---
 drivers/gpu/drm/tegra/fb.c | 59 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
index 9291209154a7..31f381262345 100644
--- a/drivers/gpu/drm/tegra/fb.c
+++ b/drivers/gpu/drm/tegra/fb.c
@@ -116,11 +116,63 @@ static struct drm_framebuffer *tegra_fb_alloc(struct drm_device *drm,
 	struct drm_framebuffer *fb;
 	unsigned int i;
 	int err;
+	struct drm_mode_fb_cmd2 mode_cmd_local;
 
 	fb = kzalloc(sizeof(*fb), GFP_KERNEL);
 	if (!fb)
 		return ERR_PTR(-ENOMEM);
 
+	/* Check for implicitly defined modifiers using
+	 * the state defined by tegra_gem_set_tiling().
+	 */
+	if (!(mode_cmd->flags & DRM_MODE_FB_MODIFIERS)) {
+		uint64_t modifier;
+
+		mode_cmd_local = *mode_cmd;
+
+		switch (planes[0]->tiling.mode) {
+		case TEGRA_BO_TILING_MODE_PITCH:
+			modifier = DRM_FORMAT_MOD_LINEAR;
+			break;
+
+		case TEGRA_BO_TILING_MODE_TILED:
+			modifier = DRM_FORMAT_MOD_NVIDIA_TEGRA_TILED;
+			break;
+
+		/* With all rigour this reconstruction of the modifier is
+		 * incomplete, as it skips some fields (like page kind).
+		 * However, along with the sector layout below it should
+		 * contain all the bits of information needed by the
+		 * scanout hardware.
+		 */
+		case TEGRA_BO_TILING_MODE_BLOCK:
+			unsigned long height = planes[0]->tiling.value;
+
+			if (height > 5) {
+				dev_err(drm->dev, "invalid block height value: %ld\n",
+					height);
+
+				err = -EINVAL;
+				goto cleanup;
+			}
+
+			modifier = DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK(height);
+			break;
+
+		default:
+			dev_err(drm->dev, "invalid tiling mode\n");
+			err = -EINVAL;
+			goto cleanup;
+		}
+
+		if (planes[0]->tiling.sector_layout == DRM_TEGRA_GEM_SECTOR_LAYOUT_GPU)
+			modifier |= DRM_FORMAT_MOD_NVIDIA_SECTOR_LAYOUT;
+
+		mode_cmd_local.modifier[0] = modifier;
+
+		mode_cmd = &mode_cmd_local;
+	}
+
 	drm_helper_mode_fill_fb_struct(drm, fb, mode_cmd);
 
 	for (i = 0; i < fb->format->num_planes; i++)
@@ -130,11 +182,14 @@ static struct drm_framebuffer *tegra_fb_alloc(struct drm_device *drm,
 	if (err < 0) {
 		dev_err(drm->dev, "failed to initialize framebuffer: %d\n",
 			err);
-		kfree(fb);
-		return ERR_PTR(err);
+		goto cleanup;
 	}
 
 	return fb;
+
+cleanup:
+	kfree(fb);
+	return ERR_PTR(err);
 }
 
 struct drm_framebuffer *tegra_fb_create(struct drm_device *drm,
-- 
2.39.1

Re: [PATCH 2/2] drm/tegra: add scanout support for implicit tiling parameters

[kernel test robot]

Hi Diogo,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tegra/for-next]
[also build test ERROR on drm/drm-next tegra-drm/drm/tegra/for-next linus/master v6.2-rc6 next-20230130]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Diogo-Ivo/drm-tegra-add-sector-layout-to-SET-GET_TILING-IOCTLs/20230120-190334
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
patch link:    https://lore.kernel.org/r/20230120105858.214440-3-diogo.ivo%40tecnico.ulisboa.pt
patch subject: [PATCH 2/2] drm/tegra: add scanout support for implicit tiling parameters
config: arm64-randconfig-r016-20230130 (https://download.01.org/0day-ci/archive/20230131/202301310334.4oiy5KGY-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 4196ca3278f78c6e19246e54ab0ecb364e37d66a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/fffef2135ccf679112cc60dee0532494c1389c78
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Diogo-Ivo/drm-tegra-add-sector-layout-to-SET-GET_TILING-IOCTLs/20230120-190334
        git checkout fffef2135ccf679112cc60dee0532494c1389c78
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/gpu/drm/tegra/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/tegra/fb.c:149:4: error: expected expression
                           unsigned long height = planes[0]->tiling.value;
                           ^
>> drivers/gpu/drm/tegra/fb.c:151:8: error: use of undeclared identifier 'height'
                           if (height > 5) {
                               ^
>> drivers/gpu/drm/tegra/fb.c:151:8: error: use of undeclared identifier 'height'
>> drivers/gpu/drm/tegra/fb.c:151:8: error: use of undeclared identifier 'height'
   drivers/gpu/drm/tegra/fb.c:153:6: error: use of undeclared identifier 'height'
                                           height);
                                           ^
   drivers/gpu/drm/tegra/fb.c:159:49: error: use of undeclared identifier 'height'
                           modifier = DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK(height);
                                                                        ^
   6 errors generated.


vim +149 drivers/gpu/drm/tegra/fb.c

   110	
   111	static struct drm_framebuffer *tegra_fb_alloc(struct drm_device *drm,
   112						      const struct drm_mode_fb_cmd2 *mode_cmd,
   113						      struct tegra_bo **planes,
   114						      unsigned int num_planes)
   115	{
   116		struct drm_framebuffer *fb;
   117		unsigned int i;
   118		int err;
   119		struct drm_mode_fb_cmd2 mode_cmd_local;
   120	
   121		fb = kzalloc(sizeof(*fb), GFP_KERNEL);
   122		if (!fb)
   123			return ERR_PTR(-ENOMEM);
   124	
   125		/* Check for implicitly defined modifiers using
   126		 * the state defined by tegra_gem_set_tiling().
   127		 */
   128		if (!(mode_cmd->flags & DRM_MODE_FB_MODIFIERS)) {
   129			uint64_t modifier;
   130	
   131			mode_cmd_local = *mode_cmd;
   132	
   133			switch (planes[0]->tiling.mode) {
   134			case TEGRA_BO_TILING_MODE_PITCH:
   135				modifier = DRM_FORMAT_MOD_LINEAR;
   136				break;
   137	
   138			case TEGRA_BO_TILING_MODE_TILED:
   139				modifier = DRM_FORMAT_MOD_NVIDIA_TEGRA_TILED;
   140				break;
   141	
   142			/* With all rigour this reconstruction of the modifier is
   143			 * incomplete, as it skips some fields (like page kind).
   144			 * However, along with the sector layout below it should
   145			 * contain all the bits of information needed by the
   146			 * scanout hardware.
   147			 */
   148			case TEGRA_BO_TILING_MODE_BLOCK:
 > 149				unsigned long height = planes[0]->tiling.value;
   150	
 > 151				if (height > 5) {
   152					dev_err(drm->dev, "invalid block height value: %ld\n",
   153						height);
   154	
   155					err = -EINVAL;
   156					goto cleanup;
   157				}
   158	
   159				modifier = DRM_FORMAT_MOD_NVIDIA_16BX2_BLOCK(height);
   160				break;
   161	
   162			default:
   163				dev_err(drm->dev, "invalid tiling mode\n");
   164				err = -EINVAL;
   165				goto cleanup;
   166			}
   167	
   168			if (planes[0]->tiling.sector_layout == DRM_TEGRA_GEM_SECTOR_LAYOUT_GPU)
   169				modifier |= DRM_FORMAT_MOD_NVIDIA_SECTOR_LAYOUT;
   170	
   171			mode_cmd_local.modifier[0] = modifier;
   172	
   173			mode_cmd = &mode_cmd_local;
   174		}
   175	
   176		drm_helper_mode_fill_fb_struct(drm, fb, mode_cmd);
   177	
   178		for (i = 0; i < fb->format->num_planes; i++)
   179			fb->obj[i] = &planes[i]->gem;
   180	
   181		err = drm_framebuffer_init(drm, fb, &tegra_fb_funcs);
   182		if (err < 0) {
   183			dev_err(drm->dev, "failed to initialize framebuffer: %d\n",
   184				err);
   185			goto cleanup;
   186		}
   187	
   188		return fb;
   189	
   190	cleanup:
   191		kfree(fb);
   192		return ERR_PTR(err);
   193	}
   194	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 0/2] drm/tegra: handle implicit scanout modifiers

[Thierry Reding]

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

On Fri, Jan 20, 2023 at 10:58:56AM +0000, Diogo Ivo wrote:
> Hello!
> 
> This patch series adds support for correctly displaying tiled
> framebuffers when no modifiers are reported by userspace.
> 
> Patch 1 adds the sector_layout parameter to the SET/GET_TILING
> IOCTLs so that userspace can set this field appropriately.
> 
> Patch 2 adds handling of the case where the buffer object
> passed to create a framebuffer is allocated with non-linear
> tiling but no modifier is reported.
> 
> Diogo Ivo (2):
>   drm/tegra: add sector layout to SET/GET_TILING IOCTLs
>   drm/tegra: add scanout support for implicit tiling parameters
> 
>  drivers/gpu/drm/tegra/drm.c  | 29 ++++++++++++++++++
>  drivers/gpu/drm/tegra/fb.c   | 59 ++++++++++++++++++++++++++++++++++--
>  include/uapi/drm/tegra_drm.h | 16 ++++++----
>  3 files changed, 96 insertions(+), 8 deletions(-)

We really don't want to use SET_TILING and GET_TILING IOCTLs anymore.
These only exist for backwards compatibility with very old userspace.
New code should use standard DRM/KMS mechanisms to deal with
framebuffer modifiers.

Thierry

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

Re: [PATCH 0/2] drm/tegra: handle implicit scanout modifiers

[Diogo Ivo]

On Tue, Jan 24, 2023 at 03:25:09PM +0100, Thierry Reding wrote:
> On Fri, Jan 20, 2023 at 10:58:56AM +0000, Diogo Ivo wrote:
> > Hello!
> > 
> > This patch series adds support for correctly displaying tiled
> > framebuffers when no modifiers are reported by userspace.
> > 
> > Patch 1 adds the sector_layout parameter to the SET/GET_TILING
> > IOCTLs so that userspace can set this field appropriately.
> > 
> > Patch 2 adds handling of the case where the buffer object
> > passed to create a framebuffer is allocated with non-linear
> > tiling but no modifier is reported.
> > 
> > Diogo Ivo (2):
> >   drm/tegra: add sector layout to SET/GET_TILING IOCTLs
> >   drm/tegra: add scanout support for implicit tiling parameters
> > 
> >  drivers/gpu/drm/tegra/drm.c  | 29 ++++++++++++++++++
> >  drivers/gpu/drm/tegra/fb.c   | 59 ++++++++++++++++++++++++++++++++++--
> >  include/uapi/drm/tegra_drm.h | 16 ++++++----
> >  3 files changed, 96 insertions(+), 8 deletions(-)
> 
> We really don't want to use SET_TILING and GET_TILING IOCTLs anymore.
> These only exist for backwards compatibility with very old userspace.
> New code should use standard DRM/KMS mechanisms to deal with
> framebuffer modifiers.

Hello,

Thank you for your review! This implementation is basically a copy of
what vc4 already does when importing resources with no modifiers
specified by userspace.

I looked into the DRM/KMS infrastructure and did not find a mechanism
to do this, but perhaps I am missing something; if this is the case,
I would be happy to submit a more fitting implementation, since handling
these implicit modifiers allows us to lift the restriction of linear
scanout buffers.

Best regards,
Diogo