[PATCH] drm/imx: Add active primary plane reconfiguration support
Liu Ying
liu.y.victor at gmail.com
Mon Aug 15 08:23:33 UTC 2016
2016-08-15 15:18 GMT+08:00 Daniel Vetter <daniel at ffwll.ch>:
> 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 at ffwll.ch>
>> Cc: Philipp Zabel <p.zabel at pengutronix.de>
>> Cc: David Airlie <airlied at linux.ie>
>> Cc: Russell King <linux at armlinux.org.uk>
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Cc: Peter Senna Tschudin <peter.senna at gmail.com>
>> Signed-off-by: Liu Ying <gnuiyl at gmail.com>
>> ---
>> 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list