[Bug 91749] Tonga IH ring misaligned
https://bugs.freedesktop.org/show_bug.cgi?id=91749 Bug ID: 91749 Summary: Tonga IH ring misaligned Product: DRI Version: DRI git Hardware: Other OS: All Status: NEW Severity: normal Priority: medium Component: DRM/AMDgpu Assignee: dri-devel at lists.freedesktop.org Reporter: jay at jcornwall.me >From amdgpu_ih.c: adev->irq.ih.ring = kzalloc(adev->irq.ih.ring_size + 8, GFP_KERNEL); if (adev->irq.ih.ring == NULL) return -ENOMEM; adev->irq.ih.rb_dma_addr = pci_map_single(adev->pdev, (void *)adev->irq.ih.ring, adev->irq.ih.ring_size, PCI_DMA_BIDIRECTIONAL); The Tonga IH_RB_BASE register requires 256B alignment. kzalloc does not guarantee this, e.g.: adev->irq.ih.ring: 0x880835B22148 adev->irq.ih.rb_dma_addr: 0x835B22148 IH_RB_BASE: 0835b221 This causes amdgpu_ih_decode_iv to read ahead of the last written IV, missing it, when this misalignment occurs. -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/c0c05286/attachment.html>
[Bug 103271] AMD R9 270X Flickering with DPM Enabled on Linux 4.1 with RadeonSI
https://bugzilla.kernel.org/show_bug.cgi?id=103271 --- Comment #7 from Kevin --- Well I tried bisecting and got this... $ git bisect start Checking out files: 100% (14034/14034), done. Previous HEAD position was d0a3997... Merge tag 'sound-4.1-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound Switched to branch 'master' Your branch is up-to-date with 'origin/master'. -- You are receiving this mail because: You are watching the assignee of the bug.
[Bug 103271] AMD R9 270X Flickering with DPM Enabled on Linux 4.1 with RadeonSI
https://bugzilla.kernel.org/show_bug.cgi?id=103271 --- Comment #8 from Alex Deucher --- You need to flag which commit or tag was good and which was bad. -- You are receiving this mail because: You are watching the assignee of the bug.
[Bug 103271] AMD R9 270X Flickering with DPM Enabled on Linux 4.1 with RadeonSI
https://bugzilla.kernel.org/show_bug.cgi?id=103271 --- Comment #9 from Kevin McCormack --- OK, I am giving it a try now, sorry for my noob-ness :\ it takes a while to compile. -- You are receiving this mail because: You are watching the assignee of the bug.
drm/amdgpu: implement cgs gpu memory callbacks
Inline...[DZ] > -Original Message- > From: Dan Carpenter [mailto:dan.carpenter at oracle.com] > Sent: Tuesday, August 25, 2015 3:51 AM > To: Zhou, David(ChunMing) > Cc: dri-devel at lists.freedesktop.org > Subject: Re: drm/amdgpu: implement cgs gpu memory callbacks > > On Mon, Aug 24, 2015 at 07:09:15AM +, Zhou, David(ChunMing) wrote: > > Hi Dan, > > Thanks for figuring out that. > > >274 min_offset = obj->placements[0].fpfn << PAGE_SHIFT; > > >275 max_offset = obj->placements[0].lpfn << PAGE_SHIFT; > > Maybe should be: > > min_offset = obj->placements[0].fpfn; > > min_offset <<= PAGE_SHIFT; > > max_offset = obj->placements[0].lpfn; > > max_offset <<= PAGE_SHIFT; > > > It's probably just simpler to be: > > min_offset = (u64)obj->placements[0].fpfn << PAGE_SHIFT; > max_offset = (u64)obj->placements[0].lpfn << PAGE_SHIFT; > > But the larger questions aer why is min_offset a u64, [DZ] max/min_offset is memory size. >and can the shift actually > wrap? [DZ] of course, adding shift wrap is better. fpfn/lpfn is page number, so < I'm just looking at static checker warnings, and I'm not very familiar > with this code so I don't know the answers. > > regards, > dan carpenter
[Bug 91749] Tonga IH ring misaligned
https://bugs.freedesktop.org/show_bug.cgi?id=91749 --- Comment #1 from Alex Deucher --- Created attachment 117903 --> https://bugs.freedesktop.org/attachment.cgi?id=117903&action=edit possible fix This should fix it. -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/86b369e8/attachment.html>
[Bug 91749] Tonga IH ring misaligned
https://bugs.freedesktop.org/show_bug.cgi?id=91749 --- Comment #2 from Jay Cornwall --- Works good, thanks. -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/045e5ab5/attachment-0001.html>
[PATCH] drm/crtc: Add a helper func to get a registered crtc from its index
This patch add a helper func to get a registered crtc from its index.
In some case, where we know the crtc's index and we want to know the
crtc too.
For example, the enable_vblank func of struct drm_driver:
In the implementation of this func, we know the index of the crtc but
we want to know the crtc. This helper func can get the crtc easily.
A sample impelmentation of enable_vblank is as shown as bellow:
int hisi_drm_crtc_enable_vblank(struct drm_device *dev, int c)
{
struct drm_crtc *crtc = drm_get_crtc_from_index(dev, c);
struct hisi_crtc *hcrtc = to_hisi_crtc(crtc);
struct hisi_crtc_ops *ops = hcrtc->ops;
int ret = 0;
if (ops->enable_vblank)
ret = ops->enable_vblank(hcrtc);
return ret;
}
Signed-off-by: Xinliang Liu
---
drivers/gpu/drm/drm_crtc.c | 25 +
include/drm/drm_crtc.h | 2 ++
2 files changed, 27 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b9ba061..8764765 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -747,6 +747,31 @@ unsigned int drm_crtc_index(struct drm_crtc *crtc)
}
EXPORT_SYMBOL(drm_crtc_index);
+/**
+ * drm_get_crtc_from_index - find a registered CRTC from the index
+ * @dev: DRM device
+ * @index: index of a registered CRTC
+ *
+ * Given a index, return the registered CRTC within a DRM
+ * device's list of CRTCs.
+ */
+struct drm_crtc *drm_get_crtc_from_index(struct drm_device *dev,
+unsigned int index)
+{
+ unsigned int index_tmp = 0;
+ struct drm_crtc *crtc;
+
+ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+ if (index_tmp == index)
+ return crtc;
+
+ index_tmp++;
+ }
+
+ BUG();
+}
+EXPORT_SYMBOL(drm_get_crtc_from_index);
+
/*
* drm_mode_remove - remove and free a mode
* @connector: connector list to modify
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 57ca8cc..3a46d39d 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1225,6 +1225,8 @@ extern int drm_crtc_init_with_planes(struct drm_device
*dev,
const struct drm_crtc_funcs *funcs);
extern void drm_crtc_cleanup(struct drm_crtc *crtc);
extern unsigned int drm_crtc_index(struct drm_crtc *crtc);
+extern struct drm_crtc *drm_get_crtc_from_index(struct drm_device *dev,
+ unsigned int index);
/**
* drm_crtc_mask - find the mask of a registered CRTC
--
1.9.1
[Bug 91741] Opengl Version Downgrades to 2.1 from 3.3 in Ubuntu 15.04
https://bugs.freedesktop.org/show_bug.cgi?id=91741 --- Comment #4 from jesnake at gmail.com --- (In reply to Grigori Goronzy from comment #1) > Please check if you have enabled floating point texture support. Thanks Goronzy , Enabling texture-float-point support helped to enable Opengl 3.3 in Mesa Thanks All for the Suggestions -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/abeae65b/attachment.html>
[Nouveau] [PATCH 0/2] drm/nouveau: add support for 2560x1440@56 over HDMI
On 18 August 2015 at 04:04, Hauke Mehrtens wrote: > On 08/08/2015 07:01 PM, Hauke Mehrtens wrote: >> These patches are adding support for outputting 2560x1440 at 56 over HDMI. >> This needs a pixel clock of 225 MHz which was not supported before. >> >> This was tested in a dual monitor setup with a GF114 (GTX 560 TI) and >> one HDMI monitor running with 2560x1440 at 56 and one DVI monitor running >> with 1920x1200 at 60. This still needs testing on other graphics cards and >> with dual link DVI. Out of curiosity, what mode does the VBIOS select to display the system's boot messages? Ben. >> >> There is no Maintainers entry for nouveau. >> >> Hauke Mehrtens (2): >> drm/nouveau: activate dual link TMDS links only when possible >> drm/nouveau: increase max pixel clock to 225 MHZ for HDMI >> >> drivers/gpu/drm/nouveau/nouveau_connector.c | 6 -- >> drivers/gpu/drm/nouveau/nv50_display.c | 8 >> drivers/gpu/drm/nouveau/nvkm/engine/disp/gf110.c | 2 +- >> drivers/gpu/drm/nouveau/nvkm/engine/disp/nv50.c | 2 +- >> 4 files changed, 10 insertions(+), 8 deletions(-) >> > > > Hi Ben, > > Some people in the IRC suggested you should look at these patches. > > There are still some parts in the code with something like "if > (pixel_lock > 165MHz)", I haven't changed that because that did not > affected me and I do not know in which situations this gets called. > > Hauke > ___ > Nouveau mailing list > Nouveau at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/nouveau
about mmap dma-buf and sync
Hi,
On 08/24/2015 07:56 PM, Michael Spang wrote:
> On Mon, Aug 24, 2015 at 1:42 PM, Thomas Hellstrom
> wrote:
>> On 08/24/2015 07:12 PM, Daniel Stone wrote:
>>> Hi,
>>>
>>> On 24 August 2015 at 18:10, Thomas Hellstrom
>>> wrote:
On 08/24/2015 07:04 PM, Daniel Stone wrote:
> On 24 August 2015 at 17:56, Thomas Hellstrom
> wrote:
>> On 08/24/2015 05:52 PM, Daniel Stone wrote:
>>> I still don't think this ameliorates the need for batching: consider
>>> the case where you update two disjoint screen regions and want them
>>> both flushed. Either you issue two separate sync calls (which can be
>>> disadvantageous performance-wise on some hardware / setups), or you
>>> accumulate the regions and only flush later. So either two ioctls (one
>>> in the style of dirtyfb and one to perform the sync/flush; you can
>>> shortcut to assume the full buffer was damaged if the former is
>>> missing), or one like this:
>>>
>>> struct dma_buf_sync_2d {
>>> enum dma_buf_sync_flags flags;
>>>
>>> __u64 stride_bytes;
>>> __u32 bytes_per_pixel;
>>> __u32 num_regions;
>>>
>>> struct {
>>> __u64 x;
>>> __u64 y;
>>> __u64 width;
>>> __u64 height;
>>> } regions[];
>>> };
>> Fine with me, although perhaps bytes_per_pixel is a bit redundant?
> Redundant how? It's not implicit in stride.
For flushing purposes, isn't it possible to cover all cases by assuming
bytes_per_pixel=1? Not that it matters much.
>>> Sure, though in that case best to replace x with line_byte_offset or
>>> something, because otherwise I guarantee you everyone will get it
>>> wrong, and it'll be a pain to track down. Like how I managed to
>>> misread it now. :)
>> OK, yeah you have a point. IMO let's go for your proposal.
>>
>> Tiago, is this OK with you?
> Is there any obstacle to making the above API a new syscall?
I think that whether this API can be accepted as a syscall or not is
really up to Dave and in the end Linus.
What speaks against is that it's not applicable to all file types, only
dma-bufs and also that we might want to extend
it in the future.
>
> The reason we have issues with ioctls is because it's not possible to
> whitelist them properly with seccomp BPF - there's no single namespace
> for ioctls.
>
> If this API is merged as a ioctl, we may not be able to actually use
> it. Which is a bit unfortunate when it has been proposed with the
> chrome renderer use case in mind.
I don't think mmap() on dma-bufs can be accepted as a _generic_ API
without a mandatory sync mechanism, and I think there is a need to
specify a sync api regardless of who is the first user.
Thanks,
Thomas
>
> Michael
drm/amdgpu: implement cgs gpu memory callbacks
On Tue, Aug 25, 2015 at 02:07:21AM +, Zhou, David(ChunMing) wrote: > >and can the shift actually > > wrap? > [DZ] of course, adding shift wrap is better. fpfn/lpfn is page number, so > <
drm/amdgpu: implement cgs gpu memory callbacks
> -Original Message-
> From: Dan Carpenter [mailto:dan.carpenter at oracle.com]
> Sent: Tuesday, August 25, 2015 1:51 PM
> To: Zhou, David(ChunMing)
> Cc: dri-devel at lists.freedesktop.org
> Subject: Re: drm/amdgpu: implement cgs gpu memory callbacks
>
> On Tue, Aug 25, 2015 at 02:07:21AM +, Zhou, David(ChunMing) wrote:
> > >and can the shift actually
> > > wrap?
> > [DZ] of course, adding shift wrap is better. fpfn/lpfn is page number, so
> <
> By "Shift wrap" I meant how you shift beyond the end of a 32bit number and it
> truncates, or if it's signed then it can wrap around (it's undefined
> actually, I
> probably should use a different word?).
>
> int main(void)
> {
> unsigned long long a = 0xf000U << 12;
> unsigned long long b = 0xf000ULL << 12;
>
> printf("%llx %llx\n", a, b);
>
> return 0;
> }
[DZ] oh, I understand your mean, with previous PAGE_SHIFT, I think it should be
like 'b' in your example without truncates.
Thanks,
David Zhou
>
> regards,
> dan carpenter
[PATCH 2/3] drm:msm: Initial Add Writeback Support (V2)
On Wed, Aug 19, 2015 at 03:00:04PM -0400, Rob Clark wrote: > On Tue, Apr 7, 2015 at 2:09 PM, Jilai Wang wrote: > So one thing that I wanted sorting out before we let userspace see > streaming writeback (where I do think v4l is the right interface), is > a way to deal w/ permissions/security.. Ie. only the kms master > should control access to writeback. Ie. an process that the > compositor isn't aware of / doesn't trust, should not be able to open > the v4l device and start snooping on the screen contents. And I don't > think just file permissions in /dev is sufficient. You likely don't > want to run your helper process doing video encode and streaming as a > privilaged user. > > One way to handle this would be some sort of dri2 style > getmagic/authmagic sort of interface between the drm/kms master, and > v4l device, to unlock streaming. But that is kind of passe. Fd > passing is the fashionable thing now. So instead we could use a dummy > v4l2_file_opererations::open() which always returns an error. So v4l > device shows up in /dev.. but no userspace can open it. And instead, > the way to get a fd for the v4l dev would be via a drm/kms ioctl (with > DRM_MASTER flag set). Once compositor gets the fd, it can use fd > passing, if needed, to hand it off to a helper process, etc. > > (probably use something like alloc_file() to get the 'struct file *', > then call directly into v4l2_fh_open(), and then get_unused_fd_flags() > + fd_install() to get fd to return to userspace) Just following up here, but consensus from the lpc track is that we don't need this as long as writeback is something which needs a specific action to initiate. I.e. if we have a separate writeback connector which won't grab any frames at all as long as it's disconnected we should be fine. Wrt session handling that's something which would need to be fixed between v4l and logind (or whatever). In general I don't like hand-rolling our own proprietary v4l-open process, it means that all the existing v4l test&dev tooling must be changed, even when you're root. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH] drm: rcar-du: Fix plane state free in plane reset handler
On Tue, Aug 18, 2015 at 09:35:44AM +0300, Laurent Pinchart wrote:
> Hi Daniel,
>
> On Friday 14 August 2015 09:30:15 Daniel Vetter wrote:
> > On Fri, Aug 14, 2015 at 12:19:03AM +0300, Laurent Pinchart wrote:
> > > On Friday 07 August 2015 17:30:08 Laurent Pinchart wrote:
> > > > On Friday 07 August 2015 14:53:22 Thierry Reding wrote:
> > > > > On Thu, Aug 06, 2015 at 03:23:00AM +0300, Laurent Pinchart wrote:
> > > > > > The plane reset handler frees the plane state and allocates a new
> > > > > > default state, but when doing so attempt to free the plane state
> > > > > > using the base plane state pointer instead of casting it to the
> > > > > > driver-specific state object that has been allocated. Fix it by
> > > > > > using the rcar_du_plane_atomic_destroy_state() function to destroy
> > > > > > the plane state instead of duplicating the code.
> > > > > >
> > > > > > Signed-off-by: Laurent Pinchart
> > > > > >
> > > > > > ---
> > > > > >
> > > > > > drivers/gpu/drm/rcar-du/rcar_du_plane.c | 45 ++
> > > > > > 1 file changed, 22 insertions(+), 23 deletions(-)
> > > > > >
> > > > > > Should the DRM core free the atomic state before calling the reset
> > > > > > operation ? That would simplify drivers.
> > > > >
> > > > > The core can't do that because drivers might have subclassed the
> > > > > state.
> > > >
> > > > But the core can call the .atomic_destroy_state() operation, can't it ?
> > >
> > > Thierry, Daniel, any comment on this ?
> >
> > Doesn't really help you since the kzalloc is still in the helper. Btw this
> > is all helper code, core won't do here anything at all ;-)
>
> Is it ? The .reset() and .atomic_destroy_state() are core plane operations,
> not helper operations.
Reset not being a helper func is an accident of history I think, it should
be moved.
> My point is that, as .reset() needs to allocate the state if no state exists,
> I'm wondering whether it wouldn't be simpler for drivers to free the state in
> the core using .atomic_destroy_state() before calling .reset() and always
> allocate a state in the driver's .reset() implementation.
>
> In peudo-code, drivers currently do (or at least should do)
>
> atomic_destroy_state(state)
> {
> driver_state = cast_to_driver_state(state);
>
> clean up driver_state;
> kfree(driver_state);
> }
>
> reset()
> {
> if (state) {
> driver_state = cast_to_driver_state(state);
>
> clean up driver_state;
Why not call destroy_state here and make the kzalloc unconditional?
Simpler and with that not much point in removing copypasting ...
> } else {
> driver_state = kzalloc(...);
> }
>
> set all fields of driver_state to default values;
> }
>
> Wouldn't it be simpler to have the core call .atomic_destroy_state() before
> .reset() and implement .reset() as
>
> reset()
> driver_state = kzalloc(...);
>
> set all fields of driver_state to default values;
> }
>
> ?
Well all the reset stuff was pretty much stop-gap, ->reset really
shouldn't be a core op. What I eventually wanted to do is lift the hw
state readout logic from i915 as the proper way to do this, since without
this you can't do fastboot. Eric is interested in fastboot for vc4, so we
discussed this a bit at lpc.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH 03/18] drm/i915: Add atomic get property interface for CRTC
On Sat, Aug 22, 2015 at 11:30:44AM +0530, Sharma, Shashank wrote:
> Thanks for the review Matt.
>
> Regards
> Shashank
> On 8/22/2015 4:10 AM, Matt Roper wrote:
> >On Thu, Aug 06, 2015 at 10:08:12PM +0530, Shashank Sharma wrote:
> >>From: Kausal Malladi
> >>
> >>This patch adds atomic get property interface for Intel CRTC. This
> >>interface will be used for get operation on any non-core DRM properties.
> >>
> >>Signed-off-by: Shashank Sharma
> >>Signed-off-by: Kausal Malladi
> >>---
> >> drivers/gpu/drm/i915/intel_atomic.c | 8
> >> drivers/gpu/drm/i915/intel_display.c | 1 +
> >> drivers/gpu/drm/i915/intel_drv.h | 4
> >> 3 files changed, 13 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> >>b/drivers/gpu/drm/i915/intel_atomic.c
> >>index 922fecf..8d04ee8 100644
> >>--- a/drivers/gpu/drm/i915/intel_atomic.c
> >>+++ b/drivers/gpu/drm/i915/intel_atomic.c
> >>@@ -327,3 +327,11 @@ int intel_crtc_atomic_set_property(struct drm_crtc
> >>*crtc,
> >>DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> >>return -EINVAL;
> >> }
> >>+
> >>+int intel_crtc_atomic_get_property(struct drm_crtc *crtc,
> >>+ const struct drm_crtc_state *state,
> >>+ struct drm_property *property,
> >>+ uint64_t *val)
> >>+{
> >>+ return 0;
> >
> >I think this should be -EINVAL since it's an unrecognized property.
> >Probably add a DRM_DEBUG_KMS() message here like we have in
> >intel_plane_atomic_get_property() as well.
> >
> >
> >Matt
> Got it.
Sounds like we need an igt for invalid properties ...
-Daniel
> >
> >>+}
> >>diff --git a/drivers/gpu/drm/i915/intel_display.c
> >>b/drivers/gpu/drm/i915/intel_display.c
> >>index 1fbf0ff..1412e21 100644
> >>--- a/drivers/gpu/drm/i915/intel_display.c
> >>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>@@ -13353,6 +13353,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs
> >>= {
> >>.page_flip = intel_crtc_page_flip,
> >>.set_property = drm_atomic_helper_crtc_set_property,
> >>.atomic_set_property = intel_crtc_atomic_set_property,
> >>+ .atomic_get_property = intel_crtc_atomic_get_property,
> >>.atomic_duplicate_state = intel_crtc_duplicate_state,
> >>.atomic_destroy_state = intel_crtc_destroy_state,
> >> };
> >>diff --git a/drivers/gpu/drm/i915/intel_drv.h
> >>b/drivers/gpu/drm/i915/intel_drv.h
> >>index c0ae529..b3dc138 100644
> >>--- a/drivers/gpu/drm/i915/intel_drv.h
> >>+++ b/drivers/gpu/drm/i915/intel_drv.h
> >>@@ -1426,6 +1426,10 @@ int intel_crtc_atomic_set_property(struct drm_crtc
> >>*plane,
> >> struct drm_crtc_state *state,
> >> struct drm_property *property,
> >> uint64_t val);
> >>+int intel_crtc_atomic_get_property(struct drm_crtc *plane,
> >>+ const struct drm_crtc_state *state,
> >>+ struct drm_property *property,
> >>+ uint64_t *val);
> >>
> >> /* intel_atomic_plane.c */
> >> struct intel_plane_state *intel_create_plane_state(struct drm_plane
> >> *plane);
> >>--
> >>1.9.1
> >>
> >
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH 08/18] drm/i915: Add pipe gamma correction handlers
On Sat, Aug 22, 2015 at 11:41:45AM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
>
> On 8/22/2015 4:10 AM, Matt Roper wrote:
> >On Thu, Aug 06, 2015 at 10:08:17PM +0530, Shashank Sharma wrote:
> >>From: Kausal Malladi
> >>
> >>I915 driver registers gamma correction as palette correction
> >>property with DRM layer. This patch adds set_property() and get_property()
> >>handlers for pipe level gamma correction.
> >>
> >>The set function attaches the Gamma correction blob to CRTC state, these
> >>values will be committed during atomic commit.
> >>
> >>Signed-off-by: Shashank Sharma
> >>Signed-off-by: Kausal Malladi
> >>---
> >> drivers/gpu/drm/i915/intel_atomic.c| 14 ++
> >> drivers/gpu/drm/i915/intel_color_manager.c | 20
> >> drivers/gpu/drm/i915/intel_drv.h | 3 +++
> >> 3 files changed, 37 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> >>b/drivers/gpu/drm/i915/intel_atomic.c
> >>index 8d04ee8..9f55e6c 100644
> >>--- a/drivers/gpu/drm/i915/intel_atomic.c
> >>+++ b/drivers/gpu/drm/i915/intel_atomic.c
> >>@@ -324,6 +324,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc
> >>*crtc,
> >> struct drm_property *property,
> >> uint64_t val)
> >> {
> >>+ struct drm_device *dev = crtc->dev;
> >>+ struct drm_mode_config *config = &dev->mode_config;
> >>+
> >>+ if (property == config->cm_palette_after_ctm_property)
> >>+ return intel_color_manager_set_pipe_gamma(dev, state,
> >>+ &crtc->base, val);
> >>+
> >>DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name);
> >>return -EINVAL;
> >> }
> >>@@ -333,5 +340,12 @@ int intel_crtc_atomic_get_property(struct drm_crtc
> >>*crtc,
> >> struct drm_property *property,
> >> uint64_t *val)
> >> {
> >>+ struct drm_device *dev = crtc->dev;
> >>+ struct drm_mode_config *config = &dev->mode_config;
> >>+
> >>+ if (property == config->cm_palette_after_ctm_property)
> >>+ *val = (state->palette_after_ctm_blob) ?
> >>+ state->palette_after_ctm_blob->base.id : 0;
> >>+
> >>return 0;
> >> }
> >>diff --git a/drivers/gpu/drm/i915/intel_color_manager.c
> >>b/drivers/gpu/drm/i915/intel_color_manager.c
> >>index 1c9c477..9a6126c 100644
> >>--- a/drivers/gpu/drm/i915/intel_color_manager.c
> >>+++ b/drivers/gpu/drm/i915/intel_color_manager.c
> >>@@ -27,6 +27,26 @@
> >>
> >> #include "intel_color_manager.h"
> >>
> >>+int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
> >>+ struct drm_crtc_state *crtc_state,
> >>+ struct drm_mode_object *obj, uint32_t blob_id)
> >>+{
> >>+ struct drm_property_blob *blob;
> >>+
> >>+ blob = drm_property_lookup_blob(dev, blob_id);
> >>+ if (!blob) {
> >>+ DRM_ERROR("Invalid Blob ID\n");
> >
> >A user can trigger this error on demand, so I think we want to keep this
> >as DRM_DEBUG_KMS (same on patches #10 and 13).
> >
> Agree.
Again needs a testcase in igt ...
-Daniel
> >
> >Matt
> >
> >>+ return -EINVAL;
> >>+ }
> >>+
> >>+ if (crtc_state->palette_after_ctm_blob)
> >>+
> >>drm_property_unreference_blob(crtc_state->palette_after_ctm_blob);
> >>+
> >>+ /* Attach the blob to be commited in state */
> >>+ crtc_state->palette_after_ctm_blob = blob;
> >>+ return 0;
> >>+}
> >>+
> >> int get_chv_pipe_gamma_capabilities(struct drm_device *dev,
> >>struct drm_palette_caps *palette_caps, struct drm_crtc *crtc)
> >> {
> >>diff --git a/drivers/gpu/drm/i915/intel_drv.h
> >>b/drivers/gpu/drm/i915/intel_drv.h
> >>index dee5f91..820ded7 100644
> >>--- a/drivers/gpu/drm/i915/intel_drv.h
> >>+++ b/drivers/gpu/drm/i915/intel_drv.h
> >>@@ -1441,5 +1441,8 @@ extern const struct drm_plane_helper_funcs
> >>intel_plane_helper_funcs;
> >> /* intel_color_manager.c */
> >> void intel_attach_color_properties_to_crtc(struct drm_device *dev,
> >>struct drm_mode_object *mode_obj);
> >>+int intel_color_manager_set_pipe_gamma(struct drm_device *dev,
> >>+ struct drm_crtc_state *crtc_state,
> >>+ struct drm_mode_object *obj, uint32_t blob_id);
> >>
> >> #endif /* __INTEL_DRV_H__ */
> >>--
> >>1.9.1
> >>
> >
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH v2 00/22] Enable gpu switching on the MacBook Pro
Hi Dave, as requested I've pushed the MacBook Pro GPU switching stuff to GitHub to ease browsing/reviewing, latest branch is gmux-v5: https://github.com/l1k/linux/commits/gmux-v5 (I had applied for a freedesktop.org account in April with bug 89906 but it's still pending, hence GitHub.) I've overhauled locking once more because previously all EDID reads were serialized even on machines which don't use vga_switcheroo at all. Seems it's impossible to fix this with mutexes and still be race-free and deadlock-free, so I've changed vgasr_mutex to an rwlock: https://github.com/l1k/linux/commit/ffa4546d85244a53541eed6799f93e8eea7f29e8 There are a number of vga_switcheroo functions added by Takashi Iwai and yourself which don't lock anything at all, I believe this was in part to avoid deadlocks (vga_switcheroo_runtime_resume_hdmi_audio locks vgasr_mutex, wakes up the GPU with vga_switcheroo_runtime_resume which locks vgasr_mutex once again). After changing vgasr_mutex to an rwlock we can safely use locking in those functions as well: https://github.com/l1k/linux/commit/cdea8f5c92039329dde47cadf20b67c7d9af0d62 With this change, on machines which don't use vga_switcheroo, the overhead in drm_get_edid() is exactly 1 read_lock + 1 read_unlock and EDID reads can happen in parallel. Thierry Reding and Alex Deucher are in favor of adding a separate drm_get_edid_switcheroo() and changing the relevant drivers (i915, nouveau, radeon, amdgpu) to use that in lieu of drm_get_edid() so that drivers which don't use vga_switcheroo avoid the lock_ddc/unlock_ddc calls. Performance-wise these additional calls should be negligible, so I think the motivation can only be better readability/clarity. One should also keep in mind that drivers which don't use vga_switcheroo currently might do so in the future, e.g. if mobile GPUs use a big.LITTLE concept like ARM CPUs already do. Best regards, Lukas
[PATCH 1/7] prime_mmap: Add new test for calling mmap() on dma-buf fds
On Fri, Aug 14, 2015 at 07:17:05PM -0300, Tiago Vignatti wrote: > Hi Daniel, > > On 08/13/2015 04:04 AM, Daniel Vetter wrote: > >On Wed, Aug 12, 2015 at 08:29:14PM -0300, Tiago Vignatti wrote: > >>+ /* Map too big */ > >>+ handle = gem_create(fd, BO_SIZE); > >>+ fill_bo(handle, BO_SIZE); > >>+ dma_buf_fd = prime_handle_to_fd(fd, handle); > >>+ igt_assert(errno == 0); > >>+ ptr = mmap(NULL, BO_SIZE * 2, PROT_READ, MAP_SHARED, dma_buf_fd, 0); > >>+ igt_assert(ptr == MAP_FAILED && errno == EINVAL); > >>+ errno = 0; > >>+ close(dma_buf_fd); > >>+ gem_close(fd, handle); > > > >That only checks for one of the conditions, trying to map something > >offset/overlapping the end of the buffer, but small enough needs to be > >tested separately. > > you mean test for smaller length with a non-zero offset? I don't think I get > what you wanted to say here maybe. Atm you test size=2*bo_size, offset=0. I also want to test size=bo_size, offset=bo_size/2 since there seems to be a bug for that case in your code. > >Also I think a testcase for invalid buffer2fd flags would be good, just > >for completeness - we seem to be missing that one. > > you mean test for different flags than the ones supported by > DRM_IOCTL_PRIME_HANDLE_TO_FD? Yup, just the normal "fill in missing coverage" we do when extending an existing interface. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH 4/7] lib: Add gem_userptr helpers
On Sun, Aug 23, 2015 at 01:10:42PM +0100, Chris Wilson wrote: > On Wed, Aug 12, 2015 at 08:29:17PM -0300, Tiago Vignatti wrote: > > This patch moves userptr definitions and helpers implementation that were > > locally in gem_userptr_benchmark and gem_userptr_blits to the library, so > > other > > tests can make use of them as well. There's no functional changes. > > We have a pattern in the helpers to use > gem_userptr() as a no-fail (failures are detected and asserted upon > inside the handler) and __gem_userptr() where the errno is passed back > to the caller. Commonly when writing tests it is simpler to just get the > handle back (and have the test fail appropriately otherwise) and doing > negative parameter testing is rarer and so delegated to the > __gem_ variant. Also please add gtkdoc when adding new wrappers (and generally those doc comments mention the distinction Chris raised). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH v2.1 3/3] drm/edid: Switch DDC when reading the EDID
On Mon, Aug 17, 2015 at 10:24:32PM +0200, Lukas Wunner wrote:
> Hi Thierry,
>
> On Mon, Aug 17, 2015 at 12:41:32PM +0200, Thierry Reding wrote:
> > On Fri, Aug 14, 2015 at 06:28:35PM +0200, Lukas Wunner wrote:
> > > @@ -1377,13 +1378,21 @@ struct edid *drm_get_edid(struct drm_connector
> > > *connector,
> > > struct i2c_adapter *adapter)
> > > {
> > > struct edid *edid;
> > > + struct pci_dev *pdev = connector->dev->pdev;
> > >
> > > - if (!drm_probe_ddc(adapter))
> > > + vga_switcheroo_lock_ddc(pdev);
> > > +
> > > + if (!drm_probe_ddc(adapter)) {
> > > + vga_switcheroo_unlock_ddc(pdev);
> > > return NULL;
> > > + }
> > >
> > > edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
> > > if (edid)
> > > drm_get_displayid(connector, edid);
> > > +
> > > + vga_switcheroo_unlock_ddc(pdev);
> > > +
> > > return edid;
> > > }
> > > EXPORT_SYMBOL(drm_get_edid);
> >
> > I think this is backwards and it'd be more explicit (though I suspect
> > slightly more work for this patch) to add a separate helper that does
> > the VGA switcheroo wrapping rather than have this in drm_get_edid()
> > where essentially every driver will go through the motions even if it
> > doesn't remotely support switcheroo.
>
> This is also something I carried over from Seth Forshee's and
> Dave Airlie's patches and I think their intention was precisely
> to do this in a centralized way in one of the generic functions,
> rather than having to modify each driver.
>
> For drivers without vga_switcheroo support, the additional cost
> amounts to one mutex_lock/unlock (because there's no handler
> registered and vga_switcheroo_lock_ddc bails out immediately
> if there's none).
>
> As an example why I think this approach was taken: Let's say that
> Tegra or other mobile SoCs have dual GPUs in the future, kind of
> like ARM big.LITTLE for graphics. We would potentially support
> those devices out of the box without having to modify the driver
> to call drm_get_edid_switcheroo() rather than drm_get_edid().
> That was kind of the idea I guess.
>
> On the other hand, a case could be made for your suggested approach
> as well: The proxying stuff will clutter drm_get_edid() and
> drm_dp_dpcd_read() with even more vga_switcheroo calls,
> as can be seen in experimental patch 20:
> http://lists.freedesktop.org/archives/dri-devel/2015-August/088154.html
>
> Also, to constrain proxying to eDP, I had to amend drm_dp_aux with
> a connector attribute so that the connector type can be checked in
> drm_dp_dpcd_read() (patch 19):
> http://lists.freedesktop.org/archives/dri-devel/2015-August/088172.html
>
> With your approach, I think this will be unnecessary because the
> functions in the drivers which would call drm_get_edid_switcheroo()
> would already know the connector type.
>
> So once again, a case can be made for either approach, I feel like
> I'm not in a position to properly judge this, and I kindly ask that
> you or another maintainer make that decision based on the additional
> information I've provided above. I'll then gladly adjust the patch.
Generally we make helpers opt-in and not opt-out since there's always the
oddball one out there which needs some additional code. The addition of
drm_do_get_edid is imo clear precedence that there's lots of fun things
going on when fetching edid. Hence I'm also voting for a separate helper
to do ddc switching.
That will also clear up the confusion with drm_dp_aux, adding a
drm_connector there feels wrong since not every dp_aux line has a
connector (e.g. for dp mst). If we can lift this relation out into drivers
(where this is known) that seems cleaner.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH v2.1 1/3] vga_switcheroo: Add support for switching only the DDC
On Fri, Aug 14, 2015 at 06:50:15PM +0200, Lukas Wunner wrote:
> Originally by Seth Forshee , 2012-10-04:
> During graphics driver initialization it's useful to be able to mux only
> the DDC to the inactive client in order to read the EDID. Add a
> switch_ddc callback to allow capable handlers to provide this
> functionality, and add vga_switcheroo_switch_ddc() to allow DRM to mux
> only the DDC.
>
> Modified by Dave Airlie , 2012-12-22:
> I can't figure out why I didn't like this, but I rewrote this [...] to
> lock/unlock the ddc lines [...]. I think I'd prefer something like that
> otherwise the interface got really ugly.
>
> Modified by Lukas Wunner , 2015-08-14:
> Don't grab vgasr_mutex in lock_ddc/unlock_ddc to avoid the following
> deadlock scenarios: (a) During switch (with vgasr_mutex already held),
> GPU is woken and probes its outputs, tries to re-acquire vgasr_mutex
> to lock DDC lines. (b) Likewise during switch, GPU is suspended and
> calls cancel_delayed_work_sync to stop output polling, if poll task
> is running at this moment we may wait forever for it to finish.
> If we don't grab vgasr_mutex the only bad thing that can happen is
> that the handler suddenly disappears. So block handler deregistration
> until DDC lines are unlocked again.
>
> WARN_ON_ONCE if unlock_ddc is called without calling lock_ddc first.
> Lock ddc_lock in stage2 to avoid race condition where reading the
> EDID and switching happens simultaneously. Switch DDC lines on
> MIGD / MDIS commands and on runtime suspend. Fix bug in stage2
> where no client had the active attribute set if switching failed.
> Fix erroneous interface documentation.
>
> If the inactive client registers before the active client then
> old_ddc_owner cannot be determined with find_active_client()
> (null pointer dereference). Therefore change semantics of the
> ->switch_ddc handler callback to return old_ddc_owner.
>
> v2.1: Overhaul locking, squash commits (requested by Daniel Vetter)
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
> Tested-by: Lukas Wunner
> [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
>
> Cc: Seth Forshee
> Cc: Dave Airlie
> Signed-off-by: Lukas Wunner
> ---
> drivers/gpu/vga/vga_switcheroo.c | 73
> +---
> include/linux/vga_switcheroo.h | 6
> 2 files changed, 74 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/vga/vga_switcheroo.c
> b/drivers/gpu/vga/vga_switcheroo.c
> index 37ac7b5..ac4ac12 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -9,12 +9,13 @@
>
> Switcher interface - methods require for ATPX and DCM
> - switchto - this throws the output MUX switch
> - - discrete_set_power - sets the power state for the discrete card
> + - switch_ddc - switch only DDC lines, return old DDC owner
> + - power_state - sets the power state for either GPU
>
> GPU driver interface
> - set_gpu_state - this should do the equiv of s/r for the card
> - this should *not* set the discrete power state
> - - switch_check - check if the device is in a position to switch now
> + - can_switch - check if the device is in a position to switch now
> */
Since you're just diggin around in switcheroo code it would be awesome to
write proper kerneldoc for all the functions exposed to drivers, move the
above to a proper kerneldoc for the vfunc tables (there's new support for
adding kerneldoc split up per member instead of one big pile squashed at
the top). And then bind it together with the overview sections with DOC
comments in Documentation/DocBook/drm.tmpl.
That would also help reviewing ;-)
>
> #include
> @@ -57,6 +58,8 @@ struct vgasr_priv {
> struct list_head clients;
>
> struct vga_switcheroo_handler *handler;
> + struct mutex ddc_lock;
> + int old_ddc_owner;
New locks imo need comments about what exactly they protect.
> };
>
> #define ID_BIT_AUDIO 0x100
> @@ -70,6 +73,7 @@ static void vga_switcheroo_debugfs_fini(struct vgasr_priv
> *priv);
> /* only one switcheroo per system */
> static struct vgasr_priv vgasr_priv = {
> .clients = LIST_HEAD_INIT(vgasr_priv.clients),
> + .ddc_lock = __MUTEX_INITIALIZER(vgasr_priv.ddc_lock),
> };
>
> static bool vga_switcheroo_ready(void)
> @@ -122,12 +126,14 @@ EXPORT_SYMBOL(vga_switcheroo_register_handler);
> void vga_switcheroo_unregister_handler(void)
> {
> mutex_lock(&vgasr_mutex);
> + mutex_lock(&vgasr_priv.ddc_lock);
> vgasr_priv.handler = NULL;
> if (vgasr_priv.active) {
> pr_info("vga_switcheroo: disabled\n");
> vga_switcheroo_debugfs_fini(&vgasr_priv);
> vgasr_priv.active = false;
> }
> + mutex_unlock(&vgasr_priv.ddc_lock);
> mutex_unlock(&vgasr_mutex);
> }
> EXPORT_SYMBOL(vga_switcheroo_unregister_handler);
> @@ -256,6 +262,43 @@ voi
[PATCH v2.1 2/3] apple-gmux: Add switch_ddc support
On Fri, Aug 14, 2015 at 06:18:55PM +0200, Lukas Wunner wrote:
> Originally by Seth Forshee , 2012-10-04:
> The gmux allows muxing the DDC independently from the display, so
> support this functionality. This will allow reading the EDID for the
> inactive GPU, fixing issues with machines that either don't have a VBT
> or have invalid mode data in the VBT.
>
> Modified by Lukas Wunner , 2015-03-27:
> If the inactive client registers before the active client then
> old_ddc_owner cannot be determined with find_active_client()
> (null pointer dereference). Therefore change semantics of the
> ->switch_ddc handler callback to return old_ddc_owner.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
> Tested-by: Pierre Moreau
> [MBP 5,3 2009 nvidia 9400M + 9600M GT pre-retina]
> Tested-by: Paul Hordiienko
> [MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
> Tested-by: William Brown
> [MBP 8,2 2011 intel SNB + amd turks pre-retina]
> Tested-by: Lukas Wunner
> [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
> Tested-by: Bruno Bierbaumer
> [MBP 11,3 2013 intel HSW + nvidia GK107 retina -- work in progress]
>
> Cc: Seth Forshee
> Signed-off-by: Lukas Wunner
> ---
> drivers/platform/x86/apple-gmux.c | 25 +++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/apple-gmux.c
> b/drivers/platform/x86/apple-gmux.c
> index 0dec3f5..08bdf1e 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -273,14 +273,34 @@ static const struct backlight_ops gmux_bl_ops = {
> .update_status = gmux_update_status,
> };
>
> +static int gmux_switch_ddc(enum vga_switcheroo_client_id id)
> +{
> + enum vga_switcheroo_client_id old_ddc_owner;
> +
> + if (gmux_read8(apple_gmux_data, GMUX_PORT_SWITCH_DDC) == 1)
> + old_ddc_owner = VGA_SWITCHEROO_IGD;
> + else
> + old_ddc_owner = VGA_SWITCHEROO_DIS;
> +
> + pr_debug("Switching gmux DDC from %d to %d\n", old_ddc_owner, id);
> +
> + if (id == old_ddc_owner)
> + return old_ddc_owner;
> +
> + if (id == VGA_SWITCHEROO_IGD)
> + gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1);
> + else
> + gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2);
> +
> + return old_ddc_owner;
> +}
> +
> static int gmux_switchto(enum vga_switcheroo_client_id id)
> {
> if (id == VGA_SWITCHEROO_IGD) {
> - gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1);
> gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DISPLAY, 2);
> gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 2);
> } else {
> - gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2);
> gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DISPLAY, 3);
> gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 3);
> }
> @@ -347,6 +367,7 @@ gmux_active_client(struct apple_gmux_data *gmux_data)
> }
If we'd required that switchto also switches ddc then the error handling
in patch 1 would be a lot simpler. switch_ddc would then only be for
temporary switching while probing.
-Daniel
>
> static struct vga_switcheroo_handler gmux_handler = {
> + .switch_ddc = gmux_switch_ddc,
> .switchto = gmux_switchto,
> .power_state = gmux_set_power_state,
> .get_client_id = gmux_get_client_id,
> --
> 2.1.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH v2 00/22] Enable gpu switching on the MacBook Pro
On Tue, Aug 25, 2015 at 09:36:47AM +0200, Lukas Wunner wrote: > Hi Dave, > > as requested I've pushed the MacBook Pro GPU switching stuff to GitHub > to ease browsing/reviewing, latest branch is gmux-v5: > https://github.com/l1k/linux/commits/gmux-v5 > > (I had applied for a freedesktop.org account in April with bug 89906 > but it's still pending, hence GitHub.) > > I've overhauled locking once more because previously all EDID reads > were serialized even on machines which don't use vga_switcheroo at all. > Seems it's impossible to fix this with mutexes and still be race-free > and deadlock-free, so I've changed vgasr_mutex to an rwlock: > https://github.com/l1k/linux/commit/ffa4546d85244a53541eed6799f93e8eea7f29e8 > > There are a number of vga_switcheroo functions added by Takashi Iwai > and yourself which don't lock anything at all, I believe this was in > part to avoid deadlocks (vga_switcheroo_runtime_resume_hdmi_audio locks > vgasr_mutex, wakes up the GPU with vga_switcheroo_runtime_resume > which locks vgasr_mutex once again). After changing vgasr_mutex to an > rwlock we can safely use locking in those functions as well: > https://github.com/l1k/linux/commit/cdea8f5c92039329dde47cadf20b67c7d9af0d62 > > With this change, on machines which don't use vga_switcheroo, the > overhead in drm_get_edid() is exactly 1 read_lock + 1 read_unlock > and EDID reads can happen in parallel. Thierry Reding and Alex Deucher > are in favor of adding a separate drm_get_edid_switcheroo() and changing > the relevant drivers (i915, nouveau, radeon, amdgpu) to use that in lieu > of drm_get_edid() so that drivers which don't use vga_switcheroo avoid > the lock_ddc/unlock_ddc calls. Performance-wise these additional calls > should be negligible, so I think the motivation can only be better > readability/clarity. One should also keep in mind that drivers which > don't use vga_switcheroo currently might do so in the future, e.g. > if mobile GPUs use a big.LITTLE concept like ARM CPUs already do. Just a quick aside: Switching to rwlocks to make lockdep happy is a fallacy - lockdep unfortunately doesn't accurately track read lock depencies. Which means very likely you didn't fix the locking inversions but just made lockdep shut up about them. Example: thread A grabs read-lock 1 and mutex 2. thread B grabs mutex 2 and write-lock 1. lockdep won't complain here, but if thread A has tries to get mutex 2 while thread B tries to write-lock 1 then there's an obvious deadlock. I'd highly suggest you try fixing the vga-switcheroo locking without resorting to rw lock primitives - that just means we need to manually prove the locking for many cases which is tons of work. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[Intel-gfx] [PATCH] drm/mm: Do DRM_MM_CREATE_TOP adj_start calculation after color_adjust
On Sun, Aug 16, 2015 at 08:52:54AM +0100, Chris Wilson wrote: > On Sun, Aug 16, 2015 at 04:02:28AM +0100, Michel Thierry wrote: > > The adj_start calculation for DRM_MM_CREATE_TOP should happen after > > mm->color_adjust. There was an inconsistency between > > drm_mm_insert_helper_range > > and drm_mm_insert_helper, as the later was already updating after > > color_adjust. > > > > Didn't spot it before, as color_adjust is only done in systems without > > LLC. But I'm not aware of anybody using this test case yet. > > > > Signed-off-by: Michel Thierry > > I sent this patch a few years ago, when I was using a top-down > allocator before llc and I have a similar patch in my tree which I kept > meaning to polish up and warn you about before PIN_HIGH. > > Reviewed-by: Chris Wilson Applied to drm-misc, thanks. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH] drm: make sure to check callbacks of ENCODER and CRTC helpers
On Sun, Aug 16, 2015 at 01:12:14PM +0900, Inki Dae wrote:
> This patch fixes NULL pointer access issues to ENCODER
> and CRTC drivers.
>
> Vendor specific DRM KMS drivers may not set their helper callbacks
> to ENCODER dn CRTC drivers. In this case, that will incur NULL
> pointer access when modeset is tried.
>
> So this patch makes sure to check if the callbacks are NULL or not.
>
> Signed-off-by: Inki Dae
Is there really a lot of value in tuning legacy crtc helpers? New drivers
should just use atomic (where we've made all these changes already),
drivers which are still transitioning can just carry around dummy
functions for now.
-Daniel
> ---
> drivers/gpu/drm/drm_crtc_helper.c | 44
> ++-
> 1 file changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c
> b/drivers/gpu/drm/drm_crtc_helper.c
> index ef53475..aa1f42f 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -318,16 +318,22 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> }
>
> encoder_funcs = encoder->helper_private;
> - if (!(ret = encoder_funcs->mode_fixup(encoder, mode,
> - adjusted_mode))) {
> - DRM_DEBUG_KMS("Encoder fixup failed\n");
> - goto done;
> + if (encoder_funcs->mode_fixup) {
> + ret = encoder_funcs->mode_fixup(encoder, mode,
> + adjusted_mode);
> + if (!ret) {
> + DRM_DEBUG_KMS("Encoder fixup failed\n");
> + goto done;
> + }
> }
> }
>
> - if (!(ret = crtc_funcs->mode_fixup(crtc, mode, adjusted_mode))) {
> - DRM_DEBUG_KMS("CRTC fixup failed\n");
> - goto done;
> + if (crtc_funcs->mode_fixup) {
> + ret = crtc_funcs->mode_fixup(crtc, mode, adjusted_mode);
> + if (!ret) {
> + DRM_DEBUG_KMS("CRTC fixup failed\n");
> + goto done;
> + }
> }
> DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
>
> @@ -343,21 +349,26 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
>
> encoder_funcs = encoder->helper_private;
> /* Disable the encoders as the first thing we do. */
> - encoder_funcs->prepare(encoder);
> + if (encoder_funcs->prepare)
> + encoder_funcs->prepare(encoder);
>
> drm_bridge_post_disable(encoder->bridge);
> }
>
> drm_crtc_prepare_encoders(dev);
>
> - crtc_funcs->prepare(crtc);
> + if (crtc_funcs->prepare)
> + crtc_funcs->prepare(crtc);
>
> /* Set up the DPLL and any encoders state that needs to adjust or depend
>* on the DPLL.
>*/
> - ret = !crtc_funcs->mode_set(crtc, mode, adjusted_mode, x, y, old_fb);
> - if (!ret)
> - goto done;
> + if (crtc_funcs->mode_set) {
> + ret = !crtc_funcs->mode_set(crtc, mode, adjusted_mode, x, y,
> + old_fb);
> + if (!ret)
> + goto done;
> + }
>
> drm_for_each_encoder(encoder, dev) {
>
> @@ -368,13 +379,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> encoder->base.id, encoder->name,
> mode->base.id, mode->name);
> encoder_funcs = encoder->helper_private;
> - encoder_funcs->mode_set(encoder, mode, adjusted_mode);
> + if (encoder_funcs->mode_set)
> + encoder_funcs->mode_set(encoder, mode, adjusted_mode);
>
> drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
> }
>
> /* Now enable the clocks, plane, pipe, and connectors that we set up. */
> - crtc_funcs->commit(crtc);
> + if (crtc_funcs->commit)
> + crtc_funcs->commit(crtc);
>
> drm_for_each_encoder(encoder, dev) {
>
> @@ -384,7 +397,8 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
> drm_bridge_pre_enable(encoder->bridge);
>
> encoder_funcs = encoder->helper_private;
> - encoder_funcs->commit(encoder);
> + if (encoder_funcs->commit)
> + encoder_funcs->commit(encoder);
>
> drm_bridge_enable(encoder->bridge);
> }
> --
> 1.9.1
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[RFC] Repurpose reserved field in vblank event to crtc_id
On Mon, Aug 17, 2015 at 04:21:22PM +0300, Ander Conselvan de Oliveira wrote: > Hi, > > Right now, if an atomic commit sends multiple vblank events it is not > possible to distiguish for each crtc each event is for in userspace. > This series changes that be repurposing the reserved field in struct > drm_event_vblank. > > Would this be a sane thing to do? Mostly just needs userspace since atm Weston has a per-crtc draw-loop and doesn't seem to care about events for big modesets. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH v2 3/7] drm/vc4: Add KMS support for Raspberry Pi.
On Tue, Aug 18, 2015 at 02:54:04PM -0700, Eric Anholt wrote: > This is the start of a full VC4 driver. Right now this just supports > configuring the display using a pre-existing video mode (because > changing the pixel clock isn't available yet, and doesn't work when it > is). However, this is enough for fbcon and bringing up X using > xf86-video-modesetting. > > Signed-off-by: Eric Anholt > --- > > v2: Drop FB_HELPER select thanks to Archit's patches. Do manual init > ordering instead of using the .load hook. Structure registration > more like tegra's, but still using the typical "component" code. > Drop no-op hooks for atomic_begin and mode_fixup() now that > they're optional. Drop sentinel in Makefile. Fix minor style > nits I noticed on another reread. fwiw Acked-by: Daniel Vetter since Eric has done all the changes I requested - simply forgot to add that in my earlier review. No full r-b since I don't have a clue about the hw (and didn't take a close look at the hw touching code hence). Cheers, Daniel > > drivers/gpu/drm/Kconfig | 2 + > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/vc4/Kconfig | 13 + > drivers/gpu/drm/vc4/Makefile | 17 + > drivers/gpu/drm/vc4/vc4_bo.c | 52 > drivers/gpu/drm/vc4/vc4_crtc.c| 565 ++ > drivers/gpu/drm/vc4/vc4_debugfs.c | 38 +++ > drivers/gpu/drm/vc4/vc4_drv.c | 271 > drivers/gpu/drm/vc4/vc4_drv.h | 120 > drivers/gpu/drm/vc4/vc4_hdmi.c| 633 > ++ > drivers/gpu/drm/vc4/vc4_hvs.c | 161 ++ > drivers/gpu/drm/vc4/vc4_kms.c | 84 + > drivers/gpu/drm/vc4/vc4_plane.c | 320 +++ > drivers/gpu/drm/vc4/vc4_regs.h| 562 + > 14 files changed, 2839 insertions(+) > create mode 100644 drivers/gpu/drm/vc4/Kconfig > create mode 100644 drivers/gpu/drm/vc4/Makefile > create mode 100644 drivers/gpu/drm/vc4/vc4_bo.c > create mode 100644 drivers/gpu/drm/vc4/vc4_crtc.c > create mode 100644 drivers/gpu/drm/vc4/vc4_debugfs.c > create mode 100644 drivers/gpu/drm/vc4/vc4_drv.c > create mode 100644 drivers/gpu/drm/vc4/vc4_drv.h > create mode 100644 drivers/gpu/drm/vc4/vc4_hdmi.c > create mode 100644 drivers/gpu/drm/vc4/vc4_hvs.c > create mode 100644 drivers/gpu/drm/vc4/vc4_kms.c > create mode 100644 drivers/gpu/drm/vc4/vc4_plane.c > create mode 100644 drivers/gpu/drm/vc4/vc4_regs.h > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 06ae500..19bb0db 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -260,3 +260,5 @@ source "drivers/gpu/drm/sti/Kconfig" > source "drivers/gpu/drm/amd/amdkfd/Kconfig" > > source "drivers/gpu/drm/imx/Kconfig" > + > +source "drivers/gpu/drm/vc4/Kconfig" > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index 8858510..283ed8e 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -42,6 +42,7 @@ obj-$(CONFIG_DRM_MGA) += mga/ > obj-$(CONFIG_DRM_I810) += i810/ > obj-$(CONFIG_DRM_I915) += i915/ > obj-$(CONFIG_DRM_MGAG200) += mgag200/ > +obj-$(CONFIG_DRM_VC4) += vc4/ > obj-$(CONFIG_DRM_CIRRUS_QEMU) += cirrus/ > obj-$(CONFIG_DRM_SIS) += sis/ > obj-$(CONFIG_DRM_SAVAGE)+= savage/ > diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig > new file mode 100644 > index 000..e810ef7 > --- /dev/null > +++ b/drivers/gpu/drm/vc4/Kconfig > @@ -0,0 +1,13 @@ > +config DRM_VC4 > + tristate "Broadcom VC4 Graphics" > + depends on ARCH_BCM2835 > + depends on DRM > + select DRM_KMS_HELPER > + select DRM_KMS_CMA_HELPER > + help > + Choose this option if you have a system that has a Broadcom > + VC4 GPU, such as the Raspberry Pi or other BCM2708/BCM2835. > + > + This driver requires that "avoid_warnings=2" be present in > + the config.txt for the firmware, to keep it from smashing > + our display setup. > diff --git a/drivers/gpu/drm/vc4/Makefile b/drivers/gpu/drm/vc4/Makefile > new file mode 100644 > index 000..32b4f9c > --- /dev/null > +++ b/drivers/gpu/drm/vc4/Makefile > @@ -0,0 +1,17 @@ > +ccflags-y := -Iinclude/drm > + > +# Please keep these build lists sorted! > + > +# core driver code > +vc4-y := \ > + vc4_bo.o \ > + vc4_crtc.o \ > + vc4_drv.o \ > + vc4_kms.o \ > + vc4_hdmi.o \ > + vc4_hvs.o \ > + vc4_plane.o > + > +vc4-$(CONFIG_DEBUG_FS) += vc4_debugfs.o > + > +obj-$(CONFIG_DRM_VC4) += vc4.o > diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c > new file mode 100644 > index 000..ab9f510 > --- /dev/null > +++ b/drivers/gpu/drm/vc4/vc4_bo.c > @@ -0,0 +1,52 @@ > +/* > + * Copyright © 2015 Broadcom > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * pub
[RFC 0/2] atomicify fbdev stuff
On Wed, Aug 19, 2015 at 06:45:15PM -0400, Rob Clark wrote: > So the issues that Inki and Bjorn ran into with ww_acquire_fini() vs > legacy fbdev codepaths (like restore_fbdev_mode() and pan_display()) > would be solved by using a single atomic update for these cases. So > I hacked up a prototype. > > (disclaimer, these are completely untested hacked-up-between-sessions- > at-conference patches.. but at least they compile ;-)) > > I think Daniel wanted something other than DRIVER_ATOMIC check, since > i915 currently supports atomic other than async. Other option would > be a separate DRIVER_ATOMIC_ASYNC flag (which has some advantage, in > that we could start using atomic modeset, but not atomic pageflip, > on i915.. and perhaps finally start work on atomic xrandr). Other > option would be to, for now, just introduce an fb_helper->use_atomic > flag, which driver could override, instead. Not a separate driver flag, but simply something to force fbdev helper to use atomic without DRIVER_ATOMIC. That way partially-converted drivers can switch things on their own. With that addressed and testing done this looks ready imo and I'll pull it in. > For now we should probably revert the ww_acquire_fini patch, and then > shoot for re-introducing it on top of these patches (hopefully working > by then) for 4.4. And maybe in the process we can fix some i915 multi- > monitor VT switch fail. Yeah, Dave already pushed the revert. -Daniel > > Rob Clark (2): > drm/fb-helper: atomic restore_fbdev_mode().. > drm/fb-helper: atomic pan_display().. > > drivers/gpu/drm/drm_atomic_helper.c | 129 -- > drivers/gpu/drm/drm_fb_helper.c | 133 > > include/drm/drm_atomic_helper.h | 6 ++ > 3 files changed, 215 insertions(+), 53 deletions(-) > > -- > 2.4.3 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH] drm: cleanup modesetting ioctls, one param per line
On Wed, Aug 19, 2015 at 08:34:45PM -0400, Alex Deucher wrote:
> On Wed, Aug 19, 2015 at 7:21 PM, Rob Clark wrote:
> > Since this already confused me once when adding addfb2.1, let's clean up
> > the header to split params one per line.
> >
> > Signed-off-by: Rob Clark
>
> Reviewed-by: Alex Deucher
Applied to drm-misc.
-Daniel
>
> > ---
> > include/uapi/drm/drm_mode.h | 42 ++
> > 1 file changed, 30 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 359107a..6c11ca4 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -105,8 +105,16 @@
> >
> > struct drm_mode_modeinfo {
> > __u32 clock;
> > - __u16 hdisplay, hsync_start, hsync_end, htotal, hskew;
> > - __u16 vdisplay, vsync_start, vsync_end, vtotal, vscan;
> > + __u16 hdisplay;
> > + __u16 hsync_start;
> > + __u16 hsync_end;
> > + __u16 htotal;
> > + __u16 hskew;
> > + __u16 vdisplay;
> > + __u16 vsync_start;
> > + __u16 vsync_end;
> > + __u16 vtotal;
> > + __u16 vscan;
> >
> > __u32 vrefresh;
> >
> > @@ -124,8 +132,10 @@ struct drm_mode_card_res {
> > __u32 count_crtcs;
> > __u32 count_connectors;
> > __u32 count_encoders;
> > - __u32 min_width, max_width;
> > - __u32 min_height, max_height;
> > + __u32 min_width;
> > + __u32 max_width;
> > + __u32 min_height;
> > + __u32 max_height;
> > };
> >
> > struct drm_mode_crtc {
> > @@ -135,7 +145,8 @@ struct drm_mode_crtc {
> > __u32 crtc_id; /**< Id */
> > __u32 fb_id; /**< Id of framebuffer */
> >
> > - __u32 x, y; /**< Position on the frameuffer */
> > + __u32 x; /**< x Position on the framebuffer */
> > + __u32 y; /**< y Position on the framebuffer */
> >
> > __u32 gamma_size;
> > __u32 mode_valid;
> > @@ -153,12 +164,16 @@ struct drm_mode_set_plane {
> > __u32 flags; /* see above flags */
> >
> > /* Signed dest location allows it to be partially off screen */
> > - __s32 crtc_x, crtc_y;
> > - __u32 crtc_w, crtc_h;
> > + __s32 crtc_x;
> > + __s32 crtc_y;
> > + __u32 crtc_w;
> > + __u32 crtc_h;
> >
> > /* Source values are 16.16 fixed point */
> > - __u32 src_x, src_y;
> > - __u32 src_h, src_w;
> > + __u32 src_x;
> > + __u32 src_y;
> > + __u32 src_h;
> > + __u32 src_w;
> > };
> >
> > struct drm_mode_get_plane {
> > @@ -244,7 +259,8 @@ struct drm_mode_get_connector {
> > __u32 connector_type_id;
> >
> > __u32 connection;
> > - __u32 mm_width, mm_height; /**< HxW in millimeters */
> > + __u32 mm_width; /**< width in millimeters */
> > + __u32 mm_height; /**< height in millimeters */
> > __u32 subpixel;
> >
> > __u32 pad;
> > @@ -327,7 +343,8 @@ struct drm_mode_get_blob {
> >
> > struct drm_mode_fb_cmd {
> > __u32 fb_id;
> > - __u32 width, height;
> > + __u32 width;
> > + __u32 height;
> > __u32 pitch;
> > __u32 bpp;
> > __u32 depth;
> > @@ -340,7 +357,8 @@ struct drm_mode_fb_cmd {
> >
> > struct drm_mode_fb_cmd2 {
> > __u32 fb_id;
> > - __u32 width, height;
> > + __u32 width;
> > + __u32 height;
> > __u32 pixel_format; /* fourcc code from drm_fourcc.h */
> > __u32 flags; /* see above flags */
> >
> > --
> > 2.4.3
> >
> > ___
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
about mmap dma-buf and sync
On Mon, Aug 24, 2015 at 04:52:13PM +0100, Daniel Stone wrote:
> Hi Thomas,
>
> On 24 August 2015 at 10:50, Thomas Hellstrom wrote:
> > In any case, Ideally I'd want the struct dma_buf_sync look something like
> >
> > enum dma_buf_sync_flags {
> > DMA_BUF_SYNC_READ = (1 << 0),
> > DMA_BUF_SYNC_WRITE = (2 << 0),
> > DMA_BUF_SYNC_RW = (3 << 0),
> > DMA_BUF_SYNC_START = (0 << 2),
> > DMA_BUF_SYNC_END = (1 << 2),
> >
> > DMA_BUF_SYNC_VALID_FLAGS_MASK = DMA_BUF_SYNC_RW |
> > DMA_BUF_SYNC_END
> > };
> >
> >
> > /* begin/end dma-buf functions used for userspace mmap. */
> > struct dma_buf_sync {
> > enum dma_buf_sync_flags flags;
> > __u64 x;
> > __u64 width;
> >
> > /* 2D Sync parameters */
> > __u64 stride;
> > __u64 y;
> > __u64 height;
> >
> > }
> >
> >
> > So again, why use a 2D sync in the first place when it's probably possible
> > to batch 1D syncs in the driver?
> > It all boils down to the number of syscalls required, and also the fact
> > that it's probably not trivial to batch sync calls in the read-write case.
> > One can, again, argue that there might be 3D cases or other obscure sync
> > cases,
> > but in that case, let's implement them in the future if the overhead
> > becomes unbearable. If the sync mechanism gets established as
> > mandatory, the problem will solve itself. And at least now we cover the
> > most common case and we don't need to add a new multiplexing
> > mechanism in the sync IOCTL since we already have one in the IOCTL
> > mechanism. We can simply add a new sync IOCTL for new obscure sync cases if
> > desired.
>
> I still don't think this ameliorates the need for batching: consider
> the case where you update two disjoint screen regions and want them
> both flushed. Either you issue two separate sync calls (which can be
> disadvantageous performance-wise on some hardware / setups), or you
> accumulate the regions and only flush later. So either two ioctls (one
> in the style of dirtyfb and one to perform the sync/flush; you can
> shortcut to assume the full buffer was damaged if the former is
> missing), or one like this:
>
> struct dma_buf_sync_2d {
> enum dma_buf_sync_flags flags;
>
> __u64 stride_bytes;
> __u32 bytes_per_pixel;
> __u32 num_regions;
>
> struct {
> __u64 x;
> __u64 y;
> __u64 width;
> __u64 height;
> } regions[];
> };
I really feel like any kind of multi-range flush interface is feature
bloat, and if we do it then we should only do it later on when there's a
clear need for it.
Afaiui dma-buf mmap will mostly be used for up/downloads, which means
there will be some explicit copy involved somewhere anyway. So similar to
userptr usage. And for those most often you just slurp in a linear block
and then let the blitter rearrange it, at least on i915.
Also thus far the consensus was that dma-buf doesn't care/know about
content layout, adding strides/bytes/whatever does bend this quite a bit.
And finally if we concentrate on the simple-case of one byte range we can
focus the discussion on correctness, which imo is the important bit here
really.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH] drm/doc: Fixing xml documentation warning
On Fri, Aug 21, 2015 at 04:46:14PM -0300, Danilo Cesar Lemes de Paula wrote:
> "/**" should be used for kernel-doc documentation only.
> It causes a warning with the new "in struct body" format.
>
> Signed-off-by: Danilo Cesar Lemes de Paula
> Cc: Randy Dunlap
> Cc: Daniel Vetter
> Cc: Laurent Pinchart
> Cc: Jonathan Corbet
> Cc: Herbert Xu
> Cc: Stephan Mueller
> Cc: Michal Marek
> Cc: linux-kernel at vger.kernel.org
> Cc: linux-doc at vger.kernel.org
> Cc: intel-gfx
> Cc: dri-devel
> Cc: Graham Whaley
Applied to drm-misc, thanks.
-Daniel
> ---
> include/drm/drm_modeset_lock.h | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
> index 70595ff..c16a3ec 100644
> --- a/include/drm/drm_modeset_lock.h
> +++ b/include/drm/drm_modeset_lock.h
> @@ -43,19 +43,19 @@ struct drm_modeset_acquire_ctx {
>
> struct ww_acquire_ctx ww_ctx;
>
> - /**
> + /*
>* Contended lock: if a lock is contended you should only call
>* drm_modeset_backoff() which drops locks and slow-locks the
>* contended lock.
>*/
> struct drm_modeset_lock *contended;
>
> - /**
> + /*
>* list of held locks (drm_modeset_lock)
>*/
> struct list_head locked;
>
> - /**
> + /*
>* Trylock mode, use only for panic handlers!
>*/
> bool trylock_only;
> @@ -70,12 +70,12 @@ struct drm_modeset_acquire_ctx {
> * Used for locking CRTCs and other modeset resources.
> */
> struct drm_modeset_lock {
> - /**
> + /*
>* modeset lock
>*/
> struct ww_mutex mutex;
>
> - /**
> + /*
>* Resources that are locked as part of an atomic update are added
>* to a list (so we know what to unlock at the end).
>*/
> --
> 2.4.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[Intel-gfx] [PATCH] doc: drm: Fix mis-spelling of i915_guc_submission includes
On Mon, Aug 24, 2015 at 02:41:21PM +0100, Graham Whaley wrote: > In commit > d1675198e: drm/i915: Integrate GuC-based command submission > > the drm.tmpl include lines reference the intel_guc_submission.c but the > patch adds the file i915_guc_submission.c. drm.tmpl fails to build with: > docproc: .//drivers/gpu/drm/i915/intel_guc_submission.c: > No such file or directory > > Change the file reference to the actual file. > > Signed-off-by: Graham Whaley Applied to drm-intel-next-queued, thanks. -Daniel > --- > This looks/feels like the right fix to me, but I am not intimate enough > with the APIs to verify for sure - some confirmation would be good. > > Documentation/DocBook/drm.tmpl | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl > index a01fca9..66bc646 100644 > --- a/Documentation/DocBook/drm.tmpl > +++ b/Documentation/DocBook/drm.tmpl > @@ -4160,8 +4160,8 @@ int num_ioctls; > > > GuC Client > -!Pdrivers/gpu/drm/i915/intel_guc_submission.c GuC-based command submissison > -!Idrivers/gpu/drm/i915/intel_guc_submission.c > +!Pdrivers/gpu/drm/i915/i915_guc_submission.c GuC-based command submissison > +!Idrivers/gpu/drm/i915/i915_guc_submission.c > > > > -- > 2.4.3 > > ___ > Intel-gfx mailing list > Intel-gfx at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
about mmap dma-buf and sync
On 08/25/2015 11:02 AM, Daniel Vetter wrote:
> On Mon, Aug 24, 2015 at 04:52:13PM +0100, Daniel Stone wrote:
>> Hi Thomas,
>>
>> On 24 August 2015 at 10:50, Thomas Hellstrom wrote:
>>> In any case, Ideally I'd want the struct dma_buf_sync look something like
>>>
>>> enum dma_buf_sync_flags {
>>> DMA_BUF_SYNC_READ = (1 << 0),
>>> DMA_BUF_SYNC_WRITE = (2 << 0),
>>> DMA_BUF_SYNC_RW = (3 << 0),
>>> DMA_BUF_SYNC_START = (0 << 2),
>>> DMA_BUF_SYNC_END = (1 << 2),
>>>
>>> DMA_BUF_SYNC_VALID_FLAGS_MASK = DMA_BUF_SYNC_RW |
>>> DMA_BUF_SYNC_END
>>> };
>>>
>>>
>>> /* begin/end dma-buf functions used for userspace mmap. */
>>> struct dma_buf_sync {
>>> enum dma_buf_sync_flags flags;
>>> __u64 x;
>>> __u64 width;
>>>
>>> /* 2D Sync parameters */
>>> __u64 stride;
>>> __u64 y;
>>> __u64 height;
>>>
>>> }
>>>
>>>
>>> So again, why use a 2D sync in the first place when it's probably possible
>>> to batch 1D syncs in the driver?
>>> It all boils down to the number of syscalls required, and also the fact
>>> that it's probably not trivial to batch sync calls in the read-write case.
>>> One can, again, argue that there might be 3D cases or other obscure sync
>>> cases,
>>> but in that case, let's implement them in the future if the overhead
>>> becomes unbearable. If the sync mechanism gets established as
>>> mandatory, the problem will solve itself. And at least now we cover the
>>> most common case and we don't need to add a new multiplexing
>>> mechanism in the sync IOCTL since we already have one in the IOCTL
>>> mechanism. We can simply add a new sync IOCTL for new obscure sync cases if
>>> desired.
>> I still don't think this ameliorates the need for batching: consider
>> the case where you update two disjoint screen regions and want them
>> both flushed. Either you issue two separate sync calls (which can be
>> disadvantageous performance-wise on some hardware / setups), or you
>> accumulate the regions and only flush later. So either two ioctls (one
>> in the style of dirtyfb and one to perform the sync/flush; you can
>> shortcut to assume the full buffer was damaged if the former is
>> missing), or one like this:
>>
>> struct dma_buf_sync_2d {
>> enum dma_buf_sync_flags flags;
>>
>> __u64 stride_bytes;
>> __u32 bytes_per_pixel;
>> __u32 num_regions;
>>
>> struct {
>> __u64 x;
>> __u64 y;
>> __u64 width;
>> __u64 height;
>> } regions[];
>> };
> I really feel like any kind of multi-range flush interface is feature
> bloat, and if we do it then we should only do it later on when there's a
> clear need for it.
IMO all the use-cases so far that wanted to do this have been 2D
updates. and having only a 1D sync will most probably scare people away
from this interface.
> Afaiui dma-buf mmap will mostly be used for up/downloads, which means
> there will be some explicit copy involved somewhere anyway. So similar to
> userptr usage. And for those most often you just slurp in a linear block
> and then let the blitter rearrange it, at least on i915.
>
> Also thus far the consensus was that dma-buf doesn't care/know about
> content layout, adding strides/bytes/whatever does bend this quite a bit.
I think with a 2D interface, the stride only applies to the sync
operation itself and is only a parameter for that sync operation.
Whether we should have multiple regions or not is not a big deal for me,
but I think at least a 2D sync is crucial.
>
> And finally if we concentrate on the simple-case of one byte range we can
> focus the discussion on correctness, which imo is the important bit here
> really.
If we only focus on correctness there are other ways to do this without
a sync: We can DMA based on accessed and dirty pages, but performance
will crawl, so IMO the sync interface is purely for performance...
> -Daniel
Thanks,
Thomas
[PATCH] drm/crtc: Add a helper func to get a registered crtc from its index
On Tue, Aug 25, 2015 at 11:13:51AM +0800, Xinliang Liu wrote:
> This patch add a helper func to get a registered crtc from its index.
> In some case, where we know the crtc's index and we want to know the
> crtc too.
>
> For example, the enable_vblank func of struct drm_driver:
> In the implementation of this func, we know the index of the crtc but
> we want to know the crtc. This helper func can get the crtc easily.
> A sample impelmentation of enable_vblank is as shown as bellow:
>
> int hisi_drm_crtc_enable_vblank(struct drm_device *dev, int c)
> {
> struct drm_crtc *crtc = drm_get_crtc_from_index(dev, c);
> struct hisi_crtc *hcrtc = to_hisi_crtc(crtc);
> struct hisi_crtc_ops *ops = hcrtc->ops;
> int ret = 0;
>
> if (ops->enable_vblank)
> ret = ops->enable_vblank(hcrtc);
>
> return ret;
> }
>
> Signed-off-by: Xinliang Liu
Yeah unfortunately drm_irq.c is still stick in the old pre-KMS days. I
think we should go a bit further here though to allow new drivers to be
completely free of int pipe:
- add a new array pointer dev->mode_conifg.crtc_arr, which is
(re-)allocated in drm_crtc_init_with_planes. Then a pipe->crtc lookup
will be just
crtc = dev->mode_config.crtc_arr[pipe];
- add new hooks for vblank handling int drm_crtc_helper_funcs for
enable_vblanke, disable_vblank, get_vblank_timestamp and get_scanoutpos.
Ofc also anotate the docs for the existing hooks and make it clear new
drivers should use the new ones. Ofc these new hooks should directly
take a struct drm_crtc * instead of inte pipe.
- change the code in drm_irq.c to wrap all callbacks and first check
whether the new ones are there and only if that's not the case call the
old ones.
With these changes drivers can be completely free of int pipe and use
struct drm_crtc exclusivly I think, and the mess would be fully restricted
to drm_irq.c.
Cheers, Daniel
> ---
> drivers/gpu/drm/drm_crtc.c | 25 +
> include/drm/drm_crtc.h | 2 ++
> 2 files changed, 27 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index b9ba061..8764765 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -747,6 +747,31 @@ unsigned int drm_crtc_index(struct drm_crtc *crtc)
> }
> EXPORT_SYMBOL(drm_crtc_index);
>
> +/**
> + * drm_get_crtc_from_index - find a registered CRTC from the index
> + * @dev: DRM device
> + * @index: index of a registered CRTC
> + *
> + * Given a index, return the registered CRTC within a DRM
> + * device's list of CRTCs.
> + */
> +struct drm_crtc *drm_get_crtc_from_index(struct drm_device *dev,
> + unsigned int index)
> +{
> + unsigned int index_tmp = 0;
> + struct drm_crtc *crtc;
> +
> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> + if (index_tmp == index)
> + return crtc;
> +
> + index_tmp++;
> + }
> +
> + BUG();
> +}
> +EXPORT_SYMBOL(drm_get_crtc_from_index);
> +
> /*
> * drm_mode_remove - remove and free a mode
> * @connector: connector list to modify
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 57ca8cc..3a46d39d 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1225,6 +1225,8 @@ extern int drm_crtc_init_with_planes(struct drm_device
> *dev,
>const struct drm_crtc_funcs *funcs);
> extern void drm_crtc_cleanup(struct drm_crtc *crtc);
> extern unsigned int drm_crtc_index(struct drm_crtc *crtc);
> +extern struct drm_crtc *drm_get_crtc_from_index(struct drm_device *dev,
> + unsigned int index);
>
> /**
> * drm_crtc_mask - find the mask of a registered CRTC
> --
> 1.9.1
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[Bug 91752] Segmentation fault with specific shader (r600)
https://bugs.freedesktop.org/show_bug.cgi?id=91752 Bug ID: 91752 Summary: Segmentation fault with specific shader (r600) Product: Mesa Version: 10.6 Hardware: Other OS: All Status: NEW Severity: normal Priority: medium Component: Drivers/Gallium/r600 Assignee: dri-devel at lists.freedesktop.org Reporter: jonas at wielicki.name QA Contact: dri-devel at lists.freedesktop.org The given apitrace [1] results in a segmentation fault. I am testing this on a sony notebook with Intel sandybridge integrated GPU and a dedicated radeon. 1) $ xrandr --setprovideroffloadsink radeon Intel 2) $ DRI_PRIME=1 apitrace replay r600-crash-01.trace Actual Results: Segmentation Fault Expected Results: The rendering should work correctly. I could reproduce this with both the mesa-libGL 10.6.3 as shipped by fedora as well as mesa 11.0.0-rc1 freshly compiled with: ./configure --enable-selinux --enable-osmesa --enable-egl --disable-gles1 --enable-gles2 --enable-shared-glapi --enable-gbm --enable-glx-tls --enable-texture-float=yes --enable-gallium-llvm --enable-llvm-shared-libs --enable-dri --enable-xa --enable-nine (flags blindly stolen from the Fedora package specification) intel GPU: 00:02.0 VGA compatible controller: Intel Corporation 2nd Generation Core Processor Family Integrated Graphics Controller (rev 09) radeon GPU: 01:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Whistler [Radeon HD 6630M/6650M/6750M/7670M/7690M] (rev ff) The same trace runs without crash on the Intel. [1]: http://sotecware.net/files/persistent/fdo-mesa-r600-bugs/r600-crash-01.trace -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/0e7b6b3c/attachment.html>
[Bug 91752] Segmentation fault with specific shader (r600)
https://bugs.freedesktop.org/show_bug.cgi?id=91752 Jonas Wielicki changed: What|Removed |Added Hardware|Other |x86-64 (AMD64) OS|All |Linux (All) -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/3fbcf668/attachment.html>
[alsa-devel] [PATCH V5 3/3] ASoC: AMD: add AMD ASoC ACP-I2S driver
On Tue, Aug 25, 2015 at 11:36 AM, Mark Brown wrote: > On Mon, Aug 24, 2015 at 04:08:31PM -0400, Alex Deucher wrote: >> On Fri, Aug 21, 2015 at 12:17 PM, Mark Brown wrote: > >> > What I'm looking for is actual code sharing where we use the same code >> > for the I2S controller block or a clear and documented understanding of >> > why it is not possible to share things. > >> Maruthi can clarify further, but it's not possible to use the >> designware driver directly because: >> 1. The i2s registers are within the same MMIO aperture as our other >> GPU registers. Our GPU driver is designed in such a way that the >> specific IP modules don't have direct access to the MMIO aperture. >> They use functions provided by the core driver to access registers. >> Thus the ACP IP module within the GPU driver does not have direct >> access to the mmio base pointer in order to pass it on. > > Please explain this in more detail, shared register ranges are very > common and are the sort of things MFDs are supposed to help with. > In our case, ACP I2S driver need not do a 'devm_ioremap_resource' to get mmio base. ACP audio IP (DMA + I2S+ Others) registers can be accessed, using GPU's MMIO base. During GPU driver design, it was decided that all the register access for entire GPU MMIO aperture (includes ACP and others) to be done in GPU module only. This is implemented in another patch in this patch series using a abstraction layer. A set of functions were defined for audio DMA and I2S functionality within GPU driver module and those function pointers were passed as platform data to ALSA PCM driver to handle both DMA and I2S. >> 2. The designware driver depends on the CLKDEV framework which we >> don't currently support. > > You need to support the clock API, it's very easy to do so so there is > no excuse for doing something custom here. > Codec acts as master in our case to provide clock to i2s controller and there wasn't a need to use clock APIs unlike in existing designware i2s driver. There is no custom implementation. >> 3. Our hardware does not support S16_LE > > If you have modified the designware IP to remove this support (why would > anyone do that?) it's a trivial quirk, if the restriction comes from > some other part of the system like the DMA driver then the constraint > will come from that part of the system. There is a bug in ACP SoC implementation (which combines internal DMA, designware I2S and other blocks) for 16bit and lower resolution. I felt , it would be better to limit functionality in I2S DAI capabilities. I will put this limitation in DMA driver capabilities, to represent overall sound card capabilities, if you suggest.
[Intel-gfx] [PATCH 1/5] Documentation: drm: Fix pdfdocs sect/title tags
On Tue, Aug 25, 2015 at 10:26:41AM +0100, Graham Whaley wrote: > Building pdfdocs shows errors with !includes and s such as: > jade:/Documentation/DocBook/drm.xml:666:11:E: document type does not > allow element "para" here; missing one of "glossary", "bibliography", > "index" start-tag > Fix by adding items and add/shuffle , and !include > items. > > Signed-off-by: Graham Whaley The idea behind having both the overview sections and the detailed function references in the same section is to have them in the same section. Is there nothing we can do to salvage that? At least you seem to add a lot more sections here, but it doesn't look entirely consistent. -Daniel > --- > Documentation/DocBook/drm.tmpl | 32 ++-- > 1 file changed, 22 insertions(+), 10 deletions(-) > > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl > index 66bc646..586f1b8 100644 > --- a/Documentation/DocBook/drm.tmpl > +++ b/Documentation/DocBook/drm.tmpl > @@ -303,6 +303,9 @@ char *date; > > !Edrivers/gpu/drm/drm_pci.c > !Edrivers/gpu/drm/drm_platform.c > + > + > + Calling Registration Directly > > New drivers that no longer rely on the services provided by the > drm_bus structure can call the low-level > @@ -2352,7 +2355,7 @@ void intel_crt_init(struct drm_device *dev) > > > > - Atomic Modeset Helper Functions Reference > + Atomic Modeset Helper Functions Introduction > > Overview > !Pdrivers/gpu/drm/drm_atomic_helper.c overview > @@ -2365,14 +2368,20 @@ void intel_crt_init(struct drm_device *dev) > Atomic State Reset and Initialization > !Pdrivers/gpu/drm/drm_atomic_helper.c atomic state reset and initialization > > + > + > + Atomic Modeset Helper Functions Reference > !Iinclude/drm/drm_atomic_helper.h > !Edrivers/gpu/drm/drm_atomic_helper.c > > > + Modeset Helper Functions Introduction > +!Pdrivers/gpu/drm/drm_crtc_helper.c overview > + > + >Modeset Helper Functions Reference > !Iinclude/drm/drm_crtc_helper.h > !Edrivers/gpu/drm/drm_crtc_helper.c > -!Pdrivers/gpu/drm/drm_crtc_helper.c overview > > >Output Probing Helper Functions Reference > @@ -2432,8 +2441,8 @@ void intel_crt_init(struct drm_device *dev) > > >Plane Helper Reference > -!Edrivers/gpu/drm/drm_plane_helper.c > !Pdrivers/gpu/drm/drm_plane_helper.c overview > +!Edrivers/gpu/drm/drm_plane_helper.c > > > Tile group > @@ -2449,6 +2458,9 @@ void intel_crt_init(struct drm_device *dev) >Default bridge callback sequence > !Pdrivers/gpu/drm/drm_bridge.c bridge callbacks > > + > + > + Bridges Function Reference > !Edrivers/gpu/drm/drm_bridge.c > > > @@ -4114,19 +4126,19 @@ int num_ioctls; > > GTT Fences and Swizzling > !Idrivers/gpu/drm/i915/i915_gem_fence.c > - > - Global GTT Fence Handling > + > + > +Global GTT Fence Handling > !Pdrivers/gpu/drm/i915/i915_gem_fence.c fence register handling > - > - > - Hardware Tiling and Swizzling Details > + > + > +Hardware Tiling and Swizzling Details > !Pdrivers/gpu/drm/i915/i915_gem_fence.c tiling swizzling details > - > > > Object Tiling IOCTLs > -!Idrivers/gpu/drm/i915/i915_gem_tiling.c > !Pdrivers/gpu/drm/i915/i915_gem_tiling.c buffer object tiling > +!Idrivers/gpu/drm/i915/i915_gem_tiling.c > > > Buffer Object Eviction > -- > 2.4.3 > > ___ > Intel-gfx mailing list > Intel-gfx at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[Bug 91753] r300g: XXX
https://bugs.freedesktop.org/show_bug.cgi?id=91753 Bug ID: 91753 Summary: r300g: XXX Product: Mesa Version: git Hardware: Other OS: All Status: NEW Severity: normal Priority: medium Component: Drivers/Gallium/r300 Assignee: dri-devel at lists.freedesktop.org Reporter: stefandoesinger at gmx.at QA Contact: dri-devel at lists.freedesktop.org When trying to run any GL application that renders something with r300g (e.g. glxgears) fails with the message "radeon: command stream overflowed". This is a regression caused by the following commit: 567394112d904096abff1d994ab952f475dfb444 is the first bad commit commit 567394112d904096abff1d994ab952f475dfb444 Author: Marek Olšák Date: Fri Jul 31 11:45:13 2015 +0200 radeon/winsys: increase the IB size for VM Luckily, there is a kernel query, so use the size from that. It currently returns 256KB. It can be increased in the kernel. Reviewed-by: Michel Dänzer I am running Linux 4.1.6 (+ some Gentoo patches) and libdrm 2.4.64. The X.org server version is 1.16.4 and the DDX driver is xf86-video-ati-7.5.0. -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/3deccce9/attachment.html>
[Bug 91753] r300g: command stream overflowed error since 567394112d904096abff1d994ab952f475dfb444
https://bugs.freedesktop.org/show_bug.cgi?id=91753 Stefan Dösinger changed: What|Removed |Added Summary|r300g: XXX |r300g: command stream ||overflowed error since ||567394112d904096abff1d994ab ||952f475dfb444 -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/119092d5/attachment.html>
[Bug 91753] r300g: command stream overflowed error since 567394112d904096abff1d994ab952f475dfb444
https://bugs.freedesktop.org/show_bug.cgi?id=91753 Emil Velikov changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #1 from Emil Velikov --- This commit has already been reverted and will be part of 11.0.0-rc2. commit a83c36b5c0c64c717ced76db89bab96648aa Author: Marek Olšák Date: Sun Aug 23 18:57:44 2015 +0200 Revert "radeon/winsys: increase the IB size for VM" This reverts commit 567394112d904096abff1d994ab952f475dfb444. It regressed performance. It looks like smaller IBs are better, because the GPU goes idle quicker and there is less waiting for buffers and fences. Cc: 11.0 -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/fc8fc7ce/attachment.html>
[Bug 91753] r300g: command stream overflowed error since 567394112d904096abff1d994ab952f475dfb444
https://bugs.freedesktop.org/show_bug.cgi?id=91753 --- Comment #2 from Stefan Dösinger --- Oh, sorry for the spam then. I bisected this on Sunday and forgot to file an actual bug report until now. I'm comping the current git code at the moment to re-test. -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/5fc67033/attachment.html>
[PATCH] drm/doc: Fixing xml documentation warning
On Fri, Aug 21, 2015 at 04:46:14PM -0300, Danilo Cesar Lemes de Paula wrote:
> "/**" should be used for kernel-doc documentation only.
> It causes a warning with the new "in struct body" format.
>
> Signed-off-by: Danilo Cesar Lemes de Paula
> Cc: Randy Dunlap
> Cc: Daniel Vetter
> Cc: Laurent Pinchart
> Cc: Jonathan Corbet
> Cc: Herbert Xu
> Cc: Stephan Mueller
> Cc: Michal Marek
> Cc: linux-kernel at vger.kernel.org
> Cc: linux-doc at vger.kernel.org
> Cc: intel-gfx
> Cc: dri-devel
> Cc: Graham Whaley
> ---
> include/drm/drm_modeset_lock.h | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
> index 70595ff..c16a3ec 100644
> --- a/include/drm/drm_modeset_lock.h
> +++ b/include/drm/drm_modeset_lock.h
> @@ -43,19 +43,19 @@ struct drm_modeset_acquire_ctx {
>
> struct ww_acquire_ctx ww_ctx;
>
> - /**
> + /*
>* Contended lock: if a lock is contended you should only call
>* drm_modeset_backoff() which drops locks and slow-locks the
>* contended lock.
>*/
> struct drm_modeset_lock *contended;
On second thought this is perfect use-case for the new per-member
kerneldoc support we've recently added.
>
> - /**
> + /*
>* list of held locks (drm_modeset_lock)
>*/
> struct list_head locked;
>
> - /**
> + /*
>* Trylock mode, use only for panic handlers!
>*/
> bool trylock_only;
> @@ -70,12 +70,12 @@ struct drm_modeset_acquire_ctx {
> * Used for locking CRTCs and other modeset resources.
> */
> struct drm_modeset_lock {
> - /**
> + /*
>* modeset lock
>*/
Same here.
-Daniel
> struct ww_mutex mutex;
>
> - /**
> + /*
>* Resources that are locked as part of an atomic update are added
>* to a list (so we know what to unlock at the end).
>*/
> --
> 2.4.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH 1/4] drm: Make drm_fb_unregister/remove accept NULL fb
These functions are used by drivers to release fbdev emulation
buffers. We need to make them resilient to NULL pointers to
make the fbdev compile/runtime knobs not cause Oopses on module
unload.
Cc: Archit Taneja
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_crtc.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 33d877c65ced..884690c81094 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -538,7 +538,12 @@ EXPORT_SYMBOL(drm_framebuffer_reference);
*/
void drm_framebuffer_unregister_private(struct drm_framebuffer *fb)
{
- struct drm_device *dev = fb->dev;
+ struct drm_device *dev;
+
+ if (!fb)
+ return;
+
+ dev = fb->dev;
mutex_lock(&dev->mode_config.fb_lock);
/* Mark fb as reaped and drop idr ref. */
@@ -589,12 +594,17 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup);
*/
void drm_framebuffer_remove(struct drm_framebuffer *fb)
{
- struct drm_device *dev = fb->dev;
+ struct drm_device *dev;
struct drm_crtc *crtc;
struct drm_plane *plane;
struct drm_mode_set set;
int ret;
+ if (!fb)
+ return;
+
+ dev = fb->dev;
+
WARN_ON(!list_empty(&fb->filp_head));
/*
--
1.8.3.1
[PATCH 2/4] drm/fb-helper: Use -errno return in restore_mode_unlocked
Using bool and returning true upon error is very uncommon. Also an int
return value is actually what all the callers which did check it seem
to have expected.
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_fb_helper.c | 19 +++
include/drm/drm_fb_helper.h | 6 +++---
2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 418d299f3b12..83aacb0cc9df 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -320,11 +320,10 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
}
EXPORT_SYMBOL(drm_fb_helper_debug_leave);
-static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
+static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
{
struct drm_device *dev = fb_helper->dev;
struct drm_plane *plane;
- bool error = false;
int i;
drm_warn_on_modeset_not_all_locked(dev);
@@ -348,14 +347,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper
*fb_helper)
if (crtc->funcs->cursor_set) {
ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0);
if (ret)
- error = true;
+ return ret;
}
ret = drm_mode_set_config_internal(mode_set);
if (ret)
- error = true;
+ return ret;
}
- return error;
+
+ return 0;
}
/**
@@ -365,12 +365,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper
*fb_helper)
* This should be called from driver's drm ->lastclose callback
* when implementing an fbcon on top of kms using this helper. This ensures
that
* the user isn't greeted with a black screen when e.g. X dies.
+ *
+ * RETURNS:
+ * Zero if everything went ok, negative error code otherwise.
*/
-bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
+int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
{
struct drm_device *dev = fb_helper->dev;
- bool ret;
- bool do_delayed = false;
+ bool do_delayed
+ int ret;
drm_modeset_lock_all(dev);
ret = restore_fbdev_mode(fb_helper);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index dbab4622b58f..67de1f10008e 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -136,7 +136,7 @@ int drm_fb_helper_set_par(struct fb_info *info);
int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
struct fb_info *info);
-bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
*fb_helper);
+int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper);
struct fb_info *drm_fb_helper_alloc_fbi(struct drm_fb_helper *fb_helper);
void drm_fb_helper_unregister_fbi(struct drm_fb_helper *fb_helper);
@@ -226,10 +226,10 @@ static inline int drm_fb_helper_check_var(struct
fb_var_screeninfo *var,
return 0;
}
-static inline bool
+static inline int
drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
{
- return true;
+ return 0;
}
static inline struct fb_info *
--
1.8.3.1
[PATCH 3/4] drm/fb-helper: Add module option to disable fbdev emulation
Faster than recompiling.
Note that restore_fbdev_mode_unlocked is a bit special and the only
one which returns an error code when fbdev isn't there - i915 needs
that one to not fall over with some additional fbcon related restore
code. Everyone else just ignores the return value or only prints a
DRM_DEBUG level message.
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_fb_helper.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 83aacb0cc9df..8259dec1de1f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -39,6 +39,11 @@
#include
#include
+static bool drm_fbdev_emulation = true;
+module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
+MODULE_PARM_DESC(fbdev_emulation,
+"Enable legacy fbdev emulation [default=true]");
+
static LIST_HEAD(kernel_fb_helper_list);
/**
@@ -99,6 +104,9 @@ int drm_fb_helper_single_add_all_connectors(struct
drm_fb_helper *fb_helper)
struct drm_connector *connector;
int i;
+ if (!drm_fbdev_emulation)
+ return 0;
+
mutex_lock(&dev->mode_config.mutex);
drm_for_each_connector(connector, dev) {
struct drm_fb_helper_connector *fb_helper_connector;
@@ -129,6 +137,9 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper
*fb_helper, struct drm_
struct drm_fb_helper_connector **temp;
struct drm_fb_helper_connector *fb_helper_connector;
+ if (!drm_fbdev_emulation)
+ return 0;
+
WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
if (fb_helper->connector_count + 1 >
fb_helper->connector_info_alloc_count) {
temp = krealloc(fb_helper->connector_info, sizeof(struct
drm_fb_helper_connector *) * (fb_helper->connector_count + 1), GFP_KERNEL);
@@ -184,6 +195,9 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper
*fb_helper,
struct drm_fb_helper_connector *fb_helper_connector;
int i, j;
+ if (!drm_fbdev_emulation)
+ return 0;
+
WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
for (i = 0; i < fb_helper->connector_count; i++) {
@@ -375,6 +389,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct
drm_fb_helper *fb_helper)
bool do_delayed
int ret;
+ if (!drm_fbdev_emulation)
+ return -ENODEV;
+
drm_modeset_lock_all(dev);
ret = restore_fbdev_mode(fb_helper);
@@ -591,6 +608,9 @@ int drm_fb_helper_init(struct drm_device *dev,
struct drm_crtc *crtc;
int i;
+ if (!drm_fbdev_emulation)
+ return 0;
+
if (!max_conn_count)
return -EINVAL;
@@ -713,6 +733,9 @@ EXPORT_SYMBOL(drm_fb_helper_release_fbi);
void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
{
+ if (!drm_fbdev_emulation)
+ return;
+
if (!list_empty(&fb_helper->kernel_fb_list)) {
list_del(&fb_helper->kernel_fb_list);
if (list_empty(&kernel_fb_helper_list)) {
@@ -1933,6 +1956,9 @@ int drm_fb_helper_initial_config(struct drm_fb_helper
*fb_helper, int bpp_sel)
struct drm_device *dev = fb_helper->dev;
int count = 0;
+ if (!drm_fbdev_emulation)
+ return 0;
+
mutex_lock(&dev->mode_config.mutex);
count = drm_fb_helper_probe_connector_modes(fb_helper,
dev->mode_config.max_width,
@@ -1976,6 +2002,9 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper
*fb_helper)
struct drm_device *dev = fb_helper->dev;
u32 max_width, max_height;
+ if (!drm_fbdev_emulation)
+ return 0;
+
mutex_lock(&fb_helper->dev->mode_config.mutex);
if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
fb_helper->delayed_hotplug = true;
--
1.8.3.1
[PATCH 4/4] fbdev: Debug knob to register without holding console_lock
When the usual fbcon legacy options are enabled we have
->register_framebuffer
->fb notifier chain calls into fbcon
->fbcon sets up console on new fbi
->fbi->set_par
->drm_fb_helper_set_par exercises full kms api
And because of locking inversion hilarity all of register_framebuffer
is done with the console lock held. Which means that the first time on
driver load we exercise _all_ the kms code (all probe paths and
modeset paths for everything connected) is under the console lock.
That means if anything goes belly-up in that big pile of code nothing
ever reaches logfiles (and the machine is dead).
Usual tactic to debug that is to temporarily remove those console_lock
calls to be able to capture backtraces. I'm fed up writing this patch
and recompiling kernels. Hence this patch here to add an unsafe,
kernel-taining option to do this at runtime.
Cc: Jean-Christophe Plagniol-Villard
Cc: Tomi Valkeinen
Cc: linux-fbdev at vger.kernel.org
Signed-off-by: Daniel Vetter
---
drivers/video/fbdev/core/fbmem.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 0705d8883ede..4e73b6f6b1c0 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1608,6 +1608,11 @@ static int do_remove_conflicting_framebuffers(struct
apertures_struct *a,
return 0;
}
+static bool lockless_register_fb;
+module_param_named_unsafe(lockless_register_fb, lockless_register_fb, bool,
0400);
+MODULE_PARM_DESC(lockless_register_fb,
+ "Lockless framebuffer registration for debugging [default=off]");
+
static int do_register_framebuffer(struct fb_info *fb_info)
{
int i, ret;
@@ -1675,15 +1680,18 @@ static int do_register_framebuffer(struct fb_info
*fb_info)
registered_fb[i] = fb_info;
event.info = fb_info;
- console_lock();
+ if (!lockless_register_fb)
+ console_lock();
if (!lock_fb_info(fb_info)) {
- console_unlock();
+ if (!lockless_register_fb)
+ console_unlock();
return -ENODEV;
}
fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
unlock_fb_info(fb_info);
- console_unlock();
+ if (!lockless_register_fb)
+ console_unlock();
return 0;
}
--
1.8.3.1
[PATCH] drm/atomic-helper: properly annotate functions in kerneldoc
Without the () the markup and more important hyperlinking wont happen. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_atomic_helper.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index bc9c94e4cbaa..904acf236f36 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -42,14 +42,14 @@ * add their own additional internal state. * * This library also provides default implementations for the check callback in - * drm_atomic_helper_check and for the commit callback with - * drm_atomic_helper_commit. But the individual stages and callbacks are expose + * drm_atomic_helper_check() and for the commit callback with + * drm_atomic_helper_commit(). But the individual stages and callbacks are expose * to allow drivers to mix and match and e.g. use the plane helpers only * together with a driver private modeset implementation. * * This library also provides implementations for all the legacy driver - * interfaces on top of the atomic interface. See drm_atomic_helper_set_config, - * drm_atomic_helper_disable_plane, drm_atomic_helper_disable_plane and the + * interfaces on top of the atomic interface. See drm_atomic_helper_set_config(), + * drm_atomic_helper_disable_plane(), drm_atomic_helper_disable_plane() and the * various functions to implement set_property callbacks. New drivers must not * implement these functions themselves but must use the provided helpers. */ @@ -1078,7 +1078,7 @@ EXPORT_SYMBOL(drm_atomic_helper_commit); * work item, which allows nice concurrent updates on disjoint sets of crtcs. * * 3. The software state is updated synchronously with - * drm_atomic_helper_swap_state. Doing this under the protection of all modeset + * drm_atomic_helper_swap_state(). Doing this under the protection of all modeset * locks means concurrent callers never see inconsistent state. And doing this * while it's guaranteed that no relevant async worker runs means that async * workers do not need grab any locks. Actually they must not grab locks, for @@ -1351,7 +1351,7 @@ EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes); * * 4. Actually commit the hardware state. * - * 5. Call drm_atomic_helper_cleanup_planes with @state, which since step 3 + * 5. Call drm_atomic_helper_cleanup_planes() with @state, which since step 3 * contains the old state. Also do any other cleanup required with that state. */ void drm_atomic_helper_swap_state(struct drm_device *dev, -- 2.5.0
[Intel-gfx] [PATCH 4/5] Documentation: drm: Convert KMS Properties HTML table to CALS
On Tue, Aug 25, 2015 at 10:26:44AM +0100, Graham Whaley wrote:
> The KMS Properties table is in HTML format, which is not supported
> for building pdfdocs, resulting in the following types of errors:
>
> jade:/Documentation/DocBook/drm.xml:34413:15:E: there is no attribute
> "border"
> jade:/Documentation/DocBook/drm.xml:34413:31:E: there is no attribute
> "cellpadding"
> jade:/Documentation/DocBook/drm.xml:34413:47:E: there is no attribute
> "cellspacing"
> jade:/Documentation/DocBook/drm.xml:34414:7:E: document type does not
> allow element "tbody" here
>
> Convert the table over to a CALS format table
Hm, long-term plan was to move this table into DOC: comments in the
source-code using markdown, which we now have (at least in
drm-intel-nightly and also planned to be merged into 4.4). Since this is
both a lot of churn I'd like to get there in just 1 step ...
-Daniel
>
> Signed-off-by: Graham Whaley
> ---
> Documentation/DocBook/drm.tmpl | 1866
>
> 1 file changed, 937 insertions(+), 929 deletions(-)
>
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 2e05a79..e5bfdd8 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2580,935 +2580,943 @@ void intel_crt_init(struct drm_device *dev)
>and an initial instance value.
>
>
> - Existing KMS Properties
> -
> - The following table gives description of drm properties exposed by
> various
> - modules/drivers.
> -
> -
> -
> -
> - Owner Module/Drivers
> - Group
> - Property Name
> - Type
> - Property Values
> - Object attached
> - Description/Restrictions
> -
> -
> - DRM
> - Generic
> - ârotationâ
> - BITMASK
> - { 0, "rotate-0" },
> - { 1, "rotate-90" },
> - { 2, "rotate-180" },
> - { 3, "rotate-270" },
> - { 4, "reflect-x" },
> - { 5, "reflect-y" }
> - CRTC, Plane
> - rotate-(degrees) rotates the image by the specified
> amount in degrees
> - in counter clockwise direction. reflect-x and reflect-y reflects the
> - image along the specified axis prior to rotation
> -
> -
> - Connector
> - âEDIDâ
> - BLOB | IMMUTABLE
> - 0
> - Connector
> - Contains id of edid blob ptr object.
> -
> -
> - âDPMSâ
> - ENUM
> - { âOnâ, âStandbyâ, âSuspendâ, âOffâ
> }
> - Connector
> - Contains DPMS operation mode value.
> -
> -
> - âPATHâ
> - BLOB | IMMUTABLE
> - 0
> - Connector
> - Contains topology path to a connector.
> -
> -
> - âTILEâ
> - BLOB | IMMUTABLE
> - 0
> - Connector
> - Contains tiling information for a connector.
> -
> -
> - âCRTC_IDâ
> - OBJECT
> - DRM_MODE_OBJECT_CRTC
> - Connector
> - CRTC that connector is attached to (atomic)
> -
> -
> - Plane
> - âtypeâ
> - ENUM | IMMUTABLE
> - { "Overlay", "Primary", "Cursor" }
> - Plane
> - Plane type
> -
> -
> - âSRC_Xâ
> - RANGE
> - Min=0, Max=UINT_MAX
> - Plane
> - Scanout source x coordinate in 16.16 fixed point
> (atomic)
> -
> -
> - âSRC_Yâ
> - RANGE
> - Min=0, Max=UINT_MAX
> - Plane
> - Scanout source y coordinate in 16.16 fixed point
> (atomic)
> -
> -
> - âSRC_Wâ
> - RANGE
> - Min=0, Max=UINT_MAX
> - Plane
> - Scanout source width in 16.16 fixed point
> (atomic)
> -
> -
> - âSRC_Hâ
> - RANGE
> - Min=0, Max=UINT_MAX
> - Plane
> - Scanout source height in 16.16 fixed point
> (atomic)
> -
> -
> - âCRTC_Xâ
> - SIGNED_RANGE
> - Min=INT_MIN, Max=INT_MAX
> - Plane
> - Scanout CRTC (destination) x coordinate (atomic)
> -
> -
> - âCRTC_Yâ
> - SIGNED_RANGE
> - Min=INT_MIN, Max=INT_MAX
> - Plane
> - Scanout CRTC (destination) y coordinate (atomic)
> -
> -
> - âCRTC_Wâ
> - RANGE
> - Min=0, Max=UINT_MAX
> - Plane
> - Scanout CRTC (destination) width (atomic)
> -
> -
> - âCRTC_Hâ
> - RANGE
> - Min=0, Max=UINT_MAX
> - Plane
> - Scanout CRTC (destination) height (atomic)
> -
> -
> - âFB_IDâ
> - OBJECT
> - DRM_MODE_OBJECT_FB
> - Plane
> - Scanout framebuffer (atomic)
> -
> -
> - âCRTC_IDâ
> - OBJECT
> - DRM_MODE_OBJECT_CRTC
> - Plane
> - CRTC that plane is attached to (atomic)
> -
> -
> - DVI-I
> - âsubconnectorâ
> - ENUM
> - { âUnknownâ, âDVI-Dâ, âDVI-Aâ }
> - Connector
> - TBD
> -
> -
> - âselect subconnectorâ
> - ENUM
> - { âAutomaticâ, âDVI-Dâ, âDVI-Aâ }
> - Connector
> - TBD
> -
> -
[PATCH 2/2] amdgpu: serialize drmPrimeFDToHandle
On Mon, Aug 24, 2015 at 9:55 AM, Zhou, Jammy wrote:
> Both patches are Reviewed-by: Jammy Zhou
I pushed the series. thanks!
Alex
>
> Regards,
> Jammy
>
> -Original Message-
> From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf
> Of Christian K?nig
> Sent: Monday, August 24, 2015 5:44 PM
> To: dri-devel at lists.freedesktop.org
> Subject: [PATCH 2/2] amdgpu: serialize drmPrimeFDToHandle
>
> From: Christian König
>
> Fixes the same problem as "intel: Serialize drmPrimeFDToHandle with
> struct_mutex".
>
> Signed-off-by: Christian König
> ---
> amdgpu/amdgpu_bo.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c index dab3804..adf4253
> 100644
> --- a/amdgpu/amdgpu_bo.c
> +++ b/amdgpu/amdgpu_bo.c
> @@ -289,6 +289,10 @@ int amdgpu_bo_import(amdgpu_device_handle dev,
> int dma_fd;
> uint64_t dma_buf_size = 0;
>
> + /* We must maintain a list of pairs , so that we always
> +* return the same amdgpu_bo instance for the same handle. */
> + pthread_mutex_lock(&dev->bo_table_mutex);
> +
> /* Convert a DMA buf handle to a KMS handle now. */
> if (type == amdgpu_bo_handle_type_dma_buf_fd) {
> uint32_t handle;
> @@ -303,6 +307,7 @@ int amdgpu_bo_import(amdgpu_device_handle dev,
> /* Query the buffer size. */
> size = lseek(shared_handle, 0, SEEK_END);
> if (size == (off_t)-1) {
> + pthread_mutex_unlock(&dev->bo_table_mutex);
> amdgpu_close_kms_handle(dev, handle);
> return -errno;
> }
> @@ -312,10 +317,6 @@ int amdgpu_bo_import(amdgpu_device_handle dev,
> shared_handle = handle;
> }
>
> - /* We must maintain a list of pairs , so that we always
> -* return the same amdgpu_bo instance for the same handle. */
> - pthread_mutex_lock(&dev->bo_table_mutex);
> -
> /* If we have already created a buffer with this handle, find it. */
> switch (type) {
> case amdgpu_bo_handle_type_gem_flink_name:
> --
> 1.9.1
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed
From: Ville Syrjälä
Calculate the number of retries we should do for each i2c-over-aux
message based on the time it takes to perform the i2c transfer vs. the
aux transfer.
This mostly matches what the DP spec recommends. The numbers didn't come
out exactly the same as the tables in the spec, but I think there's
something fishy about the AUX trasnfer size calculations in the spec.
Also the way the i2c transfer length is calculated is somewhat open to
interpretation.
Note that currently we assume 10 kHz speed for the i2c bus. Some real
world devices (eg. some Apple DP->VGA dongle) fails with less than 16
retries, and that would correspond to something close to 20 kHz, so we
assume 10 kHz to be on the safe side. Ideally we should query/set the
i2c bus speed via DPCD but for now this should at leaast remove the
regression from the 1->16 byte trasnfer size change. And of course if
the sink completes the transfer quicker this shouldn't slow things down
since we don't change the interval between retries.
Fixes a regression with some DP dongles from:
commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
Author: Simon Farnsworth
Date: Tue Feb 10 18:38:08 2015 +
drm/dp: Use large transactions for I2C over AUX
Cc: Simon Farnsworth
Cc: moosotc at gmail.coma
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/drm_dp_helper.c | 64 +++--
1 file changed, 62 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 80a02a4..2b6b7f9 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -422,6 +422,61 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter
*adapter)
I2C_FUNC_10BIT_ADDR;
}
+#define AUX_PRECHARGE_LEN 16 /* 10 to 16 */
+#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
+#define AUX_STOP_LEN 4
+#define AUX_CMD_LEN 4
+#define AUX_ADDRESS_LEN 20
+#define AUX_REPLY_PAD_LEN 4
+#define AUX_LENGTH_LEN 8
+
+#define AUX_RESPONSE_TIMEOUT 300
+
+static int drm_dp_aux_req_len(const struct drm_dp_aux_msg *msg)
+{
+ int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
+ AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
+
+ if ((msg->request & DP_AUX_I2C_READ) == 0)
+ len += msg->size * 8;
+
+ return len;
+}
+
+static int drm_dp_aux_reply_len(const struct drm_dp_aux_msg *msg)
+{
+ int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
+ AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
+
+ if (msg->request & DP_AUX_I2C_READ)
+ len += msg->size * 8;
+ else
+ len += 8; /* 0 or 1 data bytes for write reply */
+
+ return len;
+}
+
+#define I2C_START_LEN 1
+#define I2C_STOP_LEN 1
+#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
+#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
+
+static int drm_dp_i2c_msg_len(const struct drm_dp_aux_msg *msg,
+ int i2c_speed_khz)
+{
+ return (I2C_START_LEN + I2C_ADDR_LEN + msg->size * I2C_DATA_LEN +
+ I2C_STOP_LEN) * 1000 / i2c_speed_khz;
+}
+
+static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
+ int i2c_speed_khz)
+{
+ int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg) +
AUX_RESPONSE_TIMEOUT;
+ int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz) -
AUX_RESPONSE_TIMEOUT;
+
+ return DIV_ROUND_UP(i2c_len, aux_len);
+}
+
/*
* Transfer a single I2C-over-AUX message and handle various error conditions,
* retrying the transaction as appropriate. It is assumed that the
@@ -434,13 +489,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
unsigned int retry, defer_i2c;
int ret;
-
/*
* DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device
* is required to retry at least seven times upon receiving AUX_DEFER
* before giving up the AUX transaction.
+*
+* We also try to account for the i2c bus speed.
+* FIXME currently assumes 10 kHz as some real world devices seem
+* to require it. We should query/set the speed via DPCD if supported.
*/
- for (retry = 0, defer_i2c = 0; retry < (7 + defer_i2c); retry++) {
+ int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10));
+
+ for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c);
retry++) {
mutex_lock(&aux->hw_mutex);
ret = aux->transfer(aux, msg);
mutex_unlock(&aux->hw_mutex);
--
2.4.6
[PATCH] drm/atomic-helper: properly annotate functions in kerneldoc
Hi Daniel, Thank you for the patch. On Tuesday 25 August 2015 16:26:03 Daniel Vetter wrote: > Without the () the markup and more important hyperlinking wont happen. > > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/drm_atomic_helper.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c index bc9c94e4cbaa..904acf236f36 > 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -42,14 +42,14 @@ > * add their own additional internal state. > * > * This library also provides default implementations for the check > callback in > - * drm_atomic_helper_check and for the commit callback with > - * drm_atomic_helper_commit. But the individual stages and callbacks are > expose > + * drm_atomic_helper_check() and for the commit callback with > + * drm_atomic_helper_commit(). But the individual stages and callbacks are > expose While at it, could you fix the typo here with s/expose/exposed/ ? Apart from that, Acked-by: Laurent Pinchart > * to allow drivers to mix and match and e.g. use the plane helpers only > * together with a driver private modeset implementation. > * > * This library also provides implementations for all the legacy driver > - * interfaces on top of the atomic interface. See > drm_atomic_helper_set_config, > - * drm_atomic_helper_disable_plane, drm_atomic_helper_disable_plane and the > + * interfaces on top of the atomic interface. See > drm_atomic_helper_set_config(), > + * drm_atomic_helper_disable_plane(), drm_atomic_helper_disable_plane() and > the > * various functions to implement set_property callbacks. New drivers must > not > * implement these functions themselves but must use the provided helpers. */ > @@ -1078,7 +1078,7 @@ EXPORT_SYMBOL(drm_atomic_helper_commit); > * work item, which allows nice concurrent updates on disjoint sets of > crtcs. > * > * 3. The software state is updated synchronously with > - * drm_atomic_helper_swap_state. Doing this under the protection of all > modeset > + * drm_atomic_helper_swap_state(). Doing this under the protection of all > modeset > * locks means concurrent callers never see inconsistent state. And doing > this > * while it's guaranteed that no relevant async worker runs means that > async > * workers do not need grab any locks. Actually they must not grab locks, > for > @@ -1351,7 +1351,7 @@ > EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes); * > * 4. Actually commit the hardware state. > * > - * 5. Call drm_atomic_helper_cleanup_planes with @state, which since step 3 > + * 5. Call drm_atomic_helper_cleanup_planes() with @state, which since > step 3 > * contains the old state. Also do any other cleanup required with that > state. > */ > void drm_atomic_helper_swap_state(struct drm_device *dev, -- Regards, Laurent Pinchart
[PATCH] drm/dp: Adjust i2c-over-aux retry count based on message size and i2c bus speed
From: Ville Syrjälä
Calculate the number of retries we should do for each i2c-over-aux
message based on the time it takes to perform the i2c transfer vs. the
aux transfer.
This mostly matches what the DP spec recommends. The numbers didn't come
out exactly the same as the tables in the spec, but I think there's
something fishy about the AUX trasnfer size calculations in the spec.
Also the way the i2c transfer length is calculated is somewhat open to
interpretation.
Note that currently we assume 10 kHz speed for the i2c bus. Some real
world devices (eg. some Apple DP->VGA dongle) fails with less than 16
retries, and that would correspond to something close to 20 kHz, so we
assume 10 kHz to be on the safe side. Ideally we should query/set the
i2c bus speed via DPCD but for now this should at leaast remove the
regression from the 1->16 byte trasnfer size change. And of course if
the sink completes the transfer quicker this shouldn't slow things down
since we don't change the interval between retries.
Fixes a regression with some DP dongles from:
commit 1d002fa720738bcd0bddb9178e9ea0773288e1dd
Author: Simon Farnsworth
Date: Tue Feb 10 18:38:08 2015 +
drm/dp: Use large transactions for I2C over AUX
Cc: Simon Farnsworth
Cc: moosotc at gmail.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91451
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/drm_dp_helper.c | 64 +++--
1 file changed, 62 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 80a02a4..2b6b7f9 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -422,6 +422,61 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter
*adapter)
I2C_FUNC_10BIT_ADDR;
}
+#define AUX_PRECHARGE_LEN 16 /* 10 to 16 */
+#define AUX_SYNC_LEN (16 + 4) /* preamble + AUX_SYNC_END */
+#define AUX_STOP_LEN 4
+#define AUX_CMD_LEN 4
+#define AUX_ADDRESS_LEN 20
+#define AUX_REPLY_PAD_LEN 4
+#define AUX_LENGTH_LEN 8
+
+#define AUX_RESPONSE_TIMEOUT 300
+
+static int drm_dp_aux_req_len(const struct drm_dp_aux_msg *msg)
+{
+ int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
+ AUX_CMD_LEN + AUX_ADDRESS_LEN + AUX_LENGTH_LEN;
+
+ if ((msg->request & DP_AUX_I2C_READ) == 0)
+ len += msg->size * 8;
+
+ return len;
+}
+
+static int drm_dp_aux_reply_len(const struct drm_dp_aux_msg *msg)
+{
+ int len = AUX_PRECHARGE_LEN + AUX_SYNC_LEN + AUX_STOP_LEN +
+ AUX_CMD_LEN + AUX_REPLY_PAD_LEN;
+
+ if (msg->request & DP_AUX_I2C_READ)
+ len += msg->size * 8;
+ else
+ len += 8; /* 0 or 1 data bytes for write reply */
+
+ return len;
+}
+
+#define I2C_START_LEN 1
+#define I2C_STOP_LEN 1
+#define I2C_ADDR_LEN 9 /* ADDRESS + R/W + ACK/NACK */
+#define I2C_DATA_LEN 9 /* DATA + ACK/NACK */
+
+static int drm_dp_i2c_msg_len(const struct drm_dp_aux_msg *msg,
+ int i2c_speed_khz)
+{
+ return (I2C_START_LEN + I2C_ADDR_LEN + msg->size * I2C_DATA_LEN +
+ I2C_STOP_LEN) * 1000 / i2c_speed_khz;
+}
+
+static int drm_dp_i2c_retry_count(const struct drm_dp_aux_msg *msg,
+ int i2c_speed_khz)
+{
+ int aux_len = drm_dp_aux_req_len(msg) + drm_dp_aux_reply_len(msg) +
AUX_RESPONSE_TIMEOUT;
+ int i2c_len = drm_dp_i2c_msg_len(msg, i2c_speed_khz) -
AUX_RESPONSE_TIMEOUT;
+
+ return DIV_ROUND_UP(i2c_len, aux_len);
+}
+
/*
* Transfer a single I2C-over-AUX message and handle various error conditions,
* retrying the transaction as appropriate. It is assumed that the
@@ -434,13 +489,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
unsigned int retry, defer_i2c;
int ret;
-
/*
* DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device
* is required to retry at least seven times upon receiving AUX_DEFER
* before giving up the AUX transaction.
+*
+* We also try to account for the i2c bus speed.
+* FIXME currently assumes 10 kHz as some real world devices seem
+* to require it. We should query/set the speed via DPCD if supported.
*/
- for (retry = 0, defer_i2c = 0; retry < (7 + defer_i2c); retry++) {
+ int max_retries = max(7, drm_dp_i2c_retry_count(msg, 10));
+
+ for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c);
retry++) {
mutex_lock(&aux->hw_mutex);
ret = aux->transfer(aux, msg);
mutex_unlock(&aux->hw_mutex);
--
2.4.6
[PATCH] drm/atomic-helper: properly annotate functions in kerneldoc
On Tue, Aug 25, 2015 at 06:00:26PM +0300, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the patch. > > On Tuesday 25 August 2015 16:26:03 Daniel Vetter wrote: > > Without the () the markup and more important hyperlinking wont happen. > > > > Signed-off-by: Daniel Vetter > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 12 ++-- > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > b/drivers/gpu/drm/drm_atomic_helper.c index bc9c94e4cbaa..904acf236f36 > > 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -42,14 +42,14 @@ > > * add their own additional internal state. > > * > > * This library also provides default implementations for the check > > callback in > > - * drm_atomic_helper_check and for the commit callback with > > - * drm_atomic_helper_commit. But the individual stages and callbacks are > > expose > > + * drm_atomic_helper_check() and for the commit callback with > > + * drm_atomic_helper_commit(). But the individual stages and callbacks are > > expose > > While at it, could you fix the typo here with s/expose/exposed/ ? > > Apart from that, > > Acked-by: Laurent Pinchart Fixed up and patch applied to drm-misc, thanks for the review. -Daniel > > > * to allow drivers to mix and match and e.g. use the plane helpers only > > * together with a driver private modeset implementation. > > * > > * This library also provides implementations for all the legacy driver > > - * interfaces on top of the atomic interface. See > > drm_atomic_helper_set_config, > > - * drm_atomic_helper_disable_plane, drm_atomic_helper_disable_plane and the > > + * interfaces on top of the atomic interface. See > > drm_atomic_helper_set_config(), > > + * drm_atomic_helper_disable_plane(), drm_atomic_helper_disable_plane() and > > the > > * various functions to implement set_property callbacks. New drivers must > > not > > * implement these functions themselves but must use the provided helpers. */ > > @@ -1078,7 +1078,7 @@ EXPORT_SYMBOL(drm_atomic_helper_commit); > > * work item, which allows nice concurrent updates on disjoint sets of > > crtcs. > > * > > * 3. The software state is updated synchronously with > > - * drm_atomic_helper_swap_state. Doing this under the protection of all > > modeset > > + * drm_atomic_helper_swap_state(). Doing this under the protection of all > > modeset > > * locks means concurrent callers never see inconsistent state. And doing > > this > > * while it's guaranteed that no relevant async worker runs means that > > async > > * workers do not need grab any locks. Actually they must not grab locks, > > for > > @@ -1351,7 +1351,7 @@ > > EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes); * > > * 4. Actually commit the hardware state. > > * > > - * 5. Call drm_atomic_helper_cleanup_planes with @state, which since step 3 > > + * 5. Call drm_atomic_helper_cleanup_planes() with @state, which since > > step 3 > > * contains the old state. Also do any other cleanup required with that > > state. > > */ > > void drm_atomic_helper_swap_state(struct drm_device *dev, > > -- > Regards, > > Laurent Pinchart > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH] drm/fb-helper: Use -errno return in restore_mode_unlocked
Using bool and returning true upon error is very uncommon. Also an int
return value is actually what all the callers which did check it seem
to have expected.
v2: Restore hunk misplaced in a rebase, spotted by Rob.
Cc: Rob Clark
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_fb_helper.c | 19 +++
include/drm/drm_fb_helper.h | 6 +++---
2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 418d299f3b12..859134e0d72d 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -320,11 +320,10 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
}
EXPORT_SYMBOL(drm_fb_helper_debug_leave);
-static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
+static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
{
struct drm_device *dev = fb_helper->dev;
struct drm_plane *plane;
- bool error = false;
int i;
drm_warn_on_modeset_not_all_locked(dev);
@@ -348,14 +347,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper
*fb_helper)
if (crtc->funcs->cursor_set) {
ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0);
if (ret)
- error = true;
+ return ret;
}
ret = drm_mode_set_config_internal(mode_set);
if (ret)
- error = true;
+ return ret;
}
- return error;
+
+ return 0;
}
/**
@@ -365,12 +365,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper
*fb_helper)
* This should be called from driver's drm ->lastclose callback
* when implementing an fbcon on top of kms using this helper. This ensures
that
* the user isn't greeted with a black screen when e.g. X dies.
+ *
+ * RETURNS:
+ * Zero if everything went ok, negative error code otherwise.
*/
-bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
+int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
{
struct drm_device *dev = fb_helper->dev;
- bool ret;
- bool do_delayed = false;
+ bool do_delayed;
+ int ret;
drm_modeset_lock_all(dev);
ret = restore_fbdev_mode(fb_helper);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index dbab4622b58f..67de1f10008e 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -136,7 +136,7 @@ int drm_fb_helper_set_par(struct fb_info *info);
int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
struct fb_info *info);
-bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
*fb_helper);
+int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper);
struct fb_info *drm_fb_helper_alloc_fbi(struct drm_fb_helper *fb_helper);
void drm_fb_helper_unregister_fbi(struct drm_fb_helper *fb_helper);
@@ -226,10 +226,10 @@ static inline int drm_fb_helper_check_var(struct
fb_var_screeninfo *var,
return 0;
}
-static inline bool
+static inline int
drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
{
- return true;
+ return 0;
}
static inline struct fb_info *
--
1.8.3.1
drm/exynos: remove superfluous checks in g2d_check_reg_offset()
Gentle ping!
On 2015-08-18 00:51, Tobias Jakobi wrote:
> The cases of the switch statement ensure that reg_type
> can never be REG_TYPE_NONE here.
>
> Signed-off-by: Tobias Jakobi
> ---
> drivers/gpu/drm/exynos/exynos_drm_g2d.c | 8
> 1 file changed, 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> index ba00839..211af37 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> @@ -973,8 +973,6 @@ static int g2d_check_reg_offset(struct device *dev,
> goto err;
>
> reg_type = g2d_get_reg_type(reg_offset);
> - if (reg_type == REG_TYPE_NONE)
> - goto err;
>
> /* check userptr buffer type. */
> if ((cmdlist->data[index] & ~0x7fff) >> 31) {
> @@ -989,8 +987,6 @@ static int g2d_check_reg_offset(struct device *dev,
> goto err;
>
> reg_type = g2d_get_reg_type(reg_offset);
> - if (reg_type == REG_TYPE_NONE)
> - goto err;
>
> buf_desc = &buf_info->descs[reg_type];
> value = cmdlist->data[index + 1];
> @@ -1003,8 +999,6 @@ static int g2d_check_reg_offset(struct device
> *dev,
> goto err;
>
> reg_type = g2d_get_reg_type(reg_offset);
> - if (reg_type == REG_TYPE_NONE)
> - goto err;
>
> buf_desc = &buf_info->descs[reg_type];
> value = cmdlist->data[index + 1];
> @@ -1018,8 +1012,6 @@ static int g2d_check_reg_offset(struct device
> *dev,
> goto err;
>
> reg_type = g2d_get_reg_type(reg_offset);
> - if (reg_type == REG_TYPE_NONE)
> - goto err;
>
> buf_desc = &buf_info->descs[reg_type];
> value = cmdlist->data[index + 1];
drm/exynos: fix size check in g2d_check_buf_desc_is_valid()
Gentle ping!
Also please note that this is a critical fix. With the
incomplete check pagefaults can happen when the engine
accesses a invalid buffer position.
With best wishes,
Tobias
On 2015-08-18 00:51, Tobias Jakobi wrote:
> The size check was incomplete. It only computed the
> size of area of the drawing rectangle and checked if
> the size still fit inside the buffer.
>
> The correct check is to compute the position of the
> last byte that the G2D engine is going to access and
> then check if that position is still contained in the
> buffer. In particular we need the stride information
> to determine this.
>
> Signed-off-by: Tobias Jakobi
> ---
> drivers/gpu/drm/exynos/exynos_drm_g2d.c | 51
> ++---
> 1 file changed, 41 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> index 211af37..535b4ad 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> @@ -48,11 +48,13 @@
>
> /* registers for base address */
> #define G2D_SRC_BASE_ADDR0x0304
> +#define G2D_SRC_STRIDE_REG 0x0308
> #define G2D_SRC_COLOR_MODE 0x030C
> #define G2D_SRC_LEFT_TOP 0x0310
> #define G2D_SRC_RIGHT_BOTTOM 0x0314
> #define G2D_SRC_PLANE2_BASE_ADDR 0x0318
> #define G2D_DST_BASE_ADDR0x0404
> +#define G2D_DST_STRIDE_REG 0x0408
> #define G2D_DST_COLOR_MODE 0x040C
> #define G2D_DST_LEFT_TOP 0x0410
> #define G2D_DST_RIGHT_BOTTOM 0x0414
> @@ -148,6 +150,7 @@ struct g2d_cmdlist {
> * A structure of buffer description
> *
> * @format: color format
> + * @stride: buffer stride/pitch in bytes
> * @left_x: the x coordinates of left top corner
> * @top_y: the y coordinates of left top corner
> * @right_x: the x coordinates of right bottom corner
> @@ -156,6 +159,7 @@ struct g2d_cmdlist {
> */
> struct g2d_buf_desc {
> unsigned intformat;
> + unsigned intstride;
> unsigned intleft_x;
> unsigned inttop_y;
> unsigned intright_x;
> @@ -589,6 +593,7 @@ static enum g2d_reg_type g2d_get_reg_type(int
> reg_offset)
>
> switch (reg_offset) {
> case G2D_SRC_BASE_ADDR:
> + case G2D_SRC_STRIDE_REG:
> case G2D_SRC_COLOR_MODE:
> case G2D_SRC_LEFT_TOP:
> case G2D_SRC_RIGHT_BOTTOM:
> @@ -598,6 +603,7 @@ static enum g2d_reg_type g2d_get_reg_type(int
> reg_offset)
> reg_type = REG_TYPE_SRC_PLANE2;
> break;
> case G2D_DST_BASE_ADDR:
> + case G2D_DST_STRIDE_REG:
> case G2D_DST_COLOR_MODE:
> case G2D_DST_LEFT_TOP:
> case G2D_DST_RIGHT_BOTTOM:
> @@ -652,8 +658,8 @@ static bool g2d_check_buf_desc_is_valid(struct
> g2d_buf_desc *buf_desc,
> enum g2d_reg_type reg_type,
> unsigned long size)
> {
> - unsigned int width, height;
> - unsigned long area;
> + int width, height;
> + unsigned long bpp, last_pos;
>
> /*
>* check source and destination buffers only.
> @@ -662,22 +668,37 @@ static bool g2d_check_buf_desc_is_valid(struct
> g2d_buf_desc *buf_desc,
> if (reg_type != REG_TYPE_SRC && reg_type != REG_TYPE_DST)
> return true;
>
> - width = buf_desc->right_x - buf_desc->left_x;
> + /* This check also makes sure that right_x > left_x. */
> + width = (int)buf_desc->right_x - (int)buf_desc->left_x;
> if (width < G2D_LEN_MIN || width > G2D_LEN_MAX) {
> - DRM_ERROR("width[%u] is out of range!\n", width);
> + DRM_ERROR("width[%d] is out of range!\n", width);
> return false;
> }
>
> - height = buf_desc->bottom_y - buf_desc->top_y;
> + /* This check also makes sure that bottom_y > top_y. */
> + height = (int)buf_desc->bottom_y - (int)buf_desc->top_y;
> if (height < G2D_LEN_MIN || height > G2D_LEN_MAX) {
> - DRM_ERROR("height[%u] is out of range!\n", height);
> + DRM_ERROR("height[%d] is out of range!\n", height);
> return false;
> }
>
> - area = (unsigned long)width * (unsigned long)height *
> - g2d_get_buf_bpp(buf_desc->format);
> - if (area > size) {
> - DRM_ERROR("area[%lu] is out of range[%lu]!\n", area, size);
> + bpp = g2d_get_buf_bpp(buf_desc->format);
> +
> + /* Compute the position of the last byte that the engine accesses. */
> + last_pos = ((unsigned long)buf_desc->bottom_y - 1) *
> + (unsigned long)buf_desc->stride +
> + (unsigned long)buf_desc->right_x * bpp - 1;
> +
> + /*
> + * Since right_x > left_x and bottom_y > top_y we already know
> + * that the first_pos < last_pos (first_pos being the position
> + * of the first byte the engine accesses), it just rema
about mmap dma-buf and sync
On 08/25/2015 06:30 AM, Thomas Hellstrom wrote:
> On 08/25/2015 11:02 AM, Daniel Vetter wrote:
>> I really feel like any kind of multi-range flush interface is feature
>> bloat, and if we do it then we should only do it later on when there's a
>> clear need for it.
>
> IMO all the use-cases so far that wanted to do this have been 2D
> updates. and having only a 1D sync will most probably scare people away
> from this interface.
>
>> Afaiui dma-buf mmap will mostly be used for up/downloads, which means
>> there will be some explicit copy involved somewhere anyway. So similar to
>> userptr usage. And for those most often you just slurp in a linear block
>> and then let the blitter rearrange it, at least on i915.
>>
>> Also thus far the consensus was that dma-buf doesn't care/know about
>> content layout, adding strides/bytes/whatever does bend this quite a bit.
>
> I think with a 2D interface, the stride only applies to the sync
> operation itself and is only a parameter for that sync operation.
> Whether we should have multiple regions or not is not a big deal for me,
> but I think at least a 2D sync is crucial.
Right now only omap, ion and udl-fb make use of begin{,end}_cpu_access()
dma-buf interface, but it's curious that none uses those 1-d parameters
(start and len). So in that sense it seems that the tendency is to
feature bloat the API if we do the 2-d additions.
OTOH we're talking about a different usage of dma-buf right now, so the
driver might actually start to use in fact that API. That said, I
thought it was somewhat simple to turn the common code into 2-d, cause,
as I pointed in the other email, we'd be pushing the whole
responsibility of dealing with the regions and so on to the driver
implementors.
Thomas, any comments in the dma_buf_begin_cpu_access() new API I showed?
Maybe I should just clean up here the draft and sent it to the ML :)
Tiago
[PATCH 2/3] drm:msm: Initial Add Writeback Support (V2)
On Tue, Aug 25, 2015 at 3:05 AM, Daniel Vetter wrote: > On Wed, Aug 19, 2015 at 03:00:04PM -0400, Rob Clark wrote: >> On Tue, Apr 7, 2015 at 2:09 PM, Jilai Wang wrote: >> So one thing that I wanted sorting out before we let userspace see >> streaming writeback (where I do think v4l is the right interface), is >> a way to deal w/ permissions/security.. Ie. only the kms master >> should control access to writeback. Ie. an process that the >> compositor isn't aware of / doesn't trust, should not be able to open >> the v4l device and start snooping on the screen contents. And I don't >> think just file permissions in /dev is sufficient. You likely don't >> want to run your helper process doing video encode and streaming as a >> privilaged user. >> >> One way to handle this would be some sort of dri2 style >> getmagic/authmagic sort of interface between the drm/kms master, and >> v4l device, to unlock streaming. But that is kind of passe. Fd >> passing is the fashionable thing now. So instead we could use a dummy >> v4l2_file_opererations::open() which always returns an error. So v4l >> device shows up in /dev.. but no userspace can open it. And instead, >> the way to get a fd for the v4l dev would be via a drm/kms ioctl (with >> DRM_MASTER flag set). Once compositor gets the fd, it can use fd >> passing, if needed, to hand it off to a helper process, etc. >> >> (probably use something like alloc_file() to get the 'struct file *', >> then call directly into v4l2_fh_open(), and then get_unused_fd_flags() >> + fd_install() to get fd to return to userspace) > > Just following up here, but consensus from the lpc track is that we don't > need this as long as writeback is something which needs a specific action > to initiate. I.e. if we have a separate writeback connector which won't > grab any frames at all as long as it's disconnected we should be fine. Wrt > session handling that's something which would need to be fixed between > v4l and logind (or whatever). Was that consensus? I agree that something should initiate writeback from the kms side of things. But if we don't have *something* to ensure whatever is on the other end of writeback is who we think it is, it seems at least racy.. > In general I don't like hand-rolling our own proprietary v4l-open process, > it means that all the existing v4l test&dev tooling must be changed, even > when you're root. well, I know that, for example, gst v4l2src allows you to pass in an already opened v4l dev fd, which fits in pretty well with what I propose. The alternative, I think, is a dri2 style auth handshake between drm/kms and v4l, which I am less thrilled about. I would have to *assume* that userspace is at least prepared to deal with -EPERM when it tries to open a device.. at least more so than special auth ioctl sequence. BR, -R > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
[Intel-gfx] [PATCH 1/4] drm: Make drm_fb_unregister/remove accept NULL fb
On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter
wrote:
> These functions are used by drivers to release fbdev emulation
> buffers. We need to make them resilient to NULL pointers to
> make the fbdev compile/runtime knobs not cause Oopses on module
> unload.
>
> Cc: Archit Taneja
> Signed-off-by: Daniel Vetter
Reviewed-by: Rob Clark
> ---
> drivers/gpu/drm/drm_crtc.c | 14 --
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 33d877c65ced..884690c81094 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -538,7 +538,12 @@ EXPORT_SYMBOL(drm_framebuffer_reference);
> */
> void drm_framebuffer_unregister_private(struct drm_framebuffer *fb)
> {
> - struct drm_device *dev = fb->dev;
> + struct drm_device *dev;
> +
> + if (!fb)
> + return;
> +
> + dev = fb->dev;
>
> mutex_lock(&dev->mode_config.fb_lock);
> /* Mark fb as reaped and drop idr ref. */
> @@ -589,12 +594,17 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup);
> */
> void drm_framebuffer_remove(struct drm_framebuffer *fb)
> {
> - struct drm_device *dev = fb->dev;
> + struct drm_device *dev;
> struct drm_crtc *crtc;
> struct drm_plane *plane;
> struct drm_mode_set set;
> int ret;
>
> + if (!fb)
> + return;
> +
> + dev = fb->dev;
> +
> WARN_ON(!list_empty(&fb->filp_head));
>
> /*
> --
> 1.8.3.1
>
> ___
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[PATCH] drm/fb-helper: Use -errno return in restore_mode_unlocked
On Tue, Aug 25, 2015 at 11:20 AM, Daniel Vetter
wrote:
> Using bool and returning true upon error is very uncommon. Also an int
> return value is actually what all the callers which did check it seem
> to have expected.
>
> v2: Restore hunk misplaced in a rebase, spotted by Rob.
>
> Cc: Rob Clark
> Signed-off-by: Daniel Vetter
Reviewed-by: Rob Clark
> ---
> drivers/gpu/drm/drm_fb_helper.c | 19 +++
> include/drm/drm_fb_helper.h | 6 +++---
> 2 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 418d299f3b12..859134e0d72d 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -320,11 +320,10 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
> }
> EXPORT_SYMBOL(drm_fb_helper_debug_leave);
>
> -static bool restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> +static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> {
> struct drm_device *dev = fb_helper->dev;
> struct drm_plane *plane;
> - bool error = false;
> int i;
>
> drm_warn_on_modeset_not_all_locked(dev);
> @@ -348,14 +347,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper
> *fb_helper)
> if (crtc->funcs->cursor_set) {
> ret = crtc->funcs->cursor_set(crtc, NULL, 0, 0, 0);
> if (ret)
> - error = true;
> + return ret;
> }
>
> ret = drm_mode_set_config_internal(mode_set);
> if (ret)
> - error = true;
> + return ret;
> }
> - return error;
> +
> + return 0;
> }
>
> /**
> @@ -365,12 +365,15 @@ static bool restore_fbdev_mode(struct drm_fb_helper
> *fb_helper)
> * This should be called from driver's drm ->lastclose callback
> * when implementing an fbcon on top of kms using this helper. This ensures
> that
> * the user isn't greeted with a black screen when e.g. X dies.
> + *
> + * RETURNS:
> + * Zero if everything went ok, negative error code otherwise.
> */
> -bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
> *fb_helper)
> +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
> *fb_helper)
> {
> struct drm_device *dev = fb_helper->dev;
> - bool ret;
> - bool do_delayed = false;
> + bool do_delayed;
> + int ret;
>
> drm_modeset_lock_all(dev);
> ret = restore_fbdev_mode(fb_helper);
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index dbab4622b58f..67de1f10008e 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -136,7 +136,7 @@ int drm_fb_helper_set_par(struct fb_info *info);
> int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> struct fb_info *info);
>
> -bool drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
> *fb_helper);
> +int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper
> *fb_helper);
>
> struct fb_info *drm_fb_helper_alloc_fbi(struct drm_fb_helper *fb_helper);
> void drm_fb_helper_unregister_fbi(struct drm_fb_helper *fb_helper);
> @@ -226,10 +226,10 @@ static inline int drm_fb_helper_check_var(struct
> fb_var_screeninfo *var,
> return 0;
> }
>
> -static inline bool
> +static inline int
> drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
> {
> - return true;
> + return 0;
> }
>
> static inline struct fb_info *
> --
> 1.8.3.1
>
[PATCH 4/4] fbdev: Debug knob to register without holding console_lock
On Tue, Aug 25, 2015 at 9:45 AM, Daniel Vetter
wrote:
> When the usual fbcon legacy options are enabled we have
> ->register_framebuffer
> ->fb notifier chain calls into fbcon
> ->fbcon sets up console on new fbi
> ->fbi->set_par
> ->drm_fb_helper_set_par exercises full kms api
>
> And because of locking inversion hilarity all of register_framebuffer
> is done with the console lock held. Which means that the first time on
> driver load we exercise _all_ the kms code (all probe paths and
> modeset paths for everything connected) is under the console lock.
> That means if anything goes belly-up in that big pile of code nothing
> ever reaches logfiles (and the machine is dead).
>
> Usual tactic to debug that is to temporarily remove those console_lock
> calls to be able to capture backtraces. I'm fed up writing this patch
> and recompiling kernels. Hence this patch here to add an unsafe,
> kernel-taining option to do this at runtime.
>
> Cc: Jean-Christophe Plagniol-Villard
> Cc: Tomi Valkeinen
> Cc: linux-fbdev at vger.kernel.org
> Signed-off-by: Daniel Vetter
This one was causing me some problems, if I tried to enable
lockless_register_fb. It *looks* like it should work, so I'm not
quite sure what the deal is. But I'm 110% fan of getting something
like this working, because console_lock is pretty much the bane of kms
developer's existence..
I'll have to debug further on a system where I can see more than the
bottom three lines of the second to last backtrace..
BR,
-R
> ---
> drivers/video/fbdev/core/fbmem.c | 14 +++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbmem.c
> b/drivers/video/fbdev/core/fbmem.c
> index 0705d8883ede..4e73b6f6b1c0 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1608,6 +1608,11 @@ static int do_remove_conflicting_framebuffers(struct
> apertures_struct *a,
> return 0;
> }
>
> +static bool lockless_register_fb;
> +module_param_named_unsafe(lockless_register_fb, lockless_register_fb, bool,
> 0400);
> +MODULE_PARM_DESC(lockless_register_fb,
> + "Lockless framebuffer registration for debugging [default=off]");
> +
> static int do_register_framebuffer(struct fb_info *fb_info)
> {
> int i, ret;
> @@ -1675,15 +1680,18 @@ static int do_register_framebuffer(struct fb_info
> *fb_info)
> registered_fb[i] = fb_info;
>
> event.info = fb_info;
> - console_lock();
> + if (!lockless_register_fb)
> + console_lock();
> if (!lock_fb_info(fb_info)) {
> - console_unlock();
> + if (!lockless_register_fb)
> + console_unlock();
> return -ENODEV;
> }
>
> fb_notifier_call_chain(FB_EVENT_FB_REGISTERED, &event);
> unlock_fb_info(fb_info);
> - console_unlock();
> + if (!lockless_register_fb)
> + console_unlock();
> return 0;
> }
>
> --
> 1.8.3.1
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/4] drm: atomic fb-helper support
Convert fb-helper, or at least the two code-paths which potentially trigger more than a single update, to use atomic API if the driver supports it. The first patch is slightly unrelated, but I needed to add kerneldoc for 'struct drm_fb_helper' before I could add kerneldoc for the new atomic field. Danvet tricked me into writing some kerneldoc that way. In theory, after this, we can re-introduce the ww_acquire_fini() call. Rob Clark (4): drm/fb-helper: add headerdoc for drm_fb_helper drm/fb-helper: atomic restore_fbdev_mode().. drm/fb-helper: atomic pan_display().. drm/i915: enable atomic fb-helper drivers/gpu/drm/drm_atomic_helper.c | 131 +--- drivers/gpu/drm/drm_fb_helper.c | 131 drivers/gpu/drm/i915/intel_fbdev.c | 2 + include/drm/drm_atomic_helper.h | 6 ++ include/drm/drm_fb_helper.h | 22 ++ 5 files changed, 239 insertions(+), 53 deletions(-) -- 2.4.3
[PATCH 1/4] drm/fb-helper: add headerdoc for drm_fb_helper
Signed-off-by: Rob Clark
---
include/drm/drm_fb_helper.h | 14 ++
1 file changed, 14 insertions(+)
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 67de1f1..6254136 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -104,6 +104,20 @@ struct drm_fb_helper_connector {
struct drm_connector *connector;
};
+/**
+ * struct drm_fb_helper - helper to emulate fbdev on top of kms
+ * @fb: Scanout framebuffer object
+ * @dev: DRM device
+ * @crtc_count: number of possible CRTCs
+ * @crtc_info: per-CRTC helper state (mode, x/y offset, etc)
+ * @connector_count: number of connected connectors
+ * @connector_info_alloc_count: size of connector_info
+ * @funcs: driver callbacks for fb helper
+ * @fbdev: emulated fbdev device info struct
+ * @pseudo_palette: fake palette of 16 colors
+ * @kernel_fb_list: list_head in kernel_fb_helper_list
+ * @delayed_hotplug: was there a hotplug while kms master active?
+ */
struct drm_fb_helper {
struct drm_framebuffer *fb;
struct drm_device *dev;
--
2.4.3
[PATCH 2/4] drm/fb-helper: atomic restore_fbdev_mode()..
Add support for using atomic code-paths for restore_fbdev_mode().
Signed-off-by: Rob Clark
---
drivers/gpu/drm/drm_atomic_helper.c | 131 +---
drivers/gpu/drm/drm_fb_helper.c | 74
include/drm/drm_atomic_helper.h | 6 ++
include/drm/drm_fb_helper.h | 8 +++
4 files changed, 166 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index d432348..dbce582 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1502,21 +1502,9 @@ retry:
goto fail;
}
- ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
+ ret = __drm_atomic_helper_disable_plane(plane, plane_state);
if (ret != 0)
goto fail;
- drm_atomic_set_fb_for_plane(plane_state, NULL);
- plane_state->crtc_x = 0;
- plane_state->crtc_y = 0;
- plane_state->crtc_h = 0;
- plane_state->crtc_w = 0;
- plane_state->src_x = 0;
- plane_state->src_y = 0;
- plane_state->src_h = 0;
- plane_state->src_w = 0;
-
- if (plane == plane->crtc->cursor)
- state->legacy_cursor_update = true;
ret = drm_atomic_commit(state);
if (ret != 0)
@@ -1546,6 +1534,32 @@ backoff:
}
EXPORT_SYMBOL(drm_atomic_helper_disable_plane);
+/* just used from fb-helper and atomic-helper: */
+int __drm_atomic_helper_disable_plane(struct drm_plane *plane,
+ struct drm_plane_state *plane_state)
+{
+ int ret;
+
+ ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
+ if (ret != 0)
+ return ret;
+
+ drm_atomic_set_fb_for_plane(plane_state, NULL);
+ plane_state->crtc_x = 0;
+ plane_state->crtc_y = 0;
+ plane_state->crtc_h = 0;
+ plane_state->crtc_w = 0;
+ plane_state->src_x = 0;
+ plane_state->src_y = 0;
+ plane_state->src_h = 0;
+ plane_state->src_w = 0;
+
+ if (plane->crtc && (plane == plane->crtc->cursor))
+ plane_state->state->legacy_cursor_update = true;
+
+ return 0;
+}
+
static int update_output_state(struct drm_atomic_state *state,
struct drm_mode_set *set)
{
@@ -1629,8 +1643,6 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set)
{
struct drm_atomic_state *state;
struct drm_crtc *crtc = set->crtc;
- struct drm_crtc_state *crtc_state;
- struct drm_plane_state *primary_state;
int ret = 0;
state = drm_atomic_state_alloc(crtc->dev);
@@ -1639,17 +1651,54 @@ int drm_atomic_helper_set_config(struct drm_mode_set
*set)
state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
retry:
- crtc_state = drm_atomic_get_crtc_state(state, crtc);
- if (IS_ERR(crtc_state)) {
- ret = PTR_ERR(crtc_state);
+ ret = __drm_atomic_helper_set_config(set, state);
+ if (ret != 0)
goto fail;
- }
- primary_state = drm_atomic_get_plane_state(state, crtc->primary);
- if (IS_ERR(primary_state)) {
- ret = PTR_ERR(primary_state);
+ ret = drm_atomic_commit(state);
+ if (ret != 0)
goto fail;
- }
+
+ /* Driver takes ownership of state on successful commit. */
+ return 0;
+fail:
+ if (ret == -EDEADLK)
+ goto backoff;
+
+ drm_atomic_state_free(state);
+
+ return ret;
+backoff:
+ drm_atomic_state_clear(state);
+ drm_atomic_legacy_backoff(state);
+
+ /*
+* Someone might have exchanged the framebuffer while we dropped locks
+* in the backoff code. We need to fix up the fb refcount tracking the
+* core does for us.
+*/
+ crtc->primary->old_fb = crtc->primary->fb;
+
+ goto retry;
+}
+EXPORT_SYMBOL(drm_atomic_helper_set_config);
+
+/* just used from fb-helper and atomic-helper: */
+int __drm_atomic_helper_set_config(struct drm_mode_set *set,
+ struct drm_atomic_state *state)
+{
+ struct drm_crtc_state *crtc_state;
+ struct drm_plane_state *primary_state;
+ struct drm_crtc *crtc = set->crtc;
+ int ret;
+
+ crtc_state = drm_atomic_get_crtc_state(state, crtc);
+ if (IS_ERR(crtc_state))
+ return PTR_ERR(crtc_state);
+
+ primary_state = drm_atomic_get_plane_state(state, crtc->primary);
+ if (IS_ERR(primary_state))
+ return PTR_ERR(primary_state);
if (!set->mode) {
WARN_ON(set->fb);
@@ -1657,13 +1706,13 @@ retry:
ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
if (ret != 0)
- goto fail;
+ return ret;
crtc_state->active = false;
ret = drm_atomic_set_crtc_for_plane(primary_state, NULL);
if (ret != 0)
- goto fai
[PATCH 3/4] drm/fb-helper: atomic pan_display()..
Signed-off-by: Rob Clark
---
drivers/gpu/drm/drm_fb_helper.c | 57 +
1 file changed, 57 insertions(+)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 9741d79..efc4d33 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1218,6 +1218,57 @@ int drm_fb_helper_set_par(struct fb_info *info)
}
EXPORT_SYMBOL(drm_fb_helper_set_par);
+static int pan_display_atomic(struct fb_var_screeninfo *var,
+ struct fb_info *info)
+{
+ struct drm_fb_helper *fb_helper = info->par;
+ struct drm_device *dev = fb_helper->dev;
+ struct drm_atomic_state *state;
+ int i, ret;
+
+ state = drm_atomic_state_alloc(dev);
+ if (!state)
+ return -ENOMEM;
+
+ state->acquire_ctx = dev->mode_config.acquire_ctx;
+retry:
+ for(i = 0; i < fb_helper->crtc_count; i++) {
+ struct drm_mode_set *mode_set;
+
+ mode_set = &fb_helper->crtc_info[i].mode_set;
+
+ mode_set->x = var->xoffset;
+ mode_set->y = var->yoffset;
+
+ ret = __drm_atomic_helper_set_config(mode_set, state);
+ if (ret != 0)
+ goto fail;
+ }
+
+ ret = drm_atomic_commit(state);
+ if (ret != 0)
+ goto fail;
+
+ info->var.xoffset = var->xoffset;
+ info->var.yoffset = var->yoffset;
+
+ return 0;
+
+fail:
+ if (ret == -EDEADLK)
+ goto backoff;
+
+ drm_atomic_state_free(state);
+
+ return ret;
+
+backoff:
+ drm_atomic_state_clear(state);
+ drm_atomic_legacy_backoff(state);
+
+ goto retry;
+}
+
/**
* drm_fb_helper_pan_display - implementation for ->fb_pan_display
* @var: updated screen information
@@ -1241,6 +1292,11 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo
*var,
return -EBUSY;
}
+ if (fb_helper->atomic) {
+ ret = pan_display_atomic(var, info);
+ goto unlock;
+ }
+
for (i = 0; i < fb_helper->crtc_count; i++) {
modeset = &fb_helper->crtc_info[i].mode_set;
@@ -1255,6 +1311,7 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo
*var,
}
}
}
+unlock:
drm_modeset_unlock_all(dev);
return ret;
}
--
2.4.3
[PATCH 4/4] drm/i915: enable atomic fb-helper
i915 supports enough atomic to have atomic fb-helper paths, even though it does not yet advertise DRIVER_ATOMIC. Signed-off-by: Rob Clark --- drivers/gpu/drm/i915/intel_fbdev.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index 8c6a6fa..ab2b856 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -689,6 +689,8 @@ int intel_fbdev_init(struct drm_device *dev) return ret; } + ifbdev->helper.atomic = true; + dev_priv->fbdev = ifbdev; INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker); -- 2.4.3
[PATCH 1/7] drm/vc4: Add devicetree bindings for VC4.
On Mon, Aug 24, 2015 at 9:47 AM, Rob Herring wrote: > On Mon, Aug 17, 2015 at 1:30 PM, Eric Anholt wrote: >> Stephen Warren writes: >> >>> On 08/12/2015 06:56 PM, Eric Anholt wrote: Signed-off-by: Eric Anholt >>> >>> This one definitely needs a patch description, since someone might not >>> know what a VC4 is, and "git log" won't show the text from the binding >>> doc itself. I'd suggest adding the initial paragraph of the binding doc >>> as the patch description, or more. >>> diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt >> +- hvss: List of references to HVS video scalers +- encoders: List of references to output encoders (HDMI, SDTV) >>> >>> Would it make sense to make all those nodes child node of the vc4 >>> object. That way, there's no need to have these lists of objects; they >>> can be automatically built up as the DT is enumerated. I know that e.g. >>> the NVIDIA Tegra host1x binding works this way, and I think it may have >>> been inspired by other similar cases. >> >> I've looked at tegra, and the component system used by msm appears to be >> nicer than it. To follow tegra's model, it looks like I need to build >> this extra bus thing corresponding to host1x that is effectively the >> drivers/base/component.c code, so that I can get at vc4's structure from >> the component drivers. >> >>> Of course, this is only appropriate if the HW modules really are >>> logically children of the VC4 HW module. Perhaps they aren't. If they >>> aren't though, I wonder what this "vc4" module actually represents in HW? >> >> It's the subsystem, same as we use a subsystem node for msm, sti, >> rockchip, imx, and exynos. This appears to be the common model of how >> the collection of graphics-related components is represented in the DT. > > I think most of these bindings are wrong. They are grouped together > because that is what DRM wants not because that reflects the h/w. So > convince me this is one block, not that it is what other people do. I think, when it comes to more complex driver subsystems (like drm in particular) we have a bit of mismatch between how things look from the "pure hw ignoring sw" perspective, and the "how sw and in particular userspace expects things" perspective. Maybe it is less a problem in other subsystems, where bindings map to things that are only visible in the kernel, or well defined devices like uart or sata controller. But when given the choice, I'm going to err on the side of not confusing userspace and the large software stack that sits above drm/kms, over dt purity. Maybe it would be nice to have a sort of dt overlay that adds the bits needed to tie together hw blocks that should be assembled into a single logical device for linux and userspace (but maybe not some other hypothetical operating system). But so far that doesn't exist. All we have is a hammer (devicetree), everything looks like a nail. End result is we end up adding some things in the bindings which aren't purely about the hw. Until someone invents a screwdriver, I'm not sure what else to do. In the end, other hypothetical OS is free to ignore those extra fields in the bindings if it doesn't need them. So meh? BR, -R > Rob > > ___ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[PATCH v4] dma-buf: Add ioctls to allow userspace to flush
From: Daniel Vetter
The userspace might need some sort of cache coherency management e.g. when CPU
and GPU domains are being accessed through dma-buf at the same time. To
circumvent this problem there are begin/end coherency markers, that forward
directly to existing dma-buf device drivers vfunc hooks. Userspace can make use
of those markers through the DMA_BUF_IOCTL_SYNC ioctl. The sequence would be
used like following:
- mmap dma-buf fd
- for each drawing/upload cycle in CPU
1. SYNC_START ioctl
2. read/write to mmap area or a 2d sub-region of it
3. SYNC_END ioctl.
- munamp once you don't need the buffer any more
v2 (Tiago): Fix header file type names (u64 -> __u64)
v3 (Tiago): Add documentation. Use enum dma_buf_sync_flags to the begin/end
dma-buf functions. Check for overflows in start/length.
v4 (Tiago): use 2d regions for sync.
Cc: Sumit Semwal
Signed-off-by: Daniel Vetter
Signed-off-by: Tiago Vignatti
---
I'm unable to test the 2d sync properly, because begin/end access in i915
don't track mapped range for nothing.
Documentation/dma-buf-sharing.txt | 13 ++
drivers/dma-buf/dma-buf.c | 77 --
drivers/gpu/drm/i915/i915_gem_dmabuf.c | 6 ++-
include/linux/dma-buf.h| 20 +
include/uapi/linux/dma-buf.h | 57 +
5 files changed, 150 insertions(+), 23 deletions(-)
create mode 100644 include/uapi/linux/dma-buf.h
diff --git a/Documentation/dma-buf-sharing.txt
b/Documentation/dma-buf-sharing.txt
index 480c8de..8061ac0 100644
--- a/Documentation/dma-buf-sharing.txt
+++ b/Documentation/dma-buf-sharing.txt
@@ -355,6 +355,19 @@ Being able to mmap an export dma-buf buffer object has 2
main use-cases:
No special interfaces, userspace simply calls mmap on the dma-buf fd.
+ Also, the userspace might need some sort of cache coherency management e.g.
+ when CPU and GPU domains are being accessed through dma-buf at the same
+ time. To circumvent this problem there are begin/end coherency markers, that
+ forward directly to existing dma-buf device drivers vfunc hooks. Userspace
+ can make use of those markers through the DMA_BUF_IOCTL_SYNC ioctl. The
+ sequence would be used like following:
+ - mmap dma-buf fd
+ - for each drawing/upload cycle in CPU
+ 1. SYNC_START ioctl
+ 2. read/write to mmap area or a 2d sub-region of it
+ 3. SYNC_END ioctl.
+ - munamp once you don't need the buffer any more
+
2. Supporting existing mmap interfaces in importers
Similar to the motivation for kernel cpu access it is again important that
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 155c146..b6a4a06 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -251,11 +251,55 @@ out:
return events;
}
+static long dma_buf_ioctl(struct file *file,
+ unsigned int cmd, unsigned long arg)
+{
+ struct dma_buf *dmabuf;
+ struct dma_buf_sync sync;
+ enum dma_data_direction direction;
+
+ dmabuf = file->private_data;
+
+ if (!is_dma_buf_file(file))
+ return -EINVAL;
+
+ if (cmd != DMA_BUF_IOCTL_SYNC)
+ return -ENOTTY;
+
+ if (copy_from_user(&sync, (void __user *) arg, sizeof(sync)))
+ return -EFAULT;
+
+ if (sync.flags & DMA_BUF_SYNC_RW)
+ direction = DMA_BIDIRECTIONAL;
+ else if (sync.flags & DMA_BUF_SYNC_READ)
+ direction = DMA_FROM_DEVICE;
+ else if (sync.flags & DMA_BUF_SYNC_WRITE)
+ direction = DMA_TO_DEVICE;
+ else
+ return -EINVAL;
+
+ if (sync.flags & ~DMA_BUF_SYNC_VALID_FLAGS_MASK)
+ return -EINVAL;
+
+ /* TODO: check for overflowing the buffer's size - how so, checking
region by
+* region here? Maybe need to check for the other parameters as well. */
+
+ if (sync.flags & DMA_BUF_SYNC_END)
+ dma_buf_end_cpu_access(dmabuf, sync.stride_bytes,
sync.bytes_per_pixel,
+ sync.num_regions, sync.regions, direction);
+ else
+ dma_buf_begin_cpu_access(dmabuf, sync.stride_bytes,
sync.bytes_per_pixel,
+ sync.num_regions, sync.regions, direction);
+
+ return 0;
+}
+
static const struct file_operations dma_buf_fops = {
.release= dma_buf_release,
.mmap = dma_buf_mmap_internal,
.llseek = dma_buf_llseek,
.poll = dma_buf_poll,
+ .unlocked_ioctl = dma_buf_ioctl,
};
/*
@@ -539,14 +583,17 @@ EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);
* preparations. Coherency is only guaranteed in the specified range for the
* specified access direction.
* @dmabuf:[in]buffer to prepare cpu access for.
- * @start: [in]start of range for cpu access.
- * @len: [in]length of range for cpu access.
- * @
[PATCH v2 6/7] ARM: bcm2835: Add the DDC I2C controller to the device tree.
On 08/18/2015 03:54 PM, Eric Anholt wrote: > We need to use it for getting video modes over HDMI. This patch, Acked-by: Stephen Warren
[PATCH v2 7/7] ARM: bcm2835: Add VC4 to the device tree.
On 08/18/2015 03:54 PM, Eric Anholt wrote: > VC4 is the GPU (display and 3D) present on the 2835. This patch and patch 1 seem OK to me, although I'll withhold any ack until the DT binding design discussion with Rob has been resolved. I haven't looked at the OF graph bindings he mentioned so have no clue about his suggestions.
[PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
Hi Heiko,
å¨ 2015/8/24 21:03, Heiko Stuebner åé:
> Hi Yakir,
>
> Am Montag, 24. August 2015, 20:48:01 schrieb Yakir Yang:
>> å¨ 08/24/2015 12:20 PM, Krzysztof Kozlowski åé:
>>> On 24.08.2015 11:42, Yakir Yang wrote:
Hi Krzysztof,
å¨ 08/23/2015 07:43 PM, Krzysztof Kozlowski åé:
> 2015-08-24 8:23 GMT+09:00 Rob Herring :
>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang
>> wrote:
>>> Analogix dp driver is split from exynos dp driver, so we just
>>> make an copy of exynos_dp.txt, and then simplify exynos_dp.txt
>>>
>>> Beside update some exynos dtsi file with the latest change
>>> according to the devicetree binding documents.
>> You can't just change the exynos bindings and break compatibility. Is
>> there some agreement with exynos folks to do this?
> No, there is no agreement. This wasn't even sent to Exynos maintainers.
Sorry about this one, actually I have add Exynos maintainers in version
1 & version 2,
but lose some maintainers in version 3, I would fix it in bellow
versions.
> Additionally the patchset did not look interesting to me because of
> misleading subject - Documentation instead of "ARM: dts:".
>
> Yakir, please:
> 1. Provide backward compatibility. Mark old properties as deprecated
> but still support them.
Do you mean that I should keep the old properties declare in
exynos-dp.txt,
but just mark them as deprecated flag.
>>> That is one of ways how to do this. However more important is that
>>> driver should still support old bindings so such code:
>>> - if (of_property_read_u32(dp_node, "samsung,color-space",
>>> + if (of_property_read_u32(dp_node, "analogix,color-space",
>>>
>>> is probably wrong. Will the driver support old DTB in the same way as it
>>> was supporting before the change?
>> Okay, I got your means. So document is not the focus, the most important
>> is that
>> driver should support the old dts prop. If so the new analogix dp driver
>> should keep
>> the "samsung,color-space", rather then just mark it with [DEPRECATED] flag.
>>
>> But from your follow suggest, I think you agree to update driver code,
>> and just mark
>> old prop with deprecated flag. If so I think such code would not be wrong
>>
>> - if (of_property_read_u32(dp_node, "samsung,color-space",
>> + if (of_property_read_u32(dp_node, "analogix,color-space",
> In a generic driver, the property should have either none, or the analogix
> prefix. But DT-properties need to be backwards compatible, meaning an older
> Exynos devicetree should run unmodified with a newer kernel.
>
> So the common course of action is to mark the old one as deprecated but still
> test for both, so something like:
>
> if (of_property_read_u32(dp_node, "analogix,color-space",
> &dp_video_config->color_space))
> if (of_property_read_u32(dp_node, "samsung,color-space",
>&dp_video_config->color_space)) {
>
> dev_err(dev, "failed to get color-space\n");
> return ERR_PTR(-EINVAL);
> }
>
Wow, thanks a lot for your explain and code, it do help me to understand
this suggest rightly :-)
Thanks,
- Yakir
>
>
>
>
>
[alsa-devel] [PATCH V5 3/3] ASoC: AMD: add AMD ASoC ACP-I2S driver
On Mon, Aug 24, 2015 at 04:08:31PM -0400, Alex Deucher wrote: > On Fri, Aug 21, 2015 at 12:17 PM, Mark Brown wrote: > > What I'm looking for is actual code sharing where we use the same code > > for the I2S controller block or a clear and documented understanding of > > why it is not possible to share things. > Maruthi can clarify further, but it's not possible to use the > designware driver directly because: > 1. The i2s registers are within the same MMIO aperture as our other > GPU registers. Our GPU driver is designed in such a way that the > specific IP modules don't have direct access to the MMIO aperture. > They use functions provided by the core driver to access registers. > Thus the ACP IP module within the GPU driver does not have direct > access to the mmio base pointer in order to pass it on. Please explain this in more detail, shared register ranges are very common and are the sort of things MFDs are supposed to help with. > 2. The designware driver depends on the CLKDEV framework which we > don't currently support. You need to support the clock API, it's very easy to do so so there is no excuse for doing something custom here. > 3. Our hardware does not support S16_LE If you have modified the designware IP to remove this support (why would anyone do that?) it's a trivial quirk, if the restriction comes from some other part of the system like the DMA driver then the constraint will come from that part of the system. -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: Digital signature URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/f7fcd38c/attachment-0001.sig>
[PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
On Tue, 25 Aug 2015, Michal Hocko wrote: > On Fri 21-08-15 14:31:32, Eric B Munson wrote: > [...] > > I am in the middle of implementing lock on fault this way, but I cannot > > see how we will hanlde mremap of a lock on fault region. Say we have > > the following: > > > > addr = mmap(len, MAP_ANONYMOUS, ...); > > mlock(addr, len, MLOCK_ONFAULT); > > ... > > mremap(addr, len, 2 * len, ...) > > > > There is no way for mremap to know that the area being remapped was lock > > on fault so it will be locked and prefaulted by remap. How can we avoid > > this without tracking per vma if it was locked with lock or lock on > > fault? > > Yes mremap is a problem and it is very much similar to mmap(MAP_LOCKED). > It doesn't guarantee the full mlock semantic because it leaves partially > populated ranges behind without reporting any error. This was not my concern. Instead, I was wondering how to keep lock on fault sematics with mremap if we do not have a VMA flag. As a user, it would surprise me if a region I mlocked with lock on fault and then remapped to a larger size was fully populated and locked by the mremap call. > > Considering the current behavior I do not thing it would be terrible > thing to do what Konstantin was suggesting and populate only the full > ranges in a best effort mode (it is done so anyway) and document the > behavior properly. > " >If the memory segment specified by old_address and old_size is >locked (using mlock(2) or similar), then this lock is maintained >when the segment is resized and/or relocated. As a consequence, >the amount of memory locked by the process may change. > >If the range is already fully populated and the range is >enlarged the new range is attempted to be fully populated >as well to preserve the full mlock semantic but there is no >guarantee this will succeed. Partially populated (e.g. created by >mlock(MLOCK_ONFAULT)) ranges do not have the full mlock semantic >so they are not populated on resize. > " You are proposing that mremap would scan the PTEs as Vlastimil has suggested? > > So what we have as a result is that partially populated ranges are > preserved and fully populated ones work in the best effort mode the same > way as they are now. > > Does that sound at least remotely reasonably? > > > -- > Michal Hocko > SUSE Labs -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/022d9feb/attachment-0001.sig>
[PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On Tue, Aug 25, 2015 at 04:21:51PM +0200, Thierry Reding wrote: > You cited code from dw_hdmi.c earlier, it looks like it might be correct > even though it doesn't cite a reference for why this was done. Perhaps > someone on this thread, or someone involved with dw_hdmi can answer > where that code came from. dw_hdmi doesn't do any format conversion - it's hard coded to RGB, 8 bits per colour component. That's a requirement for all HDMI sinks. The reason it's hard-coded in dw_hdmi is that (a) no one has yet decided its worth the effort to get the dw_hdmi hardware to do the colourspace conversion to the YUV spaces and verify that it works, and (b) I really don't see the point when we're talking about computer like devices which work primerily with RGB and RGB is always supported by the sink. As far as greater colour depths go, the driver came from the Freescale iMX6 code base, and the hardware which feeds dw_hdmi can't do more than 8 bits per component - so going to 10, 12 or 16 bits per component is beyond what iMX6 can cope with. Hence, no one on the iMX6 side has a need for the deep colour stuff. In any case, I view this as a very low priority issue - it would be nice to have audio support on HDMI for iMX6 at some point in the next 20 years, preferably before the hardware becomes obsolete. I've been maintaining patches for this for 1.5 years now... how much longer is it going to take? My pull request to David from 15th July was ignored. My re-send of that after he returned was ignored. My reminder of it has been ignored. What's going on in DRM land? It would be nice to get _some_ kind of feedback so I know why they're not being taken, so I can fix whatever the issue is. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
[PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
Hi Krzysztof,
å¨ 2015/8/25 7:49, Krzysztof Kozlowski åé:
> On 24.08.2015 21:48, Yakir Yang wrote:
>> Hi Krzysztof,
>>
>> å¨ 08/24/2015 12:20 PM, Krzysztof Kozlowski åé:
>>> On 24.08.2015 11:42, Yakir Yang wrote:
Hi Krzysztof,
å¨ 08/23/2015 07:43 PM, Krzysztof Kozlowski åé:
> 2015-08-24 8:23 GMT+09:00 Rob Herring :
>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang
>> wrote:
>>> Analogix dp driver is split from exynos dp driver, so we just
>>> make an copy of exynos_dp.txt, and then simplify exynos_dp.txt
>>>
>>> Beside update some exynos dtsi file with the latest change
>>> according to the devicetree binding documents.
>> You can't just change the exynos bindings and break compatibility. Is
>> there some agreement with exynos folks to do this?
> No, there is no agreement. This wasn't even sent to Exynos maintainers.
Sorry about this one, actually I have add Exynos maintainers in version
1 & version 2,
but lose some maintainers in version 3, I would fix it in bellow
versions.
> Additionally the patchset did not look interesting to me because of
> misleading subject - Documentation instead of "ARM: dts:".
>
> Yakir, please:
> 1. Provide backward compatibility. Mark old properties as deprecated
> but still support them.
Do you mean that I should keep the old properties declare in
exynos-dp.txt,
but just mark them as deprecated flag.
>>> That is one of ways how to do this. However more important is that
>>> driver should still support old bindings so such code:
>>> - if (of_property_read_u32(dp_node, "samsung,color-space",
>>> + if (of_property_read_u32(dp_node, "analogix,color-space",
>>>
>>> is probably wrong. Will the driver support old DTB in the same way as it
>>> was supporting before the change?
>> Okay, I got your means. So document is not the focus, the most important
>> is that
>> driver should support the old dts prop.
> Right, the focus is on the driver.
>
>> If so the new analogix dp driver
>> should keep
>> the "samsung,color-space", rather then just mark it with [DEPRECATED] flag.
> If you are replacing a binding/property then it should be marked
> deprecated. This means that the old property is still working but new
> users of it should not be added.
Okay, so just quote Heiko's reply, such code would be need in analogix
dp driver.
if (of_property_read_u32(dp_node, "analogix,color-space",
&dp_video_config->color_space))
if (of_property_read_u32(dp_node, "samsung,color-space",
&dp_video_config->color_space)) {
dev_err(dev, "failed to get color-space\n");
return ERR_PTR(-EINVAL);
}
>> But from your follow suggest, I think you agree to update driver code,
>> and just mark
>> old prop with deprecated flag. If so I think such code would not be wrong
>>
>> - if (of_property_read_u32(dp_node, "samsung,color-space",
>> + if (of_property_read_u32(dp_node, "analogix,color-space",
> It looks wrong because it breaks backward compatibility with existing
> DTB. As I said before:
1. Provide backward compatibility. Mark old properties
as deprecated but still support them.
>
>> And actually @Rob have suggest me to remove the prefix, just use
>> "color-space" here.
> For new bindings I don't mind. But please remember about existing users,
> existing DTB and bisectability.
>
Let me show same examples, make
me understand your suggest rightly.
>>> exynos-dp already contains deprecated properties. Other ways of doing
>>> this would be:
>>> Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>>> Documentation/devicetree/bindings/rtc/s3c-rtc.txt
>>>
>>> It depends up to you. The "touchscreen" looks good because it organizes
>>> old properties in a common section. In case of exynos-dp.txt you can add
>>> at beginning of file information about new bindings and mark everything
>>> deprecated.
>> Whoops, thanks for your remind, I prefer the "touchscreen" style.
>>
1. "samsung,ycbcr-coeff" is abandoned in latest analogix-dp driver,
absolutely
I should not carry this to analogix-dp.txt document. But I should
keep this in
exynos-dp.txt document, and mark them with an little
"deprecated" flag.
[Documentation/devicetree/bindings/video/exynos_dp.txt]
Required properties for dp-controller:
[...]
-samsung,ycbcr-coeff (DEPRECATED):
YCbCr co-efficients for input video.
COLOR_YCBCR601 = 0, COLOR_YCBCR709 = 1
Is it right ?
>>> Yes, this is right.
>>>
> 2. Separate all DTS changes to a separate patch, unless bisectability
> would be hurt. Anyway you should prepare it in a such way that
> separation would be possible without breaking bisectability.
[PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On 25.08.2015 10:33, Yakir Yang wrote:
> Hi Krzysztof,
>
> å¨ 2015/8/25 7:49, Krzysztof Kozlowski åé:
>> On 24.08.2015 21:48, Yakir Yang wrote:
>>> Hi Krzysztof,
>>>
>>> å¨ 08/24/2015 12:20 PM, Krzysztof Kozlowski åé:
On 24.08.2015 11:42, Yakir Yang wrote:
> Hi Krzysztof,
>
> å¨ 08/23/2015 07:43 PM, Krzysztof Kozlowski åé:
>> 2015-08-24 8:23 GMT+09:00 Rob Herring :
>>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang
>>> wrote:
Analogix dp driver is split from exynos dp driver, so we just
make an copy of exynos_dp.txt, and then simplify exynos_dp.txt
Beside update some exynos dtsi file with the latest change
according to the devicetree binding documents.
>>> You can't just change the exynos bindings and break
>>> compatibility. Is
>>> there some agreement with exynos folks to do this?
>> No, there is no agreement. This wasn't even sent to Exynos
>> maintainers.
> Sorry about this one, actually I have add Exynos maintainers in
> version
> 1 & version 2,
> but lose some maintainers in version 3, I would fix it in bellow
> versions.
>
>> Additionally the patchset did not look interesting to me because of
>> misleading subject - Documentation instead of "ARM: dts:".
>>
>> Yakir, please:
>> 1. Provide backward compatibility. Mark old properties as deprecated
>> but still support them.
> Do you mean that I should keep the old properties declare in
> exynos-dp.txt,
> but just mark them as deprecated flag.
That is one of ways how to do this. However more important is that
driver should still support old bindings so such code:
- if (of_property_read_u32(dp_node, "samsung,color-space",
+ if (of_property_read_u32(dp_node, "analogix,color-space",
is probably wrong. Will the driver support old DTB in the same way
as it
was supporting before the change?
>>> Okay, I got your means. So document is not the focus, the most important
>>> is that
>>> driver should support the old dts prop.
>> Right, the focus is on the driver.
>>
>>> If so the new analogix dp driver
>>> should keep
>>> the "samsung,color-space", rather then just mark it with [DEPRECATED]
>>> flag.
>> If you are replacing a binding/property then it should be marked
>> deprecated. This means that the old property is still working but new
>> users of it should not be added.
>
> Okay, so just quote Heiko's reply, such code would be need in analogix
> dp driver.
>
>if (of_property_read_u32(dp_node, "analogix,color-space",
> &dp_video_config->color_space))
>if (of_property_read_u32(dp_node, "samsung,color-space",
> &dp_video_config->color_space)) {
>
> dev_err(dev, "failed to get color-space\n");
> return ERR_PTR(-EINVAL);
> }
Yes. It does not look pretty but something like this is needed.
Best regards,
Krzysztof
[PATCH 0/5] Documentation: drm: Make drm.tmpl build as pdfdoc
A series of patches to make drm.tmpl build under 'make pdfdocs'. The biggest change is patch4 which converts a large HTML table into a CALS one. It should be noted, that table renders less than ideal in the PDF - something I have not figured out how to fix. Patch1 required some changes that then required some additions. It would be good if they could be verified as suitable. This set applies against drm-intel-nightly 5606e1a after having my in-flight i915_guc_submission typo fix applied and Danilos 'Improve Markdown results' applied (which should not be needed to apply the patches, just to build the resulting pdf). Graham Whaley (5): Documentation: drm: Fix pdfdocs sect/title tags Documentation: drm: Fix pdfdocs listitem and abstract s Documentation: drm: Change to Documentation: drm: Convert KMS Properties HTML table to CALS Documentation: drm: Unify quoting methods Documentation/DocBook/drm.tmpl | 1993 1 file changed, 1011 insertions(+), 982 deletions(-) -- 2.4.3
[PATCH 1/5] Documentation: drm: Fix pdfdocs sect/title tags
Building pdfdocs shows errors with !includes and s such as: jade:/Documentation/DocBook/drm.xml:666:11:E: document type does not allow element "para" here; missing one of "glossary", "bibliography", "index" start-tag Fix by adding items and add/shuffle , and !include items. Signed-off-by: Graham Whaley --- Documentation/DocBook/drm.tmpl | 32 ++-- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index 66bc646..586f1b8 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -303,6 +303,9 @@ char *date; !Edrivers/gpu/drm/drm_pci.c !Edrivers/gpu/drm/drm_platform.c + + + Calling Registration Directly New drivers that no longer rely on the services provided by the drm_bus structure can call the low-level @@ -2352,7 +2355,7 @@ void intel_crt_init(struct drm_device *dev) - Atomic Modeset Helper Functions Reference + Atomic Modeset Helper Functions Introduction Overview !Pdrivers/gpu/drm/drm_atomic_helper.c overview @@ -2365,14 +2368,20 @@ void intel_crt_init(struct drm_device *dev) Atomic State Reset and Initialization !Pdrivers/gpu/drm/drm_atomic_helper.c atomic state reset and initialization + + + Atomic Modeset Helper Functions Reference !Iinclude/drm/drm_atomic_helper.h !Edrivers/gpu/drm/drm_atomic_helper.c + Modeset Helper Functions Introduction +!Pdrivers/gpu/drm/drm_crtc_helper.c overview + + Modeset Helper Functions Reference !Iinclude/drm/drm_crtc_helper.h !Edrivers/gpu/drm/drm_crtc_helper.c -!Pdrivers/gpu/drm/drm_crtc_helper.c overview Output Probing Helper Functions Reference @@ -2432,8 +2441,8 @@ void intel_crt_init(struct drm_device *dev) Plane Helper Reference -!Edrivers/gpu/drm/drm_plane_helper.c !Pdrivers/gpu/drm/drm_plane_helper.c overview +!Edrivers/gpu/drm/drm_plane_helper.c Tile group @@ -2449,6 +2458,9 @@ void intel_crt_init(struct drm_device *dev) Default bridge callback sequence !Pdrivers/gpu/drm/drm_bridge.c bridge callbacks + + + Bridges Function Reference !Edrivers/gpu/drm/drm_bridge.c @@ -4114,19 +4126,19 @@ int num_ioctls; GTT Fences and Swizzling !Idrivers/gpu/drm/i915/i915_gem_fence.c - - Global GTT Fence Handling + + +Global GTT Fence Handling !Pdrivers/gpu/drm/i915/i915_gem_fence.c fence register handling - - - Hardware Tiling and Swizzling Details + + +Hardware Tiling and Swizzling Details !Pdrivers/gpu/drm/i915/i915_gem_fence.c tiling swizzling details - Object Tiling IOCTLs -!Idrivers/gpu/drm/i915/i915_gem_tiling.c !Pdrivers/gpu/drm/i915/i915_gem_tiling.c buffer object tiling +!Idrivers/gpu/drm/i915/i915_gem_tiling.c Buffer Object Eviction -- 2.4.3
[PATCH 2/5] Documentation: drm: Fix pdfdocs listitem and abstract s
Fix pdfdocs errors such as: jade:/Documentation/DocBook/drm.xml:1348:20:E: character data is not allowed here by adding tags to listitems and abstracts Signed-off-by: Graham Whaley --- Documentation/DocBook/drm.tmpl | 87 +++--- 1 file changed, 48 insertions(+), 39 deletions(-) diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index 586f1b8..952eb78 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -579,9 +579,9 @@ char *date; On a fundamental level, GEM involves several operations: - Memory allocation and freeing - Command execution - Aperture management at command execution time + Memory allocation and freeing + Command execution + Aperture management at command execution time Buffer object allocation is relatively straightforward and largely provided by Linux's shmem layer, which provides memory to back each @@ -1386,20 +1386,20 @@ int max_width, max_height; The DRM core recognizes three types of planes: - + DRM_PLANE_TYPE_PRIMARY represents a "main" plane for a CRTC. Primary planes are the planes operated upon by CRTC modesetting and flipping operations described in . - - + + DRM_PLANE_TYPE_CURSOR represents a "cursor" plane for a CRTC. Cursor planes are the planes operated upon by the DRM_IOCTL_MODE_CURSOR and DRM_IOCTL_MODE_CURSOR2 ioctls. - - + + DRM_PLANE_TYPE_OVERLAY represents all non-primary, non-cursor planes. Some drivers refer to these types of planes as "sprites" internally. - + For compatibility with legacy userspace, only overlay planes are made available to userspace by default. Userspace clients may set the @@ -1503,22 +1503,22 @@ int max_width, max_height; drm_encoder_init. The function takes a pointer to the encoder functions and an encoder type. Supported types are - + DRM_MODE_ENCODER_DAC for VGA and analog on DVI-I/DVI-A - - + + DRM_MODE_ENCODER_TMDS for DVI, HDMI and (embedded) DisplayPort - - + + DRM_MODE_ENCODER_LVDS for display panels - - + + DRM_MODE_ENCODER_TVDAC for TV output (Composite, S-Video, Component, SCART) - - + + DRM_MODE_ENCODER_VIRTUAL for virtual machine displays - + @@ -1635,21 +1635,21 @@ int max_width, max_height; Supported connector types are -DRM_MODE_CONNECTOR_VGA -DRM_MODE_CONNECTOR_DVII -DRM_MODE_CONNECTOR_DVID -DRM_MODE_CONNECTOR_DVIA -DRM_MODE_CONNECTOR_Composite -DRM_MODE_CONNECTOR_SVIDEO -DRM_MODE_CONNECTOR_LVDS -DRM_MODE_CONNECTOR_Component -DRM_MODE_CONNECTOR_9PinDIN -DRM_MODE_CONNECTOR_DisplayPort -DRM_MODE_CONNECTOR_HDMIA -DRM_MODE_CONNECTOR_HDMIB -DRM_MODE_CONNECTOR_TV -DRM_MODE_CONNECTOR_eDP -DRM_MODE_CONNECTOR_VIRTUAL +DRM_MODE_CONNECTOR_VGA +DRM_MODE_CONNECTOR_DVII +DRM_MODE_CONNECTOR_DVID +DRM_MODE_CONNECTOR_DVIA +DRM_MODE_CONNECTOR_Composite +DRM_MODE_CONNECTOR_SVIDEO +DRM_MODE_CONNECTOR_LVDS +DRM_MODE_CONNECTOR_Component +DRM_MODE_CONNECTOR_9PinDIN +DRM_MODE_CONNECTOR_DisplayPort +DRM_MODE_CONNECTOR_HDMIA +DRM_MODE_CONNECTOR_HDMIB +DRM_MODE_CONNECTOR_TV +DRM_MODE_CONNECTOR_eDP +DRM_MODE_CONNECTOR_VIRTUAL @@ -3612,7 +3612,10 @@ void (*lastclose) (struct drm_device *); int (*open) (struct drm_device *, struct drm_file *); void (*preclose) (struct drm_device *, struct drm_file *); void (*postclose) (struct drm_device *, struct drm_file *); - Open and close handlers. None of those methods are mandatory. + + +Open and close handlers. None of those methods are mandatory. + The firstopen method is called by the DRM core @@ -3667,7 +3670,11 @@ void (*postclose) (struct drm_device *, struct drm_file *); File Operations const struct file_operations *fops - File operations for the DRM device node. + + + File operations for the DRM device node. + + Drivers must define the file operations s
[PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On Tue, Aug 25, 2015 at 11:12:48AM +0200, Thierry Reding wrote: > On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote: > > It goes beyond bindings IMO. The use of the component framework or not > > has been at the whim of driver writers as well. It is either used or > > private APIs are created. I'm using components and my need for it > > boils down to passing the struct drm_device pointer to the encoder. > > Other components like panels and bridges have different ways to attach > > to the DRM driver. > > I certainly support unification, but it needs to be reasonable. There > are cases where a different structure for the binding work better than > another and I think this always needs to be evaluated on a case by case > basis. It can't be a case-by-case basis. The TDA998x encoder/connector is going to be component only. This is a generic chip, which can be attached to the output of any parallel RGB+sync+clock bus. In other words, it could appear anywhere. Are you really saying that we need to support multiple schemes of attaching the driver to DRM? That's totally insane IMHO. The problem with the drm_encoder_slave stuff is that you can't sanely attach of-nodes to the drm-created i2c device. Yes, you can parse them from the DT file as a sub-node of the upper device, but that then goes against the principle of the I2C bindings, which is to list the I2C devices as a child below the I2C adapter node. If you try and put the DT node there, then the OF code will create the I2C device for you, and the drm_encoder_slave stuff won't have the control it needs to communicate through the wrapped i2c_driver stuff. So, tda998x is going component-only, as that's the _only_ sane solution for it. Now, what happens when some other DRM driver wants to use the tda998x driver, and its bindings are not compatible with the component helpers? They're pretty much stuck up the creek without a paddle. Case by case doesn't work unless you're talking about truely isolated hardware where no one shares anything. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
[PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
Hi Thierry, å¨ 2015/8/25 17:15, Thierry Reding åé: > On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote: >> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang wrote: > [...] >>> + -analogix,link-rate: >>> + max link rate supported by the eDP controller. >>> + LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 0x0A, >>> + LINK_RATE_5_40GBPS = 0x14 >> Same here. I'd rather see something like "link-rate-mbps" and use the >> actual rate. > There is no need whatsoever to hard-code this in DT. (e)DP provides the > means to detect what rate the link supports and the specification > provides guidance on how to select an appropriate one. Hmm... could you share more about this :-) I only find that drm_dp_link_probe() could get the panel link-rate and num-lanes by reading the DP_DPCD_REV messag. I don't found there are some guidance to help select the approriate one. Beside this DT prop just indicate the max eDP controller link-rate & lanes support, how could eDP detect them automatically? - Yakir >>> + -analogix,lane-count: >>> + max number of lanes supported by the eDP contoller. >>> + LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4 >> And drop the vendor prefix here. > Same as for the link rate. > > Thierry
[PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On Tue, Aug 25, 2015 at 05:41:19PM +0800, Yakir Yang wrote: > Hi Thierry, > > å¨ 2015/8/25 17:12, Thierry Reding åé: > >On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote: > >>On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux > >> wrote: > >>>On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote: > >>>>On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang wrote: > >>>>>+ -analogix,color-depth: > >>>>>+ number of bits per colour component. > >>>>>+ COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 > >>>>>= 3 > >>>>This seems pretty generic. Just use 6, 8, 10, or 12 for values. And > >>>>drop the vendor prefix. > >>>Please think about this some more. What does "color-depth" mean? Does it > >>>mean the number of bits per colour _component_, or does it mean the total > >>>number of bits to represent a particular colour. It's confusing as it > >>>stands. > >>Then "component-color-bpp" perhaps? > >There should be no need to have this in DT at all. The BPC is a property > >of the attached panel and it should come from the panel (either the > >panel driver or parsed from EDID if available). > > Actually I have send an email about this one to you in version 2, just past > from that email: > > "samsung,color_space" and "samsung,color-depth" > > The drm_display_info's color_formats and bpc indicate the monitor display > ability, but > the edp driver could not take it as input video format directly. > > For example, with my DP TV I would found "RGB444 & YCRCB422 & & YCRCB444" > support in drm_display_info.color_formats and 16bit bpc support, but RK3288 > crtc > driver could only output RGB & ITU formats, so finally analogix_dp-rockchip > driver > config crtc to RGBaaa 10bpc mode. > > In this sutiation, the analogix_dp core driver would pazzled by the > drm_display_info, > can't chose the right color_space and bpc. > > And this is the place that confused me, wish you could give some ideas about > this one :-) Your display driver should choose whatever it is capable of outputting. If the display reports that it can do 16 bits-per-color, but your display driver can't do it, then it should choose a configuration that it supports. Similarily for the color encodings. If you can't generate YCrCb444 with your hardware, then it's the driver's job to know about that and select the next appropriate configuration. But hard-coding this is not the right solution because the value in DT may end up conflicting with what the display reports. Thierry -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/6976039d/attachment-0001.sig>
[PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On Tue, Aug 25, 2015 at 10:29:39AM +0100, Russell King - ARM Linux wrote: > On Tue, Aug 25, 2015 at 11:12:48AM +0200, Thierry Reding wrote: > > On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote: > > > It goes beyond bindings IMO. The use of the component framework or not > > > has been at the whim of driver writers as well. It is either used or > > > private APIs are created. I'm using components and my need for it > > > boils down to passing the struct drm_device pointer to the encoder. > > > Other components like panels and bridges have different ways to attach > > > to the DRM driver. > > > > I certainly support unification, but it needs to be reasonable. There > > are cases where a different structure for the binding work better than > > another and I think this always needs to be evaluated on a case by case > > basis. > > It can't be a case-by-case basis. > > The TDA998x encoder/connector is going to be component only. This is > a generic chip, which can be attached to the output of any parallel > RGB+sync+clock bus. In other words, it could appear anywhere. > > Are you really saying that we need to support multiple schemes of > attaching the driver to DRM? That's totally insane IMHO. No, what I'm saying is that we should have a single scheme, but one that doesn't put any restrictions on what kind of DT binding you use or how your driver is architected. > The problem with the drm_encoder_slave stuff is that you can't sanely > attach of-nodes to the drm-created i2c device. Yes, you can parse > them from the DT file as a sub-node of the upper device, but that > then goes against the principle of the I2C bindings, which is to > list the I2C devices as a child below the I2C adapter node. If you > try and put the DT node there, then the OF code will create the I2C > device for you, and the drm_encoder_slave stuff won't have the > control it needs to communicate through the wrapped i2c_driver > stuff. > > So, tda998x is going component-only, as that's the _only_ sane solution > for it. Has anyone ever considered turning it into a DRM bridge driver? I had always envisioned component/master to be primarily useful to glue together various SoC components to form one componentized device. Now if tda998x is an I2C slave it is external to the SoC (auxiliary), so in my opinion much better off as a bridge driver. Bridge drivers don't come with any of the disadvantages that the drm_encoder_slave stuff has. They are regular drivers that are probed via their parent busses (I2C, platform, SPI, ...) and hook into DRM via an abstract interface. The DT aspect is taken care of automatically because they get instantiated by their parent bus like any other device. > Now, what happens when some other DRM driver wants to use the tda998x > driver, and its bindings are not compatible with the component helpers? > They're pretty much stuck up the creek without a paddle. I'm sure that will be very helpful response for whoever's going to end up having to deal with that situation. > Case by case doesn't work unless you're talking about truely isolated > hardware where no one shares anything. There are two different things here. The inter-driver interface, which, in my opinion, it makes a lot of sense to standardize. Like I mentioned above I think it unwise to make this interface depend upon a framework or the firmware description such as DT in order to avoid unnecessary restrictions. The second, orthogonal, issue, is the DT bindings. Those I think should absolutely be designed case by case and select whatever most accurately describes the hardware. Thierry -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/de0cd192/attachment-0001.sig>
[PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On Tue, Aug 25, 2015 at 12:40:01PM +0200, Thierry Reding wrote: > On Tue, Aug 25, 2015 at 10:29:39AM +0100, Russell King - ARM Linux wrote: > > Now, what happens when some other DRM driver wants to use the tda998x > > driver, and its bindings are not compatible with the component helpers? > > They're pretty much stuck up the creek without a paddle. > > I'm sure that will be very helpful response for whoever's going to end > up having to deal with that situation. Thank you for that comment, it's very constructive and much appreciated. I can see it's well worth me continuing to spend time on this thread. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
[PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
Hi Thierry & Rob, å¨ 2015/8/25 21:27, Rob Herring åé: > On Tue, Aug 25, 2015 at 4:15 AM, Thierry Reding wrote: >> On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote: >>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang wrote: >> [...] >>>> + -analogix,link-rate: >>>> + max link rate supported by the eDP controller. >>>> + LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = >>>> 0x0A, >>>> + LINK_RATE_5_40GBPS = 0x14 >>> Same here. I'd rather see something like "link-rate-mbps" and use the >>> actual rate. >> There is no need whatsoever to hard-code this in DT. (e)DP provides the >> means to detect what rate the link supports and the specification >> provides guidance on how to select an appropriate one. > Good, even better. I do think we still need keep this DT prop yet. I think drm_dp_help.c could get the "panel" max link-rate and lane-count, but it's not enough, we still need knew the "eDP controller" max link-rate and lane-count. Let me show the exact example that happened in my side. When I connect my board to my 2K DP-1.2 TV. Analogix dp driver would get the max link-rate from dpcd, and the max link-rate is 5.4Gbps. So if I just set eDP controller link-rate to 5.4Gbps, the DP TV just broken, do not light up normally. This reason why TV broken is the max link-rate which support by RK3288 eDP controller is 2.7Gbps. Here are the exact words that RK3288 eDP TRM said: *ï¬ï Compliant with DisplayPortTM Specification, Version 1.2. ï¬ï Compliant with eDPTM Specification, Version 1.3. ï¬ï HDCP v1.3 amendment for DisplayPortTM Revision 1.0. ï¬ï Main link containing 4 physical lanes of 2.7/1.62 Gbps/lane * ** Beside I haven't found there are some registers would indicate the eDP controller max link-rate and lane-count, so this is why I still instance that we need this DT prop to indicata "Max rate controller support". So, I wish you could agree with me on this point. Thanks, - Yakir > Rob > > > -- next part -- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/fd4c3c99/attachment-0001.html>
[PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
On 08/25/2015 03:41 PM, Michal Hocko wrote: > On Fri 21-08-15 14:31:32, Eric B Munson wrote: > [...] >> I am in the middle of implementing lock on fault this way, but I cannot >> see how we will hanlde mremap of a lock on fault region. Say we have >> the following: >> >> addr = mmap(len, MAP_ANONYMOUS, ...); >> mlock(addr, len, MLOCK_ONFAULT); >> ... >> mremap(addr, len, 2 * len, ...) >> >> There is no way for mremap to know that the area being remapped was lock >> on fault so it will be locked and prefaulted by remap. How can we avoid >> this without tracking per vma if it was locked with lock or lock on >> fault? > > Yes mremap is a problem and it is very much similar to mmap(MAP_LOCKED). > It doesn't guarantee the full mlock semantic because it leaves partially > populated ranges behind without reporting any error. Hm, that's right. > Considering the current behavior I do not thing it would be terrible > thing to do what Konstantin was suggesting and populate only the full > ranges in a best effort mode (it is done so anyway) and document the > behavior properly. > " > If the memory segment specified by old_address and old_size is > locked (using mlock(2) or similar), then this lock is maintained > when the segment is resized and/or relocated. As a consequence, > the amount of memory locked by the process may change. > > If the range is already fully populated and the range is > enlarged the new range is attempted to be fully populated > as well to preserve the full mlock semantic but there is no > guarantee this will succeed. Partially populated (e.g. created by > mlock(MLOCK_ONFAULT)) ranges do not have the full mlock semantic > so they are not populated on resize. > " > > So what we have as a result is that partially populated ranges are > preserved and fully populated ones work in the best effort mode the same > way as they are now. > > Does that sound at least remotely reasonably? I'll basically repeat what I said earlier: - mremap scanning existing pte's to figure out the population would slow it down for no good reason - it would be unreliable anyway: - example: was the area completely populated because MLOCK_ONFAULT was not used or because the process faulted it already - example: was the area not completely populated because MLOCK_ONFAULT was used, or because mmap(MAP_LOCKED) failed to populate it fully? I think the first point is a pointless regression for workloads that use just plain mlock() and don't want the onfault semantics. Unless there's some shortcut? Does vma have a counter of how much is populated? (I don't think so?)
[PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
On Tue, Aug 25, 2015 at 4:41 PM, Michal Hocko wrote: > On Fri 21-08-15 14:31:32, Eric B Munson wrote: > [...] >> I am in the middle of implementing lock on fault this way, but I cannot >> see how we will hanlde mremap of a lock on fault region. Say we have >> the following: >> >> addr = mmap(len, MAP_ANONYMOUS, ...); >> mlock(addr, len, MLOCK_ONFAULT); >> ... >> mremap(addr, len, 2 * len, ...) >> >> There is no way for mremap to know that the area being remapped was lock >> on fault so it will be locked and prefaulted by remap. How can we avoid >> this without tracking per vma if it was locked with lock or lock on >> fault? > > Yes mremap is a problem and it is very much similar to mmap(MAP_LOCKED). > It doesn't guarantee the full mlock semantic because it leaves partially > populated ranges behind without reporting any error. > > Considering the current behavior I do not thing it would be terrible > thing to do what Konstantin was suggesting and populate only the full > ranges in a best effort mode (it is done so anyway) and document the > behavior properly. > " >If the memory segment specified by old_address and old_size is >locked (using mlock(2) or similar), then this lock is maintained >when the segment is resized and/or relocated. As a consequence, >the amount of memory locked by the process may change. > >If the range is already fully populated and the range is >enlarged the new range is attempted to be fully populated >as well to preserve the full mlock semantic but there is no >guarantee this will succeed. Partially populated (e.g. created by >mlock(MLOCK_ONFAULT)) ranges do not have the full mlock semantic >so they are not populated on resize. > " > > So what we have as a result is that partially populated ranges are > preserved and fully populated ones work in the best effort mode the same > way as they are now. > > Does that sound at least remotely reasonably? The problem is that mremap have to scan ptes to detect that and old behaviour becomes very fragile: one fail and mremap will never populate that vma again. For now I think new flag "MREMAP_NOPOPULATE" is a better option. > > > -- > Michal Hocko > SUSE Labs > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majordomo at kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: mailto:"dont at kvack.org"> email at kvack.org
[PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On Tue, Aug 25, 2015 at 10:03:52PM +0800, Yakir Yang wrote: > Hi Thierry, > > å¨ 2015/8/25 17:58, Thierry Reding åé: > >On Wed, Aug 19, 2015 at 09:50:34AM -0500, Yakir Yang wrote: > >[...] > >>+ -analogix,color-space: > >>+ input video data format. > >>+ COLOR_RGB = 0, COLOR_YCBCR422 = 1, COLOR_YCBCR444 = 2 > >I don't think DT is an appropriate place to set this. To my knowledge > >this depends on the display and/or mode, so I don't think hard-coding > >it here is the right thing to do. > > Yeah, same question with my previous reply ;) I don't have an answer for you, unfortunately. But like I said, hard-coding isn't going to work. What if, for example, you set this to a fixed value and then you connect a monitor that doesn't support the specific one you set? You cited code from dw_hdmi.c earlier, it looks like it might be correct even though it doesn't cite a reference for why this was done. Perhaps someone on this thread, or someone involved with dw_hdmi can answer where that code came from. Thierry -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/79ae0ecb/attachment-0001.sig>
[Intel-gfx] [PATCH 1/5] Documentation: drm: Fix pdfdocs sect/title tags
On Tue, 2015-08-25 at 13:34 +0200, Daniel Vetter wrote: > On Tue, Aug 25, 2015 at 10:26:41AM +0100, Graham Whaley wrote: > > Building pdfdocs shows errors with !includes and s such as: > > jade:/Documentation/DocBook/drm.xml:666:11:E: document type does > > not > > allow element "para" here; missing one of "glossary", > > "bibliography", > > "index" start-tag > > Fix by adding items and add/shuffle , and > > !include > > items. > > > > Signed-off-by: Graham Whaley > > The idea behind having both the overview sections and the detailed > function references in the same section is to have them in the same > section. Is there nothing we can do to salvage that? At least you > seem to > add a lot more sections here, but it doesn't look entirely > consistent. > -Daniel Let me go re-visit and have a look. I suspect there is an underlying rule or feature of the processing that either has restrictions on include ordering or section depth when including or both. I'll search to see if there are any examples that currently work as we'd like these to. Thanks for the feedback. Graham > > > --- > > Documentation/DocBook/drm.tmpl | 32 ++ > > -- > > 1 file changed, 22 insertions(+), 10 deletions(-) > > > > diff --git a/Documentation/DocBook/drm.tmpl > > b/Documentation/DocBook/drm.tmpl > > index 66bc646..586f1b8 100644 > > --- a/Documentation/DocBook/drm.tmpl > > +++ b/Documentation/DocBook/drm.tmpl > > @@ -303,6 +303,9 @@ char *date; > > > > !Edrivers/gpu/drm/drm_pci.c > > !Edrivers/gpu/drm/drm_platform.c > > + > > + > > + Calling Registration Directly > > > > New drivers that no longer rely on the services provided > > by the > > drm_bus structure can call the > > low-level > > @@ -2352,7 +2355,7 @@ void intel_crt_init(struct drm_device *dev) > > > > > > > > - Atomic Modeset Helper Functions Reference > > + Atomic Modeset Helper Functions Introduction > > > > Overview > > !Pdrivers/gpu/drm/drm_atomic_helper.c overview > > @@ -2365,14 +2368,20 @@ void intel_crt_init(struct drm_device *dev) > > Atomic State Reset and Initialization > > !Pdrivers/gpu/drm/drm_atomic_helper.c atomic state reset and > > initialization > > > > + > > + > > + Atomic Modeset Helper Functions Reference > > !Iinclude/drm/drm_atomic_helper.h > > !Edrivers/gpu/drm/drm_atomic_helper.c > > > > > > + Modeset Helper Functions Introduction > > +!Pdrivers/gpu/drm/drm_crtc_helper.c overview > > + > > + > >Modeset Helper Functions Reference > > !Iinclude/drm/drm_crtc_helper.h > > !Edrivers/gpu/drm/drm_crtc_helper.c > > -!Pdrivers/gpu/drm/drm_crtc_helper.c overview > > > > > >Output Probing Helper Functions Reference > > @@ -2432,8 +2441,8 @@ void intel_crt_init(struct drm_device *dev) > > > > > >Plane Helper > > Reference > > -!Edrivers/gpu/drm/drm_plane_helper.c > > !Pdrivers/gpu/drm/drm_plane_helper.c overview > > +!Edrivers/gpu/drm/drm_plane_helper.c > > > > > > Tile group > > @@ -2449,6 +2458,9 @@ void intel_crt_init(struct drm_device *dev) > > Default bridge callback sequence > > !Pdrivers/gpu/drm/drm_bridge.c bridge callbacks > > > > + > > + > > + Bridges Function Reference > > !Edrivers/gpu/drm/drm_bridge.c > > > > > > @@ -4114,19 +4126,19 @@ int num_ioctls; > > > > GTT Fences and Swizzling > > !Idrivers/gpu/drm/i915/i915_gem_fence.c > > - > > - Global GTT Fence Handling > > + > > + > > +Global GTT Fence Handling > > !Pdrivers/gpu/drm/i915/i915_gem_fence.c fence register handling > > - > > - > > - Hardware Tiling and Swizzling Details > > + > > + > > +Hardware Tiling and Swizzling Details > > !Pdrivers/gpu/drm/i915/i915_gem_fence.c tiling swizzling details > > - > > > > > > Object Tiling IOCTLs > > -!Idrivers/gpu/drm/i915/i915_gem_tiling.c > > !Pdrivers/gpu/drm/i915/i915_gem_tiling.c buffer object tiling > > +!Idrivers/gpu/drm/i915/i915_gem_tiling.c > > > > > > Buffer Object Eviction > > -- > > 2.4.3 > > > > ___ > > Intel-gfx mailing list > > Intel-gfx at lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
[alsa-devel] [PATCH V5 3/3] ASoC: AMD: add AMD ASoC ACP-I2S driver
On Tue, Aug 25, 2015 at 03:26:54PM +0530, maruthi srinivas wrote: > On Tue, Aug 25, 2015 at 11:36 AM, Mark Brown wrote: > > Please explain this in more detail, shared register ranges are very > > common and are the sort of things MFDs are supposed to help with. > In our case, ACP I2S driver need not do a 'devm_ioremap_resource' to > get mmio base. That sounds like a MFD type problem... > ACP audio IP (DMA + I2S+ Others) registers can be accessed, using > GPU's MMIO base. > During GPU driver design, it was decided that all the register access > for entire GPU MMIO > aperture (includes ACP and others) to be done in GPU module only. > This is implemented in another patch in this patch series using a > abstraction layer. That sounds like converting the Designware driver to use regmap and providing a regmap would enable code sharing (you can provide a regmap for accessors if you don't use it in the main driver). > >> 2. The designware driver depends on the CLKDEV framework which we > >> don't currently support. > > You need to support the clock API, it's very easy to do so so there is > > no excuse for doing something custom here. > Codec acts as master in our case to provide clock to i2s controller and > there wasn't a need to use clock APIs unlike in existing designware i2s > driver. > There is no custom implementation. So you just need to add slave mode support to the driver. Again not a reason to just copy the code. > >> 3. Our hardware does not support S16_LE > > If you have modified the designware IP to remove this support (why would > > anyone do that?) it's a trivial quirk, if the restriction comes from > > some other part of the system like the DMA driver then the constraint > > will come from that part of the system. > There is a bug in ACP SoC implementation (which combines internal DMA, > designware I2S > and other blocks) for 16bit and lower resolution. I felt , it would be > better to limit functionality > in I2S DAI capabilities. I will put this limitation in DMA driver > capabilities, to represent overall > sound card capabilities, if you suggest. A quirk would also do the job. -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: Digital signature URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/12a21b24/attachment-0001.sig>
[PATCH 1/7] drm/vc4: Add devicetree bindings for VC4.
On Tue, Aug 25, 2015 at 3:42 PM, Rob Clark wrote: > On Mon, Aug 24, 2015 at 9:47 AM, Rob Herring wrote: >> On Mon, Aug 17, 2015 at 1:30 PM, Eric Anholt wrote: >>> Stephen Warren writes: >>> On 08/12/2015 06:56 PM, Eric Anholt wrote: > Signed-off-by: Eric Anholt This one definitely needs a patch description, since someone might not know what a VC4 is, and "git log" won't show the text from the binding doc itself. I'd suggest adding the initial paragraph of the binding doc as the patch description, or more. > diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt > b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt >>> > +- hvss: List of references to HVS video scalers > +- encoders: List of references to output encoders (HDMI, SDTV) Would it make sense to make all those nodes child node of the vc4 object. That way, there's no need to have these lists of objects; they can be automatically built up as the DT is enumerated. I know that e.g. the NVIDIA Tegra host1x binding works this way, and I think it may have been inspired by other similar cases. >>> >>> I've looked at tegra, and the component system used by msm appears to be >>> nicer than it. To follow tegra's model, it looks like I need to build >>> this extra bus thing corresponding to host1x that is effectively the >>> drivers/base/component.c code, so that I can get at vc4's structure from >>> the component drivers. >>> Of course, this is only appropriate if the HW modules really are logically children of the VC4 HW module. Perhaps they aren't. If they aren't though, I wonder what this "vc4" module actually represents in HW? >>> >>> It's the subsystem, same as we use a subsystem node for msm, sti, >>> rockchip, imx, and exynos. This appears to be the common model of how >>> the collection of graphics-related components is represented in the DT. >> >> I think most of these bindings are wrong. They are grouped together >> because that is what DRM wants not because that reflects the h/w. So >> convince me this is one block, not that it is what other people do. > > I think, when it comes to more complex driver subsystems (like drm in > particular) we have a bit of mismatch between how things look from the > "pure hw ignoring sw" perspective, and the "how sw and in particular > userspace expects things" perspective. Maybe it is less a problem in > other subsystems, where bindings map to things that are only visible > in the kernel, or well defined devices like uart or sata controller. > But when given the choice, I'm going to err on the side of not > confusing userspace and the large software stack that sits above > drm/kms, over dt purity. I wasn't implying that this should get exposed to userspace as components. V4L2 has gone that route with media controller and sub-devs. Perhaps that is needed for DRM, perhaps not. For the moment, I definitely agree the kernel should hide most/all of those details, but I don't think that means DT has to hide the details or know what components are handled by a single driver. My point was that on the DT side we have a mixture of OF graph usage, parent-child nodes or custom phandles (this case) to describe the relationships between h/w components. That's not necessarily wrong, but we should have some rules around how certain relationships are described. Then in the drivers we have a mixture of deferred probe, component API, and custom inter-module APIs to control init order. We then have a mixture of all those which leads to very few if any drivers having the same overall structure that could be shared. Should we mandate using the component API for h/w that is discrete blocks? Should we throw out the component API for something else? Can we tie the graph parsing and component API together with common code? > Maybe it would be nice to have a sort of dt overlay that adds the bits > needed to tie together hw blocks that should be assembled into a > single logical device for linux and userspace (but maybe not some > other hypothetical operating system). But so far that doesn't exist. OF graph is supposed to do this. OF graph is a double edged sword. It is very flexible, but then each platform can do something different. We need to have some level of requirements around how the OF graph is used. As an example, any system with an HDMI connector should have an "hdmi-connector" compatible node or encoder/bridge chips/blocks must have certain ports defined. > All we have is a hammer (devicetree), everything looks like a nail. > End result is we end up adding some things in the bindings which > aren't purely about the hw. Until someone invents a screwdriver, I'm > not sure what else to do. In the end, other hypothetical OS is free > to ignore those extra fields in the bindings if it doesn't need them. > So meh? We really want to err on the side of fewer bindings, not more as once used
[PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On 24.08.2015 21:48, Yakir Yang wrote: > Hi Krzysztof, > > å¨ 08/24/2015 12:20 PM, Krzysztof Kozlowski åé: >> On 24.08.2015 11:42, Yakir Yang wrote: >>> Hi Krzysztof, >>> >>> å¨ 08/23/2015 07:43 PM, Krzysztof Kozlowski åé: 2015-08-24 8:23 GMT+09:00 Rob Herring : > On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang > wrote: >> Analogix dp driver is split from exynos dp driver, so we just >> make an copy of exynos_dp.txt, and then simplify exynos_dp.txt >> >> Beside update some exynos dtsi file with the latest change >> according to the devicetree binding documents. > You can't just change the exynos bindings and break compatibility. Is > there some agreement with exynos folks to do this? No, there is no agreement. This wasn't even sent to Exynos maintainers. >>> Sorry about this one, actually I have add Exynos maintainers in version >>> 1 & version 2, >>> but lose some maintainers in version 3, I would fix it in bellow >>> versions. >>> Additionally the patchset did not look interesting to me because of misleading subject - Documentation instead of "ARM: dts:". Yakir, please: 1. Provide backward compatibility. Mark old properties as deprecated but still support them. >>> Do you mean that I should keep the old properties declare in >>> exynos-dp.txt, >>> but just mark them as deprecated flag. >> That is one of ways how to do this. However more important is that >> driver should still support old bindings so such code: >> - if (of_property_read_u32(dp_node, "samsung,color-space", >> + if (of_property_read_u32(dp_node, "analogix,color-space", >> >> is probably wrong. Will the driver support old DTB in the same way as it >> was supporting before the change? > > Okay, I got your means. So document is not the focus, the most important > is that > driver should support the old dts prop. Right, the focus is on the driver. > If so the new analogix dp driver > should keep > the "samsung,color-space", rather then just mark it with [DEPRECATED] flag. If you are replacing a binding/property then it should be marked deprecated. This means that the old property is still working but new users of it should not be added. > > But from your follow suggest, I think you agree to update driver code, > and just mark > old prop with deprecated flag. If so I think such code would not be wrong > > - if (of_property_read_u32(dp_node, "samsung,color-space", > + if (of_property_read_u32(dp_node, "analogix,color-space", It looks wrong because it breaks backward compatibility with existing DTB. As I said before: >>> 1. Provide backward compatibility. Mark old properties >>> as deprecated but still support them. > And actually @Rob have suggest me to remove the prefix, just use > "color-space" here. For new bindings I don't mind. But please remember about existing users, existing DTB and bisectability. > >> >>> Let me show same examples, make >>> me understand your suggest rightly. >> exynos-dp already contains deprecated properties. Other ways of doing >> this would be: >> Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt >> Documentation/devicetree/bindings/rtc/s3c-rtc.txt >> >> It depends up to you. The "touchscreen" looks good because it organizes >> old properties in a common section. In case of exynos-dp.txt you can add >> at beginning of file information about new bindings and mark everything >> deprecated. > > Whoops, thanks for your remind, I prefer the "touchscreen" style. > >>> 1. "samsung,ycbcr-coeff" is abandoned in latest analogix-dp driver, >>> absolutely >>> I should not carry this to analogix-dp.txt document. But I should >>> keep this in >>> exynos-dp.txt document, and mark them with an little >>> "deprecated" flag. >>> >>> [Documentation/devicetree/bindings/video/exynos_dp.txt] >>> Required properties for dp-controller: >>> [...] >>> -samsung,ycbcr-coeff (DEPRECATED): >>> YCbCr co-efficients for input video. >>> COLOR_YCBCR601 = 0, COLOR_YCBCR709 = 1 >>> >>> Is it right ? >> Yes, this is right. >> 2. Separate all DTS changes to a separate patch, unless bisectability would be hurt. Anyway you should prepare it in a such way that separation would be possible without breaking bisectability. >>> So I should separate this patch into two parts, one is name "Document:", >>> the other is "ARM: dts: ". >> Yes. >> >>> Honestly, I don't understand what the "bisectability" means in this >>> case. >> I was referring to bisectability in general. The patchset should be >> fully bisectable which means that it does not introduce any issues >> during "git bisect". This effectively means that at each intermediate >> step (after applying each patch, one by one) every existing stuff works >> the same as previously without any regression. Including booting with >> old DTB. > > Oh, thanks for your careful explain, so I guess your first comment is > talk
[PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
å¨ 2015/8/24 22:48, Rob Herring åé:
> On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux
> wrote:
>> On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote:
>>> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang wrote:
+ -analogix,color-depth:
+ number of bits per colour component.
+ COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 =
3
>>> This seems pretty generic. Just use 6, 8, 10, or 12 for values. And
>>> drop the vendor prefix.
>> Please think about this some more. What does "color-depth" mean? Does it
>> mean the number of bits per colour _component_, or does it mean the total
>> number of bits to represent a particular colour. It's confusing as it
>> stands.
> Then "component-color-bpp" perhaps?
Actually this "color-bpp" should come from crtc driver, maybe should
come from
"struct drm_crtc {".
Like rockchip stuffs, analogix_dp-rockchip call an mode_config from
rockchip_drm_vop
driver and set output mode to RGB[10:10:10], then vop driver just store
the output mode
type to the private struct "vop->connecot_out_mode". do think that this
outmode should
store into crtc, not just come from DT prop.
- Yakir
[PATCH v5 4/6] drm: bridge/dw_hdmi-i2s-audio: add audio driver
On Sat, Aug 08, 2015 at 04:35:41PM +0100, Russell King - ARM Linux wrote: > On Sat, Jun 20, 2015 at 12:28:15AM +0800, Yakir Yang wrote: > > Add ALSA based HDMI I2S audio driver for dw_hdmi. Sound card > > driver could connect to this codec through the codec dai name > > "dw-hdmi-i2s-audio". > I'm applying this patch to my tree with some changes. I haven't seen > anyone ack it, nor review it any further. I'm hoping someone can ack > this patch with the following changes below - I'll be posting it as > part of my dw-hdmi devel updates later today. I'm pretty sure I deleted this patch unread because what shows up in my inbox is: -> 197 C 08/08 Russell King - (1.1K) â ââ>Re: [PATCH v5 4/6] drm: bridge/dw_ so there's no indication that it's anything to do with audio :( -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 473 bytes Desc: Digital signature URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/9c435d3b/attachment-0001.sig>
[PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote: > On Mon, Aug 24, 2015 at 7:57 AM, Russell King - ARM Linux > wrote: > > On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote: > >> On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang wrote: > >> > + -analogix,color-depth: > >> > + number of bits per colour component. > >> > + COLOR_6 = 0, COLOR_8 = 1, COLOR_10 = 2, COLOR_12 > >> > = 3 > >> > >> This seems pretty generic. Just use 6, 8, 10, or 12 for values. And > >> drop the vendor prefix. > > > > Please think about this some more. What does "color-depth" mean? Does it > > mean the number of bits per colour _component_, or does it mean the total > > number of bits to represent a particular colour. It's confusing as it > > stands. > > Then "component-color-bpp" perhaps? There should be no need to have this in DT at all. The BPC is a property of the attached panel and it should come from the panel (either the panel driver or parsed from EDID if available). > > When we adopted the graph bindings for iMX DRM, I thought exactly at that > > time "it would be nice if this could become the standard for binding DRM > > components together" but I don't have the authority from either the DT > > perspective or the DRM perspective to mandate that. Neither does anyone > > else. That's the _real_ problem here. > > > > I've seen several DRM bindings go by which don't use the of-graph stuff, > > which means that they'll never be compatible with generic components > > which do use the of-graph stuff. > > It goes beyond bindings IMO. The use of the component framework or not > has been at the whim of driver writers as well. It is either used or > private APIs are created. I'm using components and my need for it > boils down to passing the struct drm_device pointer to the encoder. > Other components like panels and bridges have different ways to attach > to the DRM driver. I certainly support unification, but it needs to be reasonable. There are cases where a different structure for the binding work better than another and I think this always needs to be evaluated on a case by case basis. Because of that I think it makes sense to make all these framework bits opt-in, otherwise we could easily end up in a situation where drivers have to be rearchitected (or even DT bindings altered!) in order to be able to reuse code. Thierry -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/6154bc96/attachment-0001.sig>
[PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp
On Sun, Aug 23, 2015 at 06:23:14PM -0500, Rob Herring wrote: > On Wed, Aug 19, 2015 at 9:50 AM, Yakir Yang wrote: [...] > > + -analogix,link-rate: > > + max link rate supported by the eDP controller. > > + LINK_RATE_1_62GBPS = 0x6, LINK_RATE_2_70GBPS = 0x0A, > > + LINK_RATE_5_40GBPS = 0x14 > > Same here. I'd rather see something like "link-rate-mbps" and use the > actual rate. There is no need whatsoever to hard-code this in DT. (e)DP provides the means to detect what rate the link supports and the specification provides guidance on how to select an appropriate one. > > > + -analogix,lane-count: > > + max number of lanes supported by the eDP contoller. > > + LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4 > > And drop the vendor prefix here. Same as for the link rate. Thierry -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150825/6f34dcce/attachment-0001.sig>
[PATCH 3/5] Documentation: drm: Change to
Fix pdfdocs errors such as: jade:/Documentation/DocBook/drm.xml:10084:13:E: element "code" undefined by changing tags to tags. It looks like tags were introduced in DocBook v4.3, and kernel DocBook is v4.1.2. I would have used , but that introduces undesirable breaks into the paragraph flow. Signed-off-by: Graham Whaley --- Documentation/DocBook/drm.tmpl | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index 952eb78..2e05a79 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -1198,14 +1198,14 @@ int max_width, max_height; Beside some lookup structures with their own locking (which is hidden behind the interface functions) most of the modeset state is protected - by the dev-fb field to the new framebuffer pointed to -by fb. This is important so that the reference counting +drm_crtc->fb field to the new framebuffer pointed to +by fb. This is important so that the reference counting on framebuffers stays balanced. -- 2.4.3
[PATCH 4/5] Documentation: drm: Convert KMS Properties HTML table to CALS
The KMS Properties table is in HTML format, which is not supported
for building pdfdocs, resulting in the following types of errors:
jade:/Documentation/DocBook/drm.xml:34413:15:E: there is no attribute
"border"
jade:/Documentation/DocBook/drm.xml:34413:31:E: there is no attribute
"cellpadding"
jade:/Documentation/DocBook/drm.xml:34413:47:E: there is no attribute
"cellspacing"
jade:/Documentation/DocBook/drm.xml:34414:7:E: document type does not
allow element "tbody" here
Convert the table over to a CALS format table
Signed-off-by: Graham Whaley
---
Documentation/DocBook/drm.tmpl | 1866
1 file changed, 937 insertions(+), 929 deletions(-)
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 2e05a79..e5bfdd8 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2580,935 +2580,943 @@ void intel_crt_init(struct drm_device *dev)
and an initial instance value.
- Existing KMS Properties
-
- The following table gives description of drm properties exposed by
various
- modules/drivers.
-
-
-
-
- Owner Module/Drivers
- Group
- Property Name
- Type
- Property Values
- Object attached
- Description/Restrictions
-
-
- DRM
- Generic
- ârotationâ
- BITMASK
- { 0, "rotate-0" },
- { 1, "rotate-90" },
- { 2, "rotate-180" },
- { 3, "rotate-270" },
- { 4, "reflect-x" },
- { 5, "reflect-y" }
- CRTC, Plane
- rotate-(degrees) rotates the image by the specified
amount in degrees
- in counter clockwise direction. reflect-x and reflect-y reflects the
- image along the specified axis prior to rotation
-
-
- Connector
- âEDIDâ
- BLOB | IMMUTABLE
- 0
- Connector
- Contains id of edid blob ptr object.
-
-
- âDPMSâ
- ENUM
- { âOnâ, âStandbyâ, âSuspendâ, âOffâ
}
- Connector
- Contains DPMS operation mode value.
-
-
- âPATHâ
- BLOB | IMMUTABLE
- 0
- Connector
- Contains topology path to a connector.
-
-
- âTILEâ
- BLOB | IMMUTABLE
- 0
- Connector
- Contains tiling information for a connector.
-
-
- âCRTC_IDâ
- OBJECT
- DRM_MODE_OBJECT_CRTC
- Connector
- CRTC that connector is attached to (atomic)
-
-
- Plane
- âtypeâ
- ENUM | IMMUTABLE
- { "Overlay", "Primary", "Cursor" }
- Plane
- Plane type
-
-
- âSRC_Xâ
- RANGE
- Min=0, Max=UINT_MAX
- Plane
- Scanout source x coordinate in 16.16 fixed point
(atomic)
-
-
- âSRC_Yâ
- RANGE
- Min=0, Max=UINT_MAX
- Plane
- Scanout source y coordinate in 16.16 fixed point
(atomic)
-
-
- âSRC_Wâ
- RANGE
- Min=0, Max=UINT_MAX
- Plane
- Scanout source width in 16.16 fixed point
(atomic)
-
-
- âSRC_Hâ
- RANGE
- Min=0, Max=UINT_MAX
- Plane
- Scanout source height in 16.16 fixed point
(atomic)
-
-
- âCRTC_Xâ
- SIGNED_RANGE
- Min=INT_MIN, Max=INT_MAX
- Plane
- Scanout CRTC (destination) x coordinate (atomic)
-
-
- âCRTC_Yâ
- SIGNED_RANGE
- Min=INT_MIN, Max=INT_MAX
- Plane
- Scanout CRTC (destination) y coordinate (atomic)
-
-
- âCRTC_Wâ
- RANGE
- Min=0, Max=UINT_MAX
- Plane
- Scanout CRTC (destination) width (atomic)
-
-
- âCRTC_Hâ
- RANGE
- Min=0, Max=UINT_MAX
- Plane
- Scanout CRTC (destination) height (atomic)
-
-
- âFB_IDâ
- OBJECT
- DRM_MODE_OBJECT_FB
- Plane
- Scanout framebuffer (atomic)
-
-
- âCRTC_IDâ
- OBJECT
- DRM_MODE_OBJECT_CRTC
- Plane
- CRTC that plane is attached to (atomic)
-
-
- DVI-I
- âsubconnectorâ
- ENUM
- { âUnknownâ, âDVI-Dâ, âDVI-Aâ }
- Connector
- TBD
-
-
- âselect subconnectorâ
- ENUM
- { âAutomaticâ, âDVI-Dâ, âDVI-Aâ }
- Connector
- TBD
-
-
- TV
- âsubconnectorâ
- ENUM
- { "Unknown", "Composite", "SVIDEO", "Component",
"SCART" }
- Connector
- TBD
-
-
- âselect subconnectorâ
- ENUM
- { "Automatic", "Composite", "SVIDEO", "Component",
"SCART" }
- Connector
- TBD
-
-
- âmodeâ
- ENUM
- { "NTSC_M", "NTSC_J", "NTSC_443
