Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/5] igt/kms_getfb: Check the iface exists before use
On Wed, Oct 03, 2018 at 04:04:20PM +0300, Joonas Lahtinen wrote: > Quoting Antonio Argenziano (2018-10-02 23:27:46) > > > > > > On 02/10/18 01:30, Joonas Lahtinen wrote: > > > Quoting Antonio Argenziano (2018-10-01 22:53:46) > > >> Fair enough. > > >> > > >> Acked-by: Antonio Argenziano > > >> > > >> for the series. > > > > > > Please, read the following chapters (they're applicable for the patch > > > tag meanings in IGT, too): > > > > > > https://www.kernel.org/doc/html/v4.18/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by > > > https://www.kernel.org/doc/html/v4.18/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes > > > > > > If we spend the time to actually review the patches, that should be > > > documented with a proper Reviewed-by and not a vague Acked-by. > > > > KMS is really an area I do not know much about. While I can say the > > patches are looking good on the IGT side, I cannot guarantee they use > > the KMS interface appropriately therefore the 'Acked-by'. After reading > > the documentation you linked I think it fits rather well since the only > > feedback I gave was on a small oversight. > > Fair enough. For future reference, you may want to comment when giving > your Acked-by, the reason for only limited review and not full R-b. Fyi I also did review this one, but an older version, and those comments didn't get addressed. So not the only thing that went slightly sideways here. -Daniel -- 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: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/5] igt/kms_getfb: Check the iface exists before use
Quoting Antonio Argenziano (2018-10-02 23:27:46) > > > On 02/10/18 01:30, Joonas Lahtinen wrote: > > Quoting Antonio Argenziano (2018-10-01 22:53:46) > >> Fair enough. > >> > >> Acked-by: Antonio Argenziano > >> > >> for the series. > > > > Please, read the following chapters (they're applicable for the patch > > tag meanings in IGT, too): > > > > https://www.kernel.org/doc/html/v4.18/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by > > https://www.kernel.org/doc/html/v4.18/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes > > > > If we spend the time to actually review the patches, that should be > > documented with a proper Reviewed-by and not a vague Acked-by. > > KMS is really an area I do not know much about. While I can say the > patches are looking good on the IGT side, I cannot guarantee they use > the KMS interface appropriately therefore the 'Acked-by'. After reading > the documentation you linked I think it fits rather well since the only > feedback I gave was on a small oversight. Fair enough. For future reference, you may want to comment when giving your Acked-by, the reason for only limited review and not full R-b. Regards, Joonas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/5] igt/kms_getfb: Check the iface exists before use
On 02/10/18 01:30, Joonas Lahtinen wrote: Quoting Antonio Argenziano (2018-10-01 22:53:46) Fair enough. Acked-by: Antonio Argenziano for the series. Please, read the following chapters (they're applicable for the patch tag meanings in IGT, too): https://www.kernel.org/doc/html/v4.18/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by https://www.kernel.org/doc/html/v4.18/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes If we spend the time to actually review the patches, that should be documented with a proper Reviewed-by and not a vague Acked-by. KMS is really an area I do not know much about. While I can say the patches are looking good on the IGT side, I cannot guarantee they use the KMS interface appropriately therefore the 'Acked-by'. After reading the documentation you linked I think it fits rather well since the only feedback I gave was on a small oversight. Thanks, Antonio Regards, Joonas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/5] igt/kms_getfb: Check the iface exists before use
Quoting Antonio Argenziano (2018-10-01 22:53:46) > Fair enough. > > Acked-by: Antonio Argenziano > > for the series. Please, read the following chapters (they're applicable for the patch tag meanings in IGT, too): https://www.kernel.org/doc/html/v4.18/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by https://www.kernel.org/doc/html/v4.18/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes If we spend the time to actually review the patches, that should be documented with a proper Reviewed-by and not a vague Acked-by. Regards, Joonas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/5] igt/kms_getfb: Check the iface exists before use
On 01/10/18 12:43, Chris Wilson wrote:
Quoting Antonio Argenziano (2018-10-01 19:36:24)
On 28/09/18 03:19, Chris Wilson wrote:
If the driver doesn't support the getfb iface (e.g. because KMS has been
disabled), the ioctls will fail with ENOTSUP. This is expected, so skip
the test as nothing useful can be learnt.
Signed-off-by: Chris Wilson
---
tests/kms_getfb.c | 40 ++--
1 file changed, 38 insertions(+), 2 deletions(-)
diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
index 81d796a42..71d65488f 100644
--- a/tests/kms_getfb.c
+++ b/tests/kms_getfb.c
@@ -40,6 +40,40 @@
#include "drm.h"
#include "drm_fourcc.h"
+static bool has_getfb_iface(int fd)
+{
+ struct drm_mode_fb_cmd arg = { };
+ int err;
+
+ err = 0;
+ if (drmIoctl(fd, DRM_IOCTL_MODE_GETFB, ))
+ err = -errno;
+ switch (err) {
+ case -ENOTTY: /* ioctl unrecognised (kernel too old) */
+ case -ENOTSUP: /* driver doesn't support KMS */
+ return false;
+ default:
+ return true;
+ }
+}
+
+static bool has_addfb2_iface(int fd)
+{
+ struct drm_mode_fb_cmd2 arg = { };
+ int err;
+
+ err = 0;
+ if (drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, ) == 0)
+ err = -errno;
Shouldn't this^ be != 0?
Yup.
+ switch (err) {
+ case -ENOTTY: /* ioctl unrecognised (kernel too old) */
+ case -ENOTSUP: /* driver doesn't support KMS */
+ return false;
+ default:
+ return true;
Shouldn't we fail on every errno?
No. We want to be very careful in only fail at this point for errno we
know imply the interface is not supported so that we do not confuse an
illegal addfb2 request and fail later in the actual test.
Fair enough.
Acked-by: Antonio Argenziano
for the series.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/5] igt/kms_getfb: Check the iface exists before use
Quoting Antonio Argenziano (2018-10-01 19:36:24)
>
>
> On 28/09/18 03:19, Chris Wilson wrote:
> > If the driver doesn't support the getfb iface (e.g. because KMS has been
> > disabled), the ioctls will fail with ENOTSUP. This is expected, so skip
> > the test as nothing useful can be learnt.
> >
> > Signed-off-by: Chris Wilson
> > ---
> > tests/kms_getfb.c | 40 ++--
> > 1 file changed, 38 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
> > index 81d796a42..71d65488f 100644
> > --- a/tests/kms_getfb.c
> > +++ b/tests/kms_getfb.c
> > @@ -40,6 +40,40 @@
> > #include "drm.h"
> > #include "drm_fourcc.h"
> >
> > +static bool has_getfb_iface(int fd)
> > +{
> > + struct drm_mode_fb_cmd arg = { };
> > + int err;
> > +
> > + err = 0;
> > + if (drmIoctl(fd, DRM_IOCTL_MODE_GETFB, ))
> > + err = -errno;
> > + switch (err) {
> > + case -ENOTTY: /* ioctl unrecognised (kernel too old) */
> > + case -ENOTSUP: /* driver doesn't support KMS */
> > + return false;
> > + default:
> > + return true;
> > + }
> > +}
> > +
> > +static bool has_addfb2_iface(int fd)
> > +{
> > + struct drm_mode_fb_cmd2 arg = { };
> > + int err;
> > +
> > + err = 0;
> > + if (drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, ) == 0)
> > + err = -errno;
>
> Shouldn't this^ be != 0?
Yup.
> > + switch (err) {
> > + case -ENOTTY: /* ioctl unrecognised (kernel too old) */
> > + case -ENOTSUP: /* driver doesn't support KMS */
> > + return false;
> > + default:
> > + return true;
>
> Shouldn't we fail on every errno?
No. We want to be very careful in only fail at this point for errno we
know imply the interface is not supported so that we do not confuse an
illegal addfb2 request and fail later in the actual test.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/5] igt/kms_getfb: Check the iface exists before use
On 28/09/18 03:19, Chris Wilson wrote:
If the driver doesn't support the getfb iface (e.g. because KMS has been
disabled), the ioctls will fail with ENOTSUP. This is expected, so skip
the test as nothing useful can be learnt.
Signed-off-by: Chris Wilson
---
tests/kms_getfb.c | 40 ++--
1 file changed, 38 insertions(+), 2 deletions(-)
diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
index 81d796a42..71d65488f 100644
--- a/tests/kms_getfb.c
+++ b/tests/kms_getfb.c
@@ -40,6 +40,40 @@
#include "drm.h"
#include "drm_fourcc.h"
+static bool has_getfb_iface(int fd)
+{
+ struct drm_mode_fb_cmd arg = { };
+ int err;
+
+ err = 0;
+ if (drmIoctl(fd, DRM_IOCTL_MODE_GETFB, ))
+ err = -errno;
+ switch (err) {
+ case -ENOTTY: /* ioctl unrecognised (kernel too old) */
+ case -ENOTSUP: /* driver doesn't support KMS */
+ return false;
+ default:
+ return true;
+ }
+}
+
+static bool has_addfb2_iface(int fd)
+{
+ struct drm_mode_fb_cmd2 arg = { };
+ int err;
+
+ err = 0;
+ if (drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, ) == 0)
+ err = -errno;
Shouldn't this^ be != 0?
+ switch (err) {
+ case -ENOTTY: /* ioctl unrecognised (kernel too old) */
+ case -ENOTSUP: /* driver doesn't support KMS */
+ return false;
+ default:
+ return true;
Shouldn't we fail on every errno?
Thanks,
Antonio
+ }
+}
+
static void get_ccs_fb(int fd, struct drm_mode_fb_cmd2 *ret)
{
struct drm_mode_fb_cmd2 add = {
@@ -54,6 +88,7 @@ static void get_ccs_fb(int fd, struct drm_mode_fb_cmd2 *ret)
};
int size;
+ igt_require(has_addfb2_iface(fd));
igt_require_intel(fd);
/* An explanation of the magic numbers can be found in kms_ccs.c. */
@@ -191,15 +226,16 @@ static void test_duplicate_handles(int fd)
do_ioctl(fd, DRM_IOCTL_MODE_RMFB, _id);
gem_close(fd, add.handles[0]);
}
-
}
igt_main
{
int fd;
- igt_fixture
+ igt_fixture {
fd = drm_open_driver_master(DRIVER_ANY);
+ igt_require(has_getfb_iface(fd));
+ }
igt_subtest_group
test_handle_input(fd);
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
