This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
I have just put up Rob's and Sean's idea of chaining up the bridges in code, and have implemented basic panel controls as a chained bridge. This works well with ptn3460 bridge chip on exynos5250-snow board.
Still need to make use of standard list calls and figure out proper way of deleting the bridge chain. So, this is just a rough version.
Ajay Kumar (3): [RFC V2 1/3] drm: implement chaining of drm bridges [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
.../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ drivers/gpu/drm/bridge/ptn3460.c | 21 +- drivers/gpu/drm/drm_crtc.c | 13 +- include/drm/bridge/bridge_panel.h | 37 ++++ include/drm/drm_crtc.h | 2 + 8 files changed, 360 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h
As of now, we can have only one bridge hanging off the encoder. With this patch, we allow multiple bridges to hang onto the same encoder with the use of a simple next_bridge ptr.
The drm core calls bridge->funcs for the first bridge which is attached to it, and its upto the individual bridge drivers to call bridge->funcs for the next bridge in the chain.
Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com --- drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++- include/drm/drm_crtc.h | 2 ++ 2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d8b7099..fe9905f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -918,8 +918,10 @@ EXPORT_SYMBOL(drm_connector_unplug_all); * Zero on success, error code on failure. */ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, - const struct drm_bridge_funcs *funcs) + struct drm_encoder *encoder, + const struct drm_bridge_funcs *funcs) { + struct drm_bridge *temp; int ret;
drm_modeset_lock_all(dev); @@ -931,6 +933,15 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, bridge->dev = dev; bridge->funcs = funcs;
+ if (encoder->bridge) { + temp = encoder->bridge; + while (temp->next_bridge) + temp = temp->next_bridge; + + temp->next_bridge = bridge; + } else + encoder->bridge = bridge; + list_add_tail(&bridge->head, &dev->mode_config.bridge_list); dev->mode_config.num_bridge++;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e55fccb..bb6ed88 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -619,6 +619,7 @@ struct drm_bridge_funcs { struct drm_bridge { struct drm_device *dev; struct list_head head; + struct drm_bridge *next_bridge;
struct drm_mode_object base;
@@ -862,6 +863,7 @@ extern void drm_connector_cleanup(struct drm_connector *connector); extern void drm_connector_unplug_all(struct drm_device *dev);
extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, + struct drm_encoder *encoder, const struct drm_bridge_funcs *funcs); extern void drm_bridge_cleanup(struct drm_bridge *bridge);
On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar ajaykumar.rs@samsung.com wrote:
As of now, we can have only one bridge hanging off the encoder. With this patch, we allow multiple bridges to hang onto the same encoder with the use of a simple next_bridge ptr.
The drm core calls bridge->funcs for the first bridge which is attached to it, and its upto the individual bridge drivers to call bridge->funcs for the next bridge in the chain.
Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com
drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++- include/drm/drm_crtc.h | 2 ++ 2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d8b7099..fe9905f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -918,8 +918,10 @@ EXPORT_SYMBOL(drm_connector_unplug_all);
- Zero on success, error code on failure.
*/ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
const struct drm_bridge_funcs *funcs)
struct drm_encoder *encoder,const struct drm_bridge_funcs *funcs)
IMO, we should let whoever is spawning the bridges chain them together, instead of passing encoder in init().
Sean
{
struct drm_bridge *temp; int ret; drm_modeset_lock_all(dev);@@ -931,6 +933,15 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, bridge->dev = dev; bridge->funcs = funcs;
if (encoder->bridge) {temp = encoder->bridge;while (temp->next_bridge)temp = temp->next_bridge;temp->next_bridge = bridge;} elseencoder->bridge = bridge;list_add_tail(&bridge->head, &dev->mode_config.bridge_list); dev->mode_config.num_bridge++;diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e55fccb..bb6ed88 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -619,6 +619,7 @@ struct drm_bridge_funcs { struct drm_bridge { struct drm_device *dev; struct list_head head;
struct drm_bridge *next_bridge; struct drm_mode_object base;@@ -862,6 +863,7 @@ extern void drm_connector_cleanup(struct drm_connector *connector); extern void drm_connector_unplug_all(struct drm_device *dev);
extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
struct drm_encoder *encoder, const struct drm_bridge_funcs *funcs);extern void drm_bridge_cleanup(struct drm_bridge *bridge);
-- 1.8.1.2
On Tue, May 6, 2014 at 11:55 AM, Sean Paul seanpaul@chromium.org wrote:
On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar ajaykumar.rs@samsung.com wrote:
As of now, we can have only one bridge hanging off the encoder. With this patch, we allow multiple bridges to hang onto the same encoder with the use of a simple next_bridge ptr.
The drm core calls bridge->funcs for the first bridge which is attached to it, and its upto the individual bridge drivers to call bridge->funcs for the next bridge in the chain.
Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com
drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++- include/drm/drm_crtc.h | 2 ++ 2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d8b7099..fe9905f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -918,8 +918,10 @@ EXPORT_SYMBOL(drm_connector_unplug_all);
- Zero on success, error code on failure.
*/ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
const struct drm_bridge_funcs *funcs)
struct drm_encoder *encoder,const struct drm_bridge_funcs *funcs)IMO, we should let whoever is spawning the bridges chain them together, instead of passing encoder in init().
that plus, might be a good time to start adding some static-inline helper fxns for fxn ptr calls like we have for drm_panel.. not a big deal, but I guess it would be a good time to do it now before we start adding chained bridge calls in all the bridges.
but yeah, in general this is what I had in mind
BR, -R
Sean
{
struct drm_bridge *temp; int ret; drm_modeset_lock_all(dev);@@ -931,6 +933,15 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, bridge->dev = dev; bridge->funcs = funcs;
if (encoder->bridge) {temp = encoder->bridge;while (temp->next_bridge)temp = temp->next_bridge;temp->next_bridge = bridge;} elseencoder->bridge = bridge;list_add_tail(&bridge->head, &dev->mode_config.bridge_list); dev->mode_config.num_bridge++;diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e55fccb..bb6ed88 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -619,6 +619,7 @@ struct drm_bridge_funcs { struct drm_bridge { struct drm_device *dev; struct list_head head;
struct drm_bridge *next_bridge; struct drm_mode_object base;@@ -862,6 +863,7 @@ extern void drm_connector_cleanup(struct drm_connector *connector); extern void drm_connector_unplug_all(struct drm_device *dev);
extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
struct drm_encoder *encoder, const struct drm_bridge_funcs *funcs);extern void drm_bridge_cleanup(struct drm_bridge *bridge);
-- 1.8.1.2
Rob,
On Tue, May 6, 2014 at 9:42 PM, Rob Clark robdclark@gmail.com wrote:
On Tue, May 6, 2014 at 11:55 AM, Sean Paul seanpaul@chromium.org wrote:
On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar ajaykumar.rs@samsung.com wrote:
As of now, we can have only one bridge hanging off the encoder. With this patch, we allow multiple bridges to hang onto the same encoder with the use of a simple next_bridge ptr.
The drm core calls bridge->funcs for the first bridge which is attached to it, and its upto the individual bridge drivers to call bridge->funcs for the next bridge in the chain.
Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com
drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++- include/drm/drm_crtc.h | 2 ++ 2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d8b7099..fe9905f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -918,8 +918,10 @@ EXPORT_SYMBOL(drm_connector_unplug_all);
- Zero on success, error code on failure.
*/ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
const struct drm_bridge_funcs *funcs)
struct drm_encoder *encoder,const struct drm_bridge_funcs *funcs)IMO, we should let whoever is spawning the bridges chain them together, instead of passing encoder in init().
that plus, might be a good time to start adding some static-inline helper fxns for fxn ptr calls like we have for drm_panel.. not a big deal, but I guess it would be a good time to do it now before we start adding chained bridge calls in all the bridges.
Right, I will try to add a few: -- to update next_bridge ptr and form a chain. -- to call pre_enable, enable, disable and post_disable for the next bridge in the list.
Ajay
BR, -R
Sean
{
struct drm_bridge *temp; int ret; drm_modeset_lock_all(dev);@@ -931,6 +933,15 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, bridge->dev = dev; bridge->funcs = funcs;
if (encoder->bridge) {temp = encoder->bridge;while (temp->next_bridge)temp = temp->next_bridge;temp->next_bridge = bridge;} elseencoder->bridge = bridge;list_add_tail(&bridge->head, &dev->mode_config.bridge_list); dev->mode_config.num_bridge++;diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e55fccb..bb6ed88 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -619,6 +619,7 @@ struct drm_bridge_funcs { struct drm_bridge { struct drm_device *dev; struct list_head head;
struct drm_bridge *next_bridge; struct drm_mode_object base;@@ -862,6 +863,7 @@ extern void drm_connector_cleanup(struct drm_connector *connector); extern void drm_connector_unplug_all(struct drm_device *dev);
extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
struct drm_encoder *encoder, const struct drm_bridge_funcs *funcs);extern void drm_bridge_cleanup(struct drm_bridge *bridge);
-- 1.8.1.2
Sean,
On Tue, May 6, 2014 at 9:25 PM, Sean Paul seanpaul@chromium.org wrote:
On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar ajaykumar.rs@samsung.com wrote:
As of now, we can have only one bridge hanging off the encoder. With this patch, we allow multiple bridges to hang onto the same encoder with the use of a simple next_bridge ptr.
The drm core calls bridge->funcs for the first bridge which is attached to it, and its upto the individual bridge drivers to call bridge->funcs for the next bridge in the chain.
Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com
drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++- include/drm/drm_crtc.h | 2 ++ 2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d8b7099..fe9905f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -918,8 +918,10 @@ EXPORT_SYMBOL(drm_connector_unplug_all);
- Zero on success, error code on failure.
*/ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
const struct drm_bridge_funcs *funcs)
struct drm_encoder *encoder,const struct drm_bridge_funcs *funcs)IMO, we should let whoever is spawning the bridges chain them together, instead of passing encoder in init().
For this, we have to modify drm_bridge_init to return a bridge pointer and then, as Rob is suggesting, we can have some helper functions to update next_bridge ptr to form a bridge chain, and these functions can be called from the corresponding encoder implementation.
Ajay
{
struct drm_bridge *temp; int ret; drm_modeset_lock_all(dev);@@ -931,6 +933,15 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, bridge->dev = dev; bridge->funcs = funcs;
if (encoder->bridge) {temp = encoder->bridge;while (temp->next_bridge)temp = temp->next_bridge;temp->next_bridge = bridge;} elseencoder->bridge = bridge;list_add_tail(&bridge->head, &dev->mode_config.bridge_list); dev->mode_config.num_bridge++;diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e55fccb..bb6ed88 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -619,6 +619,7 @@ struct drm_bridge_funcs { struct drm_bridge { struct drm_device *dev; struct list_head head;
struct drm_bridge *next_bridge; struct drm_mode_object base;@@ -862,6 +863,7 @@ extern void drm_connector_cleanup(struct drm_connector *connector); extern void drm_connector_unplug_all(struct drm_device *dev);
extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
struct drm_encoder *encoder, const struct drm_bridge_funcs *funcs);extern void drm_bridge_cleanup(struct drm_bridge *bridge);
-- 1.8.1.2
On Tue, May 6, 2014 at 12:45 PM, Ajay kumar ajaynumb@gmail.com wrote:
Sean,
On Tue, May 6, 2014 at 9:25 PM, Sean Paul seanpaul@chromium.org wrote:
On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar ajaykumar.rs@samsung.com wrote:
As of now, we can have only one bridge hanging off the encoder. With this patch, we allow multiple bridges to hang onto the same encoder with the use of a simple next_bridge ptr.
The drm core calls bridge->funcs for the first bridge which is attached to it, and its upto the individual bridge drivers to call bridge->funcs for the next bridge in the chain.
Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com
drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++- include/drm/drm_crtc.h | 2 ++ 2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d8b7099..fe9905f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -918,8 +918,10 @@ EXPORT_SYMBOL(drm_connector_unplug_all);
- Zero on success, error code on failure.
*/ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
const struct drm_bridge_funcs *funcs)
struct drm_encoder *encoder,const struct drm_bridge_funcs *funcs)IMO, we should let whoever is spawning the bridges chain them together, instead of passing encoder in init().
For this, we have to modify drm_bridge_init to return a bridge pointer
Sorry, I don't follow this. You are passing in the bridge pointer, so presumably you already have it.
Sean
and then, as Rob is suggesting, we can have some helper functions to update next_bridge ptr to form a bridge chain, and these functions can be called from the corresponding encoder implementation.
Ajay
{
struct drm_bridge *temp; int ret; drm_modeset_lock_all(dev);@@ -931,6 +933,15 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, bridge->dev = dev; bridge->funcs = funcs;
if (encoder->bridge) {temp = encoder->bridge;while (temp->next_bridge)temp = temp->next_bridge;temp->next_bridge = bridge;} elseencoder->bridge = bridge;list_add_tail(&bridge->head, &dev->mode_config.bridge_list); dev->mode_config.num_bridge++;diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e55fccb..bb6ed88 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -619,6 +619,7 @@ struct drm_bridge_funcs { struct drm_bridge { struct drm_device *dev; struct list_head head;
struct drm_bridge *next_bridge; struct drm_mode_object base;@@ -862,6 +863,7 @@ extern void drm_connector_cleanup(struct drm_connector *connector); extern void drm_connector_unplug_all(struct drm_device *dev);
extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
struct drm_encoder *encoder, const struct drm_bridge_funcs *funcs);extern void drm_bridge_cleanup(struct drm_bridge *bridge);
-- 1.8.1.2
On Wed, May 7, 2014 at 1:33 AM, Sean Paul seanpaul@chromium.org wrote:
On Tue, May 6, 2014 at 12:45 PM, Ajay kumar ajaynumb@gmail.com wrote:
Sean,
On Tue, May 6, 2014 at 9:25 PM, Sean Paul seanpaul@chromium.org wrote:
On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar ajaykumar.rs@samsung.com wrote:
As of now, we can have only one bridge hanging off the encoder. With this patch, we allow multiple bridges to hang onto the same encoder with the use of a simple next_bridge ptr.
The drm core calls bridge->funcs for the first bridge which is attached to it, and its upto the individual bridge drivers to call bridge->funcs for the next bridge in the chain.
Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com
drivers/gpu/drm/drm_crtc.c | 13 ++++++++++++- include/drm/drm_crtc.h | 2 ++ 2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d8b7099..fe9905f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -918,8 +918,10 @@ EXPORT_SYMBOL(drm_connector_unplug_all);
- Zero on success, error code on failure.
*/ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
const struct drm_bridge_funcs *funcs)
struct drm_encoder *encoder,const struct drm_bridge_funcs *funcs)IMO, we should let whoever is spawning the bridges chain them together, instead of passing encoder in init().
For this, we have to modify drm_bridge_init to return a bridge pointer
Sorry, I don't follow this. You are passing in the bridge pointer, so presumably you already have it.
Ahh, Sorry for that. It should be ptn3460_init, and not drm_bridge_init :) I mean, ptn3460_init shall return a bridge pointer and exynos_dp(encoder) should make use of it to prepare the bridge chain.
Ajay
Sean
and then, as Rob is suggesting, we can have some helper functions to update next_bridge ptr to form a bridge chain, and these functions can be called from the corresponding encoder implementation.
Ajay
{
struct drm_bridge *temp; int ret; drm_modeset_lock_all(dev);@@ -931,6 +933,15 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, bridge->dev = dev; bridge->funcs = funcs;
if (encoder->bridge) {temp = encoder->bridge;while (temp->next_bridge)temp = temp->next_bridge;temp->next_bridge = bridge;} elseencoder->bridge = bridge;list_add_tail(&bridge->head, &dev->mode_config.bridge_list); dev->mode_config.num_bridge++;diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e55fccb..bb6ed88 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -619,6 +619,7 @@ struct drm_bridge_funcs { struct drm_bridge { struct drm_device *dev; struct list_head head;
struct drm_bridge *next_bridge; struct drm_mode_object base;@@ -862,6 +863,7 @@ extern void drm_connector_cleanup(struct drm_connector *connector); extern void drm_connector_unplug_all(struct drm_device *dev);
extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge,
struct drm_encoder *encoder, const struct drm_bridge_funcs *funcs);extern void drm_bridge_cleanup(struct drm_bridge *bridge);
-- 1.8.1.2
implement basic panel controls as a drm_bridge so that the existing bridges can make use of it.
The driver assumes that it is the last entity in the bridge chain.
Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com --- .../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ include/drm/bridge/bridge_panel.h | 37 ++++ 5 files changed, 329 insertions(+) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h
diff --git a/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt b/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt new file mode 100644 index 0000000..0f916b0 --- /dev/null +++ b/Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt @@ -0,0 +1,45 @@ +Simple panel interface for chaining along with bridges + +Required properties: + - compatible: "drm-bridge,panel" + +Optional properties: + -lcd-en-gpio: + eDP panel LCD poweron GPIO. + Indicates which GPIO needs to be powered up as output + to powerup/enable the switch to the LCD panel. + -led-en-gpio: + eDP panel LED enable GPIO. + Indicates which GPIO needs to be powered up as output + to enable the backlight. + -panel-pre-enable-delay: + delay value in ms required for panel_pre_enable process + Delay in ms needed for the eDP panel LCD unit to + powerup, and delay needed between panel_VCC and + video_enable. + -panel-enable-delay: + delay value in ms required for panel_enable process + Delay in ms needed for the eDP panel backlight/LED unit + to powerup, and delay needed between video_enable and + BL_EN. + -panel-disable-delay: + delay value in ms required for panel_disable process + Delay in ms needed for the eDP panel backlight/LED unit + powerdown, and delay needed between BL_DISABLE and + video_disable. + -panel-post-disable-delay: + delay value in ms required for panel_post_disable process + Delay in ms needed for the eDP panel LCD unit to + to powerdown, and delay between video_disable and + panel_VCC going down. + +Example: + + bridge-panel { + compatible = "drm-bridge,panel"; + led-en-gpio = <&gpx3 0 1>; + panel-pre-enable-delay = <40>; + panel-enable-delay = <20>; + panel-disable-delay = <20>; + panel-post-disable-delay = <30>; + }; diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 884923f..654c5ea 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -3,3 +3,9 @@ config DRM_PTN3460 depends on DRM select DRM_KMS_HELPER ---help--- + +config DRM_BRIDGE_PANEL + tristate "dummy bridge panel" + depends on DRM + select DRM_KMS_HELPER + ---help--- diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index b4733e1..bf433cf 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -1,3 +1,4 @@ ccflags-y := -Iinclude/drm
obj-$(CONFIG_DRM_PTN3460) += ptn3460.o +obj-$(CONFIG_DRM_BRIDGE_PANEL) += bridge_panel.o diff --git a/drivers/gpu/drm/bridge/bridge_panel.c b/drivers/gpu/drm/bridge/bridge_panel.c new file mode 100644 index 0000000..807118a --- /dev/null +++ b/drivers/gpu/drm/bridge/bridge_panel.c @@ -0,0 +1,240 @@ +/* + * Copyright (C) 2014 Samsung Electronics Co., Ltd. + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_gpio.h> +#include <linux/gpio.h> +#include <linux/delay.h> +#include <linux/regulator/consumer.h> + +#include "drmP.h" + +#include "bridge/bridge_panel.h" + +enum panel_state { + PRE_ENABLE, + ENABLE, + DISABLE, + POST_DISABLE, +}; + +struct bridge_panel { + struct drm_connector connector; + struct i2c_client *client; + struct drm_encoder *encoder; + struct drm_bridge *bridge; + struct regulator *backlight_fet; + struct regulator *lcd_fet; + int led_en_gpio; + int lcd_en_gpio; + int panel_pre_enable_delay; + int panel_enable_delay; + int panel_disable_delay; + int panel_post_disable_delay; + enum panel_state panel_state; +}; + +static void bridge_panel_pre_enable(struct drm_bridge *bridge) +{ + struct bridge_panel *panel = bridge->driver_private; + + if (panel->panel_state != POST_DISABLE) { + DRM_ERROR("invoking bridge_panel_pre_enable " \ + "from an improper state\n"); + return; + } + + if (!IS_ERR_OR_NULL(panel->lcd_fet)) + if (regulator_enable(panel->lcd_fet)) + DRM_ERROR("Failed to enable LCD fet\n"); + + if (gpio_is_valid(panel->lcd_en_gpio)) + gpio_set_value(panel->lcd_en_gpio, 1); + + msleep(panel->panel_pre_enable_delay); + + panel->panel_state = PRE_ENABLE; +} + +static void bridge_panel_enable(struct drm_bridge *bridge) +{ + struct bridge_panel *panel = bridge->driver_private; + + if (panel->panel_state != PRE_ENABLE) { + DRM_ERROR("invoking bridge_panel_enable " \ + "from an improper state\n"); + return; + } + + if (!IS_ERR_OR_NULL(panel->backlight_fet)) + if (regulator_enable(panel->backlight_fet)) + DRM_ERROR("Failed to enable LED fet\n"); + + msleep(panel->panel_enable_delay); + + if (gpio_is_valid(panel->led_en_gpio)) + gpio_set_value(panel->led_en_gpio, 1); + + panel->panel_state = ENABLE; + +} + +static void bridge_panel_disable(struct drm_bridge *bridge) +{ + struct bridge_panel *panel = bridge->driver_private; + + if (panel->panel_state != ENABLE) { + DRM_ERROR("invoking bridge_panel_disable " \ + "from an improper state\n"); + return; + } + + if (gpio_is_valid(panel->led_en_gpio)) + gpio_set_value(panel->led_en_gpio, 0); + + if (!IS_ERR_OR_NULL(panel->backlight_fet)) + regulator_disable(panel->backlight_fet); + + msleep(panel->panel_disable_delay); + + panel->panel_state = DISABLE; +} + +static void bridge_panel_post_disable(struct drm_bridge *bridge) +{ + struct bridge_panel *panel = bridge->driver_private; + + if (panel->panel_state != DISABLE) { + DRM_ERROR("invoking bridge_panel_post_disable " \ + "from an improper state\n"); + return; + } + + if (gpio_is_valid(panel->lcd_en_gpio)) + gpio_set_value(panel->lcd_en_gpio, 0); + + if (!IS_ERR_OR_NULL(panel->lcd_fet)) + regulator_disable(panel->lcd_fet); + + msleep(panel->panel_post_disable_delay); + + panel->panel_state = POST_DISABLE; +} + +void bridge_panel_destroy(struct drm_bridge *bridge) +{ + struct bridge_panel *panel = bridge->driver_private; + + drm_bridge_cleanup(bridge); + + if (gpio_is_valid(panel->lcd_en_gpio)) + gpio_free(panel->lcd_en_gpio); + if (gpio_is_valid(panel->led_en_gpio)) + gpio_free(panel->led_en_gpio); + /* Nothing else to free, we've got devm allocated memory */ +} + +struct drm_bridge_funcs bridge_panel_funcs = { + .pre_enable = bridge_panel_pre_enable, + .enable = bridge_panel_enable, + .disable = bridge_panel_disable, + .post_disable = bridge_panel_post_disable, + .destroy = bridge_panel_destroy, +}; + +int bridge_panel_init(struct drm_device *dev, struct drm_encoder *encoder, + struct i2c_client *client, struct device_node *node) +{ + int ret; + struct drm_bridge *bridge; + struct bridge_panel *panel; + + bridge = devm_kzalloc(dev->dev, sizeof(*bridge), GFP_KERNEL); + if (!bridge) { + DRM_ERROR("Failed to allocate drm bridge\n"); + return -ENOMEM; + } + + panel = devm_kzalloc(dev->dev, sizeof(*panel), GFP_KERNEL); + if (!panel) { + DRM_ERROR("Failed to allocate bridge panel\n"); + return -ENOMEM; + } + + panel->client = client; + panel->encoder = encoder; + panel->bridge = bridge; + + panel->panel_state = POST_DISABLE; + + panel->lcd_en_gpio = of_get_named_gpio(node, "lcd-en-gpio", 0); + panel->lcd_en_gpio = of_get_named_gpio(node, "led-en-gpio", 0); + + of_property_read_u32(node, "panel-pre-enable-delay", + &panel->panel_pre_enable_delay); + of_property_read_u32(node, "panel-enable-delay", + &panel->panel_enable_delay); + of_property_read_u32(node, "panel-disable-delay", + &panel->panel_disable_delay); + of_property_read_u32(node, "panel-post-disable-delay", + &panel->panel_post_disable_delay); + + panel->lcd_fet = devm_regulator_get(dev->dev, "lcd_vdd"); + if (IS_ERR(panel->lcd_fet)) + return PTR_ERR(panel->lcd_fet); + + panel->backlight_fet = devm_regulator_get(dev->dev, "vcd_led"); + if (IS_ERR(panel->backlight_fet)) + return PTR_ERR(panel->backlight_fet); + + if (gpio_is_valid(panel->lcd_en_gpio)) { + ret = devm_gpio_request_one(dev->dev, panel->lcd_en_gpio, + GPIOF_OUT_INIT_LOW, "lcd_en_gpio"); + if (ret) { + DRM_ERROR("failed to get lcd-en gpio [%d]\n", ret); + return ret; + } + } else { + panel->lcd_en_gpio = -ENODEV; + } + + if (gpio_is_valid(panel->led_en_gpio)) { + ret = devm_gpio_request_one(dev->dev, panel->led_en_gpio, + GPIOF_OUT_INIT_LOW, "led_en_gpio"); + if (ret) { + DRM_ERROR("failed to get led-en gpio [%d]\n", ret); + return ret; + } + } else { + panel->led_en_gpio = -ENODEV; + } + + ret = drm_bridge_init(dev, bridge, encoder, &bridge_panel_funcs); + if (ret) { + DRM_ERROR("Failed to initialize bridge with drm\n"); + goto err; + } + + bridge->driver_private = panel; + + return 0; + +err: + if (gpio_is_valid(panel->lcd_en_gpio)) + gpio_free(panel->lcd_en_gpio); + if (gpio_is_valid(panel->led_en_gpio)) + gpio_free(panel->led_en_gpio); + return ret; +} +EXPORT_SYMBOL(bridge_panel_init); diff --git a/include/drm/bridge/bridge_panel.h b/include/drm/bridge/bridge_panel.h new file mode 100644 index 0000000..4a1b218 --- /dev/null +++ b/include/drm/bridge/bridge_panel.h @@ -0,0 +1,37 @@ +/* + * Copyright (C) 2014 Samsung Electronics Co., Ltd. + * + * This software is licensed under the terms of the GNU General Public + * License version 2, as published by the Free Software Foundation, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _DRM_BRIDGE_PANEL_H_ +#define _DRM_BRIDGE_PANEL_H_ + +struct drm_device; +struct drm_encoder; +struct i2c_client; +struct device_node; + +#if defined(CONFIG_DRM_BRIDGE_PANEL) + +int bridge_panel_init(struct drm_device *dev, struct drm_encoder *encoder, + struct i2c_client *client, struct device_node *node); +#else + +static inline int bridge_panel_init(struct drm_device *dev, + struct drm_encoder *encoder, struct i2c_client *client, + struct device_node *node) +{ + return 0; +} + +#endif + +#endif
modify the driver to call the bridge->funcs of the next bridge in the chain. We assume that the bridge sitting next to ptn3460 is a bridge-panel.
Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com --- drivers/gpu/drm/bridge/ptn3460.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c index b171901..969869a 100644 --- a/drivers/gpu/drm/bridge/ptn3460.c +++ b/drivers/gpu/drm/bridge/ptn3460.c @@ -126,6 +126,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge) gpio_set_value(ptn_bridge->gpio_rst_n, 1); }
+ if (bridge->next_bridge) + bridge->next_bridge->funcs->pre_enable(bridge->next_bridge); /* * There's a bug in the PTN chip where it falsely asserts hotplug before * it is fully functional. We're forced to wait for the maximum start up @@ -142,6 +144,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
static void ptn3460_enable(struct drm_bridge *bridge) { + if (bridge->next_bridge) + bridge->next_bridge->funcs->enable(bridge->next_bridge); }
static void ptn3460_disable(struct drm_bridge *bridge) @@ -153,6 +157,11 @@ static void ptn3460_disable(struct drm_bridge *bridge)
ptn_bridge->enabled = false;
+ if (bridge->next_bridge) { + bridge->next_bridge->funcs->disable(bridge->next_bridge); + bridge->next_bridge->funcs->post_disable(bridge->next_bridge); + } + if (gpio_is_valid(ptn_bridge->gpio_rst_n)) gpio_set_value(ptn_bridge->gpio_rst_n, 1);
@@ -197,7 +206,10 @@ int ptn3460_get_modes(struct drm_connector *connector) return drm_add_edid_modes(connector, ptn_bridge->edid);
power_off = !ptn_bridge->enabled; - ptn3460_pre_enable(ptn_bridge->bridge); + if (power_off) { + ptn3460_pre_enable(ptn_bridge->bridge); + ptn3460_enable(ptn_bridge->bridge); + }
edid = kmalloc(EDID_LENGTH, GFP_KERNEL); if (!edid) { @@ -219,8 +231,10 @@ int ptn3460_get_modes(struct drm_connector *connector) num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
out: - if (power_off) + if (power_off) { ptn3460_disable(ptn_bridge->bridge); + ptn3460_post_disable(ptn_bridge->bridge); + }
return num_modes; } @@ -318,14 +332,13 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, goto err; }
- ret = drm_bridge_init(dev, bridge, &ptn3460_bridge_funcs); + ret = drm_bridge_init(dev, bridge, encoder, &ptn3460_bridge_funcs); if (ret) { DRM_ERROR("Failed to initialize bridge with drm\n"); goto err; }
bridge->driver_private = ptn_bridge; - encoder->bridge = bridge;
ret = drm_connector_init(dev, &ptn_bridge->connector, &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar ajaykumar.rs@samsung.com wrote:
modify the driver to call the bridge->funcs of the next bridge in the chain. We assume that the bridge sitting next to ptn3460 is a bridge-panel.
Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com
drivers/gpu/drm/bridge/ptn3460.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c index b171901..969869a 100644 --- a/drivers/gpu/drm/bridge/ptn3460.c +++ b/drivers/gpu/drm/bridge/ptn3460.c @@ -126,6 +126,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge) gpio_set_value(ptn_bridge->gpio_rst_n, 1); }
if (bridge->next_bridge)bridge->next_bridge->funcs->pre_enable(bridge->next_bridge); /* * There's a bug in the PTN chip where it falsely asserts hotplug before * it is fully functional. We're forced to wait for the maximum start up@@ -142,6 +144,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
static void ptn3460_enable(struct drm_bridge *bridge) {
if (bridge->next_bridge)bridge->next_bridge->funcs->enable(bridge->next_bridge);}
static void ptn3460_disable(struct drm_bridge *bridge) @@ -153,6 +157,11 @@ static void ptn3460_disable(struct drm_bridge *bridge)
ptn_bridge->enabled = false;
if (bridge->next_bridge) {bridge->next_bridge->funcs->disable(bridge->next_bridge);bridge->next_bridge->funcs->post_disable(bridge->next_bridge);
Shouldn't post_disable be called from ptn_3460_post_disable, instead of here?
}if (gpio_is_valid(ptn_bridge->gpio_rst_n)) gpio_set_value(ptn_bridge->gpio_rst_n, 1);@@ -197,7 +206,10 @@ int ptn3460_get_modes(struct drm_connector *connector) return drm_add_edid_modes(connector, ptn_bridge->edid);
power_off = !ptn_bridge->enabled;
ptn3460_pre_enable(ptn_bridge->bridge);
if (power_off) {ptn3460_pre_enable(ptn_bridge->bridge);ptn3460_enable(ptn_bridge->bridge);
In this case, I don't think we need to power on the entire bridge chain since we're just reading the edid from the bridge chip itself. So I think I'd prefer to break out the power_on/power_off code from the bridge chain such that we can just power up the bridge chip, check the edid and then turn it off.
In other bridges, where we're actually reading the edid from a downstream source, this decision might be different.
} edid = kmalloc(EDID_LENGTH, GFP_KERNEL); if (!edid) {@@ -219,8 +231,10 @@ int ptn3460_get_modes(struct drm_connector *connector) num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
out:
if (power_off)
if (power_off) { ptn3460_disable(ptn_bridge->bridge);ptn3460_post_disable(ptn_bridge->bridge);} return num_modes;} @@ -318,14 +332,13 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, goto err; }
ret = drm_bridge_init(dev, bridge, &ptn3460_bridge_funcs);
ret = drm_bridge_init(dev, bridge, encoder, &ptn3460_bridge_funcs); if (ret) { DRM_ERROR("Failed to initialize bridge with drm\n"); goto err; } bridge->driver_private = ptn_bridge;
encoder->bridge = bridge; ret = drm_connector_init(dev, &ptn_bridge->connector, &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS);-- 1.8.1.2
Sean,
On Tue, May 6, 2014 at 9:24 PM, Sean Paul seanpaul@chromium.org wrote:
On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar ajaykumar.rs@samsung.com wrote:
modify the driver to call the bridge->funcs of the next bridge in the chain. We assume that the bridge sitting next to ptn3460 is a bridge-panel.
Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com
drivers/gpu/drm/bridge/ptn3460.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c index b171901..969869a 100644 --- a/drivers/gpu/drm/bridge/ptn3460.c +++ b/drivers/gpu/drm/bridge/ptn3460.c @@ -126,6 +126,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge) gpio_set_value(ptn_bridge->gpio_rst_n, 1); }
if (bridge->next_bridge)bridge->next_bridge->funcs->pre_enable(bridge->next_bridge); /* * There's a bug in the PTN chip where it falsely asserts hotplug before * it is fully functional. We're forced to wait for the maximum start up@@ -142,6 +144,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
static void ptn3460_enable(struct drm_bridge *bridge) {
if (bridge->next_bridge)bridge->next_bridge->funcs->enable(bridge->next_bridge);}
static void ptn3460_disable(struct drm_bridge *bridge) @@ -153,6 +157,11 @@ static void ptn3460_disable(struct drm_bridge *bridge)
ptn_bridge->enabled = false;
if (bridge->next_bridge) {bridge->next_bridge->funcs->disable(bridge->next_bridge);bridge->next_bridge->funcs->post_disable(bridge->next_bridge);Shouldn't post_disable be called from ptn_3460_post_disable, instead of here?
Umm, right. But no point in delaying ->post_disable of the panel(which switches off power to LCD) since backlight would be already down in ->disable call itself. So, I thought of calling post_disable here itself.
}if (gpio_is_valid(ptn_bridge->gpio_rst_n)) gpio_set_value(ptn_bridge->gpio_rst_n, 1);@@ -197,7 +206,10 @@ int ptn3460_get_modes(struct drm_connector *connector) return drm_add_edid_modes(connector, ptn_bridge->edid);
power_off = !ptn_bridge->enabled;
ptn3460_pre_enable(ptn_bridge->bridge);
if (power_off) {ptn3460_pre_enable(ptn_bridge->bridge);ptn3460_enable(ptn_bridge->bridge);In this case, I don't think we need to power on the entire bridge chain since we're just reading the edid from the bridge chip itself. So I think I'd prefer to break out the power_on/power_off code from the bridge chain such that we can just power up the bridge chip, check the edid and then turn it off.
Those extra calls were added to make sure that regulator_enable/disable would be in sync for the panel. Check the other patch [2/3]. Another way of doing this is to just check if the bridge is off and switch it on by explicitly setting up the gpios here, instead of just calling ptn3460_pre_enable.
Ajay
In other bridges, where we're actually reading the edid from a downstream source, this decision might be different.
} edid = kmalloc(EDID_LENGTH, GFP_KERNEL); if (!edid) {@@ -219,8 +231,10 @@ int ptn3460_get_modes(struct drm_connector *connector) num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
out:
if (power_off)
if (power_off) { ptn3460_disable(ptn_bridge->bridge);ptn3460_post_disable(ptn_bridge->bridge);} return num_modes;} @@ -318,14 +332,13 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, goto err; }
ret = drm_bridge_init(dev, bridge, &ptn3460_bridge_funcs);
ret = drm_bridge_init(dev, bridge, encoder, &ptn3460_bridge_funcs); if (ret) { DRM_ERROR("Failed to initialize bridge with drm\n"); goto err; } bridge->driver_private = ptn_bridge;
encoder->bridge = bridge; ret = drm_connector_init(dev, &ptn_bridge->connector, &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS);-- 1.8.1.2
On Tue, May 6, 2014 at 1:03 PM, Ajay kumar ajaynumb@gmail.com wrote:
Sean,
On Tue, May 6, 2014 at 9:24 PM, Sean Paul seanpaul@chromium.org wrote:
On Mon, May 5, 2014 at 12:52 PM, Ajay Kumar ajaykumar.rs@samsung.com wrote:
modify the driver to call the bridge->funcs of the next bridge in the chain. We assume that the bridge sitting next to ptn3460 is a bridge-panel.
Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com
drivers/gpu/drm/bridge/ptn3460.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c index b171901..969869a 100644 --- a/drivers/gpu/drm/bridge/ptn3460.c +++ b/drivers/gpu/drm/bridge/ptn3460.c @@ -126,6 +126,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge) gpio_set_value(ptn_bridge->gpio_rst_n, 1); }
if (bridge->next_bridge)bridge->next_bridge->funcs->pre_enable(bridge->next_bridge); /* * There's a bug in the PTN chip where it falsely asserts hotplug before * it is fully functional. We're forced to wait for the maximum start up@@ -142,6 +144,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
static void ptn3460_enable(struct drm_bridge *bridge) {
if (bridge->next_bridge)bridge->next_bridge->funcs->enable(bridge->next_bridge);}
static void ptn3460_disable(struct drm_bridge *bridge) @@ -153,6 +157,11 @@ static void ptn3460_disable(struct drm_bridge *bridge)
ptn_bridge->enabled = false;
if (bridge->next_bridge) {bridge->next_bridge->funcs->disable(bridge->next_bridge);bridge->next_bridge->funcs->post_disable(bridge->next_bridge);Shouldn't post_disable be called from ptn_3460_post_disable, instead of here?
Umm, right. But no point in delaying ->post_disable of the panel(which switches off power to LCD) since backlight would be already down in ->disable call itself. So, I thought of calling post_disable here itself.
}if (gpio_is_valid(ptn_bridge->gpio_rst_n)) gpio_set_value(ptn_bridge->gpio_rst_n, 1);@@ -197,7 +206,10 @@ int ptn3460_get_modes(struct drm_connector *connector) return drm_add_edid_modes(connector, ptn_bridge->edid);
power_off = !ptn_bridge->enabled;
ptn3460_pre_enable(ptn_bridge->bridge);
if (power_off) {ptn3460_pre_enable(ptn_bridge->bridge);ptn3460_enable(ptn_bridge->bridge);In this case, I don't think we need to power on the entire bridge chain since we're just reading the edid from the bridge chip itself. So I think I'd prefer to break out the power_on/power_off code from the bridge chain such that we can just power up the bridge chip, check the edid and then turn it off.
Those extra calls were added to make sure that regulator_enable/disable would be in sync for the panel. Check the other patch [2/3].
Sure, but we don't need the panel to read the edid. This bridge in particular provides the edid itself, so in this case, we should leave the panel off.
Sean
Another way of doing this is to just check if the bridge is off and switch it on by explicitly setting up the gpios here, instead of just calling ptn3460_pre_enable.
Ajay
In other bridges, where we're actually reading the edid from a downstream source, this decision might be different.
} edid = kmalloc(EDID_LENGTH, GFP_KERNEL); if (!edid) {@@ -219,8 +231,10 @@ int ptn3460_get_modes(struct drm_connector *connector) num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
out:
if (power_off)
if (power_off) { ptn3460_disable(ptn_bridge->bridge);ptn3460_post_disable(ptn_bridge->bridge);} return num_modes;} @@ -318,14 +332,13 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, goto err; }
ret = drm_bridge_init(dev, bridge, &ptn3460_bridge_funcs);
ret = drm_bridge_init(dev, bridge, encoder, &ptn3460_bridge_funcs); if (ret) { DRM_ERROR("Failed to initialize bridge with drm\n"); goto err; } bridge->driver_private = ptn_bridge;
encoder->bridge = bridge; ret = drm_connector_init(dev, &ptn_bridge->connector, &ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS);-- 1.8.1.2
On 05/05/2014 09:52 PM, Ajay Kumar wrote:
This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
I have just put up Rob's and Sean's idea of chaining up the bridges in code, and have implemented basic panel controls as a chained bridge. This works well with ptn3460 bridge chip on exynos5250-snow board.
Still need to make use of standard list calls and figure out proper way of deleting the bridge chain. So, this is just a rough version.
As I understand this patchset tries to solve two things: 1. Implement panel as drm_bridge, to ease support for hardware chains: Crtc -> Encoder -> Bridge -> Panel 2. Add support to drm_bridge chaining, to allow software chains: drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
It is done using Russian doll schema, ops from the bridge calls the same ops from the next bridge and the next bridge ops can do the same.
This schema means that all the bridges including the last one are seen from the drm core point of view as a one big drm_bridge. Additionally in this particular case, the first bridge (ptn3460) implements connector so it is hard to guess what is the location of the 2nd bridge in video stream chain, sometimes it is after the connector, sometimes before. All this is quite confusing.
But if you look at the bridge from upstream video interface point of view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its video input side acts as a panel. On the output side it expects a panel, lvds panel in this case.
So why not implement ptn3460 bridge as drm_panel which internally uses another drm_panel. With this approach everything fits much better. You do not need those (pre|post)_(enable|disable) calls, you do not need to implement connector in the bridge and you have a driver following linux driver model. And no single bit changed in drm core.
I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. It was not accepted as Inki preferred drm_bridge but as I see the problems with drm_bridges I have decide to attract attention to much more cleaner solution.
[1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559 [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
Regards Andrzej
Ajay Kumar (3): [RFC V2 1/3] drm: implement chaining of drm bridges [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
.../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ drivers/gpu/drm/bridge/ptn3460.c | 21 +- drivers/gpu/drm/drm_crtc.c | 13 +- include/drm/bridge/bridge_panel.h | 37 ++++ include/drm/drm_crtc.h | 2 + 8 files changed, 360 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h
Hi Andrzej
On 2014년 05월 08일 15:41, Andrzej Hajda wrote:
On 05/05/2014 09:52 PM, Ajay Kumar wrote:
This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
I have just put up Rob's and Sean's idea of chaining up the bridges in code, and have implemented basic panel controls as a chained bridge. This works well with ptn3460 bridge chip on exynos5250-snow board.
Still need to make use of standard list calls and figure out proper way of deleting the bridge chain. So, this is just a rough version.
As I understand this patchset tries to solve two things:
- Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel 2. Add support to drm_bridge chaining, to allow software chains: drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
It is done using Russian doll schema, ops from the bridge calls the same ops from the next bridge and the next bridge ops can do the same.
This schema means that all the bridges including the last one are seen from the drm core point of view as a one big drm_bridge. Additionally in this particular case, the first bridge (ptn3460) implements connector so it is hard to guess what is the location of the 2nd bridge in video stream chain, sometimes it is after the connector, sometimes before. All this is quite confusing.
But if you look at the bridge from upstream video interface point of view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its video input side acts as a panel. On the output side it expects a panel, lvds panel in this case.
So why not implement ptn3460 bridge as drm_panel which internally uses another drm_panel. With this approach everything fits much better. You do not need those (pre|post)_(enable|disable) calls, you do not need to implement connector in the bridge and you have a driver following linux driver model. And no single bit changed in drm core.
I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. It was not accepted as Inki preferred drm_bridge but as I see the problems with drm_bridges I have decide to attract attention to much
Yes, in above email threads, I disagreed to using drm_panel framework for bridge device, especially, to implement connector/encoder to crtc driver.
However, I thought that it'd be more generic way that lvds drivers use driver-model, and the use of drm_panel infrastructure would be suitable to doing this.
So my intention in turn, was that LVDS driver uses integrated drm_bridge based on drm_panel infrastructure[1], and RFC patch[2] for it. This way is same as your proposal in the point that LVDS and Panel drivers use driver-model. The only different point is that LVDS driver has some ops specific to LVDS device, not using existing ops of drm_panel commonly: we may need to consider the characteristic of LVDS device.
[1]:http://www.spinics.net/lists/dri-devel/msg55555.html [2]:http://www.spinics.net/lists/dri-devel/msg55658.html
Thanks, Inki Dae
more cleaner solution.
Regards Andrzej
Ajay Kumar (3): [RFC V2 1/3] drm: implement chaining of drm bridges [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
.../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ drivers/gpu/drm/bridge/ptn3460.c | 21 +- drivers/gpu/drm/drm_crtc.c | 13 +- include/drm/bridge/bridge_panel.h | 37 ++++ include/drm/drm_crtc.h | 2 + 8 files changed, 360 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h
-- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Just re-sending with text mode. Sorry for this.
On 2014년 05월 08일 15:41, Andrzej Hajda wrote:
On 05/05/2014 09:52 PM, Ajay Kumar wrote:
This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
I have just put up Rob's and Sean's idea of chaining up the bridges in code, and have implemented basic panel controls as a chained bridge. This works well with ptn3460 bridge chip on exynos5250-snow board.
Still need to make use of standard list calls and figure out proper way of deleting the bridge chain. So, this is just a rough version.
As I understand this patchset tries to solve two things:
- Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel 2. Add support to drm_bridge chaining, to allow software chains: drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
It is done using Russian doll schema, ops from the bridge calls the same ops from the next bridge and the next bridge ops can do the same.
This schema means that all the bridges including the last one are seen from the drm core point of view as a one big drm_bridge. Additionally in this particular case, the first bridge (ptn3460) implements connector so it is hard to guess what is the location of the 2nd bridge in video stream chain, sometimes it is after the connector, sometimes before. All this is quite confusing.
But if you look at the bridge from upstream video interface point of view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its video input side acts as a panel. On the output side it expects a panel, lvds panel in this case.
So why not implement ptn3460 bridge as drm_panel which internally uses another drm_panel. With this approach everything fits much better. You do not need those (pre|post)_(enable|disable) calls, you do not need to implement connector in the bridge and you have a driver following linux driver model. And no single bit changed in drm core.
I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. It was not accepted as Inki preferred drm_bridge but as I see the
Yes, in above email threads, I disagreed to using drm_panel framework for bridge device, especially, to implement connector/encoder to crtc driver.
However, I thought that it'd be more generic way that lvds drivers use driver-model, and the use of drm_panel infrastructure would be suitable to doing this.
So my intention in turn, was that LVDS driver uses integrated drm_bridge based on drm_panel infrastructure[1], and RFC patch[2] for it. This way is same as your proposal in the point that LVDS and Panel drivers use driver-model. The only different point is that LVDS driver has some ops specific to LVDS device, not using existing ops of drm_panel commonly: we may need to consider the characteristic of LVDS device.
[1]:http://www.spinics.net/lists/dri-devel/msg55555.html [2]:http://www.spinics.net/lists/dri-devel/msg55658.html
Thanks, Inki Dae
problems with drm_bridges I have decide to attract attention to much more cleaner solution.
Regards Andrzej
Ajay Kumar (3): [RFC V2 1/3] drm: implement chaining of drm bridges [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
.../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ drivers/gpu/drm/bridge/ptn3460.c | 21 +- drivers/gpu/drm/drm_crtc.c | 13 +- include/drm/bridge/bridge_panel.h | 37 ++++ include/drm/drm_crtc.h | 2 + 8 files changed, 360 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h
-- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
+Dave +Thierry
On Thu, May 8, 2014 at 1:14 PM, Inki Dae inki.dae@samsung.com wrote:
Just re-sending with text mode. Sorry for this.
On 2014년 05월 08일 15:41, Andrzej Hajda wrote:
On 05/05/2014 09:52 PM, Ajay Kumar wrote:
This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
I have just put up Rob's and Sean's idea of chaining up the bridges in code, and have implemented basic panel controls as a chained bridge. This works well with ptn3460 bridge chip on exynos5250-snow board.
Still need to make use of standard list calls and figure out proper way of deleting the bridge chain. So, this is just a rough version.
As I understand this patchset tries to solve two things:
- Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel 2. Add support to drm_bridge chaining, to allow software chains: drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
It is done using Russian doll schema, ops from the bridge calls the same ops from the next bridge and the next bridge ops can do the same.
This schema means that all the bridges including the last one are seen from the drm core point of view as a one big drm_bridge. Additionally in this particular case, the first bridge (ptn3460) implements connector so it is hard to guess what is the location of the 2nd bridge in video stream chain, sometimes it is after the connector, sometimes before. All this is quite confusing.
But if you look at the bridge from upstream video interface point of view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its video input side acts as a panel. On the output side it expects a panel, lvds panel in this case.
So why not implement ptn3460 bridge as drm_panel which internally uses another drm_panel. With this approach everything fits much better. You do not need those (pre|post)_(enable|disable) calls, you do not need to implement connector in the bridge and you have a driver following linux driver model. And no single bit changed in drm core.
I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. It was not accepted as Inki preferred drm_bridge but as I see the
Yes, in above email threads, I disagreed to using drm_panel framework for bridge device, especially, to implement connector/encoder to crtc driver.
However, I thought that it'd be more generic way that lvds drivers use driver-model, and the use of drm_panel infrastructure would be suitable to doing this.
So my intention in turn, was that LVDS driver uses integrated drm_bridge based on drm_panel infrastructure[1], and RFC patch[2] for it. This way is same as your proposal in the point that LVDS and Panel drivers use driver-model. The only different point is that LVDS driver has some ops specific to LVDS device, not using existing ops of drm_panel commonly: we may need to consider the characteristic of LVDS device.
Thanks, Inki Dae
I am just consolidating the discussion happening here. 1) This patchset started from a discussion wherein I tried to combine drm_panel with drm_bridge. https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg28943.html 2) Sean and Rob suggested to implement a chain of bridges and then consider adding basic panel controls as a bridge. 3) Andrej's idea is to drop the existing bridge infrastructure and implement ptn3460 using drm_panel, the same way he has implemented http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559. 4) Inki's suggestion is to combine drm_bridge, drm_panel and drm_enhance into a single drm_hw_block.
I am currently trying to implement (2):chaining of bridges, and I think we have not yet reached to a consensus. So adding few other people from drm community to comment.
Regards, Ajay
problems with drm_bridges I have decide to attract attention to much more cleaner solution.
Regards Andrzej
Ajay Kumar (3): [RFC V2 1/3] drm: implement chaining of drm bridges [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
.../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ drivers/gpu/drm/bridge/ptn3460.c | 21 +- drivers/gpu/drm/drm_crtc.c | 13 +- include/drm/bridge/bridge_panel.h | 37 ++++ include/drm/drm_crtc.h | 2 + 8 files changed, 360 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h
-- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014년 05월 08일 19:52, Ajay kumar wrote:
+Dave +Thierry
On Thu, May 8, 2014 at 1:14 PM, Inki Dae inki.dae@samsung.com wrote:
Just re-sending with text mode. Sorry for this.
On 2014년 05월 08일 15:41, Andrzej Hajda wrote:
On 05/05/2014 09:52 PM, Ajay Kumar wrote:
This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
I have just put up Rob's and Sean's idea of chaining up the bridges in code, and have implemented basic panel controls as a chained bridge. This works well with ptn3460 bridge chip on exynos5250-snow board.
Still need to make use of standard list calls and figure out proper way of deleting the bridge chain. So, this is just a rough version.
As I understand this patchset tries to solve two things:
- Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel 2. Add support to drm_bridge chaining, to allow software chains: drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
It is done using Russian doll schema, ops from the bridge calls the same ops from the next bridge and the next bridge ops can do the same.
This schema means that all the bridges including the last one are seen from the drm core point of view as a one big drm_bridge. Additionally in this particular case, the first bridge (ptn3460) implements connector so it is hard to guess what is the location of the 2nd bridge in video stream chain, sometimes it is after the connector, sometimes before. All this is quite confusing.
But if you look at the bridge from upstream video interface point of view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its video input side acts as a panel. On the output side it expects a panel, lvds panel in this case.
So why not implement ptn3460 bridge as drm_panel which internally uses another drm_panel. With this approach everything fits much better. You do not need those (pre|post)_(enable|disable) calls, you do not need to implement connector in the bridge and you have a driver following linux driver model. And no single bit changed in drm core.
I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. It was not accepted as Inki preferred drm_bridge but as I see the
Yes, in above email threads, I disagreed to using drm_panel framework for bridge device, especially, to implement connector/encoder to crtc driver.
However, I thought that it'd be more generic way that lvds drivers use driver-model, and the use of drm_panel infrastructure would be suitable to doing this.
So my intention in turn, was that LVDS driver uses integrated drm_bridge based on drm_panel infrastructure[1], and RFC patch[2] for it. This way is same as your proposal in the point that LVDS and Panel drivers use driver-model. The only different point is that LVDS driver has some ops specific to LVDS device, not using existing ops of drm_panel commonly: we may need to consider the characteristic of LVDS device.
Thanks, Inki Dae
I am just consolidating the discussion happening here.
- This patchset started from a discussion wherein I tried to combine
drm_panel with drm_bridge. https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg28943.html 2) Sean and Rob suggested to implement a chain of bridges and then consider adding basic panel controls as a bridge. 3) Andrej's idea is to drop the existing bridge infrastructure and implement ptn3460 using drm_panel, the same way he has implemented http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559. 4) Inki's suggestion is to combine drm_bridge, drm_panel and drm_enhance into a single drm_hw_block.
And more thing, we would need to consider image enhancer device placed between crtc and connector/encoder devices. And it'd better to rename drm_hw_block to drm_bridge, and existing drm_bridge relevant codes should be removed.
Thanks, Inki Dae
I am currently trying to implement (2):chaining of bridges, and I think we have not yet reached to a consensus. So adding few other people from drm community to comment.
Regards, Ajay
problems with drm_bridges I have decide to attract attention to much more cleaner solution.
Regards Andrzej
Ajay Kumar (3): [RFC V2 1/3] drm: implement chaining of drm bridges [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
.../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ drivers/gpu/drm/bridge/ptn3460.c | 21 +- drivers/gpu/drm/drm_crtc.c | 13 +- include/drm/bridge/bridge_panel.h | 37 ++++ include/drm/drm_crtc.h | 2 + 8 files changed, 360 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h
-- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 8, 2014 at 7:57 AM, Inki Dae inki.dae@samsung.com wrote:
On 2014년 05월 08일 19:52, Ajay kumar wrote:
+Dave +Thierry
On Thu, May 8, 2014 at 1:14 PM, Inki Dae inki.dae@samsung.com wrote:
Just re-sending with text mode. Sorry for this.
On 2014년 05월 08일 15:41, Andrzej Hajda wrote:
On 05/05/2014 09:52 PM, Ajay Kumar wrote:
This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
I have just put up Rob's and Sean's idea of chaining up the bridges in code, and have implemented basic panel controls as a chained bridge. This works well with ptn3460 bridge chip on exynos5250-snow board.
Still need to make use of standard list calls and figure out proper way of deleting the bridge chain. So, this is just a rough version.
As I understand this patchset tries to solve two things:
- Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel 2. Add support to drm_bridge chaining, to allow software chains: drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
It is done using Russian doll schema, ops from the bridge calls the same ops from the next bridge and the next bridge ops can do the same.
This schema means that all the bridges including the last one are seen from the drm core point of view as a one big drm_bridge. Additionally in this particular case, the first bridge (ptn3460) implements connector so it is hard to guess what is the location of the 2nd bridge in video stream chain, sometimes it is after the connector, sometimes before. All this is quite confusing.
But if you look at the bridge from upstream video interface point of view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its video input side acts as a panel. On the output side it expects a panel, lvds panel in this case.
So why not implement ptn3460 bridge as drm_panel which internally uses another drm_panel. With this approach everything fits much better. You do not need those (pre|post)_(enable|disable) calls, you do not need to implement connector in the bridge and you have a driver following linux driver model. And no single bit changed in drm core.
I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. It was not accepted as Inki preferred drm_bridge but as I see the
Yes, in above email threads, I disagreed to using drm_panel framework for bridge device, especially, to implement connector/encoder to crtc driver.
However, I thought that it'd be more generic way that lvds drivers use driver-model, and the use of drm_panel infrastructure would be suitable to doing this.
So my intention in turn, was that LVDS driver uses integrated drm_bridge based on drm_panel infrastructure[1], and RFC patch[2] for it. This way is same as your proposal in the point that LVDS and Panel drivers use driver-model. The only different point is that LVDS driver has some ops specific to LVDS device, not using existing ops of drm_panel commonly: we may need to consider the characteristic of LVDS device.
Thanks, Inki Dae
I am just consolidating the discussion happening here.
- This patchset started from a discussion wherein I tried to combine
drm_panel with drm_bridge. https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg28943.html 2) Sean and Rob suggested to implement a chain of bridges and then consider adding basic panel controls as a bridge. 3) Andrej's idea is to drop the existing bridge infrastructure and implement ptn3460 using drm_panel, the same way he has implemented http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559. 4) Inki's suggestion is to combine drm_bridge, drm_panel and drm_enhance into a single drm_hw_block.
And more thing, we would need to consider image enhancer device placed between crtc and connector/encoder devices. And it'd better to rename drm_hw_block to drm_bridge, and existing drm_bridge relevant codes should be removed.
I don't object to changing the name to hw_block or something else. Although to avoid introducing too much confusion in this discussion, for now I am calling it bridge/drm_bridge_funcs/etc for now ;-)
BR, -R
Thanks, Inki Dae
I am currently trying to implement (2):chaining of bridges, and I think we have not yet reached to a consensus. So adding few other people from drm community to comment.
Regards, Ajay
problems with drm_bridges I have decide to attract attention to much more cleaner solution.
Regards Andrzej
Ajay Kumar (3): [RFC V2 1/3] drm: implement chaining of drm bridges [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
.../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ drivers/gpu/drm/bridge/ptn3460.c | 21 +- drivers/gpu/drm/drm_crtc.c | 13 +- include/drm/bridge/bridge_panel.h | 37 ++++ include/drm/drm_crtc.h | 2 + 8 files changed, 360 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h
-- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 08, 2014 at 02:28:12PM -0400, Rob Clark wrote:
On Thu, May 8, 2014 at 7:57 AM, Inki Dae inki.dae@samsung.com wrote:
[...]
And more thing, we would need to consider image enhancer device placed between crtc and connector/encoder devices. And it'd better to rename drm_hw_block to drm_bridge, and existing drm_bridge relevant codes should be removed.
I don't object to changing the name to hw_block or something else. Although to avoid introducing too much confusion in this discussion, for now I am calling it bridge/drm_bridge_funcs/etc for now ;-)
FWIW, I think the name drm_bridge is fine. It describes a device that takes a video signal as an input and outputs a video signal, possibly using a different encoding.
Thierry
Hi Andrej,
On Thu, May 8, 2014 at 12:11 PM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/05/2014 09:52 PM, Ajay Kumar wrote:
This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
I have just put up Rob's and Sean's idea of chaining up the bridges in code, and have implemented basic panel controls as a chained bridge. This works well with ptn3460 bridge chip on exynos5250-snow board.
Still need to make use of standard list calls and figure out proper way of deleting the bridge chain. So, this is just a rough version.
As I understand this patchset tries to solve two things:
- Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel 2. Add support to drm_bridge chaining, to allow software chains: drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
It is done using Russian doll schema, ops from the bridge calls the same ops from the next bridge and the next bridge ops can do the same.
This schema means that all the bridges including the last one are seen from the drm core point of view as a one big drm_bridge. Additionally in this particular case, the first bridge (ptn3460) implements connector so it is hard to guess what is the location of the 2nd bridge in video stream chain, sometimes it is after the connector, sometimes before. All this is quite confusing.
But if you look at the bridge from upstream video interface point of view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its video input side acts as a panel. On the output side it expects a panel, lvds panel in this case.
So why not implement ptn3460 bridge as drm_panel which internally uses another drm_panel. With this approach everything fits much better. You do not need those (pre|post)_(enable|disable) calls, you do not need to implement connector in the bridge and you have a driver following linux driver model. And no single bit changed in drm core.
This discussion should have ideally happened when Sean added bridge into drm-core! And, we do need (pre|post)_(enable|disable) calls for bridges, but the drm_panel framework supports only enable/disable.
I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. It was not accepted as Inki preferred drm_bridge but as I see the problems with drm_bridges I have decide to attract attention to much more cleaner solution. [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559 [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
Regards Andrzej
Ajay Kumar (3): [RFC V2 1/3] drm: implement chaining of drm bridges [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
.../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ drivers/gpu/drm/bridge/ptn3460.c | 21 +- drivers/gpu/drm/drm_crtc.c | 13 +- include/drm/bridge/bridge_panel.h | 37 ++++ include/drm/drm_crtc.h | 2 + 8 files changed, 360 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h
On 05/08/2014 12:26 PM, Ajay kumar wrote:
Hi Andrej,
On Thu, May 8, 2014 at 12:11 PM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/05/2014 09:52 PM, Ajay Kumar wrote:
This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
I have just put up Rob's and Sean's idea of chaining up the bridges in code, and have implemented basic panel controls as a chained bridge. This works well with ptn3460 bridge chip on exynos5250-snow board.
Still need to make use of standard list calls and figure out proper way of deleting the bridge chain. So, this is just a rough version.
As I understand this patchset tries to solve two things:
- Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel 2. Add support to drm_bridge chaining, to allow software chains: drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
It is done using Russian doll schema, ops from the bridge calls the same ops from the next bridge and the next bridge ops can do the same.
This schema means that all the bridges including the last one are seen from the drm core point of view as a one big drm_bridge. Additionally in this particular case, the first bridge (ptn3460) implements connector so it is hard to guess what is the location of the 2nd bridge in video stream chain, sometimes it is after the connector, sometimes before. All this is quite confusing.
But if you look at the bridge from upstream video interface point of view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its video input side acts as a panel. On the output side it expects a panel, lvds panel in this case.
So why not implement ptn3460 bridge as drm_panel which internally uses another drm_panel. With this approach everything fits much better. You do not need those (pre|post)_(enable|disable) calls, you do not need to implement connector in the bridge and you have a driver following linux driver model. And no single bit changed in drm core.
This discussion should have ideally happened when Sean added bridge into drm-core!
Yes, I agree with this :)
And, we do need (pre|post)_(enable|disable) calls for bridges, but the drm_panel framework supports only enable/disable.
For true bridges pre|post can be just implemented as a part of the call, for example:
bridge_enable() { /* pre-enable stuff */ panel_enable(); /* post-enable stuff */ }
And for your particular problem you have written:
The LVDS datasheet says at least 200ms delay is needed from "Valid data" to "BL on".
I am not sure what exactly means 'valid data' in this case, if it is after lcd_en gpio why not just use schedule_delayed_work. If it should be called later I guess it should be possible to find a proper callback to drm_panel.
Regards Andrzej
I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. It was not accepted as Inki preferred drm_bridge but as I see the problems with drm_bridges I have decide to attract attention to much more cleaner solution. [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559 [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
Regards Andrzej
Ajay Kumar (3): [RFC V2 1/3] drm: implement chaining of drm bridges [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
.../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ drivers/gpu/drm/bridge/ptn3460.c | 21 +- drivers/gpu/drm/drm_crtc.c | 13 +- include/drm/bridge/bridge_panel.h | 37 ++++ include/drm/drm_crtc.h | 2 + 8 files changed, 360 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h
On Thu, May 8, 2014 at 6:29 PM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/08/2014 12:26 PM, Ajay kumar wrote:
Hi Andrej,
On Thu, May 8, 2014 at 12:11 PM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/05/2014 09:52 PM, Ajay Kumar wrote:
This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
I have just put up Rob's and Sean's idea of chaining up the bridges in code, and have implemented basic panel controls as a chained bridge. This works well with ptn3460 bridge chip on exynos5250-snow board.
Still need to make use of standard list calls and figure out proper way of deleting the bridge chain. So, this is just a rough version.
As I understand this patchset tries to solve two things:
- Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel 2. Add support to drm_bridge chaining, to allow software chains: drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
It is done using Russian doll schema, ops from the bridge calls the same ops from the next bridge and the next bridge ops can do the same.
This schema means that all the bridges including the last one are seen from the drm core point of view as a one big drm_bridge. Additionally in this particular case, the first bridge (ptn3460) implements connector so it is hard to guess what is the location of the 2nd bridge in video stream chain, sometimes it is after the connector, sometimes before. All this is quite confusing.
But if you look at the bridge from upstream video interface point of view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its video input side acts as a panel. On the output side it expects a panel, lvds panel in this case.
So why not implement ptn3460 bridge as drm_panel which internally uses another drm_panel. With this approach everything fits much better. You do not need those (pre|post)_(enable|disable) calls, you do not need to implement connector in the bridge and you have a driver following linux driver model. And no single bit changed in drm core.
This discussion should have ideally happened when Sean added bridge into drm-core!
Yes, I agree with this :)
And, we do need (pre|post)_(enable|disable) calls for bridges, but the drm_panel framework supports only enable/disable.
For true bridges pre|post can be just implemented as a part of the call, for example:
bridge_enable() { /* pre-enable stuff */ panel_enable(); /* post-enable stuff */ }
It should ideally be like this: 1) panel enable - switch on the LCD unit. 2) bridge enable - switch on the bridge. 3) encoder->commit/dpms on - valid data starts coming out of DP/MIPI 4) switch on the backlight unit and enable BL_EN.
And for your particular problem you have written:
The LVDS datasheet says at least 200ms delay is needed from "Valid data" to "BL on".
I am not sure what exactly means 'valid data' in this case, if it is after lcd_en gpio why not just use schedule_delayed_work. If it should be called later I guess it should be possible to find a proper callback to drm_panel.
Right. This was already discussed. I suggested adding pre_enable/enable/disable and post_disable for drm_panel, but Thierry was not ok with it.
Regards, Ajay
I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. It was not accepted as Inki preferred drm_bridge but as I see the problems with drm_bridges I have decide to attract attention to much more cleaner solution. [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559 [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044
Regards Andrzej
Ajay Kumar (3): [RFC V2 1/3] drm: implement chaining of drm bridges [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
.../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ drivers/gpu/drm/bridge/ptn3460.c | 21 +- drivers/gpu/drm/drm_crtc.c | 13 +- include/drm/bridge/bridge_panel.h | 37 ++++ include/drm/drm_crtc.h | 2 + 8 files changed, 360 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h
On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/05/2014 09:52 PM, Ajay Kumar wrote:
This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
I have just put up Rob's and Sean's idea of chaining up the bridges in code, and have implemented basic panel controls as a chained bridge. This works well with ptn3460 bridge chip on exynos5250-snow board.
Still need to make use of standard list calls and figure out proper way of deleting the bridge chain. So, this is just a rough version.
As I understand this patchset tries to solve two things:
- Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel 2. Add support to drm_bridge chaining, to allow software chains: drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
It is done using Russian doll schema, ops from the bridge calls the same ops from the next bridge and the next bridge ops can do the same.
This schema means that all the bridges including the last one are seen from the drm core point of view as a one big drm_bridge. Additionally in this particular case, the first bridge (ptn3460) implements connector so it is hard to guess what is the location of the 2nd bridge in video stream chain, sometimes it is after the connector, sometimes before. All this is quite confusing.
But if you look at the bridge from upstream video interface point of view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its video input side acts as a panel. On the output side it expects a panel, lvds panel in this case.
tbh, this is mostly about what we call it. Perhaps "bridge" isn't the best name.. I wouldn't object to changing it.
But my thinking was to leave in drm_panel_funcs things that are just needed by the connector (get_modes().. and maybe some day we need detect/etc). Then leave everything else in drm_bridge_funcs. A panel could (if needed) implement both interfaces.
That is basically the same as what you are proposing, but without renaming bridge to panel ;-)
BR, -R
So why not implement ptn3460 bridge as drm_panel which internally uses another drm_panel. With this approach everything fits much better. You do not need those (pre|post)_(enable|disable) calls, you do not need to implement connector in the bridge and you have a driver following linux driver model. And no single bit changed in drm core.
I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. It was not accepted as Inki preferred drm_bridge but as I see the problems with drm_bridges I have decide to attract attention to much more cleaner solution.
Regards Andrzej
Ajay Kumar (3): [RFC V2 1/3] drm: implement chaining of drm bridges [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
.../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ drivers/gpu/drm/bridge/ptn3460.c | 21 +- drivers/gpu/drm/drm_crtc.c | 13 +- include/drm/bridge/bridge_panel.h | 37 ++++ include/drm/drm_crtc.h | 2 + 8 files changed, 360 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h
On 05/08/2014 08:24 PM, Rob Clark wrote:
On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/05/2014 09:52 PM, Ajay Kumar wrote:
This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
I have just put up Rob's and Sean's idea of chaining up the bridges in code, and have implemented basic panel controls as a chained bridge. This works well with ptn3460 bridge chip on exynos5250-snow board.
Still need to make use of standard list calls and figure out proper way of deleting the bridge chain. So, this is just a rough version.
As I understand this patchset tries to solve two things:
- Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel 2. Add support to drm_bridge chaining, to allow software chains: drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
It is done using Russian doll schema, ops from the bridge calls the same ops from the next bridge and the next bridge ops can do the same.
This schema means that all the bridges including the last one are seen from the drm core point of view as a one big drm_bridge. Additionally in this particular case, the first bridge (ptn3460) implements connector so it is hard to guess what is the location of the 2nd bridge in video stream chain, sometimes it is after the connector, sometimes before. All this is quite confusing.
But if you look at the bridge from upstream video interface point of view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its video input side acts as a panel. On the output side it expects a panel, lvds panel in this case.
tbh, this is mostly about what we call it. Perhaps "bridge" isn't the best name.. I wouldn't object to changing it.
But my thinking was to leave in drm_panel_funcs things that are just needed by the connector (get_modes().. and maybe some day we need detect/etc). Then leave everything else in drm_bridge_funcs. A panel could (if needed) implement both interfaces.
That is basically the same as what you are proposing, but without renaming bridge to panel ;-)
Good to hear that. However there are points which are not clear for me. But first lets clarify names, I will use panel and bridge words to describe the hardware, and drm_panel, drm_bridge to describe the software interfaces.
What bothers me: 1. You want to leave connector related callbacks in drm_panel and the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460 must implement connector internally because of this limitation. I guess it is quite typical bridge. This problem does not exists in case of using drm_panel for ptn3460.
2. drm_bridge is attached to the encoder, this and the callback order suggests the video data flow should be as below: drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
ptn3460 implements drm_bridge and drm_connector so it suggests its drm_bridge should be the last one, so there should be no place to add lvds panel implemented as a drm_bridge after it, as it is done in this patchset.
Additionally it clearly shows that there should be two categories of drm_bridges - non-terminal and terminal.
3. drm_dev uses all-or-nothing approach, ie. it will start only when all its components are up. It simplifies synchronization but is quite fragile - the whole drm will be down due to error in some of its components. For this reason I prefer drm_panel as it is not real drm component it can be attached/detached to/from drm_connector anytime. I am not really sure but drm_bridge does not allow for that.
Real life example to show importance of it: I have a phone with MIPI-DSI panel and HDMI. Due to initialization issues HDMI bridge driver sometimes fails during probe and the drmdev do not start. Of course this is development stage so I have serial console I can diagnose the problem, disable HDMI, fix the problem, etc... But what happens in case of end-user. He will see black screen - bricked phone. In case the bridge will be implemented using drm_panel he will have working phone with broken HDMI, much better.
4. And the last thing, it is more about the concept/design. drm_bridge, drm_hw_block suggests that those interfaces describes the whole device: bridge, panel, whatever. In my approach I have an interface to describe only one video input port of the device. And drm_panel is in fact misleading name, drm_sink may be better. So real panel would implement drm_sink interface. Bridge would implement drm_sink interface and it will request other drm_sink interface, to interact with the panel which is after it. This approach seems to me more flexible. Beside things I have described above it will allow to implement also more complicated devices, dsi hubs, video mixers, etc.
Regards Andrzej
BR, -R
So why not implement ptn3460 bridge as drm_panel which internally uses another drm_panel. With this approach everything fits much better. You do not need those (pre|post)_(enable|disable) calls, you do not need to implement connector in the bridge and you have a driver following linux driver model. And no single bit changed in drm core.
I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. It was not accepted as Inki preferred drm_bridge but as I see the problems with drm_bridges I have decide to attract attention to much more cleaner solution.
Regards Andrzej
Ajay Kumar (3): [RFC V2 1/3] drm: implement chaining of drm bridges [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
.../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ drivers/gpu/drm/bridge/ptn3460.c | 21 +- drivers/gpu/drm/drm_crtc.c | 13 +- include/drm/bridge/bridge_panel.h | 37 ++++ include/drm/drm_crtc.h | 2 + 8 files changed, 360 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h
On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/08/2014 08:24 PM, Rob Clark wrote:
On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/05/2014 09:52 PM, Ajay Kumar wrote:
This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
I have just put up Rob's and Sean's idea of chaining up the bridges in code, and have implemented basic panel controls as a chained bridge. This works well with ptn3460 bridge chip on exynos5250-snow board.
Still need to make use of standard list calls and figure out proper way of deleting the bridge chain. So, this is just a rough version.
As I understand this patchset tries to solve two things:
- Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel 2. Add support to drm_bridge chaining, to allow software chains: drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
It is done using Russian doll schema, ops from the bridge calls the same ops from the next bridge and the next bridge ops can do the same.
This schema means that all the bridges including the last one are seen from the drm core point of view as a one big drm_bridge. Additionally in this particular case, the first bridge (ptn3460) implements connector so it is hard to guess what is the location of the 2nd bridge in video stream chain, sometimes it is after the connector, sometimes before. All this is quite confusing.
But if you look at the bridge from upstream video interface point of view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its video input side acts as a panel. On the output side it expects a panel, lvds panel in this case.
tbh, this is mostly about what we call it. Perhaps "bridge" isn't the best name.. I wouldn't object to changing it.
But my thinking was to leave in drm_panel_funcs things that are just needed by the connector (get_modes().. and maybe some day we need detect/etc). Then leave everything else in drm_bridge_funcs. A panel could (if needed) implement both interfaces.
That is basically the same as what you are proposing, but without renaming bridge to panel ;-)
Good to hear that. However there are points which are not clear for me. But first lets clarify names, I will use panel and bridge words to describe the hardware, and drm_panel, drm_bridge to describe the software interfaces.
What bothers me:
- You want to leave connector related callbacks in drm_panel and
the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460 must implement connector internally because of this limitation. I guess it is quite typical bridge. This problem does not exists in case of using drm_panel for ptn3460.
- drm_bridge is attached to the encoder, this and the callback order
suggests the video data flow should be as below: drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
ptn3460 implements drm_bridge and drm_connector so it suggests its drm_bridge should be the last one, so there should be no place to add lvds panel implemented as a drm_bridge after it, as it is done in this patchset.
Additionally it clearly shows that there should be two categories of drm_bridges - non-terminal and terminal.
- drm_dev uses all-or-nothing approach, ie. it will start only when all
its components are up. It simplifies synchronization but is quite fragile - the whole drm will be down due to error in some of its components. For this reason I prefer drm_panel as it is not real drm component it can be attached/detached to/from drm_connector anytime. I am not really sure but drm_bridge does not allow for that.
So I do think we need to stick to this all-or-nothing approach for anything that is visible to userspace (drm_{plane,crtc,encoder,connector}). We don't currently have a way to "hotplug" those so I don't see a real smooth upgrade path to add that in a backwards compatible way that won't cause problems with old userspace.
But, that said, we have more flexibility with things not visible to userspace (drm_{panel,bridge}). I'm not sure how much we want to allow things to be completely dynamic (we already have some hard enough locking fun). But proposals/rfcs/etc welcome.
I guess I'm not completely familiar w/ ptn3460, but the fact that it needs to implement drm_connector makes me a bit suspicious. Seems like a symptom of missing things in drm_panel_funcs. It would be better to always create the connector statically, and just have _detect() -> disconnected if panel==NULL.
Real life example to show importance of it: I have a phone with MIPI-DSI panel and HDMI. Due to initialization issues HDMI bridge driver sometimes fails during probe and the drmdev do not start. Of course this is development stage so I have serial console I can diagnose the problem, disable HDMI, fix the problem, etc... But what happens in case of end-user. He will see black screen - bricked phone. In case the bridge will be implemented using drm_panel he will have working phone with broken HDMI, much better.
well, tbh, I don't think an end-user will see the device if hdmi were broken ;-)
I suppose if bridge/panel where loaded dynamically (or at least after drm device and drm_{connector,encoder,etc} are created, it would help a bit here. I'd kinda hope that isn't the only benefit/reason to make things more dynamic. Especially if we allow bridges/panels to be unloaded.. (just loading them dynamically doesn't seem as scary from locking perspective)
- And the last thing, it is more about the concept/design. drm_bridge,
drm_hw_block suggests that those interfaces describes the whole device: bridge, panel, whatever.
hmm, I don't think this is the case. I can easily see things like:
struct foo_panel { struct drm_panel base; struct drm_bridge bridge; ... }
where a panel implementation implements both panel and bridge. In fact that is kinda what I was encouraging.
BR, -R
In my approach I have an interface to describe only one video input port of the device. And drm_panel is in fact misleading name, drm_sink may be better. So real panel would implement drm_sink interface. Bridge would implement drm_sink interface and it will request other drm_sink interface, to interact with the panel which is after it. This approach seems to me more flexible. Beside things I have described above it will allow to implement also more complicated devices, dsi hubs, video mixers, etc.
Regards Andrzej
BR, -R
So why not implement ptn3460 bridge as drm_panel which internally uses another drm_panel. With this approach everything fits much better. You do not need those (pre|post)_(enable|disable) calls, you do not need to implement connector in the bridge and you have a driver following linux driver model. And no single bit changed in drm core.
I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. It was not accepted as Inki preferred drm_bridge but as I see the problems with drm_bridges I have decide to attract attention to much more cleaner solution.
Regards Andrzej
Ajay Kumar (3): [RFC V2 1/3] drm: implement chaining of drm bridges [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
.../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ drivers/gpu/drm/bridge/ptn3460.c | 21 +- drivers/gpu/drm/drm_crtc.c | 13 +- include/drm/bridge/bridge_panel.h | 37 ++++ include/drm/drm_crtc.h | 2 + 8 files changed, 360 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h
On Fri, May 9, 2014 at 7:29 PM, Rob Clark robdclark@gmail.com wrote:
On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/08/2014 08:24 PM, Rob Clark wrote:
On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/05/2014 09:52 PM, Ajay Kumar wrote:
This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
I have just put up Rob's and Sean's idea of chaining up the bridges in code, and have implemented basic panel controls as a chained bridge. This works well with ptn3460 bridge chip on exynos5250-snow board.
Still need to make use of standard list calls and figure out proper way of deleting the bridge chain. So, this is just a rough version.
As I understand this patchset tries to solve two things:
- Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel 2. Add support to drm_bridge chaining, to allow software chains: drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
It is done using Russian doll schema, ops from the bridge calls the same ops from the next bridge and the next bridge ops can do the same.
This schema means that all the bridges including the last one are seen from the drm core point of view as a one big drm_bridge. Additionally in this particular case, the first bridge (ptn3460) implements connector so it is hard to guess what is the location of the 2nd bridge in video stream chain, sometimes it is after the connector, sometimes before. All this is quite confusing.
But if you look at the bridge from upstream video interface point of view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its video input side acts as a panel. On the output side it expects a panel, lvds panel in this case.
tbh, this is mostly about what we call it. Perhaps "bridge" isn't the best name.. I wouldn't object to changing it.
But my thinking was to leave in drm_panel_funcs things that are just needed by the connector (get_modes().. and maybe some day we need detect/etc). Then leave everything else in drm_bridge_funcs. A panel could (if needed) implement both interfaces.
That is basically the same as what you are proposing, but without renaming bridge to panel ;-)
Good to hear that. However there are points which are not clear for me. But first lets clarify names, I will use panel and bridge words to describe the hardware, and drm_panel, drm_bridge to describe the software interfaces.
What bothers me:
- You want to leave connector related callbacks in drm_panel and
the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460 must implement connector internally because of this limitation. I guess it is quite typical bridge. This problem does not exists in case of using drm_panel for ptn3460.
- drm_bridge is attached to the encoder, this and the callback order
suggests the video data flow should be as below: drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
ptn3460 implements drm_bridge and drm_connector so it suggests its drm_bridge should be the last one, so there should be no place to add lvds panel implemented as a drm_bridge after it, as it is done in this patchset.
Additionally it clearly shows that there should be two categories of drm_bridges - non-terminal and terminal.
- drm_dev uses all-or-nothing approach, ie. it will start only when all
its components are up. It simplifies synchronization but is quite fragile - the whole drm will be down due to error in some of its components. For this reason I prefer drm_panel as it is not real drm component it can be attached/detached to/from drm_connector anytime. I am not really sure but drm_bridge does not allow for that.
So I do think we need to stick to this all-or-nothing approach for anything that is visible to userspace (drm_{plane,crtc,encoder,connector}). We don't currently have a way to "hotplug" those so I don't see a real smooth upgrade path to add that in a backwards compatible way that won't cause problems with old userspace.
But, that said, we have more flexibility with things not visible to userspace (drm_{panel,bridge}). I'm not sure how much we want to allow things to be completely dynamic (we already have some hard enough locking fun). But proposals/rfcs/etc welcome.
I guess I'm not completely familiar w/ ptn3460, but the fact that it needs to implement drm_connector makes me a bit suspicious. Seems like a symptom of missing things in drm_panel_funcs. It would be better to always create the connector statically, and just have _detect() -> disconnected if panel==NULL.
This is something which only Sean can answer! I guess he implemented ptn3460 as connector thinking that bridge would be the last entity in the video pipeline. If that's a real problem, we can still move out the connector part.
Regards, Ajay
Real life example to show importance of it: I have a phone with MIPI-DSI panel and HDMI. Due to initialization issues HDMI bridge driver sometimes fails during probe and the drmdev do not start. Of course this is development stage so I have serial console I can diagnose the problem, disable HDMI, fix the problem, etc... But what happens in case of end-user. He will see black screen - bricked phone. In case the bridge will be implemented using drm_panel he will have working phone with broken HDMI, much better.
well, tbh, I don't think an end-user will see the device if hdmi were broken ;-)
I suppose if bridge/panel where loaded dynamically (or at least after drm device and drm_{connector,encoder,etc} are created, it would help a bit here. I'd kinda hope that isn't the only benefit/reason to make things more dynamic. Especially if we allow bridges/panels to be unloaded.. (just loading them dynamically doesn't seem as scary from locking perspective)
- And the last thing, it is more about the concept/design. drm_bridge,
drm_hw_block suggests that those interfaces describes the whole device: bridge, panel, whatever.
hmm, I don't think this is the case. I can easily see things like:
struct foo_panel { struct drm_panel base; struct drm_bridge bridge; ... }
where a panel implementation implements both panel and bridge. In fact that is kinda what I was encouraging.
BR, -R
In my approach I have an interface to describe only one video input port of the device. And drm_panel is in fact misleading name, drm_sink may be better. So real panel would implement drm_sink interface. Bridge would implement drm_sink interface and it will request other drm_sink interface, to interact with the panel which is after it. This approach seems to me more flexible. Beside things I have described above it will allow to implement also more complicated devices, dsi hubs, video mixers, etc.
Regards Andrzej
BR, -R
So why not implement ptn3460 bridge as drm_panel which internally uses another drm_panel. With this approach everything fits much better. You do not need those (pre|post)_(enable|disable) calls, you do not need to implement connector in the bridge and you have a driver following linux driver model. And no single bit changed in drm core.
I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. It was not accepted as Inki preferred drm_bridge but as I see the problems with drm_bridges I have decide to attract attention to much more cleaner solution.
Regards Andrzej
Ajay Kumar (3): [RFC V2 1/3] drm: implement chaining of drm bridges [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
.../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ drivers/gpu/drm/bridge/ptn3460.c | 21 +- drivers/gpu/drm/drm_crtc.c | 13 +- include/drm/bridge/bridge_panel.h | 37 ++++ include/drm/drm_crtc.h | 2 + 8 files changed, 360 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h
On 05/09/2014 05:05 PM, Ajay kumar wrote:
On Fri, May 9, 2014 at 7:29 PM, Rob Clark robdclark@gmail.com wrote:
On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/08/2014 08:24 PM, Rob Clark wrote:
On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/05/2014 09:52 PM, Ajay Kumar wrote:
This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
I have just put up Rob's and Sean's idea of chaining up the bridges in code, and have implemented basic panel controls as a chained bridge. This works well with ptn3460 bridge chip on exynos5250-snow board.
Still need to make use of standard list calls and figure out proper way of deleting the bridge chain. So, this is just a rough version.
As I understand this patchset tries to solve two things:
- Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel 2. Add support to drm_bridge chaining, to allow software chains: drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
It is done using Russian doll schema, ops from the bridge calls the same ops from the next bridge and the next bridge ops can do the same.
This schema means that all the bridges including the last one are seen from the drm core point of view as a one big drm_bridge. Additionally in this particular case, the first bridge (ptn3460) implements connector so it is hard to guess what is the location of the 2nd bridge in video stream chain, sometimes it is after the connector, sometimes before. All this is quite confusing.
But if you look at the bridge from upstream video interface point of view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its video input side acts as a panel. On the output side it expects a panel, lvds panel in this case.
tbh, this is mostly about what we call it. Perhaps "bridge" isn't the best name.. I wouldn't object to changing it.
But my thinking was to leave in drm_panel_funcs things that are just needed by the connector (get_modes().. and maybe some day we need detect/etc). Then leave everything else in drm_bridge_funcs. A panel could (if needed) implement both interfaces.
That is basically the same as what you are proposing, but without renaming bridge to panel ;-)
Good to hear that. However there are points which are not clear for me. But first lets clarify names, I will use panel and bridge words to describe the hardware, and drm_panel, drm_bridge to describe the software interfaces.
What bothers me:
- You want to leave connector related callbacks in drm_panel and
the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460 must implement connector internally because of this limitation. I guess it is quite typical bridge. This problem does not exists in case of using drm_panel for ptn3460.
- drm_bridge is attached to the encoder, this and the callback order
suggests the video data flow should be as below: drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
ptn3460 implements drm_bridge and drm_connector so it suggests its drm_bridge should be the last one, so there should be no place to add lvds panel implemented as a drm_bridge after it, as it is done in this patchset.
Additionally it clearly shows that there should be two categories of drm_bridges - non-terminal and terminal.
- drm_dev uses all-or-nothing approach, ie. it will start only when all
its components are up. It simplifies synchronization but is quite fragile - the whole drm will be down due to error in some of its components. For this reason I prefer drm_panel as it is not real drm component it can be attached/detached to/from drm_connector anytime. I am not really sure but drm_bridge does not allow for that.
So I do think we need to stick to this all-or-nothing approach for anything that is visible to userspace (drm_{plane,crtc,encoder,connector}). We don't currently have a way to "hotplug" those so I don't see a real smooth upgrade path to add that in a backwards compatible way that won't cause problems with old userspace.
But, that said, we have more flexibility with things not visible to userspace (drm_{panel,bridge}). I'm not sure how much we want to allow things to be completely dynamic (we already have some hard enough locking fun). But proposals/rfcs/etc welcome.
I guess I'm not completely familiar w/ ptn3460, but the fact that it needs to implement drm_connector makes me a bit suspicious. Seems like a symptom of missing things in drm_panel_funcs. It would be better to always create the connector statically, and just have _detect() -> disconnected if panel==NULL.
ptn3460 has been implemented using drm_bridge and drm_connector, not by me, to be clear :) And to make it more clear from what I see ptn3460 exposes following ops: - pre_enable (via drm_bridge). - disable (via drm_bridge), - get_modes (via drm_connector). Other ops are exposed just to fulfill requirements of drm frameworks, I guess.
This is something which only Sean can answer! I guess he implemented ptn3460 as connector thinking that bridge would be the last entity in the video pipeline. If that's a real problem, we can still move out the connector part.
Regards, Ajay
The question is how it can be implemented using only drm_bridge.
Real life example to show importance of it: I have a phone with MIPI-DSI panel and HDMI. Due to initialization issues HDMI bridge driver sometimes fails during probe and the drmdev do not start. Of course this is development stage so I have serial console I can diagnose the problem, disable HDMI, fix the problem, etc... But what happens in case of end-user. He will see black screen - bricked phone. In case the bridge will be implemented using drm_panel he will have working phone with broken HDMI, much better.
well, tbh, I don't think an end-user will see the device if hdmi were broken ;-)
It can break also during phone utilization.
I suppose if bridge/panel where loaded dynamically (or at least after drm device and drm_{connector,encoder,etc} are created, it would help a bit here. I'd kinda hope that isn't the only benefit/reason to make things more dynamic. Especially if we allow bridges/panels to be unloaded.. (just loading them dynamically doesn't seem as scary from locking perspective)
- And the last thing, it is more about the concept/design. drm_bridge,
drm_hw_block suggests that those interfaces describes the whole device: bridge, panel, whatever.
hmm, I don't think this is the case. I can easily see things like:
struct foo_panel { struct drm_panel base; struct drm_bridge bridge; ... }
where a panel implementation implements both panel and bridge. In fact that is kinda what I was encouraging.
I guess it can work, but I see it sub-optimal. In general, looking on the hardware the same video data goes to the panel and to the bridge (if they are of the same type of course), I do not know why it couldn't be mapped to software interfaces. For example drm_sink, as I described previously (now it is cited below).
Regards Andrzej
BR, -R
In my approach I have an interface to describe only one video input port of the device. And drm_panel is in fact misleading name, drm_sink may be better. So real panel would implement drm_sink interface. Bridge would implement drm_sink interface and it will request other drm_sink interface, to interact with the panel which is after it. This approach seems to me more flexible. Beside things I have described above it will allow to implement also more complicated devices, dsi hubs, video mixers, etc.
Regards Andrzej
BR, -R
So why not implement ptn3460 bridge as drm_panel which internally uses another drm_panel. With this approach everything fits much better. You do not need those (pre|post)_(enable|disable) calls, you do not need to implement connector in the bridge and you have a driver following linux driver model. And no single bit changed in drm core.
I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. It was not accepted as Inki preferred drm_bridge but as I see the problems with drm_bridges I have decide to attract attention to much more cleaner solution.
Regards Andrzej
Ajay Kumar (3): [RFC V2 1/3] drm: implement chaining of drm bridges [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining
.../bindings/drm/bridge/bridge_panel.txt | 45 ++++ drivers/gpu/drm/bridge/Kconfig | 6 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ drivers/gpu/drm/bridge/ptn3460.c | 21 +- drivers/gpu/drm/drm_crtc.c | 13 +- include/drm/bridge/bridge_panel.h | 37 ++++ include/drm/drm_crtc.h | 2 + 8 files changed, 360 insertions(+), 5 deletions(-) create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c create mode 100644 include/drm/bridge/bridge_panel.h
On Mon, May 12, 2014 at 3:06 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/09/2014 05:05 PM, Ajay kumar wrote:
On Fri, May 9, 2014 at 7:29 PM, Rob Clark robdclark@gmail.com wrote:
On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/08/2014 08:24 PM, Rob Clark wrote:
On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/05/2014 09:52 PM, Ajay Kumar wrote: > This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: > git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git > > I have just put up Rob's and Sean's idea of chaining up the bridges > in code, and have implemented basic panel controls as a chained bridge. > This works well with ptn3460 bridge chip on exynos5250-snow board. > > Still need to make use of standard list calls and figure out proper way > of deleting the bridge chain. So, this is just a rough version. As I understand this patchset tries to solve two things:
- Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel 2. Add support to drm_bridge chaining, to allow software chains: drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
It is done using Russian doll schema, ops from the bridge calls the same ops from the next bridge and the next bridge ops can do the same.
This schema means that all the bridges including the last one are seen from the drm core point of view as a one big drm_bridge. Additionally in this particular case, the first bridge (ptn3460) implements connector so it is hard to guess what is the location of the 2nd bridge in video stream chain, sometimes it is after the connector, sometimes before. All this is quite confusing.
But if you look at the bridge from upstream video interface point of view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its video input side acts as a panel. On the output side it expects a panel, lvds panel in this case.
tbh, this is mostly about what we call it. Perhaps "bridge" isn't the best name.. I wouldn't object to changing it.
But my thinking was to leave in drm_panel_funcs things that are just needed by the connector (get_modes().. and maybe some day we need detect/etc). Then leave everything else in drm_bridge_funcs. A panel could (if needed) implement both interfaces.
That is basically the same as what you are proposing, but without renaming bridge to panel ;-)
Good to hear that. However there are points which are not clear for me. But first lets clarify names, I will use panel and bridge words to describe the hardware, and drm_panel, drm_bridge to describe the software interfaces.
What bothers me:
- You want to leave connector related callbacks in drm_panel and
the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460 must implement connector internally because of this limitation. I guess it is quite typical bridge. This problem does not exists in case of using drm_panel for ptn3460.
- drm_bridge is attached to the encoder, this and the callback order
suggests the video data flow should be as below: drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
ptn3460 implements drm_bridge and drm_connector so it suggests its drm_bridge should be the last one, so there should be no place to add lvds panel implemented as a drm_bridge after it, as it is done in this patchset.
Additionally it clearly shows that there should be two categories of drm_bridges - non-terminal and terminal.
- drm_dev uses all-or-nothing approach, ie. it will start only when all
its components are up. It simplifies synchronization but is quite fragile - the whole drm will be down due to error in some of its components. For this reason I prefer drm_panel as it is not real drm component it can be attached/detached to/from drm_connector anytime. I am not really sure but drm_bridge does not allow for that.
So I do think we need to stick to this all-or-nothing approach for anything that is visible to userspace (drm_{plane,crtc,encoder,connector}). We don't currently have a way to "hotplug" those so I don't see a real smooth upgrade path to add that in a backwards compatible way that won't cause problems with old userspace.
But, that said, we have more flexibility with things not visible to userspace (drm_{panel,bridge}). I'm not sure how much we want to allow things to be completely dynamic (we already have some hard enough locking fun). But proposals/rfcs/etc welcome.
I guess I'm not completely familiar w/ ptn3460, but the fact that it needs to implement drm_connector makes me a bit suspicious. Seems like a symptom of missing things in drm_panel_funcs. It would be better to always create the connector statically, and just have _detect() -> disconnected if panel==NULL.
ptn3460 has been implemented using drm_bridge and drm_connector, not by me, to be clear :)
sure, and afaiu it was adapted from a pre-bridge implementation on chromeos tree. So between that, and the fact that bridge and panel are relatively new, it is not unexpected that some evolution/refactoring will happen as we go.
And to make it more clear from what I see ptn3460 exposes following ops:
- pre_enable (via drm_bridge).
- disable (via drm_bridge),
- get_modes (via drm_connector).
sure, this is why I'm leaning towards saying that drm_panel_funcs should be anything a connector needs that a bridge does not need (to avoid putting fxn ptrs in drm_bridge_funcs which don't make sense for a pure bridge)
Other ops are exposed just to fulfill requirements of drm frameworks, I guess.
This is something which only Sean can answer! I guess he implemented ptn3460 as connector thinking that bridge would be the last entity in the video pipeline. If that's a real problem, we can still move out the connector part.
Regards, Ajay
The question is how it can be implemented using only drm_bridge.
I'm not entirely sure I understand why. I think you would want to have a ptn3460 bridge (pure bridge) + chaining + foo_panel which has it's bridge interface chained up to ptn3460 and a panel interface passed to the connector.
(At some point, maybe it makes sense to have a generic drm_panel_connector which drivers can re-use to avoid duplicating the connector code, but that is an implementation detail.)
Real life example to show importance of it: I have a phone with MIPI-DSI panel and HDMI. Due to initialization issues HDMI bridge driver sometimes fails during probe and the drmdev do not start. Of course this is development stage so I have serial console I can diagnose the problem, disable HDMI, fix the problem, etc... But what happens in case of end-user. He will see black screen - bricked phone. In case the bridge will be implemented using drm_panel he will have working phone with broken HDMI, much better.
well, tbh, I don't think an end-user will see the device if hdmi were broken ;-)
It can break also during phone utilization.
I suppose if bridge/panel where loaded dynamically (or at least after drm device and drm_{connector,encoder,etc} are created, it would help a bit here. I'd kinda hope that isn't the only benefit/reason to make things more dynamic. Especially if we allow bridges/panels to be unloaded.. (just loading them dynamically doesn't seem as scary from locking perspective)
- And the last thing, it is more about the concept/design. drm_bridge,
drm_hw_block suggests that those interfaces describes the whole device: bridge, panel, whatever.
hmm, I don't think this is the case. I can easily see things like:
struct foo_panel { struct drm_panel base; struct drm_bridge bridge; ... }
where a panel implementation implements both panel and bridge. In fact that is kinda what I was encouraging.
I guess it can work, but I see it sub-optimal. In general, looking on the hardware the same video data goes to the panel and to the bridge (if they are of the same type of course), I do not know why it couldn't be mapped to software interfaces. For example drm_sink, as I described previously (now it is cited below).
I'm not entirely sure why letting a panel implement multiple different interfaces (where needed) is suboptimal. It seems more sub-optimal to put panel related fxns which are only applicable to panels in drm_bridge_funcs.
Well, my initial reaction when you start talking about drm_src and drm_sinks is that this can quickly get over-designed. I'm not trying to turn kms into v4l2 unless there is a good reason. But maybe I'm assuming too much about what you are proposing.
BR, -R
Regards Andrzej
BR, -R
In my approach I have an interface to describe only one video input port of the device. And drm_panel is in fact misleading name, drm_sink may be better. So real panel would implement drm_sink interface. Bridge would implement drm_sink interface and it will request other drm_sink interface, to interact with the panel which is after it. This approach seems to me more flexible. Beside things I have described above it will allow to implement also more complicated devices, dsi hubs, video mixers, etc.
Regards Andrzej
BR, -R
So why not implement ptn3460 bridge as drm_panel which internally uses another drm_panel. With this approach everything fits much better. You do not need those (pre|post)_(enable|disable) calls, you do not need to implement connector in the bridge and you have a driver following linux driver model. And no single bit changed in drm core.
I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. It was not accepted as Inki preferred drm_bridge but as I see the problems with drm_bridges I have decide to attract attention to much more cleaner solution.
Regards Andrzej
> Ajay Kumar (3): > [RFC V2 1/3] drm: implement chaining of drm bridges > [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges > [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining > > .../bindings/drm/bridge/bridge_panel.txt | 45 ++++ > drivers/gpu/drm/bridge/Kconfig | 6 + > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ > drivers/gpu/drm/bridge/ptn3460.c | 21 +- > drivers/gpu/drm/drm_crtc.c | 13 +- > include/drm/bridge/bridge_panel.h | 37 ++++ > include/drm/drm_crtc.h | 2 + > 8 files changed, 360 insertions(+), 5 deletions(-) > create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt > create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c > create mode 100644 include/drm/bridge/bridge_panel.h >
On 05/12/2014 02:45 PM, Rob Clark wrote:
On Mon, May 12, 2014 at 3:06 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/09/2014 05:05 PM, Ajay kumar wrote:
On Fri, May 9, 2014 at 7:29 PM, Rob Clark robdclark@gmail.com wrote:
On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/08/2014 08:24 PM, Rob Clark wrote:
On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda a.hajda@samsung.com wrote: > On 05/05/2014 09:52 PM, Ajay Kumar wrote: >> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: >> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git >> >> I have just put up Rob's and Sean's idea of chaining up the bridges >> in code, and have implemented basic panel controls as a chained bridge. >> This works well with ptn3460 bridge chip on exynos5250-snow board. >> >> Still need to make use of standard list calls and figure out proper way >> of deleting the bridge chain. So, this is just a rough version. > As I understand this patchset tries to solve two things: > 1. Implement panel as drm_bridge, to ease support for hardware chains: > Crtc -> Encoder -> Bridge -> Panel > 2. Add support to drm_bridge chaining, to allow software chains: > drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel > > It is done using Russian doll schema, ops from the bridge calls the same > ops from the next bridge and the next bridge ops can do the same. > > This schema means that all the bridges including the last one are seen > from the drm core point of view as a one big drm_bridge. Additionally in > this particular case, the first bridge (ptn3460) implements connector > so it is hard to guess what is the location of the 2nd bridge in video > stream chain, sometimes it is after the connector, sometimes before. > All this is quite confusing. > > But if you look at the bridge from upstream video interface point of > view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its > video input side acts as a panel. On the output side it expects a panel, > lvds panel in this case. tbh, this is mostly about what we call it. Perhaps "bridge" isn't the best name.. I wouldn't object to changing it.
But my thinking was to leave in drm_panel_funcs things that are just needed by the connector (get_modes().. and maybe some day we need detect/etc). Then leave everything else in drm_bridge_funcs. A panel could (if needed) implement both interfaces.
That is basically the same as what you are proposing, but without renaming bridge to panel ;-)
Good to hear that. However there are points which are not clear for me. But first lets clarify names, I will use panel and bridge words to describe the hardware, and drm_panel, drm_bridge to describe the software interfaces.
What bothers me:
- You want to leave connector related callbacks in drm_panel and
the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460 must implement connector internally because of this limitation. I guess it is quite typical bridge. This problem does not exists in case of using drm_panel for ptn3460.
- drm_bridge is attached to the encoder, this and the callback order
suggests the video data flow should be as below: drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
ptn3460 implements drm_bridge and drm_connector so it suggests its drm_bridge should be the last one, so there should be no place to add lvds panel implemented as a drm_bridge after it, as it is done in this patchset.
Additionally it clearly shows that there should be two categories of drm_bridges - non-terminal and terminal.
- drm_dev uses all-or-nothing approach, ie. it will start only when all
its components are up. It simplifies synchronization but is quite fragile - the whole drm will be down due to error in some of its components. For this reason I prefer drm_panel as it is not real drm component it can be attached/detached to/from drm_connector anytime. I am not really sure but drm_bridge does not allow for that.
So I do think we need to stick to this all-or-nothing approach for anything that is visible to userspace (drm_{plane,crtc,encoder,connector}). We don't currently have a way to "hotplug" those so I don't see a real smooth upgrade path to add that in a backwards compatible way that won't cause problems with old userspace.
But, that said, we have more flexibility with things not visible to userspace (drm_{panel,bridge}). I'm not sure how much we want to allow things to be completely dynamic (we already have some hard enough locking fun). But proposals/rfcs/etc welcome.
I guess I'm not completely familiar w/ ptn3460, but the fact that it needs to implement drm_connector makes me a bit suspicious. Seems like a symptom of missing things in drm_panel_funcs. It would be better to always create the connector statically, and just have _detect() -> disconnected if panel==NULL.
ptn3460 has been implemented using drm_bridge and drm_connector, not by me, to be clear :)
sure, and afaiu it was adapted from a pre-bridge implementation on chromeos tree. So between that, and the fact that bridge and panel are relatively new, it is not unexpected that some evolution/refactoring will happen as we go.
And to make it more clear from what I see ptn3460 exposes following ops:
- pre_enable (via drm_bridge).
- disable (via drm_bridge),
- get_modes (via drm_connector).
sure, this is why I'm leaning towards saying that drm_panel_funcs should be anything a connector needs that a bridge does not need (to avoid putting fxn ptrs in drm_bridge_funcs which don't make sense for a pure bridge)
Other ops are exposed just to fulfill requirements of drm frameworks, I guess.
This is something which only Sean can answer! I guess he implemented ptn3460 as connector thinking that bridge would be the last entity in the video pipeline. If that's a real problem, we can still move out the connector part.
Regards, Ajay
The question is how it can be implemented using only drm_bridge.
I'm not entirely sure I understand why. I think you would want to have a ptn3460 bridge (pure bridge) + chaining + foo_panel which has it's bridge interface chained up to ptn3460 and a panel interface passed to the connector.
(At some point, maybe it makes sense to have a generic drm_panel_connector which drivers can re-use to avoid duplicating the connector code, but that is an implementation detail.)
Real life example to show importance of it: I have a phone with MIPI-DSI panel and HDMI. Due to initialization issues HDMI bridge driver sometimes fails during probe and the drmdev do not start. Of course this is development stage so I have serial console I can diagnose the problem, disable HDMI, fix the problem, etc... But what happens in case of end-user. He will see black screen - bricked phone. In case the bridge will be implemented using drm_panel he will have working phone with broken HDMI, much better.
well, tbh, I don't think an end-user will see the device if hdmi were broken ;-)
It can break also during phone utilization.
I suppose if bridge/panel where loaded dynamically (or at least after drm device and drm_{connector,encoder,etc} are created, it would help a bit here. I'd kinda hope that isn't the only benefit/reason to make things more dynamic. Especially if we allow bridges/panels to be unloaded.. (just loading them dynamically doesn't seem as scary from locking perspective)
- And the last thing, it is more about the concept/design. drm_bridge,
drm_hw_block suggests that those interfaces describes the whole device: bridge, panel, whatever.
hmm, I don't think this is the case. I can easily see things like:
struct foo_panel { struct drm_panel base; struct drm_bridge bridge; ... }
where a panel implementation implements both panel and bridge. In fact that is kinda what I was encouraging.
I guess it can work, but I see it sub-optimal. In general, looking on the hardware the same video data goes to the panel and to the bridge (if they are of the same type of course), I do not know why it couldn't be mapped to software interfaces. For example drm_sink, as I described previously (now it is cited below).
I'm not entirely sure why letting a panel implement multiple different interfaces (where needed) is suboptimal. It seems more sub-optimal to put panel related fxns which are only applicable to panels in drm_bridge_funcs.
foo implemented using drm_panel and drm_bridge seems to me less optimal than foo implemented with the same functionality but using drm_panel only.
Well, my initial reaction when you start talking about drm_src and drm_sinks is that this can quickly get over-designed. I'm not trying to turn kms into v4l2 unless there is a good reason. But maybe I'm assuming too much about what you are proposing.
OK, lesson learned: avoid using word sink :)
Regards Andrzej
BR, -R
Regards Andrzej
BR, -R
In my approach I have an interface to describe only one video input port of the device. And drm_panel is in fact misleading name, drm_sink may be better. So real panel would implement drm_sink interface. Bridge would implement drm_sink interface and it will request other drm_sink interface, to interact with the panel which is after it. This approach seems to me more flexible. Beside things I have described above it will allow to implement also more complicated devices, dsi hubs, video mixers, etc.
Regards Andrzej
BR, -R
> So why not implement ptn3460 bridge as drm_panel which internally uses > another drm_panel. With this approach everything fits much better. > You do not need those (pre|post)_(enable|disable) calls, you do not need > to implement connector in the bridge and you have a driver following > linux driver model. And no single bit changed in drm core. > > I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. > It was not accepted as Inki preferred drm_bridge but as I see the > problems with drm_bridges I have decide to attract attention to much > more cleaner solution. > > [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559 > [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044 > > Regards > Andrzej > > >> Ajay Kumar (3): >> [RFC V2 1/3] drm: implement chaining of drm bridges >> [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges >> [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining >> >> .../bindings/drm/bridge/bridge_panel.txt | 45 ++++ >> drivers/gpu/drm/bridge/Kconfig | 6 + >> drivers/gpu/drm/bridge/Makefile | 1 + >> drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ >> drivers/gpu/drm/bridge/ptn3460.c | 21 +- >> drivers/gpu/drm/drm_crtc.c | 13 +- >> include/drm/bridge/bridge_panel.h | 37 ++++ >> include/drm/drm_crtc.h | 2 + >> 8 files changed, 360 insertions(+), 5 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt >> create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c >> create mode 100644 include/drm/bridge/bridge_panel.h >>
On Mon, May 12, 2014 at 3:06 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/09/2014 05:05 PM, Ajay kumar wrote:
On Fri, May 9, 2014 at 7:29 PM, Rob Clark robdclark@gmail.com wrote:
On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/08/2014 08:24 PM, Rob Clark wrote:
On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/05/2014 09:52 PM, Ajay Kumar wrote: > This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: > git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git > > I have just put up Rob's and Sean's idea of chaining up the bridges > in code, and have implemented basic panel controls as a chained bridge. > This works well with ptn3460 bridge chip on exynos5250-snow board. > > Still need to make use of standard list calls and figure out proper way > of deleting the bridge chain. So, this is just a rough version. As I understand this patchset tries to solve two things:
- Implement panel as drm_bridge, to ease support for hardware chains:
Crtc -> Encoder -> Bridge -> Panel 2. Add support to drm_bridge chaining, to allow software chains: drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel
It is done using Russian doll schema, ops from the bridge calls the same ops from the next bridge and the next bridge ops can do the same.
This schema means that all the bridges including the last one are seen from the drm core point of view as a one big drm_bridge. Additionally in this particular case, the first bridge (ptn3460) implements connector so it is hard to guess what is the location of the 2nd bridge in video stream chain, sometimes it is after the connector, sometimes before. All this is quite confusing.
But if you look at the bridge from upstream video interface point of view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its video input side acts as a panel. On the output side it expects a panel, lvds panel in this case.
tbh, this is mostly about what we call it. Perhaps "bridge" isn't the best name.. I wouldn't object to changing it.
But my thinking was to leave in drm_panel_funcs things that are just needed by the connector (get_modes().. and maybe some day we need detect/etc). Then leave everything else in drm_bridge_funcs. A panel could (if needed) implement both interfaces.
That is basically the same as what you are proposing, but without renaming bridge to panel ;-)
Good to hear that. However there are points which are not clear for me. But first lets clarify names, I will use panel and bridge words to describe the hardware, and drm_panel, drm_bridge to describe the software interfaces.
What bothers me:
- You want to leave connector related callbacks in drm_panel and
the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460 must implement connector internally because of this limitation. I guess it is quite typical bridge. This problem does not exists in case of using drm_panel for ptn3460.
- drm_bridge is attached to the encoder, this and the callback order
suggests the video data flow should be as below: drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
ptn3460 implements drm_bridge and drm_connector so it suggests its drm_bridge should be the last one, so there should be no place to add lvds panel implemented as a drm_bridge after it, as it is done in this patchset.
Additionally it clearly shows that there should be two categories of drm_bridges - non-terminal and terminal.
- drm_dev uses all-or-nothing approach, ie. it will start only when all
its components are up. It simplifies synchronization but is quite fragile - the whole drm will be down due to error in some of its components. For this reason I prefer drm_panel as it is not real drm component it can be attached/detached to/from drm_connector anytime. I am not really sure but drm_bridge does not allow for that.
So I do think we need to stick to this all-or-nothing approach for anything that is visible to userspace (drm_{plane,crtc,encoder,connector}). We don't currently have a way to "hotplug" those so I don't see a real smooth upgrade path to add that in a backwards compatible way that won't cause problems with old userspace.
But, that said, we have more flexibility with things not visible to userspace (drm_{panel,bridge}). I'm not sure how much we want to allow things to be completely dynamic (we already have some hard enough locking fun). But proposals/rfcs/etc welcome.
I guess I'm not completely familiar w/ ptn3460, but the fact that it needs to implement drm_connector makes me a bit suspicious. Seems like a symptom of missing things in drm_panel_funcs. It would be better to always create the connector statically, and just have _detect() -> disconnected if panel==NULL.
ptn3460 has been implemented using drm_bridge and drm_connector, not by me, to be clear :)
Right, that was me.
And to make it more clear from what I see ptn3460 exposes following ops:
- pre_enable (via drm_bridge).
- disable (via drm_bridge),
- get_modes (via drm_connector).
Other ops are exposed just to fulfill requirements of drm frameworks, I guess.
Also detect().
This is something which only Sean can answer! I guess he implemented ptn3460 as connector thinking that bridge would be the last entity in the video pipeline. If that's a real problem, we can still move out the connector part.
Regards, Ajay
The question is how it can be implemented using only drm_bridge.
It was originally implemented this way. Check out the original drm_bridge patch and discussion here: http://lists.freedesktop.org/archives/dri-devel/2013-August/043237.html.
The tl;dr is: If you don't implement the connector as part of the bridge, you need to hack detect_connection() in drm_crtc_helper to either go to the existing connector (which, btw will be the incorrect connector type), or the bridge if there's one connected. That, or the existing connector needs to know about the bridge's existence and call into it for detect() and get_modes(). In the second case, the existing connector will probably also be the wrong connector type.
Real life example to show importance of it: I have a phone with MIPI-DSI panel and HDMI. Due to initialization issues HDMI bridge driver sometimes fails during probe and the drmdev do not start. Of course this is development stage so I have serial console I can diagnose the problem, disable HDMI, fix the problem, etc... But what happens in case of end-user. He will see black screen - bricked phone. In case the bridge will be implemented using drm_panel he will have working phone with broken HDMI, much better.
well, tbh, I don't think an end-user will see the device if hdmi were broken ;-)
It can break also during phone utilization.
I suppose if bridge/panel where loaded dynamically (or at least after drm device and drm_{connector,encoder,etc} are created, it would help a bit here. I'd kinda hope that isn't the only benefit/reason to make things more dynamic. Especially if we allow bridges/panels to be unloaded.. (just loading them dynamically doesn't seem as scary from locking perspective)
- And the last thing, it is more about the concept/design. drm_bridge,
drm_hw_block suggests that those interfaces describes the whole device: bridge, panel, whatever.
hmm, I don't think this is the case. I can easily see things like:
struct foo_panel { struct drm_panel base; struct drm_bridge bridge; ... }
where a panel implementation implements both panel and bridge. In fact that is kinda what I was encouraging.
I guess it can work, but I see it sub-optimal. In general, looking on the hardware the same video data goes to the panel and to the bridge (if they are of the same type of course), I do not know why it couldn't be mapped to software interfaces. For example drm_sink, as I described previously (now it is cited below).
Regards Andrzej
BR, -R
In my approach I have an interface to describe only one video input port of the device. And drm_panel is in fact misleading name, drm_sink may be better. So real panel would implement drm_sink interface. Bridge would implement drm_sink interface and it will request other drm_sink interface, to interact with the panel which is after it. This approach seems to me more flexible. Beside things I have described above it will allow to implement also more complicated devices, dsi hubs, video mixers, etc.
Regards Andrzej
BR, -R
So why not implement ptn3460 bridge as drm_panel which internally uses another drm_panel. With this approach everything fits much better. You do not need those (pre|post)_(enable|disable) calls, you do not need to implement connector in the bridge and you have a driver following linux driver model. And no single bit changed in drm core.
I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. It was not accepted as Inki preferred drm_bridge but as I see the problems with drm_bridges I have decide to attract attention to much more cleaner solution.
Regards Andrzej
> Ajay Kumar (3): > [RFC V2 1/3] drm: implement chaining of drm bridges > [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges > [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining > > .../bindings/drm/bridge/bridge_panel.txt | 45 ++++ > drivers/gpu/drm/bridge/Kconfig | 6 + > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ > drivers/gpu/drm/bridge/ptn3460.c | 21 +- > drivers/gpu/drm/drm_crtc.c | 13 +- > include/drm/bridge/bridge_panel.h | 37 ++++ > include/drm/drm_crtc.h | 2 + > 8 files changed, 360 insertions(+), 5 deletions(-) > create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt > create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c > create mode 100644 include/drm/bridge/bridge_panel.h >
On 05/12/2014 06:00 PM, Sean Paul wrote:
On Mon, May 12, 2014 at 3:06 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/09/2014 05:05 PM, Ajay kumar wrote:
On Fri, May 9, 2014 at 7:29 PM, Rob Clark robdclark@gmail.com wrote:
On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda a.hajda@samsung.com wrote:
On 05/08/2014 08:24 PM, Rob Clark wrote:
On Thu, May 8, 2014 at 2:41 AM, Andrzej Hajda a.hajda@samsung.com wrote: > On 05/05/2014 09:52 PM, Ajay Kumar wrote: >> This patchset is based on exynos-drm-next-todo branch of Inki Dae's tree at: >> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git >> >> I have just put up Rob's and Sean's idea of chaining up the bridges >> in code, and have implemented basic panel controls as a chained bridge. >> This works well with ptn3460 bridge chip on exynos5250-snow board. >> >> Still need to make use of standard list calls and figure out proper way >> of deleting the bridge chain. So, this is just a rough version. > As I understand this patchset tries to solve two things: > 1. Implement panel as drm_bridge, to ease support for hardware chains: > Crtc -> Encoder -> Bridge -> Panel > 2. Add support to drm_bridge chaining, to allow software chains: > drm_crtc -> drm_encoder -> drm_bridge -> drm_bridge,panel > > It is done using Russian doll schema, ops from the bridge calls the same > ops from the next bridge and the next bridge ops can do the same. > > This schema means that all the bridges including the last one are seen > from the drm core point of view as a one big drm_bridge. Additionally in > this particular case, the first bridge (ptn3460) implements connector > so it is hard to guess what is the location of the 2nd bridge in video > stream chain, sometimes it is after the connector, sometimes before. > All this is quite confusing. > > But if you look at the bridge from upstream video interface point of > view it is just a panel, edp panel in case of ptn3460, ie ptn3460 on its > video input side acts as a panel. On the output side it expects a panel, > lvds panel in this case. tbh, this is mostly about what we call it. Perhaps "bridge" isn't the best name.. I wouldn't object to changing it.
But my thinking was to leave in drm_panel_funcs things that are just needed by the connector (get_modes().. and maybe some day we need detect/etc). Then leave everything else in drm_bridge_funcs. A panel could (if needed) implement both interfaces.
That is basically the same as what you are proposing, but without renaming bridge to panel ;-)
Good to hear that. However there are points which are not clear for me. But first lets clarify names, I will use panel and bridge words to describe the hardware, and drm_panel, drm_bridge to describe the software interfaces.
What bothers me:
- You want to leave connector related callbacks in drm_panel and
the rest in drm_bridge. In case of ptn3460 it does not work, ptn3460 must implement connector internally because of this limitation. I guess it is quite typical bridge. This problem does not exists in case of using drm_panel for ptn3460.
- drm_bridge is attached to the encoder, this and the callback order
suggests the video data flow should be as below: drm_crtc -> drm_encoder [-> drm_bridge] -> drm_connector [-> drm_panel]
ptn3460 implements drm_bridge and drm_connector so it suggests its drm_bridge should be the last one, so there should be no place to add lvds panel implemented as a drm_bridge after it, as it is done in this patchset.
Additionally it clearly shows that there should be two categories of drm_bridges - non-terminal and terminal.
- drm_dev uses all-or-nothing approach, ie. it will start only when all
its components are up. It simplifies synchronization but is quite fragile - the whole drm will be down due to error in some of its components. For this reason I prefer drm_panel as it is not real drm component it can be attached/detached to/from drm_connector anytime. I am not really sure but drm_bridge does not allow for that.
So I do think we need to stick to this all-or-nothing approach for anything that is visible to userspace (drm_{plane,crtc,encoder,connector}). We don't currently have a way to "hotplug" those so I don't see a real smooth upgrade path to add that in a backwards compatible way that won't cause problems with old userspace.
But, that said, we have more flexibility with things not visible to userspace (drm_{panel,bridge}). I'm not sure how much we want to allow things to be completely dynamic (we already have some hard enough locking fun). But proposals/rfcs/etc welcome.
I guess I'm not completely familiar w/ ptn3460, but the fact that it needs to implement drm_connector makes me a bit suspicious. Seems like a symptom of missing things in drm_panel_funcs. It would be better to always create the connector statically, and just have _detect() -> disconnected if panel==NULL.
ptn3460 has been implemented using drm_bridge and drm_connector, not by me, to be clear :)
Right, that was me.
And to make it more clear from what I see ptn3460 exposes following ops:
- pre_enable (via drm_bridge).
- disable (via drm_bridge),
- get_modes (via drm_connector).
Other ops are exposed just to fulfill requirements of drm frameworks, I guess.
Also detect().
This is something which only Sean can answer! I guess he implemented ptn3460 as connector thinking that bridge would be the last entity in the video pipeline. If that's a real problem, we can still move out the connector part.
Regards, Ajay
The question is how it can be implemented using only drm_bridge.
It was originally implemented this way. Check out the original drm_bridge patch and discussion here: http://lists.freedesktop.org/archives/dri-devel/2013-August/043237.html.
The tl;dr is: If you don't implement the connector as part of the bridge, you need to hack detect_connection() in drm_crtc_helper to either go to the existing connector (which, btw will be the incorrect connector type), or the bridge if there's one connected. That, or the existing connector needs to know about the bridge's existence and call into it for detect() and get_modes(). In the second case, the existing connector will probably also be the wrong connector type.
Yes, I understand it and my idea was to use drm_panel instead of drm_bridge, at least for bridges which provides connector related functions (get_modes, detect). It seems more straightforward to implement and it simplifies chaining.
Regards Andrzej
Real life example to show importance of it: I have a phone with MIPI-DSI panel and HDMI. Due to initialization issues HDMI bridge driver sometimes fails during probe and the drmdev do not start. Of course this is development stage so I have serial console I can diagnose the problem, disable HDMI, fix the problem, etc... But what happens in case of end-user. He will see black screen - bricked phone. In case the bridge will be implemented using drm_panel he will have working phone with broken HDMI, much better.
well, tbh, I don't think an end-user will see the device if hdmi were broken ;-)
It can break also during phone utilization.
I suppose if bridge/panel where loaded dynamically (or at least after drm device and drm_{connector,encoder,etc} are created, it would help a bit here. I'd kinda hope that isn't the only benefit/reason to make things more dynamic. Especially if we allow bridges/panels to be unloaded.. (just loading them dynamically doesn't seem as scary from locking perspective)
- And the last thing, it is more about the concept/design. drm_bridge,
drm_hw_block suggests that those interfaces describes the whole device: bridge, panel, whatever.
hmm, I don't think this is the case. I can easily see things like:
struct foo_panel { struct drm_panel base; struct drm_bridge bridge; ... }
where a panel implementation implements both panel and bridge. In fact that is kinda what I was encouraging.
I guess it can work, but I see it sub-optimal. In general, looking on the hardware the same video data goes to the panel and to the bridge (if they are of the same type of course), I do not know why it couldn't be mapped to software interfaces. For example drm_sink, as I described previously (now it is cited below).
Regards Andrzej
BR, -R
In my approach I have an interface to describe only one video input port of the device. And drm_panel is in fact misleading name, drm_sink may be better. So real panel would implement drm_sink interface. Bridge would implement drm_sink interface and it will request other drm_sink interface, to interact with the panel which is after it. This approach seems to me more flexible. Beside things I have described above it will allow to implement also more complicated devices, dsi hubs, video mixers, etc.
Regards Andrzej
BR, -R
> So why not implement ptn3460 bridge as drm_panel which internally uses > another drm_panel. With this approach everything fits much better. > You do not need those (pre|post)_(enable|disable) calls, you do not need > to implement connector in the bridge and you have a driver following > linux driver model. And no single bit changed in drm core. > > I have implemented this way DSI/LVDS bridge, it was sent as RFC [1][2]. > It was not accepted as Inki preferred drm_bridge but as I see the > problems with drm_bridges I have decide to attract attention to much > more cleaner solution. > > [1]: http://permalink.gmane.org/gmane.linux.drivers.devicetree/61559 > [2]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/27044 > > Regards > Andrzej > > >> Ajay Kumar (3): >> [RFC V2 1/3] drm: implement chaining of drm bridges >> [RFC V2 2/3] drm/bridge: add a dummy panel driver to support lvds bridges >> [RFC V2 3/3] drm/bridge: ptn3460: support bridge chaining >> >> .../bindings/drm/bridge/bridge_panel.txt | 45 ++++ >> drivers/gpu/drm/bridge/Kconfig | 6 + >> drivers/gpu/drm/bridge/Makefile | 1 + >> drivers/gpu/drm/bridge/bridge_panel.c | 240 +++++++++++++++++++++ >> drivers/gpu/drm/bridge/ptn3460.c | 21 +- >> drivers/gpu/drm/drm_crtc.c | 13 +- >> include/drm/bridge/bridge_panel.h | 37 ++++ >> include/drm/drm_crtc.h | 2 + >> 8 files changed, 360 insertions(+), 5 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/drm/bridge/bridge_panel.txt >> create mode 100644 drivers/gpu/drm/bridge/bridge_panel.c >> create mode 100644 include/drm/bridge/bridge_panel.h >>
On Fri, May 09, 2014 at 09:59:34AM -0400, Rob Clark wrote:
On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda a.hajda@samsung.com wrote:
[...]
- And the last thing, it is more about the concept/design. drm_bridge,
drm_hw_block suggests that those interfaces describes the whole device: bridge, panel, whatever.
hmm, I don't think this is the case. I can easily see things like:
struct foo_panel { struct drm_panel base; struct drm_bridge bridge; ... }
where a panel implementation implements both panel and bridge. In fact that is kinda what I was encouraging.
For lack of a better entry point into the discussion, let me pick this example. It seems like in general we all agree that the basic structure would have to be something like this:
CRTC -> encoder -> bridge [ -> bridge ... ] -> panel
Where panel could optionally be a bridge/panel hybrid as Rob pointed out. I'm not sure if there's panels like this, but I suspect the use case would be where a panel had built-in controls to modify the image in some fashion (brightness, saturation, ...). I think those things exist in DCS for example.
What's missing here, and if I understand correctly what Sean said about the connector type, what we need to solve is how to wire up the connector so that it reflects the correct type. So the above would have to become something like this:
CRTC -> encoder -> bridge [ -> bridge ... ] -> panel connector -----------------------------^
One of the problems with that approach is that panel is more than just a video sink. It also controls the panel (and optionally backlight) power. The standard DRM connector helpers don't work well in that case, because drm_helper_connector_dpms() forwards changes to the CRTC and encoder, though that could possibly be solved by wrapping it in a small custom implementation that also controls the panel. Anyway, that's really just an implementation detail.
So once a complete chain from encoder to panel has been created, we need a way to find the appropriate connector type. Perhaps to achieve that it would be useful to instantiate the connector by the bridge that's connected to the panel. So essentially something like this:
struct drm_bridge { /* to allow chaining */ struct drm_bridge *next; /* for bridges directly connected to a panel or monitor */ struct drm_connector *connector; /* for bridges directly connected to a panel */ struct drm_panel *panel;
... };
To make this work in a generic way, I think we're missing one bridge instance. Currently the bridge in an encoder is completely optional, so if there is no bridge in the system this needs to be special cased. One way to unify this would be to make drm_encoder implement drm_bridge, or an alternative would be to make drm_panel implement a bridge. Both can possibly be dummies stubbed out with simple helpers.
I wonder if this would have any consequences on Dave's DP MST work, since presumably an MST hub could be considered a bridge. In that case I guess there would need to be support for multiple "next" bridges, perhaps a simple doubly-linked list instead of a next pointer. The first bridge would then be used to model the hub's input and child bridges would be used to model each of the outputs.
Thierry
On Thu, May 15, 2014 at 12:32:58PM +0200, Thierry Reding wrote:
On Fri, May 09, 2014 at 09:59:34AM -0400, Rob Clark wrote:
On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda a.hajda@samsung.com wrote:
[...]
- And the last thing, it is more about the concept/design. drm_bridge,
drm_hw_block suggests that those interfaces describes the whole device: bridge, panel, whatever.
hmm, I don't think this is the case. I can easily see things like:
struct foo_panel { struct drm_panel base; struct drm_bridge bridge; ... }
where a panel implementation implements both panel and bridge. In fact that is kinda what I was encouraging.
For lack of a better entry point into the discussion, let me pick this example. It seems like in general we all agree that the basic structure would have to be something like this:
CRTC -> encoder -> bridge [ -> bridge ... ] -> panel
Where panel could optionally be a bridge/panel hybrid as Rob pointed out. I'm not sure if there's panels like this, but I suspect the use case would be where a panel had built-in controls to modify the image in some fashion (brightness, saturation, ...). I think those things exist in DCS for example.
What's missing here, and if I understand correctly what Sean said about the connector type, what we need to solve is how to wire up the connector so that it reflects the correct type. So the above would have to become something like this:
CRTC -> encoder -> bridge [ -> bridge ... ] -> panel connector -----------------------------^
This is kinda always how I've thought it should play out: The last item in the chain actually implements the drm connector, everyone else just ignores it.
One of the problems with that approach is that panel is more than just a video sink. It also controls the panel (and optionally backlight) power. The standard DRM connector helpers don't work well in that case, because drm_helper_connector_dpms() forwards changes to the CRTC and encoder, though that could possibly be solved by wrapping it in a small custom implementation that also controls the panel. Anyway, that's really just an implementation detail.
I don't see an issue with the dpms really. At worst the overall kms driver (which provides the crtc and encoder implementation) needs to pass a suitable dpms implementation for the drm_bridge to use in the connector. Or we could just move this vfunc into the core kms funtion table since everyone uses the same (well except i915, but that's a different story).
dpms changes would then still be driven through the crtc -> encoder -> bridge ... chain.
So once a complete chain from encoder to panel has been created, we need a way to find the appropriate connector type. Perhaps to achieve that it would be useful to instantiate the connector by the bridge that's connected to the panel. So essentially something like this:
struct drm_bridge { /* to allow chaining */ struct drm_bridge *next; /* for bridges directly connected to a panel or monitor */ struct drm_connector *connector; /* for bridges directly connected to a panel */ struct drm_panel *panel;
Imo these two shouldn't be here, but instead should be embedded into the overall structure the drm_bridge driver is using.
...};
To make this work in a generic way, I think we're missing one bridge instance. Currently the bridge in an encoder is completely optional, so if there is no bridge in the system this needs to be special cased. One way to unify this would be to make drm_encoder implement drm_bridge, or an alternative would be to make drm_panel implement a bridge. Both can possibly be dummies stubbed out with simple helpers.
Yeah, imo if the panel isn't just a dumb thing but needs to actively take part in the modeset dance, it simply needs to implement itself as a drm_bridge+panel combo and do the magic in the various hooks provided.
I wonder if this would have any consequences on Dave's DP MST work, since presumably an MST hub could be considered a bridge. In that case I guess there would need to be support for multiple "next" bridges, perhaps a simple doubly-linked list instead of a next pointer. The first bridge would then be used to model the hub's input and child bridges would be used to model each of the outputs.
DP is standardize. No need to wrestle the complexity through a drm_bridge, since code reuse anywhere else (non-DP) will be know to be zero. And for all the guys implementing a DP output can simply use the helpers. So I don't see an upside of this idea.
Imo we should restrict drm_bridge to all the crazy transcoder chips/blocks commonly found on SoCs which all do non-standard stuff and where we actually have an established or anticipated need for sharing. -Daniel
On Thu, May 15, 2014 at 6:04 PM, Daniel Vetter daniel@ffwll.ch wrote:
On Thu, May 15, 2014 at 12:32:58PM +0200, Thierry Reding wrote:
On Fri, May 09, 2014 at 09:59:34AM -0400, Rob Clark wrote:
On Fri, May 9, 2014 at 5:08 AM, Andrzej Hajda a.hajda@samsung.com wrote:
[...]
- And the last thing, it is more about the concept/design. drm_bridge,
drm_hw_block suggests that those interfaces describes the whole device: bridge, panel, whatever.
hmm, I don't think this is the case. I can easily see things like:
struct foo_panel { struct drm_panel base; struct drm_bridge bridge; ... }
where a panel implementation implements both panel and bridge. In fact that is kinda what I was encouraging.
For lack of a better entry point into the discussion, let me pick this example. It seems like in general we all agree that the basic structure would have to be something like this:
CRTC -> encoder -> bridge [ -> bridge ... ] -> panelWhere panel could optionally be a bridge/panel hybrid as Rob pointed out. I'm not sure if there's panels like this, but I suspect the use case would be where a panel had built-in controls to modify the image in some fashion (brightness, saturation, ...). I think those things exist in DCS for example.
What's missing here, and if I understand correctly what Sean said about the connector type, what we need to solve is how to wire up the connector so that it reflects the correct type. So the above would have to become something like this:
CRTC -> encoder -> bridge [ -> bridge ... ] -> panel connector -----------------------------^This is kinda always how I've thought it should play out: The last item in the chain actually implements the drm connector, everyone else just ignores it.
The last bridge or the panel?
One of the problems with that approach is that panel is more than just a video sink. It also controls the panel (and optionally backlight) power. The standard DRM connector helpers don't work well in that case, because drm_helper_connector_dpms() forwards changes to the CRTC and encoder, though that could possibly be solved by wrapping it in a small custom implementation that also controls the panel. Anyway, that's really just an implementation detail.
Why did this discussion come up? Are we going to call panel controls from a common layer?
I don't see an issue with the dpms really. At worst the overall kms driver (which provides the crtc and encoder implementation) needs to pass a suitable dpms implementation for the drm_bridge to use in the connector. Or we could just move this vfunc into the core kms funtion table since everyone uses the same (well except i915, but that's a different story).
dpms changes would then still be driven through the crtc -> encoder -> bridge ... chain.
So once a complete chain from encoder to panel has been created, we need a way to find the appropriate connector type. Perhaps to achieve that it would be useful to instantiate the connector by the bridge that's connected to the panel. So essentially something like this:
struct drm_bridge { /* to allow chaining */ struct drm_bridge *next; /* for bridges directly connected to a panel or monitor */ struct drm_connector *connector; /* for bridges directly connected to a panel */ struct drm_panel *panel;Imo these two shouldn't be here, but instead should be embedded into the overall structure the drm_bridge driver is using.
... };To make this work in a generic way, I think we're missing one bridge instance. Currently the bridge in an encoder is completely optional, so if there is no bridge in the system this needs to be special cased. One way to unify this would be to make drm_encoder implement drm_bridge, or an alternative would be to make drm_panel implement a bridge. Both can possibly be dummies stubbed out with simple helpers.
Yeah, imo if the panel isn't just a dumb thing but needs to actively take part in the modeset dance, it simply needs to implement itself as a drm_bridge+panel combo and do the magic in the various hooks provided.
And, what does one mean when a real panel should implement both drm_panel and drm_bridge? who will call drm_bridge->funcs for the panel?, and who will call drm_panel->funcs for the panel? Can someone please elaborate this with existing implementation details for bridge and panel? I am really getting confused.
I wonder if this would have any consequences on Dave's DP MST work, since presumably an MST hub could be considered a bridge. In that case I guess there would need to be support for multiple "next" bridges, perhaps a simple doubly-linked list instead of a next pointer. The first bridge would then be used to model the hub's input and child bridges would be used to model each of the outputs.
DP is standardize. No need to wrestle the complexity through a drm_bridge, since code reuse anywhere else (non-DP) will be know to be zero. And for all the guys implementing a DP output can simply use the helpers. So I don't see an upside of this idea.
Imo we should restrict drm_bridge to all the crazy transcoder chips/blocks commonly found on SoCs which all do non-standard stuff and where we actually have an established or anticipated need for sharing.
-Daniel
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Thanks, Ajay
dri-devel@lists.freedesktop.org
-
Ajay Kumar -
Ajay kumar -
Andrzej Hajda -
Daniel Vetter -
Inki Dae -
Rob Clark -
Sean Paul -
Thierry Reding