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. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
