Re: [PATCH] drm/imx: Add active primary plane reconfiguration support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2016-08-15 15:18 GMT+08:00 Daniel Vetter <daniel@xxxxxxxx>:
> On Mon, Aug 15, 2016 at 02:09:13PM +0800, Liu Ying wrote:
>> We don't support configuring active primary plane on-the-fly for imx-drm.
>> The relevant CRTC should be disabled before the plane configuration.
>> Of course, the plane itself should be disabled as well.
>> For overlay plane, we currently reject active plane reconfiguration.
>> This patch adds active plane reconfiguration support by forcing CRTC
>> mode change and disabling-enabling plane in plane's ->atomic_update
>> callback.
>
> Why do you reject this for overlays? Userspace can always indicate whether
> it wants a full modeset or not through the atomic ALLOW_MODESET flag.
> Please don't create such special cases in atomic drivers, but instead
> export the full hw capabilities to userspace, and let userspace decide
> what it wants.

I'm a bit conservative when changing the code - just wanted to
touch the primary plane part only and keep the overlay plane
being rejected just the same as the non-atomic imx-drm driver did.

After removing the if() condition for the primary plane, the active
overlay plane can also be reconfigured according to my test.

So, it looks I may respin to remove the restriction for the overlay
plane.

>
> Note that for legacy interfaces there's no problem at all, we should set
> allow_modeset correctly for all of the legacy ioctl -> atomic helpers.
> -Daniel
>
>>
>> Suggested-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
>> Cc: David Airlie <airlied@xxxxxxxx>
>> Cc: Russell King <linux@xxxxxxxxxxxxxxx>
>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> Cc: Peter Senna Tschudin <peter.senna@xxxxxxxxx>
>> Signed-off-by: Liu Ying <gnuiyl@xxxxxxxxx>
>> ---
>>  drivers/gpu/drm/imx/imx-drm-core.c | 26 +++++++++++++++++++++-
>>  drivers/gpu/drm/imx/ipuv3-plane.c  | 44 +++++++++++++++++++++++++++++---------
>>  2 files changed, 59 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
>> index 6dc0ef4..c10c3ea 100644
>> --- a/drivers/gpu/drm/imx/imx-drm-core.c
>> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
>> @@ -147,10 +147,34 @@ static void imx_drm_output_poll_changed(struct drm_device *drm)
>>       drm_fbdev_cma_hotplug_event(imxdrm->fbhelper);
>>  }
>>
>> +static int imx_drm_atomic_check(struct drm_device *dev,
>> +                             struct drm_atomic_state *state)
>> +{
>> +     int ret;
>> +
>> +     ret = drm_atomic_helper_check_modeset(dev, state);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = drm_atomic_helper_check_planes(dev, state);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /*
>> +      * Check modeset again in case crtc_state->mode_changed is
>> +      * updated in plane's ->atomic_check callback.
>> +      */
>> +     ret = drm_atomic_helper_check_modeset(dev, state);
>> +     if (ret)
>> +             return ret;
>> +
>> +     return ret;
>> +}
>> +
>>  static const struct drm_mode_config_funcs imx_drm_mode_config_funcs = {
>>       .fb_create = drm_fb_cma_create,
>>       .output_poll_changed = imx_drm_output_poll_changed,
>> -     .atomic_check = drm_atomic_helper_check,
>> +     .atomic_check = imx_drm_atomic_check,
>>       .atomic_commit = drm_atomic_helper_commit,
>>  };
>>
>> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
>> index 4ad67d0..d4dde4a 100644
>> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
>> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
>> @@ -319,13 +319,21 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>>               return -EINVAL;
>>
>>       /*
>> -      * since we cannot touch active IDMAC channels, we do not support
>> -      * resizing the enabled plane or changing its format
>> +      * For primary plane, we support resizing active plane or changing
>> +      * its format by forcing CRTC mode change and disabling-enabling plane
>> +      * in plane's ->atomic_update callback.
>> +      * For overlay plane, we currently reject the active plane resizing
>> +      * or format change.
>>        */
>>       if (old_fb && (state->src_w != old_state->src_w ||
>>                             state->src_h != old_state->src_h ||
>> -                           fb->pixel_format != old_fb->pixel_format))
>> -             return -EINVAL;
>> +                           fb->pixel_format != old_fb->pixel_format)) {
>> +             if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
>> +                     crtc_state->mode_changed = true;
>> +             } else {
>> +                     return -EINVAL;
>> +             }
>> +     }
>>
>>       eba = drm_plane_state_to_eba(state);
>>
>> @@ -335,8 +343,13 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>>       if (fb->pitches[0] < 1 || fb->pitches[0] > 16384)
>>               return -EINVAL;
>>
>> -     if (old_fb && fb->pitches[0] != old_fb->pitches[0])
>> -             return -EINVAL;
>> +     if (old_fb && fb->pitches[0] != old_fb->pitches[0]) {
>> +             if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
>> +                     crtc_state->mode_changed = true;
>> +             } else {
>> +                     return -EINVAL;
>> +             }
>> +     }
>>
>>       switch (fb->pixel_format) {
>>       case DRM_FORMAT_YUV420:
>> @@ -371,8 +384,13 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>>               if (fb->pitches[1] < 1 || fb->pitches[1] > 16384)
>>                       return -EINVAL;
>>
>> -             if (old_fb && old_fb->pitches[1] != fb->pitches[1])
>> -                     return -EINVAL;
>> +             if (old_fb && old_fb->pitches[1] != fb->pitches[1]) {
>> +                     if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
>> +                             crtc_state->mode_changed = true;
>> +                     } else {
>> +                             return -EINVAL;
>> +                     }
>> +             }
>>       }
>>
>>       return 0;
>> @@ -392,8 +410,14 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>>       enum ipu_color_space ics;
>>
>>       if (old_state->fb) {
>> -             ipu_plane_atomic_set_base(ipu_plane, old_state);
>> -             return;
>> +             struct drm_crtc_state *crtc_state = state->crtc->state;
>> +
>> +             if (!crtc_state->mode_changed) {
>> +                     ipu_plane_atomic_set_base(ipu_plane, old_state);
>> +                     return;
>> +             }
>> +
>> +             ipu_disable_plane(plane);
>
> Most likely you want to put a call to disable all planes into you're
> crtc->disable function. This here smells like a hack. But I don't know imx
> hw enough to be able to tell.

Hmm.  Probably too invasive to disable all planes in crtc->disable.
The atomic core would add affected primary plane into state
to commit when the userpace asks to reconfigure an active
overlay plane only. So imo, probably not pretty much a hack -
updating one single plane a time in ->update_plane is somewhat
cleaner than disabling all planes in crtc->disable.

Regards,
Liu Ying

> -Daniel
>
>>       }
>>
>>       switch (ipu_plane->dp_flow) {
>> --
>> 2.7.4
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux