There are some typos according to dim checkpatch:
6421619a1655 (HEAD) drm/meson: Fixes for drm_crtc_vblank_on/off support
-:48: WARNING:TYPO_SPELLING: 'occurence' may be misspelled - perhaps
'occurrence'?
#48:
The issue is that vblank need to be enabled in any occurence of :
-:53: WARNING:TYPO_SPELLING: 'occurence' may be misspelled - perhaps
'occurrence'?
#53:
of these occurence solves this race condition and makes sure vblank
-:60: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description
(prefer a maximum 75 chars per line)
#60:
- Initialize and enable vblank at the first _atomic_begin()/_atomic_enable()
total: 0 errors, 3 warnings, 0 checks, 82 lines checked
But I don't mind correcting those myself. Other then that, looks good
Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>
Will push to drm-misc-fixes in just a moment with the typo fixes/cc to
stable
On Thu, 2018-11-22 at 17:01 +0100, Neil Armstrong wrote:
> Since Linux 4.17, calls to drm_crtc_vblank_on/off are mandatory, and we get
> a warning when ctrc is disabled :
> " driver forgot to call drm_crtc_vblank_off()"
>
> But, the vsync IRQ was not totally disabled due the transient hardware
> state and specific interrupt line, thus adding proper IRQ masking from
> the HHI system control registers.
>
> The last change fixes a race condition introduced by calling the added
> drm_crtc_vblank_on/off when an HPD event occurs from the HDMI connector,
> triggering a WARN_ON() in the _atomic_begin() callback when the CRTC
> is disabled, thus also triggering a WARN_ON() in drm_vblank_put() :
>
> WARNING: CPU: 0 PID: 1185 at drivers/gpu/drm/meson/meson_crtc.c:157
> meson_crtc_atomic_begin+0x78/0x80
> [...]
> Call trace:
> meson_crtc_atomic_begin+0x78/0x80
> drm_atomic_helper_commit_planes+0x140/0x218
> drm_atomic_helper_commit_tail+0x38/0x80
> commit_tail+0x7c/0x80
> drm_atomic_helper_commit+0xdc/0x150
> drm_atomic_commit+0x54/0x60
> restore_fbdev_mode_atomic+0x198/0x238
> restore_fbdev_mode+0x6c/0x1c0
> drm_fb_helper_restore_fbdev_mode_unlocked+0x7c/0xf0
> drm_fb_helper_set_par+0x34/0x60
> drm_fb_helper_hotplug_event.part.28+0xb8/0xc8
> drm_fbdev_client_hotplug+0xa4/0xe0
> drm_client_dev_hotplug+0x90/0xe0
> drm_kms_helper_hotplug_event+0x3c/0x48
> drm_helper_hpd_irq_event+0x134/0x168
> dw_hdmi_top_thread_irq+0x3c/0x50
> [...]
> WARNING: CPU: 0 PID: 1185 at drivers/gpu/drm/drm_vblank.c:1026
> drm_vblank_put+0xb4/0xc8
> [...]
> Call trace:
> drm_vblank_put+0xb4/0xc8
> drm_crtc_vblank_put+0x24/0x30
> drm_atomic_helper_wait_for_vblanks.part.9+0x130/0x2b8
> drm_atomic_helper_commit_tail+0x68/0x80
> [...]
>
> The issue is that vblank need to be enabled in any occurence of :
> - atomic_enable()
> - atomic_begin() and state->enable == true, which was not the case
>
> Moving the CRTC enable code to a common function and calling in one
> of these occurence solves this race condition and makes sure vblank
> is enabled in each call to _atomic_begin() from the HPD event leading
> to drm_atomic_helper_commit_planes().
>
> To Summarize :
> - Make sure that the CRTC code will call the drm_crtc_vblank_on()/off()
> - *Really* mask the Vsync IRQ
> - Initialize and enable vblank at the first _atomic_begin()/_atomic_enable()
>
> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> ---
> drivers/gpu/drm/meson/meson_crtc.c | 27 +++++++++++++++++++++++++--
> drivers/gpu/drm/meson/meson_venc.c | 3 +++
> 2 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_crtc.c
> b/drivers/gpu/drm/meson/meson_crtc.c
> index d78168f979db..75d97f1b2e8f 100644
> --- a/drivers/gpu/drm/meson/meson_crtc.c
> +++ b/drivers/gpu/drm/meson/meson_crtc.c
> @@ -46,6 +46,7 @@ struct meson_crtc {
> struct drm_crtc base;
> struct drm_pending_vblank_event *event;
> struct meson_drm *priv;
> + bool enabled;
> };
> #define to_meson_crtc(x) container_of(x, struct meson_crtc, base)
>
> @@ -81,8 +82,7 @@ static const struct drm_crtc_funcs meson_crtc_funcs = {
>
> };
>
> -static void meson_crtc_atomic_enable(struct drm_crtc *crtc,
> - struct drm_crtc_state *old_state)
> +static void meson_crtc_enable(struct drm_crtc *crtc)
> {
> struct meson_crtc *meson_crtc = to_meson_crtc(crtc);
> struct drm_crtc_state *crtc_state = crtc->state;
> @@ -106,6 +106,22 @@ static void meson_crtc_atomic_enable(struct drm_crtc
> *crtc,
> writel_bits_relaxed(VPP_POSTBLEND_ENABLE, VPP_POSTBLEND_ENABLE,
> priv->io_base + _REG(VPP_MISC));
>
> + drm_crtc_vblank_on(crtc);
> +
> + meson_crtc->enabled = true;
> +}
> +
> +static void meson_crtc_atomic_enable(struct drm_crtc *crtc,
> + struct drm_crtc_state *old_state)
> +{
> + struct meson_crtc *meson_crtc = to_meson_crtc(crtc);
> + struct meson_drm *priv = meson_crtc->priv;
> +
> + DRM_DEBUG_DRIVER("\n");
> +
> + if (!meson_crtc->enabled)
> + meson_crtc_enable(crtc);
> +
> priv->viu.osd1_enabled = true;
> }
>
> @@ -117,6 +133,8 @@ static void meson_crtc_atomic_disable(struct drm_crtc
> *crtc,
>
> DRM_DEBUG_DRIVER("\n");
>
> + drm_crtc_vblank_off(crtc);
> +
> priv->viu.osd1_enabled = false;
> priv->viu.osd1_commit = false;
>
> @@ -135,6 +153,8 @@ static void meson_crtc_atomic_disable(struct drm_crtc
> *crtc,
>
> crtc->state->event = NULL;
> }
> +
> + meson_crtc->enabled = false;
> }
>
> static void meson_crtc_atomic_begin(struct drm_crtc *crtc,
> @@ -143,6 +163,9 @@ static void meson_crtc_atomic_begin(struct drm_crtc *crtc,
> struct meson_crtc *meson_crtc = to_meson_crtc(crtc);
> unsigned long flags;
>
> + if (crtc->state->enable && !meson_crtc->enabled)
> + meson_crtc_enable(crtc);
> +
> if (crtc->state->event) {
> WARN_ON(drm_crtc_vblank_get(crtc) != 0);
>
> diff --git a/drivers/gpu/drm/meson/meson_venc.c
> b/drivers/gpu/drm/meson/meson_venc.c
> index 9be0376e0329..ab72ddda242d 100644
> --- a/drivers/gpu/drm/meson/meson_venc.c
> +++ b/drivers/gpu/drm/meson/meson_venc.c
> @@ -71,6 +71,7 @@
> */
>
> /* HHI Registers */
> +#define HHI_GCLK_MPEG2 0x148 /* 0x52 offset in data sheet */
> #define HHI_VDAC_CNTL0 0x2F4 /* 0xbd offset in data sheet */
> #define HHI_VDAC_CNTL1 0x2F8 /* 0xbe offset in data sheet */
> #define HHI_HDMI_PHY_CNTL0 0x3a0 /* 0xe8 offset in data sheet */
> @@ -1663,10 +1664,12 @@ unsigned int meson_venci_get_field(struct meson_drm
> *priv)
> void meson_venc_enable_vsync(struct meson_drm *priv)
> {
> writel_relaxed(2, priv->io_base + _REG(VENC_INTCTRL));
> + regmap_update_bits(priv->hhi, HHI_GCLK_MPEG2, BIT(25), BIT(25));
> }
>
> void meson_venc_disable_vsync(struct meson_drm *priv)
> {
> + regmap_update_bits(priv->hhi, HHI_GCLK_MPEG2, BIT(25), 0);
> writel_relaxed(0, priv->io_base + _REG(VENC_INTCTRL));
> }
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
