Hi Daniel,
On Mon, Aug 22, 2016 at 3:20 PM, Daniel Vetter <daniel@xxxxxxxx> 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@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>
>> Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
>> Signed-off-by: Liu Ying <gnuiyl@xxxxxxxxx>
>> ---
>> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
