* [PATCH] drm/sun4i: Check that the plane coordinates are not negative @ 2016-09-30 14:33 Maxime Ripard 2016-09-30 16:08 ` Boris Brezillon 0 siblings, 1 reply; 8+ messages in thread From: Maxime Ripard @ 2016-09-30 14:33 UTC (permalink / raw) To: linux-arm-kernel Our planes cannot be set at negative coordinates. Make sure we reject such configuration. Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> --- 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; + return 0; } -- 2.9.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] drm/sun4i: Check that the plane coordinates are not negative 2016-09-30 14:33 [PATCH] drm/sun4i: Check that the plane coordinates are not negative Maxime Ripard @ 2016-09-30 16:08 ` Boris Brezillon 2016-09-30 16:22 ` Ville Syrjälä 2016-10-03 12:58 ` Maxime Ripard 0 siblings, 2 replies; 8+ messages in thread From: Boris Brezillon @ 2016-09-30 16:08 UTC (permalink / raw) To: linux-arm-kernel On Fri, 30 Sep 2016 16:33:20 +0200 Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Our planes cannot be set at negative coordinates. Make sure we reject such > configuration. > > Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > 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? --->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) */ ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] drm/sun4i: Check that the plane coordinates are not negative 2016-09-30 16:08 ` Boris Brezillon @ 2016-09-30 16:22 ` Ville Syrjälä 2016-09-30 16:33 ` Boris Brezillon 2016-10-03 12:58 ` Maxime Ripard 1 sibling, 1 reply; 8+ messages in thread From: Ville Syrjälä @ 2016-09-30 16:22 UTC (permalink / raw) To: linux-arm-kernel 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@free-electrons.com> wrote: > > > Our planes cannot be set at negative coordinates. Make sure we reject such > > configuration. > > > > Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > --- > > 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 ;) 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. > > --->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 at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrj?l? Intel OTC ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] drm/sun4i: Check that the plane coordinates are not negative 2016-09-30 16:22 ` Ville Syrjälä @ 2016-09-30 16:33 ` Boris Brezillon 2016-09-30 20:42 ` Ville Syrjälä 0 siblings, 1 reply; 8+ messages in thread From: Boris Brezillon @ 2016-09-30 16:33 UTC (permalink / raw) To: linux-arm-kernel On Fri, 30 Sep 2016 19:22:11 +0300 Ville Syrj?l? <ville.syrjala@linux.intel.com> 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@free-electrons.com> wrote: > > > > > Our planes cannot be set at negative coordinates. Make sure we reject such > > > configuration. > > > > > > Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > > --- > > > 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 at lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] drm/sun4i: Check that the plane coordinates are not negative 2016-09-30 16:33 ` Boris Brezillon @ 2016-09-30 20:42 ` Ville Syrjälä 0 siblings, 0 replies; 8+ messages in thread From: Ville Syrjälä @ 2016-09-30 20:42 UTC (permalink / raw) To: linux-arm-kernel On Fri, Sep 30, 2016 at 06:33:48PM +0200, Boris Brezillon wrote: > On Fri, 30 Sep 2016 19:22:11 +0300 > Ville Syrj?l? <ville.syrjala@linux.intel.com> 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@free-electrons.com> wrote: > > > > > > > Our planes cannot be set at negative coordinates. Make sure we reject such > > > > configuration. > > > > > > > > Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > > > --- > > > > 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), Ah, missed that. Anyway it's still the wrong thing to check since negative coordinates are a perfectly valid thing to have. > 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. Nope. The src/dst rects are now stored in drm_plane_state. Assuming my patches went in already. I think they did. > > 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). Most hardware can't do negative coordinates natively. But when things get clipped, the negative coordinates disappear. Then the only question left is whether the resulting coordinates are fullscreen or not, and whether the hardware can handle the "not" case. > > > > > > > > > --->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 at lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- Ville Syrj?l? Intel OTC ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] drm/sun4i: Check that the plane coordinates are not negative 2016-09-30 16:08 ` Boris Brezillon 2016-09-30 16:22 ` Ville Syrjälä @ 2016-10-03 12:58 ` Maxime Ripard 2016-10-03 16:18 ` Boris Brezillon 1 sibling, 1 reply; 8+ messages in thread From: Maxime Ripard @ 2016-10-03 12:58 UTC (permalink / raw) To: linux-arm-kernel Hi Boris, 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@free-electrons.com> wrote: > > > Our planes cannot be set at negative coordinates. Make sure we reject such > > configuration. > > > > Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > --- > > 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. Indeed, that would probably work. This is even somewhat what we've been using to implement the VGA hack we use on the CHIP. Can you send that patch? Thanks, Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161003/32b3fe92/attachment.sig> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] drm/sun4i: Check that the plane coordinates are not negative 2016-10-03 12:58 ` Maxime Ripard @ 2016-10-03 16:18 ` Boris Brezillon 2016-10-12 21:09 ` Maxime Ripard 0 siblings, 1 reply; 8+ messages in thread From: Boris Brezillon @ 2016-10-03 16:18 UTC (permalink / raw) To: linux-arm-kernel On Mon, 3 Oct 2016 14:58:11 +0200 Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Hi Boris, > > 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@free-electrons.com> wrote: > > > > > Our planes cannot be set at negative coordinates. Make sure we reject such > > > configuration. > > > > > > Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > > --- > > > 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. > > Indeed, that would probably work. This is even somewhat what we've > been using to implement the VGA hack we use on the CHIP. > > Can you send that patch? Actually, Ville suggested a slightly different approach: use the ->src and ->dst in drm_plane_state. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] drm/sun4i: Check that the plane coordinates are not negative 2016-10-03 16:18 ` Boris Brezillon @ 2016-10-12 21:09 ` Maxime Ripard 0 siblings, 0 replies; 8+ messages in thread From: Maxime Ripard @ 2016-10-12 21:09 UTC (permalink / raw) To: linux-arm-kernel On Mon, Oct 03, 2016 at 06:18:40PM +0200, Boris Brezillon wrote: > On Mon, 3 Oct 2016 14:58:11 +0200 > Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > > > Hi Boris, > > > > 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@free-electrons.com> wrote: > > > > > > > Our planes cannot be set at negative coordinates. Make sure we reject such > > > > configuration. > > > > > > > > Reported-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > > > --- > > > > 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. > > > > Indeed, that would probably work. This is even somewhat what we've > > been using to implement the VGA hack we use on the CHIP. > > > > Can you send that patch? > > Actually, Ville suggested a slightly different approach: use the > ->src and ->dst in drm_plane_state. After more tests, it turns out that it's actually simpler than that.... The hardware is able to take negative coordinates. Sorry for the noise, Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161012/14a89d77/attachment.sig> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-10-12 21:09 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-09-30 14:33 [PATCH] drm/sun4i: Check that the plane coordinates are not negative Maxime Ripard 2016-09-30 16:08 ` Boris Brezillon 2016-09-30 16:22 ` Ville Syrjälä 2016-09-30 16:33 ` Boris Brezillon 2016-09-30 20:42 ` Ville Syrjälä 2016-10-03 12:58 ` Maxime Ripard 2016-10-03 16:18 ` Boris Brezillon 2016-10-12 21:09 ` Maxime Ripard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).