| Message ID | 1441894085-25662-4-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote: > This function was still using the legacy state, convert it to atomic. > While we're at it, fix the FIXME too and disable the primary plane. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++-------------- > 1 file changed, 12 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 33200403a5db..b68aa95c5460 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev) > > static void intel_update_primary_planes(struct drm_device *dev) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_crtc *crtc; > > for_each_crtc(dev, crtc) { > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_plane *plane = to_intel_plane(crtc->primary); > + struct intel_plane_state *plane_state; > > - drm_modeset_lock(&crtc->mutex, NULL); > - /* > - * FIXME: Once we have proper support for primary planes (and > - * disabling them without disabling the entire crtc) allow again > - * a NULL crtc->primary->fb. > - */ We probably should have a new FIXME here to ditch this all once we use atomic for legacy page_flips (since the only reason we have this is when CS flips are lost in the reset). -Daniel > - if (intel_crtc->active && crtc->primary->fb) > - dev_priv->display.update_primary_plane(crtc, > - crtc->primary->fb, > - crtc->x, > - crtc->y); > - drm_modeset_unlock(&crtc->mutex); > + drm_modeset_lock_crtc(crtc, &plane->base); > + > + plane_state = to_intel_plane_state(plane->base.state); > + > + if (plane_state->base.fb) > + plane->commit_plane(&plane->base, plane_state); > + else if (crtc->state->active) > + plane->disable_plane(&plane->base, crtc); > + > + drm_modeset_unlock_crtc(crtc); > } > } > > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote: > This function was still using the legacy state, convert it to atomic. > While we're at it, fix the FIXME too and disable the primary plane. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++-------------- > 1 file changed, 12 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 33200403a5db..b68aa95c5460 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev) > > static void intel_update_primary_planes(struct drm_device *dev) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_crtc *crtc; > > for_each_crtc(dev, crtc) { > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_plane *plane = to_intel_plane(crtc->primary); > + struct intel_plane_state *plane_state; > > - drm_modeset_lock(&crtc->mutex, NULL); > - /* > - * FIXME: Once we have proper support for primary planes (and > - * disabling them without disabling the entire crtc) allow again > - * a NULL crtc->primary->fb. > - */ > - if (intel_crtc->active && crtc->primary->fb) > - dev_priv->display.update_primary_plane(crtc, > - crtc->primary->fb, > - crtc->x, > - crtc->y); > - drm_modeset_unlock(&crtc->mutex); > + drm_modeset_lock_crtc(crtc, &plane->base); > + > + plane_state = to_intel_plane_state(plane->base.state); > + > + if (plane_state->base.fb) > + plane->commit_plane(&plane->base, plane_state); > + else if (crtc->state->active) > + plane->disable_plane(&plane->base, crtc); That doesn't make sense. There's no way to disable a plane with a page flip, so there's simply nothing to do here if the fb is NULL. If we can trust the plane state to be sane we should just check 'visible' here and commit the plane in that case. > + > + drm_modeset_unlock_crtc(crtc); > } > } > > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Sep 10, 2015 at 06:43:26PM +0300, Ville Syrjälä wrote: > On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote: > > This function was still using the legacy state, convert it to atomic. > > While we're at it, fix the FIXME too and disable the primary plane. > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++-------------- > > 1 file changed, 12 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 33200403a5db..b68aa95c5460 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev) > > > > static void intel_update_primary_planes(struct drm_device *dev) > > { > > - struct drm_i915_private *dev_priv = dev->dev_private; > > struct drm_crtc *crtc; > > > > for_each_crtc(dev, crtc) { > > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > + struct intel_plane *plane = to_intel_plane(crtc->primary); > > + struct intel_plane_state *plane_state; > > > > - drm_modeset_lock(&crtc->mutex, NULL); > > - /* > > - * FIXME: Once we have proper support for primary planes (and > > - * disabling them without disabling the entire crtc) allow again > > - * a NULL crtc->primary->fb. > > - */ > > - if (intel_crtc->active && crtc->primary->fb) > > - dev_priv->display.update_primary_plane(crtc, > > - crtc->primary->fb, > > - crtc->x, > > - crtc->y); > > - drm_modeset_unlock(&crtc->mutex); > > + drm_modeset_lock_crtc(crtc, &plane->base); > > + > > + plane_state = to_intel_plane_state(plane->base.state); > > + > > + if (plane_state->base.fb) > > + plane->commit_plane(&plane->base, plane_state); > > + else if (crtc->state->active) > > + plane->disable_plane(&plane->base, crtc); > > That doesn't make sense. There's no way to disable a plane with a page > flip, so there's simply nothing to do here if the fb is NULL. If we can > trust the plane state to be sane we should just check 'visible' here and > commit the plane in that case. Right missed that and squash up a fixup that Ville acked on irc. Also added the FIXME comment too. -Daniel > > > + > > + drm_modeset_unlock_crtc(crtc); > > } > > } > > > > -- > > 2.1.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Sep 10, 2015 at 06:31:02PM +0200, Daniel Vetter wrote: > On Thu, Sep 10, 2015 at 06:43:26PM +0300, Ville Syrjälä wrote: > > On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote: > > > This function was still using the legacy state, convert it to atomic. > > > While we're at it, fix the FIXME too and disable the primary plane. > > > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++-------------- > > > 1 file changed, 12 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 33200403a5db..b68aa95c5460 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev) > > > > > > static void intel_update_primary_planes(struct drm_device *dev) > > > { > > > - struct drm_i915_private *dev_priv = dev->dev_private; > > > struct drm_crtc *crtc; > > > > > > for_each_crtc(dev, crtc) { > > > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > + struct intel_plane *plane = to_intel_plane(crtc->primary); > > > + struct intel_plane_state *plane_state; > > > > > > - drm_modeset_lock(&crtc->mutex, NULL); > > > - /* > > > - * FIXME: Once we have proper support for primary planes (and > > > - * disabling them without disabling the entire crtc) allow again > > > - * a NULL crtc->primary->fb. > > > - */ > > > - if (intel_crtc->active && crtc->primary->fb) > > > - dev_priv->display.update_primary_plane(crtc, > > > - crtc->primary->fb, > > > - crtc->x, > > > - crtc->y); > > > - drm_modeset_unlock(&crtc->mutex); > > > + drm_modeset_lock_crtc(crtc, &plane->base); > > > + > > > + plane_state = to_intel_plane_state(plane->base.state); > > > + > > > + if (plane_state->base.fb) > > > + plane->commit_plane(&plane->base, plane_state); > > > + else if (crtc->state->active) > > > + plane->disable_plane(&plane->base, crtc); > > > > That doesn't make sense. There's no way to disable a plane with a page > > flip, so there's simply nothing to do here if the fb is NULL. If we can > > trust the plane state to be sane we should just check 'visible' here and > > commit the plane in that case. > > Right missed that and squash up a fixup that Ville acked on irc. Also > added the FIXME comment too. Actually I take it back. We just lost the ->active check entirely so now we're commiting a plane on a disabled pipe potentially. > -Daniel > > > > > > + > > > + drm_modeset_unlock_crtc(crtc); > > > } > > > } > > > > > > -- > > > 2.1.0 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Thu, Sep 10, 2015 at 07:34:05PM +0300, Ville Syrjälä wrote: > On Thu, Sep 10, 2015 at 06:31:02PM +0200, Daniel Vetter wrote: > > On Thu, Sep 10, 2015 at 06:43:26PM +0300, Ville Syrjälä wrote: > > > On Thu, Sep 10, 2015 at 04:07:58PM +0200, Maarten Lankhorst wrote: > > > > This function was still using the legacy state, convert it to atomic. > > > > While we're at it, fix the FIXME too and disable the primary plane. > > > > > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++-------------- > > > > 1 file changed, 12 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > index 33200403a5db..b68aa95c5460 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev) > > > > > > > > static void intel_update_primary_planes(struct drm_device *dev) > > > > { > > > > - struct drm_i915_private *dev_priv = dev->dev_private; > > > > struct drm_crtc *crtc; > > > > > > > > for_each_crtc(dev, crtc) { > > > > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > > + struct intel_plane *plane = to_intel_plane(crtc->primary); > > > > + struct intel_plane_state *plane_state; > > > > > > > > - drm_modeset_lock(&crtc->mutex, NULL); > > > > - /* > > > > - * FIXME: Once we have proper support for primary planes (and > > > > - * disabling them without disabling the entire crtc) allow again > > > > - * a NULL crtc->primary->fb. > > > > - */ > > > > - if (intel_crtc->active && crtc->primary->fb) > > > > - dev_priv->display.update_primary_plane(crtc, > > > > - crtc->primary->fb, > > > > - crtc->x, > > > > - crtc->y); > > > > - drm_modeset_unlock(&crtc->mutex); > > > > + drm_modeset_lock_crtc(crtc, &plane->base); > > > > + > > > > + plane_state = to_intel_plane_state(plane->base.state); > > > > + > > > > + if (plane_state->base.fb) > > > > + plane->commit_plane(&plane->base, plane_state); > > > > + else if (crtc->state->active) > > > > + plane->disable_plane(&plane->base, crtc); > > > > > > That doesn't make sense. There's no way to disable a plane with a page > > > flip, so there's simply nothing to do here if the fb is NULL. If we can > > > trust the plane state to be sane we should just check 'visible' here and > > > commit the plane in that case. > > > > Right missed that and squash up a fixup that Ville acked on irc. Also > > added the FIXME comment too. > > Actually I take it back. We just lost the ->active check entirely so now > we're commiting a plane on a disabled pipe potentially. Blergh and I tagged the pile already :( Maarten, can you pls send out the fixup as a separate patch asap? Thanks, Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 33200403a5db..b68aa95c5460 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3138,24 +3138,22 @@ static void intel_complete_page_flips(struct drm_device *dev) static void intel_update_primary_planes(struct drm_device *dev) { - struct drm_i915_private *dev_priv = dev->dev_private; struct drm_crtc *crtc; for_each_crtc(dev, crtc) { - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct intel_plane *plane = to_intel_plane(crtc->primary); + struct intel_plane_state *plane_state; - drm_modeset_lock(&crtc->mutex, NULL); - /* - * FIXME: Once we have proper support for primary planes (and - * disabling them without disabling the entire crtc) allow again - * a NULL crtc->primary->fb. - */ - if (intel_crtc->active && crtc->primary->fb) - dev_priv->display.update_primary_plane(crtc, - crtc->primary->fb, - crtc->x, - crtc->y); - drm_modeset_unlock(&crtc->mutex); + drm_modeset_lock_crtc(crtc, &plane->base); + + plane_state = to_intel_plane_state(plane->base.state); + + if (plane_state->base.fb) + plane->commit_plane(&plane->base, plane_state); + else if (crtc->state->active) + plane->disable_plane(&plane->base, crtc); + + drm_modeset_unlock_crtc(crtc); } }
This function was still using the legacy state, convert it to atomic. While we're at it, fix the FIXME too and disable the primary plane. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-)