| Message ID | 1431548627-2527-17-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 13, 2015 at 10:23:46PM +0200, Maarten Lankhorst wrote: > intel_crtc->config will be removed eventually, so use crtc->hwmode. > drm_atomic_helper_update_legacy_modeset_state updates hwmode, > but crtc->active will eventually be gone too. Set dotclock to zero > to indicate the crtc is inactive. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> I think adding a code comment to our assignment of crtc->hw_mode that we need this for i915_get_vblank_timestamp (and only for that) would be really good. Especially since I can't find it with a quick grep, at least in current upstream ;-) -Daniel > --- > drivers/gpu/drm/i915/i915_irq.c | 13 ++++++------- > drivers/gpu/drm/i915/intel_display.c | 3 +++ > 2 files changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index ca457317a8ac..9359ea2399f1 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -564,8 +564,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe) > u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal; > struct intel_crtc *intel_crtc = > to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > - const struct drm_display_mode *mode = > - &intel_crtc->config->base.adjusted_mode; > + const struct drm_display_mode *mode = &intel_crtc->base.hwmode; > > htotal = mode->crtc_htotal; > hsync_start = mode->crtc_hsync_start; > @@ -620,7 +619,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) > { > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - const struct drm_display_mode *mode = &crtc->config->base.adjusted_mode; > + const struct drm_display_mode *mode = &crtc->base.hwmode; > enum pipe pipe = crtc->pipe; > int position, vtotal; > > @@ -647,14 +646,14 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - const struct drm_display_mode *mode = &intel_crtc->config->base.adjusted_mode; > + const struct drm_display_mode *mode = &intel_crtc->base.hwmode; > int position; > int vbl_start, vbl_end, hsync_start, htotal, vtotal; > bool in_vbl = true; > int ret = 0; > unsigned long irqflags; > > - if (!intel_crtc->active) { > + if (WARN_ON(!mode->crtc_clock)) { > DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled " > "pipe %c\n", pipe_name(pipe)); > return 0; > @@ -796,7 +795,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe, > return -EINVAL; > } > > - if (!crtc->state->active) { > + if (!crtc->hwmode.crtc_clock) { > DRM_DEBUG_KMS("crtc %d is disabled\n", pipe); > return -EBUSY; > } > @@ -805,7 +804,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe, > return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error, > vblank_time, flags, > crtc, > - &to_intel_crtc(crtc)->config->base.adjusted_mode); > + &crtc->hwmode); > } > > static bool intel_hpd_irq_event(struct drm_device *dev, > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 9316b0be4f5b..5063ac3d028d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11467,6 +11467,9 @@ intel_modeset_update_state(struct drm_atomic_state *state) > WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc)); > > to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state); > + > + if (!crtc->state->active) > + crtc->hwmode.crtc_clock = 0; > } > > list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, May 18, 2015 at 05:49:23PM +0200, Daniel Vetter wrote: > On Wed, May 13, 2015 at 10:23:46PM +0200, Maarten Lankhorst wrote: > > intel_crtc->config will be removed eventually, so use crtc->hwmode. > > drm_atomic_helper_update_legacy_modeset_state updates hwmode, > > but crtc->active will eventually be gone too. Set dotclock to zero > > to indicate the crtc is inactive. > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > I think adding a code comment to our assignment of crtc->hw_mode that we > need this for i915_get_vblank_timestamp (and only for that) would be > really good. Especially since I can't find it with a quick grep, at least > in current upstream ;-) I don't particularly like resurrecting this zombie. Why we can't just use crtc->state->adjusted_mode (or wherever the current adjusted mode is kept)? > -Daniel > > > --- > > drivers/gpu/drm/i915/i915_irq.c | 13 ++++++------- > > drivers/gpu/drm/i915/intel_display.c | 3 +++ > > 2 files changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index ca457317a8ac..9359ea2399f1 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -564,8 +564,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe) > > u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal; > > struct intel_crtc *intel_crtc = > > to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > > - const struct drm_display_mode *mode = > > - &intel_crtc->config->base.adjusted_mode; > > + const struct drm_display_mode *mode = &intel_crtc->base.hwmode; > > > > htotal = mode->crtc_htotal; > > hsync_start = mode->crtc_hsync_start; > > @@ -620,7 +619,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) > > { > > struct drm_device *dev = crtc->base.dev; > > struct drm_i915_private *dev_priv = dev->dev_private; > > - const struct drm_display_mode *mode = &crtc->config->base.adjusted_mode; > > + const struct drm_display_mode *mode = &crtc->base.hwmode; > > enum pipe pipe = crtc->pipe; > > int position, vtotal; > > > > @@ -647,14 +646,14 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, > > struct drm_i915_private *dev_priv = dev->dev_private; > > struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > - const struct drm_display_mode *mode = &intel_crtc->config->base.adjusted_mode; > > + const struct drm_display_mode *mode = &intel_crtc->base.hwmode; > > int position; > > int vbl_start, vbl_end, hsync_start, htotal, vtotal; > > bool in_vbl = true; > > int ret = 0; > > unsigned long irqflags; > > > > - if (!intel_crtc->active) { > > + if (WARN_ON(!mode->crtc_clock)) { > > DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled " > > "pipe %c\n", pipe_name(pipe)); > > return 0; > > @@ -796,7 +795,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe, > > return -EINVAL; > > } > > > > - if (!crtc->state->active) { > > + if (!crtc->hwmode.crtc_clock) { > > DRM_DEBUG_KMS("crtc %d is disabled\n", pipe); > > return -EBUSY; > > } > > @@ -805,7 +804,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe, > > return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error, > > vblank_time, flags, > > crtc, > > - &to_intel_crtc(crtc)->config->base.adjusted_mode); > > + &crtc->hwmode); > > } > > > > static bool intel_hpd_irq_event(struct drm_device *dev, > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 9316b0be4f5b..5063ac3d028d 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -11467,6 +11467,9 @@ intel_modeset_update_state(struct drm_atomic_state *state) > > WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc)); > > > > to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state); > > + > > + if (!crtc->state->active) > > + crtc->hwmode.crtc_clock = 0; > > } > > > > list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > > -- > > 2.1.0 > > > > _______________________________________________ > > 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 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 18-05-15 om 18:28 schreef Ville Syrjälä: > On Mon, May 18, 2015 at 05:49:23PM +0200, Daniel Vetter wrote: >> On Wed, May 13, 2015 at 10:23:46PM +0200, Maarten Lankhorst wrote: >>> intel_crtc->config will be removed eventually, so use crtc->hwmode. >>> drm_atomic_helper_update_legacy_modeset_state updates hwmode, >>> but crtc->active will eventually be gone too. Set dotclock to zero >>> to indicate the crtc is inactive. >>> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> I think adding a code comment to our assignment of crtc->hw_mode that we >> need this for i915_get_vblank_timestamp (and only for that) would be >> really good. Especially since I can't find it with a quick grep, at least >> in current upstream ;-) > I don't particularly like resurrecting this zombie. Why we can't just use > crtc->state->adjusted_mode (or wherever the current adjusted mode is kept)? > Because we want to get rid of intel_crtc->config, and if drm_atomic_swap_state is moved to be done before any call to then crtc->state->adjusted_mode will not be in sync with the hw state, and any wai tfor vblank will produce funny results. Since I don't think you should want to pass a state to vblank you would have to use some crtc local variable somewhere, in this case I chose to use hwmode for that. ~Maarten
On Tue, May 19, 2015 at 08:10:46AM +0200, Maarten Lankhorst wrote: > Op 18-05-15 om 18:28 schreef Ville Syrjälä: > > On Mon, May 18, 2015 at 05:49:23PM +0200, Daniel Vetter wrote: > >> On Wed, May 13, 2015 at 10:23:46PM +0200, Maarten Lankhorst wrote: > >>> intel_crtc->config will be removed eventually, so use crtc->hwmode. > >>> drm_atomic_helper_update_legacy_modeset_state updates hwmode, > >>> but crtc->active will eventually be gone too. Set dotclock to zero > >>> to indicate the crtc is inactive. > >>> > >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> I think adding a code comment to our assignment of crtc->hw_mode that we > >> need this for i915_get_vblank_timestamp (and only for that) would be > >> really good. Especially since I can't find it with a quick grep, at least > >> in current upstream ;-) > > I don't particularly like resurrecting this zombie. Why we can't just use > > crtc->state->adjusted_mode (or wherever the current adjusted mode is kept)? > > > Because we want to get rid of intel_crtc->config, and if drm_atomic_swap_state > is moved to be done before any call to then crtc->state->adjusted_mode will not > be in sync with the hw state, and any wai tfor vblank will produce funny results. > > Since I don't think you should want to pass a state to vblank you would have to use > some crtc local variable somewhere, in this case I chose to use hwmode for that. I guess plan be could be to the required values (and only those we need, not the entire mode struct) in struct intel_crtc. Not sure whether that's worth the bother. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index ca457317a8ac..9359ea2399f1 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -564,8 +564,7 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, int pipe) u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal; struct intel_crtc *intel_crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); - const struct drm_display_mode *mode = - &intel_crtc->config->base.adjusted_mode; + const struct drm_display_mode *mode = &intel_crtc->base.hwmode; htotal = mode->crtc_htotal; hsync_start = mode->crtc_hsync_start; @@ -620,7 +619,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc) { struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - const struct drm_display_mode *mode = &crtc->config->base.adjusted_mode; + const struct drm_display_mode *mode = &crtc->base.hwmode; enum pipe pipe = crtc->pipe; int position, vtotal; @@ -647,14 +646,14 @@ static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, struct drm_i915_private *dev_priv = dev->dev_private; struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - const struct drm_display_mode *mode = &intel_crtc->config->base.adjusted_mode; + const struct drm_display_mode *mode = &intel_crtc->base.hwmode; int position; int vbl_start, vbl_end, hsync_start, htotal, vtotal; bool in_vbl = true; int ret = 0; unsigned long irqflags; - if (!intel_crtc->active) { + if (WARN_ON(!mode->crtc_clock)) { DRM_DEBUG_DRIVER("trying to get scanoutpos for disabled " "pipe %c\n", pipe_name(pipe)); return 0; @@ -796,7 +795,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe, return -EINVAL; } - if (!crtc->state->active) { + if (!crtc->hwmode.crtc_clock) { DRM_DEBUG_KMS("crtc %d is disabled\n", pipe); return -EBUSY; } @@ -805,7 +804,7 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe, return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error, vblank_time, flags, crtc, - &to_intel_crtc(crtc)->config->base.adjusted_mode); + &crtc->hwmode); } static bool intel_hpd_irq_event(struct drm_device *dev, diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9316b0be4f5b..5063ac3d028d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11467,6 +11467,9 @@ intel_modeset_update_state(struct drm_atomic_state *state) WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc)); to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state); + + if (!crtc->state->active) + crtc->hwmode.crtc_clock = 0; } list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
intel_crtc->config will be removed eventually, so use crtc->hwmode. drm_atomic_helper_update_legacy_modeset_state updates hwmode, but crtc->active will eventually be gone too. Set dotclock to zero to indicate the crtc is inactive. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/i915_irq.c | 13 ++++++------- drivers/gpu/drm/i915/intel_display.c | 3 +++ 2 files changed, 9 insertions(+), 7 deletions(-)