Re: [PATCH i-g-t v6 16/17] tests/xe_eudebug_online: Debug client which runs workloads on EU

["Grzegorzek, Dominik" <dominik.grzegorzek@intel.com>]

On Wed, 2024-09-18 at 07:08 +0200, Zbigniew Kempczyński wrote:
> On Tue, Sep 17, 2024 at 09:34:20PM +0200, Grzegorzek, Dominik wrote:
> 
> <cut>
> 
> > > > +static int count_set_bits(void *ptr, size_t size)
> > > > +{
> > > > +	uint8_t *p = ptr;
> > > > +	int count = 0;
> > > > +	int i, j;
> > > > +
> > > 
> > > hweight()?
> > > 
> > Are you proposing here to change the name or to implement it without second loop like below?
> 
> Yes, I just want to get rid of second loop.
> 
> > 
> > static int count_set_bits(void *ptr, size_t size)
> > {
> > 	uint32_t *p = ptr;
> > 	int count = 0;
> > 	int i;
> > 
> > 	igt_assert(size % 4 == 0);
> > 
> > 	for (i = 0; i < size/4; i++)
> > 		count += igt_hweight(p[i]);
> > 
> > 	return count;
> > }
> 
> You may iterate over uint8_t to cover all sizes, not only % 4.
> But if you're sure buffer will always be multiple of 4, this
> imo is ok.
> 
> <cut>
> 
> > > > +static void copy_first_bit(uint8_t *dst, uint8_t *src, int size)
> > > > +{
> > > > +	bool found = false;
> > > > +	int i, j;
> > > > +
> > > > +	for (i = 0; i < size; i++) {
> > > > +		if (found) {
> > > > +			dst[i] = 0;
> > > 
> > > Function is static, but according to line above I would add some
> > > comment that it is cleaning dst buffer. copy_first_bit() is misleading
> > > as you mean first bit set. First bit is src[0] & 1.
> > > 
> > > And what 'first' means? Having lets say src = { 0x0, 0xff, 0xcc, 0xaa }
> > > I would expect first should be most significant bit of 0xff.
> > > 
> > > 
> > > > +		} else {
> > > > +			uint32_t tmp = src[i]; /* in case dst == src */
> > > > +
> > > > +			for (j = 0; j < 8; j++) {
> > > 
> > > ffs()? But according to copy copy_nth_bit() I've doubts shouldn't this
> > > be fls()?
> > > 
> > > > +				dst[i] = tmp & (1 << j);
> > > > +				if (dst[i]) {
> > > > +					found = true;
> > > > +					break;
> > > > +				}
> > > > +			}
> > > > +		}
> > > > +	}
> > > > +}
> > > > +
> > > > +static void copy_nth_bit(uint8_t *dst, uint8_t *src, int size, int n)
> > > > +{
> > > > +	int count = 0;
> > > > +
> > > > +	for (int i = 0; i < size; i++) {
> > > > +		uint32_t tmp = src[i];
> > > > +
> > > > +		for (int j = 7; j >= 0; j--) {
> > > 
> > > I'm confused. In above function you iterate starting from least
> > > significant bit, here you start from most significant bit.
> > > Same concern about function name - shouldn't this be copy_nth_bit_set()?
> > > 
> > > > +			if (tmp & (1 << j)) {
> > > > +				count++;
> > > > +				if (count == n)
> > > > +					dst[i] |= (1 << j);
> > > > +				else
> > > > +					dst[i] &= ~(1 << j);
> > > 
> > > Do I understand correctly that you are clearing other bits in dst?
> > > It's extremely weird calling function copy_nth_bit() where it scans
> > > for n-th bit set, zeroing other bits in dst. Or I just don't understand
> > > logic behind this decision.
> > 
> > You've raised bunch of valid inaccuracies. How about:
> > 
> > static void only_nth_set_bit(uint8_t *dst, uint8_t *src, int size, int n)
> > {
> > 	int count = 0;
> > 
> > 	for (int i = 0; i < size; i++) {
> > 		if (count < n) {
> > 			uint8_t tmp = src[i];
> > 
> > 			for (int j = 0; j < 8; j++) {
> > 				if (tmp & (1 << j)) {
> > 					count++;
> > 					if (count == n)
> > 						dst[i] |= (1 << j);
> > 					else
> > 						dst[i] &= ~(1 << j);
> > 				} else {
> > 					dst[i] &= ~(1 << j);
> > 				}
> > 			}
> > 		} else {
> > 			dst[i] = 0;
> > 		}
> > 	}
> > }
> 
> Likely I would copy octet by octet from src[i] -> dst[i], tracking
> previous/current hweight and when it is bigger than n zeroing rest of bits in
> current octet. But this is implementation detail.
> 
> In above code you're copying from least significant bit, is this intended?
> Previous code was copying from most significant bit so this is
> definitely semantic change which according to hw behavior may be
> incorrect. May you double check this?
From the test pov this really doesn'm matter. We just wanted to keep single bit in the bitmask to
resume different thread. As long as different n gave different bit it was fine. However,
by 'first' I think we meant least significant bit, so I deliberately changed it that way. 

Regards,
Dominik

> 
> > 
> > static void only_first_set_bit(uint8_t *dst, uint8_t *src, int size)
> > {
> > 	return only_nth_set_bit(dst, src, size, 1);
> > }
> 
> Nice, code reuse is always good.
> 
> I'll drop rest of email to reply in another email. This will narrow
> our conversation to interesting part only.
> 
> --
> Zbigniew
>