From ec1a4ac88e6e1cbbfcbf393fede5b7b492fbce2b Mon Sep 17 00:00:00 2001 From: Hanno Schwalm Date: Mon, 6 Jul 2026 18:28:29 +0200 Subject: [PATCH] Some OpenCL pixelpipe reorganising for efficacy Whenever we have to OpenCL transform colorspace for the module's input we ensure it's written back to host memory, the cacheline dsc is updated and the fast blending cache is cleared. By doing so input cacheline invalidations are not required any more. As we also want input cl_mem data to be available as a host bound cacheline if there are importance hints - we do this to increase cache hit rate for UI responsiveness - or if we later have to go into tiling mode, we do an early test for these cases and combine device to host copy with those for colorspace transforms for fewer costly calls. Activate DT_PIPE_CAS_SHUTDOWN, now we only can set shutdown mode once per pixelpipe run. Also some shutdown log improvements, we want this via -d pipe only if relevant & new, otherwise the -d verbose switch must be used. Some overhaul of _module_pipe_stop() and it's callers as being sure about write backs. For some shutdown modes we would like to restart a OpenCL pipe at the stopping module. This requires further information for the caller so we now return dt_dev_stoptest_t instead of a gboolean. Fixes: The pixelpipe CPU path missed the _module_pipe_stop() In pixelpipe cache code we - missed setting the DT_INVALID_HASH - did a bad compare while freeing invalid cachelines resulting in less efficient use of cachelines. We also allocate cachelines with a size aligned to 4k for slightly improved perf. Also some log improvements here. Some maintenance for less parameters and readability - get_output_format() doesn't need dev as parameter - _piece_process_hash() renamed to _bcache_hash() for clarity, removed 'module' parameter - _piece_fast_blend() got the 'module' parameter removed - introduced and use _copy_image_to_host_err() for readability. To be investigated in OpenCL pipe: We currently request a host-bound cacheline for every module leading to lots of alloc-here and invalidate-in-next-module if there was no write back to host cacheline. Ideas: 1. late cacheline allocation? 2. propagate unused cacheline to next module? --- src/develop/pixelpipe_cache.c | 71 +++--- src/develop/pixelpipe_cache.h | 8 +- src/develop/pixelpipe_hb.c | 437 ++++++++++++++++------------------ src/develop/pixelpipe_hb.h | 26 +- 4 files changed, 260 insertions(+), 282 deletions(-) diff --git a/src/develop/pixelpipe_cache.c b/src/develop/pixelpipe_cache.c index 80923dd4473c..223fa47fb923 100644 --- a/src/develop/pixelpipe_cache.c +++ b/src/develop/pixelpipe_cache.c @@ -23,11 +23,6 @@ #include "libs/colorpicker.h" #include -static inline int _to_mb(size_t m) -{ - return (int)((m + 0x80000lu) / 0x400lu / 0x400lu); -} - gboolean dt_dev_pixelpipe_cache_init(dt_dev_pixelpipe_t *pipe, const int entries, const size_t size, @@ -36,7 +31,7 @@ gboolean dt_dev_pixelpipe_cache_init(dt_dev_pixelpipe_t *pipe, dt_dev_pixelpipe_cache_t *cache = &pipe->cache; cache->entries = entries; - cache->allmem = cache->hits = cache->calls = cache->tests = 0; + cache->allmem = cache->max_allmem = cache->hits = cache->calls = cache->tests = 0; cache->memlimit = limit; const size_t csize = sizeof(void *) + sizeof(size_t) + sizeof(dt_iop_buffer_dsc_t) + 2*sizeof(int32_t) + sizeof(uint64_t); @@ -86,9 +81,10 @@ void dt_dev_pixelpipe_cache_cleanup(dt_dev_pixelpipe_t *pipe) if(dt_pipe_is_full(pipe)) { - dt_print(DT_DEBUG_PIPE, "Session fullpipe cache report. hits/run=%.2f, hits/test=%.3f", - (double)(cache->hits) / fmax(1.0, pipe->runs), - (double)(cache->hits) / fmax(1.0, cache->tests)); + dt_print(DT_DEBUG_PIPE, "Session fullpipe cache report. Maximum=%zuMB. hits/run=%.2f, hits/test=%.3f", + cache->max_allmem / DT_MEGA, + (double)(cache->hits) / fmax(1.0, pipe->runs), + (double)(cache->hits) / fmax(1.0, cache->tests)); } for(int k = 0; k < cache->entries; k++) @@ -332,7 +328,7 @@ gboolean dt_dev_pixelpipe_cache_get(dt_dev_pixelpipe_t *pipe, { dt_free_align(cache->data[cline]); cache->allmem -= cache->size[cline]; - cache->data[cline] = (void *)dt_alloc_aligned(size); + cache->data[cline] = (void *)dt_alloc_aligned(dt_round_size(size, 4096)); if(cache->data[cline]) { cache->size[cline] = size; @@ -368,7 +364,7 @@ gboolean dt_dev_pixelpipe_cache_get(dt_dev_pixelpipe_t *pipe, return TRUE; } -static void _mark_invalid_cacheline(const dt_dev_pixelpipe_cache_t *cache, const int k) +static inline void _mark_invalid_cacheline(const dt_dev_pixelpipe_cache_t *cache, const int k) { cache->hash[k] = DT_INVALID_HASH; cache->ioporder[k] = 0; @@ -438,25 +434,38 @@ static size_t _free_cacheline(dt_dev_pixelpipe_cache_t *cache, const int k) cache->allmem -= removed; cache->size[k] = 0; cache->data[k] = NULL; - _mark_invalid_cacheline(cache, k); + cache->hash[k] = DT_INVALID_HASH; + cache->ioporder[k] = 0; return removed; } -static void _cline_stats(dt_dev_pixelpipe_cache_t *cache) +static inline int _used(const dt_dev_pixelpipe_cache_t *cache) { - cache->lused = cache->linvalid = cache->limportant = 0; + int cnt = 0; for(int k = DT_PIPECACHE_MIN; k < cache->entries; k++) - { - if(cache->data[k]) cache->lused++; - if(cache->data[k] && (cache->hash[k] == DT_INVALID_HASH)) cache->linvalid++; - if(cache->used[k] < 0) cache->limportant++; - } + if(cache->data[k] && cache->hash[k] != DT_INVALID_HASH) cnt++; + return cnt; +} +static inline int _important(const dt_dev_pixelpipe_cache_t *cache) +{ + int cnt = 0; + for(int k = DT_PIPECACHE_MIN; k < cache->entries; k++) + if(cache->used[k] < 0 && cache->data[k] && cache->hash[k] != DT_INVALID_HASH) cnt++; + return cnt; +} +static inline int _invalid(const dt_dev_pixelpipe_cache_t *cache) +{ + int cnt = 0; + for(int k = DT_PIPECACHE_MIN; k < cache->entries; k++) + if(cache->data[k] && cache->hash[k] == DT_INVALID_HASH) cnt++; + return cnt; } void dt_dev_pixelpipe_cache_checkmem(dt_dev_pixelpipe_t *pipe) { dt_dev_pixelpipe_cache_t *cache = &pipe->cache; + cache->max_allmem = MAX(cache->max_allmem, cache->allmem); // we have pixelpipes like export & thumbnail that just use // alternating buffers so no cleanup if(cache->entries == DT_PIPECACHE_MIN) return; @@ -464,10 +473,15 @@ void dt_dev_pixelpipe_cache_checkmem(dt_dev_pixelpipe_t *pipe) // We always free cachelines marked as not valid size_t freed = 0; size_t freed_invalid = 0; + int free_cnt = 0; + int free_invalid_cnt = 0; for(int k = DT_PIPECACHE_MIN; k < cache->entries; k++) { - if((cache->hash[k] == DT_INVALID_HASH) && cache->data) + if(cache->hash[k] == DT_INVALID_HASH && cache->data[k]) + { freed_invalid += _free_cacheline(cache, k); + free_invalid_cnt++; + } } while(cache->memlimit && (cache->memlimit < cache->allmem)) @@ -476,24 +490,22 @@ void dt_dev_pixelpipe_cache_checkmem(dt_dev_pixelpipe_t *pipe) if(k == 0) break; freed += _free_cacheline(cache, k); + free_cnt++; } - _cline_stats(cache); dt_print_pipe(DT_DEBUG_PIPE | DT_DEBUG_MEMORY, "pipe cache check", pipe, NULL, DT_DEVICE_NONE, NULL, NULL, - "%i lines (important=%i, used=%i). Freed: invalid %iMB used %iMB. Using %iMB, limit=%iMB", - cache->entries, cache->limportant, cache->lused, - _to_mb(freed_invalid), _to_mb(freed), _to_mb(cache->allmem), _to_mb(cache->memlimit)); + "Freed lines invalid=%i used=%i. Freed mem invalid %zuMB used %zuMB. Now %zuMB limit=%zuMB max=%zuMB", + free_invalid_cnt, free_cnt, freed_invalid/DT_MEGA, freed / DT_MEGA, + cache->allmem / DT_MEGA, cache->memlimit / DT_MEGA, cache->max_allmem / DT_MEGA); } void dt_dev_pixelpipe_cache_report(dt_dev_pixelpipe_t *pipe) { dt_dev_pixelpipe_cache_t *cache = &pipe->cache; - - _cline_stats(cache); dt_print_pipe(DT_DEBUG_PIPE | DT_DEBUG_MEMORY, "cache report", pipe, NULL, DT_DEVICE_NONE, NULL, NULL, - "%i lines (important=%i, used=%i, invalid=%i). Using %iMB, limit=%iMB. Hits/run=%.2f. Hits/test=%.3f", - cache->entries, cache->limportant, cache->lused, cache->linvalid, - _to_mb(cache->allmem), _to_mb(cache->memlimit), + "Lines=%i important=%i used=%i invalid=%i. Now=%zuMB limit=%zuMB max=%zuMB. Hits/run=%.2f. Hits/test=%.3f", + cache->entries, _important(cache), _used(cache), _invalid(cache), + cache->allmem / DT_MEGA, cache->memlimit / DT_MEGA, cache->max_allmem / DT_MEGA, (double)(cache->hits) / fmax(1.0, pipe->runs), (double)(cache->hits) / fmax(1.0, cache->tests)); } @@ -503,4 +515,3 @@ void dt_dev_pixelpipe_cache_report(dt_dev_pixelpipe_t *pipe) // vim: shiftwidth=2 expandtab tabstop=2 cindent // kate: tab-indents: off; indent-width 2; replace-tabs on; indent-mode cstyle; remove-trailing-spaces modified; // clang-format on - diff --git a/src/develop/pixelpipe_cache.h b/src/develop/pixelpipe_cache.h index d91ed4c8d2c8..2e2362858603 100644 --- a/src/develop/pixelpipe_cache.h +++ b/src/develop/pixelpipe_cache.h @@ -28,12 +28,13 @@ struct dt_iop_roi_t; * implements a simple pixel cache suitable for caching float images * corresponding to history items and zoom/pan settings in the develop module. * correctness is secured via the hash so make sure everything is included here. - * No caching if cl_mem, instead copied cache buffers are used. + * No caching cl_mem, instead copied cache buffers are used. */ typedef struct dt_dev_pixelpipe_cache_t { int32_t entries; size_t allmem; + size_t max_allmem; size_t memlimit; void **data; size_t *size; @@ -43,12 +44,9 @@ typedef struct dt_dev_pixelpipe_cache_t int32_t *ioporder; uint64_t calls; int32_t lastline; - // profiling & stats: + // profiling uint64_t tests; uint64_t hits; - uint32_t lused; - uint32_t linvalid; - uint32_t limportant; } dt_dev_pixelpipe_cache_t; typedef enum dt_dev_pixelpipe_cache_test_t diff --git a/src/develop/pixelpipe_hb.c b/src/develop/pixelpipe_hb.c index beb4f4004ad6..06d99130ef6e 100644 --- a/src/develop/pixelpipe_hb.c +++ b/src/develop/pixelpipe_hb.c @@ -321,7 +321,6 @@ size_t dt_get_available_pipe_mem(const dt_dev_pixelpipe_t *pipe) static void get_output_format(dt_iop_module_t *module, dt_dev_pixelpipe_t *pipe, dt_dev_pixelpipe_iop_t *piece, - dt_develop_t *dev, dt_iop_buffer_dsc_t *dsc) { if(module) return module->output_format(module, pipe, piece, dsc); @@ -348,7 +347,7 @@ void dt_dev_pixelpipe_set_input(dt_dev_pixelpipe_t *pipe, pipe->iscale = iscale; pipe->input = input; pipe->image = dev->image_storage; - get_output_format(NULL, pipe, NULL, dev, &pipe->dsc); + get_output_format(NULL, pipe, NULL, &pipe->dsc); } void dt_dev_pixelpipe_set_icc(dt_dev_pixelpipe_t *pipe, @@ -363,7 +362,7 @@ void dt_dev_pixelpipe_set_icc(dt_dev_pixelpipe_t *pipe, } void dt_dev_pixelpipe_set_shutdown(dt_dev_pixelpipe_t *pipe, - const dt_dev_pixelpipe_stopper_t stopper) + const dt_dev_pixelpipe_stopper_t stopper) { #ifndef DT_PIPE_CAS_SHUTDOWN @@ -371,16 +370,22 @@ void dt_dev_pixelpipe_set_shutdown(dt_dev_pixelpipe_t *pipe, dt_print_pipe(DT_DEBUG_PIPE | DT_DEBUG_VERBOSE, "pipe shutdown request", pipe, NULL, pipe->devid, NULL, NULL, "%s", dt_dev_pixelpipe_shutdown_to_str(stopper)); - #else +#else int expected = DT_DEV_PIXELPIPE_STOP_NO; const gboolean new = dt_atomic_CAS_int(&pipe->shutdown, &expected, stopper); - dt_print_pipe(DT_DEBUG_PIPE | DT_DEBUG_VERBOSE, "pipe shutdown request", + if(new && pipe->processing) + dt_print_pipe(DT_DEBUG_PIPE, "pipe shutdown request", pipe, NULL, pipe->devid, NULL, NULL, - "%s %s", - new ? "" : "failed, was", - new ? dt_dev_pixelpipe_shutdown_to_str(stopper) - : dt_dev_pixelpipe_shutdown_to_str(expected)); + "%s", dt_dev_pixelpipe_shutdown_to_str(stopper)); + else + dt_print_pipe(DT_DEBUG_PIPE | DT_DEBUG_VERBOSE, "pipe shutdown request", + pipe, NULL, pipe->devid, NULL, NULL, + "to %s %s. was %s", + dt_dev_pixelpipe_shutdown_to_str(stopper), + expected == DT_DEV_PIXELPIPE_STOP_NO ? "ignored" : "failed", + dt_dev_pixelpipe_shutdown_to_str(expected)); + #endif } @@ -848,6 +853,23 @@ static void _histogram_collect(dt_dev_pixelpipe_iop_t *piece, } #ifdef HAVE_OPENCL +// helper for copy image to host with local error message +static int _copy_image_to_host_err(const int devid, + void *host, + cl_mem img, + const int wd, + const int ht, + const int bpp, + const char *msg) +{ + const size_t reg[2] = { wd, ht }; + const int err = dt_opencl_read_host_from_image_raw(devid, host, img, CLIMG_ORIGIN, reg, (size_t)wd * bpp, TRUE); + if(err != CL_SUCCESS) + dt_print(DT_DEBUG_OPENCL | DT_DEBUG_PIPE, "[dt_opencl_copy_image_to_host '%s'] devid=%d %dx%d bpp=%d failed: '%s'", + msg, devid, wd, ht, bpp, cl_errstr(err)); + return err; +} + // helper to get per module histogram for OpenCL // // this algorithm is inefficient as hell when it comes to larger @@ -873,13 +895,10 @@ static void _histogram_collect_cl(const int devid, if(!pixel) return; - if(dt_opencl_copy_image_to_host(devid, pixel, img, roi->width, roi->height, sizeof(float) * 4) + if(_copy_image_to_host_err(devid, pixel, img, roi->width, roi->height, sizeof(float) * 4, "histogramm collect") != CL_SUCCESS) { dt_free_align(tmpbuf); - dt_print_pipe(DT_DEBUG_PIPE | DT_DEBUG_OPENCL, "[histogram_collect]", - piece->pipe, piece->module, devid, roi, NULL, - "Couldn't read histogramm data"); return; } @@ -1224,11 +1243,11 @@ static void _collect_histogram_on_CPU(dt_dev_pixelpipe_t *pipe, As modules might change internal mask visualizing modes not visible via parameters we clear the blending cache line whenever we invalidate pixelpipe cache lines. */ -static inline dt_hash_t _piece_process_hash(const dt_dev_pixelpipe_iop_t *piece, - const dt_iop_roi_t *roi, - const dt_iop_module_t *module, - const int position) +static inline dt_hash_t _bcache_hash(const dt_dev_pixelpipe_iop_t *piece, + const dt_iop_roi_t *roi, + const int position) { + const dt_iop_module_t *module = piece->module; dt_hash_t phash = dt_dev_pixelpipe_cache_hash(roi, piece->pipe, position -1); phash = dt_hash(phash, roi, sizeof(dt_iop_roi_t)); phash = dt_hash(phash, &module->so->op, strlen(module->so->op)); @@ -1237,9 +1256,9 @@ static inline dt_hash_t _piece_process_hash(const dt_dev_pixelpipe_iop_t *piece, return phash; } -static inline gboolean _piece_fast_blend(const dt_dev_pixelpipe_iop_t *piece, - const dt_iop_module_t *module) +static inline gboolean _piece_fast_blend(const dt_dev_pixelpipe_iop_t *piece) { + const dt_iop_module_t *module = piece->module; return dt_pipe_is_canvas(piece->pipe) && darktable.pipe_cache && dt_iop_has_focus(module) @@ -1260,31 +1279,48 @@ static inline float *_get_fast_blendcache(const size_t nfloats, return pipe->bcache_data; } -/* use _module_pipe_stop to test for the just processed module leaving a flag to shutdown the pipeline - immediately. - This requires extra care as we want pipecache data after the module and it's input data to be invalidated. - All dt_dev_pixelpipe_stopper_t enums must be served. +/* use _module_pipe_stop to test for the just processed module leaving a dt_dev_stoptest_t + to shutdown the pipeline immediately. + All dt_dev_pixelpipe_stopper_t enums are served here, + taking care about cacheline invalidations. + In case of a returned DT_DEV_STOPTEST_SAVE we would like the cl_mem input written to the + cacheline if not done yet. */ -static inline gboolean _module_pipe_stop(dt_dev_pixelpipe_t *pipe, - const dt_iop_module_t *module, - float *input) + +typedef enum dt_dev_stoptest_t +{ + DT_DEV_STOPTEST_NO = 0, + DT_DEV_STOPTEST_SAVE, + DT_DEV_STOPTEST_IGNORE, +} dt_dev_stoptest_t; + +static inline dt_dev_stoptest_t _module_pipe_stop(dt_dev_pixelpipe_t *pipe, + const dt_iop_module_t *module, + float *incacheline, + float *outcacheline) { const dt_dev_pixelpipe_stopper_t stopper = dt_atomic_get_int(&pipe->shutdown); if(stopper <= DT_DEV_PIXELPIPE_STOP_NO) - return FALSE; + return DT_DEV_STOPTEST_NO; dt_print_pipe(DT_DEBUG_PIPE, "module pipe stop", pipe, module, pipe->devid, NULL, NULL, "%s", dt_dev_pixelpipe_shutdown_to_str(stopper)); - // These case don't require any caretaking of cachelines + dt_dev_pixelpipe_invalidate_cacheline(pipe, outcacheline); + if(stopper == DT_DEV_PIXELPIPE_STOP_NODES || stopper == DT_DEV_PIXELPIPE_STOP_HQ) - return TRUE; + return DT_DEV_STOPTEST_IGNORE; + + if(stopper <= DT_DEV_PIXELPIPE_STOP_LAST) + { + dt_dev_pixelpipe_invalidate_cacheline(pipe, incacheline); + return DT_DEV_STOPTEST_IGNORE; + } - // stopper reflects the iop_order of the stopping mode so we must invalidate. - dt_dev_pixelpipe_invalidate_cacheline(pipe, input); + // stopper reflects the iop_order of the stopping mode so we invalidate following cachelines dt_dev_pixelpipe_cache_invalidate_later(pipe, stopper, "module pipe stop: "); - return TRUE; + return DT_DEV_STOPTEST_SAVE; } void dt_dev_prepare_piece_cfa(dt_dev_pixelpipe_iop_t *piece, const dt_iop_roi_t *roi) @@ -1408,18 +1444,11 @@ static gboolean _pixelpipe_process_on_CPU(dt_dev_pixelpipe_t *pipe, roi_in->width, roi_in->height, cst_from, cst_to, &input_format->cst, work_profile); - /* We just have changed the input data if cst_from != cst_to so - the cacheline cst does not reflect the module input colorspace any more! - Note: in opencl code path this is different as the in-data colorspace conversion - is always done in cl_mem and thus does not affect pipecache. - - Possible ways to handle this: - 1. for now we just invalidate that cacheline so if the pipe is reprocessed we won't - use wrong data. - 2. we should implement code for the pipe cache that modifies the data cst. + /* We just might have in-place changed the input data (cst_from != cst_to). + Please note that cacheline dt_dev_pixelpipe_cache_t dsc has been updated + via &input_format->cst so a cache hit in a following pipe won't convert + again. */ - if(cst_from != cst_to) - dt_dev_pixelpipe_invalidate_cacheline(pipe, input); if(_pipe_has_shutdown(pipe)) return TRUE; @@ -1445,9 +1474,9 @@ static gboolean _pixelpipe_process_on_CPU(dt_dev_pixelpipe_t *pipe, TRUE, dt_dev_pixelpipe_type_to_str(pipe->type)); const size_t nfloats = bpp * roi_out->width * roi_out->height / sizeof(float); - const gboolean want_bcache = _piece_fast_blend(piece, module); + const gboolean want_bcache = _piece_fast_blend(piece); const dt_hash_t phash = want_bcache - ? _piece_process_hash(piece, roi_out, module, position) + ? _bcache_hash(piece, roi_out, position) : DT_INVALID_HASH; const gboolean bcaching = want_bcache ? pipe->bcache_data && phash == pipe->bcache_hash && phash != DT_INVALID_HASH @@ -1488,14 +1517,14 @@ static gboolean _pixelpipe_process_on_CPU(dt_dev_pixelpipe_t *pipe, { dt_print_pipe(DT_DEBUG_PIPE, bcaching ? "from blend cache" : "process", - pipe, module, DT_DEVICE_CPU, roi_in, roi_out, "%s%s%s%s %.fMB", + pipe, module, DT_DEVICE_CPU, roi_in, roi_out, "%s%s%s%s %zuMB", dt_iop_colorspace_to_name(cst_to), cst_to != cst_out ? " -> " : "", cst_to != cst_out ? dt_iop_colorspace_to_name(cst_out) : "", (fitting) ? "" : " Warning: processed without tiling even if memory requirements are not met", - 1e-6 * (tiling->factor * (m_width * m_height * m_bpp) + tiling->overhead)); + (size_t)(tiling->factor * (m_width * m_height * m_bpp) + tiling->overhead) / DT_MEGA); if(bcaching) { @@ -1524,6 +1553,9 @@ static gboolean _pixelpipe_process_on_CPU(dt_dev_pixelpipe_t *pipe, | PIXELPIPE_FLOW_PROCESSED_WITH_TILING); } + if(_module_pipe_stop(pipe, module, input, *output) != DT_DEV_STOPTEST_NO) + return TRUE; + if(pfm_dump) { dt_dump_pipe_pfm(module->op, *output, roi_out->width, roi_out->height, bpp, @@ -1730,12 +1762,6 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, GList *pieces, const int pos) { - /* As this is done recursively we check for any dt_dev_pixelpipe_stopper_t - signal that might have been set. - */ - if(_pipe_has_shutdown(pipe)) - return TRUE; - dt_iop_roi_t roi_in = *roi_out; char module_name[256] = { 0 }; @@ -1773,7 +1799,7 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, if(module) g_strlcpy(module_name, module->op, MIN(sizeof(module_name), sizeof(module->op))); - get_output_format(module, pipe, piece, dev, *out_format); + get_output_format(module, pipe, piece, *out_format); const size_t bpp = dt_iop_buffer_dsc_to_bpp(*out_format); const size_t bufsize = (size_t)bpp * roi_out->width * roi_out->height; @@ -1926,7 +1952,7 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, CLARG(clout), CLARG(clout), CLARG(roi_out->width), CLARG(roi_out->height), CLARGFLOAT(0.41666666666666666667f)); if(err == CL_SUCCESS) - err = dt_opencl_copy_image_to_host(pipe->devid, *output, clout, roi_out->width, roi_out->height, bpp); + err = _copy_image_to_host_err(pipe->devid, *output, clout, roi_out->width, roi_out->height, bpp, "copy back gamma corrected"); if(err == CL_SUCCESS) done = TRUE; else @@ -2023,8 +2049,6 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, if(_pipe_has_shutdown(pipe)) return TRUE; - gboolean important_cl_input = FALSE; - dt_times_t start; dt_get_perf_times(&start); @@ -2122,8 +2146,7 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, gboolean success_opencl = TRUE; dt_iop_colorspace_type_t input_cst_cl = input_format->cst; - /* if input is on gpu memory only, remember this fact to later - * take appropriate action */ + /* if input is on gpu memory only, remember this fact to later take appropriate action */ gboolean valid_input_on_gpu_only = (cl_mem_input != NULL); /* general remark: in case of opencl errors within modules or @@ -2190,59 +2213,106 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, } } + /* As the gpu memory is limited we currently use host memory for the iop cache. + A cl_mem image to host copy is an expensive operation so we only do that if + required after a colorspace conversion or if the cacheline is of importance + for a resonsive UI: + a) for the currently focused module, as that is most likely to change again + b) if there is a hint for changed parameters in history via the flag + c) modules having the IOP_FLAGS_WRITE_PIPECACHE_IN + Only do this for screen pipes and if there is no mask display. + */ + const gboolean has_focus = module == dt_dev_gui_module(); + const gboolean last_history = darktable.develop->history_last_module == module; + const gboolean iop_wanting = module->flags() & IOP_FLAGS_WRITE_PIPECACHE_IN; + const gboolean important_input = + dt_pipe_no_mask_display(pipe) + && dt_pipe_is_screen(pipe) + && !dt_pipe_is_fast(pipe) + && dev->gui_attached + && (has_focus || last_history || iop_wanting); + if(possible_cl) { + if(*cl_mem_output) // release stale cl_mem image as soon as possible for less mem pressure + { + dt_opencl_release_mem_object(*cl_mem_output); + *cl_mem_output = NULL; + } + const dt_iop_colorspace_type_t cst_from = input_cst_cl; const dt_iop_colorspace_type_t cst_to = module->input_colorspace(module, pipe, piece); const dt_iop_colorspace_type_t cst_out = module->output_colorspace(module, pipe, piece); + const gboolean convert = cst_from != cst_to; + + if(cl_mem_input == NULL) + cl_mem_input = dt_opencl_copy_host_to_image(pipe->devid, input, roi_in.width, roi_in.height, in_bpp); + success_opencl = cl_mem_input != NULL; - if(cst_from != cst_to) - dt_print_pipe(DT_DEBUG_PIPE, "transform colorspace", + if(convert) + { + dt_free_align(pipe->bcache_data); + pipe->bcache_data = NULL; + pipe->bcache_hash = DT_INVALID_HASH; + if(success_opencl) + success_opencl = dt_ioppr_transform_image_colorspace_cl(module, pipe->devid, + cl_mem_input, cl_mem_input, + roi_in.width, roi_in.height, + input_cst_cl, cst_to, &input_cst_cl, + work_profile); + dt_print_pipe(DT_DEBUG_PIPE, success_opencl ? "transform colorspace" : "FAILED transform colorspace", pipe, module, pipe->devid, &roi_in, NULL, "%s -> %s `%s'", dt_iop_colorspace_to_name(cst_from), dt_iop_colorspace_to_name(cst_to), work_profile ? dt_colorspaces_get_name(work_profile->type, work_profile->filename) : "no work profile"); + } - gboolean didconvert = FALSE; - if(cl_mem_input != NULL || fits_on_device) + if((important_input || convert || !fits_on_device) && success_opencl) { - if(cl_mem_input == NULL) - cl_mem_input = dt_opencl_copy_host_to_image(pipe->devid, input, roi_in.width, roi_in.height, in_bpp); + // Don't write back if input came from host memory + if(convert || valid_input_on_gpu_only) + success_opencl = _copy_image_to_host_err(pipe->devid, input, cl_mem_input, roi_in.width, roi_in.height, in_bpp, "input cacheline") == CL_SUCCESS; - if(cl_mem_input) + if(success_opencl) { - didconvert = success_opencl = dt_ioppr_transform_image_colorspace_cl(module, pipe->devid, - cl_mem_input, cl_mem_input, - roi_in.width, roi_in.height, - input_cst_cl, cst_to, &input_cst_cl, - work_profile); + input_format->cst = input_cst_cl; + valid_input_on_gpu_only = FALSE; } else { - success_opencl = FALSE; + dt_opencl_release_mem_object(cl_mem_input); + cl_mem_input = NULL; + pipe->opencl_error = TRUE; + return TRUE; } } - const gboolean want_bcache = _piece_fast_blend(piece, module); + // mark it as important for both valid code paths + if(important_input && ((possible_cl && success_opencl) || !possible_cl)) + { + dt_print_pipe(DT_DEBUG_PIPE, + "importance hints", + pipe, module, pipe->devid, &roi_in, NULL, "%s%s%s", + last_history ? "last_history " : "", + has_focus ? "focus " : "", + iop_wanting ? "iop_flags " : ""); + dt_dev_pixelpipe_important_cacheline(pipe, input, roi_in.width * roi_in.height * in_bpp); + } + + const gboolean want_bcache = _piece_fast_blend(piece); const dt_hash_t phash = want_bcache - ? _piece_process_hash(piece, roi_out, module, pos) + ? _bcache_hash(piece, roi_out, pos) : DT_INVALID_HASH; const gboolean bcaching = want_bcache && pipe->bcache_data && phash == pipe->bcache_hash && phash != DT_INVALID_HASH; dt_print_pipe(DT_DEBUG_PIPE, bcaching ? "from blend cache" : "process", - pipe, module, pipe->devid, &roi_in, roi_out, "%s%s%s%s%s %.1fMB", + pipe, module, pipe->devid, &roi_in, roi_out, "%s%s%s%s %zuMB", dt_iop_colorspace_to_name(cst_to), cst_to != cst_out ? " -> " : "", cst_to != cst_out ? dt_iop_colorspace_to_name(cst_out) : "", fits_on_device ? "" : ", tiled", - *cl_mem_output ? ", stale outimage" : "", - 1e-6 * (tiling.factor_cl * (m_width * m_height * m_bpp) + tiling.overhead)); - if(*cl_mem_output) - { - dt_opencl_release_mem_object(*cl_mem_output); - *cl_mem_output = NULL; - } + (size_t)(tiling.factor_cl * (m_width * m_height * m_bpp) + tiling.overhead) / DT_MEGA); if(fits_on_device) { @@ -2321,7 +2391,7 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, float *cache = _get_fast_blendcache(out_bpp * roi_out->width * roi_out->height / sizeof(float), phash, pipe); if(cache) { - err = dt_opencl_copy_image_to_host(pipe->devid, cache, *cl_mem_output, roi_out->width, roi_out->height, out_bpp); + err = _copy_image_to_host_err(pipe->devid, cache, *cl_mem_output, roi_out->width, roi_out->height, out_bpp, "fast blendcache"); if(err != CL_SUCCESS) { // for some reason writing to bcache failed so let's clear/invalidate it now and leave the error condition @@ -2363,9 +2433,19 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, pipe->dsc.cst = module->output_colorspace(module, pipe, piece); } - if(_module_pipe_stop(pipe, module, input)) + const dt_dev_stoptest_t stoptest = _module_pipe_stop(pipe, module, input, *output); + if(stoptest != DT_DEV_STOPTEST_NO) { + dt_opencl_release_mem_object(*cl_mem_output); + *cl_mem_output = NULL; + if(valid_input_on_gpu_only && stoptest == DT_DEV_STOPTEST_SAVE) + { + if(_copy_image_to_host_err(pipe->devid, input, cl_mem_input, roi_in.width, roi_in.height, in_bpp, "stoptest input cacheline") + != CL_SUCCESS) + pipe->opencl_error = TRUE; + } dt_opencl_release_mem_object(cl_mem_input); + cl_mem_input = NULL; return TRUE; } @@ -2454,49 +2534,14 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, } else if(piece->process_tiling_ready) { - /* image is too big for direct opencl processing -> try to - * process image via tiling */ - - /* we might need to copy back valid image from device to host */ - if(cl_mem_input != NULL) - { - /* copy back to CPU buffer, then clean unneeded buffer */ - if(dt_opencl_copy_image_to_host(pipe->devid, input, cl_mem_input, roi_in.width, roi_in.height, in_bpp) - != CL_SUCCESS) - { - dt_print_pipe(DT_DEBUG_OPENCL, - "process", - pipe, module, pipe->devid, &roi_in, roi_out, "%s", - "couldn't copy input data back to host memory"); - dt_opencl_release_mem_object(cl_mem_input); - pipe->opencl_error = TRUE; - return TRUE; - } - else - input_format->cst = input_cst_cl; - - dt_opencl_release_mem_object(cl_mem_input); - cl_mem_input = NULL; - valid_input_on_gpu_only = FALSE; - } - - // transform to module input colorspace if not done already - if(success_opencl && !didconvert) - { - dt_ioppr_transform_image_colorspace(module, input, input, - roi_in.width, roi_in.height, - input_format->cst, cst_to, &input_format->cst, - work_profile); - // the cacheline cst does not reflect the module input colorspace any more! - // FIXME let's invalidate for now - dt_dev_pixelpipe_invalidate_cacheline(pipe, input); - } + /* image is too big for direct opencl processing -> try to process image via tiling */ + dt_opencl_release_mem_object(cl_mem_input); + cl_mem_input = NULL; // histogram collection for module if(success_opencl) { - _collect_histogram_on_CPU(pipe, dev, input, &roi_in, - module, piece, &pixelpipe_flow); + _collect_histogram_on_CPU(pipe, dev, input, &roi_in, module, piece, &pixelpipe_flow); } /* now call process_tiling_cl of module; module should emit @@ -2543,11 +2588,10 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, pipe->dsc.cst = module->output_colorspace(module, pipe, piece); } - if(_module_pipe_stop(pipe, module, input)) + if(_module_pipe_stop(pipe, module, input, *output)) return TRUE; - dt_iop_colorspace_type_t blend_cst = - dt_develop_blend_colorspace(piece, pipe->dsc.cst); + dt_iop_colorspace_type_t blend_cst = dt_develop_blend_colorspace(piece, pipe->dsc.cst); const gboolean blend_picking = _request_color_pick(pipe, dev, module) && _transform_for_blend(module, piece) && blend_cst != cst_to; @@ -2629,60 +2673,7 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, /* finally check, if we were successful processing this module either in tiled mode or not */ if(success_opencl) { - /* Nice, everything went fine - - Copying device buffers back to host memory is an expensive - operation but is required for the iop cache. - As the gpu memory is very restricted we currently don't use that for a cache. - - The iop cache hit rate is much more important for the UI responsiveness so we make - sure relevant cache line buffers are kept as input for reprocessing. - This is true - a) for the currently focused iop, as that is the iop which is most likely to change next - b) if there is a hint for changed parameters in history via the flag - c) modules having the IOP_FLAGS_WRITE_PIPECACHE_IN - d) in all a-c cases only for screen pipes and no mask_display - - Note: we miss writing output for cache for now to be tested via IOP_FLAGS_WRITE_PIPECACHECL_OUT - */ - const gboolean cachewrite = dt_pipe_no_mask_display(pipe) - && dt_pipe_is_screen(pipe) - && !dt_pipe_is_fast(pipe) - && dev->gui_attached; - - important_cl_input = cachewrite - && ((module == dt_dev_gui_module()) - || darktable.develop->history_last_module == module - || (module->flags() & IOP_FLAGS_WRITE_PIPECACHE_IN)); - - if(important_cl_input) - { - /* write back input into cache for faster re-usal (full pipe or preview) */ - if(cl_mem_input != NULL) - { - /* copy input to host memory, so we can find it in cache and wait via blocking flag */ - if(dt_opencl_copy_image_to_host(pipe->devid, input, cl_mem_input, roi_in.width, roi_in.height, in_bpp) - != CL_SUCCESS) - { - important_cl_input = FALSE; - dt_print_pipe(DT_DEBUG_OPENCL, - "process", pipe, module, pipe->devid, &roi_in, roi_out, "%s", - "couldn't copy input data back to host memory for caching"); - /* that's all we do here, we later make sure to invalidate cache line */ - } - else - { - dt_print_pipe(DT_DEBUG_OPENCL | DT_DEBUG_VERBOSE, - "copied CL input to host for pixelpipe cache", pipe, module, pipe->devid, &roi_in, NULL); - /* success: cache line is valid now, no need to invalidate it later */ - valid_input_on_gpu_only = FALSE; - - input_format->cst = input_cst_cl; - } - } - } - - /* we can now release cl_mem_input */ + /* Nice, everything went fine, we can now release cl_mem_input */ dt_opencl_release_mem_object(cl_mem_input); cl_mem_input = NULL; // we speculate on the next module to possibly copy back @@ -2709,18 +2700,18 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, In-place colorspace conversion to input cst is good as the cpu fallback won't do that conversion again. */ - if(dt_opencl_copy_image_to_host(pipe->devid, input, cl_mem_input, roi_in.width, roi_in.height, in_bpp) - != CL_SUCCESS) + if(valid_input_on_gpu_only) { - dt_print_pipe(DT_DEBUG_OPENCL, - "process", pipe, module, pipe->devid, &roi_in, roi_out, "%s", - "couldn't copy input data back to host memory for caching for CPU fallback"); - dt_opencl_release_mem_object(cl_mem_input); - pipe->opencl_error = TRUE; - return TRUE; + if(_copy_image_to_host_err(pipe->devid, input, cl_mem_input, roi_in.width, roi_in.height, in_bpp, "opencl failed so write back to host") + != CL_SUCCESS) + { + dt_opencl_release_mem_object(cl_mem_input); + pipe->opencl_error = TRUE; + return TRUE; + } + else + input_format->cst = input_cst_cl; } - else - input_format->cst = input_cst_cl; /* this is a good place to release event handles as we move from gpu to cpu here */ dt_opencl_finish(pipe->devid); @@ -2741,18 +2732,18 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, /* cleanup unneeded opencl input buffer, and copy back to CPU */ if(cl_mem_input != NULL) { - if(dt_opencl_copy_image_to_host(pipe->devid, input, cl_mem_input, roi_in.width, roi_in.height, in_bpp) - != CL_SUCCESS) + if(valid_input_on_gpu_only) { - dt_print_pipe(DT_DEBUG_OPENCL, - "process", pipe, module, pipe->devid, &roi_in, roi_out, "%s", - "couldn't copy input data back to host memory for CPU processing"); - dt_opencl_release_mem_object(cl_mem_input); - pipe->opencl_error = TRUE; - return TRUE; + if(_copy_image_to_host_err(pipe->devid, input, cl_mem_input, roi_in.width, roi_in.height, in_bpp, "opencl not allowed") + != CL_SUCCESS) + { + dt_opencl_release_mem_object(cl_mem_input); + pipe->opencl_error = TRUE; + return TRUE; + } + else + input_format->cst = input_cst_cl; } - else - input_format->cst = input_cst_cl; /* this is a good place to release event handles as we anyhow move from gpu to cpu here */ dt_opencl_finish(pipe->devid); @@ -2766,7 +2757,6 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, return TRUE; } - /* input is still only on GPU? Let's invalidate CPU input buffer then */ if(valid_input_on_gpu_only) dt_dev_pixelpipe_invalidate_cacheline(pipe, input); } @@ -2818,39 +2808,16 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, // special cases for active modules with available gui if(module - && darktable.develop->gui_attached - && module->enabled) + && darktable.develop->gui_attached + && module->enabled + && module->expanded + && dt_pipe_is_basic(pipe) + && (module->request_histogram & DT_REQUEST_EXPANDED)) { - // Possibly give the input buffer of the current module more weight - // as the user is likely to change that one soon (again), so keep it in cache. - // Also do this if the clbuffer has been actively written - const gboolean has_focus = module == dt_dev_gui_module(); - const gboolean last_history = darktable.develop->history_last_module == module; - if(dt_pipe_is_screen(pipe) - && dt_pipe_no_mask_display(pipe) - && (has_focus || last_history || important_cl_input || (module->flags() & IOP_FLAGS_WRITE_PIPECACHE_IN))) - { - dt_print_pipe(DT_DEBUG_PIPE, - "importance hints", - pipe, module, pipe->devid, &roi_in, NULL, "%s%s%s", - last_history ? "input_hint " : "", - has_focus ? "focus " : "", - important_cl_input ? "important_in" : ""); - dt_dev_pixelpipe_important_cacheline(pipe, input, - roi_in.width * roi_in.height * in_bpp); - if(dt_pipe_is_full(pipe) && last_history) - darktable.develop->history_last_module = NULL; - } - - if(module->expanded - && dt_pipe_is_basic(pipe) - && (module->request_histogram & DT_REQUEST_EXPANDED)) - { - dt_print_pipe(DT_DEBUG_PIPE, "internal histogram", + dt_print_pipe(DT_DEBUG_PIPE, "internal histogram", pipe, module, DT_DEVICE_NONE, &roi_in, roi_out); - pipe->nocache = TRUE; - dt_dev_pixelpipe_invalidate_cacheline(pipe, *output); - } + pipe->nocache = TRUE; + dt_dev_pixelpipe_invalidate_cacheline(pipe, *output); } // warn on NaN or infinity @@ -2859,7 +2826,7 @@ static gboolean _dev_pixelpipe_process_rec(dt_dev_pixelpipe_t *pipe, gboolean valid_output = TRUE; #ifdef HAVE_OPENCL if(*cl_mem_output != NULL) - valid_output = dt_opencl_copy_image_to_host(pipe->devid, *output, *cl_mem_output, roi_out->width, roi_out->height, bpp) + valid_output = _copy_image_to_host_err(pipe->devid, *output, *cl_mem_output, roi_out->width, roi_out->height, bpp, "NaN analysis") == CL_SUCCESS; #endif @@ -3029,8 +2996,8 @@ static gboolean _dev_pixelpipe_process_rec_and_backcopy(dt_dev_pixelpipe_t *pipe { if(*cl_mem_output != NULL) { - cl_int err = dt_opencl_copy_image_to_host(pipe->devid, *output, *cl_mem_output, - roi_out->width, roi_out->height, dt_iop_buffer_dsc_to_bpp(*out_format)); + cl_int err = _copy_image_to_host_err(pipe->devid, *output, *cl_mem_output, + roi_out->width, roi_out->height, dt_iop_buffer_dsc_to_bpp(*out_format), "final backcopy"); dt_opencl_release_mem_object(*cl_mem_output); *cl_mem_output = NULL; diff --git a/src/develop/pixelpipe_hb.h b/src/develop/pixelpipe_hb.h index 0c0acea08819..1b1aee3a5f5b 100644 --- a/src/develop/pixelpipe_hb.h +++ b/src/develop/pixelpipe_hb.h @@ -31,7 +31,7 @@ G_BEGIN_DECLS #define DT_PIPECACHE_MIN 2 -// #define DT_PIPE_CAS_SHUTDOWN +#define DT_PIPE_CAS_SHUTDOWN /** cached distorted mask at a geometric module's output boundary. * used to avoid re-distorting masks from scratch when multiple @@ -107,11 +107,14 @@ typedef enum dt_dev_pixelpipe_status_t This requires special care in - _dev_pixelpipe_process_rec() - dt_dev_process_image_job() - possibly invalidating wrong module input/output data in the pixelpipe cache, - ensure either an immediate restart of the pipe or exit of dt_dev_process_image_job() - with an error flag. - A reminder, when setting pipe->shutdown we might have to do that via dt_atomic_CAS_int() - with wxpected DT_DEV_PIXELPIPE_STOP_NO to avoid overwriting an earlier shutdown writing. + possibly + - invalidating bad input or output cachelines, + - updating the cacheline dsc if there was an inspace colorspace conversion + - ensure an immediate restart of the pipe or exit of dt_dev_process_image_job() + with an error flag. + + When setting pipe->shutdown from outside of pixelpipe internals we should use + dt_dev_pixelpipe_set_shutdown() for logs and making sure we don't overwrite shutdown. A summary about how these shutdown modes are supposed to work. @@ -120,18 +123,17 @@ typedef enum dt_dev_pixelpipe_status_t DT_DEV_PIXELPIPE_STOP_NODES Set if the pipe should stop as the pipe nodes are changed so a restart is desired asap. - As nodes are recreated, we don't have to fiddle with pixelpipe cache. DT_DEV_PIXELPIPE_STOP_HQ Used to switch between darkroom HQ modes. - Requires a restart of the pipe but pixelpipe cache can stay. DT_DEV_PIXELPIPE_STOP_LAST If the shutdown value is >= DT_DEV_PIXELPIPE_STOP_LAST it is understood as the iop_order - of a module. - Any module might set pipe->shutdown to it's iop_order, this is checked while processing - the pipe and if detected the piece input data and all pipe cachelines with at least - this iop_order will be invalidated. + of a module. Any module can use dt_dev_pixelpipe_set_shutdown() to it's iop_order at runtime, + this is checked in _module_pipe_stop() while processing the pipe. + + _module_pipe_stop() ensures valid cachelines with corrected dsc or invalidates + in case of invalid output. */ typedef enum dt_dev_pixelpipe_stopper_t