Re: [Intel-gfx] [PATCH 6/9] drm/i915: Nuke ironlake_get_initial_plane_config()

[Ville Syrjälä]

On Thu, Oct 12, 2017 at 09:17:40PM +0200, Daniel Vetter wrote:
> On Wed, Oct 11, 2017 at 07:04:52PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> > 
> > The only relevant difference between i9xx_get_initial_plane_config() and
> > ironlake_get_initial_plane_config() is the HSW/BDW TILEOFF handling.
> > Add that to i9xx_get_initial_plane_config() and nuke
> > ironlake_get_initial_plane_config().
> > 
> > Signed-off-by: Ville Syrjälä 
> 
> I'm still kinda wanting some way to test this here with every modeset, but
> still can't come up with something simple
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 79 
> > +++-
> >  1 file changed, 6 insertions(+), 73 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 21160a06ab36..82be2342d1c6 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7490,7 +7490,10 @@ i9xx_get_initial_plane_config(struct intel_crtc 
> > *crtc,
> > fourcc = i9xx_format_to_fourcc(pixel_format);
> > fb->format = drm_format_info(fourcc);
> >  
> > -   if (INTEL_GEN(dev_priv) >= 4) {
> > +   if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > +   offset = I915_READ(DSPOFFSET(plane_id));
> > +   base = I915_READ(DSPSURF(plane_id)) & 0xf000;
> > +   } else if (INTEL_GEN(dev_priv) >= 4) {
> > if (plane_config->tiling)
> > offset = I915_READ(DSPTILEOFF(plane_id));
> > else
> > @@ -8592,76 +8595,6 @@ static void ironlake_get_pfit_config(struct 
> > intel_crtc *crtc,
> > }
> >  }
> >  
> > -static void
> > -ironlake_get_initial_plane_config(struct intel_crtc *crtc,
> > - struct intel_initial_plane_config 
> > *plane_config)
> > -{
> > -   struct drm_device *dev = crtc->base.dev;
> > -   struct drm_i915_private *dev_priv = to_i915(dev);
> > -   struct intel_plane *plane = to_intel_plane(crtc->base.primary);
> > -   enum old_plane_id plane_id = plane->plane;
> > -   enum pipe pipe = crtc->pipe;
> > -   u32 val, base, offset;
> > -   int fourcc, pixel_format;
> > -   unsigned int aligned_height;
> > -   struct drm_framebuffer *fb;
> > -   struct intel_framebuffer *intel_fb;
> > -
> > -   val = I915_READ(DSPCNTR(plane_id));
> > -   if (!(val & DISPLAY_PLANE_ENABLE))
> 
> Hm, should we use plane->get_hw_state to take out this check? At least to
> partially get close to the hw state readout approach we have for
> crtc/encoders/connectors.

I guess we could. We'll still need to read DSPCNTR anyway, but I don't
think reading it twice is a real issue. I'll post a follow up with this.

> 
> Aside form these two notes, patch itself looks good.
> 
> Reviewed-by: Daniel Vetter 
> 
> > -   return;
> > -
> > -   intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
> > -   if (!intel_fb) {
> > -   DRM_DEBUG_KMS("failed to alloc fb\n");
> > -   return;
> > -   }
> > -
> > -   fb = _fb->base;
> > -
> > -   fb->dev = dev;
> > -
> > -   if (INTEL_GEN(dev_priv) >= 4) {
> > -   if (val & DISPPLANE_TILED) {
> > -   plane_config->tiling = I915_TILING_X;
> > -   fb->modifier = I915_FORMAT_MOD_X_TILED;
> > -   }
> > -   }
> > -
> > -   pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
> > -   fourcc = i9xx_format_to_fourcc(pixel_format);
> > -   fb->format = drm_format_info(fourcc);
> > -
> > -   base = I915_READ(DSPSURF(plane_id)) & 0xf000;
> > -   if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > -   offset = I915_READ(DSPOFFSET(plane_id));
> > -   } else {
> > -   if (plane_config->tiling)
> > -   offset = I915_READ(DSPTILEOFF(plane_id));
> > -   else
> > -   offset = I915_READ(DSPLINOFF(plane_id));
> > -   }
> > -   plane_config->base = base;
> > -
> > -   val = I915_READ(PIPESRC(pipe));
> > -   fb->width = ((val >> 16) & 0xfff) + 1;
> > -   fb->height = ((val >> 0) & 0xfff) + 1;
> > -
> > -   val = I915_READ(DSPSTRIDE(plane_id));
> > -   fb->pitches[0] = val & 0xffc0;
> > -
> > -   aligned_height = intel_fb_align_height(fb, 0, fb->height);
> > -
> > -   plane_config->size = fb->pitches[0] * aligned_height;
> > -
> > -   DRM_DEBUG_KMS("%s/%s with fb: size=%dx%d@%d, offset=%x, pitch %d, size 
> > 0x%x\n",
> > - crtc->base.name, plane->base.name, fb->width, fb->height,
> > - fb->format->cpp[0] * 8, base, fb->pitches[0],
> > - plane_config->size);
> > -
> > -   plane_config->fb = intel_fb;
> > -}
> > -
> >  static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
> >  struct intel_crtc_state *pipe_config)
> >  {
> > @@ -14140,7 +14073,7 @@ void intel_init_display_hooks(struct 
> > drm_i915_private *dev_priv)
> > } else if 

Re: [Intel-gfx] [PATCH 6/9] drm/i915: Nuke ironlake_get_initial_plane_config()

[Daniel Vetter]

On Wed, Oct 11, 2017 at 07:04:52PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> The only relevant difference between i9xx_get_initial_plane_config() and
> ironlake_get_initial_plane_config() is the HSW/BDW TILEOFF handling.
> Add that to i9xx_get_initial_plane_config() and nuke
> ironlake_get_initial_plane_config().
> 
> Signed-off-by: Ville Syrjälä 

I'm still kinda wanting some way to test this here with every modeset, but
still can't come up with something simple

> ---
>  drivers/gpu/drm/i915/intel_display.c | 79 
> +++-
>  1 file changed, 6 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 21160a06ab36..82be2342d1c6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7490,7 +7490,10 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
>   fourcc = i9xx_format_to_fourcc(pixel_format);
>   fb->format = drm_format_info(fourcc);
>  
> - if (INTEL_GEN(dev_priv) >= 4) {
> + if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> + offset = I915_READ(DSPOFFSET(plane_id));
> + base = I915_READ(DSPSURF(plane_id)) & 0xf000;
> + } else if (INTEL_GEN(dev_priv) >= 4) {
>   if (plane_config->tiling)
>   offset = I915_READ(DSPTILEOFF(plane_id));
>   else
> @@ -8592,76 +8595,6 @@ static void ironlake_get_pfit_config(struct intel_crtc 
> *crtc,
>   }
>  }
>  
> -static void
> -ironlake_get_initial_plane_config(struct intel_crtc *crtc,
> -   struct intel_initial_plane_config 
> *plane_config)
> -{
> - struct drm_device *dev = crtc->base.dev;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_plane *plane = to_intel_plane(crtc->base.primary);
> - enum old_plane_id plane_id = plane->plane;
> - enum pipe pipe = crtc->pipe;
> - u32 val, base, offset;
> - int fourcc, pixel_format;
> - unsigned int aligned_height;
> - struct drm_framebuffer *fb;
> - struct intel_framebuffer *intel_fb;
> -
> - val = I915_READ(DSPCNTR(plane_id));
> - if (!(val & DISPLAY_PLANE_ENABLE))

Hm, should we use plane->get_hw_state to take out this check? At least to
partially get close to the hw state readout approach we have for
crtc/encoders/connectors.

Aside form these two notes, patch itself looks good.

Reviewed-by: Daniel Vetter 

> - return;
> -
> - intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
> - if (!intel_fb) {
> - DRM_DEBUG_KMS("failed to alloc fb\n");
> - return;
> - }
> -
> - fb = _fb->base;
> -
> - fb->dev = dev;
> -
> - if (INTEL_GEN(dev_priv) >= 4) {
> - if (val & DISPPLANE_TILED) {
> - plane_config->tiling = I915_TILING_X;
> - fb->modifier = I915_FORMAT_MOD_X_TILED;
> - }
> - }
> -
> - pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
> - fourcc = i9xx_format_to_fourcc(pixel_format);
> - fb->format = drm_format_info(fourcc);
> -
> - base = I915_READ(DSPSURF(plane_id)) & 0xf000;
> - if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> - offset = I915_READ(DSPOFFSET(plane_id));
> - } else {
> - if (plane_config->tiling)
> - offset = I915_READ(DSPTILEOFF(plane_id));
> - else
> - offset = I915_READ(DSPLINOFF(plane_id));
> - }
> - plane_config->base = base;
> -
> - val = I915_READ(PIPESRC(pipe));
> - fb->width = ((val >> 16) & 0xfff) + 1;
> - fb->height = ((val >> 0) & 0xfff) + 1;
> -
> - val = I915_READ(DSPSTRIDE(plane_id));
> - fb->pitches[0] = val & 0xffc0;
> -
> - aligned_height = intel_fb_align_height(fb, 0, fb->height);
> -
> - plane_config->size = fb->pitches[0] * aligned_height;
> -
> - DRM_DEBUG_KMS("%s/%s with fb: size=%dx%d@%d, offset=%x, pitch %d, size 
> 0x%x\n",
> -   crtc->base.name, plane->base.name, fb->width, fb->height,
> -   fb->format->cpp[0] * 8, base, fb->pitches[0],
> -   plane_config->size);
> -
> - plane_config->fb = intel_fb;
> -}
> -
>  static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>struct intel_crtc_state *pipe_config)
>  {
> @@ -14140,7 +14073,7 @@ void intel_init_display_hooks(struct drm_i915_private 
> *dev_priv)
>   } else if (HAS_DDI(dev_priv)) {
>   dev_priv->display.get_pipe_config = haswell_get_pipe_config;
>   dev_priv->display.get_initial_plane_config =
> - ironlake_get_initial_plane_config;
> + i9xx_get_initial_plane_config;
>   dev_priv->display.crtc_compute_clock =
>

[Intel-gfx] [PATCH 6/9] drm/i915: Nuke ironlake_get_initial_plane_config()

[Ville Syrjala]

From: Ville Syrjälä 

The only relevant difference between i9xx_get_initial_plane_config() and
ironlake_get_initial_plane_config() is the HSW/BDW TILEOFF handling.
Add that to i9xx_get_initial_plane_config() and nuke
ironlake_get_initial_plane_config().

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/intel_display.c | 79 +++-
 1 file changed, 6 insertions(+), 73 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 21160a06ab36..82be2342d1c6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7490,7 +7490,10 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
fourcc = i9xx_format_to_fourcc(pixel_format);
fb->format = drm_format_info(fourcc);
 
-   if (INTEL_GEN(dev_priv) >= 4) {
+   if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
+   offset = I915_READ(DSPOFFSET(plane_id));
+   base = I915_READ(DSPSURF(plane_id)) & 0xf000;
+   } else if (INTEL_GEN(dev_priv) >= 4) {
if (plane_config->tiling)
offset = I915_READ(DSPTILEOFF(plane_id));
else
@@ -8592,76 +8595,6 @@ static void ironlake_get_pfit_config(struct intel_crtc 
*crtc,
}
 }
 
-static void
-ironlake_get_initial_plane_config(struct intel_crtc *crtc,
- struct intel_initial_plane_config 
*plane_config)
-{
-   struct drm_device *dev = crtc->base.dev;
-   struct drm_i915_private *dev_priv = to_i915(dev);
-   struct intel_plane *plane = to_intel_plane(crtc->base.primary);
-   enum old_plane_id plane_id = plane->plane;
-   enum pipe pipe = crtc->pipe;
-   u32 val, base, offset;
-   int fourcc, pixel_format;
-   unsigned int aligned_height;
-   struct drm_framebuffer *fb;
-   struct intel_framebuffer *intel_fb;
-
-   val = I915_READ(DSPCNTR(plane_id));
-   if (!(val & DISPLAY_PLANE_ENABLE))
-   return;
-
-   intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
-   if (!intel_fb) {
-   DRM_DEBUG_KMS("failed to alloc fb\n");
-   return;
-   }
-
-   fb = _fb->base;
-
-   fb->dev = dev;
-
-   if (INTEL_GEN(dev_priv) >= 4) {
-   if (val & DISPPLANE_TILED) {
-   plane_config->tiling = I915_TILING_X;
-   fb->modifier = I915_FORMAT_MOD_X_TILED;
-   }
-   }
-
-   pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
-   fourcc = i9xx_format_to_fourcc(pixel_format);
-   fb->format = drm_format_info(fourcc);
-
-   base = I915_READ(DSPSURF(plane_id)) & 0xf000;
-   if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
-   offset = I915_READ(DSPOFFSET(plane_id));
-   } else {
-   if (plane_config->tiling)
-   offset = I915_READ(DSPTILEOFF(plane_id));
-   else
-   offset = I915_READ(DSPLINOFF(plane_id));
-   }
-   plane_config->base = base;
-
-   val = I915_READ(PIPESRC(pipe));
-   fb->width = ((val >> 16) & 0xfff) + 1;
-   fb->height = ((val >> 0) & 0xfff) + 1;
-
-   val = I915_READ(DSPSTRIDE(plane_id));
-   fb->pitches[0] = val & 0xffc0;
-
-   aligned_height = intel_fb_align_height(fb, 0, fb->height);
-
-   plane_config->size = fb->pitches[0] * aligned_height;
-
-   DRM_DEBUG_KMS("%s/%s with fb: size=%dx%d@%d, offset=%x, pitch %d, size 
0x%x\n",
- crtc->base.name, plane->base.name, fb->width, fb->height,
- fb->format->cpp[0] * 8, base, fb->pitches[0],
- plane_config->size);
-
-   plane_config->fb = intel_fb;
-}
-
 static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 struct intel_crtc_state *pipe_config)
 {
@@ -14140,7 +14073,7 @@ void intel_init_display_hooks(struct drm_i915_private 
*dev_priv)
} else if (HAS_DDI(dev_priv)) {
dev_priv->display.get_pipe_config = haswell_get_pipe_config;
dev_priv->display.get_initial_plane_config =
-   ironlake_get_initial_plane_config;
+   i9xx_get_initial_plane_config;
dev_priv->display.crtc_compute_clock =
haswell_crtc_compute_clock;
dev_priv->display.crtc_enable = haswell_crtc_enable;
@@ -14148,7 +14081,7 @@ void intel_init_display_hooks(struct drm_i915_private 
*dev_priv)
} else if (HAS_PCH_SPLIT(dev_priv)) {
dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
dev_priv->display.get_initial_plane_config =
-   ironlake_get_initial_plane_config;
+   i9xx_get_initial_plane_config;
dev_priv->display.crtc_compute_clock =