[v2,12/23] drm/exynos: don't track enabled state at exynos_crtc
diff mbox

Message ID 1436192424-18194-13-git-send-email-gustavo@padovan.org
State New
Headers show

Commit Message

Gustavo Padovan July 6, 2015, 2:20 p.m. UTC
From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

struct drm_crtc already stores the enabled state of the crtc
thus we don't need to replicate enabled in exynos_drm_crtc.

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

Comments

Daniel Vetter July 6, 2015, 5:41 p.m. UTC | #1
On Mon, Jul 06, 2015 at 11:20:13AM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> struct drm_crtc already stores the enabled state of the crtc
> thus we don't need to replicate enabled in exynos_drm_crtc.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Note that exynos_crtc->enabled doesn't match drm_crtc->enabled perfectly
since the exynos one reflect hw state (including dpms). You want to look
at crtc->state->active instead on functions enabling stuff, and
old_crtc_state->active on functions disabling stuff (we don't wire that
through everywhere yet).
-Daniel

> ---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 16 ----------------
>  drivers/gpu/drm/exynos/exynos_drm_drv.h  |  1 -
>  2 files changed, 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index 9bc2353..5ab8972 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -26,14 +26,9 @@ static void exynos_drm_crtc_enable(struct drm_crtc *crtc)
>  {
>  	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>  
> -	if (exynos_crtc->enabled)
> -		return;
> -
>  	if (exynos_crtc->ops->enable)
>  		exynos_crtc->ops->enable(exynos_crtc);
>  
> -	exynos_crtc->enabled = true;
> -
>  	drm_crtc_vblank_on(crtc);
>  }
>  
> @@ -41,9 +36,6 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
>  {
>  	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>  
> -	if (!exynos_crtc->enabled)
> -		return;
> -
>  	/* wait for the completion of page flip. */
>  	if (!wait_event_timeout(exynos_crtc->pending_flip_queue,
>  				(exynos_crtc->event == NULL), HZ/20))
> @@ -53,8 +45,6 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
>  
>  	if (exynos_crtc->ops->disable)
>  		exynos_crtc->ops->disable(exynos_crtc);
> -
> -	exynos_crtc->enabled = false;
>  }
>  
>  static bool
> @@ -171,9 +161,6 @@ int exynos_drm_crtc_enable_vblank(struct drm_device *dev, int pipe)
>  	struct exynos_drm_crtc *exynos_crtc =
>  		to_exynos_crtc(private->crtc[pipe]);
>  
> -	if (!exynos_crtc->enabled)
> -		return -EPERM;
> -
>  	if (exynos_crtc->ops->enable_vblank)
>  		return exynos_crtc->ops->enable_vblank(exynos_crtc);
>  
> @@ -186,9 +173,6 @@ void exynos_drm_crtc_disable_vblank(struct drm_device *dev, int pipe)
>  	struct exynos_drm_crtc *exynos_crtc =
>  		to_exynos_crtc(private->crtc[pipe]);
>  
> -	if (!exynos_crtc->enabled)
> -		return;
> -
>  	if (exynos_crtc->ops->disable_vblank)
>  		exynos_crtc->ops->disable_vblank(exynos_crtc);
>  }
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index 5bd1d3c..d3a8f0a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -185,7 +185,6 @@ struct exynos_drm_crtc {
>  	struct drm_crtc			base;
>  	enum exynos_drm_output_type	type;
>  	unsigned int			pipe;
> -	bool				enabled;
>  	wait_queue_head_t		pending_flip_queue;
>  	struct drm_pending_vblank_event	*event;
>  	const struct exynos_drm_crtc_ops	*ops;
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Joonyoung Shim July 9, 2015, 8:09 a.m. UTC | #2
+Cc Andrzej,

On 07/07/2015 02:41 AM, Daniel Vetter wrote:
> On Mon, Jul 06, 2015 at 11:20:13AM -0300, Gustavo Padovan wrote:
>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>
>> struct drm_crtc already stores the enabled state of the crtc
>> thus we don't need to replicate enabled in exynos_drm_crtc.
>>

I think exynos_crtc->enabled can replace flags for power state of each
hw driver(e.g. "powered" of mixer driver, "suspended" of fimd driver).
Further, we can add other flag bit for instead of using special flag
in hw driver like vsync state("int_en" of mixer driver, "irq_flags" of
fimd driver.)

>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Note that exynos_crtc->enabled doesn't match drm_crtc->enabled perfectly
> since the exynos one reflect hw state (including dpms). You want to look
> at crtc->state->active instead on functions enabling stuff, and
> old_crtc_state->active on functions disabling stuff (we don't wire that
> through everywhere yet).
> -Daniel
> 
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 16 ----------------
>>  drivers/gpu/drm/exynos/exynos_drm_drv.h  |  1 -
>>  2 files changed, 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> index 9bc2353..5ab8972 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> @@ -26,14 +26,9 @@ static void exynos_drm_crtc_enable(struct drm_crtc *crtc)
>>  {
>>  	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>>  
>> -	if (exynos_crtc->enabled)
>> -		return;
>> -
>>  	if (exynos_crtc->ops->enable)
>>  		exynos_crtc->ops->enable(exynos_crtc);
>>  
>> -	exynos_crtc->enabled = true;
>> -
>>  	drm_crtc_vblank_on(crtc);
>>  }
>>  
>> @@ -41,9 +36,6 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
>>  {
>>  	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
>>  
>> -	if (!exynos_crtc->enabled)
>> -		return;
>> -
>>  	/* wait for the completion of page flip. */
>>  	if (!wait_event_timeout(exynos_crtc->pending_flip_queue,
>>  				(exynos_crtc->event == NULL), HZ/20))
>> @@ -53,8 +45,6 @@ static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
>>  
>>  	if (exynos_crtc->ops->disable)
>>  		exynos_crtc->ops->disable(exynos_crtc);
>> -
>> -	exynos_crtc->enabled = false;
>>  }
>>  
>>  static bool
>> @@ -171,9 +161,6 @@ int exynos_drm_crtc_enable_vblank(struct drm_device *dev, int pipe)
>>  	struct exynos_drm_crtc *exynos_crtc =
>>  		to_exynos_crtc(private->crtc[pipe]);
>>  
>> -	if (!exynos_crtc->enabled)
>> -		return -EPERM;
>> -
>>  	if (exynos_crtc->ops->enable_vblank)
>>  		return exynos_crtc->ops->enable_vblank(exynos_crtc);
>>  
>> @@ -186,9 +173,6 @@ void exynos_drm_crtc_disable_vblank(struct drm_device *dev, int pipe)
>>  	struct exynos_drm_crtc *exynos_crtc =
>>  		to_exynos_crtc(private->crtc[pipe]);
>>  
>> -	if (!exynos_crtc->enabled)
>> -		return;
>> -
>>  	if (exynos_crtc->ops->disable_vblank)
>>  		exynos_crtc->ops->disable_vblank(exynos_crtc);
>>  }
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> index 5bd1d3c..d3a8f0a 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> @@ -185,7 +185,6 @@ struct exynos_drm_crtc {
>>  	struct drm_crtc			base;
>>  	enum exynos_drm_output_type	type;
>>  	unsigned int			pipe;
>> -	bool				enabled;
>>  	wait_queue_head_t		pending_flip_queue;
>>  	struct drm_pending_vblank_event	*event;
>>  	const struct exynos_drm_crtc_ops	*ops;
>> -- 
>> 2.1.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Gustavo Padovan July 9, 2015, 10:56 p.m. UTC | #3
2015-07-09 Joonyoung Shim <jy0922.shim@samsung.com>:

> +Cc Andrzej,
> 
> On 07/07/2015 02:41 AM, Daniel Vetter wrote:
> > On Mon, Jul 06, 2015 at 11:20:13AM -0300, Gustavo Padovan wrote:
> >> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >>
> >> struct drm_crtc already stores the enabled state of the crtc
> >> thus we don't need to replicate enabled in exynos_drm_crtc.
> >>
> 
> I think exynos_crtc->enabled can replace flags for power state of each
> hw driver(e.g. "powered" of mixer driver, "suspended" of fimd driver).
> Further, we can add other flag bit for instead of using special flag
> in hw driver like vsync state("int_en" of mixer driver, "irq_flags" of
> fimd driver.)

The reason I removed it is because crtc->state->active already stores
the same information as exynos_crtc->enabled. I think we could rely
on active as well for mixer powered and suspended.

	Gustavo
Joonyoung Shim July 10, 2015, 12:47 a.m. UTC | #4
On 07/10/2015 07:56 AM, Gustavo Padovan wrote:
> 2015-07-09 Joonyoung Shim <jy0922.shim@samsung.com>:
> 
>> +Cc Andrzej,
>>
>> On 07/07/2015 02:41 AM, Daniel Vetter wrote:
>>> On Mon, Jul 06, 2015 at 11:20:13AM -0300, Gustavo Padovan wrote:
>>>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>>>
>>>> struct drm_crtc already stores the enabled state of the crtc
>>>> thus we don't need to replicate enabled in exynos_drm_crtc.
>>>>
>>
>> I think exynos_crtc->enabled can replace flags for power state of each
>> hw driver(e.g. "powered" of mixer driver, "suspended" of fimd driver).
>> Further, we can add other flag bit for instead of using special flag
>> in hw driver like vsync state("int_en" of mixer driver, "irq_flags" of
>> fimd driver.)
> 
> The reason I removed it is because crtc->state->active already stores
> the same information as exynos_crtc->enabled. I think we could rely
> on active as well for mixer powered and suspended.
> 

Then it may need another stuff on struct exynos_crtc if we try to
replace other hw flag like int_en or irq_flags.

Anyway, the reason that you remove it is ok to me now.

Thanks.

Patch
diff mbox

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 9bc2353..5ab8972 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -26,14 +26,9 @@  static void exynos_drm_crtc_enable(struct drm_crtc *crtc)
 {
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
 
-	if (exynos_crtc->enabled)
-		return;
-
 	if (exynos_crtc->ops->enable)
 		exynos_crtc->ops->enable(exynos_crtc);
 
-	exynos_crtc->enabled = true;
-
 	drm_crtc_vblank_on(crtc);
 }
 
@@ -41,9 +36,6 @@  static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
 {
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
 
-	if (!exynos_crtc->enabled)
-		return;
-
 	/* wait for the completion of page flip. */
 	if (!wait_event_timeout(exynos_crtc->pending_flip_queue,
 				(exynos_crtc->event == NULL), HZ/20))
@@ -53,8 +45,6 @@  static void exynos_drm_crtc_disable(struct drm_crtc *crtc)
 
 	if (exynos_crtc->ops->disable)
 		exynos_crtc->ops->disable(exynos_crtc);
-
-	exynos_crtc->enabled = false;
 }
 
 static bool
@@ -171,9 +161,6 @@  int exynos_drm_crtc_enable_vblank(struct drm_device *dev, int pipe)
 	struct exynos_drm_crtc *exynos_crtc =
 		to_exynos_crtc(private->crtc[pipe]);
 
-	if (!exynos_crtc->enabled)
-		return -EPERM;
-
 	if (exynos_crtc->ops->enable_vblank)
 		return exynos_crtc->ops->enable_vblank(exynos_crtc);
 
@@ -186,9 +173,6 @@  void exynos_drm_crtc_disable_vblank(struct drm_device *dev, int pipe)
 	struct exynos_drm_crtc *exynos_crtc =
 		to_exynos_crtc(private->crtc[pipe]);
 
-	if (!exynos_crtc->enabled)
-		return;
-
 	if (exynos_crtc->ops->disable_vblank)
 		exynos_crtc->ops->disable_vblank(exynos_crtc);
 }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 5bd1d3c..d3a8f0a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -185,7 +185,6 @@  struct exynos_drm_crtc {
 	struct drm_crtc			base;
 	enum exynos_drm_output_type	type;
 	unsigned int			pipe;
-	bool				enabled;
 	wait_queue_head_t		pending_flip_queue;
 	struct drm_pending_vblank_event	*event;
 	const struct exynos_drm_crtc_ops	*ops;