Message ID | cover.9e7b6a702e4d90a13d08fbe3b0a319cbf4db687f.1516617243.git-series.maxime.ripard@free-electrons.com |
---|---|
Headers | show |
Series | drm/sun4i: Support more planes, zpos and plane-wide alpha | expand |
Maxime Ripard <maxime.ripard@free-electrons.com> writes: > Now that the drm_format_info has a alpha field to tell if a format embeds > an alpha component in it, let's use it. > > Cc: Eric Anholt <eric@anholt.net> > Reviewed-by: Daniel Vetter <daniel.vetter@intel.com> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> I had sent my r-b for the last version of this.
On Mon, Jan 22, 2018 at 6:35 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > The sun4i_plane_desc structure was somehow indented to two tabulations > instead of one as we shoud do. Fix that. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> Acked-by: Chen-Yu Tsai <wens@csie.org>
On Mon, Jan 22, 2018 at 6:35 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > There was a typo in the width spelling of the (unused) > SUN4I_BACKEND_IYUVLINEWITDTH_REG macro. Fix it. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> Acked-by: Chen-Yu Tsai <wens@csie.org>
On Mon, Jan 22, 2018 at 6:35 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > In order to support normalized zpos, we need to call > drm_atomic_normalize_zpos in our driver's drm_mode_config_funcs' > atomic_check. > > Let's duplicate the definition of drm_atomic_helper_check for now. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> Acked-by: Chen-Yu Tsai <wens@csie.org>
On Mon, Jan 22, 2018 at 6:35 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > The function supposed to update a plane's coordinates is called in both > branches of our function. Let's move it out the if statement. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> Acked-by: Chen-Yu Tsai <wens@csie.org>
On Mon, Jan 22, 2018 at 6:35 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > The our plane state zpos value will be set only if there's an existing ^^^ extra word Otherwise, Acked-by: Chen-Yu Tsai <wens@csie.org> > state attached to the plane when creating the property. > > However, this is not the case during the probe, and we therefore need to > put our default value in our reset hook. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > drivers/gpu/drm/sun4i/sun4i_layer.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c > index c448cb6b9fa9..03549646528a 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_layer.c > +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c > @@ -28,6 +28,7 @@ struct sun4i_plane_desc { > > static void sun4i_backend_layer_reset(struct drm_plane *plane) > { > + struct sun4i_layer *layer = plane_to_sun4i_layer(plane); > struct sun4i_layer_state *state; > > if (plane->state) { > @@ -43,6 +44,7 @@ static void sun4i_backend_layer_reset(struct drm_plane *plane) > if (state) { > plane->state = &state->state; > plane->state->plane = plane; > + plane->state->zpos = layer->id; > } > } > > -- > git-series 0.9.1
On Mon, Jan 22, 2018 at 6:35 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Our various planes have a configurable zpos, that combined with the pipes > allow to configure the composition. > > Since the interaction between the pipes, zpos and alphas framebuffers are ^^^ is > not trivials, let's just enable the zpos as an immutable property for now, ^^^^^^^ trivial > and use that zpos in our atomic_update part. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > drivers/gpu/drm/sun4i/sun4i_backend.c | 15 +++++++++++++++ > drivers/gpu/drm/sun4i/sun4i_backend.h | 2 ++ > drivers/gpu/drm/sun4i/sun4i_framebuffer.c | 4 ++++ > drivers/gpu/drm/sun4i/sun4i_layer.c | 3 +++ > 4 files changed, 24 insertions(+) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c > index a18c86a15748..c4986054909b 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c > @@ -272,6 +272,21 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend, > return 0; > } > > +int sun4i_backend_update_layer_zpos(struct sun4i_backend *backend, int layer, > + struct drm_plane *plane) > +{ > + struct drm_plane_state *state = plane->state; > + unsigned int priority = state->normalized_zpos; > + > + DRM_DEBUG_DRIVER("Setting layer %d priority to %d\n", layer, priority); You might want to make the statement less ambiguous, like "Setting layer %d's priority ..." Otherwise, Reviewed-by: Chen-Yu Tsai <wens@csie.org> > + > + regmap_update_bits(backend->engine.regs, SUN4I_BACKEND_ATTCTL_REG0(layer), > + SUN4I_BACKEND_ATTCTL_REG0_LAY_PRISEL_MASK, > + SUN4I_BACKEND_ATTCTL_REG0_LAY_PRISEL(priority)); > + > + return 0; > +} > + > static bool sun4i_backend_plane_uses_scaler(struct drm_plane_state *state) > { > u16 src_h = state->src_h >> 16; > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.h b/drivers/gpu/drm/sun4i/sun4i_backend.h > index 1ca8b7db6807..04a4f11b87a8 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_backend.h > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.h > @@ -182,5 +182,7 @@ int sun4i_backend_update_layer_buffer(struct sun4i_backend *backend, > int layer, struct drm_plane *plane); > int sun4i_backend_update_layer_frontend(struct sun4i_backend *backend, > int layer, uint32_t in_fmt); > +int sun4i_backend_update_layer_zpos(struct sun4i_backend *backend, > + int layer, struct drm_plane *plane); > > #endif /* _SUN4I_BACKEND_H_ */ > diff --git a/drivers/gpu/drm/sun4i/sun4i_framebuffer.c b/drivers/gpu/drm/sun4i/sun4i_framebuffer.c > index e68004844abe..5b3986437a50 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_framebuffer.c > +++ b/drivers/gpu/drm/sun4i/sun4i_framebuffer.c > @@ -35,6 +35,10 @@ static int sun4i_de_atomic_check(struct drm_device *dev, > if (ret) > return ret; > > + ret = drm_atomic_normalize_zpos(dev, state); > + if (ret) > + return ret; > + > return drm_atomic_helper_check_planes(dev, state); > } > > diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c > index 03549646528a..fbf25d59cf88 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_layer.c > +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c > @@ -115,6 +115,7 @@ static void sun4i_backend_layer_atomic_update(struct drm_plane *plane, > } > > sun4i_backend_update_layer_coord(backend, layer->id, plane); > + sun4i_backend_update_layer_zpos(backend, layer->id, plane); > sun4i_backend_layer_enable(backend, layer->id, true); > } > > @@ -237,6 +238,8 @@ struct drm_plane **sun4i_layers_init(struct drm_device *drm, > return ERR_CAST(layer); > }; > > + drm_plane_create_zpos_immutable_property(&layer->plane, i); > + > DRM_DEBUG_DRIVER("Assigning %s plane to pipe %d\n", > i ? "overlay" : "primary", plane->pipe); > regmap_update_bits(engine->regs, SUN4I_BACKEND_ATTCTL_REG0(i), > -- > git-series 0.9.1
On Mon, Jan 22, 2018 at 6:35 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Due to the way the composition is done in hardware, we can only have a > single alpha-enabled plane active at a time, placed in the second (highest > priority) pipe. > > Make sure of that in our atomic_check to not end up in an impossible > scenario. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > drivers/gpu/drm/sun4i/sun4i_backend.c | 50 ++++++++++++++++++++++++++++- > drivers/gpu/drm/sun4i/sun4i_backend.h | 2 +- > drivers/gpu/drm/sun4i/sun4i_layer.c | 23 +------------- > 3 files changed, 53 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c > index c4986054909b..eb1749d2c0d5 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_backend.c > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c > @@ -329,6 +329,8 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine, > struct drm_atomic_state *state = crtc_state->state; > struct drm_device *drm = state->dev; > struct drm_plane *plane; > + unsigned int num_planes = 0; > + unsigned int num_alpha_planes = 0; > unsigned int num_frontend_planes = 0; > > DRM_DEBUG_DRIVER("Starting checking our planes\n"); > @@ -341,6 +343,7 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine, > drm_atomic_get_plane_state(state, plane); > struct sun4i_layer_state *layer_state = > state_to_sun4i_layer_state(plane_state); > + struct drm_framebuffer *fb = plane_state->fb; > > if (sun4i_backend_plane_uses_frontend(plane_state)) { > DRM_DEBUG_DRIVER("Using the frontend for plane %d\n", > @@ -351,6 +354,50 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine, > } else { > layer_state->uses_frontend = false; > } > + > + DRM_DEBUG_DRIVER("Plane FB format is %s\n", > + drm_get_format_name(fb->format->format, > + &format_name)); > + if (fb->format->has_alpha) > + num_alpha_planes++; > + > + num_planes++; > + } > + > + /* > + * The hardware is a bit unusual here. > + * > + * Even though it supports 4 layers, it does the composition > + * in two separate steps. > + * > + * The first one is assigning a layer to one of its two > + * pipes. If more that 1 layer is assigned to the same pipe, > + * and if pixels overlaps, the pipe will take the pixel from > + * the layer with the highest priority. > + * > + * The second step is the actual alpha blending, that takes > + * the two pipes as input, and uses the eventual alpha > + * component to do the transparency between the two. > + * > + * This two steps scenario makes us unable to guarantee a > + * robust alpha blending between the 4 layers in all > + * situations, since this means that we need to have one layer > + * with alpha at the lowest position of our two pipes. > + * > + * However, we cannot even do that, since the hardware has a > + * bug where the lowest plane of the lowest pipe (pipe 0, > + * priority 0), if it has any alpha, will discard the pixel > + * entirely and just display the pixels in the background > + * color (black by default). > + * > + * Since means that we effectively have only three valid ^^^^^ This > + * configurations with alpha, all of them with the alpha being > + * on pipe1 with the lowest position, which can be 1, 2 or 3 > + * depending on the number of planes and their zpos. > + */ > + if (num_alpha_planes > SUN4I_BACKEND_NUM_ALPHA_LAYERS) { > + DRM_DEBUG_DRIVER("Too many planes with alpha, rejecting...\n"); > + return -EINVAL; > } > > if (num_frontend_planes > SUN4I_BACKEND_NUM_FRONTEND_LAYERS) { > @@ -358,6 +405,9 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine, > return -EINVAL; > } > > + DRM_DEBUG_DRIVER("State valid with %u planes, %u alpha, %u video\n", > + num_planes, num_alpha_planes, num_frontend_planes); > + > return 0; > } > > diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.h b/drivers/gpu/drm/sun4i/sun4i_backend.h > index 04a4f11b87a8..52e77591186a 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_backend.h > +++ b/drivers/gpu/drm/sun4i/sun4i_backend.h > @@ -146,6 +146,8 @@ > #define SUN4I_BACKEND_HWCCOLORTAB_OFF 0x4c00 > #define SUN4I_BACKEND_PIPE_OFF(p) (0x5000 + (0x400 * (p))) > > +#define SUN4I_BACKEND_NUM_LAYERS 4 > +#define SUN4I_BACKEND_NUM_ALPHA_LAYERS 1 > #define SUN4I_BACKEND_NUM_FRONTEND_LAYERS 1 > > struct sun4i_backend { > diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c > index fbf25d59cf88..900e716443b8 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_layer.c > +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c > @@ -201,32 +201,11 @@ struct drm_plane **sun4i_layers_init(struct drm_device *drm, > struct sun4i_backend *backend = engine_to_sun4i_backend(engine); > int i; > > - planes = devm_kcalloc(drm->dev, ARRAY_SIZE(sun4i_backend_planes) + 1, > + planes = devm_kcalloc(drm->dev, SUN4I_BACKEND_NUM_LAYERS, > sizeof(*planes), GFP_KERNEL); The returned "planes" array has to have a sentinel at the end, as we never return the actual number of layers created. That is what the +1 was for in the original code. Otherwise, Reviewed-by: Chen-Yu Tsai <wens@csie.org> > if (!planes) > return ERR_PTR(-ENOMEM); > > - /* > - * The hardware is a bit unusual here. > - * > - * Even though it supports 4 layers, it does the composition > - * in two separate steps. > - * > - * The first one is assigning a layer to one of its two > - * pipes. If more that 1 layer is assigned to the same pipe, > - * and if pixels overlaps, the pipe will take the pixel from > - * the layer with the highest priority. > - * > - * The second step is the actual alpha blending, that takes > - * the two pipes as input, and uses the eventual alpha > - * component to do the transparency between the two. > - * > - * This two steps scenario makes us unable to guarantee a > - * robust alpha blending between the 4 layers in all > - * situations. So we just expose two layers, one per pipe. On > - * SoCs that support it, sprites could fill the need for more > - * layers. > - */ > for (i = 0; i < ARRAY_SIZE(sun4i_backend_planes); i++) { > const struct sun4i_plane_desc *plane = &sun4i_backend_planes[i]; > struct sun4i_layer *layer; > -- > git-series 0.9.1
On Mon, Jan 22, 2018 at 6:35 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Since we now have a way to enforce the zpos, check for the number of alpha > planes, the only missing part is to assign our pipe automatically instead > of hardcoding it. > > The algorithm is quite simple, but requires two iterations over the list of > planes. > > In the first one (which is the same one that we've had to check for alpha, > the frontend usage, and so on), we order the planes by their zpos. > > We can then do a second iteration over that array by ascending zpos > starting with the pipe 0. When and if we encounter our alpha plane, we put > it and all the other subsequent planes in the second pipe. > > And since we have runtime checks and pipe assignments now, we can just > remove the static declaration of the planes we used to have. It would be slightly better if you could split this out into a separate patch. It's not immediately obvious to others that the changes to sun4i_layer.c are self-contained, while the change to sun4i_layer.h is part of the new pipeline tracking thing. Plus I think the way you structured everything means it won't break if you split it out. Reviewed-by: Chen-Yu Tsai <wens@csie.org>
On Mon, Jan 22, 2018 at 6:35 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Now that we have everything in place, we can make zpos configurable now. > Change the zpos property from an immutable one to a regular. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
Hi Eric, On Tue, Jan 23, 2018 at 07:35:10AM +1100, Eric Anholt wrote: > Maxime Ripard <maxime.ripard@free-electrons.com> writes: > > > Now that the drm_format_info has a alpha field to tell if a format embeds > > an alpha component in it, let's use it. > > > > Cc: Eric Anholt <eric@anholt.net> > > Reviewed-by: Daniel Vetter <daniel.vetter@intel.com> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > I had sent my r-b for the last version of this. Sorry, I forgot to carry it :/ Thanks for your review, I pushed the patches 1-5 to drm-misc Maxime
On Mon, Jan 29, 2018 at 10:22:54AM +0800, Chen-Yu Tsai wrote: > On Mon, Jan 22, 2018 at 6:35 PM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > Since we now have a way to enforce the zpos, check for the number of alpha > > planes, the only missing part is to assign our pipe automatically instead > > of hardcoding it. > > > > The algorithm is quite simple, but requires two iterations over the list of > > planes. > > > > In the first one (which is the same one that we've had to check for alpha, > > the frontend usage, and so on), we order the planes by their zpos. > > > > We can then do a second iteration over that array by ascending zpos > > starting with the pipe 0. When and if we encounter our alpha plane, we put > > it and all the other subsequent planes in the second pipe. > > > > And since we have runtime checks and pipe assignments now, we can just > > remove the static declaration of the planes we used to have. > > It would be slightly better if you could split this out into a separate > patch. It's not immediately obvious to others that the changes to > sun4i_layer.c are self-contained, while the change to sun4i_layer.h > is part of the new pipeline tracking thing. Plus I think the way you > structured everything means it won't break if you split it out. You're right, I'll split this patch and resend it. I've pushed the patches 9-15 to drm-misc, with your suggestions when you had some. Thanks! Maxime