diff mbox

[v3,12/32] drm/exynos: Split manager/display/subdrv

Message ID 1383063198-10526-13-git-send-email-seanpaul@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Sean Paul Oct. 29, 2013, 4:12 p.m. UTC
This patch splits display and manager from subdrv. The result is that
crtc functions can directly call into manager callbacks and encoder
functions can directly call into display callbacks. This will allow
us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
with common code.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---

Changes in v2:
	- Pass display into display_ops instead of context
Changes in v3:
	- Changed vidi args to exynos_drm_display instead of void
	- Moved exynos_drm_hdmi.c removal into next patch

 drivers/gpu/drm/exynos/exynos_drm_connector.c |  50 ++---
 drivers/gpu/drm/exynos/exynos_drm_core.c      | 181 +++++++++++++-----
 drivers/gpu/drm/exynos/exynos_drm_crtc.c      | 115 +++++++++---
 drivers/gpu/drm/exynos/exynos_drm_crtc.h      |  20 +-
 drivers/gpu/drm/exynos/exynos_drm_drv.c       |  29 +--
 drivers/gpu/drm/exynos/exynos_drm_drv.h       | 106 +++++++----
 drivers/gpu/drm/exynos/exynos_drm_encoder.c   | 258 ++++----------------------
 drivers/gpu/drm/exynos/exynos_drm_encoder.h   |  18 +-
 drivers/gpu/drm/exynos/exynos_drm_fb.c        |   4 +-
 drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 161 ++++++++--------
 drivers/gpu/drm/exynos/exynos_drm_hdmi.c      | 211 +++++++++------------
 drivers/gpu/drm/exynos/exynos_drm_hdmi.h      |   2 +
 drivers/gpu/drm/exynos/exynos_drm_plane.c     |  15 +-
 drivers/gpu/drm/exynos/exynos_drm_vidi.c      | 129 ++++++-------
 14 files changed, 615 insertions(+), 684 deletions(-)

Comments

Inki Dae Oct. 31, 2013, 10:30 a.m. UTC | #1
> -----Original Message-----
> From: Sean Paul [mailto:seanpaul@chromium.org]
> Sent: Wednesday, October 30, 2013 1:13 AM
> To: dri-devel@lists.freedesktop.org; inki.dae@samsung.com
> Cc: airlied@linux.ie; tomasz.figa@gmail.com; marcheu@chromium.org; Sean
> Paul
> Subject: [PATCH v3 12/32] drm/exynos: Split manager/display/subdrv
> 
> This patch splits display and manager from subdrv. The result is that
> crtc functions can directly call into manager callbacks and encoder
> functions can directly call into display callbacks. This will allow
> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
> with common code.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> 
> Changes in v2:
> 	- Pass display into display_ops instead of context
> Changes in v3:
> 	- Changed vidi args to exynos_drm_display instead of void
> 	- Moved exynos_drm_hdmi.c removal into next patch
> 
>  drivers/gpu/drm/exynos/exynos_drm_connector.c |  50 ++---
>  drivers/gpu/drm/exynos/exynos_drm_core.c      | 181 +++++++++++++-----
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c      | 115 +++++++++---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.h      |  20 +-
>  drivers/gpu/drm/exynos/exynos_drm_drv.c       |  29 +--
>  drivers/gpu/drm/exynos/exynos_drm_drv.h       | 106 +++++++----
>  drivers/gpu/drm/exynos/exynos_drm_encoder.c   | 258
++++------------------
> ----
>  drivers/gpu/drm/exynos/exynos_drm_encoder.h   |  18 +-
>  drivers/gpu/drm/exynos/exynos_drm_fb.c        |   4 +-
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 161 ++++++++--------
>  drivers/gpu/drm/exynos/exynos_drm_hdmi.c      | 211 +++++++++------------
>  drivers/gpu/drm/exynos/exynos_drm_hdmi.h      |   2 +
>  drivers/gpu/drm/exynos/exynos_drm_plane.c     |  15 +-
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c      | 129 ++++++-------
>  14 files changed, 615 insertions(+), 684 deletions(-)
> 
>  static int exynos_drm_create_enc_conn(struct drm_device *dev,
> -					struct exynos_drm_subdrv *subdrv)
> +					struct exynos_drm_display *display)
>  {
>  	struct drm_encoder *encoder;
>  	struct drm_connector *connector;
> +	struct exynos_drm_manager *manager;
>  	int ret;
> +	unsigned long possible_crtcs = 0;
> 
> -	subdrv->manager->dev = subdrv->dev;
> +	/* Find possible crtcs for this display */
> +	list_for_each_entry(manager, &exynos_drm_manager_list, list)
> +		if (manager->type == display->type)
> +			possible_crtcs |= 1 << manager->pipe;
> 
>  	/* create and initialize a encoder for this sub driver. */
> -	encoder = exynos_drm_encoder_create(dev, subdrv->manager,
> -			(1 << MAX_CRTC) - 1);
> +	encoder = exynos_drm_encoder_create(dev, display, possible_crtcs);
>  	if (!encoder) {
>  		DRM_ERROR("failed to create encoder\n");
>  		return -EFAULT;
>  	}
> -	subdrv->encoder = encoder;
> +	display->encoder = encoder;
> 
> -	if (subdrv->manager->display_ops->type == EXYNOS_DISPLAY_TYPE_LCD)
> {
> +	if (display->type == EXYNOS_DISPLAY_TYPE_LCD) {
>  		ret = exynos_drm_attach_lcd_bridge(dev, encoder);

The lvds bridge related codes need more reviews so I'll merge this patch
manually excepting the lvds bridge related codes. Let's discuss lvds bridge
later.

Thanks,
Inki Dae

>  		if (ret)
>  			return 0;
> @@ -91,7 +98,7 @@ static int exynos_drm_create_enc_conn(struct drm_device
> *dev,
>  		goto err_destroy_encoder;
>  	}
> 
> -	subdrv->connector = connector;
> +	display->connector = connector;
> 
>  	return 0;
Sean Paul Oct. 31, 2013, 4:08 p.m. UTC | #2
On Thu, Oct 31, 2013 at 6:30 AM, Inki Dae <inki.dae@samsung.com> wrote:
>
>
>> -----Original Message-----
>> From: Sean Paul [mailto:seanpaul@chromium.org]
>> Sent: Wednesday, October 30, 2013 1:13 AM
>> To: dri-devel@lists.freedesktop.org; inki.dae@samsung.com
>> Cc: airlied@linux.ie; tomasz.figa@gmail.com; marcheu@chromium.org; Sean
>> Paul
>> Subject: [PATCH v3 12/32] drm/exynos: Split manager/display/subdrv
>>
>> This patch splits display and manager from subdrv. The result is that
>> crtc functions can directly call into manager callbacks and encoder
>> functions can directly call into display callbacks. This will allow
>> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
>> with common code.
>>
>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> ---
>>
>> Changes in v2:
>>       - Pass display into display_ops instead of context
>> Changes in v3:
>>       - Changed vidi args to exynos_drm_display instead of void
>>       - Moved exynos_drm_hdmi.c removal into next patch
>>
>>  drivers/gpu/drm/exynos/exynos_drm_connector.c |  50 ++---
>>  drivers/gpu/drm/exynos/exynos_drm_core.c      | 181 +++++++++++++-----
>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c      | 115 +++++++++---
>>  drivers/gpu/drm/exynos/exynos_drm_crtc.h      |  20 +-
>>  drivers/gpu/drm/exynos/exynos_drm_drv.c       |  29 +--
>>  drivers/gpu/drm/exynos/exynos_drm_drv.h       | 106 +++++++----
>>  drivers/gpu/drm/exynos/exynos_drm_encoder.c   | 258
> ++++------------------
>> ----
>>  drivers/gpu/drm/exynos/exynos_drm_encoder.h   |  18 +-
>>  drivers/gpu/drm/exynos/exynos_drm_fb.c        |   4 +-
>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 161 ++++++++--------
>>  drivers/gpu/drm/exynos/exynos_drm_hdmi.c      | 211 +++++++++------------
>>  drivers/gpu/drm/exynos/exynos_drm_hdmi.h      |   2 +
>>  drivers/gpu/drm/exynos/exynos_drm_plane.c     |  15 +-
>>  drivers/gpu/drm/exynos/exynos_drm_vidi.c      | 129 ++++++-------
>>  14 files changed, 615 insertions(+), 684 deletions(-)
>>
>>  static int exynos_drm_create_enc_conn(struct drm_device *dev,
>> -                                     struct exynos_drm_subdrv *subdrv)
>> +                                     struct exynos_drm_display *display)
>>  {
>>       struct drm_encoder *encoder;
>>       struct drm_connector *connector;
>> +     struct exynos_drm_manager *manager;
>>       int ret;
>> +     unsigned long possible_crtcs = 0;
>>
>> -     subdrv->manager->dev = subdrv->dev;
>> +     /* Find possible crtcs for this display */
>> +     list_for_each_entry(manager, &exynos_drm_manager_list, list)
>> +             if (manager->type == display->type)
>> +                     possible_crtcs |= 1 << manager->pipe;
>>
>>       /* create and initialize a encoder for this sub driver. */
>> -     encoder = exynos_drm_encoder_create(dev, subdrv->manager,
>> -                     (1 << MAX_CRTC) - 1);
>> +     encoder = exynos_drm_encoder_create(dev, display, possible_crtcs);
>>       if (!encoder) {
>>               DRM_ERROR("failed to create encoder\n");
>>               return -EFAULT;
>>       }
>> -     subdrv->encoder = encoder;
>> +     display->encoder = encoder;
>>
>> -     if (subdrv->manager->display_ops->type == EXYNOS_DISPLAY_TYPE_LCD)
>> {
>> +     if (display->type == EXYNOS_DISPLAY_TYPE_LCD) {
>>               ret = exynos_drm_attach_lcd_bridge(dev, encoder);
>
> The lvds bridge related codes need more reviews

I think everyone agrees that there need not be a 1:1 binding to driver
relationship, so what is left to discuss?

Sean



> so I'll merge this patch
> manually excepting the lvds bridge related codes. Let's discuss lvds bridge
> later.
>
> Thanks,
> Inki Dae
>
>>               if (ret)
>>                       return 0;
>> @@ -91,7 +98,7 @@ static int exynos_drm_create_enc_conn(struct drm_device
>> *dev,
>>               goto err_destroy_encoder;
>>       }
>>
>> -     subdrv->connector = connector;
>> +     display->connector = connector;
>>
>>       return 0;
>
Inki Dae Nov. 1, 2013, 4:20 a.m. UTC | #3
2013/11/1 Sean Paul <seanpaul@chromium.org>:
> On Thu, Oct 31, 2013 at 6:30 AM, Inki Dae <inki.dae@samsung.com> wrote:
>>
>>
>>> -----Original Message-----
>>> From: Sean Paul [mailto:seanpaul@chromium.org]
>>> Sent: Wednesday, October 30, 2013 1:13 AM
>>> To: dri-devel@lists.freedesktop.org; inki.dae@samsung.com
>>> Cc: airlied@linux.ie; tomasz.figa@gmail.com; marcheu@chromium.org; Sean
>>> Paul
>>> Subject: [PATCH v3 12/32] drm/exynos: Split manager/display/subdrv
>>>
>>> This patch splits display and manager from subdrv. The result is that
>>> crtc functions can directly call into manager callbacks and encoder
>>> functions can directly call into display callbacks. This will allow
>>> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
>>> with common code.
>>>
>>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>>> ---
>>>
>>> Changes in v2:
>>>       - Pass display into display_ops instead of context
>>> Changes in v3:
>>>       - Changed vidi args to exynos_drm_display instead of void
>>>       - Moved exynos_drm_hdmi.c removal into next patch
>>>
>>>  drivers/gpu/drm/exynos/exynos_drm_connector.c |  50 ++---
>>>  drivers/gpu/drm/exynos/exynos_drm_core.c      | 181 +++++++++++++-----
>>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c      | 115 +++++++++---
>>>  drivers/gpu/drm/exynos/exynos_drm_crtc.h      |  20 +-
>>>  drivers/gpu/drm/exynos/exynos_drm_drv.c       |  29 +--
>>>  drivers/gpu/drm/exynos/exynos_drm_drv.h       | 106 +++++++----
>>>  drivers/gpu/drm/exynos/exynos_drm_encoder.c   | 258
>> ++++------------------
>>> ----
>>>  drivers/gpu/drm/exynos/exynos_drm_encoder.h   |  18 +-
>>>  drivers/gpu/drm/exynos/exynos_drm_fb.c        |   4 +-
>>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 161 ++++++++--------
>>>  drivers/gpu/drm/exynos/exynos_drm_hdmi.c      | 211 +++++++++------------
>>>  drivers/gpu/drm/exynos/exynos_drm_hdmi.h      |   2 +
>>>  drivers/gpu/drm/exynos/exynos_drm_plane.c     |  15 +-
>>>  drivers/gpu/drm/exynos/exynos_drm_vidi.c      | 129 ++++++-------
>>>  14 files changed, 615 insertions(+), 684 deletions(-)
>>>
>>>  static int exynos_drm_create_enc_conn(struct drm_device *dev,
>>> -                                     struct exynos_drm_subdrv *subdrv)
>>> +                                     struct exynos_drm_display *display)
>>>  {
>>>       struct drm_encoder *encoder;
>>>       struct drm_connector *connector;
>>> +     struct exynos_drm_manager *manager;
>>>       int ret;
>>> +     unsigned long possible_crtcs = 0;
>>>
>>> -     subdrv->manager->dev = subdrv->dev;
>>> +     /* Find possible crtcs for this display */
>>> +     list_for_each_entry(manager, &exynos_drm_manager_list, list)
>>> +             if (manager->type == display->type)
>>> +                     possible_crtcs |= 1 << manager->pipe;
>>>
>>>       /* create and initialize a encoder for this sub driver. */
>>> -     encoder = exynos_drm_encoder_create(dev, subdrv->manager,
>>> -                     (1 << MAX_CRTC) - 1);
>>> +     encoder = exynos_drm_encoder_create(dev, display, possible_crtcs);
>>>       if (!encoder) {
>>>               DRM_ERROR("failed to create encoder\n");
>>>               return -EFAULT;
>>>       }
>>> -     subdrv->encoder = encoder;
>>> +     display->encoder = encoder;
>>>
>>> -     if (subdrv->manager->display_ops->type == EXYNOS_DISPLAY_TYPE_LCD)
>>> {
>>> +     if (display->type == EXYNOS_DISPLAY_TYPE_LCD) {
>>>               ret = exynos_drm_attach_lcd_bridge(dev, encoder);
>>
>> The lvds bridge related codes need more reviews
>
> I think everyone agrees that there need not be a 1:1 binding to driver
> relationship, so what is left to discuss?
>

I also agree to that no 1:1 binding. But this way it's not clear to me
yet. For this, I already commented that I'll try to find a better way
to you. So know that it's not that I don't agree to this way. If there
is no any better way, then I'll merge this lvds support to top of the
re-factoring patch series. any problem?  I'd really like to focus on
the re-factoring patch series as of now. And we also have other patch
series to should be reviewed. BTW, did Dave accept that lvds driver?
This lvds driver should be placed in drivers/gpu/drm/bridge/.

Thanks,
Inki Dae

> Sean
>
>
>
>> so I'll merge this patch
>> manually excepting the lvds bridge related codes. Let's discuss lvds bridge
>> later.
>>
>> Thanks,
>> Inki Dae
>>
>>>               if (ret)
>>>                       return 0;
>>> @@ -91,7 +98,7 @@ static int exynos_drm_create_enc_conn(struct drm_device
>>> *dev,
>>>               goto err_destroy_encoder;
>>>       }
>>>
>>> -     subdrv->connector = connector;
>>> +     display->connector = connector;
>>>
>>>       return 0;
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Tomasz Figa Nov. 10, 2013, 9:09 p.m. UTC | #4
Hi Sean,

On Tuesday 29 of October 2013 12:12:58 Sean Paul wrote:
> This patch splits display and manager from subdrv. The result is that
> crtc functions can directly call into manager callbacks and encoder
> functions can directly call into display callbacks. This will allow
> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
> with common code.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> 
> Changes in v2:
> 	- Pass display into display_ops instead of context
> Changes in v3:
> 	- Changed vidi args to exynos_drm_display instead of void
> 	- Moved exynos_drm_hdmi.c removal into next patch
> 
>  drivers/gpu/drm/exynos/exynos_drm_connector.c |  50 ++---
>  drivers/gpu/drm/exynos/exynos_drm_core.c      | 181 +++++++++++++-----
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c      | 115 +++++++++---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.h      |  20 +-
>  drivers/gpu/drm/exynos/exynos_drm_drv.c       |  29 +--
>  drivers/gpu/drm/exynos/exynos_drm_drv.h       | 106 +++++++----
>  drivers/gpu/drm/exynos/exynos_drm_encoder.c   | 258 ++++----------------------
>  drivers/gpu/drm/exynos/exynos_drm_encoder.h   |  18 +-
>  drivers/gpu/drm/exynos/exynos_drm_fb.c        |   4 +-
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 161 ++++++++--------
>  drivers/gpu/drm/exynos/exynos_drm_hdmi.c      | 211 +++++++++------------
>  drivers/gpu/drm/exynos/exynos_drm_hdmi.h      |   2 +
>  drivers/gpu/drm/exynos/exynos_drm_plane.c     |  15 +-
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c      | 129 ++++++-------
>  14 files changed, 615 insertions(+), 684 deletions(-)

This patch is really heavy, making it very hard to review. I wonder if it
couldn't really be split into several smaller patches...

Anyway, please see my comments below, for the points I haven't overlooked
due to the complexity of this patch.

> diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c b/drivers/gpu/drm/exynos/exynos_drm_core.c
> index 08ca4f9..e76098d 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_core.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_core.c
> @@ -16,11 +16,14 @@
>  #include <drm/drmP.h>
>  #include <drm/bridge/ptn3460.h>

Huh? Including a specific bridge chip inside a generic Exynos DRM core?
I know this is not a part of this patch, but... this is _broken_.

You may want to support multiple bridge chips in Exynos DRM core and you
may also want to support PTN3460 in other DRM cores. It can't be done this
way.

This series is very nice, but the whole effect is destroyed by basing it
on top of such insanity... Please rebase it on top of something more
reasonable (preferably Inki's next branch directly).

Otherwise, this patch seems reasonable (except its size).

Best regards,
Tomasz
Sean Paul Nov. 12, 2013, 5:51 p.m. UTC | #5
On Sun, Nov 10, 2013 at 4:09 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Sean,
>
> On Tuesday 29 of October 2013 12:12:58 Sean Paul wrote:
>> This patch splits display and manager from subdrv. The result is that
>> crtc functions can directly call into manager callbacks and encoder
>> functions can directly call into display callbacks. This will allow
>> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
>> with common code.
>>
>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> ---
>>
>> Changes in v2:
>>       - Pass display into display_ops instead of context
>> Changes in v3:
>>       - Changed vidi args to exynos_drm_display instead of void
>>       - Moved exynos_drm_hdmi.c removal into next patch
>>
>>  drivers/gpu/drm/exynos/exynos_drm_connector.c |  50 ++---
>>  drivers/gpu/drm/exynos/exynos_drm_core.c      | 181 +++++++++++++-----
>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c      | 115 +++++++++---
>>  drivers/gpu/drm/exynos/exynos_drm_crtc.h      |  20 +-
>>  drivers/gpu/drm/exynos/exynos_drm_drv.c       |  29 +--
>>  drivers/gpu/drm/exynos/exynos_drm_drv.h       | 106 +++++++----
>>  drivers/gpu/drm/exynos/exynos_drm_encoder.c   | 258 ++++----------------------
>>  drivers/gpu/drm/exynos/exynos_drm_encoder.h   |  18 +-
>>  drivers/gpu/drm/exynos/exynos_drm_fb.c        |   4 +-
>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 161 ++++++++--------
>>  drivers/gpu/drm/exynos/exynos_drm_hdmi.c      | 211 +++++++++------------
>>  drivers/gpu/drm/exynos/exynos_drm_hdmi.h      |   2 +
>>  drivers/gpu/drm/exynos/exynos_drm_plane.c     |  15 +-
>>  drivers/gpu/drm/exynos/exynos_drm_vidi.c      | 129 ++++++-------
>>  14 files changed, 615 insertions(+), 684 deletions(-)
>
> This patch is really heavy, making it very hard to review. I wonder if it
> couldn't really be split into several smaller patches...
>

I've distilled it down as much as possible. Unfortunately the
encoder/crtc were fused pretty tightly and the effects of that are
far-reaching.

> Anyway, please see my comments below, for the points I haven't overlooked
> due to the complexity of this patch.
>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c b/drivers/gpu/drm/exynos/exynos_drm_core.c
>> index 08ca4f9..e76098d 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_core.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_core.c
>> @@ -16,11 +16,14 @@
>>  #include <drm/drmP.h>
>>  #include <drm/bridge/ptn3460.h>
>
> Huh? Including a specific bridge chip inside a generic Exynos DRM core?
> I know this is not a part of this patch, but... this is _broken_.
>
> You may want to support multiple bridge chips in Exynos DRM core and you
> may also want to support PTN3460 in other DRM cores. It can't be done this
> way.
>
> This series is very nice, but the whole effect is destroyed by basing it
> on top of such insanity... Please rebase it on top of something more
> reasonable (preferably Inki's next branch directly).
>

Well, that was blunt. Unfortunately, I don't know how else to do this
other than this broken, unreasonable and insane way.

This has already been discussed at length in a few other places.
Without some help from the drm layer, I don't see any other way to do
this than to enumerate the possible bridges in each drm driver.

I based the current set off the "Add some missing bits for
exynos5250-snow" series I sent up a little while ago. This set was
sent to the relevant dt people, acked by Olof, and there are no
outstanding review comments. Since it's required for me to test on
Real Hardware, I assumed this was an appropriate base.

If you think this is _broken_, unreasonable, and insane, I'd love to
hear an alternative.

Sean



> Otherwise, this patch seems reasonable (except its size).
>
> Best regards,
> Tomasz
>
Tomasz Figa Nov. 12, 2013, 6:35 p.m. UTC | #6
On Tuesday 12 of November 2013 12:51:11 Sean Paul wrote:
> On Sun, Nov 10, 2013 at 4:09 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> > Hi Sean,
> >
> > On Tuesday 29 of October 2013 12:12:58 Sean Paul wrote:
> >> This patch splits display and manager from subdrv. The result is that
> >> crtc functions can directly call into manager callbacks and encoder
> >> functions can directly call into display callbacks. This will allow
> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
> >> with common code.
> >>
> >> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> >> ---
> >>
> >> Changes in v2:
> >>       - Pass display into display_ops instead of context
> >> Changes in v3:
> >>       - Changed vidi args to exynos_drm_display instead of void
> >>       - Moved exynos_drm_hdmi.c removal into next patch
> >>
> >>  drivers/gpu/drm/exynos/exynos_drm_connector.c |  50 ++---
> >>  drivers/gpu/drm/exynos/exynos_drm_core.c      | 181 +++++++++++++-----
> >>  drivers/gpu/drm/exynos/exynos_drm_crtc.c      | 115 +++++++++---
> >>  drivers/gpu/drm/exynos/exynos_drm_crtc.h      |  20 +-
> >>  drivers/gpu/drm/exynos/exynos_drm_drv.c       |  29 +--
> >>  drivers/gpu/drm/exynos/exynos_drm_drv.h       | 106 +++++++----
> >>  drivers/gpu/drm/exynos/exynos_drm_encoder.c   | 258 ++++----------------------
> >>  drivers/gpu/drm/exynos/exynos_drm_encoder.h   |  18 +-
> >>  drivers/gpu/drm/exynos/exynos_drm_fb.c        |   4 +-
> >>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 161 ++++++++--------
> >>  drivers/gpu/drm/exynos/exynos_drm_hdmi.c      | 211 +++++++++------------
> >>  drivers/gpu/drm/exynos/exynos_drm_hdmi.h      |   2 +
> >>  drivers/gpu/drm/exynos/exynos_drm_plane.c     |  15 +-
> >>  drivers/gpu/drm/exynos/exynos_drm_vidi.c      | 129 ++++++-------
> >>  14 files changed, 615 insertions(+), 684 deletions(-)
> >
> > This patch is really heavy, making it very hard to review. I wonder if it
> > couldn't really be split into several smaller patches...
> >
> 
> I've distilled it down as much as possible. Unfortunately the
> encoder/crtc were fused pretty tightly and the effects of that are
> far-reaching.

I was afraid of this. Well, I guess nothing can be done about it then.

> 
> > Anyway, please see my comments below, for the points I haven't overlooked
> > due to the complexity of this patch.
> >
> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c b/drivers/gpu/drm/exynos/exynos_drm_core.c
> >> index 08ca4f9..e76098d 100644
> >> --- a/drivers/gpu/drm/exynos/exynos_drm_core.c
> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_core.c
> >> @@ -16,11 +16,14 @@
> >>  #include <drm/drmP.h>
> >>  #include <drm/bridge/ptn3460.h>
> >
> > Huh? Including a specific bridge chip inside a generic Exynos DRM core?
> > I know this is not a part of this patch, but... this is _broken_.
> >
> > You may want to support multiple bridge chips in Exynos DRM core and you
> > may also want to support PTN3460 in other DRM cores. It can't be done this
> > way.
> >
> > This series is very nice, but the whole effect is destroyed by basing it
> > on top of such insanity... Please rebase it on top of something more
> > reasonable (preferably Inki's next branch directly).
> >
> 
> Well, that was blunt. Unfortunately, I don't know how else to do this
> other than this broken, unreasonable and insane way.

Sorry, this might have been a bit too harsh, but just imagine myself doing
my regular patch review round, hoping (as usual) to see only code that is
not less than great, while suddenly stumbling upon a line of code that
I would expect at most in some vendor tree or maybe several years ago in
sources of a 2.4 kernel.

> 
> This has already been discussed at length in a few other places.
> Without some help from the drm layer, I don't see any other way to do
> this than to enumerate the possible bridges in each drm driver.
> 
> I based the current set off the "Add some missing bits for
> exynos5250-snow" series I sent up a little while ago. This set was
> sent to the relevant dt people, acked by Olof, and there are no
> outstanding review comments. Since it's required for me to test on
> Real Hardware, I assumed this was an appropriate base.
> 
> If you think this is _broken_, unreasonable, and insane, I'd love to
> hear an alternative.

Well, I should say that Laurent would probably have something to say about
this, but I understand that the really good solution need to take some
time to be developed, so...

What about at least creating a minimal, temporary abstraction layer that
would provide all you originally needed from drm/bridge/ptn3460.h, but in
a bridge chip agnostic way?

I believe we already have established Device Tree bindings for video
interfaces (used currently by V4L) and you can safely use them to bind
your video devices together. Based on this assumption, you can implement
a local parser for a subset of those bindings that is enough to achieve
everything you need - bind a bridge to a video output of the SoC.

As for your explicit need to include headers of particular bridges,
looking at your Chromium kernel sources[1] I can see that all you need
is an init routine for the bridge. A rewritten find_bridge() that would
instead take some generic cookie of a device driving a connector could
return a generic (exynos_)drm_bridge cookie that could be further used
to call functions like (exynos_)drm_bridge_init(), which would then
use the cookie to call appropriate bridge-specific init function.

Of course, as a temporary solution, this can be made Exynos-specific,
but this won't add dependencies on particular bridge chips to Exynos DRM
core.

[1] http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel-next.git;a=blob;f=drivers/gpu/drm/exynos/exynos_drm_drv.c;h=099baa836784ea5d0eff8c2189abab35a3b342c5;hb=refs/heads/chromeos-3.8

Best regards,
Tomasz
Olof Johansson Nov. 26, 2013, 6 p.m. UTC | #7
On Tue, Nov 12, 2013 at 10:35 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On Tuesday 12 of November 2013 12:51:11 Sean Paul wrote:
>> On Sun, Nov 10, 2013 at 4:09 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> > Hi Sean,
>> >
>> > On Tuesday 29 of October 2013 12:12:58 Sean Paul wrote:
>> >> This patch splits display and manager from subdrv. The result is that
>> >> crtc functions can directly call into manager callbacks and encoder
>> >> functions can directly call into display callbacks. This will allow
>> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
>> >> with common code.
>> >>
>> >> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> >> ---
>> >>
>> >> Changes in v2:
>> >>       - Pass display into display_ops instead of context
>> >> Changes in v3:
>> >>       - Changed vidi args to exynos_drm_display instead of void
>> >>       - Moved exynos_drm_hdmi.c removal into next patch
>> >>
>> >>  drivers/gpu/drm/exynos/exynos_drm_connector.c |  50 ++---
>> >>  drivers/gpu/drm/exynos/exynos_drm_core.c      | 181 +++++++++++++-----
>> >>  drivers/gpu/drm/exynos/exynos_drm_crtc.c      | 115 +++++++++---
>> >>  drivers/gpu/drm/exynos/exynos_drm_crtc.h      |  20 +-
>> >>  drivers/gpu/drm/exynos/exynos_drm_drv.c       |  29 +--
>> >>  drivers/gpu/drm/exynos/exynos_drm_drv.h       | 106 +++++++----
>> >>  drivers/gpu/drm/exynos/exynos_drm_encoder.c   | 258 ++++----------------------
>> >>  drivers/gpu/drm/exynos/exynos_drm_encoder.h   |  18 +-
>> >>  drivers/gpu/drm/exynos/exynos_drm_fb.c        |   4 +-
>> >>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 161 ++++++++--------
>> >>  drivers/gpu/drm/exynos/exynos_drm_hdmi.c      | 211 +++++++++------------
>> >>  drivers/gpu/drm/exynos/exynos_drm_hdmi.h      |   2 +
>> >>  drivers/gpu/drm/exynos/exynos_drm_plane.c     |  15 +-
>> >>  drivers/gpu/drm/exynos/exynos_drm_vidi.c      | 129 ++++++-------
>> >>  14 files changed, 615 insertions(+), 684 deletions(-)
>> >
>> > This patch is really heavy, making it very hard to review. I wonder if it
>> > couldn't really be split into several smaller patches...
>> >
>>
>> I've distilled it down as much as possible. Unfortunately the
>> encoder/crtc were fused pretty tightly and the effects of that are
>> far-reaching.
>
> I was afraid of this. Well, I guess nothing can be done about it then.
>
>>
>> > Anyway, please see my comments below, for the points I haven't overlooked
>> > due to the complexity of this patch.
>> >
>> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c b/drivers/gpu/drm/exynos/exynos_drm_core.c
>> >> index 08ca4f9..e76098d 100644
>> >> --- a/drivers/gpu/drm/exynos/exynos_drm_core.c
>> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_core.c
>> >> @@ -16,11 +16,14 @@
>> >>  #include <drm/drmP.h>
>> >>  #include <drm/bridge/ptn3460.h>
>> >
>> > Huh? Including a specific bridge chip inside a generic Exynos DRM core?
>> > I know this is not a part of this patch, but... this is _broken_.
>> >
>> > You may want to support multiple bridge chips in Exynos DRM core and you
>> > may also want to support PTN3460 in other DRM cores. It can't be done this
>> > way.
>> >
>> > This series is very nice, but the whole effect is destroyed by basing it
>> > on top of such insanity... Please rebase it on top of something more
>> > reasonable (preferably Inki's next branch directly).
>> >
>>
>> Well, that was blunt. Unfortunately, I don't know how else to do this
>> other than this broken, unreasonable and insane way.
>
> Sorry, this might have been a bit too harsh, but just imagine myself doing
> my regular patch review round, hoping (as usual) to see only code that is
> not less than great, while suddenly stumbling upon a line of code that
> I would expect at most in some vendor tree or maybe several years ago in
> sources of a 2.4 kernel.

More like code written in the same style as x86 DRM stuff, where
they're not used to overabstracting things from day one to make things
generic instead of supporting the only known chip combination to date.


>> This has already been discussed at length in a few other places.
>> Without some help from the drm layer, I don't see any other way to do
>> this than to enumerate the possible bridges in each drm driver.
>>
>> I based the current set off the "Add some missing bits for
>> exynos5250-snow" series I sent up a little while ago. This set was
>> sent to the relevant dt people, acked by Olof, and there are no
>> outstanding review comments. Since it's required for me to test on
>> Real Hardware, I assumed this was an appropriate base.
>>
>> If you think this is _broken_, unreasonable, and insane, I'd love to
>> hear an alternative.
>
> Well, I should say that Laurent would probably have something to say about
> this, but I understand that the really good solution need to take some
> time to be developed, so...
>
> What about at least creating a minimal, temporary abstraction layer that
> would provide all you originally needed from drm/bridge/ptn3460.h, but in
> a bridge chip agnostic way?
>
> I believe we already have established Device Tree bindings for video
> interfaces (used currently by V4L) and you can safely use them to bind
> your video devices together. Based on this assumption, you can implement
> a local parser for a subset of those bindings that is enough to achieve
> everything you need - bind a bridge to a video output of the SoC.
>
> As for your explicit need to include headers of particular bridges,
> looking at your Chromium kernel sources[1] I can see that all you need
> is an init routine for the bridge. A rewritten find_bridge() that would
> instead take some generic cookie of a device driving a connector could
> return a generic (exynos_)drm_bridge cookie that could be further used
> to call functions like (exynos_)drm_bridge_init(), which would then
> use the cookie to call appropriate bridge-specific init function.
>
> Of course, as a temporary solution, this can be made Exynos-specific,
> but this won't add dependencies on particular bridge chips to Exynos DRM
> core.

On the other hand, there's a chance more abstraction than that is
needed when the second or third chip is added. Until then, there's
little point in doing premature work on abstracting too much of it
out. I'd rather see the code go in sooner and iterate in the tree.


-Olof
Thierry Reding Nov. 27, 2013, 10:04 a.m. UTC | #8
On Tue, Nov 26, 2013 at 10:00:13AM -0800, Olof Johansson wrote:
[...]
> More like code written in the same style as x86 DRM stuff, where
> they're not used to overabstracting things from day one to make things
> generic instead of supporting the only known chip combination to date.
[...]
> On the other hand, there's a chance more abstraction than that is
> needed when the second or third chip is added. Until then, there's
> little point in doing premature work on abstracting too much of it
> out. I'd rather see the code go in sooner and iterate in the tree.

+1

Thierry
Tomasz Figa Nov. 28, 2013, 11:04 p.m. UTC | #9
On Tuesday 26 of November 2013 10:00:13 Olof Johansson wrote:
> On Tue, Nov 12, 2013 at 10:35 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> > On Tuesday 12 of November 2013 12:51:11 Sean Paul wrote:
> >> On Sun, Nov 10, 2013 at 4:09 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> >> > Hi Sean,
> >> >
> >> > On Tuesday 29 of October 2013 12:12:58 Sean Paul wrote:
> >> >> This patch splits display and manager from subdrv. The result is that
> >> >> crtc functions can directly call into manager callbacks and encoder
> >> >> functions can directly call into display callbacks. This will allow
> >> >> us to remove the exynos_drm_hdmi shim and support mixer/hdmi & fimd/dp
> >> >> with common code.
> >> >>
> >> >> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> >> >> ---
> >> >>
> >> >> Changes in v2:
> >> >>       - Pass display into display_ops instead of context
> >> >> Changes in v3:
> >> >>       - Changed vidi args to exynos_drm_display instead of void
> >> >>       - Moved exynos_drm_hdmi.c removal into next patch
> >> >>
> >> >>  drivers/gpu/drm/exynos/exynos_drm_connector.c |  50 ++---
> >> >>  drivers/gpu/drm/exynos/exynos_drm_core.c      | 181 +++++++++++++-----
> >> >>  drivers/gpu/drm/exynos/exynos_drm_crtc.c      | 115 +++++++++---
> >> >>  drivers/gpu/drm/exynos/exynos_drm_crtc.h      |  20 +-
> >> >>  drivers/gpu/drm/exynos/exynos_drm_drv.c       |  29 +--
> >> >>  drivers/gpu/drm/exynos/exynos_drm_drv.h       | 106 +++++++----
> >> >>  drivers/gpu/drm/exynos/exynos_drm_encoder.c   | 258 ++++----------------------
> >> >>  drivers/gpu/drm/exynos/exynos_drm_encoder.h   |  18 +-
> >> >>  drivers/gpu/drm/exynos/exynos_drm_fb.c        |   4 +-
> >> >>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 161 ++++++++--------
> >> >>  drivers/gpu/drm/exynos/exynos_drm_hdmi.c      | 211 +++++++++------------
> >> >>  drivers/gpu/drm/exynos/exynos_drm_hdmi.h      |   2 +
> >> >>  drivers/gpu/drm/exynos/exynos_drm_plane.c     |  15 +-
> >> >>  drivers/gpu/drm/exynos/exynos_drm_vidi.c      | 129 ++++++-------
> >> >>  14 files changed, 615 insertions(+), 684 deletions(-)
> >> >
> >> > This patch is really heavy, making it very hard to review. I wonder if it
> >> > couldn't really be split into several smaller patches...
> >> >
> >>
> >> I've distilled it down as much as possible. Unfortunately the
> >> encoder/crtc were fused pretty tightly and the effects of that are
> >> far-reaching.
> >
> > I was afraid of this. Well, I guess nothing can be done about it then.
> >
> >>
> >> > Anyway, please see my comments below, for the points I haven't overlooked
> >> > due to the complexity of this patch.
> >> >
> >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c b/drivers/gpu/drm/exynos/exynos_drm_core.c
> >> >> index 08ca4f9..e76098d 100644
> >> >> --- a/drivers/gpu/drm/exynos/exynos_drm_core.c
> >> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_core.c
> >> >> @@ -16,11 +16,14 @@
> >> >>  #include <drm/drmP.h>
> >> >>  #include <drm/bridge/ptn3460.h>
> >> >
> >> > Huh? Including a specific bridge chip inside a generic Exynos DRM core?
> >> > I know this is not a part of this patch, but... this is _broken_.
> >> >
> >> > You may want to support multiple bridge chips in Exynos DRM core and you
> >> > may also want to support PTN3460 in other DRM cores. It can't be done this
> >> > way.
> >> >
> >> > This series is very nice, but the whole effect is destroyed by basing it
> >> > on top of such insanity... Please rebase it on top of something more
> >> > reasonable (preferably Inki's next branch directly).
> >> >
> >>
> >> Well, that was blunt. Unfortunately, I don't know how else to do this
> >> other than this broken, unreasonable and insane way.
> >
> > Sorry, this might have been a bit too harsh, but just imagine myself doing
> > my regular patch review round, hoping (as usual) to see only code that is
> > not less than great, while suddenly stumbling upon a line of code that
> > I would expect at most in some vendor tree or maybe several years ago in
> > sources of a 2.4 kernel.
> 
> More like code written in the same style as x86 DRM stuff, where
> they're not used to overabstracting things from day one to make things
> generic instead of supporting the only known chip combination to date.

No, not really. They don't have any modularity on x86. Just a graphics
card with everything on it, so they can freely do such things, as they
don't have to account for all we need to account for on ARM platforms.

Also, I wouldn't call making a driver usable and useful for more than
just one fixed platform "overabstracting"...

> 
> 
> >> This has already been discussed at length in a few other places.
> >> Without some help from the drm layer, I don't see any other way to do
> >> this than to enumerate the possible bridges in each drm driver.
> >>
> >> I based the current set off the "Add some missing bits for
> >> exynos5250-snow" series I sent up a little while ago. This set was
> >> sent to the relevant dt people, acked by Olof, and there are no
> >> outstanding review comments. Since it's required for me to test on
> >> Real Hardware, I assumed this was an appropriate base.
> >>
> >> If you think this is _broken_, unreasonable, and insane, I'd love to
> >> hear an alternative.
> >
> > Well, I should say that Laurent would probably have something to say about
> > this, but I understand that the really good solution need to take some
> > time to be developed, so...
> >
> > What about at least creating a minimal, temporary abstraction layer that
> > would provide all you originally needed from drm/bridge/ptn3460.h, but in
> > a bridge chip agnostic way?
> >
> > I believe we already have established Device Tree bindings for video
> > interfaces (used currently by V4L) and you can safely use them to bind
> > your video devices together. Based on this assumption, you can implement
> > a local parser for a subset of those bindings that is enough to achieve
> > everything you need - bind a bridge to a video output of the SoC.
> >
> > As for your explicit need to include headers of particular bridges,
> > looking at your Chromium kernel sources[1] I can see that all you need
> > is an init routine for the bridge. A rewritten find_bridge() that would
> > instead take some generic cookie of a device driving a connector could
> > return a generic (exynos_)drm_bridge cookie that could be further used
> > to call functions like (exynos_)drm_bridge_init(), which would then
> > use the cookie to call appropriate bridge-specific init function.
> >
> > Of course, as a temporary solution, this can be made Exynos-specific,
> > but this won't add dependencies on particular bridge chips to Exynos DRM
> > core.
> 
> On the other hand, there's a chance more abstraction than that is
> needed when the second or third chip is added. Until then, there's
> little point in doing premature work on abstracting too much of it
> out. I'd rather see the code go in sooner and iterate in the tree.

First of all, this is heavily Chromebook specific and that's not the only
Exynos-based platform using Exynos DRM. In fact, before conversion to DT,
we had full support for parallel and DSI displays on several Exynos4 based
boards.

I really regret that we allowed for such regression back then, but now
the only thing we can do about it is to at least not make a step backwards
with this and make DRM bridge something useful (or at least usable).

Best regards,
Tomasz
Daniel Vetter Nov. 29, 2013, 7:52 a.m. UTC | #10
On Fri, Nov 29, 2013 at 12:04:04AM +0100, Tomasz Figa wrote:
> On Tuesday 26 of November 2013 10:00:13 Olof Johansson wrote:
> > More like code written in the same style as x86 DRM stuff, where
> > they're not used to overabstracting things from day one to make things
> > generic instead of supporting the only known chip combination to date.
> 
> No, not really. They don't have any modularity on x86. Just a graphics
> card with everything on it, so they can freely do such things, as they
> don't have to account for all we need to account for on ARM platforms.

I guess you didn't bother looking at x86 drivers then ... in i915 we have
different blocks spanning 7 major hw iterations, reused in funny (and
overlapping) ways. We have clock trees, nested power domains, interrupt
controllers and forwarders, different coherency domains, off-chip
functiosn in the PCH, the full shebang.

In the code itself we have piles of vtables for the different parts of the
chip, mmio base offsets (since hw engineers just love to move things
around) and otherwise neatly abstracted helpers for different parts of the
chip to be able to reuse things across generations.

The _only_ difference I see compared to an arm soc is that this entire
madness is neatly wrapped up into a fake pci device. And some of the
really remote mmio ranges are windowed into general hodgepodge mmio
register bar so that we don't have to hunt it down on different pci
devices. But occasionally we do even that.

So as per the discussion at KS about soc gfx drivers where all the ip
blocks come from the same vendor I really don't see why arm drivers need
to be so much different than more traditional x86 drivers. Since nowadays
(at least with intel) that means we _are_ talking about an soc.

Cheers, Daniel
Tomasz Figa Nov. 29, 2013, 9:10 a.m. UTC | #11
On Friday 29 of November 2013 08:52:22 Daniel Vetter wrote:
> On Fri, Nov 29, 2013 at 12:04:04AM +0100, Tomasz Figa wrote:
> > On Tuesday 26 of November 2013 10:00:13 Olof Johansson wrote:
> > > More like code written in the same style as x86 DRM stuff, where
> > > they're not used to overabstracting things from day one to make things
> > > generic instead of supporting the only known chip combination to date.
> > 
> > No, not really. They don't have any modularity on x86. Just a graphics
> > card with everything on it, so they can freely do such things, as they
> > don't have to account for all we need to account for on ARM platforms.
> 
> I guess you didn't bother looking at x86 drivers then ... in i915 we have
> different blocks spanning 7 major hw iterations, reused in funny (and
> overlapping) ways. We have clock trees, nested power domains, interrupt
> controllers and forwarders, different coherency domains, off-chip
> functiosn in the PCH, the full shebang.
> 
> In the code itself we have piles of vtables for the different parts of the
> chip, mmio base offsets (since hw engineers just love to move things
> around) and otherwise neatly abstracted helpers for different parts of the
> chip to be able to reuse things across generations.
> 
> The _only_ difference I see compared to an arm soc is that this entire
> madness is neatly wrapped up into a fake pci device. And some of the
> really remote mmio ranges are windowed into general hodgepodge mmio
> register bar so that we don't have to hunt it down on different pci
> devices. But occasionally we do even that.
> 
> So as per the discussion at KS about soc gfx drivers where all the ip
> blocks come from the same vendor I really don't see why arm drivers need
> to be so much different than more traditional x86 drivers. Since nowadays
> (at least with intel) that means we _are_ talking about an soc.

I would mostly agree with you if we were discussing SoC-internal
components here. Mostly, because the ARM world is more complex and you
can see the same IP across completely different SoCs from different
vendors.

However, the topic here is about external devices, outside the SoC, such
as different kind of bridges, like the PTN3460 eDP to LVDS bridge, which
are likely to be reused across different platforms. Similar thing is with
using different bridges on different boards using the same SoC platform.
I don't think having an abstraction here would be any overabstraction at
all. Anything less would be a huge "underabstraction" in fact.

Best regards,
Tomasz
Thierry Reding Nov. 29, 2013, 10:16 a.m. UTC | #12
On Fri, Nov 29, 2013 at 12:04:04AM +0100, Tomasz Figa wrote:
> On Tuesday 26 of November 2013 10:00:13 Olof Johansson wrote:
> > On Tue, Nov 12, 2013 at 10:35 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
[...]
> > > Sorry, this might have been a bit too harsh, but just imagine myself doing
> > > my regular patch review round, hoping (as usual) to see only code that is
> > > not less than great, while suddenly stumbling upon a line of code that
> > > I would expect at most in some vendor tree or maybe several years ago in
> > > sources of a 2.4 kernel.
> > 
> > More like code written in the same style as x86 DRM stuff, where
> > they're not used to overabstracting things from day one to make things
> > generic instead of supporting the only known chip combination to date.
> 
> No, not really. They don't have any modularity on x86. Just a graphics
> card with everything on it, so they can freely do such things, as they
> don't have to account for all we need to account for on ARM platforms.

I don't think there's such a clear difference. A particular GPU on x86
is equally modular as a specific GPU on an ARM SoC. The problem is that
on x86 there's much less mix-and-matching going on.

So theoretically they do need to account for the same craziness. That's
not done, however, because there's no need for it. It doesn't happen in
practice.

> Also, I wouldn't call making a driver usable and useful for more than
> just one fixed platform "overabstracting"...

It is if there's never more than the single fixed platform. Since none
of us can predict the future I think it's entirely reasonable if we do
concentrate on solving the problems that we have now. It doesn't mean
that we should write code in a way that makes future enhancements
unnecessarily difficult, but I very much prefer to see code merged that
supports one specific use-case that we have now rather than working on
the code for years on end in an attempt to make it generic enough to
support everything that the future can possibly hold.

Chances are pretty good that we won't be able to predict one specific
use-case and then rewrite everything anyway. Linux has been pretty
successful so far in a large part because it has evolved organically.
We're not doing ourselves any good by requiring everything to be perfect
from the beginning.

We all know what a disaster DT has been and still is. It makes it very
difficult to get new features supported because we now have a component
that we can no longer change at will and that cannot evolve organically
anymore. That impedes progress.

For this particular issue no DT is involved. There's no need for ABI
stability because it's just in-tree code and we can change it to our
heart's content. We can refactor things when an actual need arises. By
then chances are pretty good we'll have a much better understanding of
what exactly we need and therefore we can come up with a much better
solution than at this point in time.

Thierry
Daniel Vetter Nov. 29, 2013, 10:25 a.m. UTC | #13
On Fri, Nov 29, 2013 at 10:10 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> I would mostly agree with you if we were discussing SoC-internal
> components here. Mostly, because the ARM world is more complex and you
> can see the same IP across completely different SoCs from different
> vendors.

Yeah, hence the restriction to IP blocks from the same vendor. Heck we
have external IP blocks in our gpus, too. It's just that the driver to
make that one work is in no shape for upstreaming :( So even for that
case of smashing a random decode engine into your gpu we've seen this
on intel. And I'd guess the radeon uvd block is a similarly alien
piece (although at least not shared anywhere else iirc).

> However, the topic here is about external devices, outside the SoC, such
> as different kind of bridges, like the PTN3460 eDP to LVDS bridge, which
> are likely to be reused across different platforms. Similar thing is with
> using different bridges on different boards using the same SoC platform.
> I don't think having an abstraction here would be any overabstraction at
> all. Anything less would be a huge "underabstraction" in fact.

Hm, I've thought Sean's rework was mostly about the driver core and
not so much about off-chip blocks. But I admit to not have read
through all the details of this thread, so please forgive me my
ignorance ;-)

Cheers, Daniel
Rob Clark Nov. 29, 2013, 2:13 p.m. UTC | #14
On Fri, Nov 29, 2013 at 4:10 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> I would mostly agree with you if we were discussing SoC-internal
> components here. Mostly, because the ARM world is more complex and you
> can see the same IP across completely different SoCs from different
> vendors.
>
> However, the topic here is about external devices, outside the SoC, such
> as different kind of bridges, like the PTN3460 eDP to LVDS bridge, which
> are likely to be reused across different platforms. Similar thing is with
> using different bridges on different boards using the same SoC platform.
> I don't think having an abstraction here would be any overabstraction at
> all. Anything less would be a huge "underabstraction" in fact.


I think no one is arguing that we don't eventually need some better
abstraction.  But as long as it is one-bridge and one-user, if the
patches otherwise have merit, add functionality that was missing
before and don't regress, then lack of infrastructure to match up
bridge and driver isn't something I will care about too much yet.
Things are allowed to be in-progress.  A missing abstraction for a 1:1
relationship is fine.

Now as we start getting a few more bridge devices and users, then I'll
start blocking patches until some sort of generic bridge loader is
sorted out.

BR,
-R
Tomasz Figa Nov. 29, 2013, 5:05 p.m. UTC | #15
On Friday 29 of November 2013 09:13:19 Rob Clark wrote:
> On Fri, Nov 29, 2013 at 4:10 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> > I would mostly agree with you if we were discussing SoC-internal
> > components here. Mostly, because the ARM world is more complex and you
> > can see the same IP across completely different SoCs from different
> > vendors.
> >
> > However, the topic here is about external devices, outside the SoC, such
> > as different kind of bridges, like the PTN3460 eDP to LVDS bridge, which
> > are likely to be reused across different platforms. Similar thing is with
> > using different bridges on different boards using the same SoC platform.
> > I don't think having an abstraction here would be any overabstraction at
> > all. Anything less would be a huge "underabstraction" in fact.
> 
> 
> I think no one is arguing that we don't eventually need some better
> abstraction.  But as long as it is one-bridge and one-user, if the
> patches otherwise have merit, add functionality that was missing
> before and don't regress, then lack of infrastructure to match up
> bridge and driver isn't something I will care about too much yet.
> Things are allowed to be in-progress.  A missing abstraction for a 1:1
> relationship is fine.

This is not just one-bridge, one-user. This is about users of Exynos DRM
we already have in-tree, such as Trats, Trats2 or Arndale, that the DRM
bridge infrastructure could be used on and finally allowing to have
display support on them. Of course you could merge this as is and
then let someone else completely rewrite it (most likely in the same
release cycle), but since it's not really much more work, I don't
think there is any sense.

Moreover, let's stick to modern kernel driver coding standards. I don't
think that "I want this patchset merged so badly" is really a good excuse
to get around it. After all, there would be no GKH's staging tree, if
nobody cared about quality in mainline.

Best regards,
Tomasz
Rob Clark Nov. 29, 2013, 6:35 p.m. UTC | #16
On Fri, Nov 29, 2013 at 12:05 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> On Friday 29 of November 2013 09:13:19 Rob Clark wrote:
>> On Fri, Nov 29, 2013 at 4:10 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> > I would mostly agree with you if we were discussing SoC-internal
>> > components here. Mostly, because the ARM world is more complex and you
>> > can see the same IP across completely different SoCs from different
>> > vendors.
>> >
>> > However, the topic here is about external devices, outside the SoC, such
>> > as different kind of bridges, like the PTN3460 eDP to LVDS bridge, which
>> > are likely to be reused across different platforms. Similar thing is with
>> > using different bridges on different boards using the same SoC platform.
>> > I don't think having an abstraction here would be any overabstraction at
>> > all. Anything less would be a huge "underabstraction" in fact.
>>
>>
>> I think no one is arguing that we don't eventually need some better
>> abstraction.  But as long as it is one-bridge and one-user, if the
>> patches otherwise have merit, add functionality that was missing
>> before and don't regress, then lack of infrastructure to match up
>> bridge and driver isn't something I will care about too much yet.
>> Things are allowed to be in-progress.  A missing abstraction for a 1:1
>> relationship is fine.
>
> This is not just one-bridge, one-user. This is about users of Exynos DRM
> we already have in-tree, such as Trats, Trats2 or Arndale, that the DRM
> bridge infrastructure could be used on and finally allowing to have
> display support on them. Of course you could merge this as is and
> then let someone else completely rewrite it (most likely in the same
> release cycle), but since it's not really much more work, I don't
> think there is any sense.

well, I'm not quite sure where I there is a pending complete
re-write..  it looks like the hard ptn3460 dependency is just a few
lines in one function.  Otherwise I'd agree with you.

(and even the patch that touches the code calling ptn3460_init() is
just moving it around from what I see)

> Moreover, let's stick to modern kernel driver coding standards. I don't
> think that "I want this patchset merged so badly" is really a good excuse
> to get around it. After all, there would be no GKH's staging tree, if
> nobody cared about quality in mainline.

And with my quality hat on, I could say that I'm not too fond of
unused (or 1:1 client to user) abstractions.  That is something that
should be introduced as we merge our 2nd or 3rd bridge.

BR,
-R

> Best regards,
> Tomasz
>
Inki Dae Nov. 30, 2013, 5:25 a.m. UTC | #17
Hi all,

How about moving this discussion to other related email thread,
"[PATCH v3 31/32] drm/exynos: Move lvds bridge discovery into DP
driver"? lvds related codes have already been removed from this patch
and moved into eDP bus driver. It would be more good to make a
dicussion about actual codes.

Thanks,
Inki Dae

2013/11/30 Rob Clark <robdclark@gmail.com>:
> On Fri, Nov 29, 2013 at 12:05 PM, Tomasz Figa <t.figa@samsung.com> wrote:
>> On Friday 29 of November 2013 09:13:19 Rob Clark wrote:
>>> On Fri, Nov 29, 2013 at 4:10 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>> > I would mostly agree with you if we were discussing SoC-internal
>>> > components here. Mostly, because the ARM world is more complex and you
>>> > can see the same IP across completely different SoCs from different
>>> > vendors.
>>> >
>>> > However, the topic here is about external devices, outside the SoC, such
>>> > as different kind of bridges, like the PTN3460 eDP to LVDS bridge, which
>>> > are likely to be reused across different platforms. Similar thing is with
>>> > using different bridges on different boards using the same SoC platform.
>>> > I don't think having an abstraction here would be any overabstraction at
>>> > all. Anything less would be a huge "underabstraction" in fact.
>>>
>>>
>>> I think no one is arguing that we don't eventually need some better
>>> abstraction.  But as long as it is one-bridge and one-user, if the
>>> patches otherwise have merit, add functionality that was missing
>>> before and don't regress, then lack of infrastructure to match up
>>> bridge and driver isn't something I will care about too much yet.
>>> Things are allowed to be in-progress.  A missing abstraction for a 1:1
>>> relationship is fine.
>>
>> This is not just one-bridge, one-user. This is about users of Exynos DRM
>> we already have in-tree, such as Trats, Trats2 or Arndale, that the DRM
>> bridge infrastructure could be used on and finally allowing to have
>> display support on them. Of course you could merge this as is and
>> then let someone else completely rewrite it (most likely in the same
>> release cycle), but since it's not really much more work, I don't
>> think there is any sense.
>
> well, I'm not quite sure where I there is a pending complete
> re-write..  it looks like the hard ptn3460 dependency is just a few
> lines in one function.  Otherwise I'd agree with you.
>
> (and even the patch that touches the code calling ptn3460_init() is
> just moving it around from what I see)
>
>> Moreover, let's stick to modern kernel driver coding standards. I don't
>> think that "I want this patchset merged so badly" is really a good excuse
>> to get around it. After all, there would be no GKH's staging tree, if
>> nobody cared about quality in mainline.
>
> And with my quality hat on, I could say that I'm not too fond of
> unused (or 1:1 client to user) abstractions.  That is something that
> should be introduced as we merge our 2nd or 3rd bridge.
>
> BR,
> -R
>
>> Best regards,
>> Tomasz
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Sean Paul Dec. 3, 2013, 9:38 p.m. UTC | #18
On Fri, Nov 29, 2013 at 12:05 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> On Friday 29 of November 2013 09:13:19 Rob Clark wrote:
>> On Fri, Nov 29, 2013 at 4:10 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> > I would mostly agree with you if we were discussing SoC-internal
>> > components here. Mostly, because the ARM world is more complex and you
>> > can see the same IP across completely different SoCs from different
>> > vendors.
>> >
>> > However, the topic here is about external devices, outside the SoC, such
>> > as different kind of bridges, like the PTN3460 eDP to LVDS bridge, which
>> > are likely to be reused across different platforms. Similar thing is with
>> > using different bridges on different boards using the same SoC platform.
>> > I don't think having an abstraction here would be any overabstraction at
>> > all. Anything less would be a huge "underabstraction" in fact.
>>
>>
>> I think no one is arguing that we don't eventually need some better
>> abstraction.  But as long as it is one-bridge and one-user, if the
>> patches otherwise have merit, add functionality that was missing
>> before and don't regress, then lack of infrastructure to match up
>> bridge and driver isn't something I will care about too much yet.
>> Things are allowed to be in-progress.  A missing abstraction for a 1:1
>> relationship is fine.
>
> This is not just one-bridge, one-user. This is about users of Exynos DRM
> we already have in-tree, such as Trats, Trats2 or Arndale, that the DRM
> bridge infrastructure could be used on and finally allowing to have
> display support on them. Of course you could merge this as is and
> then let someone else completely rewrite it (most likely in the same
> release cycle), but since it's not really much more work, I don't
> think there is any sense.
>
> Moreover, let's stick to modern kernel driver coding standards. I don't
> think that "I want this patchset merged so badly" is really a good excuse
> to get around it. After all, there would be no GKH's staging tree, if
> nobody cared about quality in mainline.
>

I don't see how this change makes the code quality any worse, it's a
noop on boards without the bridge.

I proposed a general solution in v2, to which Laurent said he was
working on a patch. I didn't feel the need to do anything more general
here since it would be more work to rip it out later.

I am happy to pick up this work, however:
- I don't want to step on Laurent's toes
- It's probably more useful to have a general framework with 2 or more
bridges, so I don't think it should be a blocker on the ptn stuff

Sean



> Best regards,
> Tomasz
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox

Patch

diff --git a/drivers/gpu/drm/exynos/exynos_drm_connector.c b/drivers/gpu/drm/exynos/exynos_drm_connector.c
index ca270e2..9a16dbe 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_connector.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_connector.c
@@ -23,26 +23,20 @@ 
 				drm_connector)
 
 struct exynos_drm_connector {
-	struct drm_connector	drm_connector;
-	uint32_t		encoder_id;
-	struct exynos_drm_manager *manager;
+	struct drm_connector		drm_connector;
+	uint32_t			encoder_id;
+	struct exynos_drm_display	*display;
 };
 
 static int exynos_drm_connector_get_modes(struct drm_connector *connector)
 {
 	struct exynos_drm_connector *exynos_connector =
 					to_exynos_connector(connector);
-	struct exynos_drm_manager *manager = exynos_connector->manager;
-	struct exynos_drm_display_ops *display_ops = manager->display_ops;
+	struct exynos_drm_display *display = exynos_connector->display;
 	struct edid *edid = NULL;
 	unsigned int count = 0;
 	int ret;
 
-	if (!display_ops) {
-		DRM_DEBUG_KMS("display_ops is null.\n");
-		return 0;
-	}
-
 	/*
 	 * if get_edid() exists then get_edid() callback of hdmi side
 	 * is called to get edid data through i2c interface else
@@ -51,8 +45,8 @@  static int exynos_drm_connector_get_modes(struct drm_connector *connector)
 	 * P.S. in case of lcd panel, count is always 1 if success
 	 * because lcd panel has only one mode.
 	 */
-	if (display_ops->get_edid) {
-		edid = display_ops->get_edid(manager->dev, connector);
+	if (display->ops->get_edid) {
+		edid = display->ops->get_edid(display, connector);
 		if (IS_ERR_OR_NULL(edid)) {
 			ret = PTR_ERR(edid);
 			edid = NULL;
@@ -75,8 +69,8 @@  static int exynos_drm_connector_get_modes(struct drm_connector *connector)
 			return 0;
 		}
 
-		if (display_ops->get_panel)
-			panel = display_ops->get_panel(manager->dev);
+		if (display->ops->get_panel)
+			panel = display->ops->get_panel(display);
 		else {
 			drm_mode_destroy(connector->dev, mode);
 			return 0;
@@ -105,14 +99,13 @@  static int exynos_drm_connector_mode_valid(struct drm_connector *connector,
 {
 	struct exynos_drm_connector *exynos_connector =
 					to_exynos_connector(connector);
-	struct exynos_drm_manager *manager = exynos_connector->manager;
-	struct exynos_drm_display_ops *display_ops = manager->display_ops;
+	struct exynos_drm_display *display = exynos_connector->display;
 	int ret = MODE_BAD;
 
 	DRM_DEBUG_KMS("%s\n", __FILE__);
 
-	if (display_ops && display_ops->check_mode)
-		if (!display_ops->check_mode(manager->dev, mode))
+	if (display->ops->check_mode)
+		if (!display->ops->check_mode(display, mode))
 			ret = MODE_OK;
 
 	return ret;
@@ -151,8 +144,7 @@  static int exynos_drm_connector_fill_modes(struct drm_connector *connector,
 {
 	struct exynos_drm_connector *exynos_connector =
 					to_exynos_connector(connector);
-	struct exynos_drm_manager *manager = exynos_connector->manager;
-	struct exynos_drm_manager_ops *ops = manager->ops;
+	struct exynos_drm_display *display = exynos_connector->display;
 	unsigned int width, height;
 
 	width = max_width;
@@ -162,8 +154,8 @@  static int exynos_drm_connector_fill_modes(struct drm_connector *connector,
 	 * if specific driver want to find desired_mode using maxmum
 	 * resolution then get max width and height from that driver.
 	 */
-	if (ops && ops->get_max_resol)
-		ops->get_max_resol(manager, &width, &height);
+	if (display->ops->get_max_resol)
+		display->ops->get_max_resol(display, &width, &height);
 
 	return drm_helper_probe_single_connector_modes(connector, width,
 							height);
@@ -175,13 +167,11 @@  exynos_drm_connector_detect(struct drm_connector *connector, bool force)
 {
 	struct exynos_drm_connector *exynos_connector =
 					to_exynos_connector(connector);
-	struct exynos_drm_manager *manager = exynos_connector->manager;
-	struct exynos_drm_display_ops *display_ops =
-					manager->display_ops;
+	struct exynos_drm_display *display = exynos_connector->display;
 	enum drm_connector_status status = connector_status_disconnected;
 
-	if (display_ops && display_ops->is_connected) {
-		if (display_ops->is_connected(manager->dev))
+	if (display->ops->is_connected) {
+		if (display->ops->is_connected(display))
 			status = connector_status_connected;
 		else
 			status = connector_status_disconnected;
@@ -211,7 +201,7 @@  struct drm_connector *exynos_drm_connector_create(struct drm_device *dev,
 						   struct drm_encoder *encoder)
 {
 	struct exynos_drm_connector *exynos_connector;
-	struct exynos_drm_manager *manager = exynos_drm_get_manager(encoder);
+	struct exynos_drm_display *display = exynos_drm_get_display(encoder);
 	struct drm_connector *connector;
 	int type;
 	int err;
@@ -222,7 +212,7 @@  struct drm_connector *exynos_drm_connector_create(struct drm_device *dev,
 
 	connector = &exynos_connector->drm_connector;
 
-	switch (manager->display_ops->type) {
+	switch (display->type) {
 	case EXYNOS_DISPLAY_TYPE_HDMI:
 		type = DRM_MODE_CONNECTOR_HDMIA;
 		connector->interlace_allowed = true;
@@ -245,7 +235,7 @@  struct drm_connector *exynos_drm_connector_create(struct drm_device *dev,
 		goto err_connector;
 
 	exynos_connector->encoder_id = encoder->base.id;
-	exynos_connector->manager = manager;
+	exynos_connector->display = display;
 	connector->dpms = DRM_MODE_DPMS_OFF;
 	connector->encoder = encoder;
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_core.c b/drivers/gpu/drm/exynos/exynos_drm_core.c
index 08ca4f9..e76098d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_core.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_core.c
@@ -16,11 +16,14 @@ 
 #include <drm/drmP.h>
 #include <drm/bridge/ptn3460.h>
 #include "exynos_drm_drv.h"
+#include "exynos_drm_crtc.h"
 #include "exynos_drm_encoder.h"
 #include "exynos_drm_connector.h"
 #include "exynos_drm_fbdev.h"
 
 static LIST_HEAD(exynos_drm_subdrv_list);
+static LIST_HEAD(exynos_drm_manager_list);
+static LIST_HEAD(exynos_drm_display_list);
 
 struct bridge_init {
 	struct i2c_client *client;
@@ -57,24 +60,28 @@  static int exynos_drm_attach_lcd_bridge(struct drm_device *dev,
 }
 
 static int exynos_drm_create_enc_conn(struct drm_device *dev,
-					struct exynos_drm_subdrv *subdrv)
+					struct exynos_drm_display *display)
 {
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
+	struct exynos_drm_manager *manager;
 	int ret;
+	unsigned long possible_crtcs = 0;
 
-	subdrv->manager->dev = subdrv->dev;
+	/* Find possible crtcs for this display */
+	list_for_each_entry(manager, &exynos_drm_manager_list, list)
+		if (manager->type == display->type)
+			possible_crtcs |= 1 << manager->pipe;
 
 	/* create and initialize a encoder for this sub driver. */
-	encoder = exynos_drm_encoder_create(dev, subdrv->manager,
-			(1 << MAX_CRTC) - 1);
+	encoder = exynos_drm_encoder_create(dev, display, possible_crtcs);
 	if (!encoder) {
 		DRM_ERROR("failed to create encoder\n");
 		return -EFAULT;
 	}
-	subdrv->encoder = encoder;
+	display->encoder = encoder;
 
-	if (subdrv->manager->display_ops->type == EXYNOS_DISPLAY_TYPE_LCD) {
+	if (display->type == EXYNOS_DISPLAY_TYPE_LCD) {
 		ret = exynos_drm_attach_lcd_bridge(dev, encoder);
 		if (ret)
 			return 0;
@@ -91,7 +98,7 @@  static int exynos_drm_create_enc_conn(struct drm_device *dev,
 		goto err_destroy_encoder;
 	}
 
-	subdrv->connector = connector;
+	display->connector = connector;
 
 	return 0;
 
@@ -100,21 +107,6 @@  err_destroy_encoder:
 	return ret;
 }
 
-static void exynos_drm_destroy_enc_conn(struct exynos_drm_subdrv *subdrv)
-{
-	if (subdrv->encoder) {
-		struct drm_encoder *encoder = subdrv->encoder;
-		encoder->funcs->destroy(encoder);
-		subdrv->encoder = NULL;
-	}
-
-	if (subdrv->connector) {
-		struct drm_connector *connector = subdrv->connector;
-		connector->funcs->destroy(connector);
-		subdrv->connector = NULL;
-	}
-}
-
 static int exynos_drm_subdrv_probe(struct drm_device *dev,
 					struct exynos_drm_subdrv *subdrv)
 {
@@ -146,10 +138,98 @@  static void exynos_drm_subdrv_remove(struct drm_device *dev,
 		subdrv->remove(dev, subdrv->dev);
 }
 
+int exynos_drm_initialize_managers(struct drm_device *dev)
+{
+	struct exynos_drm_manager *manager, *n;
+	int ret, pipe = 0;
+
+	list_for_each_entry(manager, &exynos_drm_manager_list, list) {
+		if (manager->ops->initialize) {
+			ret = manager->ops->initialize(manager, dev, pipe);
+			if (ret) {
+				DRM_ERROR("Mgr init [%d] failed with %d\n",
+						manager->type, ret);
+				goto err;
+			}
+		}
+
+		manager->drm_dev = dev;
+		manager->pipe = pipe++;
+
+		ret = exynos_drm_crtc_create(manager);
+		if (ret) {
+			DRM_ERROR("CRTC create [%d] failed with %d\n",
+					manager->type, ret);
+			goto err;
+		}
+	}
+	return 0;
+
+err:
+	list_for_each_entry_safe(manager, n, &exynos_drm_manager_list, list) {
+		if (pipe-- > 0)
+			exynos_drm_manager_unregister(manager);
+		else
+			list_del(&manager->list);
+	}
+	return ret;
+}
+
+void exynos_drm_remove_managers(struct drm_device *dev)
+{
+	struct exynos_drm_manager *manager, *n;
+
+	list_for_each_entry_safe(manager, n, &exynos_drm_manager_list, list)
+		exynos_drm_manager_unregister(manager);
+}
+
+int exynos_drm_initialize_displays(struct drm_device *dev)
+{
+	struct exynos_drm_display *display, *n;
+	int ret, initialized = 0;
+
+	list_for_each_entry(display, &exynos_drm_display_list, list) {
+		if (display->ops->initialize) {
+			ret = display->ops->initialize(display, dev);
+			if (ret) {
+				DRM_ERROR("Display init [%d] failed with %d\n",
+						display->type, ret);
+				goto err;
+			}
+		}
+
+		initialized++;
+
+		ret = exynos_drm_create_enc_conn(dev, display);
+		if (ret) {
+			DRM_ERROR("Encoder create [%d] failed with %d\n",
+					display->type, ret);
+			goto err;
+		}
+	}
+	return 0;
+
+err:
+	list_for_each_entry_safe(display, n, &exynos_drm_display_list, list) {
+		if (initialized-- > 0)
+			exynos_drm_display_unregister(display);
+		else
+			list_del(&display->list);
+	}
+	return ret;
+}
+
+void exynos_drm_remove_displays(struct drm_device *dev)
+{
+	struct exynos_drm_display *display, *n;
+
+	list_for_each_entry_safe(display, n, &exynos_drm_display_list, list)
+		exynos_drm_display_unregister(display);
+}
+
 int exynos_drm_device_register(struct drm_device *dev)
 {
 	struct exynos_drm_subdrv *subdrv, *n;
-	unsigned int fine_cnt = 0;
 	int err;
 
 	if (!dev)
@@ -162,30 +242,8 @@  int exynos_drm_device_register(struct drm_device *dev)
 			list_del(&subdrv->list);
 			continue;
 		}
-
-		/*
-		 * if manager is null then it means that this sub driver
-		 * doesn't need encoder and connector.
-		 */
-		if (!subdrv->manager) {
-			fine_cnt++;
-			continue;
-		}
-
-		err = exynos_drm_create_enc_conn(dev, subdrv);
-		if (err) {
-			DRM_DEBUG("failed to create encoder and connector.\n");
-			exynos_drm_subdrv_remove(dev, subdrv);
-			list_del(&subdrv->list);
-			continue;
-		}
-
-		fine_cnt++;
 	}
 
-	if (!fine_cnt)
-		return -EINVAL;
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(exynos_drm_device_register);
@@ -201,13 +259,44 @@  int exynos_drm_device_unregister(struct drm_device *dev)
 
 	list_for_each_entry(subdrv, &exynos_drm_subdrv_list, list) {
 		exynos_drm_subdrv_remove(dev, subdrv);
-		exynos_drm_destroy_enc_conn(subdrv);
 	}
 
 	return 0;
 }
 EXPORT_SYMBOL_GPL(exynos_drm_device_unregister);
 
+int exynos_drm_manager_register(struct exynos_drm_manager *manager)
+{
+	BUG_ON(!manager->ops);
+	list_add_tail(&manager->list, &exynos_drm_manager_list);
+	return 0;
+}
+
+int exynos_drm_manager_unregister(struct exynos_drm_manager *manager)
+{
+	if (manager->ops->remove)
+		manager->ops->remove(manager);
+
+	list_del(&manager->list);
+	return 0;
+}
+
+int exynos_drm_display_register(struct exynos_drm_display *display)
+{
+	BUG_ON(!display->ops);
+	list_add_tail(&display->list, &exynos_drm_display_list);
+	return 0;
+}
+
+int exynos_drm_display_unregister(struct exynos_drm_display *display)
+{
+	if (display->ops->remove)
+		display->ops->remove(display);
+
+	list_del(&display->list);
+	return 0;
+}
+
 int exynos_drm_subdrv_register(struct exynos_drm_subdrv *subdrv)
 {
 	if (!subdrv)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index ebc0150..347d62d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -33,6 +33,7 @@  enum exynos_crtc_mode {
  *
  * @drm_crtc: crtc object.
  * @drm_plane: pointer of private plane object for this crtc
+ * @manager: the manager associated with this crtc
  * @pipe: a crtc index created at load() with a new crtc object creation
  *	and the crtc object would be set to private->crtc array
  *	to get a crtc object corresponding to this pipe from private->crtc
@@ -46,6 +47,7 @@  enum exynos_crtc_mode {
 struct exynos_drm_crtc {
 	struct drm_crtc			drm_crtc;
 	struct drm_plane		*plane;
+	struct exynos_drm_manager	*manager;
 	unsigned int			pipe;
 	unsigned int			dpms;
 	enum exynos_crtc_mode		mode;
@@ -56,6 +58,7 @@  struct exynos_drm_crtc {
 static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode)
 {
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
+	struct exynos_drm_manager *manager = exynos_crtc->manager;
 
 	DRM_DEBUG_KMS("crtc[%d] mode[%d]\n", crtc->base.id, mode);
 
@@ -71,7 +74,9 @@  static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode)
 		drm_vblank_off(crtc->dev, exynos_crtc->pipe);
 	}
 
-	exynos_drm_fn_encoder(crtc, &mode, exynos_drm_encoder_crtc_dpms);
+	if (manager->ops->dpms)
+		manager->ops->dpms(manager, mode);
+
 	exynos_crtc->dpms = mode;
 }
 
@@ -83,9 +88,15 @@  static void exynos_drm_crtc_prepare(struct drm_crtc *crtc)
 static void exynos_drm_crtc_commit(struct drm_crtc *crtc)
 {
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
+	struct exynos_drm_manager *manager = exynos_crtc->manager;
 
 	exynos_drm_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
+
 	exynos_plane_commit(exynos_crtc->plane);
+
+	if (manager->ops->commit)
+		manager->ops->commit(manager);
+
 	exynos_plane_dpms(exynos_crtc->plane, DRM_MODE_DPMS_ON);
 }
 
@@ -107,7 +118,6 @@  exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
 	struct drm_plane *plane = exynos_crtc->plane;
 	unsigned int crtc_w;
 	unsigned int crtc_h;
-	int pipe = exynos_crtc->pipe;
 	int ret;
 
 	/*
@@ -127,8 +137,6 @@  exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
 	plane->crtc = crtc;
 	plane->fb = crtc->fb;
 
-	exynos_drm_fn_encoder(crtc, &pipe, exynos_drm_encoder_crtc_pipe);
-
 	return 0;
 }
 
@@ -318,21 +326,24 @@  static void exynos_drm_crtc_attach_mode_property(struct drm_crtc *crtc)
 	drm_object_attach_property(&crtc->base, prop, 0);
 }
 
-int exynos_drm_crtc_create(struct drm_device *dev, unsigned int nr)
+int exynos_drm_crtc_create(struct exynos_drm_manager *manager)
 {
 	struct exynos_drm_crtc *exynos_crtc;
-	struct exynos_drm_private *private = dev->dev_private;
+	struct exynos_drm_private *private = manager->drm_dev->dev_private;
 	struct drm_crtc *crtc;
 
 	exynos_crtc = kzalloc(sizeof(*exynos_crtc), GFP_KERNEL);
 	if (!exynos_crtc)
 		return -ENOMEM;
 
-	exynos_crtc->pipe = nr;
-	exynos_crtc->dpms = DRM_MODE_DPMS_OFF;
 	init_waitqueue_head(&exynos_crtc->pending_flip_queue);
 	atomic_set(&exynos_crtc->pending_flip, 0);
-	exynos_crtc->plane = exynos_plane_init(dev, 1 << nr, true);
+
+	exynos_crtc->dpms = DRM_MODE_DPMS_OFF;
+	exynos_crtc->manager = manager;
+	exynos_crtc->pipe = manager->pipe;
+	exynos_crtc->plane = exynos_plane_init(manager->drm_dev,
+				1 << manager->pipe, true);
 	if (!exynos_crtc->plane) {
 		kfree(exynos_crtc);
 		return -ENOMEM;
@@ -340,9 +351,9 @@  int exynos_drm_crtc_create(struct drm_device *dev, unsigned int nr)
 
 	crtc = &exynos_crtc->drm_crtc;
 
-	private->crtc[nr] = crtc;
+	private->crtc[manager->pipe] = crtc;
 
-	drm_crtc_init(dev, crtc, &exynos_crtc_funcs);
+	drm_crtc_init(manager->drm_dev, crtc, &exynos_crtc_funcs);
 	drm_crtc_helper_add(crtc, &exynos_crtc_helper_funcs);
 
 	exynos_drm_crtc_attach_mode_property(crtc);
@@ -350,39 +361,41 @@  int exynos_drm_crtc_create(struct drm_device *dev, unsigned int nr)
 	return 0;
 }
 
-int exynos_drm_crtc_enable_vblank(struct drm_device *dev, int crtc)
+int exynos_drm_crtc_enable_vblank(struct drm_device *dev, int pipe)
 {
 	struct exynos_drm_private *private = dev->dev_private;
 	struct exynos_drm_crtc *exynos_crtc =
-		to_exynos_crtc(private->crtc[crtc]);
+		to_exynos_crtc(private->crtc[pipe]);
+	struct exynos_drm_manager *manager = exynos_crtc->manager;
 
 	if (exynos_crtc->dpms != DRM_MODE_DPMS_ON)
 		return -EPERM;
 
-	exynos_drm_fn_encoder(private->crtc[crtc], &crtc,
-			exynos_drm_enable_vblank);
+	if (manager->ops->enable_vblank)
+		manager->ops->enable_vblank(manager);
 
 	return 0;
 }
 
-void exynos_drm_crtc_disable_vblank(struct drm_device *dev, int crtc)
+void exynos_drm_crtc_disable_vblank(struct drm_device *dev, int pipe)
 {
 	struct exynos_drm_private *private = dev->dev_private;
 	struct exynos_drm_crtc *exynos_crtc =
-		to_exynos_crtc(private->crtc[crtc]);
+		to_exynos_crtc(private->crtc[pipe]);
+	struct exynos_drm_manager *manager = exynos_crtc->manager;
 
 	if (exynos_crtc->dpms != DRM_MODE_DPMS_ON)
 		return;
 
-	exynos_drm_fn_encoder(private->crtc[crtc], &crtc,
-			exynos_drm_disable_vblank);
+	if (manager->ops->disable_vblank)
+		manager->ops->disable_vblank(manager);
 }
 
-void exynos_drm_crtc_finish_pageflip(struct drm_device *dev, int crtc)
+void exynos_drm_crtc_finish_pageflip(struct drm_device *dev, int pipe)
 {
 	struct exynos_drm_private *dev_priv = dev->dev_private;
 	struct drm_pending_vblank_event *e, *t;
-	struct drm_crtc *drm_crtc = dev_priv->crtc[crtc];
+	struct drm_crtc *drm_crtc = dev_priv->crtc[pipe];
 	struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(drm_crtc);
 	unsigned long flags;
 
@@ -391,15 +404,71 @@  void exynos_drm_crtc_finish_pageflip(struct drm_device *dev, int crtc)
 	list_for_each_entry_safe(e, t, &dev_priv->pageflip_event_list,
 			base.link) {
 		/* if event's pipe isn't same as crtc then ignore it. */
-		if (crtc != e->pipe)
+		if (pipe != e->pipe)
 			continue;
 
 		list_del(&e->base.link);
 		drm_send_vblank_event(dev, -1, e);
-		drm_vblank_put(dev, crtc);
+		drm_vblank_put(dev, pipe);
 		atomic_set(&exynos_crtc->pending_flip, 0);
 		wake_up(&exynos_crtc->pending_flip_queue);
 	}
 
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 }
+
+void exynos_drm_crtc_plane_mode_set(struct drm_crtc *crtc,
+			struct exynos_drm_overlay *overlay)
+{
+	struct exynos_drm_manager *manager = to_exynos_crtc(crtc)->manager;
+
+	if (manager->ops->win_mode_set)
+		manager->ops->win_mode_set(manager, overlay);
+}
+
+void exynos_drm_crtc_plane_commit(struct drm_crtc *crtc, int zpos)
+{
+	struct exynos_drm_manager *manager = to_exynos_crtc(crtc)->manager;
+
+	if (manager->ops->win_commit)
+		manager->ops->win_commit(manager, zpos);
+}
+
+void exynos_drm_crtc_plane_enable(struct drm_crtc *crtc, int zpos)
+{
+	struct exynos_drm_manager *manager = to_exynos_crtc(crtc)->manager;
+
+	if (manager->ops->win_enable)
+		manager->ops->win_enable(manager, zpos);
+}
+
+void exynos_drm_crtc_plane_disable(struct drm_crtc *crtc, int zpos)
+{
+	struct exynos_drm_manager *manager = to_exynos_crtc(crtc)->manager;
+
+	if (manager->ops->win_disable)
+		manager->ops->win_disable(manager, zpos);
+}
+
+void exynos_drm_crtc_complete_scanout(struct drm_framebuffer *fb)
+{
+	struct exynos_drm_manager *manager;
+	struct drm_device *dev = fb->dev;
+	struct drm_crtc *crtc;
+
+	/*
+	 * make sure that overlay data are updated to real hardware
+	 * for all encoders.
+	 */
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		manager = to_exynos_crtc(crtc)->manager;
+
+		/*
+		 * wait for vblank interrupt
+		 * - this makes sure that overlay data are updated to
+		 *	real hardware.
+		 */
+		if (manager->ops->wait_for_vblank)
+			manager->ops->wait_for_vblank(manager);
+	}
+}
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.h b/drivers/gpu/drm/exynos/exynos_drm_crtc.h
index 3e197e6..c27b66c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.h
@@ -15,9 +15,21 @@ 
 #ifndef _EXYNOS_DRM_CRTC_H_
 #define _EXYNOS_DRM_CRTC_H_
 
-int exynos_drm_crtc_create(struct drm_device *dev, unsigned int nr);
-int exynos_drm_crtc_enable_vblank(struct drm_device *dev, int crtc);
-void exynos_drm_crtc_disable_vblank(struct drm_device *dev, int crtc);
-void exynos_drm_crtc_finish_pageflip(struct drm_device *dev, int crtc);
+struct drm_device;
+struct drm_crtc;
+struct exynos_drm_manager;
+struct exynos_drm_overlay;
+
+int exynos_drm_crtc_create(struct exynos_drm_manager *manager);
+int exynos_drm_crtc_enable_vblank(struct drm_device *dev, int pipe);
+void exynos_drm_crtc_disable_vblank(struct drm_device *dev, int pipe);
+void exynos_drm_crtc_finish_pageflip(struct drm_device *dev, int pipe);
+void exynos_drm_crtc_complete_scanout(struct drm_framebuffer *fb);
+
+void exynos_drm_crtc_plane_mode_set(struct drm_crtc *crtc,
+			struct exynos_drm_overlay *overlay);
+void exynos_drm_crtc_plane_commit(struct drm_crtc *crtc, int zpos);
+void exynos_drm_crtc_plane_enable(struct drm_crtc *crtc, int zpos);
+void exynos_drm_crtc_plane_disable(struct drm_crtc *crtc, int zpos);
 
 #endif
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index 617748e..250903c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -72,15 +72,9 @@  static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
 
 	exynos_drm_mode_config_init(dev);
 
-	/*
-	 * EXYNOS4 is enough to have two CRTCs and each crtc would be used
-	 * without dependency of hardware.
-	 */
-	for (nr = 0; nr < MAX_CRTC; nr++) {
-		ret = exynos_drm_crtc_create(dev, nr);
-		if (ret)
-			goto err_release_iommu_mapping;
-	}
+	ret = exynos_drm_initialize_managers(dev);
+	if (ret)
+		goto err_mode_config_cleanup;
 
 	for (nr = 0; nr < MAX_PLANE; nr++) {
 		struct drm_plane *plane;
@@ -88,12 +82,16 @@  static int exynos_drm_load(struct drm_device *dev, unsigned long flags)
 
 		plane = exynos_plane_init(dev, possible_crtcs, false);
 		if (!plane)
-			goto err_release_iommu_mapping;
+			goto err_manager_cleanup;
 	}
 
+	ret = exynos_drm_initialize_displays(dev);
+	if (ret)
+		goto err_manager_cleanup;
+
 	ret = drm_vblank_init(dev, MAX_CRTC);
 	if (ret)
-		goto err_release_iommu_mapping;
+		goto err_display_cleanup;
 
 	/*
 	 * probe sub drivers such as display controller and hdmi driver,
@@ -125,7 +123,12 @@  err_drm_device:
 	exynos_drm_device_unregister(dev);
 err_vblank:
 	drm_vblank_cleanup(dev);
-err_release_iommu_mapping:
+err_display_cleanup:
+	exynos_drm_remove_displays(dev);
+err_manager_cleanup:
+	exynos_drm_remove_managers(dev);
+err_mode_config_cleanup:
+	drm_mode_config_cleanup(dev);
 	drm_release_iommu_mapping(dev);
 err_crtc:
 	drm_mode_config_cleanup(dev);
@@ -140,6 +143,8 @@  static int exynos_drm_unload(struct drm_device *dev)
 	exynos_drm_device_unregister(dev);
 	drm_vblank_cleanup(dev);
 	drm_kms_helper_poll_fini(dev);
+	exynos_drm_remove_displays(dev);
+	exynos_drm_remove_managers(dev);
 	drm_mode_config_cleanup(dev);
 
 	drm_release_iommu_mapping(dev);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index e31c088..dc730e5 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -122,34 +122,68 @@  struct exynos_drm_overlay {
  * Exynos DRM Display Structure.
  *	- this structure is common to analog tv, digital tv and lcd panel.
  *
- * @type: one of EXYNOS_DISPLAY_TYPE_LCD and HDMI.
  * @initialize: initializes the display with drm_dev
+ * @remove: cleans up the display for removal
  * @is_connected: check for that display is connected or not.
+ * @get_max_resol: get maximum resolution to specific hardware.
  * @get_edid: get edid modes from display driver.
  * @get_panel: get panel object from display driver.
+ * @mode_fixup: fix mode data comparing to hw specific display mode.
+ * @mode_set: convert drm_display_mode to hw specific display mode and
+ *	      would be called by encoder->mode_set().
  * @check_mode: check if mode is valid or not.
  * @dpms: display device on or off.
+ * @commit: apply changes to hw
  */
+struct exynos_drm_display;
 struct exynos_drm_display_ops {
+	int (*initialize)(struct exynos_drm_display *display,
+				struct drm_device *drm_dev);
+	void (*remove)(struct exynos_drm_display *display);
+	bool (*is_connected)(struct exynos_drm_display *display);
+	void (*get_max_resol)(struct exynos_drm_display *display,
+				unsigned int *width,
+				unsigned int *height);
+	struct edid *(*get_edid)(struct exynos_drm_display *display,
+				struct drm_connector *connector);
+	void *(*get_panel)(struct exynos_drm_display *display);
+	void (*mode_fixup)(struct exynos_drm_display *display,
+				struct drm_connector *connector,
+				const struct drm_display_mode *mode,
+				struct drm_display_mode *adjusted_mode);
+	void (*mode_set)(struct exynos_drm_display *display,
+				struct drm_display_mode *mode);
+	int (*check_mode)(struct exynos_drm_display *display,
+				struct drm_display_mode *mode);
+	void (*dpms)(struct exynos_drm_display *display, int mode);
+	void (*commit)(struct exynos_drm_display *display);
+};
+
+/*
+ * Exynos drm display structure, maps 1:1 with an encoder/connector
+ *
+ * @list: the list entry for this manager
+ * @type: one of EXYNOS_DISPLAY_TYPE_LCD and HDMI.
+ * @encoder: encoder object this display maps to
+ * @connector: connector object this display maps to
+ * @ops: pointer to callbacks for exynos drm specific functionality
+ * @ctx: A pointer to the display's implementation specific context
+ */
+struct exynos_drm_display {
+	struct list_head list;
 	enum exynos_drm_output_type type;
-	int (*initialize)(struct device *dev, struct drm_device *drm_dev);
-	bool (*is_connected)(struct device *dev);
-	struct edid *(*get_edid)(struct device *dev,
-			struct drm_connector *connector);
-	void *(*get_panel)(struct device *dev);
-	int (*check_mode)(struct device *dev, struct drm_display_mode *mode);
-	int (*dpms)(struct device *dev, int mode);
+	struct drm_encoder *encoder;
+	struct drm_connector *connector;
+	struct exynos_drm_display_ops *ops;
+	void *ctx;
 };
 
 /*
  * Exynos drm manager ops
  *
  * @initialize: initializes the manager with drm_dev
+ * @remove: cleans up the manager for removal
  * @dpms: control device power.
- * @mode_fixup: fix mode data comparing to hw specific display mode.
- * @mode_set: convert drm_display_mode to hw specific display mode and
- *	      would be called by encoder->mode_set().
- * @get_max_resol: get maximum resolution to specific hardware.
  * @commit: set current hw specific display mode to hw.
  * @enable_vblank: specific driver callback for enabling vblank interrupt.
  * @disable_vblank: specific driver callback for disabling vblank interrupt.
@@ -163,15 +197,9 @@  struct exynos_drm_display_ops {
 struct exynos_drm_manager;
 struct exynos_drm_manager_ops {
 	int (*initialize)(struct exynos_drm_manager *mgr,
-				struct drm_device *drm_dev);
+				struct drm_device *drm_dev, int pipe);
+	void (*remove)(struct exynos_drm_manager *mgr);
 	void (*dpms)(struct exynos_drm_manager *mgr, int mode);
-	void (*mode_fixup)(struct exynos_drm_manager *mgr,
-				struct drm_connector *connector,
-				const struct drm_display_mode *mode,
-				struct drm_display_mode *adjusted_mode);
-	void (*mode_set)(struct exynos_drm_manager *mgr, void *mode);
-	void (*get_max_resol)(struct exynos_drm_manager *mgr,
-				unsigned int *width, unsigned int *height);
 	void (*commit)(struct exynos_drm_manager *mgr);
 	int (*enable_vblank)(struct exynos_drm_manager *mgr);
 	void (*disable_vblank)(struct exynos_drm_manager *mgr);
@@ -184,25 +212,21 @@  struct exynos_drm_manager_ops {
 };
 
 /*
- * Exynos drm common manager structure.
+ * Exynos drm common manager structure, maps 1:1 with a crtc
  *
- * @dev: pointer to device object for subdrv device driver.
- *	sub drivers such as display controller or hdmi driver,
- *	have their own device object.
- * @ops: pointer to callbacks for exynos drm specific framebuffer.
- *	these callbacks should be set by specific drivers such fimd
- *	or hdmi driver and are used to control hardware global registers.
- * @display: pointer to callbacks for exynos drm specific framebuffer.
- *	these callbacks should be set by specific drivers such fimd
- *	or hdmi driver and are used to control display devices such as
- *	analog tv, digital tv and lcd panel and also get timing data for them.
+ * @list: the list entry for this manager
+ * @type: one of EXYNOS_DISPLAY_TYPE_LCD and HDMI.
+ * @drm_dev: pointer to the drm device
+ * @pipe: the pipe number for this crtc/manager
+ * @ops: pointer to callbacks for exynos drm specific functionality
  * @ctx: A pointer to the manager's implementation specific context
  */
 struct exynos_drm_manager {
-	struct device *dev;
+	struct list_head list;
+	enum exynos_drm_output_type type;
+	struct drm_device *drm_dev;
 	int pipe;
 	struct exynos_drm_manager_ops *ops;
-	struct exynos_drm_display_ops *display_ops;
 	void *ctx;
 };
 
@@ -267,14 +291,11 @@  struct exynos_drm_private {
  *	by probe callback.
  * @open: this would be called with drm device file open.
  * @close: this would be called with drm device file close.
- * @encoder: encoder object owned by this sub driver.
- * @connector: connector object owned by this sub driver.
  */
 struct exynos_drm_subdrv {
 	struct list_head list;
 	struct device *dev;
 	struct drm_device *drm_dev;
-	struct exynos_drm_manager *manager;
 
 	int (*probe)(struct drm_device *drm_dev, struct device *dev);
 	void (*remove)(struct drm_device *drm_dev, struct device *dev);
@@ -282,9 +303,6 @@  struct exynos_drm_subdrv {
 			struct drm_file *file);
 	void (*close)(struct drm_device *drm_dev, struct device *dev,
 			struct drm_file *file);
-
-	struct drm_encoder *encoder;
-	struct drm_connector *connector;
 };
 
 /*
@@ -299,6 +317,16 @@  int exynos_drm_device_register(struct drm_device *dev);
  */
 int exynos_drm_device_unregister(struct drm_device *dev);
 
+int exynos_drm_initialize_managers(struct drm_device *dev);
+void exynos_drm_remove_managers(struct drm_device *dev);
+int exynos_drm_initialize_displays(struct drm_device *dev);
+void exynos_drm_remove_displays(struct drm_device *dev);
+
+int exynos_drm_manager_register(struct exynos_drm_manager *manager);
+int exynos_drm_manager_unregister(struct exynos_drm_manager *manager);
+int exynos_drm_display_register(struct exynos_drm_display *display);
+int exynos_drm_display_unregister(struct exynos_drm_display *display);
+
 /*
  * this function would be called by sub drivers such as display controller
  * or hdmi driver to register this sub driver object to exynos drm driver
diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.c b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
index efe4e60..d4ae664 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_encoder.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.c
@@ -26,24 +26,23 @@ 
  * exynos specific encoder structure.
  *
  * @drm_encoder: encoder object.
- * @manager: specific encoder has its own manager to control a hardware
- *	appropriately and we can access a hardware drawing on this manager.
+ * @display: the display structure that maps to this encoder
  */
 struct exynos_drm_encoder {
 	struct drm_crtc			*old_crtc;
 	struct drm_encoder		drm_encoder;
-	struct exynos_drm_manager	*manager;
+	struct exynos_drm_display	*display;
 };
 
 static void exynos_drm_encoder_dpms(struct drm_encoder *encoder, int mode)
 {
-	struct exynos_drm_manager *manager = exynos_drm_get_manager(encoder);
-	struct exynos_drm_display_ops *display_ops = manager->display_ops;
+	struct exynos_drm_encoder *exynos_encoder = to_exynos_encoder(encoder);
+	struct exynos_drm_display *display = exynos_encoder->display;
 
 	DRM_DEBUG_KMS("encoder dpms: %d\n", mode);
 
-	if (display_ops && display_ops->dpms)
-		display_ops->dpms(manager->ctx, mode);
+	if (display->ops->dpms)
+		display->ops->dpms(display, mode);
 }
 
 static bool
@@ -52,15 +51,17 @@  exynos_drm_encoder_mode_fixup(struct drm_encoder *encoder,
 			       struct drm_display_mode *adjusted_mode)
 {
 	struct drm_device *dev = encoder->dev;
+	struct exynos_drm_encoder *exynos_encoder = to_exynos_encoder(encoder);
+	struct exynos_drm_display *display = exynos_encoder->display;
 	struct drm_connector *connector;
-	struct exynos_drm_manager *manager = exynos_drm_get_manager(encoder);
-	struct exynos_drm_manager_ops *manager_ops = manager->ops;
 
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		if (connector->encoder == encoder)
-			if (manager_ops && manager_ops->mode_fixup)
-				manager_ops->mode_fixup(manager, connector,
-							mode, adjusted_mode);
+		if (connector->encoder != encoder)
+			continue;
+
+		if (display->ops->mode_fixup)
+			display->ops->mode_fixup(display, connector, mode,
+					adjusted_mode);
 	}
 
 	return true;
@@ -102,8 +103,7 @@  static void exynos_drm_encoder_mode_set(struct drm_encoder *encoder,
 {
 	struct drm_device *dev = encoder->dev;
 	struct drm_connector *connector;
-	struct exynos_drm_manager *manager;
-	struct exynos_drm_manager_ops *manager_ops;
+	struct exynos_drm_display *display;
 
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
 		if (connector->encoder == encoder) {
@@ -123,11 +123,11 @@  static void exynos_drm_encoder_mode_set(struct drm_encoder *encoder,
 						encoder->crtc);
 			}
 
-			manager = exynos_drm_get_manager(encoder);
-			manager_ops = manager->ops;
+			display = exynos_encoder->display;
 
-			if (manager_ops && manager_ops->mode_set)
-				manager_ops->mode_set(manager, adjusted_mode);
+			if (display->ops->mode_set)
+				display->ops->mode_set(display,
+							adjusted_mode);
 
 			exynos_encoder->old_crtc = encoder->crtc;
 		}
@@ -142,39 +142,15 @@  static void exynos_drm_encoder_prepare(struct drm_encoder *encoder)
 static void exynos_drm_encoder_commit(struct drm_encoder *encoder)
 {
 	struct exynos_drm_encoder *exynos_encoder = to_exynos_encoder(encoder);
-	struct exynos_drm_manager *manager = exynos_encoder->manager;
-	struct exynos_drm_manager_ops *manager_ops = manager->ops;
-
-	if (manager_ops && manager_ops->commit)
-		manager_ops->commit(manager);
-}
+	struct exynos_drm_display *display = exynos_encoder->display;
 
-void exynos_drm_encoder_complete_scanout(struct drm_framebuffer *fb)
-{
-	struct exynos_drm_encoder *exynos_encoder;
-	struct exynos_drm_manager_ops *ops;
-	struct drm_device *dev = fb->dev;
-	struct drm_encoder *encoder;
+	if (display->ops->dpms)
+		display->ops->dpms(display, DRM_MODE_DPMS_ON);
 
-	/*
-	 * make sure that overlay data are updated to real hardware
-	 * for all encoders.
-	 */
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
-		exynos_encoder = to_exynos_encoder(encoder);
-		ops = exynos_encoder->manager->ops;
-
-		/*
-		 * wait for vblank interrupt
-		 * - this makes sure that overlay data are updated to
-		 *	real hardware.
-		 */
-		if (ops->wait_for_vblank)
-			ops->wait_for_vblank(exynos_encoder->manager);
-	}
+	if (display->ops->commit)
+		display->ops->commit(display);
 }
 
-
 static void exynos_drm_encoder_disable(struct drm_encoder *encoder)
 {
 	struct drm_plane *plane;
@@ -200,10 +176,7 @@  static struct drm_encoder_helper_funcs exynos_encoder_helper_funcs = {
 
 static void exynos_drm_encoder_destroy(struct drm_encoder *encoder)
 {
-	struct exynos_drm_encoder *exynos_encoder =
-		to_exynos_encoder(encoder);
-
-	exynos_encoder->manager->pipe = -1;
+	struct exynos_drm_encoder *exynos_encoder = to_exynos_encoder(encoder);
 
 	drm_encoder_cleanup(encoder);
 	kfree(exynos_encoder);
@@ -218,13 +191,12 @@  static unsigned int exynos_drm_encoder_clones(struct drm_encoder *encoder)
 	struct drm_encoder *clone;
 	struct drm_device *dev = encoder->dev;
 	struct exynos_drm_encoder *exynos_encoder = to_exynos_encoder(encoder);
-	struct exynos_drm_display_ops *display_ops =
-				exynos_encoder->manager->display_ops;
+	struct exynos_drm_display *display = exynos_encoder->display;
 	unsigned int clone_mask = 0;
 	int cnt = 0;
 
 	list_for_each_entry(clone, &dev->mode_config.encoder_list, head) {
-		switch (display_ops->type) {
+		switch (display->type) {
 		case EXYNOS_DISPLAY_TYPE_LCD:
 		case EXYNOS_DISPLAY_TYPE_HDMI:
 		case EXYNOS_DISPLAY_TYPE_VIDI:
@@ -248,24 +220,20 @@  void exynos_drm_encoder_setup(struct drm_device *dev)
 
 struct drm_encoder *
 exynos_drm_encoder_create(struct drm_device *dev,
-			   struct exynos_drm_manager *manager,
+			   struct exynos_drm_display *display,
 			   unsigned long possible_crtcs)
 {
 	struct drm_encoder *encoder;
 	struct exynos_drm_encoder *exynos_encoder;
-	int ret;
 
-	if (!manager || !possible_crtcs)
-		return NULL;
-
-	if (!manager->dev)
+	if (!possible_crtcs)
 		return NULL;
 
 	exynos_encoder = kzalloc(sizeof(*exynos_encoder), GFP_KERNEL);
 	if (!exynos_encoder)
 		return NULL;
 
-	exynos_encoder->manager = manager;
+	exynos_encoder->display = display;
 	encoder = &exynos_encoder->drm_encoder;
 	encoder->possible_crtcs = possible_crtcs;
 
@@ -276,174 +244,12 @@  exynos_drm_encoder_create(struct drm_device *dev,
 
 	drm_encoder_helper_add(encoder, &exynos_encoder_helper_funcs);
 
-	if (manager->ops && manager->ops->initialize) {
-		ret = manager->ops->initialize(manager, dev);
-		if (ret) {
-			DRM_ERROR("Manager initialize failed %d\n", ret);
-			goto error;
-		}
-	}
-
-	if (manager->display_ops && manager->display_ops->initialize) {
-		ret = manager->display_ops->initialize(manager->dev, dev);
-		if (ret) {
-			DRM_ERROR("Display initialize failed %d\n", ret);
-			goto error;
-		}
-	}
-
 	DRM_DEBUG_KMS("encoder has been created\n");
 
 	return encoder;
-
-error:
-	exynos_drm_encoder_destroy(&exynos_encoder->drm_encoder);
-	return NULL;
 }
 
-struct exynos_drm_manager *exynos_drm_get_manager(struct drm_encoder *encoder)
+struct exynos_drm_display *exynos_drm_get_display(struct drm_encoder *encoder)
 {
-	return to_exynos_encoder(encoder)->manager;
-}
-
-void exynos_drm_fn_encoder(struct drm_crtc *crtc, void *data,
-			    void (*fn)(struct drm_encoder *, void *))
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_encoder *encoder;
-	struct exynos_drm_private *private = dev->dev_private;
-	struct exynos_drm_manager *manager;
-
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
-		/*
-		 * if crtc is detached from encoder, check pipe,
-		 * otherwise check crtc attached to encoder
-		 */
-		if (!encoder->crtc) {
-			manager = to_exynos_encoder(encoder)->manager;
-			if (manager->pipe < 0 ||
-					private->crtc[manager->pipe] != crtc)
-				continue;
-		} else {
-			if (encoder->crtc != crtc)
-				continue;
-		}
-
-		fn(encoder, data);
-	}
-}
-
-void exynos_drm_enable_vblank(struct drm_encoder *encoder, void *data)
-{
-	struct exynos_drm_manager *manager =
-		to_exynos_encoder(encoder)->manager;
-	struct exynos_drm_manager_ops *manager_ops = manager->ops;
-	int crtc = *(int *)data;
-
-	if (manager->pipe != crtc)
-		return;
-
-	if (manager_ops->enable_vblank)
-		manager_ops->enable_vblank(manager);
-}
-
-void exynos_drm_disable_vblank(struct drm_encoder *encoder, void *data)
-{
-	struct exynos_drm_manager *manager =
-		to_exynos_encoder(encoder)->manager;
-	struct exynos_drm_manager_ops *manager_ops = manager->ops;
-	int crtc = *(int *)data;
-
-	if (manager->pipe != crtc)
-		return;
-
-	if (manager_ops->disable_vblank)
-		manager_ops->disable_vblank(manager);
-}
-
-void exynos_drm_encoder_crtc_dpms(struct drm_encoder *encoder, void *data)
-{
-	struct exynos_drm_encoder *exynos_encoder = to_exynos_encoder(encoder);
-	struct exynos_drm_manager *manager = exynos_encoder->manager;
-	struct exynos_drm_manager_ops *manager_ops = manager->ops;
-	int mode = *(int *)data;
-
-	if (manager_ops && manager_ops->dpms)
-		manager_ops->dpms(manager, mode);
-
-	/*
-	 * if this condition is ok then it means that the crtc is already
-	 * detached from encoder and last function for detaching is properly
-	 * done, so clear pipe from manager to prevent repeated call.
-	 */
-	if (mode > DRM_MODE_DPMS_ON) {
-		if (!encoder->crtc)
-			manager->pipe = -1;
-	}
-}
-
-void exynos_drm_encoder_crtc_pipe(struct drm_encoder *encoder, void *data)
-{
-	struct exynos_drm_manager *manager =
-		to_exynos_encoder(encoder)->manager;
-	int pipe = *(int *)data;
-
-	/*
-	 * when crtc is detached from encoder, this pipe is used
-	 * to select manager operation
-	 */
-	manager->pipe = pipe;
-}
-
-void exynos_drm_encoder_plane_mode_set(struct drm_encoder *encoder, void *data)
-{
-	struct exynos_drm_manager *manager =
-		to_exynos_encoder(encoder)->manager;
-	struct exynos_drm_manager_ops *manager_ops = manager->ops;
-	struct exynos_drm_overlay *overlay = data;
-
-	if (manager_ops && manager_ops->win_mode_set)
-		manager_ops->win_mode_set(manager, overlay);
-}
-
-void exynos_drm_encoder_plane_commit(struct drm_encoder *encoder, void *data)
-{
-	struct exynos_drm_manager *manager =
-		to_exynos_encoder(encoder)->manager;
-	struct exynos_drm_manager_ops *manager_ops = manager->ops;
-	int zpos = DEFAULT_ZPOS;
-
-	if (data)
-		zpos = *(int *)data;
-
-	if (manager_ops && manager_ops->win_commit)
-		manager_ops->win_commit(manager, zpos);
-}
-
-void exynos_drm_encoder_plane_enable(struct drm_encoder *encoder, void *data)
-{
-	struct exynos_drm_manager *manager =
-		to_exynos_encoder(encoder)->manager;
-	struct exynos_drm_manager_ops *manager_ops = manager->ops;
-	int zpos = DEFAULT_ZPOS;
-
-	if (data)
-		zpos = *(int *)data;
-
-	if (manager_ops && manager_ops->win_enable)
-		manager_ops->win_enable(manager, zpos);
-}
-
-void exynos_drm_encoder_plane_disable(struct drm_encoder *encoder, void *data)
-{
-	struct exynos_drm_manager *manager =
-		to_exynos_encoder(encoder)->manager;
-	struct exynos_drm_manager_ops *manager_ops = manager->ops;
-	int zpos = DEFAULT_ZPOS;
-
-	if (data)
-		zpos = *(int *)data;
-
-	if (manager_ops && manager_ops->win_disable)
-		manager_ops->win_disable(manager, zpos);
+	return to_exynos_encoder(encoder)->display;
 }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_encoder.h b/drivers/gpu/drm/exynos/exynos_drm_encoder.h
index 0f3e5e2..b7a1620 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_encoder.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_encoder.h
@@ -18,20 +18,8 @@  struct exynos_drm_manager;
 
 void exynos_drm_encoder_setup(struct drm_device *dev);
 struct drm_encoder *exynos_drm_encoder_create(struct drm_device *dev,
-					       struct exynos_drm_manager *mgr,
-					       unsigned long possible_crtcs);
-struct exynos_drm_manager *
-exynos_drm_get_manager(struct drm_encoder *encoder);
-void exynos_drm_fn_encoder(struct drm_crtc *crtc, void *data,
-			    void (*fn)(struct drm_encoder *, void *));
-void exynos_drm_enable_vblank(struct drm_encoder *encoder, void *data);
-void exynos_drm_disable_vblank(struct drm_encoder *encoder, void *data);
-void exynos_drm_encoder_crtc_dpms(struct drm_encoder *encoder, void *data);
-void exynos_drm_encoder_crtc_pipe(struct drm_encoder *encoder, void *data);
-void exynos_drm_encoder_plane_mode_set(struct drm_encoder *encoder, void *data);
-void exynos_drm_encoder_plane_commit(struct drm_encoder *encoder, void *data);
-void exynos_drm_encoder_plane_enable(struct drm_encoder *encoder, void *data);
-void exynos_drm_encoder_plane_disable(struct drm_encoder *encoder, void *data);
-void exynos_drm_encoder_complete_scanout(struct drm_framebuffer *fb);
+			struct exynos_drm_display *mgr,
+			unsigned long possible_crtcs);
+struct exynos_drm_display *exynos_drm_get_display(struct drm_encoder *encoder);
 
 #endif
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index ea39e0e..c7c08d0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -22,7 +22,7 @@ 
 #include "exynos_drm_fb.h"
 #include "exynos_drm_gem.h"
 #include "exynos_drm_iommu.h"
-#include "exynos_drm_encoder.h"
+#include "exynos_drm_crtc.h"
 
 #define to_exynos_fb(x)	container_of(x, struct exynos_drm_fb, fb)
 
@@ -71,7 +71,7 @@  static void exynos_drm_fb_destroy(struct drm_framebuffer *fb)
 	unsigned int i;
 
 	/* make sure that overlay data are updated before relesing fb. */
-	exynos_drm_encoder_complete_scanout(fb);
+	exynos_drm_crtc_complete_scanout(fb);
 
 	drm_framebuffer_cleanup(fb);
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index b980b05..d2b8ccb 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -105,7 +105,6 @@  struct fimd_win_data {
 };
 
 struct fimd_context {
-	struct exynos_drm_subdrv	subdrv;
 	struct device			*dev;
 	struct drm_device		*drm_dev;
 	int				irq;
@@ -120,6 +119,7 @@  struct fimd_context {
 	u32				vidcon0;
 	u32				vidcon1;
 	bool				suspended;
+	int				pipe;
 	struct mutex			lock;
 	wait_queue_head_t		wait_vsync_queue;
 	atomic_t			wait_vsync_event;
@@ -147,22 +147,22 @@  static inline struct fimd_driver_data *drm_fimd_get_driver_data(
 	return (struct fimd_driver_data *)of_id->data;
 }
 
-static bool fimd_display_is_connected(struct device *dev)
+static bool fimd_display_is_connected(struct exynos_drm_display *display)
 {
 	/* TODO. */
 
 	return true;
 }
 
-static void *fimd_get_panel(struct device *dev)
+static void *fimd_get_panel(struct exynos_drm_display *display)
 {
-	struct exynos_drm_manager *mgr = get_fimd_manager(dev);
-	struct fimd_context *ctx = mgr->ctx;
+	struct fimd_context *ctx = display->ctx;
 
 	return &ctx->panel;
 }
 
-static int fimd_check_mode(struct device *dev, struct drm_display_mode *mode)
+static int fimd_check_mode(struct exynos_drm_display *display,
+			struct drm_display_mode *mode)
 {
 	/* TODO. */
 
@@ -170,12 +170,16 @@  static int fimd_check_mode(struct device *dev, struct drm_display_mode *mode)
 }
 
 static struct exynos_drm_display_ops fimd_display_ops = {
-	.type = EXYNOS_DISPLAY_TYPE_LCD,
 	.is_connected = fimd_display_is_connected,
 	.get_panel = fimd_get_panel,
 	.check_mode = fimd_check_mode,
 };
 
+static struct exynos_drm_display fimd_display = {
+	.type = EXYNOS_DISPLAY_TYPE_LCD,
+	.ops = &fimd_display_ops,
+};
+
 static void fimd_win_mode_set(struct exynos_drm_manager *mgr,
 			struct exynos_drm_overlay *overlay)
 {
@@ -484,15 +488,45 @@  static void fimd_win_disable(struct exynos_drm_manager *mgr, int zpos)
 }
 
 static int fimd_mgr_initialize(struct exynos_drm_manager *mgr,
-			struct drm_device *drm_dev)
+			struct drm_device *drm_dev, int pipe)
 {
 	struct fimd_context *ctx = mgr->ctx;
 
 	ctx->drm_dev = drm_dev;
+	ctx->pipe = pipe;
+
+	/*
+	 * enable drm irq mode.
+	 * - with irq_enabled = 1, we can use the vblank feature.
+	 *
+	 * P.S. note that we wouldn't use drm irq handler but
+	 *	just specific driver own one instead because
+	 *	drm framework supports only one irq handler.
+	 */
+	ctx->drm_dev->irq_enabled = 1;
+
+	/*
+	 * with vblank_disable_allowed = 1, vblank interrupt will be disabled
+	 * by drm timer once a current process gives up ownership of
+	 * vblank event.(after drm_vblank_put function is called)
+	 */
+	drm_dev->vblank_disable_allowed = 1;
+
+	/* attach this sub driver to iommu mapping if supported. */
+	if (is_drm_iommu_supported(ctx->drm_dev))
+		drm_iommu_attach_device(ctx->drm_dev, ctx->dev);
 
 	return 0;
 }
 
+static void fimd_mgr_remove(struct exynos_drm_manager *mgr)
+{
+	struct fimd_context *ctx = mgr->ctx;
+
+	if (is_drm_iommu_supported(ctx->drm_dev))
+		drm_iommu_detach_device(ctx->drm_dev, ctx->dev);
+}
+
 static void fimd_dpms(struct exynos_drm_manager *mgr, int mode)
 {
 	struct fimd_context *ctx = mgr->ctx;
@@ -526,23 +560,6 @@  static void fimd_dpms(struct exynos_drm_manager *mgr, int mode)
 	mutex_unlock(&ctx->lock);
 }
 
-static void fimd_apply(struct exynos_drm_manager *mgr)
-{
-	struct fimd_context *ctx = mgr->ctx;
-	struct exynos_drm_manager_ops *mgr_ops = mgr->ops;
-	struct fimd_win_data *win_data;
-	int i;
-
-	for (i = 0; i < WINDOWS_NR; i++) {
-		win_data = &ctx->win_data[i];
-		if (win_data->enabled && (mgr_ops && mgr_ops->win_commit))
-			mgr_ops->win_commit(mgr, i);
-	}
-
-	if (mgr_ops && mgr_ops->commit)
-		mgr_ops->commit(mgr);
-}
-
 static void fimd_commit(struct exynos_drm_manager *mgr)
 {
 	struct fimd_context *ctx = mgr->ctx;
@@ -599,6 +616,21 @@  static void fimd_commit(struct exynos_drm_manager *mgr)
 	writel(val, ctx->regs + VIDCON0);
 }
 
+static void fimd_apply(struct exynos_drm_manager *mgr)
+{
+	struct fimd_context *ctx = mgr->ctx;
+	struct fimd_win_data *win_data;
+	int i;
+
+	for (i = 0; i < WINDOWS_NR; i++) {
+		win_data = &ctx->win_data[i];
+		if (win_data->enabled)
+			fimd_win_commit(mgr, i);
+	}
+
+	fimd_commit(mgr);
+}
+
 static int fimd_enable_vblank(struct exynos_drm_manager *mgr)
 {
 	struct fimd_context *ctx = mgr->ctx;
@@ -663,6 +695,7 @@  static void fimd_wait_for_vblank(struct exynos_drm_manager *mgr)
 
 static struct exynos_drm_manager_ops fimd_manager_ops = {
 	.initialize = fimd_mgr_initialize,
+	.remove = fimd_mgr_remove,
 	.dpms = fimd_dpms,
 	.commit = fimd_commit,
 	.enable_vblank = fimd_enable_vblank,
@@ -674,16 +707,13 @@  static struct exynos_drm_manager_ops fimd_manager_ops = {
 };
 
 static struct exynos_drm_manager fimd_manager = {
-	.pipe		= -1,
-	.ops		= &fimd_manager_ops,
-	.display_ops	= &fimd_display_ops,
+	.type = EXYNOS_DISPLAY_TYPE_LCD,
+	.ops = &fimd_manager_ops,
 };
 
 static irqreturn_t fimd_irq_handler(int irq, void *dev_id)
 {
 	struct fimd_context *ctx = (struct fimd_context *)dev_id;
-	struct exynos_drm_subdrv *subdrv = &ctx->subdrv;
-	struct exynos_drm_manager *manager = subdrv->manager;
 	u32 val;
 
 	val = readl(ctx->regs + VIDINTCON1);
@@ -693,11 +723,11 @@  static irqreturn_t fimd_irq_handler(int irq, void *dev_id)
 		writel(VIDINTCON1_INT_FRAME, ctx->regs + VIDINTCON1);
 
 	/* check the crtc is detached already from encoder */
-	if (manager->pipe < 0 || !ctx->drm_dev)
+	if (ctx->pipe < 0 || !ctx->drm_dev)
 		goto out;
 
-	drm_handle_vblank(ctx->drm_dev, manager->pipe);
-	exynos_drm_crtc_finish_pageflip(ctx->drm_dev, manager->pipe);
+	drm_handle_vblank(ctx->drm_dev, ctx->pipe);
+	exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
 
 	/* set wait vsync event to zero and wake up queue. */
 	if (atomic_read(&ctx->wait_vsync_event)) {
@@ -708,39 +738,6 @@  out:
 	return IRQ_HANDLED;
 }
 
-static int fimd_subdrv_probe(struct drm_device *drm_dev, struct device *dev)
-{
-	/*
-	 * enable drm irq mode.
-	 * - with irq_enabled = 1, we can use the vblank feature.
-	 *
-	 * P.S. note that we wouldn't use drm irq handler but
-	 *	just specific driver own one instead because
-	 *	drm framework supports only one irq handler.
-	 */
-	drm_dev->irq_enabled = 1;
-
-	/*
-	 * with vblank_disable_allowed = 1, vblank interrupt will be disabled
-	 * by drm timer once a current process gives up ownership of
-	 * vblank event.(after drm_vblank_put function is called)
-	 */
-	drm_dev->vblank_disable_allowed = 1;
-
-	/* attach this sub driver to iommu mapping if supported. */
-	if (is_drm_iommu_supported(drm_dev))
-		drm_iommu_attach_device(drm_dev, dev);
-
-	return 0;
-}
-
-static void fimd_subdrv_remove(struct drm_device *drm_dev, struct device *dev)
-{
-	/* detach this sub driver from iommu mapping if supported. */
-	if (is_drm_iommu_supported(drm_dev))
-		drm_iommu_detach_device(drm_dev, dev);
-}
-
 static int fimd_configure_clocks(struct fimd_context *ctx, struct device *dev)
 {
 	struct videomode *vm = &ctx->panel.vm;
@@ -826,9 +823,8 @@  static int fimd_clock(struct fimd_context *ctx, bool enable)
 	return 0;
 }
 
-static void fimd_window_suspend(struct device *dev)
+static void fimd_window_suspend(struct exynos_drm_manager *mgr)
 {
-	struct exynos_drm_manager *mgr = get_fimd_manager(dev);
 	struct fimd_context *ctx = mgr->ctx;
 	struct fimd_win_data *win_data;
 	int i;
@@ -841,9 +837,8 @@  static void fimd_window_suspend(struct device *dev)
 	fimd_wait_for_vblank(mgr);
 }
 
-static void fimd_window_resume(struct device *dev)
+static void fimd_window_resume(struct exynos_drm_manager *mgr)
 {
-	struct exynos_drm_manager *mgr = get_fimd_manager(dev);
 	struct fimd_context *ctx = mgr->ctx;
 	struct fimd_win_data *win_data;
 	int i;
@@ -858,7 +853,6 @@  static void fimd_window_resume(struct device *dev)
 static int fimd_activate(struct exynos_drm_manager *mgr, bool enable)
 {
 	struct fimd_context *ctx = mgr->ctx;
-	struct device *dev = ctx->subdrv.dev;
 
 	if (enable) {
 		int ret;
@@ -873,11 +867,11 @@  static int fimd_activate(struct exynos_drm_manager *mgr, bool enable)
 		if (test_and_clear_bit(0, &ctx->irq_flags))
 			fimd_enable_vblank(mgr);
 
-		fimd_window_resume(dev);
+		fimd_window_resume(mgr);
 
 		fimd_apply(mgr);
 	} else {
-		fimd_window_suspend(dev);
+		fimd_window_suspend(mgr);
 
 		fimd_clock(ctx, false);
 		ctx->suspended = true;
@@ -914,7 +908,6 @@  static int fimd_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct fimd_context *ctx;
-	struct exynos_drm_subdrv *subdrv;
 	struct resource *res;
 	int win;
 	int ret = -EINVAL;
@@ -961,27 +954,22 @@  static int fimd_probe(struct platform_device *pdev)
 	DRM_INIT_WAITQUEUE(&ctx->wait_vsync_queue);
 	atomic_set(&ctx->wait_vsync_event, 0);
 
-	fimd_manager.ctx = ctx;
-
-	subdrv = &ctx->subdrv;
-
-	subdrv->dev = dev;
-	subdrv->manager = &fimd_manager;
-	subdrv->probe = fimd_subdrv_probe;
-	subdrv->remove = fimd_subdrv_remove;
-
 	mutex_init(&ctx->lock);
 
 	platform_set_drvdata(pdev, &fimd_manager);
 
+	fimd_manager.ctx = ctx;
+	exynos_drm_manager_register(&fimd_manager);
+
+	fimd_display.ctx = ctx;
+	exynos_drm_display_register(&fimd_display);
+
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 
 	for (win = 0; win < WINDOWS_NR; win++)
 		fimd_clear_win(ctx, win);
 
-	exynos_drm_subdrv_register(subdrv);
-
 	return 0;
 }
 
@@ -991,7 +979,8 @@  static int fimd_remove(struct platform_device *pdev)
 	struct exynos_drm_manager *mgr = platform_get_drvdata(pdev);
 	struct fimd_context *ctx = mgr->ctx;
 
-	exynos_drm_subdrv_unregister(&ctx->subdrv);
+	exynos_drm_display_unregister(&fimd_display);
+	exynos_drm_manager_unregister(&fimd_manager);
 
 	if (ctx->suspended)
 		goto out;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
index f9a9324..b0b09b2 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
@@ -23,11 +23,6 @@ 
 #include "exynos_drm_drv.h"
 #include "exynos_drm_hdmi.h"
 
-#define to_context(dev)		platform_get_drvdata(to_platform_device(dev))
-#define to_subdrv(dev)		to_context(dev)
-#define get_ctx_from_subdrv(subdrv)	container_of(subdrv,\
-					struct drm_hdmi_context, subdrv);
-
 /* platform device pointer for common drm hdmi device. */
 static struct platform_device *exynos_drm_hdmi_pdev;
 
@@ -41,7 +36,6 @@  static struct exynos_hdmi_ops *hdmi_ops;
 static struct exynos_mixer_ops *mixer_ops;
 
 struct drm_hdmi_context {
-	struct exynos_drm_subdrv	subdrv;
 	struct exynos_drm_hdmi_context	*hdmi_ctx;
 	struct exynos_drm_hdmi_context	*mixer_ctx;
 
@@ -97,10 +91,10 @@  void exynos_mixer_ops_register(struct exynos_mixer_ops *ops)
 		mixer_ops = ops;
 }
 
-static int drm_hdmi_display_initialize(struct device *dev,
+static int drm_hdmi_display_initialize(struct exynos_drm_display *display,
 		struct drm_device *drm_dev)
 {
-	struct drm_hdmi_context *ctx = to_context(dev);
+	struct drm_hdmi_context *ctx = display->ctx;
 
 	if (hdmi_ops && hdmi_ops->initialize)
 		return hdmi_ops->initialize(ctx->hdmi_ctx->ctx, drm_dev);
@@ -109,9 +103,9 @@  static int drm_hdmi_display_initialize(struct device *dev,
 }
 
 
-static bool drm_hdmi_is_connected(struct device *dev)
+static bool drm_hdmi_is_connected(struct exynos_drm_display *display)
 {
-	struct drm_hdmi_context *ctx = to_context(dev);
+	struct drm_hdmi_context *ctx = display->ctx;
 
 	if (hdmi_ops && hdmi_ops->is_connected)
 		return hdmi_ops->is_connected(ctx->hdmi_ctx->ctx);
@@ -119,10 +113,10 @@  static bool drm_hdmi_is_connected(struct device *dev)
 	return false;
 }
 
-static struct edid *drm_hdmi_get_edid(struct device *dev,
+static struct edid *drm_hdmi_get_edid(struct exynos_drm_display *display,
 			struct drm_connector *connector)
 {
-	struct drm_hdmi_context *ctx = to_context(dev);
+	struct drm_hdmi_context *ctx = display->ctx;
 
 	if (hdmi_ops && hdmi_ops->get_edid)
 		return hdmi_ops->get_edid(ctx->hdmi_ctx->ctx, connector);
@@ -151,68 +145,28 @@  static int drm_hdmi_check_mode_ctx(struct drm_hdmi_context *ctx,
 	return 0;
 }
 
-static int drm_hdmi_check_mode(struct device *dev,
+static int drm_hdmi_check_mode(struct exynos_drm_display *display,
 		struct drm_display_mode *mode)
 {
-	struct drm_hdmi_context *ctx = to_context(dev);
+	struct drm_hdmi_context *ctx = display->ctx;
 
 	return drm_hdmi_check_mode_ctx(ctx, mode);
 }
 
-static int drm_hdmi_display_dpms(struct device *dev, int mode)
+static void drm_hdmi_display_dpms(struct exynos_drm_display *display, int mode)
 {
-	struct drm_hdmi_context *ctx = to_context(dev);
+	struct drm_hdmi_context *ctx = display->ctx;
 
 	if (hdmi_ops && hdmi_ops->dpms)
 		hdmi_ops->dpms(ctx->hdmi_ctx->ctx, mode);
-
-	return 0;
 }
 
-static struct exynos_drm_display_ops drm_hdmi_display_ops = {
-	.type = EXYNOS_DISPLAY_TYPE_HDMI,
-	.initialize = drm_hdmi_display_initialize,
-	.is_connected = drm_hdmi_is_connected,
-	.get_edid = drm_hdmi_get_edid,
-	.check_mode = drm_hdmi_check_mode,
-	.dpms = drm_hdmi_display_dpms,
-};
-
-static int drm_hdmi_enable_vblank(struct exynos_drm_manager *mgr)
-{
-	struct drm_hdmi_context *ctx = mgr->ctx;
-	struct exynos_drm_subdrv *subdrv = &ctx->subdrv;
-	struct exynos_drm_manager *manager = subdrv->manager;
-
-	if (mixer_ops && mixer_ops->enable_vblank)
-		return mixer_ops->enable_vblank(ctx->mixer_ctx->ctx,
-						manager->pipe);
-
-	return 0;
-}
-
-static void drm_hdmi_disable_vblank(struct exynos_drm_manager *mgr)
-{
-	struct drm_hdmi_context *ctx = mgr->ctx;
-
-	if (mixer_ops && mixer_ops->disable_vblank)
-		return mixer_ops->disable_vblank(ctx->mixer_ctx->ctx);
-}
-
-static void drm_hdmi_wait_for_vblank(struct exynos_drm_manager *mgr)
-{
-	struct drm_hdmi_context *ctx = mgr->ctx;
-
-	if (mixer_ops && mixer_ops->wait_for_vblank)
-		mixer_ops->wait_for_vblank(ctx->mixer_ctx->ctx);
-}
-
-static void drm_hdmi_mode_fixup(struct exynos_drm_manager *mgr,
+static void drm_hdmi_mode_fixup(struct exynos_drm_display *display,
 				struct drm_connector *connector,
 				const struct drm_display_mode *mode,
 				struct drm_display_mode *adjusted_mode)
 {
-	struct drm_hdmi_context *ctx = mgr->ctx;
+	struct drm_hdmi_context *ctx = display->ctx;
 	struct drm_display_mode *m;
 	int mode_ok;
 
@@ -252,23 +206,66 @@  static void drm_hdmi_mode_fixup(struct exynos_drm_manager *mgr,
 	}
 }
 
-static void drm_hdmi_mode_set(struct exynos_drm_manager *mgr, void *mode)
+static void drm_hdmi_mode_set(struct exynos_drm_display *display,
+			struct drm_display_mode *mode)
 {
-	struct drm_hdmi_context *ctx = mgr->ctx;
+	struct drm_hdmi_context *ctx = display->ctx;
 
 	if (hdmi_ops && hdmi_ops->mode_set)
 		hdmi_ops->mode_set(ctx->hdmi_ctx->ctx, mode);
 }
 
-static void drm_hdmi_get_max_resol(struct exynos_drm_manager *mgr,
+static void drm_hdmi_get_max_resol(struct exynos_drm_display *display,
 				unsigned int *width, unsigned int *height)
 {
-	struct drm_hdmi_context *ctx = mgr->ctx;
+	struct drm_hdmi_context *ctx = display->ctx;
 
 	if (hdmi_ops && hdmi_ops->get_max_resol)
 		hdmi_ops->get_max_resol(ctx->hdmi_ctx->ctx, width, height);
 }
 
+static struct exynos_drm_display_ops drm_hdmi_display_ops = {
+	.initialize = drm_hdmi_display_initialize,
+	.is_connected = drm_hdmi_is_connected,
+	.get_edid = drm_hdmi_get_edid,
+	.check_mode = drm_hdmi_check_mode,
+	.dpms = drm_hdmi_display_dpms,
+	.mode_fixup = drm_hdmi_mode_fixup,
+	.mode_set = drm_hdmi_mode_set,
+	.get_max_resol = drm_hdmi_get_max_resol,
+};
+
+static struct exynos_drm_display hdmi_display = {
+	.type = EXYNOS_DISPLAY_TYPE_HDMI,
+	.ops = &drm_hdmi_display_ops,
+};
+
+static int drm_hdmi_enable_vblank(struct exynos_drm_manager *mgr)
+{
+	struct drm_hdmi_context *ctx = mgr->ctx;
+
+	if (mixer_ops && mixer_ops->enable_vblank)
+		return mixer_ops->enable_vblank(ctx->mixer_ctx->ctx, mgr->pipe);
+
+	return 0;
+}
+
+static void drm_hdmi_disable_vblank(struct exynos_drm_manager *mgr)
+{
+	struct drm_hdmi_context *ctx = mgr->ctx;
+
+	if (mixer_ops && mixer_ops->disable_vblank)
+		return mixer_ops->disable_vblank(ctx->mixer_ctx->ctx);
+}
+
+static void drm_hdmi_wait_for_vblank(struct exynos_drm_manager *mgr)
+{
+	struct drm_hdmi_context *ctx = mgr->ctx;
+
+	if (mixer_ops && mixer_ops->wait_for_vblank)
+		mixer_ops->wait_for_vblank(ctx->mixer_ctx->ctx);
+}
+
 static void drm_hdmi_commit(struct exynos_drm_manager *mgr)
 {
 	struct drm_hdmi_context *ctx = mgr->ctx;
@@ -277,11 +274,25 @@  static void drm_hdmi_commit(struct exynos_drm_manager *mgr)
 		hdmi_ops->commit(ctx->hdmi_ctx->ctx);
 }
 
-static int drm_hdmi_mgr_initialize(struct exynos_drm_manager *mgr, struct drm_device *drm_dev)
+static int drm_hdmi_mgr_initialize(struct exynos_drm_manager *mgr,
+			struct drm_device *drm_dev, int pipe)
 {
 	struct drm_hdmi_context *ctx = mgr->ctx;
 	int ret = 0;
 
+	if (!hdmi_ctx) {
+		DRM_ERROR("hdmi context not initialized.\n");
+		return -EFAULT;
+	}
+
+	if (!mixer_ctx) {
+		DRM_ERROR("mixer context not initialized.\n");
+		return -EFAULT;
+	}
+
+	ctx->hdmi_ctx = hdmi_ctx;
+	ctx->mixer_ctx = mixer_ctx;
+
 	if (mixer_ops && mixer_ops->initialize)
 		ret = mixer_ops->initialize(ctx->mixer_ctx->ctx, drm_dev);
 
@@ -291,6 +302,14 @@  static int drm_hdmi_mgr_initialize(struct exynos_drm_manager *mgr, struct drm_de
 	return ret;
 }
 
+static void drm_hdmi_mgr_remove(struct exynos_drm_manager *mgr)
+{
+	struct drm_hdmi_context *ctx = mgr->ctx;
+
+	if (mixer_ops->iommu_on)
+		mixer_ops->iommu_on(ctx->mixer_ctx->ctx, false);
+}
+
 static void drm_hdmi_dpms(struct exynos_drm_manager *mgr, int mode)
 {
 	struct drm_hdmi_context *ctx = mgr->ctx;
@@ -345,13 +364,11 @@  static void drm_mixer_win_disable(struct exynos_drm_manager *mgr, int zpos)
 
 static struct exynos_drm_manager_ops drm_hdmi_manager_ops = {
 	.initialize = drm_hdmi_mgr_initialize,
+	.remove = drm_hdmi_mgr_remove,
 	.dpms = drm_hdmi_dpms,
 	.enable_vblank = drm_hdmi_enable_vblank,
 	.disable_vblank = drm_hdmi_disable_vblank,
 	.wait_for_vblank = drm_hdmi_wait_for_vblank,
-	.mode_fixup = drm_hdmi_mode_fixup,
-	.mode_set = drm_hdmi_mode_set,
-	.get_max_resol = drm_hdmi_get_max_resol,
 	.commit = drm_hdmi_commit,
 	.win_mode_set = drm_mixer_win_mode_set,
 	.win_commit = drm_mixer_win_commit,
@@ -359,55 +376,13 @@  static struct exynos_drm_manager_ops drm_hdmi_manager_ops = {
 };
 
 static struct exynos_drm_manager hdmi_manager = {
-	.pipe		= -1,
+	.type		= EXYNOS_DISPLAY_TYPE_HDMI,
 	.ops		= &drm_hdmi_manager_ops,
-	.display_ops	= &drm_hdmi_display_ops,
 };
 
-static int hdmi_subdrv_probe(struct drm_device *drm_dev,
-		struct device *dev)
-{
-	struct exynos_drm_subdrv *subdrv = to_subdrv(dev);
-	struct drm_hdmi_context *ctx;
-
-	if (!hdmi_ctx) {
-		DRM_ERROR("hdmi context not initialized.\n");
-		return -EFAULT;
-	}
-
-	if (!mixer_ctx) {
-		DRM_ERROR("mixer context not initialized.\n");
-		return -EFAULT;
-	}
-
-	ctx = get_ctx_from_subdrv(subdrv);
-
-	if (!ctx) {
-		DRM_ERROR("no drm hdmi context.\n");
-		return -EFAULT;
-	}
-
-	ctx->hdmi_ctx = hdmi_ctx;
-	ctx->mixer_ctx = mixer_ctx;
-
-	return 0;
-}
-
-static void hdmi_subdrv_remove(struct drm_device *drm_dev, struct device *dev)
-{
-	struct drm_hdmi_context *ctx;
-	struct exynos_drm_subdrv *subdrv = to_subdrv(dev);
-
-	ctx = get_ctx_from_subdrv(subdrv);
-
-	if (mixer_ops->iommu_on)
-		mixer_ops->iommu_on(ctx->mixer_ctx->ctx, false);
-}
-
 static int exynos_drm_hdmi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct exynos_drm_subdrv *subdrv;
 	struct drm_hdmi_context *ctx;
 
 	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
@@ -415,26 +390,18 @@  static int exynos_drm_hdmi_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	hdmi_manager.ctx = ctx;
+	hdmi_display.ctx = ctx;
 
-	subdrv = &ctx->subdrv;
-
-	subdrv->dev = dev;
-	subdrv->manager = &hdmi_manager;
-	subdrv->probe = hdmi_subdrv_probe;
-	subdrv->remove = hdmi_subdrv_remove;
-
-	platform_set_drvdata(pdev, subdrv);
-
-	exynos_drm_subdrv_register(subdrv);
+	exynos_drm_manager_register(&hdmi_manager);
+	exynos_drm_display_register(&hdmi_display);
 
 	return 0;
 }
 
 static int exynos_drm_hdmi_remove(struct platform_device *pdev)
 {
-	struct drm_hdmi_context *ctx = platform_get_drvdata(pdev);
-
-	exynos_drm_subdrv_unregister(&ctx->subdrv);
+	exynos_drm_display_unregister(&hdmi_display);
+	exynos_drm_manager_unregister(&hdmi_manager);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
index 923239b..37059ea 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
@@ -19,10 +19,12 @@ 
  * exynos hdmi common context structure.
  *
  * @drm_dev: pointer to drm_device.
+ * @pipe: pipe for mixer
  * @ctx: pointer to the context of specific device driver.
  *	this context should be hdmi_context or mixer_context.
  */
 struct exynos_drm_hdmi_context {
+	int			pipe;
 	void			*ctx;
 };
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index cff3aed..e0db2b3 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -13,7 +13,7 @@ 
 
 #include <drm/exynos_drm.h>
 #include "exynos_drm_drv.h"
-#include "exynos_drm_encoder.h"
+#include "exynos_drm_crtc.h"
 #include "exynos_drm_fb.h"
 #include "exynos_drm_gem.h"
 #include "exynos_drm_plane.h"
@@ -139,7 +139,7 @@  int exynos_plane_mode_set(struct drm_plane *plane, struct drm_crtc *crtc,
 			overlay->crtc_x, overlay->crtc_y,
 			overlay->crtc_width, overlay->crtc_height);
 
-	exynos_drm_fn_encoder(crtc, overlay, exynos_drm_encoder_plane_mode_set);
+	exynos_drm_crtc_plane_mode_set(crtc, overlay);
 
 	return 0;
 }
@@ -149,8 +149,7 @@  void exynos_plane_commit(struct drm_plane *plane)
 	struct exynos_plane *exynos_plane = to_exynos_plane(plane);
 	struct exynos_drm_overlay *overlay = &exynos_plane->overlay;
 
-	exynos_drm_fn_encoder(plane->crtc, &overlay->zpos,
-			exynos_drm_encoder_plane_commit);
+	exynos_drm_crtc_plane_commit(plane->crtc, overlay->zpos);
 }
 
 void exynos_plane_dpms(struct drm_plane *plane, int mode)
@@ -162,17 +161,13 @@  void exynos_plane_dpms(struct drm_plane *plane, int mode)
 		if (exynos_plane->enabled)
 			return;
 
-		exynos_drm_fn_encoder(plane->crtc, &overlay->zpos,
-				exynos_drm_encoder_plane_enable);
-
+		exynos_drm_crtc_plane_enable(plane->crtc, overlay->zpos);
 		exynos_plane->enabled = true;
 	} else {
 		if (!exynos_plane->enabled)
 			return;
 
-		exynos_drm_fn_encoder(plane->crtc, &overlay->zpos,
-				exynos_drm_encoder_plane_disable);
-
+		exynos_drm_crtc_plane_disable(plane->crtc, overlay->zpos);
 		exynos_plane->enabled = false;
 	}
 }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index ce0e2ea..90dacaf 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -45,7 +45,7 @@  struct vidi_win_data {
 };
 
 struct vidi_context {
-	struct exynos_drm_subdrv	subdrv;
+	struct drm_device		*drm_dev;
 	struct drm_crtc			*crtc;
 	struct vidi_win_data		win_data[WINDOWS_NR];
 	struct edid			*raw_edid;
@@ -58,6 +58,7 @@  struct vidi_context {
 	bool				direct_vblank;
 	struct work_struct		work;
 	struct mutex			lock;
+	int				pipe;
 };
 
 static const char fake_edid_info[] = {
@@ -85,10 +86,9 @@  static const char fake_edid_info[] = {
 	0x00, 0x00, 0x00, 0x06
 };
 
-static bool vidi_display_is_connected(struct device *dev)
+static bool vidi_display_is_connected(struct exynos_drm_display *display)
 {
-	struct exynos_drm_manager *mgr = get_vidi_mgr(dev);
-	struct vidi_context *ctx = mgr->ctx;
+	struct vidi_context *ctx = display->ctx;
 
 	/*
 	 * connection request would come from user side
@@ -97,11 +97,10 @@  static bool vidi_display_is_connected(struct device *dev)
 	return ctx->connected ? true : false;
 }
 
-static struct edid *vidi_get_edid(struct device *dev,
+static struct edid *vidi_get_edid(struct exynos_drm_display *display,
 			struct drm_connector *connector)
 {
-	struct exynos_drm_manager *mgr = get_vidi_mgr(dev);
-	struct vidi_context *ctx = mgr->ctx;
+	struct vidi_context *ctx = display->ctx;
 	struct edid *edid;
 	int edid_len;
 
@@ -124,14 +123,8 @@  static struct edid *vidi_get_edid(struct device *dev,
 	return edid;
 }
 
-static void *vidi_get_panel(struct device *dev)
-{
-	/* TODO. */
-
-	return NULL;
-}
-
-static int vidi_check_mode(struct device *dev, struct drm_display_mode *mode)
+static int vidi_check_mode(struct exynos_drm_display *display,
+			struct drm_display_mode *mode)
 {
 	/* TODO. */
 
@@ -139,13 +132,16 @@  static int vidi_check_mode(struct device *dev, struct drm_display_mode *mode)
 }
 
 static struct exynos_drm_display_ops vidi_display_ops = {
-	.type = EXYNOS_DISPLAY_TYPE_VIDI,
 	.is_connected = vidi_display_is_connected,
 	.get_edid = vidi_get_edid,
-	.get_panel = vidi_get_panel,
 	.check_mode = vidi_check_mode,
 };
 
+static struct exynos_drm_display vidi_display = {
+	.type = EXYNOS_DISPLAY_TYPE_VIDI,
+	.ops = &vidi_display_ops,
+};
+
 static void vidi_dpms(struct exynos_drm_manager *mgr, int mode)
 {
 	struct vidi_context *ctx = mgr->ctx;
@@ -325,7 +321,38 @@  static void vidi_win_disable(struct exynos_drm_manager *mgr, int zpos)
 	/* TODO. */
 }
 
+static int vidi_mgr_initialize(struct exynos_drm_manager *mgr,
+			struct drm_device *drm_dev, int pipe)
+{
+	struct vidi_context *ctx = mgr->ctx;
+
+	DRM_ERROR("vidi initialize ct=%p dev=%p pipe=%d\n", ctx, drm_dev, pipe);
+
+	ctx->drm_dev = drm_dev;
+	ctx->pipe = pipe;
+
+	/*
+	 * enable drm irq mode.
+	 * - with irq_enabled = 1, we can use the vblank feature.
+	 *
+	 * P.S. note that we wouldn't use drm irq handler but
+	 *	just specific driver own one instead because
+	 *	drm framework supports only one irq handler.
+	 */
+	drm_dev->irq_enabled = 1;
+
+	/*
+	 * with vblank_disable_allowed = 1, vblank interrupt will be disabled
+	 * by drm timer once a current process gives up ownership of
+	 * vblank event.(after drm_vblank_put function is called)
+	 */
+	drm_dev->vblank_disable_allowed = 1;
+
+	return 0;
+}
+
 static struct exynos_drm_manager_ops vidi_manager_ops = {
+	.initialize = vidi_mgr_initialize,
 	.dpms = vidi_dpms,
 	.commit = vidi_commit,
 	.enable_vblank = vidi_enable_vblank,
@@ -336,19 +363,16 @@  static struct exynos_drm_manager_ops vidi_manager_ops = {
 };
 
 static struct exynos_drm_manager vidi_manager = {
-	.pipe		= -1,
-	.ops		= &vidi_manager_ops,
-	.display_ops	= &vidi_display_ops,
+	.type = EXYNOS_DISPLAY_TYPE_VIDI,
+	.ops = &vidi_manager_ops,
 };
 
 static void vidi_fake_vblank_handler(struct work_struct *work)
 {
 	struct vidi_context *ctx = container_of(work, struct vidi_context,
 					work);
-	struct exynos_drm_subdrv *subdrv = &ctx->subdrv;
-	struct exynos_drm_manager *manager = subdrv->manager;
 
-	if (manager->pipe < 0)
+	if (ctx->pipe < 0)
 		return;
 
 	/* refresh rate is about 50Hz. */
@@ -357,7 +381,7 @@  static void vidi_fake_vblank_handler(struct work_struct *work)
 	mutex_lock(&ctx->lock);
 
 	if (ctx->direct_vblank) {
-		drm_handle_vblank(subdrv->drm_dev, manager->pipe);
+		drm_handle_vblank(ctx->drm_dev, ctx->pipe);
 		ctx->direct_vblank = false;
 		mutex_unlock(&ctx->lock);
 		return;
@@ -365,34 +389,7 @@  static void vidi_fake_vblank_handler(struct work_struct *work)
 
 	mutex_unlock(&ctx->lock);
 
-	exynos_drm_crtc_finish_pageflip(subdrv->drm_dev, manager->pipe);
-}
-
-static int vidi_subdrv_probe(struct drm_device *drm_dev, struct device *dev)
-{
-	/*
-	 * enable drm irq mode.
-	 * - with irq_enabled = 1, we can use the vblank feature.
-	 *
-	 * P.S. note that we wouldn't use drm irq handler but
-	 *	just specific driver own one instead because
-	 *	drm framework supports only one irq handler.
-	 */
-	drm_dev->irq_enabled = 1;
-
-	/*
-	 * with vblank_disable_allowed = 1, vblank interrupt will be disabled
-	 * by drm timer once a current process gives up ownership of
-	 * vblank event.(after drm_vblank_put function is called)
-	 */
-	drm_dev->vblank_disable_allowed = 1;
-
-	return 0;
-}
-
-static void vidi_subdrv_remove(struct drm_device *drm_dev, struct device *dev)
-{
-	/* TODO. */
+	exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
 }
 
 static int vidi_power_on(struct exynos_drm_manager *mgr, bool enable)
@@ -462,7 +459,7 @@  static int vidi_store_connection(struct device *dev,
 
 	DRM_DEBUG_KMS("requested connection.\n");
 
-	drm_helper_hpd_irq_event(ctx->subdrv.drm_dev);
+	drm_helper_hpd_irq_event(ctx->drm_dev);
 
 	return len;
 }
@@ -475,8 +472,7 @@  int vidi_connection_ioctl(struct drm_device *drm_dev, void *data,
 {
 	struct vidi_context *ctx = NULL;
 	struct drm_encoder *encoder;
-	struct exynos_drm_manager *manager;
-	struct exynos_drm_display_ops *display_ops;
+	struct exynos_drm_display *display;
 	struct drm_exynos_vidi_connection *vidi = data;
 	int edid_len;
 
@@ -492,11 +488,10 @@  int vidi_connection_ioctl(struct drm_device *drm_dev, void *data,
 
 	list_for_each_entry(encoder, &drm_dev->mode_config.encoder_list,
 								head) {
-		manager = exynos_drm_get_manager(encoder);
-		display_ops = manager->display_ops;
+		display = exynos_drm_get_display(encoder);
 
-		if (display_ops->type == EXYNOS_DISPLAY_TYPE_VIDI) {
-			ctx = manager->ctx;
+		if (display->type == EXYNOS_DISPLAY_TYPE_VIDI) {
+			ctx = display->ctx;
 			break;
 		}
 	}
@@ -536,7 +531,7 @@  int vidi_connection_ioctl(struct drm_device *drm_dev, void *data,
 	}
 
 	ctx->connected = vidi->connection;
-	drm_helper_hpd_irq_event(ctx->subdrv.drm_dev);
+	drm_helper_hpd_irq_event(ctx->drm_dev);
 
 	return 0;
 }
@@ -545,7 +540,6 @@  static int vidi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct vidi_context *ctx;
-	struct exynos_drm_subdrv *subdrv;
 	int ret;
 
 	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
@@ -557,12 +551,7 @@  static int vidi_probe(struct platform_device *pdev)
 	INIT_WORK(&ctx->work, vidi_fake_vblank_handler);
 
 	vidi_manager.ctx = ctx;
-
-	subdrv = &ctx->subdrv;
-	subdrv->dev = dev;
-	subdrv->manager = &vidi_manager;
-	subdrv->probe = vidi_subdrv_probe;
-	subdrv->remove = vidi_subdrv_remove;
+	vidi_display.ctx = ctx;
 
 	mutex_init(&ctx->lock);
 
@@ -572,7 +561,8 @@  static int vidi_probe(struct platform_device *pdev)
 	if (ret < 0)
 		DRM_INFO("failed to create connection sysfs.\n");
 
-	exynos_drm_subdrv_register(subdrv);
+	exynos_drm_manager_register(&vidi_manager);
+	exynos_drm_display_register(&vidi_display);
 
 	return 0;
 }
@@ -581,7 +571,8 @@  static int vidi_remove(struct platform_device *pdev)
 {
 	struct vidi_context *ctx = platform_get_drvdata(pdev);
 
-	exynos_drm_subdrv_unregister(&ctx->subdrv);
+	exynos_drm_display_unregister(&vidi_display);
+	exynos_drm_manager_unregister(&vidi_manager);
 
 	if (ctx->raw_edid != (struct edid *)fake_edid_info) {
 		kfree(ctx->raw_edid);