diff mbox series

[1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes()

Message ID 20181101184646.14753-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/3] drm/atomic: Use explicit old crtc state in drm_atomic_add_affected_planes() | expand

Commit Message

Ville Syrjälä Nov. 1, 2018, 6:46 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Replace 'crtc->state' with the explicit old crtc state.

Actually it shouldn't matter whether we use the old or the new
crtc state here since any plane that has been removed from the
crtc since the crtc state was duplicated will have been added
to the atomic state already. That is, you can't call
drm_atomic_set_crtc_for_plane() without having the new
plane state already in hand.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Daniel Vetter Nov. 5, 2018, 9:26 a.m. UTC | #1 
On Thu, Nov 01, 2018 at 08:46:44PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Replace 'crtc->state' with the explicit old crtc state.
> 
> Actually it shouldn't matter whether we use the old or the new
> crtc state here since any plane that has been removed from the
> crtc since the crtc state was duplicated will have been added
> to the atomic state already. That is, you can't call
> drm_atomic_set_crtc_for_plane() without having the new
> plane state already in hand.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I think this will break amdgpu_notify_freesync because that doesn't grab
the crtc state first. Which worked with the old stuff, because adding a
connector or plane will also add it's crtc. But with the new logic we'll
just oops I think.

Otoh, it's dead code, so what exactly are amd people doing again :-)

Adding Harry so he's aware, but patch here looks good.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_atomic.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 3dbfbddae7e6..064c48075917 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -927,6 +927,8 @@ int
>  drm_atomic_add_affected_planes(struct drm_atomic_state *state,
>  			       struct drm_crtc *crtc)
>  {
> +	const struct drm_crtc_state *old_crtc_state =
> +		drm_atomic_get_old_crtc_state(state, crtc);
>  	struct drm_plane *plane;
>  
>  	WARN_ON(!drm_atomic_get_new_crtc_state(state, crtc));
> @@ -934,7 +936,7 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
>  	DRM_DEBUG_ATOMIC("Adding all current planes for [CRTC:%d:%s] to %p\n",
>  			 crtc->base.id, crtc->name, state);
>  
> -	drm_for_each_plane_mask(plane, state->dev, crtc->state->plane_mask) {
> +	drm_for_each_plane_mask(plane, state->dev, old_crtc_state->plane_mask) {
>  		struct drm_plane_state *plane_state =
>  			drm_atomic_get_plane_state(state, plane);
>  
> -- 
> 2.18.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Ville Syrjälä Nov. 5, 2018, 2:04 p.m. UTC | #2 
On Mon, Nov 05, 2018 at 10:26:01AM +0100, Daniel Vetter wrote:
> On Thu, Nov 01, 2018 at 08:46:44PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Replace 'crtc->state' with the explicit old crtc state.
> > 
> > Actually it shouldn't matter whether we use the old or the new
> > crtc state here since any plane that has been removed from the
> > crtc since the crtc state was duplicated will have been added
> > to the atomic state already. That is, you can't call
> > drm_atomic_set_crtc_for_plane() without having the new
> > plane state already in hand.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I think this will break amdgpu_notify_freesync because that doesn't grab
> the crtc state first. Which worked with the old stuff, because adding a
> connector or plane will also add it's crtc. But with the new logic we'll
> just oops I think.

drm_atomic_add_affected_connectors() will add the crtc to the
state. So looks like it shouldn't oops.

> 
> Otoh, it's dead code, so what exactly are amd people doing again :-)

Heh. Thanks for the review.

> 
> Adding Harry so he's aware, but patch here looks good.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 3dbfbddae7e6..064c48075917 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -927,6 +927,8 @@ int
> >  drm_atomic_add_affected_planes(struct drm_atomic_state *state,
> >  			       struct drm_crtc *crtc)
> >  {
> > +	const struct drm_crtc_state *old_crtc_state =
> > +		drm_atomic_get_old_crtc_state(state, crtc);
> >  	struct drm_plane *plane;
> >  
> >  	WARN_ON(!drm_atomic_get_new_crtc_state(state, crtc));
> > @@ -934,7 +936,7 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
> >  	DRM_DEBUG_ATOMIC("Adding all current planes for [CRTC:%d:%s] to %p\n",
> >  			 crtc->base.id, crtc->name, state);
> >  
> > -	drm_for_each_plane_mask(plane, state->dev, crtc->state->plane_mask) {
> > +	drm_for_each_plane_mask(plane, state->dev, old_crtc_state->plane_mask) {
> >  		struct drm_plane_state *plane_state =
> >  			drm_atomic_get_plane_state(state, plane);
> >  
> > -- 
> > 2.18.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Harry Wentland Nov. 5, 2018, 2:30 p.m. UTC | #3 
On 2018-11-05 9:04 a.m., Ville Syrjälä wrote:
> On Mon, Nov 05, 2018 at 10:26:01AM +0100, Daniel Vetter wrote:
>> On Thu, Nov 01, 2018 at 08:46:44PM +0200, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Replace 'crtc->state' with the explicit old crtc state.
>>>
>>> Actually it shouldn't matter whether we use the old or the new
>>> crtc state here since any plane that has been removed from the
>>> crtc since the crtc state was duplicated will have been added
>>> to the atomic state already. That is, you can't call
>>> drm_atomic_set_crtc_for_plane() without having the new
>>> plane state already in hand.
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> I think this will break amdgpu_notify_freesync because that doesn't grab
>> the crtc state first. Which worked with the old stuff, because adding a
>> connector or plane will also add it's crtc. But with the new logic we'll
>> just oops I think.
> 
> drm_atomic_add_affected_connectors() will add the crtc to the
> state. So looks like it shouldn't oops.
> 

Thanks. I thought I was too tired on a Monday morning to spot the oops.

This code should be on the way out with the variable refresh patches from Nick. It's currently only used by our DKMS driver.

Looks good to me.

Acked-by: Harry Wentland <harry.wentland@amd.com>

Harry

>>
>> Otoh, it's dead code, so what exactly are amd people doing again :-)
> 
> Heh. Thanks for the review.
> 
>>
>> Adding Harry so he's aware, but patch here looks good.
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> ---
>>>  drivers/gpu/drm/drm_atomic.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>> index 3dbfbddae7e6..064c48075917 100644
>>> --- a/drivers/gpu/drm/drm_atomic.c
>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>> @@ -927,6 +927,8 @@ int
>>>  drm_atomic_add_affected_planes(struct drm_atomic_state *state,
>>>  			       struct drm_crtc *crtc)
>>>  {
>>> +	const struct drm_crtc_state *old_crtc_state =
>>> +		drm_atomic_get_old_crtc_state(state, crtc);
>>>  	struct drm_plane *plane;
>>>  
>>>  	WARN_ON(!drm_atomic_get_new_crtc_state(state, crtc));
>>> @@ -934,7 +936,7 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
>>>  	DRM_DEBUG_ATOMIC("Adding all current planes for [CRTC:%d:%s] to %p\n",
>>>  			 crtc->base.id, crtc->name, state);
>>>  
>>> -	drm_for_each_plane_mask(plane, state->dev, crtc->state->plane_mask) {
>>> +	drm_for_each_plane_mask(plane, state->dev, old_crtc_state->plane_mask) {
>>>  		struct drm_plane_state *plane_state =
>>>  			drm_atomic_get_plane_state(state, plane);
>>>  
>>> -- 
>>> 2.18.1
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>
Ville Syrjälä Nov. 5, 2018, 2:39 p.m. UTC | #4 
On Mon, Nov 05, 2018 at 02:30:50PM +0000, Wentland, Harry wrote:
> 
> 
> On 2018-11-05 9:04 a.m., Ville Syrjälä wrote:
> > On Mon, Nov 05, 2018 at 10:26:01AM +0100, Daniel Vetter wrote:
> >> On Thu, Nov 01, 2018 at 08:46:44PM +0200, Ville Syrjala wrote:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> Replace 'crtc->state' with the explicit old crtc state.
> >>>
> >>> Actually it shouldn't matter whether we use the old or the new
> >>> crtc state here since any plane that has been removed from the
> >>> crtc since the crtc state was duplicated will have been added
> >>> to the atomic state already. That is, you can't call
> >>> drm_atomic_set_crtc_for_plane() without having the new
> >>> plane state already in hand.
> >>>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> I think this will break amdgpu_notify_freesync because that doesn't grab
> >> the crtc state first. Which worked with the old stuff, because adding a
> >> connector or plane will also add it's crtc. But with the new logic we'll
> >> just oops I think.
> > 
> > drm_atomic_add_affected_connectors() will add the crtc to the
> > state. So looks like it shouldn't oops.
> > 
> 
> Thanks. I thought I was too tired on a Monday morning to spot the oops.
> 
> This code should be on the way out with the variable refresh patches from Nick. It's currently only used by our DKMS driver.
> 
> Looks good to me.
> 
> Acked-by: Harry Wentland <harry.wentland@amd.com>
> 
> Harry
> 
> >>
> >> Otoh, it's dead code, so what exactly are amd people doing again :-)
> > 
> > Heh. Thanks for the review.
> > 
> >>
> >> Adding Harry so he's aware, but patch here looks good.
> >>
> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>> ---
> >>>  drivers/gpu/drm/drm_atomic.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>> index 3dbfbddae7e6..064c48075917 100644
> >>> --- a/drivers/gpu/drm/drm_atomic.c
> >>> +++ b/drivers/gpu/drm/drm_atomic.c
> >>> @@ -927,6 +927,8 @@ int
> >>>  drm_atomic_add_affected_planes(struct drm_atomic_state *state,
> >>>  			       struct drm_crtc *crtc)
> >>>  {
> >>> +	const struct drm_crtc_state *old_crtc_state =
> >>> +		drm_atomic_get_old_crtc_state(state, crtc);
> >>>  	struct drm_plane *plane;
> >>>  
> >>>  	WARN_ON(!drm_atomic_get_new_crtc_state(state, crtc));

Oh, and if the crtc hadn't been added to the state this WARN would
have already triggered before my change.

> >>> @@ -934,7 +936,7 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
> >>>  	DRM_DEBUG_ATOMIC("Adding all current planes for [CRTC:%d:%s] to %p\n",
> >>>  			 crtc->base.id, crtc->name, state);
> >>>  
> >>> -	drm_for_each_plane_mask(plane, state->dev, crtc->state->plane_mask) {
> >>> +	drm_for_each_plane_mask(plane, state->dev, old_crtc_state->plane_mask) {
> >>>  		struct drm_plane_state *plane_state =
> >>>  			drm_atomic_get_plane_state(state, plane);
> >>>  
> >>> -- 
> >>> 2.18.1
> >>>
> >>> _______________________________________________
> >>> dri-devel mailing list
> >>> dri-devel@lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>
> >> -- 
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> >
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 3dbfbddae7e6..064c48075917 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -927,6 +927,8 @@  int
 drm_atomic_add_affected_planes(struct drm_atomic_state *state,
 			       struct drm_crtc *crtc)
 {
+	const struct drm_crtc_state *old_crtc_state =
+		drm_atomic_get_old_crtc_state(state, crtc);
 	struct drm_plane *plane;
 
 	WARN_ON(!drm_atomic_get_new_crtc_state(state, crtc));
@@ -934,7 +936,7 @@  drm_atomic_add_affected_planes(struct drm_atomic_state *state,
 	DRM_DEBUG_ATOMIC("Adding all current planes for [CRTC:%d:%s] to %p\n",
 			 crtc->base.id, crtc->name, state);
 
-	drm_for_each_plane_mask(plane, state->dev, crtc->state->plane_mask) {
+	drm_for_each_plane_mask(plane, state->dev, old_crtc_state->plane_mask) {
 		struct drm_plane_state *plane_state =
 			drm_atomic_get_plane_state(state, plane);