| 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
Hi, This serie aims at enhancing the support for our planes in the current drm driver on the first generation of Allwinner's display engine. This also introduces a few generic stuff, as well as some conversion for some other drivers. This series basically implements three things that look orthogonal, but due to the way the hardware works are kind of related. The main feature is that instead of implementing 2 planes per backend, we are now able to use the planes that are available in hardware. This was unsupported before because of the way the composition works in the hardware. Indeed, the planes are first grouped into 2 pipes that are doing a basic composition, in case of overlapping planes, it just takes whatever plane has the highest priority (=> zpos). Then, the alpha blending is done between the two pipes. This was simplified so far by only using two planes, one for each pipe, which was allowing us to have an illusion of proper alpha blending. This is further complicated by the bug/feature that the lowest plane must not have any alpha at all, otherwise the pixel will turn black, no matter what the value of alpha is. This basically means that we can have a plane with alpha only in the second pipe. However, as we have more and more blocks being worked on, 2 planes are getting really limited and we need to support all 4 of them. This is mostly possible by extending our atomic_check and to make sure that we enforce those constraints, and assign the pipes automatically. This is done by looking at the number of planes using an alpha component, and we then end up in various scenarios: - 0 plane with alpha => we don't care for the pipes at all. All the planes are assigned to the first pipe - 1 plane with alpha => we assign all the planes without alpha below the plane with alpha to the first pipe, and then all the remaining planes to the second pipe. The plane with alpha will be the lowest plane on that pipe, which means that whatever plane is above it will have precedence, but the alpha component will remain and will be used on pixels that are not overlapping - 2-4 planes with alpha => we can't operate that way, we just reject the configuration. In addition to the formats that embed an alpha component, we also add support for plane-wide alpha property, and in order to tweak the configuration the way we want to, we also add support for configurable zpos. Let me know what you think, Maxime Changes from v1: - Document the behaviour on concurrent usage of the alpha property and an alpha component in the format - Allowed for higher alpha values - Moved the alpha value from a helper to the struct drm_format_info - Collected tags Maxime Ripard (19): drm/fourcc: Add a alpha field to drm_format_info drm/atmel-hlcdc: Use the alpha format field in drm_format_info drm/atmel-exynos: Use the alpha format field in drm_format_info drm/rockchip: Use the alpha format field in drm_format_info drm/vc4: Use the alpha format field in drm_format_info drm/blend: Add a generic alpha property drm/atmel-hclcdc: Convert to the new generic alpha property drm/rcar-du: Convert to the new generic alpha property drm/sun4i: backend: Fix structure indentation drm/sun4i: backend: Fix define typo drm/sun4i: framebuffer: Add a custom atomic_check drm/sun4i: backend: Move the coord function in the shared part drm/sun4i: backend: Set a default zpos in our reset hook drm/sun4i: backend: Add support for zpos drm/sun4i: backend: Check for the number of alpha planes drm/sun4i: backend: Assign the pipes automatically drm/sun4i: backend: Make zpos configurable drm/sun4i: Add support for plane alpha drm/sun4i: backend: Remove ARGB spoofing Documentation/gpu/kms-properties.csv | 2 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 13 +-- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 113 ++------------- drivers/gpu/drm/drm_atomic.c | 4 +- drivers/gpu/drm/drm_atomic_helper.c | 4 +- drivers/gpu/drm/drm_blend.c | 32 ++++- drivers/gpu/drm/drm_fourcc.c | 50 +++---- drivers/gpu/drm/exynos/exynos_mixer.c | 14 +-- drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 +- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 5 +- drivers/gpu/drm/rcar-du/rcar_du_plane.c | 15 +-- drivers/gpu/drm/rcar-du/rcar_du_plane.h | 2 +- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 42 +------ drivers/gpu/drm/rcar-du/rcar_du_vsp.h | 2 +- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 13 +-- drivers/gpu/drm/sun4i/sun4i_backend.c | 124 +++++++++++++++-- drivers/gpu/drm/sun4i/sun4i_backend.h | 11 +- drivers/gpu/drm/sun4i/sun4i_framebuffer.c | 18 +- drivers/gpu/drm/sun4i/sun4i_layer.c | 84 ++---------- drivers/gpu/drm/sun4i/sun4i_layer.h | 1 +- drivers/gpu/drm/vc4/vc4_plane.c | 19 +-- include/drm/drm_blend.h | 1 +- include/drm/drm_fourcc.h | 2 +- include/drm/drm_plane.h | 6 +- 24 files changed, 275 insertions(+), 303 deletions(-) base-commit: 53b519deaf2e507b121b64abcb4ba0da075bd6a7