On Mon, Aug 15, 2016 at 04:23:33PM +0800, Liu Ying wrote:
> 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.
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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
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
