On Wed, Jul 30, 2014 at 09:44:32PM +0530, Ajay kumar wrote: > On Wed, Jul 30, 2014 at 9:10 PM, Thierry Reding > <thierry.reding@xxxxxxxxx> wrote: > > On Wed, Jul 30, 2014 at 08:46:44PM +0530, Ajay kumar wrote: > >> On Wed, Jul 30, 2014 at 5:35 PM, Thierry Reding <thierry.reding@xxxxxxxxx> wrote: > >> > On Sat, Jul 26, 2014 at 12:52:09AM +0530, Ajay Kumar wrote: > > [...] > >> >> +int ptn3460_post_encoder_init(struct drm_bridge *bridge) > >> >> +{ > >> >> + struct ptn3460_bridge *ptn_bridge = bridge->driver_private; > >> >> + int ret; > >> >> + > >> >> + /* bridge is attached to encoder. > >> >> + * safe to remove it from the bridge_lookup table. > >> >> + */ > >> >> + drm_bridge_remove_from_lookup(bridge); > >> > > >> > No, you should never do this. First, you're not adding it back to the > >> > registry when the bridge is detached, so unloading and reloading the > >> > display driver will fail. Second there should never be a need to remove > >> > it from the registry as long as the driver itself is loaded. If you're > >> > concerned about a single bridge being used multiple times, there's > >> > already code to handle that in your previous patch: > >> > > >> > int drm_bridge_attach_encoder(...) > >> > { > >> > ... > >> > > >> > if (bridge->encoder) > >> > return -EBUSY; > >> > > >> > ... > >> > } > >> > > >> > Generally the registry should contain a list of bridges that have been > >> > registered, whether they're used or not is irrelevant. > >> I was just wondering if it is ok to have a node in two independent lists? > >> bridge_lookup_table and the other mode_config.bridge_list? > > > > Oh, it reuses the head field for the registry. I hadn't noticed before. > > No, you certainly can't have the same node in two lists. Honestly I > > don't quite understand why there was a need to expose drm_bridge as a > > drm_mode_object in the first place since it's never exported to > > userspace. > > > > So I think that perhaps we could simply get rid of the base field and > > not tie in drm_bridge objects with the DRM object as we currently do. > > But until Sean or Rob comment on this it might be better to simply add > > another struct list_head field for the registry. That way both can > > coexist and we can independently still decide to remove the base and > > head fields if they're no longer needed. > Ok. What shall I name the new list_head? "list" would be a good choice in my opinion. > > .get_modes() still needs to be done from the bridge because that is the > > most closely connected to the display controller and therefore dictates > > the timing that the display controller needs to generate. > > > > Querying the panel's .get_modes() might be useful to figure out which > > emulation mode to use in the bridge. > But, get_modes from panel returns me only the no_of_modes but > not the actual mode structure. How do I compare the list of supported > emulation modes? You could iterate over the connector's probed_modes list which should contain all the modes that the panel reported (after .get_modes() was called). Thierry
Attachment:
pgpXXGtIu8jAu.pgp
Description: PGP signature