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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

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