The following three patches address various minor nits. They are all safe and I have been running with them for several months on a wide variety of AMD GPUs
The first patch: [PATCH 1/3] drm/radeon/kms: use crtc-specific dpms functions in does not change the functionality and affects the function that is used only by r100 ASICs, but make the code more correct (strictly speaking).
The second patch: [PATCH 2/3] drm/radeon/kms: fix the crtc number check makes the crtc number check consistent with the way it is done in the rest of the driver code without affecting the functionality. It was actually sent a few months ago and reviewed, but apparently forgotten (cf. http://lists.freedesktop.org/archives/dri-devel/2011-May/010935.html)
The third patch: [PATCH 3/3] drm/radeon/kms: use num_crtc instead of hard-coded value Prevents a possible trouble waiting to happen if AMD ever makes a GPU with more than 6 CRTCs. I don't know what's cooking in their kitchen but if such a GPU ever comes out, this will prevent at least one regression ;-)
it's better that radeon_crtc_commit and radeon_crtc_prepare call crtc-specific dpms functions instead of hard-coding them to radeon_crtc_dpms.
Signed-off-by: Ilija Hadzic ihadzic@research.bell-labs.com --- drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 14 ++++++++++---- 1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c index 41a5d48..0690a5b 100644 --- a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c +++ b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c @@ -1036,8 +1036,11 @@ static void radeon_crtc_prepare(struct drm_crtc *crtc) * The hardware wedges sometimes if you reconfigure one CRTC * whilst another is running (see fdo bug #24611). */ - list_for_each_entry(crtci, &dev->mode_config.crtc_list, head) - radeon_crtc_dpms(crtci, DRM_MODE_DPMS_OFF); + list_for_each_entry(crtci, &dev->mode_config.crtc_list, head) { + struct drm_crtc_helper_funcs *crtc_funcs = crtci->helper_private; + if (crtc_funcs->dpms) + crtc_funcs->dpms(crtci, DRM_MODE_DPMS_OFF); + } }
static void radeon_crtc_commit(struct drm_crtc *crtc) @@ -1049,8 +1052,11 @@ static void radeon_crtc_commit(struct drm_crtc *crtc) * Reenable the CRTCs that should be running. */ list_for_each_entry(crtci, &dev->mode_config.crtc_list, head) { - if (crtci->enabled) - radeon_crtc_dpms(crtci, DRM_MODE_DPMS_ON); + if (crtci->enabled) { + struct drm_crtc_helper_funcs *crtc_funcs = crtci->helper_private; + if (crtc_funcs->dpms) + crtc_funcs->dpms(crtci, DRM_MODE_DPMS_ON); + } } }
On Die, 2011-10-25 at 22:40 -0400, Ilija Hadzic wrote:
it's better that radeon_crtc_commit and radeon_crtc_prepare call crtc-specific dpms functions instead of hard-coding them to radeon_crtc_dpms.
Is it really better? If it's always radeon_crtc_dpms anyway, this obfuscates that fact (and introduces a guaranteed branch prediction miss, but that probably doesn't matter here).
On Wed, 26 Oct 2011, Michel [ISO-8859-1] D�nzer wrote:
On Die, 2011-10-25 at 22:40 -0400, Ilija Hadzic wrote:
it's better that radeon_crtc_commit and radeon_crtc_prepare call crtc-specific dpms functions instead of hard-coding them to radeon_crtc_dpms.
Is it really better? If it's always radeon_crtc_dpms anyway, this obfuscates that fact (and introduces a guaranteed branch prediction miss, but that probably doesn't matter here).
My reasoning for believing that it's better was that the functions are fairly generic: they loops through all CRTCs and disable/enable them all.
Although at this time these functions are only used as helpers for legacy crtcs (so indeed the pointer always resolves to radeon_crtc_dpms), if they are ever called through some other path (in the context of atombios CRTC), it doesn't hurt to make them ready for that.
Having said that, I don't really mind whatever the destiny of this patch ends up being.
(and yes, branch prediction slot penalty really don't matter here; it's a slow path anyway)
-- Ilija
the crtc check in radeon_get_vblank_timestamp_kms should be against the num_crtc field in radeon_device not against num_crtcs in drm_device
Signed-off-by: Ilija Hadzic ihadzic@research.bell-labs.com --- drivers/gpu/drm/radeon/radeon_kms.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index a749c26..540a9ee 100644 --- a/drivers/gpu/drm/radeon/radeon_kms.c +++ b/drivers/gpu/drm/radeon/radeon_kms.c @@ -347,7 +347,7 @@ int radeon_get_vblank_timestamp_kms(struct drm_device *dev, int crtc, struct drm_crtc *drmcrtc; struct radeon_device *rdev = dev->dev_private;
- if (crtc < 0 || crtc >= dev->num_crtcs) { + if (crtc < 0 || crtc >= rdev->num_crtc) { DRM_ERROR("Invalid crtc %d\n", crtc); return -EINVAL; }
On Tue, Oct 25, 2011 at 10:40 PM, Ilija Hadzic ihadzic@research.bell-labs.com wrote:
the crtc check in radeon_get_vblank_timestamp_kms should be against the num_crtc field in radeon_device not against num_crtcs in drm_device
This more correct, but would there ever be a case where they are different?
Alex
Reviewed-by: Alex Deucher alexander.deucher@amd.com
Signed-off-by: Ilija Hadzic ihadzic@research.bell-labs.com
drivers/gpu/drm/radeon/radeon_kms.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index a749c26..540a9ee 100644 --- a/drivers/gpu/drm/radeon/radeon_kms.c +++ b/drivers/gpu/drm/radeon/radeon_kms.c @@ -347,7 +347,7 @@ int radeon_get_vblank_timestamp_kms(struct drm_device *dev, int crtc, struct drm_crtc *drmcrtc; struct radeon_device *rdev = dev->dev_private;
- if (crtc < 0 || crtc >= dev->num_crtcs) {
- if (crtc < 0 || crtc >= rdev->num_crtc) {
DRM_ERROR("Invalid crtc %d\n", crtc); return -EINVAL; } -- 1.7.7
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
radeon_driver_irq_preinstall_kms and radeon_driver_irq_uninstall_kms hard code the loop to 6 which happens to be the current maximum number of crtcs; if one day an ASIC with more crtcs comes out, this is a trouble waiting to happen. it's better to use num_crtc instead (for ASICs that have fewer than 6 CRTCs, this is still OK because higher numbers won't be looked at)
Signed-off-by: Ilija Hadzic ihadzic@research.bell-labs.com --- drivers/gpu/drm/radeon/radeon_irq_kms.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_irq_kms.c b/drivers/gpu/drm/radeon/radeon_irq_kms.c index 9ec830c..a0f9d24 100644 --- a/drivers/gpu/drm/radeon/radeon_irq_kms.c +++ b/drivers/gpu/drm/radeon/radeon_irq_kms.c @@ -69,7 +69,7 @@ void radeon_driver_irq_preinstall_kms(struct drm_device *dev) rdev->irq.gui_idle = false; for (i = 0; i < rdev->num_crtc; i++) rdev->irq.crtc_vblank_int[i] = false; - for (i = 0; i < 6; i++) { + for (i = 0; i < rdev->num_crtc; i++) { rdev->irq.hpd[i] = false; rdev->irq.pflip[i] = false; } @@ -101,7 +101,7 @@ void radeon_driver_irq_uninstall_kms(struct drm_device *dev) rdev->irq.gui_idle = false; for (i = 0; i < rdev->num_crtc; i++) rdev->irq.crtc_vblank_int[i] = false; - for (i = 0; i < 6; i++) { + for (i = 0; i < rdev->num_crtc; i++) { rdev->irq.hpd[i] = false; rdev->irq.pflip[i] = false; }
On Tue, Oct 25, 2011 at 10:40 PM, Ilija Hadzic ihadzic@research.bell-labs.com wrote:
radeon_driver_irq_preinstall_kms and radeon_driver_irq_uninstall_kms hard code the loop to 6 which happens to be the current maximum number of crtcs; if one day an ASIC with more crtcs comes out, this is a trouble waiting to happen. it's better to use num_crtc instead (for ASICs that have fewer than 6 CRTCs, this is still OK because higher numbers won't be looked at)
This is actually not quite right. The number of HPD (Hot Plug Detect) pins is not equal to the number of crtcs. Radeons have supported 6 HPD pins long before we supported 6 crtcs (e.g., cards with more connectors than crtcs). The logic should probably look like:
#define RADEON_MAX_HPD_PINS 6
for (i = 0; i < rdev->num_crtc; i++) { rdev->irq.crtc_vblank_int[i] = false; rdev->irq.pflip[i] = false; }
for (i = 0; i < RADEON_MAX_HPD_PINS; i++) rdev->irq.hpd[i] = false;
Alex
Signed-off-by: Ilija Hadzic ihadzic@research.bell-labs.com
drivers/gpu/drm/radeon/radeon_irq_kms.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_irq_kms.c b/drivers/gpu/drm/radeon/radeon_irq_kms.c index 9ec830c..a0f9d24 100644 --- a/drivers/gpu/drm/radeon/radeon_irq_kms.c +++ b/drivers/gpu/drm/radeon/radeon_irq_kms.c @@ -69,7 +69,7 @@ void radeon_driver_irq_preinstall_kms(struct drm_device *dev) rdev->irq.gui_idle = false; for (i = 0; i < rdev->num_crtc; i++) rdev->irq.crtc_vblank_int[i] = false;
- for (i = 0; i < 6; i++) {
- for (i = 0; i < rdev->num_crtc; i++) {
rdev->irq.hpd[i] = false; rdev->irq.pflip[i] = false; } @@ -101,7 +101,7 @@ void radeon_driver_irq_uninstall_kms(struct drm_device *dev) rdev->irq.gui_idle = false; for (i = 0; i < rdev->num_crtc; i++) rdev->irq.crtc_vblank_int[i] = false;
- for (i = 0; i < 6; i++) {
- for (i = 0; i < rdev->num_crtc; i++) {
rdev->irq.hpd[i] = false; rdev->irq.pflip[i] = false; } -- 1.7.7
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, 26 Oct 2011, Alex Deucher wrote:
This is actually not quite right. The number of HPD (Hot Plug Detect) pins is not equal to the number of crtcs. Radeons have supported 6 HPD pins long before we supported 6 crtcs (e.g., cards with more connectors than crtcs). The logic should probably look like: [SNIP]
I have just sent out a patch that I think rectifies this. Hopefully, it's good now.
-- Ilija
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Ilija Hadzic -
Michel Dänzer