Re: [PATCH v2 4/5] drm: Add support for handling linear tile formats

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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?

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel