Re: [Intel-gfx] [PATCH v3 04/18] drm/selftest: Add drm damage helper selftest

[Deepak Singh Rawat]

> 
> On Tue, Oct 16, 2018 at 02:21:17PM +0200, Daniel Vetter wrote:
> > On Mon, Oct 15, 2018 at 04:11:41PM +0000, Deepak Singh Rawat wrote:
> > > > On Wed, Oct 10, 2018 at 05:16:43PM -0700, Deepak Rawat wrote:
> > > > > Selftest for drm damage helper iterator functions.
> > > > >
> > > > > Cc: ville.syrj...@linux.intel.com
> > > > > Cc: Daniel Vetter 
> > > > > Cc: Pekka Paalanen 
> > > > > Cc: Daniel Stone 
> > > > > Cc: intel-gfx@lists.freedesktop.org
> > > > > Cc: igt-...@lists.freedesktop.org
> > > > > Cc: petri.latv...@intel.com
> > > > > Cc: ch...@chris-wilson.co.uk
> > > > > Signed-off-by: Deepak Rawat 
> > > > > ---
> > > > >  drivers/gpu/drm/selftests/Makefile|   3 +-
> > > > >  .../selftests/drm_damage_helper_selftests.h   |  22 +
> > > > >  .../drm/selftests/test-drm_damage_helper.c| 844
> > > > ++
> > > > >  3 files changed, 868 insertions(+), 1 deletion(-)
> > > > >  create mode 100644
> > > > drivers/gpu/drm/selftests/drm_damage_helper_selftests.h
> > > > >  create mode 100644 drivers/gpu/drm/selftests/test-
> > > > drm_damage_helper.c
> > > > >
> > > > > diff --git a/drivers/gpu/drm/selftests/Makefile
> > > > b/drivers/gpu/drm/selftests/Makefile
> > > > > index 9fc349fa18e9..88ac216f5962 100644
> > > > > --- a/drivers/gpu/drm/selftests/Makefile
> > > > > +++ b/drivers/gpu/drm/selftests/Makefile
> > > > > @@ -1 +1,2 @@
> > > > > -obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-
> drm-
> > > > helper.o
> > > > > +obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-
> drm-
> > > > helper.o \
> > > > > + test-drm_damage_helper.o
> > > >
> > > > With the testcase intagrated into the test-drm-helper.ko module, for
> > > > patches 1-4 in this series:
> > > >
> > > > Reviewed-by: Daniel Vetter 
> > > >
> > > > Obviously needs some adjusting on the igt side too, since we seem to
> be
> > > > missing the igt scaffolding for tests-drm-helper.ko.
> > > > -Daniel
> > >
> > > Hi Daniel,
> > >
> > > Thanks for the review. I am a little confused here. Should we have single
> > > kernel module for drm plane helper selftest and damage helper selftest?
> > > Also shall I rename the kernel selfttest to kms_*?
> > >
> > > For user-space igt test it should be it makes sense to rename to
> kms_selftets?
> >
> > Since I went back on this way too many times:
> > - igt should be called kms_selftest. Please work together with igt
> >   maintainers (Arek and Petri), since we also need to update the CI
> >   building infrastructure to make sure it updates the list of subtests
> >   implemented by the kernel.
> >
> > - Kernel module I'd call test-drm_modeset.ko. That kernel module can then
> >   include the existing test-drm-helper.c (could probably rename to
> >   test-drm_plane_helper.c for clarity) and your new damage helper (named
> >   test-drm_damage_helper.c for consistency).
> >
> > Does that make sense to everyone?
> 
> I was trying to add some selftests, as well here [1], with that in
> mind, I think it makes sense to have just one module, call it
> "test-drm_modeset" or whatever and separate the tests source code base
> on whatever core functionality they are testing.
> 
> Besides compiling everything together, probably some stuff will have
> to move out of test-drm-helper.c into some common header. For example
> this "FAIL/FAIL_ON" macros
> 

Hi,

Thanks for your input. I have similar change in mind after suggestion from
Daniel. Below is initial draft I did yesterday, will move common code to
a common header.

I hope this aligns with what you are doing.

---
 drivers/gpu/drm/selftests/Makefile|  4 ++-
 .../gpu/drm/selftests/drm_helper_selftests.c  | 27 +++
 .../gpu/drm/selftests/drm_helper_selftests.h  | 15 +--
 ...-helper.c => drm_plane_helper_selftests.c} | 16 ---
 .../selftests/drm_plane_helper_selftests.h|  9 +++
 5 files changed, 51 insertions(+), 20 deletions(-)
 create mode 100644 drivers/gpu/drm/selftests/drm_helper_selftests.c
 rename drivers/gpu/drm/selftests/{test-drm-helper.c => 
drm_plane_helper_selftests.c} (96%)
 create mode 100644 drivers/gpu/drm/selftest

Re: [Intel-gfx] [PATCH v3 04/18] drm/selftest: Add drm damage helper selftest

[Deepak Singh Rawat]

> > > Obviously needs some adjusting on the igt side too, since we seem to be
> > > missing the igt scaffolding for tests-drm-helper.ko.
> > > -Daniel
> >
> > Hi Daniel,
> >
> > Thanks for the review. I am a little confused here. Should we have single
> > kernel module for drm plane helper selftest and damage helper selftest?
> > Also shall I rename the kernel selfttest to kms_*?
> >
> > For user-space igt test it should be it makes sense to rename to
> kms_selftets?
> 
> Since I went back on this way too many times:
> - igt should be called kms_selftest. Please work together with igt
>   maintainers (Arek and Petri), since we also need to update the CI
>   building infrastructure to make sure it updates the list of subtests
>   implemented by the kernel.
> 
> - Kernel module I'd call test-drm_modeset.ko. That kernel module can then
>   include the existing test-drm-helper.c (could probably rename to
>   test-drm_plane_helper.c for clarity) and your new damage helper (named
>   test-drm_damage_helper.c for consistency).
> 
> Does that make sense to everyone?
> 

Yes it makes sense to me and in fact I had similar changes in mind. And, 
since existing plane selftest are not invoked from igt it's safe to rename
kernel module.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 04/18] drm/selftest: Add drm damage helper selftest

[Deepak Singh Rawat]

> On Wed, Oct 10, 2018 at 05:16:43PM -0700, Deepak Rawat wrote:
> > Selftest for drm damage helper iterator functions.
> >
> > Cc: ville.syrj...@linux.intel.com
> > Cc: Daniel Vetter 
> > Cc: Pekka Paalanen 
> > Cc: Daniel Stone 
> > Cc: intel-gfx@lists.freedesktop.org
> > Cc: igt-...@lists.freedesktop.org
> > Cc: petri.latv...@intel.com
> > Cc: ch...@chris-wilson.co.uk
> > Signed-off-by: Deepak Rawat 
> > ---
> >  drivers/gpu/drm/selftests/Makefile|   3 +-
> >  .../selftests/drm_damage_helper_selftests.h   |  22 +
> >  .../drm/selftests/test-drm_damage_helper.c| 844
> ++
> >  3 files changed, 868 insertions(+), 1 deletion(-)
> >  create mode 100644
> drivers/gpu/drm/selftests/drm_damage_helper_selftests.h
> >  create mode 100644 drivers/gpu/drm/selftests/test-
> drm_damage_helper.c
> >
> > diff --git a/drivers/gpu/drm/selftests/Makefile
> b/drivers/gpu/drm/selftests/Makefile
> > index 9fc349fa18e9..88ac216f5962 100644
> > --- a/drivers/gpu/drm/selftests/Makefile
> > +++ b/drivers/gpu/drm/selftests/Makefile
> > @@ -1 +1,2 @@
> > -obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm-
> helper.o
> > +obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm-
> helper.o \
> > +   test-drm_damage_helper.o
> 
> With the testcase intagrated into the test-drm-helper.ko module, for
> patches 1-4 in this series:
> 
> Reviewed-by: Daniel Vetter 
> 
> Obviously needs some adjusting on the igt side too, since we seem to be
> missing the igt scaffolding for tests-drm-helper.ko.
> -Daniel

Hi Daniel,

Thanks for the review. I am a little confused here. Should we have single
kernel module for drm plane helper selftest and damage helper selftest?
Also shall I rename the kernel selfttest to kms_*?

For user-space igt test it should be it makes sense to rename to kms_selftets?

> 
> > diff --git a/drivers/gpu/drm/selftests/drm_damage_helper_selftests.h
> b/drivers/gpu/drm/selftests/drm_damage_helper_selftests.h
> > new file mode 100644
> > index ..3a1cbe05bef0
> > --- /dev/null
> > +++ b/drivers/gpu/drm/selftests/drm_damage_helper_selftests.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +selftest(damage_iter_no_damage, igt_damage_iter_no_damage)
> > +selftest(damage_iter_no_damage_fractional_src,
> igt_damage_iter_no_damage_fractional_src)
> > +selftest(damage_iter_no_damage_src_moved,
> igt_damage_iter_no_damage_src_moved)
> > +selftest(damage_iter_no_damage_fractional_src_moved,
> igt_damage_iter_no_damage_fractional_src_moved)
> > +selftest(damage_iter_no_damage_not_visible,
> igt_damage_iter_no_damage_not_visible)
> > +selftest(damage_iter_no_damage_no_crtc,
> igt_damage_iter_no_damage_no_crtc)
> > +selftest(damage_iter_no_damage_no_fb,
> igt_damage_iter_no_damage_no_fb)
> > +selftest(damage_iter_simple_damage,
> igt_damage_iter_simple_damage)
> > +selftest(damage_iter_single_damage, igt_damage_iter_single_damage)
> > +selftest(damage_iter_single_damage_intersect_src,
> igt_damage_iter_single_damage_intersect_src)
> > +selftest(damage_iter_single_damage_outside_src,
> igt_damage_iter_single_damage_outside_src)
> > +selftest(damage_iter_single_damage_fractional_src,
> igt_damage_iter_single_damage_fractional_src)
> > +selftest(damage_iter_single_damage_intersect_fractional_src,
> igt_damage_iter_single_damage_intersect_fractional_src)
> > +selftest(damage_iter_single_damage_outside_fractional_src,
> igt_damage_iter_single_damage_outside_fractional_src)
> > +selftest(damage_iter_single_damage_src_moved,
> igt_damage_iter_single_damage_src_moved)
> > +selftest(damage_iter_single_damage_fractional_src_moved,
> igt_damage_iter_single_damage_fractional_src_moved)
> > +selftest(damage_iter_damage, igt_damage_iter_damage)
> > +selftest(damage_iter_damage_one_intersect,
> igt_damage_iter_damage_one_intersect)
> > +selftest(damage_iter_damage_one_outside,
> igt_damage_iter_damage_one_outside)
> > +selftest(damage_iter_damage_src_moved,
> igt_damage_iter_damage_src_moved)
> > +selftest(damage_iter_damage_not_visible,
> igt_damage_iter_damage_not_visible)
> > diff --git a/drivers/gpu/drm/selftests/test-drm_damage_helper.c
> b/drivers/gpu/drm/selftests/test-drm_damage_helper.c
> > new file mode 100644
> > index ..17754734c47a
> > --- /dev/null
> > +++ b/drivers/gpu/drm/selftests/test-drm_damage_helper.c
> > @@ -0,0 +1,844 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Test case for drm_damage_helper functions
> > + */
> > +
> > +#define pr_fmt(fmt) "drm_damage_helper: " fmt
> > +
> > +#include 
> > +#include 
> > +
> > +#define TESTS "drm_damage_helper_selftests.h"
> > +#include "drm_selftest.h"
> > +
> > +#define FAIL(test, msg, ...) \
> > +   do { \
> > +   if (test) { \
> > +   pr_err("%s/%u: " msg, __FUNCTION__, __LINE__,
> ##__VA_ARGS__); \
> > +   return -EINVAL; \
> > +   } \
> > +   } while (0)
> > +
> > +#define FAIL_ON(x) FAIL((x), "%s", 

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 3/6] lib/igt_fb: Check for cairo surface success

[Deepak Singh Rawat]

> 
> On Wed, Oct 10, 2018 at 05:21:01PM -0700, Deepak Rawat wrote:
> > For vmwgfx cairo surface creation fails due to stride mismatch, add a
> > igt_require_f() for surface.
> 
> Hmm. What kind of pixel format are you using?
> 
> It seems to me cairo should be happy with just a 4 byte aligned
> stride. How bad would it be to change vmw_dumb_create() to give
> you that? Having working cairo stuff should make igt_fb much more
> useful for you in general.

I think pixel format was SVGA3D_R5G6B5. For vmwgfx pitch is simply
width * bpp and for only some old length framebuffer, surface creation
was failing. I didn't pursued this in much detail.

Yes I plan to add vmwgfx private ioctl for buffer allocation to igt and
I think that would solve this.

> 
> Alternative would to not use dumb buffers I suppose, but then
> we'd need to make sure the igt_fb size calculations are OK for
> vmwgfx. Currently igt_fb assumes Intel hw in the sense that
> linear buffers will get a 64byte aligned stride.
> 
> >
> > v2: Check for surface creation failure.
> >
> > Signed-off-by: Deepak Rawat 
> > ---
> >  lib/igt_fb.c | 4 
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > index 335ece69..1bb6d324 100644
> > --- a/lib/igt_fb.c
> > +++ b/lib/igt_fb.c
> > @@ -1433,6 +1433,10 @@ static void create_cairo_surface__gtt(int fd,
> struct igt_fb *fb)
> > cairo_image_surface_create_for_data(ptr,
> > drm_format_to_cairo(fb-
> >drm_format),
> > fb->width, fb->height, fb-
> >strides[0]);
> > +   igt_require_f(cairo_surface_status(fb->cairo_surface) ==
> CAIRO_STATUS_SUCCESS,
> > + "Unable to create a cairo surface: %s\n",
> > + cairo_status_to_string(cairo_surface_status(fb-
> >cairo_surface)));
> > +
> > fb->domain = I915_GEM_DOMAIN_GTT;
> >
> > cairo_surface_set_user_data(fb->cairo_surface,
> > --
> > 2.17.1
> >
> > ___
> > igt-dev mailing list
> > igt-...@lists.freedesktop.org
> >
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.fr
> eedesktop.org%2Fmailman%2Flistinfo%2Figt-
> devdata=02%7C01%7Cdrawat%40vmware.com%7C55643c7eddb14e60
> aa3c08d62f8b500c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C63
> 6748672729440089sdata=pE7NX1LoOz5qhBZhp%2F2wArWHvKdSF0ctx
> TXyNW5%2Fcig%3Dreserved=0
> 
> --
> Ville Syrjälä
> Intel
___
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 4/6] lib: Don't call igt_require_fb_modifiers() when no modifier

[Deepak Singh Rawat]

> On Wed, Oct 10, 2018 at 05:21:02PM -0700, Deepak Rawat wrote:
> > vmwgfx doesn't support fb modifier so skip igt_require_fb_modifiers()
> > when modifier are not passed.
> >
> > Signed-off-by: Deepak Rawat 
> > ---
> >  lib/ioctl_wrappers.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> > index 0929c43f..3a11cb6e 100644
> > --- a/lib/ioctl_wrappers.c
> > +++ b/lib/ioctl_wrappers.c
> > @@ -1678,7 +1678,10 @@ int __kms_addfb(int fd, uint32_t handle,
> > struct drm_mode_fb_cmd2 f;
> > int ret, i;
> >
> > -   igt_require_fb_modifiers(fd);
> > +   if (modifier)
> > +   igt_require_fb_modifiers(fd);
> > +   else
> > +   flags &= ~DRM_MODE_FB_MODIFIERS;
> 
> This would theoretically change the behviour for i915 at least. Without
> the modifiers flag the kernel will pick the modifier for us based on
> the bo tiling, which in theory might not be what we wanted. But at least
> igt_fb should be fine with that.
> 
> Maybe it would be better to just not pass the flags from the caller at
> all, and instead have __kms_addfb() check if the driver has modifiers
> or not and set the flag based on that?
> 

Thanks Ville for the review. I thought of checking for modifiers support
in __kms_addfb() but I discarded the idea as it looked an overkill to me,
checking each time __kms_addfb() is called. May be static variable will
be a good idea? And yes resetting the flag from caller looks a better way.

Thanks,
Deepak
___
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/6] lib/igt_vmwgfx: Add vmwgfx device

[Deepak Singh Rawat]

> > > This seems not needed? For pure generic kms tests I think it'd be great if
> > > we don't have to sprinkle driver-specific checks all over. Which you seem
> > > to achive in your series here.
> > >
> > > So not clear why this here is needed?
> > > -Daniel
> >
> > Hi Daniel,
> >
> > Thanks for the review. You are right for kms tests having vmwgfx driver
> type
> > is not needed but I added vmwgfx device type because I plan to add more
> > vmwgfx test cases, and since vmwgfx buffer allocation is private ioctl
> > having a separate device type might be needed.
> 
> Oh, this sounds awesome. And yes, for private ioctl tests vmwgfx is
> obviuosly very much welcome!
> 
> > I can drop this until I have some vmwgfx specific test cases.
> 
> Sounds like a good plan to me.
> -Daniel

I saw latest email from Perti mentioning that this might be needed for loading
ko. I am really not sure about that but I will test without vmwgfx as a separate
driver type and see if things work.

Thanks,
Deepak


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

Re: [Intel-gfx] [PATCH i-g-t 6/6] tests/plane_damage: Integrate kernel selftest test-drm_damage_helper

[Deepak Singh Rawat]

> > diff --git a/tests/meson.build b/tests/meson.build
> > index 697ff515..5acd7aa2 100644
> > --- a/tests/meson.build
> > +++ b/tests/meson.build
> > @@ -9,6 +9,7 @@ test_progs = [
> > 'debugfs_test',
> > 'drm_import_export',
> > 'drm_mm',
> > +   'drm_plane_damage',
> 
> For future proofing I think it'd be much better if we call this drm_kms or
> similar. The individual subtest results will be all exposed, but there's a
> bit a problem when we always have to upgrade both igt and the kernel at
> the same time. At least with the current CI infrastructure.

Do you mean drm_kms_plane_damage? IIUC this is to allow running test
from run-test.sh? I suppose in that case it should be named
kms_plane_damage, because all other kms test starts with kms_*.

> 
> I'm also asking the ARM folks to type selftests for the new block_* format
> description stuff, so this will come in handy real soon.
> -Daniel
> 
> > 'drm_read',
> > 'drv_getparams_basic',
> > 'drv_hangman',
> > --
> > 2.17.1
___
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/6] lib/igt_vmwgfx: Add vmwgfx device

[Deepak Singh Rawat]

> 
> On Wed, Oct 10, 2018 at 05:20:59PM -0700, Deepak Rawat wrote:
> > Add DRIVER_VMWGFX to represent vmwgfx device for running igt tests.
> >
> > v2: Don't remove second virtio_gpu
> >
> > Signed-off-by: Deepak Rawat 
> > ---
> >  lib/drmtest.c | 8 
> >  lib/drmtest.h | 3 +++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/lib/drmtest.c b/lib/drmtest.c
> > index fee9d33a..9d013a00 100644
> > --- a/lib/drmtest.c
> > +++ b/lib/drmtest.c
> > @@ -105,6 +105,11 @@ bool is_i915_device(int fd)
> > return __is_device(fd, "i915");
> >  }
> >
> > +bool is_vmwgfx_device(int fd)
> > +{
> > +   return __is_device(fd, "vmwg");
> > +}
> > +
> >  static bool has_known_intel_chipset(int fd)
> >  {
> > struct drm_i915_getparam gp;
> > @@ -206,6 +211,7 @@ static const struct module {
> > { DRIVER_VGEM, "vgem" },
> > { DRIVER_VIRTIO, "virtio-gpu" },
> > { DRIVER_VIRTIO, "virtio_gpu" },
> > +   { DRIVER_VMWGFX, "vmwgfx" },
> > {}
> >  };
> >
> > @@ -348,6 +354,8 @@ static const char *chipset_to_str(int chipset)
> > return "virtio";
> > case DRIVER_AMDGPU:
> > return "amdgpu";
> > +   case DRIVER_VMWGFX:
> > +   return "vmwgfx";
> > case DRIVER_ANY:
> > return "any";
> > default:
> > diff --git a/lib/drmtest.h b/lib/drmtest.h
> > index 949865ee..0213fb51 100644
> > --- a/lib/drmtest.h
> > +++ b/lib/drmtest.h
> > @@ -43,6 +43,7 @@
> >  #define DRIVER_VGEM(1 << 2)
> >  #define DRIVER_VIRTIO  (1 << 3)
> >  #define DRIVER_AMDGPU  (1 << 4)
> > +#define DRIVER_VMWGFX  (1 << 5)
> 
> This seems not needed? For pure generic kms tests I think it'd be great if
> we don't have to sprinkle driver-specific checks all over. Which you seem
> to achive in your series here.
> 
> So not clear why this here is needed?
> -Daniel

Hi Daniel,

Thanks for the review. You are right for kms tests having vmwgfx driver type
is not needed but I added vmwgfx device type because I plan to add more
vmwgfx test cases, and since vmwgfx buffer allocation is private ioctl
having a separate device type might be needed.

I can drop this until I have some vmwgfx specific test cases.

> 
> >  /*
> >   * Exclude DRVER_VGEM from DRIVER_ANY since if you run on a system
> >   * with vgem as well as a supported driver, you can end up with a
> > @@ -80,6 +81,8 @@ void igt_require_intel(int fd);
> >
> >  bool is_i915_device(int fd);
> >
> > +bool is_vmwgfx_device(int fd);
> > +
> >  /**
> >   * do_or_die:
> >   * @x: command
> > --
> > 2.17.1
> >
> > ___
> > igt-dev mailing list
> > igt-...@lists.freedesktop.org
> >
> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.fr
> eedesktop.org%2Fmailman%2Flistinfo%2Figt-
> devdata=02%7C01%7Cdrawat%40vmware.com%7C5486d67be73647b1
> 1d2708d62f58307d%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C6
> 36748453147406904sdata=QcT4VTQth8GuXgjZejCsnFPDsMFbtqrUOW
> uSDr8zbG8%3Dreserved=0
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ff
> wll.chdata=02%7C01%7Cdrawat%40vmware.com%7C5486d67be73647
> b11d2708d62f58307d%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7
> C636748453147406904sdata=4SKi8P5PeyCAUsoZh%2BsYFC%2FU2RiEx5
> qp2gPP7bPr3Bo%3Dreserved=0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 4/5] lib/igt_fb: Check for stride before creating cairo surface

[Deepak Singh Rawat]

> Quoting Deepak Rawat (2018-09-06 01:03:49)
> > Cairo surface creation will fail if stride of provided buffer is not
> > same as expected by cairo. This fails for vmwgfx odd length framebuffer
> > as in vmwgfx stride is always width * bpp.
> >
> > Signed-off-by: Deepak Rawat 
> > ---
> >  lib/igt_fb.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > index ba995a1a..2724e323 100644
> > --- a/lib/igt_fb.c
> > +++ b/lib/igt_fb.c
> > @@ -1349,6 +1349,9 @@ static void create_cairo_surface__gtt(int fd,
> struct igt_fb *fb)
> > ptr = gem_mmap__gtt(fd, fb->gem_handle, fb->size,
> > PROT_READ | PROT_WRITE);
> >
> > +   igt_require(fb->stride == cairo_format_stride_for_width(
> > +   drm_format_to_cairo(fb->drm_format), fb->width));
> > +
> > fb->cairo_surface =
> > cairo_image_surface_create_for_data(ptr,
> > 
> > drm_format_to_cairo(fb->drm_format),
> 
> Is there not a
> igt_require_f/igt_assert_f(cairo_surface_status(fb->cairo_surface) ==
> CAIRO_STATUS_SUCCESS,
> "Unable to create a cairo surface: %s",
> cairo_status_to_string(cairo_surface_status(fb->cairo_surface)));
> here?
> -Chris

Thanks Chris for the review.

No there wasn't a check like that but I guess I can add that instead of
checking for stride. 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/7] drm/vmwgfx: Stop updating plane->fb

[Deepak Singh Rawat]

> 
> On Fri, Apr 06, 2018 at 10:35:00PM +0300, Ville Syrjälä wrote:
> > On Fri, Apr 06, 2018 at 07:14:51PM +0000, Deepak Singh Rawat wrote:
> > > This makes sense once we got rid of plane->fb
> > >
> > > Will this go to drm-next?
> >
> > The plan is to push to drm-misc-next once we get all
> > the ducks in a row.
> >
> > > Could you please CC
> > > me so that I can do some testing myself. Thanks.
> >
> > Here's a branch if you want a head start:
> > git://github.com/vsyrjala/linux.git plane_fb_crtc_nuke_2
> >
> > I'd definitely appreciate some testing of this stuff. Wouldn't
> > want to break you stuff accidentally.
> 
> Did we get anywhere with testing this? I'd like to land the remaining
> bits, but I'd feel much safer doing that if it was tested.

Hi Ville,

I did some basic mode-setting testing by taking your patches to
vmwgfx private branch and things seems to work fine.

Thanks,
Deepak

> 
> >
> > >
> > > Reviewed-by: Deepak Rawat <dra...@vmware.com>
> > >
> > >
> > > >
> > > > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > > >
> > > > We want to get rid of plane->fb on atomic drivers. Stop setting it.
> > > >
> > > > Cc: Thomas Hellstrom <thellst...@vmware.com>
> > > > Cc: Sinclair Yeh <s...@vmware.com>
> > > > Cc: VMware Graphics <linux-graphics-maintai...@vmware.com>
> > > > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> > > > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 2 --
> > > >  drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 2 --
> > > >  2 files changed, 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> > > > b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> > > > index 648f8127f65a..bbd3f19b1a0b 100644
> > > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> > > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> > > > @@ -525,8 +525,6 @@
> vmw_sou_primary_plane_atomic_update(struct
> > > > drm_plane *plane,
> > > >  */
> > > > if (ret != 0)
> > > > DRM_ERROR("Failed to update screen.\n");
> > > > -
> > > > -   crtc->primary->fb = plane->state->fb;
> > > > } else {
> > > > /*
> > > >  * When disabling a plane, CRTC and FB should always be
> > > > NULL
> > > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> > > > b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> > > > index 67331f01ef32..90445bc590cb 100644
> > > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> > > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> > > > @@ -1285,8 +1285,6 @@
> vmw_stdu_primary_plane_atomic_update(struct
> > > > drm_plane *plane,
> > > >  1, 1, NULL, 
> > > > crtc);
> > > > if (ret)
> > > > DRM_ERROR("Failed to update STDU.\n");
> > > > -
> > > > -   crtc->primary->fb = plane->state->fb;
> > > > } else {
> > > > crtc = old_state->crtc;
> > > > stdu = vmw_crtc_to_stdu(crtc);
> > > > --
> > > > 2.16.1
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lists.freedesktop.org_mailman_listinfo_intel-
> 2Dgfx=DwIDAw=uilaK90D4TOVoH58JNXRgQ=zOOG28inJK0762SxAf-
> cyZdStnd2NQpRu98lJP2iYGw=3J7W8_yE3JhMDcN3FfZN8bWZON61wueSY
> YfSGxPNHVE=TqYFqV1NCzCnakZHMWVyJ9k42n0CUm5Kcl9xW2Cdvz4=
> 
> --
> Ville Syrjälä
> Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/7] drm/vmwgfx: Stop updating plane->fb

[Deepak Singh Rawat]

This makes sense once we got rid of plane->fb

Will this go to drm-next? Could you please CC
me so that I can do some testing myself. Thanks.

Reviewed-by: Deepak Rawat 


> 
> From: Ville Syrjälä 
> 
> We want to get rid of plane->fb on atomic drivers. Stop setting it.
> 
> Cc: Thomas Hellstrom 
> Cc: Sinclair Yeh 
> Cc: VMware Graphics 
> Cc: Daniel Vetter 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 2 --
>  drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 2 --
>  2 files changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> index 648f8127f65a..bbd3f19b1a0b 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
> @@ -525,8 +525,6 @@ vmw_sou_primary_plane_atomic_update(struct
> drm_plane *plane,
>*/
>   if (ret != 0)
>   DRM_ERROR("Failed to update screen.\n");
> -
> - crtc->primary->fb = plane->state->fb;
>   } else {
>   /*
>* When disabling a plane, CRTC and FB should always be
> NULL
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index 67331f01ef32..90445bc590cb 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -1285,8 +1285,6 @@ vmw_stdu_primary_plane_atomic_update(struct
> drm_plane *plane,
>1, 1, NULL, crtc);
>   if (ret)
>   DRM_ERROR("Failed to update STDU.\n");
> -
> - crtc->primary->fb = plane->state->fb;
>   } else {
>   crtc = old_state->crtc;
>   stdu = vmw_crtc_to_stdu(crtc);
> --
> 2.16.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/7] drm/vmwgfx: Stop using plane->fb in atomic_enable()

[Deepak Singh Rawat]

> 
> From: Ville Syrjälä 
> 
> Instead of looking at the (soon to be deprecated) plane->fb we'll
> examing plane->state->fb instead. We can do this because
> vmw_du_crtc_atomic_check() prevents us from enabling a crtc
> without the primary plane also being enabled.
> 
> Due to that same reason, I'm actually not sure what the checks here are
> for NULL fb. If we can't enable the crtc without an enabled plane
> we should always have an fb. But I'll leave that for someone else
> to figure out.

Hi Ville,

AFAIK the NULL check is set or clear the implicit framebuffer property
which is specific to vmwgfx and for current hardware version is disabled.
I have this future TODO work item to get rid of implicit placement property
or at least make it read only.

I still don’t have complete understanding of atomic state but this patch looks
good to me.

Reviewed-by: Deepak Rawat 

> 
> Cc: Thomas Hellstrom 
> Cc: Sinclair Yeh 
> Cc: VMware Graphics 
> Cc: Daniel Vetter 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> index 90445bc590cb..152e96cb1c01 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
> @@ -414,6 +414,7 @@ static void vmw_stdu_crtc_helper_prepare(struct
> drm_crtc *crtc)
>  static void vmw_stdu_crtc_atomic_enable(struct drm_crtc *crtc,
>   struct drm_crtc_state *old_state)
>  {
> + struct drm_plane_state *plane_state = crtc->primary->state;
>   struct vmw_private *dev_priv;
>   struct vmw_screen_target_display_unit *stdu;
>   struct vmw_framebuffer *vfb;
> @@ -422,7 +423,7 @@ static void vmw_stdu_crtc_atomic_enable(struct
> drm_crtc *crtc,
> 
>   stdu = vmw_crtc_to_stdu(crtc);
>   dev_priv = vmw_priv(crtc->dev);
> - fb   = crtc->primary->fb;
> + fb   = plane_state->fb;
> 
>   vfb = (fb) ? vmw_framebuffer_to_vfb(fb) : NULL;
> 
> --
> 2.16.1
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/7] drm/vmwgfx: Stop using plane->fb in vmw_kms_update_implicit_fb()

[Deepak Singh Rawat]

Reviewed-by: Deepak Rawat 

> 
> From: Ville Syrjälä 
> 
> The only caller of vmw_kms_update_implicit_fb() is the page_flip
> hook which itself gets called with the plane mutex already held.
> Hence we can look at plane->state safely. Toss in a lockdep assert
> to make the situation more clear.
> 
> Cc: Thomas Hellstrom 
> Cc: Sinclair Yeh 
> Cc: VMware Graphics 
> Cc: Daniel Vetter 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 5a824125c231..a93d290b0f35 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -2811,14 +2811,17 @@ void vmw_kms_update_implicit_fb(struct
> vmw_private *dev_priv,
>   struct drm_crtc *crtc)
>  {
>   struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
> + struct drm_plane *plane = crtc->primary;
>   struct vmw_framebuffer *vfb;
> 
> + lockdep_assert_held(>mutex);
> +
>   mutex_lock(_priv->global_kms_state_mutex);
> 
>   if (!du->is_implicit)
>   goto out_unlock;
> 
> - vfb = vmw_framebuffer_to_vfb(crtc->primary->fb);
> + vfb = vmw_framebuffer_to_vfb(plane->state->fb);
>   WARN_ON_ONCE(dev_priv->num_implicit != 1 &&
>dev_priv->implicit_fb != vfb);
> 
> --
> 2.16.1
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/7] drm/vmwgfx: Stop using plane->fb in vmw_kms_atomic_check_modeset()

[Deepak Singh Rawat]

> 
> On Thu, Apr 05, 2018 at 08:15:05PM +, Deepak Singh Rawat wrote:
> >
> > >
> > > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > >
> > > Instead of looking at plane->fb let's look at the proper new
> > > plane state.
> > >
> > > Not that the code makes a ton of sense. It's only going through the
> > > crtcs in the atomic state, so assuming not all of them are included
> > > we're not even calculating the total bandwidth here. Also we're
> > > not considering whether each crtc is actually enabled or not.
> > >
> > > Cc: Thomas Hellstrom <thellst...@vmware.com>
> > > Cc: Sinclair Yeh <s...@vmware.com>
> > > Cc: VMware Graphics <linux-graphics-maintai...@vmware.com>
> > > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> > > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 10 +++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > index 6728c6247b4b..a2a796b4cc23 100644
> > > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> > > @@ -1536,9 +1536,13 @@ vmw_kms_atomic_check_modeset(struct
> > > drm_device *dev,
> > >   unsigned long requested_bb_mem = 0;
> > >
> > >   if (dev_priv->active_display_unit ==
> > > vmw_du_screen_target) {
> > > - if (crtc->primary->fb) {
> > > - int cpp = crtc->primary->fb->pitches[0] /
> > > -   crtc->primary->fb->width;
> > > + struct drm_plane *plane = crtc->primary;
> > > + struct drm_plane_state *plane_state;
> > > +
> > > + plane_state =
> > > drm_atomic_get_new_plane_state(state, plane);
> > > +
> > > + if (plane_state && plane_state->fb) {
> > > + int cpp = plane_state->fb->format->cpp[0];
> >
> > Hi Ville,
> >
> > Thanks for the patch, recently I have done some refactoring of this code
> area
> > which is no yet sent to dri-devel. But the refactored code eliminated the
> need
> > to look the fb
> >
> > https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__cgit.freedesktop.org_mesa_vmwgfx_commit_-3Fid-
> 3Dc54cdb6549b7d1c04ff61e639fc0e6de0dcc1ed6=DwIDAw=uilaK90D4T
> OVoH58JNXRgQ=zOOG28inJK0762SxAf-
> cyZdStnd2NQpRu98lJP2iYGw=lrHQqnjNpBdFNzU0f4b3rLTtaYp0VRzCgZztJ
> ew0kz0=mz1Kt2NO_HIpgVtQ9xHvKRQLGXx2HSqY8xt0oiEpGWg=
> 
> Hmm. What's the timelike for landing that stuff?

I am not sure, I still think there is more work here to clean this up. I guess 
for now
we can have your patch to take care of avoiding plane->fb.

> 
> A cursory glance tells me we should just change the current code with
> s/cpp/4/ and it should still be fine?

Yes replacing cpp with 4 is fine as that is what virtual hardware always look 
for.

> 
> BTW the drm_for_each_crtc(crtc, dev) loop in there doesn't look entirely
> kosher. It's potentially going to access crtc->state w/o holding the lock.

Thanks for the suggestion, I will look into this.

> 
> >
> > There is still some future work to be done in this area.
> >
> > >
> > >   requested_bb_mem += crtc->mode.hdisplay
> > > * cpp *
> > >   crtc->mode.vdisplay;
> > > --
> > > 2.16.1
> 
> --
> Ville Syrjälä
> Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/7] drm/vmwgfx: Stop using plane->fb in vmw_kms_atomic_check_modeset()

[Deepak Singh Rawat]

> 
> From: Ville Syrjälä 
> 
> Instead of looking at plane->fb let's look at the proper new
> plane state.
> 
> Not that the code makes a ton of sense. It's only going through the
> crtcs in the atomic state, so assuming not all of them are included
> we're not even calculating the total bandwidth here. Also we're
> not considering whether each crtc is actually enabled or not.
> 
> Cc: Thomas Hellstrom 
> Cc: Sinclair Yeh 
> Cc: VMware Graphics 
> Cc: Daniel Vetter 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 10 +++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 6728c6247b4b..a2a796b4cc23 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -1536,9 +1536,13 @@ vmw_kms_atomic_check_modeset(struct
> drm_device *dev,
>   unsigned long requested_bb_mem = 0;
> 
>   if (dev_priv->active_display_unit ==
> vmw_du_screen_target) {
> - if (crtc->primary->fb) {
> - int cpp = crtc->primary->fb->pitches[0] /
> -   crtc->primary->fb->width;
> + struct drm_plane *plane = crtc->primary;
> + struct drm_plane_state *plane_state;
> +
> + plane_state =
> drm_atomic_get_new_plane_state(state, plane);
> +
> + if (plane_state && plane_state->fb) {
> + int cpp = plane_state->fb->format->cpp[0];

Hi Ville,

Thanks for the patch, recently I have done some refactoring of this code area
which is no yet sent to dri-devel. But the refactored code eliminated the need
to look the fb

https://cgit.freedesktop.org/mesa/vmwgfx/commit/?id=c54cdb6549b7d1c04ff61e639fc0e6de0dcc1ed6

There is still some future work to be done in this area. 

> 
>   requested_bb_mem += crtc->mode.hdisplay
> * cpp *
>   crtc->mode.vdisplay;
> --
> 2.16.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx