[PATCH 0/5] drm/simpledrm: Various improvements

[PATCH 0/5] drm/simpledrm: Various improvements

[Thomas Zimmermann]

This patchset contains various improvements to simpledrm that have
piled up over time.

Thomas Zimmermann (5):
  drm/simpledrm: Compute linestride with drm_format_info_min_pitch()
  drm/simpledrm: Use drm_atomic_get_new_plane_state()
  drm/simpledrm: Remove !fb check from atomic_update
  drm/simpledrm: Iterate over damage clips
  drm/simpledrm: Synchronize access to GEM BOs

 drivers/gpu/drm/tiny/simpledrm.c | 48 +++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 20 deletions(-)


base-commit: a7d5d07d5ac5ac58ec81932b3f732e3127d17af9
-- 
2.37.3

[PATCH 1/5] drm/simpledrm: Compute linestride with drm_format_info_min_pitch()

[Thomas Zimmermann]

If not given, compute the stride with drm_format_info_min_pitch(). It's
the standard helper for this purpose.

Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: fd9e3169e42b ("drm/simpledrm: Compute framebuffer stride if not set")
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/tiny/simpledrm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index ea5b3239a659..51d01e34d5eb 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -687,8 +687,11 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 		drm_err(dev, "no simplefb configuration found\n");
 		return ERR_PTR(-ENODEV);
 	}
-	if (!stride)
-		stride = DIV_ROUND_UP(drm_format_info_bpp(format, 0) * width, 8);
+	if (!stride) {
+		stride = drm_format_info_min_pitch(format, 0, width);
+		if (drm_WARN_ON(dev, !stride))
+			return ERR_PTR(-EINVAL);
+	}
 
 	sdev->mode = simpledrm_mode(width, height);
 	sdev->format = format;
-- 
2.37.3

[PATCH 2/5] drm/simpledrm: Use drm_atomic_get_new_plane_state()

[Thomas Zimmermann]

Lookup the plane's state in atomic_update with the helper
drm_atomic_get_new_plane_state(). Also rename the helpers'
state arguments. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/simpledrm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 51d01e34d5eb..14782a50f816 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -470,10 +470,10 @@ static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
 };
 
 static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
-							 struct drm_atomic_state *old_state)
+							 struct drm_atomic_state *state)
 {
-	struct drm_plane_state *plane_state = plane->state;
-	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(old_state, plane);
+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
 	struct drm_framebuffer *fb = plane_state->fb;
 	struct drm_device *dev = plane->dev;
@@ -503,7 +503,7 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
 }
 
 static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plane,
-							  struct drm_atomic_state *old_state)
+							  struct drm_atomic_state *state)
 {
 	struct drm_device *dev = plane->dev;
 	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
-- 
2.37.3

[PATCH 3/5] drm/simpledrm: Remove !fb check from atomic_update

[Thomas Zimmermann]

The primary plane implements atomic_disable, so atomic_update will
not be called without a framebuffer set. Remove the test for !fb.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/simpledrm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 14782a50f816..8fab22a26e26 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -482,9 +482,6 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
 	struct drm_rect src_clip, dst_clip;
 	int idx;
 
-	if (!fb)
-		return;
-
 	if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, &src_clip))
 		return;
 
-- 
2.37.3

[PATCH 4/5] drm/simpledrm: Iterate over damage clips

[Thomas Zimmermann]

Iterate over all damage clips and updated them one by one. Replaces
the merging of damage areas, which can result in significant overhead
if damage areas are not close to each other.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/simpledrm.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 8fab22a26e26..5f242558891e 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -478,23 +478,25 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
 	struct drm_framebuffer *fb = plane_state->fb;
 	struct drm_device *dev = plane->dev;
 	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
-	struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
-	struct drm_rect src_clip, dst_clip;
+	struct drm_atomic_helper_damage_iter iter;
+	struct drm_rect damage;
 	int idx;
 
-	if (!drm_atomic_helper_damage_merged(old_plane_state, plane_state, &src_clip))
+	if (!drm_dev_enter(dev, &idx))
 		return;
 
-	dst_clip = plane_state->dst;
-	if (!drm_rect_intersect(&dst_clip, &src_clip))
-		return;
+	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
+	drm_atomic_for_each_plane_damage(&iter, &damage) {
+		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
+		struct drm_rect dst_clip = plane_state->dst;
 
-	if (!drm_dev_enter(dev, &idx))
-		return;
+		if (!drm_rect_intersect(&dst_clip, &damage))
+			continue;
 
-	iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
-	drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data, fb,
-		    &src_clip);
+		iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format, &dst_clip));
+		drm_fb_blit(&dst, &sdev->pitch, sdev->format->format, shadow_plane_state->data, fb,
+			    &damage);
+	}
 
 	drm_dev_exit(idx);
 }
-- 
2.37.3

[PATCH 5/5] drm/simpledrm: Synchronize access to GEM BOs

[Thomas Zimmermann]

Synchronize CPU access to GEM BOs with other drivers when updating the
screen buffer. Imported buffers might otherwise contain stale data.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/simpledrm.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 5f242558891e..18489779fb8a 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -480,11 +480,15 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
 	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
 	struct drm_atomic_helper_damage_iter iter;
 	struct drm_rect damage;
-	int idx;
+	int ret, idx;
 
-	if (!drm_dev_enter(dev, &idx))
+	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+	if (ret)
 		return;
 
+	if (!drm_dev_enter(dev, &idx))
+		goto out_drm_gem_fb_end_cpu_access;
+
 	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
 	drm_atomic_for_each_plane_damage(&iter, &damage) {
 		struct iosys_map dst = IOSYS_MAP_INIT_VADDR(sdev->screen_base);
@@ -499,6 +503,8 @@ static void simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
 	}
 
 	drm_dev_exit(idx);
+out_drm_gem_fb_end_cpu_access:
+	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
 }
 
 static void simpledrm_primary_plane_helper_atomic_disable(struct drm_plane *plane,
-- 
2.37.3

RE: [PATCH 2/5] drm/simpledrm: Use drm_atomic_get_new_plane_state()

[Ruhl, Michael J]

>-----Original Message-----
>From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
>Thomas Zimmermann
>Sent: Thursday, September 22, 2022 9:10 AM
>To: javierm@redhat.com; airlied@linux.ie; daniel@ffwll.ch
>Cc: Thomas Zimmermann <tzimmermann@suse.de>; dri-
>devel@lists.freedesktop.org
>Subject: [PATCH 2/5] drm/simpledrm: Use
>drm_atomic_get_new_plane_state()
>
>Lookup the plane's state in atomic_update with the helper
>drm_atomic_get_new_plane_state(). Also rename the helpers'
>state arguments. No functional changes.
>
>Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>---
> drivers/gpu/drm/tiny/simpledrm.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/tiny/simpledrm.c
>b/drivers/gpu/drm/tiny/simpledrm.c
>index 51d01e34d5eb..14782a50f816 100644
>--- a/drivers/gpu/drm/tiny/simpledrm.c
>+++ b/drivers/gpu/drm/tiny/simpledrm.c
>@@ -470,10 +470,10 @@ static const uint64_t
>simpledrm_primary_plane_format_modifiers[] = {
> };
>
> static void simpledrm_primary_plane_helper_atomic_update(struct
>drm_plane *plane,
>-							 struct
>drm_atomic_state *old_state)
>+							 struct
>drm_atomic_state *state)
> {
>-	struct drm_plane_state *plane_state = plane->state;
>-	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(old_state, plane);
>+	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);

Going from plane->state to drm_atomic_get_new_plane_state seems like a slight function change.

If this is the equivalent and the "right" way to do this, should the ->state part of the data
structure be pruned?

The comment for drm_atomic_get_new_plane_state also says that it can return NULL.

would plane->state be NULL in this case?

Thanks,

M

>+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> 	struct drm_framebuffer *fb = plane_state->fb;
> 	struct drm_device *dev = plane->dev;
>@@ -503,7 +503,7 @@ static void
>simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
> }
>
> static void simpledrm_primary_plane_helper_atomic_disable(struct
>drm_plane *plane,
>-							  struct
>drm_atomic_state *old_state)
>+							  struct
>drm_atomic_state *state)
> {
> 	struct drm_device *dev = plane->dev;
> 	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
>--
>2.37.3

Re: [PATCH 1/5] drm/simpledrm: Compute linestride with drm_format_info_min_pitch()

[Javier Martinez Canillas]

Hello Thomas,

On 9/22/22 15:09, Thomas Zimmermann wrote:
> If not given, compute the stride with drm_format_info_min_pitch(). It's
> the standard helper for this purpose.
> 
> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: fd9e3169e42b ("drm/simpledrm: Compute framebuffer stride if not set")
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/tiny/simpledrm.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/5] drm/simpledrm: Use drm_atomic_get_new_plane_state()

[Javier Martinez Canillas]

On 9/22/22 15:09, Thomas Zimmermann wrote:
> Lookup the plane's state in atomic_update with the helper
> drm_atomic_get_new_plane_state(). Also rename the helpers'
> state arguments. No functional changes.
> 

I think this was Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> ?

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 3/5] drm/simpledrm: Remove !fb check from atomic_update

[Javier Martinez Canillas]

On 9/22/22 15:09, Thomas Zimmermann wrote:
> The primary plane implements atomic_disable, so atomic_update will
> not be called without a framebuffer set. Remove the test for !fb.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/tiny/simpledrm.c | 3 ---
>  1 file changed, 3 deletions(-)
> 

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/5] drm/simpledrm: Use drm_atomic_get_new_plane_state()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3392 bytes --]

Hi

Am 22.09.22 um 18:12 schrieb Ruhl, Michael J:
>> -----Original Message-----
>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
>> Thomas Zimmermann
>> Sent: Thursday, September 22, 2022 9:10 AM
>> To: javierm@redhat.com; airlied@linux.ie; daniel@ffwll.ch
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>; dri-
>> devel@lists.freedesktop.org
>> Subject: [PATCH 2/5] drm/simpledrm: Use
>> drm_atomic_get_new_plane_state()
>>
>> Lookup the plane's state in atomic_update with the helper
>> drm_atomic_get_new_plane_state(). Also rename the helpers'
>> state arguments. No functional changes.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>> drivers/gpu/drm/tiny/simpledrm.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c
>> b/drivers/gpu/drm/tiny/simpledrm.c
>> index 51d01e34d5eb..14782a50f816 100644
>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>> @@ -470,10 +470,10 @@ static const uint64_t
>> simpledrm_primary_plane_format_modifiers[] = {
>> };
>>
>> static void simpledrm_primary_plane_helper_atomic_update(struct
>> drm_plane *plane,
>> -							 struct
>> drm_atomic_state *old_state)
>> +							 struct
>> drm_atomic_state *state)
>> {
>> -	struct drm_plane_state *plane_state = plane->state;
>> -	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(old_state, plane);
>> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> 
> Going from plane->state to drm_atomic_get_new_plane_state seems like a slight function change.
> 
> If this is the equivalent and the "right" way to do this, should the ->state part of the data
> structure be pruned?

The plane->state field is still relevant. I recently learned that the 
state-lookup helpers are supposed to be used in all atomic_check/commit 
functions. Ville gave a description here:

   https://lore.kernel.org/dri-devel/Yx9pij4LmFHrq81V@intel.com/

> 
> The comment for drm_atomic_get_new_plane_state also says that it can return NULL.
> 
> would plane->state be NULL in this case?

I don't think so. In atomic_update, we know that we're supposed to 
change the plane. That requires the state. Maybe it's different in the 
plane's atomic_disable helper, we don't access the state there.

Best regards
Thomas

> 
> Thanks,
> 
> M
> 
>> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
>> 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
>> 	struct drm_framebuffer *fb = plane_state->fb;
>> 	struct drm_device *dev = plane->dev;
>> @@ -503,7 +503,7 @@ static void
>> simpledrm_primary_plane_helper_atomic_update(struct drm_plane *plane
>> }
>>
>> static void simpledrm_primary_plane_helper_atomic_disable(struct
>> drm_plane *plane,
>> -							  struct
>> drm_atomic_state *old_state)
>> +							  struct
>> drm_atomic_state *state)
>> {
>> 	struct drm_device *dev = plane->dev;
>> 	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
>> --
>> 2.37.3
> 

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 4/5] drm/simpledrm: Iterate over damage clips

[Javier Martinez Canillas]

On 9/22/22 15:09, Thomas Zimmermann wrote:
> Iterate over all damage clips and updated them one by one. Replaces
> the merging of damage areas, which can result in significant overhead
> if damage areas are not close to each other.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/5] drm/simpledrm: Use drm_atomic_get_new_plane_state()

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 804 bytes --]

Hi

Am 23.09.22 um 09:47 schrieb Javier Martinez Canillas:
> On 9/22/22 15:09, Thomas Zimmermann wrote:
>> Lookup the plane's state in atomic_update with the helper
>> drm_atomic_get_new_plane_state(). Also rename the helpers'
>> state arguments. No functional changes.
>>
> 
> I think this was Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> ?

Not directly, but it seems fair to give him credit.

> 
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

Thanks for the r-bs.

Best regards
Thomas

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 5/5] drm/simpledrm: Synchronize access to GEM BOs

[Javier Martinez Canillas]

On 9/22/22 15:09, Thomas Zimmermann wrote:
> Synchronize CPU access to GEM BOs with other drivers when updating the
> screen buffer. Imported buffers might otherwise contain stale data.
>

Can you please elaborate what the problem is? The framebuffers memory is
setup by the firmware and would never come from an imported dma-buf, so
could the GEM BOs even be shared with other drivers?

Or is this done just for the sake of correctness ?
 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

The change looks good to me though if is for the latter.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 5/5] drm/simpledrm: Synchronize access to GEM BOs

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1486 bytes --]

Hi

Am 23.09.22 um 10:06 schrieb Javier Martinez Canillas:
> On 9/22/22 15:09, Thomas Zimmermann wrote:
>> Synchronize CPU access to GEM BOs with other drivers when updating the
>> screen buffer. Imported buffers might otherwise contain stale data.
>>
> 
> Can you please elaborate what the problem is? The framebuffers memory is
> setup by the firmware and would never come from an imported dma-buf, so
> could the GEM BOs even be shared with other drivers?
> 
> Or is this done just for the sake of correctness ?

This isn't about the scanout buffer that we get from the firmware. This 
is about synchronizing access to the GEM BO memory buffers. Our BOs' 
memory buffer are allocated from SHMEM or could be imported via dma-buf. 
In the latter case, another driver or hardware might modify their 
content concurrently. We need to synchronize before reading the memory 
from our CPU. The pattern is

   1) sync BO memory content via begin_cpu_access()
   2) blit from BO mem to scanout buffer
   3) release BO memory via end_cpu_access()

Best regards
Thomas

>   
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> The change looks good to me though if is for the latter.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 5/5] drm/simpledrm: Synchronize access to GEM BOs

[Javier Martinez Canillas]

On 9/23/22 12:43, Thomas Zimmermann wrote:
> Hi
> 
> Am 23.09.22 um 10:06 schrieb Javier Martinez Canillas:
>> On 9/22/22 15:09, Thomas Zimmermann wrote:
>>> Synchronize CPU access to GEM BOs with other drivers when updating the
>>> screen buffer. Imported buffers might otherwise contain stale data.
>>>
>>
>> Can you please elaborate what the problem is? The framebuffers memory is
>> setup by the firmware and would never come from an imported dma-buf, so
>> could the GEM BOs even be shared with other drivers?
>>
>> Or is this done just for the sake of correctness ?
> 
> This isn't about the scanout buffer that we get from the firmware. This 
> is about synchronizing access to the GEM BO memory buffers. Our BOs' 
> memory buffer are allocated from SHMEM or could be imported via dma-buf. 
> In the latter case, another driver or hardware might modify their 
> content concurrently. We need to synchronize before reading the memory 
> from our CPU. The pattern is
> 
>    1) sync BO memory content via begin_cpu_access()
>    2) blit from BO mem to scanout buffer
>    3) release BO memory via end_cpu_access()
>

Yeah, I got that part. What I was asking is if importing from dma-buf is
a real use case for simpledrm or if this patch was more about making the
driver correct and aligned with what other DRM drivers do?

In any case I agree with the change, it's just the rationale for it that
wasn't clear to me.
-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 5/5] drm/simpledrm: Synchronize access to GEM BOs

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2018 bytes --]

Hi

Am 23.09.22 um 12:58 schrieb Javier Martinez Canillas:
> On 9/23/22 12:43, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 23.09.22 um 10:06 schrieb Javier Martinez Canillas:
>>> On 9/22/22 15:09, Thomas Zimmermann wrote:
>>>> Synchronize CPU access to GEM BOs with other drivers when updating the
>>>> screen buffer. Imported buffers might otherwise contain stale data.
>>>>
>>>
>>> Can you please elaborate what the problem is? The framebuffers memory is
>>> setup by the firmware and would never come from an imported dma-buf, so
>>> could the GEM BOs even be shared with other drivers?
>>>
>>> Or is this done just for the sake of correctness ?
>>
>> This isn't about the scanout buffer that we get from the firmware. This
>> is about synchronizing access to the GEM BO memory buffers. Our BOs'
>> memory buffer are allocated from SHMEM or could be imported via dma-buf.
>> In the latter case, another driver or hardware might modify their
>> content concurrently. We need to synchronize before reading the memory
>> from our CPU. The pattern is
>>
>>     1) sync BO memory content via begin_cpu_access()
>>     2) blit from BO mem to scanout buffer
>>     3) release BO memory via end_cpu_access()
>>
> 
> Yeah, I got that part. What I was asking is if importing from dma-buf is
> a real use case for simpledrm or if this patch was more about making the
> driver correct and aligned with what other DRM drivers do?

In a multi-GPU scenario, it could happen that one display uses 
simpledrm. If the other GPU has a native driver that exports the BO for 
screen mirroring, the synchronization might be needed. Admittedly it's 
not a likely scenario. But still...

Best regards
Thomas

> 
> In any case I agree with the change, it's just the rationale for it that
> wasn't clear to me.

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

RE: [PATCH 2/5] drm/simpledrm: Use drm_atomic_get_new_plane_state()

[Ruhl, Michael J]

>-----Original Message-----
>From: Thomas Zimmermann <tzimmermann@suse.de>
>Sent: Friday, September 23, 2022 3:53 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>; javierm@redhat.com;
>airlied@linux.ie; daniel@ffwll.ch
>Cc: dri-devel@lists.freedesktop.org
>Subject: Re: [PATCH 2/5] drm/simpledrm: Use
>drm_atomic_get_new_plane_state()
>
>Hi
>
>Am 22.09.22 um 18:12 schrieb Ruhl, Michael J:
>>> -----Original Message-----
>>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
>>> Thomas Zimmermann
>>> Sent: Thursday, September 22, 2022 9:10 AM
>>> To: javierm@redhat.com; airlied@linux.ie; daniel@ffwll.ch
>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>; dri-
>>> devel@lists.freedesktop.org
>>> Subject: [PATCH 2/5] drm/simpledrm: Use
>>> drm_atomic_get_new_plane_state()
>>>
>>> Lookup the plane's state in atomic_update with the helper
>>> drm_atomic_get_new_plane_state(). Also rename the helpers'
>>> state arguments. No functional changes.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>> drivers/gpu/drm/tiny/simpledrm.c | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tiny/simpledrm.c
>>> b/drivers/gpu/drm/tiny/simpledrm.c
>>> index 51d01e34d5eb..14782a50f816 100644
>>> --- a/drivers/gpu/drm/tiny/simpledrm.c
>>> +++ b/drivers/gpu/drm/tiny/simpledrm.c
>>> @@ -470,10 +470,10 @@ static const uint64_t
>>> simpledrm_primary_plane_format_modifiers[] = {
>>> };
>>>
>>> static void simpledrm_primary_plane_helper_atomic_update(struct
>>> drm_plane *plane,
>>> -							 struct
>>> drm_atomic_state *old_state)
>>> +							 struct
>>> drm_atomic_state *state)
>>> {
>>> -	struct drm_plane_state *plane_state = plane->state;
>>> -	struct drm_plane_state *old_plane_state =
>drm_atomic_get_old_plane_state(old_state, plane);
>>> +	struct drm_plane_state *plane_state =
>drm_atomic_get_new_plane_state(state, plane);
>>
>> Going from plane->state to drm_atomic_get_new_plane_state seems like a
>slight function change.
>>
>> If this is the equivalent and the "right" way to do this, should the ->state
>part of the data
>> structure be pruned?
>
>The plane->state field is still relevant. I recently learned that the
>state-lookup helpers are supposed to be used in all atomic_check/commit
>functions. Ville gave a description here:
>
>   https://lore.kernel.org/dri-devel/Yx9pij4LmFHrq81V@intel.com/
>
>>
>> The comment for drm_atomic_get_new_plane_state also says that it can
>return NULL.
>>
>> would plane->state be NULL in this case?
>
>I don't think so. In atomic_update, we know that we're supposed to
>change the plane. That requires the state. Maybe it's different in the
>plane's atomic_disable helper, we don't access the state there.

Ok, that makes sense.

If you need it:

Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com>

M


>Best regards
>Thomas
>
>>
>> Thanks,
>>
>> M
>>
>>> +	struct drm_plane_state *old_plane_state =
>drm_atomic_get_old_plane_state(state, plane);
>>> 	struct drm_shadow_plane_state *shadow_plane_state =
>to_drm_shadow_plane_state(plane_state);
>>> 	struct drm_framebuffer *fb = plane_state->fb;
>>> 	struct drm_device *dev = plane->dev;
>>> @@ -503,7 +503,7 @@ static void
>>> simpledrm_primary_plane_helper_atomic_update(struct drm_plane
>*plane
>>> }
>>>
>>> static void simpledrm_primary_plane_helper_atomic_disable(struct
>>> drm_plane *plane,
>>> -							  struct
>>> drm_atomic_state *old_state)
>>> +							  struct
>>> drm_atomic_state *state)
>>> {
>>> 	struct drm_device *dev = plane->dev;
>>> 	struct simpledrm_device *sdev = simpledrm_device_of_dev(dev);
>>> --
>>> 2.37.3
>>
>
>--
>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