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.gheor...@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.
> > >
> > > - 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.
> > >
> > >
> > > 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.
And upon further reflection that was in fact even baked into some
of the code in framebuffer_check() since years ago. But those sanity
checks can't catch someone using the tile row size as the pitch since
that's always greater or equal to the linear pitch, but clearly the
way the code was written assumes linear pitch.
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
