Re: [Intel-gfx] [PATCH 08/12] drm/irq: Add kms-native crtc interface functions
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.vet...@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 tred...@nvidia.com
pgpk7N3wuAH7q.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/12] drm/irq: Add kms-native crtc interface functions
On Thu, May 15, 2014 at 9:34 AM, Thierry Reding thierry.red...@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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/12] drm/irq: Add kms-native crtc interface functions
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.red...@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
pgp8l8OtR49k1.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/12] drm/irq: Add kms-native crtc interface functions
On Thu, May 15, 2014 at 12:42 PM, Thierry Reding
thierry.red...@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.red...@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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 08/12] drm/irq: Add kms-native crtc interface functions
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.vet...@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(-)
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));
+}
