From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 01/10] drm/i915: introduce struct intel_crtc_config Date: Fri, 22 Feb 2013 12:23:36 +0200 Message-ID: <20130222102336.GK4469@intel.com> References: <1361491014-13888-1-git-send-email-daniel.vetter@ffwll.ch> <1361491014-13888-2-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 84362E5CC4 for ; Fri, 22 Feb 2013 02:23:40 -0800 (PST) Content-Disposition: inline In-Reply-To: <1361491014-13888-2-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Fri, Feb 22, 2013 at 12:56:45AM +0100, Daniel Vetter wrote: > Currently only containing the requested and the adjusted mode. And > only crtc callbacks are converted somewhat to it, encoders will be > done on a as-needed basis (simply too much churn in one patch > otherwise). > = > Future patches will add tons more useful stuff to this struct, > starting with the very simple. > = > v2: Store the pipe_config in the intel_crtc, so that the ->mode-set, > ->enable and also ->disable have easy access to it. > = > v3: Store the pipe config in the right crtc ... > = > v4: Rebased. > = > v5: Fixup an OOPS when trying to kfree an ERR_PTR. > = > v6: Used drm_moode_copy and some other small cleanups as suggested > by Ville Syrj=E4l=E4. > = > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/i915_drv.h | 4 +- > drivers/gpu/drm/i915/intel_display.c | 83 +++++++++++++++++++++---------= ------ > drivers/gpu/drm/i915/intel_drv.h | 7 +++ > 3 files changed, 58 insertions(+), 36 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_= drv.h > index e95337c..8fb14fb 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -271,6 +271,8 @@ struct drm_i915_error_state { > struct intel_display_error_state *display; > }; > = > +struct intel_crtc_config; > + > struct drm_i915_display_funcs { > bool (*fbc_enabled)(struct drm_device *dev); > void (*enable_fbc)(struct drm_crtc *crtc, unsigned long interval); > @@ -284,8 +286,6 @@ struct drm_i915_display_funcs { > struct drm_display_mode *mode); > void (*modeset_global_resources)(struct drm_device *dev); > int (*crtc_mode_set)(struct drm_crtc *crtc, > - struct drm_display_mode *mode, > - struct drm_display_mode *adjusted_mode, > int x, int y, > struct drm_framebuffer *old_fb); > void (*crtc_enable)(struct drm_crtc *crtc); > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/= intel_display.c > index b521198..5a3e231 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3915,15 +3915,15 @@ bool intel_connector_get_hw_state(struct intel_co= nnector *connector) > return encoder->get_hw_state(encoder, &pipe); > } > = > -static bool intel_crtc_mode_fixup(struct drm_crtc *crtc, > - const struct drm_display_mode *mode, > - struct drm_display_mode *adjusted_mode) > +static bool intel_crtc_compute_config(struct drm_crtc *crtc, > + struct intel_crtc_config *pipe_config) > { > struct drm_device *dev =3D crtc->dev; > + struct drm_display_mode *adjusted_mode =3D &pipe_config->adjusted_mode; > = > if (HAS_PCH_SPLIT(dev)) { > /* FDI link clock is fixed at 2.7G */ > - if (mode->clock * 3 > IRONLAKE_FDI_FREQ * 4) > + if (pipe_config->requested_mode.clock * 3 > IRONLAKE_FDI_FREQ * 4) > return false; > } > = > @@ -4609,14 +4609,15 @@ static void intel_set_pipe_timings(struct intel_c= rtc *intel_crtc, > } > = > static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > - struct drm_display_mode *mode, > - struct drm_display_mode *adjusted_mode, > int x, int y, > struct drm_framebuffer *fb) > { > struct drm_device *dev =3D crtc->dev; > struct drm_i915_private *dev_priv =3D dev->dev_private; > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > + struct drm_display_mode *adjusted_mode =3D > + &intel_crtc->config.adjusted_mode; > + struct drm_display_mode *mode =3D &intel_crtc->config.requested_mode; > int pipe =3D intel_crtc->pipe; > int plane =3D intel_crtc->plane; > int refclk, num_connectors =3D 0; > @@ -5579,14 +5580,15 @@ static uint32_t ironlake_compute_dpll(struct inte= l_crtc *intel_crtc, > } > = > static int ironlake_crtc_mode_set(struct drm_crtc *crtc, > - struct drm_display_mode *mode, > - struct drm_display_mode *adjusted_mode, > int x, int y, > struct drm_framebuffer *fb) > { > struct drm_device *dev =3D crtc->dev; > struct drm_i915_private *dev_priv =3D dev->dev_private; > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > + struct drm_display_mode *adjusted_mode =3D > + &intel_crtc->config.adjusted_mode; > + struct drm_display_mode *mode =3D &intel_crtc->config.requested_mode; > int pipe =3D intel_crtc->pipe; > int plane =3D intel_crtc->plane; > int num_connectors =3D 0; > @@ -5745,14 +5747,15 @@ static void haswell_modeset_global_resources(stru= ct drm_device *dev) > } > = > static int haswell_crtc_mode_set(struct drm_crtc *crtc, > - struct drm_display_mode *mode, > - struct drm_display_mode *adjusted_mode, > int x, int y, > struct drm_framebuffer *fb) > { > struct drm_device *dev =3D crtc->dev; > struct drm_i915_private *dev_priv =3D dev->dev_private; > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > + struct drm_display_mode *adjusted_mode =3D > + &intel_crtc->config.adjusted_mode; > + struct drm_display_mode *mode =3D &intel_crtc->config.requested_mode; > int pipe =3D intel_crtc->pipe; > int plane =3D intel_crtc->plane; > int num_connectors =3D 0; > @@ -5829,8 +5832,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *c= rtc, > } > = > static int intel_crtc_mode_set(struct drm_crtc *crtc, > - struct drm_display_mode *mode, > - struct drm_display_mode *adjusted_mode, > int x, int y, > struct drm_framebuffer *fb) > { > @@ -5839,6 +5840,9 @@ static int intel_crtc_mode_set(struct drm_crtc *crt= c, > struct drm_encoder_helper_funcs *encoder_funcs; > struct intel_encoder *encoder; > struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > + struct drm_display_mode *adjusted_mode =3D > + &intel_crtc->config.adjusted_mode; > + struct drm_display_mode *mode =3D &intel_crtc->config.requested_mode; > int pipe =3D intel_crtc->pipe; > int ret; > = > @@ -5849,8 +5853,8 @@ static int intel_crtc_mode_set(struct drm_crtc *crt= c, > = > drm_vblank_pre_modeset(dev, pipe); > = > - ret =3D dev_priv->display.crtc_mode_set(crtc, mode, adjusted_mode, > - x, y, fb); > + ret =3D dev_priv->display.crtc_mode_set(crtc, x, y, fb); > + > drm_vblank_post_modeset(dev, pipe); > = > if (ret !=3D 0) > @@ -7478,19 +7482,24 @@ static void intel_modeset_commit_output_state(str= uct drm_device *dev) > } > } > = > -static struct drm_display_mode * > -intel_modeset_adjusted_mode(struct drm_crtc *crtc, > - struct drm_display_mode *mode) > +static struct intel_crtc_config * > +intel_modeset_pipe_config(struct drm_crtc *crtc, > + struct drm_display_mode *mode) > { > struct drm_device *dev =3D crtc->dev; > - struct drm_display_mode *adjusted_mode; > struct drm_encoder_helper_funcs *encoder_funcs; > struct intel_encoder *encoder; > + struct intel_crtc_config *pipe_config; > = > - adjusted_mode =3D drm_mode_duplicate(dev, mode); > - if (!adjusted_mode) > + pipe_config =3D kzalloc(sizeof(*pipe_config), GFP_KERNEL); > + if (!pipe_config) > return ERR_PTR(-ENOMEM); > = > + drm_mode_copy(&pipe_config->adjusted_mode, mode); > + pipe_config->adjusted_mode.base.id =3D 0; > + drm_mode_copy(&pipe_config->requested_mode, mode); > + pipe_config->requested_mode.base.id =3D 0; drm_mode_copy() preserves the destination mode's id, and since you kzalloc() the whole thing there's no need to zero the ids explicitly. That was pretty much why I suggeted it. The rest looks good to me. Reviewed-by: Ville Syrj=E4l=E4 -- = Ville Syrj=E4l=E4 Intel OTC