| Message ID | 1471599419-29009-4-git-send-email-gnuiyl@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 19, 2016 at 05:36:59PM +0800, Liu Ying wrote: > We don't support configuring active 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. > > This patch adds active plane reconfiguration support by forcing CRTC > mode change in plane's ->atomic_check callback and adding disable > operation for all appropriate active planes(when necessary) in CRTC's > ->atomic_disable callback. The atomic core would reenabled the > affected CRTC and planes at atomic commit stage. > > 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> > Cc: Lucas Stach <l.stach@pengutronix.de> > Signed-off-by: Liu Ying <gnuiyl@gmail.com> > --- > v2->v3: > * Disable all appropriate affected planes(when necessary) in CRTC's > ->atomic_disable callback, but not in each plane's ->atomic_update callback, > as suggested by Daniel Vetter. > * +Cc Lucas Stach, as he tested the patch v2. > > v1->v2: > * Do not reject reconfiguring an active overlay plane. > > drivers/gpu/drm/imx/imx-drm-core.c | 26 +++++++++++++++++++++++++- > drivers/gpu/drm/imx/ipuv3-crtc.c | 26 ++++++++++++++++++++++++++ > drivers/gpu/drm/imx/ipuv3-plane.c | 21 ++++++++++++++------- > 3 files changed, 65 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c > index 438bac8..d61b048 100644 > --- a/drivers/gpu/drm/imx/imx-drm-core.c > +++ b/drivers/gpu/drm/imx/imx-drm-core.c > @@ -146,10 +146,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-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c > index 83c46bd..0779eed 100644 > --- a/drivers/gpu/drm/imx/ipuv3-crtc.c > +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c > @@ -76,6 +76,32 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc, > crtc->state->event = NULL; > } > spin_unlock_irq(&crtc->dev->event_lock); > + > + /* > + * The relevant plane's ->atomic_check callback may set > + * crtc_state->mode_changed to be true when the active > + * plane needs to be reconfigured. In this case and only > + * this case, active_changed is false - we disable all the > + * appropriate active planes here. > + */ > + if (!crtc->state->active_changed) { > + struct drm_plane *plane; > + > + drm_atomic_crtc_state_for_each_plane(plane, old_crtc_state) { > + const struct drm_plane_helper_funcs *plane_funcs = > + plane->helper_private; > + > + /* > + * Filter out the plane which is explicitly required > + * to be disabled by the user via atomic commit > + * so that it won't be accidentally disabled twice. > + */ > + if (!plane->state->crtc) > + continue; I think the better place would be to do the filtering in commit_planes(), not here. And would be great to include your patch to fix up disable_planes_on_crtc(), too. -Daniel > + > + plane_funcs->atomic_disable(plane, NULL); > + } > + } > } > > static void imx_drm_crtc_reset(struct drm_crtc *crtc) > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c > index 4ad67d0..6063ebe 100644 > --- a/drivers/gpu/drm/imx/ipuv3-plane.c > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c > @@ -319,13 +319,16 @@ 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 > + * We support resizing active plane or changing its format by > + * forcing CRTC mode change in plane's ->atomic_check callback > + * and disabling all appropriate active planes in CRTC's > + * ->atomic_disable callback. The planes will be reenabled in > + * plane's ->atomic_update callback. > */ > 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; > + crtc_state->mode_changed = true; > > eba = drm_plane_state_to_eba(state); > > @@ -336,7 +339,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > return -EINVAL; > > if (old_fb && fb->pitches[0] != old_fb->pitches[0]) > - return -EINVAL; > + crtc_state->mode_changed = true; > > switch (fb->pixel_format) { > case DRM_FORMAT_YUV420: > @@ -372,7 +375,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, > return -EINVAL; > > if (old_fb && old_fb->pitches[1] != fb->pitches[1]) > - return -EINVAL; > + crtc_state->mode_changed = true; > } > > return 0; > @@ -392,8 +395,12 @@ 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; > + } > } > > switch (ipu_plane->dp_flow) { > -- > 2.7.4 >
Hi Daniel, On Mon, Aug 22, 2016 at 3:20 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Aug 19, 2016 at 05:36:59PM +0800, Liu Ying wrote: >> We don't support configuring active 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. >> >> This patch adds active plane reconfiguration support by forcing CRTC >> mode change in plane's ->atomic_check callback and adding disable >> operation for all appropriate active planes(when necessary) in CRTC's >> ->atomic_disable callback. The atomic core would reenabled the >> affected CRTC and planes at atomic commit stage. >> >> 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> >> Cc: Lucas Stach <l.stach@pengutronix.de> >> Signed-off-by: Liu Ying <gnuiyl@gmail.com> >> --- >> v2->v3: >> * Disable all appropriate affected planes(when necessary) in CRTC's >> ->atomic_disable callback, but not in each plane's ->atomic_update callback, >> as suggested by Daniel Vetter. >> * +Cc Lucas Stach, as he tested the patch v2. >> >> v1->v2: >> * Do not reject reconfiguring an active overlay plane. >> >> drivers/gpu/drm/imx/imx-drm-core.c | 26 +++++++++++++++++++++++++- >> drivers/gpu/drm/imx/ipuv3-crtc.c | 26 ++++++++++++++++++++++++++ >> drivers/gpu/drm/imx/ipuv3-plane.c | 21 ++++++++++++++------- >> 3 files changed, 65 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c >> index 438bac8..d61b048 100644 >> --- a/drivers/gpu/drm/imx/imx-drm-core.c >> +++ b/drivers/gpu/drm/imx/imx-drm-core.c >> @@ -146,10 +146,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-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c >> index 83c46bd..0779eed 100644 >> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c >> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c >> @@ -76,6 +76,32 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc, >> crtc->state->event = NULL; >> } >> spin_unlock_irq(&crtc->dev->event_lock); >> + >> + /* >> + * The relevant plane's ->atomic_check callback may set >> + * crtc_state->mode_changed to be true when the active >> + * plane needs to be reconfigured. In this case and only >> + * this case, active_changed is false - we disable all the >> + * appropriate active planes here. >> + */ >> + if (!crtc->state->active_changed) { >> + struct drm_plane *plane; >> + >> + drm_atomic_crtc_state_for_each_plane(plane, old_crtc_state) { >> + const struct drm_plane_helper_funcs *plane_funcs = >> + plane->helper_private; >> + >> + /* >> + * Filter out the plane which is explicitly required >> + * to be disabled by the user via atomic commit >> + * so that it won't be accidentally disabled twice. >> + */ >> + if (!plane->state->crtc) >> + continue; > > I think the better place would be to do the filtering in commit_planes(), > not here. And would be great to include your patch to fix up > disable_planes_on_crtc(), too. Do you mean we can do the filtering by checking (!plane->state->crtc) right before plane_funcs->atomic_disable in commit_planes()? Won't it filter out all the apples? commit_planes() doesn't know whether the driver calls disable_planes_on_crtc() or not. imo, doing the filtering in crtc_funcs->atomic_disable is sane. Regards, Liu Ying > -Daniel > >> + >> + plane_funcs->atomic_disable(plane, NULL); >> + } >> + } >> } >> >> static void imx_drm_crtc_reset(struct drm_crtc *crtc) >> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c >> index 4ad67d0..6063ebe 100644 >> --- a/drivers/gpu/drm/imx/ipuv3-plane.c >> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c >> @@ -319,13 +319,16 @@ 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 >> + * We support resizing active plane or changing its format by >> + * forcing CRTC mode change in plane's ->atomic_check callback >> + * and disabling all appropriate active planes in CRTC's >> + * ->atomic_disable callback. The planes will be reenabled in >> + * plane's ->atomic_update callback. >> */ >> 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; >> + crtc_state->mode_changed = true; >> >> eba = drm_plane_state_to_eba(state); >> >> @@ -336,7 +339,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, >> return -EINVAL; >> >> if (old_fb && fb->pitches[0] != old_fb->pitches[0]) >> - return -EINVAL; >> + crtc_state->mode_changed = true; >> >> switch (fb->pixel_format) { >> case DRM_FORMAT_YUV420: >> @@ -372,7 +375,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, >> return -EINVAL; >> >> if (old_fb && old_fb->pitches[1] != fb->pitches[1]) >> - return -EINVAL; >> + crtc_state->mode_changed = true; >> } >> >> return 0; >> @@ -392,8 +395,12 @@ 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; >> + } >> } >> >> switch (ipu_plane->dp_flow) { >> -- >> 2.7.4 >> > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Mon, Aug 22, 2016 at 9:43 AM, Ying Liu <gnuiyl@gmail.com> wrote: >>> + >>> + /* >>> + * The relevant plane's ->atomic_check callback may set >>> + * crtc_state->mode_changed to be true when the active >>> + * plane needs to be reconfigured. In this case and only >>> + * this case, active_changed is false - we disable all the >>> + * appropriate active planes here. >>> + */ >>> + if (!crtc->state->active_changed) { >>> + struct drm_plane *plane; >>> + >>> + drm_atomic_crtc_state_for_each_plane(plane, old_crtc_state) { >>> + const struct drm_plane_helper_funcs *plane_funcs = >>> + plane->helper_private; >>> + >>> + /* >>> + * Filter out the plane which is explicitly required >>> + * to be disabled by the user via atomic commit >>> + * so that it won't be accidentally disabled twice. >>> + */ >>> + if (!plane->state->crtc) >>> + continue; >> >> I think the better place would be to do the filtering in commit_planes(), >> not here. And would be great to include your patch to fix up >> disable_planes_on_crtc(), too. > > Do you mean we can do the filtering by checking (!plane->state->crtc) > right before plane_funcs->atomic_disable in commit_planes()? > Won't it filter out all the apples? > commit_planes() doesn't know whether the driver calls > disable_planes_on_crtc() or not. You need to filter on needs_modeset(crtc_state), and it needs to be optional like active_only, using a flag. > imo, doing the filtering in crtc_funcs->atomic_disable is sane. It is not sane in general, since usually you need to call disable_planes_on_crtc to avoid upsetting your hardware. And for that use-case filtering in disable will lead to hung hardware. Which really means that you need to filter in commit_planes. -Daniel
On Mon, Aug 22, 2016 at 3:53 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Aug 22, 2016 at 9:43 AM, Ying Liu <gnuiyl@gmail.com> wrote: >>>> + >>>> + /* >>>> + * The relevant plane's ->atomic_check callback may set >>>> + * crtc_state->mode_changed to be true when the active >>>> + * plane needs to be reconfigured. In this case and only >>>> + * this case, active_changed is false - we disable all the >>>> + * appropriate active planes here. >>>> + */ >>>> + if (!crtc->state->active_changed) { >>>> + struct drm_plane *plane; >>>> + >>>> + drm_atomic_crtc_state_for_each_plane(plane, old_crtc_state) { >>>> + const struct drm_plane_helper_funcs *plane_funcs = >>>> + plane->helper_private; >>>> + >>>> + /* >>>> + * Filter out the plane which is explicitly required >>>> + * to be disabled by the user via atomic commit >>>> + * so that it won't be accidentally disabled twice. >>>> + */ >>>> + if (!plane->state->crtc) >>>> + continue; >>> >>> I think the better place would be to do the filtering in commit_planes(), >>> not here. And would be great to include your patch to fix up >>> disable_planes_on_crtc(), too. >> >> Do you mean we can do the filtering by checking (!plane->state->crtc) >> right before plane_funcs->atomic_disable in commit_planes()? >> Won't it filter out all the apples? >> commit_planes() doesn't know whether the driver calls >> disable_planes_on_crtc() or not. > > You need to filter on needs_modeset(crtc_state), and it needs to be > optional like active_only, using a flag. Then, IIUC, the flag will be CRTC specific and be dynamically changeable. I'm not clear about the way to implement this. > >> imo, doing the filtering in crtc_funcs->atomic_disable is sane. > > It is not sane in general, since usually you need to call > disable_planes_on_crtc to avoid upsetting your hardware. And for that Calling disable_planes_on_crtc() in crtc_funcs->atomic_disable is a bit over-kill and will cause the issue of disabling plane twice, unless the filtering is done in disable_planes_on_crtc() (which was rejected by you on irc) or in commit_planes()?(which I'm not clear about the way to implement, as I mentioned before). So I chose to do the filtering in the imx-drm specific crtc_funcs->atomic_disable function. > use-case filtering in disable will lead to hung hardware. Which really Hung hardware due to filtering? How? > means that you need to filter in commit_planes. Really don't understand the rational for filtering in commit_planes... Did I miss anything? Regards, Liu Ying > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Aug 22, 2016 at 04:23:36PM +0800, Ying Liu wrote: > On Mon, Aug 22, 2016 at 3:53 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Mon, Aug 22, 2016 at 9:43 AM, Ying Liu <gnuiyl@gmail.com> wrote: > >>>> + > >>>> + /* > >>>> + * The relevant plane's ->atomic_check callback may set > >>>> + * crtc_state->mode_changed to be true when the active > >>>> + * plane needs to be reconfigured. In this case and only > >>>> + * this case, active_changed is false - we disable all the > >>>> + * appropriate active planes here. > >>>> + */ > >>>> + if (!crtc->state->active_changed) { > >>>> + struct drm_plane *plane; > >>>> + > >>>> + drm_atomic_crtc_state_for_each_plane(plane, old_crtc_state) { > >>>> + const struct drm_plane_helper_funcs *plane_funcs = > >>>> + plane->helper_private; > >>>> + > >>>> + /* > >>>> + * Filter out the plane which is explicitly required > >>>> + * to be disabled by the user via atomic commit > >>>> + * so that it won't be accidentally disabled twice. > >>>> + */ > >>>> + if (!plane->state->crtc) > >>>> + continue; > >>> > >>> I think the better place would be to do the filtering in commit_planes(), > >>> not here. And would be great to include your patch to fix up > >>> disable_planes_on_crtc(), too. > >> > >> Do you mean we can do the filtering by checking (!plane->state->crtc) > >> right before plane_funcs->atomic_disable in commit_planes()? > >> Won't it filter out all the apples? > >> commit_planes() doesn't know whether the driver calls > >> disable_planes_on_crtc() or not. > > > > You need to filter on needs_modeset(crtc_state), and it needs to be > > optional like active_only, using a flag. > > Then, IIUC, the flag will be CRTC specific and be dynamically > changeable. I'm not clear about the way to implement this. > > > > >> imo, doing the filtering in crtc_funcs->atomic_disable is sane. > > > > It is not sane in general, since usually you need to call > > disable_planes_on_crtc to avoid upsetting your hardware. And for that > > Calling disable_planes_on_crtc() in crtc_funcs->atomic_disable is > a bit over-kill and will cause the issue of disabling plane twice, unless > the filtering is done in disable_planes_on_crtc() (which was rejected > by you on irc) or in commit_planes()?(which I'm not clear about > the way to implement, as I mentioned before). So I chose to do the > filtering in the imx-drm specific crtc_funcs->atomic_disable function. For filtering in commit_planes I think the best plane is to - add a new flag NO_DISABLE_AFTER_MODESET - which does exactly what it says on the tin: If the commmit_planes helper would call plane_funcs->atomic_disable, but drm_atomic_crtc_needs_modeset() for that crtc is true then you skip the ->atomic_disable call. This assumes that all disables have already been committed when disabling the crtc. For a lot of hardware it only makes sense to set both this flag and ACTIVE_ONLY, but there's probably some fun hardware out there where this is not the case (yours seems to be one example). Now it's not pretty to have 2 boolean arguments for the same function, so we also need to convert that into a bitflag field. End result: #define COMMIT_ACTIVE_ONLY BIT(0) #define COMMIT_NO_DISABLE_AFTER_MODESET BIT(1) Ofc kernel-doc should explain what the flags are for and when to use them. void drm_atomic_helper_commit_planes(struct drm_device *dev, struct drm_atomic_state *state, unsigned flags); A bit more work, but I think overall worth it. > > use-case filtering in disable will lead to hung hardware. Which really > > Hung hardware due to filtering? How? If you shut down the crtc and keep the planes running the hw dies. Yup, this happens. > > means that you need to filter in commit_planes. > > Really don't understand the rational for filtering in commit_planes... > Did I miss anything? See above, it might work for your driver, but it definitely restricts the usefulness of the helpers in general. And helpers which are only useful for 1 driver aren't useful. I hope this explains my idea a bit better, otherwise probably best if you ping me on irc. -Daniel
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index 438bac8..d61b048 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -146,10 +146,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-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c index 83c46bd..0779eed 100644 --- a/drivers/gpu/drm/imx/ipuv3-crtc.c +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c @@ -76,6 +76,32 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc, crtc->state->event = NULL; } spin_unlock_irq(&crtc->dev->event_lock); + + /* + * The relevant plane's ->atomic_check callback may set + * crtc_state->mode_changed to be true when the active + * plane needs to be reconfigured. In this case and only + * this case, active_changed is false - we disable all the + * appropriate active planes here. + */ + if (!crtc->state->active_changed) { + struct drm_plane *plane; + + drm_atomic_crtc_state_for_each_plane(plane, old_crtc_state) { + const struct drm_plane_helper_funcs *plane_funcs = + plane->helper_private; + + /* + * Filter out the plane which is explicitly required + * to be disabled by the user via atomic commit + * so that it won't be accidentally disabled twice. + */ + if (!plane->state->crtc) + continue; + + plane_funcs->atomic_disable(plane, NULL); + } + } } static void imx_drm_crtc_reset(struct drm_crtc *crtc) diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index 4ad67d0..6063ebe 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -319,13 +319,16 @@ 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 + * We support resizing active plane or changing its format by + * forcing CRTC mode change in plane's ->atomic_check callback + * and disabling all appropriate active planes in CRTC's + * ->atomic_disable callback. The planes will be reenabled in + * plane's ->atomic_update callback. */ 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; + crtc_state->mode_changed = true; eba = drm_plane_state_to_eba(state); @@ -336,7 +339,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, return -EINVAL; if (old_fb && fb->pitches[0] != old_fb->pitches[0]) - return -EINVAL; + crtc_state->mode_changed = true; switch (fb->pixel_format) { case DRM_FORMAT_YUV420: @@ -372,7 +375,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, return -EINVAL; if (old_fb && old_fb->pitches[1] != fb->pitches[1]) - return -EINVAL; + crtc_state->mode_changed = true; } return 0; @@ -392,8 +395,12 @@ 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; + } } switch (ipu_plane->dp_flow) {
We don't support configuring active 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. This patch adds active plane reconfiguration support by forcing CRTC mode change in plane's ->atomic_check callback and adding disable operation for all appropriate active planes(when necessary) in CRTC's ->atomic_disable callback. The atomic core would reenabled the affected CRTC and planes at atomic commit stage. 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> Cc: Lucas Stach <l.stach@pengutronix.de> Signed-off-by: Liu Ying <gnuiyl@gmail.com> --- v2->v3: * Disable all appropriate affected planes(when necessary) in CRTC's ->atomic_disable callback, but not in each plane's ->atomic_update callback, as suggested by Daniel Vetter. * +Cc Lucas Stach, as he tested the patch v2. v1->v2: * Do not reject reconfiguring an active overlay plane. drivers/gpu/drm/imx/imx-drm-core.c | 26 +++++++++++++++++++++++++- drivers/gpu/drm/imx/ipuv3-crtc.c | 26 ++++++++++++++++++++++++++ drivers/gpu/drm/imx/ipuv3-plane.c | 21 ++++++++++++++------- 3 files changed, 65 insertions(+), 8 deletions(-)