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

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

[Neil Armstrong]

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 */
 	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);
 
 	priv->viu.osd1_enabled = true;
 }
-- 
2.19.2


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Martin Blumenstingl]

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()


Regards
Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Neil Armstrong]

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
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic