linux-kernel - Re: [PATCH v6 09/17] drm/vkms: Introduce pixel_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 13 May 2024 09:15:14 +0200
From: Louis Chauvet <louis.chauvet@...tlin.com>
To: Pekka Paalanen <pekka.paalanen@...labora.com>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>,
	Melissa Wen <melissa.srw@...il.com>,
	Maíra Canal <mairacanal@...eup.net>,
	Haneen Mohammed <hamohammed.sa@...il.com>,
	Daniel Vetter <daniel@...ll.ch>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, rdunlap@...radead.org,
	arthurgrillo@...eup.net, Jonathan Corbet <corbet@....net>,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
	jeremie.dautheribes@...tlin.com, miquel.raynal@...tlin.com,
	thomas.petazzoni@...tlin.com, seanpaul@...gle.com,
	marcheu@...gle.com, nicolejadeyee@...gle.com
Subject: Re: [PATCH v6 09/17] drm/vkms: Introduce pixel_read_direction enum

[...]

> > +/**
> > + * direction_for_rotation() - Get the correct reading direction for a given rotation
> > + *
> > + * @rotation: Rotation to analyze. It correspond the field @frame_info.rotation.
> > + *
> > + * This function will use the @rotation setting of a source plane to compute the reading
> > + * direction in this plane which correspond to a "left to right writing" in the CRTC.
> > + * For example, if the buffer is reflected on X axis, the pixel must be read from right to left
> > + * to be written from left to right on the CRTC.
> > + */
> > +static enum pixel_read_direction direction_for_rotation(unsigned int rotation)
> > +{
> > +	struct drm_rect tmp_a, tmp_b;
> > +	int x, y;
> > +
> > +	/*
> > +	 * The direction is computed by rotating the vector AB (top-left to top-right) in a
> > +	 * 1x1 square.
> 
> Points A and B are depicted as zero-size rectangles on the CRTC.
> The CRTC writing direction is from A to B. The plane reading direction
> is discovered by inverse-transforming A and B.
> 
> (If you want, you can add that to the comment.)

It is better, thanks!
 
> > +	 */
> > +
> > +	tmp_a = DRM_RECT_INIT(0, 0, 0, 0);
> > +	tmp_b = DRM_RECT_INIT(1, 0, 0, 0);
> > +	drm_rect_rotate_inv(&tmp_a, 1, 1, rotation);
> > +	drm_rect_rotate_inv(&tmp_b, 1, 1, rotation);
> > +
> > +	x = tmp_b.x1 - tmp_a.x1;
> > +	y = tmp_b.y1 - tmp_a.y1;
> > +
> > +	if (x == 1)
> > +		return READ_LEFT_TO_RIGHT;
> > +	else if (x == -1)
> > +		return READ_RIGHT_TO_LEFT;
> > +	else if (y == 1)
> > +		return READ_TOP_TO_BOTTOM;
> > +	else if (y == -1)
> > +		return READ_BOTTOM_TO_TOP;
> 
> I find this code practically obvious. Excellent!
> 
> If you want to be more strict, each condition could also require the
> other component to be zero.

I will add it.

[...]

> > + */
> > +static int get_block_step_byte(struct drm_framebuffer *fb, enum pixel_read_direction direction,
> > +			       int plane_index)
> > +{
> > +	switch (direction) {
> > +	case READ_LEFT_TO_RIGHT:
> > +		return fb->format->char_per_block[plane_index];
> > +	case READ_RIGHT_TO_LEFT:
> > +		return -fb->format->char_per_block[plane_index];
> > +	case READ_TOP_TO_BOTTOM:
> > +		return (int)fb->pitches[plane_index];
> > +	case READ_BOTTOM_TO_TOP:
> > +		return -(int)fb->pitches[plane_index];
> 
> I'm not sure if this is correct for formats with block_h > 1.
> 
> If a pitch is the theoretical count of bytes per line, then this should
> return block_h * pitch. But I'm not exactly sure what is correct here.

I think it is related to my answer to patch 07/17. If pitch is what you 
describe, yes, the step should be block_h * DIV_ROUND_UP(fb_width / 
block_w) (or something similar).

If I take X0L2 with a buffer of 9x5 pixels, the step between two blocks 
verticaly must be 40 bytes. Your formula and interpretation of pitch will 
give only 36 bytes (a row only need 18 bytes). So a new "pitch" is needed.

> Aside from this problem, looks good.
> 
> 
> Thanks,
> pq
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * packed_pixels_addr_1x1() - Get the pointer to the block containing the pixel at the given
> >   * coordinates
> > 
> 



-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com