| Message ID | 1481828705.2548.156.camel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 15, 2016 at 05:05:05PM -0200, Paulo Zanoni wrote: > Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala@linux.intel.com > escreveu: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > With plane->plane now purely reserved for the primary planes, let's > > not even populate it for cursors and sprites. Let's switch the type > > to enum plane as well since it's no longer being abused for anything > > else. > > Since it's a primary plane thing, I think it makes more sense to just > leave it at struct intel_crtc instead of having a field that's unused > and doesn't make sense in some cases. Primary plane aren't really primary planes on old platforms though. That's just a software concept that has no bearing on the hardware. > The crtc struct already includes > some fields that are specific to primary planes, so I think it's a good > place. Or we could create a new class: struct intel_primary_plane { > struct intel_plane base; enum plane legacy_plane }; > > Following is a patch suggestion (probably impossible to apply due to my > editor breaking long lines). Any arguments against it? > > --8<---------------------------------------------------------- > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index bc1af87..c54b1a7 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15065,14 +15065,6 @@ intel_primary_plane_create(struct > drm_i915_private *dev_priv, enum pipe pipe) > state->scaler_id = -1; > } > primary->pipe = pipe; > - /* > - * On gen2/3 only plane A can do FBC, but the panel fitter and > LVDS > - * port is hooked to pipe B. Hence we want plane A feeding > pipe B. > - */ > - if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4) > - primary->plane = (enum plane) !pipe; > - else > - primary->plane = (enum plane) pipe; > primary->id = PLANE_PRIMARY; > primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe); > primary->check_plane = intel_check_primary_plane; > @@ -15120,7 +15112,7 @@ intel_primary_plane_create(struct > drm_i915_private *dev_priv, enum pipe pipe) > 0, &intel_plane_funcs, > intel_primary_formats, > num_formats, > DRM_PLANE_TYPE_PRIMARY, > - "plane %c", > plane_name(primary->plane)); > + "plane %c", > pipe_name(pipe)); /* FIXME */ > if (ret) > goto fail; > > @@ -15272,7 +15264,6 @@ intel_cursor_plane_create(struct > drm_i915_private *dev_priv, enum pipe pipe) > cursor->can_scale = false; > cursor->max_downscale = 1; > cursor->pipe = pipe; > - cursor->plane = pipe; > cursor->id = PLANE_CURSOR; > cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe); > cursor->check_plane = intel_check_cursor_plane; > @@ -15390,7 +15381,14 @@ static int intel_crtc_init(struct > drm_i915_private *dev_priv, enum pipe pipe) > goto fail; > > intel_crtc->pipe = pipe; > - intel_crtc->plane = primary->plane; > + /* > + * On gen2/3 only plane A can do FBC, but the panel fitter and > LVDS > + * port is hooked to pipe B. Hence we want plane A feeding > pipe B. > + */ > + if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4) > + intel_crtc->plane = (enum plane) !pipe; > + else > + intel_crtc->plane = (enum plane) pipe; > > intel_crtc->cursor_base = ~0; > intel_crtc->cursor_cntl = ~0; > @@ -16866,11 +16864,11 @@ void i915_redisable_vga(struct > drm_i915_private *dev_priv) > intel_display_power_put(dev_priv, POWER_DOMAIN_VGA); > } > > -static bool primary_get_hw_state(struct intel_plane *plane) > +static bool primary_get_hw_state(struct intel_crtc *crtc) > { > - struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > > - return I915_READ(DSPCNTR(plane->plane)) & > DISPLAY_PLANE_ENABLE; > + return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE; > } > > /* FIXME read out full plane state for all planes */ > @@ -16880,8 +16878,7 @@ static void readout_plane_state(struct > intel_crtc *crtc) > struct intel_plane_state *plane_state = > to_intel_plane_state(primary->state); > > - plane_state->base.visible = crtc->active && > - primary_get_hw_state(to_intel_plane(primary)); > + plane_state->base.visible = crtc->active && > primary_get_hw_state(crtc); > > if (plane_state->base.visible) > crtc->base.state->plane_mask |= 1 << > drm_plane_index(primary); > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 362f698..b2bdd49 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -767,7 +767,6 @@ struct intel_plane_wm_parameters { > > struct intel_plane { > struct drm_plane base; > - u8 plane; > enum plane_id id; > enum pipe pipe; > bool can_scale; > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > b/drivers/gpu/drm/i915/intel_sprite.c > index 63154a5..1044095 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct drm_i915_private > *dev_priv, > } > > intel_plane->pipe = pipe; > - intel_plane->plane = plane; > intel_plane->id = PLANE_SPRITE0 + plane; > intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER_SPRITE(pipe, > plane); > intel_plane->check_plane = intel_check_sprite_plane; > > --8<---------------------------------------------------------- > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 1 - > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > drivers/gpu/drm/i915/intel_sprite.c | 1 - > > 3 files changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index b6d5d5e5cc99..f180f14fcf3a 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -15188,7 +15188,6 @@ intel_cursor_plane_create(struct > > drm_i915_private *dev_priv, enum pipe pipe) > > cursor->can_scale = false; > > cursor->max_downscale = 1; > > cursor->pipe = pipe; > > - cursor->plane = pipe; > > cursor->id = PLANE_CURSOR; > > cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe); > > cursor->check_plane = intel_check_cursor_plane; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index c1b245853ba9..d14718e09911 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -767,7 +767,7 @@ struct intel_plane_wm_parameters { > > > > struct intel_plane { > > struct drm_plane base; > > - u8 plane; > > + enum plane plane; > > enum plane_id id; > > enum pipe pipe; > > bool can_scale; > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > b/drivers/gpu/drm/i915/intel_sprite.c > > index 63154a5a9305..1044095d0084 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct > > drm_i915_private *dev_priv, > > } > > > > intel_plane->pipe = pipe; > > - intel_plane->plane = plane; > > intel_plane->id = PLANE_SPRITE0 + plane; > > intel_plane->frontbuffer_bit = > > INTEL_FRONTBUFFER_SPRITE(pipe, plane); > > intel_plane->check_plane = intel_check_sprite_plane;
Em Qui, 2016-12-15 às 21:11 +0200, Ville Syrjälä escreveu: > On Thu, Dec 15, 2016 at 05:05:05PM -0200, Paulo Zanoni wrote: > > > > Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala@linux.intel.com > > escreveu: > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > With plane->plane now purely reserved for the primary planes, > > > let's > > > not even populate it for cursors and sprites. Let's switch the > > > type > > > to enum plane as well since it's no longer being abused for > > > anything > > > else. > > > > Since it's a primary plane thing, I think it makes more sense to > > just > > leave it at struct intel_crtc instead of having a field that's > > unused > > and doesn't make sense in some cases. > > Primary plane aren't really primary planes on old platforms though. > That's just a software concept that has no bearing on the hardware. Please explain more since that's not my understanding. I just opened the gen 2 and 3 spec docs and they do contain tons of "primary plane" references. I know that the hardware is a little more flexible there, but as far as I understand we don't even implement that. Besides, both our driver and our API do have the concept of a primary plane, so it makes sense for the code to be organized that way, IMHO. Besides, I think our code organizations should always better fit the new hardware, since otherwise we'll make i915.ko development even harder than it is for new contributors/employees. (And when I see those specs and realize how different the HW was, then I remember that in order to explain to the new people why some things are the way they are in our code I have to explain how the HW was 10 years ago, I start thinking again that maybe we should just have i915- old.ko and i915-new.ko, since either we make our abstractions/design fit one thing or we make it fit another...) > > > > > The crtc struct already includes > > some fields that are specific to primary planes, so I think it's a > > good > > place. Or we could create a new class: struct intel_primary_plane { > > struct intel_plane base; enum plane legacy_plane }; > > > > Following is a patch suggestion (probably impossible to apply due > > to my > > editor breaking long lines). Any arguments against it? > > > > --8<---------------------------------------------------------- > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index bc1af87..c54b1a7 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -15065,14 +15065,6 @@ intel_primary_plane_create(struct > > drm_i915_private *dev_priv, enum pipe pipe) > > state->scaler_id = -1; > > } > > primary->pipe = pipe; > > - /* > > - * On gen2/3 only plane A can do FBC, but the panel fitter > > and > > LVDS > > - * port is hooked to pipe B. Hence we want plane A feeding > > pipe B. > > - */ > > - if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4) > > - primary->plane = (enum plane) !pipe; > > - else > > - primary->plane = (enum plane) pipe; > > primary->id = PLANE_PRIMARY; > > primary->frontbuffer_bit = > > INTEL_FRONTBUFFER_PRIMARY(pipe); > > primary->check_plane = intel_check_primary_plane; > > @@ -15120,7 +15112,7 @@ intel_primary_plane_create(struct > > drm_i915_private *dev_priv, enum pipe pipe) > > 0, > > &intel_plane_funcs, > > intel_primary_forma > > ts, > > num_formats, > > DRM_PLANE_TYPE_PRIM > > ARY, > > - "plane %c", > > plane_name(primary->plane)); > > + "plane %c", > > pipe_name(pipe)); /* FIXME */ > > if (ret) > > goto fail; > > > > @@ -15272,7 +15264,6 @@ intel_cursor_plane_create(struct > > drm_i915_private *dev_priv, enum pipe pipe) > > cursor->can_scale = false; > > cursor->max_downscale = 1; > > cursor->pipe = pipe; > > - cursor->plane = pipe; > > cursor->id = PLANE_CURSOR; > > cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe); > > cursor->check_plane = intel_check_cursor_plane; > > @@ -15390,7 +15381,14 @@ static int intel_crtc_init(struct > > drm_i915_private *dev_priv, enum pipe pipe) > > goto fail; > > > > intel_crtc->pipe = pipe; > > - intel_crtc->plane = primary->plane; > > + /* > > + * On gen2/3 only plane A can do FBC, but the panel fitter > > and > > LVDS > > + * port is hooked to pipe B. Hence we want plane A feeding > > pipe B. > > + */ > > + if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4) > > + intel_crtc->plane = (enum plane) !pipe; > > + else > > + intel_crtc->plane = (enum plane) pipe; > > > > intel_crtc->cursor_base = ~0; > > intel_crtc->cursor_cntl = ~0; > > @@ -16866,11 +16864,11 @@ void i915_redisable_vga(struct > > drm_i915_private *dev_priv) > > intel_display_power_put(dev_priv, POWER_DOMAIN_VGA); > > } > > > > -static bool primary_get_hw_state(struct intel_plane *plane) > > +static bool primary_get_hw_state(struct intel_crtc *crtc) > > { > > - struct drm_i915_private *dev_priv = to_i915(plane- > > >base.dev); > > + struct drm_i915_private *dev_priv = to_i915(crtc- > > >base.dev); > > > > - return I915_READ(DSPCNTR(plane->plane)) & > > DISPLAY_PLANE_ENABLE; > > + return I915_READ(DSPCNTR(crtc->plane)) & > > DISPLAY_PLANE_ENABLE; > > } > > > > /* FIXME read out full plane state for all planes */ > > @@ -16880,8 +16878,7 @@ static void readout_plane_state(struct > > intel_crtc *crtc) > > struct intel_plane_state *plane_state = > > to_intel_plane_state(primary->state); > > > > - plane_state->base.visible = crtc->active && > > - primary_get_hw_state(to_intel_plane(primary)); > > + plane_state->base.visible = crtc->active && > > primary_get_hw_state(crtc); > > > > if (plane_state->base.visible) > > crtc->base.state->plane_mask |= 1 << > > drm_plane_index(primary); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 362f698..b2bdd49 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -767,7 +767,6 @@ struct intel_plane_wm_parameters { > > > > struct intel_plane { > > struct drm_plane base; > > - u8 plane; > > enum plane_id id; > > enum pipe pipe; > > bool can_scale; > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > b/drivers/gpu/drm/i915/intel_sprite.c > > index 63154a5..1044095 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct > > drm_i915_private > > *dev_priv, > > } > > > > intel_plane->pipe = pipe; > > - intel_plane->plane = plane; > > intel_plane->id = PLANE_SPRITE0 + plane; > > intel_plane->frontbuffer_bit = > > INTEL_FRONTBUFFER_SPRITE(pipe, > > plane); > > intel_plane->check_plane = intel_check_sprite_plane; > > > > --8<---------------------------------------------------------- > > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 1 - > > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > > drivers/gpu/drm/i915/intel_sprite.c | 1 - > > > 3 files changed, 1 insertion(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index b6d5d5e5cc99..f180f14fcf3a 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -15188,7 +15188,6 @@ intel_cursor_plane_create(struct > > > drm_i915_private *dev_priv, enum pipe pipe) > > > cursor->can_scale = false; > > > cursor->max_downscale = 1; > > > cursor->pipe = pipe; > > > - cursor->plane = pipe; > > > cursor->id = PLANE_CURSOR; > > > cursor->frontbuffer_bit = > > > INTEL_FRONTBUFFER_CURSOR(pipe); > > > cursor->check_plane = intel_check_cursor_plane; > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > b/drivers/gpu/drm/i915/intel_drv.h > > > index c1b245853ba9..d14718e09911 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -767,7 +767,7 @@ struct intel_plane_wm_parameters { > > > > > > struct intel_plane { > > > struct drm_plane base; > > > - u8 plane; > > > + enum plane plane; > > > enum plane_id id; > > > enum pipe pipe; > > > bool can_scale; > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > index 63154a5a9305..1044095d0084 100644 > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct > > > drm_i915_private *dev_priv, > > > } > > > > > > intel_plane->pipe = pipe; > > > - intel_plane->plane = plane; > > > intel_plane->id = PLANE_SPRITE0 + plane; > > > intel_plane->frontbuffer_bit = > > > INTEL_FRONTBUFFER_SPRITE(pipe, plane); > > > intel_plane->check_plane = intel_check_sprite_plane; >
On Thu, Dec 15, 2016 at 05:50:02PM -0200, Paulo Zanoni wrote: > Em Qui, 2016-12-15 às 21:11 +0200, Ville Syrjälä escreveu: > > On Thu, Dec 15, 2016 at 05:05:05PM -0200, Paulo Zanoni wrote: > > > > > > Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala@linux.intel.com > > > escreveu: > > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > With plane->plane now purely reserved for the primary planes, > > > > let's > > > > not even populate it for cursors and sprites. Let's switch the > > > > type > > > > to enum plane as well since it's no longer being abused for > > > > anything > > > > else. > > > > > > Since it's a primary plane thing, I think it makes more sense to > > > just > > > leave it at struct intel_crtc instead of having a field that's > > > unused > > > and doesn't make sense in some cases. > > > > Primary plane aren't really primary planes on old platforms though. > > That's just a software concept that has no bearing on the hardware. > > Please explain more since that's not my understanding. I just opened > the gen 2 and 3 spec docs and they do contain tons of "primary plane" > references. I know that the hardware is a little more flexible there, Yep. Planes can be moved to any pipe in general. > but as far as I understand we don't even implement that. Not at the moment. But I have plans... > Besides, both > our driver and our API do have the concept of a primary plane, so it > makes sense for the code to be organized that way, IMHO. A plane is a plane, a pipe is a pipe. IMO you're trying to make it more confusing by mixing up the two. > Besides, I > think our code organizations should always better fit the new hardware, > since otherwise we'll make i915.ko development even harder than it is > for new contributors/employees. If you don't mix planes and pipes there can't be any confusion. > > (And when I see those specs and realize how different the HW was, then > I remember that in order to explain to the new people why some things > are the way they are in our code I have to explain how the HW was 10 > years ago, I start thinking again that maybe we should just have i915- > old.ko and i915-new.ko, since either we make our abstractions/design > fit one thing or we make it fit another...) From the register POV hw was almost identical up to BDW at least. It's just a few bits (the pipe selector) that vanished from the registers. SKL+ is a little more different but still the registers even live in the same address. Also given what recent history has shown us, I wouldn't be at all surprised if we would go back to a flexible plane<->pipe assignment in the hardware at some point in the future. A lot of the other features that were present in old hardware has been making a comeback in recent years. > > > > > > > > > The crtc struct already includes > > > some fields that are specific to primary planes, so I think it's a > > > good > > > place. Or we could create a new class: struct intel_primary_plane { > > > struct intel_plane base; enum plane legacy_plane }; > > > > > > Following is a patch suggestion (probably impossible to apply due > > > to my > > > editor breaking long lines). Any arguments against it? > > > > > > --8<---------------------------------------------------------- > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index bc1af87..c54b1a7 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -15065,14 +15065,6 @@ intel_primary_plane_create(struct > > > drm_i915_private *dev_priv, enum pipe pipe) > > > state->scaler_id = -1; > > > } > > > primary->pipe = pipe; > > > - /* > > > - * On gen2/3 only plane A can do FBC, but the panel fitter > > > and > > > LVDS > > > - * port is hooked to pipe B. Hence we want plane A feeding > > > pipe B. > > > - */ > > > - if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4) > > > - primary->plane = (enum plane) !pipe; > > > - else > > > - primary->plane = (enum plane) pipe; > > > primary->id = PLANE_PRIMARY; > > > primary->frontbuffer_bit = > > > INTEL_FRONTBUFFER_PRIMARY(pipe); > > > primary->check_plane = intel_check_primary_plane; > > > @@ -15120,7 +15112,7 @@ intel_primary_plane_create(struct > > > drm_i915_private *dev_priv, enum pipe pipe) > > > 0, > > > &intel_plane_funcs, > > > intel_primary_forma > > > ts, > > > num_formats, > > > DRM_PLANE_TYPE_PRIM > > > ARY, > > > - "plane %c", > > > plane_name(primary->plane)); > > > + "plane %c", > > > pipe_name(pipe)); /* FIXME */ > > > if (ret) > > > goto fail; > > > > > > @@ -15272,7 +15264,6 @@ intel_cursor_plane_create(struct > > > drm_i915_private *dev_priv, enum pipe pipe) > > > cursor->can_scale = false; > > > cursor->max_downscale = 1; > > > cursor->pipe = pipe; > > > - cursor->plane = pipe; > > > cursor->id = PLANE_CURSOR; > > > cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe); > > > cursor->check_plane = intel_check_cursor_plane; > > > @@ -15390,7 +15381,14 @@ static int intel_crtc_init(struct > > > drm_i915_private *dev_priv, enum pipe pipe) > > > goto fail; > > > > > > intel_crtc->pipe = pipe; > > > - intel_crtc->plane = primary->plane; > > > + /* > > > + * On gen2/3 only plane A can do FBC, but the panel fitter > > > and > > > LVDS > > > + * port is hooked to pipe B. Hence we want plane A feeding > > > pipe B. > > > + */ > > > + if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4) > > > + intel_crtc->plane = (enum plane) !pipe; > > > + else > > > + intel_crtc->plane = (enum plane) pipe; > > > > > > intel_crtc->cursor_base = ~0; > > > intel_crtc->cursor_cntl = ~0; > > > @@ -16866,11 +16864,11 @@ void i915_redisable_vga(struct > > > drm_i915_private *dev_priv) > > > intel_display_power_put(dev_priv, POWER_DOMAIN_VGA); > > > } > > > > > > -static bool primary_get_hw_state(struct intel_plane *plane) > > > +static bool primary_get_hw_state(struct intel_crtc *crtc) > > > { > > > - struct drm_i915_private *dev_priv = to_i915(plane- > > > >base.dev); > > > + struct drm_i915_private *dev_priv = to_i915(crtc- > > > >base.dev); > > > > > > - return I915_READ(DSPCNTR(plane->plane)) & > > > DISPLAY_PLANE_ENABLE; > > > + return I915_READ(DSPCNTR(crtc->plane)) & > > > DISPLAY_PLANE_ENABLE; > > > } > > > > > > /* FIXME read out full plane state for all planes */ > > > @@ -16880,8 +16878,7 @@ static void readout_plane_state(struct > > > intel_crtc *crtc) > > > struct intel_plane_state *plane_state = > > > to_intel_plane_state(primary->state); > > > > > > - plane_state->base.visible = crtc->active && > > > - primary_get_hw_state(to_intel_plane(primary)); > > > + plane_state->base.visible = crtc->active && > > > primary_get_hw_state(crtc); > > > > > > if (plane_state->base.visible) > > > crtc->base.state->plane_mask |= 1 << > > > drm_plane_index(primary); > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > b/drivers/gpu/drm/i915/intel_drv.h > > > index 362f698..b2bdd49 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -767,7 +767,6 @@ struct intel_plane_wm_parameters { > > > > > > struct intel_plane { > > > struct drm_plane base; > > > - u8 plane; > > > enum plane_id id; > > > enum pipe pipe; > > > bool can_scale; > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > index 63154a5..1044095 100644 > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct > > > drm_i915_private > > > *dev_priv, > > > } > > > > > > intel_plane->pipe = pipe; > > > - intel_plane->plane = plane; > > > intel_plane->id = PLANE_SPRITE0 + plane; > > > intel_plane->frontbuffer_bit = > > > INTEL_FRONTBUFFER_SPRITE(pipe, > > > plane); > > > intel_plane->check_plane = intel_check_sprite_plane; > > > > > > --8<---------------------------------------------------------- > > > > > > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 1 - > > > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > > > drivers/gpu/drm/i915/intel_sprite.c | 1 - > > > > 3 files changed, 1 insertion(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index b6d5d5e5cc99..f180f14fcf3a 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -15188,7 +15188,6 @@ intel_cursor_plane_create(struct > > > > drm_i915_private *dev_priv, enum pipe pipe) > > > > cursor->can_scale = false; > > > > cursor->max_downscale = 1; > > > > cursor->pipe = pipe; > > > > - cursor->plane = pipe; > > > > cursor->id = PLANE_CURSOR; > > > > cursor->frontbuffer_bit = > > > > INTEL_FRONTBUFFER_CURSOR(pipe); > > > > cursor->check_plane = intel_check_cursor_plane; > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > index c1b245853ba9..d14718e09911 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -767,7 +767,7 @@ struct intel_plane_wm_parameters { > > > > > > > > struct intel_plane { > > > > struct drm_plane base; > > > > - u8 plane; > > > > + enum plane plane; > > > > enum plane_id id; > > > > enum pipe pipe; > > > > bool can_scale; > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > > index 63154a5a9305..1044095d0084 100644 > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct > > > > drm_i915_private *dev_priv, > > > > } > > > > > > > > intel_plane->pipe = pipe; > > > > - intel_plane->plane = plane; > > > > intel_plane->id = PLANE_SPRITE0 + plane; > > > > intel_plane->frontbuffer_bit = > > > > INTEL_FRONTBUFFER_SPRITE(pipe, plane); > > > > intel_plane->check_plane = intel_check_sprite_plane; > >
Em Qui, 2016-12-15 às 21:59 +0200, Ville Syrjälä escreveu: > On Thu, Dec 15, 2016 at 05:50:02PM -0200, Paulo Zanoni wrote: > > > > Em Qui, 2016-12-15 às 21:11 +0200, Ville Syrjälä escreveu: > > > > > > On Thu, Dec 15, 2016 at 05:05:05PM -0200, Paulo Zanoni wrote: > > > > > > > > > > > > Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala@linux.intel.co > > > > m > > > > escreveu: > > > > > > > > > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > With plane->plane now purely reserved for the primary planes, > > > > > let's > > > > > not even populate it for cursors and sprites. Let's switch > > > > > the > > > > > type > > > > > to enum plane as well since it's no longer being abused for > > > > > anything > > > > > else. > > > > > > > > Since it's a primary plane thing, I think it makes more sense > > > > to > > > > just > > > > leave it at struct intel_crtc instead of having a field that's > > > > unused > > > > and doesn't make sense in some cases. > > > > > > Primary plane aren't really primary planes on old platforms > > > though. > > > That's just a software concept that has no bearing on the > > > hardware. > > > > Please explain more since that's not my understanding. I just > > opened > > the gen 2 and 3 spec docs and they do contain tons of "primary > > plane" > > references. I know that the hardware is a little more flexible > > there, > > Yep. Planes can be moved to any pipe in general. > > > > > but as far as I understand we don't even implement that. > > Not at the moment. But I have plans... > > > > > Besides, both > > our driver and our API do have the concept of a primary plane, so > > it > > makes sense for the code to be organized that way, IMHO. > > A plane is a plane, a pipe is a pipe. IMO you're trying to make it > more confusing by mixing up the two. I think that goes against your original argument to remove the plane- >plane assignment for everything except for primary planes. Anyway, your argumentation is very short and dismissive, it's hard to understand what exactly you are thinking and try to move the discussion forward. I suppose we'll just have to agree to disagree here. In one solution we create an inconsistency where the plane struct has a field that's only applicable and can only be used to some specific planes (even though "every plane is a plane"). In the other solution we create the inconsistency where the crtc<->plane connection is assigned in the way not chosen by the hardware and this may be a problem if we ever get to implement the plane assignment flexibility that's allowed by the hardware. Maybe having a 3rd opinion here would help. > > > > > Besides, I > > think our code organizations should always better fit the new > > hardware, > > since otherwise we'll make i915.ko development even harder than it > > is > > for new contributors/employees. > > If you don't mix planes and pipes there can't be any confusion. > > > > > > > (And when I see those specs and realize how different the HW was, > > then > > I remember that in order to explain to the new people why some > > things > > are the way they are in our code I have to explain how the HW was > > 10 > > years ago, I start thinking again that maybe we should just have > > i915- > > old.ko and i915-new.ko, since either we make our > > abstractions/design > > fit one thing or we make it fit another...) > > From the register POV hw was almost identical up to BDW at least. > It's > just a few bits (the pipe selector) that vanished from the registers. > SKL+ is a little more different but still the registers even live in > the > same address. > > Also given what recent history has shown us, I wouldn't be at all > surprised if we would go back to a flexible plane<->pipe assignment > in the hardware at some point in the future. A lot of the other > features that were present in old hardware has been making a comeback > in recent years. > > > > > > > > > > > > > > > > > > > > > > > The crtc struct already includes > > > > some fields that are specific to primary planes, so I think > > > > it's a > > > > good > > > > place. Or we could create a new class: struct > > > > intel_primary_plane { > > > > struct intel_plane base; enum plane legacy_plane }; > > > > > > > > Following is a patch suggestion (probably impossible to apply > > > > due > > > > to my > > > > editor breaking long lines). Any arguments against it? > > > > > > > > --8<---------------------------------------------------------- > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index bc1af87..c54b1a7 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -15065,14 +15065,6 @@ intel_primary_plane_create(struct > > > > drm_i915_private *dev_priv, enum pipe pipe) > > > > state->scaler_id = -1; > > > > } > > > > primary->pipe = pipe; > > > > - /* > > > > - * On gen2/3 only plane A can do FBC, but the panel > > > > fitter > > > > and > > > > LVDS > > > > - * port is hooked to pipe B. Hence we want plane A > > > > feeding > > > > pipe B. > > > > - */ > > > > - if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4) > > > > - primary->plane = (enum plane) !pipe; > > > > - else > > > > - primary->plane = (enum plane) pipe; > > > > primary->id = PLANE_PRIMARY; > > > > primary->frontbuffer_bit = > > > > INTEL_FRONTBUFFER_PRIMARY(pipe); > > > > primary->check_plane = intel_check_primary_plane; > > > > @@ -15120,7 +15112,7 @@ intel_primary_plane_create(struct > > > > drm_i915_private *dev_priv, enum pipe pipe) > > > > 0, > > > > &intel_plane_funcs, > > > > intel_primary_f > > > > orma > > > > ts, > > > > num_formats, > > > > DRM_PLANE_TYPE_ > > > > PRIM > > > > ARY, > > > > - "plane %c", > > > > plane_name(primary->plane)); > > > > + "plane %c", > > > > pipe_name(pipe)); /* FIXME */ > > > > if (ret) > > > > goto fail; > > > > > > > > @@ -15272,7 +15264,6 @@ intel_cursor_plane_create(struct > > > > drm_i915_private *dev_priv, enum pipe pipe) > > > > cursor->can_scale = false; > > > > cursor->max_downscale = 1; > > > > cursor->pipe = pipe; > > > > - cursor->plane = pipe; > > > > cursor->id = PLANE_CURSOR; > > > > cursor->frontbuffer_bit = > > > > INTEL_FRONTBUFFER_CURSOR(pipe); > > > > cursor->check_plane = intel_check_cursor_plane; > > > > @@ -15390,7 +15381,14 @@ static int intel_crtc_init(struct > > > > drm_i915_private *dev_priv, enum pipe pipe) > > > > goto fail; > > > > > > > > intel_crtc->pipe = pipe; > > > > - intel_crtc->plane = primary->plane; > > > > + /* > > > > + * On gen2/3 only plane A can do FBC, but the panel > > > > fitter > > > > and > > > > LVDS > > > > + * port is hooked to pipe B. Hence we want plane A > > > > feeding > > > > pipe B. > > > > + */ > > > > + if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4) > > > > + intel_crtc->plane = (enum plane) !pipe; > > > > + else > > > > + intel_crtc->plane = (enum plane) pipe; > > > > > > > > intel_crtc->cursor_base = ~0; > > > > intel_crtc->cursor_cntl = ~0; > > > > @@ -16866,11 +16864,11 @@ void i915_redisable_vga(struct > > > > drm_i915_private *dev_priv) > > > > intel_display_power_put(dev_priv, POWER_DOMAIN_VGA); > > > > } > > > > > > > > -static bool primary_get_hw_state(struct intel_plane *plane) > > > > +static bool primary_get_hw_state(struct intel_crtc *crtc) > > > > { > > > > - struct drm_i915_private *dev_priv = to_i915(plane- > > > > > > > > > > base.dev); > > > > + struct drm_i915_private *dev_priv = to_i915(crtc- > > > > > > > > > > base.dev); > > > > > > > > - return I915_READ(DSPCNTR(plane->plane)) & > > > > DISPLAY_PLANE_ENABLE; > > > > + return I915_READ(DSPCNTR(crtc->plane)) & > > > > DISPLAY_PLANE_ENABLE; > > > > } > > > > > > > > /* FIXME read out full plane state for all planes */ > > > > @@ -16880,8 +16878,7 @@ static void readout_plane_state(struct > > > > intel_crtc *crtc) > > > > struct intel_plane_state *plane_state = > > > > to_intel_plane_state(primary->state); > > > > > > > > - plane_state->base.visible = crtc->active && > > > > - primary_get_hw_state(to_intel_plane(primary)); > > > > + plane_state->base.visible = crtc->active && > > > > primary_get_hw_state(crtc); > > > > > > > > if (plane_state->base.visible) > > > > crtc->base.state->plane_mask |= 1 << > > > > drm_plane_index(primary); > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > index 362f698..b2bdd49 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -767,7 +767,6 @@ struct intel_plane_wm_parameters { > > > > > > > > struct intel_plane { > > > > struct drm_plane base; > > > > - u8 plane; > > > > enum plane_id id; > > > > enum pipe pipe; > > > > bool can_scale; > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > > index 63154a5..1044095 100644 > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct > > > > drm_i915_private > > > > *dev_priv, > > > > } > > > > > > > > intel_plane->pipe = pipe; > > > > - intel_plane->plane = plane; > > > > intel_plane->id = PLANE_SPRITE0 + plane; > > > > intel_plane->frontbuffer_bit = > > > > INTEL_FRONTBUFFER_SPRITE(pipe, > > > > plane); > > > > intel_plane->check_plane = intel_check_sprite_plane; > > > > > > > > --8<---------------------------------------------------------- > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_display.c | 1 - > > > > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > > > > drivers/gpu/drm/i915/intel_sprite.c | 1 - > > > > > 3 files changed, 1 insertion(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > > index b6d5d5e5cc99..f180f14fcf3a 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > @@ -15188,7 +15188,6 @@ intel_cursor_plane_create(struct > > > > > drm_i915_private *dev_priv, enum pipe pipe) > > > > > cursor->can_scale = false; > > > > > cursor->max_downscale = 1; > > > > > cursor->pipe = pipe; > > > > > - cursor->plane = pipe; > > > > > cursor->id = PLANE_CURSOR; > > > > > cursor->frontbuffer_bit = > > > > > INTEL_FRONTBUFFER_CURSOR(pipe); > > > > > cursor->check_plane = intel_check_cursor_plane; > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > > index c1b245853ba9..d14718e09911 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > > @@ -767,7 +767,7 @@ struct intel_plane_wm_parameters { > > > > > > > > > > struct intel_plane { > > > > > struct drm_plane base; > > > > > - u8 plane; > > > > > + enum plane plane; > > > > > enum plane_id id; > > > > > enum pipe pipe; > > > > > bool can_scale; > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > > > index 63154a5a9305..1044095d0084 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct > > > > > drm_i915_private *dev_priv, > > > > > } > > > > > > > > > > intel_plane->pipe = pipe; > > > > > - intel_plane->plane = plane; > > > > > intel_plane->id = PLANE_SPRITE0 + plane; > > > > > intel_plane->frontbuffer_bit = > > > > > INTEL_FRONTBUFFER_SPRITE(pipe, plane); > > > > > intel_plane->check_plane = intel_check_sprite_plane; > > > >
On Fri, 2016-12-16 at 12:07 -0200, Paulo Zanoni wrote: > Em Qui, 2016-12-15 às 21:59 +0200, Ville Syrjälä escreveu: > > > > On Thu, Dec 15, 2016 at 05:50:02PM -0200, Paulo Zanoni wrote: > > > > > > > > > Em Qui, 2016-12-15 às 21:11 +0200, Ville Syrjälä escreveu: > > > > > > > > > > > > On Thu, Dec 15, 2016 at 05:05:05PM -0200, Paulo Zanoni wrote: > > > > > > > > > > > > > > > > > > > > Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala@linux.intel.co > > > > > m > > > > > escreveu: > > > > > > > > > > > > > > > > > > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > > > With plane->plane now purely reserved for the primary planes, > > > > > > let's > > > > > > not even populate it for cursors and sprites. Let's switch > > > > > > the > > > > > > type > > > > > > to enum plane as well since it's no longer being abused for > > > > > > anything > > > > > > else. > > > > > Since it's a primary plane thing, I think it makes more sense > > > > > to > > > > > just > > > > > leave it at struct intel_crtc instead of having a field that's > > > > > unused > > > > > and doesn't make sense in some cases. > > > > Primary plane aren't really primary planes on old platforms > > > > though. > > > > That's just a software concept that has no bearing on the > > > > hardware. > > > Please explain more since that's not my understanding. I just > > > opened > > > the gen 2 and 3 spec docs and they do contain tons of "primary > > > plane" > > > references. I know that the hardware is a little more flexible > > > there, > > Yep. Planes can be moved to any pipe in general. > > > > > > > > > > > but as far as I understand we don't even implement that. > > Not at the moment. But I have plans... > > > > > > > > > > > Besides, both > > > our driver and our API do have the concept of a primary plane, so > > > it > > > makes sense for the code to be organized that way, IMHO. > > A plane is a plane, a pipe is a pipe. IMO you're trying to make it > > more confusing by mixing up the two. > I think that goes against your original argument to remove the plane- > > > > plane assignment for everything except for primary planes. > Anyway, your argumentation is very short and dismissive, it's hard to > understand what exactly you are thinking and try to move the discussion > forward. > > I suppose we'll just have to agree to disagree here. In one solution we > create an inconsistency where the plane struct has a field that's only > applicable and can only be used to some specific planes (even though > "every plane is a plane"). In the other solution we create the > inconsistency where the crtc<->plane connection is assigned in the way > not chosen by the hardware and this may be a problem if we ever get to > implement the plane assignment flexibility that's allowed by the > hardware. Maybe having a 3rd opinion here would help. I find the two plane fields in struct intel_plane a bit confusing, but I don't think the right solution is to leave that in intel_crtc. If I understand correctly, we have a problem of uniquely identifying a plane, since that takes different forms in different platforms. Neither of the generically named enums, enum plane and enum plane_id, is sufficiently generic. Would it make sense to turn things up-side down? Now we are trying to make a crtc be able to find the proper registers for a plane. Instead we could just not do it and define everything in terms of functions that take planes. Then we could hide the details of mapping some bit of information in intel_plane to the appropriate registers in platform-dependent vfuncs. And perhaps give those enums a platform-dependent name, i.e., enum i8xx_plane_id, enum skl_plane_id, etc. Anyway, I haven't looked much into it, so just ignore me if I'm way off. Ander > > > > > > > > > > > > > Besides, I > > > think our code organizations should always better fit the new > > > hardware, > > > since otherwise we'll make i915.ko development even harder than it > > > is > > > for new contributors/employees. > > If you don't mix planes and pipes there can't be any confusion. > > > > > > > > > > > > > > (And when I see those specs and realize how different the HW was, > > > then > > > I remember that in order to explain to the new people why some > > > things > > > are the way they are in our code I have to explain how the HW was > > > 10 > > > years ago, I start thinking again that maybe we should just have > > > i915- > > > old.ko and i915-new.ko, since either we make our > > > abstractions/design > > > fit one thing or we make it fit another...) > > From the register POV hw was almost identical up to BDW at least. > > It's > > just a few bits (the pipe selector) that vanished from the registers. > > SKL+ is a little more different but still the registers even live in > > the > > same address. > > > > Also given what recent history has shown us, I wouldn't be at all > > surprised if we would go back to a flexible plane<->pipe assignment > > in the hardware at some point in the future. A lot of the other > > features that were present in old hardware has been making a comeback > > in recent years. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The crtc struct already includes > > > > > some fields that are specific to primary planes, so I think > > > > > it's a > > > > > good > > > > > place. Or we could create a new class: struct > > > > > intel_primary_plane { > > > > > struct intel_plane base; enum plane legacy_plane }; > > > > > > > > > > Following is a patch suggestion (probably impossible to apply > > > > > due > > > > > to my > > > > > editor breaking long lines). Any arguments against it? > > > > > > > > > > --8<---------------------------------------------------------- > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > > index bc1af87..c54b1a7 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > @@ -15065,14 +15065,6 @@ intel_primary_plane_create(struct > > > > > drm_i915_private *dev_priv, enum pipe pipe) > > > > > state->scaler_id = -1; > > > > > } > > > > > primary->pipe = pipe; > > > > > - /* > > > > > - * On gen2/3 only plane A can do FBC, but the panel > > > > > fitter > > > > > and > > > > > LVDS > > > > > - * port is hooked to pipe B. Hence we want plane A > > > > > feeding > > > > > pipe B. > > > > > - */ > > > > > - if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4) > > > > > - primary->plane = (enum plane) !pipe; > > > > > - else > > > > > - primary->plane = (enum plane) pipe; > > > > > primary->id = PLANE_PRIMARY; > > > > > primary->frontbuffer_bit = > > > > > INTEL_FRONTBUFFER_PRIMARY(pipe); > > > > > primary->check_plane = intel_check_primary_plane; > > > > > @@ -15120,7 +15112,7 @@ intel_primary_plane_create(struct > > > > > drm_i915_private *dev_priv, enum pipe pipe) > > > > > 0, > > > > > &intel_plane_funcs, > > > > > intel_primary_f > > > > > orma > > > > > ts, > > > > > num_formats, > > > > > DRM_PLANE_TYPE_ > > > > > PRIM > > > > > ARY, > > > > > - "plane %c", > > > > > plane_name(primary->plane)); > > > > > + "plane %c", > > > > > pipe_name(pipe)); /* FIXME */ > > > > > if (ret) > > > > > goto fail; > > > > > > > > > > @@ -15272,7 +15264,6 @@ intel_cursor_plane_create(struct > > > > > drm_i915_private *dev_priv, enum pipe pipe) > > > > > cursor->can_scale = false; > > > > > cursor->max_downscale = 1; > > > > > cursor->pipe = pipe; > > > > > - cursor->plane = pipe; > > > > > cursor->id = PLANE_CURSOR; > > > > > cursor->frontbuffer_bit = > > > > > INTEL_FRONTBUFFER_CURSOR(pipe); > > > > > cursor->check_plane = intel_check_cursor_plane; > > > > > @@ -15390,7 +15381,14 @@ static int intel_crtc_init(struct > > > > > drm_i915_private *dev_priv, enum pipe pipe) > > > > > goto fail; > > > > > > > > > > intel_crtc->pipe = pipe; > > > > > - intel_crtc->plane = primary->plane; > > > > > + /* > > > > > + * On gen2/3 only plane A can do FBC, but the panel > > > > > fitter > > > > > and > > > > > LVDS > > > > > + * port is hooked to pipe B. Hence we want plane A > > > > > feeding > > > > > pipe B. > > > > > + */ > > > > > + if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4) > > > > > + intel_crtc->plane = (enum plane) !pipe; > > > > > + else > > > > > + intel_crtc->plane = (enum plane) pipe; > > > > > > > > > > intel_crtc->cursor_base = ~0; > > > > > intel_crtc->cursor_cntl = ~0; > > > > > @@ -16866,11 +16864,11 @@ void i915_redisable_vga(struct > > > > > drm_i915_private *dev_priv) > > > > > intel_display_power_put(dev_priv, POWER_DOMAIN_VGA); > > > > > } > > > > > > > > > > -static bool primary_get_hw_state(struct intel_plane *plane) > > > > > +static bool primary_get_hw_state(struct intel_crtc *crtc) > > > > > { > > > > > - struct drm_i915_private *dev_priv = to_i915(plane- > > > > > > > > > > > > > > > > > > base.dev); > > > > > + struct drm_i915_private *dev_priv = to_i915(crtc- > > > > > > > > > > > > > > > > > > base.dev); > > > > > > > > > > - return I915_READ(DSPCNTR(plane->plane)) & > > > > > DISPLAY_PLANE_ENABLE; > > > > > + return I915_READ(DSPCNTR(crtc->plane)) & > > > > > DISPLAY_PLANE_ENABLE; > > > > > } > > > > > > > > > > /* FIXME read out full plane state for all planes */ > > > > > @@ -16880,8 +16878,7 @@ static void readout_plane_state(struct > > > > > intel_crtc *crtc) > > > > > struct intel_plane_state *plane_state = > > > > > to_intel_plane_state(primary->state); > > > > > > > > > > - plane_state->base.visible = crtc->active && > > > > > - primary_get_hw_state(to_intel_plane(primary)); > > > > > + plane_state->base.visible = crtc->active && > > > > > primary_get_hw_state(crtc); > > > > > > > > > > if (plane_state->base.visible) > > > > > crtc->base.state->plane_mask |= 1 << > > > > > drm_plane_index(primary); > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > > index 362f698..b2bdd49 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > > @@ -767,7 +767,6 @@ struct intel_plane_wm_parameters { > > > > > > > > > > struct intel_plane { > > > > > struct drm_plane base; > > > > > - u8 plane; > > > > > enum plane_id id; > > > > > enum pipe pipe; > > > > > bool can_scale; > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > > > index 63154a5..1044095 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct > > > > > drm_i915_private > > > > > *dev_priv, > > > > > } > > > > > > > > > > intel_plane->pipe = pipe; > > > > > - intel_plane->plane = plane; > > > > > intel_plane->id = PLANE_SPRITE0 + plane; > > > > > intel_plane->frontbuffer_bit = > > > > > INTEL_FRONTBUFFER_SPRITE(pipe, > > > > > plane); > > > > > intel_plane->check_plane = intel_check_sprite_plane; > > > > > > > > > > --8<---------------------------------------------------------- > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/intel_display.c | 1 - > > > > > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > > > > > drivers/gpu/drm/i915/intel_sprite.c | 1 - > > > > > > 3 files changed, 1 insertion(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > > > index b6d5d5e5cc99..f180f14fcf3a 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > > @@ -15188,7 +15188,6 @@ intel_cursor_plane_create(struct > > > > > > drm_i915_private *dev_priv, enum pipe pipe) > > > > > > cursor->can_scale = false; > > > > > > cursor->max_downscale = 1; > > > > > > cursor->pipe = pipe; > > > > > > - cursor->plane = pipe; > > > > > > cursor->id = PLANE_CURSOR; > > > > > > cursor->frontbuffer_bit = > > > > > > INTEL_FRONTBUFFER_CURSOR(pipe); > > > > > > cursor->check_plane = intel_check_cursor_plane; > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > > > index c1b245853ba9..d14718e09911 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > > > @@ -767,7 +767,7 @@ struct intel_plane_wm_parameters { > > > > > > > > > > > > struct intel_plane { > > > > > > struct drm_plane base; > > > > > > - u8 plane; > > > > > > + enum plane plane; > > > > > > enum plane_id id; > > > > > > enum pipe pipe; > > > > > > bool can_scale; > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > > > > index 63154a5a9305..1044095d0084 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct > > > > > > drm_i915_private *dev_priv, > > > > > > } > > > > > > > > > > > > intel_plane->pipe = pipe; > > > > > > - intel_plane->plane = plane; > > > > > > intel_plane->id = PLANE_SPRITE0 + plane; > > > > > > intel_plane->frontbuffer_bit = > > > > > > INTEL_FRONTBUFFER_SPRITE(pipe, plane); > > > > > > intel_plane->check_plane = intel_check_sprite_plane; > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Dec 15, 2016 at 09:59:51PM +0200, Ville Syrjälä wrote: > On Thu, Dec 15, 2016 at 05:50:02PM -0200, Paulo Zanoni wrote: > > Em Qui, 2016-12-15 às 21:11 +0200, Ville Syrjälä escreveu: > > > On Thu, Dec 15, 2016 at 05:05:05PM -0200, Paulo Zanoni wrote: > > > > > > > > Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala@linux.intel.com > > > > escreveu: > > > > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > With plane->plane now purely reserved for the primary planes, > > > > > let's > > > > > not even populate it for cursors and sprites. Let's switch the > > > > > type > > > > > to enum plane as well since it's no longer being abused for > > > > > anything > > > > > else. > > > > > > > > Since it's a primary plane thing, I think it makes more sense to > > > > just > > > > leave it at struct intel_crtc instead of having a field that's > > > > unused > > > > and doesn't make sense in some cases. > > > > > > Primary plane aren't really primary planes on old platforms though. > > > That's just a software concept that has no bearing on the hardware. > > > > Please explain more since that's not my understanding. I just opened > > the gen 2 and 3 spec docs and they do contain tons of "primary plane" > > references. I know that the hardware is a little more flexible there, > > Yep. Planes can be moved to any pipe in general. > > > but as far as I understand we don't even implement that. > > Not at the moment. But I have plans... I'm not even sure where to find bspecs for the really old platforms...how flexible were those primary planes? Can we drive both "primary" planes at the same time on a single CRTC if we're not using the second pipe? If not, is there some other benefit we get from not enforcing a fixed pipe-plane association? Matt > > > Besides, both > > our driver and our API do have the concept of a primary plane, so it > > makes sense for the code to be organized that way, IMHO. > > A plane is a plane, a pipe is a pipe. IMO you're trying to make it > more confusing by mixing up the two. > > > Besides, I > > think our code organizations should always better fit the new hardware, > > since otherwise we'll make i915.ko development even harder than it is > > for new contributors/employees. > > If you don't mix planes and pipes there can't be any confusion. > > > > > (And when I see those specs and realize how different the HW was, then > > I remember that in order to explain to the new people why some things > > are the way they are in our code I have to explain how the HW was 10 > > years ago, I start thinking again that maybe we should just have i915- > > old.ko and i915-new.ko, since either we make our abstractions/design > > fit one thing or we make it fit another...) > > From the register POV hw was almost identical up to BDW at least. It's > just a few bits (the pipe selector) that vanished from the registers. > SKL+ is a little more different but still the registers even live in the > same address. > > Also given what recent history has shown us, I wouldn't be at all > surprised if we would go back to a flexible plane<->pipe assignment > in the hardware at some point in the future. A lot of the other > features that were present in old hardware has been making a comeback > in recent years. > > > > > > > > > > > > > > The crtc struct already includes > > > > some fields that are specific to primary planes, so I think it's a > > > > good > > > > place. Or we could create a new class: struct intel_primary_plane { > > > > struct intel_plane base; enum plane legacy_plane }; > > > > > > > > Following is a patch suggestion (probably impossible to apply due > > > > to my > > > > editor breaking long lines). Any arguments against it? > > > > > > > > --8<---------------------------------------------------------- > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index bc1af87..c54b1a7 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -15065,14 +15065,6 @@ intel_primary_plane_create(struct > > > > drm_i915_private *dev_priv, enum pipe pipe) > > > > state->scaler_id = -1; > > > > } > > > > primary->pipe = pipe; > > > > - /* > > > > - * On gen2/3 only plane A can do FBC, but the panel fitter > > > > and > > > > LVDS > > > > - * port is hooked to pipe B. Hence we want plane A feeding > > > > pipe B. > > > > - */ > > > > - if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4) > > > > - primary->plane = (enum plane) !pipe; > > > > - else > > > > - primary->plane = (enum plane) pipe; > > > > primary->id = PLANE_PRIMARY; > > > > primary->frontbuffer_bit = > > > > INTEL_FRONTBUFFER_PRIMARY(pipe); > > > > primary->check_plane = intel_check_primary_plane; > > > > @@ -15120,7 +15112,7 @@ intel_primary_plane_create(struct > > > > drm_i915_private *dev_priv, enum pipe pipe) > > > > 0, > > > > &intel_plane_funcs, > > > > intel_primary_forma > > > > ts, > > > > num_formats, > > > > DRM_PLANE_TYPE_PRIM > > > > ARY, > > > > - "plane %c", > > > > plane_name(primary->plane)); > > > > + "plane %c", > > > > pipe_name(pipe)); /* FIXME */ > > > > if (ret) > > > > goto fail; > > > > > > > > @@ -15272,7 +15264,6 @@ intel_cursor_plane_create(struct > > > > drm_i915_private *dev_priv, enum pipe pipe) > > > > cursor->can_scale = false; > > > > cursor->max_downscale = 1; > > > > cursor->pipe = pipe; > > > > - cursor->plane = pipe; > > > > cursor->id = PLANE_CURSOR; > > > > cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe); > > > > cursor->check_plane = intel_check_cursor_plane; > > > > @@ -15390,7 +15381,14 @@ static int intel_crtc_init(struct > > > > drm_i915_private *dev_priv, enum pipe pipe) > > > > goto fail; > > > > > > > > intel_crtc->pipe = pipe; > > > > - intel_crtc->plane = primary->plane; > > > > + /* > > > > + * On gen2/3 only plane A can do FBC, but the panel fitter > > > > and > > > > LVDS > > > > + * port is hooked to pipe B. Hence we want plane A feeding > > > > pipe B. > > > > + */ > > > > + if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4) > > > > + intel_crtc->plane = (enum plane) !pipe; > > > > + else > > > > + intel_crtc->plane = (enum plane) pipe; > > > > > > > > intel_crtc->cursor_base = ~0; > > > > intel_crtc->cursor_cntl = ~0; > > > > @@ -16866,11 +16864,11 @@ void i915_redisable_vga(struct > > > > drm_i915_private *dev_priv) > > > > intel_display_power_put(dev_priv, POWER_DOMAIN_VGA); > > > > } > > > > > > > > -static bool primary_get_hw_state(struct intel_plane *plane) > > > > +static bool primary_get_hw_state(struct intel_crtc *crtc) > > > > { > > > > - struct drm_i915_private *dev_priv = to_i915(plane- > > > > >base.dev); > > > > + struct drm_i915_private *dev_priv = to_i915(crtc- > > > > >base.dev); > > > > > > > > - return I915_READ(DSPCNTR(plane->plane)) & > > > > DISPLAY_PLANE_ENABLE; > > > > + return I915_READ(DSPCNTR(crtc->plane)) & > > > > DISPLAY_PLANE_ENABLE; > > > > } > > > > > > > > /* FIXME read out full plane state for all planes */ > > > > @@ -16880,8 +16878,7 @@ static void readout_plane_state(struct > > > > intel_crtc *crtc) > > > > struct intel_plane_state *plane_state = > > > > to_intel_plane_state(primary->state); > > > > > > > > - plane_state->base.visible = crtc->active && > > > > - primary_get_hw_state(to_intel_plane(primary)); > > > > + plane_state->base.visible = crtc->active && > > > > primary_get_hw_state(crtc); > > > > > > > > if (plane_state->base.visible) > > > > crtc->base.state->plane_mask |= 1 << > > > > drm_plane_index(primary); > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > index 362f698..b2bdd49 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -767,7 +767,6 @@ struct intel_plane_wm_parameters { > > > > > > > > struct intel_plane { > > > > struct drm_plane base; > > > > - u8 plane; > > > > enum plane_id id; > > > > enum pipe pipe; > > > > bool can_scale; > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > > index 63154a5..1044095 100644 > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct > > > > drm_i915_private > > > > *dev_priv, > > > > } > > > > > > > > intel_plane->pipe = pipe; > > > > - intel_plane->plane = plane; > > > > intel_plane->id = PLANE_SPRITE0 + plane; > > > > intel_plane->frontbuffer_bit = > > > > INTEL_FRONTBUFFER_SPRITE(pipe, > > > > plane); > > > > intel_plane->check_plane = intel_check_sprite_plane; > > > > > > > > --8<---------------------------------------------------------- > > > > > > > > > > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_display.c | 1 - > > > > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > > > > drivers/gpu/drm/i915/intel_sprite.c | 1 - > > > > > 3 files changed, 1 insertion(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > > index b6d5d5e5cc99..f180f14fcf3a 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > @@ -15188,7 +15188,6 @@ intel_cursor_plane_create(struct > > > > > drm_i915_private *dev_priv, enum pipe pipe) > > > > > cursor->can_scale = false; > > > > > cursor->max_downscale = 1; > > > > > cursor->pipe = pipe; > > > > > - cursor->plane = pipe; > > > > > cursor->id = PLANE_CURSOR; > > > > > cursor->frontbuffer_bit = > > > > > INTEL_FRONTBUFFER_CURSOR(pipe); > > > > > cursor->check_plane = intel_check_cursor_plane; > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > > index c1b245853ba9..d14718e09911 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > > @@ -767,7 +767,7 @@ struct intel_plane_wm_parameters { > > > > > > > > > > struct intel_plane { > > > > > struct drm_plane base; > > > > > - u8 plane; > > > > > + enum plane plane; > > > > > enum plane_id id; > > > > > enum pipe pipe; > > > > > bool can_scale; > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > > > index 63154a5a9305..1044095d0084 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > > @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct > > > > > drm_i915_private *dev_priv, > > > > > } > > > > > > > > > > intel_plane->pipe = pipe; > > > > > - intel_plane->plane = plane; > > > > > intel_plane->id = PLANE_SPRITE0 + plane; > > > > > intel_plane->frontbuffer_bit = > > > > > INTEL_FRONTBUFFER_SPRITE(pipe, plane); > > > > > intel_plane->check_plane = intel_check_sprite_plane; > > > > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Dec 19, 2016 at 10:24:15AM -0800, Matt Roper wrote: > On Thu, Dec 15, 2016 at 09:59:51PM +0200, Ville Syrjälä wrote: > > On Thu, Dec 15, 2016 at 05:50:02PM -0200, Paulo Zanoni wrote: > > > Em Qui, 2016-12-15 às 21:11 +0200, Ville Syrjälä escreveu: > > > > On Thu, Dec 15, 2016 at 05:05:05PM -0200, Paulo Zanoni wrote: > > > > > > > > > > Em Ter, 2016-11-22 às 18:02 +0200, ville.syrjala@linux.intel.com > > > > > escreveu: > > > > > > > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > > > With plane->plane now purely reserved for the primary planes, > > > > > > let's > > > > > > not even populate it for cursors and sprites. Let's switch the > > > > > > type > > > > > > to enum plane as well since it's no longer being abused for > > > > > > anything > > > > > > else. > > > > > > > > > > Since it's a primary plane thing, I think it makes more sense to > > > > > just > > > > > leave it at struct intel_crtc instead of having a field that's > > > > > unused > > > > > and doesn't make sense in some cases. > > > > > > > > Primary plane aren't really primary planes on old platforms though. > > > > That's just a software concept that has no bearing on the hardware. > > > > > > Please explain more since that's not my understanding. I just opened > > > the gen 2 and 3 spec docs and they do contain tons of "primary plane" > > > references. I know that the hardware is a little more flexible there, > > > > Yep. Planes can be moved to any pipe in general. > > > > > but as far as I understand we don't even implement that. > > > > Not at the moment. But I have plans... > > I'm not even sure where to find bspecs for the really old > platforms...how flexible were those primary planes? Can we drive both > "primary" planes at the same time on a single CRTC if we're not using > the second pipe? There are three "primary" planes, two cursors, and one video overlay. Any one can be assigned to either pipe more or less freely (some restrctions do apply). The planes aren't uniform (not even the "primary" planes) and instead have different capabilities between them.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index bc1af87..c54b1a7 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15065,14 +15065,6 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) state->scaler_id = -1; } primary->pipe = pipe; - /* - * On gen2/3 only plane A can do FBC, but the panel fitter and LVDS - * port is hooked to pipe B. Hence we want plane A feeding pipe B. - */ - if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4) - primary->plane = (enum plane) !pipe; - else - primary->plane = (enum plane) pipe; primary->id = PLANE_PRIMARY; primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe); primary->check_plane = intel_check_primary_plane; @@ -15120,7 +15112,7 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) 0, &intel_plane_funcs, intel_primary_formats, num_formats, DRM_PLANE_TYPE_PRIMARY, - "plane %c", plane_name(primary->plane)); + "plane %c", pipe_name(pipe)); /* FIXME */ if (ret) goto fail; @@ -15272,7 +15264,6 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe) cursor->can_scale = false; cursor->max_downscale = 1; cursor->pipe = pipe; - cursor->plane = pipe; cursor->id = PLANE_CURSOR; cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe); cursor->check_plane = intel_check_cursor_plane; @@ -15390,7 +15381,14 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe) goto fail; intel_crtc->pipe = pipe; - intel_crtc->plane = primary->plane; + /* + * On gen2/3 only plane A can do FBC, but the panel fitter and LVDS + * port is hooked to pipe B. Hence we want plane A feeding pipe B. + */ + if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4) + intel_crtc->plane = (enum plane) !pipe; + else + intel_crtc->plane = (enum plane) pipe; intel_crtc->cursor_base = ~0; intel_crtc->cursor_cntl = ~0; @@ -16866,11 +16864,11 @@ void i915_redisable_vga(struct drm_i915_private *dev_priv) intel_display_power_put(dev_priv, POWER_DOMAIN_VGA); } -static bool primary_get_hw_state(struct intel_plane *plane) +static bool primary_get_hw_state(struct intel_crtc *crtc) { - struct drm_i915_private *dev_priv = to_i915(plane->base.dev); + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); - return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE; + return I915_READ(DSPCNTR(crtc->plane)) & DISPLAY_PLANE_ENABLE; } /* FIXME read out full plane state for all planes */ @@ -16880,8 +16878,7 @@ static void readout_plane_state(struct intel_crtc *crtc) struct intel_plane_state *plane_state = to_intel_plane_state(primary->state); - plane_state->base.visible = crtc->active && - primary_get_hw_state(to_intel_plane(primary)); + plane_state->base.visible = crtc->active && primary_get_hw_state(crtc); if (plane_state->base.visible) crtc->base.state->plane_mask |= 1 << drm_plane_index(primary); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 362f698..b2bdd49 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -767,7 +767,6 @@ struct intel_plane_wm_parameters { struct intel_plane { struct drm_plane base; - u8 plane; enum plane_id id; enum pipe pipe; bool can_scale; diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 63154a5..1044095 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1111,7 +1111,6 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv, } intel_plane->pipe = pipe; - intel_plane->plane = plane; intel_plane->id = PLANE_SPRITE0 + plane; intel_plane->frontbuffer_bit = INTEL_FRONTBUFFER_SPRITE(pipe,