Re: [PATCH] drm/meson: Fix atomic mode switching regression

Hi Martin,

On 09/01/2019 23:18, Martin Blumenstingl wrote:

Hi Neil,

On Wed, Jan 9, 2019 at 2:36 PM Neil Armstrong narmstrong@baylibre.com wrote:

...

Since commit 2bcd3ecab773 when switching mode from X11 (ubuntu mate for example) the display gets blurry, looking like an invalid framebuffer width.

This commit fixed atomic crtc modesetting but didn't update the display parameters when changing mode, but only when starting a mode setting after a crtc disable.

This commit setups the crctc parameter in _begin() and _enable() to take in account the current ctrc parameters.

Reported-by: Tony McKahan tonymckahan@gmail.com Fixes: 2bcd3ecab773 ("drm/meson: Fixes for drm_crtc_vblank_on/off support") Signed-off-by: Neil Armstrong narmstrong@baylibre.com

---

drivers/gpu/drm/meson/meson_crtc.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_crtc.c b/drivers/gpu/drm/meson/meson_crtc.c index 75d97f1b2e8f..5bb432021caf 100644 --- a/drivers/gpu/drm/meson/meson_crtc.c +++ b/drivers/gpu/drm/meson/meson_crtc.c @@ -82,14 +82,12 @@ static const struct drm_crtc_funcs meson_crtc_funcs = {

};

-static void meson_crtc_enable(struct drm_crtc *crtc) +static void meson_crtc_setup(struct drm_crtc *crtc) { struct meson_crtc *meson_crtc = to_meson_crtc(crtc); struct drm_crtc_state *crtc_state = crtc->state; struct meson_drm *priv = meson_crtc->priv;

•   DRM_DEBUG_DRIVER("\n");
  

•   if (!crtc_state) {
            DRM_ERROR("Invalid crtc_state\n");
            return;
  

@@ -98,6 +96,16 @@ static void meson_crtc_enable(struct drm_crtc *crtc) /* Enable VPP Postblend */

nit-pick: this "enable" comment is now in meson_crtc_setup(). I would drop it because my interpretation of the following lines is now "setting VPP_POSTBLEND_H_SIZE enables the VPP postblend"

...

    writel(crtc_state->mode.hdisplay,
           priv->io_base + _REG(VPP_POSTBLEND_H_SIZE));

+}

+static void meson_crtc_enable(struct drm_crtc *crtc) +{

•   struct meson_crtc *meson_crtc = to_meson_crtc(crtc);
  

•   struct meson_drm *priv = meson_crtc->priv;
  

•   DRM_DEBUG_DRIVER("\n");
  

•   meson_crtc_setup(crtc);
  
    /* VD1 Preblend vertical start/end */
    writel(FIELD_PREP(GENMASK(11, 0), 2303),
  

@@ -121,6 +129,8 @@ static void meson_crtc_atomic_enable(struct drm_crtc *crtc,

    if (!meson_crtc->enabled)
            meson_crtc_enable(crtc);

•   else
  

•           meson_crtc_setup(crtc);
  

it's probably only personal preference, but have you thought about re-ordering this: meson_crtc_setup(crtc);

if (!meson_crtc->enabled) meson_crtc_enable(crtc);

with that you could get rid of the meson_crtc_setup() call in meson_crtc_enable(), leaving only one code-path (instead of two) which calls meson_crtc_setup()

All of this is wrong, I only needed to use `atomic_commit_tail = drm_atomic_helper_commit_tail_rpm` in meson_drv for the fbdev.

This fix is wrong, pushing a right one.

Neil

Regards Martin