On Mon, Sep 03, 2018 at 09:26:24AM +0200, Daniel Vetter wrote:
On Fri, Aug 31, 2018 at 05:26:37PM +0100, Alexandru-Cosmin Gheorghe wrote:
Hi,
On Fri, Aug 31, 2018 at 05:12:24PM +0200, Daniel Vetter wrote:
On Fri, Aug 31, 2018 at 02:20:31PM +0300, Ville Syrjälä wrote:
On Fri, Aug 31, 2018 at 10:14:14AM +0200, Daniel Vetter wrote:
On Thu, Aug 23, 2018 at 06:43:40PM +0100, Alexandru-Cosmin Gheorghe wrote:
On Wed, Aug 22, 2018 at 10:18:51PM +0200, Daniel Vetter wrote: > On Tue, Aug 21, 2018 at 07:30:03PM +0100, Alexandru Gheorghe wrote: > > The previous patch added tile_w and tile_h, which represent the > > horizontal and vertical sizes of a tile. > > > > This one uses that to plumb through drm core in order to be able to > > handle linear tile formats without the need for drivers to roll up > > their own implementation. > > > > This patch had been written with Mali-dp X0L2 and X0L0 in mind which > > is a 1 plane YCbCr 420 format with a 2x2 tile, that uses in average 2 > > bytes per pixel and where tiles are laid out in a linear manner. > > > > Now what are the restrictions: > > > > 1. Pitch in bytes is expected to cover at least tile_h * width in > > pixels. Due to this the places where the pitch is checked/used need to > > be updated to take into consideration the tile_w, tile_h and > > tile_size. > > tile_size = cpp * tile_w * tile_h > > > > 2. When doing source cropping plane_src_x/y need to be a multiple of > > tile_w/tile_h and we need to take into consideration the tile_w/tile_h > > when computing the start address. > > > > For all non-tiled formats the tile_w and tile_h will be 1, so if I > > didn't miss anything nothing should change. > > > > Regarding multi-planar linear tile formats, I'm not sure how those > > should be handle I kind of assumed that tile_h/tile_w will have to be > > divided by horizontal/subsampling. Anyway, I think it's best to just > > put an warning in there and handle it when someone tries to add > > support for them. > > > > Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com > > --- > > drivers/gpu/drm/drm_atomic.c | 8 +++ > > drivers/gpu/drm/drm_fb_cma_helper.c | 11 ++++- > > drivers/gpu/drm/drm_fourcc.c | 52 ++++++++++++++++++++ > > drivers/gpu/drm/drm_framebuffer.c | 19 +++++-- > > drivers/gpu/drm/drm_gem_framebuffer_helper.c | 10 ++-- > > include/drm/drm_fourcc.h | 2 + > > 6 files changed, 94 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 3eb061e11e2e..7a3e893a4cd1 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -1087,6 +1087,14 @@ static int drm_atomic_plane_check(struct drm_plane *plane, > > return -ENOSPC; > > } > > > > + /* Make sure source coordinates are a multiple of tile sizes */ > > + if ((state->src_x >> 16) % state->fb->format->tile_w || > > + (state->src_y >> 16) % state->fb->format->tile_h) { > > + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] Source coordinates do not meet tile restrictions", > > + plane->base.id, plane->name); > > + return -EINVAL; > > + } > > + > > if (plane_switching_crtc(state->state, plane, state)) { > > DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n", > > plane->base.id, plane->name); > > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c > > index 47e0e2f6642d..4d8052adce67 100644 > > --- a/drivers/gpu/drm/drm_fb_cma_helper.c > > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c > > @@ -87,6 +87,8 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, > > struct drm_gem_cma_object *obj; > > dma_addr_t paddr; > > u8 h_div = 1, v_div = 1; > > + u32 tile_w = drm_format_tile_width(fb->format, plane); > > + u32 tile_h = drm_format_tile_height(fb->format, plane); > > > > obj = drm_fb_cma_get_gem_obj(fb, plane); > > if (!obj) > > @@ -99,8 +101,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, > > v_div = fb->format->vsub; > > } > > > > - paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div; > > - paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div; > > + paddr += (fb->format->cpp[plane] * tile_w * (state->src_x >> 16)) > > + / h_div; > > + /* > > + * For tile formats pitches are expected to cover at least > > + * width * tile_h pixels > > + */ > > + paddr += ((fb->pitches[plane] / tile_h) * (state->src_y >> 16)) / v_div; > > > > return paddr; > > } > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > > index f55cd93ba2d0..d6c9c5aa4036 100644 > > --- a/drivers/gpu/drm/drm_fourcc.c > > +++ b/drivers/gpu/drm/drm_fourcc.c > > @@ -557,3 +557,55 @@ int drm_format_plane_height(int height, uint32_t format, int plane) > > return height / info->vsub; > > } > > EXPORT_SYMBOL(drm_format_plane_height); > > + > > +/** > > + * drm_format_tile_width - width of a tile for tile formats, should be 1 for all > > + * non-tiled formats. > > + * @format: pixel format > > + * @plane: plane index > > + * > > + * Returns: > > + * The width of a tile, depending on the plane index and horizontal sub-sampling > > + */ > > +uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane) > > +{ > > + WARN_ON(!info->tile_w); > > + if (plane == 0 || info->tile_w == 1) > > + return info->tile_w; > > + > > + /* > > + * Multi planar tiled formats have never been tested, check that > > + * buffer restrictions and source cropping meet the format layout > > + * expectations. > > + */ > > + WARN_ON("Multi-planar tiled formats unsupported"); > > + WARN_ON(info->tile_w % info->hsub); > > + return info->tile_w / info->hsub; > > +} > > +EXPORT_SYMBOL(drm_format_tile_width); > > + > > +/** > > + * drm_format_tile_height - height of a tile for tile formats, should be 1 for > > + * all non-tiled formats. > > + * @format: pixel format > > + * @plane: plane index > > + * > > + * Returns: > > + * The height of a tile, depending on the plane index and vertical sub-sampling > > + */ > > +uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane) > > +{ > > + WARN_ON(!info->tile_h); > > + if (plane == 0 || info->tile_h == 1) > > + return info->tile_h; > > + > > + /* > > + * Multi planar tiled formats have never been tested, check that > > + * buffer restrictions and source cropping meet the format layout > > + * expectations. > > + */ > > + WARN_ON("Multi-planar tiled formats unsupported"); > > + WARN_ON(info->tile_h % info->vsub); > > + return info->tile_h / info->vsub; > > +} > > +EXPORT_SYMBOL(drm_format_tile_height); > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > > index 781af1d42d76..57509e51cb80 100644 > > --- a/drivers/gpu/drm/drm_framebuffer.c > > +++ b/drivers/gpu/drm/drm_framebuffer.c > > @@ -191,19 +191,32 @@ static int framebuffer_check(struct drm_device *dev, > > unsigned int width = fb_plane_width(r->width, info, i); > > unsigned int height = fb_plane_height(r->height, info, i); > > unsigned int cpp = info->cpp[i]; > > + unsigned int tile_w = drm_format_tile_width(info, i); > > + unsigned int tile_h = drm_format_tile_height(info, i); > > + unsigned int tile_size = cpp * tile_w * tile_h; > > + unsigned int num_htiles; > > + unsigned int num_vtiles; > > > > if (!r->handles[i]) { > > DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i); > > return -EINVAL; > > } > > > > - if ((uint64_t) width * cpp > UINT_MAX) > > + if ((width % tile_w) || (height % tile_h)) { > > I think this is too strict. You can carve out a sub-part of anything > really with a drm_framebuffer. See below, we only checked that width * cpp > < pitches, so leftover bytes/bits was always ok. > > Your driver might have additional constraints, but that's a different > story.
This seem like the only thing that we have in common with DRM_FORMAT_MOD_SAMSUNG_64_32_TILE/NV12MT, which is even more restrictive than that and it wants the width to be a multiple of 128.
> > > + DRM_DEBUG_KMS("buffer width/height need to be a multiple of tile dimensions\n"); > > + return -EINVAL; > > + } > > + > > + num_htiles = width / tile_w; > > + num_vtiles = height / tile_h; > > + > > + if ((uint64_t)num_htiles * tile_size > UINT_MAX) > > return -ERANGE; > > > > - if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX) > > + if ((uint64_t)num_vtiles * r->pitches[i] + r->offsets[i] > UINT_MAX) > > return -ERANGE; > > > > - if (r->pitches[i] < width * cpp) { > > + if (r->pitches[i] < num_htiles * tile_size) { > > Essentially you define ->pitches now to mean a row of tiles. At least for > bigger tiled formats (using modifiers) we don't use it like that. But it > also gets awkward quickly if we keep pitches to mean bytes in one row of > pixels. For that case we'd have
That's how everything started, we have already internal userspace and drivers that expect and pass pitch as a row of tiles in bytes.
For the tiles format in this patchset(X0L2/X0L0) which orders the tiles in linear manner the pitch will at least get you at the beginning of a row.
I'm not sure I understand what's the meaning of a pitch for DRM_FORMAT_MOD_SAMSUNG_64_32_TILE which orders the tiles in Z shape. How far will a pitch for this format get you, it seems that using just witdth * cpp will get you somewhere in the middle.
So there's 2 interpretations of pitch if you have a tile/block layout:
- What you do, where pitch = num_tiles_per_row * tile_size. Adding pitch to the base address will then advance tile_h pixels. Let's call this alex_pitch.
Is more or less mali_pitch, rather than alex_pitch.
- What I'm proposing, and what NV12MT does, and what every other driver does (afaik, I didn't carefully check them all): Pretend there's no tile, and compute the pitch as if it's a linear format. The reason behind this is that fairly often for tiled layouts (but not always), you have magic remapping hardware, which can present a linear layout to you. So for NV12MT that would resolve the Z-shape and macroblock layout, and give you a normal NV12 framebuffer. Let's call this drm_pitch. Note that the drm pitch does _not_ make any sense when you add it to the physical base address, only if you add mutliple's of tile_h, to get an entire tile row.
That's exactly what I'm proposing, for linear tile formats we should use a pitch that naturaly has sense which is drm_pitch * tile_h.
For any other shape of arranging arranging the tiles like the Z shape of NV12MT, the fake pitch meaning would remain unchanged.
The relationship is drm_pitch = alex_pitch / tile_h. I'm not aware of any format where that doesn't devide evenly, so in practice it doesn't matter which one you pick. Imo either has up/downsides.
Since everyone is already using the linear pitch approach I'd suggest sticking with that. Might make life a little easier for generic userspace (though it still needs to know other details of the tiling format of course).
Yeah, that's what I meant to add/clarify. The fake linear pitch is, to the best of my knowledge, already the standard we're using in drm. -Daniel
Everyone except Mali. Still, it seems like a good idea to transform something "fake" in something with meaning, of course without breaking the userspace :).
Do you mean this is already used like this in drm/malidp code? Or does "mali" here stand for the blob? Need to know, so we know what we'd break.
Blob and a bunch of internal stuff, nothing in the mainline. That's why I was trying to use the same convention in drm/malidp.
As Ville points out in his reply, drm_pitch is pretty much what the drm_framebuffer pitch is defined as, so we can't change that.
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch