The assignment already done in drm_client_buffer_vmap(),
just trival clean, no functional change.
Signed-off-by: Sui Jingfeng <[email protected]>
---
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.c
index 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);
drm_client_buffer_vunmap(buffer);
--
2.25.1
(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 <[email protected]>
> ---
> 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.c
> index 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.
I 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's memory 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
Thomas
>
> drm_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
Am 29.03.23 um 11:04 schrieb Thomas Zimmermann:
> (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 <[email protected]>
>> ---
>> 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.c
>> index 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
'prevent correct uses'
> unmap code.
>
> I 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's memory 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
> Thomas
>
>> drm_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
On 2023/3/29 17:04, 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 <[email protected]>
>> ---
>> 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.c
>> index 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.
>
> I 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's
> memory 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.
>
Ok. that's fine then.
> Best regards
> Thomas
>
>> drm_client_buffer_vunmap(buffer);
>
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 <[email protected]>
>>---
>> 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.c
>>index 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 explicit
>
>I 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's
sounds 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 used
thanks
Lucas De Marchi
>memory 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
>Thomas
>
>> drm_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
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 <[email protected]>
>>> ---
>>> 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.c
>>> index 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 explicit
>
>>
>> I 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's
>
> sounds 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 used
I 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.
Best regards
Thomas
>
> thanks
> Lucas De Marchi
>
>> memory 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
>> Thomas
>>
>>> drm_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
On Thu, Mar 30, 2023 at 08:57:36AM +0200, 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 <[email protected]>
>>>>---
>>>>?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.c
>>>>index 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 explicit
>>
>>>
>>>I 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's
>>
>>sounds 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 used
>
>I 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.
yeah, a split would make it better rather than just make iosys_ptr
replace the current cases where we copy the struct. In this case most
places passing a buffer around in the same driver would migrate to
iosys_ptr.
thanks
Lucas De Marchi
>
>Best regards
>Thomas
>
>>
>>thanks
>>Lucas De Marchi
>>
>>>memory 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
>>>Thomas
>>>
>>>>???? drm_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
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 <[email protected]>
>>>> ---
>>>> 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.c
>>>> index 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 explicit
>>
>>>
>>> I 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's
>>
>> sounds 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 used
>
> I 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 ram
and which one is for mapping vram?
> Best regards
> Thomas
>
>>
>> thanks
>> Lucas De Marchi
>>
>>> memory 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
>>> Thomas
>>>
>>>> drm_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
>>
>>
>>
>
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 <[email protected]>
>>>>> ---
>>>>> 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.c
>>>>> index 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 explicit
>>>
>>>>
>>>> I 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's
>>>
>>> sounds 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 used
>>
>> I 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 ram
>
> and 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
>> Thomas
>>
>>>
>>> thanks
>>> Lucas De Marchi
>>>
>>>> memory 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
>>>> Thomas
>>>>
>>>>> drm_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
On 2023/3/30 15:26, Thomas Zimmermann wrote:
> 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 <[email protected]>
>>>>>> ---
>>>>>> 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.c
>>>>>> index 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 explicit
>>>>
>>>>>
>>>>> I 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's
>>>>
>>>> sounds 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 used
>>>
>>> I 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 ram
>>
>> and 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.
>
I somewhat miss the point, sound like const pointer(const void* const p)
V.S. plain pointer (void *)
I understand what you meant then.
> Best regards
> Thomas
>
>>
>>
>>> Best regards
>>> Thomas
>>>
>>>>
>>>> thanks
>>>> Lucas De Marchi
>>>>
>>>>> memory 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
>>>>> Thomas
>>>>>
>>>>>> drm_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
>>>>
>>>>
>>>>
>>>
>
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 <[email protected]>
>>>> ---
>>>> 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.c
>>>> index 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 explicit
>>
>>>
>>> I 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's
>>
>> sounds 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 used
>
> I 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.
>
This sound fine and interesting.
I don't have enough time to do this, as I'm focus on developing driver
for my company's hardware.
But I will keep an eye on it, see what interesting will be happen.
Becuase the fbdev related code will also be used on my company's
ls7a1000 and ls7a2000 platform.
It is useful and interesting, Our device at least can functional as a
big frame buffer and more than framebuffer.
The bandwidth for VRAM write is very high(when write combine is open).
at least 10+ times batter than aspeed bmc.
glxgear on [email protected]+ls7a1000 can run above 1000+fps. while with
the aspeed bmc it is just 100+ fps all so.
> Best regards
> Thomas
>
>>
>> thanks
>> Lucas De Marchi
>>
>>> memory 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
>>> Thomas
>>>
>>>> drm_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
>>
>>
>>
>
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 <[email protected]>
>>>> ---
>>>> 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.c
>>>> index 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 explicit
>>
>>>
>>> I 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's
>>
>> sounds 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 used
>
> I 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.
>
Then merge this trivial patch first?
It will not prohibit anyone achieve a big goal or releasing a better
patch in the future.
> Best regards
> Thomas
>
>>
>> thanks
>> Lucas De Marchi
>>
>>> memory 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
>>> Thomas
>>>
>>>> drm_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
>>
>>
>>
>
On 2023/3/30 15:26, Thomas Zimmermann wrote:
> 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 <[email protected]>
>>>>>> ---
>>>>>> 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.c
>>>>>> index 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 explicit
>>>>
>>>>>
>>>>> I 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's
>>>>
>>>> sounds 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 used
>>>
>>> I 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 ram
>>
>> and 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.
>
They are all pointers in its very nature.
The hard part to make ensure that iosys_map can not be replaced with
iosys_ptr,
They should not overlap in functional, I meant.
> Best regards
> Thomas
>
>>
>>
>>> Best regards
>>> Thomas
>>>
>>>>
>>>> thanks
>>>> Lucas De Marchi
>>>>
>>>>> memory 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
>>>>> Thomas
>>>>>
>>>>>> drm_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
>>>>
>>>>
>>>>
>>>
>
On 2023/3/29 17:04, 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 <[email protected]>
>> ---
>> 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.c
>> index 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.
>
Yes, I just realized that.
iosys_map_incr() change the internal state of a opaque structure, this
is somewhat evil.
if it is non-opaque, then this is abstract failure.
You have to worry about that if it is changed by a accident call
iosys_map_incr() from other place.
The map should be const, I guess most programmer expect the map be a const.
making it const please, copy on demand, modify the copy only, leave the
original mapping untouched.
Hope this could eliminate the embarrassing.
Sorry for missing the point.
> I 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's
> memory 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.
>
I have also get some idea from you idea.
> Best regards
> Thomas
>
>> drm_client_buffer_vunmap(buffer);
>