Re: [PATCH v6 9/9] drm/omap: Add a 'right overlay' to plane state

[Neil Armstrong]

Hi,

On 27/10/2021 14:50, Tomi Valkeinen wrote:
> On 18/10/2021 17:28, Neil Armstrong wrote:
>> From: Benoit Parrot <bparrot@xxxxxx>
>>
>> If the drm_plane has a source width that's greater than the max width
>> supported by a single hw overlay, then we assign a 'r_overlay' to it in
>> omap_plane_atomic_check().
>>
>> Both overlays should have the capabilities required to handle the source
>> framebuffer. The only parameters that vary between the left and right
>> hwoverlays are the src_w, crtc_w, src_x and crtc_x as we just even chop
>> the fb into left and right halves.
>>
>> We also take care of not creating odd width size when dealing with YUV
>> formats.
>>
>> Since both halves need to be 'appear' side by side the zpos is
>> recalculated when dealing with dual overlay cases so that the other
>> planes zpos is consistent.
>>
>> Depending on user space usage it is possible that on occasion the number
>> of requested planes exceeds the numbers of overlays required to display
>> them. In that case a failure would be returned for the plane that cannot
>> be handled at that time. It is up to user space to make sure the H/W
>> resource are not over-subscribed.
>>
>> Signed-off-by: Benoit Parrot <bparrot@xxxxxx>
>> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>> ---
>>   drivers/gpu/drm/omapdrm/omap_drv.c     |  91 ++++++++++++++++++-
>>   drivers/gpu/drm/omapdrm/omap_fb.c      |  33 ++++++-
>>   drivers/gpu/drm/omapdrm/omap_fb.h      |   4 +-
>>   drivers/gpu/drm/omapdrm/omap_overlay.c |  23 ++++-
>>   drivers/gpu/drm/omapdrm/omap_overlay.h |   3 +-
>>   drivers/gpu/drm/omapdrm/omap_plane.c   | 120 +++++++++++++++++++++++--
>>   drivers/gpu/drm/omapdrm/omap_plane.h   |   1 +
>>   7 files changed, 263 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
>> index c7912374d393..f088b6313950 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -117,6 +117,95 @@ static void omap_atomic_commit_tail(struct drm_atomic_state *old_state)
>>       dispc_runtime_put(priv->dispc);
>>   }
>>   +static int drm_atomic_state_normalized_zpos_cmp(const void *a, const void *b)
>> +{
>> +    const struct drm_plane_state *sa = *(struct drm_plane_state **)a;
>> +    const struct drm_plane_state *sb = *(struct drm_plane_state **)b;
>> +
>> +    if (sa->normalized_zpos != sb->normalized_zpos)
>> +        return sa->normalized_zpos - sb->normalized_zpos;
>> +    else
>> +        return sa->plane->base.id - sb->plane->base.id;
>> +}
> 
> I think this, or the function below, needs a comment. I don't think it's obvious what's the logic here. It's mostly a copy of drm_atomic_normalize_zpos and drm_atomic_helper_crtc_normalize_zpos, if I'm not mistaken, it migth be good to point out what the difference is.

It's explained in the commit message:
>> Since both halves need to be 'appear' side by side the zpos is
>> recalculated when dealing with dual overlay cases so that the other
>> planes zpos is consistent.

Not sure what to add more, should I add it as comment in the code ?

> 
> I'm also wondering if these normalize_zpos functions should be split to a separate patch (without the is_omap_plane_dual_overlay call part).

It's tied to the introduction of 'right overlay', impossible to implement it without the
dual overlays functions and variables.

> 
>> +static int omap_atomic_update_normalize_zpos(struct drm_device *dev,
>> +                         struct drm_atomic_state *state)
>> +{
>> +    struct drm_crtc *crtc;
>> +    struct drm_crtc_state *old_state, *new_state;
>> +    struct drm_plane *plane;
>> +    int c, i, n, inc;
>> +    int total_planes = dev->mode_config.num_total_plane;
>> +    struct drm_plane_state **states;
>> +    int ret = 0;
>> +
>> +    states = kmalloc_array(total_planes, sizeof(*states), GFP_KERNEL);
>> +    if (!states)
>> +        return -ENOMEM;
>> +
>> +    for_each_oldnew_crtc_in_state(state, crtc, old_state, new_state, c) {
>> +        if (old_state->plane_mask == new_state->plane_mask &&
>> +            !new_state->zpos_changed)
>> +            continue;
>> +
>> +        /* Reset plane increment and index value for every crtc */
>> +        n = 0;
>> +
>> +        /*
>> +         * Normalization process might create new states for planes
>> +         * which normalized_zpos has to be recalculated.
>> +         */
>> +        drm_for_each_plane_mask(plane, dev, new_state->plane_mask) {
>> +            struct drm_plane_state *plane_state =
>> +                drm_atomic_get_plane_state(new_state->state,
>> +                               plane);
>> +            if (IS_ERR(plane_state)) {
>> +                ret = PTR_ERR(plane_state);
>> +                goto done;
>> +            }
>> +            states[n++] = plane_state;
>> +        }
>> +
>> +        sort(states, n, sizeof(*states),
>> +             drm_atomic_state_normalized_zpos_cmp, NULL);
>> +
>> +        for (i = 0, inc = 0; i < n; i++) {
>> +            plane = states[i]->plane;
>> +
>> +            states[i]->normalized_zpos = i + inc;
>> +            DRM_DEBUG_ATOMIC("[PLANE:%d:%s] updated normalized zpos value %d\n",
>> +                     plane->base.id, plane->name,
>> +                     states[i]->normalized_zpos);
>> +
>> +            if (is_omap_plane_dual_overlay(states[i]))
>> +                inc++;
>> +        }
>> +        new_state->zpos_changed = true;
>> +    }
>> +
>> +done:
>> +    kfree(states);
>> +    return ret;
>> +}
>> +
>> +static int omap_atomic_check(struct drm_device *dev,
>> +                 struct drm_atomic_state *state)
>> +{
>> +    int ret;
>> +
>> +    ret = drm_atomic_helper_check(dev, state);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (dev->mode_config.normalize_zpos) {
>> +        ret = omap_atomic_update_normalize_zpos(dev, state);
>> +        if (ret)
>> +            return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +

[...]

Thanks,
Neil