[PATCH] drm/exynos: fixed a issue that plane isn't disabled when released

[PATCH] drm/exynos: fixed a issue that plane isn't disabled when released

[Inki Dae]

when drm is released, drm framebuffers are released and all crtcs using
same framebuffer and also all gem buffers used but plane isn't disabled
so when crtc and encoder are turned on, overlay can access to invalid memory
because plane still has memory address released already.
this patch makes sure that each plane is disabled when released.

Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_encoder.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
index 92f9acf..96a10c3 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
@@ -214,12 +214,27 @@ static void exynos_drm_encoder_commit(struct drm_encoder *encoder)
 		manager_ops->commit(manager->dev);
 }
 
+static void exynos_drm_encoder_disable(struct drm_encoder *encoder)
+{
+	struct drm_plane *plane;
+	struct drm_device *dev = encoder->dev;
+
+	exynos_drm_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
+
+	/* all planes connected to this encoder should be also disabled. */
+	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
+		if (plane->crtc == encoder->crtc)
+			plane->funcs->disable_plane(plane);
+	}
+}
+
 static struct drm_encoder_helper_funcs exynos_encoder_helper_funcs = {
 	.dpms		= exynos_drm_encoder_dpms,
 	.mode_fixup	= exynos_drm_encoder_mode_fixup,
 	.mode_set	= exynos_drm_encoder_mode_set,
 	.prepare	= exynos_drm_encoder_prepare,
 	.commit		= exynos_drm_encoder_commit,
+	.disable	= exynos_drm_encoder_disable,
 };
 
 static void exynos_drm_encoder_destroy(struct drm_encoder *encoder)
-- 
1.7.4.1

Re: [PATCH] drm/exynos: fixed a issue that plane isn't disabled when released

[Paul Menzel]

[-- Attachment #1.1: Type: text/plain, Size: 2686 bytes --]

Dear Inki,


Am Freitag, den 24.08.2012, 18:27 +0900 schrieb Inki Dae:

You can shorten the commit summary by leaving out the word issue, which
is redundant. Maybe one of the following? I do not understand the
process of releasing so it might be wrong.

        drm/exynos: Disable plane after release
        drm/exynos: Disable plane when being released
        drm/exynos: Fix disabling of plane when released

> when drm is released, drm framebuffers are released and all crtcs using
> same framebuffer and also all gem buffers used but plane isn't disabled
> so when crtc and encoder are turned on, overlay can access to invalid memory
> because plane still has memory address released already.
> this patch makes sure that each plane is disabled when released.

Please use sentences to make it easier to understand.

Also describe the solution. Maybe something like this.

        This patch ensures that each plane is disabled when released, by
        adding a new function and adding that to the helper functions.

Is there a report for that issue? Did you experience it in your setup?
Is this tested?

> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_encoder.c |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
> index 92f9acf..96a10c3 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
> @@ -214,12 +214,27 @@ static void exynos_drm_encoder_commit(struct drm_encoder *encoder)
>  		manager_ops->commit(manager->dev);
>  }
>  
> +static void exynos_drm_encoder_disable(struct drm_encoder *encoder)
> +{
> +	struct drm_plane *plane;
> +	struct drm_device *dev = encoder->dev;
> +
> +	exynos_drm_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
> +
> +	/* all planes connected to this encoder should be also disabled. */
> +	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> +		if (plane->crtc == encoder->crtc)
> +			plane->funcs->disable_plane(plane);
> +	}
> +}
> +
>  static struct drm_encoder_helper_funcs exynos_encoder_helper_funcs = {
>  	.dpms		= exynos_drm_encoder_dpms,
>  	.mode_fixup	= exynos_drm_encoder_mode_fixup,
>  	.mode_set	= exynos_drm_encoder_mode_set,
>  	.prepare	= exynos_drm_encoder_prepare,
>  	.commit		= exynos_drm_encoder_commit,
> +	.disable	= exynos_drm_encoder_disable,
>  };
>  
>  static void exynos_drm_encoder_destroy(struct drm_encoder *encoder)


Thanks,

Paul

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: fixed a issue that plane isn't disabled when released

[InKi Dae]

2012/8/24 Paul Menzel <paulepanter@users.sourceforge.net>:
> Dear Inki,
>
>
> Am Freitag, den 24.08.2012, 18:27 +0900 schrieb Inki Dae:
>
> You can shorten the commit summary by leaving out the word issue, which
> is redundant. Maybe one of the following? I do not understand the
> process of releasing so it might be wrong.
>
>         drm/exynos: Disable plane after release
>         drm/exynos: Disable plane when being released
>         drm/exynos: Fix disabling of plane when released
>
>> when drm is released, drm framebuffers are released and all crtcs using
>> same framebuffer and also all gem buffers used but plane isn't disabled
>> so when crtc and encoder are turned on, overlay can access to invalid memory
>> because plane still has memory address released already.
>> this patch makes sure that each plane is disabled when released.
>
> Please use sentences to make it easier to understand.
>
> Also describe the solution. Maybe something like this.
>
>         This patch ensures that each plane is disabled when released, by
>         adding a new function and adding that to the helper functions.
>
> Is there a report for that issue? Did you experience it in your setup?
> Is this tested?
>

Thank you for your advice. acutally, I missed some sentences for
describing this patch so I will send it again.

Thanks,
Inki Dae

>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_encoder.c |   15 +++++++++++++++
>>  1 files changed, 15 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
>> index 92f9acf..96a10c3 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
>> @@ -214,12 +214,27 @@ static void exynos_drm_encoder_commit(struct drm_encoder *encoder)
>>               manager_ops->commit(manager->dev);
>>  }
>>
>> +static void exynos_drm_encoder_disable(struct drm_encoder *encoder)
>> +{
>> +     struct drm_plane *plane;
>> +     struct drm_device *dev = encoder->dev;
>> +
>> +     exynos_drm_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
>> +
>> +     /* all planes connected to this encoder should be also disabled. */
>> +     list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
>> +             if (plane->crtc == encoder->crtc)
>> +                     plane->funcs->disable_plane(plane);
>> +     }
>> +}
>> +
>>  static struct drm_encoder_helper_funcs exynos_encoder_helper_funcs = {
>>       .dpms           = exynos_drm_encoder_dpms,
>>       .mode_fixup     = exynos_drm_encoder_mode_fixup,
>>       .mode_set       = exynos_drm_encoder_mode_set,
>>       .prepare        = exynos_drm_encoder_prepare,
>>       .commit         = exynos_drm_encoder_commit,
>> +     .disable        = exynos_drm_encoder_disable,
>>  };
>>
>>  static void exynos_drm_encoder_destroy(struct drm_encoder *encoder)
>
>
> Thanks,
>
> Paul
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[PATCH] drm/exynos: Disable plane when released

[Inki Dae]

From: Inki Dae <inki.dae@samsung.com>

this patch ensures that each plane connected to encoder is disabled
when released, by adding disable callback function of encoder helper

we had faced with one issue that invalid memory is accessed by dma
once drm is released and then the dma is turned on again. actually,
in our case, page fault was incurred with iommu. the reason is that
a gem buffer accessed by the dma is also released once drm is released.

so this patch would fix this issue ensuring the dma is disabled
when released.

Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_encoder.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
index 92f9acf..96a10c3 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
@@ -214,12 +214,27 @@ static void exynos_drm_encoder_commit(struct drm_encoder *encoder)
 		manager_ops->commit(manager->dev);
 }
 
+static void exynos_drm_encoder_disable(struct drm_encoder *encoder)
+{
+	struct drm_plane *plane;
+	struct drm_device *dev = encoder->dev;
+
+	exynos_drm_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
+
+	/* all planes connected to this encoder should be also disabled. */
+	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
+		if (plane->crtc == encoder->crtc)
+			plane->funcs->disable_plane(plane);
+	}
+}
+
 static struct drm_encoder_helper_funcs exynos_encoder_helper_funcs = {
 	.dpms		= exynos_drm_encoder_dpms,
 	.mode_fixup	= exynos_drm_encoder_mode_fixup,
 	.mode_set	= exynos_drm_encoder_mode_set,
 	.prepare	= exynos_drm_encoder_prepare,
 	.commit		= exynos_drm_encoder_commit,
+	.disable	= exynos_drm_encoder_disable,
 };
 
 static void exynos_drm_encoder_destroy(struct drm_encoder *encoder)
-- 
1.7.5.4