* [PATCH 0/2] drm/tegra: handle implicit scanout modifiers
@ 2023-01-20 10:58 Diogo Ivo
2023-01-20 10:58 ` [PATCH 1/2] drm/tegra: add sector layout to SET/GET_TILING IOCTLs Diogo Ivo
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Diogo Ivo @ 2023-01-20 10:58 UTC (permalink / raw)
To: diogo.ivo, thierry.reding, airlied, daniel, jonathanh
Cc: dri-devel, linux-tegra
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] drm/tegra: add sector layout to SET/GET_TILING IOCTLs
2023-01-20 10:58 [PATCH 0/2] drm/tegra: handle implicit scanout modifiers Diogo Ivo
@ 2023-01-20 10:58 ` Diogo Ivo
2023-01-20 10:58 ` [PATCH 2/2] drm/tegra: add scanout support for implicit tiling parameters Diogo Ivo
2023-01-24 14:25 ` [PATCH 0/2] drm/tegra: handle implicit scanout modifiers Thierry Reding
2 siblings, 0 replies; 6+ messages in thread
From: Diogo Ivo @ 2023-01-20 10:58 UTC (permalink / raw)
To: diogo.ivo, thierry.reding, airlied, daniel, jonathanh
Cc: dri-devel, linux-tegra
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm/tegra: add scanout support for implicit tiling parameters
2023-01-20 10:58 [PATCH 0/2] drm/tegra: handle implicit scanout modifiers Diogo Ivo
2023-01-20 10:58 ` [PATCH 1/2] drm/tegra: add sector layout to SET/GET_TILING IOCTLs Diogo Ivo
@ 2023-01-20 10:58 ` Diogo Ivo
2023-01-30 19:43 ` kernel test robot
2023-01-24 14:25 ` [PATCH 0/2] drm/tegra: handle implicit scanout modifiers Thierry Reding
2 siblings, 1 reply; 6+ messages in thread
From: Diogo Ivo @ 2023-01-20 10:58 UTC (permalink / raw)
To: diogo.ivo, thierry.reding, airlied, daniel, jonathanh
Cc: dri-devel, linux-tegra
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] drm/tegra: handle implicit scanout modifiers
2023-01-20 10:58 [PATCH 0/2] drm/tegra: handle implicit scanout modifiers Diogo Ivo
2023-01-20 10:58 ` [PATCH 1/2] drm/tegra: add sector layout to SET/GET_TILING IOCTLs Diogo Ivo
2023-01-20 10:58 ` [PATCH 2/2] drm/tegra: add scanout support for implicit tiling parameters Diogo Ivo
@ 2023-01-24 14:25 ` Thierry Reding
2023-01-30 14:49 ` Diogo Ivo
2 siblings, 1 reply; 6+ messages in thread
From: Thierry Reding @ 2023-01-24 14:25 UTC (permalink / raw)
To: Diogo Ivo; +Cc: airlied, daniel, jonathanh, dri-devel, linux-tegra
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] drm/tegra: handle implicit scanout modifiers
2023-01-24 14:25 ` [PATCH 0/2] drm/tegra: handle implicit scanout modifiers Thierry Reding
@ 2023-01-30 14:49 ` Diogo Ivo
0 siblings, 0 replies; 6+ messages in thread
From: Diogo Ivo @ 2023-01-30 14:49 UTC (permalink / raw)
To: Thierry Reding; +Cc: airlied, daniel, jonathanh, dri-devel, linux-tegra
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/tegra: add scanout support for implicit tiling parameters
2023-01-20 10:58 ` [PATCH 2/2] drm/tegra: add scanout support for implicit tiling parameters Diogo Ivo
@ 2023-01-30 19:43 ` kernel test robot
0 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2023-01-30 19:43 UTC (permalink / raw)
To: Diogo Ivo, thierry.reding, airlied, daniel, jonathanh
Cc: llvm, oe-kbuild-all, linux-tegra, dri-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-30 19:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-20 10:58 [PATCH 0/2] drm/tegra: handle implicit scanout modifiers Diogo Ivo
2023-01-20 10:58 ` [PATCH 1/2] drm/tegra: add sector layout to SET/GET_TILING IOCTLs Diogo Ivo
2023-01-20 10:58 ` [PATCH 2/2] drm/tegra: add scanout support for implicit tiling parameters Diogo Ivo
2023-01-30 19:43 ` kernel test robot
2023-01-24 14:25 ` [PATCH 0/2] drm/tegra: handle implicit scanout modifiers Thierry Reding
2023-01-30 14:49 ` Diogo Ivo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).