Linux-Amlogic Archive on lore.kernel.org
 help / Atom feed
* [PATCH] drm/meson: Fix atomic mode switching regression
@ 2019-01-09 13:36 Neil Armstrong
  2019-01-09 22:18 ` Martin Blumenstingl
  0 siblings, 1 reply; 3+ messages in thread
From: Neil Armstrong @ 2019-01-09 13:36 UTC (permalink / raw)
  To: daniel; +Cc: linux-amlogic, linux-kernel, dri-devel, 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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] drm/meson: Fix atomic mode switching regression
  2019-01-09 13:36 [PATCH] drm/meson: Fix atomic mode switching regression Neil Armstrong
@ 2019-01-09 22:18 ` Martin Blumenstingl
  2019-01-14 15:25   ` Neil Armstrong
  0 siblings, 1 reply; 3+ messages in thread
From: Martin Blumenstingl @ 2019-01-09 22:18 UTC (permalink / raw)
  To: Neil Armstrong; +Cc: linux-amlogic, dri-devel, linux-kernel, daniel

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] drm/meson: Fix atomic mode switching regression
  2019-01-09 22:18 ` Martin Blumenstingl
@ 2019-01-14 15:25   ` Neil Armstrong
  0 siblings, 0 replies; 3+ messages in thread
From: Neil Armstrong @ 2019-01-14 15:25 UTC (permalink / raw)
  To: Martin Blumenstingl; +Cc: linux-amlogic, dri-devel, linux-kernel, daniel

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-09 13:36 [PATCH] drm/meson: Fix atomic mode switching regression Neil Armstrong
2019-01-09 22:18 ` Martin Blumenstingl
2019-01-14 15:25   ` Neil Armstrong

Linux-Amlogic Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-amlogic/0 linux-amlogic/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-amlogic linux-amlogic/ https://lore.kernel.org/linux-amlogic \
		linux-amlogic@lists.infradead.org linux-amlogic@archiver.kernel.org
	public-inbox-index linux-amlogic


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-amlogic


AGPL code for this site: git clone https://public-inbox.org/ public-inbox