On Fri, Jun 3, 2016 at 11:40 AM, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > On Friday 03 Jun 2016 08:55:36 Daniel Vetter wrote: >> On Fri, Jun 03, 2016 at 01:54:44AM +0300, Laurent Pinchart wrote: >> > On Monday 30 May 2016 16:54:10 Daniel Vetter wrote: >> >> On Mon, May 30, 2016 at 11:58:27AM +0200, Maarten Lankhorst wrote: >> >>> Op 30-05-16 om 11:18 schreef Laurent Pinchart: >> >>>> Hi Daniel, >> >>>> >> >>>> Thank you for the patch. >> >>>> >> >>>> This looks good to me as the resulting code is mostly similar. >> >>>> However, the for_each_*_in_state macros end with an for_each_if() >> >>>> that tests if the object's state is NULL, which isn't present in this >> >>>> code. I'm wondering whether that was an oversight on my side possibly >> >>>> leading to a crash when dereferencing a NULL state, or an unneeded >> >>>> check in the macros. Can atomic_state->*_states[i] be NULL if >> >>>> atomic_state->*[i] is not NULL ? >> >>> >> >>> Not in any normal case. >> >> >> >> Yeah, the drm_atomic_get_*_state functions only ever fill in both of >> >> neither. If this gets out of sync it's a bug ;-) >> > >> > Should the check be removed then ? Or replaced by a WARN_ON() ? >> >> In all the places I converted here I nuked those checks, since they moved >> into the loop now. Not sure what checks you're talking about. > > I'm talking about the for_each_if() check inside the for_each_*_in_state > macros. The rule is drm_atomic_state->plane[i] != NULL iff drm_atomic_state->plane_state != NULL. So you can check either of them for the same result. But you still need to check one of them, otherwise all the loops in drivers and helpers will oops. Not sure why you want to remove that check, your driver had the equivalent (which I removed) too. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
