[13/13] drm/sun4i: Align VI buffer addresses for subsampled formats

This is a partial revert of commit 79ac1c945ab8 ("drm/sun4i: layers:
Use drm_fb_dma_get_gem_addr() to get display memory").

Chroma must start at the beginning of a subsampling block, for example
chroma start address for NV12 must be aligned to 2 pixels.
drm_fb_dma_get_gem_addr() offsets luma by the exact source coordinates
and chroma by the coordinates divided by the subsampling factor, so for
odd offsets both planes no longer describe the same pixel, which the
Display Engine scaler can't handle.

Align source coordinates down for all planes instead. Remaining shift
of one pixel is already compensated with scaler phase shift in
sun8i_vi_layer_update_coord().

Fixes: 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to get display memory")
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)
  

On Tue, Aug 4, 2026 at 12:11 AM Jernej Skrabec <jernej.skrabec@gmail.com> wrote:
>
> This is a partial revert of commit 79ac1c945ab8 ("drm/sun4i: layers:
> Use drm_fb_dma_get_gem_addr() to get display memory").
>
> Chroma must start at the beginning of a subsampling block, for example
> chroma start address for NV12 must be aligned to 2 pixels.
> drm_fb_dma_get_gem_addr() offsets luma by the exact source coordinates
> and chroma by the coordinates divided by the subsampling factor, so for
> odd offsets both planes no longer describe the same pixel, which the
> Display Engine scaler can't handle.
>
> Align source coordinates down for all planes instead. Remaining shift
> of one pixel is already compensated with scaler phase shift in
> sun8i_vi_layer_update_coord().

Well I think this applies to the format in general, and probably should
be fixed in drm_fb_dma_get_gem_addr() instead?

> Fixes: 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to get display memory")
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> index 09f668c8af24..ad036cb9d88e 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> @@ -197,15 +197,31 @@ static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer,
>         struct drm_plane_state *state = plane->state;
>         struct drm_framebuffer *fb = state->fb;
>         const struct drm_format_info *format = fb->format;
> +       struct drm_gem_dma_object *gem;
> +       u32 dx, dy, src_x, src_y;
>         dma_addr_t dma_addr;
>         u32 ch_base;
>         int i;
>
>         ch_base = sun8i_channel_base(layer);
>
> +       /* Adjust x and y to be divisible by subsampling factor */
> +       src_x = (state->src.x1 >> 16) & ~(format->hsub - 1);
> +       src_y = (state->src.y1 >> 16) & ~(format->vsub - 1);

AFAICT the only difference compared to drm_fb_dma_get_gem_addr()
is the masking here, i.e. round_down().

> +
>         for (i = 0; i < format->num_planes; i++) {
> -               /* Get the start of the displayed memory */
> -               dma_addr = drm_fb_dma_get_gem_addr(fb, state, i);
> +               gem = drm_fb_dma_get_gem_obj(fb, i);
> +               dma_addr = gem->dma_addr + fb->offsets[i];
> +
> +               dx = src_x;
> +               dy = src_y;
> +               if (i > 0) {
> +                       dx /= format->hsub;
> +                       dy /= format->vsub;
> +               }
> +
> +               dma_addr += dx * format->cpp[i];
> +               dma_addr += dy * fb->pitches[i];


Where as the helper has (or used to have before the blocksize stuff):

    paddr += (format->cpp[plane] * (state->src_x >> 16)) / fb->format->hsub;
    paddr += (fb->pitches[plane] * (state->src_y >> 16)) / fb->format->vsub;

Am I missing something?


ChenYu

>
>                 /* Set the line width */
>                 DRM_DEBUG_DRIVER("Layer %d. line width: %d bytes\n",
> --
> 2.43.0
>
  

On Tue, Aug 4, 2026 at 1:25 AM Chen-Yu Tsai <wens@kernel.org> wrote:
>
> On Tue, Aug 4, 2026 at 12:11 AM Jernej Skrabec <jernej.skrabec@gmail.com> wrote:
> >
> > This is a partial revert of commit 79ac1c945ab8 ("drm/sun4i: layers:
> > Use drm_fb_dma_get_gem_addr() to get display memory").
> >
> > Chroma must start at the beginning of a subsampling block, for example
> > chroma start address for NV12 must be aligned to 2 pixels.
> > drm_fb_dma_get_gem_addr() offsets luma by the exact source coordinates
> > and chroma by the coordinates divided by the subsampling factor, so for
> > odd offsets both planes no longer describe the same pixel, which the
> > Display Engine scaler can't handle.
> >
> > Align source coordinates down for all planes instead. Remaining shift
> > of one pixel is already compensated with scaler phase shift in
> > sun8i_vi_layer_update_coord().
>
> Well I think this applies to the format in general, and probably should
> be fixed in drm_fb_dma_get_gem_addr() instead?
>
> > Fixes: 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to get display memory")
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > index 09f668c8af24..ad036cb9d88e 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > @@ -197,15 +197,31 @@ static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer,
> >         struct drm_plane_state *state = plane->state;
> >         struct drm_framebuffer *fb = state->fb;
> >         const struct drm_format_info *format = fb->format;
> > +       struct drm_gem_dma_object *gem;
> > +       u32 dx, dy, src_x, src_y;
> >         dma_addr_t dma_addr;
> >         u32 ch_base;
> >         int i;
> >
> >         ch_base = sun8i_channel_base(layer);
> >
> > +       /* Adjust x and y to be divisible by subsampling factor */
> > +       src_x = (state->src.x1 >> 16) & ~(format->hsub - 1);
> > +       src_y = (state->src.y1 >> 16) & ~(format->vsub - 1);
>
> AFAICT the only difference compared to drm_fb_dma_get_gem_addr()
> is the masking here, i.e. round_down().
>
> > +
> >         for (i = 0; i < format->num_planes; i++) {
> > -               /* Get the start of the displayed memory */
> > -               dma_addr = drm_fb_dma_get_gem_addr(fb, state, i);
> > +               gem = drm_fb_dma_get_gem_obj(fb, i);
> > +               dma_addr = gem->dma_addr + fb->offsets[i];
> > +
> > +               dx = src_x;
> > +               dy = src_y;
> > +               if (i > 0) {
> > +                       dx /= format->hsub;
> > +                       dy /= format->vsub;
> > +               }
> > +
> > +               dma_addr += dx * format->cpp[i];
> > +               dma_addr += dy * fb->pitches[i];
>
>
> Where as the helper has (or used to have before the blocksize stuff):
>
>     paddr += (format->cpp[plane] * (state->src_x >> 16)) / fb->format->hsub;
>     paddr += (fb->pitches[plane] * (state->src_y >> 16)) / fb->format->vsub;
>
> Am I missing something?

After some headbanging on my end I see that the offset for the Y plane
needs to be rounded down.

But instead of reverting the whole thing and open-coding the helper
again, could you adjust the address returned by the helper for odd
offsets?

And just a heads up, this also needs a clipped version of
drm_fb_dma_get_gem_addr() as sun8i_ui_layer_update_coord() uses the
clipped dimensions. I am currently working on this part.


ChenYu

> >
> >                 /* Set the line width */
> >                 DRM_DEBUG_DRIVER("Layer %d. line width: %d bytes\n",
> > --
> > 2.43.0
> >
  

Dne torek, 4. avgust 2026 ob 13:14:38 Srednjeevropski poletni čas je Chen-Yu Tsai napisal(a):
> On Tue, Aug 4, 2026 at 1:25 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> >
> > On Tue, Aug 4, 2026 at 12:11 AM Jernej Skrabec <jernej.skrabec@gmail.com> wrote:
> > >
> > > This is a partial revert of commit 79ac1c945ab8 ("drm/sun4i: layers:
> > > Use drm_fb_dma_get_gem_addr() to get display memory").
> > >
> > > Chroma must start at the beginning of a subsampling block, for example
> > > chroma start address for NV12 must be aligned to 2 pixels.
> > > drm_fb_dma_get_gem_addr() offsets luma by the exact source coordinates
> > > and chroma by the coordinates divided by the subsampling factor, so for
> > > odd offsets both planes no longer describe the same pixel, which the
> > > Display Engine scaler can't handle.
> > >
> > > Align source coordinates down for all planes instead. Remaining shift
> > > of one pixel is already compensated with scaler phase shift in
> > > sun8i_vi_layer_update_coord().
> >
> > Well I think this applies to the format in general, and probably should
> > be fixed in drm_fb_dma_get_gem_addr() instead?
> >
> > > Fixes: 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to get display memory")
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > ---
> > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 20 ++++++++++++++++++--
> > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > index 09f668c8af24..ad036cb9d88e 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > @@ -197,15 +197,31 @@ static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer,
> > >         struct drm_plane_state *state = plane->state;
> > >         struct drm_framebuffer *fb = state->fb;
> > >         const struct drm_format_info *format = fb->format;
> > > +       struct drm_gem_dma_object *gem;
> > > +       u32 dx, dy, src_x, src_y;
> > >         dma_addr_t dma_addr;
> > >         u32 ch_base;
> > >         int i;
> > >
> > >         ch_base = sun8i_channel_base(layer);
> > >
> > > +       /* Adjust x and y to be divisible by subsampling factor */
> > > +       src_x = (state->src.x1 >> 16) & ~(format->hsub - 1);
> > > +       src_y = (state->src.y1 >> 16) & ~(format->vsub - 1);
> >
> > AFAICT the only difference compared to drm_fb_dma_get_gem_addr()
> > is the masking here, i.e. round_down().
> >
> > > +
> > >         for (i = 0; i < format->num_planes; i++) {
> > > -               /* Get the start of the displayed memory */
> > > -               dma_addr = drm_fb_dma_get_gem_addr(fb, state, i);
> > > +               gem = drm_fb_dma_get_gem_obj(fb, i);
> > > +               dma_addr = gem->dma_addr + fb->offsets[i];
> > > +
> > > +               dx = src_x;
> > > +               dy = src_y;
> > > +               if (i > 0) {
> > > +                       dx /= format->hsub;
> > > +                       dy /= format->vsub;
> > > +               }
> > > +
> > > +               dma_addr += dx * format->cpp[i];
> > > +               dma_addr += dy * fb->pitches[i];
> >
> >
> > Where as the helper has (or used to have before the blocksize stuff):
> >
> >     paddr += (format->cpp[plane] * (state->src_x >> 16)) / fb->format->hsub;
> >     paddr += (fb->pitches[plane] * (state->src_y >> 16)) / fb->format->vsub;
> >
> > Am I missing something?
> 
> After some headbanging on my end I see that the offset for the Y plane
> needs to be rounded down.
> 
> But instead of reverting the whole thing and open-coding the helper
> again, could you adjust the address returned by the helper for odd
> offsets?

Yes, that's also an option. I'll do it in v2.

> 
> And just a heads up, this also needs a clipped version of
> drm_fb_dma_get_gem_addr() as sun8i_ui_layer_update_coord() uses the
> clipped dimensions. I am currently working on this part.

Can you explain a bit more? I don't see why it needs any adjustement.

Best regards,
Jernej

> 
> 
> ChenYu
> 
> > >
> > >                 /* Set the line width */
> > >                 DRM_DEBUG_DRIVER("Layer %d. line width: %d bytes\n",
> > > --
> > > 2.43.0
> > >
>
  

On Wed, Aug 5, 2026 at 12:25 AM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
>
> Dne torek, 4. avgust 2026 ob 13:14:38 Srednjeevropski poletni čas je Chen-Yu Tsai napisal(a):
> > On Tue, Aug 4, 2026 at 1:25 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> > >
> > > On Tue, Aug 4, 2026 at 12:11 AM Jernej Skrabec <jernej.skrabec@gmail.com> wrote:
> > > >
> > > > This is a partial revert of commit 79ac1c945ab8 ("drm/sun4i: layers:
> > > > Use drm_fb_dma_get_gem_addr() to get display memory").
> > > >
> > > > Chroma must start at the beginning of a subsampling block, for example
> > > > chroma start address for NV12 must be aligned to 2 pixels.
> > > > drm_fb_dma_get_gem_addr() offsets luma by the exact source coordinates
> > > > and chroma by the coordinates divided by the subsampling factor, so for
> > > > odd offsets both planes no longer describe the same pixel, which the
> > > > Display Engine scaler can't handle.
> > > >
> > > > Align source coordinates down for all planes instead. Remaining shift
> > > > of one pixel is already compensated with scaler phase shift in
> > > > sun8i_vi_layer_update_coord().
> > >
> > > Well I think this applies to the format in general, and probably should
> > > be fixed in drm_fb_dma_get_gem_addr() instead?
> > >
> > > > Fixes: 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to get display memory")
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > > ---
> > > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 20 ++++++++++++++++++--
> > > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > index 09f668c8af24..ad036cb9d88e 100644
> > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > @@ -197,15 +197,31 @@ static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer,
> > > >         struct drm_plane_state *state = plane->state;
> > > >         struct drm_framebuffer *fb = state->fb;
> > > >         const struct drm_format_info *format = fb->format;
> > > > +       struct drm_gem_dma_object *gem;
> > > > +       u32 dx, dy, src_x, src_y;
> > > >         dma_addr_t dma_addr;
> > > >         u32 ch_base;
> > > >         int i;
> > > >
> > > >         ch_base = sun8i_channel_base(layer);
> > > >
> > > > +       /* Adjust x and y to be divisible by subsampling factor */
> > > > +       src_x = (state->src.x1 >> 16) & ~(format->hsub - 1);
> > > > +       src_y = (state->src.y1 >> 16) & ~(format->vsub - 1);
> > >
> > > AFAICT the only difference compared to drm_fb_dma_get_gem_addr()
> > > is the masking here, i.e. round_down().
> > >
> > > > +
> > > >         for (i = 0; i < format->num_planes; i++) {
> > > > -               /* Get the start of the displayed memory */
> > > > -               dma_addr = drm_fb_dma_get_gem_addr(fb, state, i);
> > > > +               gem = drm_fb_dma_get_gem_obj(fb, i);
> > > > +               dma_addr = gem->dma_addr + fb->offsets[i];
> > > > +
> > > > +               dx = src_x;
> > > > +               dy = src_y;
> > > > +               if (i > 0) {
> > > > +                       dx /= format->hsub;
> > > > +                       dy /= format->vsub;
> > > > +               }
> > > > +
> > > > +               dma_addr += dx * format->cpp[i];
> > > > +               dma_addr += dy * fb->pitches[i];
> > >
> > >
> > > Where as the helper has (or used to have before the blocksize stuff):
> > >
> > >     paddr += (format->cpp[plane] * (state->src_x >> 16)) / fb->format->hsub;
> > >     paddr += (fb->pitches[plane] * (state->src_y >> 16)) / fb->format->vsub;
> > >
> > > Am I missing something?
> >
> > After some headbanging on my end I see that the offset for the Y plane
> > needs to be rounded down.
> >
> > But instead of reverting the whole thing and open-coding the helper
> > again, could you adjust the address returned by the helper for odd
> > offsets?
>
> Yes, that's also an option. I'll do it in v2.
>
> >
> > And just a heads up, this also needs a clipped version of
> > drm_fb_dma_get_gem_addr() as sun8i_ui_layer_update_coord() uses the
> > clipped dimensions. I am currently working on this part.
>
> Can you explain a bit more? I don't see why it needs any adjustement.

My understanding is that drm_atomic_helper_check_plane_state() calculates
the "clipped" rectangles for the plane using values from userspace in
state->src_[xywh] and state->crtc_[xywh] and puts them in state->src
and state->dst, respectively. If the overlay is moved partially outside
the screen, the overlay is "clipped".


Say we have a screen of 1920x1080, with an overlay buffer that is 1280x720.
Say state->src_x and state->src_y are (-50, 0), given by userspace.
drm_atomic_helper_check_plane_state() will calculate the clipped & scaled
rectangles and put them in state->src. This latter rectangle is what is
used sun8i_ui_layer_update_coord().

So we would have:

    (src_x, src_y) = (-50, 0), (src_w, src_h) = (1280, 720)

The clipped numbers are

    (src.x1, src.y1) = (0, 0), (src.x2, src.y2) = (1230, 720)

src, not src_[xywh], is what sun8i layers uses to program the coordinates,
and prior to the drm_fb_dma_get_gem_addr() conversion, also to calculate
the buffer start address. drm_fb_dma_get_gem_addr() however uses src_[xy]
to calculate the address.


Essentially, when overlaying a clipped plane, the start address needs to
be adjusted if the source (top left) offset is outside the screen.
clipping == automatic cropping to fit the screen.

I don't know if userspace applications routinely do this, but I think this
needs to be restored to the prior behavior.


ChenYu
  

Dne torek, 4. avgust 2026 ob 19:04:10 Srednjeevropski poletni čas je Chen-Yu Tsai napisal(a):
> On Wed, Aug 5, 2026 at 12:25 AM Jernej Škrabec <jernej.skrabec@gmail.com> wrote:
> >
> > Dne torek, 4. avgust 2026 ob 13:14:38 Srednjeevropski poletni čas je Chen-Yu Tsai napisal(a):
> > > On Tue, Aug 4, 2026 at 1:25 AM Chen-Yu Tsai <wens@kernel.org> wrote:
> > > >
> > > > On Tue, Aug 4, 2026 at 12:11 AM Jernej Skrabec <jernej.skrabec@gmail.com> wrote:
> > > > >
> > > > > This is a partial revert of commit 79ac1c945ab8 ("drm/sun4i: layers:
> > > > > Use drm_fb_dma_get_gem_addr() to get display memory").
> > > > >
> > > > > Chroma must start at the beginning of a subsampling block, for example
> > > > > chroma start address for NV12 must be aligned to 2 pixels.
> > > > > drm_fb_dma_get_gem_addr() offsets luma by the exact source coordinates
> > > > > and chroma by the coordinates divided by the subsampling factor, so for
> > > > > odd offsets both planes no longer describe the same pixel, which the
> > > > > Display Engine scaler can't handle.
> > > > >
> > > > > Align source coordinates down for all planes instead. Remaining shift
> > > > > of one pixel is already compensated with scaler phase shift in
> > > > > sun8i_vi_layer_update_coord().
> > > >
> > > > Well I think this applies to the format in general, and probably should
> > > > be fixed in drm_fb_dma_get_gem_addr() instead?
> > > >
> > > > > Fixes: 79ac1c945ab8 ("drm/sun4i: layers: Use drm_fb_dma_get_gem_addr() to get display memory")
> > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > > > ---
> > > > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 20 ++++++++++++++++++--
> > > > >  1 file changed, 18 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > index 09f668c8af24..ad036cb9d88e 100644
> > > > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > > > @@ -197,15 +197,31 @@ static void sun8i_vi_layer_update_buffer(struct sun8i_layer *layer,
> > > > >         struct drm_plane_state *state = plane->state;
> > > > >         struct drm_framebuffer *fb = state->fb;
> > > > >         const struct drm_format_info *format = fb->format;
> > > > > +       struct drm_gem_dma_object *gem;
> > > > > +       u32 dx, dy, src_x, src_y;
> > > > >         dma_addr_t dma_addr;
> > > > >         u32 ch_base;
> > > > >         int i;
> > > > >
> > > > >         ch_base = sun8i_channel_base(layer);
> > > > >
> > > > > +       /* Adjust x and y to be divisible by subsampling factor */
> > > > > +       src_x = (state->src.x1 >> 16) & ~(format->hsub - 1);
> > > > > +       src_y = (state->src.y1 >> 16) & ~(format->vsub - 1);
> > > >
> > > > AFAICT the only difference compared to drm_fb_dma_get_gem_addr()
> > > > is the masking here, i.e. round_down().
> > > >
> > > > > +
> > > > >         for (i = 0; i < format->num_planes; i++) {
> > > > > -               /* Get the start of the displayed memory */
> > > > > -               dma_addr = drm_fb_dma_get_gem_addr(fb, state, i);
> > > > > +               gem = drm_fb_dma_get_gem_obj(fb, i);
> > > > > +               dma_addr = gem->dma_addr + fb->offsets[i];
> > > > > +
> > > > > +               dx = src_x;
> > > > > +               dy = src_y;
> > > > > +               if (i > 0) {
> > > > > +                       dx /= format->hsub;
> > > > > +                       dy /= format->vsub;
> > > > > +               }
> > > > > +
> > > > > +               dma_addr += dx * format->cpp[i];
> > > > > +               dma_addr += dy * fb->pitches[i];
> > > >
> > > >
> > > > Where as the helper has (or used to have before the blocksize stuff):
> > > >
> > > >     paddr += (format->cpp[plane] * (state->src_x >> 16)) / fb->format->hsub;
> > > >     paddr += (fb->pitches[plane] * (state->src_y >> 16)) / fb->format->vsub;
> > > >
> > > > Am I missing something?
> > >
> > > After some headbanging on my end I see that the offset for the Y plane
> > > needs to be rounded down.
> > >
> > > But instead of reverting the whole thing and open-coding the helper
> > > again, could you adjust the address returned by the helper for odd
> > > offsets?
> >
> > Yes, that's also an option. I'll do it in v2.
> >
> > >
> > > And just a heads up, this also needs a clipped version of
> > > drm_fb_dma_get_gem_addr() as sun8i_ui_layer_update_coord() uses the
> > > clipped dimensions. I am currently working on this part.
> >
> > Can you explain a bit more? I don't see why it needs any adjustement.
> 
> My understanding is that drm_atomic_helper_check_plane_state() calculates
> the "clipped" rectangles for the plane using values from userspace in
> state->src_[xywh] and state->crtc_[xywh] and puts them in state->src
> and state->dst, respectively. If the overlay is moved partially outside
> the screen, the overlay is "clipped".
> 
> 
> Say we have a screen of 1920x1080, with an overlay buffer that is 1280x720.
> Say state->src_x and state->src_y are (-50, 0), given by userspace.
> drm_atomic_helper_check_plane_state() will calculate the clipped & scaled
> rectangles and put them in state->src. This latter rectangle is what is
> used sun8i_ui_layer_update_coord().
> 
> So we would have:
> 
>     (src_x, src_y) = (-50, 0), (src_w, src_h) = (1280, 720)
> 
> The clipped numbers are
> 
>     (src.x1, src.y1) = (0, 0), (src.x2, src.y2) = (1230, 720)
> 
> src, not src_[xywh], is what sun8i layers uses to program the coordinates,
> and prior to the drm_fb_dma_get_gem_addr() conversion, also to calculate
> the buffer start address. drm_fb_dma_get_gem_addr() however uses src_[xy]
> to calculate the address.
> 
> 
> Essentially, when overlaying a clipped plane, the start address needs to
> be adjusted if the source (top left) offset is outside the screen.
> clipping == automatic cropping to fit the screen.
> 
> I don't know if userspace applications routinely do this, but I think this
> needs to be restored to the prior behavior.

Uh, it would be nice if this is fixed. But I think it's not too common
for app to use negative coordinates.

Best regards,
Jernej