Re: [PATCH 2/3] drm/i915: Plumb the full atomic state into icl_check_nv12_planes()

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

On Thu, May 30, 2024 at 03:57:16PM +0300, Jani Nikula wrote:
> On Tue, 28 May 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > icl_check_nv12_planes() needs the full atomic state. Instead of
> > digging that out from dubious sources plumb it in explicitly.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Are most to_intel_atomic_state() uses suspect...?

The ones of the form to_intel_atomic_state(foo_obj_state->state).

Though I guess "suspect" is a bit strong. They do work as long
as its done before swap_state(), but after that they just give
you NULL. Which can easily bite you when you think you found
some useful helper function and then proceed to call it during
the actual commit phase and get greeted by an oops. So "undesirable"
is perhaps a better way to put it. I would like to get rid of all
of these, if possible.

> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 071ba95a1472..dbbc72494a46 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -4033,11 +4033,12 @@ static int icl_add_linked_planes(struct intel_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > -static int icl_check_nv12_planes(struct intel_crtc_state *crtc_state)
> > +static int icl_check_nv12_planes(struct intel_atomic_state *state,
> > +				 struct intel_crtc *crtc)
> >  {
> > -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > -	struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state);
> > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +	struct intel_crtc_state *crtc_state =
> > +		intel_atomic_get_new_crtc_state(state, crtc);
> >  	struct intel_plane *plane, *linked;
> >  	struct intel_plane_state *plane_state;
> >  	int i;
> > @@ -5786,7 +5787,7 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state)
> >  					    new_crtc_state, i) {
> >  		u8 old_active_planes, new_active_planes;
> >  
> > -		ret = icl_check_nv12_planes(new_crtc_state);
> > +		ret = icl_check_nv12_planes(state, crtc);
> >  		if (ret)
> >  			return ret;
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel