On 12/10/2021 15:34, Tomi Valkeinen wrote:
> On 23/09/2021 10:06, Neil Armstrong wrote:
>> From: Benoit Parrot <bpar...@ti.com>
>>
>> (re)assign the hw overlays to planes based on required caps, and to
>> handle situations where we could not modify an in-use plane.
>>
>> This means all planes advertise the superset of formats and properties.
>> Userspace must (as always) use atomic TEST_ONLY step for atomic updates,
>> as not all planes may be available for use on every frame.
>>
>> The mapping of hwoverlays to plane is stored in omap_global_state, so
>> that state updates are atomically committed in the same way that
>> plane/etc state updates are managed. This is needed because the
>> omap_plane_state keeps a pointer to the hwoverlay, and we don't want
>> global state to become out of sync with the plane state if an atomic
>> update fails, we hit deadlock/ backoff scenario, etc. The use of
>> global_state_lock keeps multiple parallel updates which both re-assign
>> hwoverlays properly serialized.
>>
>> Signed-off-by: Benoit Parrot <bpar...@ti.com>
>> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>
>> ---
>> drivers/gpu/drm/omapdrm/omap_drv.h | 3 +
>> drivers/gpu/drm/omapdrm/omap_overlay.c | 140 ++++++++++++++++++++
>> drivers/gpu/drm/omapdrm/omap_overlay.h | 9 ++
>> drivers/gpu/drm/omapdrm/omap_plane.c | 170 ++++++++++++++++++++-----
>> 4 files changed, 287 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h
>> b/drivers/gpu/drm/omapdrm/omap_drv.h
>> index 280cdd27bc8e..2d5928f05a23 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
>> @@ -49,6 +49,9 @@ struct omap_drm_pipeline {
>> #define to_omap_global_state(x) container_of(x, struct omap_global_state,
>> base)
>> struct omap_global_state {
>> struct drm_private_state base;
>> +
>> + /* global atomic state of assignment between overlays and planes */
>> + struct drm_plane *hwoverlay_to_plane[8];
>> };
>> struct omap_drm_private {
>> diff --git a/drivers/gpu/drm/omapdrm/omap_overlay.c
>> b/drivers/gpu/drm/omapdrm/omap_overlay.c
>> index 2b1416d2aad2..f1a23c2203aa 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_overlay.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_overlay.c
>> @@ -21,6 +21,146 @@ static const char * const overlay_id_to_name[] = {
>> [OMAP_DSS_VIDEO3] = "vid3",
>> };
>> +static struct omap_hw_overlay *
>> +omap_plane_find_free_overlay(struct drm_device *dev,
>> + struct drm_plane *hwoverlay_to_plane[],
>> + u32 caps, u32 fourcc, u32 crtc_mask)
>> +{
>> + struct omap_drm_private *priv = dev->dev_private;
>> + int i;
>> +
>> + DBG("caps: %x fourcc: %x crtc: %x", caps, fourcc, crtc_mask);
>> +
>> + for (i = 0; i < priv->num_ovls; i++) {
>> + struct omap_hw_overlay *cur = priv->overlays[i];
>> +
>> + DBG("%d: id: %d cur->caps: %x cur->crtc: %x",
>> + cur->idx, cur->overlay_id, cur->caps, cur->possible_crtcs);
>> +
>> + /* skip if already in-use */
>> + if (hwoverlay_to_plane[cur->idx])
>> + continue;
>> +
>> + /* check if allowed on crtc */
>> + if (!(cur->possible_crtcs & crtc_mask))
>> + continue;
>> +
>> + /* skip if doesn't support some required caps: */
>> + if (caps & ~cur->caps)
>> + continue;
>> +
>> + /* check supported format */
>> + if (!dispc_ovl_color_mode_supported(priv->dispc,
>> + cur->overlay_id, fourcc))
>> + continue;
>> +
>> + return cur;
>> + }
>> +
>> + DBG("no match");
>> + return NULL;
>> +}
>> +
>> +int omap_overlay_assign(struct drm_atomic_state *s, struct drm_plane *plane,
>> + u32 caps, u32 fourcc, u32 crtc_mask,
>> + struct omap_hw_overlay **overlay)
>> +{
>> + struct omap_drm_private *priv = s->dev->dev_private;
>> + struct omap_global_state *new_global_state, *old_global_state;
>> + struct drm_plane **overlay_map;
>> + struct omap_hw_overlay *ovl;
>> +
>> + new_global_state = omap_get_global_state(s);
>> + if (IS_ERR(new_global_state))
>> + return PTR_ERR(new_global_state);
>> +
>> + /*
>> + * grab old_state after omap_get_global_state(),
>> + * since now we hold lock:
>> + */
>> + old_global_state = omap_get_existing_global_state(priv);
>> + DBG("new_global_state: %p old_global_state: %p",
>> + new_global_state, old_global_state);
>> +
>> + overlay_map = new_global_state->hwoverlay_to_plane;
>> +
>> + if (!*overlay) {
>> + ovl = omap_plane_find_free_overlay(s->dev, overlay_map,
>> + caps, fourcc, crtc_mask);
>> + if (!ovl)
>> + return -ENOMEM;
>> +
>> + ovl->possible_crtcs = crtc_mask;
>> + overlay_map[ovl->idx] = plane;
>> + *overlay = ovl;
>> +
>> + DBG("%s: assign to plane %s caps %x on crtc %x",
>> + (*overlay)->name, plane->name, caps, crtc_mask);
>> + }
>> +
>> + return 0;
>> +}
>
> Why would one call this function with overlay == NULL? What does the function
> do in that case?
This function is only called with !*overlay, no ide why it would be called with
*overlay != NULL...
I'll try to simplify this.
>
>> +
>> +void omap_overlay_release(struct drm_atomic_state *s,
>> + struct drm_plane *plane,
>> + struct omap_hw_overlay *overlay)
>
> It feels odd to pass both plane and overlay here. If we're unassigning (is
> that a verb? =) the overlay, isn't it enough to just pass either the plane or
> the overlay?
Passing the overlay is necessary when having 2 overlays for a plane, but the
plane parameter is definitely
unneeded here.
>
>> +{
>> + struct omap_global_state *state = omap_get_global_state(s);
>> + struct drm_plane **overlay_map = state->hwoverlay_to_plane;
>> +
>> + if (!overlay)
>> + return;
>> +
>> + if (WARN_ON(!overlay_map[overlay->idx]))
>> + return;
>> + /*
>> + * Check that the overlay we are releasing is actually
>> + * assigned to the plane we are trying to release it from.
>> + */
>> + if (overlay_map[overlay->idx] == plane) {
>> + DBG("%s: release from plane %s", overlay->name, plane->name);
>> +
>> + overlay_map[overlay->idx] = NULL;
>> + }
>
> So it's normal that overlay_map[overlay->idx] != plane? When does that happen?
It's impossible since the overlay is taken from the plane state, will simplify
this,
remove the plane parameter and remove the unnecessary checks.
>
>> +}
>> +
>> +void omap_overlay_disable(struct drm_atomic_state *s,
>> + struct drm_plane *plane,
>> + struct omap_hw_overlay *overlay)
>> +{
>> + struct omap_drm_private *priv = s->dev->dev_private;
>> + struct drm_plane **overlay_map;
>> + struct omap_global_state *old_state;
>> +
>> + old_state = omap_get_existing_global_state(priv);
>> + overlay_map = old_state->hwoverlay_to_plane;
>> +
>> + if (!overlay)
>> + return;
>> +
>
> You can move the if above to the beginning of the func.
>
>> + /*
>> + * Check that the overlay we are trying to disable has not
>> + * been re-assigned to another plane already
>> + */
>> + if (!overlay_map[overlay->idx]) {
>> + DBG("%s: on %s disabled", overlay->name, plane->name);
>> +
>> + /* disable the overlay */
>> + dispc_ovl_enable(priv->dispc, overlay->overlay_id, false);
>> +
>> + /*
>> + * Since we are disabling this overlay in this
>> + * atomic cycle we can reset the available crtcs
>> + * it can be used on
>> + */
>> + overlay->possible_crtcs = (1 << priv->num_pipes) - 1;
>
> This is changing data in the overlay struct itself. Shouldn't the mutable
> data be in the atomic state?
>
> I'm also a bit confused on what the "possible_crtcs" is. But I think it's
> either a bitmask of all the crtcs when the overlay is free, or a mask of a
> single crtc when allocated.
>
> I think a variable that's either 0 when the overlay is not assigned, or the
> mask of the crtc when in use, would be more clear. Also with some other name
> (active_crtc?).
I dropped entirely this possible_crtcs.
>
>> + }
>> +
>> + /*
>> + * Otherwise the overlay is still in use so leave it alone
>> + */
>> +}
>
> This function also has interesting behavior, possibly not disabling the hw
> overlay. I think these new exposed overlay functions could use a comment
> above each of them, explaining what they do.
I renamed this function to "omap_overlay_update_state()" which will disable an
overlay if not more used.
This name looks much more appropriate.
>
>> +
>> static void omap_overlay_destroy(struct omap_hw_overlay *overlay)
>> {
>> kfree(overlay);
>> diff --git a/drivers/gpu/drm/omapdrm/omap_overlay.h
>> b/drivers/gpu/drm/omapdrm/omap_overlay.h
>> index 892fecb67adb..d5033ee481c2 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_overlay.h
>> +++ b/drivers/gpu/drm/omapdrm/omap_overlay.h
>> @@ -28,4 +28,13 @@ struct omap_hw_overlay {
>> int omap_hwoverlays_init(struct omap_drm_private *priv);
>> void omap_hwoverlays_destroy(struct omap_drm_private *priv);
>> +int omap_overlay_assign(struct drm_atomic_state *s, struct drm_plane *plane,
>> + u32 caps, u32 fourcc, u32 crtc_mask,
>> + struct omap_hw_overlay **overlay);
>> +void omap_overlay_release(struct drm_atomic_state *s,
>> + struct drm_plane *plane,
>> + struct omap_hw_overlay *overlay);
>> +void omap_overlay_disable(struct drm_atomic_state *s,
>> + struct drm_plane *plane,
>> + struct omap_hw_overlay *overlay);
>> #endif /* __OMAPDRM_OVERLAY_H__ */
>> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
>> b/drivers/gpu/drm/omapdrm/omap_plane.c
>> index bda794b4c915..4b400a8bfe9e 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
>> @@ -8,6 +8,7 @@
>> #include <drm/drm_atomic_helper.h>
>> #include <drm/drm_gem_atomic_helper.h>
>> #include <drm/drm_plane_helper.h>
>> +#include <drm/drm_fourcc.h>
>> #include "omap_dmm_tiler.h"
>> #include "omap_drv.h"
>> @@ -21,6 +22,8 @@
>> struct omap_plane_state {
>> /* Must be first. */
>> struct drm_plane_state base;
>> +
>> + struct omap_hw_overlay *overlay;
>> };
>> #define to_omap_plane(x) container_of(x, struct omap_plane, base)
>> @@ -29,8 +32,6 @@ struct omap_plane {
>> struct drm_plane base;
>> enum omap_plane_id id;
>> const char *name;
>> -
>> - struct omap_hw_overlay *overlay;
>> };
>> static int omap_plane_prepare_fb(struct drm_plane *plane,
>> @@ -58,10 +59,27 @@ static void omap_plane_atomic_update(struct drm_plane
>> *plane,
>> struct omap_plane *omap_plane = to_omap_plane(plane);
>> struct drm_plane_state *new_state =
>> drm_atomic_get_new_plane_state(state,
>> plane);
>> - enum omap_plane_id ovl_id = omap_plane->overlay->overlay_id;
>> + struct drm_plane_state *old_state =
>> drm_atomic_get_old_plane_state(state,
>> + plane);
>> + struct omap_plane_state *new_omap_state;
>> + struct omap_plane_state *old_omap_state;
>> struct omap_overlay_info info;
>> + enum omap_plane_id ovl_id;
>> int ret;
>> + new_omap_state = to_omap_plane_state(new_state);
>> + old_omap_state = to_omap_plane_state(old_state);
>> +
>> + /* Cleanup previously held overlay if needed */
>> + omap_overlay_disable(old_state->state, plane, old_omap_state->overlay);
>> +
>> + if (!new_omap_state->overlay) {
>> + DBG("[PLANE:%d:%s] overlay_id: ??? (%p)", plane->base.id,
>> plane->name,
>> + new_omap_state->overlay);
>
> I wonder what the ??? means here.
>
> Already in earlier patches I wondered if the DBG prints were good or not, but
> I thought I'll comment on those after I can run this and see what they
> actually print. But some of them felt like debug prints used during
> development, not something to leave in the production code (at least in the
> form they were).
I changed the print to something more interesting.
>
>> + return;
>> + }
>> +
>> + ovl_id = new_omap_state->overlay->overlay_id;
>> DBG("%s, crtc=%p fb=%p", omap_plane->name, new_state->crtc,
>> new_state->fb);
>> @@ -80,9 +98,9 @@ static void omap_plane_atomic_update(struct drm_plane
>> *plane,
>> /* update scanout: */
>> omap_framebuffer_update_scanout(new_state->fb, new_state, &info);
>> - DBG("%dx%d -> %dx%d (%d)", info.width, info.height,
>> - info.out_width, info.out_height,
>> - info.screen_width);
>> + DBG("%s: %dx%d -> %dx%d (%d)",
>> + new_omap_state->overlay->name, info.width, info.height,
>> + info.out_width, info.out_height, info.screen_width);
>> DBG("%d,%d %pad %pad", info.pos_x, info.pos_y,
>> &info.paddr, &info.p_uv_addr);
>> @@ -105,55 +123,66 @@ static void omap_plane_atomic_disable(struct
>> drm_plane *plane,
>> {
>> struct drm_plane_state *new_state =
>> drm_atomic_get_new_plane_state(state,
>> plane);
>> - struct omap_drm_private *priv = plane->dev->dev_private;
>> - struct omap_plane *omap_plane = to_omap_plane(plane);
>> - enum omap_plane_id ovl_id = omap_plane->overlay->overlay_id;
>> + struct drm_plane_state *old_state =
>> drm_atomic_get_old_plane_state(state,
>> + plane);
>> + struct omap_plane_state *new_omap_state;
>> + struct omap_plane_state *old_omap_state;
>> +
>> + new_omap_state = to_omap_plane_state(new_state);
>> + old_omap_state = to_omap_plane_state(old_state);
>> +
>> + if (!old_omap_state->overlay)
>> + return;
>> new_state->rotation = DRM_MODE_ROTATE_0;
>> - new_state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 :
>> omap_plane->id;
>> + new_state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
>> + ? 0 : old_omap_state->overlay->overlay_id;
>
> I'm not sure if I'm right here, but... doesn't the above mean that the
> plane's zpos can change, depending on which hw overlay it happened to use?
I'll need to investigate more on this.
>
>> - dispc_ovl_enable(priv->dispc, ovl_id, false);
>> + omap_overlay_disable(old_state->state, plane, old_omap_state->overlay);
>> + new_omap_state->overlay = NULL;
>> }
>> +#define FRAC_16_16(mult, div) (((mult) << 16) / (div))
>> static int omap_plane_atomic_check(struct drm_plane *plane,
>> struct drm_atomic_state *state)
>> {
>> struct drm_plane_state *new_plane_state =
>> drm_atomic_get_new_plane_state(state,
>> plane);
>> + struct drm_plane_state *old_plane_state =
>> drm_atomic_get_old_plane_state(state,
>> + plane);
>> + struct drm_crtc *crtc;
>> struct omap_drm_private *priv = plane->dev->dev_private;
>> + struct omap_plane_state *omap_state =
>> to_omap_plane_state(new_plane_state);
>> + struct omap_global_state *omap_overlay_global_state;
>> + u32 crtc_mask;
>> + u32 fourcc;
>> + u32 caps = 0;
>> + bool new_hw_overlay = false;
>> + int min_scale, max_scale;
>> + int ret;
>> struct drm_crtc_state *crtc_state;
>> u16 width, height;
>> u32 width_fp, height_fp;
>> - if (!new_plane_state->fb)
>> - return 0;
>> + omap_overlay_global_state = omap_get_global_state(state);
>> + if (IS_ERR(omap_overlay_global_state))
>> + return PTR_ERR(omap_overlay_global_state);
>> + DBG("%s: omap_overlay_global_state: %p", plane->name,
>> + omap_overlay_global_state);
>> dispc_ovl_get_max_size(priv->dispc, &width, &height);
>> width_fp = width << 16;
>> height_fp = height << 16;
>> - /* crtc should only be NULL when disabling (i.e.,
>> !new_plane_state->fb) */
>> - if (WARN_ON(!new_plane_state->crtc))
>> + crtc = new_plane_state->crtc ? new_plane_state->crtc :
>> plane->state->crtc;
>> + if (!crtc)
>> return 0;
>> - crtc_state = drm_atomic_get_existing_crtc_state(state,
>> - new_plane_state->crtc);
>> + crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
>> /* we should have a crtc state if the plane is attached to a crtc */
>> if (WARN_ON(!crtc_state))
>> return 0;
>> - if (!crtc_state->enable)
>> - return 0;
>> -
>> - if (new_plane_state->crtc_x < 0 || new_plane_state->crtc_y < 0)
>> - return -EINVAL;
>> -
>> - if (new_plane_state->crtc_x + new_plane_state->crtc_w >
>> crtc_state->adjusted_mode.hdisplay)
>> - return -EINVAL;
>> -
>> - if (new_plane_state->crtc_y + new_plane_state->crtc_h >
>> crtc_state->adjusted_mode.vdisplay)
>> - return -EINVAL;
>> -
>> /* Make sure dimensions are within bounds. */
>> if (new_plane_state->src_h > height_fp || new_plane_state->crtc_h >
>> height)
>> return -EINVAL;
>> @@ -161,9 +190,81 @@ static int omap_plane_atomic_check(struct drm_plane
>> *plane,
>> if (new_plane_state->src_w > width_fp || new_plane_state->crtc_w >
>> width)
>> return -EINVAL;
>> - if (new_plane_state->rotation != DRM_MODE_ROTATE_0 &&
>> - !omap_framebuffer_supports_rotation(new_plane_state->fb))
>> - return -EINVAL;
>> +
>> + /*
>> + * Note: these are just sanity checks to filter out totally bad scaling
>> + * factors. The real limits must be calculated case by case, and
>> + * unfortunately we currently do those checks only at the commit
>> + * phase in dispc.
>> + */
>> + min_scale = FRAC_16_16(1, 8);
>> + max_scale = FRAC_16_16(8, 1);
>> +
>> + ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
>> + min_scale, max_scale,
>> + true, true);
>
> This piece here feels a bit out of context (wrt. to this patch). Why do we
> need it here, when adding support to hw overlays?
>
> And I think it's fine to use the above helper, but maybe it can be in a
> separate patch earlier?
I'll move it out of this patch.
>
>
>> + if (ret)
>> + return ret;
>> +
>> + DBG("%s: check (%d -> %d)", plane->name,
>> + old_plane_state->visible, new_plane_state->visible);
>
> Here's an example of the debug prints I mentioned earlier. I think this
> prints:
>
> "GFX: check (1 -> 0)"
>
> I'm fine with terse debug prints, but... maybe something like "visible 1 ->
> 0" would be much better? =)
Fixed
>
>> + if (new_plane_state->visible) {
>> + if (new_plane_state->rotation != DRM_MODE_ROTATE_0 &&
>> + !omap_framebuffer_supports_rotation(new_plane_state->fb))
>> + return -EINVAL;
>> +
>> + if ((new_plane_state->src_w >> 16) != new_plane_state->crtc_w ||
>> + (new_plane_state->src_h >> 16) != new_plane_state->crtc_h)
>> + caps |= OMAP_DSS_OVL_CAP_SCALE;
>> +
>> + fourcc = new_plane_state->fb->format->format;
>> + crtc_mask = drm_crtc_mask(new_plane_state->crtc);
>> +
>> + /*
>> + * (re)allocate hw overlay if we don't have one or
>> + * there is a caps mismatch
>> + */
>> + if (!omap_state->overlay ||
>> + (caps & ~omap_state->overlay->caps)) {
>> + new_hw_overlay = true;
>> + } else {
>> + /* check if allowed on crtc */
>> + if (!(omap_state->overlay->possible_crtcs & crtc_mask))
>> + new_hw_overlay = true;
>> +
>> + /* check supported format */
>> + if (!dispc_ovl_color_mode_supported(priv->dispc,
>> + omap_state->overlay->overlay_id,
>> + fourcc))
>> + new_hw_overlay = true;
>> + }
>> +
>> + if (new_hw_overlay) {
>> + struct omap_hw_overlay *old_ovl = omap_state->overlay;
>> + struct omap_hw_overlay *new_ovl = NULL;
>> +
>> + omap_overlay_release(state, plane, old_ovl);
>> +
>> + ret = omap_overlay_assign(state, plane, caps,
>> + fourcc, crtc_mask, &new_ovl);
>> + if (ret) {
>> + DBG("%s: failed to assign hw_overlay(s)!",
>> + plane->name);
>> + omap_state->overlay = NULL;
>> + return ret;
>> + }
>> +
>> + omap_state->overlay = new_ovl;
>> + }
>> + } else {
>> + omap_overlay_release(state, plane, omap_state->overlay);
>> + omap_state->overlay = NULL;
>> + }
>> +
>> + if (omap_state->overlay)
>> + DBG("plane: %s overlay_id: %d", plane->name,
>> + omap_state->overlay->overlay_id);
>> return 0;
>> }
>> @@ -230,7 +331,7 @@ static void omap_plane_reset(struct drm_plane *plane)
>> * plane.
>> */
>> plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY
>> - ? 0 : omap_plane->overlay->overlay_id;
>> + ? 0 : omap_plane->id;
>
> Hmm... On plane reset we now set the zpos to plane->id, but on disable we
> reset zpos to overlay->id. And before this patch we set the vice versa. I
> don't follow.
I'll aswell need to investigate more on this.
>
> Tomi
Neil
