On Wed, Jun 29, 2022 at 04:36:49PM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2022-06-10 00:40:25)
> > Centralize props value formatting in the AtomicRequest.add() function to
> > avoid having to call it manually through the code base.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > tests/kmstest.py | 18 ++++++++++--------
> > 1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/tests/kmstest.py b/tests/kmstest.py
> > index 14e28cd47fbd..2afaa513aa4d 100755
> > --- a/tests/kmstest.py
> > +++ b/tests/kmstest.py
> > @@ -258,15 +258,20 @@ class AtomicRequest(pykms.AtomicReq):
> > self.__test = test
> > self.__props = {}
> >
> > + def __format_props(self, props):
>
> This is only validating the value arguements right, to ensure they are
> limited to 64 bit...
That's right, and it's mostly for negative values, that Python would
otherwise handle incorrectly (if I recall correctly that's due to the
bindings for the add() method mapping to the uint64_t, without automatic
conversion for negative values).
> > + return {k: v & ((1 << 64) - 1) for k, v in props.items()}
> > +
>
> Not this patch, but seems like 'add()' would benefit from some
> documentation here to show how to use it, what to pass etc.
Good point.
> It's complex to read from just this context, but I can just about see
> that this patch is cleaning things up.
>
> Reviewed-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
>
> > def add(self, obj, *kwargs):
> > if obj.id not in self.__props:
> > self.__props[obj.id] = {}
> > - props = self.__props[obj.id]
> > + obj_props = self.__props[obj.id]
> >
> > if len(kwargs) == 1 and isinstance(kwargs[0], collections.abc.Mapping):
> > - props.update(kwargs[0])
> > + props = self.__format_props(kwargs[0])
> > elif len(kwargs) == 2:
> > - props[kwargs[0]] = kwargs[1]
> > + props = self.__format_props({ kwargs[0]: = kwargs[1] })
> > +
> > + obj_props.update(props)
> >
> > super().add(obj, *kwargs)
> >
> > @@ -309,9 +314,6 @@ class KMSTest(object):
> > def __del__(self):
> > self.logger.close()
> >
> > - def __format_props(self, props):
> > - return {k: v & ((1 << 64) - 1) for k, v in props.items()}
> > -
> > def atomic_crtc_disable(self, crtc, sync=True):
> > req = AtomicRequest(self)
> > req.add(crtc, {'ACTIVE': 0, 'MODE_ID': 0})
> > @@ -368,7 +370,7 @@ class KMSTest(object):
> >
> > def atomic_plane_set(self, plane, crtc, source, destination, fb, sync=False):
> > req = AtomicRequest(self)
> > - req.add(plane, self.__format_props({
> > + req.add(plane, {
> > 'FB_ID': fb.id,
> > 'CRTC_ID': crtc.id,
> > 'SRC_X': int(source.left * 65536),
> > @@ -379,7 +381,7 @@ class KMSTest(object):
> > 'CRTC_Y': destination.top,
> > 'CRTC_W': destination.width,
> > 'CRTC_H': destination.height,
> > - }))
> > + })
> > if sync:
> > return req.commit_sync()
> > else:
--
Regards,
Laurent Pinchart
