| Message ID | 1406316130-4744-7-git-send-email-ajaykumar.rs@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, Jul 26, 2014 at 12:52:08AM +0530, Ajay Kumar wrote: > This patch tries to seperate drm_bridge implementation > into 2 parts, a drm part and a non_drm part. > > A set of helper functions are defined in this patch to make > bridge driver probe independent of the drm flow. > > The bridge devices register themselves on a lookup table > when they get probed by calling "drm_bridge_add_for_lookup". > > The parent encoder driver waits till the bridge is available in the > lookup table(by calling "of_drm_find_bridge") and then continues with > its initialization. > > The encoder driver should call "drm_bridge_attach_encoder" to pass on > the drm_device and the encoder pointers to the bridge object. > > Now that the drm_device pointer is available, the encoder then calls > "bridge->funcs->post_encoder_init" to allow the bridge to continue > registering itself with the drm core. > > Also, non driver model based ptn3460 driver is removed in this patch. Why is it removed in this patch? Can't you do this incrementally rather than remove the driver in this patch and add it again later? If you do it this way then we'll always have this one commit where devices that have a ptn3460 don't work, so it becomes impossible to bisect across this commit. > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c [...] > +int drm_bridge_add_for_lookup(struct drm_bridge *bridge) > +{ > + mutex_lock(&bridge_lock); > + list_add_tail(&bridge->head, &bridge_lookup); > + mutex_unlock(&bridge_lock); > + > + return 0; > +} > +EXPORT_SYMBOL(drm_bridge_add_for_lookup); > + > +void drm_bridge_remove_from_lookup(struct drm_bridge *bridge) > +{ > + mutex_lock(&bridge_lock); > + list_del_init(&bridge->head); > + mutex_unlock(&bridge_lock); > +} > +EXPORT_SYMBOL(drm_bridge_remove_from_lookup); The "_for_lookup" and "_from_lookup" suffixes aren't useful in my opinion. > +int drm_bridge_attach_encoder(struct drm_bridge *bridge, > + struct drm_encoder *encoder) And this could simply be "drm_bridge_attach()" since we'll only ever want to attach it to encoders. > +{ > + if (!bridge || !encoder) > + return -EINVAL; > + > + if (bridge->encoder) > + return -EBUSY; > + > + encoder->bridge = bridge; > + bridge->encoder = encoder; > + bridge->drm_dev = encoder->dev; Should this function perhaps call the bridge's ->post_encoder_init()? And it should probably call drm_bridge_init() too, since the DRM device is now available. > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c [...] Maybe since you're introducing a new drm_bridge.c file above already it would make sense to move out existing drm_bridge related code in a preparatory patch? Maybe Sean or Rob can comment on whether there was a specific reason to include it in drm_crtc.c in the first place. > @@ -1012,8 +1010,7 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, > if (ret) > goto out; > > - bridge->dev = dev; > - bridge->funcs = funcs; > + bridge->drm_dev = dev; This sets ->drm_dev, but it was already set in drm_bridge_attach(), so I think that's one more argument to call this function when attaching. > diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c [...] > @@ -1370,7 +1361,7 @@ static const struct component_ops exynos_dp_ops = { > static int exynos_dp_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > - struct device_node *panel_node; > + struct device_node *panel_node, *bridge_node; Nit: I don't think you'll need two variables here, since once you've obtained the real panel or bridge objects you no longer need the OF nodes. > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index e529b68..e5a41ad 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -619,6 +619,7 @@ struct drm_plane { > > /** > * drm_bridge_funcs - drm_bridge control functions > + * @post_encoder_init: called by the parent encoder Maybe rename this to "attach" to make it more obvious when exactly it's called? > * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge > * @disable: Called right before encoder prepare, disables the bridge > * @post_disable: Called right after encoder prepare, for lockstepped disable > @@ -628,6 +629,7 @@ struct drm_plane { > * @destroy: make object go away > */ > struct drm_bridge_funcs { > + int (*post_encoder_init)(struct drm_bridge *bridge); > bool (*mode_fixup)(struct drm_bridge *bridge, > const struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode); > @@ -648,15 +650,19 @@ struct drm_bridge_funcs { > * @base: base mode object > * @funcs: control functions > * @driver_private: pointer to the bridge driver's internal context > + * @connector_polled: polled flag needed for registering connector Can you explain why this new field is needed? It seems like a completely unrelated change. Thierry
On Wed, Jul 30, 2014 at 4:49 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Sat, Jul 26, 2014 at 12:52:08AM +0530, Ajay Kumar wrote: >> This patch tries to seperate drm_bridge implementation >> into 2 parts, a drm part and a non_drm part. >> >> A set of helper functions are defined in this patch to make >> bridge driver probe independent of the drm flow. >> >> The bridge devices register themselves on a lookup table >> when they get probed by calling "drm_bridge_add_for_lookup". >> >> The parent encoder driver waits till the bridge is available in the >> lookup table(by calling "of_drm_find_bridge") and then continues with >> its initialization. >> >> The encoder driver should call "drm_bridge_attach_encoder" to pass on >> the drm_device and the encoder pointers to the bridge object. >> >> Now that the drm_device pointer is available, the encoder then calls >> "bridge->funcs->post_encoder_init" to allow the bridge to continue >> registering itself with the drm core. >> >> Also, non driver model based ptn3460 driver is removed in this patch. > > Why is it removed in this patch? Can't you do this incrementally rather > than remove the driver in this patch and add it again later? If you do > it this way then we'll always have this one commit where devices that > have a ptn3460 don't work, so it becomes impossible to bisect across > this commit. Ok. I will try to modify ptn3460 to support driver model in this patch itself. And then, adding panel support, converting it to gpiod interface and other cleanups should follow. >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > [...] >> +int drm_bridge_add_for_lookup(struct drm_bridge *bridge) >> +{ >> + mutex_lock(&bridge_lock); >> + list_add_tail(&bridge->head, &bridge_lookup); >> + mutex_unlock(&bridge_lock); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(drm_bridge_add_for_lookup); >> + >> +void drm_bridge_remove_from_lookup(struct drm_bridge *bridge) >> +{ >> + mutex_lock(&bridge_lock); >> + list_del_init(&bridge->head); >> + mutex_unlock(&bridge_lock); >> +} >> +EXPORT_SYMBOL(drm_bridge_remove_from_lookup); > > The "_for_lookup" and "_from_lookup" suffixes aren't useful in my > opinion. Ok. I will just remove the suffix. >> +int drm_bridge_attach_encoder(struct drm_bridge *bridge, >> + struct drm_encoder *encoder) > > And this could simply be "drm_bridge_attach()" since we'll only ever > want to attach it to encoders. Right. >> +{ >> + if (!bridge || !encoder) >> + return -EINVAL; >> + >> + if (bridge->encoder) >> + return -EBUSY; >> + >> + encoder->bridge = bridge; >> + bridge->encoder = encoder; >> + bridge->drm_dev = encoder->dev; > > Should this function perhaps call the bridge's ->post_encoder_init()? > And it should probably call drm_bridge_init() too, since the DRM device > is now available. This will cleanup some code in both the drivers. I will change it. >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > [...] > > Maybe since you're introducing a new drm_bridge.c file above already it > would make sense to move out existing drm_bridge related code in a > preparatory patch? > > Maybe Sean or Rob can comment on whether there was a specific reason to > include it in drm_crtc.c in the first place. > >> @@ -1012,8 +1010,7 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, >> if (ret) >> goto out; >> >> - bridge->dev = dev; >> - bridge->funcs = funcs; >> + bridge->drm_dev = dev; > > This sets ->drm_dev, but it was already set in drm_bridge_attach(), so I > think that's one more argument to call this function when attaching. Point accepted. >> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c > [...] >> @@ -1370,7 +1361,7 @@ static const struct component_ops exynos_dp_ops = { >> static int exynos_dp_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> - struct device_node *panel_node; >> + struct device_node *panel_node, *bridge_node; > > Nit: I don't think you'll need two variables here, since once you've > obtained the real panel or bridge objects you no longer need the OF > nodes. Right. >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >> index e529b68..e5a41ad 100644 >> --- a/include/drm/drm_crtc.h >> +++ b/include/drm/drm_crtc.h >> @@ -619,6 +619,7 @@ struct drm_plane { >> >> /** >> * drm_bridge_funcs - drm_bridge control functions >> + * @post_encoder_init: called by the parent encoder > > Maybe rename this to "attach" to make it more obvious when exactly it's > called? "post_encoder_attach"? >> * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge >> * @disable: Called right before encoder prepare, disables the bridge >> * @post_disable: Called right after encoder prepare, for lockstepped disable >> @@ -628,6 +629,7 @@ struct drm_plane { >> * @destroy: make object go away >> */ >> struct drm_bridge_funcs { >> + int (*post_encoder_init)(struct drm_bridge *bridge); >> bool (*mode_fixup)(struct drm_bridge *bridge, >> const struct drm_display_mode *mode, >> struct drm_display_mode *adjusted_mode); >> @@ -648,15 +650,19 @@ struct drm_bridge_funcs { >> * @base: base mode object >> * @funcs: control functions >> * @driver_private: pointer to the bridge driver's internal context >> + * @connector_polled: polled flag needed for registering connector > > Can you explain why this new field is needed? It seems like a completely > unrelated change. How do I select this flag for the bridge chip? Assume if I did select DRM_CONNECTOR_POLL_HPD, where to call drm_helper_hpd_irq_event in the driver? Is post_encoder_init a right place? Without the polled flag, I get display very late. Please throw some light on this! Ajay
On Wed, Jul 30, 2014 at 08:01:44PM +0530, Ajay kumar wrote: > On Wed, Jul 30, 2014 at 4:49 PM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Sat, Jul 26, 2014 at 12:52:08AM +0530, Ajay Kumar wrote: > >> This patch tries to seperate drm_bridge implementation > >> into 2 parts, a drm part and a non_drm part. > >> > >> A set of helper functions are defined in this patch to make > >> bridge driver probe independent of the drm flow. > >> > >> The bridge devices register themselves on a lookup table > >> when they get probed by calling "drm_bridge_add_for_lookup". > >> > >> The parent encoder driver waits till the bridge is available in the > >> lookup table(by calling "of_drm_find_bridge") and then continues with > >> its initialization. > >> > >> The encoder driver should call "drm_bridge_attach_encoder" to pass on > >> the drm_device and the encoder pointers to the bridge object. > >> > >> Now that the drm_device pointer is available, the encoder then calls > >> "bridge->funcs->post_encoder_init" to allow the bridge to continue > >> registering itself with the drm core. > >> > >> Also, non driver model based ptn3460 driver is removed in this patch. > > > > Why is it removed in this patch? Can't you do this incrementally rather > > than remove the driver in this patch and add it again later? If you do > > it this way then we'll always have this one commit where devices that > > have a ptn3460 don't work, so it becomes impossible to bisect across > > this commit. > Ok. I will try to modify ptn3460 to support driver model in this patch itself. > And then, adding panel support, converting it to gpiod interface and other > cleanups should follow. I think it should even be possible to do this in more separate steps. For example you could add the new bridge infrastructure without touching any of the existing drivers (so that they are completely unaffected by the changes) and then start converting one by one. For some of the changes this may be difficult (such as the dev -> drm_dev rename to make room for the new struct device *dev). But that could for example be done in a preparatory patch that first renames the field, so that the "infrastructure" patch can add the new field without renaming any fields and therefore needing changes to drivers directly. The goal of that whole exercise is to allow display drivers to keep working with the existing API (ptn3460_init()) while we convert the bridge drivers to register with the new framework. Then we can more safely convert each display driver individually to make use of the new framework and once all drivers have been converted the old API can simply be removed. That way there should be no impact on existing functionality at any point. > >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > [...] > > > > Maybe since you're introducing a new drm_bridge.c file above already it > > would make sense to move out existing drm_bridge related code in a > > preparatory patch? > > > > Maybe Sean or Rob can comment on whether there was a specific reason to > > include it in drm_crtc.c in the first place. > > > >> @@ -1012,8 +1010,7 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, > >> if (ret) > >> goto out; > >> > >> - bridge->dev = dev; > >> - bridge->funcs = funcs; > >> + bridge->drm_dev = dev; > > > > This sets ->drm_dev, but it was already set in drm_bridge_attach(), so I > > think that's one more argument to call this function when attaching. > Point accepted. I forgot to mention earlier. drm_dev seems redundant to me. I'd go with just "drm". > >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > >> index e529b68..e5a41ad 100644 > >> --- a/include/drm/drm_crtc.h > >> +++ b/include/drm/drm_crtc.h > >> @@ -619,6 +619,7 @@ struct drm_plane { > >> > >> /** > >> * drm_bridge_funcs - drm_bridge control functions > >> + * @post_encoder_init: called by the parent encoder > > > > Maybe rename this to "attach" to make it more obvious when exactly it's > > called? > "post_encoder_attach"? "post_encoder_" doesn't contain much information, or even ambiguous information. What does "post" "encoder" mean? A bridge is always attached to an encoder, so "encoder" can be dropped. Now "post" has implications as to the time when it is called, but does it mean after the encoder has been initialized, or after the encoder has been removed? Simply "attach" means it's called by the parent encoder to initialize the bridge once it's been attached to an encoder. So obviously it's after the encoder has been initialized. "attach" has all he information required. Any prefix is redundant in my opinion and removing prefixes gives shorter names and reduces the number of keypresses. > >> * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge > >> * @disable: Called right before encoder prepare, disables the bridge > >> * @post_disable: Called right after encoder prepare, for lockstepped disable > >> @@ -628,6 +629,7 @@ struct drm_plane { > >> * @destroy: make object go away > >> */ > >> struct drm_bridge_funcs { > >> + int (*post_encoder_init)(struct drm_bridge *bridge); > >> bool (*mode_fixup)(struct drm_bridge *bridge, > >> const struct drm_display_mode *mode, > >> struct drm_display_mode *adjusted_mode); > >> @@ -648,15 +650,19 @@ struct drm_bridge_funcs { > >> * @base: base mode object > >> * @funcs: control functions > >> * @driver_private: pointer to the bridge driver's internal context > >> + * @connector_polled: polled flag needed for registering connector > > > > Can you explain why this new field is needed? It seems like a completely > > unrelated change. > How do I select this flag for the bridge chip? > Assume if I did select DRM_CONNECTOR_POLL_HPD, where to call > drm_helper_hpd_irq_event in the driver? Is post_encoder_init a right place? > > Without the polled flag, I get display very late. > Please throw some light on this! I just don't understand why it's necessary to implement this field in the drm_bridge. Every bridge driver will already implement a connector, in which case it can simply set the connector's .polled field, can't it? It seems like the only reason you have it in drm_bridge is so that the encoder driver can set it. But I don't see why it should be doing that. The polled state is a property of the connector, and the encoder driver doesn't know anything about it. So if the bridge has a way to detect HPD then it should be setting up the connector to properly report it. For example if the bridge has an input pin to detect it, then it could use a GPIO to receive interrupts and call drm_helper_hpd_irq_event() in the interrupt handler. Perhaps you can explain the exact setup where you need this (or point me at the code since I can't seem to find the relevant location) so that I can gain a better understanding. Thierry
On Wed, Jul 30, 2014 at 8:38 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Jul 30, 2014 at 08:01:44PM +0530, Ajay kumar wrote: >> On Wed, Jul 30, 2014 at 4:49 PM, Thierry Reding >> <thierry.reding@gmail.com> wrote: >> > On Sat, Jul 26, 2014 at 12:52:08AM +0530, Ajay Kumar wrote: >> >> This patch tries to seperate drm_bridge implementation >> >> into 2 parts, a drm part and a non_drm part. >> >> >> >> A set of helper functions are defined in this patch to make >> >> bridge driver probe independent of the drm flow. >> >> >> >> The bridge devices register themselves on a lookup table >> >> when they get probed by calling "drm_bridge_add_for_lookup". >> >> >> >> The parent encoder driver waits till the bridge is available in the >> >> lookup table(by calling "of_drm_find_bridge") and then continues with >> >> its initialization. >> >> >> >> The encoder driver should call "drm_bridge_attach_encoder" to pass on >> >> the drm_device and the encoder pointers to the bridge object. >> >> >> >> Now that the drm_device pointer is available, the encoder then calls >> >> "bridge->funcs->post_encoder_init" to allow the bridge to continue >> >> registering itself with the drm core. >> >> >> >> Also, non driver model based ptn3460 driver is removed in this patch. >> > >> > Why is it removed in this patch? Can't you do this incrementally rather >> > than remove the driver in this patch and add it again later? If you do >> > it this way then we'll always have this one commit where devices that >> > have a ptn3460 don't work, so it becomes impossible to bisect across >> > this commit. >> Ok. I will try to modify ptn3460 to support driver model in this patch itself. >> And then, adding panel support, converting it to gpiod interface and other >> cleanups should follow. > > I think it should even be possible to do this in more separate steps. > For example you could add the new bridge infrastructure without touching > any of the existing drivers (so that they are completely unaffected by > the changes) and then start converting one by one. > > For some of the changes this may be difficult (such as the dev -> > drm_dev rename to make room for the new struct device *dev). But that > could for example be done in a preparatory patch that first renames the > field, so that the "infrastructure" patch can add the new field without > renaming any fields and therefore needing changes to drivers directly. > > The goal of that whole exercise is to allow display drivers to keep > working with the existing API (ptn3460_init()) while we convert the > bridge drivers to register with the new framework. Then we can more > safely convert each display driver individually to make use of the new > framework and once all drivers have been converted the old API can > simply be removed. > > That way there should be no impact on existing functionality at any > point. As of now only exynos_dp uses ptn3460_init. And, also only 2 drivers use drm_bridge_init. It should be really easy to bisect if something goes wrong. Still, I will try to divide it so that each patch contains minimal change. >> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> > [...] >> > >> > Maybe since you're introducing a new drm_bridge.c file above already it >> > would make sense to move out existing drm_bridge related code in a >> > preparatory patch? >> > >> > Maybe Sean or Rob can comment on whether there was a specific reason to >> > include it in drm_crtc.c in the first place. >> > >> >> @@ -1012,8 +1010,7 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, >> >> if (ret) >> >> goto out; >> >> >> >> - bridge->dev = dev; >> >> - bridge->funcs = funcs; >> >> + bridge->drm_dev = dev; >> > >> > This sets ->drm_dev, but it was already set in drm_bridge_attach(), so I >> > think that's one more argument to call this function when attaching. >> Point accepted. > > I forgot to mention earlier. drm_dev seems redundant to me. I'd go with > just "drm". Ok. >> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >> >> index e529b68..e5a41ad 100644 >> >> --- a/include/drm/drm_crtc.h >> >> +++ b/include/drm/drm_crtc.h >> >> @@ -619,6 +619,7 @@ struct drm_plane { >> >> >> >> /** >> >> * drm_bridge_funcs - drm_bridge control functions >> >> + * @post_encoder_init: called by the parent encoder >> > >> > Maybe rename this to "attach" to make it more obvious when exactly it's >> > called? >> "post_encoder_attach"? > > "post_encoder_" doesn't contain much information, or even ambiguous > information. What does "post" "encoder" mean? A bridge is always > attached to an encoder, so "encoder" can be dropped. Now "post" has > implications as to the time when it is called, but does it mean after > the encoder has been initialized, or after the encoder has been removed? > Simply "attach" means it's called by the parent encoder to initialize > the bridge once it's been attached to an encoder. So obviously it's > after the encoder has been initialized. "attach" has all he information > required. Any prefix is redundant in my opinion and removing prefixes > gives shorter names and reduces the number of keypresses. Finally, what name it should have? >> >> * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge >> >> * @disable: Called right before encoder prepare, disables the bridge >> >> * @post_disable: Called right after encoder prepare, for lockstepped disable >> >> @@ -628,6 +629,7 @@ struct drm_plane { >> >> * @destroy: make object go away >> >> */ >> >> struct drm_bridge_funcs { >> >> + int (*post_encoder_init)(struct drm_bridge *bridge); >> >> bool (*mode_fixup)(struct drm_bridge *bridge, >> >> const struct drm_display_mode *mode, >> >> struct drm_display_mode *adjusted_mode); >> >> @@ -648,15 +650,19 @@ struct drm_bridge_funcs { >> >> * @base: base mode object >> >> * @funcs: control functions >> >> * @driver_private: pointer to the bridge driver's internal context >> >> + * @connector_polled: polled flag needed for registering connector >> > >> > Can you explain why this new field is needed? It seems like a completely >> > unrelated change. >> How do I select this flag for the bridge chip? >> Assume if I did select DRM_CONNECTOR_POLL_HPD, where to call >> drm_helper_hpd_irq_event in the driver? Is post_encoder_init a right place? >> >> Without the polled flag, I get display very late. >> Please throw some light on this! > > I just don't understand why it's necessary to implement this field in > the drm_bridge. Every bridge driver will already implement a connector, > in which case it can simply set the connector's .polled field, can't it? > > It seems like the only reason you have it in drm_bridge is so that the > encoder driver can set it. But I don't see why it should be doing that. > The polled state is a property of the connector, and the encoder driver > doesn't know anything about it. So if the bridge has a way to detect HPD > then it should be setting up the connector to properly report it. For > example if the bridge has an input pin to detect it, then it could use a > GPIO to receive interrupts and call drm_helper_hpd_irq_event() in the > interrupt handler. Hmm. Are we allowed to call drm_helper_hpd_irq_event() the way DSI panels use it? Like the last step in panel probe? For bridges, it will be in post_encoder_init! > Perhaps you can explain the exact setup where you need this (or point me > at the code since I can't seem to find the relevant location) so that I > can gain a better understanding. I can see bridge getting detected only when I set polled member of bridge connector to DRM_CONNECTOR_POLL_HPD, because exynos_drm also calls drm_helper_hpd_irq_event() to force detect all connectors at the end of drm_load. If I don't set the polled member, I see bridge getting detected after quite sometime. Ajay
On Wed, Jul 30, 2014 at 09:33:28PM +0530, Ajay kumar wrote: > On Wed, Jul 30, 2014 at 8:38 PM, Thierry Reding <thierry.reding@gmail.com> wrote: [...] > > I think it should even be possible to do this in more separate steps. > > For example you could add the new bridge infrastructure without touching > > any of the existing drivers (so that they are completely unaffected by > > the changes) and then start converting one by one. > > > > For some of the changes this may be difficult (such as the dev -> > > drm_dev rename to make room for the new struct device *dev). But that > > could for example be done in a preparatory patch that first renames the > > field, so that the "infrastructure" patch can add the new field without > > renaming any fields and therefore needing changes to drivers directly. > > > > The goal of that whole exercise is to allow display drivers to keep > > working with the existing API (ptn3460_init()) while we convert the > > bridge drivers to register with the new framework. Then we can more > > safely convert each display driver individually to make use of the new > > framework and once all drivers have been converted the old API can > > simply be removed. > > > > That way there should be no impact on existing functionality at any > > point. > As of now only exynos_dp uses ptn3460_init. > And, also only 2 drivers use drm_bridge_init. > It should be really easy to bisect if something goes wrong. > Still, I will try to divide it so that each patch contains minimal change. Thanks. > >> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > >> >> index e529b68..e5a41ad 100644 > >> >> --- a/include/drm/drm_crtc.h > >> >> +++ b/include/drm/drm_crtc.h > >> >> @@ -619,6 +619,7 @@ struct drm_plane { > >> >> > >> >> /** > >> >> * drm_bridge_funcs - drm_bridge control functions > >> >> + * @post_encoder_init: called by the parent encoder > >> > > >> > Maybe rename this to "attach" to make it more obvious when exactly it's > >> > called? > >> "post_encoder_attach"? > > > > "post_encoder_" doesn't contain much information, or even ambiguous > > information. What does "post" "encoder" mean? A bridge is always > > attached to an encoder, so "encoder" can be dropped. Now "post" has > > implications as to the time when it is called, but does it mean after > > the encoder has been initialized, or after the encoder has been removed? > > Simply "attach" means it's called by the parent encoder to initialize > > the bridge once it's been attached to an encoder. So obviously it's > > after the encoder has been initialized. "attach" has all he information > > required. Any prefix is redundant in my opinion and removing prefixes > > gives shorter names and reduces the number of keypresses. > Finally, what name it should have? I originally proposed "attach" as a more concise name and I still think that's the best alternative. > >> >> * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge > >> >> * @disable: Called right before encoder prepare, disables the bridge > >> >> * @post_disable: Called right after encoder prepare, for lockstepped disable > >> >> @@ -628,6 +629,7 @@ struct drm_plane { > >> >> * @destroy: make object go away > >> >> */ > >> >> struct drm_bridge_funcs { > >> >> + int (*post_encoder_init)(struct drm_bridge *bridge); > >> >> bool (*mode_fixup)(struct drm_bridge *bridge, > >> >> const struct drm_display_mode *mode, > >> >> struct drm_display_mode *adjusted_mode); > >> >> @@ -648,15 +650,19 @@ struct drm_bridge_funcs { > >> >> * @base: base mode object > >> >> * @funcs: control functions > >> >> * @driver_private: pointer to the bridge driver's internal context > >> >> + * @connector_polled: polled flag needed for registering connector > >> > > >> > Can you explain why this new field is needed? It seems like a completely > >> > unrelated change. > >> How do I select this flag for the bridge chip? > >> Assume if I did select DRM_CONNECTOR_POLL_HPD, where to call > >> drm_helper_hpd_irq_event in the driver? Is post_encoder_init a right place? > >> > >> Without the polled flag, I get display very late. > >> Please throw some light on this! > > > > I just don't understand why it's necessary to implement this field in > > the drm_bridge. Every bridge driver will already implement a connector, > > in which case it can simply set the connector's .polled field, can't it? > > > > It seems like the only reason you have it in drm_bridge is so that the > > encoder driver can set it. But I don't see why it should be doing that. > > The polled state is a property of the connector, and the encoder driver > > doesn't know anything about it. So if the bridge has a way to detect HPD > > then it should be setting up the connector to properly report it. For > > example if the bridge has an input pin to detect it, then it could use a > > GPIO to receive interrupts and call drm_helper_hpd_irq_event() in the > > interrupt handler. > Hmm. Are we allowed to call drm_helper_hpd_irq_event() the way > DSI panels use it? Like the last step in panel probe? > For bridges, it will be in post_encoder_init! drm_helper_hpd_irq_event() should only be called when a hotplug event is detected. For all other cases detection should already happen when DRM initializes. I see that on Tegra we call drm_helper_hpd_irq_event() in the DSI host's ->attach(), but I don't remember why that's there and I don't see why it would be necessary either. I'll try to remove it and see if things still work without. > > Perhaps you can explain the exact setup where you need this (or point me > > at the code since I can't seem to find the relevant location) so that I > > can gain a better understanding. > > I can see bridge getting detected only when I set polled member of > bridge connector to DRM_CONNECTOR_POLL_HPD, because exynos_drm > also calls drm_helper_hpd_irq_event() to force detect all connectors at the > end of drm_load. That shouldn't be necessary. DRM automatically force detects all outputs (at least if you use drm_helper_probe_single_connector_modes(), which seems to be the case for Exynos). Thierry
Hello Ajay, On Thu, Jul 31, 2014 at 12:58 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Jul 30, 2014 at 09:33:28PM +0530, Ajay kumar wrote: >> On Wed, Jul 30, 2014 at 8:38 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > [...] >> > I think it should even be possible to do this in more separate steps. >> > For example you could add the new bridge infrastructure without touching >> > any of the existing drivers (so that they are completely unaffected by >> > the changes) and then start converting one by one. >> > >> > For some of the changes this may be difficult (such as the dev -> >> > drm_dev rename to make room for the new struct device *dev). But that >> > could for example be done in a preparatory patch that first renames the >> > field, so that the "infrastructure" patch can add the new field without >> > renaming any fields and therefore needing changes to drivers directly. >> > >> > The goal of that whole exercise is to allow display drivers to keep >> > working with the existing API (ptn3460_init()) while we convert the >> > bridge drivers to register with the new framework. Then we can more >> > safely convert each display driver individually to make use of the new >> > framework and once all drivers have been converted the old API can >> > simply be removed. >> > >> > That way there should be no impact on existing functionality at any >> > point. >> As of now only exynos_dp uses ptn3460_init. >> And, also only 2 drivers use drm_bridge_init. >> It should be really easy to bisect if something goes wrong. >> Still, I will try to divide it so that each patch contains minimal change. > > Thanks. > Do you plan to address Thierry's concerns and re-spin this patch? Same question for patches: "drm/bridge: Add i2c based driver for ptn3460 bridge" "drm/bridge: Add i2c based driver for ps8622/ps8625 bridge" Thanks a lot and best regards, Javier
Hi Javier, On Sat, Aug 23, 2014 at 5:03 AM, Javier Martinez Canillas <javier@dowhile0.org> wrote: > Hello Ajay, > > On Thu, Jul 31, 2014 at 12:58 PM, Thierry Reding > <thierry.reding@gmail.com> wrote: >> On Wed, Jul 30, 2014 at 09:33:28PM +0530, Ajay kumar wrote: >>> On Wed, Jul 30, 2014 at 8:38 PM, Thierry Reding <thierry.reding@gmail.com> wrote: >> [...] >>> > I think it should even be possible to do this in more separate steps. >>> > For example you could add the new bridge infrastructure without touching >>> > any of the existing drivers (so that they are completely unaffected by >>> > the changes) and then start converting one by one. >>> > >>> > For some of the changes this may be difficult (such as the dev -> >>> > drm_dev rename to make room for the new struct device *dev). But that >>> > could for example be done in a preparatory patch that first renames the >>> > field, so that the "infrastructure" patch can add the new field without >>> > renaming any fields and therefore needing changes to drivers directly. >>> > >>> > The goal of that whole exercise is to allow display drivers to keep >>> > working with the existing API (ptn3460_init()) while we convert the >>> > bridge drivers to register with the new framework. Then we can more >>> > safely convert each display driver individually to make use of the new >>> > framework and once all drivers have been converted the old API can >>> > simply be removed. >>> > >>> > That way there should be no impact on existing functionality at any >>> > point. >>> As of now only exynos_dp uses ptn3460_init. >>> And, also only 2 drivers use drm_bridge_init. >>> It should be really easy to bisect if something goes wrong. >>> Still, I will try to divide it so that each patch contains minimal change. >> >> Thanks. >> > > Do you plan to address Thierry's concerns and re-spin this patch? > > Same question for patches: > > "drm/bridge: Add i2c based driver for ptn3460 bridge" > "drm/bridge: Add i2c based driver for ps8622/ps8625 bridge" Yes, I will. I was caught up with some other work. I will be sending a version ASAP. Ajay
Hello Ajay, On Mon, Aug 25, 2014 at 8:11 AM, Ajay kumar <ajaynumb@gmail.com> wrote: >> >> Do you plan to address Thierry's concerns and re-spin this patch? >> >> Same question for patches: >> >> "drm/bridge: Add i2c based driver for ptn3460 bridge" >> "drm/bridge: Add i2c based driver for ps8622/ps8625 bridge" > Yes, I will. I was caught up with some other work. > I will be sending a version ASAP. > Great, glad to know that you will be doing a re-spin! > Ajay Thanks a lot and best regards, Javier
Hi Ajay, Thank you for the patch. I think we're moving in the right direction, but we're not there yet. On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: > This patch tries to seperate drm_bridge implementation > into 2 parts, a drm part and a non_drm part. > > A set of helper functions are defined in this patch to make > bridge driver probe independent of the drm flow. > > The bridge devices register themselves on a lookup table > when they get probed by calling "drm_bridge_add_for_lookup". > > The parent encoder driver waits till the bridge is available in the > lookup table(by calling "of_drm_find_bridge") and then continues with > its initialization. Before the introduction of the component framework I would have said this is the way to go. Now, I think bridges should register themselves as components, and the DRM master driver should use the component framework to get a reference to the bridges it needs. > The encoder driver should call "drm_bridge_attach_encoder" to pass on > the drm_device and the encoder pointers to the bridge object. > > Now that the drm_device pointer is available, the encoder then calls > "bridge->funcs->post_encoder_init" to allow the bridge to continue > registering itself with the drm core. This is what really bothers me with DRM bridge. The framework assumes that a bridge will always bridge an encoder and a connector. Beside lacking support for chained bridges, this creates an artificial split between bridges and encoders by modeling the same components using drm_encoder or drm_bridge depending on their position in the video output pipeline. I would like to see drm_bridge becoming more self-centric, removing the awareness of the upstream encoder and downstream connector. I'll give this a try, but it will conflict with this patch, so I'd like to share opinions and coordinate efforts sooner than later if possible. > Also, non driver model based ptn3460 driver is removed in this patch. > > Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> > --- > .../devicetree/bindings/drm/bridge/ptn3460.txt | 27 -- > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/bridge/Kconfig | 12 +- > drivers/gpu/drm/bridge/Makefile | 2 - > drivers/gpu/drm/bridge/ptn3460.c | 343 ----------------- > drivers/gpu/drm/drm_bridge.c | 89 +++++ > drivers/gpu/drm/drm_crtc.c | 9 +- > drivers/gpu/drm/exynos/Kconfig | 3 +- > drivers/gpu/drm/exynos/exynos_dp_core.c | 57 ++-- > drivers/gpu/drm/exynos/exynos_dp_core.h | 1 + > drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 3 +- > include/drm/bridge/ptn3460.h | 37 --- > include/drm/drm_crtc.h | 16 +- > 13 files changed, 147 insertions(+), 453 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/drm/bridge/ptn3460.txt > delete mode 100644 drivers/gpu/drm/bridge/ptn3460.c > create mode 100644 drivers/gpu/drm/drm_bridge.c > delete mode 100644 include/drm/bridge/ptn3460.h
Hi Laurent, On Mon, Sep 15, 2014 at 11:07 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Ajay, > > Thank you for the patch. > > I think we're moving in the right direction, but we're not there yet. > > On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: >> This patch tries to seperate drm_bridge implementation >> into 2 parts, a drm part and a non_drm part. >> >> A set of helper functions are defined in this patch to make >> bridge driver probe independent of the drm flow. >> >> The bridge devices register themselves on a lookup table >> when they get probed by calling "drm_bridge_add_for_lookup". >> >> The parent encoder driver waits till the bridge is available in the >> lookup table(by calling "of_drm_find_bridge") and then continues with >> its initialization. > > Before the introduction of the component framework I would have said this is > the way to go. Now, I think bridges should register themselves as components, > and the DRM master driver should use the component framework to get a > reference to the bridges it needs. Well, I have modified the bridge framework exactly the way Thierry wanted it to be, I mean the same way the current panel framework is. And, I don't think there is a problem with that. What problem are you facing with current bridge implementation? What is the advantage of using the component framework to register bridges? >> The encoder driver should call "drm_bridge_attach_encoder" to pass on >> the drm_device and the encoder pointers to the bridge object. >> >> Now that the drm_device pointer is available, the encoder then calls >> "bridge->funcs->post_encoder_init" to allow the bridge to continue >> registering itself with the drm core. > > This is what really bothers me with DRM bridge. > > The framework assumes that a bridge will always bridge an encoder and a > connector. Beside lacking support for chained bridges, this creates an > artificial split between bridges and encoders by modeling the same components > using drm_encoder or drm_bridge depending on their position in the video > output pipeline. > > I would like to see drm_bridge becoming more self-centric, removing the > awareness of the upstream encoder and downstream connector. I'll give this a > try, but it will conflict with this patch, so I'd like to share opinions and > coordinate efforts sooner than later if possible. I am not really able to understand how you want "drm_bridge" to be. As of now, there are many platforms using drm_bridge and they don't have a problem with current implementation. Regarding chained bridges: Can't you add this once my patchset is merged? As an additional feature? To be honest, I have spent quite sometime for working on this patchset. All I started with was to add drm_panel support to drm_bridge. When I sent the first patchset for that, Daniel, Rob and Thierry raised a concern that current bridge framework itself is not proper and hence they asked me to fix that first. And we have reached till here based on their comments only. Without this patchset, you cannot bring an X server based display on snow and peach_pit. Also, day by day the number of platforms using drm_bridge is increasing. And, I don't really see a problem with the current approach(which is exactly the same way panel framework is). And, I am no decision maker here. I would expect the top guys to comment! Ajay >> Also, non driver model based ptn3460 driver is removed in this patch. >> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> >> --- >> .../devicetree/bindings/drm/bridge/ptn3460.txt | 27 -- >> drivers/gpu/drm/Makefile | 1 + >> drivers/gpu/drm/bridge/Kconfig | 12 +- >> drivers/gpu/drm/bridge/Makefile | 2 - >> drivers/gpu/drm/bridge/ptn3460.c | 343 ----------------- >> drivers/gpu/drm/drm_bridge.c | 89 +++++ >> drivers/gpu/drm/drm_crtc.c | 9 +- >> drivers/gpu/drm/exynos/Kconfig | 3 +- >> drivers/gpu/drm/exynos/exynos_dp_core.c | 57 ++-- >> drivers/gpu/drm/exynos/exynos_dp_core.h | 1 + >> drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 3 +- >> include/drm/bridge/ptn3460.h | 37 --- >> include/drm/drm_crtc.h | 16 +- >> 13 files changed, 147 insertions(+), 453 deletions(-) >> delete mode 100644 Documentation/devicetree/bindings/drm/bridge/ptn3460.txt >> delete mode 100644 drivers/gpu/drm/bridge/ptn3460.c >> create mode 100644 drivers/gpu/drm/drm_bridge.c >> delete mode 100644 include/drm/bridge/ptn3460.h > > -- > Regards, > > Laurent Pinchart >
>> >> Before the introduction of the component framework I would have said this is >> the way to go. Now, I think bridges should register themselves as components, >> and the DRM master driver should use the component framework to get a >> reference to the bridges it needs. > Well, I have modified the bridge framework exactly the way Thierry wanted it > to be, I mean the same way the current panel framework is. > And, I don't think there is a problem with that. > What problem are you facing with current bridge implementation? > What is the advantage of using the component framework to register bridges? At this point I'd rather merge something that keep iterating out of tree, if this enables current hw to work and is better than what we have we should merge it, and if we can get interest in using the component framework then we should look at that as a separate problem. Dave.
Hi Ajay, On Wednesday 17 September 2014 14:37:30 Ajay kumar wrote: > On Mon, Sep 15, 2014 at 11:07 PM, Laurent Pinchart wrote: > > Hi Ajay, > > > > Thank you for the patch. > > > > I think we're moving in the right direction, but we're not there yet. > > > > On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: > >> This patch tries to seperate drm_bridge implementation > >> into 2 parts, a drm part and a non_drm part. > >> > >> A set of helper functions are defined in this patch to make > >> bridge driver probe independent of the drm flow. > >> > >> The bridge devices register themselves on a lookup table > >> when they get probed by calling "drm_bridge_add_for_lookup". > >> > >> The parent encoder driver waits till the bridge is available in the > >> lookup table(by calling "of_drm_find_bridge") and then continues with > >> its initialization. > > > > Before the introduction of the component framework I would have said this > > is the way to go. Now, I think bridges should register themselves as > > components, and the DRM master driver should use the component framework > > to get a reference to the bridges it needs. > > Well, I have modified the bridge framework exactly the way Thierry wanted it > to be, I mean the same way the current panel framework is. > And, I don't think there is a problem with that. > What problem are you facing with current bridge implementation? > What is the advantage of using the component framework to register bridges? There are several advantages. - The component framework has been designed with this exact problem in mind, piecing multiple components into a display device. This patch set introduces yet another framework, without any compelling reason as far as I can see. Today DRM drivers already need to use three different frameworks (component, I2C slave encoder and panel), and we're adding a fourth one to make the mess even messier. This is really a headlong rush, we need to stop and fix the design mistakes. - The component framework solves the probe ordering problem. Bridges can use deferred probing, but when a bridge requires a resources (such as a clock for instance) provided by the display controller, this will break. > >> The encoder driver should call "drm_bridge_attach_encoder" to pass on > >> the drm_device and the encoder pointers to the bridge object. > >> > >> Now that the drm_device pointer is available, the encoder then calls > >> "bridge->funcs->post_encoder_init" to allow the bridge to continue > >> registering itself with the drm core. > > > > This is what really bothers me with DRM bridge. > > > > The framework assumes that a bridge will always bridge an encoder and a > > connector. Beside lacking support for chained bridges, this creates an > > artificial split between bridges and encoders by modeling the same > > components using drm_encoder or drm_bridge depending on their position in > > the video output pipeline. > > > > I would like to see drm_bridge becoming more self-centric, removing the > > awareness of the upstream encoder and downstream connector. I'll give this > > a try, but it will conflict with this patch, so I'd like to share > > opinions and coordinate efforts sooner than later if possible. > > I am not really able to understand how you want "drm_bridge" to be. > As of now, there are many platforms using drm_bridge and they don't > have a problem with current implementation. > Regarding chained bridges: Can't you add this once my patchset is merged? > As an additional feature? Yes, as I mentioned in another e-mail this can be fixed later. I want to start discussing it though. > To be honest, I have spent quite sometime for working on this patchset. > All I started with was to add drm_panel support to drm_bridge. > When I sent the first patchset for that, Daniel, Rob and Thierry raised a > concern that current bridge framework itself is not proper and hence > they asked me to fix that first. And we have reached till here based on > their comments only. > > Without this patchset, you cannot bring an X server > based display on snow and peach_pit. Also, day by day the number of > platforms using drm_bridge is increasing. That's exactly why I'd like to use the component framework now, as the conversion will become more complex as time goes by. > And, I don't really see a problem with the current approach(which is exactly > the same way panel framework is). And, I am no decision maker here. I would > expect the top guys to comment!
On Wed, Sep 17, 2014 at 2:57 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Ajay, > > On Wednesday 17 September 2014 14:37:30 Ajay kumar wrote: >> On Mon, Sep 15, 2014 at 11:07 PM, Laurent Pinchart wrote: >> > Hi Ajay, >> > >> > Thank you for the patch. >> > >> > I think we're moving in the right direction, but we're not there yet. >> > >> > On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: >> >> This patch tries to seperate drm_bridge implementation >> >> into 2 parts, a drm part and a non_drm part. >> >> >> >> A set of helper functions are defined in this patch to make >> >> bridge driver probe independent of the drm flow. >> >> >> >> The bridge devices register themselves on a lookup table >> >> when they get probed by calling "drm_bridge_add_for_lookup". >> >> >> >> The parent encoder driver waits till the bridge is available in the >> >> lookup table(by calling "of_drm_find_bridge") and then continues with >> >> its initialization. >> > >> > Before the introduction of the component framework I would have said this >> > is the way to go. Now, I think bridges should register themselves as >> > components, and the DRM master driver should use the component framework >> > to get a reference to the bridges it needs. >> >> Well, I have modified the bridge framework exactly the way Thierry wanted it >> to be, I mean the same way the current panel framework is. >> And, I don't think there is a problem with that. >> What problem are you facing with current bridge implementation? >> What is the advantage of using the component framework to register bridges? > > There are several advantages. > > - The component framework has been designed with this exact problem in mind, > piecing multiple components into a display device. This patch set introduces > yet another framework, without any compelling reason as far as I can see. Without this bridge registration framework, there is no way you can pass on a drm_device pointer to the bridge driver. That is why we added a lookup framework. > Today DRM drivers already need to use three different frameworks (component, > I2C slave encoder and panel), and we're adding a fourth one to make the mess > even messier. This is really a headlong rush, we need to stop and fix the > design mistakes. > > - The component framework solves the probe ordering problem. Bridges can use > deferred probing, but when a bridge requires a resources (such as a clock for > instance) provided by the display controller, this will break. This is exactly the way sti drm uses bridge I think. It uses component framework to wait till the master device initializes and then passes on a drm_device pointer to the component devices. But please know that it is feasible in case of sti, because the bridge they use must be a embedded chip on the SOC, but not a third party device. And, the case which you mentioned(clock instance need to be passed to bridge driver) happens only in the case of bridges embedded on the SOC, but not a third party device. So, you are always allowed to use component framework for that. But, assume the bridge chip is a third party device(ex: ptn3460 or ps8622) which sits on an i2c bus. In that case, your approach poses the foll problems: The way master and components are binded varies from platform to platform. i.e the way exynos consolidates and adds the components is very much different the way msm/sti/armada does the same! So, when one needs to use a third party device as a bridge, they will end up hacking up their drm layer to support this third party device. With my approach, only the corresponding encoder driver needs to know about the bridge, that too just the phandle to bridge node. The platform specific drm layer can still be unaware of the bridge and stuff. With your approach, the way we would specify the bridge node will change from platform to platform resulting in non-uniformity. Also, the platform specific drm layer needs to be aware of the bridge. And, I assume these are the reasons why drm_panel doesn't use component framework. Because all the panels are often third party and hence can be reused across platforms, and so are ptn3460/ps8622. >> >> The encoder driver should call "drm_bridge_attach_encoder" to pass on >> >> the drm_device and the encoder pointers to the bridge object. >> >> >> >> Now that the drm_device pointer is available, the encoder then calls >> >> "bridge->funcs->post_encoder_init" to allow the bridge to continue >> >> registering itself with the drm core. >> > >> > This is what really bothers me with DRM bridge. >> > >> > The framework assumes that a bridge will always bridge an encoder and a >> > connector. Beside lacking support for chained bridges, this creates an >> > artificial split between bridges and encoders by modeling the same >> > components using drm_encoder or drm_bridge depending on their position in >> > the video output pipeline. >> > >> > I would like to see drm_bridge becoming more self-centric, removing the >> > awareness of the upstream encoder and downstream connector. I'll give this >> > a try, but it will conflict with this patch, so I'd like to share >> > opinions and coordinate efforts sooner than later if possible. >> >> I am not really able to understand how you want "drm_bridge" to be. >> As of now, there are many platforms using drm_bridge and they don't >> have a problem with current implementation. >> Regarding chained bridges: Can't you add this once my patchset is merged? >> As an additional feature? > > Yes, as I mentioned in another e-mail this can be fixed later. I want to start > discussing it though. >> To be honest, I have spent quite sometime for working on this patchset. >> All I started with was to add drm_panel support to drm_bridge. >> When I sent the first patchset for that, Daniel, Rob and Thierry raised a >> concern that current bridge framework itself is not proper and hence >> they asked me to fix that first. And we have reached till here based on >> their comments only. >> >> Without this patchset, you cannot bring an X server >> based display on snow and peach_pit. Also, day by day the number of >> platforms using drm_bridge is increasing. > > That's exactly why I'd like to use the component framework now, as the > conversion will become more complex as time goes by. As I have explained above, using component framework for third party bridges is a bad idea. And all other private bridges, already use the component framework(ex: sti). So, ideally this point should not be a blocker for this patchset to get in. :) Ajay
On Wed, Sep 17, 2014 at 12:27:13PM +0300, Laurent Pinchart wrote: > Hi Ajay, > > On Wednesday 17 September 2014 14:37:30 Ajay kumar wrote: > > On Mon, Sep 15, 2014 at 11:07 PM, Laurent Pinchart wrote: > > > Hi Ajay, > > > > > > Thank you for the patch. > > > > > > I think we're moving in the right direction, but we're not there yet. > > > > > > On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: > > >> This patch tries to seperate drm_bridge implementation > > >> into 2 parts, a drm part and a non_drm part. > > >> > > >> A set of helper functions are defined in this patch to make > > >> bridge driver probe independent of the drm flow. > > >> > > >> The bridge devices register themselves on a lookup table > > >> when they get probed by calling "drm_bridge_add_for_lookup". > > >> > > >> The parent encoder driver waits till the bridge is available in the > > >> lookup table(by calling "of_drm_find_bridge") and then continues with > > >> its initialization. > > > > > > Before the introduction of the component framework I would have said this > > > is the way to go. Now, I think bridges should register themselves as > > > components, and the DRM master driver should use the component framework > > > to get a reference to the bridges it needs. > > > > Well, I have modified the bridge framework exactly the way Thierry wanted it > > to be, I mean the same way the current panel framework is. > > And, I don't think there is a problem with that. > > What problem are you facing with current bridge implementation? > > What is the advantage of using the component framework to register bridges? > > There are several advantages. > > - The component framework has been designed with this exact problem in mind, > piecing multiple components into a display device. No. Component framework was designed with multi-device drivers in mind. That is, drivers that need to combine two or more platform devices into a single logical device. Typically that includes display controllers and encoders (in various looks) for DRM. Panels and bridges are in my opinion different because they are outside of the DRM driver. They aren't part of the device complex that an SoC provides. They represent hardware that is external to the SoC and the DRM driver and can be shared across SoCs. Forcing panels and bridges to register as components will require all drivers to implement master/component support solely for accessing this external hardware. What you're suggesting is like saying that clocks or regulators should register as components so that their users can get them that way. In fact by that argument everything that's referenced by phandle would need to register as component (PHYs, LEDs, GPIOs, I2C controllers, ...). > This patch set introduces > yet another framework, without any compelling reason as far as I can see. > Today DRM drivers already need to use three different frameworks (component, > I2C slave encoder and panel), and we're adding a fourth one to make the mess > even messier. Panel and bridge aren't really frameworks. Rather they are a simple registry to allow drivers to register panels and bridges and display drivers to look them up. > This is really a headlong rush, we need to stop and fix the design mistakes. Can you point out specific design mistakes? I don't see any, but I'm obviously biased. > - The component framework solves the probe ordering problem. Bridges can use > deferred probing, but when a bridge requires a resources (such as a clock for > instance) provided by the display controller, this will break. Panel and bridges can support deferred probing without the component framework just fine. > > Without this patchset, you cannot bring an X server > > based display on snow and peach_pit. Also, day by day the number of > > platforms using drm_bridge is increasing. > > That's exactly why I'd like to use the component framework now, as the > conversion will become more complex as time goes by. No it won't. If we ever do decide that component framework is a better fit then the conversion may be more work but it would still be largely mechanical. Thierry
Hi Thierry, On Monday 22 September 2014 09:40:38 Thierry Reding wrote: > On Wed, Sep 17, 2014 at 12:27:13PM +0300, Laurent Pinchart wrote: > > On Wednesday 17 September 2014 14:37:30 Ajay kumar wrote: > > > On Mon, Sep 15, 2014 at 11:07 PM, Laurent Pinchart wrote: > > > > Hi Ajay, > > > > > > > > Thank you for the patch. > > > > > > > > I think we're moving in the right direction, but we're not there yet. > > > > > > > > On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: > > > >> This patch tries to seperate drm_bridge implementation > > > >> into 2 parts, a drm part and a non_drm part. > > > >> > > > >> A set of helper functions are defined in this patch to make > > > >> bridge driver probe independent of the drm flow. > > > >> > > > >> The bridge devices register themselves on a lookup table > > > >> when they get probed by calling "drm_bridge_add_for_lookup". > > > >> > > > >> The parent encoder driver waits till the bridge is available in the > > > >> lookup table(by calling "of_drm_find_bridge") and then continues with > > > >> its initialization. > > > > > > > > Before the introduction of the component framework I would have said > > > > this is the way to go. Now, I think bridges should register themselves > > > > as components, and the DRM master driver should use the component > > > > framework to get a reference to the bridges it needs. > > > > > > Well, I have modified the bridge framework exactly the way Thierry > > > wanted it to be, I mean the same way the current panel framework is. > > > And, I don't think there is a problem with that. > > > What problem are you facing with current bridge implementation? > > > What is the advantage of using the component framework to register > > > bridges? > > > > There are several advantages. > > > > - The component framework has been designed with this exact problem in > > mind, piecing multiple components into a display device. > > No. Component framework was designed with multi-device drivers in mind. > That is, drivers that need to combine two or more platform devices into > a single logical device. Typically that includes display controllers and > encoders (in various looks) for DRM. I disagree. AFAIK the component framework was designed to easily combine multiple devices into a single logical device, regardless of which bus each device is connected to. That's what makes the component framework useful : it allows master drivers to build logical devices from heterogeneous components without having to use one API per bus and/or component type. If the only goal had been to combine platform devices on an SoC, simpler device-specific solutions would likely have been used instead. > Panels and bridges are in my opinion different because they are outside > of the DRM driver. They aren't part of the device complex that an SoC > provides. They represent hardware that is external to the SoC and the > DRM driver and can be shared across SoCs. They represent hardware external to the SoC, but internal to the logical DRM device. > Forcing panels and bridges to register as components will require all > drivers to implement master/component support solely for accessing this > external hardware. > > What you're suggesting is like saying that clocks or regulators should > register as components so that their users can get them that way. In > fact by that argument everything that's referenced by phandle would need > to register as component (PHYs, LEDs, GPIOs, I2C controllers, ...). No, that's very different. The device you list are clearly external resources, while the bridges and panels are components part of a logical display device. > > This patch set introduces yet another framework, without any compelling > > reason as far as I can see. Today DRM drivers already need to use three > > different frameworks (component, I2C slave encoder and panel), and we're > > adding a fourth oneto make the mess even messier. > > Panel and bridge aren't really frameworks. Rather they are a simple > registry to allow drivers to register panels and bridges and display > drivers to look them up. Regardless of how you call them, we have three interfaces. > > This is really a headlong rush, we need to stop and fix the design > > mistakes. > > Can you point out specific design mistakes? I don't see any, but I'm > obviously biased. The slave encoder / bridge split is what I consider a design mistake. Those two interfaces serve the same purpose, they should really be merged. > > - The component framework solves the probe ordering problem. Bridges can > > use deferred probing, but when a bridge requires a resources (such as a > > clock for instance) provided by the display controller, this will break. > > Panel and bridges can support deferred probing without the component > framework just fine. Not if the bridge requires a clock provided by the display controller, in which case there's a dependency loop. > > > Without this patchset, you cannot bring an X server based display on > > > snow and peach_pit. Also, day by day the number of platforms using > > > drm_bridge is increasing. > > > > That's exactly why I'd like to use the component framework now, as the > > conversion will become more complex as time goes by. > > No it won't. If we ever do decide that component framework is a better > fit then the conversion may be more work but it would still be largely > mechanical. Are you volunteering to perform the conversion ? :-)
On Tue, Sep 23, 2014 at 03:29:13AM +0300, Laurent Pinchart wrote: > Hi Thierry, > > On Monday 22 September 2014 09:40:38 Thierry Reding wrote: > > On Wed, Sep 17, 2014 at 12:27:13PM +0300, Laurent Pinchart wrote: > > > On Wednesday 17 September 2014 14:37:30 Ajay kumar wrote: > > > > On Mon, Sep 15, 2014 at 11:07 PM, Laurent Pinchart wrote: > > > > > Hi Ajay, > > > > > > > > > > Thank you for the patch. > > > > > > > > > > I think we're moving in the right direction, but we're not there yet. > > > > > > > > > > On Saturday 26 July 2014 00:52:08 Ajay Kumar wrote: > > > > >> This patch tries to seperate drm_bridge implementation > > > > >> into 2 parts, a drm part and a non_drm part. > > > > >> > > > > >> A set of helper functions are defined in this patch to make > > > > >> bridge driver probe independent of the drm flow. > > > > >> > > > > >> The bridge devices register themselves on a lookup table > > > > >> when they get probed by calling "drm_bridge_add_for_lookup". > > > > >> > > > > >> The parent encoder driver waits till the bridge is available in the > > > > >> lookup table(by calling "of_drm_find_bridge") and then continues with > > > > >> its initialization. > > > > > > > > > > Before the introduction of the component framework I would have said > > > > > this is the way to go. Now, I think bridges should register themselves > > > > > as components, and the DRM master driver should use the component > > > > > framework to get a reference to the bridges it needs. > > > > > > > > Well, I have modified the bridge framework exactly the way Thierry > > > > wanted it to be, I mean the same way the current panel framework is. > > > > And, I don't think there is a problem with that. > > > > What problem are you facing with current bridge implementation? > > > > What is the advantage of using the component framework to register > > > > bridges? > > > > > > There are several advantages. > > > > > > - The component framework has been designed with this exact problem in > > > mind, piecing multiple components into a display device. > > > > No. Component framework was designed with multi-device drivers in mind. > > That is, drivers that need to combine two or more platform devices into > > a single logical device. Typically that includes display controllers and > > encoders (in various looks) for DRM. > > I disagree. AFAIK the component framework was designed to easily combine > multiple devices into a single logical device, regardless of which bus each > device is connected to. That's what makes the component framework useful : it > allows master drivers to build logical devices from heterogeneous components > without having to use one API per bus and/or component type. But this doesn't work really well once you leave the SoC. For component/ master to work you need to usually (i.e. using DT) have access to a device tree node for each of the devices so that you can create a list of needed devices. Once you leave the SoC, the number of combinations that you can have becomes non-deterministic. A driver that wants to pull this off would need to more or less manually look up phandles and traverse from SoC via bridges to panels to find all the devices that make up the logical device. > If the only goal > had been to combine platform devices on an SoC, simpler device-specific > solutions would likely have been used instead. I think one of the goals was to replace any of the simpler device- specific solutions with a generic one. But one of the issues with component/master is that it's viral. That is it requires users to register as master themselves in order to pull in the components. While that makes sense for on-SoC devices I think it's a mistake to use it for external hardware like bridges and panels that can be shared across SoCs. If we make component/master mandatory for bridges and panels, then we also force every driver that wants to use them to implement component/master support. Furthermore I did try a while back to convert the Tegra DRM driver to use component/master and couldn't make it work. When I proposed patches to enhance the API so that it would work for Tegra I got silence on one side and "just keep using what you currently have" on the other side. So in other words since I can't use component/master on Tegra it means that whatever driver gets added that registers a component can't be used on Tegra either. > > Panels and bridges are in my opinion different because they are outside > > of the DRM driver. They aren't part of the device complex that an SoC > > provides. They represent hardware that is external to the SoC and the > > DRM driver and can be shared across SoCs. > > They represent hardware external to the SoC, but internal to the logical DRM > device. That depends largely on how you look at it. From my point of view the logical DRM device stops at the connector (well I think it actually stops somewhat earlier already, with connector only being the handle through which the outputs are configured). Panels are, at least theoretically, hotpluggable. That is, if you have a device that has both a panel and HDMI but you never use HDMI, then you should be able to just unload the panel driver but keep the DRM device running for HDMI. If you use component/master that's impossible because as soon as the panel component goes away the complete DRM device goes away. > > Forcing panels and bridges to register as components will require all > > drivers to implement master/component support solely for accessing this > > external hardware. > > > > What you're suggesting is like saying that clocks or regulators should > > register as components so that their users can get them that way. In > > fact by that argument everything that's referenced by phandle would need > > to register as component (PHYs, LEDs, GPIOs, I2C controllers, ...). > > No, that's very different. The device you list are clearly external resources, > while the bridges and panels are components part of a logical display device. Like I said above, I consider bridges and panels external resources, too. I can easily imagine (actually I don't have to because it was recently discussed for a project) setups where some board defines a standard header that display modules can be connected to and hence whatever bridges or panels are attached can change at runtime. > > > This patch set introduces yet another framework, without any compelling > > > reason as far as I can see. Today DRM drivers already need to use three > > > different frameworks (component, I2C slave encoder and panel), and we're > > > adding a fourth oneto make the mess even messier. > > > > Panel and bridge aren't really frameworks. Rather they are a simple > > registry to allow drivers to register panels and bridges and display > > drivers to look them up. > > Regardless of how you call them, we have three interfaces. We need an interface to control the bridges and panels anyway. component and master only gives you a generic way to handle the registration and initialization. Adding a drm_*_lookup() function to the interface to retrieve a resource isn't as bloated as you make it out to be. Implementing component/master is much more complicated for (in this case) too little advantage. > > > This is really a headlong rush, we need to stop and fix the design > > > mistakes. > > > > Can you point out specific design mistakes? I don't see any, but I'm > > obviously biased. > > The slave encoder / bridge split is what I consider a design mistake. Those > two interfaces serve the same purpose, they should really be merged. Agreed. > > > - The component framework solves the probe ordering problem. Bridges can > > > use deferred probing, but when a bridge requires a resources (such as a > > > clock for instance) provided by the display controller, this will break. > > > > Panel and bridges can support deferred probing without the component > > framework just fine. > > Not if the bridge requires a clock provided by the display controller, in > which case there's a dependency loop. I don't see how component/master would solve that differently than the current proposal for DRM bridge (or the existing DRM panel for that matter). > > > > Without this patchset, you cannot bring an X server based display on > > > > snow and peach_pit. Also, day by day the number of platforms using > > > > drm_bridge is increasing. > > > > > > That's exactly why I'd like to use the component framework now, as the > > > conversion will become more complex as time goes by. > > > > No it won't. If we ever do decide that component framework is a better > > fit then the conversion may be more work but it would still be largely > > mechanical. > > Are you volunteering to perform the conversion ? :-) No. I'm still convinced that we won't need it. Less work for everyone. =) Thierry
diff --git a/Documentation/devicetree/bindings/drm/bridge/ptn3460.txt b/Documentation/devicetree/bindings/drm/bridge/ptn3460.txt deleted file mode 100644 index 52b93b2..0000000 --- a/Documentation/devicetree/bindings/drm/bridge/ptn3460.txt +++ /dev/null @@ -1,27 +0,0 @@ -ptn3460 bridge bindings - -Required properties: - - compatible: "nxp,ptn3460" - - reg: i2c address of the bridge - - powerdown-gpio: OF device-tree gpio specification - - reset-gpio: OF device-tree gpio specification - - edid-emulation: The EDID emulation entry to use - +-------+------------+------------------+ - | Value | Resolution | Description | - | 0 | 1024x768 | NXP Generic | - | 1 | 1920x1080 | NXP Generic | - | 2 | 1920x1080 | NXP Generic | - | 3 | 1600x900 | Samsung LTM200KT | - | 4 | 1920x1080 | Samsung LTM230HT | - | 5 | 1366x768 | NXP Generic | - | 6 | 1600x900 | ChiMei M215HGE | - +-------+------------+------------------+ - -Example: - lvds-bridge@20 { - compatible = "nxp,ptn3460"; - reg = <0x20>; - powerdown-gpio = <&gpy2 5 1 0 0>; - reset-gpio = <&gpx1 5 1 0 0>; - edid-emulation = <5>; - }; diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index af9a609..14a8b45 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -20,6 +20,7 @@ drm-$(CONFIG_COMPAT) += drm_ioc32.o drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o drm-$(CONFIG_PCI) += ati_pcigart.o drm-$(CONFIG_DRM_PANEL) += drm_panel.o +drm-$(CONFIG_DRM_BRIDGE) += drm_bridge.o drm-usb-y := drm_usb.o diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 884923f..1e2f96c 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -1,5 +1,9 @@ -config DRM_PTN3460 - tristate "PTN3460 DP/LVDS bridge" +config DRM_BRIDGE + tristate depends on DRM - select DRM_KMS_HELPER - ---help--- + help + Bridge registration and lookup framework. + +menu "bridge chips" + depends on DRM_BRIDGE +endmenu diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index b4733e1..be16eca 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -1,3 +1 @@ ccflags-y := -Iinclude/drm - -obj-$(CONFIG_DRM_PTN3460) += ptn3460.o diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c deleted file mode 100644 index d466696..0000000 --- a/drivers/gpu/drm/bridge/ptn3460.c +++ /dev/null @@ -1,343 +0,0 @@ -/* - * NXP PTN3460 DP/LVDS bridge driver - * - * Copyright (C) 2013 Google, Inc. - * - * 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/i2c.h> -#include <linux/gpio.h> -#include <linux/delay.h> - -#include "drmP.h" -#include "drm_edid.h" -#include "drm_crtc.h" -#include "drm_crtc_helper.h" - -#include "bridge/ptn3460.h" - -#define PTN3460_EDID_ADDR 0x0 -#define PTN3460_EDID_EMULATION_ADDR 0x84 -#define PTN3460_EDID_ENABLE_EMULATION 0 -#define PTN3460_EDID_EMULATION_SELECTION 1 -#define PTN3460_EDID_SRAM_LOAD_ADDR 0x85 - -struct ptn3460_bridge { - struct drm_connector connector; - struct i2c_client *client; - struct drm_encoder *encoder; - struct drm_bridge *bridge; - struct edid *edid; - int gpio_pd_n; - int gpio_rst_n; - u32 edid_emulation; - bool enabled; -}; - -static int ptn3460_read_bytes(struct ptn3460_bridge *ptn_bridge, char addr, - u8 *buf, int len) -{ - int ret; - - ret = i2c_master_send(ptn_bridge->client, &addr, 1); - if (ret <= 0) { - DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); - return ret; - } - - ret = i2c_master_recv(ptn_bridge->client, buf, len); - if (ret <= 0) { - DRM_ERROR("Failed to recv i2c data, ret=%d\n", ret); - return ret; - } - - return 0; -} - -static int ptn3460_write_byte(struct ptn3460_bridge *ptn_bridge, char addr, - char val) -{ - int ret; - char buf[2]; - - buf[0] = addr; - buf[1] = val; - - ret = i2c_master_send(ptn_bridge->client, buf, ARRAY_SIZE(buf)); - if (ret <= 0) { - DRM_ERROR("Failed to send i2c command, ret=%d\n", ret); - return ret; - } - - return 0; -} - -static int ptn3460_select_edid(struct ptn3460_bridge *ptn_bridge) -{ - int ret; - char val; - - /* Load the selected edid into SRAM (accessed at PTN3460_EDID_ADDR) */ - ret = ptn3460_write_byte(ptn_bridge, PTN3460_EDID_SRAM_LOAD_ADDR, - ptn_bridge->edid_emulation); - if (ret) { - DRM_ERROR("Failed to transfer edid to sram, ret=%d\n", ret); - return ret; - } - - /* Enable EDID emulation and select the desired EDID */ - val = 1 << PTN3460_EDID_ENABLE_EMULATION | - ptn_bridge->edid_emulation << PTN3460_EDID_EMULATION_SELECTION; - - ret = ptn3460_write_byte(ptn_bridge, PTN3460_EDID_EMULATION_ADDR, val); - if (ret) { - DRM_ERROR("Failed to write edid value, ret=%d\n", ret); - return ret; - } - - return 0; -} - -static void ptn3460_pre_enable(struct drm_bridge *bridge) -{ - struct ptn3460_bridge *ptn_bridge = bridge->driver_private; - int ret; - - if (ptn_bridge->enabled) - return; - - if (gpio_is_valid(ptn_bridge->gpio_pd_n)) - gpio_set_value(ptn_bridge->gpio_pd_n, 1); - - if (gpio_is_valid(ptn_bridge->gpio_rst_n)) { - gpio_set_value(ptn_bridge->gpio_rst_n, 0); - udelay(10); - gpio_set_value(ptn_bridge->gpio_rst_n, 1); - } - - /* - * 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 - * time specified in the chip's datasheet to make sure we're really up. - */ - msleep(90); - - ret = ptn3460_select_edid(ptn_bridge); - if (ret) - DRM_ERROR("Select edid failed ret=%d\n", ret); - - ptn_bridge->enabled = true; -} - -static void ptn3460_enable(struct drm_bridge *bridge) -{ -} - -static void ptn3460_disable(struct drm_bridge *bridge) -{ - struct ptn3460_bridge *ptn_bridge = bridge->driver_private; - - if (!ptn_bridge->enabled) - return; - - ptn_bridge->enabled = false; - - if (gpio_is_valid(ptn_bridge->gpio_rst_n)) - gpio_set_value(ptn_bridge->gpio_rst_n, 1); - - if (gpio_is_valid(ptn_bridge->gpio_pd_n)) - gpio_set_value(ptn_bridge->gpio_pd_n, 0); -} - -static void ptn3460_post_disable(struct drm_bridge *bridge) -{ -} - -void ptn3460_bridge_destroy(struct drm_bridge *bridge) -{ - struct ptn3460_bridge *ptn_bridge = bridge->driver_private; - - drm_bridge_cleanup(bridge); - if (gpio_is_valid(ptn_bridge->gpio_pd_n)) - gpio_free(ptn_bridge->gpio_pd_n); - if (gpio_is_valid(ptn_bridge->gpio_rst_n)) - gpio_free(ptn_bridge->gpio_rst_n); - /* Nothing else to free, we've got devm allocated memory */ -} - -struct drm_bridge_funcs ptn3460_bridge_funcs = { - .pre_enable = ptn3460_pre_enable, - .enable = ptn3460_enable, - .disable = ptn3460_disable, - .post_disable = ptn3460_post_disable, - .destroy = ptn3460_bridge_destroy, -}; - -int ptn3460_get_modes(struct drm_connector *connector) -{ - struct ptn3460_bridge *ptn_bridge; - u8 *edid; - int ret, num_modes; - bool power_off; - - ptn_bridge = container_of(connector, struct ptn3460_bridge, connector); - - if (ptn_bridge->edid) - return drm_add_edid_modes(connector, ptn_bridge->edid); - - power_off = !ptn_bridge->enabled; - ptn3460_pre_enable(ptn_bridge->bridge); - - edid = kmalloc(EDID_LENGTH, GFP_KERNEL); - if (!edid) { - DRM_ERROR("Failed to allocate edid\n"); - return 0; - } - - ret = ptn3460_read_bytes(ptn_bridge, PTN3460_EDID_ADDR, edid, - EDID_LENGTH); - if (ret) { - kfree(edid); - num_modes = 0; - goto out; - } - - ptn_bridge->edid = (struct edid *)edid; - drm_mode_connector_update_edid_property(connector, ptn_bridge->edid); - - num_modes = drm_add_edid_modes(connector, ptn_bridge->edid); - -out: - if (power_off) - ptn3460_disable(ptn_bridge->bridge); - - return num_modes; -} - -struct drm_encoder *ptn3460_best_encoder(struct drm_connector *connector) -{ - struct ptn3460_bridge *ptn_bridge; - - ptn_bridge = container_of(connector, struct ptn3460_bridge, connector); - - return ptn_bridge->encoder; -} - -struct drm_connector_helper_funcs ptn3460_connector_helper_funcs = { - .get_modes = ptn3460_get_modes, - .best_encoder = ptn3460_best_encoder, -}; - -enum drm_connector_status ptn3460_detect(struct drm_connector *connector, - bool force) -{ - return connector_status_connected; -} - -void ptn3460_connector_destroy(struct drm_connector *connector) -{ - drm_connector_cleanup(connector); -} - -struct drm_connector_funcs ptn3460_connector_funcs = { - .dpms = drm_helper_connector_dpms, - .fill_modes = drm_helper_probe_single_connector_modes, - .detect = ptn3460_detect, - .destroy = ptn3460_connector_destroy, -}; - -int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, - struct i2c_client *client, struct device_node *node) -{ - int ret; - struct drm_bridge *bridge; - struct ptn3460_bridge *ptn_bridge; - - bridge = devm_kzalloc(dev->dev, sizeof(*bridge), GFP_KERNEL); - if (!bridge) { - DRM_ERROR("Failed to allocate drm bridge\n"); - return -ENOMEM; - } - - ptn_bridge = devm_kzalloc(dev->dev, sizeof(*ptn_bridge), GFP_KERNEL); - if (!ptn_bridge) { - DRM_ERROR("Failed to allocate ptn bridge\n"); - return -ENOMEM; - } - - ptn_bridge->client = client; - ptn_bridge->encoder = encoder; - ptn_bridge->bridge = bridge; - ptn_bridge->gpio_pd_n = of_get_named_gpio(node, "powerdown-gpio", 0); - if (gpio_is_valid(ptn_bridge->gpio_pd_n)) { - ret = gpio_request_one(ptn_bridge->gpio_pd_n, - GPIOF_OUT_INIT_HIGH, "PTN3460_PD_N"); - if (ret) { - DRM_ERROR("Request powerdown-gpio failed (%d)\n", ret); - return ret; - } - } - - ptn_bridge->gpio_rst_n = of_get_named_gpio(node, "reset-gpio", 0); - if (gpio_is_valid(ptn_bridge->gpio_rst_n)) { - /* - * Request the reset pin low to avoid the bridge being - * initialized prematurely - */ - ret = gpio_request_one(ptn_bridge->gpio_rst_n, - GPIOF_OUT_INIT_LOW, "PTN3460_RST_N"); - if (ret) { - DRM_ERROR("Request reset-gpio failed (%d)\n", ret); - gpio_free(ptn_bridge->gpio_pd_n); - return ret; - } - } - - ret = of_property_read_u32(node, "edid-emulation", - &ptn_bridge->edid_emulation); - if (ret) { - DRM_ERROR("Can't read edid emulation value\n"); - goto err; - } - - ret = drm_bridge_init(dev, bridge, &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); - if (ret) { - DRM_ERROR("Failed to initialize connector with drm\n"); - goto err; - } - drm_connector_helper_add(&ptn_bridge->connector, - &ptn3460_connector_helper_funcs); - drm_connector_register(&ptn_bridge->connector); - drm_mode_connector_attach_encoder(&ptn_bridge->connector, encoder); - - return 0; - -err: - if (gpio_is_valid(ptn_bridge->gpio_pd_n)) - gpio_free(ptn_bridge->gpio_pd_n); - if (gpio_is_valid(ptn_bridge->gpio_rst_n)) - gpio_free(ptn_bridge->gpio_rst_n); - return ret; -} -EXPORT_SYMBOL(ptn3460_init); diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c new file mode 100644 index 0000000..84645e6 --- /dev/null +++ b/drivers/gpu/drm/drm_bridge.c @@ -0,0 +1,89 @@ +/* + * Copyright (c) 2014 Samsung Electronics Co., Ltd + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sub license, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the + * next paragraph) shall be included in all copies or substantial portions + * of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + */ + +#include <linux/err.h> +#include <linux/module.h> + +#include <drm/drm_crtc.h> + +static DEFINE_MUTEX(bridge_lock); +static LIST_HEAD(bridge_lookup); + +int drm_bridge_add_for_lookup(struct drm_bridge *bridge) +{ + mutex_lock(&bridge_lock); + list_add_tail(&bridge->head, &bridge_lookup); + mutex_unlock(&bridge_lock); + + return 0; +} +EXPORT_SYMBOL(drm_bridge_add_for_lookup); + +void drm_bridge_remove_from_lookup(struct drm_bridge *bridge) +{ + mutex_lock(&bridge_lock); + list_del_init(&bridge->head); + mutex_unlock(&bridge_lock); +} +EXPORT_SYMBOL(drm_bridge_remove_from_lookup); + +int drm_bridge_attach_encoder(struct drm_bridge *bridge, + struct drm_encoder *encoder) +{ + if (!bridge || !encoder) + return -EINVAL; + + if (bridge->encoder) + return -EBUSY; + + encoder->bridge = bridge; + bridge->encoder = encoder; + bridge->drm_dev = encoder->dev; + + return 0; +} +EXPORT_SYMBOL(drm_bridge_attach_encoder); + +#ifdef CONFIG_OF +struct drm_bridge *of_drm_find_bridge(struct device_node *np) +{ + struct drm_bridge *bridge; + + mutex_lock(&bridge_lock); + + list_for_each_entry(bridge, &bridge_lookup, head) { + if (bridge->dev->of_node == np) { + mutex_unlock(&bridge_lock); + return bridge; + } + } + + mutex_unlock(&bridge_lock); + return NULL; +} +EXPORT_SYMBOL(of_drm_find_bridge); +#endif + +MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>"); +MODULE_DESCRIPTION("DRM bridge infrastructure"); +MODULE_LICENSE("GPL and additional rights"); diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 1ccf5cb..436e75d 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -993,7 +993,6 @@ EXPORT_SYMBOL(drm_connector_unplug_all); * drm_bridge_init - initialize a drm transcoder/bridge * @dev: drm device * @bridge: transcoder/bridge to set up - * @funcs: bridge function table * * Initialises a preallocated bridge. Bridges should be * subclassed as part of driver connector objects. @@ -1001,8 +1000,7 @@ EXPORT_SYMBOL(drm_connector_unplug_all); * Returns: * 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) +int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge) { int ret; @@ -1012,8 +1010,7 @@ int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, if (ret) goto out; - bridge->dev = dev; - bridge->funcs = funcs; + bridge->drm_dev = dev; list_add_tail(&bridge->head, &dev->mode_config.bridge_list); dev->mode_config.num_bridge++; @@ -1032,7 +1029,7 @@ EXPORT_SYMBOL(drm_bridge_init); */ void drm_bridge_cleanup(struct drm_bridge *bridge) { - struct drm_device *dev = bridge->dev; + struct drm_device *dev = bridge->drm_dev; drm_modeset_lock_all(dev); drm_mode_object_put(dev, &bridge->base); diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig index 7f9f6f9..bdef294 100644 --- a/drivers/gpu/drm/exynos/Kconfig +++ b/drivers/gpu/drm/exynos/Kconfig @@ -51,9 +51,10 @@ config DRM_EXYNOS_DSI config DRM_EXYNOS_DP bool "EXYNOS DRM DP driver support" - depends on DRM_EXYNOS_FIMD && ARCH_EXYNOS && (DRM_PTN3460=n || DRM_PTN3460=y || DRM_PTN3460=DRM_EXYNOS) + depends on DRM_EXYNOS_FIMD && ARCH_EXYNOS default DRM_EXYNOS select DRM_PANEL + select DRM_BRIDGE help This enables support for DP device. diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c index 49356cc..055a9e1 100644 --- a/drivers/gpu/drm/exynos/exynos_dp_core.c +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c @@ -28,7 +28,6 @@ #include <drm/drm_crtc.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_panel.h> -#include <drm/bridge/ptn3460.h> #include "exynos_drm_drv.h" #include "exynos_dp_core.h" @@ -986,33 +985,23 @@ static struct drm_connector_helper_funcs exynos_dp_connector_helper_funcs = { .best_encoder = exynos_dp_best_encoder, }; -static bool find_bridge(const char *compat, struct bridge_init *bridge) +static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp, + struct drm_encoder *encoder) { - bridge->client = NULL; - bridge->node = of_find_compatible_node(NULL, NULL, compat); - if (!bridge->node) - return false; - - bridge->client = of_find_i2c_device_by_node(bridge->node); - if (!bridge->client) - return false; - - return true; -} - -/* returns the number of bridges attached */ -static int exynos_drm_attach_lcd_bridge(struct drm_device *dev, - struct drm_encoder *encoder) -{ - struct bridge_init bridge; int ret; - if (find_bridge("nxp,ptn3460", &bridge)) { - ret = ptn3460_init(dev, encoder, bridge.client, bridge.node); - if (!ret) - return 1; + dp->bridge->connector_polled = DRM_CONNECTOR_POLL_HPD; + ret = drm_bridge_attach_encoder(dp->bridge, encoder); + if (ret) { + DRM_ERROR("Failed to attach bridge to encoder\n"); + return ret; } - return 0; + + /* Allow the bridge to continue with rest of initialization */ + if (dp->bridge->funcs && dp->bridge->funcs->post_encoder_init) + return dp->bridge->funcs->post_encoder_init(dp->bridge); + + return -ENODEV; } static int exynos_dp_create_connector(struct exynos_drm_display *display, @@ -1025,9 +1014,11 @@ static int exynos_dp_create_connector(struct exynos_drm_display *display, dp->encoder = encoder; /* Pre-empt DP connector creation if there's a bridge */ - ret = exynos_drm_attach_lcd_bridge(dp->drm_dev, encoder); - if (ret) - return 0; + if (dp->bridge) { + ret = exynos_drm_attach_lcd_bridge(dp, encoder); + if (!ret) + return 0; + } connector->polled = DRM_CONNECTOR_POLL_HPD; @@ -1279,7 +1270,7 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data) if (ret) return ret; - if (!dp->panel) { + if (!dp->panel && !dp->bridge) { ret = exynos_dp_dt_parse_panel(dp); if (ret) return ret; @@ -1370,7 +1361,7 @@ static const struct component_ops exynos_dp_ops = { static int exynos_dp_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct device_node *panel_node; + struct device_node *panel_node, *bridge_node; struct exynos_dp_device *dp; int ret; @@ -1392,6 +1383,14 @@ static int exynos_dp_probe(struct platform_device *pdev) return -EPROBE_DEFER; } + bridge_node = of_parse_phandle(dev->of_node, "bridge", 0); + if (bridge_node) { + dp->bridge = of_drm_find_bridge(bridge_node); + of_node_put(bridge_node); + if (!dp->bridge) + return -EPROBE_DEFER; + } + exynos_dp_display.ctx = dp; ret = component_add(&pdev->dev, &exynos_dp_ops); diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.h b/drivers/gpu/drm/exynos/exynos_dp_core.h index a1aee69..3b0ba93 100644 --- a/drivers/gpu/drm/exynos/exynos_dp_core.h +++ b/drivers/gpu/drm/exynos/exynos_dp_core.h @@ -150,6 +150,7 @@ struct exynos_dp_device { struct drm_connector connector; struct drm_encoder *encoder; struct drm_panel *panel; + struct drm_bridge *bridge; struct clk *clock; unsigned int irq; void __iomem *reg_base; diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c index f6cf745..0309539 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c @@ -221,8 +221,9 @@ struct drm_bridge *hdmi_bridge_init(struct hdmi *hdmi) hdmi_bridge->hdmi = hdmi_reference(hdmi); bridge = &hdmi_bridge->base; + bridge->funcs = &hdmi_bridge_funcs; - drm_bridge_init(hdmi->dev, bridge, &hdmi_bridge_funcs); + drm_bridge_init(hdmi->dev, bridge); return bridge; diff --git a/include/drm/bridge/ptn3460.h b/include/drm/bridge/ptn3460.h deleted file mode 100644 index ff62344..0000000 --- a/include/drm/bridge/ptn3460.h +++ /dev/null @@ -1,37 +0,0 @@ -/* - * Copyright (C) 2013 Google, Inc. - * - * 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_PTN3460_H_ -#define _DRM_BRIDGE_PTN3460_H_ - -struct drm_device; -struct drm_encoder; -struct i2c_client; -struct device_node; - -#if defined(CONFIG_DRM_PTN3460) || defined(CONFIG_DRM_PTN3460_MODULE) - -int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder, - struct i2c_client *client, struct device_node *node); -#else - -static inline int ptn3460_init(struct drm_device *dev, - struct drm_encoder *encoder, struct i2c_client *client, - struct device_node *node) -{ - return 0; -} - -#endif - -#endif diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index e529b68..e5a41ad 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -619,6 +619,7 @@ struct drm_plane { /** * drm_bridge_funcs - drm_bridge control functions + * @post_encoder_init: called by the parent encoder * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge * @disable: Called right before encoder prepare, disables the bridge * @post_disable: Called right after encoder prepare, for lockstepped disable @@ -628,6 +629,7 @@ struct drm_plane { * @destroy: make object go away */ struct drm_bridge_funcs { + int (*post_encoder_init)(struct drm_bridge *bridge); bool (*mode_fixup)(struct drm_bridge *bridge, const struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode); @@ -648,15 +650,19 @@ struct drm_bridge_funcs { * @base: base mode object * @funcs: control functions * @driver_private: pointer to the bridge driver's internal context + * @connector_polled: polled flag needed for registering connector */ struct drm_bridge { - struct drm_device *dev; + struct device *dev; + struct drm_device *drm_dev; + struct drm_encoder *encoder; struct list_head head; struct drm_mode_object base; const struct drm_bridge_funcs *funcs; void *driver_private; + int connector_polled; }; /** @@ -895,8 +901,12 @@ extern void drm_connector_cleanup(struct drm_connector *connector); /* helper to unplug all connectors from sysfs for device */ extern void drm_connector_unplug_all(struct drm_device *dev); -extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, - const struct drm_bridge_funcs *funcs); +extern int drm_bridge_add_for_lookup(struct drm_bridge *bridge); +extern void drm_bridge_remove_from_lookup(struct drm_bridge *bridge); +extern struct drm_bridge *of_drm_find_bridge(struct device_node *np); +extern int drm_bridge_attach_encoder(struct drm_bridge *bridge, + struct drm_encoder *encoder); +extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge); extern void drm_bridge_cleanup(struct drm_bridge *bridge); extern int drm_encoder_init(struct drm_device *dev,
This patch tries to seperate drm_bridge implementation into 2 parts, a drm part and a non_drm part. A set of helper functions are defined in this patch to make bridge driver probe independent of the drm flow. The bridge devices register themselves on a lookup table when they get probed by calling "drm_bridge_add_for_lookup". The parent encoder driver waits till the bridge is available in the lookup table(by calling "of_drm_find_bridge") and then continues with its initialization. The encoder driver should call "drm_bridge_attach_encoder" to pass on the drm_device and the encoder pointers to the bridge object. Now that the drm_device pointer is available, the encoder then calls "bridge->funcs->post_encoder_init" to allow the bridge to continue registering itself with the drm core. Also, non driver model based ptn3460 driver is removed in this patch. Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com> --- .../devicetree/bindings/drm/bridge/ptn3460.txt | 27 -- drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/bridge/Kconfig | 12 +- drivers/gpu/drm/bridge/Makefile | 2 - drivers/gpu/drm/bridge/ptn3460.c | 343 -------------------- drivers/gpu/drm/drm_bridge.c | 89 +++++ drivers/gpu/drm/drm_crtc.c | 9 +- drivers/gpu/drm/exynos/Kconfig | 3 +- drivers/gpu/drm/exynos/exynos_dp_core.c | 57 ++-- drivers/gpu/drm/exynos/exynos_dp_core.h | 1 + drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 3 +- include/drm/bridge/ptn3460.h | 37 --- include/drm/drm_crtc.h | 16 +- 13 files changed, 147 insertions(+), 453 deletions(-) delete mode 100644 Documentation/devicetree/bindings/drm/bridge/ptn3460.txt delete mode 100644 drivers/gpu/drm/bridge/ptn3460.c create mode 100644 drivers/gpu/drm/drm_bridge.c delete mode 100644 include/drm/bridge/ptn3460.h