| Message ID | 1436192424-18194-13-git-send-email-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
+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 >
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
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.
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;