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

[Wentland, Harry]

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 at 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 at 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 at 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 at 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 at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>