[PATCH 1/4] drm/exynos: Remove exynos_plane_dpms() call with no effect

[PATCH 1/4] drm/exynos: Remove exynos_plane_dpms() call with no effect

[Joonyoung Shim]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

exynos_plane_dpms(DRM_MODE_DPMS_ON) calls the win_enable()'s callback
from the underlying layer. However neither one of these layers implement
win_enable() - FIMD, Mixer and VIDI. Thus the call to exynos_plane_dpms()
is pointless.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index a85c451..fff2e55 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -66,8 +66,6 @@ static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
 
 	if (exynos_crtc->ops->commit)
 		exynos_crtc->ops->commit(exynos_crtc);
-
-	exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_ON);
 }
 
 static bool
-- 
1.9.1

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

[PATCH 2/4] drm/exynos: remove mode property of exynos crtc

[Joonyoung Shim]

This was added by commit 3b8d1cf818c2 ("drm/exynos: add property for
crtc mode"). Currently we can control a plane used for crtc using
primary plane by universal plane feature. Stop to use non-standard
property to control primary plane.

Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 60 --------------------------------
 drivers/gpu/drm/exynos/exynos_drm_drv.h  |  8 -----
 2 files changed, 68 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index fff2e55..48ccab7 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -232,70 +232,12 @@ static void exynos_drm_crtc_destroy(struct drm_crtc *crtc)
 	kfree(exynos_crtc);
 }
 
-static int exynos_drm_crtc_set_property(struct drm_crtc *crtc,
-					struct drm_property *property,
-					uint64_t val)
-{
-	struct drm_device *dev = crtc->dev;
-	struct exynos_drm_private *dev_priv = dev->dev_private;
-	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
-
-	if (property == dev_priv->crtc_mode_property) {
-		enum exynos_crtc_mode mode = val;
-
-		if (mode == exynos_crtc->mode)
-			return 0;
-
-		exynos_crtc->mode = mode;
-
-		switch (mode) {
-		case CRTC_MODE_NORMAL:
-			exynos_drm_crtc_commit(crtc);
-			break;
-		case CRTC_MODE_BLANK:
-			exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_OFF);
-			break;
-		default:
-			break;
-		}
-
-		return 0;
-	}
-
-	return -EINVAL;
-}
-
 static struct drm_crtc_funcs exynos_crtc_funcs = {
 	.set_config	= drm_crtc_helper_set_config,
 	.page_flip	= exynos_drm_crtc_page_flip,
 	.destroy	= exynos_drm_crtc_destroy,
-	.set_property	= exynos_drm_crtc_set_property,
-};
-
-static const struct drm_prop_enum_list mode_names[] = {
-	{ CRTC_MODE_NORMAL, "normal" },
-	{ CRTC_MODE_BLANK, "blank" },
 };
 
-static void exynos_drm_crtc_attach_mode_property(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct exynos_drm_private *dev_priv = dev->dev_private;
-	struct drm_property *prop;
-
-	prop = dev_priv->crtc_mode_property;
-	if (!prop) {
-		prop = drm_property_create_enum(dev, 0, "mode", mode_names,
-						ARRAY_SIZE(mode_names));
-		if (!prop)
-			return;
-
-		dev_priv->crtc_mode_property = prop;
-	}
-
-	drm_object_attach_property(&crtc->base, prop, 0);
-}
-
 struct exynos_drm_crtc *exynos_drm_crtc_create(struct drm_device *drm_dev,
 					       int pipe,
 					       enum exynos_drm_output_type type,
@@ -338,8 +280,6 @@ struct exynos_drm_crtc *exynos_drm_crtc_create(struct drm_device *drm_dev,
 
 	drm_crtc_helper_add(crtc, &exynos_crtc_helper_funcs);
 
-	exynos_drm_crtc_attach_mode_property(crtc);
-
 	return exynos_crtc;
 
 err_crtc:
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index d490b49..1aceafc 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -197,11 +197,6 @@ struct exynos_drm_crtc_ops {
 	void (*te_handler)(struct exynos_drm_crtc *crtc);
 };
 
-enum exynos_crtc_mode {
-	CRTC_MODE_NORMAL,	/* normal mode */
-	CRTC_MODE_BLANK,	/* The private plane of crtc is blank */
-};
-
 /*
  * Exynos specific crtc structure.
  *
@@ -215,7 +210,6 @@ enum exynos_crtc_mode {
  *	we can refer to the crtc to current hardware interrupt occurred through
  *	this pipe value.
  * @dpms: store the crtc dpms value
- * @mode: store the crtc mode value
  * @ops: pointer to callbacks for exynos drm specific functionality
  * @ctx: A pointer to the crtc's implementation specific context
  */
@@ -224,7 +218,6 @@ struct exynos_drm_crtc {
 	enum exynos_drm_output_type	type;
 	unsigned int			pipe;
 	unsigned int			dpms;
-	enum exynos_crtc_mode		mode;
 	wait_queue_head_t		pending_flip_queue;
 	atomic_t			pending_flip;
 	struct exynos_drm_crtc_ops	*ops;
@@ -265,7 +258,6 @@ struct exynos_drm_private {
 	 */
 	struct drm_crtc *crtc[MAX_CRTC];
 	struct drm_property *plane_zpos_property;
-	struct drm_property *crtc_mode_property;
 
 	unsigned long da_start;
 	unsigned long da_space_size;
-- 
1.9.1

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

[PATCH 3/4] drm/exynos: remove exynos_plane_dpms

[Joonyoung Shim]

The exynos_plane_dpms function handles enabled flag of exynos plane and
calls internal hw driver callback function for hw overlay on/off. But
it causes state disharmory problem currently and is will be obstacle to
apply atomic operation later to keep non-standard per-plane dpms state
like enabled flag.

Let's remove enabled flag, it just stop to recall internal callback
function but hw drivers can handle it properly. And call internal
callback function directly then we can remove unnecessary
exynos_plane_dpms function

Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 33 ++++++-------------------------
 drivers/gpu/drm/exynos/exynos_drm_plane.h |  1 -
 2 files changed, 6 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index 358cff6..2dfb847 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -145,32 +145,6 @@ void exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
 		exynos_crtc->ops->win_mode_set(exynos_crtc, exynos_plane);
 }
 
-void exynos_plane_dpms(struct drm_plane *plane, int mode)
-{
-	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
-	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(plane->crtc);
-
-	if (mode == DRM_MODE_DPMS_ON) {
-		if (exynos_plane->enabled)
-			return;
-
-		if (exynos_crtc->ops->win_enable)
-			exynos_crtc->ops->win_enable(exynos_crtc,
-						     exynos_plane->zpos);
-
-		exynos_plane->enabled = true;
-	} else {
-		if (!exynos_plane->enabled)
-			return;
-
-		if (exynos_crtc->ops->win_disable)
-			exynos_crtc->ops->win_disable(exynos_crtc,
-						      exynos_plane->zpos);
-
-		exynos_plane->enabled = false;
-	}
-}
-
 int
 exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
@@ -199,7 +173,12 @@ exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 static int exynos_disable_plane(struct drm_plane *plane)
 {
-	exynos_plane_dpms(plane, DRM_MODE_DPMS_OFF);
+	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
+	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(plane->crtc);
+
+	if (exynos_crtc->ops->win_disable)
+		exynos_crtc->ops->win_disable(exynos_crtc,
+					      exynos_plane->zpos);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.h b/drivers/gpu/drm/exynos/exynos_drm_plane.h
index 59d4075..9d3c374 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.h
@@ -20,7 +20,6 @@ int exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 			unsigned int crtc_w, unsigned int crtc_h,
 			uint32_t src_x, uint32_t src_y,
 			uint32_t src_w, uint32_t src_h);
-void exynos_plane_dpms(struct drm_plane *plane, int mode);
 struct drm_plane *exynos_plane_init(struct drm_device *dev,
 				    unsigned long possible_crtcs,
 				    enum drm_plane_type type);
-- 
1.9.1

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

[PATCH 4/4] drm/exynos: fix NULL pointer reference

[Joonyoung Shim]

There is a case called disable_plane callback function even if
plane->crtc is NULL from exynos_drm_encoder_disable and it will cause
NULL pointer reference error.

Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_encoder.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
index 7e282e3..57de0bd 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
@@ -102,7 +102,7 @@ static void exynos_drm_encoder_disable(struct drm_encoder *encoder)
 
 	/* all planes connected to this encoder should be also disabled. */
 	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
-		if (plane->crtc == encoder->crtc)
+		if (plane->crtc && (plane->crtc == encoder->crtc))
 			plane->funcs->disable_plane(plane);
 	}
 }
-- 
1.9.1

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

Re: [PATCH 1/4] drm/exynos: Remove exynos_plane_dpms() call with no effect

[Daniel Vetter]

On Thu, Feb 05, 2015 at 04:11:35PM +0900, Joonyoung Shim wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> exynos_plane_dpms(DRM_MODE_DPMS_ON) calls the win_enable()'s callback
> from the underlying layer. However neither one of these layers implement
> win_enable() - FIMD, Mixer and VIDI. Thus the call to exynos_plane_dpms()
> is pointless.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

I guess you want to apply this patch after patch 2&3 for bisecting. But
makes sense otherwise. For the entire series:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index a85c451..fff2e55 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -66,8 +66,6 @@ static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
>  
>  	if (exynos_crtc->ops->commit)
>  		exynos_crtc->ops->commit(exynos_crtc);
> -
> -	exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_ON);
>  }
>  
>  static bool
> -- 
> 1.9.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/exynos: fix NULL pointer reference

[Gustavo Padovan]

Hi,


2015-02-05 Joonyoung Shim <jy0922.shim@samsung.com>:

> There is a case called disable_plane callback function even if
> plane->crtc is NULL from exynos_drm_encoder_disable and it will cause
> NULL pointer reference error.
> 
> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_encoder.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

You need to merge this patch with patch 3 or you will break the bisect.
The NULL pointer is introduced by the previous patch, so fix it in the same
patch.

Once that is fixed:

Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

for the whole series.

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

Re: [PATCH 4/4] drm/exynos: fix NULL pointer reference

[Joonyoung Shim]

Hi,

On 02/05/2015 10:07 PM, Gustavo Padovan wrote:
> Hi,
> 
> 
> 2015-02-05 Joonyoung Shim <jy0922.shim@samsung.com>:
> 
>> There is a case called disable_plane callback function even if
>> plane->crtc is NULL from exynos_drm_encoder_disable and it will cause
>> NULL pointer reference error.
>>
>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_encoder.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> You need to merge this patch with patch 3 or you will break the bisect.
> The NULL pointer is introduced by the previous patch, so fix it in the same
> patch.
> 

Yeah, it is revealed by patch 3 but it was original problem from before
patch 3. How about apply this patch earlier than patch 3?

> Once that is fixed:
> 
> Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 

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

Re: [PATCH 1/4] drm/exynos: Remove exynos_plane_dpms() call with no effect

[Joonyoung Shim]

Hi,

On 02/05/2015 06:40 PM, Daniel Vetter wrote:
> On Thu, Feb 05, 2015 at 04:11:35PM +0900, Joonyoung Shim wrote:
>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>
>> exynos_plane_dpms(DRM_MODE_DPMS_ON) calls the win_enable()'s callback
>> from the underlying layer. However neither one of these layers implement
>> win_enable() - FIMD, Mixer and VIDI. Thus the call to exynos_plane_dpms()
>> is pointless.
>>
>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> I guess you want to apply this patch after patch 2&3 for bisecting. But

Yeah, but i just applied this than modify original patch.

> makes sense otherwise. For the entire series:
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks.

>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> index a85c451..fff2e55 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> @@ -66,8 +66,6 @@ static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
>>  
>>  	if (exynos_crtc->ops->commit)
>>  		exynos_crtc->ops->commit(exynos_crtc);
>> -
>> -	exynos_plane_dpms(crtc->primary, DRM_MODE_DPMS_ON);
>>  }
>>  
>>  static bool
>> -- 
>> 1.9.1
>>
> 

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

Re: [PATCH 4/4] drm/exynos: fix NULL pointer reference

[Gustavo Padovan]

Hi,

2015-02-06 Joonyoung Shim <jy0922.shim@samsung.com>:

> Hi,
> 
> On 02/05/2015 10:07 PM, Gustavo Padovan wrote:
> > Hi,
> > 
> > 
> > 2015-02-05 Joonyoung Shim <jy0922.shim@samsung.com>:
> > 
> >> There is a case called disable_plane callback function even if
> >> plane->crtc is NULL from exynos_drm_encoder_disable and it will cause
> >> NULL pointer reference error.
> >>
> >> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> >> ---
> >>  drivers/gpu/drm/exynos/exynos_drm_encoder.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > You need to merge this patch with patch 3 or you will break the bisect.
> > The NULL pointer is introduced by the previous patch, so fix it in the same
> > patch.
> > 
> 
> Yeah, it is revealed by patch 3 but it was original problem from before
> patch 3. How about apply this patch earlier than patch 3?

Okay. That sounds good too.

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

Re: [PATCH 4/4] drm/exynos: fix NULL pointer reference

[Inki Dae]

On 2015년 02월 05일 16:11, Joonyoung Shim wrote:
> There is a case called disable_plane callback function even if
> plane->crtc is NULL from exynos_drm_encoder_disable and it will cause
> NULL pointer reference error.

Applied. (patch 1 though 4)

Thanks,
Inki Dae

> 
> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_encoder.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
> index 7e282e3..57de0bd 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
> @@ -102,7 +102,7 @@ static void exynos_drm_encoder_disable(struct drm_encoder *encoder)
>  
>  	/* all planes connected to this encoder should be also disabled. */
>  	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
> -		if (plane->crtc == encoder->crtc)
> +		if (plane->crtc && (plane->crtc == encoder->crtc))
>  			plane->funcs->disable_plane(plane);
>  	}
>  }
> 

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