[Intel-gfx] [PATCH 5/7 v2] drm/i915: Add setters for min/max frequency
Provide a standardized sysfs interface for setting min, and max frequencies. The code which reads the limits were lifted from the debugfs files. As a brief explanation, the limits are similar to the CPU p-states. We have 3 states: RP0 - ie. max frequency RP1 - ie. "preferred min" frequency RPn - seriously lowest frequency Initially Daniel asked me to clamp the writes to supported values, but in conforming to the way the cpufreq drivers seem to work, instead return -EINVAL (noticed by Jesse in discussion). The values can be used by userspace wishing to control the limits of the GPU (see the CC list for people who care). v2: add the dropped mutex_unlock in error cases (Chris) EINVAL on both too min, or too max (Daniel) CC: Arjan van de Ven CC: Jacob Pan CC: Daniel Vetter CC: Jesse Barnes Signed-off-by: Ben Widawsky --- drivers/gpu/drm/i915/i915_sysfs.c | 84 ++- 1 file changed, 82 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 6c7f905..4f470cf 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -238,6 +238,46 @@ static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute return snprintf(buf, PAGE_SIZE, "%d", ret); } +static ssize_t gt_max_freq_mhz_store(struct device *kdev, +struct device_attribute *attr, +const char *buf, size_t count) +{ + struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev); + struct drm_device *dev = minor->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + u32 val, rp_state_cap, hw_max, hw_min; + ssize_t ret; + + ret = kstrtou32(buf, 0, &val); + if (ret) + return ret; + + ret = mutex_lock_interruptible(&dev->struct_mutex); + if (ret) + return ret; + + rp_state_cap = I915_READ(GEN6_RP_STATE_CAP); + hw_max = (rp_state_cap & 0xff) * GT_FREQUENCY_MULTIPLIER; + hw_min = ((rp_state_cap & 0xff) >> 16) * GT_FREQUENCY_MULTIPLIER; + + if (val < hw_min || val > hw_max) { + mutex_unlock(&dev->struct_mutex); + return -EINVAL; + } + + if (val < dev_priv->rps.min_delay) + val = dev_priv->rps.min_delay; + + if (dev_priv->rps.cur_delay > val) + gen6_set_rps(dev_priv->dev, val); + + dev_priv->rps.max_delay = val / GT_FREQUENCY_MULTIPLIER; + + mutex_unlock(&dev->struct_mutex); + + return count; +} + static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) { struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev); @@ -255,9 +295,49 @@ static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute return snprintf(buf, PAGE_SIZE, "%d", ret); } +static ssize_t gt_min_freq_mhz_store(struct device *kdev, +struct device_attribute *attr, +const char *buf, size_t count) +{ + struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev); + struct drm_device *dev = minor->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + u32 val, rp_state_cap, hw_max, hw_min; + ssize_t ret; + + ret = kstrtou32(buf, 0, &val); + if (ret) + return ret; + + ret = mutex_lock_interruptible(&dev->struct_mutex); + if (ret) + return ret; + + rp_state_cap = I915_READ(GEN6_RP_STATE_CAP); + hw_max = (rp_state_cap & 0xff) * GT_FREQUENCY_MULTIPLIER; + hw_min = ((rp_state_cap & 0xff) >> 16) * GT_FREQUENCY_MULTIPLIER; + + if (val < hw_min || val > hw_max) { + mutex_unlock(&dev->struct_mutex); + return -EINVAL; + } + + if (val > dev_priv->rps.max_delay) + val = dev_priv->rps.max_delay; + + if (dev_priv->rps.cur_delay < val) + gen6_set_rps(dev_priv->dev, val); + + dev_priv->rps.min_delay = val / GT_FREQUENCY_MULTIPLIER; + + mutex_unlock(&dev->struct_mutex); + + return count; +} + static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show, NULL); -static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, NULL); -static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, NULL); +static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, gt_max_freq_mhz_store); +static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, gt_min_freq_mhz_store); static const struct attribute *gen6_attrs[] = { &dev_attr_gt_cur_freq_mhz.attr, -- 1.7.12 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/24] drm/i915: Replace the array of pages with a scatterlist
On Tue, 4 Sep 2012 21:02:57 +0100 Chris Wilson wrote: > Rather than have multiple data structures for describing our page layout > in conjunction with the array of pages, we can migrate all users over to > a scatterlist. > > One major advantage, other than unifying the page tracking structures, > this offers is that we replace the vmalloc'ed array (which can be up to > a megabyte in size) with a chain of individual pages which helps reduce > memory pressure. > > The disadvantage is that we then do not have a simple array to iterate, > or to access randomly. The common case for this is in the relocation > processing, which will typically fit within a single scatterlist page > and so be almost the same cost as the simple array. For iterating over > the array, the extra function call could be optimised away, but in > reality is an insignificant cost of either binding the pages, or > performing the pwrite/pread. > > Signed-off-by: Chris Wilson Now that my eyes are done bleeding, easy ones: ERROR: space required after that ',' (ctx:VxV) #69: FILE: drivers/char/agp/intel-gtt.c:99: + for_each_sg(st->sgl, sg, num_entries,i) ^ WARNING: Prefer pr_err(... to printk(KERN_ERR, ... #189: FILE: drivers/gpu/drm/drm_cache.c:117: + printk(KERN_ERR "Timed out waiting for cache flush.\n"); WARNING: Prefer pr_err(... to printk(KERN_ERR, ... #191: FILE: drivers/gpu/drm/drm_cache.c:119: + printk(KERN_ERR "Architecture has no drm_cache.c support\n"); In addition to the inline comments, it would have been even slightly easier to review without the s/page/i since it seems to just be for no compelling reason anyway. > --- > drivers/char/agp/intel-gtt.c | 51 +--- > drivers/gpu/drm/drm_cache.c| 23 ++ > drivers/gpu/drm/i915/i915_drv.h| 18 +++-- > drivers/gpu/drm/i915/i915_gem.c| 79 -- > drivers/gpu/drm/i915/i915_gem_dmabuf.c | 99 +++ > drivers/gpu/drm/i915/i915_gem_execbuffer.c |3 +- > drivers/gpu/drm/i915/i915_gem_gtt.c| 121 > ++-- > drivers/gpu/drm/i915/i915_gem_tiling.c | 16 ++-- > drivers/gpu/drm/i915/i915_irq.c| 25 +++--- > drivers/gpu/drm/i915/intel_ringbuffer.c|9 ++- > include/drm/drmP.h |1 + > include/drm/intel-gtt.h| 10 +-- > 12 files changed, 236 insertions(+), 219 deletions(-) > > diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c > index 58e32f7..511d1b1 100644 > --- a/drivers/char/agp/intel-gtt.c > +++ b/drivers/char/agp/intel-gtt.c > @@ -84,40 +84,33 @@ static struct _intel_private { > #define IS_IRONLAKE intel_private.driver->is_ironlake > #define HAS_PGTBL_EN intel_private.driver->has_pgtbl_enable > > -int intel_gtt_map_memory(struct page **pages, unsigned int num_entries, > - struct scatterlist **sg_list, int *num_sg) > +static int intel_gtt_map_memory(struct page **pages, > + unsigned int num_entries, > + struct sg_table *st) > { > - struct sg_table st; > struct scatterlist *sg; > int i; > > - if (*sg_list) > - return 0; /* already mapped (for e.g. resume */ > - > DBG("try mapping %lu pages\n", (unsigned long)num_entries); > > - if (sg_alloc_table(&st, num_entries, GFP_KERNEL)) > + if (sg_alloc_table(st, num_entries, GFP_KERNEL)) > goto err; > > - *sg_list = sg = st.sgl; > - > - for (i = 0 ; i < num_entries; i++, sg = sg_next(sg)) > + for_each_sg(st->sgl, sg, num_entries,i) > sg_set_page(sg, pages[i], PAGE_SIZE, 0); > > - *num_sg = pci_map_sg(intel_private.pcidev, *sg_list, > - num_entries, PCI_DMA_BIDIRECTIONAL); > - if (unlikely(!*num_sg)) > + if (!pci_map_sg(intel_private.pcidev, > + st->sgl, st->nents, PCI_DMA_BIDIRECTIONAL)) > goto err; > > return 0; > > err: > - sg_free_table(&st); > + sg_free_table(st); > return -ENOMEM; > } > -EXPORT_SYMBOL(intel_gtt_map_memory); > > -void intel_gtt_unmap_memory(struct scatterlist *sg_list, int num_sg) > +static void intel_gtt_unmap_memory(struct scatterlist *sg_list, int num_sg) > { > struct sg_table st; > DBG("try unmapping %lu pages\n", (unsigned long)mem->page_count); > @@ -130,7 +123,6 @@ void intel_gtt_unmap_memory(struct scatterlist *sg_list, > int num_sg) > > sg_free_table(&st); > } > -EXPORT_SYMBOL(intel_gtt_unmap_memory); > > static void intel_fake_agp_enable(struct agp_bridge_data *bridge, u32 mode) > { > @@ -879,8 +871,7 @@ static bool i830_check_flags(unsigned int flags) > return false; > } > > -void intel_gtt_insert_sg_entries(struct scatterlist *sg_list, > - unsigned int sg_le
Re: [Intel-gfx] [PATCH 00/58] modeset-rework, the basic conversion
On Wed, 5 Sep 2012 16:23:55 -0700 Jesse Barnes wrote: > On Sun, 19 Aug 2012 21:12:17 +0200 > Daniel Vetter wrote: > > > Hi all, > > > > Changes since last time around: > > - The prep patches are all merged now. > > - I've left out the actual DP fixes/cleanups, I think we should merge those > > in a > > separte step. > > - A few bugfixes (thanks to Paulo, Jani and Chris). > > - I've also applied a few bikesheds for naming that Paulo suggested (but > > I'm not > > sure whether I've sent those out already in a previous patchbomb). > > > > Essentially this is just the core rework, which addes the new get_hw_state > > code, > > refactors all the encoders to use the new functions and finally reworks the > > modeset logic to disable/enable entire pipes, always (and with a > > deterministic > > order). > > > > For merging to -next, I plan to pull in everything with a real merge > > commit. For > > that reason I've put up a modeset-rework-base branch onto my private fdo > > repo[1]. > > That way I can put a short documentation for the new modeset design into the > > merge commit (stichted together from the previous patchbomb cover letters), > > documenting my folly assumptions for eternity. > > > > I'll also plan to put tags for the entire series in the merge commit, so if > > you > > have tested this on a few machines, read through and agree with the new > > designs, > > please reply with your tested-by/acked-by/reviewed-by tags. > > > > Flames, comments and test reports highly welcome. > > Ok I've tested on Ironlake, Cantiga, Crestline, and Pineview so far and > things look good. I ran testdisplay both with and without VGA > attached (the ILK has a eDP panel), and tried S3 and S4 both with and > without VGA both in the console and in X. > > There was one issue on Pineview where the VGA seemed to get > "forgotten", but I haven't isolated it yet. I canceled testdisplay > part way through and that seemed to confuse fbcon about what was there. > > I also see an issue with 1280x800 modes across all platforms, but that > may just be the monitor, I need to test more. > > I'm testing Montara (that's 855 for you youngsters) now, but it's slow > so not all my builds have completed there yet. > > Overall though: > Tested-by: Jesse Barnes > Looks like I just found an issue with DPMS on fbcon on SNB though. Leaving the system for awhile either results in corruption (part of the fbcon on the screen) or just the screen left on. This is on LVDS. -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/24] drm/i915: Pin backing pages for pread
On Tue, 4 Sep 2012 21:02:56 +0100 Chris Wilson wrote: > By using the recently introduced pinning of pages, we can safely drop > the mutex in the knowledge that the pages are not going to disappear > beneath us, and so we can simplify the code for iterating over the pages. > > Signed-off-by: Chris Wilson If you fix patch3 to explain, it explain here too. Write is always scarier for me anyway. With fixed patch 3, it's Reviewed-by: Ben Widawsky -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/24] drm/i915: Pin backing pages for pwrite
On Tue, 4 Sep 2012 21:02:55 +0100 Chris Wilson wrote: > By using the recently introduced pinning of pages, we can safely drop > the mutex in the knowledge that the pages are not going to disappear > beneath us, and so we can simplify the code for iterating over the pages. > > Signed-off-by: Chris Wilson > --- > drivers/gpu/drm/i915/i915_gem.c | 37 + > 1 file changed, 13 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index aa088ef..8a4eac0 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -690,7 +690,7 @@ shmem_pwrite_fast(struct page *page, int > shmem_page_offset, int page_length, > page_length); > kunmap_atomic(vaddr); > > - return ret; > + return ret ? -EFAULT : 0; > } > > /* Only difference to the fast-path function is that this can handle bit17 > @@ -724,7 +724,7 @@ shmem_pwrite_slow(struct page *page, int > shmem_page_offset, int page_length, >page_do_bit17_swizzling); > kunmap(page); > > - return ret; > + return ret ? -EFAULT : 0; > } > > static int > @@ -733,7 +733,6 @@ i915_gem_shmem_pwrite(struct drm_device *dev, > struct drm_i915_gem_pwrite *args, > struct drm_file *file) > { > - struct address_space *mapping = > obj->base.filp->f_path.dentry->d_inode->i_mapping; > ssize_t remain; > loff_t offset; > char __user *user_data; Without digging to deep to see if you looked already. It would be nice if we can get a DRM_INFO or something for cases where return isn't actually EFAULT. > @@ -742,7 +741,6 @@ i915_gem_shmem_pwrite(struct drm_device *dev, > int hit_slowpath = 0; > int needs_clflush_after = 0; > int needs_clflush_before = 0; > - int release_page; > > user_data = (char __user *) (uintptr_t) args->data_ptr; > remain = args->size; > @@ -768,6 +766,12 @@ i915_gem_shmem_pwrite(struct drm_device *dev, > && obj->cache_level == I915_CACHE_NONE) > needs_clflush_before = 1; > > + ret = i915_gem_object_get_pages(obj); > + if (ret) > + return ret; > + > + i915_gem_object_pin_pages(obj); > + > offset = args->offset; > obj->dirty = 1; > > @@ -793,18 +797,7 @@ i915_gem_shmem_pwrite(struct drm_device *dev, > ((shmem_page_offset | page_length) > & (boot_cpu_data.x86_clflush_size - 1)); > > - if (obj->pages) { > - page = obj->pages[offset >> PAGE_SHIFT]; > - release_page = 0; > - } else { > - page = shmem_read_mapping_page(mapping, offset >> > PAGE_SHIFT); > - if (IS_ERR(page)) { > - ret = PTR_ERR(page); > - goto out; > - } > - release_page = 1; > - } > - > + page = obj->pages[offset >> PAGE_SHIFT]; > page_do_bit17_swizzling = obj_do_bit17_swizzling && > (page_to_phys(page) & (1 << 17)) != 0; > So the obvious question is what about the page caching? Can you add to the commit message for my edification why previously the shmem page is released from the page cache and now it isn't? > @@ -816,26 +809,20 @@ i915_gem_shmem_pwrite(struct drm_device *dev, > goto next_page; > > hit_slowpath = 1; > - page_cache_get(page); > mutex_unlock(&dev->struct_mutex); > - > ret = shmem_pwrite_slow(page, shmem_page_offset, page_length, > user_data, page_do_bit17_swizzling, > partial_cacheline_write, > needs_clflush_after); > > mutex_lock(&dev->struct_mutex); > - page_cache_release(page); > + > next_page: > set_page_dirty(page); > mark_page_accessed(page); > - if (release_page) > - page_cache_release(page); > > - if (ret) { > - ret = -EFAULT; > + if (ret) > goto out; > - } > > remain -= page_length; > user_data += page_length; > @@ -843,6 +830,8 @@ next_page: > } > > out: > + i915_gem_object_unpin_pages(obj); > + > if (hit_slowpath) { > /* Fixup: Kill any reinstated backing storage pages */ > if (obj->madv == __I915_MADV_PURGED) -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/7 v2] drm/i915: Add current GPU freq to sysfs
On 2012-09-06 14:09, Michael Larabel wrote: Have you discussed this with Radeon/Nouveau to see if they would adopt a common sysfs interface? In the past, Eugeni Dodonov and I had talked about this with a desire to have the open-source drivers expose the useful information for monitoring in a similar manner for helping userspace (current/max/min clock speeds, voltage, etc). [My personal interest is from monitoring this information in a more standardized way from the Phoronix Test Suite system monitoring components rather than writing driver-specific paths like it is now.] Martin (CCed) has also expressed interest from the Nouveau side in calling for some standardization. -- Michael We can... I did very briefly look at what Nouveau does with the hwmon interface, and what DRM provided, and thought it'd be more work than I wanted to sign up for (which is just about nothing sysfs other than display). Thanks for bringing him into the thread though. On 09/06/2012 03:54 PM, Ben Widawsky wrote: Userspace applications such as PowerTOP are interesting in being able to read the current GPU frequency. The patch itself sets up a generic array for gen6 attributes so we can easily add other items in the future (and it also happens to be just about the cleanest way to do this). The patch is a nice addition to commit 1ac02185dff3afac146d745ba220dc6672d1d162 Author: Daniel Vetter Date: Thu Aug 30 13:26:48 2012 +0200 drm/i915: add a tracepoint for gpu frequency changes Reading the GPU frequncy can be done by reading a file like: /sys/class/drm/card0/render_frequency_mhz v2: rename the sysfs file to gt_cur_freq_mhz (Daniel) Use kdev naming (Chris) Use DEVICE_ATTR over __ATTR (Ben) Add min/max freq (Ben/Daniel) user the new #define for frequency multiplier CC: Arjan van de Ven CC: Jacob Pan Signed-off-by: Ben Widawsky --- drivers/gpu/drm/i915/i915_sysfs.c | 71 +++ 1 file changed, 71 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index c701a17..6c7f905 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -203,6 +203,70 @@ static struct bin_attribute dpf_attrs = { .mmap = NULL }; +static ssize_t gt_cur_freq_mhz_show(struct device *kdev, + struct device_attribute *attr, char *buf) +{ + struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev); + struct drm_device *dev = minor->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + int ret; + + ret = i915_mutex_lock_interruptible(dev); + if (ret) + return ret; + + ret = dev_priv->rps.cur_delay * GT_FREQUENCY_MULTIPLIER; + mutex_unlock(&dev->struct_mutex); + + return snprintf(buf, PAGE_SIZE, "%d", ret); +} + +static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) +{ + struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev); + struct drm_device *dev = minor->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + int ret; + + ret = i915_mutex_lock_interruptible(dev); + if (ret) + return ret; + + ret = dev_priv->rps.max_delay * GT_FREQUENCY_MULTIPLIER; + mutex_unlock(&dev->struct_mutex); + + return snprintf(buf, PAGE_SIZE, "%d", ret); +} + +static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) +{ + struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev); + struct drm_device *dev = minor->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + int ret; + + ret = i915_mutex_lock_interruptible(dev); + if (ret) + return ret; + + ret = dev_priv->rps.min_delay * GT_FREQUENCY_MULTIPLIER; + mutex_unlock(&dev->struct_mutex); + + return snprintf(buf, PAGE_SIZE, "%d", ret); +} + +static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show, NULL); +static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, NULL); +static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, NULL); + +static const struct attribute *gen6_attrs[] = { + &dev_attr_gt_cur_freq_mhz.attr, + &dev_attr_gt_max_freq_mhz.attr, + &dev_attr_gt_min_freq_mhz.attr, + NULL, +}; + + void i915_setup_sysfs(struct drm_device *dev) { int ret; @@ -220,10 +284,17 @@ void i915_setup_sysfs(struct drm_device *dev) if (ret) DRM_ERROR("l3 parity sysfs setup failed\n"); } + + if (INTEL_INFO(dev)->gen >= 6) { + ret = sysfs_create_files(&dev->primary->kdev.kobj, gen6_attrs); + if (ret) + DRM_ERROR("gen6 sysfs setup failed\n"); + } } void i915_teardown_sysfs(struct drm_device *dev) { + sysfs_remove_file
Re: [Intel-gfx] [PATCH 02/24] drm/i915: Pin backing pages whilst exporting through a dmabuf vmap
On Tue, 4 Sep 2012 21:02:54 +0100 Chris Wilson wrote: > We need to refcount our pages in order to prevent reaping them at > inopportune times, such as when they currently vmapped or exported to > another driver. However, we also wish to keep the lazy deallocation of > our pages so we need to take a pin/unpinned approach rather than a > simple refcount. I've not followed the dmabuf development much but is there no interface to map partial objects, ie. have some pages of an object pinned, but not all? > > Signed-off-by: Chris Wilson With comment below addressed or not it's: Reviewed-by: Ben Widawsky > --- > drivers/gpu/drm/i915/i915_drv.h| 12 > drivers/gpu/drm/i915/i915_gem.c| 11 +-- > drivers/gpu/drm/i915/i915_gem_dmabuf.c |8 ++-- > 3 files changed, 27 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index f180874..0747472 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -994,6 +994,7 @@ struct drm_i915_gem_object { > unsigned int has_global_gtt_mapping:1; > > struct page **pages; > + int pages_pin_count; > > /** >* DMAR support > @@ -1327,6 +1328,17 @@ void i915_gem_release_mmap(struct drm_i915_gem_object > *obj); > void i915_gem_lastclose(struct drm_device *dev); > > int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj); > +static inline void i915_gem_object_pin_pages(struct drm_i915_gem_object *obj) > +{ > + BUG_ON(obj->pages == NULL); > + obj->pages_pin_count++; > +} > +static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object > *obj) > +{ > + BUG_ON(obj->pages_pin_count == 0); > + obj->pages_pin_count--; > +} > + Big fan of BUG_ON(!mutex_is_locked()) here. > int __must_check i915_mutex_lock_interruptible(struct drm_device *dev); > int i915_gem_object_sync(struct drm_i915_gem_object *obj, >struct intel_ring_buffer *to); > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 66fbd9f..aa088ef 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1699,6 +1699,9 @@ i915_gem_object_put_pages(struct drm_i915_gem_object > *obj) > > BUG_ON(obj->gtt_space); > > + if (obj->pages_pin_count) > + return -EBUSY; > + > ops->put_pages(obj); > > list_del(&obj->gtt_list); > @@ -1830,6 +1833,8 @@ i915_gem_object_get_pages(struct drm_i915_gem_object > *obj) > if (obj->sg_table || obj->pages) > return 0; > > + BUG_ON(obj->pages_pin_count); > + > ret = ops->get_pages(obj); > if (ret) > return ret; > @@ -3736,6 +3741,7 @@ void i915_gem_free_object(struct drm_gem_object > *gem_obj) > dev_priv->mm.interruptible = was_interruptible; > } > > + obj->pages_pin_count = 0; > i915_gem_object_put_pages(obj); > i915_gem_object_free_mmap_offset(obj); > > @@ -4395,9 +4401,10 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, > struct shrink_control *sc) > > cnt = 0; > list_for_each_entry(obj, &dev_priv->mm.unbound_list, gtt_list) > - cnt += obj->base.size >> PAGE_SHIFT; > + if (obj->pages_pin_count == 0) > + cnt += obj->base.size >> PAGE_SHIFT; > list_for_each_entry(obj, &dev_priv->mm.bound_list, gtt_list) > - if (obj->pin_count == 0) > + if (obj->pin_count == 0 && obj->pages_pin_count == 0) > cnt += obj->base.size >> PAGE_SHIFT; > > mutex_unlock(&dev->struct_mutex); > diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > index e4f1141..eca4726 100644 > --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > @@ -50,6 +50,8 @@ static struct sg_table *i915_gem_map_dma_buf(struct > dma_buf_attachment *attachme > /* link the pages into an SG then map the sg */ > sg = drm_prime_pages_to_sg(obj->pages, npages); > nents = dma_map_sg(attachment->dev, sg->sgl, sg->nents, dir); > + i915_gem_object_pin_pages(obj); > + > out: > mutex_unlock(&dev->struct_mutex); > return sg; > @@ -102,6 +104,7 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) > } > > obj->vmapping_count = 1; > + i915_gem_object_pin_pages(obj); > out_unlock: > mutex_unlock(&dev->struct_mutex); > return obj->dma_buf_vmapping; > @@ -117,10 +120,11 @@ static void i915_gem_dmabuf_vunmap(struct dma_buf > *dma_buf, void *vaddr) > if (ret) > return; > > - --obj->vmapping_count; > - if (obj->vmapping_count == 0) { > + if (--obj->vmapping_count == 0) { > vunmap(obj->dma_buf_vmapping); > obj->dma_buf_vmapping = NULL; > + > + i915_gem_object_unpin_pag
Re: [Intel-gfx] [PATCH 01/24] drm/i915: Introduce drm_i915_gem_object_ops
On Tue, 4 Sep 2012 21:02:53 +0100 Chris Wilson wrote: > In order to specialise functions depending upon the type of object, we > can attach vfuncs to each object via a new ->ops pointer. > > For instance, this will be used in future patches to only bind pages from > a dma-buf for the duration that the object is used by the GPU - and so > prevent them from pinning those pages for the entire of the object. > > Signed-off-by: Chris Wilson With comments below (addressed or not) this is: Reviewed-by: Ben Widawsky > --- > drivers/gpu/drm/i915/i915_drv.h| 12 +- > drivers/gpu/drm/i915/i915_gem.c| 71 > +--- > drivers/gpu/drm/i915/i915_gem_dmabuf.c |4 +- > 3 files changed, 60 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index f16ab5e..f180874 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -898,9 +898,16 @@ enum i915_cache_level { > I915_CACHE_LLC_MLC, /* gen6+, in docs at least! */ > }; > > +struct drm_i915_gem_object_ops { > + int (*get_pages)(struct drm_i915_gem_object *); > + void (*put_pages)(struct drm_i915_gem_object *); > +}; > + For the sake of "namespace" can we name this gem_get_pages() or something similar? I would love comments describing what get/put should do for when people other than you want to add new ops. If you'd prefer to put them in the definitions of the get/put functions, that's even better. > struct drm_i915_gem_object { > struct drm_gem_object base; > > + const struct drm_i915_gem_object_ops *ops; > + > /** Current space allocated to this object in the GTT, if any. */ > struct drm_mm_node *gtt_space; > struct list_head gtt_list; > @@ -1305,7 +1312,8 @@ int i915_gem_wait_ioctl(struct drm_device *dev, void > *data, > struct drm_file *file_priv); > void i915_gem_load(struct drm_device *dev); > int i915_gem_init_object(struct drm_gem_object *obj); > -void i915_gem_object_init(struct drm_i915_gem_object *obj); > +void i915_gem_object_init(struct drm_i915_gem_object *obj, > + const struct drm_i915_gem_object_ops *ops); > struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, > size_t size); > void i915_gem_free_object(struct drm_gem_object *obj); > @@ -1318,7 +1326,7 @@ int __must_check i915_gem_object_unbind(struct > drm_i915_gem_object *obj); > void i915_gem_release_mmap(struct drm_i915_gem_object *obj); > void i915_gem_lastclose(struct drm_device *dev); > > -int __must_check i915_gem_object_get_pages_gtt(struct drm_i915_gem_object > *obj); > +int __must_check i915_gem_object_get_pages(struct drm_i915_gem_object *obj); > int __must_check i915_mutex_lock_interruptible(struct drm_device *dev); > int i915_gem_object_sync(struct drm_i915_gem_object *obj, >struct intel_ring_buffer *to); > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 87a64e5..66fbd9f 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1650,18 +1650,12 @@ i915_gem_object_is_purgeable(struct > drm_i915_gem_object *obj) > return obj->madv == I915_MADV_DONTNEED; > } > > -static int > +static void > i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj) > { > int page_count = obj->base.size / PAGE_SIZE; > int ret, i; > > - BUG_ON(obj->gtt_space); > - > - if (obj->pages == NULL) > - return 0; > - > - BUG_ON(obj->gtt_space); > BUG_ON(obj->madv == __I915_MADV_PURGED); > > ret = i915_gem_object_set_to_cpu_domain(obj, true); > @@ -1693,9 +1687,21 @@ i915_gem_object_put_pages_gtt(struct > drm_i915_gem_object *obj) > > drm_free_large(obj->pages); > obj->pages = NULL; > +} > > - list_del(&obj->gtt_list); > +static int > +i915_gem_object_put_pages(struct drm_i915_gem_object *obj) > +{ > + const struct drm_i915_gem_object_ops *ops = obj->ops; > + > + if (obj->sg_table || obj->pages == NULL) > + return 0; > + > + BUG_ON(obj->gtt_space); > > + ops->put_pages(obj); > + > + list_del(&obj->gtt_list); > if (i915_gem_object_is_purgeable(obj)) > i915_gem_object_truncate(obj); > > @@ -1712,7 +1718,7 @@ i915_gem_purge(struct drm_i915_private *dev_priv, long > target) >&dev_priv->mm.unbound_list, >gtt_list) { > if (i915_gem_object_is_purgeable(obj) && > - i915_gem_object_put_pages_gtt(obj) == 0) { > + i915_gem_object_put_pages(obj) == 0) { > count += obj->base.size >> PAGE_SHIFT; > if (count >= target) > return count; > @@ -1724,7 +1730,7 @@ i915_gem_purge(struct drm_i915_p
Re: [Intel-gfx] [PATCH 6/7] drm/i915: Show render P state thresholds in sysfs
On Thu, 6 Sep 2012 13:54:09 -0700 Ben Widawsky wrote: > This is useful for userspace utilities which wish to use the previous > interface, specifically for micromanaging the increase/decrease steps by > setting min == max. > > CC: Jacob Pan > CC: Jesse Barnes > Signed-off-by: Ben Widawsky > --- > drivers/gpu/drm/i915/i915_sysfs.c | 36 > 1 file changed, 36 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c > b/drivers/gpu/drm/i915/i915_sysfs.c > index 5a5e610..e3a31ae 100644 > --- a/drivers/gpu/drm/i915/i915_sysfs.c > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > @@ -333,10 +333,46 @@ static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, > gt_cur_freq_mhz_show, NULL); > static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, > gt_max_freq_mhz_store); > static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, > gt_min_freq_mhz_store); > > + > +static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute > *attr, char *buf); > +static DEVICE_ATTR(gt_RP0_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL); > +static DEVICE_ATTR(gt_RP1_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL); > +static DEVICE_ATTR(gt_RPn_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL); > + > +/* For now we have a static number of RP states */ > +static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute > *attr, char *buf) > +{ > + struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev); > + struct drm_device *dev = minor->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + u32 val, rp_state_cap; > + ssize_t ret; > + > + ret = mutex_lock_interruptible(&dev->struct_mutex); > + if (ret) > + return ret; > + rp_state_cap = I915_READ(GEN6_RP_STATE_CAP); > + mutex_unlock(&dev->struct_mutex); > + > + if (attr == &dev_attr_gt_RP0_freq_mhz) { > + val = ((rp_state_cap & 0xff) >> 0) * > GT_FREQUENCY_MULTIPLIER; > + } else if (attr == &dev_attr_gt_RP1_freq_mhz) { > + val = ((rp_state_cap & 0x00ff00) >> 8) * > GT_FREQUENCY_MULTIPLIER; > + } else if (attr == &dev_attr_gt_RPn_freq_mhz) { > + val = ((rp_state_cap & 0xff) >> 16) * > GT_FREQUENCY_MULTIPLIER; > + } else { > + BUG(); > + } > + return snprintf(buf, PAGE_SIZE, "%d", val); > +} > + > static const struct attribute *gen6_attrs[] = { > &dev_attr_gt_cur_freq_mhz.attr, > &dev_attr_gt_max_freq_mhz.attr, > &dev_attr_gt_min_freq_mhz.attr, > + &dev_attr_gt_RP0_freq_mhz.attr, > + &dev_attr_gt_RP1_freq_mhz.attr, > + &dev_attr_gt_RPn_freq_mhz.attr, > NULL, > }; > If you add documentation: Reviewed-by: Jesse Barnes -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/7] drm/i915: Add setters for min/max frequency
On Thu, 6 Sep 2012 13:54:08 -0700 Ben Widawsky wrote: > Provide a standardized sysfs interface for setting min, and max > frequencies. The code which reads the limits were lifted from the > debugfs files. As a brief explanation, the limits are similar to the CPU > p-states. We have 3 states: > > RP0 - ie. max frequency > RP1 - ie. "local min" frequency I'd say "preferred min" or "vmin" here instead. > RPn - seriously lowest frequency > > Initially Daniel asked me to clamp the writes to supported values, but > in conforming to the way the cpufreq drivers seem to work, instead > return -EINVAL (noticed by Jesse in discussion). > > The values can be used by userspace wishing to control the limits of the > GPU (see the CC list for people who care). Can you add this and any other i915 bits that are missing to the sysfs documentation under Documentation/? Thanks for moving to -EINVAL, it makes a lot more sense to me. -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/7] drm/i915: #define gpu freq multipler
On Thu, 6 Sep 2012 13:54:06 -0700 Ben Widawsky wrote: > Magic numbers are bad mmmkay. In this case in particular the value is > especially weird because the docs say multiple things. We'll need this > value for sysfs, so extracting it is useful for that as well. Reviewed-by: Jesse Barnes -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 5/5] drm/i915: disable the cpu edp port after the cpu pipe
See bspec, Vol3 Part2, Section 1.1.3 "Display Mode Set Sequence". This applies to all platforms where we currently support eDP on, i.e. ilk, snb & ivb. Without this change we fail to light up the eDP port on previously unused crtcs (likely because something is stuck on the old pipe), and we also fail to properly disable the old pipe (i.e. bit 30 in the PIPECONF register is stuck as set until the next reboot). v2: Rebased on top of the edp panel off sequence changes in 3.6-rc2. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=44001 Signed-Off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_dp.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 07f9521..f28353d 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1315,15 +1315,20 @@ static void intel_disable_dp(struct intel_encoder *encoder) ironlake_edp_backlight_off(intel_dp); intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); ironlake_edp_panel_off(intel_dp); - intel_dp_link_down(intel_dp); + + /* cpu edp my only be disable _after_ the cpu pipe/plane is disabled. */ + if (!is_cpu_edp(intel_dp)) + intel_dp_link_down(intel_dp); } static void intel_post_disable_dp(struct intel_encoder *encoder) { struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); - if (is_cpu_edp(intel_dp)) + if (is_cpu_edp(intel_dp)) { + intel_dp_link_down(intel_dp); ironlake_edp_pll_off(intel_dp); + } } static void intel_enable_dp(struct intel_encoder *encoder) -- 1.7.11.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/5] drm/i915: rip out dp port enabling cludges^Wchecks
These have been added because dp links are fiddle things and don't like it when we try to re-train an enabled output (or disable a disabled output harder). And because the crtc helper code is ridiculously bad add tracking the modeset state. But with the new code in place it is simply a bug to disable a disabled encoder or to enable an enabled encoder again. Hence convert these to WARNs (and bail out for safety), but flatten all conditionals in the code itself. Signed-Off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_dp.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 7c746d1..07f9521 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1333,15 +1333,15 @@ static void intel_enable_dp(struct intel_encoder *encoder) struct drm_i915_private *dev_priv = dev->dev_private; uint32_t dp_reg = I915_READ(intel_dp->output_reg); + if (WARN_ON(dp_reg & DP_PORT_EN)) + return; + ironlake_edp_panel_vdd_on(intel_dp); intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); - if (!(dp_reg & DP_PORT_EN)) { - intel_dp_start_link_train(intel_dp); - ironlake_edp_panel_on(intel_dp); - ironlake_edp_panel_vdd_off(intel_dp, true); - intel_dp_complete_link_train(intel_dp); - } else - ironlake_edp_panel_vdd_off(intel_dp, false); + intel_dp_start_link_train(intel_dp); + ironlake_edp_panel_on(intel_dp); + ironlake_edp_panel_vdd_off(intel_dp, true); + intel_dp_complete_link_train(intel_dp); ironlake_edp_backlight_on(intel_dp); } @@ -1913,7 +1913,7 @@ intel_dp_link_down(struct intel_dp *intel_dp) struct drm_i915_private *dev_priv = dev->dev_private; uint32_t DP = intel_dp->DP; - if ((I915_READ(intel_dp->output_reg) & DP_PORT_EN) == 0) + if (WARN_ON((I915_READ(intel_dp->output_reg) & DP_PORT_EN) == 0)) return; DRM_DEBUG_KMS("\n"); -- 1.7.11.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/5] drm/i915: robustify edp_pll_on/off
With the previous patch to clean up where exactly these two functions are getting called, this patch can tackle the enable/disable code itself: - WARN if the port enable bit is in the wrong state or if the edp pll bit is in the wrong state, just for paranoia's sake. - Don't disable the edp pll harder in the modeset functions just for fun. - Don't set the edp pll enable flag in intel_dp->DP in modeset, do that while changing the actual hw state. We do the same with the actual port enable bit, so this is a bit more consistent. - Track the current DP register value when setting things up and add some comments how intel_dp->DP is used in the disable code. v2: Be more careful with resetting intel_dp->DP - otherwise dpms off->on will fail spectacularly, becuase we enable the eDP port when we should only enable the eDP pll. Signed-Off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_dp.c | 29 ++--- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index c72d4f3..7c746d1 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -887,7 +887,6 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode, intel_dp->DP |= intel_crtc->pipe << 29; /* don't miss out required setting for eDP */ - intel_dp->DP |= DP_PLL_ENABLE; if (adjusted_mode->clock < 20) intel_dp->DP |= DP_PLL_FREQ_160MHZ; else @@ -909,7 +908,6 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode, if (is_cpu_edp(intel_dp)) { /* don't miss out required setting for eDP */ - intel_dp->DP |= DP_PLL_ENABLE; if (adjusted_mode->clock < 20) intel_dp->DP |= DP_PLL_FREQ_160MHZ; else @@ -1192,8 +1190,15 @@ static void ironlake_edp_pll_on(struct intel_dp *intel_dp) DRM_DEBUG_KMS("\n"); dpa_ctl = I915_READ(DP_A); - dpa_ctl |= DP_PLL_ENABLE; - I915_WRITE(DP_A, dpa_ctl); + WARN(dpa_ctl & DP_PLL_ENABLE, "dp pll on, should be off\n"); + WARN(dpa_ctl & DP_PORT_EN, "dp port still on, should be off\n"); + + /* We don't adjust intel_dp->DP while tearing down the link, to +* facilitate link retraining (e.g. after hotplug). Hence clear all +* enable bits here to ensure that we don't enable too much. */ + intel_dp->DP &= ~(DP_PORT_EN | DP_AUDIO_OUTPUT_ENABLE); + intel_dp->DP |= DP_PLL_ENABLE; + I915_WRITE(DP_A, intel_dp->DP); POSTING_READ(DP_A); udelay(200); } @@ -1209,6 +1214,13 @@ static void ironlake_edp_pll_off(struct intel_dp *intel_dp) to_intel_crtc(crtc)->pipe); dpa_ctl = I915_READ(DP_A); + WARN((dpa_ctl & DP_PLL_ENABLE) == 0, +"dp pll off, should be on\n"); + WARN(dpa_ctl & DP_PORT_EN, "dp port still on, should be off\n"); + + /* We can't rely on the value tracked for the DP register in +* intel_dp->DP because link_down must not change that (otherwise link +* re-training will fail. */ dpa_ctl &= ~DP_PLL_ENABLE; I915_WRITE(DP_A, dpa_ctl); POSTING_READ(DP_A); @@ -1906,13 +1918,6 @@ intel_dp_link_down(struct intel_dp *intel_dp) DRM_DEBUG_KMS("\n"); - if (is_edp(intel_dp)) { - DP &= ~DP_PLL_ENABLE; - I915_WRITE(intel_dp->output_reg, DP); - POSTING_READ(intel_dp->output_reg); - udelay(100); - } - if (HAS_PCH_CPT(dev) && (IS_GEN7(dev) || !is_cpu_edp(intel_dp))) { DP &= ~DP_LINK_TRAIN_MASK_CPT; I915_WRITE(intel_dp->output_reg, DP | DP_LINK_TRAIN_PAT_IDLE_CPT); @@ -2456,6 +2461,8 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port) intel_dp->output_reg = output_reg; intel_dp->port = port; + /* Preserve the current hw state. */ + intel_dp->DP = I915_READ(intel_dp->output_reg); intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL); if (!intel_connector) { -- 1.7.11.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/5] drm/i915: clean up the cpu edp pll special case
By using the new pre_enabel/post_disable functions. To ensure that we only frob the cpu edp pll while the pipe is off add the relevant asserts. Thanks to the new output state staging, this is now really easy. Signed-Off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_dp.c | 74 +++-- 1 file changed, 27 insertions(+), 47 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index c59710d..c72d4f3 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -808,9 +808,6 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode, } } -static void ironlake_edp_pll_on(struct drm_encoder *encoder); -static void ironlake_edp_pll_off(struct drm_encoder *encoder); - static void intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode) @@ -821,14 +818,6 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode, struct drm_crtc *crtc = intel_dp->base.base.crtc; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - /* Turn on the eDP PLL if needed */ - if (is_edp(intel_dp)) { - if (!is_pch_edp(intel_dp)) - ironlake_edp_pll_on(encoder); - else - ironlake_edp_pll_off(encoder); - } - /* * There are four kinds of DP registers: * @@ -1191,12 +1180,16 @@ static void ironlake_edp_backlight_off(struct intel_dp *intel_dp) msleep(intel_dp->backlight_off_delay); } -static void ironlake_edp_pll_on(struct drm_encoder *encoder) +static void ironlake_edp_pll_on(struct intel_dp *intel_dp) { - struct drm_device *dev = encoder->dev; + struct drm_device *dev = intel_dp->base.base.dev; + struct drm_crtc *crtc = intel_dp->base.base.crtc; struct drm_i915_private *dev_priv = dev->dev_private; u32 dpa_ctl; + assert_pipe_disabled(dev_priv, +to_intel_crtc(crtc)->pipe); + DRM_DEBUG_KMS("\n"); dpa_ctl = I915_READ(DP_A); dpa_ctl |= DP_PLL_ENABLE; @@ -1205,12 +1198,16 @@ static void ironlake_edp_pll_on(struct drm_encoder *encoder) udelay(200); } -static void ironlake_edp_pll_off(struct drm_encoder *encoder) +static void ironlake_edp_pll_off(struct intel_dp *intel_dp) { - struct drm_device *dev = encoder->dev; + struct drm_device *dev = intel_dp->base.base.dev; + struct drm_crtc *crtc = intel_dp->base.base.crtc; struct drm_i915_private *dev_priv = dev->dev_private; u32 dpa_ctl; + assert_pipe_disabled(dev_priv, +to_intel_crtc(crtc)->pipe); + dpa_ctl = I915_READ(DP_A); dpa_ctl &= ~DP_PLL_ENABLE; I915_WRITE(DP_A, dpa_ctl); @@ -1309,6 +1306,14 @@ static void intel_disable_dp(struct intel_encoder *encoder) intel_dp_link_down(intel_dp); } +static void intel_post_disable_dp(struct intel_encoder *encoder) +{ + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); + + if (is_cpu_edp(intel_dp)) + ironlake_edp_pll_off(intel_dp); +} + static void intel_enable_dp(struct intel_encoder *encoder) { struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); @@ -1328,39 +1333,12 @@ static void intel_enable_dp(struct intel_encoder *encoder) ironlake_edp_backlight_on(intel_dp); } -static void -intel_dp_dpms(struct drm_connector *connector, int mode) +static void intel_pre_enable_dp(struct intel_encoder *encoder) { - struct intel_dp *intel_dp = intel_attached_dp(connector); - - /* DP supports only 2 dpms states. */ - if (mode != DRM_MODE_DPMS_ON) - mode = DRM_MODE_DPMS_OFF; - - if (mode == connector->dpms) - return; - - connector->dpms = mode; - - /* Only need to change hw state when actually enabled */ - if (!intel_dp->base.base.crtc) { - intel_dp->base.connectors_active = false; - return; - } - - if (mode != DRM_MODE_DPMS_ON) { - intel_encoder_dpms(&intel_dp->base, mode); - - if (is_cpu_edp(intel_dp)) - ironlake_edp_pll_off(&intel_dp->base.base); - } else { - if (is_cpu_edp(intel_dp)) - ironlake_edp_pll_on(&intel_dp->base.base); - - intel_encoder_dpms(&intel_dp->base, mode); - } + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); - intel_modeset_check_state(connector->dev); + if (is_cpu_edp(intel_dp)) + ironlake_edp_pll_on(intel_dp); } /* @@ -2391,7 +2369,7 @@ static const struct drm_encoder_helper_funcs intel_dp_helper_funcs = { }; static const struct drm_connector_funcs intel_dp_connector_funcs = { - .dpms = intel_dp_dpms, + .dpms = intel_co
[Intel-gfx] [PATCH 1/5] drm/i915: add encoder->pre_enable/post_disable
The cpu eDP encoder has some horrible hacks to set up the DP pll at the right time. To be able to move them to the right place, add some more encoder callbacks so that this can happen at the right time. LVDS has some similar funky hacks, but that would require more work (we need to move around the pll setup a bit). Hence for now only wire these new callbacks up for ilk+ - we only have cpu eDP on these platforms. Signed-Off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_display.c | 8 drivers/gpu/drm/i915/intel_drv.h | 2 ++ 2 files changed, 10 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index bff0936..1d31364 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3189,6 +3189,10 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) assert_fdi_rx_disabled(dev_priv, pipe); } + for_each_encoder_on_crtc(dev, crtc, encoder) + if (encoder->pre_enable) + encoder->pre_enable(encoder); + /* Enable panel fitting for LVDS */ if (dev_priv->pch_pf_size && (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) || HAS_eDP)) { @@ -3258,6 +3262,10 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) I915_WRITE(PF_CTL(pipe), 0); I915_WRITE(PF_WIN_SZ(pipe), 0); + for_each_encoder_on_crtc(dev, crtc, encoder) + if (encoder->post_disable) + encoder->post_disable(encoder); + ironlake_fdi_disable(crtc); intel_disable_transcoder(dev_priv, pipe); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 4f2b2d6..1306f05 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -153,7 +153,9 @@ struct intel_encoder { bool connectors_active; void (*hot_plug)(struct intel_encoder *); void (*enable)(struct intel_encoder *); + void (*pre_enable)(struct intel_encoder *); void (*disable)(struct intel_encoder *); + void (*post_disable)(struct intel_encoder *); /* Read out the current hw state of this connector, returning true if * the encoder is active. If the encoder is enabled it also set the pipe * it is connected to in the pipe parameter. */ -- 1.7.11.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/5] cpu edp fixes
Hi all! Finally. Now that the modeset-rework is merge I can finally dump the cpu edp fixes and cleanups. Avoids my ivb edp from getting angry and leaving random pipes stuck in the active state and the panel in the passive state. Again, has been part of the modeset-rework branch since a longer time, so has seen tons of testing. Flames, comments & review highly welcome. Cheers, Daniel Daniel Vetter (5): drm/i915: add encoder->pre_enable/post_disable drm/i915: clean up the cpu edp pll special case drm/i915: robustify edp_pll_on/off drm/i915: rip out dp port enabling cludges^Wchecks drm/i915: disable the cpu edp port after the cpu pipe drivers/gpu/drm/i915/intel_display.c | 8 +++ drivers/gpu/drm/i915/intel_dp.c | 126 --- drivers/gpu/drm/i915/intel_drv.h | 2 + 3 files changed, 69 insertions(+), 67 deletions(-) -- 1.7.11.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: fixup the plane->pipe fixup code
We need to check whether the _other plane is on our pipe, not whether our plane is on the other pipe. Otherwise if not both pipes/planes are active, we won't properly clean up the mess and set up our desired plane->pipe mapping. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=51265 Tested-by: Vladyslav Shtabovenko Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_display.c | 28 ++-- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index fd9c275..ce9b247 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7994,11 +7994,27 @@ static void intel_enable_pipe_a(struct drm_device *dev) } +static bool +intel_check_plane_mapping(struct intel_crtc *crtc) +{ + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; + u32 reg, val; + + reg = DSPCNTR(!crtc->plane); + val = I915_READ(reg); + + if ((val & DISPLAY_PLANE_ENABLE) == 0 && + (!!(val & DISPPLANE_SEL_PIPE_MASK) == crtc->pipe)) + return false; + + return true; +} + static void intel_sanitize_crtc(struct intel_crtc *crtc) { struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - u32 reg, val; + u32 reg; /* Clear any frame start delays used for debugging left by the BIOS */ reg = PIPECONF(crtc->pipe); @@ -8006,17 +8022,10 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) /* We need to sanitize the plane -> pipe mapping first because this will * disable the crtc (and hence change the state) if it is wrong. */ - if (!HAS_PCH_SPLIT(dev)) { + if (!HAS_PCH_SPLIT(dev) && !intel_check_plane_mapping(crtc)) { struct intel_connector *connector; bool plane; - reg = DSPCNTR(crtc->plane); - val = I915_READ(reg); - - if ((val & DISPLAY_PLANE_ENABLE) == 0 && - (!!(val & DISPPLANE_SEL_PIPE_MASK) == crtc->pipe)) - goto ok; - DRM_DEBUG_KMS("[CRTC:%d] wrong plane connection detected!\n", crtc->base.base.id); @@ -8040,7 +8049,6 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) WARN_ON(crtc->active); crtc->base.enabled = false; } -ok: if (dev_priv->quirks & QUIRK_PIPEA_FORCE && crtc->pipe == PIPE_A && !crtc->active) { -- 1.7.11.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/4] drm/i915: update dpms property in set_mode
Hopefully this makes userspace slightly less confused about us frobbing the dpms state behind its back. Yeah, it would be better to be more careful with not changing the dpms state, but that is quite more invasive. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_display.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 805324d..bff0936 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6806,7 +6806,13 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes) intel_crtc = to_intel_crtc(connector->encoder->crtc); if (prepare_pipes & (1 << intel_crtc->pipe)) { + struct drm_property *dpms_property = + dev->mode_config.dpms_property; + connector->dpms = DRM_MODE_DPMS_ON; + drm_connector_property_set_value(connector, +dpms_property, +DRM_MODE_DPMS_ON); intel_encoder = to_intel_encoder(connector->encoder); intel_encoder->connectors_active = true; -- 1.7.11.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/4] drm/i915: don't call dpms funcs after set_mode
... because our current set_mode implementation doesn't bother to adjust for the dpms state, we just forcefully update it. So stop pretending that we're better than we are and rip out this extranous call. Note that this totally confuses userspace, because the exposed connector property isn't actually updated ... Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_display.c | 10 -- 1 file changed, 10 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 0973797..805324d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7236,7 +7236,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set) struct drm_mode_set save_set; struct intel_set_config *config; int ret; - int i; BUG_ON(!set); BUG_ON(!set->crtc); @@ -7300,15 +7299,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set) ret = -EINVAL; goto fail; } - - if (set->crtc->enabled) { - DRM_DEBUG_KMS("Setting connector DPMS state to on\n"); - for (i = 0; i < set->num_connectors; i++) { - DRM_DEBUG_KMS("\t[CONNECTOR:%d:%s] set DPMS on\n", set->connectors[i]->base.id, - drm_get_connector_name(set->connectors[i])); - set->connectors[i]->funcs->dpms(set->connectors[i], DRM_MODE_DPMS_ON); - } - } } else if (config->fb_changed) { ret = intel_pipe_set_base(set->crtc, set->x, set->y, set->fb); -- 1.7.11.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/4] drm/i915: don't disable fdi links harder in ilk_crtc_enable
Because they should have been disabled when shutting down the display pipe previously. To ensure that this is the case, add a few assserts instead of unconditionally disabling the fdi link. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_display.c | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 6c06109..0973797 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3182,10 +3182,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) is_pch_port = intel_crtc_driving_pch(crtc); - if (is_pch_port) + if (is_pch_port) { ironlake_fdi_pll_enable(intel_crtc); - else - ironlake_fdi_disable(crtc); + } else { + assert_fdi_tx_disabled(dev_priv, pipe); + assert_fdi_rx_disabled(dev_priv, pipe); + } /* Enable panel fitting for LVDS */ if (dev_priv->pch_pf_size && -- 1.7.11.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm/i915: rip out intel_disable_pch_ports
Even with the old crtc helper code we should have disabled all encoders on that pipe by now, and with the new code this would definitely paper over a bug. We already have the necessary checks in place in intel_disable_transcoder, so if we accidentally leave a pch port on, this will be caught. Hence just rip this all out. Note that up to the patch in this giant modeset series that removes the LVDS special case to avoid disabling LVDS in the encoder->prepare callback ("drm/i915/lvds: ditch ->prepare special case"), this was not the case for all outputs. Also note that in commit 1b3c7a47f993bf9ab6c4c7cc3bbf5588052b58f4 Author: Zhenyu Wang Date: Wed Nov 25 13:09:38 2009 +0800 drm/i915: Fix LVDS stability issue on Ironlake this was already discovered independently and worked around. How I bloody hate this entire mess of cludges piled on top of other cludges. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_display.c | 60 1 file changed, 60 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e061acd..6c06109 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1860,59 +1860,6 @@ static void intel_disable_plane(struct drm_i915_private *dev_priv, intel_wait_for_vblank(dev_priv->dev, pipe); } -static void disable_pch_dp(struct drm_i915_private *dev_priv, - enum pipe pipe, int reg, u32 port_sel) -{ - u32 val = I915_READ(reg); - if (dp_pipe_enabled(dev_priv, pipe, port_sel, val)) { - DRM_DEBUG_KMS("Disabling pch dp %x on pipe %d\n", reg, pipe); - I915_WRITE(reg, val & ~DP_PORT_EN); - } -} - -static void disable_pch_hdmi(struct drm_i915_private *dev_priv, -enum pipe pipe, int reg) -{ - u32 val = I915_READ(reg); - if (hdmi_pipe_enabled(dev_priv, pipe, val)) { - DRM_DEBUG_KMS("Disabling pch HDMI %x on pipe %d\n", - reg, pipe); - I915_WRITE(reg, val & ~PORT_ENABLE); - } -} - -/* Disable any ports connected to this transcoder */ -static void intel_disable_pch_ports(struct drm_i915_private *dev_priv, - enum pipe pipe) -{ - u32 reg, val; - - val = I915_READ(PCH_PP_CONTROL); - I915_WRITE(PCH_PP_CONTROL, val | PANEL_UNLOCK_REGS); - - disable_pch_dp(dev_priv, pipe, PCH_DP_B, TRANS_DP_PORT_SEL_B); - disable_pch_dp(dev_priv, pipe, PCH_DP_C, TRANS_DP_PORT_SEL_C); - disable_pch_dp(dev_priv, pipe, PCH_DP_D, TRANS_DP_PORT_SEL_D); - - reg = PCH_ADPA; - val = I915_READ(reg); - if (adpa_pipe_enabled(dev_priv, pipe, val)) - I915_WRITE(reg, val & ~ADPA_DAC_ENABLE); - - reg = PCH_LVDS; - val = I915_READ(reg); - if (lvds_pipe_enabled(dev_priv, pipe, val)) { - DRM_DEBUG_KMS("disable lvds on pipe %d val 0x%08x\n", pipe, val); - I915_WRITE(reg, val & ~LVDS_PORT_EN); - POSTING_READ(reg); - udelay(100); - } - - disable_pch_hdmi(dev_priv, pipe, HDMIB); - disable_pch_hdmi(dev_priv, pipe, HDMIC); - disable_pch_hdmi(dev_priv, pipe, HDMID); -} - int intel_pin_and_fence_fb_obj(struct drm_device *dev, struct drm_i915_gem_object *obj, @@ -3311,13 +3258,6 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) ironlake_fdi_disable(crtc); - /* This is a horrible layering violation; we should be doing this in -* the connector/encoder ->prepare instead, but we don't always have -* enough information there about the config to know whether it will -* actually be necessary or just cause undesired flicker. -*/ - intel_disable_pch_ports(dev_priv, pipe); - intel_disable_transcoder(dev_priv, pipe); if (HAS_PCH_CPT(dev)) { -- 1.7.11.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/4] some follow-up fixlets from the modeset rework
Hi all, Just a few small follow-ups - 2 patches to make pch/fdi handling more strict and add more asserts to check that. - 2 patches to make dpms handling around modesets more strict. All four have been included in the modeset-rework branch since a long time and hence seen tons of testing already. But I've figured they're not part of the core rework, hence I've split them out. Flames, comments & review highly welcome. Cheers, Daniel Daniel Vetter (4): drm/i915: rip out intel_disable_pch_ports drm/i915: don't disable fdi links harder in ilk_crtc_enable drm/i915: don't call dpms funcs after set_mode drm/i915: update dpms property in set_mode drivers/gpu/drm/i915/intel_display.c | 84 +--- 1 file changed, 11 insertions(+), 73 deletions(-) -- 1.7.11.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/58] modeset-rework, the basic conversion
On Sun, Aug 19, 2012 at 09:12:17PM +0200, Daniel Vetter wrote: > Hi all, > > Changes since last time around: > - The prep patches are all merged now. > - I've left out the actual DP fixes/cleanups, I think we should merge those > in a > separte step. > - A few bugfixes (thanks to Paulo, Jani and Chris). > - I've also applied a few bikesheds for naming that Paulo suggested (but I'm > not > sure whether I've sent those out already in a previous patchbomb). > > Essentially this is just the core rework, which addes the new get_hw_state > code, > refactors all the encoders to use the new functions and finally reworks the > modeset logic to disable/enable entire pipes, always (and with a deterministic > order). > > For merging to -next, I plan to pull in everything with a real merge commit. > For > that reason I've put up a modeset-rework-base branch onto my private fdo > repo[1]. > That way I can put a short documentation for the new modeset design into the > merge commit (stichted together from the previous patchbomb cover letters), > documenting my folly assumptions for eternity. > > I'll also plan to put tags for the entire series in the merge commit, so if > you > have tested this on a few machines, read through and agree with the new > designs, > please reply with your tested-by/acked-by/reviewed-by tags. > > Flames, comments and test reports highly welcome. Ok, I've just merged the entire pile. Thanks everyone for testing feedback, comments and review. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 7/7] drm/i915: expose energy counter on SNB and IVB
From: Jesse Barnes On SNB and IVB, there's an MSR (also exposed through MCHBAR) we can use to read out the amount of energy used over time. Expose this in sysfs to make it easy to do power comparisons with different configurations. If the platform supports it, the file will show up under the drm/card0/power subdirectory of the PCI device in sysfs as gt_energy_uJ. The value in the file is a running total of energy (in microjoules) consumed by the graphics device. v2: move to sysfs (Ben, Daniel) expose a simple value (Chris) drop unrelated hunk (Ben) Signed-off-by: Jesse Barnes v3: by Ben Tied it into existing rc6 sysfs entries and named that a more generic "power attrs." Fixed rebase conflicts. Signed-off-by: Ben Widawsky --- drivers/gpu/drm/i915/i915_reg.h | 2 ++ drivers/gpu/drm/i915/i915_sysfs.c | 25 +++-- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index d0b60f2..0e34f5f 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1242,6 +1242,8 @@ #define CLKCFG_MEM_800 (3 << 4) #define CLKCFG_MEM_MASK(7 << 4) +#define SECP_NRG_STTS (MCHBAR_MIRROR_BASE_SNB + 0x592c) + #define TSC1 0x11001 #define TSE (1<<0) #define TR10x11006 diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index e3a31ae..6bbc9af 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -76,22 +76,43 @@ show_rc6pp_ms(struct device *kdev, struct device_attribute *attr, char *buf) return snprintf(buf, PAGE_SIZE, "%u", rc6pp_residency); } +#define MSR_IA32_PACKAGE_POWER_SKU_UNIT0x0606 + +static ssize_t +show_gt_energy_uJ(struct device *dev, struct device_attribute *attr, char *buf) +{ + struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev); + struct drm_i915_private *dev_priv = dminor->dev->dev_private; + u64 ppsu; + u32 val, units; + + rdmsrl(MSR_IA32_PACKAGE_POWER_SKU_UNIT, ppsu); + + ppsu = (ppsu & 0x1f00) >> 8; + units = 100 / (1 << ppsu); /* convert to uJ */ + val = I915_READ(SECP_NRG_STTS); + + return snprintf(buf, PAGE_SIZE, "%u", val * units); +} + static DEVICE_ATTR(rc6_enable, S_IRUGO, show_rc6_mask, NULL); static DEVICE_ATTR(rc6_residency_ms, S_IRUGO, show_rc6_ms, NULL); static DEVICE_ATTR(rc6p_residency_ms, S_IRUGO, show_rc6p_ms, NULL); static DEVICE_ATTR(rc6pp_residency_ms, S_IRUGO, show_rc6pp_ms, NULL); +static DEVICE_ATTR(gt_energy_uJ, S_IRUGO, show_gt_energy_uJ, NULL); -static struct attribute *rc6_attrs[] = { +static struct attribute *power_attrs[] = { &dev_attr_rc6_enable.attr, &dev_attr_rc6_residency_ms.attr, &dev_attr_rc6p_residency_ms.attr, &dev_attr_rc6pp_residency_ms.attr, + &dev_attr_gt_energy_uJ.attr, NULL }; static struct attribute_group rc6_attr_group = { .name = power_group_name, - .attrs = rc6_attrs + .attrs = power_attrs }; #endif -- 1.7.12 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/7] drm/i915: Show render P state thresholds in sysfs
This is useful for userspace utilities which wish to use the previous interface, specifically for micromanaging the increase/decrease steps by setting min == max. CC: Jacob Pan CC: Jesse Barnes Signed-off-by: Ben Widawsky --- drivers/gpu/drm/i915/i915_sysfs.c | 36 1 file changed, 36 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 5a5e610..e3a31ae 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -333,10 +333,46 @@ static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show, NULL); static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, gt_max_freq_mhz_store); static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, gt_min_freq_mhz_store); + +static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf); +static DEVICE_ATTR(gt_RP0_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL); +static DEVICE_ATTR(gt_RP1_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL); +static DEVICE_ATTR(gt_RPn_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL); + +/* For now we have a static number of RP states */ +static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) +{ + struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev); + struct drm_device *dev = minor->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + u32 val, rp_state_cap; + ssize_t ret; + + ret = mutex_lock_interruptible(&dev->struct_mutex); + if (ret) + return ret; + rp_state_cap = I915_READ(GEN6_RP_STATE_CAP); + mutex_unlock(&dev->struct_mutex); + + if (attr == &dev_attr_gt_RP0_freq_mhz) { + val = ((rp_state_cap & 0xff) >> 0) * GT_FREQUENCY_MULTIPLIER; + } else if (attr == &dev_attr_gt_RP1_freq_mhz) { + val = ((rp_state_cap & 0x00ff00) >> 8) * GT_FREQUENCY_MULTIPLIER; + } else if (attr == &dev_attr_gt_RPn_freq_mhz) { + val = ((rp_state_cap & 0xff) >> 16) * GT_FREQUENCY_MULTIPLIER; + } else { + BUG(); + } + return snprintf(buf, PAGE_SIZE, "%d", val); +} + static const struct attribute *gen6_attrs[] = { &dev_attr_gt_cur_freq_mhz.attr, &dev_attr_gt_max_freq_mhz.attr, &dev_attr_gt_min_freq_mhz.attr, + &dev_attr_gt_RP0_freq_mhz.attr, + &dev_attr_gt_RP1_freq_mhz.attr, + &dev_attr_gt_RPn_freq_mhz.attr, NULL, }; -- 1.7.12 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 5/7] drm/i915: Add setters for min/max frequency
Provide a standardized sysfs interface for setting min, and max frequencies. The code which reads the limits were lifted from the debugfs files. As a brief explanation, the limits are similar to the CPU p-states. We have 3 states: RP0 - ie. max frequency RP1 - ie. "local min" frequency RPn - seriously lowest frequency Initially Daniel asked me to clamp the writes to supported values, but in conforming to the way the cpufreq drivers seem to work, instead return -EINVAL (noticed by Jesse in discussion). The values can be used by userspace wishing to control the limits of the GPU (see the CC list for people who care). CC: Arjan van de Ven CC: Jacob Pan CC: Daniel Vetter CC: Jesse Barnes Signed-off-by: Ben Widawsky --- drivers/gpu/drm/i915/i915_sysfs.c | 78 ++- 1 file changed, 76 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 6c7f905..5a5e610 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -238,6 +238,43 @@ static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute return snprintf(buf, PAGE_SIZE, "%d", ret); } +static ssize_t gt_max_freq_mhz_store(struct device *kdev, +struct device_attribute *attr, +const char *buf, size_t count) +{ + struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev); + struct drm_device *dev = minor->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + u32 val, rp_state_cap, hw_max; + ssize_t ret; + + ret = kstrtou32(buf, 0, &val); + if (ret) + return ret; + + ret = mutex_lock_interruptible(&dev->struct_mutex); + if (ret) + return ret; + + rp_state_cap = I915_READ(GEN6_RP_STATE_CAP); + hw_max = (rp_state_cap & 0xff) * GT_FREQUENCY_MULTIPLIER; + + if (val > hw_max) + return -EINVAL; + + if (val < dev_priv->rps.min_delay) + val = dev_priv->rps.min_delay; + + if (dev_priv->rps.cur_delay > val) + gen6_set_rps(dev_priv->dev, val); + + dev_priv->rps.max_delay = val / GT_FREQUENCY_MULTIPLIER; + + mutex_unlock(&dev->struct_mutex); + + return count; +} + static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) { struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev); @@ -255,9 +292,46 @@ static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute return snprintf(buf, PAGE_SIZE, "%d", ret); } +static ssize_t gt_min_freq_mhz_store(struct device *kdev, +struct device_attribute *attr, +const char *buf, size_t count) +{ + struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev); + struct drm_device *dev = minor->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + u32 val, rp_state_cap, hw_min; + ssize_t ret; + + ret = kstrtou32(buf, 0, &val); + if (ret) + return ret; + + ret = mutex_lock_interruptible(&dev->struct_mutex); + if (ret) + return ret; + + rp_state_cap = I915_READ(GEN6_RP_STATE_CAP); + hw_min = ((rp_state_cap & 0xff) >> 16) * GT_FREQUENCY_MULTIPLIER; + + if (val < hw_min) + return -EINVAL; + + if (val > dev_priv->rps.max_delay) + val = dev_priv->rps.max_delay; + + if (dev_priv->rps.cur_delay < val) + gen6_set_rps(dev_priv->dev, val); + + dev_priv->rps.min_delay = val / GT_FREQUENCY_MULTIPLIER; + + mutex_unlock(&dev->struct_mutex); + + return count; +} + static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show, NULL); -static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, NULL); -static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, NULL); +static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, gt_max_freq_mhz_store); +static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, gt_min_freq_mhz_store); static const struct attribute *gen6_attrs[] = { &dev_attr_gt_cur_freq_mhz.attr, -- 1.7.12 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/7 v2] drm/i915: Add current GPU freq to sysfs
Userspace applications such as PowerTOP are interesting in being able to read the current GPU frequency. The patch itself sets up a generic array for gen6 attributes so we can easily add other items in the future (and it also happens to be just about the cleanest way to do this). The patch is a nice addition to commit 1ac02185dff3afac146d745ba220dc6672d1d162 Author: Daniel Vetter Date: Thu Aug 30 13:26:48 2012 +0200 drm/i915: add a tracepoint for gpu frequency changes Reading the GPU frequncy can be done by reading a file like: /sys/class/drm/card0/render_frequency_mhz v2: rename the sysfs file to gt_cur_freq_mhz (Daniel) Use kdev naming (Chris) Use DEVICE_ATTR over __ATTR (Ben) Add min/max freq (Ben/Daniel) user the new #define for frequency multiplier CC: Arjan van de Ven CC: Jacob Pan Signed-off-by: Ben Widawsky --- drivers/gpu/drm/i915/i915_sysfs.c | 71 +++ 1 file changed, 71 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index c701a17..6c7f905 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -203,6 +203,70 @@ static struct bin_attribute dpf_attrs = { .mmap = NULL }; +static ssize_t gt_cur_freq_mhz_show(struct device *kdev, + struct device_attribute *attr, char *buf) +{ + struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev); + struct drm_device *dev = minor->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + int ret; + + ret = i915_mutex_lock_interruptible(dev); + if (ret) + return ret; + + ret = dev_priv->rps.cur_delay * GT_FREQUENCY_MULTIPLIER; + mutex_unlock(&dev->struct_mutex); + + return snprintf(buf, PAGE_SIZE, "%d", ret); +} + +static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) +{ + struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev); + struct drm_device *dev = minor->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + int ret; + + ret = i915_mutex_lock_interruptible(dev); + if (ret) + return ret; + + ret = dev_priv->rps.max_delay * GT_FREQUENCY_MULTIPLIER; + mutex_unlock(&dev->struct_mutex); + + return snprintf(buf, PAGE_SIZE, "%d", ret); +} + +static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) +{ + struct drm_minor *minor = container_of(kdev, struct drm_minor, kdev); + struct drm_device *dev = minor->dev; + struct drm_i915_private *dev_priv = dev->dev_private; + int ret; + + ret = i915_mutex_lock_interruptible(dev); + if (ret) + return ret; + + ret = dev_priv->rps.min_delay * GT_FREQUENCY_MULTIPLIER; + mutex_unlock(&dev->struct_mutex); + + return snprintf(buf, PAGE_SIZE, "%d", ret); +} + +static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show, NULL); +static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR, gt_max_freq_mhz_show, NULL); +static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR, gt_min_freq_mhz_show, NULL); + +static const struct attribute *gen6_attrs[] = { + &dev_attr_gt_cur_freq_mhz.attr, + &dev_attr_gt_max_freq_mhz.attr, + &dev_attr_gt_min_freq_mhz.attr, + NULL, +}; + + void i915_setup_sysfs(struct drm_device *dev) { int ret; @@ -220,10 +284,17 @@ void i915_setup_sysfs(struct drm_device *dev) if (ret) DRM_ERROR("l3 parity sysfs setup failed\n"); } + + if (INTEL_INFO(dev)->gen >= 6) { + ret = sysfs_create_files(&dev->primary->kdev.kobj, gen6_attrs); + if (ret) + DRM_ERROR("gen6 sysfs setup failed\n"); + } } void i915_teardown_sysfs(struct drm_device *dev) { + sysfs_remove_files(&dev->primary->kdev.kobj, gen6_attrs); device_remove_bin_file(&dev->primary->kdev, &dpf_attrs); sysfs_unmerge_group(&dev->primary->kdev.kobj, &rc6_attr_group); } -- 1.7.12 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/7] drm/i915: #define gpu freq multipler
Magic numbers are bad mmmkay. In this case in particular the value is especially weird because the docs say multiple things. We'll need this value for sysfs, so extracting it is useful for that as well. CC: Jesse Barnes Signed-off-by: Ben Widawsky --- drivers/gpu/drm/i915/i915_debugfs.c | 22 +++--- drivers/gpu/drm/i915/i915_drv.h | 2 ++ 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 3d886af..73d1f98 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -930,7 +930,7 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused) seq_printf(m, "Render p-state limit: %d\n", rp_state_limits & 0xff); seq_printf(m, "CAGF: %dMHz\n", ((rpstat & GEN6_CAGF_MASK) >> - GEN6_CAGF_SHIFT) * 50); + GEN6_CAGF_SHIFT) * GT_FREQUENCY_MULTIPLIER); seq_printf(m, "RP CUR UP EI: %dus\n", rpupei & GEN6_CURICONT_MASK); seq_printf(m, "RP CUR UP: %dus\n", rpcurup & @@ -946,15 +946,15 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused) max_freq = (rp_state_cap & 0xff) >> 16; seq_printf(m, "Lowest (RPN) frequency: %dMHz\n", - max_freq * 50); + max_freq * GT_FREQUENCY_MULTIPLIER); max_freq = (rp_state_cap & 0xff00) >> 8; seq_printf(m, "Nominal (RP1) frequency: %dMHz\n", - max_freq * 50); + max_freq * GT_FREQUENCY_MULTIPLIER); max_freq = rp_state_cap & 0xff; seq_printf(m, "Max non-overclocked (RP0) frequency: %dMHz\n", - max_freq * 50); + max_freq * GT_FREQUENCY_MULTIPLIER); } else { seq_printf(m, "no P-state info available\n"); } @@ -1308,7 +1308,7 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused) continue; } ia_freq = I915_READ(GEN6_PCODE_DATA); - seq_printf(m, "%d\t\t%d\n", gpu_freq * 50, ia_freq * 100); + seq_printf(m, "%d\t\t%d\n", gpu_freq * GT_FREQUENCY_MULTIPLIER, ia_freq * 100); } mutex_unlock(&dev->struct_mutex); @@ -1735,7 +1735,7 @@ i915_max_freq_read(struct file *filp, return ret; len = snprintf(buf, sizeof(buf), - "max freq: %d\n", dev_priv->rps.max_delay * 50); + "max freq: %d\n", dev_priv->rps.max_delay * GT_FREQUENCY_MULTIPLIER); mutex_unlock(&dev->struct_mutex); if (len > sizeof(buf)) @@ -1778,9 +1778,9 @@ i915_max_freq_write(struct file *filp, /* * Turbo will still be enabled, but won't go above the set value. */ - dev_priv->rps.max_delay = val / 50; + dev_priv->rps.max_delay = val / GT_FREQUENCY_MULTIPLIER; - gen6_set_rps(dev, val / 50); + gen6_set_rps(dev, val / GT_FREQUENCY_MULTIPLIER); mutex_unlock(&dev->struct_mutex); return cnt; @@ -1811,7 +1811,7 @@ i915_min_freq_read(struct file *filp, char __user *ubuf, size_t max, return ret; len = snprintf(buf, sizeof(buf), - "min freq: %d\n", dev_priv->rps.min_delay * 50); + "min freq: %d\n", dev_priv->rps.min_delay * GT_FREQUENCY_MULTIPLIER); mutex_unlock(&dev->struct_mutex); if (len > sizeof(buf)) @@ -1852,9 +1852,9 @@ i915_min_freq_write(struct file *filp, const char __user *ubuf, size_t cnt, /* * Turbo will still be enabled, but won't go below the set value. */ - dev_priv->rps.min_delay = val / 50; + dev_priv->rps.min_delay = val / GT_FREQUENCY_MULTIPLIER; - gen6_set_rps(dev, val / 50); + gen6_set_rps(dev, val / GT_FREQUENCY_MULTIPLIER); mutex_unlock(&dev->struct_mutex); return cnt; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index f16ab5e..51af543 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1162,6 +1162,8 @@ struct drm_i915_file_private { #define HAS_L3_GPU_CACHE(dev) (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) +#define GT_FREQUENCY_MULTIPLIER 50 + #include "i915_trace.h" /** -- 1.7.12 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/7] drm/i915: variable renames
From: Ben Widawsky Name variables a bit better for copy-pasters. This got turned up as part of review for upcoming sysfs patches. CC: Chris Wilson Signed-off-by: Ben Widawsky --- drivers/gpu/drm/i915/i915_sysfs.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index da733a3..c701a17 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -46,32 +46,32 @@ static u32 calc_residency(struct drm_device *dev, const u32 reg) } static ssize_t -show_rc6_mask(struct device *dev, struct device_attribute *attr, char *buf) +show_rc6_mask(struct device *kdev, struct device_attribute *attr, char *buf) { - struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev); + struct drm_minor *dminor = container_of(kdev, struct drm_minor, kdev); return snprintf(buf, PAGE_SIZE, "%x", intel_enable_rc6(dminor->dev)); } static ssize_t -show_rc6_ms(struct device *dev, struct device_attribute *attr, char *buf) +show_rc6_ms(struct device *kdev, struct device_attribute *attr, char *buf) { - struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev); + struct drm_minor *dminor = container_of(kdev, struct drm_minor, kdev); u32 rc6_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6); return snprintf(buf, PAGE_SIZE, "%u", rc6_residency); } static ssize_t -show_rc6p_ms(struct device *dev, struct device_attribute *attr, char *buf) +show_rc6p_ms(struct device *kdev, struct device_attribute *attr, char *buf) { - struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev); + struct drm_minor *dminor = container_of(kdev, struct drm_minor, kdev); u32 rc6p_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6p); return snprintf(buf, PAGE_SIZE, "%u", rc6p_residency); } static ssize_t -show_rc6pp_ms(struct device *dev, struct device_attribute *attr, char *buf) +show_rc6pp_ms(struct device *kdev, struct device_attribute *attr, char *buf) { - struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev); + struct drm_minor *dminor = container_of(kdev, struct drm_minor, kdev); u32 rc6pp_residency = calc_residency(dminor->dev, GEN6_GT_GFX_RC6pp); return snprintf(buf, PAGE_SIZE, "%u", rc6pp_residency); } -- 1.7.12 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/7] drm/i915: Enable some sysfs stuff without CONFIG_PM
The original patch was actually incorrect in stubbing out the sysfs for l3 parity. commit 5ab3633d6907018b0b830a720e877c3884d679c3 Author: Hunt Xu Date: Sun Jul 1 03:45:07 2012 + drm/i915: make rc6 in sysfs functions conditional Unfortunately Hunt didn't respond to my review comments, and Daniel sucked in the patch again ignoring. Worst of all, I'm too lazy to write the patch for what I originally wanted, which was to keep rc6 sysfs even without CONFIG_PM. This simpler patch does enough to enable us to add more sysfs entries though. NOTE: This is already picked up in dinq, but is here in case anyone is looking to test this series on it's own. Signed-off-by: Ben Widawsky --- drivers/gpu/drm/i915/i915_sysfs.c | 15 +++ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index c5ee7ee..da733a3 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -93,6 +93,7 @@ static struct attribute_group rc6_attr_group = { .name = power_group_name, .attrs = rc6_attrs }; +#endif static int l3_access_valid(struct drm_device *dev, loff_t offset) { @@ -206,13 +207,14 @@ void i915_setup_sysfs(struct drm_device *dev) { int ret; +#ifdef CONFIG_PM if (INTEL_INFO(dev)->gen >= 6) { ret = sysfs_merge_group(&dev->primary->kdev.kobj, &rc6_attr_group); if (ret) DRM_ERROR("RC6 residency sysfs setup failed\n"); } - +#endif if (HAS_L3_GPU_CACHE(dev)) { ret = device_create_bin_file(&dev->primary->kdev, &dpf_attrs); if (ret) @@ -225,14 +227,3 @@ void i915_teardown_sysfs(struct drm_device *dev) device_remove_bin_file(&dev->primary->kdev, &dpf_attrs); sysfs_unmerge_group(&dev->primary->kdev.kobj, &rc6_attr_group); } -#else -void i915_setup_sysfs(struct drm_device *dev) -{ - return; -} - -void i915_teardown_sysfs(struct drm_device *dev) -{ - return; -} -#endif /* CONFIG_PM */ -- 1.7.12 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/7] moar sysfs stuff
Not very thoroughly tested yet. I'll be adding igt tests ASAP. Ben Widawsky (6): drm/i915: Enable some sysfs stuff without CONFIG_PM drm/i915: variable renames drm/i915: #define gpu freq multipler drm/i915: Add current GPU freq to sysfs drm/i915: Add setters for min/max frequency drm/i915: Show render P state thresholds in sysfs Jesse Barnes (1): drm/i915: expose energy counter on SNB and IVB drivers/gpu/drm/i915/i915_debugfs.c | 22 ++-- drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_reg.h | 2 + drivers/gpu/drm/i915/i915_sysfs.c | 237 4 files changed, 230 insertions(+), 33 deletions(-) -- 1.7.12 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: no longer call drm_helper_resume_force_mode
On Wed, 5 Sep 2012 13:04:54 -0700 Jesse Barnes wrote: > On Wed, 5 Sep 2012 21:56:08 +0200 > Daniel Vetter wrote: > > > On Wed, Sep 5, 2012 at 8:31 PM, Jesse Barnes > > wrote: > > > On Wed, 29 Aug 2012 23:13:29 +0200 > > > Daniel Vetter wrote: > > > > > >> Since this only calls crtc helper functions, of which a shocking > > >> amount are NULL. > > >> > > >> Now the curious thing is how the new modeset code worked with this > > >> function call still present: > > >> > > >> Thanks to the hw state readout and the suspend fixes to properly > > >> quiescent the register state, nothing is actually enabled at resume > > >> (if the bios doesn't set up anything). Which means resume_force_mode > > >> doesn't actually do anything and hence nothing blows up at resume > > >> time. > > >> > > >> The other reason things do work is that the fbcon layer has it's own > > >> resume notifier callback, which restores the mode. And thanks to the > > >> force vt switch at suspend/resume, that then forces X to restore it's > > >> own mode. > > >> > > >> Hence everything still worked (as long as the bios doesn't enable > > >> anything). And we can just kill the call to resume_force_mode. > > >> > > >> The upside of both this patch and the preceeding patch to quiescent > > >> the modeset state is that our resume path is much simpler: > > >> - We now longer restore bogus register values (which most often would > > >> enable the backlight a bit and a few ports), causing flickering. > > >> - We now longer call resume_force_mode to restore a mode that the > > >> fbcon layer would overwrite right away anyway. > > >> > > >> Signed-off-by: Daniel Vetter > > >> --- > > >> drivers/gpu/drm/i915/i915_drv.c | 5 - > > >> 1 file changed, 5 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/i915/i915_drv.c > > >> b/drivers/gpu/drm/i915/i915_drv.c > > >> index fe7512a..cd6697c 100644 > > >> --- a/drivers/gpu/drm/i915/i915_drv.c > > >> +++ b/drivers/gpu/drm/i915/i915_drv.c > > >> @@ -549,11 +549,6 @@ static int i915_drm_thaw(struct drm_device *dev) > > >> intel_modeset_setup_hw_state(dev); > > >> drm_mode_config_reset(dev); > > >> drm_irq_install(dev); > > >> - > > >> - /* Resume the modeset for every activated CRTC */ > > >> - mutex_lock(&dev->mode_config.mutex); > > >> - drm_helper_resume_force_mode(dev); > > >> - mutex_unlock(&dev->mode_config.mutex); > > >> } > > >> > > >> intel_opregion_init(dev); > > > > > > Wouldn't the fb layer's modeset end up being a no-op if the suspended > > > mode was the same as the fb mode (often the case)? Or at the very > > > least just a flip rather than a full mode set. > > > > I guess most of the flicker was because the register restoring > > restored a bunch of crap (since the old modeset state wasn't properly > > cleared before suspending). > > > > > Though we do need to deal with non-fb, non-X resumes as well. kmscon > > > and wayland will expect to be restored at resume time even if CONFIG_VT > > > and the fb layer aren't compiled into the kernel. > > > > Tbh I was rather surprised that when I've noticed this little issue > > here the restore still worked - until I've noticed by looking at the > > logs that both the fbcon and the X server restore their modes. > > > > I'm not sure what exactly we should do here, since even with the > > current code the concept of a controlling node isn't really defined in > > the kms interface (fbcon uses a bunch of funky checks to ensure it > > doesn't clobber the output state of someone else). But for now (with > > fbcon pretty much being non-optional) things keep on working, and > > afaict actually work a bit better overall. > > It's probably ok for now, but at some point we'll want some code that > restores the suspend mode if fbcon isn't enabled... > So acked/reviewed-by: Jesse Barnes But note that if someone complains we'll need to add this back... -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: push commit_output_state past the crtc/encoder preparing
On Wed, 5 Sep 2012 12:50:26 -0700 Jesse Barnes wrote: > On Wed, 5 Sep 2012 21:48:52 +0200 > Daniel Vetter wrote: > > > On Wed, Sep 5, 2012 at 8:28 PM, Jesse Barnes > > wrote: > > > > > > The variables have me confused a little... I would have expected > > > update_state to take modeset_pipes rather than prepare_pipes. Could > > > you use either? Or will that not catch cases where we updated a pipe > > > that was already on? > > > > The abstract idea for these masks was the following: Any pipe that > > changes anything goes into prepare_pipes. For any pipe that also > > changes the mode, it goes in addition into the modeset_pipes mask, so > > the later is a subset of prepare pipes. The idea here was to avoid the > > modeset step where not necessary (e.g. when disabling the 2nd output > > of a cloned crtc we only need to disable/enable, not change anything > > with the mode or clocks). But after some in-depth discussion with > > Paulo Zanoni I think we'll move large parts of the mode_set step into > > the enable function (at least for hsw due to funky ordering > > requirements), so I think this disdinction doesn't make sense. > > > > The disable mask just contains those pipes that get fully disable (and > > which then also get removed from the prepares/modset masks). > > > > Hence I pass the prepares mask into update_states, not just the modeset > > mask. > > Ok, that makes some sense. Hopefully we can preserve the full mode set > vs simple update behavior even after the refactoring for HSW. > Reviewed-by: Jesse Barnes -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: use the gmbus irq for waits
We need two special things to properly wire this up: - Add another argument to gmbus_wait_hw_status to pass in the correct interrupt bit in gmbus4. - Since we can only get an irq for one of the two events we want, hand-roll the wait_event_timeout code so that we wake up every jiffie and can check for NAKs. This way we also subsume gmbus support for platforms without interrupts (or where those are not yet enabled). The important bit really is to only enable one gmbus interrupt source at the same time - with that piece of lore figured out, this seems to work flawlessly. Ben Widawsky rightfully complained the lack of measurements for the claimed benefits (especially since the first version was actually broken and fell back to bit-banging). Previously reading the 256 byte hdmi EDID takes about 72 ms here. With this patch it's down to 33 ms. Given that transfering the 256 bytes over i2c at wire speed takes 20.5ms alone, the reduction in additional overhead is rather nice. v2: Chris Wilson wondered whether GMBUS4 might contain some set bits when booting up an hence result in some spurious interrupts. Since we clear GMBUS4 after every wait and we do gmbus transfer really early in the setup sequence to detect displays the window is small, but still be paranoid and clear it properly. v3: Clarify the comment that gmbus irq generation can only support one kind of event, why it bothers us and how we work around that limit. Cc: Daniel Kurtz Signed-off-by: Daniel Vetter fixup --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_irq.c | 4 drivers/gpu/drm/i915/intel_i2c.c | 45 ++-- 3 files changed, 40 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5c8d5e3..72db70d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -418,6 +418,8 @@ typedef struct drm_i915_private { */ uint32_t gpio_mmio_base; + wait_queue_head_t gmbus_wait_queue; + struct pci_dev *bridge_dev; struct intel_ring_buffer ring[I915_NUM_RINGS]; uint32_t next_seqno; diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 8415fa6..dadc86b 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -598,7 +598,11 @@ out: static void gmbus_irq_handler(struct drm_device *dev) { + struct drm_i915_private *dev_priv = (drm_i915_private_t *) dev->dev_private; + DRM_DEBUG_DRIVER("GMBUS interrupt\n"); + + wake_up_all(&dev_priv->gmbus_wait_queue); } static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir) diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 57decac..7413595 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -64,6 +64,7 @@ intel_i2c_reset(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; I915_WRITE(dev_priv->gpio_mmio_base + GMBUS0, 0); + I915_WRITE(dev_priv->gpio_mmio_base + GMBUS4, 0); } static void intel_i2c_quirk_set(struct drm_i915_private *dev_priv, bool enable) @@ -205,20 +206,38 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin) static int gmbus_wait_hw_status(struct drm_i915_private *dev_priv, -u32 gmbus2_status) +u32 gmbus2_status, +u32 gmbus4_irq_en) { - int ret; + int i; int reg_offset = dev_priv->gpio_mmio_base; - u32 gmbus2; + u32 gmbus2 = 0; + DEFINE_WAIT(wait); + + /* Important: The hw handles only the first bit, so set only one! Since +* we also need to check for NAKs besides the hw ready/idle signal, we +* need to wake up periodically and check that ourselves. */ + I915_WRITE(GMBUS4 + reg_offset, gmbus4_irq_en); - ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) & - (GMBUS_SATOER | gmbus2_status), - 50); + for (i = 0; i < msecs_to_jiffies(50) + 1; i++) { + prepare_to_wait(&dev_priv->gmbus_wait_queue, &wait, + TASK_UNINTERRUPTIBLE); + + gmbus2 = I915_READ(GMBUS2 + reg_offset); + if (gmbus2 & (GMBUS_SATOER | gmbus2_status)) + break; + + schedule_timeout(1); + } + finish_wait(&dev_priv->gmbus_wait_queue, &wait); + + I915_WRITE(GMBUS4 + reg_offset, 0); if (gmbus2 & GMBUS_SATOER) return -ENXIO; - - return ret; + if (gmbus2 & gmbus2_status) + return 0; + return -ETIMEDOUT; } static int @@ -239,7 +258,8 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg, int ret; u32 val, loop = 0; - ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY); +
[Intel-gfx] [PATCH] drm/i915: reorder setup sequence to have irqs for output setup
Otherwise the new&shiny irq-driven gmbus and dp aux code won't work that well. Noticed since the dp aux code doesn't have an automatic fallback with a timeout (since the hw provides for that already). Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_dma.c | 5 ++--- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/intel_display.c | 15 +-- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 2cba7b4..58320e0 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1325,9 +1325,8 @@ static int i915_load_modeset_init(struct drm_device *dev) if (ret) goto cleanup_gem_stolen; - intel_modeset_gem_init(dev); - - ret = drm_irq_install(dev); + /* Note: This also enables irqs. */ + ret = intel_modeset_gem_init(dev); if (ret) goto cleanup_gem; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 9fce782..5c8d5e3 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1536,7 +1536,7 @@ static inline void intel_unregister_dsm_handler(void) { return; } /* modesetting */ extern void intel_modeset_init_hw(struct drm_device *dev); extern void intel_modeset_init(struct drm_device *dev); -extern void intel_modeset_gem_init(struct drm_device *dev); +extern int intel_modeset_gem_init(struct drm_device *dev); extern void intel_modeset_cleanup(struct drm_device *dev); extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state); extern void intel_modeset_setup_hw_state(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 6658367..01289eb 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7948,7 +7948,6 @@ void intel_modeset_init(struct drm_device *dev) /* Just disable it once at startup */ i915_disable_vga(dev); - intel_setup_outputs(dev); } static void @@ -8206,13 +8205,25 @@ void intel_modeset_setup_hw_state(struct drm_device *dev) drm_mode_config_reset(dev); } -void intel_modeset_gem_init(struct drm_device *dev) +int intel_modeset_gem_init(struct drm_device *dev) { + int ret; + intel_modeset_init_hw(dev); intel_setup_overlay(dev); + ret = drm_irq_install(dev); + if (ret) + return ret; + + /* Importanat: Output setup needs working interrupts to use +* interrupt-driven gmbus and dp aux. */ + intel_setup_outputs(dev); + intel_modeset_setup_hw_state(dev); + + return 0; } void intel_modeset_cleanup(struct drm_device *dev) -- 1.7.11.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: use gmbus irq to wait for gmbus idle
On Thu, 6 Sep 2012 09:10:02 +0200, Daniel Vetter wrote: > GMBUS_ACTIVE has inverted sense and so doesn't fit into the > wait_hw_status helper, hence create a new gmbus_wait_idle functions. > Also, we only care about the idle irq event and nothing else, which > allows us to use the wait_event_timeout helper directly without > jumping through hoops to catch NAKs. > > Since gen2/3 don't have gmbus interrupts, handle them separately with > the old wait_for macro. > > This shaves another few ms off reading EDID from a hdmi screen on my > testbox here. EDID reading with interrupt driven gmbus is now as fast > as with busy-looping gmbus at 28 ms here (with negligible cpu > overhead). I'll put my neck on the line and say I can't spot any other mistakes: Reviewed-by: Chris Wilson A couple of triffling things, you could use whitespace more uniformly and the comment for only enabling one interrupt source could do with the explanation that then means we also have to poll for NAK. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/4] drm/i915: use gmbus irq to wait for gmbus idle
GMBUS_ACTIVE has inverted sense and so doesn't fit into the wait_hw_status helper, hence create a new gmbus_wait_idle functions. Also, we only care about the idle irq event and nothing else, which allows us to use the wait_event_timeout helper directly without jumping through hoops to catch NAKs. Since gen2/3 don't have gmbus interrupts, handle them separately with the old wait_for macro. This shaves another few ms off reading EDID from a hdmi screen on my testbox here. EDID reading with interrupt driven gmbus is now as fast as with busy-looping gmbus at 28 ms here (with negligible cpu overhead). Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_i2c.c | 32 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 86f2b8c..faf85b3 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -204,6 +204,7 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin) algo->data = bus; } +#define HAS_GMBUS_IRQ(dev) (INTEL_INFO(dev)->gen >= 4) static int gmbus_wait_hw_status(struct drm_i915_private *dev_priv, u32 gmbus2_status, @@ -239,6 +240,31 @@ gmbus_wait_hw_status(struct drm_i915_private *dev_priv, } static int +gmbus_wait_idle(struct drm_i915_private *dev_priv) +{ + int ret; + int reg_offset = dev_priv->gpio_mmio_base; + +#define C ((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0) + + if (!HAS_GMBUS_IRQ(dev_priv->dev)) + return wait_for(C, 10); + + /* Important: The hw handles only the first bit, so set only one! */ + I915_WRITE(GMBUS4 + reg_offset, GMBUS_IDLE_EN); + + ret = wait_event_timeout(dev_priv->gmbus_wait_queue, C, 10); + + I915_WRITE(GMBUS4 + reg_offset, 0); + + if (ret) + return 0; + else + return -ETIMEDOUT; +#undef C +} + +static int gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg, u32 gmbus1_index) { @@ -405,8 +431,7 @@ gmbus_xfer(struct i2c_adapter *adapter, * We will re-enable it at the start of the next xfer, * till then let it sleep. */ - if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, -10)) { + if (gmbus_wait_idle(dev_priv)) { DRM_DEBUG_KMS("GMBUS [%s] timed out waiting for idle\n", adapter->name); ret = -ETIMEDOUT; @@ -430,8 +455,7 @@ clear_err: * it's slow responding and only answers on the 2nd retry. */ ret = -ENXIO; - if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, -10)) { + if (gmbus_wait_idle(dev_priv)) { DRM_DEBUG_KMS("GMBUS [%s] timed out after NAK\n", adapter->name); ret = -ETIMEDOUT; -- 1.7.11.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/4] drm/i915: use the gmbus irq for waits
We need two special things to properly wire this up: - Add another argument to gmbus_wait_hw_status to pass in the correct interrupt bit in gmbus4. - Since we can only get an irq for one of the two events we want, hand-roll the wait_event_timeout code so that we wake up every jiffie and can check for NAKs. This way we also subsume gmbus support for platforms without interrupts (or where those are not yet enabled). The important bit really is to only enable one gmbus interrupt source at the same time - with that piece of lore figured out, this seems to work flawlessly. Ben Widawsky rightfully complained the lack of measurements for the claimed benefits (especially since the first version was actually broken and fell back to bit-banging). Previously reading the 256 byte hdmi EDID takes about 72 ms here. With this patch it's down to 33 ms. Given that transfering the 256 bytes over i2c at wire speed takes 20.5ms alone, the reduction in additional overhead is rather nice. v2: Chris Wilson wondered whether GMBUS4 might contain some set bits when booting up an hence result in some spurious interrupts. Since we clear GMBUS4 after every wait and we do gmbus transfer really early in the setup sequence to detect displays the window is small, but still be paranoid and clear it properly. Cc: Daniel Kurtz Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_irq.c | 4 drivers/gpu/drm/i915/intel_i2c.c | 43 ++-- 3 files changed, 38 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 9fce782..d93ce14 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -418,6 +418,8 @@ typedef struct drm_i915_private { */ uint32_t gpio_mmio_base; + wait_queue_head_t gmbus_wait_queue; + struct pci_dev *bridge_dev; struct intel_ring_buffer ring[I915_NUM_RINGS]; uint32_t next_seqno; diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 8415fa6..dadc86b 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -598,7 +598,11 @@ out: static void gmbus_irq_handler(struct drm_device *dev) { + struct drm_i915_private *dev_priv = (drm_i915_private_t *) dev->dev_private; + DRM_DEBUG_DRIVER("GMBUS interrupt\n"); + + wake_up_all(&dev_priv->gmbus_wait_queue); } static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir) diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 57decac..86f2b8c 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -64,6 +64,7 @@ intel_i2c_reset(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; I915_WRITE(dev_priv->gpio_mmio_base + GMBUS0, 0); + I915_WRITE(dev_priv->gpio_mmio_base + GMBUS4, 0); } static void intel_i2c_quirk_set(struct drm_i915_private *dev_priv, bool enable) @@ -205,20 +206,36 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin) static int gmbus_wait_hw_status(struct drm_i915_private *dev_priv, -u32 gmbus2_status) +u32 gmbus2_status, +u32 gmbus4_irq_en) { - int ret; + int i; int reg_offset = dev_priv->gpio_mmio_base; - u32 gmbus2; + u32 gmbus2 = 0; + DEFINE_WAIT(wait); + + /* Important: The hw handles only the first bit, so set only one! */ + I915_WRITE(GMBUS4 + reg_offset, gmbus4_irq_en); - ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) & - (GMBUS_SATOER | gmbus2_status), - 50); + for (i = 0; i < msecs_to_jiffies(50) + 1; i++) { + prepare_to_wait(&dev_priv->gmbus_wait_queue, &wait, + TASK_UNINTERRUPTIBLE); + + gmbus2 = I915_READ(GMBUS2 + reg_offset); + if (gmbus2 & (GMBUS_SATOER | gmbus2_status)) + break; + + schedule_timeout(1); + } + finish_wait(&dev_priv->gmbus_wait_queue, &wait); + + I915_WRITE(GMBUS4 + reg_offset, 0); if (gmbus2 & GMBUS_SATOER) return -ENXIO; - - return ret; + if (gmbus2 & gmbus2_status) + return 0; + return -ETIMEDOUT; } static int @@ -239,7 +256,8 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg, int ret; u32 val, loop = 0; - ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY); + ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY, + GMBUS_HW_RDY_EN); if (ret) return ret; @@ -283,7 +301,8 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg) I915_WRITE
[Intel-gfx] [PATCH 2/4] drm/i915: wire up gmbus irq handler
Only enables the interrupt and puts a irq handler into place, doesn't do anything yet. Unfortunately there's no gmbus interrupt support for gen2/3 (safe for pnv, but there the irq is marked as "Test mode"). Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_irq.c | 18 ++ 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index a61b41a..8415fa6 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -596,6 +596,11 @@ out: return ret; } +static void gmbus_irq_handler(struct drm_device *dev) +{ + DRM_DEBUG_DRIVER("GMBUS interrupt\n"); +} + static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir) { drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; @@ -607,7 +612,7 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir) SDE_AUDIO_POWER_SHIFT); if (pch_iir & SDE_GMBUS) - DRM_DEBUG_DRIVER("PCH GMBUS interrupt\n"); + gmbus_irq_handler(dev); if (pch_iir & SDE_AUDIO_HDCP_MASK) DRM_DEBUG_DRIVER("PCH HDCP audio interrupt\n"); @@ -650,7 +655,7 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) DRM_DEBUG_DRIVER("AUX channel interrupt\n"); if (pch_iir & SDE_GMBUS_CPT) - DRM_DEBUG_DRIVER("PCH GMBUS interrupt\n"); + gmbus_irq_handler(dev); if (pch_iir & SDE_AUDIO_CP_REQ_CPT) DRM_DEBUG_DRIVER("Audio CP request interrupt\n"); @@ -1841,12 +1846,14 @@ static int ironlake_irq_postinstall(struct drm_device *dev) hotplug_mask = (SDE_CRT_HOTPLUG_CPT | SDE_PORTB_HOTPLUG_CPT | SDE_PORTC_HOTPLUG_CPT | - SDE_PORTD_HOTPLUG_CPT); + SDE_PORTD_HOTPLUG_CPT | + SDE_GMBUS_CPT); } else { hotplug_mask = (SDE_CRT_HOTPLUG | SDE_PORTB_HOTPLUG | SDE_PORTC_HOTPLUG | SDE_PORTD_HOTPLUG | + SDE_GMBUS | SDE_AUX_MASK); } @@ -1906,7 +1913,8 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) hotplug_mask = (SDE_CRT_HOTPLUG_CPT | SDE_PORTB_HOTPLUG_CPT | SDE_PORTC_HOTPLUG_CPT | - SDE_PORTD_HOTPLUG_CPT); + SDE_PORTD_HOTPLUG_CPT | + SDE_GMBUS_CPT); dev_priv->pch_irq_mask = ~hotplug_mask; I915_WRITE(SDEIIR, I915_READ(SDEIIR)); @@ -1959,6 +1967,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev) POSTING_READ(VLV_IER); i915_enable_pipestat(dev_priv, 0, pipestat_enable); + i915_enable_pipestat(dev_priv, 0, PIPE_GMBUS_INTERRUPT_STATUS); i915_enable_pipestat(dev_priv, 1, pipestat_enable); I915_WRITE(VLV_IIR, 0x); @@ -2454,6 +2463,7 @@ static int i965_irq_postinstall(struct drm_device *dev) dev_priv->pipestat[0] = 0; dev_priv->pipestat[1] = 0; + i915_enable_pipestat(dev_priv, 0, PIPE_GMBUS_INTERRUPT_STATUS); /* * Enable some error detection, note the instruction error mask -- 1.7.11.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm/i915: extract gmbus_wait_hw_status
The gmbus interrupt generation is rather fiddly: We can only ever enable one interrupt source (but we always want to check for NAK in addition to the real bit). And the bits in the gmbus status register don't map at all to the bis in the irq register. To prepare for this mess, start by extracting the hw status wait loop into it's own function, consolidate the NAK error handling a bit. To keep things flexible, pass in the status bit we care about (in addition to any NAK signalling). v2: I've failed to notice that the sens of GMBUS_ACTIVE is inverted, Chris Wilson gladly pointed that out for me. To keep things simple, ignore that case for now (we only need to idle the gmbus controller at the end of an entire i2c transaction, not after every message). Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_i2c.c | 46 ++-- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index b9755f6..57decac 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -204,6 +204,24 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin) } static int +gmbus_wait_hw_status(struct drm_i915_private *dev_priv, +u32 gmbus2_status) +{ + int ret; + int reg_offset = dev_priv->gpio_mmio_base; + u32 gmbus2; + + ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) & + (GMBUS_SATOER | gmbus2_status), + 50); + + if (gmbus2 & GMBUS_SATOER) + return -ENXIO; + + return ret; +} + +static int gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg, u32 gmbus1_index) { @@ -220,15 +238,10 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg, while (len) { int ret; u32 val, loop = 0; - u32 gmbus2; - ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) & - (GMBUS_SATOER | GMBUS_HW_RDY), - 50); + ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY); if (ret) - return -ETIMEDOUT; - if (gmbus2 & GMBUS_SATOER) - return -ENXIO; + return ret; val = I915_READ(GMBUS3 + reg_offset); do { @@ -262,7 +275,6 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg) GMBUS_SLAVE_WRITE | GMBUS_SW_RDY); while (len) { int ret; - u32 gmbus2; val = loop = 0; do { @@ -271,13 +283,9 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg) I915_WRITE(GMBUS3 + reg_offset, val); - ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) & - (GMBUS_SATOER | GMBUS_HW_RDY), - 50); + ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY); if (ret) - return -ETIMEDOUT; - if (gmbus2 & GMBUS_SATOER) - return -ENXIO; + return ret; } return 0; } @@ -346,8 +354,6 @@ gmbus_xfer(struct i2c_adapter *adapter, I915_WRITE(GMBUS0 + reg_offset, bus->reg0); for (i = 0; i < num; i++) { - u32 gmbus2; - if (gmbus_is_index_read(msgs, i, num)) { ret = gmbus_xfer_index_read(dev_priv, &msgs[i]); i += 1; /* set i to the index of the read xfer */ @@ -362,13 +368,11 @@ gmbus_xfer(struct i2c_adapter *adapter, if (ret == -ENXIO) goto clear_err; - ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) & - (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), - 50); + ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE); + if (ret == -ENXIO) + goto clear_err; if (ret) goto timeout; - if (gmbus2 & GMBUS_SATOER) - goto clear_err; } /* Generate a STOP condition on the bus. Note that gmbus can't generata -- 1.7.11.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 48/58] drm/i915: use staged outuput config in tv->mode_fixup
On Wed, Sep 05, 2012 at 11:02:15AM -0700, Jesse Barnes wrote: > On Sun, 19 Aug 2012 21:13:05 +0200 > Daniel Vetter wrote: > > > The "is this encoder cloned" check will be reused by the lvds encoder, > > hence exract it. > > > > v2: Be a bit more careful about that we need to check the new, staged > > ouput configuration in the check_non_cloned helper ... > > > > Signed-off-by: Daniel Vetter > > --- > > drivers/gpu/drm/i915/intel_display.c | 22 ++ > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_tv.c | 7 ++- > > 3 files changed, 25 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index c7bd573..c59569e 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -6563,6 +6563,28 @@ static struct drm_crtc_helper_funcs > > intel_helper_funcs = { > > .disable = intel_crtc_disable, > > }; > > > > +bool intel_encoder_check_non_cloned(struct intel_encoder *encoder) > > +{ > > + struct intel_encoder *other_encoder; > > + struct drm_crtc *crtc = &encoder->new_crtc->base; > > + > > + if (WARN_ON(!crtc)) > > + return true; > > + > > + list_for_each_entry(other_encoder, > > + &crtc->dev->mode_config.encoder_list, > > + base.head) { > > + > > + if (&other_encoder->new_crtc->base != crtc || > > + encoder == other_encoder) > > + continue; > > + else > > + return false; > > + } > > + > > + return true; > > +} > > encoder_is_cloned() would make the callers more readable and avoid the > double negative... Ok, got a bit bored and applied this bikeshed here, too. Won't bother with resending though. -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/58] modeset-rework, the basic conversion
On Thu, Sep 06, 2012 at 08:55:40AM +0200, Daniel Vetter wrote: > On Wed, Sep 05, 2012 at 04:23:55PM -0700, Jesse Barnes wrote: > > On Sun, 19 Aug 2012 21:12:17 +0200 > > Daniel Vetter wrote: > > > > > Hi all, > > > > > > Changes since last time around: > > > - The prep patches are all merged now. > > > - I've left out the actual DP fixes/cleanups, I think we should merge > > > those in a > > > separte step. > > > - A few bugfixes (thanks to Paulo, Jani and Chris). > > > - I've also applied a few bikesheds for naming that Paulo suggested (but > > > I'm not > > > sure whether I've sent those out already in a previous patchbomb). > > > > > > Essentially this is just the core rework, which addes the new > > > get_hw_state code, > > > refactors all the encoders to use the new functions and finally reworks > > > the > > > modeset logic to disable/enable entire pipes, always (and with a > > > deterministic > > > order). > > > > > > For merging to -next, I plan to pull in everything with a real merge > > > commit. For > > > that reason I've put up a modeset-rework-base branch onto my private fdo > > > repo[1]. > > > That way I can put a short documentation for the new modeset design into > > > the > > > merge commit (stichted together from the previous patchbomb cover > > > letters), > > > documenting my folly assumptions for eternity. > > > > > > I'll also plan to put tags for the entire series in the merge commit, so > > > if you > > > have tested this on a few machines, read through and agree with the new > > > designs, > > > please reply with your tested-by/acked-by/reviewed-by tags. > > > > > > Flames, comments and test reports highly welcome. > > > > Ok I've tested on Ironlake, Cantiga, Crestline, and Pineview so far and > > things look good. I ran testdisplay both with and without VGA > > attached (the ILK has a eDP panel), and tried S3 and S4 both with and > > without VGA both in the console and in X. > > > > There was one issue on Pineview where the VGA seemed to get > > "forgotten", but I haven't isolated it yet. I canceled testdisplay > > part way through and that seemed to confuse fbcon about what was there. > > QA reported a similar issue where fbcon refuses to light up the display > after testdisplay completed a while back: > > https://bugs.freedesktop.org/show_bug.cgi?id=42194 > > > I also see an issue with 1280x800 modes across all platforms, but that > > may just be the monitor, I need to test more. > > > > I'm testing Montara (that's 855 for you youngsters) now, but it's slow > > so not all my builds have completed there yet. > > > > Overall though: > > Tested-by: Jesse Barnes > > Cool, thanks a lot for review&testing. I'll do the merge now (need to slap > all your r-b's onto patches first). Actually I'm still lacking an r-b on two patches ... I'll annoy you again later today. -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx