Re: [PATCH 1/3] drm/i915: Added a return type for panel fitter config functions

[Akash Goel <akash.goel@intel.com>]

On Fri, 2014-06-20 at 15:03 +0100, Chris Wilson wrote:
> On Fri, Jun 20, 2014 at 07:25:52PM +0530, akash.goel@intel.com wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 912e9c4..6117639 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -824,12 +824,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >  	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
> >  		intel_fixed_panel_mode(intel_connector->panel.fixed_mode,
> >  				       adjusted_mode);
> > -		if (!HAS_PCH_SPLIT(dev))
> > -			intel_gmch_panel_fitting(intel_crtc, pipe_config,
> > -						 intel_connector->panel.fitting_mode);
> > -		else
> > -			intel_pch_panel_fitting(intel_crtc, pipe_config,
> > -						intel_connector->panel.fitting_mode);
> > +		if (!HAS_PCH_SPLIT(dev)) {
> > +			if (!intel_gmch_panel_fitting(intel_crtc, pipe_config,
> > +						 intel_connector->panel.fitting_mode))
> > +				return false;
> > +		} else {
> > +			if (!intel_pch_panel_fitting(intel_crtc, pipe_config,
> > +						 intel_connector->panel.fitting_mode))
> > +				return false;
> > +		}
> >  	}
> >  
> >  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> > @@ -3782,7 +3785,7 @@ intel_dp_set_property(struct drm_connector *connector,
> >  
> >  done:
> >  	if (intel_encoder->base.crtc)
> > -		intel_crtc_restore_mode(intel_encoder->base.crtc);
> > +		return intel_crtc_restore_mode(intel_encoder->base.crtc);
> 
> But you don't unwind the property change after failure.
Sorry, will handle this in next version of the patch.
> 
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index ab5962b..860d531 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -727,7 +727,7 @@ void intel_mark_busy(struct drm_device *dev);
> >  void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
> >  			struct intel_engine_cs *ring);
> >  void intel_mark_idle(struct drm_device *dev);
> > -void intel_crtc_restore_mode(struct drm_crtc *crtc);
> > +int intel_crtc_restore_mode(struct drm_crtc *crtc);
> >  void intel_crtc_update_dpms(struct drm_crtc *crtc);
> >  void intel_encoder_destroy(struct drm_encoder *encoder);
> >  void intel_connector_dpms(struct drm_connector *, int mode);
> > @@ -918,10 +918,10 @@ int intel_panel_init(struct intel_panel *panel,
> >  void intel_panel_fini(struct intel_panel *panel);
> >  void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
> >  			    struct drm_display_mode *adjusted_mode);
> > -void intel_pch_panel_fitting(struct intel_crtc *crtc,
> > +bool intel_pch_panel_fitting(struct intel_crtc *crtc,
> >  			     struct intel_crtc_config *pipe_config,
> >  			     int fitting_mode);
> > -void intel_gmch_panel_fitting(struct intel_crtc *crtc,
> > +bool intel_gmch_panel_fitting(struct intel_crtc *crtc,
> >  			      struct intel_crtc_config *pipe_config,
> >  			      int fitting_mode);
> 
> Only make significnt changes like this to one interface at a time.
Fine will split this patch in 2 parts.
> -Chris
>