Re: [PATCH 1/4] drm/fb-dma-helper: Add drm_fb_dma_get_gem_clipped_addr()

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 08.09.26 um 11:07 schrieb Chen-Yu Tsai:
> drm_fb_dma_get_gem_addr() returns the DMA address to the "unclipped"
> framebuffer. However some display drivers want the "clipped" framebuffer
> instead, as they are also using the clipped coordinates to program the
> hardware.
>
> Some of these drivers are open-coding drm_fb_dma_get_gem_addr() with
> the source coordinates replaced, while others have been incorrectly
> converted to using drm_fb_dma_get_gem_addr(), which would end up
> causing incorrect parts of the framebuffer to be displayed if it were
> somehow clipped.
>
> Add drm_fb_dma_get_gem_clipped_addr(), a "clipped" version of
> drm_fb_dma_get_gem_addr() for these drivers to use.
>
> Cc: <stable@vger.kernel.org> # dependency for next patch
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>   drivers/gpu/drm/drm_fb_dma_helper.c | 63 +++++++++++++++++++++--------
>   include/drm/drm_fb_dma_helper.h     |  4 ++
>   2 files changed, 51 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c
> index fd71969d2fb1..a260e7cd5667 100644
> --- a/drivers/gpu/drm/drm_fb_dma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_dma_helper.c
> @@ -59,20 +59,10 @@ struct drm_gem_dma_object *drm_fb_dma_get_gem_obj(struct drm_framebuffer *fb,
>   }
>   EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_obj);
>   
> -/**
> - * drm_fb_dma_get_gem_addr() - Get DMA (bus) address for framebuffer, for pixel
> - * formats where values are grouped in blocks this will get you the beginning of
> - * the block
> - * @fb: The framebuffer
> - * @state: Which state of drm plane
> - * @plane: Which plane
> - * Return the DMA GEM address for given framebuffer.
> - *
> - * This function will usually be called from the PLANE callback functions.
> - */
> -dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
> -				   struct drm_plane_state *state,
> -				   unsigned int plane)
> +static dma_addr_t _drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
> +					   unsigned int plane,
> +					   unsigned int x,
> +					   unsigned int y)
>   {
>   	struct drm_gem_dma_object *obj;
>   	dma_addr_t dma_addr;
> @@ -96,8 +86,8 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
>   		v_div = fb->format->vsub;
>   	}
>   
> -	sample_x = (state->src_x >> 16) / h_div;
> -	sample_y = (state->src_y >> 16) / v_div;
> +	sample_x = x / h_div;
> +	sample_y = y / v_div;
>   	block_start_y = (sample_y / block_h) * block_h;
>   	num_hblocks = sample_x / block_w;

The current code already mixes up responsibilities of the involved 
modules. It's a good opportunity to improve that. I think there should 
be a block-offset helper for the framebuffer. That function will return 
the byte offset of a pixel's block from the beginning of the framebuffer 
plane.

/* in drm_framebuffer.{c,h} */
u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, plane, 
x, y)
{
     /* here goes the current offset calculation from the gem-dma code */
}

In the GEM-DMA code, you can then write your helpers like this

drm_fb_dma_get_gem_addr(...)
{
     obj = drm_fb_dma_get_gem_ob(fb)

     offset = drm_framebuffer_get_block_offset(obj->base, state->src_x, 
state->src_y)

     dma_addr = obj->dma_addr + offset;

     return dma_addr
}

_get_clipped_gem_addr()
{
     /* likewise */
}

This better structures the responsibility. It also works for other GEM 
code besides GEM-DMA.

Best regards
Thomas


>   
> @@ -106,8 +96,49 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
>   
>   	return dma_addr;
>   }
> +
> +/**
> + * drm_fb_dma_get_gem_addr() - Get DMA (bus) address for unclipped framebuffer,
> + * for pixel formats where values are grouped in blocks this will get you the
> + * beginning of the block
> + * @fb: The framebuffer
> + * @state: Which state of drm plane
> + * @plane: Which plane
> + *
> + * This function will usually be called from the PLANE callback functions.
> + *
> + * Return: GEM DMA address for given framebuffer, unclipped.
> + */
> +dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
> +				   struct drm_plane_state *state,
> +				   unsigned int plane)
> +{
> +	return _drm_fb_dma_get_gem_addr(fb, plane, state->src_x >> 16,
> +					state->src_y >> 16);
> +}
>   EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_addr);
>   
> +/**
> + * drm_fb_dma_get_gem_clipped_addr() - Get DMA (bus) address for clipped
> + * framebuffer, for pixel formats where values are grouped in blocks this
> + * will get you the beginning of the block
> + * @fb: The framebuffer
> + * @state: Which state of drm plane
> + * @plane: Which plane
> + *
> + * This function will usually be called from the PLANE callback functions.
> + *
> + * Return: GEM DMA address for given framebuffer, clipped.
> + */
> +dma_addr_t drm_fb_dma_get_gem_clipped_addr(struct drm_framebuffer *fb,
> +					   struct drm_plane_state *state,
> +					   unsigned int plane)
> +{
> +	return _drm_fb_dma_get_gem_addr(fb, plane, state->src.x1 >> 16,
> +					state->src.y1 >> 16);
> +}
> +EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_clipped_addr);
> +
>   /**
>    * drm_fb_dma_sync_non_coherent - Sync GEM object to non-coherent backing
>    *	memory
> diff --git a/include/drm/drm_fb_dma_helper.h b/include/drm/drm_fb_dma_helper.h
> index c950732c6d36..b2a0bd7ef9d0 100644
> --- a/include/drm/drm_fb_dma_helper.h
> +++ b/include/drm/drm_fb_dma_helper.h
> @@ -17,6 +17,10 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
>   				   struct drm_plane_state *state,
>   				   unsigned int plane);
>   
> +dma_addr_t drm_fb_dma_get_gem_clipped_addr(struct drm_framebuffer *fb,
> +					   struct drm_plane_state *state,
> +					   unsigned int plane);
> +
>   void drm_fb_dma_sync_non_coherent(struct drm_device *drm,
>   				  struct drm_plane_state *old_state,
>   				  struct drm_plane_state *state);

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)