All of lore.kernel.org
 help / color / mirror / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 04/26] drm/rcar-du: Use for_each_*_in_state
Date: Fri, 03 Jun 2016 12:50:31 +0300	[thread overview]
Message-ID: <2473335.q8E3kATvZ6@avalon> (raw)
In-Reply-To: <CAKMK7uE1wU8cU89Vqnv_9B6QxomjZPftbUOzKU_+ouM4mgOLbw@mail.gmail.com>

Hi Daniel,

On Friday 03 Jun 2016 11:45:56 Daniel Vetter wrote:
> On Fri, Jun 3, 2016 at 11:40 AM, Laurent Pinchart 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.

My bad, I had misread the for_each_*_in_state macros and thought they were 
checking both. Sorry for the noise.

-- 
Regards,

Laurent Pinchart

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2016-06-03  9:50 UTC|newest]

Thread overview: 76+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-05-29 18:34 [PATCH 00/26] RFC: generic nonblocking support in the atomic helpers Daniel Vetter
2016-05-29 18:34 ` [PATCH 01/26] drm/atomic-helper: use for_each_*_in_state more Daniel Vetter
2016-05-30 12:17   ` Maarten Lankhorst
2016-05-30 15:02     ` Daniel Vetter
2016-05-29 18:34 ` [PATCH 02/26] drm/i915: Use drm_atomic_get_existing_plane_state Daniel Vetter
2016-05-30 13:10   ` [Intel-gfx] " Maarten Lankhorst
2016-05-29 18:35 ` [PATCH 03/26] drm/msm: Use for_each_*_in_state Daniel Vetter
2016-05-29 18:35 ` [PATCH 04/26] drm/rcar-du: " Daniel Vetter
2016-05-30  9:18   ` Laurent Pinchart
2016-05-30  9:58     ` Maarten Lankhorst
2016-05-30 14:54       ` [Intel-gfx] " Daniel Vetter
2016-06-02 22:54         ` Laurent Pinchart
2016-06-03  6:55           ` [Intel-gfx] " Daniel Vetter
2016-06-03  9:40             ` Laurent Pinchart
2016-06-03  9:45               ` Daniel Vetter
2016-06-03  9:50                 ` Laurent Pinchart [this message]
2016-05-29 18:35 ` [PATCH 05/26] drm/vc4: Use for_each_plane_in_state Daniel Vetter
2016-05-29 18:35 ` [PATCH 06/26] drm/atomic: Add __drm_atomic_get_current_plane_state Daniel Vetter
2016-05-30 11:42   ` [Intel-gfx] " Maarten Lankhorst
2016-05-30 15:05     ` Daniel Vetter
2016-05-31  8:35       ` [Intel-gfx] " Maarten Lankhorst
2016-05-29 18:35 ` [PATCH 07/26] drm/exynos: Use for_each_crtc_in_state Daniel Vetter
2016-05-31 12:19   ` Inki Dae
2016-06-13  0:52   ` Inki Dae
2016-05-29 18:35 ` [PATCH 08/26] drm: Consolidate connector arrays in drm_atomic_state Daniel Vetter
2016-05-30 14:59   ` Ville Syrjälä
2016-05-30 15:13     ` Daniel Vetter
2016-05-30 15:25       ` [Intel-gfx] " Ville Syrjälä
2016-05-30 15:33         ` Daniel Vetter
2016-05-30 15:45           ` Ville Syrjälä
2016-05-30 15:48             ` [Intel-gfx] " Daniel Vetter
2016-05-29 18:35 ` [PATCH 09/26] drm: Consolidate plane " Daniel Vetter
2016-05-29 18:35 ` [PATCH 10/26] drm: Consolidate crtc " Daniel Vetter
2016-05-29 18:35 ` [PATCH 11/26] drm/fence: add fence to drm_pending_event Daniel Vetter
2016-05-29 18:35 ` [PATCH 12/26] drm/atomic-helper: Massage swap_state signature somewhat Daniel Vetter
2016-05-30 13:08   ` Maarten Lankhorst
2016-05-30 15:09     ` [Intel-gfx] " Daniel Vetter
2016-05-31  8:43       ` Maarten Lankhorst
2016-05-31 10:46         ` Daniel Vetter
2016-05-31 12:20           ` Maarten Lankhorst
2016-05-29 18:35 ` [PATCH 13/26] drm/arc: Nuke event_list Daniel Vetter
2016-05-29 18:35   ` Daniel Vetter
2016-05-29 18:35 ` [PATCH 14/26] drm/arc: Actually bother with handling atomic events Daniel Vetter
2016-05-29 18:35   ` Daniel Vetter
2016-05-29 18:35 ` [PATCH 15/26] drm/arc: Implement nonblocking commit correctly Daniel Vetter
2016-05-29 18:35   ` Daniel Vetter
2016-05-30  8:15   ` [Intel-gfx] " Maarten Lankhorst
2016-05-30  8:15     ` Maarten Lankhorst
2016-05-30  9:24     ` Daniel Vetter
2016-05-30  9:24       ` Daniel Vetter
2016-05-30  9:36       ` [Intel-gfx] " Maarten Lankhorst
2016-05-30  9:36         ` Maarten Lankhorst
2016-05-30 15:10         ` [Intel-gfx] " Daniel Vetter
2016-05-30 15:10           ` Daniel Vetter
2016-05-29 18:35 ` [PATCH 16/26] drm/hdlcd: Use helper support for nonblocking commits Daniel Vetter
2016-05-31 11:02   ` Liviu Dudau
2016-05-31 11:07     ` Daniel Vetter
2016-05-31 13:13       ` Liviu Dudau
2016-05-29 18:35 ` [PATCH 17/26] drm/fsl-du: Implement some semblance of vblank event handling Daniel Vetter
2016-05-29 18:35 ` [PATCH 18/26] drm/hisilicon: " Daniel Vetter
2016-05-29 18:35 ` [PATCH 19/26] drm/sun4i: " Daniel Vetter
2016-06-01 16:18   ` Maxime Ripard
2016-06-01 22:02     ` Daniel Vetter
2016-05-29 18:35 ` [PATCH 20/26] drm/atomic: kerneldoc for drm_atomic_crtc_needs_modeset Daniel Vetter
2016-05-29 18:35 ` [PATCH 21/26] drm/atomic-helper: nonblocking commit support Daniel Vetter
2016-05-30  8:01   ` [PATCH] " Daniel Vetter
2016-05-31 14:22     ` [Intel-gfx] " Maarten Lankhorst
2016-05-31 14:33       ` Daniel Vetter
2016-05-29 18:35 ` [PATCH 22/26] drm/i915: Signal drm events for atomic Daniel Vetter
2016-05-29 18:35 ` [PATCH 23/26] drm/i915: Roll out the helper nonblock tracking Daniel Vetter
2016-05-29 18:35 ` [PATCH 24/26] drm/rockchip: convert to helper nonblocking atomic commit Daniel Vetter
2016-05-29 18:35 ` [PATCH 25/26] drm/rockchip: Nuke pending event handling in preclose Daniel Vetter
2016-05-29 18:35 ` [PATCH 26/26] drm/virtio: Don't reinvent a flipping wheel Daniel Vetter
2016-05-31 13:40 ` ✗ Ro.CI.BAT: failure for RFC: generic nonblocking support in the atomic helpers Patchwork
2016-05-31 13:42 ` Patchwork
2016-05-31 14:29 ` Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2473335.q8E3kATvZ6@avalon \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=daniel.vetter@intel.com \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.