On Fri, 30 Sep 2016 19:22:11 +0300
Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Fri, Sep 30, 2016 at 06:08:26PM +0200, Boris Brezillon wrote:
> > On Fri, 30 Sep 2016 16:33:20 +0200
> > Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > > Our planes cannot be set at negative coordinates. Make sure we reject such
> > > configuration.
> > >
> > > Reported-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/sun4i/sun4i_layer.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
> > > index f0035bf5efea..f5463c4c2cde 100644
> > > --- a/drivers/gpu/drm/sun4i/sun4i_layer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
> > > @@ -29,6 +29,9 @@ struct sun4i_plane_desc {
> > > static int sun4i_backend_layer_atomic_check(struct drm_plane *plane,
> > > struct drm_plane_state *state)
> > > {
> > > + if ((state->crtc_x < 0) || (state->crtc_y < 0))
> > > + return -EINVAL;
> > > +
> >
> > Hm, I think it's a perfectly valid use case from the DRM framework and
> > DRM user PoV: you may want to place your plane at a negative CRTC
> > offset (which means part of the plane will be hidden).
> >
> > Maybe I'm wrong, but it seems you can support that by adapting the
> > start address of your framebuffer pointer and the layer size.
> >
> > Have you tried doing something like that?
>
> You shouldn't even be looking at the user provided coordinates. Also
> you can't return an error from the atomic update hook. The void return
> value should have been a decent hint ;)
Note that Maxime is not returning a value from the atomic update
implementation (it's done in the atomic_check implem), and I'm not
checking crtc_x,y consistency at all (which is obviously wrong), I'm
just blindly patching the values in sun4i_backend helpers.
> The right fix would be
> to move all the error handling into the atomic check hook, which
> probably should just call the helper to clip the coordinates and
> whatnot. Then the update hook can just use at the clipped
> coordinates when programming the hw registers.
That's probably the best approach indeed, but that means having our
private sun4i_plane_state struct where we would store the patched
crtc_{w,h,x,y} info.
Anyway, before we do that, that's probably better to check if it really
works on this HW (which is why I sent this informal patch).
>
> >
> > --->8---
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > index 3ab560450a82..6b68804f3035 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> > @@ -110,15 +110,30 @@ int sun4i_backend_update_layer_coord(struct sun4i_backend *backend,
> > {
> > struct drm_plane_state *state = plane->state;
> > struct drm_framebuffer *fb = state->fb;
> > + int crtc_w, crtc_h, crtc_x, crtc_y;
> >
> > DRM_DEBUG_DRIVER("Updating layer %d\n", layer);
> >
> > + crtc_x = state->crtc_x;
> > + crtc_y = state->crtc_y;
> > + crtc_w = state->crtc_w;
> > + crtc_h = state->crtc_h;
> > +
> > + if (crtc_x < 0) {
> > + crtc_w += crtx_x;
> > + crtc_x = 0;
> > + }
> > +
> > + if (crtc_y < 0) {
> > + crtc_h += crtx_y;
> > + crtc_y = 0;
> > + }
> > +
> > if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> > DRM_DEBUG_DRIVER("Primary layer, updating global size W: %u H: %u\n",
> > state->crtc_w, state->crtc_h);
> > regmap_write(backend->regs, SUN4I_BACKEND_DISSIZE_REG,
> > - SUN4I_BACKEND_DISSIZE(state->crtc_w,
> > - state->crtc_h));
> > + SUN4I_BACKEND_DISSIZE(crtc_w, crtc_h));
> > }
> >
> > /* Set the line width */
> > @@ -130,15 +145,13 @@ int sun4i_backend_update_layer_coord(struct sun4i_backend *backend,
> > DRM_DEBUG_DRIVER("Layer size W: %u H: %u\n",
> > state->crtc_w, state->crtc_h);
> > regmap_write(backend->regs, SUN4I_BACKEND_LAYSIZE_REG(layer),
> > - SUN4I_BACKEND_LAYSIZE(state->crtc_w,
> > - state->crtc_h));
> > + SUN4I_BACKEND_LAYSIZE(crtc_w, crtc_h));
> >
> > /* Set base coordinates */
> > DRM_DEBUG_DRIVER("Layer coordinates X: %d Y: %d\n",
> > state->crtc_x, state->crtc_y);
> > regmap_write(backend->regs, SUN4I_BACKEND_LAYCOOR_REG(layer),
> > - SUN4I_BACKEND_LAYCOOR(state->crtc_x,
> > - state->crtc_y));
> > + SUN4I_BACKEND_LAYCOOR(crtc_x, crtc_y));
> >
> > return 0;
> > }
> > @@ -198,6 +211,12 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend,
> > paddr += (state->src_x >> 16) * bpp;
> > paddr += (state->src_y >> 16) * fb->pitches[0];
> >
> > + if (state->crtc_x < 0)
> > + paddr -= bpp * state->crtc_x;
> > +
> > + if (state->crtc_y < 0)
> > + paddr -= fb->pitches[0] * state->crtc_y;
> > +
> > DRM_DEBUG_DRIVER("Setting buffer address to %pad\n", &paddr);
> >
> > /* Write the 32 lower bits of the address (in bits) */
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
