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 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
>> Fixes: 2bcd3ecab773 ("drm/meson: Fixes for drm_crtc_vblank_on/off support")
>> Signed-off-by: Neil Armstrong
>> ---
>> 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
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/meson: Fix atomic mode switching regression
Hi Neil,
On Wed, Jan 9, 2019 at 2:36 PM Neil Armstrong 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
> Fixes: 2bcd3ecab773 ("drm/meson: Fixes for drm_crtc_vblank_on/off support")
> Signed-off-by: Neil Armstrong
> ---
> 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/meson: Fix atomic mode switching regression
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
Fixes: 2bcd3ecab773 ("drm/meson: Fixes for drm_crtc_vblank_on/off support")
Signed-off-by: Neil Armstrong
---
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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
