On Wed, 14 Dec 2016, Daniel Vetter <daniel@xxxxxxxx> wrote: > On Wed, Dec 14, 2016 at 01:22:15PM +0200, Jani Nikula wrote: >> On Wed, 14 Dec 2016, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote: >> > +/** >> > + * drm_for_each_connector_iter - connector_list iterator macro >> > + * @connector: struct &drm_connector pointer used as cursor >> > + * @iter: struct &drm_connector_list_iter >> > + * >> > + * Note that @connector is only valid within the list body, if you want to use >> > + * @connector after calling drm_connector_list_iter_put() then you need to grab >> > + * your own reference first using drm_connector_reference(). >> > + */ >> > +#define drm_for_each_connector_iter(connector, iter) \ >> > + while ((connector = drm_connector_list_iter_next(iter))) >> > + >> >> Observe that in most, if not all, cases you lock over the for loop, but >> not more. That means you always get/put right around the loop. >> >> You could have a variant of get() that returns the first item, and a >> variant of next() that does put() automatically when it's about to >> return NULL, and implement most of the loops like this: >> >> #define drm_for_each_connector_simple(dev, iter, connector) \ >> for (connector = drm_connector_list_iter_get_first(dev, iter); \ >> connector != NULL; \ >> connector = drm_connector_list_iter_next_put(iter)) >> >> In the long run, that should be called just drm_for_each_connector. >> >> The only case where you'd have to call put() explicitly is when you >> break out of the loop early. Otherwise all looping would be dead simple, >> without all the gets and puts, just like they are now. Perhaps the >> naming of the functions should be such that this is the most common >> case. Perhaps you don't actually need the versions with "manual" locking >> at all. > > I had this in an earlier iteration of this patch series. The implemenation > was somewhat misguided (as in it used srcu and some other wizzardry that I > now managed to remove), but otherwise was exactly what you've asking for > here. > > The amount of leaking was mindboggling. > > And that was only me being sloppyin in converting the piles of existing > loop, not even ongoing maintenance of new loops additions done by people > who're not well versed in the finer details of connector_list walking and > the refcounting dance involved. > > Given that I've concluded that hiding those details is a bad choice, and > to top it off the new code enforces matching get/put using lockdep. We do > pay a price in that simple loops become a bit more verbose, but > unfortunately there's no way to create something which is guarnateed to > get destructed when leaving a code block (unlike in C++). And without that > guarantee I don't think it'll be maintainable long-term. > > I expect that drm_for_each_connector will stay around for a long time > (maybe even forever). As long as your driver doesn't hotplug connectors, > it's perfectly fine. Only core + helpers + any driver supporting mst > really need to switch over. > > Overall not the prettiest thing, but still an acceptable tradeoff imo. Fair enough. Jani. -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
