From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 01/35] drm/i915: Add scaled paramater to update_sprite_watermarks() Date: Tue, 30 Jul 2013 21:30:40 +0300 Message-ID: <20130730183040.GX5004@intel.com> References: <1373014667-19484-1-git-send-email-ville.syrjala@linux.intel.com> <1373014667-19484-2-git-send-email-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id 73496E6E6B for ; Tue, 30 Jul 2013 11:30:57 -0700 (PDT) Content-Disposition: inline In-Reply-To: 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: Paulo Zanoni Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, Jul 30, 2013 at 03:26:12PM -0300, Paulo Zanoni wrote: > 2013/7/5 : > > From: Ville Syrj=E4l=E4 > > > > Fro calculating watermarks we want to know whether sprites are > = > "Fro" > = > > scaled. Pass that information to update_sprite_watermarks() so that > > eventually we may do some watermark pre-computing. > = > On this patch you're also renaming some variables from "enable" to > "enabled", but not all of them. You should probably either rename them > all, or none. Example: intel_update_sprite_watermarks definition at > intel_drv.h says "bool enabled", but the implementation inside > intel_pm.c says "bool enable", but there are also other examples. The idea was to use "enabled" consistently, but it seems I messed up. I can fix that up. > = > Besides the styling detail the patch looks correct, so if Daniel/Ville > consider my suggestion is just a bikeshed, Reviewed-by: Paulo Zanoni > . > = > And another bikeshed would be to create variables called "scaled" > inside the update_plane funcs :) You mean "bool scaled =3D crtc_w !=3D src_w || crtc_h !=3D src_h;" or so? Yeah I suppose could make the code a bit easier to parse. > = > > > > Signed-off-by: Ville Syrj=E4l=E4 > > --- > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > drivers/gpu/drm/i915/intel_drv.h | 7 ++++--- > > drivers/gpu/drm/i915/intel_pm.c | 13 +++++++------ > > drivers/gpu/drm/i915/intel_sprite.c | 11 +++++++---- > > 4 files changed, 19 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i91= 5_drv.h > > index fd0f589..99eb980 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -361,7 +361,7 @@ struct drm_i915_display_funcs { > > void (*update_wm)(struct drm_device *dev); > > void (*update_sprite_wm)(struct drm_device *dev, int pipe, > > uint32_t sprite_width, int pixel_size, > > - bool enable); > > + bool enable, bool scaled); > > void (*modeset_global_resources)(struct drm_device *dev); > > /* Returns the active state of the crtc, and if the crtc is act= ive, > > * fills out the pipe-config with the hw state. */ > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/in= tel_drv.h > > index 5dfc1a0..3371ecc 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -353,7 +353,8 @@ struct intel_plane { > > * for the watermark calculations. Currently only Haswell uses = this. > > */ > > struct { > > - bool enable; > > + bool enabled; > > + bool scaled; > > uint8_t bytes_per_pixel; > > uint32_t horiz_pixels; > > } wm; > > @@ -772,8 +773,8 @@ extern void intel_ddi_init(struct drm_device *dev, = enum port port); > > /* For use by IVB LP watermark workaround in intel_sprite.c */ > > extern void intel_update_watermarks(struct drm_device *dev); > > extern void intel_update_sprite_watermarks(struct drm_device *dev, int= pipe, > > - uint32_t sprite_width, > > - int pixel_size, bool enable); > > + uint32_t sprite_width, int p= ixel_size, > > + bool enabled, bool scaled); > > > > extern unsigned long intel_gen4_compute_page_offset(int *x, int *y, > > unsigned int tiling= _mode, > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/int= el_pm.c > > index 7cfd3b7..beca186 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -2388,7 +2388,7 @@ static void hsw_compute_wm_parameters(struct drm_= device *dev, > > pipe =3D intel_plane->pipe; > > p =3D ¶ms[pipe]; > > > > - p->sprite_enabled =3D intel_plane->wm.enable; > > + p->sprite_enabled =3D intel_plane->wm.enabled; > > p->spr_bytes_per_pixel =3D intel_plane->wm.bytes_per_pi= xel; > > p->spr_horiz_pixels =3D intel_plane->wm.horiz_pixels; > > > > @@ -2616,7 +2616,7 @@ static void haswell_update_wm(struct drm_device *= dev) > > > > static void haswell_update_sprite_wm(struct drm_device *dev, int pipe, > > uint32_t sprite_width, int pixel_s= ize, > > - bool enable) > > + bool enabled, bool scaled) > > { > > struct drm_plane *plane; > > > > @@ -2624,7 +2624,8 @@ static void haswell_update_sprite_wm(struct drm_d= evice *dev, int pipe, > > struct intel_plane *intel_plane =3D to_intel_plane(plan= e); > > > > if (intel_plane->pipe =3D=3D pipe) { > > - intel_plane->wm.enable =3D enable; > > + intel_plane->wm.enabled =3D enabled; > > + intel_plane->wm.scaled =3D scaled; > > intel_plane->wm.horiz_pixels =3D sprite_width += 1; > > intel_plane->wm.bytes_per_pixel =3D pixel_size; > > break; > > @@ -2712,7 +2713,7 @@ sandybridge_compute_sprite_srwm(struct drm_device= *dev, int plane, > > > > static void sandybridge_update_sprite_wm(struct drm_device *dev, int p= ipe, > > uint32_t sprite_width, int pix= el_size, > > - bool enable) > > + bool enable, bool scaled) > > { > > struct drm_i915_private *dev_priv =3D dev->dev_private; > > int latency =3D SNB_READ_WM0_LATENCY() * 100; /* In unit 0.= 1us */ > > @@ -2835,13 +2836,13 @@ void intel_update_watermarks(struct drm_device = *dev) > > > > void intel_update_sprite_watermarks(struct drm_device *dev, int pipe, > > uint32_t sprite_width, int pixel_si= ze, > > - bool enable) > > + bool enable, bool scaled) > > { > > struct drm_i915_private *dev_priv =3D dev->dev_private; > > > > if (dev_priv->display.update_sprite_wm) > > dev_priv->display.update_sprite_wm(dev, pipe, sprite_wi= dth, > > - pixel_size, enable); > > + pixel_size, enable, = scaled); > > } > > > > static struct drm_i915_gem_object * > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915= /intel_sprite.c > > index 1fa5612..5a1f3fd 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -114,7 +114,8 @@ vlv_update_plane(struct drm_plane *dplane, struct d= rm_framebuffer *fb, > > crtc_w--; > > crtc_h--; > > > > - intel_update_sprite_watermarks(dev, pipe, crtc_w, pixel_size, t= rue); > > + intel_update_sprite_watermarks(dev, pipe, crtc_w, pixel_size, t= rue, > > + src_w !=3D crtc_w || src_h !=3D = crtc_h); > > > > I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]); > > I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x); > > @@ -268,7 +269,8 @@ ivb_update_plane(struct drm_plane *plane, struct dr= m_framebuffer *fb, > > crtc_w--; > > crtc_h--; > > > > - intel_update_sprite_watermarks(dev, pipe, crtc_w, pixel_size, t= rue); > > + intel_update_sprite_watermarks(dev, pipe, crtc_w, pixel_size, t= rue, > > + src_w !=3D crtc_w || src_h !=3D = crtc_h); > > > > /* > > * IVB workaround: must disable low power watermarks for at lea= st > > @@ -335,7 +337,7 @@ ivb_disable_plane(struct drm_plane *plane) > > > > dev_priv->sprite_scaling_enabled &=3D ~(1 << pipe); > > > > - intel_update_sprite_watermarks(dev, pipe, 0, 0, false); > > + intel_update_sprite_watermarks(dev, pipe, 0, 0, false, false); > > > > /* potentially re-enable LP watermarks */ > > if (scaling_was_enabled && !dev_priv->sprite_scaling_enabled) > > @@ -455,7 +457,8 @@ ilk_update_plane(struct drm_plane *plane, struct dr= m_framebuffer *fb, > > crtc_w--; > > crtc_h--; > > > > - intel_update_sprite_watermarks(dev, pipe, crtc_w, pixel_size, t= rue); > > + intel_update_sprite_watermarks(dev, pipe, crtc_w, pixel_size, t= rue, > > + src_w !=3D crtc_w || src_h !=3D = crtc_h); > > > > dvsscale =3D 0; > > if (IS_GEN5(dev) || crtc_w !=3D src_w || crtc_h !=3D src_h) > > -- > > 1.8.1.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > = > = > = > -- = > Paulo Zanoni -- = Ville Syrj=E4l=E4 Intel OTC