[igt-dev] [PATCH v7 i-g-t 0/4] Add support for testing writeback connectors

[igt-dev] [PATCH v7 i-g-t 0/4] Add support for testing writeback connectors

[Rodrigo Siqueira]

[-- Attachment #1.1: Type: text/plain, Size: 2136 bytes --]

Hi,

A couple of months ago, I updated and re-submitted a patchset made by
Brian Starkey and Liviu Dudau for adding a writeback connectors test to
IGT. It is important to highlight that DRM already have writeback
connectors support, which is a way to expose in DRM the hardware
functionality from display engines that allows writing back into memory
the result of the DE's composition of supported planes.

After I resubmitted the patchset, Simon Ser provides a long and detailed
review for all of the patches (thanks Simon). As a result, I finally had
time to go through all the details and prepare this new version. Follows
some notes:

1. Patchset author

Brian Starkey is the original author of this patchset, and I'm just
trying to upstream his changes. Note that during this patch submission,
the mail server from google going to overwrite Brian's mail by mine;
this happens on the mail server side for avoiding malicious users to
send emails as someone else. Note that I could spend time figuring out
how to fix it, but I think this is not worth since I can fix it during
the merge process (if it got accepted).

2. Drop the clone commits from the series

After Simon's review, we decided to drop the last two patches of the
original series since it was related to cloning output, and VKMS does
not support it yet. However, after we finish with this series, I can try
to take a look at this feature or maybe propose it as a GSoC/Outreachy
project.

3. Changes

Most of the changes happened in the second patch.

Thanks

Brian Starkey (4):
  lib/igt_kms: Add writeback support
  kms_writeback: Add initial writeback tests
  lib: Add function to hash a framebuffer
  kms_writeback: Add writeback-check-output

 lib/igt_fb.c           |  68 +++++++
 lib/igt_fb.h           |   2 +
 lib/igt_kms.c          |  59 ++++++
 lib/igt_kms.h          |   6 +
 tests/Makefile.sources |   1 +
 tests/kms_writeback.c  | 413 +++++++++++++++++++++++++++++++++++++++++
 tests/meson.build      |   1 +
 7 files changed, 550 insertions(+)
 create mode 100644 tests/kms_writeback.c

-- 
2.23.0

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v7 i-g-t 0/4] Add support for testing writeback connectors

[Arkadiusz Hiler]

On Mon, Oct 21, 2019 at 09:54:48PM -0300, Rodrigo Siqueira wrote:
> Hi,
> 
> A couple of months ago, I updated and re-submitted a patchset made by
> Brian Starkey and Liviu Dudau for adding a writeback connectors test to
> IGT. It is important to highlight that DRM already have writeback
> connectors support, which is a way to expose in DRM the hardware
> functionality from display engines that allows writing back into memory
> the result of the DE's composition of supported planes.
> 
> After I resubmitted the patchset, Simon Ser provides a long and detailed
> review for all of the patches (thanks Simon). As a result, I finally had
> time to go through all the details and prepare this new version. Follows
> some notes:
> 
> 1. Patchset author
> 
> Brian Starkey is the original author of this patchset, and I'm just
> trying to upstream his changes. Note that during this patch submission,
> the mail server from google going to overwrite Brian's mail by mine;
> this happens on the mail server side for avoiding malicious users to
> send emails as someone else. Note that I could spend time figuring out
> how to fix it, but I think this is not worth since I can fix it during
> the merge process (if it got accepted).

Not sure about your workflow, but properly configured `git send-email`
handles all of that for you.

My .gitconfig is quite basic:

[user]
        name = Arkadiusz Hiler
        email = arkadiusz.hiler@intel.com

[sendemail]
        smtpUser = arkadiusz.hiler@intel.com
        smtpServer = smtpserver.address.example.com

Then when you `git send-email` those patches the tool is clever enough
to recognize that you are not the author.

To have the correct attribution it automatically adds has another
"From:" header-like entry in the email body - this one specifies the
original author - and then sends the patch as you. This is something
that `git am` understands when applying patches.

See example here:
  https://lists.freedesktop.org/archives/igt-dev/2019-October/016966.html

All I had to do was just:
  $ git send-email -4 --to=igt-dev@lists.freedesktop.org


Another issue with how the patches are sent is that they are not chained
correctly. None of them had In-Reply-To header which means they are seen
as a separate patch series by patchwork and people's tooling:

0/4:  https://patchwork.freedesktop.org/series/68352/
1/4:  https://patchwork.freedesktop.org/series/68353/
2/4:  https://patchwork.freedesktop.org/series/68354/
3/4:  https://patchwork.freedesktop.org/series/68355/
4/4:  https://patchwork.freedesktop.org/series/68356/

This means it does not get any automatic testing (whether it is GitLab
Pipeline for static checks or Intel's HW CI) and it's much harder for
people to deal with reviewing and applying them.

Patches 1 through 4 should have been sent with the following header:
  In-Reply-To: <message-id-of-the-cover-letter>

This is also something that is easily fixable with a proper use of
`git send-email`:
  $ git format-patch -4 --cover-letter
  $ $EDITOR 0000-*.patch
  $ git send-email *.patch

-- 
Cheers,
Arek
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v7 i-g-t 0/4] Add support for testing writeback connectors

[Liviu Dudau]

On Mon, Oct 21, 2019 at 09:54:48PM -0300, Rodrigo Siqueira wrote:
> Hi,

Hi Rodrigo,

The whole series looks good to me, you can add my Reviewed-by tag if you want.

Do you plan to push the patches into igt?

Best regards,
Liviu

> 
> A couple of months ago, I updated and re-submitted a patchset made by
> Brian Starkey and Liviu Dudau for adding a writeback connectors test to
> IGT. It is important to highlight that DRM already have writeback
> connectors support, which is a way to expose in DRM the hardware
> functionality from display engines that allows writing back into memory
> the result of the DE's composition of supported planes.
> 
> After I resubmitted the patchset, Simon Ser provides a long and detailed
> review for all of the patches (thanks Simon). As a result, I finally had
> time to go through all the details and prepare this new version. Follows
> some notes:
> 
> 1. Patchset author
> 
> Brian Starkey is the original author of this patchset, and I'm just
> trying to upstream his changes. Note that during this patch submission,
> the mail server from google going to overwrite Brian's mail by mine;
> this happens on the mail server side for avoiding malicious users to
> send emails as someone else. Note that I could spend time figuring out
> how to fix it, but I think this is not worth since I can fix it during
> the merge process (if it got accepted).
> 
> 2. Drop the clone commits from the series
> 
> After Simon's review, we decided to drop the last two patches of the
> original series since it was related to cloning output, and VKMS does
> not support it yet. However, after we finish with this series, I can try
> to take a look at this feature or maybe propose it as a GSoC/Outreachy
> project.
> 
> 3. Changes
> 
> Most of the changes happened in the second patch.
> 
> Thanks
> 
> Brian Starkey (4):
>   lib/igt_kms: Add writeback support
>   kms_writeback: Add initial writeback tests
>   lib: Add function to hash a framebuffer
>   kms_writeback: Add writeback-check-output
> 
>  lib/igt_fb.c           |  68 +++++++
>  lib/igt_fb.h           |   2 +
>  lib/igt_kms.c          |  59 ++++++
>  lib/igt_kms.h          |   6 +
>  tests/Makefile.sources |   1 +
>  tests/kms_writeback.c  | 413 +++++++++++++++++++++++++++++++++++++++++
>  tests/meson.build      |   1 +
>  7 files changed, 550 insertions(+)
>  create mode 100644 tests/kms_writeback.c
> 
> -- 
> 2.23.0



-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v7 i-g-t 0/4] Add support for testing writeback connectors

[Arkadiusz Hiler]

On Mon, Nov 04, 2019 at 03:47:51PM +0000, Liviu Dudau wrote:
> On Mon, Oct 21, 2019 at 09:54:48PM -0300, Rodrigo Siqueira wrote:
> > Hi,
> 
> Hi Rodrigo,
> 
> The whole series looks good to me, you can add my Reviewed-by tag if you want.
> 
> Do you plan to push the patches into igt?

Thanks for the review. I have resend the series so it's picked up
correctly by the testing infrascructure.
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev