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@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@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