Re: [PATCH V6 6/8] drm/bridge: Modify drm_bridge core to support driver model

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6329 bytes --]

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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel