| Message ID | 20220303180013.512219-5-balasubramani.vivekanandan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Use the memcpy_from_wc function from drm | expand |
On Thu, Mar 03, 2022 at 11:30:10PM +0530, Balasubramani Vivekanandan wrote: >memcpy_from_wc functions in i915_memcpy.c will be removed and replaced >by the implementation in drm_cache.c. >Updated to use the functions provided by drm_cache.c. > >v2: Check if the log object allocated from local memory or system memory > and according setup the iosys_map (Lucas) > >Cc: Lucas De Marchi <lucas.demarchi@intel.com> > >Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com> >--- > drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > >diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c >index a24dc6441872..b9db765627ea 100644 >--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c >+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c >@@ -3,6 +3,7 @@ > * Copyright © 2014-2019 Intel Corporation > */ > >+#include <drm/drm_cache.h> > #include <linux/debugfs.h> > #include <linux/string_helpers.h> > >@@ -206,6 +207,7 @@ static void guc_read_update_log_buffer(struct intel_guc_log *log) > enum guc_log_buffer_type type; > void *src_data, *dst_data; > bool new_overflow; >+ struct iosys_map src_map; > > mutex_lock(&log->relay.lock); > >@@ -282,14 +284,21 @@ static void guc_read_update_log_buffer(struct intel_guc_log *log) > } > > /* Just copy the newly written data */ >+ if (i915_gem_object_is_lmem(log->vma->obj)) >+ iosys_map_set_vaddr_iomem(&src_map, (void __iomem *)src_data); >+ else >+ iosys_map_set_vaddr(&src_map, src_data); It would be better to keep this outside of the loop. So inside the loop we can use only iosys_map_incr(&src_map, buffer_size). However you'd also have to handle the read_offset. The iosys_map_ API has both a src_offset and dst_offset due to situations like that. Maybe this is missing in the new drm_memcpy_* function you're adding? This function was not correct wrt to IO memory access with the other 2 places in this function doing plain memcpy(). Since we are starting to use iosys_map here, we probably should handle this commit as "migrate to iosys_map", and convert those. In your current final state we have 3 variables aliasing the same memory location. IMO it will be error prone to keep it like that +Michal, some questions: - I'm not very familiar with the relayfs API. Is the `dst_data += PAGE_SIZE;` really correct? - Could you double check this patch and ack if ok? Heads up that since the log buffer is potentially in lmem, we will need to convert this function to take that into account. All those accesses to log_buf_state need to use the proper kernel abstraction for system vs I/O memory. thanks Lucas De Marchi >+ > if (read_offset > write_offset) { >- i915_memcpy_from_wc(dst_data, src_data, write_offset); >+ drm_memcpy_from_wc_vaddr(dst_data, &src_map, >+ write_offset); > bytes_to_copy = buffer_size - read_offset; > } else { > bytes_to_copy = write_offset - read_offset; > } >- i915_memcpy_from_wc(dst_data + read_offset, >- src_data + read_offset, bytes_to_copy); >+ iosys_map_incr(&src_map, read_offset); >+ drm_memcpy_from_wc_vaddr(dst_data + read_offset, &src_map, >+ bytes_to_copy); > > src_data += buffer_size; > dst_data += buffer_size; >-- >2.25.1 >
On 21.03.2022 22:14, Lucas De Marchi wrote: > On Thu, Mar 03, 2022 at 11:30:10PM +0530, Balasubramani Vivekanandan wrote: >> memcpy_from_wc functions in i915_memcpy.c will be removed and replaced >> by the implementation in drm_cache.c. >> Updated to use the functions provided by drm_cache.c. >> >> v2: Check if the log object allocated from local memory or system memory >> and according setup the iosys_map (Lucas) >> >> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >> >> Signed-off-by: Balasubramani Vivekanandan >> <balasubramani.vivekanandan@intel.com> >> --- >> drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 15 ++++++++++++--- >> 1 file changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c >> b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c >> index a24dc6441872..b9db765627ea 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c >> @@ -3,6 +3,7 @@ >> * Copyright © 2014-2019 Intel Corporation >> */ >> >> +#include <drm/drm_cache.h> >> #include <linux/debugfs.h> >> #include <linux/string_helpers.h> >> >> @@ -206,6 +207,7 @@ static void guc_read_update_log_buffer(struct >> intel_guc_log *log) >> enum guc_log_buffer_type type; >> void *src_data, *dst_data; >> bool new_overflow; >> + struct iosys_map src_map; >> >> mutex_lock(&log->relay.lock); >> >> @@ -282,14 +284,21 @@ static void guc_read_update_log_buffer(struct >> intel_guc_log *log) >> } >> >> /* Just copy the newly written data */ >> + if (i915_gem_object_is_lmem(log->vma->obj)) >> + iosys_map_set_vaddr_iomem(&src_map, (void __iomem >> *)src_data); >> + else >> + iosys_map_set_vaddr(&src_map, src_data); > > It would be better to keep this outside of the loop. So inside the loop > we can use only iosys_map_incr(&src_map, buffer_size). However you'd > also have to handle the read_offset. The iosys_map_ API has both a > src_offset and dst_offset due to situations like that. Maybe this is > missing in the new drm_memcpy_* function you're adding? > > This function was not correct wrt to IO memory access with the other > 2 places in this function doing plain memcpy(). Since we are starting to > use iosys_map here, we probably should handle this commit as "migrate to > iosys_map", and convert those. In your current final state > we have 3 variables aliasing the same memory location. IMO it will be > error prone to keep it like that > > +Michal, some questions: @Lucas, better to ask Alan who is making some changes around GuC log @Alan, can you help answer below questions? thanks, Michal > > - I'm not very familiar with the relayfs API. Is the `dst_data += > PAGE_SIZE;` > really correct? > > - Could you double check this patch and ack if ok? > > Heads up that since the log buffer is potentially in lmem, we will need > to convert this function to take that into account. All those accesses > to log_buf_state need to use the proper kernel abstraction for system vs > I/O memory. > > thanks > Lucas De Marchi > >> + >> if (read_offset > write_offset) { >> - i915_memcpy_from_wc(dst_data, src_data, write_offset); >> + drm_memcpy_from_wc_vaddr(dst_data, &src_map, >> + write_offset); >> bytes_to_copy = buffer_size - read_offset; >> } else { >> bytes_to_copy = write_offset - read_offset; >> } >> - i915_memcpy_from_wc(dst_data + read_offset, >> - src_data + read_offset, bytes_to_copy); >> + iosys_map_incr(&src_map, read_offset); >> + drm_memcpy_from_wc_vaddr(dst_data + read_offset, &src_map, >> + bytes_to_copy); >> >> src_data += buffer_size; >> dst_data += buffer_size; >> -- >> 2.25.1 >>
On 21.03.2022 14:14, Lucas De Marchi wrote: > On Thu, Mar 03, 2022 at 11:30:10PM +0530, Balasubramani Vivekanandan wrote: > > memcpy_from_wc functions in i915_memcpy.c will be removed and replaced > > by the implementation in drm_cache.c. > > Updated to use the functions provided by drm_cache.c. > > > > v2: Check if the log object allocated from local memory or system memory > > and according setup the iosys_map (Lucas) > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > > Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com> > > --- > > drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c > > index a24dc6441872..b9db765627ea 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c > > @@ -3,6 +3,7 @@ > > * Copyright © 2014-2019 Intel Corporation > > */ > > > > +#include <drm/drm_cache.h> > > #include <linux/debugfs.h> > > #include <linux/string_helpers.h> > > > > @@ -206,6 +207,7 @@ static void guc_read_update_log_buffer(struct intel_guc_log *log) > > enum guc_log_buffer_type type; > > void *src_data, *dst_data; > > bool new_overflow; > > + struct iosys_map src_map; > > > > mutex_lock(&log->relay.lock); > > > > @@ -282,14 +284,21 @@ static void guc_read_update_log_buffer(struct intel_guc_log *log) > > } > > > > /* Just copy the newly written data */ > > + if (i915_gem_object_is_lmem(log->vma->obj)) > > + iosys_map_set_vaddr_iomem(&src_map, (void __iomem *)src_data); > > + else > > + iosys_map_set_vaddr(&src_map, src_data); > > It would be better to keep this outside of the loop. So inside the loop > we can use only iosys_map_incr(&src_map, buffer_size). However you'd > also have to handle the read_offset. The iosys_map_ API has both a > src_offset and dst_offset due to situations like that. Maybe this is > missing in the new drm_memcpy_* function you're adding? > > This function was not correct wrt to IO memory access with the other > 2 places in this function doing plain memcpy(). Since we are starting to > use iosys_map here, we probably should handle this commit as "migrate to > iosys_map", and convert those. In your current final state > we have 3 variables aliasing the same memory location. IMO it will be > error prone to keep it like that yes, it is a good suggestion to completely change the reading of the GuC log for the relay to use the iosys_map interfaces. Though it was planned eventually, doing it now with this series will avoid mixing of memcpy() and drm_memcpy_*(which needs iosys_map parameters) functions. I will do the changes. > > +Michal, some questions: > > - I'm not very familiar with the relayfs API. Is the `dst_data += PAGE_SIZE;` > really correct? > > - Could you double check this patch and ack if ok? > > Heads up that since the log buffer is potentially in lmem, we will need > to convert this function to take that into account. All those accesses > to log_buf_state need to use the proper kernel abstraction for system vs > I/O memory. > > thanks > Lucas De Marchi > > > + > > if (read_offset > write_offset) { > > - i915_memcpy_from_wc(dst_data, src_data, write_offset); > > + drm_memcpy_from_wc_vaddr(dst_data, &src_map, > > + write_offset); > > bytes_to_copy = buffer_size - read_offset; > > } else { > > bytes_to_copy = write_offset - read_offset; > > } > > - i915_memcpy_from_wc(dst_data + read_offset, > > - src_data + read_offset, bytes_to_copy); > > + iosys_map_incr(&src_map, read_offset); > > + drm_memcpy_from_wc_vaddr(dst_data + read_offset, &src_map, > > + bytes_to_copy); > > > > src_data += buffer_size; > > dst_data += buffer_size; > > -- > > 2.25.1 > >
On 3/21/2022 2:14 PM, Lucas De Marchi wrote: > On Thu, Mar 03, 2022 at 11:30:10PM +0530, Balasubramani Vivekanandan > wrote: >> memcpy_from_wc functions in i915_memcpy.c will be removed and replaced >> by the implementation in drm_cache.c. >> Updated to use the functions provided by drm_cache.c. >> >> v2: Check if the log object allocated from local memory or system memory >> and according setup the iosys_map (Lucas) >> >> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >> >> Signed-off-by: Balasubramani Vivekanandan >> <balasubramani.vivekanandan@intel.com> >> --- >> drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 15 ++++++++++++--- >> 1 file changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c >> b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c >> index a24dc6441872..b9db765627ea 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c >> @@ -3,6 +3,7 @@ >> * Copyright © 2014-2019 Intel Corporation >> */ >> >> +#include <drm/drm_cache.h> >> #include <linux/debugfs.h> >> #include <linux/string_helpers.h> >> >> @@ -206,6 +207,7 @@ static void guc_read_update_log_buffer(struct >> intel_guc_log *log) >> enum guc_log_buffer_type type; >> void *src_data, *dst_data; >> bool new_overflow; >> + struct iosys_map src_map; >> >> mutex_lock(&log->relay.lock); >> >> @@ -282,14 +284,21 @@ static void guc_read_update_log_buffer(struct >> intel_guc_log *log) >> } >> >> /* Just copy the newly written data */ >> + if (i915_gem_object_is_lmem(log->vma->obj)) >> + iosys_map_set_vaddr_iomem(&src_map, (void __iomem >> *)src_data); >> + else >> + iosys_map_set_vaddr(&src_map, src_data); > > It would be better to keep this outside of the loop. So inside the loop > we can use only iosys_map_incr(&src_map, buffer_size). However you'd > also have to handle the read_offset. The iosys_map_ API has both a > src_offset and dst_offset due to situations like that. Maybe this is > missing in the new drm_memcpy_* function you're adding? > > This function was not correct wrt to IO memory access with the other > 2 places in this function doing plain memcpy(). Since we are starting to > use iosys_map here, we probably should handle this commit as "migrate to > iosys_map", and convert those. In your current final state > we have 3 variables aliasing the same memory location. IMO it will be > error prone to keep it like that > > +Michal, some questions: > > - I'm not very familiar with the relayfs API. Is the `dst_data += > PAGE_SIZE;` > really correct? This is a bit weird due to how i915 uses the relay for the GuC logs, but it should be functionally correct. Each relay buffer is the same size of the GuC log buffer in i915 (which is guaranteed to be greater than PAGE_SIZE) and we always switch to a new relay buffer each time we dump new data, so we're guaranteed to have the space we need. We do some pointer magic because instead of just blindly copying the whole local log buffer to the relay buffer, we copy the header (which is in the first page) first, then we copy the rest of the logs (2nd page and onwards) based on what the header tells us has been filled out. > > - Could you double check this patch and ack if ok? The approach looks good to me, but I agree that at this point we might as well do a full conversion to iosys map. As you already mentioned, the memcpy that copies the header would also need to be updated for that, because it accesses the same memory as src_data, while the other memcpy is from the local copy of the header to the relay, so it should be safe to not convert. Daniele > > Heads up that since the log buffer is potentially in lmem, we will need > to convert this function to take that into account. All those accesses > to log_buf_state need to use the proper kernel abstraction for system vs > I/O memory. > > thanks > Lucas De Marchi > >> + >> if (read_offset > write_offset) { >> - i915_memcpy_from_wc(dst_data, src_data, write_offset); >> + drm_memcpy_from_wc_vaddr(dst_data, &src_map, >> + write_offset); >> bytes_to_copy = buffer_size - read_offset; >> } else { >> bytes_to_copy = write_offset - read_offset; >> } >> - i915_memcpy_from_wc(dst_data + read_offset, >> - src_data + read_offset, bytes_to_copy); >> + iosys_map_incr(&src_map, read_offset); >> + drm_memcpy_from_wc_vaddr(dst_data + read_offset, &src_map, >> + bytes_to_copy); >> >> src_data += buffer_size; >> dst_data += buffer_size; >> -- >> 2.25.1 >>
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c index a24dc6441872..b9db765627ea 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c @@ -3,6 +3,7 @@ * Copyright © 2014-2019 Intel Corporation */ +#include <drm/drm_cache.h> #include <linux/debugfs.h> #include <linux/string_helpers.h> @@ -206,6 +207,7 @@ static void guc_read_update_log_buffer(struct intel_guc_log *log) enum guc_log_buffer_type type; void *src_data, *dst_data; bool new_overflow; + struct iosys_map src_map; mutex_lock(&log->relay.lock); @@ -282,14 +284,21 @@ static void guc_read_update_log_buffer(struct intel_guc_log *log) } /* Just copy the newly written data */ + if (i915_gem_object_is_lmem(log->vma->obj)) + iosys_map_set_vaddr_iomem(&src_map, (void __iomem *)src_data); + else + iosys_map_set_vaddr(&src_map, src_data); + if (read_offset > write_offset) { - i915_memcpy_from_wc(dst_data, src_data, write_offset); + drm_memcpy_from_wc_vaddr(dst_data, &src_map, + write_offset); bytes_to_copy = buffer_size - read_offset; } else { bytes_to_copy = write_offset - read_offset; } - i915_memcpy_from_wc(dst_data + read_offset, - src_data + read_offset, bytes_to_copy); + iosys_map_incr(&src_map, read_offset); + drm_memcpy_from_wc_vaddr(dst_data + read_offset, &src_map, + bytes_to_copy); src_data += buffer_size; dst_data += buffer_size;
memcpy_from_wc functions in i915_memcpy.c will be removed and replaced by the implementation in drm_cache.c. Updated to use the functions provided by drm_cache.c. v2: Check if the log object allocated from local memory or system memory and according setup the iosys_map (Lucas) Cc: Lucas De Marchi <lucas.demarchi@intel.com> Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)