| Message ID | 1527701452-1934-1-git-send-email-alexandru-cosmin.gheorghe@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 30, 2018 at 06:30:52PM +0100, Alexandru Gheorghe wrote: > drm_private_state has a back pointer to the drm_atomic_state, > however that was not initialized in drm_atomic_get_private_obj_state > after duplication, as it is the case for other drm atomic getters > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> > --- > drivers/gpu/drm/drm_atomic.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 7d25c42..249aaf8 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1108,6 +1108,7 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, > state->private_objs[index].old_state = obj->state; > state->private_objs[index].new_state = obj_state; > state->private_objs[index].ptr = obj; > + obj_state->state = state; Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> I guess no one ever used that pointer. Should we add some WARNs to drm_atomic_helper_swap_state() to make sure these are correct? I think in general life might be nicer if we didn't even have these pointers at all. As it stands it's pretty easy to accidentally use them when you're not supposed to (eg. after swap_state() try to use the new_state->state). But there's tons of code that would need to be touched to eliminate them so not a very pleasant project. > > state->num_private_objs = num_objs; > > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, Thank you for the review. On Wed, May 30, 2018 at 11:22:27PM +0300, Ville Syrjälä wrote: > On Wed, May 30, 2018 at 06:30:52PM +0100, Alexandru Gheorghe wrote: > > drm_private_state has a back pointer to the drm_atomic_state, > > however that was not initialized in drm_atomic_get_private_obj_state > > after duplication, as it is the case for other drm atomic getters > > > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> > > --- > > drivers/gpu/drm/drm_atomic.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 7d25c42..249aaf8 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -1108,6 +1108,7 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, > > state->private_objs[index].old_state = obj->state; > > state->private_objs[index].new_state = obj_state; > > state->private_objs[index].ptr = obj; > > + obj_state->state = state; > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > I guess no one ever used that pointer. Should we add some WARNs > to drm_atomic_helper_swap_state() to make sure these are correct? That would've caught this, but I'm not sure if it's not to late for a warning, we just got lucky nobody ever tried to access that pointer yet. > > I think in general life might be nicer if we didn't even have these > pointers at all. As it stands it's pretty easy to accidentally > use them when you're not supposed to (eg. after swap_state() try > to use the new_state->state). But there's tons of code that would > need to be touched to eliminate them so not a very pleasant project. I agree easy to accidentally use them after swap, but I suppose is to late to eliminate them or if we should. > > > > > state->num_private_objs = num_objs; > > > > -- > > 2.7.4 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel
On Wed, May 30, 2018 at 11:22:27PM +0300, Ville Syrjälä wrote: > On Wed, May 30, 2018 at 06:30:52PM +0100, Alexandru Gheorghe wrote: > > drm_private_state has a back pointer to the drm_atomic_state, > > however that was not initialized in drm_atomic_get_private_obj_state > > after duplication, as it is the case for other drm atomic getters > > > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> > > --- > > drivers/gpu/drm/drm_atomic.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 7d25c42..249aaf8 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -1108,6 +1108,7 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, > > state->private_objs[index].old_state = obj->state; > > state->private_objs[index].new_state = obj_state; > > state->private_objs[index].ptr = obj; > > + obj_state->state = state; > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> And finally pushed to frm-misc-next. Almost forgot about this one. Thanks for the patch. > > I guess no one ever used that pointer. Should we add some WARNs > to drm_atomic_helper_swap_state() to make sure these are correct? > > I think in general life might be nicer if we didn't even have these > pointers at all. As it stands it's pretty easy to accidentally > use them when you're not supposed to (eg. after swap_state() try > to use the new_state->state). But there's tons of code that would > need to be touched to eliminate them so not a very pleasant project. > > > > > state->num_private_objs = num_objs; > > > > -- > > 2.7.4 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel
On Fri, Jun 15, 2018 at 11:38:44PM +0300, Ville Syrjälä wrote: > On Wed, May 30, 2018 at 11:22:27PM +0300, Ville Syrjälä wrote: > > On Wed, May 30, 2018 at 06:30:52PM +0100, Alexandru Gheorghe wrote: > > > drm_private_state has a back pointer to the drm_atomic_state, > > > however that was not initialized in drm_atomic_get_private_obj_state > > > after duplication, as it is the case for other drm atomic getters > > > > > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> > > > --- > > > drivers/gpu/drm/drm_atomic.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > index 7d25c42..249aaf8 100644 > > > --- a/drivers/gpu/drm/drm_atomic.c > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > @@ -1108,6 +1108,7 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, > > > state->private_objs[index].old_state = obj->state; > > > state->private_objs[index].new_state = obj_state; > > > state->private_objs[index].ptr = obj; > > > + obj_state->state = state; > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > And finally pushed to frm-misc-next. Almost forgot about this one. > > Thanks for the patch. Thanks for pushing it. > > > > > I guess no one ever used that pointer. Should we add some WARNs > > to drm_atomic_helper_swap_state() to make sure these are correct? > > > > I think in general life might be nicer if we didn't even have these > > pointers at all. As it stands it's pretty easy to accidentally > > use them when you're not supposed to (eg. after swap_state() try > > to use the new_state->state). But there's tons of code that would > > need to be touched to eliminate them so not a very pleasant project. > > > > > > > > state->num_private_objs = num_objs; > > > > > > -- > > > 2.7.4 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > Ville Syrjälä > > Intel > > -- > Ville Syrjälä > Intel > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7d25c42..249aaf8 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1108,6 +1108,7 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state, state->private_objs[index].old_state = obj->state; state->private_objs[index].new_state = obj_state; state->private_objs[index].ptr = obj; + obj_state->state = state; state->num_private_objs = num_objs;
drm_private_state has a back pointer to the drm_atomic_state, however that was not initialized in drm_atomic_get_private_obj_state after duplication, as it is the case for other drm atomic getters Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com> --- drivers/gpu/drm/drm_atomic.c | 1 + 1 file changed, 1 insertion(+)