The following patches contain some fixes and cleanups for the drm core.
- fix memory holes - make some initialization / deinitialization more symmetric - add convenience functions for creating properties - remove DRM_CONNECTOR_MAX_PROPERTY limitation
All patches tested on a GeForce 6200 LE with the nouveau driver and a DELL E6220 Laptop using the intel driver.
Please review and consider applying
Sascha
Sascha Hauer (20): drm crtc: use drm_mode_destroy instead of kfree in drm_mode_remove drm crtc: add forgotten idr cleanup functions drm drm_edit: drm modes have to be free with drm_mode_destroy drm drm_fb_helper: destroy modes drm: add proper return value for drm_mode_crtc_set_gamma_size drm fb helper: use drm_helper_connector_dpms to do dpms drm fb helper: remove unused variable conn_limit drm fb helper: remove unused variable crtc_id drm fb_helper: use lists for crtcs. drm: remove now unused crtc_count parameter from drm_fb_helper_init drm fb helper: add the connectors inside drm_fb_helper_initial_config drm crtc_helper: use list_for_each_entry drm crtc: Fix locking comments drm: add convenience function to create an enum property drm: add convenience function to create an range property drm: store connector properties in list drm: remove checks for same value in set_prop drm: do not call drm_connector_property_set_value from drivers drm exynos: use drm_fb_helper_set_par directly drm: do not set fb_info->pixmap fields
drivers/gpu/drm/drm_crtc.c | 315 +++++++++++++-------------- drivers/gpu/drm/drm_crtc_helper.c | 12 +- drivers/gpu/drm/drm_edid.c | 2 +- drivers/gpu/drm/drm_fb_helper.c | 216 +++++++------------ drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 49 +---- drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 4 - drivers/gpu/drm/gma500/cdv_intel_lvds.c | 33 +--- drivers/gpu/drm/gma500/framebuffer.c | 15 +- drivers/gpu/drm/gma500/psb_intel_lvds.c | 36 +--- drivers/gpu/drm/gma500/psb_intel_sdvo.c | 45 +---- drivers/gpu/drm/i2c/ch7006_drv.c | 5 +- drivers/gpu/drm/i915/intel_dp.c | 11 - drivers/gpu/drm/i915/intel_fb.c | 11 +- drivers/gpu/drm/i915/intel_hdmi.c | 5 - drivers/gpu/drm/i915/intel_modes.c | 28 +-- drivers/gpu/drm/i915/intel_sdvo.c | 35 +--- drivers/gpu/drm/i915/intel_tv.c | 31 +--- drivers/gpu/drm/nouveau/nouveau_connector.c | 32 +-- drivers/gpu/drm/nouveau/nouveau_display.c | 20 +- drivers/gpu/drm/nouveau/nouveau_fbcon.c | 11 +- drivers/gpu/drm/radeon/radeon_connectors.c | 24 +-- drivers/gpu/drm/radeon/radeon_display.c | 70 ++----- drivers/gpu/drm/radeon/radeon_fb.c | 11 +- drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 14 -- drivers/video/nvidia/nvidia.c | 6 - include/drm/drm_crtc.h | 24 ++- include/drm/drm_fb_helper.h | 9 +- 27 files changed, 356 insertions(+), 718 deletions(-)
Modes are created using drm_mode_create which does a drm_mode_object_get, so use drm_mode_destroy in drm_mode_remove which does a drm_mode_object_put.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_crtc.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 5e818a8..8389fd3 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -442,7 +442,7 @@ void drm_mode_remove(struct drm_connector *connector, struct drm_display_mode *mode) { list_del(&mode->head); - kfree(mode); + drm_mode_destroy(connector->dev, mode); } EXPORT_SYMBOL(drm_mode_remove);
drm_mode_config_init initializes the idr with idr_init, so add the missing counterparts in drm_mode_config_cleanup.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_crtc.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 8389fd3..33ebe29 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1048,6 +1048,9 @@ void drm_mode_config_cleanup(struct drm_device *dev) head) { plane->funcs->destroy(plane); } + + idr_remove_all(&dev->mode_config.crtc_idr); + idr_destroy(&dev->mode_config.crtc_idr); } EXPORT_SYMBOL(drm_mode_config_cleanup);
to add the missing drm_mode_object_put for that mode.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_edid.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index ece03fc..770d894 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -745,7 +745,7 @@ drm_mode_std(struct drm_connector *connector, struct edid *edid, */ mode = drm_gtf_mode(dev, hsize, vsize, vrefresh_rate, 0, 0); if (drm_mode_hsync(mode) > drm_gtf2_hbreak(edid)) { - kfree(mode); + drm_mode_destroy(dev, mode); mode = drm_gtf_mode_complex(dev, hsize, vsize, vrefresh_rate, 0, 0, drm_gtf2_m(edid),
drm_setup_crtcs allocated modes using drm_mode_duplicate. Free them in drm_fb_helper_crtc_free.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_fb_helper.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index aada26f..77fec5a 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -430,8 +430,11 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper) for (i = 0; i < helper->connector_count; i++) kfree(helper->connector_info[i]); kfree(helper->connector_info); - for (i = 0; i < helper->crtc_count; i++) + for (i = 0; i < helper->crtc_count; i++) { kfree(helper->crtc_info[i].mode_set.connectors); + if (helper->crtc_info[i].mode_set.mode) + drm_mode_destroy(helper->dev, helper->crtc_info[i].mode_set.mode); + } kfree(helper->crtc_info); }
drm_mode_crtc_set_gamma_size returns boolean true for success and false for failure. This is not very kernel conform, so change it to return 0 for success and a propert error code otherwise. Noone checks the return value, so no users have to be fixed.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_crtc.c | 6 +++--- include/drm/drm_crtc.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 33ebe29..df6e413 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3024,7 +3024,7 @@ void drm_mode_connector_detach_encoder(struct drm_connector *connector, } EXPORT_SYMBOL(drm_mode_connector_detach_encoder);
-bool drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc, +int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc, int gamma_size) { crtc->gamma_size = gamma_size; @@ -3032,10 +3032,10 @@ bool drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc, crtc->gamma_store = kzalloc(gamma_size * sizeof(uint16_t) * 3, GFP_KERNEL); if (!crtc->gamma_store) { crtc->gamma_size = 0; - return false; + return -ENOMEM; }
- return true; + return 0; } EXPORT_SYMBOL(drm_mode_crtc_set_gamma_size);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 4cd4be2..8d593ad 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -919,7 +919,7 @@ extern int drm_mode_connector_attach_encoder(struct drm_connector *connector, struct drm_encoder *encoder); extern void drm_mode_connector_detach_encoder(struct drm_connector *connector, struct drm_encoder *encoder); -extern bool drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc, +extern int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc, int gamma_size); extern struct drm_mode_object *drm_mode_object_find(struct drm_device *dev, uint32_t id, uint32_t type);
drm_fb_helper_on|off currently manually searches for encoders to turn on/off. Make this simpler by using the helper function.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_fb_helper.c | 80 +++++---------------------------------- 1 files changed, 10 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 77fec5a..4fc38a7f 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -306,91 +306,31 @@ static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { }; #endif
-static void drm_fb_helper_on(struct fb_info *info) +static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) { struct drm_fb_helper *fb_helper = info->par; struct drm_device *dev = fb_helper->dev; struct drm_crtc *crtc; - struct drm_crtc_helper_funcs *crtc_funcs; struct drm_connector *connector; - struct drm_encoder *encoder; int i, j;
/* - * For each CRTC in this fb, turn the crtc on then, - * find all associated encoders and turn them on. + * For each CRTC in this fb, turn the connectors on/off. */ mutex_lock(&dev->mode_config.mutex); for (i = 0; i < fb_helper->crtc_count; i++) { crtc = fb_helper->crtc_info[i].mode_set.crtc; - crtc_funcs = crtc->helper_private;
if (!crtc->enabled) continue;
- crtc_funcs->dpms(crtc, DRM_MODE_DPMS_ON); - - /* Walk the connectors & encoders on this fb turning them on */ + /* Walk the connectors & encoders on this fb turning them on/off */ for (j = 0; j < fb_helper->connector_count; j++) { connector = fb_helper->connector_info[j]->connector; - connector->dpms = DRM_MODE_DPMS_ON; + drm_helper_connector_dpms(connector, dpms_mode); drm_connector_property_set_value(connector, - dev->mode_config.dpms_property, - DRM_MODE_DPMS_ON); - } - /* Found a CRTC on this fb, now find encoders */ - list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { - if (encoder->crtc == crtc) { - struct drm_encoder_helper_funcs *encoder_funcs; - - encoder_funcs = encoder->helper_private; - encoder_funcs->dpms(encoder, DRM_MODE_DPMS_ON); - } - } - } - mutex_unlock(&dev->mode_config.mutex); -} - -static void drm_fb_helper_off(struct fb_info *info, int dpms_mode) -{ - struct drm_fb_helper *fb_helper = info->par; - struct drm_device *dev = fb_helper->dev; - struct drm_crtc *crtc; - struct drm_crtc_helper_funcs *crtc_funcs; - struct drm_connector *connector; - struct drm_encoder *encoder; - int i, j; - - /* - * For each CRTC in this fb, find all associated encoders - * and turn them off, then turn off the CRTC. - */ - mutex_lock(&dev->mode_config.mutex); - for (i = 0; i < fb_helper->crtc_count; i++) { - crtc = fb_helper->crtc_info[i].mode_set.crtc; - crtc_funcs = crtc->helper_private; - - if (!crtc->enabled) - continue; - - /* Walk the connectors on this fb and mark them off */ - for (j = 0; j < fb_helper->connector_count; j++) { - connector = fb_helper->connector_info[j]->connector; - connector->dpms = dpms_mode; - drm_connector_property_set_value(connector, - dev->mode_config.dpms_property, - dpms_mode); - } - /* Found a CRTC on this fb, now find encoders */ - list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { - if (encoder->crtc == crtc) { - struct drm_encoder_helper_funcs *encoder_funcs; - - encoder_funcs = encoder->helper_private; - encoder_funcs->dpms(encoder, dpms_mode); - } + dev->mode_config.dpms_property, dpms_mode); } - crtc_funcs->dpms(crtc, DRM_MODE_DPMS_OFF); } mutex_unlock(&dev->mode_config.mutex); } @@ -400,23 +340,23 @@ int drm_fb_helper_blank(int blank, struct fb_info *info) switch (blank) { /* Display: On; HSync: On, VSync: On */ case FB_BLANK_UNBLANK: - drm_fb_helper_on(info); + drm_fb_helper_dpms(info, DRM_MODE_DPMS_ON); break; /* Display: Off; HSync: On, VSync: On */ case FB_BLANK_NORMAL: - drm_fb_helper_off(info, DRM_MODE_DPMS_STANDBY); + drm_fb_helper_dpms(info, DRM_MODE_DPMS_STANDBY); break; /* Display: Off; HSync: Off, VSync: On */ case FB_BLANK_HSYNC_SUSPEND: - drm_fb_helper_off(info, DRM_MODE_DPMS_STANDBY); + drm_fb_helper_dpms(info, DRM_MODE_DPMS_STANDBY); break; /* Display: Off; HSync: On, VSync: Off */ case FB_BLANK_VSYNC_SUSPEND: - drm_fb_helper_off(info, DRM_MODE_DPMS_SUSPEND); + drm_fb_helper_dpms(info, DRM_MODE_DPMS_SUSPEND); break; /* Display: Off; HSync: Off, VSync: Off */ case FB_BLANK_POWERDOWN: - drm_fb_helper_off(info, DRM_MODE_DPMS_OFF); + drm_fb_helper_dpms(info, DRM_MODE_DPMS_OFF); break; } return 0;
conn_limit is set but never used. Remove it from struct drm_fb_helper.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_fb_helper.c | 2 +- include/drm/drm_fb_helper.h | 1 - 2 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 4fc38a7f..7b37874 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -421,7 +421,7 @@ int drm_fb_helper_init(struct drm_device *dev, fb_helper->crtc_info[i].mode_set.crtc = crtc; i++; } - fb_helper->conn_limit = max_conn_count; + return 0; out_free: drm_fb_helper_crtc_free(fb_helper); diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 6e3076a..55e10d6 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -74,7 +74,6 @@ struct drm_fb_helper { int connector_count; struct drm_fb_helper_connector **connector_info; struct drm_fb_helper_funcs *funcs; - int conn_limit; struct fb_info *fbdev; u32 pseudo_palette[17]; struct list_head kernel_fb_list;
crtc_id is set but never used, so remove it from struct drm_fb_helper_crtc.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_fb_helper.c | 1 - include/drm/drm_fb_helper.h | 1 - 2 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 7b37874..7740dd2 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -417,7 +417,6 @@ int drm_fb_helper_init(struct drm_device *dev,
i = 0; list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { - fb_helper->crtc_info[i].crtc_id = crtc->base.id; fb_helper->crtc_info[i].mode_set.crtc = crtc; i++; } diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 55e10d6..5120b01 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -35,7 +35,6 @@ struct drm_fb_helper; #include <linux/kgdb.h>
struct drm_fb_helper_crtc { - uint32_t crtc_id; struct drm_mode_set mode_set; struct drm_display_mode *desired_mode; };
The fb helper uses fixed size arrays for the associated crtcs. This is an unnecessary limitation, so instead use a list to store the crtcs and allocate them dynamically.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_fb_helper.c | 129 ++++++++++++++++++++------------------- include/drm/drm_fb_helper.h | 3 +- 2 files changed, 68 insertions(+), 64 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 7740dd2..f292a78 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -147,15 +147,14 @@ int drm_fb_helper_debug_enter(struct fb_info *info) { struct drm_fb_helper *helper = info->par; struct drm_crtc_helper_funcs *funcs; - int i;
if (list_empty(&kernel_fb_helper_list)) return false;
list_for_each_entry(helper, &kernel_fb_helper_list, kernel_fb_list) { - for (i = 0; i < helper->crtc_count; i++) { - struct drm_mode_set *mode_set = - &helper->crtc_info[i].mode_set; + struct drm_fb_helper_crtc *helper_crtc; + list_for_each_entry(helper_crtc, &helper->crtc_list, list) { + struct drm_mode_set *mode_set = &helper_crtc->mode_set;
if (!mode_set->crtc->enabled) continue; @@ -194,10 +193,10 @@ int drm_fb_helper_debug_leave(struct fb_info *info) struct drm_crtc *crtc; struct drm_crtc_helper_funcs *funcs; struct drm_framebuffer *fb; - int i; + struct drm_fb_helper_crtc *helper_crtc;
- for (i = 0; i < helper->crtc_count; i++) { - struct drm_mode_set *mode_set = &helper->crtc_info[i].mode_set; + list_for_each_entry(helper_crtc, &helper->crtc_list, list) { + struct drm_mode_set *mode_set = &helper_crtc->mode_set; crtc = mode_set->crtc; funcs = crtc->helper_private; fb = drm_mode_config_fb(crtc); @@ -221,10 +220,12 @@ EXPORT_SYMBOL(drm_fb_helper_debug_leave);
bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper) { + struct drm_fb_helper_crtc *helper_crtc; bool error = false; - int i, ret; - for (i = 0; i < fb_helper->crtc_count; i++) { - struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set; + int ret; + + list_for_each_entry(helper_crtc, &fb_helper->crtc_list, list) { + struct drm_mode_set *mode_set = &helper_crtc->mode_set; ret = drm_crtc_helper_set_config(mode_set); if (ret) error = true; @@ -312,14 +313,15 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) struct drm_device *dev = fb_helper->dev; struct drm_crtc *crtc; struct drm_connector *connector; - int i, j; + struct drm_fb_helper_crtc *helper_crtc; + int j;
/* * For each CRTC in this fb, turn the connectors on/off. */ mutex_lock(&dev->mode_config.mutex); - for (i = 0; i < fb_helper->crtc_count; i++) { - crtc = fb_helper->crtc_info[i].mode_set.crtc; + list_for_each_entry(helper_crtc, &fb_helper->crtc_list, list) { + crtc = helper_crtc->mode_set.crtc;
if (!crtc->enabled) continue; @@ -365,17 +367,20 @@ EXPORT_SYMBOL(drm_fb_helper_blank);
static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper) { + struct drm_fb_helper_crtc *helper_crtc, *tmp; int i;
for (i = 0; i < helper->connector_count; i++) kfree(helper->connector_info[i]); + kfree(helper->connector_info); - for (i = 0; i < helper->crtc_count; i++) { - kfree(helper->crtc_info[i].mode_set.connectors); - if (helper->crtc_info[i].mode_set.mode) - drm_mode_destroy(helper->dev, helper->crtc_info[i].mode_set.mode); + + list_for_each_entry_safe(helper_crtc, tmp, &helper->crtc_list, list) { + kfree(helper_crtc->mode_set.connectors); + if (helper_crtc->mode_set.mode) + drm_mode_destroy(helper->dev, helper_crtc->mode_set.mode); + kfree(helper_crtc); } - kfree(helper->crtc_info); }
int drm_fb_helper_init(struct drm_device *dev, @@ -383,42 +388,40 @@ int drm_fb_helper_init(struct drm_device *dev, int crtc_count, int max_conn_count) { struct drm_crtc *crtc; + struct drm_fb_helper_crtc *helper_crtc; int ret = 0; - int i;
fb_helper->dev = dev;
INIT_LIST_HEAD(&fb_helper->kernel_fb_list); + INIT_LIST_HEAD(&fb_helper->crtc_list);
- fb_helper->crtc_info = kcalloc(crtc_count, sizeof(struct drm_fb_helper_crtc), GFP_KERNEL); - if (!fb_helper->crtc_info) + fb_helper->crtc_count = 0; + fb_helper->connector_info = kcalloc(dev->mode_config.num_connector, + sizeof(struct drm_fb_helper_connector *), GFP_KERNEL); + if (!fb_helper->connector_info) return -ENOMEM;
- fb_helper->crtc_count = crtc_count; - fb_helper->connector_info = kcalloc(dev->mode_config.num_connector, sizeof(struct drm_fb_helper_connector *), GFP_KERNEL); - if (!fb_helper->connector_info) { - kfree(fb_helper->crtc_info); - return -ENOMEM; - } fb_helper->connector_count = 0;
- for (i = 0; i < crtc_count; i++) { - fb_helper->crtc_info[i].mode_set.connectors = + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { + helper_crtc = kzalloc(sizeof(*helper_crtc), GFP_KERNEL); + if (!helper_crtc) + goto out_free; + + helper_crtc->mode_set.crtc = crtc; + helper_crtc->mode_set.connectors = kcalloc(max_conn_count, sizeof(struct drm_connector *), GFP_KERNEL);
- if (!fb_helper->crtc_info[i].mode_set.connectors) { + if (!helper_crtc->mode_set.connectors) { ret = -ENOMEM; goto out_free; } - fb_helper->crtc_info[i].mode_set.num_connectors = 0; - } - - i = 0; - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { - fb_helper->crtc_info[i].mode_set.crtc = crtc; - i++; + helper_crtc->mode_set.num_connectors = 0; + list_add_tail(&helper_crtc->list, &fb_helper->crtc_list); + fb_helper->crtc_count++; }
return 0; @@ -515,11 +518,12 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) struct drm_crtc_helper_funcs *crtc_funcs; u16 *red, *green, *blue, *transp; struct drm_crtc *crtc; - int i, j, rc = 0; + struct drm_fb_helper_crtc *helper_crtc; + int j, rc = 0; int start;
- for (i = 0; i < fb_helper->crtc_count; i++) { - crtc = fb_helper->crtc_info[i].mode_set.crtc; + list_for_each_entry(helper_crtc, &fb_helper->crtc_list, list) { + crtc = helper_crtc->mode_set.crtc; crtc_funcs = crtc->helper_private;
red = cmap->red; @@ -642,9 +646,9 @@ int drm_fb_helper_set_par(struct fb_info *info) struct drm_fb_helper *fb_helper = info->par; struct drm_device *dev = fb_helper->dev; struct fb_var_screeninfo *var = &info->var; + struct drm_fb_helper_crtc *helper_crtc; struct drm_crtc *crtc; int ret; - int i;
if (var->pixclock != 0) { DRM_ERROR("PIXEL CLOCK SET\n"); @@ -652,9 +656,9 @@ int drm_fb_helper_set_par(struct fb_info *info) }
mutex_lock(&dev->mode_config.mutex); - for (i = 0; i < fb_helper->crtc_count; i++) { - crtc = fb_helper->crtc_info[i].mode_set.crtc; - ret = crtc->funcs->set_config(&fb_helper->crtc_info[i].mode_set); + list_for_each_entry(helper_crtc, &fb_helper->crtc_list, list) { + crtc = helper_crtc->mode_set.crtc; + ret = crtc->funcs->set_config(&helper_crtc->mode_set); if (ret) { mutex_unlock(&dev->mode_config.mutex); return ret; @@ -676,15 +680,15 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, struct drm_fb_helper *fb_helper = info->par; struct drm_device *dev = fb_helper->dev; struct drm_mode_set *modeset; + struct drm_fb_helper_crtc *helper_crtc; struct drm_crtc *crtc; int ret = 0; - int i;
mutex_lock(&dev->mode_config.mutex); - for (i = 0; i < fb_helper->crtc_count; i++) { - crtc = fb_helper->crtc_info[i].mode_set.crtc; + list_for_each_entry(helper_crtc, &fb_helper->crtc_list, list) { + modeset = &helper_crtc->mode_set; + crtc = modeset->crtc;
- modeset = &fb_helper->crtc_info[i].mode_set;
modeset->x = var->xoffset; modeset->y = var->yoffset; @@ -705,6 +709,7 @@ EXPORT_SYMBOL(drm_fb_helper_pan_display); int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, int preferred_bpp) { + struct drm_fb_helper_crtc *helper_crtc; int new_fb = 0; int crtc_count = 0; int i; @@ -755,13 +760,13 @@ int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, }
crtc_count = 0; - for (i = 0; i < fb_helper->crtc_count; i++) { + list_for_each_entry(helper_crtc, &fb_helper->crtc_list, list) { struct drm_display_mode *desired_mode; - desired_mode = fb_helper->crtc_info[i].desired_mode; + desired_mode = helper_crtc->desired_mode;
if (desired_mode) { if (gamma_size == 0) - gamma_size = fb_helper->crtc_info[i].mode_set.crtc->gamma_size; + gamma_size = helper_crtc->mode_set.crtc->gamma_size; if (desired_mode->hdisplay < sizes.fb_width) sizes.fb_width = desired_mode->hdisplay; if (desired_mode->vdisplay < sizes.fb_height) @@ -790,16 +795,15 @@ int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, info = fb_helper->fbdev;
/* set the fb pointer */ - for (i = 0; i < fb_helper->crtc_count; i++) { - fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb; - } + list_for_each_entry(helper_crtc, &fb_helper->crtc_list, list) + helper_crtc->mode_set.fb = fb_helper->fb;
if (new_fb) { info->var.pixclock = 0; if (register_framebuffer(info) < 0) { return -EINVAL; } - + drm_fb_helper_blank(FB_BLANK_UNBLANK, info); printk(KERN_INFO "fb%d: %s frame buffer device\n", info->node, info->fix.id);
@@ -1184,12 +1188,13 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
/* select a crtc for this connector and then attempt to configure remaining connectors */ - for (c = 0; c < fb_helper->crtc_count; c++) { - crtc = &fb_helper->crtc_info[c]; - + c = 0; + list_for_each_entry(crtc, &fb_helper->crtc_list, list) { if ((encoder->possible_crtcs & (1 << c)) == 0) { + c++; continue; } + c++;
for (o = 0; o < n; o++) if (best_crtcs[o] == crtc) @@ -1224,7 +1229,7 @@ out: static void drm_setup_crtcs(struct drm_fb_helper *fb_helper) { struct drm_device *dev = fb_helper->dev; - struct drm_fb_helper_crtc **crtcs; + struct drm_fb_helper_crtc **crtcs, *helper_crtc; struct drm_display_mode **modes; struct drm_encoder *encoder; struct drm_mode_set *modeset; @@ -1264,10 +1269,8 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper)
/* need to set the modesets up here for use later */ /* fill out the connector<->crtc mappings into the modesets */ - for (i = 0; i < fb_helper->crtc_count; i++) { - modeset = &fb_helper->crtc_info[i].mode_set; - modeset->num_connectors = 0; - } + list_for_each_entry(helper_crtc, &fb_helper->crtc_list, list) + helper_crtc->mode_set.num_connectors = 0;
for (i = 0; i < fb_helper->connector_count; i++) { struct drm_display_mode *mode = modes[i]; diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 5120b01..e1e1c02 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -37,6 +37,7 @@ struct drm_fb_helper; struct drm_fb_helper_crtc { struct drm_mode_set mode_set; struct drm_display_mode *desired_mode; + struct list_head list; };
struct drm_fb_helper_surface_size { @@ -69,7 +70,7 @@ struct drm_fb_helper { struct drm_device *dev; struct drm_display_mode *mode; int crtc_count; - struct drm_fb_helper_crtc *crtc_info; + struct list_head crtc_list; int connector_count; struct drm_fb_helper_connector **connector_info; struct drm_fb_helper_funcs *funcs;
On Wed, Feb 1, 2012 at 10:38 AM, Sascha Hauer s.hauer@pengutronix.de wrote:
The fb helper uses fixed size arrays for the associated crtcs. This is an unnecessary limitation, so instead use a list to store the crtcs and allocate them dynamically.
I need more reasons on why this is a unnecessary limitation, for what?
Its a lot less cache friendly to use a linked list here for not much gain,
do you want to attach/detach crtcs from fb at runtime?
Dave.
On Fri, Feb 03, 2012 at 10:04:27AM +0000, Dave Airlie wrote:
On Wed, Feb 1, 2012 at 10:38 AM, Sascha Hauer s.hauer@pengutronix.de wrote:
The fb helper uses fixed size arrays for the associated crtcs. This is an unnecessary limitation, so instead use a list to store the crtcs and allocate them dynamically.
I need more reasons on why this is a unnecessary limitation, for what?
Its a lot less cache friendly to use a linked list here for not much gain,
How often do you change modes? This code is run when the user changes virtual terminals, which is not very performance critical.
do you want to attach/detach crtcs from fb at runtime?
I am working on a mid layer to connect simple framebuffer devices to kms and it would be convenient to just add a crtc to a drm device once it appears. This works fine for connectors and encoders, why not also for crtcs?
Sascha
On Sat, Feb 4, 2012 at 10:47 AM, Sascha Hauer s.hauer@pengutronix.de wrote:
On Fri, Feb 03, 2012 at 10:04:27AM +0000, Dave Airlie wrote:
On Wed, Feb 1, 2012 at 10:38 AM, Sascha Hauer s.hauer@pengutronix.de wrote:
The fb helper uses fixed size arrays for the associated crtcs. This is an unnecessary limitation, so instead use a list to store the crtcs and allocate them dynamically.
I need more reasons on why this is a unnecessary limitation, for what?
Its a lot less cache friendly to use a linked list here for not much gain,
How often do you change modes? This code is run when the user changes virtual terminals, which is not very performance critical.
But why make it worse unless you have a good reason?
I am working on a mid layer to connect simple framebuffer devices to kms and it would be convenient to just add a crtc to a drm device once it appears. This works fine for connectors and encoders, why not also for crtcs?
I'd like to see the midlayer before changing the core too much, I hate midlayers as they always cause more pain in the long run than they solve, the DRM is one of the worst examples of midlayer design and I'd rather not propogate it any further.
Hopefully you meant to say you are working on a set of helper functions that kms drivers for simple framebuffers can use, btw what is going to count as a simple framebuffer device? 1 crtc/connector? I've got to write a bunch of "simple" kms drivers but I'd really rather this developed the other way. Write a drivers for two simple devices, then carve the common code into helpers.
Dave.
On Sat, Feb 04, 2012 at 11:21:34AM +0000, Dave Airlie wrote:
On Sat, Feb 4, 2012 at 10:47 AM, Sascha Hauer s.hauer@pengutronix.de wrote:
On Fri, Feb 03, 2012 at 10:04:27AM +0000, Dave Airlie wrote:
On Wed, Feb 1, 2012 at 10:38 AM, Sascha Hauer s.hauer@pengutronix.de wrote:
The fb helper uses fixed size arrays for the associated crtcs. This is an unnecessary limitation, so instead use a list to store the crtcs and allocate them dynamically.
I need more reasons on why this is a unnecessary limitation, for what?
Its a lot less cache friendly to use a linked list here for not much gain,
How often do you change modes? This code is run when the user changes virtual terminals, which is not very performance critical.
But why make it worse unless you have a good reason?
I am working on a mid layer to connect simple framebuffer devices to kms and it would be convenient to just add a crtc to a drm device once it appears. This works fine for connectors and encoders, why not also for crtcs?
I'd like to see the midlayer before changing the core too much, I hate midlayers as they always cause more pain in the long run than they solve, the DRM is one of the worst examples of midlayer design and I'd rather not propogate it any further.
Hopefully you meant to say you are working on a set of helper functions that kms drivers for simple framebuffers can use
midlayer is a tainted word and I regret calling it like this right after sending my mail. Let's call it helper then ;)
, btw what is going to count as a simple framebuffer device? 1 crtc/connector?
It's not limited in the count of crtcs/connectors/encoders.
I've got to write a bunch of "simple" kms drivers but I'd really rather this developed the other way. Write a drivers for two simple devices, then carve the common code into helpers.
My goal is to write a driver for the i.MX IPU driver. I realized that I had to duplicate most of the exynos driver. Having written several framebuffer drivers (for i.MX1, netx, i.MX28) I know that all these devices are not very different, so my code is written with having these kind of devices in my mind.
Anyway, my code does not need this particular patch, so we can delay (or drop) it.
Sascha
As the crtcs are now allocated dynamically we don't need the crtc_count parameter anymore.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_fb_helper.c | 2 +- drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 8 ++------ drivers/gpu/drm/gma500/framebuffer.c | 3 +-- drivers/gpu/drm/i915/intel_fb.c | 4 +--- drivers/gpu/drm/nouveau/nouveau_fbcon.c | 3 +-- drivers/gpu/drm/radeon/radeon_fb.c | 4 +--- include/drm/drm_fb_helper.h | 3 +-- 7 files changed, 8 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index f292a78..231255b 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -385,7 +385,7 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
int drm_fb_helper_init(struct drm_device *dev, struct drm_fb_helper *fb_helper, - int crtc_count, int max_conn_count) + int max_conn_count) { struct drm_crtc *crtc; struct drm_fb_helper_crtc *helper_crtc; diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index d7ae29d..f9f8db2 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -294,7 +294,6 @@ int exynos_drm_fbdev_init(struct drm_device *dev) struct exynos_drm_fbdev *fbdev; struct exynos_drm_private *private = dev->dev_private; struct drm_fb_helper *helper; - unsigned int num_crtc; int ret;
DRM_DEBUG_KMS("%s\n", __FILE__); @@ -311,9 +310,7 @@ int exynos_drm_fbdev_init(struct drm_device *dev) private->fb_helper = helper = &fbdev->drm_fb_helper; helper->funcs = &exynos_drm_fb_helper_funcs;
- num_crtc = dev->mode_config.num_crtc; - - ret = drm_fb_helper_init(dev, helper, num_crtc, MAX_CONNECTOR); + ret = drm_fb_helper_init(dev, helper, MAX_CONNECTOR); if (ret < 0) { DRM_ERROR("failed to initialize drm fb helper.\n"); goto err_init; @@ -438,8 +435,7 @@ int exynos_drm_fbdev_reinit(struct drm_device *dev)
drm_fb_helper_fini(fb_helper);
- ret = drm_fb_helper_init(dev, fb_helper, - dev->mode_config.num_crtc, MAX_CONNECTOR); + ret = drm_fb_helper_init(dev, fb_helper, MAX_CONNECTOR); if (ret < 0) { DRM_ERROR("failed to initialize drm fb helper\n"); return ret; diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index 830dfdd6b..5eb185d 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -623,8 +623,7 @@ int psb_fbdev_init(struct drm_device *dev) dev_priv->fbdev = fbdev; fbdev->psb_fb_helper.funcs = &psb_fb_helper_funcs;
- drm_fb_helper_init(dev, &fbdev->psb_fb_helper, dev_priv->ops->crtcs, - INTELFB_CONN_LIMIT); + drm_fb_helper_init(dev, &fbdev->psb_fb_helper, INTELFB_CONN_LIMIT);
drm_fb_helper_single_add_all_connectors(&fbdev->psb_fb_helper); drm_fb_helper_initial_config(&fbdev->psb_fb_helper, 32); diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c index 571375a..a96c2ae 100644 --- a/drivers/gpu/drm/i915/intel_fb.c +++ b/drivers/gpu/drm/i915/intel_fb.c @@ -235,9 +235,7 @@ int intel_fbdev_init(struct drm_device *dev) dev_priv->fbdev = ifbdev; ifbdev->helper.funcs = &intel_fb_helper_funcs;
- ret = drm_fb_helper_init(dev, &ifbdev->helper, - dev_priv->num_pipe, - INTELFB_CONN_LIMIT); + ret = drm_fb_helper_init(dev, &ifbdev->helper, INTELFB_CONN_LIMIT); if (ret) { kfree(ifbdev); return ret; diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c index 9892218..a4cc944 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c @@ -500,8 +500,7 @@ int nouveau_fbcon_init(struct drm_device *dev) dev_priv->nfbdev = nfbdev; nfbdev->helper.funcs = &nouveau_fbcon_helper_funcs;
- ret = drm_fb_helper_init(dev, &nfbdev->helper, - nv_two_heads(dev) ? 2 : 1, 4); + ret = drm_fb_helper_init(dev, &nfbdev->helper, 4); if (ret) { kfree(nfbdev); return ret; diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c index cf2bf35..6d68944 100644 --- a/drivers/gpu/drm/radeon/radeon_fb.c +++ b/drivers/gpu/drm/radeon/radeon_fb.c @@ -377,9 +377,7 @@ int radeon_fbdev_init(struct radeon_device *rdev) rdev->mode_info.rfbdev = rfbdev; rfbdev->helper.funcs = &radeon_fb_helper_funcs;
- ret = drm_fb_helper_init(rdev->ddev, &rfbdev->helper, - rdev->num_crtc, - RADEONFB_CONN_LIMIT); + ret = drm_fb_helper_init(rdev->ddev, &rfbdev->helper, RADEONFB_CONN_LIMIT); if (ret) { kfree(rfbdev); return ret; diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index e1e1c02..e1afac5 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -87,8 +87,7 @@ int drm_fb_helper_single_fb_probe(struct drm_fb_helper *helper, int preferred_bpp);
int drm_fb_helper_init(struct drm_device *dev, - struct drm_fb_helper *helper, int crtc_count, - int max_conn); + struct drm_fb_helper *helper, int max_conn); void drm_fb_helper_fini(struct drm_fb_helper *helper); int drm_fb_helper_blank(int blank, struct fb_info *info); int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
drm_fb_helper_single_add_all_connectors is always called in conjunction with drm_fb_helper_initial_config, so call drm_fb_helper_single_add_all_connectors inside drm_fb_helper_initial_config and make drm_fb_helper_single_add_all_connectors static.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_fb_helper.c | 5 +++-- drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 13 ------------- drivers/gpu/drm/gma500/framebuffer.c | 1 - drivers/gpu/drm/i915/intel_fb.c | 1 - drivers/gpu/drm/nouveau/nouveau_fbcon.c | 2 -- drivers/gpu/drm/radeon/radeon_fb.c | 1 - include/drm/drm_fb_helper.h | 1 - 7 files changed, 3 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 231255b..b54298f 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -44,7 +44,7 @@ MODULE_LICENSE("GPL and additional rights"); static LIST_HEAD(kernel_fb_helper_list);
/* simple single crtc case helper function */ -int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper) +static int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper) { struct drm_device *dev = fb_helper->dev; struct drm_connector *connector; @@ -69,7 +69,6 @@ fail: fb_helper->connector_count = 0; return -ENOMEM; } -EXPORT_SYMBOL(drm_fb_helper_single_add_all_connectors);
static int drm_fb_helper_parse_command_line(struct drm_fb_helper *fb_helper) { @@ -1313,6 +1312,8 @@ bool drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) struct drm_device *dev = fb_helper->dev; int count = 0;
+ drm_fb_helper_single_add_all_connectors(fb_helper); + /* disable all the possible outputs/crtcs before entering KMS mode */ drm_helper_disable_unused_functions(fb_helper->dev);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index f9f8db2..706c906 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -316,13 +316,6 @@ int exynos_drm_fbdev_init(struct drm_device *dev) goto err_init; }
- ret = drm_fb_helper_single_add_all_connectors(helper); - if (ret < 0) { - DRM_ERROR("failed to register drm_fb_helper_connector.\n"); - goto err_setup; - - } - ret = drm_fb_helper_initial_config(helper, PREFERRED_BPP); if (ret < 0) { DRM_ERROR("failed to set up hw configuration.\n"); @@ -444,12 +437,6 @@ int exynos_drm_fbdev_reinit(struct drm_device *dev) if (!list_empty(&temp_list)) list_replace(&temp_list, &fb_helper->kernel_fb_list);
- ret = drm_fb_helper_single_add_all_connectors(fb_helper); - if (ret < 0) { - DRM_ERROR("failed to add fb helper to connectors\n"); - goto err; - } - ret = drm_fb_helper_initial_config(fb_helper, PREFERRED_BPP); if (ret < 0) { DRM_ERROR("failed to set up hw configuration.\n"); diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index 5eb185d..c7eadaf 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -625,7 +625,6 @@ int psb_fbdev_init(struct drm_device *dev)
drm_fb_helper_init(dev, &fbdev->psb_fb_helper, INTELFB_CONN_LIMIT);
- drm_fb_helper_single_add_all_connectors(&fbdev->psb_fb_helper); drm_fb_helper_initial_config(&fbdev->psb_fb_helper, 32); return 0; } diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c index a96c2ae..8f81286 100644 --- a/drivers/gpu/drm/i915/intel_fb.c +++ b/drivers/gpu/drm/i915/intel_fb.c @@ -241,7 +241,6 @@ int intel_fbdev_init(struct drm_device *dev) return ret; }
- drm_fb_helper_single_add_all_connectors(&ifbdev->helper); drm_fb_helper_initial_config(&ifbdev->helper, 32); return 0; } diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c index a4cc944..01061bb 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c @@ -506,8 +506,6 @@ int nouveau_fbcon_init(struct drm_device *dev) return ret; }
- drm_fb_helper_single_add_all_connectors(&nfbdev->helper); - if (dev_priv->vram_size <= 32 * 1024 * 1024) preferred_bpp = 8; else if (dev_priv->vram_size <= 64 * 1024 * 1024) diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c index 6d68944..ed5a642 100644 --- a/drivers/gpu/drm/radeon/radeon_fb.c +++ b/drivers/gpu/drm/radeon/radeon_fb.c @@ -383,7 +383,6 @@ int radeon_fbdev_init(struct radeon_device *rdev) return ret; }
- drm_fb_helper_single_add_all_connectors(&rfbdev->helper); drm_fb_helper_initial_config(&rfbdev->helper, bpp_sel); return 0; } diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index e1afac5..b989958 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -113,7 +113,6 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info);
int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper); bool drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel); -int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper); int drm_fb_helper_debug_enter(struct fb_info *info); int drm_fb_helper_debug_leave(struct fb_info *info);
list_for_each_entry_safe is for walking a list safe against removal of entries. Here, no entries are removed, so use list_for_each_entry.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_crtc_helper.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 84a4a80..d761d12 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -44,12 +44,12 @@ module_param_named(poll, drm_kms_helper_poll, bool, 0600); static void drm_mode_validate_flag(struct drm_connector *connector, int flags) { - struct drm_display_mode *mode, *t; + struct drm_display_mode *mode;
if (flags == (DRM_MODE_FLAG_DBLSCAN | DRM_MODE_FLAG_INTERLACE)) return;
- list_for_each_entry_safe(mode, t, &connector->modes, head) { + list_for_each_entry(mode, &connector->modes, head) { if ((mode->flags & DRM_MODE_FLAG_INTERLACE) && !(flags & DRM_MODE_FLAG_INTERLACE)) mode->status = MODE_NO_INTERLACE; @@ -87,7 +87,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, uint32_t maxX, uint32_t maxY) { struct drm_device *dev = connector->dev; - struct drm_display_mode *mode, *t; + struct drm_display_mode *mode; struct drm_connector_helper_funcs *connector_funcs = connector->helper_private; int count = 0; @@ -96,7 +96,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, drm_get_connector_name(connector)); /* set all modes to the unverified state */ - list_for_each_entry_safe(mode, t, &connector->modes, head) + list_for_each_entry(mode, &connector->modes, head) mode->status = MODE_UNVERIFIED;
if (connector->force) { @@ -136,7 +136,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, mode_flags |= DRM_MODE_FLAG_DBLSCAN; drm_mode_validate_flag(connector, mode_flags);
- list_for_each_entry_safe(mode, t, &connector->modes, head) { + list_for_each_entry(mode, &connector->modes, head) { if (mode->status == MODE_OK) mode->status = connector_funcs->mode_valid(connector, mode); @@ -152,7 +152,7 @@ prune:
DRM_DEBUG_KMS("[CONNECTOR:%d:%s] probed modes :\n", connector->base.id, drm_get_connector_name(connector)); - list_for_each_entry_safe(mode, t, &connector->modes, head) { + list_for_each_entry(mode, &connector->modes, head) { mode->vrefresh = drm_mode_vrefresh(mode);
drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
Several comments above functions say that the caller must hold the mode_config lock, but the functions take the lock themselves. Fix the comments.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_crtc.c | 21 +++++++++++++++------ 1 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index df6e413..322bc7b 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -454,7 +454,7 @@ EXPORT_SYMBOL(drm_mode_remove); * @name: user visible name of the connector * * LOCKING: - * Caller must hold @dev's mode_config lock. + * Takes mode config lock. * * Initialises a preallocated connector. Connectors should be * subclassed as part of driver connector objects. @@ -497,7 +497,7 @@ EXPORT_SYMBOL(drm_connector_init); * @connector: connector to cleanup * * LOCKING: - * Caller must hold @dev's mode_config lock. + * Takes mode config lock. * * Cleans up the connector but doesn't free the object. */ @@ -1314,7 +1314,7 @@ out: * @arg: arg from ioctl * * LOCKING: - * Caller? (FIXME) + * Takes mode config lock. * * Construct a CRTC configuration structure to return to the user. * @@ -1374,7 +1374,7 @@ out: * @arg: arg from ioctl * * LOCKING: - * Caller? (FIXME) + * Takes mode config lock. * * Construct a connector configuration structure to return to the user. * @@ -1556,6 +1556,9 @@ out: * @data: ioctl data * @file_priv: DRM file info * + * LOCKING: + * Takes mode config lock. + * * Return an plane count and set of IDs. */ int drm_mode_getplane_res(struct drm_device *dev, void *data, @@ -1602,6 +1605,9 @@ out: * @data: ioctl data * @file_priv: DRM file info * + * LOCKING: + * Takes mode config lock. + * * Return plane info, including formats supported, gamma size, any * current fb, etc. */ @@ -1667,6 +1673,9 @@ out: * @data: ioctl data* * @file_prive: DRM file info * + * LOCKING: + * Takes mode config lock. + * * Set plane info, including placement, fb, scaling, and other factors. * Or pass a NULL fb to disable. */ @@ -1797,7 +1806,7 @@ out: * @arg: arg from ioctl * * LOCKING: - * Caller? (FIXME) + * Takes mode config lock. * * Build a new CRTC configuration based on user request. * @@ -2278,7 +2287,7 @@ out: * @arg: arg from ioctl * * LOCKING: - * Caller? (FIXME) + * Takes mode config lock. * * Lookup the FB given its ID and return info about it. *
Creating an enum property is a common pattern, so create a convenience function for this and use it where appropriate.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_crtc.c | 100 +++++++++++++--------------- drivers/gpu/drm/i915/intel_modes.c | 28 +++----- drivers/gpu/drm/nouveau/nouveau_display.c | 10 ++-- drivers/gpu/drm/radeon/radeon_display.c | 43 +++---------- include/drm/drm_crtc.h | 14 +++- 5 files changed, 83 insertions(+), 112 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 322bc7b..3988c62 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -38,11 +38,6 @@ #include "drm_edid.h" #include "drm_fourcc.h"
-struct drm_prop_enum_list { - int type; - char *name; -}; - /* Avoid boilerplate. I'm tired of typing. */ #define DRM_ENUM_NAME_FN(fnname, list) \ char *fnname(int val) \ @@ -658,7 +653,6 @@ static int drm_mode_create_standard_connector_properties(struct drm_device *dev) { struct drm_property *edid; struct drm_property *dpms; - int i;
/* * Standard properties (apply to all connectors) @@ -668,11 +662,9 @@ static int drm_mode_create_standard_connector_properties(struct drm_device *dev) "EDID", 0); dev->mode_config.edid_property = edid;
- dpms = drm_property_create(dev, DRM_MODE_PROP_ENUM, - "DPMS", ARRAY_SIZE(drm_dpms_enum_list)); - for (i = 0; i < ARRAY_SIZE(drm_dpms_enum_list); i++) - drm_property_add_enum(dpms, i, drm_dpms_enum_list[i].type, - drm_dpms_enum_list[i].name); + dpms = drm_property_enum_create(dev, 0, + "DPMS", drm_dpms_enum_list, + ARRAY_SIZE(drm_dpms_enum_list)); dev->mode_config.dpms_property = dpms;
return 0; @@ -688,30 +680,21 @@ int drm_mode_create_dvi_i_properties(struct drm_device *dev) { struct drm_property *dvi_i_selector; struct drm_property *dvi_i_subconnector; - int i;
if (dev->mode_config.dvi_i_select_subconnector_property) return 0;
dvi_i_selector = - drm_property_create(dev, DRM_MODE_PROP_ENUM, + drm_property_enum_create(dev, 0, "select subconnector", + drm_dvi_i_select_enum_list, ARRAY_SIZE(drm_dvi_i_select_enum_list)); - for (i = 0; i < ARRAY_SIZE(drm_dvi_i_select_enum_list); i++) - drm_property_add_enum(dvi_i_selector, i, - drm_dvi_i_select_enum_list[i].type, - drm_dvi_i_select_enum_list[i].name); dev->mode_config.dvi_i_select_subconnector_property = dvi_i_selector;
- dvi_i_subconnector = - drm_property_create(dev, DRM_MODE_PROP_ENUM | - DRM_MODE_PROP_IMMUTABLE, + dvi_i_subconnector = drm_property_enum_create(dev, DRM_MODE_PROP_IMMUTABLE, "subconnector", + drm_dvi_i_subconnector_enum_list, ARRAY_SIZE(drm_dvi_i_subconnector_enum_list)); - for (i = 0; i < ARRAY_SIZE(drm_dvi_i_subconnector_enum_list); i++) - drm_property_add_enum(dvi_i_subconnector, i, - drm_dvi_i_subconnector_enum_list[i].type, - drm_dvi_i_subconnector_enum_list[i].name); dev->mode_config.dvi_i_subconnector_property = dvi_i_subconnector;
return 0; @@ -742,23 +725,17 @@ int drm_mode_create_tv_properties(struct drm_device *dev, int num_modes, /* * Basic connector properties */ - tv_selector = drm_property_create(dev, DRM_MODE_PROP_ENUM, + tv_selector = drm_property_enum_create(dev, 0, "select subconnector", + drm_tv_select_enum_list, ARRAY_SIZE(drm_tv_select_enum_list)); - for (i = 0; i < ARRAY_SIZE(drm_tv_select_enum_list); i++) - drm_property_add_enum(tv_selector, i, - drm_tv_select_enum_list[i].type, - drm_tv_select_enum_list[i].name); dev->mode_config.tv_select_subconnector_property = tv_selector;
tv_subconnector = - drm_property_create(dev, DRM_MODE_PROP_ENUM | - DRM_MODE_PROP_IMMUTABLE, "subconnector", + drm_property_enum_create(dev, DRM_MODE_PROP_IMMUTABLE, + "subconnector", + drm_tv_subconnector_enum_list, ARRAY_SIZE(drm_tv_subconnector_enum_list)); - for (i = 0; i < ARRAY_SIZE(drm_tv_subconnector_enum_list); i++) - drm_property_add_enum(tv_subconnector, i, - drm_tv_subconnector_enum_list[i].type, - drm_tv_subconnector_enum_list[i].name); dev->mode_config.tv_subconnector_property = tv_subconnector;
/* @@ -845,18 +822,14 @@ EXPORT_SYMBOL(drm_mode_create_tv_properties); int drm_mode_create_scaling_mode_property(struct drm_device *dev) { struct drm_property *scaling_mode; - int i;
if (dev->mode_config.scaling_mode_property) return 0;
scaling_mode = - drm_property_create(dev, DRM_MODE_PROP_ENUM, "scaling mode", + drm_property_enum_create(dev, 0, "scaling mode", + drm_scaling_mode_enum_list, ARRAY_SIZE(drm_scaling_mode_enum_list)); - for (i = 0; i < ARRAY_SIZE(drm_scaling_mode_enum_list); i++) - drm_property_add_enum(scaling_mode, i, - drm_scaling_mode_enum_list[i].type, - drm_scaling_mode_enum_list[i].name);
dev->mode_config.scaling_mode_property = scaling_mode;
@@ -874,18 +847,14 @@ EXPORT_SYMBOL(drm_mode_create_scaling_mode_property); int drm_mode_create_dithering_property(struct drm_device *dev) { struct drm_property *dithering_mode; - int i;
if (dev->mode_config.dithering_mode_property) return 0;
dithering_mode = - drm_property_create(dev, DRM_MODE_PROP_ENUM, "dithering", + drm_property_enum_create(dev, 0, "dithering", + drm_dithering_mode_enum_list, ARRAY_SIZE(drm_dithering_mode_enum_list)); - for (i = 0; i < ARRAY_SIZE(drm_dithering_mode_enum_list); i++) - drm_property_add_enum(dithering_mode, i, - drm_dithering_mode_enum_list[i].type, - drm_dithering_mode_enum_list[i].name); dev->mode_config.dithering_mode_property = dithering_mode;
return 0; @@ -902,20 +871,15 @@ EXPORT_SYMBOL(drm_mode_create_dithering_property); int drm_mode_create_dirty_info_property(struct drm_device *dev) { struct drm_property *dirty_info; - int i;
if (dev->mode_config.dirty_info_property) return 0;
dirty_info = - drm_property_create(dev, DRM_MODE_PROP_ENUM | - DRM_MODE_PROP_IMMUTABLE, + drm_property_enum_create(dev, DRM_MODE_PROP_IMMUTABLE, "dirty", + drm_dirty_info_enum_list, ARRAY_SIZE(drm_dirty_info_enum_list)); - for (i = 0; i < ARRAY_SIZE(drm_dirty_info_enum_list); i++) - drm_property_add_enum(dirty_info, i, - drm_dirty_info_enum_list[i].type, - drm_dirty_info_enum_list[i].name); dev->mode_config.dirty_info_property = dirty_info;
return 0; @@ -2629,6 +2593,34 @@ fail: } EXPORT_SYMBOL(drm_property_create);
+struct drm_property *drm_property_enum_create(struct drm_device *dev, int flags, + const char *name, + const struct drm_prop_enum_list *props, + int num_values) +{ + struct drm_property *property; + int i, ret; + + flags |= DRM_MODE_PROP_ENUM; + + property = drm_property_create(dev, flags, name, num_values); + if (!property) + return NULL; + + for (i = 0; i < num_values; i++) { + ret = drm_property_add_enum(property, i, + props[i].type, + props[i].name); + if (ret) { + drm_property_destroy(dev, property); + return NULL; + } + } + + return property; +} +EXPORT_SYMBOL(drm_property_enum_create); + int drm_property_add_enum(struct drm_property *property, int index, uint64_t value, const char *name) { diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c index be2c6fe..56c2fb1 100644 --- a/drivers/gpu/drm/i915/intel_modes.c +++ b/drivers/gpu/drm/i915/intel_modes.c @@ -83,10 +83,10 @@ int intel_ddc_get_modes(struct drm_connector *connector, return ret; }
-static const char *force_audio_names[] = { - "off", - "auto", - "on", +static const struct drm_prop_enum_list force_audio_names[] = { + { -1, "off" }, + { 0, "auto" }, + { 1, "on" }, };
void @@ -95,27 +95,24 @@ intel_attach_force_audio_property(struct drm_connector *connector) struct drm_device *dev = connector->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct drm_property *prop; - int i;
prop = dev_priv->force_audio_property; if (prop == NULL) { - prop = drm_property_create(dev, DRM_MODE_PROP_ENUM, + prop = drm_property_enum_create(dev, 0, "audio", + force_audio_names, ARRAY_SIZE(force_audio_names)); if (prop == NULL) return;
- for (i = 0; i < ARRAY_SIZE(force_audio_names); i++) - drm_property_add_enum(prop, i, i-1, force_audio_names[i]); - dev_priv->force_audio_property = prop; } drm_connector_attach_property(connector, prop, 0); }
-static const char *broadcast_rgb_names[] = { - "Full", - "Limited 16:235", +static const struct drm_prop_enum_list broadcast_rgb_names[] = { + { 0, "Full" }, + { 1, "Limited 16:235" }, };
void @@ -124,19 +121,16 @@ intel_attach_broadcast_rgb_property(struct drm_connector *connector) struct drm_device *dev = connector->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct drm_property *prop; - int i;
prop = dev_priv->broadcast_rgb_property; if (prop == NULL) { - prop = drm_property_create(dev, DRM_MODE_PROP_ENUM, + prop = drm_property_enum_create(dev, DRM_MODE_PROP_ENUM, "Broadcast RGB", + broadcast_rgb_names, ARRAY_SIZE(broadcast_rgb_names)); if (prop == NULL) return;
- for (i = 0; i < ARRAY_SIZE(broadcast_rgb_names); i++) - drm_property_add_enum(prop, i, i, broadcast_rgb_names[i]); - dev_priv->broadcast_rgb_property = prop; }
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c index 3cb52bc..45adade 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -155,20 +155,20 @@ static const struct drm_mode_config_funcs nouveau_mode_config_funcs = { };
-struct drm_prop_enum_list { +struct nouveau_drm_prop_enum_list { u8 gen_mask; int type; char *name; };
-static struct drm_prop_enum_list underscan[] = { +static struct nouveau_drm_prop_enum_list underscan[] = { { 6, UNDERSCAN_AUTO, "auto" }, { 6, UNDERSCAN_OFF, "off" }, { 6, UNDERSCAN_ON, "on" }, {} };
-static struct drm_prop_enum_list dither_mode[] = { +static struct nouveau_drm_prop_enum_list dither_mode[] = { { 7, DITHERING_MODE_AUTO, "auto" }, { 7, DITHERING_MODE_OFF, "off" }, { 1, DITHERING_MODE_ON, "on" }, @@ -178,7 +178,7 @@ static struct drm_prop_enum_list dither_mode[] = { {} };
-static struct drm_prop_enum_list dither_depth[] = { +static struct nouveau_drm_prop_enum_list dither_depth[] = { { 6, DITHERING_DEPTH_AUTO, "auto" }, { 6, DITHERING_DEPTH_6BPC, "6 bpc" }, { 6, DITHERING_DEPTH_8BPC, "8 bpc" }, @@ -186,7 +186,7 @@ static struct drm_prop_enum_list dither_depth[] = { };
#define PROP_ENUM(p,gen,n,list) do { \ - struct drm_prop_enum_list *l = (list); \ + struct nouveau_drm_prop_enum_list *l = (list); \ int c = 0; \ while (l->gen_mask) { \ if (l->gen_mask & (1 << (gen))) \ diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index 8c49fef..eba3529 100644 --- a/drivers/gpu/drm/radeon/radeon_display.c +++ b/drivers/gpu/drm/radeon/radeon_display.c @@ -1124,11 +1124,6 @@ static const struct drm_mode_config_funcs radeon_mode_funcs = { .output_poll_changed = radeon_output_poll_changed };
-struct drm_prop_enum_list { - int type; - char *name; -}; - static struct drm_prop_enum_list radeon_tmds_pll_enum_list[] = { { 0, "driver" }, { 1, "bios" }, @@ -1153,7 +1148,7 @@ static struct drm_prop_enum_list radeon_underscan_enum_list[] =
static int radeon_modeset_create_props(struct radeon_device *rdev) { - int i, sz; + int sz;
if (rdev->is_atom_bios) { rdev->mode_info.coherent_mode_property = @@ -1170,15 +1165,9 @@ static int radeon_modeset_create_props(struct radeon_device *rdev) if (!ASIC_IS_AVIVO(rdev)) { sz = ARRAY_SIZE(radeon_tmds_pll_enum_list); rdev->mode_info.tmds_pll_property = - drm_property_create(rdev->ddev, - DRM_MODE_PROP_ENUM, - "tmds_pll", sz); - for (i = 0; i < sz; i++) { - drm_property_add_enum(rdev->mode_info.tmds_pll_property, - i, - radeon_tmds_pll_enum_list[i].type, - radeon_tmds_pll_enum_list[i].name); - } + drm_property_enum_create(rdev->ddev, 0, + "tmds_pll", + radeon_tmds_pll_enum_list, sz); }
rdev->mode_info.load_detect_property = @@ -1194,27 +1183,15 @@ static int radeon_modeset_create_props(struct radeon_device *rdev)
sz = ARRAY_SIZE(radeon_tv_std_enum_list); rdev->mode_info.tv_std_property = - drm_property_create(rdev->ddev, - DRM_MODE_PROP_ENUM, - "tv standard", sz); - for (i = 0; i < sz; i++) { - drm_property_add_enum(rdev->mode_info.tv_std_property, - i, - radeon_tv_std_enum_list[i].type, - radeon_tv_std_enum_list[i].name); - } + drm_property_enum_create(rdev->ddev, 0, + "tv standard", + radeon_tv_std_enum_list, sz);
sz = ARRAY_SIZE(radeon_underscan_enum_list); rdev->mode_info.underscan_property = - drm_property_create(rdev->ddev, - DRM_MODE_PROP_ENUM, - "underscan", sz); - for (i = 0; i < sz; i++) { - drm_property_add_enum(rdev->mode_info.underscan_property, - i, - radeon_underscan_enum_list[i].type, - radeon_underscan_enum_list[i].name); - } + drm_property_enum_create(rdev->ddev, 0, + "underscan", + radeon_underscan_enum_list, sz);
rdev->mode_info.underscan_hborder_property = drm_property_create(rdev->ddev, diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 8d593ad..cdbbb40 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -394,7 +394,7 @@ struct drm_crtc { s64 framedur_ns, linedur_ns, pixeldur_ns;
/* if you are using the helper */ - void *helper_private; + struct drm_crtc_helper_funcs *helper_private; };
@@ -481,7 +481,7 @@ struct drm_encoder {
struct drm_crtc *crtc; const struct drm_encoder_funcs *funcs; - void *helper_private; + struct drm_encoder_helper_funcs *helper_private; };
enum drm_connector_force { @@ -573,7 +573,7 @@ struct drm_connector { /* requested DPMS state */ int dpms;
- void *helper_private; + struct drm_connector_helper_funcs *helper_private;
/* forced on connector */ enum drm_connector_force force; @@ -807,6 +807,10 @@ struct drm_mode_config { #define obj_to_blob(x) container_of(x, struct drm_property_blob, base) #define obj_to_plane(x) container_of(x, struct drm_plane, base)
+struct drm_prop_enum_list { + int type; + char *name; +};
extern void drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, @@ -904,6 +908,10 @@ extern int drm_connector_attach_property(struct drm_connector *connector, struct drm_property *property, uint64_t init_val); extern struct drm_property *drm_property_create(struct drm_device *dev, int flags, const char *name, int num_values); +extern struct drm_property *drm_property_enum_create(struct drm_device *dev, int flags, + const char *name, + const struct drm_prop_enum_list *props, + int num_values); extern void drm_property_destroy(struct drm_device *dev, struct drm_property *property); extern int drm_property_add_enum(struct drm_property *property, int index, uint64_t value, const char *name);
On Wed, 1 Feb 2012 11:38:32 +0100, Sascha Hauer s.hauer@pengutronix.de wrote:
Creating an enum property is a common pattern, so create a convenience function for this and use it where appropriate.
Similar naming comments apply as for drm_property_create_range. However, I did spot something anomalous...
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 8d593ad..cdbbb40 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -394,7 +394,7 @@ struct drm_crtc { s64 framedur_ns, linedur_ns, pixeldur_ns;
/* if you are using the helper */
- void *helper_private;
- struct drm_crtc_helper_funcs *helper_private;
};
@@ -481,7 +481,7 @@ struct drm_encoder {
struct drm_crtc *crtc; const struct drm_encoder_funcs *funcs;
- void *helper_private;
- struct drm_encoder_helper_funcs *helper_private;
};
enum drm_connector_force { @@ -573,7 +573,7 @@ struct drm_connector { /* requested DPMS state */ int dpms;
- void *helper_private;
struct drm_connector_helper_funcs *helper_private;
/* forced on connector */ enum drm_connector_force force;
This is a separate chunk. -Chris
On Wed, Feb 01, 2012 at 11:48:53AM +0000, Chris Wilson wrote:
On Wed, 1 Feb 2012 11:38:32 +0100, Sascha Hauer s.hauer@pengutronix.de wrote:
Creating an enum property is a common pattern, so create a convenience function for this and use it where appropriate.
Similar naming comments apply as for drm_property_create_range.
Indeed, drm_property_create_range sounds better. Will change.
However, I did spot something anomalous...
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 8d593ad..cdbbb40 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -394,7 +394,7 @@ struct drm_crtc { s64 framedur_ns, linedur_ns, pixeldur_ns;
/* if you are using the helper */
- void *helper_private;
- struct drm_crtc_helper_funcs *helper_private;
};
@@ -481,7 +481,7 @@ struct drm_encoder {
struct drm_crtc *crtc; const struct drm_encoder_funcs *funcs;
- void *helper_private;
- struct drm_encoder_helper_funcs *helper_private;
};
enum drm_connector_force { @@ -573,7 +573,7 @@ struct drm_connector { /* requested DPMS state */ int dpms;
- void *helper_private;
struct drm_connector_helper_funcs *helper_private;
/* forced on connector */ enum drm_connector_force force;
This is a separate chunk.
I know I created this chunk but couldn't find it in my commits. You found it ;). yes, should be a seperate patch.
Sascha
----- Original Message -----
From: "Chris Wilson" chris@chris-wilson.co.uk To: "Sascha Hauer" s.hauer@pengutronix.de, dri-devel@lists.freedesktop.org Cc: kernel@pengutronix.de Sent: Wednesday, 1 February, 2012 11:48:53 AM Subject: Re: [PATCH 14/20] drm: add convenience function to create an enum property
On Wed, 1 Feb 2012 11:38:32 +0100, Sascha Hauer s.hauer@pengutronix.de wrote:
Creating an enum property is a common pattern, so create a convenience function for this and use it where appropriate.
Similar naming comments apply as for drm_property_create_range. However, I did spot something anomalous...
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 8d593ad..cdbbb40 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -394,7 +394,7 @@ struct drm_crtc { s64 framedur_ns, linedur_ns, pixeldur_ns;
/* if you are using the helper */
- void *helper_private;
- struct drm_crtc_helper_funcs *helper_private;
};
@@ -481,7 +481,7 @@ struct drm_encoder {
struct drm_crtc *crtc; const struct drm_encoder_funcs *funcs;
- void *helper_private;
- struct drm_encoder_helper_funcs *helper_private;
};
enum drm_connector_force { @@ -573,7 +573,7 @@ struct drm_connector { /* requested DPMS state */ int dpms;
- void *helper_private;
struct drm_connector_helper_funcs *helper_private;
/* forced on connector */ enum drm_connector_force force;
This is a separate chunk.
And totally wrong, using the helper should remain optional, and the helper includes should not be included into the main headers.
Dave.
On Wed, Feb 01, 2012 at 07:55:40AM -0500, David Airlie wrote:
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 8d593ad..cdbbb40 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -394,7 +394,7 @@ struct drm_crtc { s64 framedur_ns, linedur_ns, pixeldur_ns;
/* if you are using the helper */
- void *helper_private;
- struct drm_crtc_helper_funcs *helper_private;
};
@@ -481,7 +481,7 @@ struct drm_encoder {
struct drm_crtc *crtc; const struct drm_encoder_funcs *funcs;
- void *helper_private;
- struct drm_encoder_helper_funcs *helper_private;
};
enum drm_connector_force { @@ -573,7 +573,7 @@ struct drm_connector { /* requested DPMS state */ int dpms;
- void *helper_private;
struct drm_connector_helper_funcs *helper_private;
/* forced on connector */ enum drm_connector_force force;
This is a separate chunk.
And totally wrong, using the helper should remain optional, and the helper includes should not be included into the main headers.
The intention was to prevent people from thinking that they should invent their own set of helper functions. As these are only pointers we could also add a
struct drm_crtc_helper_funcs; struct drm_connector_helper_funcs; struct drm_encoder_helper_funcs;
to drm_crtc.h without having to include the helper includes.
Sascha
On Wed, Feb 01, 2012 at 02:05:38PM +0100, Sascha Hauer wrote:
On Wed, Feb 01, 2012 at 07:55:40AM -0500, David Airlie wrote:
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 8d593ad..cdbbb40 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -394,7 +394,7 @@ struct drm_crtc { s64 framedur_ns, linedur_ns, pixeldur_ns;
/* if you are using the helper */
- void *helper_private;
- struct drm_crtc_helper_funcs *helper_private;
};
@@ -481,7 +481,7 @@ struct drm_encoder {
struct drm_crtc *crtc; const struct drm_encoder_funcs *funcs;
- void *helper_private;
- struct drm_encoder_helper_funcs *helper_private;
};
enum drm_connector_force { @@ -573,7 +573,7 @@ struct drm_connector { /* requested DPMS state */ int dpms;
- void *helper_private;
struct drm_connector_helper_funcs *helper_private;
/* forced on connector */ enum drm_connector_force force;
This is a separate chunk.
And totally wrong, using the helper should remain optional, and the helper includes should not be included into the main headers.
The intention was to prevent people from thinking that they should invent their own set of helper functions. As these are only pointers we could also add a
struct drm_crtc_helper_funcs; struct drm_connector_helper_funcs; struct drm_encoder_helper_funcs;
to drm_crtc.h without having to include the helper includes.
I like this, it makes it a notch more obvious what these are for. And we have precedence for slightly violating abstraction in this way: E.g. the vfs isn't splattered with void pointers just because you're free to ignore a lot of stuff (like the pagecache) when implementing an fs ...
Cheers, Daniel
On Wed, Feb 1, 2012 at 1:05 PM, Sascha Hauer s.hauer@pengutronix.de wrote:
On Wed, Feb 01, 2012 at 07:55:40AM -0500, David Airlie wrote:
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 8d593ad..cdbbb40 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -394,7 +394,7 @@ struct drm_crtc { Â s64 framedur_ns, linedur_ns, pixeldur_ns;
/* if you are using the helper */
- void *helper_private;
- struct drm_crtc_helper_funcs *helper_private;
};
@@ -481,7 +481,7 @@ struct drm_encoder {
struct drm_crtc *crtc; Â const struct drm_encoder_funcs *funcs;
- void *helper_private;
- struct drm_encoder_helper_funcs *helper_private;
};
enum drm_connector_force { @@ -573,7 +573,7 @@ struct drm_connector { Â /* requested DPMS state */ Â int dpms;
- void *helper_private;
- struct drm_connector_helper_funcs *helper_private;
/* forced on connector */ Â enum drm_connector_force force;
This is a separate chunk.
And totally wrong, using the helper should remain optional, and the helper includes should not be included into the main headers.
The intention was to prevent people from thinking that they should invent their own set of helper functions. As these are only pointers we could also add a
struct drm_crtc_helper_funcs; struct drm_connector_helper_funcs; struct drm_encoder_helper_funcs;
to drm_crtc.h without having to include the helper includes.
This might be better, though driver can invent their own helpers if they need it, its the whole point of using helpers and not forcing drivers to do stuff one single way.
Dave.
On Fri, Feb 03, 2012 at 10:08:12AM +0000, Dave Airlie wrote:
On Wed, Feb 1, 2012 at 1:05 PM, Sascha Hauer s.hauer@pengutronix.de wrote:
On Wed, Feb 01, 2012 at 07:55:40AM -0500, David Airlie wrote:
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 8d593ad..cdbbb40 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -394,7 +394,7 @@ struct drm_crtc { Â s64 framedur_ns, linedur_ns, pixeldur_ns;
/* if you are using the helper */
- void *helper_private;
- struct drm_crtc_helper_funcs *helper_private;
};
@@ -481,7 +481,7 @@ struct drm_encoder {
struct drm_crtc *crtc; Â const struct drm_encoder_funcs *funcs;
- void *helper_private;
- struct drm_encoder_helper_funcs *helper_private;
};
enum drm_connector_force { @@ -573,7 +573,7 @@ struct drm_connector { Â /* requested DPMS state */ Â int dpms;
- void *helper_private;
- struct drm_connector_helper_funcs *helper_private;
/* forced on connector */ Â enum drm_connector_force force;
This is a separate chunk.
And totally wrong, using the helper should remain optional, and the helper includes should not be included into the main headers.
The intention was to prevent people from thinking that they should invent their own set of helper functions. As these are only pointers we could also add a
struct drm_crtc_helper_funcs; struct drm_connector_helper_funcs; struct drm_encoder_helper_funcs;
to drm_crtc.h without having to include the helper includes.
This might be better, though driver can invent their own helpers if they need it, its the whole point of using helpers and not forcing drivers to do stuff one single way.
I hope to get the drivers more uniform. When people want to invent their own helpers I think the question we have to ask is what is wrong with the helpers and what is missing and try to fix/implement this before adding a new set of helpers. If someone really has a good reason to implement new helpers we can easily revert this change, but we shouldn't motivate him to do so.
Sascha
Creating a range property is a common pattern, so create a convenience function for this and use it where appropriate.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_crtc.c | 69 ++++++++++++----------------- drivers/gpu/drm/gma500/framebuffer.c | 5 +-- drivers/gpu/drm/gma500/psb_intel_sdvo.c | 28 +++--------- drivers/gpu/drm/i2c/ch7006_drv.c | 5 +-- drivers/gpu/drm/i915/intel_sdvo.c | 30 +++--------- drivers/gpu/drm/nouveau/nouveau_display.c | 10 +--- drivers/gpu/drm/radeon/radeon_display.c | 27 +++--------- include/drm/drm_crtc.h | 3 + 8 files changed, 56 insertions(+), 121 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 3988c62..1ebcedf 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -742,28 +742,16 @@ int drm_mode_create_tv_properties(struct drm_device *dev, int num_modes, * Other, TV specific properties: margins & TV modes. */ dev->mode_config.tv_left_margin_property = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "left margin", 2); - dev->mode_config.tv_left_margin_property->values[0] = 0; - dev->mode_config.tv_left_margin_property->values[1] = 100; + drm_property_range_create(dev, 0, "left margin", 0, 100);
dev->mode_config.tv_right_margin_property = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "right margin", 2); - dev->mode_config.tv_right_margin_property->values[0] = 0; - dev->mode_config.tv_right_margin_property->values[1] = 100; + drm_property_range_create(dev, 0, "right margin", 0, 100);
dev->mode_config.tv_top_margin_property = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "top margin", 2); - dev->mode_config.tv_top_margin_property->values[0] = 0; - dev->mode_config.tv_top_margin_property->values[1] = 100; + drm_property_range_create(dev, 0, "top margin", 0, 100);
dev->mode_config.tv_bottom_margin_property = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "bottom margin", 2); - dev->mode_config.tv_bottom_margin_property->values[0] = 0; - dev->mode_config.tv_bottom_margin_property->values[1] = 100; + drm_property_range_create(dev, 0, "bottom margin", 0, 100);
dev->mode_config.tv_mode_property = drm_property_create(dev, DRM_MODE_PROP_ENUM, @@ -773,40 +761,22 @@ int drm_mode_create_tv_properties(struct drm_device *dev, int num_modes, i, modes[i]);
dev->mode_config.tv_brightness_property = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "brightness", 2); - dev->mode_config.tv_brightness_property->values[0] = 0; - dev->mode_config.tv_brightness_property->values[1] = 100; + drm_property_range_create(dev, 0, "brightness", 0, 100);
dev->mode_config.tv_contrast_property = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "contrast", 2); - dev->mode_config.tv_contrast_property->values[0] = 0; - dev->mode_config.tv_contrast_property->values[1] = 100; + drm_property_range_create(dev, 0, "contrast", 0, 100);
dev->mode_config.tv_flicker_reduction_property = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "flicker reduction", 2); - dev->mode_config.tv_flicker_reduction_property->values[0] = 0; - dev->mode_config.tv_flicker_reduction_property->values[1] = 100; + drm_property_range_create(dev, 0, "flicker reduction", 0, 100);
dev->mode_config.tv_overscan_property = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "overscan", 2); - dev->mode_config.tv_overscan_property->values[0] = 0; - dev->mode_config.tv_overscan_property->values[1] = 100; + drm_property_range_create(dev, 0, "overscan", 0, 100);
dev->mode_config.tv_saturation_property = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "saturation", 2); - dev->mode_config.tv_saturation_property->values[0] = 0; - dev->mode_config.tv_saturation_property->values[1] = 100; + drm_property_range_create(dev, 0, "saturation", 0, 100);
dev->mode_config.tv_hue_property = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "hue", 2); - dev->mode_config.tv_hue_property->values[0] = 0; - dev->mode_config.tv_hue_property->values[1] = 100; + drm_property_range_create(dev, 0, "hue", 0, 100);
return 0; } @@ -2621,6 +2591,25 @@ struct drm_property *drm_property_enum_create(struct drm_device *dev, int flags, } EXPORT_SYMBOL(drm_property_enum_create);
+struct drm_property *drm_property_range_create(struct drm_device *dev, int flags, + const char *name, + uint64_t min, uint64_t max) +{ + struct drm_property *property; + + flags |= DRM_MODE_PROP_RANGE; + + property = drm_property_create(dev, flags, name, 2); + if (!property) + return NULL; + + property->values[0] = min; + property->values[1] = max; + + return property; +} +EXPORT_SYMBOL(drm_property_range_create); + int drm_property_add_enum(struct drm_property *property, int index, uint64_t value, const char *name) { diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index c7eadaf..f359620 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -723,10 +723,7 @@ static int psb_create_backlight_property(struct drm_device *dev) if (dev_priv->backlight_property) return 0;
- backlight = drm_property_create(dev, DRM_MODE_PROP_RANGE, - "backlight", 2); - backlight->values[0] = 0; - backlight->values[1] = 100; + backlight = drm_property_range_create(dev, 0, "backlight", 0, 100);
dev_priv->backlight_property = backlight;
diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c index 88b4297..63a11dc 100644 --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c @@ -2312,10 +2312,8 @@ static bool psb_intel_sdvo_tv_create_property(struct psb_intel_sdvo *psb_intel_s psb_intel_sdvo_connector->max_##name = data_value[0]; \ psb_intel_sdvo_connector->cur_##name = response; \ psb_intel_sdvo_connector->name = \ - drm_property_create(dev, DRM_MODE_PROP_RANGE, #name, 2); \ + drm_property_range_create(dev, 0, #name, 0, data_value[0]); \ if (!psb_intel_sdvo_connector->name) return false; \ - psb_intel_sdvo_connector->name->values[0] = 0; \ - psb_intel_sdvo_connector->name->values[1] = data_value[0]; \ drm_connector_attach_property(connector, \ psb_intel_sdvo_connector->name, \ psb_intel_sdvo_connector->cur_##name); \ @@ -2349,25 +2347,19 @@ psb_intel_sdvo_create_enhance_property_tv(struct psb_intel_sdvo *psb_intel_sdvo, psb_intel_sdvo_connector->left_margin = data_value[0] - response; psb_intel_sdvo_connector->right_margin = psb_intel_sdvo_connector->left_margin; psb_intel_sdvo_connector->left = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "left_margin", 2); + drm_property_range_create(dev, 0, "left_margin", 0, data_value[0]); if (!psb_intel_sdvo_connector->left) return false;
- psb_intel_sdvo_connector->left->values[0] = 0; - psb_intel_sdvo_connector->left->values[1] = data_value[0]; drm_connector_attach_property(connector, psb_intel_sdvo_connector->left, psb_intel_sdvo_connector->left_margin);
psb_intel_sdvo_connector->right = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "right_margin", 2); + drm_property_range_create(dev, 0, "right_margin", 0, data_value[0]); if (!psb_intel_sdvo_connector->right) return false;
- psb_intel_sdvo_connector->right->values[0] = 0; - psb_intel_sdvo_connector->right->values[1] = data_value[0]; drm_connector_attach_property(connector, psb_intel_sdvo_connector->right, psb_intel_sdvo_connector->right_margin); @@ -2391,25 +2383,19 @@ psb_intel_sdvo_create_enhance_property_tv(struct psb_intel_sdvo *psb_intel_sdvo, psb_intel_sdvo_connector->top_margin = data_value[0] - response; psb_intel_sdvo_connector->bottom_margin = psb_intel_sdvo_connector->top_margin; psb_intel_sdvo_connector->top = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "top_margin", 2); + drm_property_range_create(dev, 0, "top_margin", 0, data_value[0]); if (!psb_intel_sdvo_connector->top) return false;
- psb_intel_sdvo_connector->top->values[0] = 0; - psb_intel_sdvo_connector->top->values[1] = data_value[0]; drm_connector_attach_property(connector, psb_intel_sdvo_connector->top, psb_intel_sdvo_connector->top_margin);
psb_intel_sdvo_connector->bottom = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "bottom_margin", 2); + drm_property_range_create(dev, 0, "bottom_margin", 0, data_value[0]); if (!psb_intel_sdvo_connector->bottom) return false;
- psb_intel_sdvo_connector->bottom->values[0] = 0; - psb_intel_sdvo_connector->bottom->values[1] = data_value[0]; drm_connector_attach_property(connector, psb_intel_sdvo_connector->bottom, psb_intel_sdvo_connector->bottom_margin); @@ -2438,12 +2424,10 @@ psb_intel_sdvo_create_enhance_property_tv(struct psb_intel_sdvo *psb_intel_sdvo, psb_intel_sdvo_connector->max_dot_crawl = 1; psb_intel_sdvo_connector->cur_dot_crawl = response & 0x1; psb_intel_sdvo_connector->dot_crawl = - drm_property_create(dev, DRM_MODE_PROP_RANGE, "dot_crawl", 2); + drm_property_range_create(dev, 0, "dot_crawl", 0, 1); if (!psb_intel_sdvo_connector->dot_crawl) return false;
- psb_intel_sdvo_connector->dot_crawl->values[0] = 0; - psb_intel_sdvo_connector->dot_crawl->values[1] = 1; drm_connector_attach_property(connector, psb_intel_sdvo_connector->dot_crawl, psb_intel_sdvo_connector->cur_dot_crawl); diff --git a/drivers/gpu/drm/i2c/ch7006_drv.c b/drivers/gpu/drm/i2c/ch7006_drv.c index 07d55df..aa9be26 100644 --- a/drivers/gpu/drm/i2c/ch7006_drv.c +++ b/drivers/gpu/drm/i2c/ch7006_drv.c @@ -252,10 +252,7 @@ static int ch7006_encoder_create_resources(struct drm_encoder *encoder,
drm_mode_create_tv_properties(dev, NUM_TV_NORMS, ch7006_tv_norm_names);
- priv->scale_property = drm_property_create(dev, DRM_MODE_PROP_RANGE, - "scale", 2); - priv->scale_property->values[0] = 0; - priv->scale_property->values[1] = 2; + priv->scale_property = drm_property_range_create(dev, 0, "scale", 0, 2);
drm_connector_attach_property(connector, conf->tv_select_subconnector_property, priv->select_subconnector); diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index e334ec3..24d9e62 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -2277,10 +2277,8 @@ static bool intel_sdvo_tv_create_property(struct intel_sdvo *intel_sdvo, intel_sdvo_connector->max_##name = data_value[0]; \ intel_sdvo_connector->cur_##name = response; \ intel_sdvo_connector->name = \ - drm_property_create(dev, DRM_MODE_PROP_RANGE, #name, 2); \ + drm_property_range_create(dev, 0, #name, 0, data_value[0]); \ if (!intel_sdvo_connector->name) return false; \ - intel_sdvo_connector->name->values[0] = 0; \ - intel_sdvo_connector->name->values[1] = data_value[0]; \ drm_connector_attach_property(connector, \ intel_sdvo_connector->name, \ intel_sdvo_connector->cur_##name); \ @@ -2314,25 +2312,19 @@ intel_sdvo_create_enhance_property_tv(struct intel_sdvo *intel_sdvo, intel_sdvo_connector->left_margin = data_value[0] - response; intel_sdvo_connector->right_margin = intel_sdvo_connector->left_margin; intel_sdvo_connector->left = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "left_margin", 2); + drm_property_range_create(dev, 0, "left_margin", 0, data_value[0]); if (!intel_sdvo_connector->left) return false;
- intel_sdvo_connector->left->values[0] = 0; - intel_sdvo_connector->left->values[1] = data_value[0]; drm_connector_attach_property(connector, intel_sdvo_connector->left, intel_sdvo_connector->left_margin);
intel_sdvo_connector->right = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "right_margin", 2); + drm_property_range_create(dev, 0, "right_margin", 0, data_value[0]); if (!intel_sdvo_connector->right) return false;
- intel_sdvo_connector->right->values[0] = 0; - intel_sdvo_connector->right->values[1] = data_value[0]; drm_connector_attach_property(connector, intel_sdvo_connector->right, intel_sdvo_connector->right_margin); @@ -2356,25 +2348,21 @@ intel_sdvo_create_enhance_property_tv(struct intel_sdvo *intel_sdvo, intel_sdvo_connector->top_margin = data_value[0] - response; intel_sdvo_connector->bottom_margin = intel_sdvo_connector->top_margin; intel_sdvo_connector->top = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "top_margin", 2); + drm_property_range_create(dev, 0, + "top_margin", 0, data_value[0]); if (!intel_sdvo_connector->top) return false;
- intel_sdvo_connector->top->values[0] = 0; - intel_sdvo_connector->top->values[1] = data_value[0]; drm_connector_attach_property(connector, intel_sdvo_connector->top, intel_sdvo_connector->top_margin);
intel_sdvo_connector->bottom = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "bottom_margin", 2); + drm_property_range_create(dev, 0, + "bottom_margin", 0, data_value[0]); if (!intel_sdvo_connector->bottom) return false;
- intel_sdvo_connector->bottom->values[0] = 0; - intel_sdvo_connector->bottom->values[1] = data_value[0]; drm_connector_attach_property(connector, intel_sdvo_connector->bottom, intel_sdvo_connector->bottom_margin); @@ -2403,12 +2391,10 @@ intel_sdvo_create_enhance_property_tv(struct intel_sdvo *intel_sdvo, intel_sdvo_connector->max_dot_crawl = 1; intel_sdvo_connector->cur_dot_crawl = response & 0x1; intel_sdvo_connector->dot_crawl = - drm_property_create(dev, DRM_MODE_PROP_RANGE, "dot_crawl", 2); + drm_property_range_create(dev, 0, "dot_crawl", 0, 1); if (!intel_sdvo_connector->dot_crawl) return false;
- intel_sdvo_connector->dot_crawl->values[0] = 0; - intel_sdvo_connector->dot_crawl->values[1] = 1; drm_connector_attach_property(connector, intel_sdvo_connector->dot_crawl, intel_sdvo_connector->cur_dot_crawl); diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c index 45adade..a16d171 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -271,16 +271,10 @@ nouveau_display_create(struct drm_device *dev) PROP_ENUM(disp->underscan_property, gen, "underscan", underscan);
disp->underscan_hborder_property = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "underscan hborder", 2); - disp->underscan_hborder_property->values[0] = 0; - disp->underscan_hborder_property->values[1] = 128; + drm_property_range_create(dev, 0, "underscan hborder", 0, 128);
disp->underscan_vborder_property = - drm_property_create(dev, DRM_MODE_PROP_RANGE, - "underscan vborder", 2); - disp->underscan_vborder_property->values[0] = 0; - disp->underscan_vborder_property->values[1] = 128; + drm_property_range_create(dev, 0, "underscan vborder", 0, 128);
dev->mode_config.funcs = (void *)&nouveau_mode_config_funcs; dev->mode_config.fb_base = pci_resource_start(dev->pdev, 1); diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index eba3529..fd59af9 100644 --- a/drivers/gpu/drm/radeon/radeon_display.c +++ b/drivers/gpu/drm/radeon/radeon_display.c @@ -1152,14 +1152,9 @@ static int radeon_modeset_create_props(struct radeon_device *rdev)
if (rdev->is_atom_bios) { rdev->mode_info.coherent_mode_property = - drm_property_create(rdev->ddev, - DRM_MODE_PROP_RANGE, - "coherent", 2); + drm_property_range_create(rdev->ddev, 0 , "coherent", 0, 1); if (!rdev->mode_info.coherent_mode_property) return -ENOMEM; - - rdev->mode_info.coherent_mode_property->values[0] = 0; - rdev->mode_info.coherent_mode_property->values[1] = 1; }
if (!ASIC_IS_AVIVO(rdev)) { @@ -1171,13 +1166,9 @@ static int radeon_modeset_create_props(struct radeon_device *rdev) }
rdev->mode_info.load_detect_property = - drm_property_create(rdev->ddev, - DRM_MODE_PROP_RANGE, - "load detection", 2); + drm_property_range_create(rdev->ddev, 0, "load detection", 0, 1); if (!rdev->mode_info.load_detect_property) return -ENOMEM; - rdev->mode_info.load_detect_property->values[0] = 0; - rdev->mode_info.load_detect_property->values[1] = 1;
drm_mode_create_scaling_mode_property(rdev->ddev);
@@ -1194,22 +1185,16 @@ static int radeon_modeset_create_props(struct radeon_device *rdev) radeon_underscan_enum_list, sz);
rdev->mode_info.underscan_hborder_property = - drm_property_create(rdev->ddev, - DRM_MODE_PROP_RANGE, - "underscan hborder", 2); + drm_property_range_create(rdev->ddev, 0, + "underscan hborder", 0, 128); if (!rdev->mode_info.underscan_hborder_property) return -ENOMEM; - rdev->mode_info.underscan_hborder_property->values[0] = 0; - rdev->mode_info.underscan_hborder_property->values[1] = 128;
rdev->mode_info.underscan_vborder_property = - drm_property_create(rdev->ddev, - DRM_MODE_PROP_RANGE, - "underscan vborder", 2); + drm_property_range_create(rdev->ddev, 0, + "underscan vborder", 0, 128); if (!rdev->mode_info.underscan_vborder_property) return -ENOMEM; - rdev->mode_info.underscan_vborder_property->values[0] = 0; - rdev->mode_info.underscan_vborder_property->values[1] = 128;
return 0; } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index cdbbb40..c03ad9a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -912,6 +912,9 @@ extern struct drm_property *drm_property_enum_create(struct drm_device *dev, int const char *name, const struct drm_prop_enum_list *props, int num_values); +struct drm_property *drm_property_range_create(struct drm_device *dev, int flags, + const char *name, + uint64_t min, uint64_t max); extern void drm_property_destroy(struct drm_device *dev, struct drm_property *property); extern int drm_property_add_enum(struct drm_property *property, int index, uint64_t value, const char *name);
On Wed, 1 Feb 2012 11:38:33 +0100, Sascha Hauer s.hauer@pengutronix.de wrote:
Creating a range property is a common pattern, so create a convenience function for this and use it where appropriate.
This is pure bikeshedding, but I would prefer drm_property_create_range as the object being created is the drm_property and this is just a variation upon the creation theme. -Chris
Storing the properties associated to a connector in a list allows us to drop the current limitation on a maximum number of properties per connector.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_crtc.c | 109 ++++++++++++++++++++----------------------- include/drm/drm_crtc.h | 5 +- 2 files changed, 53 insertions(+), 61 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 1ebcedf..b815e69 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -140,6 +140,12 @@ struct drm_conn_prop_enum_list { int count; };
+struct drm_connector_property { + struct drm_property *base; + uint64_t val; + struct list_head list; +}; + /* * Connector and encoder types. */ @@ -470,6 +476,7 @@ void drm_connector_init(struct drm_device *dev, INIT_LIST_HEAD(&connector->user_modes); INIT_LIST_HEAD(&connector->probed_modes); INIT_LIST_HEAD(&connector->modes); + INIT_LIST_HEAD(&connector->properties); connector->edid_blob_ptr = NULL;
list_add_tail(&connector->head, &dev->mode_config.connector_list); @@ -954,6 +961,7 @@ void drm_mode_config_cleanup(struct drm_device *dev) struct drm_framebuffer *fb, *fbt; struct drm_property *property, *pt; struct drm_plane *plane, *plt; + struct drm_connector_property *cprop, *cpt;
list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list, head) { @@ -962,6 +970,10 @@ void drm_mode_config_cleanup(struct drm_device *dev)
list_for_each_entry_safe(connector, ot, &dev->mode_config.connector_list, head) { + list_for_each_entry_safe(cprop, cpt, &connector->properties, list) { + list_del(&cprop->list); + kfree(cprop); + } connector->funcs->destroy(connector); }
@@ -1324,6 +1336,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, struct drm_mode_object *obj; struct drm_connector *connector; struct drm_display_mode *mode; + struct drm_connector_property *cprop; int mode_count = 0; int props_count = 0; int encoders_count = 0; @@ -1353,11 +1366,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, } connector = obj_to_connector(obj);
- for (i = 0; i < DRM_CONNECTOR_MAX_PROPERTY; i++) { - if (connector->property_ids[i] != 0) { - props_count++; - } - } + list_for_each_entry(cprop, &connector->properties, list) + props_count++;
for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) { if (connector->encoder_ids[i] != 0) { @@ -1410,21 +1420,17 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, copied = 0; prop_ptr = (uint32_t __user *)(unsigned long)(out_resp->props_ptr); prop_values = (uint64_t __user *)(unsigned long)(out_resp->prop_values_ptr); - for (i = 0; i < DRM_CONNECTOR_MAX_PROPERTY; i++) { - if (connector->property_ids[i] != 0) { - if (put_user(connector->property_ids[i], - prop_ptr + copied)) { - ret = -EFAULT; - goto out; - } + list_for_each_entry(cprop, &connector->properties, list) { + if (put_user(cprop->base->base.id, prop_ptr + copied)) { + ret = -EFAULT; + goto out; + }
- if (put_user(connector->property_values[i], - prop_values + copied)) { - ret = -EFAULT; - goto out; - } - copied++; + if (put_user(cprop->val, prop_values + copied)) { + ret = -EFAULT; + goto out; } + copied++; } } out_resp->count_props = props_count; @@ -2662,18 +2668,16 @@ EXPORT_SYMBOL(drm_property_destroy); int drm_connector_attach_property(struct drm_connector *connector, struct drm_property *property, uint64_t init_val) { - int i; + struct drm_connector_property *cprop;
- for (i = 0; i < DRM_CONNECTOR_MAX_PROPERTY; i++) { - if (connector->property_ids[i] == 0) { - connector->property_ids[i] = property->base.id; - connector->property_values[i] = init_val; - break; - } - } + cprop = kzalloc(sizeof(*cprop), GFP_KERNEL); + if (!cprop) + return -ENOMEM; + + cprop->val = init_val; + cprop->base = property; + list_add_tail(&cprop->list, &connector->properties);
- if (i == DRM_CONNECTOR_MAX_PROPERTY) - return -EINVAL; return 0; } EXPORT_SYMBOL(drm_connector_attach_property); @@ -2681,36 +2685,32 @@ EXPORT_SYMBOL(drm_connector_attach_property); int drm_connector_property_set_value(struct drm_connector *connector, struct drm_property *property, uint64_t value) { - int i; + struct drm_connector_property *cprop;
- for (i = 0; i < DRM_CONNECTOR_MAX_PROPERTY; i++) { - if (connector->property_ids[i] == property->base.id) { - connector->property_values[i] = value; - break; + list_for_each_entry(cprop, &connector->properties, list) { + if (cprop->base->base.id == property->base.id) { + cprop->val = value; + return 0; } }
- if (i == DRM_CONNECTOR_MAX_PROPERTY) - return -EINVAL; - return 0; + return -EINVAL; } EXPORT_SYMBOL(drm_connector_property_set_value);
int drm_connector_property_get_value(struct drm_connector *connector, struct drm_property *property, uint64_t *val) { - int i; + struct drm_connector_property *cprop;
- for (i = 0; i < DRM_CONNECTOR_MAX_PROPERTY; i++) { - if (connector->property_ids[i] == property->base.id) { - *val = connector->property_values[i]; - break; + list_for_each_entry(cprop, &connector->properties, list) { + if (cprop->base->base.id == property->base.id) { + *val = cprop->val; + return 0; } }
- if (i == DRM_CONNECTOR_MAX_PROPERTY) - return -EINVAL; - return 0; + return -EINVAL; } EXPORT_SYMBOL(drm_connector_property_get_value);
@@ -2916,6 +2916,7 @@ int drm_mode_connector_property_set_ioctl(struct drm_device *dev, struct drm_mode_connector_set_property *out_resp = data; struct drm_mode_object *obj; struct drm_property *property; + struct drm_connector_property *cprop; struct drm_connector *connector; int ret = -EINVAL; int i; @@ -2931,20 +2932,12 @@ int drm_mode_connector_property_set_ioctl(struct drm_device *dev, } connector = obj_to_connector(obj);
- for (i = 0; i < DRM_CONNECTOR_MAX_PROPERTY; i++) { - if (connector->property_ids[i] == out_resp->prop_id) - break; - } - - if (i == DRM_CONNECTOR_MAX_PROPERTY) { - goto out; - } - - obj = drm_mode_object_find(dev, out_resp->prop_id, DRM_MODE_OBJECT_PROPERTY); - if (!obj) { - goto out; - } - property = obj_to_property(obj); + list_for_each_entry(cprop, &connector->properties, list) + if (cprop->base->base.id == out_resp->prop_id) + goto found; + goto out; +found: + property = cprop->base;
if (property->flags & DRM_MODE_PROP_IMMUTABLE) goto out; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index c03ad9a..40f2107 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -451,7 +451,6 @@ struct drm_encoder_funcs { };
#define DRM_CONNECTOR_MAX_UMODES 16 -#define DRM_CONNECTOR_MAX_PROPERTY 16 #define DRM_CONNECTOR_LEN 32 #define DRM_CONNECTOR_MAX_ENCODER 3
@@ -565,8 +564,8 @@ struct drm_connector {
struct list_head user_modes; struct drm_property_blob *edid_blob_ptr; - u32 property_ids[DRM_CONNECTOR_MAX_PROPERTY]; - uint64_t property_values[DRM_CONNECTOR_MAX_PROPERTY]; + + struct list_head properties;
uint8_t polled; /* DRM_CONNECTOR_POLL_* */
The drivers currently check in set_property whether the property is unchanged. move this check into the core and do not bother the drivers with checking for unchanged properties.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/drm_crtc.c | 5 ++++ drivers/gpu/drm/gma500/cdv_intel_lvds.c | 10 -------- drivers/gpu/drm/gma500/psb_intel_lvds.c | 11 --------- drivers/gpu/drm/gma500/psb_intel_sdvo.c | 12 ---------- drivers/gpu/drm/i915/intel_dp.c | 6 ----- drivers/gpu/drm/i915/intel_tv.c | 27 ++++------------------ drivers/gpu/drm/nouveau/nouveau_connector.c | 32 ++++++++++---------------- drivers/gpu/drm/radeon/radeon_connectors.c | 24 ++++++------------- 8 files changed, 30 insertions(+), 97 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index b815e69..e03d4b8 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2961,6 +2961,11 @@ found: } }
+ if (cprop->val == out_resp->value) { + ret = 0; + goto out; + } + /* Do DPMS ourselves */ if (property == connector->dev->mode_config.dpms_property) { if (connector->funcs->dpms) diff --git a/drivers/gpu/drm/gma500/cdv_intel_lvds.c b/drivers/gpu/drm/gma500/cdv_intel_lvds.c index 50e744b..7569e8e 100644 --- a/drivers/gpu/drm/gma500/cdv_intel_lvds.c +++ b/drivers/gpu/drm/gma500/cdv_intel_lvds.c @@ -457,8 +457,6 @@ int cdv_intel_lvds_set_property(struct drm_connector *connector, if (!strcmp(property->name, "scaling mode") && encoder) { struct psb_intel_crtc *crtc = to_psb_intel_crtc(encoder->crtc); - uint64_t curValue; - if (!crtc) return -1;
@@ -473,14 +471,6 @@ int cdv_intel_lvds_set_property(struct drm_connector *connector, return -1; }
- if (drm_connector_property_get_value(connector, - property, - &curValue)) - return -1; - - if (curValue == value) - return 0; - if (drm_connector_property_set_value(connector, property, value)) diff --git a/drivers/gpu/drm/gma500/psb_intel_lvds.c b/drivers/gpu/drm/gma500/psb_intel_lvds.c index a25e4ca..7c9498ea 100644 --- a/drivers/gpu/drm/gma500/psb_intel_lvds.c +++ b/drivers/gpu/drm/gma500/psb_intel_lvds.c @@ -587,8 +587,6 @@ int psb_intel_lvds_set_property(struct drm_connector *connector, if (!strcmp(property->name, "scaling mode")) { struct psb_intel_crtc *crtc = to_psb_intel_crtc(encoder->crtc); - uint64_t curval; - if (!crtc) goto set_prop_error;
@@ -603,14 +601,6 @@ int psb_intel_lvds_set_property(struct drm_connector *connector, goto set_prop_error; }
- if (drm_connector_property_get_value(connector, - property, - &curval)) - goto set_prop_error; - - if (curval == value) - goto set_prop_done; - if (drm_connector_property_set_value(connector, property, value)) @@ -647,7 +637,6 @@ int psb_intel_lvds_set_property(struct drm_connector *connector, hfuncs->dpms(encoder, value); }
-set_prop_done: return 0; set_prop_error: return -1; diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c index 63a11dc..b9c6da9 100644 --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c @@ -1751,10 +1751,6 @@ psb_intel_sdvo_set_property(struct drm_connector *connector, if (val >= TV_FORMAT_NUM) return -EINVAL;
- if (psb_intel_sdvo->tv_format_index == - psb_intel_sdvo_connector->tv_format_supported[val]) - return 0; - psb_intel_sdvo->tv_format_index = psb_intel_sdvo_connector->tv_format_supported[val]; goto done; } else if (IS_TV_OR_LVDS(psb_intel_sdvo_connector)) { @@ -1762,8 +1758,6 @@ psb_intel_sdvo_set_property(struct drm_connector *connector, if (psb_intel_sdvo_connector->left == property) { drm_connector_property_set_value(connector, psb_intel_sdvo_connector->right, val); - if (psb_intel_sdvo_connector->left_margin == temp_value) - return 0;
psb_intel_sdvo_connector->left_margin = temp_value; psb_intel_sdvo_connector->right_margin = temp_value; @@ -1774,8 +1768,6 @@ psb_intel_sdvo_set_property(struct drm_connector *connector, } else if (psb_intel_sdvo_connector->right == property) { drm_connector_property_set_value(connector, psb_intel_sdvo_connector->left, val); - if (psb_intel_sdvo_connector->right_margin == temp_value) - return 0;
psb_intel_sdvo_connector->left_margin = temp_value; psb_intel_sdvo_connector->right_margin = temp_value; @@ -1786,8 +1778,6 @@ psb_intel_sdvo_set_property(struct drm_connector *connector, } else if (psb_intel_sdvo_connector->top == property) { drm_connector_property_set_value(connector, psb_intel_sdvo_connector->bottom, val); - if (psb_intel_sdvo_connector->top_margin == temp_value) - return 0;
psb_intel_sdvo_connector->top_margin = temp_value; psb_intel_sdvo_connector->bottom_margin = temp_value; @@ -1798,8 +1788,6 @@ psb_intel_sdvo_set_property(struct drm_connector *connector, } else if (psb_intel_sdvo_connector->bottom == property) { drm_connector_property_set_value(connector, psb_intel_sdvo_connector->top, val); - if (psb_intel_sdvo_connector->bottom_margin == temp_value) - return 0;
psb_intel_sdvo_connector->top_margin = temp_value; psb_intel_sdvo_connector->bottom_margin = temp_value; diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index db3b461..0024b59 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2223,9 +2223,6 @@ intel_dp_set_property(struct drm_connector *connector, int i = val; bool has_audio;
- if (i == intel_dp->force_audio) - return 0; - intel_dp->force_audio = i;
if (i == 0) @@ -2241,9 +2238,6 @@ intel_dp_set_property(struct drm_connector *connector, }
if (property == dev_priv->broadcast_rgb_property) { - if (val == !!intel_dp->color_range) - return 0; - intel_dp->color_range = val ? DP_COLOR_RANGE_16_235 : 0; goto done; } diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c index 1571be3..6eb11fe 100644 --- a/drivers/gpu/drm/i915/intel_tv.c +++ b/drivers/gpu/drm/i915/intel_tv.c @@ -1380,44 +1380,27 @@ intel_tv_set_property(struct drm_connector *connector, struct drm_property *prop struct intel_tv *intel_tv = intel_attached_tv(connector); struct drm_crtc *crtc = intel_tv->base.base.crtc; int ret = 0; - bool changed = false;
ret = drm_connector_property_set_value(connector, property, val); if (ret < 0) goto out;
- if (property == dev->mode_config.tv_left_margin_property && - intel_tv->margin[TV_MARGIN_LEFT] != val) { + if (property == dev->mode_config.tv_left_margin_property) { intel_tv->margin[TV_MARGIN_LEFT] = val; - changed = true; - } else if (property == dev->mode_config.tv_right_margin_property && - intel_tv->margin[TV_MARGIN_RIGHT] != val) { + } else if (property == dev->mode_config.tv_right_margin_property) { intel_tv->margin[TV_MARGIN_RIGHT] = val; - changed = true; - } else if (property == dev->mode_config.tv_top_margin_property && - intel_tv->margin[TV_MARGIN_TOP] != val) { + } else if (property == dev->mode_config.tv_top_margin_property) { intel_tv->margin[TV_MARGIN_TOP] = val; - changed = true; - } else if (property == dev->mode_config.tv_bottom_margin_property && - intel_tv->margin[TV_MARGIN_BOTTOM] != val) { + } else if (property == dev->mode_config.tv_bottom_margin_property) { intel_tv->margin[TV_MARGIN_BOTTOM] = val; - changed = true; } else if (property == dev->mode_config.tv_mode_property) { - if (val >= ARRAY_SIZE(tv_modes)) { - ret = -EINVAL; - goto out; - } - if (!strcmp(intel_tv->tv_format, tv_modes[val].name)) - goto out; - intel_tv->tv_format = tv_modes[val].name; - changed = true; } else { ret = -EINVAL; goto out; }
- if (changed && crtc) + if (crtc) drm_crtc_helper_set_mode(crtc, &crtc->mode, crtc->x, crtc->y, crtc->fb); out: diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c index f3ce34b..c149943 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.c +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c @@ -467,39 +467,31 @@ nouveau_connector_set_property(struct drm_connector *connector,
/* Underscan */ if (property == disp->underscan_property) { - if (nv_connector->underscan != value) { - nv_connector->underscan = value; - if (!nv_crtc || !nv_crtc->set_scale) - return 0; + nv_connector->underscan = value; + if (!nv_crtc || !nv_crtc->set_scale) + return 0;
- return nv_crtc->set_scale(nv_crtc, true); - } + return nv_crtc->set_scale(nv_crtc, true);
return 0; }
if (property == disp->underscan_hborder_property) { - if (nv_connector->underscan_hborder != value) { - nv_connector->underscan_hborder = value; - if (!nv_crtc || !nv_crtc->set_scale) - return 0; + nv_connector->underscan_hborder = value; + if (!nv_crtc || !nv_crtc->set_scale) + return 0;
- return nv_crtc->set_scale(nv_crtc, true); - } + return nv_crtc->set_scale(nv_crtc, true);
return 0; }
if (property == disp->underscan_vborder_property) { - if (nv_connector->underscan_vborder != value) { - nv_connector->underscan_vborder = value; - if (!nv_crtc || !nv_crtc->set_scale) - return 0; - - return nv_crtc->set_scale(nv_crtc, true); - } + nv_connector->underscan_vborder = value; + if (!nv_crtc || !nv_crtc->set_scale) + return 0;
- return 0; + return nv_crtc->set_scale(nv_crtc, true); }
/* Dithering */ diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index e7cb3ab..b0d2dcd 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -319,10 +319,8 @@ int radeon_connector_set_property(struct drm_connector *connector, struct drm_pr
dig = radeon_encoder->enc_priv; new_coherent_mode = val ? true : false; - if (dig->coherent_mode != new_coherent_mode) { - dig->coherent_mode = new_coherent_mode; - radeon_property_change_mode(&radeon_encoder->base); - } + dig->coherent_mode = new_coherent_mode; + radeon_property_change_mode(&radeon_encoder->base); }
if (property == rdev->mode_info.underscan_property) { @@ -333,10 +331,8 @@ int radeon_connector_set_property(struct drm_connector *connector, struct drm_pr
radeon_encoder = to_radeon_encoder(encoder);
- if (radeon_encoder->underscan_type != val) { - radeon_encoder->underscan_type = val; - radeon_property_change_mode(&radeon_encoder->base); - } + radeon_encoder->underscan_type = val; + radeon_property_change_mode(&radeon_encoder->base); }
if (property == rdev->mode_info.underscan_hborder_property) { @@ -347,10 +343,8 @@ int radeon_connector_set_property(struct drm_connector *connector, struct drm_pr
radeon_encoder = to_radeon_encoder(encoder);
- if (radeon_encoder->underscan_hborder != val) { - radeon_encoder->underscan_hborder = val; - radeon_property_change_mode(&radeon_encoder->base); - } + radeon_encoder->underscan_hborder = val; + radeon_property_change_mode(&radeon_encoder->base); }
if (property == rdev->mode_info.underscan_vborder_property) { @@ -361,10 +355,8 @@ int radeon_connector_set_property(struct drm_connector *connector, struct drm_pr
radeon_encoder = to_radeon_encoder(encoder);
- if (radeon_encoder->underscan_vborder != val) { - radeon_encoder->underscan_vborder = val; - radeon_property_change_mode(&radeon_encoder->base); - } + radeon_encoder->underscan_vborder = val; + radeon_property_change_mode(&radeon_encoder->base); }
if (property == rdev->mode_info.tv_std_property) {
On Wed, 1 Feb 2012 11:38:35 +0100, Sascha Hauer s.hauer@pengutronix.de wrote:
The drivers currently check in set_property whether the property is unchanged. move this check into the core and do not bother the drivers with checking for unchanged properties.
This patch seems to have functional side-effects beyond the description.
For example,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index db3b461..0024b59 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2223,9 +2223,6 @@ intel_dp_set_property(struct drm_connector *connector, int i = val; bool has_audio;
if (i == intel_dp->force_audio)return 0;
Here we are checking against the current value of the intel_dp, which may in theory be modified elsewhere as well, and avoiding the modeswitch if it is unnecessary. -Chris
On Wed, Feb 01, 2012 at 11:55:44AM +0000, Chris Wilson wrote:
On Wed, 1 Feb 2012 11:38:35 +0100, Sascha Hauer s.hauer@pengutronix.de wrote:
The drivers currently check in set_property whether the property is unchanged. move this check into the core and do not bother the drivers with checking for unchanged properties.
This patch seems to have functional side-effects beyond the description.
For example,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index db3b461..0024b59 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2223,9 +2223,6 @@ intel_dp_set_property(struct drm_connector *connector, int i = val; bool has_audio;
if (i == intel_dp->force_audio)return 0;Here we are checking against the current value of the intel_dp, which may in theory be modified elsewhere as well, and avoiding the modeswitch if it is unnecessary.
In case of force_audio I just checked that the current value is not changed elsewhere, but I must admit that I haven't checked this for all other properties. Would the patch be ok if I review for side effects?
BTW if someone changes the underlying variable of a property outside of the .set_property callback there are likely problems elsewhere because without calling drm_connector_property_set_value the values of the variable and the property backing store will be inconsistent. The calls to drm_connector_property_set_value in turn are quite easy reviewable, those are very few.
Sascha
On Wed, 1 Feb 2012 13:13:44 +0100, Sascha Hauer s.hauer@pengutronix.de wrote:
On Wed, Feb 01, 2012 at 11:55:44AM +0000, Chris Wilson wrote:
On Wed, 1 Feb 2012 11:38:35 +0100, Sascha Hauer s.hauer@pengutronix.de wrote:
The drivers currently check in set_property whether the property is unchanged. move this check into the core and do not bother the drivers with checking for unchanged properties.
This patch seems to have functional side-effects beyond the description.
For example,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index db3b461..0024b59 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2223,9 +2223,6 @@ intel_dp_set_property(struct drm_connector *connector, int i = val; bool has_audio;
if (i == intel_dp->force_audio)return 0;Here we are checking against the current value of the intel_dp, which may in theory be modified elsewhere as well, and avoiding the modeswitch if it is unnecessary.
In case of force_audio I just checked that the current value is not changed elsewhere, but I must admit that I haven't checked this for all other properties. Would the patch be ok if I review for side effects?
BTW if someone changes the underlying variable of a property outside of the .set_property callback there are likely problems elsewhere because without calling drm_connector_property_set_value the values of the variable and the property backing store will be inconsistent. The calls to drm_connector_property_set_value in turn are quite easy reviewable, those are very few.
Sure, it just jumped out as being not in the same pattern as the rest. I'd break out the ones that are different, like intel_dp, into their own patch so that the above analysis can be recorded along with the change and that we have something easy to bisect/change-our-minds about later. ;-) -Chris
If a property has changed successfully the core will call drm_connector_property_set_value, so do not duplicate this call in the drivers.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 4 ---- drivers/gpu/drm/gma500/cdv_intel_lvds.c | 23 ++++++----------------- drivers/gpu/drm/gma500/psb_intel_lvds.c | 25 +++++++------------------ drivers/gpu/drm/gma500/psb_intel_sdvo.c | 5 ----- drivers/gpu/drm/i915/intel_dp.c | 5 ----- drivers/gpu/drm/i915/intel_hdmi.c | 5 ----- drivers/gpu/drm/i915/intel_sdvo.c | 5 ----- drivers/gpu/drm/i915/intel_tv.c | 4 ---- 8 files changed, 13 insertions(+), 63 deletions(-)
diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c index de25560..87b7bd6 100644 --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c @@ -195,10 +195,6 @@ static int cdv_hdmi_set_property(struct drm_connector *connector, if (curValue == value) return 0;
- if (drm_connector_property_set_value(connector, - property, value)) - return -1; - centre = (curValue == DRM_MODE_SCALE_NO_SCALE) || (value == DRM_MODE_SCALE_NO_SCALE);
diff --git a/drivers/gpu/drm/gma500/cdv_intel_lvds.c b/drivers/gpu/drm/gma500/cdv_intel_lvds.c index 7569e8e..aeb9624 100644 --- a/drivers/gpu/drm/gma500/cdv_intel_lvds.c +++ b/drivers/gpu/drm/gma500/cdv_intel_lvds.c @@ -471,11 +471,6 @@ int cdv_intel_lvds_set_property(struct drm_connector *connector, return -1; }
- if (drm_connector_property_set_value(connector, - property, - value)) - return -1; - if (crtc->saved_mode.hdisplay != 0 && crtc->saved_mode.vdisplay != 0) { if (!drm_crtc_helper_set_mode(encoder->crtc, @@ -486,20 +481,14 @@ int cdv_intel_lvds_set_property(struct drm_connector *connector, return -1; } } else if (!strcmp(property->name, "backlight") && encoder) { - if (drm_connector_property_set_value(connector, - property, - value)) - return -1; - else { #ifdef CONFIG_BACKLIGHT_CLASS_DEVICE - struct drm_psb_private *dev_priv = - encoder->dev->dev_private; - struct backlight_device *bd = - dev_priv->backlight_device; - bd->props.brightness = value; - backlight_update_status(bd); + struct drm_psb_private *dev_priv = + encoder->dev->dev_private; + struct backlight_device *bd = + dev_priv->backlight_device; + bd->props.brightness = value; + backlight_update_status(bd); #endif - } } else if (!strcmp(property->name, "DPMS") && encoder) { struct drm_encoder_helper_funcs *helpers = encoder->helper_private; diff --git a/drivers/gpu/drm/gma500/psb_intel_lvds.c b/drivers/gpu/drm/gma500/psb_intel_lvds.c index 7c9498ea..112d48a 100644 --- a/drivers/gpu/drm/gma500/psb_intel_lvds.c +++ b/drivers/gpu/drm/gma500/psb_intel_lvds.c @@ -601,11 +601,6 @@ int psb_intel_lvds_set_property(struct drm_connector *connector, goto set_prop_error; }
- if (drm_connector_property_set_value(connector, - property, - value)) - goto set_prop_error; - if (crtc->saved_mode.hdisplay != 0 && crtc->saved_mode.vdisplay != 0) { if (!drm_crtc_helper_set_mode(encoder->crtc, @@ -616,21 +611,15 @@ int psb_intel_lvds_set_property(struct drm_connector *connector, goto set_prop_error; } } else if (!strcmp(property->name, "backlight")) { - if (drm_connector_property_set_value(connector, - property, - value)) - goto set_prop_error; - else { #ifdef CONFIG_BACKLIGHT_CLASS_DEVICE - struct drm_psb_private *devp = - encoder->dev->dev_private; - struct backlight_device *bd = devp->backlight_device; - if (bd) { - bd->props.brightness = value; - backlight_update_status(bd); - } -#endif + struct drm_psb_private *devp = + encoder->dev->dev_private; + struct backlight_device *bd = devp->backlight_device; + if (bd) { + bd->props.brightness = value; + backlight_update_status(bd); } +#endif } else if (!strcmp(property->name, "DPMS")) { struct drm_encoder_helper_funcs *hfuncs = encoder->helper_private; diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c index b9c6da9..4396be8 100644 --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c @@ -1703,11 +1703,6 @@ psb_intel_sdvo_set_property(struct drm_connector *connector, struct drm_psb_private *dev_priv = connector->dev->dev_private; uint16_t temp_value; uint8_t cmd; - int ret; - - ret = drm_connector_property_set_value(connector, property, val); - if (ret) - return ret;
if (property == dev_priv->force_audio_property) { int i = val; diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 0024b59..32fba41 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2213,11 +2213,6 @@ intel_dp_set_property(struct drm_connector *connector, { struct drm_i915_private *dev_priv = connector->dev->dev_private; struct intel_dp *intel_dp = intel_attached_dp(connector); - int ret; - - ret = drm_connector_property_set_value(connector, property, val); - if (ret) - return ret;
if (property == dev_priv->force_audio_property) { int i = val; diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 64541f7..8218be5 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -395,11 +395,6 @@ intel_hdmi_set_property(struct drm_connector *connector, { struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); struct drm_i915_private *dev_priv = connector->dev->dev_private; - int ret; - - ret = drm_connector_property_set_value(connector, property, val); - if (ret) - return ret;
if (property == dev_priv->force_audio_property) { int i = val; diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 24d9e62..7f4e758 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -1669,11 +1669,6 @@ intel_sdvo_set_property(struct drm_connector *connector, struct drm_i915_private *dev_priv = connector->dev->dev_private; uint16_t temp_value; uint8_t cmd; - int ret; - - ret = drm_connector_property_set_value(connector, property, val); - if (ret) - return ret;
if (property == dev_priv->force_audio_property) { int i = val; diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c index 6eb11fe..5d074f5 100644 --- a/drivers/gpu/drm/i915/intel_tv.c +++ b/drivers/gpu/drm/i915/intel_tv.c @@ -1381,10 +1381,6 @@ intel_tv_set_property(struct drm_connector *connector, struct drm_property *prop struct drm_crtc *crtc = intel_tv->base.base.crtc; int ret = 0;
- ret = drm_connector_property_set_value(connector, property, val); - if (ret < 0) - goto out; - if (property == dev->mode_config.tv_left_margin_property) { intel_tv->margin[TV_MARGIN_LEFT] = val; } else if (property == dev->mode_config.tv_right_margin_property) {
info->fix.visual already is correctly set from drm_fb_helper_fill_fix. info->fix.line_length is also set from drm_fb_helper_fill_fix, so drm_fb_helper_set_par directly instead of a custom exynos_drm_fbdev_set_par.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de Cc: Inki Dae inki.dae@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 28 +--------------------------- 1 files changed, 1 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index 706c906..e4bb88e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -46,39 +46,13 @@ struct exynos_drm_fbdev { struct exynos_drm_gem_obj *exynos_gem_obj; };
-static int exynos_drm_fbdev_set_par(struct fb_info *info) -{ - struct fb_var_screeninfo *var = &info->var; - - switch (var->bits_per_pixel) { - case 32: - case 24: - case 18: - case 16: - case 12: - info->fix.visual = FB_VISUAL_TRUECOLOR; - break; - case 1: - info->fix.visual = FB_VISUAL_MONO01; - break; - default: - info->fix.visual = FB_VISUAL_PSEUDOCOLOR; - break; - } - - info->fix.line_length = (var->xres_virtual * var->bits_per_pixel) / 8; - - return drm_fb_helper_set_par(info); -} - - static struct fb_ops exynos_drm_fb_ops = { .owner = THIS_MODULE, .fb_fillrect = cfb_fillrect, .fb_copyarea = cfb_copyarea, .fb_imageblit = cfb_imageblit, .fb_check_var = drm_fb_helper_check_var, - .fb_set_par = exynos_drm_fbdev_set_par, + .fb_set_par = drm_fb_helper_set_par, .fb_blank = drm_fb_helper_blank, .fb_pan_display = drm_fb_helper_pan_display, .fb_setcmap = drm_fb_helper_setcmap,
-----Original Message----- From: Sascha Hauer [mailto:s.hauer@pengutronix.de] Sent: Wednesday, February 01, 2012 7:39 PM To: dri-devel@lists.freedesktop.org Cc: kernel@pengutronix.de; Sascha Hauer; Inki Dae Subject: [PATCH 19/20] drm exynos: use drm_fb_helper_set_par directly
info->fix.visual already is correctly set from drm_fb_helper_fill_fix. info->fix.line_length is also set from drm_fb_helper_fill_fix, so drm_fb_helper_set_par directly instead of a custom exynos_drm_fbdev_set_par.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de Cc: Inki Dae inki.dae@samsung.com
drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 28
+------------------------
1 files changed, 1 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index 706c906..e4bb88e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -46,39 +46,13 @@ struct exynos_drm_fbdev { struct exynos_drm_gem_obj *exynos_gem_obj; };
-static int exynos_drm_fbdev_set_par(struct fb_info *info) -{
- struct fb_var_screeninfo *var = &info->var;
- switch (var->bits_per_pixel) {
- case 32:
- case 24:
- case 18:
- case 16:
- case 12:
info->fix.visual = FB_VISUAL_TRUECOLOR;break;- case 1:
info->fix.visual = FB_VISUAL_MONO01;break;- default:
info->fix.visual = FB_VISUAL_PSEUDOCOLOR;break;- }
- info->fix.line_length = (var->xres_virtual * var->bits_per_pixel) /
8;
- return drm_fb_helper_set_par(info);
-}
static struct fb_ops exynos_drm_fb_ops = { .owner = THIS_MODULE, .fb_fillrect = cfb_fillrect, .fb_copyarea = cfb_copyarea, .fb_imageblit = cfb_imageblit, .fb_check_var = drm_fb_helper_check_var,
- .fb_set_par = exynos_drm_fbdev_set_par,
- .fb_set_par = drm_fb_helper_set_par, .fb_blank = drm_fb_helper_blank, .fb_pan_display = drm_fb_helper_pan_display, .fb_setcmap = drm_fb_helper_setcmap,
-- 1.7.8.3
Tested-by: Inki Dae inki.dae@samsung.com Thanks.
The drm drivers set the fb_info->pixmap fields without setting fb_info->pixmap.addr. If this is not set the fb core will overwrite these all fb_info->pixmap fields anyway, so there is not much point in setting them in the first place.
Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- drivers/gpu/drm/gma500/framebuffer.c | 6 ------ drivers/gpu/drm/i915/intel_fb.c | 6 ------ drivers/gpu/drm/nouveau/nouveau_fbcon.c | 6 ------ drivers/gpu/drm/radeon/radeon_fb.c | 6 ------ drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 14 -------------- drivers/video/nvidia/nvidia.c | 6 ------ 6 files changed, 0 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index f359620..9ff4e5d 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -507,12 +507,6 @@ static int psbfb_create(struct psb_fbdev *fbdev, info->fix.mmio_start = pci_resource_start(dev->pdev, 0); info->fix.mmio_len = pci_resource_len(dev->pdev, 0);
- info->pixmap.size = 64 * 1024; - info->pixmap.buf_align = 8; - info->pixmap.access_align = 32; - info->pixmap.flags = FB_PIXMAP_SYSTEM; - info->pixmap.scan_align = 1; - dev_info(dev->dev, "allocated %dx%d fb\n", psbfb->base.width, psbfb->base.height);
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c index 8f81286..e4c7038 100644 --- a/drivers/gpu/drm/i915/intel_fb.c +++ b/drivers/gpu/drm/i915/intel_fb.c @@ -152,12 +152,6 @@ static int intelfb_create(struct intel_fbdev *ifbdev, drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth); drm_fb_helper_fill_var(info, &ifbdev->helper, sizes->fb_width, sizes->fb_height);
- info->pixmap.size = 64*1024; - info->pixmap.buf_align = 8; - info->pixmap.access_align = 32; - info->pixmap.flags = FB_PIXMAP_SYSTEM; - info->pixmap.scan_align = 1; - DRM_DEBUG_KMS("allocated %dx%d fb: 0x%08x, bo %p\n", fb->width, fb->height, obj->gtt_offset, obj); diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c index 01061bb..43fd21e 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c @@ -381,12 +381,6 @@ nouveau_fbcon_create(struct nouveau_fbdev *nfbdev, goto out_unref; }
- info->pixmap.size = 64*1024; - info->pixmap.buf_align = 8; - info->pixmap.access_align = 32; - info->pixmap.flags = FB_PIXMAP_SYSTEM; - info->pixmap.scan_align = 1; - mutex_unlock(&dev->struct_mutex);
if (dev_priv->channel && !nouveau_nofbaccel) { diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c index ed5a642..f5b600f 100644 --- a/drivers/gpu/drm/radeon/radeon_fb.c +++ b/drivers/gpu/drm/radeon/radeon_fb.c @@ -254,12 +254,6 @@ static int radeonfb_create(struct radeon_fbdev *rfbdev, info->apertures->ranges[0].base = rdev->ddev->mode_config.fb_base; info->apertures->ranges[0].size = rdev->mc.aper_size;
- info->pixmap.size = 64*1024; - info->pixmap.buf_align = 8; - info->pixmap.access_align = 32; - info->pixmap.flags = FB_PIXMAP_SYSTEM; - info->pixmap.scan_align = 1; - if (info->screen_base == NULL) { ret = -ENOSPC; goto out_unref; diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c index 34e51a1..34d1e15 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c @@ -515,20 +515,6 @@ int vmw_fb_init(struct vmw_private *vmw_priv) info->var.xres = initial_width; info->var.yres = initial_height;
-#if 0 - info->pixmap.size = 64*1024; - info->pixmap.buf_align = 8; - info->pixmap.access_align = 32; - info->pixmap.flags = FB_PIXMAP_SYSTEM; - info->pixmap.scan_align = 1; -#else - info->pixmap.size = 0; - info->pixmap.buf_align = 8; - info->pixmap.access_align = 32; - info->pixmap.flags = FB_PIXMAP_SYSTEM; - info->pixmap.scan_align = 1; -#endif - info->apertures = alloc_apertures(1); if (!info->apertures) { ret = -ENOMEM; diff --git a/drivers/video/nvidia/nvidia.c b/drivers/video/nvidia/nvidia.c index fe13ac5..8e29b57 100644 --- a/drivers/video/nvidia/nvidia.c +++ b/drivers/video/nvidia/nvidia.c @@ -1167,12 +1167,6 @@ static int __devinit nvidia_set_fbinfo(struct fb_info *info) ((info->var.bits_per_pixel + 7) >> 3); info->var.yres_virtual = info->screen_size / lpitch;
- info->pixmap.scan_align = 4; - info->pixmap.buf_align = 4; - info->pixmap.access_align = 32; - info->pixmap.size = 8 * 1024; - info->pixmap.flags = FB_PIXMAP_SYSTEM; - if (!hwcur) info->fbops->fb_cursor = NULL;
On Wed, 1 Feb 2012 11:38:38 +0100, Sascha Hauer s.hauer@pengutronix.de wrote:
The drm drivers set the fb_info->pixmap fields without setting fb_info->pixmap.addr. If this is not set the fb core will overwrite these all fb_info->pixmap fields anyway, so there is not much point in setting them in the first place.
Might be worth replacing those blocks with a comment: /* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */ -Chris
Hi David,
On Wed, Feb 01, 2012 at 11:38:18AM +0100, Sascha Hauer wrote:
The following patches contain some fixes and cleanups for the drm core.
- fix memory holes
- make some initialization / deinitialization more symmetric
- add convenience functions for creating properties
- remove DRM_CONNECTOR_MAX_PROPERTY limitation
All patches tested on a GeForce 6200 LE with the nouveau driver and a DELL E6220 Laptop using the intel driver.
Please review and consider applying
Is the series otherwise series ok? If yes I would integrate the comments received so far and repost.
Sascha
On Thu, Feb 2, 2012 at 2:13 PM, Sascha Hauer s.hauer@pengutronix.de wrote:
Hi David,
On Wed, Feb 01, 2012 at 11:38:18AM +0100, Sascha Hauer wrote:
The following patches contain some fixes and cleanups for the drm core.
- fix memory holes
- make some initialization / deinitialization more symmetric
- add convenience functions for creating properties
- remove DRM_CONNECTOR_MAX_PROPERTY limitation
All patches tested on a GeForce 6200 LE with the nouveau driver and a DELL E6220 Laptop using the intel driver.
Please review and consider applying
Is the series otherwise series ok? If yes I would integrate the comments received so far and repost.
Hi Sascha,
I've merged some of the easier one into -next, pushed out now.
The rest I've replied to or would like to see review comments addressed.
Thanks, Dave.
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Dave Airlie -
David Airlie -
Inki Dae -
Sascha Hauer