* [PATCH] drm/atomic: Set current atomic state in drm_private_state
@ 2018-05-30 17:30 Alexandru Gheorghe
2018-05-30 20:22 ` Ville Syrjälä
0 siblings, 1 reply; 5+ messages in thread
From: Alexandru Gheorghe @ 2018-05-30 17:30 UTC (permalink / raw)
To: gustavo, maarten.lankhorst, seanpaul, airlied, dri-devel,
liviu.dudau, brian.starkey
Cc: nd, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/atomic: Set current atomic state in drm_private_state
2018-05-30 17:30 [PATCH] drm/atomic: Set current atomic state in drm_private_state Alexandru Gheorghe
@ 2018-05-30 20:22 ` Ville Syrjälä
2018-05-31 9:56 ` Alexandru-Cosmin Gheorghe
2018-06-15 20:38 ` Ville Syrjälä
0 siblings, 2 replies; 5+ messages in thread
From: Ville Syrjälä @ 2018-05-30 20:22 UTC (permalink / raw)
To: Alexandru Gheorghe; +Cc: airlied, liviu.dudau, dri-devel, nd
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/atomic: Set current atomic state in drm_private_state
2018-05-30 20:22 ` Ville Syrjälä
@ 2018-05-31 9:56 ` Alexandru-Cosmin Gheorghe
2018-06-15 20:38 ` Ville Syrjälä
1 sibling, 0 replies; 5+ messages in thread
From: Alexandru-Cosmin Gheorghe @ 2018-05-31 9:56 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: airlied, liviu.dudau, dri-devel, nd
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/atomic: Set current atomic state in drm_private_state
2018-05-30 20:22 ` Ville Syrjälä
2018-05-31 9:56 ` Alexandru-Cosmin Gheorghe
@ 2018-06-15 20:38 ` Ville Syrjälä
2018-06-17 8:23 ` Alexandru-Cosmin Gheorghe
1 sibling, 1 reply; 5+ messages in thread
From: Ville Syrjälä @ 2018-06-15 20:38 UTC (permalink / raw)
To: Alexandru Gheorghe; +Cc: airlied, liviu.dudau, dri-devel, nd
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/atomic: Set current atomic state in drm_private_state
2018-06-15 20:38 ` Ville Syrjälä
@ 2018-06-17 8:23 ` Alexandru-Cosmin Gheorghe
0 siblings, 0 replies; 5+ messages in thread
From: Alexandru-Cosmin Gheorghe @ 2018-06-17 8:23 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: airlied, nd, liviu.dudau, dri-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-06-17 8:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-05-30 17:30 [PATCH] drm/atomic: Set current atomic state in drm_private_state Alexandru Gheorghe
2018-05-30 20:22 ` Ville Syrjälä
2018-05-31 9:56 ` Alexandru-Cosmin Gheorghe
2018-06-15 20:38 ` Ville Syrjälä
2018-06-17 8:23 ` Alexandru-Cosmin Gheorghe
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.