Received-SPF: pass (google.com: domain of linux-kernel+bounces-83461-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 27 Feb 2024 16:02:10 +0100
From: Louis Chauvet <louis.chauvet@bootlin.com>
To: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>,
	Melissa Wen <melissa.srw@gmail.com>,
	=?iso-8859-1?Q?Ma=EDra?= Canal <mairacanal@riseup.net>,
	Haneen Mohammed <hamohammed.sa@gmail.com>,
	Daniel Vetter <daniel@ffwll.ch>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	David Airlie <airlied@gmail.com>, arthurgrillo@riseup.net,
	Jonathan Corbet <corbet@lwn.net>, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, jeremie.dautheribes@bootlin.com,
	miquel.raynal@bootlin.com, thomas.petazzoni@bootlin.com
Subject: Re: [PATCH v2 3/9] drm/vkms: write/update the documentation for
 pixel conversion and pixel write functions
Message-ID: <Zd35cimh8BLICUuY@localhost.localdomain>
Mail-Followup-To: Pekka Paalanen <pekka.paalanen@collabora.com>,
	Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>,
	Melissa Wen <melissa.srw@gmail.com>,
	=?iso-8859-1?Q?Ma=EDra?= Canal <mairacanal@riseup.net>,
	Haneen Mohammed <hamohammed.sa@gmail.com>,
	Daniel Vetter <daniel@ffwll.ch>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	David Airlie <airlied@gmail.com>, arthurgrillo@riseup.net,
	Jonathan Corbet <corbet@lwn.net>, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, jeremie.dautheribes@bootlin.com,
	miquel.raynal@bootlin.com, thomas.petazzoni@bootlin.com
References: <20240223-yuv-v2-0-aa6be2827bb7@bootlin.com>
 <20240223-yuv-v2-3-aa6be2827bb7@bootlin.com>
 <20240226133700.3fef91d9.pekka.paalanen@collabora.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240226133700.3fef91d9.pekka.paalanen@collabora.com>

[...]

> > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > index 172830a3936a..cb7a49b7c8e7 100644
> > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > @@ -9,6 +9,17 @@
> >  
> >  #include "vkms_formats.h"
> >  
> > +/**
> > + * packed_pixels_offset() - Get the offset of the block containing the pixel at coordinates x/y
> > + * in the first plane
> > + *
> > + * @frame_info: Buffer metadata
> > + * @x: The x coordinate of the wanted pixel in the buffer
> > + * @y: The y coordinate of the wanted pixel in the buffer
> > + *
> > + * The caller must be aware that this offset is not always a pointer to a pixel. If individual
> > + * pixel values are needed, they have to be extracted from the resulting block.
> 
> Just wondering how the caller will be able to extract the right pixel
> from the block without re-using the knowledge already used in this
> function. I'd also expect the function to round down x,y to be
> divisible by block dimensions, but that's not visible in this email.
> Then the caller needs the remainder from the round-down, too?

You are right, the current implementation is only working when block_h == 
block_w == 1. I think I wrote the documentation for PATCHv2 5/9, but when 
backporting this comment for PATCHv2 3/9 I forgot to update it.
The new comment will be:

 * pixels_offset() - Get the offset of a given pixel data at coordinate 
 * x/y in the first plane
   [...]
 * The caller must ensure that the framebuffer associated with this 
 * request uses a pixel format where block_h == block_w == 1.
 * If this requirement is not fulfilled, the resulting offset can be 
 * completly wrong.

And yes, even after PATCHv2 5/9 it is not clear what is the offset. Is 
this better to replace the last sentence? (I will do the same update for 
the last sentence of packed_pixels_addr)

   [...]
 * The returned offset correspond to the offset of the block containing the pixel at coordinates 
 * x/y.
 * The caller must use this offset with care, as for formats with block_h != 1 or block_w != 1 
 * the requested pixel value may have to be extracted from the block, even if they are 
 * individually adressable.
 
> > + */
> >  static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y)
> >  {
> >  	struct drm_framebuffer *fb = frame_info->fb;
> > @@ -17,12 +28,13 @@ static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int
> >  			      + (x * fb->format->cpp[0]);
> >  }
> >  

[...]

> > +/**
> > + * Retrieve the correct read_pixel function for a specific format.
> > + * The returned pointer is NULL for unsupported pixel formats. The caller must ensure that the
> > + * pointer is valid before using it in a vkms_plane_state.
> > + *
> > + * @format: 4cc of the format
> 
> Since there are many different 4cc style pixel format definition tables
> in existence with conflicting definitions, it would not hurt to be more
> specific that this is about DRM_FORMAT_* or drm_fourcc.h.

Is this better?

   @format: DRM_FORMAT_* value for which to obtain a conversion function (see [drm_fourcc.h])

> > + */
> >  void *get_pixel_conversion_function(u32 format)
> >  {
> >  	switch (format) {
> > @@ -247,6 +280,13 @@ void *get_pixel_conversion_function(u32 format)
> >  	}
> >  }
> >  
> > +/**
> > + * Retrieve the correct write_pixel function for a specific format.
> > + * The returned pointer is NULL for unsupported pixel formats. The caller must ensure that the
> > + * pointer is valid before using it in a vkms_writeback_job.
> > + *
> > + * @format: 4cc of the format
> 
> This too.

Ack, I will use the same as above

> > + */
> >  void *get_pixel_write_function(u32 format)
> >  {
> >  	switch (format) {
> > 
> 
> I couldn't check if the docs are correct since the patch context is not
> wide enough, but they all sound plausible to me.

I checked again, I don't see other errors than your first comment.
 
> 
> Thanks,
> pq

Kind regards,
Louis Chauvet

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