| Message ID | 1400093477-3217-9-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 14, 2014 at 08:51:10PM +0200, Daniel Vetter wrote: > We need to start somewhere ... With this the only places left in i915 > where we use pipe integers is in the interrupt handling code. And > there it actually makes some amount of sense. Very much welcome addition. Some minor comments below. > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_irq.c | 81 ++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_display.c | 22 +++++----- Perhaps move the i915 changes into a separate commit? > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c [...] > /** > + * drm_crtc_vblank_get - get a reference count on vblank events > + * @dev: drm device > + * @crtc: which CRTC to own > + * > + * Acquire a reference count on vblank events to avoid having them disabled > + * while in use. > + * > + * This is the native kms version of drm_vblank_off(). > + * > + * Returns: > + * Zero on success, nonzero on failure. > + */ > +int drm_crtc_vblank_get(struct drm_device *dev, struct drm_crtc *crtc) > +{ > + return drm_vblank_get(dev, drm_crtc_index(crtc)); > +} > +EXPORT_SYMBOL(drm_crtc_vblank_get); This seems slightly backwards. Since drm_vblank_get() is what's being deprecated here, wouldn't it make more sense to write drm_crtc_vblank_get() in terms of struct drm_crtc and make drm_vblank_get() call that instead? I can't seem to find a helper to get the CRTC from an index, but it seems like that wouldn't be hard to do. I guess it doesn't matter all that much either way, though, since we could equally well make that change when drm_vblank_get() is dropped, in which case at least there's no longer a need for the reverse lookup. I'd still prefer to have i915 changes in a separate commit, but otherwise: Reviewed-by: Thierry Reding <treding@nvidia.com>
On Thu, May 15, 2014 at 9:34 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > This seems slightly backwards. Since drm_vblank_get() is what's being > deprecated here, wouldn't it make more sense to write > drm_crtc_vblank_get() in terms of struct drm_crtc and make > drm_vblank_get() call that instead? I can't seem to find a helper to get > the CRTC from an index, but it seems like that wouldn't be hard to do. Two reasons against this: - More ugly churn since it's a flag day, and when handling vblank refactorings what I _definitely_ want to avoid is whole-subsystem wide flag days. - drm_crtc_ is the common prefix used by many of the crtc functions. What I actually forgotten to do is drop the dev parameter, we can fish that out of the crtc. Then it should be even more obvious that this is a crtc function and rightfully deserve the drm_crtc_ prefix ;-) > I guess it doesn't matter all that much either way, though, since we > could equally well make that change when drm_vblank_get() is dropped, in > which case at least there's no longer a need for the reverse lookup. Yeah, the reverse lookup is something I want to add later on eventually. But that requires more thought since it only makes sense if we also switch the driver callbacks for vblank_enable/disable over. > I'd still prefer to have i915 changes in a separate commit, but > otherwise: Will do, makes indeed more sense. -Daniel
On Thu, May 15, 2014 at 12:10:16PM +0200, Daniel Vetter wrote: > On Thu, May 15, 2014 at 9:34 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > This seems slightly backwards. Since drm_vblank_get() is what's being > > deprecated here, wouldn't it make more sense to write > > drm_crtc_vblank_get() in terms of struct drm_crtc and make > > drm_vblank_get() call that instead? I can't seem to find a helper to get > > the CRTC from an index, but it seems like that wouldn't be hard to do. > > Two reasons against this: > - More ugly churn since it's a flag day, and when handling vblank > refactorings what I _definitely_ want to avoid is whole-subsystem wide > flag days. > - drm_crtc_ is the common prefix used by many of the crtc functions. > > What I actually forgotten to do is drop the dev parameter, we can fish > that out of the crtc. Then it should be even more obvious that this is > a crtc function and rightfully deserve the drm_crtc_ prefix ;-) I think you misunderstood what I was saying. What I proposed wasn't that drm_vblank_get() was replaced by a new implementation with different signature. Rather my suggestion was to implement the old drm_vblank_get() by calling drm_crtc_vblank_get() rather than the other way around. Something like this: int drm_crtc_vblank_get(struct drm_crtc *crtc) { new code using CRTC } int drm_vblank_get(struct drm_device *drm, int crtc) { struct drm_crtc *c = drm_crtc_from_index(crtc); return drm_crtc_vblank_get(c); } > > I guess it doesn't matter all that much either way, though, since we > > could equally well make that change when drm_vblank_get() is dropped, in > > which case at least there's no longer a need for the reverse lookup. > > Yeah, the reverse lookup is something I want to add later on > eventually. But that requires more thought since it only makes sense > if we also switch the driver callbacks for vblank_enable/disable over. On that note, is there a plan to move the vblank fields out of the DRM device and into drm_crtc as well? That seems like a logical next step since presumably every CRTC can handle it's own vblank events itself. Thierry
On Thu, May 15, 2014 at 12:42 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, May 15, 2014 at 12:10:16PM +0200, Daniel Vetter wrote: >> On Thu, May 15, 2014 at 9:34 AM, Thierry Reding >> <thierry.reding@gmail.com> wrote: >> > This seems slightly backwards. Since drm_vblank_get() is what's being >> > deprecated here, wouldn't it make more sense to write >> > drm_crtc_vblank_get() in terms of struct drm_crtc and make >> > drm_vblank_get() call that instead? I can't seem to find a helper to get >> > the CRTC from an index, but it seems like that wouldn't be hard to do. >> >> Two reasons against this: >> - More ugly churn since it's a flag day, and when handling vblank >> refactorings what I _definitely_ want to avoid is whole-subsystem wide >> flag days. >> - drm_crtc_ is the common prefix used by many of the crtc functions. >> >> What I actually forgotten to do is drop the dev parameter, we can fish >> that out of the crtc. Then it should be even more obvious that this is >> a crtc function and rightfully deserve the drm_crtc_ prefix ;-) > > I think you misunderstood what I was saying. What I proposed wasn't that > drm_vblank_get() was replaced by a new implementation with different > signature. Rather my suggestion was to implement the old > drm_vblank_get() by calling drm_crtc_vblank_get() rather than the other > way around. > > Something like this: > > int drm_crtc_vblank_get(struct drm_crtc *crtc) > { > new code using CRTC > } > > int drm_vblank_get(struct drm_device *drm, int crtc) > { > struct drm_crtc *c = drm_crtc_from_index(crtc); > > return drm_crtc_vblank_get(c); > } As long as the actual code doesn't deal in real drm_crtcs that imo makes little sense. It's really just the interface shim to start the long journey into a saner world ;-) >> > I guess it doesn't matter all that much either way, though, since we >> > could equally well make that change when drm_vblank_get() is dropped, in >> > which case at least there's no longer a need for the reverse lookup. >> >> Yeah, the reverse lookup is something I want to add later on >> eventually. But that requires more thought since it only makes sense >> if we also switch the driver callbacks for vblank_enable/disable over. > > On that note, is there a plan to move the vblank fields out of the DRM > device and into drm_crtc as well? That seems like a logical next step > since presumably every CRTC can handle it's own vblank events itself. Yeah, I think that's where we eventually want to go to. The problem is that the vblank code is deeply intertwined with legacy user-mode-setting drivers. We might need to do a copy-paste of drm_irq.c for kms drivers into a new drm_crtc_vblank.c file which exclusively deals with drm_crtcs. But I don't have any clear idea yet how to make that transition happen, hence this patch to start with something small and something we clearly want. -Daniel
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 5ff986bd4de4..51ebe9086be9 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -916,6 +916,8 @@ static int drm_vblank_enable(struct drm_device *dev, int crtc) * Acquire a reference count on vblank events to avoid having them disabled * while in use. * + * This is the legacy version of drm_crtc_vblank_get(). + * * Returns: * Zero on success, nonzero on failure. */ @@ -941,12 +943,33 @@ int drm_vblank_get(struct drm_device *dev, int crtc) EXPORT_SYMBOL(drm_vblank_get); /** + * drm_crtc_vblank_get - get a reference count on vblank events + * @dev: drm device + * @crtc: which CRTC to own + * + * Acquire a reference count on vblank events to avoid having them disabled + * while in use. + * + * This is the native kms version of drm_vblank_off(). + * + * Returns: + * Zero on success, nonzero on failure. + */ +int drm_crtc_vblank_get(struct drm_device *dev, struct drm_crtc *crtc) +{ + return drm_vblank_get(dev, drm_crtc_index(crtc)); +} +EXPORT_SYMBOL(drm_crtc_vblank_get); + +/** * drm_vblank_put - give up ownership of vblank events * @dev: drm device * @crtc: which counter to give up * * Release ownership of a given vblank counter, turning off interrupts * if possible. Disable interrupts after drm_vblank_offdelay milliseconds. + * + * This is the legacy version of drm_crtc_vblank_put(). */ void drm_vblank_put(struct drm_device *dev, int crtc) { @@ -961,6 +984,22 @@ void drm_vblank_put(struct drm_device *dev, int crtc) EXPORT_SYMBOL(drm_vblank_put); /** + * drm_crtc_vblank_put - give up ownership of vblank events + * @dev: drm device + * @crtc: which counter to give up + * + * Release ownership of a given vblank counter, turning off interrupts + * if possible. Disable interrupts after drm_vblank_offdelay milliseconds. + * + * This is the native kms version of drm_vblank_put(). + */ +void drm_crtc_vblank_put(struct drm_device *dev, struct drm_crtc *crtc) +{ + drm_vblank_put(dev, drm_crtc_index(crtc)); +} +EXPORT_SYMBOL(drm_crtc_vblank_put); + +/** * drm_vblank_off - disable vblank events on a CRTC * @dev: drm device * @crtc: CRTC in question @@ -971,6 +1010,8 @@ EXPORT_SYMBOL(drm_vblank_put); * * Drivers must use this function when the hardware vblank counter can get * reset, e.g. when suspending. + * + * This is the legacy version of drm_crtc_vblank_off(). */ void drm_vblank_off(struct drm_device *dev, int crtc) { @@ -1004,6 +1045,26 @@ void drm_vblank_off(struct drm_device *dev, int crtc) EXPORT_SYMBOL(drm_vblank_off); /** + * drm_crtc_vblank_off - disable vblank events on a CRTC + * @dev: drm device + * @crtc: CRTC in question + * + * Drivers can use this function to shut down the vblank interrupt handling when + * disabling a crtc. This function ensures that the latest vblank frame count is + * stored so that drm_vblank_on can restore it again. + * + * Drivers must use this function when the hardware vblank counter can get + * reset, e.g. when suspending. + * + * This is the native kms version of drm_vblank_off(). + */ +void drm_crtc_vblank_off(struct drm_device *dev, struct drm_crtc *crtc) +{ + drm_vblank_off(dev, drm_crtc_index(crtc)); +} +EXPORT_SYMBOL(drm_crtc_vblank_off); + +/** * drm_vblank_on - enable vblank events on a CRTC * @dev: drm device * @crtc: CRTC in question @@ -1012,6 +1073,8 @@ EXPORT_SYMBOL(drm_vblank_off); * drm_vblank_off() again. Note that calls to drm_vblank_on() and * drm_vblank_off() can be unbalanced and so can also be unconditionaly called * in driver load code to reflect the current hardware state of the crtc. + * + * This is the legacy version of drm_crtc_vblank_on(). */ void drm_vblank_on(struct drm_device *dev, int crtc) { @@ -1026,6 +1089,24 @@ void drm_vblank_on(struct drm_device *dev, int crtc) EXPORT_SYMBOL(drm_vblank_on); /** + * drm_crtc_vblank_on - enable vblank events on a CRTC + * @dev: drm device + * @crtc: CRTC in question + * + * This functions restores the vblank interrupt state captured with + * drm_vblank_off() again. Note that calls to drm_vblank_on() and + * drm_vblank_off() can be unbalanced and so can also be unconditionaly called + * in driver load code to reflect the current hardware state of the crtc. + * + * This is the native kms version of drm_vblank_on(). + */ +void drm_crtc_vblank_on(struct drm_device *dev, struct drm_crtc *crtc) +{ + drm_vblank_on(dev, drm_crtc_index(crtc)); +} +EXPORT_SYMBOL(drm_crtc_vblank_on); + +/** * drm_vblank_pre_modeset - account for vblanks across mode sets * @dev: drm device * @crtc: CRTC in question diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d0eff53a8ad1..d4abaa4bf2f4 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3664,7 +3664,7 @@ static void ilk_crtc_disable_planes(struct drm_crtc *crtc) int plane = intel_crtc->plane; intel_crtc_wait_for_pending_flips(crtc); - drm_vblank_off(dev, pipe); + drm_crtc_vblank_off(dev, crtc); if (dev_priv->fbc.plane == plane) intel_disable_fbc(dev); @@ -3740,7 +3740,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) */ intel_wait_for_vblank(dev, intel_crtc->pipe); - drm_vblank_on(dev, pipe); + drm_crtc_vblank_on(dev, crtc); } /* IPS only exists on ULT machines and is tied to pipe A. */ @@ -3833,7 +3833,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) haswell_mode_set_planes_workaround(intel_crtc); ilk_crtc_enable_planes(crtc); - drm_vblank_on(dev, pipe); + drm_crtc_vblank_on(dev, crtc); } static void ironlake_pfit_disable(struct intel_crtc *crtc) @@ -4358,7 +4358,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) for_each_encoder_on_crtc(dev, crtc, encoder) encoder->enable(encoder); - drm_vblank_on(dev, pipe); + drm_crtc_vblank_on(dev, crtc); } static void i9xx_crtc_enable(struct drm_crtc *crtc) @@ -4407,7 +4407,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc) for_each_encoder_on_crtc(dev, crtc, encoder) encoder->enable(encoder); - drm_vblank_on(dev, pipe); + drm_crtc_vblank_on(dev, crtc); } static void i9xx_pfit_disable(struct intel_crtc *crtc) @@ -4442,7 +4442,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) /* Give the overlay scaler a chance to disable if it's on this pipe */ intel_crtc_wait_for_pending_flips(crtc); - drm_vblank_off(dev, pipe); + drm_crtc_vblank_off(dev, crtc); if (dev_priv->fbc.plane == plane) intel_disable_fbc(dev); @@ -8526,7 +8526,7 @@ static void do_intel_finish_page_flip(struct drm_device *dev, if (work->event) drm_send_vblank_event(dev, intel_crtc->pipe, work->event); - drm_vblank_put(dev, intel_crtc->pipe); + drm_crtc_vblank_put(dev, crtc); spin_unlock_irqrestore(&dev->event_lock, flags); @@ -8918,7 +8918,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, work->old_fb_obj = to_intel_framebuffer(old_fb)->obj; INIT_WORK(&work->work, intel_unpin_work_fn); - ret = drm_vblank_get(dev, intel_crtc->pipe); + ret = drm_crtc_vblank_get(dev, crtc); if (ret) goto free_work; @@ -8927,7 +8927,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, if (intel_crtc->unpin_work) { spin_unlock_irqrestore(&dev->event_lock, flags); kfree(work); - drm_vblank_put(dev, intel_crtc->pipe); + drm_crtc_vblank_put(dev, crtc); DRM_DEBUG_DRIVER("flip queue: crtc already busy\n"); return -EBUSY; @@ -8979,7 +8979,7 @@ cleanup: intel_crtc->unpin_work = NULL; spin_unlock_irqrestore(&dev->event_lock, flags); - drm_vblank_put(dev, intel_crtc->pipe); + drm_crtc_vblank_put(dev, crtc); free_work: kfree(work); @@ -10579,6 +10579,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base; drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); + + WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe); } enum pipe intel_get_pipe_from_connector(struct intel_connector *connector) diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 7339b2b00724..455c782422dd 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1359,9 +1359,14 @@ extern void drm_send_vblank_event(struct drm_device *dev, int crtc, extern bool drm_handle_vblank(struct drm_device *dev, int crtc); extern int drm_vblank_get(struct drm_device *dev, int crtc); extern void drm_vblank_put(struct drm_device *dev, int crtc); +extern int drm_crtc_vblank_get(struct drm_device *dev, struct drm_crtc *crtc); +extern void drm_crtc_vblank_put(struct drm_device *dev, struct drm_crtc *crtc); extern void drm_vblank_off(struct drm_device *dev, int crtc); extern void drm_vblank_on(struct drm_device *dev, int crtc); +extern void drm_crtc_vblank_off(struct drm_device *dev, struct drm_crtc *crtc); +extern void drm_crtc_vblank_on(struct drm_device *dev, struct drm_crtc *crtc); extern void drm_vblank_cleanup(struct drm_device *dev); + extern u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc, struct timeval *tvblank, unsigned flags); extern int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
We need to start somewhere ... With this the only places left in i915 where we use pipe integers is in the interrupt handling code. And there it actually makes some amount of sense. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_irq.c | 81 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_display.c | 22 +++++----- include/drm/drmP.h | 5 +++ 3 files changed, 98 insertions(+), 10 deletions(-)