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@xxxxxxx> 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@xxxxxxx>
> >> > > > ---
> >> > > > 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.
> >
> > 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.
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
