On Wed, Jul 30, 2014 at 8:38 PM, Thierry Reding <thierry.reding@xxxxxxxxx> 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@xxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html