[Intel-gfx] [PATCH 4/9] drm/i915: split intel_update_plane into check() and commit()

[Ville Syrjälä]

On Fri, Aug 29, 2014 at 12:09:39PM -0300, Gustavo Padovan wrote:
> 2014-08-29 Ville Syrjälä <ville.syrjala at linux.intel.com>:
> 
> > On Thu, Aug 28, 2014 at 02:40:08PM -0300, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
> > > 
> > > Due to the upcoming atomic modesetting feature we need to separate
> > > some update functions into a check step that can fail and a commit
> > > step that should, ideally, never fail.
> > > 
> > > This commit splits intel_update_plane() and its commit part can still
> > > fail due to the fb pinning procedure.
> > > 
> > > Signed-off-by: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/intel_sprite.c | 128 ++++++++++++++++++++++++++----------
> > >  1 file changed, 93 insertions(+), 35 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index 4cbe286..b1cb606 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -844,22 +844,32 @@ static bool colorkey_enabled(struct intel_plane *intel_plane)
> > >  	return key.flags != I915_SET_COLORKEY_NONE;
> > >  }
> > >  
> > > +static bool get_visible(struct drm_rect *src, struct drm_rect *dst,
> > 
> > get_visisble() is not a good name here IMO, also I think this split is at
> > a slightly too low level. What we really want is to start creating some
> > kind of plane config struct that can be passed to both check and commit,
> > and then check can already store all the clipped coordinates etc. to the
> > plane config and commit can just look them up w/o recomputing them.
> 
> What do you really mean by too low level here?

I mean you just roughtly cut it in half from the middle and duplicated a
bit of the clipping logic in both halves. That's actually fairly broken
since you seem to have left the extra src coordinate frobbing (align) that
we do after clipping only to the check() part. So you'd need to yank all
that extra code to the "get_visible" function as well.

But with the plane config you can avoid doing all that work twice since
check will do it and just pass the adjusted coordinates to commit.

> Would grabbing
> struct drm_plane_state from the atomic branch fix this for you?

That looks like it's meant to house the user requested coordinates
rather than the clipped ones. What you could do is just shovel the
drm_rects we use during the clipping into a new i915 specific plane
config struct and pass that to both check and commit. We can later
look into moving that stuff into some core struct if seems like a win
for more than one driver.

> 
> I'll get a v2 of these patches working with struct drm_plane_state.
> 
> 	Gustavo

-- 
Ville Syrjälä
Intel OTC