Hi Kieran,
On Tue, Jun 18, 2019 at 06:14:35PM +0100, Kieran Bingham wrote:
> 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/
Oh my, I can do better :-/
> > 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.
Indeed. I will already add """pymkms.AtomicReq wrapper to track state
changes""" to document this class.
> > + 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:'
Both work but I agree that 'not in' reads better.
> > + 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 '}'
I'll remove them.
> > + 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
I expect a KeyError, I'll add it.
> > + 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,
Laurent Pinchart
