Re: [PATCH 3/3] drm/i915/glk: Fix Geminilake scalers mode programming

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

On Wed, Feb 22, 2017 at 04:44:10PM +0200, Ander Conselvan De Oliveira wrote:
> On Wed, 2017-02-22 at 16:41 +0200, Ville Syrjälä wrote:
> > On Wed, Feb 22, 2017 at 12:29:09PM +0200, Ander Conselvan de Oliveira wrote:
> > > Geminilake scalers can do 7x7 filtering for all supported input sizes,
> > > so it doesn't need the "high quality" mode programming, which was
> > > actually removed from that platform.
> > > 
> > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_atomic.c  | 14 ++++++++------
> > >  drivers/gpu/drm/i915/intel_display.c |  2 +-
> > >  drivers/gpu/drm/i915/intel_drv.h     |  6 +++---
> > >  3 files changed, 12 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> > > index aa9160e..e65818e 100644
> > > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > > @@ -121,7 +121,7 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
> > >  
> > >  /**
> > >   * intel_atomic_setup_scalers() - setup scalers for crtc per staged requests
> > > - * @dev: DRM device
> > > + * @dev_priv: i915 device
> > 
> > This change makes the diff quite messy for what's essentially a
> > addition of two lines of code.
> 
> There's no dev_priv in that function. While adding the dev_priv, it just seemed
> like pure laziness not to change the parameter. Didn't seem like a big deal
> since it has only one caller, but I'll split if you insist.

to_i915(dev) still works, so there's no real need for changing the
parameter at this time. I think sneaking in cosmetic changes should only
be done if it's really just a few lines, whereas in this case the
cosmetic change makes up most of the patch leading to a very low
signal to noise ratio.

> 
> Ander
> 
> > 
> > >   * @crtc: intel crtc
> > >   * @crtc_state: incoming crtc_state to validate and setup scalers
> > >   *
> > > @@ -136,9 +136,9 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
> > >   *         0 - scalers were setup succesfully
> > >   *         error code - otherwise
> > >   */
> > > -int intel_atomic_setup_scalers(struct drm_device *dev,
> > > -	struct intel_crtc *intel_crtc,
> > > -	struct intel_crtc_state *crtc_state)
> > > +int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
> > > +			       struct intel_crtc *intel_crtc,
> > > +			       struct intel_crtc_state *crtc_state)
> > >  {
> > >  	struct drm_plane *plane = NULL;
> > >  	struct intel_plane *intel_plane;
> > > @@ -199,7 +199,7 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
> > >  			 */
> > >  			if (!plane) {
> > >  				struct drm_plane_state *state;
> > > -				plane = drm_plane_from_index(dev, i);
> > > +				plane = drm_plane_from_index(&dev_priv->drm, i);
> > >  				state = drm_atomic_get_plane_state(drm_state, plane);
> > >  				if (IS_ERR(state)) {
> > >  					DRM_DEBUG_KMS("Failed to add [PLANE:%d] to drm_state\n",
> > > @@ -247,7 +247,9 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
> > >  		}
> > >  
> > >  		/* set scaler mode */
> > > -		if (num_scalers_need == 1 && intel_crtc->pipe != PIPE_C) {
> > > +		if (IS_GEMINILAKE(dev_priv)) {
> > > +			scaler_state->scalers[*scaler_id].mode = 0;
> > > +		} else if (num_scalers_need == 1 && intel_crtc->pipe != PIPE_C) {
> > >  			/*
> > >  			 * when only 1 scaler is in use on either pipe A or B,
> > >  			 * scaler 0 operates in high quality (HQ) mode.
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 000fb31..65e76d1 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -10959,7 +10959,7 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> > >  			ret = skl_update_scaler_crtc(pipe_config);
> > >  
> > >  		if (!ret)
> > > -			ret = intel_atomic_setup_scalers(dev, intel_crtc,
> > > +			ret = intel_atomic_setup_scalers(dev_priv, intel_crtc,
> > >  							 pipe_config);
> > >  	}
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 0edc499..d9d96d6 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1910,9 +1910,9 @@ intel_atomic_get_existing_plane_state(struct drm_atomic_state *state,
> > >  	return to_intel_plane_state(plane_state);
> > >  }
> > >  
> > > -int intel_atomic_setup_scalers(struct drm_device *dev,
> > > -	struct intel_crtc *intel_crtc,
> > > -	struct intel_crtc_state *crtc_state);
> > > +int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
> > > +			       struct intel_crtc *intel_crtc,
> > > +			       struct intel_crtc_state *crtc_state);
> > >  
> > >  /* intel_atomic_plane.c */
> > >  struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
> > > -- 
> > > 2.9.3
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 

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