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?
>
> 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.
-Daniel
>
>>
>> > > >
>> > > > 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)) {
>> > > > + 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) {
>> > > > DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i);
>> > > > return -EINVAL;
>> > > > }
>> > > > diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> > > > index 2810d4131411..3d01a1a9d5d2 100644
>> > > > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> > > > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
>> > > > @@ -161,6 +161,11 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
>> > > > unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
>> > > > unsigned int height = mode_cmd->height / (i ? info->vsub : 1);
>> > > > unsigned int min_size;
>> > > > + unsigned int tile_w = drm_format_tile_width(info, i);
>> > > > + unsigned int tile_h = drm_format_tile_height(info, i);
>> > > > + unsigned int tile_size = info->cpp[i] * tile_w * tile_h;
>> > > > + unsigned int num_htiles = width / tile_w;
>> > > > + unsigned int num_vtiles = height / tile_h;
>> > > >
>> > > > objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]);
>> > > > if (!objs[i]) {
>> > > > @@ -169,9 +174,8 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file,
>> > > > goto err_gem_object_put;
>> > > > }
>> > > >
>> > > > - min_size = (height - 1) * mode_cmd->pitches[i]
>> > > > - + width * info->cpp[i]
>> > > > - + mode_cmd->offsets[i];
>> > > > + min_size = (num_vtiles - 1) * mode_cmd->pitches[i]
>> > > > + + num_htiles * tile_size + mode_cmd->offsets[i];
>> > > >
>> > > > if (objs[i]->size < min_size) {
>> > > > drm_gem_object_put_unlocked(objs[i]);
>> > > > diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
>> > > > index 41681cf2b140..001afca9bcff 100644
>> > > > --- a/include/drm/drm_fourcc.h
>> > > > +++ b/include/drm/drm_fourcc.h
>> > > > @@ -76,6 +76,8 @@ int drm_format_horz_chroma_subsampling(uint32_t format);
>> > > > int drm_format_vert_chroma_subsampling(uint32_t format);
>> > > > int drm_format_plane_width(int width, uint32_t format, int plane);
>> > > > int drm_format_plane_height(int height, uint32_t format, int plane);
>> > > > +uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane);
>> > > > +uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane);
>> > > > const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
>> > > >
>> > > > #endif /* __DRM_FOURCC_H__ */
>> > > > --
>> > > > 2.18.0
>> > > >
>> > > > _______________________________________________
>> > > > dri-devel mailing list
>> > > > dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> > >
>> > > --
>> > > Ville Syrjälä
>> > > Intel
>> > > _______________________________________________
>> > > dri-devel mailing list
>> > > dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >
>> > --
>> > Cheers,
>> > Alex G
>>
>> --
>> Ville Syrjälä
>> Intel
>
> --
> Cheers,
> Alex G
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
