Re: [PATCH libdrm] intel/intel_chipset.h: Sync Cannonlake IDs.
This is matching both the kernel and the spec.
Reviewed-by: Rafael Antognolli <rafael.antogno...@intel.com>.
On Wed, Feb 14, 2018 at 05:42:24PM -0800, Rodrigo Vivi wrote:
> Let's sync CNL ids with Spec and kernel.
>
> Sync with kernel commit '3f43031b1693 ("drm/i915/cnl:
> Add Cannonlake PCI IDs for another SKU.")' and
> commit 'e3890d05b342 ("drm/i915/cnl: Sync PCI ID with Spec.")'
>
> Cc: James Ausmus <james.aus...@intel.com>
> Cc: Lucas De Marchi <lucas.demar...@intel.com>
> Cc: Paulo Zanoni <paulo.r.zan...@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.v...@intel.com>
> ---
> intel/intel_chipset.h | 52
> +++
> 1 file changed, 28 insertions(+), 24 deletions(-)
>
> diff --git a/intel/intel_chipset.h b/intel/intel_chipset.h
> index 3818e71e..01d250e8 100644
> --- a/intel/intel_chipset.h
> +++ b/intel/intel_chipset.h
> @@ -241,16 +241,20 @@
> #define PCI_CHIP_COFFEELAKE_U_GT3_4 0x3EA7
> #define PCI_CHIP_COFFEELAKE_U_GT3_5 0x3EA8
>
> -#define PCI_CHIP_CANNONLAKE_U_GT2_0 0x5A52
> -#define PCI_CHIP_CANNONLAKE_U_GT2_1 0x5A5A
> -#define PCI_CHIP_CANNONLAKE_U_GT2_2 0x5A42
> -#define PCI_CHIP_CANNONLAKE_U_GT2_3 0x5A4A
> -#define PCI_CHIP_CANNONLAKE_Y_GT2_0 0x5A51
> -#define PCI_CHIP_CANNONLAKE_Y_GT2_1 0x5A59
> -#define PCI_CHIP_CANNONLAKE_Y_GT2_2 0x5A41
> -#define PCI_CHIP_CANNONLAKE_Y_GT2_3 0x5A49
> -#define PCI_CHIP_CANNONLAKE_Y_GT2_4 0x5A71
> -#define PCI_CHIP_CANNONLAKE_Y_GT2_5 0x5A79
> +#define PCI_CHIP_CANNONLAKE_00x5A51
> +#define PCI_CHIP_CANNONLAKE_10x5A59
> +#define PCI_CHIP_CANNONLAKE_20x5A41
> +#define PCI_CHIP_CANNONLAKE_30x5A49
> +#define PCI_CHIP_CANNONLAKE_40x5A52
> +#define PCI_CHIP_CANNONLAKE_50x5A5A
> +#define PCI_CHIP_CANNONLAKE_60x5A42
> +#define PCI_CHIP_CANNONLAKE_70x5A4A
> +#define PCI_CHIP_CANNONLAKE_80x5A50
> +#define PCI_CHIP_CANNONLAKE_90x5A40
> +#define PCI_CHIP_CANNONLAKE_10 0x5A54
> +#define PCI_CHIP_CANNONLAKE_11 0x5A5C
> +#define PCI_CHIP_CANNONLAKE_12 0x5A44
> +#define PCI_CHIP_CANNONLAKE_13 0x5A4C
>
> #define IS_MOBILE(devid) ((devid) == PCI_CHIP_I855_GM || \
>(devid) == PCI_CHIP_I915_GM || \
> @@ -515,20 +519,20 @@
>IS_GEMINILAKE(devid) || \
>IS_COFFEELAKE(devid))
>
> -#define IS_CNL_Y(devid) ((devid) == PCI_CHIP_CANNONLAKE_Y_GT2_0
> || \
> - (devid) == PCI_CHIP_CANNONLAKE_Y_GT2_1 || \
> - (devid) == PCI_CHIP_CANNONLAKE_Y_GT2_2 || \
> - (devid) == PCI_CHIP_CANNONLAKE_Y_GT2_3 || \
> - (devid) == PCI_CHIP_CANNONLAKE_Y_GT2_4 || \
> - (devid) == PCI_CHIP_CANNONLAKE_Y_GT2_5)
> -
> -#define IS_CNL_U(devid) ((devid) == PCI_CHIP_CANNONLAKE_U_GT2_0
> || \
> - (devid) == PCI_CHIP_CANNONLAKE_U_GT2_1 || \
> - (devid) == PCI_CHIP_CANNONLAKE_U_GT2_2 || \
> - (devid) == PCI_CHIP_CANNONLAKE_U_GT2_3)
> -
> -#define IS_CANNONLAKE(devid) (IS_CNL_U(devid) || \
> - IS_CNL_Y(devid))
> +#define IS_CANNONLAKE(devid) ((devid) == PCI_CHIP_CANNONLAKE_0 || \
> + (devid) == PCI_CHIP_CANNONLAKE_1 || \
> + (devid) == PCI_CHIP_CANNONLAKE_2 || \
> + (devid) == PCI_CHIP_CANNONLAKE_3 || \
> + (devid) == PCI_CHIP_CANNONLAKE_4 || \
> + (devid) == PCI_CHIP_CANNONLAKE_5 || \
> + (devid) == PCI_CHIP_CANNONLAKE_6 || \
> + (devid) == PCI_CHIP_CANNONLAKE_7 || \
> + (devid) == PCI_CHIP_CANNONLAKE_8 || \
> + (devid) == PCI_CHIP_CANNONLAKE_9 || \
> + (devid) == PCI_CHIP_CANNONLAKE_10 || \
> + (devid) == PCI_CHIP_CANNONLAKE_11 || \
> + (devid) == PCI_CHIP_CANNONLAKE_12 || \
> + (devid) == PCI_CHIP_CANNONLAKE_13)
>
> #define IS_GEN10(devid) (IS_CANNONLAKE(devid))
>
> --
> 2.13.6
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/7] drm/doc: Polish for drm_plane.[hc]
On Wed, Sep 28, 2016 at 11:11:52AM +0300, Jani Nikula wrote:
> On Wed, 28 Sep 2016, Rafael Antognolli wrote:
> > Hi Daniel,
> >
> > On Wed, Sep 21, 2016 at 10:59:25AM +0200, Daniel Vetter wrote:
> >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> >> index 1407715736a5..256219bfd07b 100644
> >> --- a/include/drm/drm_plane.h
> >> +++ b/include/drm/drm_plane.h
> >> @@ -319,10 +319,48 @@ struct drm_plane_funcs {
> >>void (*early_unregister)(struct drm_plane *plane);
> >> };
> >>
> >> +/**
> >> + * enum drm_plane_type - uapi plane type enumeration
> >> + *
> >> + * For historical reasons not all planes are made the same. This
> >> enumeration is
> >> + * used to tell the different types of planes apart to implement the
> >> different
> >> + * uapi semantics for them. For userspace which is universal plane aware
> >> and
> >> + * which is using that atomic IOCTL there's no difference between these
> >> planes
> >> + * (beyong what the driver and hardware can support of course).
> >> + *
> >> + * For compatibility with legacy userspace, only overlay planes are made
> >> + * available to userspace by default. Userspace clients may set the
> >> + * DRM_CLIENT_CAP_UNIVERSAL_PLANES client capability bit to indicate that
> >> they
> >> + * wish to receive a universal plane list containing all plane types. See
> >> also
> >> + * drm_for_each_legacy_plane().
> >> + */
> >> enum drm_plane_type {
> >> - DRM_PLANE_TYPE_OVERLAY,
> >> + /**
> >> + * @DRM_PLANE_TYPE_PRIMARY:
> >> + *
> >> + * Primary planes represent a "main" plane for a CRTC. Primary planes
> >> + * are the planes operated upon by CRTC modesetting and flipping
> >> + * operations described in the page_flip and set_config hooks in struct
> >> + * _crtc_funcs.
> >> + */
> >>DRM_PLANE_TYPE_PRIMARY,
> >> +
> >> + /**
> >> + * @DRM_PLANE_TYPE_CURSOR:
> >> + *
> >> + * Cursor planes represent 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_CURSOR,
> >> +
> >> + /**
> >> + * @DRM_PLANE_TYPE_OVERLAY:
> >> + *
> >> + * Overlay planes represent all non-primary, non-cursor planes. Some
> >> + * drivers refer to these types of planes as "sprites" internally.
> >> + */
> >> + DRM_PLANE_TYPE_OVERLAY,
> >> };
> >
> > This is changing the order (and consequently the values) of these enums.
> > But it is not updated in libdrm. I noticed this is causing an issue when
> > playing with robclark's version of kmscube (branch atomic):
> >
> > https://github.com/robclark/kmscube/tree/atomic
> >
> > It looks like IGT also uses this macro from libdrm:
> >
> > $ git grep -n DRM_PLANE_TYPE_PRIMARY
> > lib/igt_kms.c:1398: case DRM_PLANE_TYPE_PRIMARY:
> > tests/kms_frontbuffer_tracking.c:2302: drm.plane_types[i] ==
> > DRM_PLANE_TYPE_PRIMARY)
> > tests/kms_frontbuffer_tracking.c:2602: drm.plane_types[i] ==
> > DRM_PLANE_TYPE_PRIMARY) {
> > tests/kms_frontbuffer_tracking.c:2741: drm.plane_types[i] ==
> > DRM_PLANE_TYPE_PRIMARY)
> >
> > Anyway, should we update it on libdrm, bring the order back to its
> > original values, or something else? Or am I missing something?
>
> You're absolutely right. But you're missing the fix has already landed
> in drm-misc tree:
>
> commit 226714dc7c6af6d0acee449eb2afce08d128edad
> Author: Daniel Vetter
> Date: Fri Sep 23 08:35:25 2016 +0200
>
> drm: Fix plane type uabi breakage
>
> BR,
> Jani.
Oh, I was sure I had checked drm-intel-nightly too, but I clearly
didn't. Sorry for the noise.
Regards,
Rafael
[PATCH 2/7] drm/doc: Polish for drm_plane.[hc]
Hi Daniel,
On Wed, Sep 21, 2016 at 10:59:25AM +0200, Daniel Vetter wrote:
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 1407715736a5..256219bfd07b 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -319,10 +319,48 @@ struct drm_plane_funcs {
> void (*early_unregister)(struct drm_plane *plane);
> };
>
> +/**
> + * enum drm_plane_type - uapi plane type enumeration
> + *
> + * For historical reasons not all planes are made the same. This enumeration
> is
> + * used to tell the different types of planes apart to implement the
> different
> + * uapi semantics for them. For userspace which is universal plane aware and
> + * which is using that atomic IOCTL there's no difference between these
> planes
> + * (beyong what the driver and hardware can support of course).
> + *
> + * For compatibility with legacy userspace, only overlay planes are made
> + * available to userspace by default. Userspace clients may set the
> + * DRM_CLIENT_CAP_UNIVERSAL_PLANES client capability bit to indicate that
> they
> + * wish to receive a universal plane list containing all plane types. See
> also
> + * drm_for_each_legacy_plane().
> + */
> enum drm_plane_type {
> - DRM_PLANE_TYPE_OVERLAY,
> + /**
> + * @DRM_PLANE_TYPE_PRIMARY:
> + *
> + * Primary planes represent a "main" plane for a CRTC. Primary planes
> + * are the planes operated upon by CRTC modesetting and flipping
> + * operations described in the page_flip and set_config hooks in struct
> + * _crtc_funcs.
> + */
> DRM_PLANE_TYPE_PRIMARY,
> +
> + /**
> + * @DRM_PLANE_TYPE_CURSOR:
> + *
> + * Cursor planes represent 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_CURSOR,
> +
> + /**
> + * @DRM_PLANE_TYPE_OVERLAY:
> + *
> + * Overlay planes represent all non-primary, non-cursor planes. Some
> + * drivers refer to these types of planes as "sprites" internally.
> + */
> + DRM_PLANE_TYPE_OVERLAY,
> };
This is changing the order (and consequently the values) of these enums.
But it is not updated in libdrm. I noticed this is causing an issue when
playing with robclark's version of kmscube (branch atomic):
https://github.com/robclark/kmscube/tree/atomic
It looks like IGT also uses this macro from libdrm:
$ git grep -n DRM_PLANE_TYPE_PRIMARY
lib/igt_kms.c:1398: case DRM_PLANE_TYPE_PRIMARY:
tests/kms_frontbuffer_tracking.c:2302: drm.plane_types[i] ==
DRM_PLANE_TYPE_PRIMARY)
tests/kms_frontbuffer_tracking.c:2602: drm.plane_types[i] ==
DRM_PLANE_TYPE_PRIMARY) {
tests/kms_frontbuffer_tracking.c:2741: drm.plane_types[i] ==
DRM_PLANE_TYPE_PRIMARY)
Anyway, should we update it on libdrm, bring the order back to its
original values, or something else? Or am I missing something?
Thanks,
Rafael
[PATCH v3] dma-buf/sync_file: Increment refcount of fence when all are signaled.
When we merge several fences, if all of them are signaled already, we
still keep one of them. So instead of using add_fence(), which will not
increase the refcount of signaled fences, we should explicitly call
fence_get() for the fence we are keeping.
This patch fixes a kernel panic that can be triggered by creating a fence that
is expired (or increasing the timeline until it expires), then creating a
merged fence out of it, and deleting the merged fence. This will make the
original expired fence's refcount go to zero.
Testcase: igt/sw_sync/sync_expired_merge
Signed-off-by: Rafael Antognolli
Reviewed-by: Chris Wilson
Reviewed-by: Gustavo Padovan
---
drivers/dma-buf/sync_file.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index abb5fda..0fe7ec2 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -253,10 +253,8 @@ static struct sync_file *sync_file_merge(const char *name,
struct sync_file *a,
for (; i_b < b_num_fences; i_b++)
add_fence(fences, , b_fences[i_b]);
- if (i == 0) {
- add_fence(fences, , a_fences[0]);
- i++;
- }
+ if (i == 0)
+ fences[i++] = fence_get(a_fences[0]);
if (num_fences > i) {
nfences = krealloc(fences, i * sizeof(*fences),
--
2.7.4
[PATCH v2] dma-buf/sync_file: Increment refcount of fence when all are signaled.
When we merge several fences, if all of them are signaled already, we
still keep one of them. So instead of using add_fence(), which will not
increase the refcount of signaled fences, we should explicitly call
fence_get() for the fence we are keeping.
This patch fixes a kernel panic that can be triggered by creating a fence that
is expired (or increasing the timeline until it expires), then creating a
merged fence out of it, and deleting the merged fence. This will make the
original expired fence's refcount go to zero.
Testcase: igt/sw_sync/sync_expired_merge
Signed-off-by: Rafael Antognolli
---
drivers/dma-buf/sync_file.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index abb5fda..0fe7ec2 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -253,10 +253,8 @@ static struct sync_file *sync_file_merge(const char *name,
struct sync_file *a,
for (; i_b < b_num_fences; i_b++)
add_fence(fences, , b_fences[i_b]);
- if (i == 0) {
- add_fence(fences, , a_fences[0]);
- i++;
- }
+ if (i == 0)
+ fences[i++] = fence_get(a_fences[0]);
if (num_fences > i) {
nfences = krealloc(fences, i * sizeof(*fences),
--
2.7.4
[PATCH] dma-buf/sync-file: Avoid enable fence signaling if poll(.timeout=0)
Hi Chris and Gustavo,
On Mon, Aug 29, 2016 at 07:16:13PM +0100, Chris Wilson wrote:
> If we being polled with a timeout of zero, a nonblocking busy query,
> we don't need to install any fence callbacks as we will not be waiting.
> As we only install the callback once, the overhead comes from the atomic
> bit test that also causes serialisation between threads.
>
> Signed-off-by: Chris Wilson
> Cc: Sumit Semwal
> Cc: Gustavo Padovan
> Cc: linux-media at vger.kernel.org
> Cc: dri-devel at lists.freedesktop.org
> Cc: linaro-mm-sig at lists.linaro.org
> ---
> drivers/dma-buf/sync_file.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 486d29c1a830..abb5fdab75fd 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -306,7 +306,8 @@ static unsigned int sync_file_poll(struct file *file,
> poll_table *wait)
>
> poll_wait(file, _file->wq, wait);
>
> - if (!test_and_set_bit(POLL_ENABLED, _file->fence->flags)) {
> + if (!poll_does_not_wait(wait) &&
> + !test_and_set_bit(POLL_ENABLED, _file->fence->flags)) {
> if (fence_add_callback(sync_file->fence, _file->cb,
> fence_check_cb_func) < 0)
> wake_up_all(_file->wq);
This commit is causing an error on one of the tests that Robert Foss
submitted for i-g-t. The one that does random merge of fences from
different timelines. A simple version of the test that still triggers
this is:
static void test_sync_simple_merge(void)
{
int fence1, fence2, fence_merge, timeline1, timeline2;
int ret;
timeline1 = sw_sync_timeline_create();
timeline2 = sw_sync_timeline_create();
fence1 = sw_sync_fence_create(timeline1, 1);
fence2 = sw_sync_fence_create(timeline2, 2);
fence_merge = sw_sync_merge(fence1, fence2);
sw_sync_timeline_inc(timeline1, 5);
sw_sync_timeline_inc(timeline2, 5);
ret = sw_sync_wait(fence_merge, 0);
igt_assert_f(ret > 0, "Failure triggering fence\n");
sw_sync_fence_destroy(fence_merge);
sw_sync_fence_destroy(fence1);
sw_sync_fence_destroy(fence2);
sw_sync_timeline_destroy(timeline1);
sw_sync_timeline_destroy(timeline2);
}
It looks like you cannot trust fence_is_signaled() without a
fence_add_callback(). I think the fence_array->num_pending won't get
updated. Although I couldn't figure out why it only happens if you merge
fences from different timelines.
Regards,
Rafael
[PATCH] dma-buf/sync_file: Increment refcount of fence when all are signaled.
When we merge several fences, if all of them are signaled already, we
still keep one of them. So instead of using add_fence(), which will not
increase the refcount of signaled fences, we should explicitly call
fence_get() for the fence we are keeping.
This patch fixes a kernel panic that can be triggered by creating a fence that
is expired (or increasing the timeline until it expires), then creating a
merged fence out of it, and deleting the merged fence. This will make the
original expired fence's refcount go to zero.
Signed-off-by: Rafael Antognolli
---
drivers/dma-buf/sync_file.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index abb5fda..0fe7ec2 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -253,10 +253,8 @@ static struct sync_file *sync_file_merge(const char *name,
struct sync_file *a,
for (; i_b < b_num_fences; i_b++)
add_fence(fences, , b_fences[i_b]);
- if (i == 0) {
- add_fence(fences, , a_fences[0]);
- i++;
- }
+ if (i == 0)
+ fences[i++] = fence_get(a_fences[0]);
if (num_fences > i) {
nfences = krealloc(fences, i * sizeof(*fences),
--
2.7.4
[PATCH] dma-buf/sync_file: Always increment refcount when merging fences.
On Wed, Sep 14, 2016 at 11:04:01AM -0300, Gustavo Padovan wrote:
> 2016-09-14 Chris Wilson :
>
> > On Tue, Sep 13, 2016 at 04:24:27PM -0700, Rafael Antognolli wrote:
> > > The refcount of a fence should be increased whenever it is added to a
> > > merged
> > > fence, since it will later be decreased when the merged fence is
> > > destroyed.
> > > Failing to do so will cause the original fence to be freed if the merged
> > > fence
> > > gets freed, but other places still referencing won't know about it.
> > >
> > > This patch fixes a kernel panic that can be triggered by creating a fence
> > > that
> > > is expired (or increasing the timeline until it expires), then creating a
> > > merged fence out of it, and deleting the merged fence. This will make the
> > > original expired fence's refcount go to zero.
> > >
> > > Signed-off-by: Rafael Antognolli
> > > ---
> > >
> > > Sample code to trigger the mentioned kernel panic (might need to be
> > > executed a
> > > couple times before it actually breaks everything):
> > >
> > > static void test_sync_expired_merge(void)
> > > {
> > >int iterations = 1 << 20;
> > >int timeline;
> > >int i;
> > >int fence_expired, fence_merged;
> > >
> > >timeline = sw_sync_timeline_create();
> > >
> > >sw_sync_timeline_inc(timeline, 100);
> > >fence_expired = sw_sync_fence_create(timeline, 1);
> > >fence_merged = sw_sync_merge(fence_expired, fence_expired);
> > >sw_sync_fence_destroy(fence_merged);
> > >
> > >for (i = 0; i < iterations; i++) {
> > >int fence = sw_sync_merge(fence_expired, fence_expired);
> > >
> > >igt_assert_f(sw_sync_wait(fence, -1) > 0,
> > > "Failure waiting on fence\n");
> > >sw_sync_fence_destroy(fence);
> > >}
> > >
> > >sw_sync_fence_destroy(fence_expired);
> > > }
> > >
> > > drivers/dma-buf/sync_file.c | 7 ++-
> > > 1 file changed, 2 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> > > index 486d29c..6ce6b8f 100644
> > > --- a/drivers/dma-buf/sync_file.c
> > > +++ b/drivers/dma-buf/sync_file.c
> > > @@ -178,11 +178,8 @@ static struct fence **get_fences(struct sync_file
> > > *sync_file, int *num_fences)
> > > static void add_fence(struct fence **fences, int *i, struct fence *fence)
> > > {
> > > fences[*i] = fence;
> > > -
> > > - if (!fence_is_signaled(fence)) {
> > > - fence_get(fence);
> > > - (*i)++;
> > > - }
> > > + fence_get(fence);
> > > + (*i)++;
> > > }
> >
> > I think you'll find it's the caller:
> >
> > if (i == 0) {
> > add_fence(fences, , a_fences[0]);
> > i++;
> > }
> >
> > that does the unexpected.
> >
> > This should be
> >
> > if (i == 0)
> > fences[i++] = fence_get(a_fences[0]);
>
> You are right. Otherwise we would miss the extra reference. i == 0 here
> means that all fences are signalled.
>
> >
> >
> > That ensures the sync_file inherits the signaled status without having
> > to keep all fences.
OK, so if you are building a merged fence out of several signaled
fences, then you only keep one of them instead? I haven't noticed that
was the intention.
> > I think there still seems to be a memory leak when calling
> > sync_file_set_fence() here with i == 1.
>
> I think that is something we discussed already, we don't hold an extra
> ref there for i == 1 because it would leak the fence.
>
I agree, we already increase the reference count in add_fence anyway (or
here, assuming we do what Chris suggested. But I also believe that's the
cause of confusion.
IMHO it would be better to call fence_get() inside fence_array_create(),
so it would match the fence_put in fence_array_release(). And if we have
only one fence, fence_get it inside sync_file_set_fence().
Rafael
[PATCH i-g-t] tests/sw_sync: Add subtest test_sync_expired_merge
This test creates an already expired fence, then creates a merged fence
out of that expired one (passed twice to the merge operation), and
finally closes the merged fence. It shows that if the refcounts are
wrong on the original expired fence, it might get freed while still in
use. Usually a kernel panick will follow.
Signed-off-by: Rafael Antognolli
---
tests/sw_sync.c | 28
1 file changed, 28 insertions(+)
diff --git a/tests/sw_sync.c b/tests/sw_sync.c
index 4336659..31cde50 100644
--- a/tests/sw_sync.c
+++ b/tests/sw_sync.c
@@ -582,6 +582,31 @@ static void test_sync_multi_producer_single_consumer(void)
pthread_join(threads[i], NULL);
}
+static void test_sync_expired_merge(void)
+{
+ int iterations = 1 << 20;
+ int timeline;
+ int i;
+ int fence_expired, fence_merged;
+
+ timeline = sw_sync_timeline_create();
+
+ sw_sync_timeline_inc(timeline, 100);
+ fence_expired = sw_sync_fence_create(timeline, 1);
+ fence_merged = sw_sync_merge(fence_expired, fence_expired);
+ sw_sync_fence_destroy(fence_merged);
+
+ for (i = 0; i < iterations; i++) {
+ int fence = sw_sync_merge(fence_expired, fence_expired);
+
+ igt_assert_f(sw_sync_wait(fence, -1) > 0,
+"Failure waiting on fence\n");
+ sw_sync_fence_destroy(fence);
+ }
+
+ sw_sync_fence_destroy(fence_expired);
+}
+
static void test_sync_random_merge(void)
{
int i, size, ret;
@@ -687,6 +712,9 @@ igt_main
igt_subtest("sync_multi_producer_single_consumer")
test_sync_multi_producer_single_consumer();
+ igt_subtest("sync_expired_merge")
+ test_sync_expired_merge();
+
igt_subtest("sync_random_merge")
test_sync_random_merge();
}
--
2.7.4
[PATCH] dma-buf/sync_file: Always increment refcount when merging fences.
The refcount of a fence should be increased whenever it is added to a merged
fence, since it will later be decreased when the merged fence is destroyed.
Failing to do so will cause the original fence to be freed if the merged fence
gets freed, but other places still referencing won't know about it.
This patch fixes a kernel panic that can be triggered by creating a fence that
is expired (or increasing the timeline until it expires), then creating a
merged fence out of it, and deleting the merged fence. This will make the
original expired fence's refcount go to zero.
Signed-off-by: Rafael Antognolli
---
Sample code to trigger the mentioned kernel panic (might need to be executed a
couple times before it actually breaks everything):
static void test_sync_expired_merge(void)
{
int iterations = 1 << 20;
int timeline;
int i;
int fence_expired, fence_merged;
timeline = sw_sync_timeline_create();
sw_sync_timeline_inc(timeline, 100);
fence_expired = sw_sync_fence_create(timeline, 1);
fence_merged = sw_sync_merge(fence_expired, fence_expired);
sw_sync_fence_destroy(fence_merged);
for (i = 0; i < iterations; i++) {
int fence = sw_sync_merge(fence_expired, fence_expired);
igt_assert_f(sw_sync_wait(fence, -1) > 0,
"Failure waiting on fence\n");
sw_sync_fence_destroy(fence);
}
sw_sync_fence_destroy(fence_expired);
}
drivers/dma-buf/sync_file.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 486d29c..6ce6b8f 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -178,11 +178,8 @@ static struct fence **get_fences(struct sync_file
*sync_file, int *num_fences)
static void add_fence(struct fence **fences, int *i, struct fence *fence)
{
fences[*i] = fence;
-
- if (!fence_is_signaled(fence)) {
- fence_get(fence);
- (*i)++;
- }
+ fence_get(fence);
+ (*i)++;
}
/**
--
2.7.4
[PATCH v9 3/3] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.
Hi Lukas,
Sorry for the late answer, I went on vacation and then was busy with
something else. Anyway, I tried to address all comments (yours and
Daniel's) on a new version. See some comments below.
On Sun, Dec 06, 2015 at 05:08:49PM +0100, Lukas Wunner wrote:
> Hi Rafael,
>
> On Thu, Dec 03, 2015 at 02:54:02PM -0800, Rafael Antognolli wrote:
> > So far, the i915 driver and some other drivers set it to the drm_device,
> > which doesn't allow one to know which DP a given aux channel is related
> > to. Changing this to be the drm_connector provides proper nesting, still
> > allowing one to get the drm_device from it. Some drivers already set it
> > to the drm_connector.
> >
> > This also removes the need to add a sysfs link for the i2c device under
> > the connector, as it will already be there.
> >
> > v2:
>
> Two minor nits, I think this should be "v9" instead of "v2" because
> this was newly added since v8, and the subject should be prefixed
> "drm/i915" (or "drm/i915/dp" or whatever) instead of "drm/dp" because
> this only touches i915 and not drm core.
>
>
> > - As a side effect, drm_dp_aux_unregister() must be called before
> > intel_connector_unregister(), as both the aux.dev and the i2c adapter
> > dev are children of the drm_connector device now. Calling
> > drm_dp_aux_unregister() before prevents them from being destroyed
> > twice.
>
> I haven't verified what Ville wrote (that there are WARNs because of
> this ordering issue), but if this is true then we need to make sure
> all other drivers get the order right, otherwise they'd spew WARNs
> as well once this gets merged.
>
> I've checked that for every driver and the only one affected is radeon.
> A fix is now in my GitHub repo, feel free to include the commit in your
> series if you want to. I haven't been able to actually test it though
> as I don't have a radeon card:
> https://github.com/l1k/linux/commit/485e197a7cac88e24348150526988149428feeaa
>
Nice, I added it to the series, though I also don't have a radeon to
test it.
> >
> > Signed-off-by: Rafael Antognolli
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 22 --
> > 1 file changed, 4 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index f2bfca0..515893c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1162,14 +1162,12 @@ static void intel_aux_reg_init(struct intel_dp
> > *intel_dp)
> > static void
> > intel_dp_aux_fini(struct intel_dp *intel_dp)
> > {
> > - drm_dp_aux_unregister(_dp->aux);
> > kfree(intel_dp->aux.name);
> > }
>
> Hm, instead of moving the single call of drm_dp_aux_unregister()
> to intel_dp_connector_unregister() I think it would be more clear
> to call intel_dp_aux_fini() from intel_dp_connector_unregister()
> and remove its invocation from intel_dp_encoder_destroy().
>
> Ville introduced intel_dp_aux_fini() very recently with a121f4e5fae5,
> he should probably have a say on how to solve this.
I makes sense to me, so I did what you suggest.
Thanks for the review,
Rafael
> >
> > static int
> > intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector
> > *connector)
> > {
> > - struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > enum port port = intel_dig_port->port;
> > int ret;
> > @@ -1180,7 +1178,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct
> > intel_connector *connector)
> > if (!intel_dp->aux.name)
> > return -ENOMEM;
> >
> > - intel_dp->aux.dev = dev->dev;
> > + intel_dp->aux.dev = connector->base.kdev;
> > intel_dp->aux.transfer = intel_dp_aux_transfer;
> >
> > DRM_DEBUG_KMS("registering %s bus for %s\n",
> > @@ -1195,27 +1193,15 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct
> > intel_connector *connector)
> > return ret;
> > }
> >
> > - ret = sysfs_create_link(>base.kdev->kobj,
> > - _dp->aux.ddc.dev.kobj,
> > - intel_dp->aux.ddc.dev.kobj.name);
> > - if (ret < 0) {
> > - DRM_ERROR("sysfs_create_link() for %s failed (%d)\n",
> > - intel_dp->aux.name, ret);
> > - intel_dp_aux_fini(intel_dp);
> > -
[PATCH v10 4/4] drm/radeon: Fix WARN_ON if DRM_DP_AUX_CHARDEV is enabled
From: Lukas Wunner <lu...@wunner.de>
Rafael Antognolli's new DRM_DP_AUX_CHARDEV feature causes a WARN_ON
if drm_dp_aux->dev == drm_connector->kdev and drm_dp_aux_unregister()
is called after drm_connector_unregister(). radeon is the only driver
affected by this besides i915. (amdgpu calls drm_dp_aux_unregister()
before drm_connector_unregister().)
Cc: Rafael Antognolli
Cc: Ville Syrjälä
Cc: Alex Deucher
Signed-off-by: Lukas Wunner
---
drivers/gpu/drm/radeon/radeon_display.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_display.c
b/drivers/gpu/drm/radeon/radeon_display.c
index b3bb923..a885dae 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -1684,6 +1684,9 @@ void radeon_modeset_fini(struct radeon_device *rdev)
radeon_fbdev_fini(rdev);
kfree(rdev->mode_info.bios_hardcoded_edid);
+ /* free i2c buses */
+ radeon_i2c_fini(rdev);
+
if (rdev->mode_info.mode_config_initialized) {
radeon_afmt_fini(rdev);
drm_kms_helper_poll_fini(rdev->ddev);
@@ -1691,8 +1694,6 @@ void radeon_modeset_fini(struct radeon_device *rdev)
drm_mode_config_cleanup(rdev->ddev);
rdev->mode_info.mode_config_initialized = false;
}
- /* free i2c buses */
- radeon_i2c_fini(rdev);
}
static bool is_hdtv_mode(const struct drm_display_mode *mode)
--
2.4.3
[PATCH v10 3/4] drm/i915: Set aux.dev to the drm_connector device, instead of drm_device.
So far, the i915 driver and some other drivers set it to the drm_device,
which doesn't allow one to know which DP a given aux channel is related
to. Changing this to be the drm_connector provides proper nesting, still
allowing one to get the drm_device from it. Some drivers already set it
to the drm_connector.
This also removes the need to add a sysfs link for the i2c device under
the connector, as it will already be there.
v9:
- As a side effect, drm_dp_aux_unregister() must be called before
intel_connector_unregister(), as both the aux.dev and the i2c adapter
dev are children of the drm_connector device now. Calling
drm_dp_aux_unregister() before prevents them from being destroyed
twice.
v10:
- move aux_fini() to connector_unregister(), instead of moving
drm_dp_aux_unregister() outside of connector_register().
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/i915/intel_dp.c | 18 ++
1 file changed, 2 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e2bea710..da704c6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1188,7 +1188,6 @@ intel_dp_aux_fini(struct intel_dp *intel_dp)
static int
intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
{
- struct drm_device *dev = intel_dp_to_dev(intel_dp);
struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
enum port port = intel_dig_port->port;
int ret;
@@ -1199,7 +1198,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct
intel_connector *connector)
if (!intel_dp->aux.name)
return -ENOMEM;
- intel_dp->aux.dev = dev->dev;
+ intel_dp->aux.dev = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
DRM_DEBUG_KMS("registering %s bus for %s\n",
@@ -1214,16 +1213,6 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct
intel_connector *connector)
return ret;
}
- ret = sysfs_create_link(>base.kdev->kobj,
- _dp->aux.ddc.dev.kobj,
- intel_dp->aux.ddc.dev.kobj.name);
- if (ret < 0) {
- DRM_ERROR("sysfs_create_link() for %s failed (%d)\n",
- intel_dp->aux.name, ret);
- intel_dp_aux_fini(intel_dp);
- return ret;
- }
-
return 0;
}
@@ -1232,9 +1221,7 @@ intel_dp_connector_unregister(struct intel_connector
*intel_connector)
{
struct intel_dp *intel_dp = intel_attached_dp(_connector->base);
- if (!intel_connector->mst_port)
- sysfs_remove_link(_connector->base.kdev->kobj,
- intel_dp->aux.ddc.dev.kobj.name);
+ intel_dp_aux_fini(intel_dp);
intel_connector_unregister(intel_connector);
}
@@ -4868,7 +4855,6 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
struct intel_dp *intel_dp = _dig_port->dp;
- intel_dp_aux_fini(intel_dp);
intel_dp_mst_encoder_cleanup(intel_dig_port);
if (is_edp(intel_dp)) {
cancel_delayed_work_sync(_dp->panel_vdd_work);
--
2.4.3
[PATCH v10 2/4] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.
It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
the connector device pointer was correctly set in the aux helper struct.
Two main operations are provided on the registers read and write. The
address of the register to be read or written is given using lseek. The
seek position is updated upon read or write.
v2:
- lseek is used to select the register to read/write
- read/write are used instead of ioctl
- no blocking_notifier is used, just a direct callback
v3:
- use drm_dp_aux_dev prefix for public functions
- chardev is named drm_dp_auxN
- read/write don't allocate a buffer anymore, and transfer up to 16 bytes a
time
- remove notifier list from the implementation
- option on menuconfig is now a boolean
- add inline stub functions to avoid breakage when this option is disabled
v4:
- fix build system changes - actually disable this module when not selected.
v5:
- Use kref to avoid device closing while still in use
- Don't use list, use an idr for storing aux_dev
- Remove "connector" attribute
- set aux.dev to the connector drm_connector device, instead of
drm_device
v6:
- Use atomic_t for usage count
- Use a mutex instead of spinlock for idr lock
- Destroy chardev immediately on unregister
- other minor suggestions from Ville
v7:
- style fixes
- error handling fixes
v8:
- more error handling fixes
v9:
- remove module_init and module_exit, and add drm_dp_aux_dev_init/exit
to drm_kms_helper_init/exit.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/Kconfig | 8 +
drivers/gpu/drm/Makefile| 1 +
drivers/gpu/drm/drm_dp_aux_dev.c| 368
drivers/gpu/drm/drm_dp_helper.c | 16 +-
drivers/gpu/drm/drm_kms_helper_common.c | 15 +-
include/drm/drm_dp_aux_dev.h| 62 ++
6 files changed, 468 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
create mode 100644 include/drm/drm_dp_aux_dev.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 59babd5..dff87ca 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,14 @@ config DRM_MIPI_DSI
bool
depends on DRM
+config DRM_DP_AUX_CHARDEV
+ bool "DRM DP AUX Interface"
+ depends on DRM
+ help
+ Choose this option to enable a /dev/drm_dp_auxN node that allows to
+ read and write values to arbitrary DPCD registers on the DP aux
+ channel.
+
config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index dfe513f..424fcb7 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -30,6 +30,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o
drm_probe_helper.o \
drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
+drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
new file mode 100644
index 000..f73b38b
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -0,0 +1,368 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *Rafael Antognolli
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct drm_dp_aux_dev {
+ unsigned index;
+ struct drm
[PATCH v10 1/4] drm/kms_helper: Add a common place to call init and exit functions.
The module_init and module_exit functions will start here, and call the
subsequent init's and exit's.
v10:
- Keep __init on drm_fb_helper init function.
- Move MODULE_* macros to the common file.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/Makefile| 4 ++-
drivers/gpu/drm/drm_crtc_helper.c | 3 ---
drivers/gpu/drm/drm_fb_helper.c | 9 +++
drivers/gpu/drm/drm_kms_helper_common.c | 47 +
include/drm/drm_fb_helper.h | 6 +
5 files changed, 60 insertions(+), 9 deletions(-)
create mode 100644 drivers/gpu/drm/drm_kms_helper_common.c
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index f858aa2..dfe513f 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -24,7 +24,9 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
drm-y += $(drm-m)
drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
- drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o
+ drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
+ drm_kms_helper_common.o
+
drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
diff --git a/drivers/gpu/drm/drm_crtc_helper.c
b/drivers/gpu/drm/drm_crtc_helper.c
index 5d4bc64..baac181 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -73,9 +73,6 @@
* _crtc_helper_funcs, struct _encoder_helper_funcs and struct
* _connector_helper_funcs.
*/
-MODULE_AUTHOR("David Airlie, Jesse Barnes");
-MODULE_DESCRIPTION("DRM KMS helper");
-MODULE_LICENSE("GPL and additional rights");
/**
* drm_helper_move_panel_connectors_to_head() - move panels to the front in the
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 1e103c4..c27b964 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2175,9 +2175,9 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
* but the module doesn't depend on any fb console symbols. At least
* attempt to load fbcon to avoid leaving the system without a usable console.
*/
-#if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT)
-static int __init drm_fb_helper_modinit(void)
+int __init drm_fb_helper_modinit(void)
{
+#if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT)
const char *name = "fbcon";
struct module *fbcon;
@@ -2187,8 +2187,7 @@ static int __init drm_fb_helper_modinit(void)
if (!fbcon)
request_module_nowait(name);
+#endif
return 0;
}
-
-module_init(drm_fb_helper_modinit);
-#endif
+EXPORT_SYMBOL(drm_fb_helper_modinit);
diff --git a/drivers/gpu/drm/drm_kms_helper_common.c
b/drivers/gpu/drm/drm_kms_helper_common.c
new file mode 100644
index 000..d361005
--- /dev/null
+++ b/drivers/gpu/drm/drm_kms_helper_common.c
@@ -0,0 +1,47 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *Rafael Antognolli
+ *
+ */
+
+#include
+#include
+
+MODULE_AUTHOR("David Airlie, Jesse Barnes");
+MODULE_DESCRIPTION("DRM KMS helper");
+MODULE_LICENSE("GPL and additional rights");
+
+static int __init drm_kms_helper_init(void)
+{
+ /* Call init functions from specific kms helpers here */
+ return drm_fb_helper_modinit();
+}
+
+static void __exit drm_kms_helper_exit(void)
+{
+ /* Call exit functions from specific kms helpers here */
+}
+
+module_init(drm_kms_helper_init);
+module_exit(drm_kms_helper_exit);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index d8a40df..062723b 100644
--
[PATCH v10 0/4] Add drm_dp_aux chardev support.
This series implement support to a drm_dp_aux chardev that allows reading and writing an arbitrary amount of bytes to arbitrary dpcd register addresses using regular read, write and lseek operations. Lukas Wunner (1): drm/radeon: Fix WARN_ON if DRM_DP_AUX_CHARDEV is enabled Rafael Antognolli (3): drm/kms_helper: Add a common place to call init and exit functions. drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. drm/i915: Set aux.dev to the drm_connector device, instead of drm_device. drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile| 5 +- drivers/gpu/drm/drm_crtc_helper.c | 3 - drivers/gpu/drm/drm_dp_aux_dev.c| 368 drivers/gpu/drm/drm_dp_helper.c | 16 +- drivers/gpu/drm/drm_fb_helper.c | 9 +- drivers/gpu/drm/drm_kms_helper_common.c | 60 ++ drivers/gpu/drm/i915/intel_dp.c | 18 +- drivers/gpu/drm/radeon/radeon_display.c | 5 +- include/drm/drm_dp_aux_dev.h| 62 ++ include/drm/drm_fb_helper.h | 6 + 11 files changed, 532 insertions(+), 28 deletions(-) create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c create mode 100644 drivers/gpu/drm/drm_kms_helper_common.c create mode 100644 include/drm/drm_dp_aux_dev.h -- 2.4.3
[PATCH v9 3/3] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.
So far, the i915 driver and some other drivers set it to the drm_device,
which doesn't allow one to know which DP a given aux channel is related
to. Changing this to be the drm_connector provides proper nesting, still
allowing one to get the drm_device from it. Some drivers already set it
to the drm_connector.
This also removes the need to add a sysfs link for the i2c device under
the connector, as it will already be there.
v2:
- As a side effect, drm_dp_aux_unregister() must be called before
intel_connector_unregister(), as both the aux.dev and the i2c adapter
dev are children of the drm_connector device now. Calling
drm_dp_aux_unregister() before prevents them from being destroyed
twice.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/i915/intel_dp.c | 22 --
1 file changed, 4 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f2bfca0..515893c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1162,14 +1162,12 @@ static void intel_aux_reg_init(struct intel_dp
*intel_dp)
static void
intel_dp_aux_fini(struct intel_dp *intel_dp)
{
- drm_dp_aux_unregister(_dp->aux);
kfree(intel_dp->aux.name);
}
static int
intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector)
{
- struct drm_device *dev = intel_dp_to_dev(intel_dp);
struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
enum port port = intel_dig_port->port;
int ret;
@@ -1180,7 +1178,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct
intel_connector *connector)
if (!intel_dp->aux.name)
return -ENOMEM;
- intel_dp->aux.dev = dev->dev;
+ intel_dp->aux.dev = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
DRM_DEBUG_KMS("registering %s bus for %s\n",
@@ -1195,27 +1193,15 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct
intel_connector *connector)
return ret;
}
- ret = sysfs_create_link(>base.kdev->kobj,
- _dp->aux.ddc.dev.kobj,
- intel_dp->aux.ddc.dev.kobj.name);
- if (ret < 0) {
- DRM_ERROR("sysfs_create_link() for %s failed (%d)\n",
- intel_dp->aux.name, ret);
- intel_dp_aux_fini(intel_dp);
- return ret;
- }
-
return 0;
}
static void
intel_dp_connector_unregister(struct intel_connector *intel_connector)
{
- struct intel_dp *intel_dp = intel_attached_dp(_connector->base);
-
- if (!intel_connector->mst_port)
- sysfs_remove_link(_connector->base.kdev->kobj,
- intel_dp->aux.ddc.dev.kobj.name);
+ struct intel_dp *intel_dp =
+ enc_to_intel_dp(_connector->encoder->base);
+ drm_dp_aux_unregister(_dp->aux);
intel_connector_unregister(intel_connector);
}
--
2.4.3
[PATCH v9 1/3] drm/kms_helper: Add a common place to call init and exit functions.
The module_init and module_exit functions will start here, and call the
subsequent init's and exit's.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/Makefile| 4 ++-
drivers/gpu/drm/drm_fb_helper.c | 9 +++
drivers/gpu/drm/drm_kms_helper_common.c | 43 +
include/drm/drm_fb_helper.h | 6 +
4 files changed, 56 insertions(+), 6 deletions(-)
create mode 100644 drivers/gpu/drm/drm_kms_helper_common.c
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1e9ff4c..76399da 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -24,7 +24,9 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
drm-y += $(drm-m)
drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
- drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o
+ drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
+ drm_kms_helper_common.o
+
drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 69cbab5..576f769 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2175,9 +2175,9 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event);
* but the module doesn't depend on any fb console symbols. At least
* attempt to load fbcon to avoid leaving the system without a usable console.
*/
-#if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT)
-static int __init drm_fb_helper_modinit(void)
+int drm_fb_helper_modinit(void)
{
+#if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT)
const char *name = "fbcon";
struct module *fbcon;
@@ -2187,8 +2187,7 @@ static int __init drm_fb_helper_modinit(void)
if (!fbcon)
request_module_nowait(name);
+#endif
return 0;
}
-
-module_init(drm_fb_helper_modinit);
-#endif
+EXPORT_SYMBOL(drm_fb_helper_modinit);
diff --git a/drivers/gpu/drm/drm_kms_helper_common.c
b/drivers/gpu/drm/drm_kms_helper_common.c
new file mode 100644
index 000..920b2fb
--- /dev/null
+++ b/drivers/gpu/drm/drm_kms_helper_common.c
@@ -0,0 +1,43 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *Rafael Antognolli
+ *
+ */
+
+#include
+#include
+
+static int __init drm_kms_helper_init(void)
+{
+ /* Call init functions from specific kms helpers here */
+ return drm_fb_helper_modinit();
+}
+
+static void __exit drm_kms_helper_exit(void)
+{
+ /* Call exit functions from specific kms helpers here */
+}
+
+module_init(drm_kms_helper_init);
+module_exit(drm_kms_helper_exit);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 87b090c..4fed7a2 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -148,6 +148,7 @@ struct drm_fb_helper {
};
#ifdef CONFIG_DRM_FBDEV_EMULATION
+int drm_fb_helper_modinit(void);
void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper
*helper,
const struct drm_fb_helper_funcs *funcs);
int drm_fb_helper_init(struct drm_device *dev,
@@ -212,6 +213,11 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper
*fb_helper, struct drm_
int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
struct drm_connector *connector);
#else
+static inline int drm_fb_helper_modinit(void)
+{
+ return 0;
+}
+
static inline void drm_fb_helper_prepare(struct drm_device *dev,
struct drm_fb_helper *helper,
[PATCH v9 0/3] Add drm_dp_aux chardev support.
This series implement support to a drm_dp_aux chardev that allows reading and writing an arbitrary amount of bytes to arbitrary dpcd register addresses using regular read, write and lseek operations. Rafael Antognolli (3): drm/kms_helper: Add a common place to call init and exit functions. drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. drm/dp: Set aux.dev to the drm_connector device, instead of drm_device. drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile| 5 +- drivers/gpu/drm/drm_dp_aux_dev.c| 368 drivers/gpu/drm/drm_dp_helper.c | 16 +- drivers/gpu/drm/drm_fb_helper.c | 9 +- drivers/gpu/drm/drm_kms_helper_common.c | 56 + drivers/gpu/drm/i915/intel_dp.c | 22 +- include/drm/drm_dp_aux_dev.h| 62 ++ include/drm/drm_fb_helper.h | 6 + 9 files changed, 527 insertions(+), 25 deletions(-) create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c create mode 100644 drivers/gpu/drm/drm_kms_helper_common.c create mode 100644 include/drm/drm_dp_aux_dev.h -- 2.4.3
[Intel-gfx] [PATCH v8 2/2] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.
On Tue, Nov 24, 2015 at 10:31:41PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 02, 2015 at 12:33:48PM -0800, Rafael Antognolli wrote:
> > So far, the i915 driver and some other drivers set it to the drm_device,
> > which doesn't allow one to know which DP a given aux channel is related
> > to. Changing this to be the drm_connector provides proper nesting, still
> > allowing one to get the drm_device from it. Some drivers already set it
> > to the drm_connector.
> >
> > This also removes the need to add a sysfs link for the i2c device under
> > the connector, as it will already be there.
> >
> > Signed-off-by: Rafael Antognolli
>
> I gave aux_dev a bit of a testing here, and it appaers to work quite
> splendidly.
>
> This patch however causes lots of WARN spew if I unload the driver
> while in middle of dumping the DPCD via the aux_dev. It appears we
> we clean up things the wrong order now.
It looks like device_destroy is being called twice on the drm_aux_dev
and i2c adapter devices.
If I understood correctly, this is happening because I changed the
parent device, from the drm_device to the drm_connector, but the
drm_connector is being destroyed before the drm_aux_dev and i2c adapter
devices, which already causes them to be destroyed too.
Changing the drm_dp_aux_unregister() from inside intel_dp_aux_fini() to
intel_dp_connector_unregister(), right before
intel_connector_unregister(), seems to fix everything, and it still
makes sense in my opinion, since the auxdev is going to be unregistered
before we destroy the drm_connector. Do you think that would be ok?
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 19 ++-
> > 1 file changed, 2 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 8287df4..7aacc08 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1078,36 +1078,21 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct
> > intel_connector *connector)
> > intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
> >
> > intel_dp->aux.name = name;
> > - intel_dp->aux.dev = dev->dev;
> > + intel_dp->aux.dev = connector->base.kdev;
> > intel_dp->aux.transfer = intel_dp_aux_transfer;
> >
> > DRM_DEBUG_KMS("registering %s bus for %s\n", name,
> > connector->base.kdev->kobj.name);
> >
> > ret = drm_dp_aux_register(_dp->aux);
> > - if (ret < 0) {
> > + if (ret < 0)
> > DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n",
> > name, ret);
> > - return;
> > - }
> > -
> > - ret = sysfs_create_link(>base.kdev->kobj,
> > - _dp->aux.ddc.dev.kobj,
> > - intel_dp->aux.ddc.dev.kobj.name);
> > - if (ret < 0) {
> > - DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name,
> > ret);
> > - drm_dp_aux_unregister(_dp->aux);
> > - }
> > }
> >
> > static void
> > intel_dp_connector_unregister(struct intel_connector *intel_connector)
> > {
> > - struct intel_dp *intel_dp = intel_attached_dp(_connector->base);
> > -
> > - if (!intel_connector->mst_port)
> > - sysfs_remove_link(_connector->base.kdev->kobj,
> > - intel_dp->aux.ddc.dev.kobj.name);
> > intel_connector_unregister(intel_connector);
> > }
> >
> > --
> > 2.4.3
> >
> > ___
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
[PATCH v8 2/2] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.
So far, the i915 driver and some other drivers set it to the drm_device,
which doesn't allow one to know which DP a given aux channel is related
to. Changing this to be the drm_connector provides proper nesting, still
allowing one to get the drm_device from it. Some drivers already set it
to the drm_connector.
This also removes the need to add a sysfs link for the i2c device under
the connector, as it will already be there.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/i915/intel_dp.c | 19 ++-
1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8287df4..7aacc08 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1078,36 +1078,21 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct
intel_connector *connector)
intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
intel_dp->aux.name = name;
- intel_dp->aux.dev = dev->dev;
+ intel_dp->aux.dev = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
DRM_DEBUG_KMS("registering %s bus for %s\n", name,
connector->base.kdev->kobj.name);
ret = drm_dp_aux_register(_dp->aux);
- if (ret < 0) {
+ if (ret < 0)
DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n",
name, ret);
- return;
- }
-
- ret = sysfs_create_link(>base.kdev->kobj,
- _dp->aux.ddc.dev.kobj,
- intel_dp->aux.ddc.dev.kobj.name);
- if (ret < 0) {
- DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name,
ret);
- drm_dp_aux_unregister(_dp->aux);
- }
}
static void
intel_dp_connector_unregister(struct intel_connector *intel_connector)
{
- struct intel_dp *intel_dp = intel_attached_dp(_connector->base);
-
- if (!intel_connector->mst_port)
- sysfs_remove_link(_connector->base.kdev->kobj,
- intel_dp->aux.ddc.dev.kobj.name);
intel_connector_unregister(intel_connector);
}
--
2.4.3
[PATCH v8 1/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.
It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
the connector device pointer was correctly set in the aux helper struct.
Two main operations are provided on the registers read and write. The
address of the register to be read or written is given using lseek. The
seek position is updated upon read or write.
v2:
- lseek is used to select the register to read/write
- read/write are used instead of ioctl
- no blocking_notifier is used, just a direct callback
v3:
- use drm_dp_aux_dev prefix for public functions
- chardev is named drm_dp_auxN
- read/write don't allocate a buffer anymore, and transfer up to 16 bytes a
time
- remove notifier list from the implementation
- option on menuconfig is now a boolean
- add inline stub functions to avoid breakage when this option is disabled
v4:
- fix build system changes - actually disable this module when not selected.
v5:
- Use kref to avoid device closing while still in use
- Don't use list, use an idr for storing aux_dev
- Remove "connector" attribute
- set aux.dev to the connector drm_connector device, instead of
drm_device
v6:
- Use atomic_t for usage count
- Use a mutex instead of spinlock for idr lock
- Destroy chardev immediately on unregister
- other minor suggestions from Ville
v7:
- style fixes
- error handling fixes
v8:
- more error handling fixes
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/Kconfig | 8 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_dp_aux_dev.c | 373 +++
drivers/gpu/drm/drm_dp_helper.c | 16 +-
include/drm/drm_dp_aux_dev.h | 50 ++
5 files changed, 447 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
create mode 100644 include/drm/drm_dp_aux_dev.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index c4bf9a1..daefcce 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,14 @@ config DRM_MIPI_DSI
bool
depends on DRM
+config DRM_DP_AUX_CHARDEV
+ bool "DRM DP AUX Interface"
+ depends on DRM
+ help
+ Choose this option to enable a /dev/drm_dp_auxN node that allows to
+ read and write values to arbitrary DPCD registers on the DP aux
+ channel.
+
config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1e9ff4c..e48ec8f 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -28,6 +28,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o
drm_probe_helper.o \
drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
+drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
new file mode 100644
index 000..0269556
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -0,0 +1,373 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *Rafael Antognolli
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct drm_dp_aux_dev {
+ unsigned index;
+ struct drm_dp_aux *aux;
+ struct device *dev;
+ struct kref refcount;
+ atomic_t usecount;
+};
+
+#define DRM_AUX_MINORS 256
+#define AUX_MAX_OFFSET (1 << 20)
+st
[PATCH v8 0/2] Add drm_dp_aux chardev support.
This series implement support to a drm_dp_aux chardev that allows reading and writing an arbitrary amount of bytes to arbitrary dpcd register addresses using regular read, write and lseek operations. Rafael Antognolli (2): drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. drm/dp: Set aux.dev to the drm_connector device, instead of drm_device. drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_dp_aux_dev.c | 373 +++ drivers/gpu/drm/drm_dp_helper.c | 16 +- drivers/gpu/drm/i915/intel_dp.c | 19 +- include/drm/drm_dp_aux_dev.h | 50 ++ 6 files changed, 449 insertions(+), 18 deletions(-) create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c create mode 100644 include/drm/drm_dp_aux_dev.h -- 2.4.3
[PATCH v7 2/2] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.
So far, the i915 driver and some other drivers set it to the drm_device,
which doesn't allow one to know which DP a given aux channel is related
to. Changing this to be the drm_connector provides proper nesting, still
allowing one to get the drm_device from it. Some drivers already set it
to the drm_connector.
This also removes the need to add a sysfs link for the i2c device under
the connector, as it will already be there.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/i915/intel_dp.c | 19 ++-
1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8287df4..7aacc08 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1078,36 +1078,21 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct
intel_connector *connector)
intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
intel_dp->aux.name = name;
- intel_dp->aux.dev = dev->dev;
+ intel_dp->aux.dev = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
DRM_DEBUG_KMS("registering %s bus for %s\n", name,
connector->base.kdev->kobj.name);
ret = drm_dp_aux_register(_dp->aux);
- if (ret < 0) {
+ if (ret < 0)
DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n",
name, ret);
- return;
- }
-
- ret = sysfs_create_link(>base.kdev->kobj,
- _dp->aux.ddc.dev.kobj,
- intel_dp->aux.ddc.dev.kobj.name);
- if (ret < 0) {
- DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name,
ret);
- drm_dp_aux_unregister(_dp->aux);
- }
}
static void
intel_dp_connector_unregister(struct intel_connector *intel_connector)
{
- struct intel_dp *intel_dp = intel_attached_dp(_connector->base);
-
- if (!intel_connector->mst_port)
- sysfs_remove_link(_connector->base.kdev->kobj,
- intel_dp->aux.ddc.dev.kobj.name);
intel_connector_unregister(intel_connector);
}
--
2.4.3
[PATCH v7 1/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.
It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
the connector device pointer was correctly set in the aux helper struct.
Two main operations are provided on the registers read and write. The
address of the register to be read or written is given using lseek. The
seek position is updated upon read or write.
v2:
- lseek is used to select the register to read/write
- read/write are used instead of ioctl
- no blocking_notifier is used, just a direct callback
v3:
- use drm_dp_aux_dev prefix for public functions
- chardev is named drm_dp_auxN
- read/write don't allocate a buffer anymore, and transfer up to 16 bytes a
time
- remove notifier list from the implementation
- option on menuconfig is now a boolean
- add inline stub functions to avoid breakage when this option is disabled
v4:
- fix build system changes - actually disable this module when not selected.
v5:
- Use kref to avoid device closing while still in use
- Don't use list, use an idr for storing aux_dev
- Remove "connector" attribute
- set aux.dev to the connector drm_connector device, instead of
drm_device
v6:
- Use atomic_t for usage count
- Use a mutex instead of spinlock for idr lock
- Destroy chardev immediately on unregister
- other minor suggestions from Ville
v7:
- style fixes
- error handling fixes
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/Kconfig | 8 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_dp_aux_dev.c | 374 +++
drivers/gpu/drm/drm_dp_helper.c | 5 +
include/drm/drm_dp_aux_dev.h | 50 ++
5 files changed, 438 insertions(+)
create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
create mode 100644 include/drm/drm_dp_aux_dev.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index c4bf9a1..daefcce 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,14 @@ config DRM_MIPI_DSI
bool
depends on DRM
+config DRM_DP_AUX_CHARDEV
+ bool "DRM DP AUX Interface"
+ depends on DRM
+ help
+ Choose this option to enable a /dev/drm_dp_auxN node that allows to
+ read and write values to arbitrary DPCD registers on the DP aux
+ channel.
+
config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1e9ff4c..e48ec8f 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -28,6 +28,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o
drm_probe_helper.o \
drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
+drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
new file mode 100644
index 000..1a3ca39
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -0,0 +1,374 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *Rafael Antognolli
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct drm_dp_aux_dev {
+ unsigned index;
+ struct drm_dp_aux *aux;
+ struct device *dev;
+ struct kref refcount;
+ atomic_t usecount;
+};
+
+#define DRM_AUX_MINORS 256
+#define AUX_MAX_OFFSET (1 << 20)
+static DEFINE_IDR(aux_idr);
+static DEFINE_MUTEX(aux_idr_mute
[PATCH v7 0/2] Add drm_dp_aux chardev support.
This series implement support to a drm_dp_aux chardev that allows reading and writing an arbitrary amount of bytes to arbitrary dpcd register addresses using regular read, write and lseek operations. Rafael Antognolli (2): drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. drm/dp: Set aux.dev to the drm_connector device, instead of drm_device. drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_dp_aux_dev.c | 374 +++ drivers/gpu/drm/drm_dp_helper.c | 5 + drivers/gpu/drm/i915/intel_dp.c | 19 +- include/drm/drm_dp_aux_dev.h | 50 ++ 6 files changed, 440 insertions(+), 17 deletions(-) create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c create mode 100644 include/drm/drm_dp_aux_dev.h -- 2.4.3
[PATCH v6 1/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
On Fri, Oct 30, 2015 at 12:04:17PM +0200, Ville Syrjälä wrote: > On Thu, Oct 29, 2015 at 04:23:45PM -0700, Rafael Antognolli wrote: > > This module is heavily based on i2c-dev. Once loaded, it provides one > > dev node per DP AUX channel, named drm_dp_auxN, where N is an integer. > > > > It's possible to know which connector owns this aux channel by looking > > at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if > > the connector device pointer was correctly set in the aux helper struct. > > > > Two main operations are provided on the registers read and write. The > > address of the register to be read or written is given using lseek. The > > seek position is updated upon read or write. > > Note that we (i915 folks at least) generally like to have the changelog > in the commit message itself. So maybe move it here for the final > version. > > Anyways, this is looking rather nice now. I spotted a few remaining style > issues, and some error handling problems. Once those are dealt with > I think we should be done. > > > > > Signed-off-by: Rafael Antognolli > > --- > > drivers/gpu/drm/Kconfig | 8 + > > drivers/gpu/drm/Makefile | 1 + > > drivers/gpu/drm/drm_dp_aux_dev.c | 381 > > +++ > > drivers/gpu/drm/drm_dp_helper.c | 5 + > > include/drm/drm_dp_aux_dev.h | 50 + > > 5 files changed, 445 insertions(+) > > create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c > > create mode 100644 include/drm/drm_dp_aux_dev.h > > > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > > index c4bf9a1..daefcce 100644 > > --- a/drivers/gpu/drm/Kconfig > > +++ b/drivers/gpu/drm/Kconfig > > @@ -25,6 +25,14 @@ config DRM_MIPI_DSI > > bool > > depends on DRM > > > > +config DRM_DP_AUX_CHARDEV > > + bool "DRM DP AUX Interface" > > + depends on DRM > > + help > > + Choose this option to enable a /dev/drm_dp_auxN node that allows to > > + read and write values to arbitrary DPCD registers on the DP aux > > + channel. > > + > > config DRM_KMS_HELPER > > tristate > > depends on DRM > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > > index 1e9ff4c..e48ec8f 100644 > > --- a/drivers/gpu/drm/Makefile > > +++ b/drivers/gpu/drm/Makefile > > @@ -28,6 +28,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o > > drm_probe_helper.o \ > > drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o > > drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o > > drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o > > +drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o > > > > obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o > > > > diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c > > b/drivers/gpu/drm/drm_dp_aux_dev.c > > new file mode 100644 > > index 000..16dbc2e > > --- /dev/null > > +++ b/drivers/gpu/drm/drm_dp_aux_dev.c > > @@ -0,0 +1,381 @@ > > +/* > > + * Copyright © 2015 Intel Corporation > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a > > + * copy of this software and associated documentation files (the > > "Software"), > > + * to deal in the Software without restriction, including without > > limitation > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > > + * and/or sell copies of the Software, and to permit persons to whom the > > + * Software is furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice (including the > > next > > + * paragraph) shall be included in all copies or substantial portions of > > the > > + * Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS > > OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR > > OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > > DEALINGS > > + * IN THE SOFTWARE. > > + * > > + * Authors: > > + *Rafael Antognolli > > + * > &
[PATCH v6 2/2] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.
So far, the i915 driver and some other drivers set it to the drm_device,
which doesn't allow one to know which DP a given aux channel is related
to. Changing this to be the drm_connector provides proper nesting, still
allowing one to get the drm_device from it. Some drivers already set it
to the drm_connector.
This also removes the need to add a sysfs link for the i2c device under
the connector, as it will already be there.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/i915/intel_dp.c | 19 ++-
1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8287df4..7aacc08 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1078,36 +1078,21 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct
intel_connector *connector)
intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
intel_dp->aux.name = name;
- intel_dp->aux.dev = dev->dev;
+ intel_dp->aux.dev = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
DRM_DEBUG_KMS("registering %s bus for %s\n", name,
connector->base.kdev->kobj.name);
ret = drm_dp_aux_register(_dp->aux);
- if (ret < 0) {
+ if (ret < 0)
DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n",
name, ret);
- return;
- }
-
- ret = sysfs_create_link(>base.kdev->kobj,
- _dp->aux.ddc.dev.kobj,
- intel_dp->aux.ddc.dev.kobj.name);
- if (ret < 0) {
- DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name,
ret);
- drm_dp_aux_unregister(_dp->aux);
- }
}
static void
intel_dp_connector_unregister(struct intel_connector *intel_connector)
{
- struct intel_dp *intel_dp = intel_attached_dp(_connector->base);
-
- if (!intel_connector->mst_port)
- sysfs_remove_link(_connector->base.kdev->kobj,
- intel_dp->aux.ddc.dev.kobj.name);
intel_connector_unregister(intel_connector);
}
--
2.4.3
[PATCH v6 1/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.
It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
the connector device pointer was correctly set in the aux helper struct.
Two main operations are provided on the registers read and write. The
address of the register to be read or written is given using lseek. The
seek position is updated upon read or write.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/Kconfig | 8 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_dp_aux_dev.c | 381 +++
drivers/gpu/drm/drm_dp_helper.c | 5 +
include/drm/drm_dp_aux_dev.h | 50 +
5 files changed, 445 insertions(+)
create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
create mode 100644 include/drm/drm_dp_aux_dev.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index c4bf9a1..daefcce 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,14 @@ config DRM_MIPI_DSI
bool
depends on DRM
+config DRM_DP_AUX_CHARDEV
+ bool "DRM DP AUX Interface"
+ depends on DRM
+ help
+ Choose this option to enable a /dev/drm_dp_auxN node that allows to
+ read and write values to arbitrary DPCD registers on the DP aux
+ channel.
+
config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1e9ff4c..e48ec8f 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -28,6 +28,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o
drm_probe_helper.o \
drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
+drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
new file mode 100644
index 000..16dbc2e
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -0,0 +1,381 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *Rafael Antognolli
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct drm_dp_aux_dev {
+ unsigned index;
+ struct drm_dp_aux *aux;
+ struct device *dev;
+ struct kref refcount;
+ atomic_t usecount;
+};
+
+#define DRM_AUX_MINORS 256
+#define AUX_MAX_OFFSET (1 << 20)
+static DEFINE_IDR(aux_idr);
+static DEFINE_MUTEX(aux_idr_mutex);
+static struct class *drm_dp_aux_dev_class;
+static int drm_dev_major = -1;
+
+static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
+{
+ struct drm_dp_aux_dev *aux_dev = NULL;
+
+ mutex_lock(_idr_mutex);
+ aux_dev = idr_find(_idr, index);
+ if (!kref_get_unless_zero(_dev->refcount))
+ aux_dev = NULL;
+ mutex_unlock(_idr_mutex);
+
+ return aux_dev;
+}
+
+static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct drm_dp_aux *aux)
+{
+ struct drm_dp_aux_dev *aux_dev;
+ int index;
+
+
+ aux_dev = kzalloc(sizeof(*aux_dev), GFP_KERNEL);
+ if (!aux_dev)
+ return ERR_PTR(-ENOMEM);
+ aux_dev->aux = aux;
+ atomic_set(_dev->usecount, 1);
+ kref_init(_dev->refcount);
+
+ mutex_lock(_idr_mutex);
+ index = idr_alloc_cyclic(_idr, aux_dev, 0, DRM_AUX_MINORS,
+GFP_KERNEL);
+ mutex_unlock(_idr_mutex);
+ if (index < 0) {
+
[PATCH v6 0/2] Add drm_dp_aux chardev support.
This series implement support to a drm_dp_aux chardev that allows reading and writing an arbitrary amount of bytes to arbitrary dpcd register addresses using regular read, write and lseek operations. v2: - lseek is used to select the register to read/write - read/write are used instead of ioctl - no blocking_notifier is used, just a direct callback v3: - use drm_dp_aux_dev prefix for public functions - chardev is named drm_dp_auxN - read/write don't allocate a buffer anymore, and transfer up to 16 bytes a time - remove notifier list from the implementation - option on menuconfig is now a boolean - add inline stub functions to avoid breakage when this option is disabled v4: - fix build system changes - actually disable this module when not selected. v5: - Use kref to avoid device closing while still in use - Don't use list, use an idr for storing aux_dev - Remove "connector" attribute - set aux.dev to the connector drm_connector device, instead of drm_device v6: - Use atomic_t for usage count - Use a mutex instead of spinlock for idr lock - Destroy chardev immediately on unregister - other minor suggestions from Ville Rafael Antognolli (2): drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. drm/dp: Set aux.dev to the drm_connector device, instead of drm_device. drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_dp_aux_dev.c | 381 +++ drivers/gpu/drm/drm_dp_helper.c | 5 + drivers/gpu/drm/i915/intel_dp.c | 19 +- include/drm/drm_dp_aux_dev.h | 50 + 6 files changed, 447 insertions(+), 17 deletions(-) create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c create mode 100644 include/drm/drm_dp_aux_dev.h -- 2.4.3
[PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.
On Tue, Sep 29, 2015 at 06:25:44PM +0200, Daniel Vetter wrote:
> On Tue, Sep 29, 2015 at 05:27:33PM +0200, Lukas Wunner wrote:
> > Hi Daniel,
> >
> > On Tue, Sep 29, 2015 at 05:04:03PM +0200, Daniel Vetter wrote:
> > > On Tue, Sep 29, 2015 at 02:49:20PM +0200, Lukas Wunner wrote:
> > > > On Mon, Sep 28, 2015 at 04:45:35PM -0700, Rafael Antognolli wrote:
> > > > > This is useful to determine which connector owns this AUX channel.
> > > >
> > > > WTF? I posted a patch in August which does exactly that:
> > > > http://lists.freedesktop.org/archives/dri-devel/2015-August/088172.html
> > > >
> > > > Can also be pulled in from this git repo:
> > > > https://github.com/l1k/linux/commit/b78b38d53fc0fc4fa0f6acf699b0fcad56ec1fe6
> > > >
> > > > My patch has the advantage that it updates all the drivers which use
> > > > drm_dp_aux to fill that attribute. Yours only updates i915.
> > > >
> > > > Daniel Vetter criticized storing a drm_connector pointer in drm_dp_aux,
> > > > quote:
> > > >
> > > > "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."
> > > >
> > > > So now Intel itself does precisely what Daniel criticized? Confusing!
> > > >
> > > > Source:
> > > > http://lists.freedesktop.org/archives/dri-devel/2015-August/089108.html
> > >
> > > Critism is still valid, and thinking about this again a cleaner solution
> > > would be to just have a correct parent/child relationship in the device
> > > hirarchy. I.e. add a struct device *parent to the aux channel structure
> > > which should point to the right connector.
> >
> > We already have that:
> >
> > struct drm_dp_aux {
> > const char *name;
> > struct i2c_adapter ddc;
> > struct device *dev; <---
> > struct mutex hw_mutex;
> > ssize_t (*transfer)(struct drm_dp_aux *aux,
> > struct drm_dp_aux_msg *msg);
> > unsigned i2c_nack_count, i2c_defer_count;
> > };
> >
> > What Rafael is struggling with is that you cannot unambiguously
> > get from drm_dp_aux->dev to the drm_connector. (The drm_device
> > may have multiple drm_connectors with type
> > DRM_MODE_CONNECTOR_DisplayPort.)
>
> What I meant to say is that we don't need that, if instead of filling in
> the overall dev in dp_aux->dev we fill in the connector sysfs device
> thing. The we have proper nesting, like with i2c buses. And then there's
> no need for a connector property in sysfs to show that link (which should
> be done with a proper sysfs link anyway).
OK, I sent a new version, which does not add a new *connector pointer,
and uses the dev pointer on the struct to store the drm_connector
device, instead of the drm_device device. Is that what you meant? In
any case, as I mention on the patch, it is already how some drivers do,
while others store the drm_device.
This leaves the aux device, for instance in my case, at:
/sys/class/drm/card0/card0-eDP-1/drm_dp_aux0
If this is what you wanted, I can send other patches to the proper
mailing lists, trying to update other drivers.
--
Rafael
> >
> > >
> > > Thanks for pointing out that I missed properly delayering this.
> > > -Daniel
> > >
> > > >
> > > >
> > > > Best regards,
> > > >
> > > > Lukas
> > > >
> > > >
> > > > >
> > > > > Signed-off-by: Rafael Antognolli
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_dp.c | 1 +
> > > > > include/drm/drm_dp_helper.h | 1 +
> > > > > 2 files changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 77f7330..f90439d 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp,
> > > > > struct intel_connector *connector)
> > > > >
> > > > > i
[PATCH v5 2/2] drm/dp: Set aux.dev to the drm_connector device, instead of drm_device.
So far, the i915 driver and some other drivers set it to the drm_device,
which doesn't allow one to know which DP a given aux channel is related
to. Changing this to be the drm_connector provides proper nesting, still
allowing one to get the drm_device from it. Some drivers already set it
to the drm_connector.
This also removes the need to add a sysfs link for the i2c device under
the connector, as it will already be there.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/i915/intel_dp.c | 19 ++-
1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 18bcfbe..0acdf0f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1078,36 +1078,21 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct
intel_connector *connector)
intel_dp->aux_ch_ctl_reg = intel_dp->output_reg + 0x10;
intel_dp->aux.name = name;
- intel_dp->aux.dev = dev->dev;
+ intel_dp->aux.dev = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
DRM_DEBUG_KMS("registering %s bus for %s\n", name,
connector->base.kdev->kobj.name);
ret = drm_dp_aux_register(_dp->aux);
- if (ret < 0) {
+ if (ret < 0)
DRM_ERROR("drm_dp_aux_register() for %s failed (%d)\n",
name, ret);
- return;
- }
-
- ret = sysfs_create_link(>base.kdev->kobj,
- _dp->aux.ddc.dev.kobj,
- intel_dp->aux.ddc.dev.kobj.name);
- if (ret < 0) {
- DRM_ERROR("sysfs_create_link() for %s failed (%d)\n", name,
ret);
- drm_dp_aux_unregister(_dp->aux);
- }
}
static void
intel_dp_connector_unregister(struct intel_connector *intel_connector)
{
- struct intel_dp *intel_dp = intel_attached_dp(_connector->base);
-
- if (!intel_connector->mst_port)
- sysfs_remove_link(_connector->base.kdev->kobj,
- intel_dp->aux.ddc.dev.kobj.name);
intel_connector_unregister(intel_connector);
}
--
2.4.3
[PATCH v5 1/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.
It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
the connector device pointer was correctly set in the aux helper struct.
Two main operations are provided on the registers read and write. The
address of the register to be read or written is given using lseek. The
seek position is updated upon read or write.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/Kconfig | 8 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_dp_aux_dev.c | 373 +++
drivers/gpu/drm/drm_dp_helper.c | 7 +
include/drm/drm_dp_aux_dev.h | 50 ++
5 files changed, 439 insertions(+)
create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
create mode 100644 include/drm/drm_dp_aux_dev.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 1a0a8df..64fa0f4 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,14 @@ config DRM_MIPI_DSI
bool
depends on DRM
+config DRM_DP_AUX_CHARDEV
+ bool "DRM DP AUX Interface"
+ depends on DRM
+ help
+ Choose this option to enable a /dev/drm_dp_auxN node that allows to
+ read and write values to arbitrary DPCD registers on the DP aux
+ channel.
+
config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index e814517..e5bc0ca 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -28,6 +28,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o
drm_probe_helper.o \
drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
+drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
new file mode 100644
index 000..a2861e2
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -0,0 +1,373 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *Rafael Antognolli
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct drm_dp_aux_dev {
+ unsigned index;
+ struct drm_dp_aux *aux;
+ struct device *dev;
+ struct kref refcount;
+ bool removed;
+ spinlock_t removed_lock;
+};
+
+#define DRM_AUX_MINORS 256
+#define AUX_MAX_OFFSET (1 << 20)
+static DEFINE_IDR(aux_idr);
+static DEFINE_SPINLOCK(aux_idr_lock);
+static struct class *drm_dp_aux_dev_class;
+static int drm_dev_major = -1;
+
+static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
+{
+ struct drm_dp_aux_dev *aux_dev = NULL;
+
+ spin_lock(_idr_lock);
+ aux_dev = idr_find(_idr, index);
+ if (!kref_get_unless_zero(_dev->refcount))
+ aux_dev = NULL;
+ spin_unlock(_idr_lock);
+
+ return aux_dev;
+}
+
+static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct drm_dp_aux *aux)
+{
+ struct drm_dp_aux_dev *aux_dev;
+ int index;
+
+
+ aux_dev = kzalloc(sizeof(*aux_dev), GFP_KERNEL);
+ if (!aux_dev)
+ return ERR_PTR(-ENOMEM);
+ aux_dev->aux = aux;
+ aux_dev->removed = false;
+ spin_lock_init(_dev->removed_lock);
+ kref_init(_dev->refcount);
+
+ idr_preload(GFP_KERNEL);
+ spin_lock(_idr_lock);
+ index = idr_alloc_cyclic(_idr, aux_dev, 0, DRM_AUX_MINORS,
+
[PATCH v5 0/2] Add drm_dp_aux chardev support.
This series implement support to a drm_dp_aux chardev that allows reading and writing an arbitrary amount of bytes to arbitrary dpcd register addresses using regular read, write and lseek operations. v2: - lseek is used to select the register to read/write - read/write are used instead of ioctl - no blocking_notifier is used, just a direct callback v3: - use drm_dp_aux_dev prefix for public functions - chardev is named drm_dp_auxN - read/write don't allocate a buffer anymore, and transfer up to 16 bytes a time - remove notifier list from the implementation - option on menuconfig is now a boolean - add inline stub functions to avoid breakage when this option is disabled v4: - fix build system changes - actually disable this module when not selected. v5: - Use kref to avoid device closing while still in use - Don't use list, use an idr for storing aux_dev - Remove "connector" attribute - set aux.dev to the connector drm_connector device, instead of drm_device Rafael Antognolli (2): drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. drm/dp: Set aux.dev to the drm_connector device, instead of drm_device. drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_dp_aux_dev.c | 373 +++ drivers/gpu/drm/drm_dp_helper.c | 7 + drivers/gpu/drm/i915/intel_dp.c | 14 +- include/drm/drm_dp_aux_dev.h | 50 ++ 6 files changed, 441 insertions(+), 12 deletions(-) create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c create mode 100644 include/drm/drm_dp_aux_dev.h -- 2.4.3
[PATCH v4 2/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
Thanks Ville for the review, I'm addressing all the issues but have some
questions on the ones mentioned below:
On Tue, Sep 29, 2015 at 05:09:23PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 28, 2015 at 04:45:36PM -0700, Rafael Antognolli wrote:
> > This module is heavily based on i2c-dev. Once loaded, it provides one
> > dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.
> >
> > It's possible to know which connector owns this aux channel by looking
> > at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
> > the connector device pointer was correctly set in the aux helper struct.
> >
> > Two main operations are provided on the registers read and write. The
> > address of the register to be read or written is given using lseek. The
> > seek position is updated upon read or write.
> >
> > Signed-off-by: Rafael Antognolli
> > ---
> > drivers/gpu/drm/Kconfig | 8 +
> > drivers/gpu/drm/Makefile | 1 +
> > drivers/gpu/drm/drm_dp_aux_dev.c | 357
> > +++
> > drivers/gpu/drm/drm_dp_helper.c | 5 +
> > include/drm/drm_dp_aux_dev.h | 50 ++
> > 5 files changed, 421 insertions(+)
> > create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
> > create mode 100644 include/drm/drm_dp_aux_dev.h
> >
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 1a0a8df..64fa0f4 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -25,6 +25,14 @@ config DRM_MIPI_DSI
> > bool
> > depends on DRM
> >
> > +config DRM_DP_AUX_CHARDEV
> > + bool "DRM DP AUX Interface"
> > + depends on DRM
> > + help
> > + Choose this option to enable a /dev/drm_dp_auxN node that allows to
> > + read and write values to arbitrary DPCD registers on the DP aux
> > + channel.
> > +
> > config DRM_KMS_HELPER
> > tristate
> > depends on DRM
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 45e7719..e5ae7f0 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -25,6 +25,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o
> > drm_probe_helper.o \
> > drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> > drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> > drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> > +drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> >
> > obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> >
> > diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c
> > b/drivers/gpu/drm/drm_dp_aux_dev.c
> > new file mode 100644
> > index 000..e337081
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> > @@ -0,0 +1,357 @@
> > +/*
> > + * Copyright © 2015 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the
> > next
> > + * paragraph) shall be included in all copies or substantial portions of
> > the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + * Authors:
> > + *Rafael Antognolli
> > + *
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +struct drm_dp_aux_dev {
> > +
[PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.
On Tue, Sep 29, 2015 at 02:49:20PM +0200, Lukas Wunner wrote:
> Hi Rafael,
>
> On Mon, Sep 28, 2015 at 04:45:35PM -0700, Rafael Antognolli wrote:
> > This is useful to determine which connector owns this AUX channel.
>
> WTF? I posted a patch in August which does exactly that:
> http://lists.freedesktop.org/archives/dri-devel/2015-August/088172.html
>
> Can also be pulled in from this git repo:
> https://github.com/l1k/linux/commit/b78b38d53fc0fc4fa0f6acf699b0fcad56ec1fe6
>
> My patch has the advantage that it updates all the drivers which use
> drm_dp_aux to fill that attribute. Yours only updates i915.
>
> Daniel Vetter criticized storing a drm_connector pointer in drm_dp_aux,
> quote:
>
> "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."
>
> So now Intel itself does precisely what Daniel criticized? Confusing!
I'm sorry, I don't follow this list as closely yet, so I didn't see that
patch there, otherwise I would have talked to Daniel about it in the
first place.
Thanks,
Rafael
> Source:
> http://lists.freedesktop.org/archives/dri-devel/2015-August/089108.html
>
>
> Best regards,
>
> Lukas
>
>
> >
> > Signed-off-by: Rafael Antognolli
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 1 +
> > include/drm/drm_dp_helper.h | 1 +
> > 2 files changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 77f7330..f90439d 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct
> > intel_connector *connector)
> >
> > intel_dp->aux.name = name;
> > intel_dp->aux.dev = dev->dev;
> > + intel_dp->aux.connector = connector->base.kdev;
> > intel_dp->aux.transfer = intel_dp_aux_transfer;
> >
> > DRM_DEBUG_KMS("registering %s bus for %s\n", name,
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index 9ec4716..e009b5d 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -702,6 +702,7 @@ struct drm_dp_aux {
> > const char *name;
> > struct i2c_adapter ddc;
> > struct device *dev;
> > + struct device *connector;
> > struct mutex hw_mutex;
> > ssize_t (*transfer)(struct drm_dp_aux *aux,
> > struct drm_dp_aux_msg *msg);
> > --
> > 2.4.3
> >
> > ___
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH i-g-t] tests: Adding kms_dp_aux_dev test.
This new test makes some basic testing on the proposed drm_dp_aux_dev
interface. If the feature is enabled and the drm_dp_aux_dev class is
present, it will check for available DP aux channels and test them for:
- basic seek to 0 and read 1 byte
- seek to the last address and read, to confirm 0 is returned
- seek one more byte and confirm that EINVAL is returned
- try to read 64 bytes when at 8 bytes from the end of the
address space
- try to read 64 bytes at the address 0.
So far, no write checks are done.
Signed-off-by: Rafael Antognolli
---
tests/.gitignore | 1 +
tests/Makefile.sources | 1 +
tests/kms_dp_aux_dev.c | 134 +
3 files changed, 136 insertions(+)
create mode 100644 tests/kms_dp_aux_dev.c
diff --git a/tests/.gitignore b/tests/.gitignore
index dc8bb53..efdad75 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -127,6 +127,7 @@ kms_3d
kms_addfb_basic
kms_crtc_background_color
kms_cursor_crc
+kms_dp_aux_dev
kms_draw_crc
kms_fbc_crc
kms_fbcon_fbt
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 2e2e088..dc07737 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -64,6 +64,7 @@ TESTS_progs_M = \
gem_write_read_ring_switch \
kms_addfb_basic \
kms_cursor_crc \
+ kms_dp_aux_dev \
kms_draw_crc \
kms_fbc_crc \
kms_fbcon_fbt \
diff --git a/tests/kms_dp_aux_dev.c b/tests/kms_dp_aux_dev.c
new file mode 100644
index 000..aab8c95
--- /dev/null
+++ b/tests/kms_dp_aux_dev.c
@@ -0,0 +1,134 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+/** @file kms_dp_aux_dev.c
+ *
+ * This is a test of functionality of drm_dp_aux_dev.
+ */
+
+#include "igt.h"
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+#define DRM_DP_AUX_SYSFS_PATH "/sys/class/drm_dp_aux_dev"
+
+IGT_TEST_DESCRIPTION("Test of drm_aux_dev nodes.");
+
+static void drm_dp_aux_test(const char *devname)
+{
+ char basedir[] = "/dev/";
+ char name[256 + sizeof(basedir)];
+ off_t address;
+ uint8_t buf;
+ uint8_t bigbuf[64];
+ int fd;
+
+ ssize_t res;
+
+ snprintf(name, sizeof(name), "%s%s", basedir, devname);
+ igt_info("Running test on %s\n", name);
+
+ /* Check if this DP aux channel is connected before continuing tests */
+ fd = open(name, O_RDONLY);
+ igt_assert_lte(0, fd);
+ address = lseek(fd, 0x0, SEEK_SET);
+ igt_assert_eq(address, 0x0);
+ res = read(fd, , sizeof(buf));
+ igt_skip_on(res < 0 && errno == ETIMEDOUT);
+
+
+ /* Channel is connected, start tests */
+ igt_assert_eq(res, sizeof(buf));
+
+ /* Test reads on the end of address space */
+ address = lseek(fd, 0, SEEK_END);
+ igt_assert_eq(address, 1 << 20);
+ res = read(fd, , sizeof(buf));
+ igt_assert_eq(res, 0);
+
+ address = lseek(fd, 1, SEEK_CUR);
+ igt_assert_eq(address, -1);
+ igt_assert_eq(errno, EINVAL);
+
+ address = lseek(fd, -8, SEEK_END);
+ res = read(fd, bigbuf, sizeof(bigbuf));
+ igt_assert_eq(res, 8);
+
+ /* Test reading more than 16 bytes at once */
+ address = lseek(fd, 0, SEEK_SET);
+ res = read(fd, bigbuf, sizeof(bigbuf));
+ igt_assert_eq(res, sizeof(bigbuf));
+}
+
+static void for_each_dp_aux(DIR *dir)
+{
+ int count = 0;
+ struct dirent *dirent;
+
+ if (!dir)
+ return;
+
+ rewinddir(dir);
+ while ((dirent = readdir(dir))) {
+ if (dirent->d_name[0] == '.')
+ continue;
+
+ count++;
+ igt_subtest(dirent->d_name)
+
[PATCH v4 2/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.
It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
the connector device pointer was correctly set in the aux helper struct.
Two main operations are provided on the registers read and write. The
address of the register to be read or written is given using lseek. The
seek position is updated upon read or write.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/Kconfig | 8 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_dp_aux_dev.c | 357 +++
drivers/gpu/drm/drm_dp_helper.c | 5 +
include/drm/drm_dp_aux_dev.h | 50 ++
5 files changed, 421 insertions(+)
create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
create mode 100644 include/drm/drm_dp_aux_dev.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 1a0a8df..64fa0f4 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,14 @@ config DRM_MIPI_DSI
bool
depends on DRM
+config DRM_DP_AUX_CHARDEV
+ bool "DRM DP AUX Interface"
+ depends on DRM
+ help
+ Choose this option to enable a /dev/drm_dp_auxN node that allows to
+ read and write values to arbitrary DPCD registers on the DP aux
+ channel.
+
config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 45e7719..e5ae7f0 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -25,6 +25,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o
drm_probe_helper.o \
drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
+drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
new file mode 100644
index 000..e337081
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -0,0 +1,357 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *Rafael Antognolli
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct drm_dp_aux_dev {
+ struct list_head list;
+ unsigned index;
+ struct drm_dp_aux *aux;
+ struct device *dev;
+};
+
+#define DRM_AUX_MINORS 256
+static int aux_max_offset = 1 << 20;
+static int drm_dp_aux_dev_count = 0;
+static LIST_HEAD(drm_dp_aux_dev_list);
+static DEFINE_SPINLOCK(drm_dp_aux_dev_list_lock);
+
+static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
+{
+ struct drm_dp_aux_dev *aux_dev;
+
+ spin_lock(_dp_aux_dev_list_lock);
+ list_for_each_entry(aux_dev, _dp_aux_dev_list, list) {
+ if (aux_dev->index == index)
+ goto found;
+ }
+
+ aux_dev = NULL;
+found:
+ spin_unlock(_dp_aux_dev_list_lock);
+ return aux_dev;
+}
+
+static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_aux(struct drm_dp_aux *aux)
+{
+ struct drm_dp_aux_dev *aux_dev;
+
+ spin_lock(_dp_aux_dev_list_lock);
+ list_for_each_entry(aux_dev, _dp_aux_dev_list, list) {
+ if (aux_dev->aux == aux)
+ goto found;
+ }
+
+ aux_dev = NULL;
+found:
+ spin_unlock(_dp_aux_dev_list_lock);
+ return aux_dev;
+}
+
+static struct drm_dp_aux_dev *get_free_drm_dp_aux_dev(struct drm_dp_aux *aux)
+{
+ struct drm_dp_aux_dev *aux_d
[PATCH v4 1/2] drm/dp: Store the drm_connector device pointer on the helper.
This is useful to determine which connector owns this AUX channel.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/i915/intel_dp.c | 1 +
include/drm/drm_dp_helper.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 77f7330..f90439d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct
intel_connector *connector)
intel_dp->aux.name = name;
intel_dp->aux.dev = dev->dev;
+ intel_dp->aux.connector = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
DRM_DEBUG_KMS("registering %s bus for %s\n", name,
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 9ec4716..e009b5d 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -702,6 +702,7 @@ struct drm_dp_aux {
const char *name;
struct i2c_adapter ddc;
struct device *dev;
+ struct device *connector;
struct mutex hw_mutex;
ssize_t (*transfer)(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg);
--
2.4.3
[PATCH v4 0/2] Add drm_dp_aux chardev support.
This series implement support to a drm_dp_aux chardev that allows reading and writing an arbitrary amount of bytes to arbitrary dpcd register addresses using regular read, write and lseek operations. v2: - lseek is used to select the register to read/write - read/write are used instead of ioctl - no blocking_notifier is used, just a direct callback v3: - use drm_dp_aux_dev prefix for public functions - chardev is named drm_dp_auxN - read/write don't allocate a buffer anymore, and transfer up to 16 bytes a time - remove notifier list from the implementation - option on menuconfig is now a boolean - add inline stub functions to avoid breakage when this option is disabled v4: - fix build system changes - actually disable this module when not selected. Rafael Antognolli (2): drm/dp: Store the drm_connector device pointer on the helper. drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_dp_aux_dev.c | 357 +++ drivers/gpu/drm/drm_dp_helper.c | 5 + drivers/gpu/drm/i915/intel_dp.c | 1 + include/drm/drm_dp_aux_dev.h | 50 ++ include/drm/drm_dp_helper.h | 1 + 7 files changed, 423 insertions(+) create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c create mode 100644 include/drm/drm_dp_aux_dev.h -- 2.4.3
[PATCH RFC v2 0/3] Add a drm_aux-dev module.
Oops, sorry, this email shouldn't be in the patch set, please disconsider it. -- Rafael On Fri, Sep 25, 2015 at 04:54:53PM -0700, Rafael Antognolli wrote: > Second attempt at implementing a module that allows reading/writing arbitrary > dpcd registers. Changes to this version: > - lseek is used to select the register to read/write; > - read/write are used instead of ioctl; > - no blocking_notifier is used, just a direct callback. > > One thing to notice is that I am not updating the file offset during read or > write, which is kind of breaking the filesystem abstraction. But i2c-dev > doesn't do it either, so I assumed it's fine. > > Rafael Antognolli (3): > drm/dp: Keep a list of drm_dp_aux helper. > drm/dp: Store the drm_connector device pointer on the helper. > drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. > > drivers/gpu/drm/Kconfig | 4 + > drivers/gpu/drm/Makefile| 1 + > drivers/gpu/drm/drm_aux-dev.c | 326 > > drivers/gpu/drm/drm_dp_helper.c | 73 + > drivers/gpu/drm/i915/intel_dp.c | 1 + > include/drm/drm_dp_helper.h | 6 + > 6 files changed, 411 insertions(+) > create mode 100644 drivers/gpu/drm/drm_aux-dev.c > > -- > 2.4.3 >
[PATCH v3 2/2] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_dp_auxN, where N is an integer.
It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if
the connector device pointer was correctly set in the aux helper struct.
Two main operations are provided on the registers read and write. The
address of the register to be read or written is given using lseek. The
seek position is updated upon read or write.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/Kconfig | 8 +
drivers/gpu/drm/Makefile | 3 +-
drivers/gpu/drm/drm_dp_aux_dev.c | 357 +++
drivers/gpu/drm/drm_dp_helper.c | 5 +
include/drm/drm_dp_aux_dev.h | 50 ++
5 files changed, 422 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c
create mode 100644 include/drm/drm_dp_aux_dev.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 1a0a8df..64fa0f4 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,14 @@ config DRM_MIPI_DSI
bool
depends on DRM
+config DRM_DP_AUX_CHARDEV
+ bool "DRM DP AUX Interface"
+ depends on DRM
+ help
+ Choose this option to enable a /dev/drm_dp_auxN node that allows to
+ read and write values to arbitrary DPCD registers on the DP aux
+ channel.
+
config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 45e7719..1afcbed 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -21,7 +21,8 @@ drm-$(CONFIG_DRM_PANEL) += drm_panel.o
drm-$(CONFIG_OF) += drm_of.o
drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
- drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o
+ drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
+ drm_dp_aux_dev.o
drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
new file mode 100644
index 000..e337081
--- /dev/null
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -0,0 +1,357 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *Rafael Antognolli
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct drm_dp_aux_dev {
+ struct list_head list;
+ unsigned index;
+ struct drm_dp_aux *aux;
+ struct device *dev;
+};
+
+#define DRM_AUX_MINORS 256
+static int aux_max_offset = 1 << 20;
+static int drm_dp_aux_dev_count = 0;
+static LIST_HEAD(drm_dp_aux_dev_list);
+static DEFINE_SPINLOCK(drm_dp_aux_dev_list_lock);
+
+static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
+{
+ struct drm_dp_aux_dev *aux_dev;
+
+ spin_lock(_dp_aux_dev_list_lock);
+ list_for_each_entry(aux_dev, _dp_aux_dev_list, list) {
+ if (aux_dev->index == index)
+ goto found;
+ }
+
+ aux_dev = NULL;
+found:
+ spin_unlock(_dp_aux_dev_list_lock);
+ return aux_dev;
+}
+
+static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_aux(struct drm_dp_aux *aux)
+{
+ struct drm_dp_aux_dev *aux_dev;
+
+ spin_lock(_dp_aux_dev_list_lock);
+ list_for_each_entry(aux_dev, _dp_aux_dev_list, list) {
+ if (aux_dev->aux == aux)
+ goto found;
+ }
+
+ aux_dev = NULL;
+found:
+ spin_unlock(_dp_aux_dev_list_lock);
+
[PATCH v3 1/2] drm/dp: Store the drm_connector device pointer on the helper.
This is useful to determine which connector owns this AUX channel.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/i915/intel_dp.c | 1 +
include/drm/drm_dp_helper.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 06a2b10..1b1df1c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct
intel_connector *connector)
intel_dp->aux.name = name;
intel_dp->aux.dev = dev->dev;
+ intel_dp->aux.connector = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
DRM_DEBUG_KMS("registering %s bus for %s\n", name,
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 9ec4716..e009b5d 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -702,6 +702,7 @@ struct drm_dp_aux {
const char *name;
struct i2c_adapter ddc;
struct device *dev;
+ struct device *connector;
struct mutex hw_mutex;
ssize_t (*transfer)(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg);
--
2.4.3
[PATCH v3 0/2] Add drm_dp_aux chardev support.
This series implement support to a drm_dp_aux chardev that allows reading and writing an arbitrary amount of bytes to arbitrary dpcd register addresses using regular read, write and lseek operations. v2: - lseek is used to select the register to read/write - read/write are used instead of ioctl - no blocking_notifier is used, just a direct callback v3: - use drm_dp_aux_dev prefix for public functions - chardev is named drm_dp_auxN - read/write don't allocate a buffer anymore, and transfer up to 16 bytes a time - remove notifier list from the implementation - option on menuconfig is now a boolean - add inline stub functions to avoid breakage when this option is disabled Rafael Antognolli (2): drm/dp: Store the drm_connector device pointer on the helper. drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. drivers/gpu/drm/Kconfig | 8 + drivers/gpu/drm/Makefile | 3 +- drivers/gpu/drm/drm_dp_aux_dev.c | 357 +++ drivers/gpu/drm/drm_dp_helper.c | 5 + drivers/gpu/drm/i915/intel_dp.c | 1 + include/drm/drm_dp_aux_dev.h | 50 ++ include/drm/drm_dp_helper.h | 1 + 7 files changed, 424 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c create mode 100644 include/drm/drm_dp_aux_dev.h -- 2.4.3
[PATCH RFC v2 0/3] Add a drm_aux-dev module.
Second attempt at implementing a module that allows reading/writing arbitrary dpcd registers. Changes to this version: - lseek is used to select the register to read/write; - read/write are used instead of ioctl; - no blocking_notifier is used, just a direct callback. One thing to notice is that I am not updating the file offset during read or write, which is kind of breaking the filesystem abstraction. But i2c-dev doesn't do it either, so I assumed it's fine. Rafael Antognolli (3): drm/dp: Keep a list of drm_dp_aux helper. drm/dp: Store the drm_connector device pointer on the helper. drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. drivers/gpu/drm/Kconfig | 4 + drivers/gpu/drm/Makefile| 1 + drivers/gpu/drm/drm_aux-dev.c | 326 drivers/gpu/drm/drm_dp_helper.c | 73 + drivers/gpu/drm/i915/intel_dp.c | 1 + include/drm/drm_dp_helper.h | 6 + 6 files changed, 411 insertions(+) create mode 100644 drivers/gpu/drm/drm_aux-dev.c -- 2.4.3
[PATCH RFC v2 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
On Tue, Sep 22, 2015 at 02:17:51PM +0200, Daniel Vetter wrote:
> On Tue, Sep 22, 2015 at 03:00:54PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 15, 2015 at 04:55:04PM -0700, Rafael Antognolli wrote:
> > > This module is heavily based on i2c-dev. Once loaded, it provides one
> > > dev node per DP AUX channel, named drm_aux-N.
> > >
> > > It's possible to know which connector owns this aux channel by looking
> > > at the respective sysfs /sys/class/drm_aux-dev/drm_aux-N/connector, if
> > > the connector device pointer was correctly set in the aux helper struct.
> > >
> > > Two main operations are provided on the registers: read and write. The
> > > address of the register to be read or written is given using lseek.
> > > Reading or writing does not update the offset of the file.
> > >
> > > Signed-off-by: Rafael Antognolli
> > > ---
> > > drivers/gpu/drm/Kconfig | 4 +
> > > drivers/gpu/drm/Makefile | 1 +
> > > drivers/gpu/drm/drm_aux-dev.c | 326
> > > ++
> > > 3 files changed, 331 insertions(+)
> > > create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> > >
> > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > index 1a0a8df..eae847c 100644
> > > --- a/drivers/gpu/drm/Kconfig
> > > +++ b/drivers/gpu/drm/Kconfig
> > > @@ -25,6 +25,10 @@ config DRM_MIPI_DSI
> > > bool
> > > depends on DRM
> > >
> > > +config DRM_AUX_CHARDEV
> > > + tristate "DRM DP AUX Interface"
> > > + depends on DRM
> > > +
> > > config DRM_KMS_HELPER
> > > tristate
> > > depends on DRM
> > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > index 45e7719..a1a94306 100644
> > > --- a/drivers/gpu/drm/Makefile
> > > +++ b/drivers/gpu/drm/Makefile
> > > @@ -32,6 +32,7 @@ CFLAGS_drm_trace_points.o := -I$(src)
> > >
> > > obj-$(CONFIG_DRM)+= drm.o
> > > obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
> > > +obj-$(CONFIG_DRM_AUX_CHARDEV) += drm_aux-dev.o
> > > obj-$(CONFIG_DRM_TTM)+= ttm/
> > > obj-$(CONFIG_DRM_TDFX) += tdfx/
> > > obj-$(CONFIG_DRM_R128) += r128/
> > > diff --git a/drivers/gpu/drm/drm_aux-dev.c b/drivers/gpu/drm/drm_aux-dev.c
> > > new file mode 100644
> > > index 000..fcc334a
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/drm_aux-dev.c
> > > @@ -0,0 +1,326 @@
> > > +/*
> > > + * Copyright © 2015 Intel Corporation
> > > + *
> > > + * Permission is hereby granted, free of charge, to any person obtaining
> > > a
> > > + * copy of this software and associated documentation files (the
> > > "Software"),
> > > + * to deal in the Software without restriction, including without
> > > limitation
> > > + * the rights to use, copy, modify, merge, publish, distribute,
> > > sublicense,
> > > + * and/or sell copies of the Software, and to permit persons to whom the
> > > + * Software is furnished to do so, subject to the following conditions:
> > > + *
> > > + * The above copyright notice and this permission notice (including the
> > > next
> > > + * paragraph) shall be included in all copies or substantial portions of
> > > the
> > > + * Software.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > > EXPRESS OR
> > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > > MERCHANTABILITY,
> > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> > > SHALL
> > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > > OTHER
> > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > > ARISING
> > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > > DEALINGS
> > > + * IN THE SOFTWARE.
> > > + *
> > > + * Authors:
> > > + *Rafael Antognolli
> > > + *
> > > + */
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +struct drm_aux_dev {
> > > + struct li
[PATCH RFC v2 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
On Tue, Sep 22, 2015 at 10:59:51AM +0200, Daniel Vetter wrote:
> On Tue, Sep 15, 2015 at 04:55:04PM -0700, Rafael Antognolli wrote:
> > This module is heavily based on i2c-dev. Once loaded, it provides one
> > dev node per DP AUX channel, named drm_aux-N.
> >
> > It's possible to know which connector owns this aux channel by looking
> > at the respective sysfs /sys/class/drm_aux-dev/drm_aux-N/connector, if
> > the connector device pointer was correctly set in the aux helper struct.
> >
> > Two main operations are provided on the registers: read and write. The
> > address of the register to be read or written is given using lseek.
> > Reading or writing does not update the offset of the file.
>
> I think not updating the read position is very surprising. Would it be
> hard to fix that?
No, not hard at all. But I assume then I should update the write
position too, right?
BTW, i2c-dev doesn't update either of them, but I'm not sure why.
> >
> > Signed-off-by: Rafael Antognolli
> > ---
> > drivers/gpu/drm/Kconfig | 4 +
> > drivers/gpu/drm/Makefile | 1 +
> > drivers/gpu/drm/drm_aux-dev.c | 326
> > ++
> > 3 files changed, 331 insertions(+)
> > create mode 100644 drivers/gpu/drm/drm_aux-dev.c
> >
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 1a0a8df..eae847c 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -25,6 +25,10 @@ config DRM_MIPI_DSI
> > bool
> > depends on DRM
> >
> > +config DRM_AUX_CHARDEV
> > + tristate "DRM DP AUX Interface"
> > + depends on DRM
> > +
> > config DRM_KMS_HELPER
> > tristate
> > depends on DRM
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 45e7719..a1a94306 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -32,6 +32,7 @@ CFLAGS_drm_trace_points.o := -I$(src)
> >
> > obj-$(CONFIG_DRM) += drm.o
> > obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
> > +obj-$(CONFIG_DRM_AUX_CHARDEV) += drm_aux-dev.o
> > obj-$(CONFIG_DRM_TTM) += ttm/
> > obj-$(CONFIG_DRM_TDFX) += tdfx/
> > obj-$(CONFIG_DRM_R128) += r128/
> > diff --git a/drivers/gpu/drm/drm_aux-dev.c b/drivers/gpu/drm/drm_aux-dev.c
> > new file mode 100644
> > index 000..fcc334a
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_aux-dev.c
> > @@ -0,0 +1,326 @@
> > +/*
> > + * Copyright © 2015 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the
> > next
> > + * paragraph) shall be included in all copies or substantial portions of
> > the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + * Authors:
> > + *Rafael Antognolli
> > + *
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +struct drm_aux_dev {
> > + struct list_head list;
> > + unsigned index;
> > + struct drm_dp_aux *aux;
> > + struct device *dev;
> > +};
> > +
> > +#define DRM_AUX_MINORS 256
> > +static int drm_aux_dev_count = 0;
> > +static LIST_HEAD(drm_aux_dev_list);
> > +static DEFINE_SPINLOCK(drm_aux_dev_list_lock);
> > +
> > +static struct drm_aux_dev *dr
[PATCH 0/3] RFC: Add a drm_aux-dev module.
On Wed, Sep 16, 2015 at 11:47:21PM +0300, Ville Syrjälä wrote: > On Wed, Sep 16, 2015 at 01:09:54PM -0700, Daniel Vetter wrote: > > On Tue, Sep 15, 2015 at 10:03:09AM -0700, Rafael Antognolli wrote: > > > On Tue, Sep 15, 2015 at 07:46:55PM +0300, Ville Syrjälä wrote: > > > > On Tue, Sep 15, 2015 at 07:41:06PM +0300, Ville Syrjälä wrote: > > > > > On Tue, Sep 15, 2015 at 09:34:15AM -0700, Rafael Antognolli wrote: > > > > > > On Tue, Sep 15, 2015 at 10:35:19AM +0300, Ville Syrjälä wrote: > > > > > > > On Mon, Sep 14, 2015 at 04:12:29PM -0700, Rafael Antognolli wrote: > > > > > > > > This is a tentative implementation of a module that allows > > > > > > > > reading/writing > > > > > > > > arbitrary dpcd registers, following the suggestion from Daniel > > > > > > > > Vetter. It > > > > > > > > assumes the drm aux helpers were used by the driver. > > > > > > > > > > > > > > > > I tried to follow the i2c-dev implementation as close as > > > > > > > > possible, but the only > > > > > > > > operations that are provided on the dev node are two different > > > > > > > > ioctl's, one for > > > > > > > > reading a register and another one for writing it. > > > > > > > > > > > > > > Why would you use ioctls instead of normal read/write syscalls? > > > > > > > > > > > > > > > > > > > For writing, that should work fine, I can easily change that if you > > > > > > prefer. > > > > > > > > > > > > For reading, one has to first tell which register address is going > > > > > > to be > > > > > > read. > > > > > > > > > > seek() > > > > > > > > Sorry typo. Make that lseek(). > > > > > > > Oh, nice, I'll update the patch with this and remove the notifier stuff, > > > thanks! > > > > Well there's also other modes like i2c over aux, and that would be easier > > to support with an ioctl in a uniform way. So not sure how much value > > there is in reusing read/write for i2c ... > > We already have i2c-dev for i2c. So not sure what you're after here. > > i2c is a bit of a mess on the protocol level. Lots of devices do > interesting things with it, so it can't really make do with just > read/write/lseek. Apart from i2c-over-aux, the rest is all about > DPCD access, and for that read/write/lseek is all you ever need. I just noticed that I forgot to cc you guys, but yesterday I sent an updated version of the patch set to this list: http://lists.freedesktop.org/archives/dri-devel/2015-September/090366.html I also don't have a strong opinion about ioctl vs read/write/lseek, but at least my second implementation did get a little cleaner. Thanks, Rafael
[PATCH RFC v2 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_aux-N.
It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux-dev/drm_aux-N/connector, if
the connector device pointer was correctly set in the aux helper struct.
Two main operations are provided on the registers: read and write. The
address of the register to be read or written is given using lseek.
Reading or writing does not update the offset of the file.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/Kconfig | 4 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_aux-dev.c | 326 ++
3 files changed, 331 insertions(+)
create mode 100644 drivers/gpu/drm/drm_aux-dev.c
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 1a0a8df..eae847c 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,10 @@ config DRM_MIPI_DSI
bool
depends on DRM
+config DRM_AUX_CHARDEV
+ tristate "DRM DP AUX Interface"
+ depends on DRM
+
config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 45e7719..a1a94306 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -32,6 +32,7 @@ CFLAGS_drm_trace_points.o := -I$(src)
obj-$(CONFIG_DRM) += drm.o
obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
+obj-$(CONFIG_DRM_AUX_CHARDEV) += drm_aux-dev.o
obj-$(CONFIG_DRM_TTM) += ttm/
obj-$(CONFIG_DRM_TDFX) += tdfx/
obj-$(CONFIG_DRM_R128) += r128/
diff --git a/drivers/gpu/drm/drm_aux-dev.c b/drivers/gpu/drm/drm_aux-dev.c
new file mode 100644
index 000..fcc334a
--- /dev/null
+++ b/drivers/gpu/drm/drm_aux-dev.c
@@ -0,0 +1,326 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *Rafael Antognolli
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct drm_aux_dev {
+ struct list_head list;
+ unsigned index;
+ struct drm_dp_aux *aux;
+ struct device *dev;
+};
+
+#define DRM_AUX_MINORS 256
+static int drm_aux_dev_count = 0;
+static LIST_HEAD(drm_aux_dev_list);
+static DEFINE_SPINLOCK(drm_aux_dev_list_lock);
+
+static struct drm_aux_dev *drm_aux_dev_get_by_minor(unsigned index)
+{
+ struct drm_aux_dev *aux_dev;
+
+ spin_lock(_aux_dev_list_lock);
+ list_for_each_entry(aux_dev, _aux_dev_list, list) {
+ if (aux_dev->index == index)
+ goto found;
+ }
+
+ aux_dev = NULL;
+found:
+ spin_unlock(_aux_dev_list_lock);
+ return aux_dev;
+}
+
+static struct drm_aux_dev *drm_aux_dev_get_by_aux(struct drm_dp_aux *aux)
+{
+ struct drm_aux_dev *aux_dev;
+
+ spin_lock(_aux_dev_list_lock);
+ list_for_each_entry(aux_dev, _aux_dev_list, list) {
+ if (aux_dev->aux == aux)
+ goto found;
+ }
+
+ aux_dev = NULL;
+found:
+ spin_unlock(_aux_dev_list_lock);
+ return aux_dev;
+}
+
+static struct drm_aux_dev *get_free_drm_aux_dev(struct drm_dp_aux *aux)
+{
+ struct drm_aux_dev *aux_dev;
+ int index;
+
+ spin_lock(_aux_dev_list_lock);
+ index = drm_aux_dev_count;
+ spin_unlock(_aux_dev_list_lock);
+ if (index >= DRM_AUX_MINORS) {
+ printk(KERN_ERR "i2c-dev: Out of device minors (%d)\n",
+ index);
+ return ERR_PTR(-ENODEV);
+ }
+
+ aux_dev = kzalloc(sizeof(*aux_dev), GFP_KERNEL);
+ if (!aux_dev)
+ return ERR_PTR(-ENOMEM);
+ aux_dev->aux = aux;
+ aux_dev->index = index;
+
+ spin_lock(_aux_dev_list_lock);
+ drm_aux_dev_c
[PATCH RFC v2 2/3] drm/dp: Store the drm_connector device pointer on the helper.
This is useful to determine which connector owns this AUX channel.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/i915/intel_dp.c | 1 +
include/drm/drm_dp_helper.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a687250..23addc3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1079,6 +1079,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct
intel_connector *connector)
intel_dp->aux.name = name;
intel_dp->aux.dev = dev->dev;
+ intel_dp->aux.connector = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
DRM_DEBUG_KMS("registering %s bus for %s\n", name,
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index f92de26..d762535 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -702,6 +702,7 @@ struct drm_dp_aux {
const char *name;
struct i2c_adapter ddc;
struct device *dev;
+ struct device *connector;
struct mutex hw_mutex;
ssize_t (*transfer)(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg);
--
2.4.3
[PATCH RFC v2 1/3] drm/dp: Keep a list of drm_dp_aux helper.
This list will be used to get the aux channels registered through the
helpers. One function is provided to set a callback for added/removed
aux channels, and another function to iterate over the list of aux
channels.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/drm_dp_helper.c | 73 +
include/drm/drm_dp_helper.h | 5 +++
2 files changed, 78 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 9535c5b..41bdd93 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -746,6 +746,56 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
.master_xfer = drm_dp_i2c_xfer,
};
+struct drm_dp_aux_node {
+ struct klist_node list;
+ struct drm_dp_aux *aux;
+};
+
+static DEFINE_KLIST(drm_dp_aux_list, NULL, NULL);
+
+static int (*aux_dev_cb)(int action, struct drm_dp_aux *aux) = NULL;
+
+void drm_dp_aux_set_aux_dev(int (*fn)(int, struct drm_dp_aux *))
+{
+ aux_dev_cb = fn;
+}
+EXPORT_SYMBOL(drm_dp_aux_set_aux_dev);
+
+static struct drm_dp_aux *next_aux(struct klist_iter *i)
+{
+ struct klist_node *n = klist_next(i);
+ struct drm_dp_aux *aux = NULL;
+ struct drm_dp_aux_node *aux_node;
+
+ if (n) {
+ aux_node = container_of(n, struct drm_dp_aux_node, list);
+ aux = aux_node->aux;
+ }
+ return aux;
+}
+
+int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *))
+{
+ struct klist_iter i;
+ struct drm_dp_aux *aux;
+ int error = 0;
+
+ klist_iter_init(_dp_aux_list, );
+ while ((aux = next_aux()) && !error)
+ error = fn(aux, data);
+ klist_iter_exit();
+ return error;
+}
+EXPORT_SYMBOL(drm_dp_aux_for_each);
+
+static int drm_dp_aux_dev_inform(int action, struct drm_dp_aux *aux)
+{
+ if (aux_dev_cb) {
+ return aux_dev_cb(action, aux);
+ }
+ return 0;
+}
+
/**
* drm_dp_aux_register() - initialise and register aux channel
* @aux: DisplayPort AUX channel
@@ -754,6 +804,7 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
*/
int drm_dp_aux_register(struct drm_dp_aux *aux)
{
+ struct drm_dp_aux_node *aux_node;
mutex_init(>hw_mutex);
aux->ddc.algo = _dp_i2c_algo;
@@ -768,6 +819,14 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
sizeof(aux->ddc.name));
+ /* add aux to list and notify listeners */
+ aux_node = kzalloc(sizeof(*aux_node), GFP_KERNEL);
+ if (!aux_node)
+ return -ENOMEM;
+ aux_node->aux = aux;
+ klist_add_tail(_node->list, _dp_aux_list);
+ drm_dp_aux_dev_inform(DRM_DP_ADD_AUX, aux);
+
return i2c_add_adapter(>ddc);
}
EXPORT_SYMBOL(drm_dp_aux_register);
@@ -778,6 +837,20 @@ EXPORT_SYMBOL(drm_dp_aux_register);
*/
void drm_dp_aux_unregister(struct drm_dp_aux *aux)
{
+ struct klist_iter i;
+ struct klist_node *n;
+
+ klist_iter_init(_dp_aux_list, );
+ while ((n = klist_next())) {
+ struct drm_dp_aux_node *aux_node =
+ container_of(n, struct drm_dp_aux_node, list);
+ if (aux_node->aux == aux) {
+ klist_del(n);
+ kfree(aux_node);
+ break;
+ }
+ }
+ drm_dp_aux_dev_inform(DRM_DP_DEL_AUX, aux);
i2c_del_adapter(>ddc);
}
EXPORT_SYMBOL(drm_dp_aux_unregister);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 9ec4716..f92de26 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -763,7 +763,12 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct
drm_dp_link *link);
int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link);
int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
+#define DRM_DP_ADD_AUX 0x01
+#define DRM_DP_DEL_AUX 0x02
+
int drm_dp_aux_register(struct drm_dp_aux *aux);
void drm_dp_aux_unregister(struct drm_dp_aux *aux);
+void drm_dp_aux_set_aux_dev(int (*fn)(int, struct drm_dp_aux *));
+int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *));
#endif /* _DRM_DP_HELPER_H_ */
--
2.4.3
[PATCH RFC v2 0/3] Add a drm_aux-dev module.
Second attempt at implementing a module that allows reading/writing arbitrary dpcd registers. Changes to this version: - lseek is used to select the register to read/write; - read/write are used instead of ioctl; - no blocking_notifier is used, just a direct callback. One thing to notice is that I am not updating the file offset during read or write, which is kind of breaking the filesystem abstraction. But i2c-dev doesn't do it either, so I assumed it's fine. Rafael Antognolli (3): drm/dp: Keep a list of drm_dp_aux helper. drm/dp: Store the drm_connector device pointer on the helper. drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. drivers/gpu/drm/Kconfig | 4 + drivers/gpu/drm/Makefile| 1 + drivers/gpu/drm/drm_aux-dev.c | 326 drivers/gpu/drm/drm_dp_helper.c | 73 + drivers/gpu/drm/i915/intel_dp.c | 1 + include/drm/drm_dp_helper.h | 6 + 6 files changed, 411 insertions(+) create mode 100644 drivers/gpu/drm/drm_aux-dev.c -- 2.4.3
[PATCH 0/3] RFC: Add a drm_aux-dev module.
On Tue, Sep 15, 2015 at 07:46:55PM +0300, Ville Syrjälä wrote: > On Tue, Sep 15, 2015 at 07:41:06PM +0300, Ville Syrjälä wrote: > > On Tue, Sep 15, 2015 at 09:34:15AM -0700, Rafael Antognolli wrote: > > > On Tue, Sep 15, 2015 at 10:35:19AM +0300, Ville Syrjälä wrote: > > > > On Mon, Sep 14, 2015 at 04:12:29PM -0700, Rafael Antognolli wrote: > > > > > This is a tentative implementation of a module that allows > > > > > reading/writing > > > > > arbitrary dpcd registers, following the suggestion from Daniel > > > > > Vetter. It > > > > > assumes the drm aux helpers were used by the driver. > > > > > > > > > > I tried to follow the i2c-dev implementation as close as possible, > > > > > but the only > > > > > operations that are provided on the dev node are two different > > > > > ioctl's, one for > > > > > reading a register and another one for writing it. > > > > > > > > Why would you use ioctls instead of normal read/write syscalls? > > > > > > > > > > For writing, that should work fine, I can easily change that if you > > > prefer. > > > > > > For reading, one has to first tell which register address is going to be > > > read. > > > > seek() > > Sorry typo. Make that lseek(). > Oh, nice, I'll update the patch with this and remove the notifier stuff, thanks! > > > > > So it would require to first write the address on the file, to > > > then read. It was suggested that an ioctl to be used, and I understood > > > it was to solve this, but maybe I'm wrong. > > > > > > I don't have any particular preference for the API, could easily change > > > this code to use just read/writes. > > > > > > Thanks, > > > Rafael > > > > > > > > > > > > > I have at least 2 open questions: > > > > > > > > > > * Right now the AUX channels are stored in a global list inside the > > > > >drm_dp_helper implementation, and I assume that's not ideal. A > > > > > different > > > > >approach would be to iterate over the list of connectors, instead > > > > > of the > > > > >list of AUX channels, but that would require the struct > > > > > drm_connector or > > > > >similar to know about the respective aux helper. It could be added > > > > > as a > > > > >function to register that, or as a new method on the > > > > > drm_connector_funcs to > > > > >retrieve the aux channel helper. > > > > > > > > > > * From the created sysfs drm_aux-dev device it's possible to know > > > > > what is the > > > > >respective connector, but not the other way around. Is this good > > > > > enough? > > > > > > > > > > Please provide any feedback you have and tell me if this is the idea > > > > > you had > > > > > initially for this kind of implementation. Or, if it's nothing like > > > > > this, let > > > > > me know what else you had in mind. > > > > > > > > > > If I'm going in the right direction, I'll refine the patch to provide > > > > > full > > > > > documentation and tests if needed. > > > > > > > > > > Rafael Antognolli (3): > > > > > drm/dp: Keep a list of drm_dp_aux helper. > > > > > drm/dp: Store the drm_connector device pointer on the helper. > > > > > drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. > > > > > > > > > > Documentation/ioctl/ioctl-number.txt | 1 + > > > > > drivers/gpu/drm/Kconfig | 4 + > > > > > drivers/gpu/drm/Makefile | 1 + > > > > > drivers/gpu/drm/drm_aux-dev.c| 390 > > > > > +++ > > > > > drivers/gpu/drm/drm_dp_helper.c | 71 +++ > > > > > drivers/gpu/drm/i915/intel_dp.c | 1 + > > > > > include/drm/drm_dp_helper.h | 7 + > > > > > include/uapi/linux/Kbuild| 1 + > > > > > include/uapi/linux/drm_aux-dev.h | 45 > > > > > 9 files changed, 521 insertions(+) > > > > > create mode 100644 drivers/gpu/drm/drm_aux-dev.c > > > > > create mode 100644 include/uapi/linux/drm_aux-dev.h > > > > > > > > > > -- > > > > > 2.4.0 > > > > > > > > > > ___ > > > > > dri-devel mailing list > > > > > dri-devel at lists.freedesktop.org > > > > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel OTC > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Ville Syrjälä > Intel OTC
[PATCH 0/3] RFC: Add a drm_aux-dev module.
On Tue, Sep 15, 2015 at 10:35:19AM +0300, Ville Syrjälä wrote: > On Mon, Sep 14, 2015 at 04:12:29PM -0700, Rafael Antognolli wrote: > > This is a tentative implementation of a module that allows reading/writing > > arbitrary dpcd registers, following the suggestion from Daniel Vetter. It > > assumes the drm aux helpers were used by the driver. > > > > I tried to follow the i2c-dev implementation as close as possible, but the > > only > > operations that are provided on the dev node are two different ioctl's, one > > for > > reading a register and another one for writing it. > > Why would you use ioctls instead of normal read/write syscalls? > For writing, that should work fine, I can easily change that if you prefer. For reading, one has to first tell which register address is going to be read. So it would require to first write the address on the file, to then read. It was suggested that an ioctl to be used, and I understood it was to solve this, but maybe I'm wrong. I don't have any particular preference for the API, could easily change this code to use just read/writes. Thanks, Rafael > > > > I have at least 2 open questions: > > > > * Right now the AUX channels are stored in a global list inside the > >drm_dp_helper implementation, and I assume that's not ideal. A different > >approach would be to iterate over the list of connectors, instead of the > >list of AUX channels, but that would require the struct drm_connector or > >similar to know about the respective aux helper. It could be added as a > >function to register that, or as a new method on the drm_connector_funcs > > to > >retrieve the aux channel helper. > > > > * From the created sysfs drm_aux-dev device it's possible to know what is > > the > >respective connector, but not the other way around. Is this good enough? > > > > Please provide any feedback you have and tell me if this is the idea you had > > initially for this kind of implementation. Or, if it's nothing like this, > > let > > me know what else you had in mind. > > > > If I'm going in the right direction, I'll refine the patch to provide full > > documentation and tests if needed. > > > > Rafael Antognolli (3): > > drm/dp: Keep a list of drm_dp_aux helper. > > drm/dp: Store the drm_connector device pointer on the helper. > > drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. > > > > Documentation/ioctl/ioctl-number.txt | 1 + > > drivers/gpu/drm/Kconfig | 4 + > > drivers/gpu/drm/Makefile | 1 + > > drivers/gpu/drm/drm_aux-dev.c| 390 > > +++ > > drivers/gpu/drm/drm_dp_helper.c | 71 +++ > > drivers/gpu/drm/i915/intel_dp.c | 1 + > > include/drm/drm_dp_helper.h | 7 + > > include/uapi/linux/Kbuild| 1 + > > include/uapi/linux/drm_aux-dev.h | 45 > > 9 files changed, 521 insertions(+) > > create mode 100644 drivers/gpu/drm/drm_aux-dev.c > > create mode 100644 include/uapi/linux/drm_aux-dev.h > > > > -- > > 2.4.0 > > > > ___ > > dri-devel mailing list > > dri-devel at lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel OTC
[PATCH 1/3] drm/dp: Keep a list of drm_dp_aux helper.
On Tue, Sep 15, 2015 at 10:46:43AM +0300, Ville Syrjälä wrote:
> On Mon, Sep 14, 2015 at 04:12:30PM -0700, Rafael Antognolli wrote:
> > This list will be used to get the aux channels registered through the
> > helpers. Two functions are provided to register/unregister notifier
> > listeners on the list, and another functiont to iterate over the list of
> > aux channels.
> >
> > Signed-off-by: Rafael Antognolli
> > ---
> > drivers/gpu/drm/drm_dp_helper.c | 71
> > +
> > include/drm/drm_dp_helper.h | 6
> > 2 files changed, 77 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c
> > b/drivers/gpu/drm/drm_dp_helper.c
> > index 291734e..01a1489 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -710,6 +710,54 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
> > .master_xfer = drm_dp_i2c_xfer,
> > };
> >
> > +struct drm_dp_aux_node {
> > + struct klist_node list;
> > + struct drm_dp_aux *aux;
> > +};
> > +
> > +static DEFINE_KLIST(drm_dp_aux_list, NULL, NULL);
> > +
> > +static BLOCKING_NOTIFIER_HEAD(aux_notifier);
> > +
> > +int drm_dp_aux_register_notifier(struct notifier_block *nb)
> > +{
> > + return blocking_notifier_chain_register(_notifier, nb);
> > +}
> > +EXPORT_SYMBOL(drm_dp_aux_register_notifier);
>
> Why is this notifier stuff needed? Why not just register/unregister the
> aux-dev directly?
>
I am not sure it's needed, I was just looking for the best way of
informing aux-dev that a new aux channel was added.
By register/unregister the aux-dev directly, do you mean making this
drm_dp_helper aware of the aux dev, when it's registered, and directly
calling some callback to inform that new aux channels were added?
> >+
> > +int drm_dp_aux_unregister_notifier(struct notifier_block *nb)
> > +{
> > + return blocking_notifier_chain_unregister(_notifier, nb);
> > +}
> > +EXPORT_SYMBOL(drm_dp_aux_unregister_notifier);
> > +
> > +static struct drm_dp_aux *next_aux(struct klist_iter *i)
> > +{
> > + struct klist_node *n = klist_next(i);
> > + struct drm_dp_aux *aux = NULL;
> > + struct drm_dp_aux_node *aux_node;
> > +
> > + if (n) {
> > + aux_node = container_of(n, struct drm_dp_aux_node, list);
> > + aux = aux_node->aux;
> > + }
> > + return aux;
> > +}
> > +
> > +int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *))
> > +{
> > + struct klist_iter i;
> > + struct drm_dp_aux *aux;
> > + int error = 0;
> > +
> > + klist_iter_init(_dp_aux_list, );
> > + while ((aux = next_aux()) && !error)
> > + error = fn(aux, data);
> > + klist_iter_exit();
> > + return error;
> > +}
> > +EXPORT_SYMBOL(drm_dp_aux_for_each);
> > +
> > /**
> > * drm_dp_aux_register() - initialise and register aux channel
> > * @aux: DisplayPort AUX channel
> > @@ -718,6 +766,7 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
> > */
> > int drm_dp_aux_register(struct drm_dp_aux *aux)
> > {
> > + struct drm_dp_aux_node *aux_node;
> > mutex_init(>hw_mutex);
> >
> > aux->ddc.algo = _dp_i2c_algo;
> > @@ -732,6 +781,14 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
> > strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
> > sizeof(aux->ddc.name));
> >
> > + /* add aux to list and notify listeners */
> > + aux_node = kzalloc(sizeof(*aux_node), GFP_KERNEL);
> > + if (!aux_node)
> > + return -ENOMEM;
> > + aux_node->aux = aux;
> > + klist_add_tail(_node->list, _dp_aux_list);
> > + blocking_notifier_call_chain(_notifier, DRM_DP_ADD_AUX, aux);
> > +
> > return i2c_add_adapter(>ddc);
> > }
> > EXPORT_SYMBOL(drm_dp_aux_register);
> > @@ -742,6 +799,20 @@ EXPORT_SYMBOL(drm_dp_aux_register);
> > */
> > void drm_dp_aux_unregister(struct drm_dp_aux *aux)
> > {
> > + struct klist_iter i;
> > + struct klist_node *n;
> > +
> > + klist_iter_init(_dp_aux_list, );
> > + while ((n = klist_next())) {
> > + struct drm_dp_aux_node *aux_node =
> > + container_of(n, struct drm_dp_aux_node, list);
> > + if (aux_node->aux == aux) {
> > + klist_del(n);
> > + kfree(aux_node);
>
[PATCH 3/3] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
This module is heavily based on i2c-dev. Once loaded, it provides one
dev node per DP AUX channel, named drm_aux-N.
It's possible to know which connector owns this aux channel by looking
at the respective sysfs /sys/class/drm_aux-dev/drm_aux-N/connector, if
the connector device pointer was correctly set in the aux helper struct.
Two main operations are provided on the registers through ioctls: read
and write; and a helper struct is provided for that, used as:
- address is the address of the register to read/write;
- len is the number of bytes to read/write;
- buf is a pointer to a buffer where to store the read data, or
where the data to be written is already stored;
- the return value is the number of bytes read/written, on
success, or the error code otherwise. The number of
read/written bytes is also stored on the len field of the
struct.
Signed-off-by: Rafael Antognolli
---
Documentation/ioctl/ioctl-number.txt | 1 +
drivers/gpu/drm/Kconfig | 4 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_aux-dev.c| 390 +++
include/uapi/linux/Kbuild| 1 +
include/uapi/linux/drm_aux-dev.h | 45
6 files changed, 442 insertions(+)
create mode 100644 drivers/gpu/drm/drm_aux-dev.c
create mode 100644 include/uapi/linux/drm_aux-dev.h
diff --git a/Documentation/ioctl/ioctl-number.txt
b/Documentation/ioctl/ioctl-number.txt
index 611c522..ea76980 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -93,6 +93,7 @@ Code Seq#(hex) Include FileComments
';'64-7F linux/vfio.h
'@'00-0F linux/radeonfb.hconflict!
'@'00-0F drivers/video/aty/aty128fb.cconflict!
+'@'10-2F drm_aux-dev
'A'00-1F linux/apm_bios.hconflict!
'A'00-0F linux/agpgart.h conflict!
and drivers/char/agp/compat_ioctl.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 1a0a8df..eae847c 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -25,6 +25,10 @@ config DRM_MIPI_DSI
bool
depends on DRM
+config DRM_AUX_CHARDEV
+ tristate "DRM DP AUX Interface"
+ depends on DRM
+
config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 45e7719..a1a94306 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -32,6 +32,7 @@ CFLAGS_drm_trace_points.o := -I$(src)
obj-$(CONFIG_DRM) += drm.o
obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
+obj-$(CONFIG_DRM_AUX_CHARDEV) += drm_aux-dev.o
obj-$(CONFIG_DRM_TTM) += ttm/
obj-$(CONFIG_DRM_TDFX) += tdfx/
obj-$(CONFIG_DRM_R128) += r128/
diff --git a/drivers/gpu/drm/drm_aux-dev.c b/drivers/gpu/drm/drm_aux-dev.c
new file mode 100644
index 000..3ae9064
--- /dev/null
+++ b/drivers/gpu/drm/drm_aux-dev.c
@@ -0,0 +1,390 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *Rafael Antognolli
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct drm_aux_dev {
+ struct list_head list;
+ unsigned index;
+ struct drm_dp_aux *aux;
+ struct device *dev;
+};
+
+#define DRM_AUX_MINORS 256
+static int drm_aux_dev_count = 0;
+static LIST_HEAD(drm_aux_dev_list);
+static DEFINE_SPINLOCK(drm_aux_dev_list_lock);
+
+static struct drm_aux_dev *drm_aux_dev_get_by_minor(unsigned index)
+{
+ struct drm_aux_dev *aux_dev;
+
+ spin_lock(_aux_dev_list_lock);
+ list_for_each_entry(aux_dev, _aux_dev_list, list) {
+ if (aux_dev->index == index)
+ goto found;
+ }
[PATCH 2/3] drm/dp: Store the drm_connector device pointer on the helper.
This is useful to determine which connector owns this AUX channel.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/i915/intel_dp.c | 1 +
include/drm/drm_dp_helper.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f8f4d99..6f481fc 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1078,6 +1078,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, struct
intel_connector *connector)
intel_dp->aux.name = name;
intel_dp->aux.dev = dev->dev;
+ intel_dp->aux.connector = connector->base.kdev;
intel_dp->aux.transfer = intel_dp_aux_transfer;
DRM_DEBUG_KMS("registering %s bus for %s\n", name,
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 023620c..e481fbd 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -702,6 +702,7 @@ struct drm_dp_aux {
const char *name;
struct i2c_adapter ddc;
struct device *dev;
+ struct device *connector;
struct mutex hw_mutex;
ssize_t (*transfer)(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg);
--
2.4.0
[PATCH 1/3] drm/dp: Keep a list of drm_dp_aux helper.
This list will be used to get the aux channels registered through the
helpers. Two functions are provided to register/unregister notifier
listeners on the list, and another functiont to iterate over the list of
aux channels.
Signed-off-by: Rafael Antognolli
---
drivers/gpu/drm/drm_dp_helper.c | 71 +
include/drm/drm_dp_helper.h | 6
2 files changed, 77 insertions(+)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 291734e..01a1489 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -710,6 +710,54 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
.master_xfer = drm_dp_i2c_xfer,
};
+struct drm_dp_aux_node {
+ struct klist_node list;
+ struct drm_dp_aux *aux;
+};
+
+static DEFINE_KLIST(drm_dp_aux_list, NULL, NULL);
+
+static BLOCKING_NOTIFIER_HEAD(aux_notifier);
+
+int drm_dp_aux_register_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_register(_notifier, nb);
+}
+EXPORT_SYMBOL(drm_dp_aux_register_notifier);
+
+int drm_dp_aux_unregister_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_unregister(_notifier, nb);
+}
+EXPORT_SYMBOL(drm_dp_aux_unregister_notifier);
+
+static struct drm_dp_aux *next_aux(struct klist_iter *i)
+{
+ struct klist_node *n = klist_next(i);
+ struct drm_dp_aux *aux = NULL;
+ struct drm_dp_aux_node *aux_node;
+
+ if (n) {
+ aux_node = container_of(n, struct drm_dp_aux_node, list);
+ aux = aux_node->aux;
+ }
+ return aux;
+}
+
+int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *))
+{
+ struct klist_iter i;
+ struct drm_dp_aux *aux;
+ int error = 0;
+
+ klist_iter_init(_dp_aux_list, );
+ while ((aux = next_aux()) && !error)
+ error = fn(aux, data);
+ klist_iter_exit();
+ return error;
+}
+EXPORT_SYMBOL(drm_dp_aux_for_each);
+
/**
* drm_dp_aux_register() - initialise and register aux channel
* @aux: DisplayPort AUX channel
@@ -718,6 +766,7 @@ static const struct i2c_algorithm drm_dp_i2c_algo = {
*/
int drm_dp_aux_register(struct drm_dp_aux *aux)
{
+ struct drm_dp_aux_node *aux_node;
mutex_init(>hw_mutex);
aux->ddc.algo = _dp_i2c_algo;
@@ -732,6 +781,14 @@ int drm_dp_aux_register(struct drm_dp_aux *aux)
strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev),
sizeof(aux->ddc.name));
+ /* add aux to list and notify listeners */
+ aux_node = kzalloc(sizeof(*aux_node), GFP_KERNEL);
+ if (!aux_node)
+ return -ENOMEM;
+ aux_node->aux = aux;
+ klist_add_tail(_node->list, _dp_aux_list);
+ blocking_notifier_call_chain(_notifier, DRM_DP_ADD_AUX, aux);
+
return i2c_add_adapter(>ddc);
}
EXPORT_SYMBOL(drm_dp_aux_register);
@@ -742,6 +799,20 @@ EXPORT_SYMBOL(drm_dp_aux_register);
*/
void drm_dp_aux_unregister(struct drm_dp_aux *aux)
{
+ struct klist_iter i;
+ struct klist_node *n;
+
+ klist_iter_init(_dp_aux_list, );
+ while ((n = klist_next())) {
+ struct drm_dp_aux_node *aux_node =
+ container_of(n, struct drm_dp_aux_node, list);
+ if (aux_node->aux == aux) {
+ klist_del(n);
+ kfree(aux_node);
+ break;
+ }
+ }
+ blocking_notifier_call_chain(_notifier, DRM_DP_DEL_AUX, aux);
i2c_del_adapter(>ddc);
}
EXPORT_SYMBOL(drm_dp_aux_unregister);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 8c52d0ef1..023620c 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -763,7 +763,13 @@ int drm_dp_link_power_up(struct drm_dp_aux *aux, struct
drm_dp_link *link);
int drm_dp_link_power_down(struct drm_dp_aux *aux, struct drm_dp_link *link);
int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
+#define DRM_DP_ADD_AUX 0x01
+#define DRM_DP_DEL_AUX 0x02
+
int drm_dp_aux_register(struct drm_dp_aux *aux);
void drm_dp_aux_unregister(struct drm_dp_aux *aux);
+int drm_dp_aux_register_notifier(struct notifier_block *nb);
+int drm_dp_aux_unregister_notifier(struct notifier_block *nb);
+int drm_dp_aux_for_each(void *data, int (*fn)(struct drm_dp_aux *, void *));
#endif /* _DRM_DP_HELPER_H_ */
--
2.4.0
[PATCH 0/3] RFC: Add a drm_aux-dev module.
This is a tentative implementation of a module that allows reading/writing arbitrary dpcd registers, following the suggestion from Daniel Vetter. It assumes the drm aux helpers were used by the driver. I tried to follow the i2c-dev implementation as close as possible, but the only operations that are provided on the dev node are two different ioctl's, one for reading a register and another one for writing it. I have at least 2 open questions: * Right now the AUX channels are stored in a global list inside the drm_dp_helper implementation, and I assume that's not ideal. A different approach would be to iterate over the list of connectors, instead of the list of AUX channels, but that would require the struct drm_connector or similar to know about the respective aux helper. It could be added as a function to register that, or as a new method on the drm_connector_funcs to retrieve the aux channel helper. * From the created sysfs drm_aux-dev device it's possible to know what is the respective connector, but not the other way around. Is this good enough? Please provide any feedback you have and tell me if this is the idea you had initially for this kind of implementation. Or, if it's nothing like this, let me know what else you had in mind. If I'm going in the right direction, I'll refine the patch to provide full documentation and tests if needed. Rafael Antognolli (3): drm/dp: Keep a list of drm_dp_aux helper. drm/dp: Store the drm_connector device pointer on the helper. drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers. Documentation/ioctl/ioctl-number.txt | 1 + drivers/gpu/drm/Kconfig | 4 + drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/drm_aux-dev.c| 390 +++ drivers/gpu/drm/drm_dp_helper.c | 71 +++ drivers/gpu/drm/i915/intel_dp.c | 1 + include/drm/drm_dp_helper.h | 7 + include/uapi/linux/Kbuild| 1 + include/uapi/linux/drm_aux-dev.h | 45 9 files changed, 521 insertions(+) create mode 100644 drivers/gpu/drm/drm_aux-dev.c create mode 100644 include/uapi/linux/drm_aux-dev.h -- 2.4.0
[RFC 11/13] drm/dp: Add helper to dump DPCD
Hi, I'm back from vacation, so I'll be looking at this again. On Thu, Aug 20, 2015 at 04:26:42PM -0700, Rafael Antognolli wrote: > On Mon, Aug 17, 2015 at 10:02:04AM +0300, Jani Nikula wrote: > > On Fri, 14 Aug 2015, Rafael Antognolli > > wrote: > > > On Fri, Aug 14, 2015 at 02:56:55PM +0300, Jani Nikula wrote: > > >> On Wed, 12 Aug 2015, Thierry Reding wrote: > > >> > From: Thierry Reding > > >> > > > >> > The new drm_dp_dpcd_dump() helper dumps the contents of a DPCD to a > > >> > seq_file and can be used to make the DPCD available via debugfs for > > >> > example. > > >> > > >> See i915/i915_debugfs.c for one DPCD dump implementation. > > >> > > >> Around the time that was added, there was also some discussion (and > > >> patches [1]) to expose a read/write debugfs interface to DPCD, letting > > >> userspace access arbitrary DPCD registers. > > >> > > >> Just this week there was some discussion about revisiting that. It was > > >> about accessing some proprietary panel features, but there's also the > > >> ease of debugging without having to keep updating the kernel to dump > > >> more. > > >> > > >> I think it would be great to agree on a common debugfs interface to > > >> access DPCD arbitrarily. Last time I checked, the blocker to that was > > >> access to the aux channel from generic code; it's always driver > > >> specific. SMOP. ;) > > > > > > Do you mean it would require the generic code/interface to somehow route > > > this to the driver specific code? I am not sure yet how this works (if > > > there's something like it around), but I'll take a look. > > > > Drivers can choose to support DP AUX any way they like, and they don't > > have to use the common helpers to do so. It's pretty much an > > implementation detail. I think we could require the use of the common > > helpers to support generic DPCD access from debugfs, but we'd still have > > to come up with a way to get hold of struct drm_dp_aux (again, driver > > internals) given a drm_connector in generic debugfs code. > > I was under the assumption they always used the helpers, but I > understand that's not always the case. > > Anyway, I was going to propose a new helper to "store" the drm_dp_aux > inside the drm_connector. And just expose something on debugfs if it was > used. Something like: > > int drm_dp_aux_store(struct drm_dp_aux *aux, > struct drm_connector *connector); > > The helpers don't seem to know about drm_connector's though, so I'm not sure > this is a good approach. > > > Thierry, do you see any problems with having a connector function to get > > hold of its drm_dp_aux? > > Would this be a new function pointer inside struct drm_connector_funcs? > If so, I'm fine with this approach too. > > Regarding the interface, you mentioned that you didn't like it because > it had a state. What about just dumping the content of the register into > dmesg when one tries to read it, and use a different sintaxe for > writing, passing both the register addr and the value? > > Daniel had another suggestion, regarding an ioctl in debugfs, but I'm > not sure I clearly understand that yet. Regarding the interface, this is the comment Daniel did a while ago: "As mentioned in another thread I think the right approach here is to expose the dp aux interface through some ioctls in debugfs or dev somewhere, and then add simple tools on top of that. Similar to what we have with i2c. That would allow us to implement additional things on top like mst inspection or using the i2c-over-dp-aux sidechannel." If exposing through an ioctl, shouldn't it be yet another DRM ioctl? And if not, then I should create another /dev node/file specifically for this purpose, right? And if we are going with ioctls, I still don't understand exactly how it relates to debugfs. Thanks, Rafael
[RFC 11/13] drm/dp: Add helper to dump DPCD
On Mon, Aug 17, 2015 at 10:02:04AM +0300, Jani Nikula wrote: > On Fri, 14 Aug 2015, Rafael Antognolli wrote: > > On Fri, Aug 14, 2015 at 02:56:55PM +0300, Jani Nikula wrote: > >> On Wed, 12 Aug 2015, Thierry Reding wrote: > >> > From: Thierry Reding > >> > > >> > The new drm_dp_dpcd_dump() helper dumps the contents of a DPCD to a > >> > seq_file and can be used to make the DPCD available via debugfs for > >> > example. > >> > >> See i915/i915_debugfs.c for one DPCD dump implementation. > >> > >> Around the time that was added, there was also some discussion (and > >> patches [1]) to expose a read/write debugfs interface to DPCD, letting > >> userspace access arbitrary DPCD registers. > >> > >> Just this week there was some discussion about revisiting that. It was > >> about accessing some proprietary panel features, but there's also the > >> ease of debugging without having to keep updating the kernel to dump > >> more. > >> > >> I think it would be great to agree on a common debugfs interface to > >> access DPCD arbitrarily. Last time I checked, the blocker to that was > >> access to the aux channel from generic code; it's always driver > >> specific. SMOP. ;) > > > > Do you mean it would require the generic code/interface to somehow route > > this to the driver specific code? I am not sure yet how this works (if > > there's something like it around), but I'll take a look. > > Drivers can choose to support DP AUX any way they like, and they don't > have to use the common helpers to do so. It's pretty much an > implementation detail. I think we could require the use of the common > helpers to support generic DPCD access from debugfs, but we'd still have > to come up with a way to get hold of struct drm_dp_aux (again, driver > internals) given a drm_connector in generic debugfs code. I was under the assumption they always used the helpers, but I understand that's not always the case. Anyway, I was going to propose a new helper to "store" the drm_dp_aux inside the drm_connector. And just expose something on debugfs if it was used. Something like: int drm_dp_aux_store(struct drm_dp_aux *aux, struct drm_connector *connector); The helpers don't seem to know about drm_connector's though, so I'm not sure this is a good approach. > Thierry, do you see any problems with having a connector function to get > hold of its drm_dp_aux? Would this be a new function pointer inside struct drm_connector_funcs? If so, I'm fine with this approach too. Regarding the interface, you mentioned that you didn't like it because it had a state. What about just dumping the content of the register into dmesg when one tries to read it, and use a different sintaxe for writing, passing both the register addr and the value? Daniel had another suggestion, regarding an ioctl in debugfs, but I'm not sure I clearly understand that yet. Thanks, Rafael
[RFC 11/13] drm/dp: Add helper to dump DPCD
On Fri, Aug 14, 2015 at 02:56:55PM +0300, Jani Nikula wrote:
> On Wed, 12 Aug 2015, Thierry Reding wrote:
> > From: Thierry Reding
> >
> > The new drm_dp_dpcd_dump() helper dumps the contents of a DPCD to a
> > seq_file and can be used to make the DPCD available via debugfs for
> > example.
>
> See i915/i915_debugfs.c for one DPCD dump implementation.
>
> Around the time that was added, there was also some discussion (and
> patches [1]) to expose a read/write debugfs interface to DPCD, letting
> userspace access arbitrary DPCD registers.
>
> Just this week there was some discussion about revisiting that. It was
> about accessing some proprietary panel features, but there's also the
> ease of debugging without having to keep updating the kernel to dump
> more.
>
> I think it would be great to agree on a common debugfs interface to
> access DPCD arbitrarily. Last time I checked, the blocker to that was
> access to the aux channel from generic code; it's always driver
> specific. SMOP. ;)
Do you mean it would require the generic code/interface to somehow route
this to the driver specific code? I am not sure yet how this works (if
there's something like it around), but I'll take a look.
> I could put some effort into this (maybe Rafael too?), as long as we
> could agree on the interface. As I wrote in the referenced thread, I
> wasn't thrilled about what was proposed.
>
Yes, I'm willing to put effort into this, for sure. Any help pointing to
which direction to follow is greatly appreciated.
Thanks,
Rafael
>
>
> [1] http://mid.gmane.org/1428493301-20293-1-git-send-email-durgadoss.r at
> intel.com
>
>
>
> >
> > Signed-off-by: Thierry Reding
> > ---
> > drivers/gpu/drm/drm_dp_helper.c | 146
> >
> > include/drm/drm_dp_helper.h | 2 +
> > 2 files changed, 148 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c
> > b/drivers/gpu/drm/drm_dp_helper.c
> > index 8968201ea93c..ea74884c9cb3 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -27,6 +27,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -292,6 +293,151 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux
> > *aux,
> > }
> > EXPORT_SYMBOL(drm_dp_dpcd_read_link_status);
> >
> > +/**
> > + * drm_dp_dpcd_dump() - dump DPCD content
> > + * @aux: DisplayPort AUX channel
> > + * @s: destination for DPCD dump
> > + *
> > + * Reads registers from the DPCD via a DisplayPort AUX channel and dumps
> > them
> > + * to a seq_file.
> > + */
> > +void drm_dp_dpcd_dump(struct drm_dp_aux *aux, struct seq_file *s)
> > +{
> > +#define DUMP_REG(aux, offset) ({ \
> > + u8 value; \
> > + int err;\
> > + err = drm_dp_dpcd_readb(aux, offset, ); \
> > + if (err < 0) { \
> > + dev_err((aux)->dev, "failed to read %s: %d\n", \
> > + #offset, err); \
> > + return; \
> > + } \
> > + seq_printf(s, "%-35s 0x%04x 0x%02x\n", #offset, offset, \
> > + value); \
> > + })
> > +
> > + DUMP_REG(aux, DP_DPCD_REV);
> > + DUMP_REG(aux, DP_MAX_LINK_RATE);
> > + DUMP_REG(aux, DP_MAX_LANE_COUNT);
> > + DUMP_REG(aux, DP_MAX_DOWNSPREAD);
> > + DUMP_REG(aux, DP_NORP);
> > + DUMP_REG(aux, DP_DOWNSTREAMPORT_PRESENT);
> > + DUMP_REG(aux, DP_MAIN_LINK_CHANNEL_CODING);
> > + DUMP_REG(aux, DP_DOWN_STREAM_PORT_COUNT);
> > + DUMP_REG(aux, DP_RECEIVE_PORT_0_CAP_0);
> > + DUMP_REG(aux, DP_RECEIVE_PORT_0_BUFFER_SIZE);
> > + DUMP_REG(aux, DP_RECEIVE_PORT_1_CAP_0);
> > + DUMP_REG(aux, DP_RECEIVE_PORT_1_BUFFER_SIZE);
> > + DUMP_REG(aux, DP_I2C_SPEED_CAP);
> > + DUMP_REG(aux, DP_EDP_CONFIGURATION_CAP);
> > + DUMP_REG(aux, DP_TRAINING_AUX_RD_INTERVAL);
> > + DUMP_REG(aux, DP_ADAPTER_CAP);
> > + DUMP_REG(aux, DP_SUPPORTED_LINK_RATES);
> > + DUMP_REG(aux, DP_FAUX_CAP);
> > + DUMP_REG(aux, DP_MSTM_CAP);
> > + DUMP_REG(aux, DP_NUMBER_OF_AUDIO_ENDPOINTS);
> > + DUMP_REG(aux, DP_AV_GRANULARITY);
> > + DUMP_REG(aux, DP_AUD_DEC_LAT0);
> > + DUMP_REG(aux, DP_AUD_DEC_LAT1);
> > + DUMP_REG(aux, DP_AUD_PP_LAT0);
> > + DUMP_REG(aux, DP_AUD_PP_LAT1);
> > + DUMP_REG(aux, DP_VID_INTER_LAT);
> > + DUMP_REG(aux, DP_VID_PROG_LAT);
> > + DUMP_REG(aux, DP_REP_LAT);
> > + DUMP_REG(aux, DP_AUD_DEL_INS0);
> > + DUMP_REG(aux, DP_AUD_DEL_INS1);
> > + DUMP_REG(aux, DP_AUD_DEL_INS2);
> > + DUMP_REG(aux, DP_RECEIVER_ALPM_CAP);
> > + DUMP_REG(aux, DP_AUD_DEL_INS0);
> > + DUMP_REG(aux, DP_GUID);
> > + DUMP_REG(aux,
