drm/exynos: Refactor manager/display/overlay [chromiumos/third_party/kernel : chromeos-3.4]
22 views
Skip to first unread message
Sean Paul (Code Review)
Oct 24, 2012, 5:44:00 PM10/24/12
to Olof Johansson, Mandeep Singh Baines
Sean Paul has uploaded a new change for review.
Change subject: drm/exynos: Refactor manager/display/overlay
......................................................................
drm/exynos: Refactor manager/display/overlay
The exynos_drm_manager/display/overlay architecture wasn't really
serving its purpose. It attempted to split callbacks into logical
sections, but those sections didn't really make sense, and required us
to have a layer of abstraction such as exynos_drm_hdmi on top of the
controller and panel to call the appropriate functions.
This patch removes manager/display/overlay as well as exynos_drm_hdmi,
and adds a new abstraction which splits the callbacks to controller and
panel.
Right now, this just cleans things up, making the code a little more
readable. In the short term, it allows us to hook fimd into just
the controller side, and add dp/mipi as panel implementations. In the
long term, it removes all of the device pointers, so we should be able
to remove all of the various platform drivers more easily.
BUG=chrome-os-partner:11158,chrome-os-partner:10717
TEST=Tested on snow, hdmi hotplugged, booted with/without hdmi,
suspend/resume tested. No regressions detected.
Change-Id: Ia1bd6bbd67d49af2abecf36c43bfe765ae00f38d
Signed-off-by: Sean Paul <sean...@chromium.org>
---
M drivers/gpu/drm/exynos/Makefile
M drivers/gpu/drm/exynos/exynos_drm_connector.c
M drivers/gpu/drm/exynos/exynos_drm_core.c
M drivers/gpu/drm/exynos/exynos_drm_crtc.c
M drivers/gpu/drm/exynos/exynos_drm_drv.c
M drivers/gpu/drm/exynos/exynos_drm_drv.h
M drivers/gpu/drm/exynos/exynos_drm_encoder.c
M drivers/gpu/drm/exynos/exynos_drm_encoder.h
M drivers/gpu/drm/exynos/exynos_drm_fimd.c
D drivers/gpu/drm/exynos/exynos_drm_hdmi.c
D drivers/gpu/drm/exynos/exynos_drm_hdmi.h
M drivers/gpu/drm/exynos/exynos_drm_vidi.c
M drivers/gpu/drm/exynos/exynos_hdmi.c
M drivers/gpu/drm/exynos/exynos_hdmi.h
M drivers/gpu/drm/exynos/exynos_mixer.c
15 files changed, 708 insertions(+), 1,334 deletions(-)
git pull ssh://gerrit.chromium.org:29418/chromiumos/third_party/kernel refs/changes/77/36477/1
--
To view, visit https://gerrit.chromium.org/gerrit/36477
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia1bd6bbd67d49af2abecf36c43bfe765ae00f38d
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Sean Paul <sean...@chromium.org>
Change subject: drm/exynos: Refactor manager/display/overlay
......................................................................
drm/exynos: Refactor manager/display/overlay
The exynos_drm_manager/display/overlay architecture wasn't really
serving its purpose. It attempted to split callbacks into logical
sections, but those sections didn't really make sense, and required us
to have a layer of abstraction such as exynos_drm_hdmi on top of the
controller and panel to call the appropriate functions.
This patch removes manager/display/overlay as well as exynos_drm_hdmi,
and adds a new abstraction which splits the callbacks to controller and
panel.
Right now, this just cleans things up, making the code a little more
readable. In the short term, it allows us to hook fimd into just
the controller side, and add dp/mipi as panel implementations. In the
long term, it removes all of the device pointers, so we should be able
to remove all of the various platform drivers more easily.
BUG=chrome-os-partner:11158,chrome-os-partner:10717
TEST=Tested on snow, hdmi hotplugged, booted with/without hdmi,
suspend/resume tested. No regressions detected.
Change-Id: Ia1bd6bbd67d49af2abecf36c43bfe765ae00f38d
Signed-off-by: Sean Paul <sean...@chromium.org>
---
M drivers/gpu/drm/exynos/Makefile
M drivers/gpu/drm/exynos/exynos_drm_connector.c
M drivers/gpu/drm/exynos/exynos_drm_core.c
M drivers/gpu/drm/exynos/exynos_drm_crtc.c
M drivers/gpu/drm/exynos/exynos_drm_drv.c
M drivers/gpu/drm/exynos/exynos_drm_drv.h
M drivers/gpu/drm/exynos/exynos_drm_encoder.c
M drivers/gpu/drm/exynos/exynos_drm_encoder.h
M drivers/gpu/drm/exynos/exynos_drm_fimd.c
D drivers/gpu/drm/exynos/exynos_drm_hdmi.c
D drivers/gpu/drm/exynos/exynos_drm_hdmi.h
M drivers/gpu/drm/exynos/exynos_drm_vidi.c
M drivers/gpu/drm/exynos/exynos_hdmi.c
M drivers/gpu/drm/exynos/exynos_hdmi.h
M drivers/gpu/drm/exynos/exynos_mixer.c
15 files changed, 708 insertions(+), 1,334 deletions(-)
git pull ssh://gerrit.chromium.org:29418/chromiumos/third_party/kernel refs/changes/77/36477/1
--
To view, visit https://gerrit.chromium.org/gerrit/36477
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia1bd6bbd67d49af2abecf36c43bfe765ae00f38d
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Sean Paul <sean...@chromium.org>
Stéphane Marchesin (Code Review)
Oct 26, 2012, 7:33:36 PM10/26/12
to Sean Paul, Gerrit
Stéphane Marchesin has posted comments on this change.
Change subject: drm/exynos: Refactor manager/display/overlay
......................................................................
Patch Set 2: (6 inline comments)
....................................................
File drivers/gpu/drm/exynos/exynos_drm_connector.c
Line 294: #ifdef CONFIG_DRM_EXYNOS_HDMI
Why are those #ifdefs needed now?
....................................................
File drivers/gpu/drm/exynos/exynos_drm_display.c
Line 25:
Most of the functions in this file are just wrappers, I don't think we should do that as it adds complexity but no real gain.
I think you only need init/remove/probe/dpms, and those could be in the drv and encoder code, respectively. Am I missing something?
Line 183:
Those 2 functions are the same.
....................................................
File drivers/gpu/drm/exynos/exynos_drm_display.h
Line 65: struct exynos_controller_ops {
I think we should name this encoder_ops, so that we don't add new semantics. Things are already confusing enough.
....................................................
File drivers/gpu/drm/exynos/exynos_drm_encoder.c
Line 201: #ifdef CONFIG_DRM_EXYNOS_FIMD
Same, why those #ifdef?
Line 317: * enable_vblank hasn't already been called, that's a bug.
So what about the case where the encoder turns on, and then drm enables vblank before the unit is powered on? Could this be a workaround for another case of the bug we discussed?
--
To view, visit https://gerrit.chromium.org/gerrit/36477
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1bd6bbd67d49af2abecf36c43bfe765ae00f38d
Gerrit-PatchSet: 2
Gerrit-Reviewer: Sean Paul <sean...@chromium.org>
Gerrit-Reviewer: Stéphane Marchesin <mar...@chromium.org>
Change subject: drm/exynos: Refactor manager/display/overlay
......................................................................
....................................................
File drivers/gpu/drm/exynos/exynos_drm_connector.c
Line 294: #ifdef CONFIG_DRM_EXYNOS_HDMI
Why are those #ifdefs needed now?
....................................................
File drivers/gpu/drm/exynos/exynos_drm_display.c
Line 25:
Most of the functions in this file are just wrappers, I don't think we should do that as it adds complexity but no real gain.
I think you only need init/remove/probe/dpms, and those could be in the drv and encoder code, respectively. Am I missing something?
Line 183:
Those 2 functions are the same.
....................................................
File drivers/gpu/drm/exynos/exynos_drm_display.h
Line 65: struct exynos_controller_ops {
I think we should name this encoder_ops, so that we don't add new semantics. Things are already confusing enough.
....................................................
File drivers/gpu/drm/exynos/exynos_drm_encoder.c
Line 201: #ifdef CONFIG_DRM_EXYNOS_FIMD
Same, why those #ifdef?
Line 317: * enable_vblank hasn't already been called, that's a bug.
So what about the case where the encoder turns on, and then drm enables vblank before the unit is powered on? Could this be a workaround for another case of the bug we discussed?
--
To view, visit https://gerrit.chromium.org/gerrit/36477
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: Ia1bd6bbd67d49af2abecf36c43bfe765ae00f38d
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Sean Paul <sean...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Sean Paul <sean...@chromium.org>
Gerrit-Reviewer: Sean Paul <sean...@chromium.org>
Gerrit-Reviewer: Stéphane Marchesin <mar...@chromium.org>
Stéphane Marchesin (Code Review)
Oct 26, 2012, 8:13:39 PM10/26/12
to Sean Paul, Gerrit
Stéphane Marchesin has posted comments on this change.
Change subject: drm/exynos: Refactor manager/display/overlay
......................................................................
Patch Set 3: I would prefer that you didn't submit this
......................................................................
Marking -1 so I can tell what I already reviewed.
--
To view, visit https://gerrit.chromium.org/gerrit/36477
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: Ia1bd6bbd67d49af2abecf36c43bfe765ae00f38d
Gerrit-PatchSet: 3
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Sean Paul <sean...@chromium.org>
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Sean Paul <sean...@chromium.org>
Sean Paul (Code Review)
Oct 29, 2012, 12:08:14 PM10/29/12
to Stéphane Marchesin, Gerrit
Sean Paul has posted comments on this change.
Change subject: drm/exynos: Refactor manager/display/overlay
......................................................................
....................................................
File drivers/gpu/drm/exynos/exynos_drm_connector.c
Line 294: #ifdef CONFIG_DRM_EXYNOS_HDMI
They're not. It's kruft that I added early on so I didn't have to deal with allocated displays that do not have implementations.
Will remove.
....................................................
File drivers/gpu/drm/exynos/exynos_drm_display.c
Line 25:
No, I originally thought we might be able to move common code in here, but there's not really enough to justify it.
I'll remove.
Line 183:
Yeah. This is a little messed up, and relates to one of my TODOs elsewhere in the code.
I originally had dpms pointing to power, but realized that we're calling dpms, then power one after another on init. Since no one implements both, we're fine.
Like other places in the code, dpms/power needs to be cleaned up.
....................................................
File drivers/gpu/drm/exynos/exynos_drm_display.h
Line 65: struct exynos_controller_ops {
Unfortunately, this doesn't map to encoder_ops; some, like page_flip, are crtc ops.
These are meant to map directly to functions of FIMD/Mixer/VIDI, not drm constructs.
I'm happy to rename it, but I fear encoder_ops might make things more confusing.
....................................................
File drivers/gpu/drm/exynos/exynos_drm_encoder.c
Line 201: #ifdef CONFIG_DRM_EXYNOS_FIMD
Will remove.
Line 317: * enable_vblank hasn't already been called, that's a bug.
Yeah, your theory makes sense to me. I still think it's a hack, and needs more investigation. I plan on coming back to fix up all of the TODOs, but I wanted to avoid increasing the scope of this (already large) CL.
--
To view, visit https://gerrit.chromium.org/gerrit/36477
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Change subject: drm/exynos: Refactor manager/display/overlay
......................................................................
Patch Set 2: (6 inline comments)
Thanks for the review, some responses inline. I'll respin.
....................................................
File drivers/gpu/drm/exynos/exynos_drm_connector.c
Line 294: #ifdef CONFIG_DRM_EXYNOS_HDMI
Will remove.
....................................................
File drivers/gpu/drm/exynos/exynos_drm_display.c
Line 25:
I'll remove.
Line 183:
Yeah. This is a little messed up, and relates to one of my TODOs elsewhere in the code.
I originally had dpms pointing to power, but realized that we're calling dpms, then power one after another on init. Since no one implements both, we're fine.
Like other places in the code, dpms/power needs to be cleaned up.
....................................................
File drivers/gpu/drm/exynos/exynos_drm_display.h
Line 65: struct exynos_controller_ops {
These are meant to map directly to functions of FIMD/Mixer/VIDI, not drm constructs.
I'm happy to rename it, but I fear encoder_ops might make things more confusing.
....................................................
File drivers/gpu/drm/exynos/exynos_drm_encoder.c
Line 201: #ifdef CONFIG_DRM_EXYNOS_FIMD
Line 317: * enable_vblank hasn't already been called, that's a bug.
--
To view, visit https://gerrit.chromium.org/gerrit/36477
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1bd6bbd67d49af2abecf36c43bfe765ae00f38d
Gerrit-PatchSet: 2
Gerrit-Change-Id: Ia1bd6bbd67d49af2abecf36c43bfe765ae00f38d
Gerrit-PatchSet: 2
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Sean Paul <sean...@chromium.org>
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Sean Paul <sean...@chromium.org>
Stéphane Marchesin (Code Review)
Oct 31, 2012, 7:23:45 PM10/31/12
to Sean Paul, Gerrit, Daniel Kurtz, Mandeep Singh Baines
Stéphane Marchesin has posted comments on this change.
Change subject: drm/exynos: Refactor manager/display/overlay
......................................................................
Patch Set 5: (1 inline comment)
....................................................
File drivers/gpu/drm/exynos/exynos_drm_connector.c
Line 134: ret = drm_add_edid_modes(connector, edid);
Please test the return value here and bail, otherwise you can get a corrupted edid into your display_info struct. Please also print a warning about edid being invalid.
--
To view, visit https://gerrit.chromium.org/gerrit/36477
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1bd6bbd67d49af2abecf36c43bfe765ae00f38d
Gerrit-PatchSet: 5
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Mandeep Singh Baines <m...@chromium.org>
Change subject: drm/exynos: Refactor manager/display/overlay
......................................................................
....................................................
File drivers/gpu/drm/exynos/exynos_drm_connector.c
Line 134: ret = drm_add_edid_modes(connector, edid);
Please test the return value here and bail, otherwise you can get a corrupted edid into your display_info struct. Please also print a warning about edid being invalid.
--
To view, visit https://gerrit.chromium.org/gerrit/36477
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: Ia1bd6bbd67d49af2abecf36c43bfe765ae00f38d
Gerrit-PatchSet: 5
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Sean Paul <sean...@chromium.org>
Gerrit-Reviewer: Daniel Kurtz <djk...@chromium.org>
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Sean Paul <sean...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Mandeep Singh Baines <m...@chromium.org>
Stéphane Marchesin (Code Review)
Nov 8, 2012, 7:07:48 PM11/8/12
to Sean Paul, Gerrit, Daniel Kurtz, Mandeep Singh Baines
Stéphane Marchesin has posted comments on this change.
Change subject: drm/exynos: Refactor manager/display/overlay
......................................................................
Patch Set 10: Looks good to me, approved
......................................................................
--
To view, visit https://gerrit.chromium.org/gerrit/36477
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-Change-Id: Ia1bd6bbd67d49af2abecf36c43bfe765ae00f38d
Gerrit-PatchSet: 10
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Sean Paul <sean...@chromium.org>
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Sean Paul <sean...@chromium.org>
Sean Paul (Code Review)
Nov 13, 2012, 9:01:32 AM11/13/12
to Stéphane Marchesin, Gerrit, Daniel Kurtz, Mandeep Singh Baines
Sean Paul has posted comments on this change.
Change subject: drm/exynos: Refactor manager/display/overlay
......................................................................
Patch Set 10: Verified; Ready
--
To view, visit https://gerrit.chromium.org/gerrit/36477
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Change subject: drm/exynos: Refactor manager/display/overlay
......................................................................
--
To view, visit https://gerrit.chromium.org/gerrit/36477
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1bd6bbd67d49af2abecf36c43bfe765ae00f38d
Gerrit-PatchSet: 10
Gerrit-Change-Id: Ia1bd6bbd67d49af2abecf36c43bfe765ae00f38d
Gerrit-PatchSet: 10
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Sean Paul <sean...@chromium.org>
Gerrit-Branch: chromeos-3.4
Gerrit-Owner: Sean Paul <sean...@chromium.org>
Reply all
Reply to author
Forward
0 new messages