[PATCH v2 4/5] drm: Add support for handling linear tile formats
Daniel Vetter
daniel at ffwll.ch
Fri Aug 31 08:03:26 UTC 2018
On Thu, Aug 23, 2018 at 06:19:31PM +0100, Alexandru-Cosmin Gheorghe wrote:
> On Thu, Aug 23, 2018 at 05:30:36PM +0300, Ville Syrjälä wrote:
> > On Wed, Aug 22, 2018 at 09:48:29PM +0200, Daniel Vetter wrote:
> > > On Wed, Aug 22, 2018 at 4:05 PM, Alexandru-Cosmin Gheorghe
> > > <Alexandru-Cosmin.Gheorghe at arm.com> wrote:
> > > > On Wed, Aug 22, 2018 at 04:45:34PM +0300, Ville Syrjälä wrote:
> > > >> On Wed, Aug 22, 2018 at 02:36:06PM +0100, Alexandru-Cosmin Gheorghe wrote:
> > > >> > On Wed, Aug 22, 2018 at 04:18:54PM +0300, Ville Syrjälä 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 at 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;
> > > >> > > > + }
> > > >> > >
> > > >> > > Feels rather wrong to me to put such hardware specific limitations into
> > > >> > > the core.
> > > >> >
> > > >> > Yeah, the problem is that otherwise drm_fb_cma_get_gem_addr wouldn't
> > > >> > work.
> > > >>
> > > >> If that guy is supposed to give you a tile aligned address then it could
> > > >> just do that and leave it up to the driver to deal with the remainder of
> > > >> the coordinates?
> > > >>
> > > >> >
> > > >> > An alternative, would be to include it in the driver and put an WARN
> > > >> > in drm_fb_cma_get_gem_addr in case someone else uses it with a
> > > >> > src_x/src_y tile multiples.
> > > >> >
> > > >> > What do you think about that ?
> > > >> >
> > > >> > >
> > > >> > > > +
> > > >> > > > 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;
> > > >>
> > > >> Presumably this thing should be using the clipped coordinates instead of
> > > >> the raw user provided ones?
> > > >
> > > > So you expect the driver to thinker with state->src_x and state->src_y
> > > > to be tiled aligned?
> >
> > I was referring to using state->src.x1 etc. Using src_x/y will cause you
> > to scan out the wrong stuff if the plane top/left edge got clipped.
> > And as a side note: drivers are not even allowed to muck with src_x/y
> > etc. because that would corrupt the state that user has set up.
>
> Yes, it seems this function should have used src.x1, instead of
> src_x/y, but I'm not sure it can be easily fix since not all the
> drivers calling it call drm_atomic_helper_check_plane_state that
> populates src/dst rectangles.
>
> >
> > > >
> > > > What do you expect to happen if this function gets passed a
> > > > src_x/src_y that's not tile aligned, WARN about it or just silently
> > > > return the beginning of the tile and document that behaviour.
> > >
> > > WARN really hard and return NULL or something for any tiled format.
> > > Imo a plain address just doesn't make sense for a tile layout: To get
> > > a good starting point you need both a base address (which will be
> > > aligned to tile rows ofc), plus a x/y offset within. The driver can
> > > then decided whether it must have a tile offset of 0, or what exactly
> > > it can handle. So yeah I think we need a new helper here that gets
> > > this right.
> >
> > Yeah, something that either returns the remaining intra-tile x/y
> > offsets, or the tile aligned x/y offsets and lets the driver deal
> > with the remainder in whatever way is appropriate (which could even
> > be -EINVAL in case the hw can't simply handle it). The latter being
> > pretty much how i915 does things.
>
> Looking at DRM_FORMAT_MOD_SAMSUNG_64_32_TILE with its Z shape
> ordering of the tiles, it doesn't seem there could be any generic way
> of getting the address of a pixel, especially using just the tile_w
> and tile_h.
The Z-layout within a macroblock is totally irrelevant. What I have in
mind is something like intel_compute_tile_offset, but with explicit return
values instead of filling in the plane state (makes it easier to
integrate). What this does (more or less at least):
- Align the base address of the framebuffer to the nearest full tile row.
This is a full physical address.
- From that base address, which is fully aligned to tiles (so doesn't deal
with Z-shaped or anything like that), compute the remaining x/y offset
in _pixels_. Not bytes, because as you correctly note, for many tile
layouts a byte offset makes no sense at all.
Then drivers can convert that pixel offset into something meaningful for
them:
- Some have direct x/y pixel offset registers.
- Some just outright require that you align with the tile/block, so
anything != 0 would be converted to a -EINVAL.
- Some have other means (like converting to a byte address, because they
only deal with tile/block formats where that makes sense).
This concept was derived from the gallium format stuff (that's at least
where I've seen it first), and 3d gpus deal with a _lot_ fancier tile
layouts than even NV12MT :-) And it works.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list