| Message ID | 1464546923-13439-5-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 ? On Sunday 29 May 2016 20:35:01 Daniel Vetter wrote: > We want to hide drm_atomic_state internals better. > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 8 ++++---- > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 20 ++++++++------------ > 2 files changed, 12 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index e70a4f33d970..f315c55c1f65 > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c > @@ -288,6 +288,8 @@ static int rcar_du_atomic_commit(struct drm_device *dev, > { > struct rcar_du_device *rcdu = dev->dev_private; > struct rcar_du_commit *commit; > + struct drm_crtc *crtc; > + struct drm_crtc_state *crtc_state; > unsigned int i; > int ret; > > @@ -309,10 +311,8 @@ static int rcar_du_atomic_commit(struct drm_device > *dev, /* Wait until all affected CRTCs have completed previous commits and > * mark them as pending. > */ > - for (i = 0; i < dev->mode_config.num_crtc; ++i) { > - if (state->crtcs[i]) > - commit->crtcs |= 1 << drm_crtc_index(state->crtcs[i]); > - } > + for_each_crtc_in_state(state, crtc, crtc_state, i) > + commit->crtcs |= 1 << drm_crtc_index(crtc); > > spin_lock(&rcdu->commit.wait.lock); > ret = wait_event_interruptible_locked(rcdu->commit.wait, > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c > b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index d445e67f78e1..bfe31ca870cc > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c > @@ -140,18 +140,17 @@ int rcar_du_atomic_check_planes(struct drm_device > *dev, bool needs_realloc = false; > unsigned int groups = 0; > unsigned int i; > + struct drm_plane *drm_plane; > + struct drm_plane_state *drm_plane_state; > > /* Check if hardware planes need to be reallocated. */ > - for (i = 0; i < dev->mode_config.num_total_plane; ++i) { > + for_each_plane_in_state(state, drm_plane, drm_plane_state, i) { > struct rcar_du_plane_state *plane_state; > struct rcar_du_plane *plane; > unsigned int index; > > - if (!state->planes[i]) > - continue; > - > - plane = to_rcar_plane(state->planes[i]); > - plane_state = to_rcar_plane_state(state->plane_states[i]); > + plane = to_rcar_plane(drm_plane); > + plane_state = to_rcar_plane_state(drm_plane_state); > > dev_dbg(rcdu->dev, "%s: checking plane (%u,%tu)\n", __func__, > plane->group->index, plane - plane->group->planes); > @@ -247,18 +246,15 @@ int rcar_du_atomic_check_planes(struct drm_device > *dev, } > > /* Reallocate hardware planes for each plane that needs it. */ > - for (i = 0; i < dev->mode_config.num_total_plane; ++i) { > + for_each_plane_in_state(state, drm_plane, drm_plane_state, i) { > struct rcar_du_plane_state *plane_state; > struct rcar_du_plane *plane; > unsigned int crtc_planes; > unsigned int free; > int idx; > > - if (!state->planes[i]) > - continue; > - > - plane = to_rcar_plane(state->planes[i]); > - plane_state = to_rcar_plane_state(state->plane_states[i]); > + plane = to_rcar_plane(drm_plane); > + plane_state = to_rcar_plane_state(drm_plane_state); > > dev_dbg(rcdu->dev, "%s: allocating plane (%u,%tu)\n", __func__, > plane->group->index, plane - plane->group->planes);
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.
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 ;-) -Daniel
Hi Daniel, 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() ?
On Fri, Jun 03, 2016 at 01:54:44AM +0300, Laurent Pinchart wrote: > Hi Daniel, > > 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. -Daniel
Hi Daniel, 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.
On Fri, Jun 3, 2016 at 11:40 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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
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.
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index e70a4f33d970..f315c55c1f65 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -288,6 +288,8 @@ static int rcar_du_atomic_commit(struct drm_device *dev, { struct rcar_du_device *rcdu = dev->dev_private; struct rcar_du_commit *commit; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; unsigned int i; int ret; @@ -309,10 +311,8 @@ static int rcar_du_atomic_commit(struct drm_device *dev, /* Wait until all affected CRTCs have completed previous commits and * mark them as pending. */ - for (i = 0; i < dev->mode_config.num_crtc; ++i) { - if (state->crtcs[i]) - commit->crtcs |= 1 << drm_crtc_index(state->crtcs[i]); - } + for_each_crtc_in_state(state, crtc, crtc_state, i) + commit->crtcs |= 1 << drm_crtc_index(crtc); spin_lock(&rcdu->commit.wait.lock); ret = wait_event_interruptible_locked(rcdu->commit.wait, diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index d445e67f78e1..bfe31ca870cc 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c @@ -140,18 +140,17 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, bool needs_realloc = false; unsigned int groups = 0; unsigned int i; + struct drm_plane *drm_plane; + struct drm_plane_state *drm_plane_state; /* Check if hardware planes need to be reallocated. */ - for (i = 0; i < dev->mode_config.num_total_plane; ++i) { + for_each_plane_in_state(state, drm_plane, drm_plane_state, i) { struct rcar_du_plane_state *plane_state; struct rcar_du_plane *plane; unsigned int index; - if (!state->planes[i]) - continue; - - plane = to_rcar_plane(state->planes[i]); - plane_state = to_rcar_plane_state(state->plane_states[i]); + plane = to_rcar_plane(drm_plane); + plane_state = to_rcar_plane_state(drm_plane_state); dev_dbg(rcdu->dev, "%s: checking plane (%u,%tu)\n", __func__, plane->group->index, plane - plane->group->planes); @@ -247,18 +246,15 @@ int rcar_du_atomic_check_planes(struct drm_device *dev, } /* Reallocate hardware planes for each plane that needs it. */ - for (i = 0; i < dev->mode_config.num_total_plane; ++i) { + for_each_plane_in_state(state, drm_plane, drm_plane_state, i) { struct rcar_du_plane_state *plane_state; struct rcar_du_plane *plane; unsigned int crtc_planes; unsigned int free; int idx; - if (!state->planes[i]) - continue; - - plane = to_rcar_plane(state->planes[i]); - plane_state = to_rcar_plane_state(state->plane_states[i]); + plane = to_rcar_plane(drm_plane); + plane_state = to_rcar_plane_state(drm_plane_state); dev_dbg(rcdu->dev, "%s: allocating plane (%u,%tu)\n", __func__, plane->group->index, plane - plane->group->planes);
We want to hide drm_atomic_state internals better. Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 8 ++++---- drivers/gpu/drm/rcar-du/rcar_du_plane.c | 20 ++++++++------------ 2 files changed, 12 insertions(+), 16 deletions(-)