Hi Am 30.03.23 um 09:17 schrieb Sui Jingfeng:
Hi, On 2023/3/30 14:57, Thomas Zimmermann wrote:Hi Am 30.03.23 um 06:17 schrieb Lucas De Marchi:On Wed, Mar 29, 2023 at 11:04:17AM +0200, Thomas Zimmermann wrote:(cc'ing Lucas) Hi Am 25.03.23 um 08:46 schrieb Sui Jingfeng:The assignment already done in drm_client_buffer_vmap(), just trival clean, no functional change. Signed-off-by: Sui Jingfeng <15330273260@xxxxxx> --- drivers/gpu/drm/drm_fbdev_generic.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.cindex 4d6325e91565..1da48e71c7f1 100644 --- a/drivers/gpu/drm/drm_fbdev_generic.c +++ b/drivers/gpu/drm/drm_fbdev_generic.c@@ -282,7 +282,7 @@ static int drm_fbdev_damage_blit(struct drm_fb_helper *fb_helper,struct drm_clip_rect *clip) { struct drm_client_buffer *buffer = fb_helper->buffer; - struct iosys_map map, dst; + struct iosys_map map; int ret; /*@@ -302,8 +302,7 @@ static int drm_fbdev_damage_blit(struct drm_fb_helper *fb_helper,if (ret) goto out; - dst = map; - drm_fbdev_damage_blit_real(fb_helper, clip, &dst); + drm_fbdev_damage_blit_real(fb_helper, clip, &map);I see what you're doing and it's probably correct in this case.But there's a larger issue with this iosys interfaces. Sometimes the address has to be modified (see calls of iosys_map_incr()). That can prevent incorrect uses of the mapping in other places, especially in unmap code.using a initializer for the cases it's needed IMO would make these kind of problems go away, because then the intent is explicitI think it would make sense to consider a separate structure for the I/O location. The buffer as a whole would still be represented by struct iosys_map. And that new structure, let's call it struct iosys_ptr, would point to an actual location within the buffer'ssounds fine to me, but I'd have to take a deeper look later (or when someone writes the patch). It seems we'd replicate almost the entire API to just accomodate the 2 structs. And the different types will lead to confusion when one or the other should be usedI think we can split the current interface onto two categories: mapping and I/O. The former would use iosys_map and the latter would use iosys_ptr. And we'd need a helper that turns gets a ptr for a given map.If I find the tine, I'll probably type up a patch.Here i fix a typo, 'tine' -> 'time' As far as i can see, they are two major type of memory in the system.System memory or VRAM, for the gpu with dedicate video ram, VRAM is belong to the IO memory category.But there are system choose carveout part of system ram as video ram(i915?, for example).the name iosys_map and iosys_ptr have no difference at the first sight, tell me which one is for mapping system ramand which one is for mapping vram?
As you say correctly, graphics buffers and be in various locations. They can even move between I/O and system memory. The idea behind iosys_map ("I/O and/or system mapping") is that it's a single interface that can handle both.
Best regards Thomas
Best regards Thomasthanks Lucas De Marchimemory range. A few locations and helpers would need changes, but there are not so many callers that it's an issue. This would also allow for a few debugging tests that ensure that iosys_ptr always operates within the bounds of an iosys_map.I've long considered this idea, but there was no pressure to work on it. Maybe now.Best regards Thomasdrm_client_buffer_vunmap(buffer);-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
