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.
Time to fix those drivers?