[i-g-t PATCH v2 0/2] Avoid calling DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID

[i-g-t PATCH v2 0/2] Avoid calling DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID

[Tomeu Vizoso]

Hi,

these two patches remove the two last uses of
DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID, with the intention of making the
tests runnable on !i915.

I have looked in the kernel and cannot find any point where the pipe ID
didn't match the index of the CRTC as returned by GETRESOURCES, so I'm
not increasing the required libdrm version for now.

I wasn't able to fully run intel-gpu-overlay with the kms backend, but I
have checked with gdb that the changed code keeps working correctly (on
my system the CRTC with ID 21 is chosen for the pipe 0).

Thanks,

Tomeu

Changes in v2:
- Fix include path as suggested by Thomas
- Remove some dead code

Micah Fedke (2):
  lib: update kmstest_get_pipe_from_crtc_id
  overlay: Remove crtc<->pipe mapping code from kms-overlay

 lib/igt_kms.c             | 33 ++++++++++++++++++++++++---------
 overlay/Makefile.am       |  4 ++--
 overlay/kms/kms-overlay.c | 16 ++++------------
 3 files changed, 30 insertions(+), 23 deletions(-)

-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[i-g-t PATCH v2 1/2] lib: update kmstest_get_pipe_from_crtc_id

[Tomeu Vizoso]

From: Micah Fedke <micah.fedke@collabora.co.uk>

This function uses an intel-specific ioctl to fetch a mapping between pipes and
crtc ids, but this technique is outdated as the crtc id is now always
equivalent to its index in the array of crtcs returned by the kernel.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

Changes in v2: None

 lib/igt_kms.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 9f18aef72ea0..1d9acce31676 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -261,20 +261,35 @@ void kmstest_dump_mode(drmModeModeInfo *mode)
  * @fd: DRM fd
  * @crtc_id: DRM CRTC id
  *
- * Returns: The pipe number for the given DRM CRTC @crtc_id. This maps directly
- * to an enum pipe value used in other helper functions.
+ * Returns: The crtc index for the given DRM CRTC ID @crtc_id. The crtc index
+ * is the equivalent of the pipe id.  This value maps directly to an enum pipe
+ * value used in other helper functions.  Returns 0 if the index could not be
+ * determined.
  */
+
 int kmstest_get_pipe_from_crtc_id(int fd, int crtc_id)
 {
-	struct drm_i915_get_pipe_from_crtc_id pfci;
-	int ret;
+	drmModeRes *res;
+	drmModeCrtc *crtc;
+	int i, cur_id;
+
+	res = drmModeGetResources(fd);
+	igt_assert(res);
+
+	for (i = 0; i < res->count_crtcs; i++) {
+		crtc = drmModeGetCrtc(fd, res->crtcs[i]);
+		igt_assert(crtc);
+		cur_id = crtc->crtc_id;
+		drmModeFreeCrtc(crtc);
+		if (cur_id == crtc_id)
+			break;
+	}
+
+	drmModeFreeResources(res);
 
-	memset(&pfci, 0, sizeof(pfci));
-	pfci.crtc_id = crtc_id;
-	ret = drmIoctl(fd, DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID, &pfci);
-	igt_assert(ret == 0);
+	igt_assert(i < res->count_crtcs);
 
-	return pfci.pipe;
+	return i;
 }
 
 /**
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [i-g-t PATCH v2 1/2] lib: update kmstest_get_pipe_from_crtc_id

[Daniel Vetter]

On Wed, Mar 09, 2016 at 04:57:37PM +0100, Tomeu Vizoso wrote:
> From: Micah Fedke <micah.fedke@collabora.co.uk>
> 
> This function uses an intel-specific ioctl to fetch a mapping between pipes and
> crtc ids, but this technique is outdated as the crtc id is now always
> equivalent to its index in the array of crtcs returned by the kernel.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Ok, there's 2 use-cases for this one really:
- intel hw specific stuff, like CS scanline waits (like in the overlay, or
  other places)
- to figure out the pipe ID for the vblank ioctl

The former is i915 specific, the later is generic. I think the correct way
to fix this is to have a helper to encode a drmCrtc into the vblank flags.
That's needed anyway since the rules are silly: First a flag to select
between pipe 0 and 1, then a bitfield for 2 and later.

Once we have that helper (maybe call it kmstests_vblank_flags_for_crtc or
similar) we can roll it out to all the places that use the vblank ioctl.
Or which check vblank events (from atomic or legacy page flips), and
should be left with just the i915-specific usage.

A bit more work, but I think cleaner in intent.

Thoughts?
-Daniel

> ---
> 
> Changes in v2: None
> 
>  lib/igt_kms.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index 9f18aef72ea0..1d9acce31676 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -261,20 +261,35 @@ void kmstest_dump_mode(drmModeModeInfo *mode)
>   * @fd: DRM fd
>   * @crtc_id: DRM CRTC id
>   *
> - * Returns: The pipe number for the given DRM CRTC @crtc_id. This maps directly
> - * to an enum pipe value used in other helper functions.
> + * Returns: The crtc index for the given DRM CRTC ID @crtc_id. The crtc index
> + * is the equivalent of the pipe id.  This value maps directly to an enum pipe
> + * value used in other helper functions.  Returns 0 if the index could not be
> + * determined.
>   */
> +
>  int kmstest_get_pipe_from_crtc_id(int fd, int crtc_id)
>  {
> -	struct drm_i915_get_pipe_from_crtc_id pfci;
> -	int ret;
> +	drmModeRes *res;
> +	drmModeCrtc *crtc;
> +	int i, cur_id;
> +
> +	res = drmModeGetResources(fd);
> +	igt_assert(res);
> +
> +	for (i = 0; i < res->count_crtcs; i++) {
> +		crtc = drmModeGetCrtc(fd, res->crtcs[i]);
> +		igt_assert(crtc);
> +		cur_id = crtc->crtc_id;
> +		drmModeFreeCrtc(crtc);
> +		if (cur_id == crtc_id)
> +			break;
> +	}
> +
> +	drmModeFreeResources(res);
>  
> -	memset(&pfci, 0, sizeof(pfci));
> -	pfci.crtc_id = crtc_id;
> -	ret = drmIoctl(fd, DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID, &pfci);
> -	igt_assert(ret == 0);
> +	igt_assert(i < res->count_crtcs);
>  
> -	return pfci.pipe;
> +	return i;
>  }
>  
>  /**
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [i-g-t PATCH v2 1/2] lib: update kmstest_get_pipe_from_crtc_id

[Tomeu Vizoso]

On 18 April 2016 at 14:09, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Mar 09, 2016 at 04:57:37PM +0100, Tomeu Vizoso wrote:
>> From: Micah Fedke <micah.fedke@collabora.co.uk>
>>
>> This function uses an intel-specific ioctl to fetch a mapping between pipes and
>> crtc ids, but this technique is outdated as the crtc id is now always
>> equivalent to its index in the array of crtcs returned by the kernel.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>
> Ok, there's 2 use-cases for this one really:
> - intel hw specific stuff, like CS scanline waits (like in the overlay, or
>   other places)
> - to figure out the pipe ID for the vblank ioctl
>
> The former is i915 specific, the later is generic. I think the correct way
> to fix this is to have a helper to encode a drmCrtc into the vblank flags.
> That's needed anyway since the rules are silly: First a flag to select
> between pipe 0 and 1, then a bitfield for 2 and later.
>
> Once we have that helper (maybe call it kmstests_vblank_flags_for_crtc or
> similar) we can roll it out to all the places that use the vblank ioctl.
> Or which check vblank events (from atomic or legacy page flips), and
> should be left with just the i915-specific usage.
>
> A bit more work, but I think cleaner in intent.
>
> Thoughts?

Sounds good in principle, will give it a try.

Thanks,

Tomeu

> -Daniel
>
>> ---
>>
>> Changes in v2: None
>>
>>  lib/igt_kms.c | 33 ++++++++++++++++++++++++---------
>>  1 file changed, 24 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>> index 9f18aef72ea0..1d9acce31676 100644
>> --- a/lib/igt_kms.c
>> +++ b/lib/igt_kms.c
>> @@ -261,20 +261,35 @@ void kmstest_dump_mode(drmModeModeInfo *mode)
>>   * @fd: DRM fd
>>   * @crtc_id: DRM CRTC id
>>   *
>> - * Returns: The pipe number for the given DRM CRTC @crtc_id. This maps directly
>> - * to an enum pipe value used in other helper functions.
>> + * Returns: The crtc index for the given DRM CRTC ID @crtc_id. The crtc index
>> + * is the equivalent of the pipe id.  This value maps directly to an enum pipe
>> + * value used in other helper functions.  Returns 0 if the index could not be
>> + * determined.
>>   */
>> +
>>  int kmstest_get_pipe_from_crtc_id(int fd, int crtc_id)
>>  {
>> -     struct drm_i915_get_pipe_from_crtc_id pfci;
>> -     int ret;
>> +     drmModeRes *res;
>> +     drmModeCrtc *crtc;
>> +     int i, cur_id;
>> +
>> +     res = drmModeGetResources(fd);
>> +     igt_assert(res);
>> +
>> +     for (i = 0; i < res->count_crtcs; i++) {
>> +             crtc = drmModeGetCrtc(fd, res->crtcs[i]);
>> +             igt_assert(crtc);
>> +             cur_id = crtc->crtc_id;
>> +             drmModeFreeCrtc(crtc);
>> +             if (cur_id == crtc_id)
>> +                     break;
>> +     }
>> +
>> +     drmModeFreeResources(res);
>>
>> -     memset(&pfci, 0, sizeof(pfci));
>> -     pfci.crtc_id = crtc_id;
>> -     ret = drmIoctl(fd, DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID, &pfci);
>> -     igt_assert(ret == 0);
>> +     igt_assert(i < res->count_crtcs);
>>
>> -     return pfci.pipe;
>> +     return i;
>>  }
>>
>>  /**
>> --
>> 2.5.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [i-g-t PATCH v2 1/2] lib: update kmstest_get_pipe_from_crtc_id

[Daniel Vetter]

On Mon, Apr 18, 2016 at 02:20:43PM +0200, Tomeu Vizoso wrote:
> On 18 April 2016 at 14:09, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Mar 09, 2016 at 04:57:37PM +0100, Tomeu Vizoso wrote:
> >> From: Micah Fedke <micah.fedke@collabora.co.uk>
> >>
> >> This function uses an intel-specific ioctl to fetch a mapping between pipes and
> >> crtc ids, but this technique is outdated as the crtc id is now always
> >> equivalent to its index in the array of crtcs returned by the kernel.
> >>
> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >
> > Ok, there's 2 use-cases for this one really:
> > - intel hw specific stuff, like CS scanline waits (like in the overlay, or
> >   other places)
> > - to figure out the pipe ID for the vblank ioctl
> >
> > The former is i915 specific, the later is generic. I think the correct way
> > to fix this is to have a helper to encode a drmCrtc into the vblank flags.
> > That's needed anyway since the rules are silly: First a flag to select
> > between pipe 0 and 1, then a bitfield for 2 and later.
> >
> > Once we have that helper (maybe call it kmstests_vblank_flags_for_crtc or
> > similar) we can roll it out to all the places that use the vblank ioctl.
> > Or which check vblank events (from atomic or legacy page flips), and
> > should be left with just the i915-specific usage.
> >
> > A bit more work, but I think cleaner in intent.
> >
> > Thoughts?
> 
> Sounds good in principle, will give it a try.

Otoh, sounds like busywork on 2nd consideration. Mostly I just thought of
this because of the overlay patch. But really, numbers match exactly
between vblank pipe encoding and i915 pipe, so might as well go with this
one patch here.

Applied 1/2 (and only that one) to igt, thanks.
-Daniel

> 
> Thanks,
> 
> Tomeu
> 
> > -Daniel
> >
> >> ---
> >>
> >> Changes in v2: None
> >>
> >>  lib/igt_kms.c | 33 ++++++++++++++++++++++++---------
> >>  1 file changed, 24 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> >> index 9f18aef72ea0..1d9acce31676 100644
> >> --- a/lib/igt_kms.c
> >> +++ b/lib/igt_kms.c
> >> @@ -261,20 +261,35 @@ void kmstest_dump_mode(drmModeModeInfo *mode)
> >>   * @fd: DRM fd
> >>   * @crtc_id: DRM CRTC id
> >>   *
> >> - * Returns: The pipe number for the given DRM CRTC @crtc_id. This maps directly
> >> - * to an enum pipe value used in other helper functions.
> >> + * Returns: The crtc index for the given DRM CRTC ID @crtc_id. The crtc index
> >> + * is the equivalent of the pipe id.  This value maps directly to an enum pipe
> >> + * value used in other helper functions.  Returns 0 if the index could not be
> >> + * determined.
> >>   */
> >> +
> >>  int kmstest_get_pipe_from_crtc_id(int fd, int crtc_id)
> >>  {
> >> -     struct drm_i915_get_pipe_from_crtc_id pfci;
> >> -     int ret;
> >> +     drmModeRes *res;
> >> +     drmModeCrtc *crtc;
> >> +     int i, cur_id;
> >> +
> >> +     res = drmModeGetResources(fd);
> >> +     igt_assert(res);
> >> +
> >> +     for (i = 0; i < res->count_crtcs; i++) {
> >> +             crtc = drmModeGetCrtc(fd, res->crtcs[i]);
> >> +             igt_assert(crtc);
> >> +             cur_id = crtc->crtc_id;
> >> +             drmModeFreeCrtc(crtc);
> >> +             if (cur_id == crtc_id)
> >> +                     break;
> >> +     }
> >> +
> >> +     drmModeFreeResources(res);
> >>
> >> -     memset(&pfci, 0, sizeof(pfci));
> >> -     pfci.crtc_id = crtc_id;
> >> -     ret = drmIoctl(fd, DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID, &pfci);
> >> -     igt_assert(ret == 0);
> >> +     igt_assert(i < res->count_crtcs);
> >>
> >> -     return pfci.pipe;
> >> +     return i;
> >>  }
> >>
> >>  /**
> >> --
> >> 2.5.0
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[i-g-t PATCH v2 2/2] overlay: Remove crtc<->pipe mapping code from kms-overlay

[Tomeu Vizoso]

From: Micah Fedke <micah.fedke@collabora.co.uk>

the crtc id is now always equivalent to its index in the array of crtcs
returned by the kernel

Signed-off-by: Micah Fedke <micah.fedke@collabora.co.uk>
[tomeu: Fixed include path and removed some dead code]
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

Changes in v2:
- Fix include path as suggested by Thomas
- Remove some dead code

 overlay/Makefile.am       |  4 ++--
 overlay/kms/kms-overlay.c | 16 ++++------------
 2 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/overlay/Makefile.am b/overlay/Makefile.am
index c648875d24a7..eed2f9293ce0 100644
--- a/overlay/Makefile.am
+++ b/overlay/Makefile.am
@@ -3,8 +3,8 @@ bin_PROGRAMS = intel-gpu-overlay
 endif
 
 AM_CPPFLAGS = -I.
-AM_CFLAGS = $(DRM_CFLAGS) $(PCIACCESS_CFLAGS) $(CWARNFLAGS) $(CAIRO_CFLAGS) $(OVERLAY_CFLAGS)
-LDADD = $(DRM_LIBS) $(PCIACCESS_LIBS) $(CAIRO_LIBS) $(OVERLAY_LIBS)
+AM_CFLAGS = -I$(top_srcdir)/lib $(DRM_CFLAGS) $(PCIACCESS_CFLAGS) $(CWARNFLAGS) $(CAIRO_CFLAGS) $(OVERLAY_CFLAGS)
+LDADD = $(top_builddir)/lib/libintel_tools.la $(LIBUNWIND_LIBS) $(DRM_LIBS) $(PCIACCESS_LIBS) $(CAIRO_LIBS) $(OVERLAY_LIBS)
 
 intel_gpu_overlay_SOURCES = \
 	i915_pciids.h \
diff --git a/overlay/kms/kms-overlay.c b/overlay/kms/kms-overlay.c
index cfb3d5ae1dd7..494d57eeeb6b 100644
--- a/overlay/kms/kms-overlay.c
+++ b/overlay/kms/kms-overlay.c
@@ -38,6 +38,7 @@
 #include <xf86drmMode.h>
 #include <i915_drm.h>
 #include "../overlay.h"
+#include "igt_kms.h"
 //#include "rgb2yuv.h"
 
 #ifndef ALIGN
@@ -240,20 +241,11 @@ kms_overlay_create(struct config *config, int *width, int *height)
 	priv->crtc = 0;
 
 	for (i = 0; i < kmode->count_crtcs; i++) {
-		struct drm_i915_get_pipe_from_crtc_id get_pipe;
-
-		get_pipe.pipe = 0;
-		get_pipe.crtc_id = kmode->crtcs[i];
-		if (drmIoctl(priv->fd,
-			     DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID,
-			     &get_pipe)) {
-			continue;
-		}
-
-		if (get_pipe.pipe != pipe)
+		if (kmstest_get_pipe_from_crtc_id(priv->fd,
+						  kmode->crtcs[i]) != pipe)
 			continue;
 
-		priv->crtc = get_pipe.crtc_id;
+		priv->crtc = kmode->crtcs[i];
 	}
 
 	if (priv->crtc == 0)
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [i-g-t PATCH v2 0/2] Avoid calling DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID

[Tomeu Vizoso]

Hello,

can these two patches be pushed if everybody is fine with them?

Thanks,

Tomeu

On 9 March 2016 at 16:57, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> Hi,
>
> these two patches remove the two last uses of
> DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID, with the intention of making the
> tests runnable on !i915.
>
> I have looked in the kernel and cannot find any point where the pipe ID
> didn't match the index of the CRTC as returned by GETRESOURCES, so I'm
> not increasing the required libdrm version for now.
>
> I wasn't able to fully run intel-gpu-overlay with the kms backend, but I
> have checked with gdb that the changed code keeps working correctly (on
> my system the CRTC with ID 21 is chosen for the pipe 0).
>
> Thanks,
>
> Tomeu
>
> Changes in v2:
> - Fix include path as suggested by Thomas
> - Remove some dead code
>
> Micah Fedke (2):
>   lib: update kmstest_get_pipe_from_crtc_id
>   overlay: Remove crtc<->pipe mapping code from kms-overlay
>
>  lib/igt_kms.c             | 33 ++++++++++++++++++++++++---------
>  overlay/Makefile.am       |  4 ++--
>  overlay/kms/kms-overlay.c | 16 ++++------------
>  3 files changed, 30 insertions(+), 23 deletions(-)
>
> --
> 2.5.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx