Hi Laurent,
On 17/06/2019 17:12, Laurent Pinchart wrote:
> The KMSTest.atomic_crtc_disable() method deactivate a CRTC but doesn't
s/deactivate/deactivates/
> fully disables it, which requires setting the MODE_ID to 0. Furthermore
s/disables/disable/
> it doesn't de-associate the CRTC from connectors and planes, which cause
s/cause/causes/
> atomic check failures as a fully disabled CRTC can't be associated with
> connectors. It can also lead to the next test failing due to resources
> still being allocated to the CRTC.
>
> To fix this, introduce an AtomicRequest class that wraps around
> pykms.AtomicReq, and stores a copy of all the properties. When the
> request is committed the properties are added to a global state, which
> is then used to locate and release connectors and planes associated with
> the CRTC in KMSTest.atomic_crtc_disable().
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Other than minor spelling and pep8 style comments,
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> ---
> tests/kms-test-crc.py | 2 +-
> tests/kmstest.py | 65 +++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 60 insertions(+), 7 deletions(-)
>
> diff --git a/tests/kms-test-crc.py b/tests/kms-test-crc.py
> index 30d8bea796dc..29147e5bd0a3 100755
> --- a/tests/kms-test-crc.py
> +++ b/tests/kms-test-crc.py
> @@ -56,7 +56,7 @@ class CRCTest(kmstest.KMSTest):
> self.fail("atomic mode set failed with %d" % ret)
> continue
>
> - req = pykms.AtomicReq(self.card)
> + req = kmstest.AtomicRequest(self.card)
>
> offset = 100
> for plane in planes:
> diff --git a/tests/kmstest.py b/tests/kmstest.py
> index 12454df12f2d..adb67c82c368 100755
> --- a/tests/kmstest.py
> +++ b/tests/kmstest.py
> @@ -1,5 +1,6 @@
> #!/usr/bin/python3
>
> +import collections.abc
> import errno
> import fcntl
> import os
> @@ -208,6 +209,40 @@ class Rect(object):
> self.height = height
>
>
> +class AtomicRequest(pykms.AtomicReq):
This is quite terse for reading.
Is it worth adding some documentation to this class?
But it seems there isn't much documentation in the code at all ... so
perhaps that's a project task rather than a 'patch' task.
> + def __init__(self, test):
> + super().__init__(test.card)
> + self.__test = test
> + self.__props = {}
> +
> + def add(self, obj, *kwargs):
> + if not obj.id in self.__props:
This should be 'if obj.id not in self.__props:'
> + self.__props[obj.id] = {}
> + props = self.__props[obj.id]
> +
> + if len(kwargs) == 1 and isinstance(kwargs[0], collections.abc.Mapping):
> + props.update(kwargs[0])
> + elif len(kwargs) == 2:
> + props[kwargs[0]] = kwargs[1]
> +
> + super().add(obj, *kwargs)
> +
> + def commit(self, data=0, allow_modeset=False):
> + ret = super().commit(data, allow_modeset)
> + if ret == 0:
> + self.__test._props.update(self.__props)
> + return ret
> +
> + def commit_sync(self, allow_modeset=False):
> + ret = super().commit_sync(allow_modeset)
> + if ret == 0:
> + self.__test._props.update(self.__props)
> + return ret
> +
> + def __repr__(self):
> + return repr(self.__props)
> +
> +
> class KMSTest(object):
> def __init__(self, use_default_key_handler=False):
> if not getattr(self, 'main', None):
> @@ -217,6 +252,8 @@ class KMSTest(object):
> if not self.card.has_atomic:
> raise RuntimeError("Device doesn't support the atomic API")
>
> + self._props = {}
> +
> logname = self.__class__.__name__
> self.logger = Logger(logname)
>
> @@ -233,8 +270,24 @@ class KMSTest(object):
> return {k: v & ((1 << 64) - 1) for k, v in props.items()}
>
> def atomic_crtc_disable(self, crtc, sync=True):
> - req = pykms.AtomicReq(self.card)
> - req.add(crtc, 'ACTIVE', False)
> + req = AtomicRequest(self)
> + req.add(crtc, { 'ACTIVE': 0, 'MODE_ID': 0 })
PEP8 doesn't like the whitespace after { and before }
./tests/kmstest.py:274:24: E201 whitespace after '{'
./tests/kmstest.py:274:50: E202 whitespace before '}'
> + for connector in self.card.connectors:
> + if connector.id in self._props:
> + props = self._props[connector.id]
> + try:
> + if props['CRTC_ID'] == crtc.id:
> + req.add(connector, 'CRTC_ID', 0)
> + except:
And it doesn't like a 'bare except'.
"./tests/kmstest.py:281:17: E722 do not use bare 'except'"
What exceptions do you expect here? As we are simply 'passing' I guess
this one isn't too relevant
> + pass
> + for plane in self.card.planes:
> + if plane.id in self._props:
> + props = self._props[plane.id]
> + try:
> + if props['CRTC_ID'] == crtc.id:
> + req.add(plane, {'CRTC_ID': 0, 'FB_ID': 0})
> + except:
> + pass
Same here, but equally it's also a pass, so this might be OK.
> if sync:
> return req.commit_sync(True)
> else:
> @@ -249,7 +302,7 @@ class KMSTest(object):
> # the commit completes.
> mode_blob = mode.to_blob(self.card)
>
> - req = pykms.AtomicReq(self.card)
> + req = AtomicRequest(self)
> req.add(connector, 'CRTC_ID', crtc.id)
> req.add(crtc, { 'ACTIVE': 1, 'MODE_ID': mode_blob.id })
> if fb:
> @@ -271,7 +324,7 @@ class KMSTest(object):
> return req.commit(0, True)
>
> def atomic_plane_set(self, plane, crtc, source, destination, fb, sync=False):
> - req = pykms.AtomicReq(self.card)
> + req = AtomicRequest(self)
> req.add(plane, self.__format_props({
> 'FB_ID': fb.id,
> 'CRTC_ID': crtc.id,
> @@ -290,7 +343,7 @@ class KMSTest(object):
> return req.commit(0)
>
> def atomic_plane_disable(self, plane, sync=True):
> - req = pykms.AtomicReq(self.card)
> + req = AtomicRequest(self)
> req.add(plane, { "FB_ID": 0, 'CRTC_ID': 0 })
>
> if sync:
> @@ -299,7 +352,7 @@ class KMSTest(object):
> return req.commit(0)
>
> def atomic_planes_disable(self, sync=True):
> - req = pykms.AtomicReq(self.card)
> + req = AtomicRequest(self)
> for plane in self.card.planes:
> req.add(plane, { "FB_ID": 0, 'CRTC_ID': 0 })
>
>
--
Regards
--
Kieran
