Re: [PATCH 1/5] drm/i915/ttm: Add I915_BO_PREALLOC
> Add a mechanism to preserve existing data when creating a TTM > object with the I915_BO_ALLOC_USER flag. This will be used in the subsequent > patch where the I915_BO_ALLOC_USER flag will be applied to the framebuffer > object. For a pre-allocated framebuffer without the I915_BO_PREALLOC flag, > TTM would clear the content, which is not desirable. ack! Andi
Re: [PATCH 1/5] drm/i915/ttm: Add I915_BO_PREALLOC
Hi Andi, On 4/5/2023 1:53 PM, Andi Shyti wrote: Hi Nirmoy, Add a mechanism to keep existing data when creating a ttm object with I915_BO_ALLOC_USER flag. why do we need this mechanism? What was the logic behind? These are all questions people might have when checking this commit. Please be a bit more explicative. Agree, the commit message is bit short. I will add more content in next revision. you don't need to send a new version just for this commit log. You could just propose a new commit log in the reply and if it's OK, add it before pushing it. Let me know what do you think about: Add a mechanism to preserve existing data when creating a TTM object with the I915_BO_ALLOC_USER flag. This will be used in the subsequent patch where the I915_BO_ALLOC_USER flag will be applied to the framebuffer object. For a pre-allocated framebuffer without the I915_BO_PREALLOC flag, TTM would clear the content, which is not desirable. Thanks, Nirmoy As you wish. Andi Cc: Matthew Auld Cc: Andi Shyti Cc: Andrzej Hajda Cc: Ville Syrjälä Cc: Jani Nikula Cc: Imre Deak Signed-off-by: Nirmoy Das Reviewed-by: Andi Shyti Thanks, Nirmoy Thanks, Andi
Re: [PATCH 1/5] drm/i915/ttm: Add I915_BO_PREALLOC
Hi Nirmoy, > > > Add a mechanism to keep existing data when creating > > > a ttm object with I915_BO_ALLOC_USER flag. > > why do we need this mechanism? What was the logic behind? These > > are all questions people might have when checking this commit. > > Please be a bit more explicative. > > > Agree, the commit message is bit short. I will add more content in next > revision. you don't need to send a new version just for this commit log. You could just propose a new commit log in the reply and if it's OK, add it before pushing it. As you wish. Andi > > > > > Cc: Matthew Auld > > > Cc: Andi Shyti > > > Cc: Andrzej Hajda > > > Cc: Ville Syrjälä > > > Cc: Jani Nikula > > > Cc: Imre Deak > > > Signed-off-by: Nirmoy Das > > Reviewed-by: Andi Shyti > > > Thanks, > > Nirmoy > > > > > Thanks, > > Andi
Re: [PATCH 1/5] drm/i915/ttm: Add I915_BO_PREALLOC
On 4/4/2023 6:23 PM, Andi Shyti wrote: Hi Nirmoy, On Tue, Apr 04, 2023 at 04:30:56PM +0200, Nirmoy Das wrote: Add a mechanism to keep existing data when creating a ttm object with I915_BO_ALLOC_USER flag. why do we need this mechanism? What was the logic behind? These are all questions people might have when checking this commit. Please be a bit more explicative. Agree, the commit message is bit short. I will add more content in next revision. Cc: Matthew Auld Cc: Andi Shyti Cc: Andrzej Hajda Cc: Ville Syrjälä Cc: Jani Nikula Cc: Imre Deak Signed-off-by: Nirmoy Das Reviewed-by: Andi Shyti Thanks, Nirmoy Thanks, Andi
Re: [PATCH 1/5] drm/i915/ttm: Add I915_BO_PREALLOC
On 4/4/2023 5:30 PM, Andrzej Hajda wrote:
On 04.04.2023 16:30, Nirmoy Das wrote:
Add a mechanism to keep existing data when creating
a ttm object with I915_BO_ALLOC_USER flag.
Cc: Matthew Auld
Cc: Andi Shyti
Cc: Andrzej Hajda
Cc: Ville Syrjälä
Cc: Jani Nikula
Cc: Imre Deak
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 15 +++
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 5 +++--
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index 5dcbbef31d44..830c11431ee8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -328,6 +328,12 @@ struct drm_i915_gem_object {
*/
#define I915_BO_ALLOC_GPU_ONLY BIT(6)
#define I915_BO_ALLOC_CCS_AUX BIT(7)
+/*
+ * Object is allowed to retain its initial data and will not be
cleared on first
+ * access if used along with I915_BO_ALLOC_USER. This is mainly to keep
+ * preallocated framebuffer data intact while transitioning it to
i915drmfb.
+ */
+#define I915_BO_PREALLOC BIT(8)
#define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \
I915_BO_ALLOC_VOLATILE | \
I915_BO_ALLOC_CPU_CLEAR | \
@@ -335,10 +341,11 @@ struct drm_i915_gem_object {
I915_BO_ALLOC_PM_VOLATILE | \
I915_BO_ALLOC_PM_EARLY | \
I915_BO_ALLOC_GPU_ONLY | \
- I915_BO_ALLOC_CCS_AUX)
-#define I915_BO_READONLY BIT(8)
-#define I915_TILING_QUIRK_BIT 9 /* unknown swizzling; do not
release! */
-#define I915_BO_PROTECTED BIT(10)
+ I915_BO_ALLOC_CCS_AUX | \
+ I915_BO_PREALLOC)
+#define I915_BO_READONLY BIT(9)
+#define I915_TILING_QUIRK_BIT 10 /* unknown swizzling; do not
release! */
+#define I915_BO_PROTECTED BIT(11)
/**
* @mem_flags - Mutable placement-related flags
*
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
index dd188dfcc423..69eb20ed4d47 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
@@ -576,7 +576,7 @@ int i915_ttm_move(struct ttm_buffer_object *bo,
bool evict,
struct dma_fence *migration_fence = NULL;
struct ttm_tt *ttm = bo->ttm;
struct i915_refct_sgt *dst_rsgt;
- bool clear;
+ bool clear, prealloc_bo;
int ret;
if (GEM_WARN_ON(i915_ttm_is_ghost_object(bo))) {
@@ -632,7 +632,8 @@ int i915_ttm_move(struct ttm_buffer_object *bo,
bool evict,
return PTR_ERR(dst_rsgt);
clear = !i915_ttm_cpu_maps_iomem(bo->resource) && (!ttm ||
!ttm_tt_is_populated(ttm));
- if (!(clear && ttm && !(ttm->page_flags &
TTM_TT_FLAG_ZERO_ALLOC))) {
+ prealloc_bo = obj->flags & I915_BO_PREALLOC;
+ if (!(clear && ttm && !((ttm->page_flags &
TTM_TT_FLAG_ZERO_ALLOC) && !prealloc_bo))) {
This looks like school exercise for complicated usage of logical
operators, and I have problem with understanding this :)
Couldn't this be somehow simplified?
(I thought I sent this email yesterday but was stuck in oAuth pop up
sign-in)
Yes, this can be improved I think, took me while too.
Anyway as the patch just reuses existing code:
Reviewed-by: Andrzej Hajda
Thanks Andrzej,
Nirmoy
Regards
Andrzej
struct i915_deps deps;
i915_deps_init(&deps, GFP_KERNEL | __GFP_NORETRY |
__GFP_NOWARN);
Re: [PATCH 1/5] drm/i915/ttm: Add I915_BO_PREALLOC
Hi Nirmoy, On Tue, Apr 04, 2023 at 04:30:56PM +0200, Nirmoy Das wrote: > Add a mechanism to keep existing data when creating > a ttm object with I915_BO_ALLOC_USER flag. why do we need this mechanism? What was the logic behind? These are all questions people might have when checking this commit. Please be a bit more explicative. > Cc: Matthew Auld > Cc: Andi Shyti > Cc: Andrzej Hajda > Cc: Ville Syrjälä > Cc: Jani Nikula > Cc: Imre Deak > Signed-off-by: Nirmoy Das Reviewed-by: Andi Shyti Thanks, Andi
Re: [PATCH 1/5] drm/i915/ttm: Add I915_BO_PREALLOC
On 04.04.2023 16:30, Nirmoy Das wrote:
Add a mechanism to keep existing data when creating
a ttm object with I915_BO_ALLOC_USER flag.
Cc: Matthew Auld
Cc: Andi Shyti
Cc: Andrzej Hajda
Cc: Ville Syrjälä
Cc: Jani Nikula
Cc: Imre Deak
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 15 +++
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 5 +++--
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index 5dcbbef31d44..830c11431ee8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -328,6 +328,12 @@ struct drm_i915_gem_object {
*/
#define I915_BO_ALLOC_GPU_ONLY BIT(6)
#define I915_BO_ALLOC_CCS_AUX BIT(7)
+/*
+ * Object is allowed to retain its initial data and will not be cleared on
first
+ * access if used along with I915_BO_ALLOC_USER. This is mainly to keep
+ * preallocated framebuffer data intact while transitioning it to i915drmfb.
+ */
+#define I915_BO_PREALLOC BIT(8)
#define I915_BO_ALLOC_FLAGS (I915_BO_ALLOC_CONTIGUOUS | \
I915_BO_ALLOC_VOLATILE | \
I915_BO_ALLOC_CPU_CLEAR | \
@@ -335,10 +341,11 @@ struct drm_i915_gem_object {
I915_BO_ALLOC_PM_VOLATILE | \
I915_BO_ALLOC_PM_EARLY | \
I915_BO_ALLOC_GPU_ONLY | \
-I915_BO_ALLOC_CCS_AUX)
-#define I915_BO_READONLY BIT(8)
-#define I915_TILING_QUIRK_BIT 9 /* unknown swizzling; do not release! */
-#define I915_BO_PROTECTED BIT(10)
+I915_BO_ALLOC_CCS_AUX | \
+I915_BO_PREALLOC)
+#define I915_BO_READONLY BIT(9)
+#define I915_TILING_QUIRK_BIT 10 /* unknown swizzling; do not release! */
+#define I915_BO_PROTECTED BIT(11)
/**
* @mem_flags - Mutable placement-related flags
*
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
index dd188dfcc423..69eb20ed4d47 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
@@ -576,7 +576,7 @@ int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
struct dma_fence *migration_fence = NULL;
struct ttm_tt *ttm = bo->ttm;
struct i915_refct_sgt *dst_rsgt;
- bool clear;
+ bool clear, prealloc_bo;
int ret;
if (GEM_WARN_ON(i915_ttm_is_ghost_object(bo))) {
@@ -632,7 +632,8 @@ int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
return PTR_ERR(dst_rsgt);
clear = !i915_ttm_cpu_maps_iomem(bo->resource) && (!ttm || !ttm_tt_is_populated(ttm));
- if (!(clear && ttm && !(ttm->page_flags & TTM_TT_FLAG_ZERO_ALLOC))) {
+ prealloc_bo = obj->flags & I915_BO_PREALLOC;
+ if (!(clear && ttm && !((ttm->page_flags & TTM_TT_FLAG_ZERO_ALLOC) &&
!prealloc_bo))) {
This looks like school exercise for complicated usage of logical
operators, and I have problem with understanding this :)
Couldn't this be somehow simplified?
Anyway as the patch just reuses existing code:
Reviewed-by: Andrzej Hajda
Regards
Andrzej
struct i915_deps deps;
i915_deps_init(&deps, GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
