Re: [PATCH v4 3/7] drm/atomic-helper: Add NO_DISABLE_AFTER_MODESET flag support for plane commit

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Aug 29, 2016 at 4:25 PM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Fri, Aug 26, 2016 at 03:30:40PM +0800, Liu Ying wrote:
>> Drivers may set the NO_DISABLE_AFTER_MODESET flag in the 'flags' parameter
>> of the helper drm_atomic_helper_commit_planes() if the relevant display
>> controllers(e.g., IPUv3 for imx-drm) require to disable a CRTC's planes
>> when the CRTC is disabled. The helper would skip the ->atomic_disable
>> call for a plane if the CRTC of the old plane state needs a modesetting
>> operation. Of course, the drivers need to disable the planes in their CRTC
>> disable callbacks since no one else would do that.
>>
>> Suggested-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> Cc: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
>> Cc: David Airlie <airlied@xxxxxxxx>
>> Cc: Russell King <linux@xxxxxxxxxxxxxxx>
>> Cc: Peter Senna Tschudin <peter.senna@xxxxxxxxx>
>> Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
>> Signed-off-by: Liu Ying <gnuiyl@xxxxxxxxx>
>
> A few small nits below, otherwise looks good. I merged patches 1&2 into
> drm-misc already.

I'll address all your below comments and send v2 for this one separately.

Thanks.

Regards,
Liu Ying

>
> Thanks, Daniel
>
>> ---
>> v4:
>> * Newly introduced in v4.
>>
>>  drivers/gpu/drm/arm/malidp_drv.c             |  3 +-
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c |  2 +-
>>  drivers/gpu/drm/drm_atomic_helper.c          | 46 ++++++++++++++++++++--------
>>  drivers/gpu/drm/exynos/exynos_drm_drv.c      |  2 +-
>>  drivers/gpu/drm/imx/imx-drm-core.c           |  3 +-
>>  drivers/gpu/drm/mediatek/mtk_drm_drv.c       |  6 ++--
>>  drivers/gpu/drm/msm/msm_atomic.c             |  2 +-
>>  drivers/gpu/drm/omapdrm/omap_drv.c           |  2 +-
>>  drivers/gpu/drm/rcar-du/rcar_du_kms.c        |  3 +-
>>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c   |  3 +-
>>  drivers/gpu/drm/sti/sti_drv.c                |  2 +-
>>  drivers/gpu/drm/tegra/drm.c                  |  3 +-
>>  drivers/gpu/drm/tilcdc/tilcdc_drv.c          |  2 +-
>>  drivers/gpu/drm/vc4/vc4_kms.c                |  2 +-
>>  drivers/gpu/drm/virtio/virtgpu_display.c     |  3 +-
>>  include/drm/drm_atomic_helper.h              |  6 +++-
>>  16 files changed, 61 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
>> index 82171d2..c383d72 100644
>> --- a/drivers/gpu/drm/arm/malidp_drv.c
>> +++ b/drivers/gpu/drm/arm/malidp_drv.c
>> @@ -91,7 +91,8 @@ static void malidp_atomic_commit_tail(struct drm_atomic_state *state)
>>
>>       drm_atomic_helper_commit_modeset_disables(drm, state);
>>       drm_atomic_helper_commit_modeset_enables(drm, state);
>> -     drm_atomic_helper_commit_planes(drm, state, true);
>> +     drm_atomic_helper_commit_planes(drm, state,
>> +                                     DRM_PLANE_COMMIT_ACTIVE_ONLY);
>>
>>       malidp_atomic_commit_hw_done(state);
>>
>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> index d4a3d61..8e7483d 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> @@ -457,7 +457,7 @@ atmel_hlcdc_dc_atomic_complete(struct atmel_hlcdc_dc_commit *commit)
>>
>>       /* Apply the atomic update. */
>>       drm_atomic_helper_commit_modeset_disables(dev, old_state);
>> -     drm_atomic_helper_commit_planes(dev, old_state, false);
>> +     drm_atomic_helper_commit_planes(dev, old_state, 0);
>>       drm_atomic_helper_commit_modeset_enables(dev, old_state);
>>
>>       drm_atomic_helper_wait_for_vblanks(dev, old_state);
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 30c52a8..cf19b6e 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1148,7 +1148,8 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>>   *
>>   *     drm_atomic_helper_commit_modeset_enables(dev, state);
>>   *
>> - *     drm_atomic_helper_commit_planes(dev, state, true);
>> + *     drm_atomic_helper_commit_planes(dev, state,
>> + *                                     DRM_PLANE_COMMIT_ACTIVE_ONLY);
>>   *
>>   * for committing the atomic update to hardware.  See the kerneldoc entries for
>>   * these three functions for more details.
>> @@ -1159,7 +1160,7 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *state)
>>
>>       drm_atomic_helper_commit_modeset_disables(dev, state);
>>
>> -     drm_atomic_helper_commit_planes(dev, state, false);
>> +     drm_atomic_helper_commit_planes(dev, state, 0);
>>
>>       drm_atomic_helper_commit_modeset_enables(dev, state);
>>
>> @@ -1678,7 +1679,7 @@ bool plane_crtc_active(struct drm_plane_state *state)
>>   * drm_atomic_helper_commit_planes - commit plane state
>>   * @dev: DRM device
>>   * @old_state: atomic state object with old state structures
>> - * @active_only: Only commit on active CRTC if set
>> + * @flags: flags for committing plane state
>>   *
>>   * This function commits the new plane state using the plane and atomic helper
>>   * functions for planes and crtcs. It assumes that the atomic state has already
>> @@ -1698,25 +1699,33 @@ bool plane_crtc_active(struct drm_plane_state *state)
>>   * most drivers don't need to be immediately notified of plane updates for a
>>   * disabled CRTC.
>>   *
>> - * Unless otherwise needed, drivers are advised to set the @active_only
>> - * parameters to true in order not to receive plane update notifications related
>> - * to a disabled CRTC. This avoids the need to manually ignore plane updates in
>> + * Unless otherwise needed, drivers are advised to set the ACTIVE_ONLY flag in
>> + * @flags in order not to receive plane update notifications related to a
>> + * disabled CRTC. This avoids the need to manually ignore plane updates in
>>   * driver code when the driver and/or hardware can't or just don't need to deal
>>   * with updates on disabled CRTCs, for example when supporting runtime PM.
>> - *
>
> Please leave this newline here to start a new paragraph. I think that
> makes the docs more readable.
>
>> - * The drm_atomic_helper_commit() default implementation only sets @active_only
>> - * to false to most closely match the behaviour of the legacy helpers. This should
>> - * not be copied blindly by drivers.
>> + * Drivers may set the NO_DISABLE_AFTER_MODESET flag in @flags if the relevant
>> + * display controllers require to disable a CRTC's planes when the CRTC is
>> + * disabled. This function would skip the ->atomic_disable call for a plane if
>> + * the CRTC of the old plane state needs a modesetting operation. Of course,
>> + * the drivers need to disable the planes in their CRTC disable callbacks
>> + * since no one else would do that.
>> + *
>> + * The drm_atomic_helper_commit() default implementation doesn't set the
>> + * ACTIVE_ONLY flag to most closely match the behaviour of the legacy helpers.
>> + * This should not be copied blindly by drivers.
>>   */
>>  void drm_atomic_helper_commit_planes(struct drm_device *dev,
>>                                    struct drm_atomic_state *old_state,
>> -                                  bool active_only)
>> +                                  uint32_t flags)
>>  {
>>       struct drm_crtc *crtc;
>>       struct drm_crtc_state *old_crtc_state;
>>       struct drm_plane *plane;
>>       struct drm_plane_state *old_plane_state;
>>       int i;
>> +     bool active_only = flags & DRM_PLANE_COMMIT_ACTIVE_ONLY;
>> +     bool no_disable = flags & DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET;
>>
>>       for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
>>               const struct drm_crtc_helper_funcs *funcs;
>> @@ -1760,9 +1769,20 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
>>               /*
>>                * Special-case disabling the plane if drivers support it.
>>                */
>> -             if (disabling && funcs->atomic_disable)
>> +             if (disabling && funcs->atomic_disable) {
>> +                     struct drm_crtc_state *crtc_state;
>> +
>> +                     crtc_state = old_plane_state->crtc->state;
>> +                     WARN_ON(!crtc_state);
>> +                     if (!crtc_state)
>> +                             continue;
>
> This is already enforced by the atomic core, I don't think there's any
> point in checking for the crtc state here. Please remove the WARN_ON and
> if () continue;
>
>> +
>> +                     if (drm_atomic_crtc_needs_modeset(crtc_state) &&
>> +                         no_disable)
>> +                             continue;
>> +
>>                       funcs->atomic_disable(plane, old_plane_state);
>> -             else if (plane->state->crtc || disabling)
>> +             } else if (plane->state->crtc || disabling)
>>                       funcs->atomic_update(plane, old_plane_state);
>
> Checkpatch says you should have {} for the else too.
>
>>       }
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> index 877d2ef..486943e 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> @@ -105,7 +105,7 @@ static void exynos_atomic_commit_complete(struct exynos_atomic_commit *commit)
>>               atomic_inc(&exynos_crtc->pending_update);
>>       }
>>
>> -     drm_atomic_helper_commit_planes(dev, state, false);
>> +     drm_atomic_helper_commit_planes(dev, state, 0);
>>
>>       exynos_atomic_wait_for_commit(state);
>>
>> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
>> index 438bac8..56dfc4c 100644
>> --- a/drivers/gpu/drm/imx/imx-drm-core.c
>> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
>> @@ -193,7 +193,8 @@ static void imx_drm_atomic_commit_tail(struct drm_atomic_state *state)
>>
>>       drm_atomic_helper_commit_modeset_disables(dev, state);
>>
>> -     drm_atomic_helper_commit_planes(dev, state, true);
>> +     drm_atomic_helper_commit_planes(dev, state,
>> +                                     DRM_PLANE_COMMIT_ACTIVE_ONLY);
>>
>>       drm_atomic_helper_commit_modeset_enables(dev, state);
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>> index 0e769ab..72c1ae4 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
>> @@ -70,13 +70,15 @@ static void mtk_atomic_complete(struct mtk_drm_private *private,
>>        *
>>        *     drm_atomic_helper_commit_modeset_disables(dev, state);
>>        *     drm_atomic_helper_commit_modeset_enables(dev, state);
>> -      *     drm_atomic_helper_commit_planes(dev, state, true);
>> +      *     drm_atomic_helper_commit_planes(dev, state,
>> +      *                                     DRM_PLANE_COMMIT_ACTIVE_ONLY);
>>        *
>>        * See the kerneldoc entries for these three functions for more details.
>>        */
>>       drm_atomic_helper_commit_modeset_disables(drm, state);
>>       drm_atomic_helper_commit_modeset_enables(drm, state);
>> -     drm_atomic_helper_commit_planes(drm, state, true);
>> +     drm_atomic_helper_commit_planes(drm, state,
>> +                                     DRM_PLANE_COMMIT_ACTIVE_ONLY);
>>
>>       drm_atomic_helper_wait_for_vblanks(drm, state);
>>
>> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
>> index 4a8a6f1..5df252c 100644
>> --- a/drivers/gpu/drm/msm/msm_atomic.c
>> +++ b/drivers/gpu/drm/msm/msm_atomic.c
>> @@ -118,7 +118,7 @@ static void complete_commit(struct msm_commit *c, bool async)
>>
>>       drm_atomic_helper_commit_modeset_disables(dev, state);
>>
>> -     drm_atomic_helper_commit_planes(dev, state, false);
>> +     drm_atomic_helper_commit_planes(dev, state, 0);
>>
>>       drm_atomic_helper_commit_modeset_enables(dev, state);
>>
>> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
>> index 3dd78f2..e1cfba5 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> @@ -96,7 +96,7 @@ static void omap_atomic_complete(struct omap_atomic_state_commit *commit)
>>       dispc_runtime_get();
>>
>>       drm_atomic_helper_commit_modeset_disables(dev, old_state);
>> -     drm_atomic_helper_commit_planes(dev, old_state, false);
>> +     drm_atomic_helper_commit_planes(dev, old_state, 0);
>>       drm_atomic_helper_commit_modeset_enables(dev, old_state);
>>
>>       omap_atomic_wait_for_completion(dev, old_state);
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> index f03eb55..bd9c3bb 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> @@ -257,7 +257,8 @@ static void rcar_du_atomic_complete(struct rcar_du_commit *commit)
>>       /* Apply the atomic update. */
>>       drm_atomic_helper_commit_modeset_disables(dev, old_state);
>>       drm_atomic_helper_commit_modeset_enables(dev, old_state);
>> -     drm_atomic_helper_commit_planes(dev, old_state, true);
>> +     drm_atomic_helper_commit_planes(dev, old_state,
>> +                                     DRM_PLANE_COMMIT_ACTIVE_ONLY);
>>
>>       drm_atomic_helper_wait_for_vblanks(dev, old_state);
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>> index 55c5273..7ca8f34 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>> @@ -233,7 +233,8 @@ rockchip_atomic_commit_tail(struct drm_atomic_state *state)
>>
>>       drm_atomic_helper_commit_modeset_enables(dev, state);
>>
>> -     drm_atomic_helper_commit_planes(dev, state, true);
>> +     drm_atomic_helper_commit_planes(dev, state,
>> +                                     DRM_PLANE_COMMIT_ACTIVE_ONLY);
>>
>>       drm_atomic_helper_commit_hw_done(state);
>>
>> diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c
>> index f8311b2..7cd3804 100644
>> --- a/drivers/gpu/drm/sti/sti_drv.c
>> +++ b/drivers/gpu/drm/sti/sti_drv.c
>> @@ -178,7 +178,7 @@ static void sti_atomic_complete(struct sti_private *private,
>>        */
>>
>>       drm_atomic_helper_commit_modeset_disables(drm, state);
>> -     drm_atomic_helper_commit_planes(drm, state, false);
>> +     drm_atomic_helper_commit_planes(drm, state, 0);
>>       drm_atomic_helper_commit_modeset_enables(drm, state);
>>
>>       drm_atomic_helper_wait_for_vblanks(drm, state);
>> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
>> index 755264d9d..4b9f1c7 100644
>> --- a/drivers/gpu/drm/tegra/drm.c
>> +++ b/drivers/gpu/drm/tegra/drm.c
>> @@ -57,7 +57,8 @@ static void tegra_atomic_complete(struct tegra_drm *tegra,
>>
>>       drm_atomic_helper_commit_modeset_disables(drm, state);
>>       drm_atomic_helper_commit_modeset_enables(drm, state);
>> -     drm_atomic_helper_commit_planes(drm, state, true);
>> +     drm_atomic_helper_commit_planes(drm, state,
>> +                                     DRM_PLANE_COMMIT_ACTIVE_ONLY);
>>
>>       drm_atomic_helper_wait_for_vblanks(drm, state);
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> index 3404d24..4405e4b 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> @@ -118,7 +118,7 @@ static int tilcdc_commit(struct drm_device *dev,
>>
>>       drm_atomic_helper_commit_modeset_disables(dev, state);
>>
>> -     drm_atomic_helper_commit_planes(dev, state, false);
>> +     drm_atomic_helper_commit_planes(dev, state, 0);
>>
>>       drm_atomic_helper_commit_modeset_enables(dev, state);
>>
>> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
>> index 4ac894d..c1f65c6 100644
>> --- a/drivers/gpu/drm/vc4/vc4_kms.c
>> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
>> @@ -44,7 +44,7 @@ vc4_atomic_complete_commit(struct vc4_commit *c)
>>
>>       drm_atomic_helper_commit_modeset_disables(dev, state);
>>
>> -     drm_atomic_helper_commit_planes(dev, state, false);
>> +     drm_atomic_helper_commit_planes(dev, state, 0);
>>
>>       drm_atomic_helper_commit_modeset_enables(dev, state);
>>
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
>> index 4e192aa..7cf3678 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_display.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_display.c
>> @@ -338,7 +338,8 @@ static void vgdev_atomic_commit_tail(struct drm_atomic_state *state)
>>
>>       drm_atomic_helper_commit_modeset_disables(dev, state);
>>       drm_atomic_helper_commit_modeset_enables(dev, state);
>> -     drm_atomic_helper_commit_planes(dev, state, true);
>> +     drm_atomic_helper_commit_planes(dev, state,
>> +                                     DRM_PLANE_COMMIT_ACTIVE_ONLY);
>>
>>       drm_atomic_helper_commit_hw_done(state);
>>
>> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
>> index 1abf2c0..f866828 100644
>> --- a/include/drm/drm_atomic_helper.h
>> +++ b/include/drm/drm_atomic_helper.h
>> @@ -65,9 +65,13 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>>
>>  int drm_atomic_helper_prepare_planes(struct drm_device *dev,
>>                                    struct drm_atomic_state *state);
>> +
>> +#define DRM_PLANE_COMMIT_ACTIVE_ONLY                 BIT(0)
>> +#define DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET    BIT(1)
>> +
>>  void drm_atomic_helper_commit_planes(struct drm_device *dev,
>>                                    struct drm_atomic_state *state,
>> -                                  bool active_only);
>> +                                  uint32_t flags);
>>  void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
>>                                     struct drm_atomic_state *old_state);
>>  void drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state);
>> --
>> 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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux