[PATCH] drm/atomic: Set current atomic state in drm_private_state

[PATCH] drm/atomic: Set current atomic state in drm_private_state

[Alexandru Gheorghe]

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;
 
 	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

Re: [PATCH] drm/atomic: Set current atomic state in drm_private_state

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Set current atomic state in drm_private_state

[Alexandru-Cosmin Gheorghe]

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

-- 
Cheers,
Alex G
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Set current atomic state in drm_private_state

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Set current atomic state in drm_private_state

[Alexandru-Cosmin Gheorghe]

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

-- 
Cheers,
Alex G
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel