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
