On Mon, Aug 15, 2016 at 04:23:33PM +0800, Liu Ying wrote:
2016-08-15 15:18 GMT+08:00 Daniel Vetter daniel@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@ffwll.ch Cc: Philipp Zabel p.zabel@pengutronix.de Cc: David Airlie airlied@linux.ie Cc: Russell King linux@armlinux.org.uk Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Peter Senna Tschudin peter.senna@gmail.com Signed-off-by: Liu Ying gnuiyl@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.
See drm_atomic_helper_disable_planes_on_crtc(), that's the helper you're supposed to look for. Unfortunately it's buggy :( It looks at plane->state->crtc, which is the new crtc, and not the old crtc. The correct fix is to pass in the old crtc_state into this function, so that we can loop over the crtc_state->plane_mask and shut down all the planes. But to be able to do that from crtc callbacks, we first need to add new atomic versions which pass down the state. I discussed this issue with Ben Skeggs, and I think he's looking into fixing it. But that will take some time.
If you don't want to fix this all the only other option is to have a loop over all planes in a hw-specific way to shut down all the planes which are active from crtc->disable. -Daniel
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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel