[PATCH] dma-buf: check the return value of kstrdup()
From: Xiaoke Wang kstrdup() is a memory allocation function which can return NULL when some internaly memory errors happen. It is better to check the return value of it to prevent further wrong memory access. Signed-off-by: Xiaoke Wang --- drivers/dma-buf/selftest.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/dma-buf/selftest.c b/drivers/dma-buf/selftest.c index c60b694..2c29e2a 100644 --- a/drivers/dma-buf/selftest.c +++ b/drivers/dma-buf/selftest.c @@ -50,6 +50,9 @@ static bool apply_subtest_filter(const char *caller, const char *name) bool result = true; filter = kstrdup(__st_filter, GFP_KERNEL); + if (!filter) + return false; + for (sep = filter; (tok = strsep(&sep, ","));) { bool allow = true; char *sl; --
[PATCH] drm/i915: make a handful of read-only arrays static const
Don't populate the read-only arrays on the stack but instead make them static const. Also makes the object code a little smaller. Reformat the statements to clear up checkpatch warning. Signed-off-by: Colin Ian King --- drivers/gpu/drm/i915/display/intel_vdsc.c | 16 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c index 3faea903b9ae..d49f66237ec3 100644 --- a/drivers/gpu/drm/i915/display/intel_vdsc.c +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c @@ -378,10 +378,18 @@ calculate_rc_params(struct rc_parameters *rc, { int bpc = vdsc_cfg->bits_per_component; int bpp = vdsc_cfg->bits_per_pixel >> 4; - int ofs_und6[] = { 0, -2, -2, -4, -6, -6, -8, -8, -8, -10, -10, -12, -12, -12, -12 }; - int ofs_und8[] = { 2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -10, -12, -12, -12 }; - int ofs_und12[] = { 2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -10, -12, -12, -12 }; - int ofs_und15[] = { 10, 8, 6, 4, 2, 0, -2, -4, -6, -8, -10, -10, -12, -12, -12 }; + static const int ofs_und6[] = { + 0, -2, -2, -4, -6, -6, -8, -8, -8, -10, -10, -12, -12, -12, -12 + }; + static const int ofs_und8[] = { + 2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -10, -12, -12, -12 + }; + static const int ofs_und12[] = { + 2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -10, -12, -12, -12 + }; + static const int ofs_und15[] = { + 10, 8, 6, 4, 2, 0, -2, -4, -6, -8, -10, -10, -12, -12, -12 + }; int qp_bpc_modifier = (bpc - 8) * 2; u32 res, buf_i, bpp_i; -- 2.34.1
[PATCH] drm/i915/selftests: check the return value of kstrdup()
From: Xiaoke Wang kstrdup() is a memory allocation function which can return NULL when some internaly memory errors happen. It is better to check the return value of it to prevent further wrong memory access. Signed-off-by: Xiaoke Wang --- drivers/gpu/drm/i915/selftests/i915_selftest.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/selftests/i915_selftest.c b/drivers/gpu/drm/i915/selftests/i915_selftest.c index 484759c..1bcd065 100644 --- a/drivers/gpu/drm/i915/selftests/i915_selftest.c +++ b/drivers/gpu/drm/i915/selftests/i915_selftest.c @@ -246,6 +246,9 @@ static bool apply_subtest_filter(const char *caller, const char *name) bool result = true; filter = kstrdup(i915_selftest.filter, GFP_KERNEL); + if (!filter) + return false; + for (sep = filter; (tok = strsep(&sep, ","));) { bool allow = true; char *sl; --
Re: [PATCH v3 3/9] gpu: host1x: Add context device management code
On 2/22/22 18:24, Christoph Hellwig wrote: On Fri, Feb 18, 2022 at 01:39:46PM +0200, Mikko Perttunen wrote: + +/* + * Due to an issue with T194 NVENC, only 38 bits can be used. + * Anyway, 256GiB of IOVA ought to be enough for anyone. + */ +static dma_addr_t context_device_dma_mask = DMA_BIT_MASK(38); You need a mask per device. Please don't share the same variable for multiple masks. +EXPORT_SYMBOL(host1x_context_alloc); All this low-level code really should be EXPORT_SYMBOL_GPL. Thanks, will fix (and same for patch 2). Mikko
Re: [PATCH v11 1/6] drm: Add arch arm64 for drm_clflush_virt_range
Hi Michael, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on drm-tip/drm-tip] [also build test WARNING on drm/drm-next] [cannot apply to drm-intel/for-linux-next v5.17-rc5 next-20220222] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Michael-Cheng/Use-drm_clflush-instead-of-clflush/20220223-140110 base: git://anongit.freedesktop.org/drm/drm-tip drm-tip config: h8300-randconfig-r014-20220221 (https://download.01.org/0day-ci/archive/20220223/202202231707.syvm9ofu-...@intel.com/config) compiler: h8300-linux-gcc (GCC) 11.2.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/f4c92ba1f52db578a26ac9944e2cbe52c548e8e9 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Michael-Cheng/Use-drm_clflush-instead-of-clflush/20220223-140110 git checkout f4c92ba1f52db578a26ac9944e2cbe52c548e8e9 # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=h8300 SHELL=/bin/bash drivers/gpu/drm/ If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All warnings (new ones prefixed by >>): In file included from drivers/gpu/drm/drm_cache.c:31: >> include/linux/cacheflush.h:12:46: warning: 'struct folio' declared inside >> parameter list will not be visible outside of this definition or declaration 12 | static inline void flush_dcache_folio(struct folio *folio) | ^ vim +12 include/linux/cacheflush.h 522a0032af00550 Matthew Wilcox (Oracle 2021-11-06 6) 522a0032af00550 Matthew Wilcox (Oracle 2021-11-06 7) #if ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE 522a0032af00550 Matthew Wilcox (Oracle 2021-11-06 8) #ifndef ARCH_IMPLEMENTS_FLUSH_DCACHE_FOLIO 522a0032af00550 Matthew Wilcox (Oracle 2021-11-06 9) void flush_dcache_folio(struct folio *folio); 522a0032af00550 Matthew Wilcox (Oracle 2021-11-06 10) #endif 522a0032af00550 Matthew Wilcox (Oracle 2021-11-06 11) #else 522a0032af00550 Matthew Wilcox (Oracle 2021-11-06 @12) static inline void flush_dcache_folio(struct folio *folio) 522a0032af00550 Matthew Wilcox (Oracle 2021-11-06 13) { 522a0032af00550 Matthew Wilcox (Oracle 2021-11-06 14) } 522a0032af00550 Matthew Wilcox (Oracle 2021-11-06 15) #define ARCH_IMPLEMENTS_FLUSH_DCACHE_FOLIO 0 522a0032af00550 Matthew Wilcox (Oracle 2021-11-06 16) #endif /* ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE */ 522a0032af00550 Matthew Wilcox (Oracle 2021-11-06 17) --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
Re: [PATCH] drm/msm: Avoid dirtyfb stalls on video mode displays
On 19/02/2022 22:39, Rob Clark wrote: From: Rob Clark Someone on IRC once asked an innocent enough sounding question: Why with xf86-video-modesetting is es2gears limited at 120fps. So I broke out the perfetto tracing mesa MR and took a look. It turns out the problem was drm_atomic_helper_dirtyfb(), which would end up waiting for vblank.. es2gears would rapidly push two frames to Xorg, which would blit them to screen and in idle hook (I assume) call the DIRTYFB ioctl. Which in turn would do an atomic update to flush the dirty rects, which would stall until the next vblank. And then the whole process would repeat. But this is a bit silly, we only need dirtyfb for command mode DSI panels. So lets just skip it otherwise. Signed-off-by: Rob Clark --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 13 + drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 9 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 + drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 9 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 1 + drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h | 1 + drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 8 +++ drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 1 + drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h | 1 + drivers/gpu/drm/msm/msm_fb.c | 64 ++- drivers/gpu/drm/msm/msm_kms.h | 2 + 11 files changed, 109 insertions(+), 1 deletion(-) I have checked your previous dirtyfb patch (and corresponding discussion). I'm not fond of the idea of acquiring locks, computing the value, then releasing the locks and reacquiring the locks again. I understand the original needs_dirtyfb approach was frowned upon. Do we have a chance of introducing drm_atomic_helper_dirtyfb_unlocked()? Which would perform all the steps of the orignal helper, but without locks acquirement (and state allocation/freeing)? [skipped] diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c index 4d34df5354e0..1b0648baeae2 100644 --- a/drivers/gpu/drm/msm/msm_fb.c +++ b/drivers/gpu/drm/msm/msm_fb.c @@ -24,10 +24,72 @@ struct msm_framebuffer { static struct drm_framebuffer *msm_framebuffer_init(struct drm_device *dev, const struct drm_mode_fb_cmd2 *mode_cmd, struct drm_gem_object **bos); +static int msm_framebuffer_dirtyfb(struct drm_framebuffer *fb, + struct drm_file *file_priv, unsigned int flags, + unsigned int color, struct drm_clip_rect *clips, + unsigned int num_clips) +{ + struct msm_drm_private *priv = fb->dev->dev_private; + struct drm_modeset_acquire_ctx ctx; + struct drm_plane *plane; + bool needs_flush = false; + int ret = 0; + + /* +* When called from ioctl, we are interruptible, but not when called +* internally (ie. defio worker) +*/ + drm_modeset_acquire_init(&ctx, + file_priv ? DRM_MODESET_ACQUIRE_INTERRUPTIBLE : 0); + +retry: + drm_for_each_plane(plane, fb->dev) { + struct drm_plane_state *plane_state; + struct drm_crtc *crtc; + + ret = drm_modeset_lock(&plane->mutex, &ctx); + if (ret) + goto out; + + if (plane->state->fb != fb) { + drm_modeset_unlock(&plane->mutex); + continue; + } + + crtc = plane->state->crtc; + + ret = drm_modeset_lock(&crtc->mutex, &ctx); + if (ret) + goto out; + + if (priv->kms->funcs->needs_dirtyfb(crtc)) { + needs_flush = true; + break; + } + } + +out: + if (ret == -EDEADLK) { + ret = drm_modeset_backoff(&ctx); + if (!ret) + goto retry; + } + + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); + + if (needs_flush) { This bit triggers my paranoia. The driver computes the value with the locks being held and then performs some action depending on the computed value after releasing the locks. I'd prefer to acquire modesetting locks for all the planes (and allocate atomic state), check if the dirtyfb processing is needed, then call drm_atomic_helper_dirtyfb_unlocked() withith the same locks section. + ret = drm_atomic_helper_dirtyfb(fb, file_priv, flags, + color, clips, num_clips); + } + + return ret; +} + static const struct drm_framebuffer_funcs msm_framebuffer_funcs = { .create_handle = drm_gem_fb_create_handle, .destroy = drm_gem_fb_destroy, - .dirty = drm_atomic_helper_dirtyfb, + .dirty = msm_framebuffer_dirtyfb, }; #ifdef CONFIG_DEBUG_FS diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h ind
Re: [PATCH] drm/arm: arm hdlcd select DRM_GEM_CMA_HELPER
On Tue, 25 Jan 2022 12:37:38 + Liviu Dudau wrote: Hi, > On Mon, Jan 24, 2022 at 04:24:37PM +, carsten.haitz...@foss.arm.com wrote: > > From: Carsten Haitzler > > > > Without DRM_GEM_CMA_HELPER HDLCD won't build. This needs to be there too. > > > > Fixes: 09717af7d13d ("drm: Remove CONFIG_DRM_KMS_CMA_HELPER option") > > > > Signed-off-by: Carsten Haitzler > > Acked-by: Liviu Dudau > > I will add Steven's reviewed-by as well when pushing it. Did this go anywhere? I see my .config just selecting HDLCD still failing with -rc5. Any chance this can be taken now, as this is a regression introduced with -rc1? Cheers, Andre > > --- > > drivers/gpu/drm/arm/Kconfig | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig > > index 58a242871b28..6e3f1d600541 100644 > > --- a/drivers/gpu/drm/arm/Kconfig > > +++ b/drivers/gpu/drm/arm/Kconfig > > @@ -6,6 +6,7 @@ config DRM_HDLCD > > depends on DRM && OF && (ARM || ARM64 || COMPILE_TEST) > > depends on COMMON_CLK > > select DRM_KMS_HELPER > > + select DRM_GEM_CMA_HELPER > > help > > Choose this option if you have an ARM High Definition Colour LCD > > controller. > > -- > > 2.30.1 > > >
[PATCH v6 00/12] clk: Improve clock range handling
Hi, This is a follow-up of the discussion here: https://lore.kernel.org/linux-clk/20210319150355.xzw7ikwdaga2dwhv@gilmour/ and here: https://lore.kernel.org/all/20210914093515.260031-1-max...@cerno.tech/ While the initial proposal implemented a new API to temporarily raise and lower clock rates based on consumer workloads, Stephen suggested an alternative approach implemented here. The main issue that needed to be addressed in our case was that in a situation where we would have multiple calls to clk_set_rate_range, we would end up with a clock at the maximum of the minimums being set. This would be expected, but the issue was that if one of the users was to relax or drop its requirements, the rate would be left unchanged, even though the ideal rate would have changed. So something like clk_set_rate(user1_clk, 1000); clk_set_min_rate(user1_clk, 2000); clk_set_min_rate(user2_clk, 3000); clk_set_min_rate(user2_clk, 1000); Would leave the clock running at 3000Hz, while the minimum would now be 2000Hz. This was mostly due to the fact that the core only triggers a rate change in clk_set_rate_range() if the current rate is outside of the boundaries, but not if it's within the new boundaries. That series changes that and will trigger a rate change on every call, with the former rate being tried again. This way, providers have a chance to follow whatever policy they see fit for a given clock each time the boundaries change. This series also implements some kunit tests, first to test a few rate related functions in the CCF, and then extends it to make sure that behaviour has some test coverage. Let me know what you think Maxime Changes from v5: - Changed clk_hw_create_clk for clk_hw_get_clk since the former isn't exported to modules. - Added fix for clk_hw_get_clk Changes from v4: - Rename the test file - Move all the tests to the first patch, and fix them up as fixes are done - Improved the test conditions - Added more tests - Improved commit messages - Fixed a regression where two disjoints clock ranges would now be accepted Changes from v3: - Renamed the test file and Kconfig option - Add option to .kunitconfig - Switch to kunit_kzalloc - Use KUNIT_EXPECT_* instead of KUNIT_ASSERT_* where relevant - Test directly relevant calls instead of going through a temporary variable - Switch to more precise KUNIT_ASSERT_* macros where relevant Changes from v2: - Rebased on current next - Rewrote the whole thing according to Stephen reviews - Implemented some kunit tests Changes from v1: - Return NULL in clk_request_start if clk pointer is NULL - Test for clk_req pointer in clk_request_done - Add another user in vc4 - Rebased on top of v5.15-rc1 Maxime Ripard (12): clk: Fix clk_hw_get_clk() when dev is NULL clk: Introduce Kunit Tests for the framework clk: Enforce that disjoints limits are invalid clk: Always clamp the rounded rate clk: Use clamp instead of open-coding our own clk: Always set the rate on clk_set_range_rate clk: Add clk_drop_range clk: bcm: rpi: Add variant structure clk: bcm: rpi: Set a default minimum rate clk: bcm: rpi: Run some clocks at the minimum rate allowed drm/vc4: Add logging and comments drm/vc4: hdmi: Remove clock rate initialization drivers/clk/.kunitconfig | 1 + drivers/clk/Kconfig | 7 + drivers/clk/Makefile | 1 + drivers/clk/bcm/clk-raspberrypi.c | 125 - drivers/clk/clk.c | 76 ++- drivers/clk/clk_test.c| 790 ++ drivers/gpu/drm/vc4/vc4_hdmi.c| 13 - drivers/gpu/drm/vc4/vc4_kms.c | 11 + include/linux/clk.h | 11 + 9 files changed, 980 insertions(+), 55 deletions(-) create mode 100644 drivers/clk/clk_test.c -- 2.35.1
[PATCH v6 02/12] clk: Introduce Kunit Tests for the framework
Let's test various parts of the rate-related clock API with the kunit testing framework. Cc: kunit-...@googlegroups.com Suggested-by: Stephen Boyd Signed-off-by: Maxime Ripard --- drivers/clk/.kunitconfig | 1 + drivers/clk/Kconfig | 7 + drivers/clk/Makefile | 1 + drivers/clk/clk_test.c | 786 +++ 4 files changed, 795 insertions(+) create mode 100644 drivers/clk/clk_test.c diff --git a/drivers/clk/.kunitconfig b/drivers/clk/.kunitconfig index 3754fdb9485a..cdbc7d7deba9 100644 --- a/drivers/clk/.kunitconfig +++ b/drivers/clk/.kunitconfig @@ -1,3 +1,4 @@ CONFIG_KUNIT=y CONFIG_COMMON_CLK=y +CONFIG_CLK_KUNIT_TEST=y CONFIG_CLK_GATE_KUNIT_TEST=y diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 3cdf33470a75..2ef6eca297ff 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -429,6 +429,13 @@ source "drivers/clk/xilinx/Kconfig" source "drivers/clk/zynqmp/Kconfig" # Kunit test cases +config CLK_KUNIT_TEST + tristate "Basic Clock Framework Kunit Tests" if !KUNIT_ALL_TESTS + depends on KUNIT + default KUNIT_ALL_TESTS + help + Kunit tests for the common clock framework. + config CLK_GATE_KUNIT_TEST tristate "Basic gate type Kunit test" if !KUNIT_ALL_TESTS depends on KUNIT diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 6a98291350b6..8f9b1daba411 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -2,6 +2,7 @@ # common clock types obj-$(CONFIG_HAVE_CLK) += clk-devres.o clk-bulk.o clkdev.o obj-$(CONFIG_COMMON_CLK) += clk.o +obj-$(CONFIG_CLK_KUNIT_TEST) += clk_test.o obj-$(CONFIG_COMMON_CLK) += clk-divider.o obj-$(CONFIG_COMMON_CLK) += clk-fixed-factor.o obj-$(CONFIG_COMMON_CLK) += clk-fixed-rate.o diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c new file mode 100644 index ..0ca6cd391c8e --- /dev/null +++ b/drivers/clk/clk_test.c @@ -0,0 +1,786 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Kunit test for clk rate management + */ +#include +#include +#include + +/* Needed for clk_hw_get_clk() */ +#include "clk.h" + +#include + +#define DUMMY_CLOCK_INIT_RATE (42 * 1000 * 1000) +#define DUMMY_CLOCK_RATE_1 (142 * 1000 * 1000) +#define DUMMY_CLOCK_RATE_2 (242 * 1000 * 1000) + +struct clk_dummy_context { + struct clk_hw hw; + unsigned long rate; +}; + +static unsigned long clk_dummy_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + struct clk_dummy_context *ctx = + container_of(hw, struct clk_dummy_context, hw); + + return ctx->rate; +} + +static int clk_dummy_determine_rate(struct clk_hw *hw, +struct clk_rate_request *req) +{ + /* Just return the same rate without modifying it */ + return 0; +} + +static int clk_dummy_maximize_rate(struct clk_hw *hw, + struct clk_rate_request *req) +{ + /* +* If there's a maximum set, always run the clock at the maximum +* allowed. +*/ + if (req->max_rate < ULONG_MAX) + req->rate = req->max_rate; + + return 0; +} + +static int clk_dummy_minimize_rate(struct clk_hw *hw, + struct clk_rate_request *req) +{ + /* +* If there's a minimum set, always run the clock at the minimum +* allowed. +*/ + if (req->min_rate > 0) + req->rate = req->min_rate; + + return 0; +} + +static int clk_dummy_set_rate(struct clk_hw *hw, + unsigned long rate, + unsigned long parent_rate) +{ + struct clk_dummy_context *ctx = + container_of(hw, struct clk_dummy_context, hw); + + ctx->rate = rate; + return 0; +} + +static const struct clk_ops clk_dummy_rate_ops = { + .recalc_rate = clk_dummy_recalc_rate, + .determine_rate = clk_dummy_determine_rate, + .set_rate = clk_dummy_set_rate, +}; + +static const struct clk_ops clk_dummy_maximize_rate_ops = { + .recalc_rate = clk_dummy_recalc_rate, + .determine_rate = clk_dummy_maximize_rate, + .set_rate = clk_dummy_set_rate, +}; + +static const struct clk_ops clk_dummy_minimize_rate_ops = { + .recalc_rate = clk_dummy_recalc_rate, + .determine_rate = clk_dummy_minimize_rate, + .set_rate = clk_dummy_set_rate, +}; + +static int clk_test_init_with_ops(struct kunit *test, const struct clk_ops *ops) +{ + struct clk_dummy_context *ctx; + struct clk_init_data init = { }; + int ret; + + ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL); + if (!ctx) + return -ENOMEM; + ctx->rate = DUMMY_CLOCK_INIT_RATE; + test->priv = ctx; + + init.name = "test_dummy_rate"; + init.ops = ops; + ctx->hw.init = &init; + + ret = clk_hw_re
[PATCH v6 01/12] clk: Fix clk_hw_get_clk() when dev is NULL
Any registered clk_core structure can have a NULL pointer in its dev field. While never actually documented, this is evidenced by the wide usage of clk_register and clk_hw_register with a NULL device pointer, and the fact that the core of_clk_hw_register() function also passes a NULL device pointer. A call to clk_hw_get_clk() on a clk_hw struct whose clk_core is in that case will result in a NULL pointer derefence when it calls dev_name() on that NULL device pointer. Add a test for this case and use NULL as the dev_id if the device pointer is NULL. Fixes: 30d6f8c15d2c ("clk: add api to get clk consumer from clk_hw") Signed-off-by: Maxime Ripard --- drivers/clk/clk.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 8de6a22498e7..fff5edb89d6d 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3773,8 +3773,9 @@ struct clk *clk_hw_create_clk(struct device *dev, struct clk_hw *hw, struct clk *clk_hw_get_clk(struct clk_hw *hw, const char *con_id) { struct device *dev = hw->core->dev; + const char *name = dev ? dev_name(dev) : NULL; - return clk_hw_create_clk(dev, hw, dev_name(dev), con_id); + return clk_hw_create_clk(dev, hw, name, con_id); } EXPORT_SYMBOL(clk_hw_get_clk); -- 2.35.1
[PATCH v6 03/12] clk: Enforce that disjoints limits are invalid
If we were to have two users of the same clock, doing something like: clk_set_rate_range(user1, 1000, 2000); clk_set_rate_range(user2, 3000, 4000); The second call would fail with -EINVAL, preventing from getting in a situation where we end up with impossible limits. However, this is never explicitly checked against and enforced, and works by relying on an undocumented behaviour of clk_set_rate(). Indeed, on the first clk_set_rate_range will make sure the current clock rate is within the new range, so it will be between 1000 and 2000Hz. On the second clk_set_rate_range(), it will consider (rightfully), that our current clock is outside of the 3000-4000Hz range, and will call clk_core_set_rate_nolock() to set it to 3000Hz. clk_core_set_rate_nolock() will then call clk_calc_new_rates() that will eventually check that our rate 3000Hz rate is outside the min 3000Hz max 2000Hz range, will bail out, the error will propagate and we'll eventually return -EINVAL. This solely relies on the fact that clk_calc_new_rates(), and in particular clk_core_determine_round_nolock(), won't modify the new rate allowing the error to be reported. That assumption won't be true for all drivers, and most importantly we'll break that assumption in a later patch. It can also be argued that we shouldn't even reach the point where we're calling clk_core_set_rate_nolock(). Let's make an explicit check for disjoints range before we're doing anything. Signed-off-by: Maxime Ripard --- drivers/clk/clk.c | 24 1 file changed, 24 insertions(+) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index fff5edb89d6d..112911138a7b 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -632,6 +632,24 @@ static void clk_core_get_boundaries(struct clk_core *core, *max_rate = min(*max_rate, clk_user->max_rate); } +static bool clk_core_check_boundaries(struct clk_core *core, + unsigned long min_rate, + unsigned long max_rate) +{ + struct clk *user; + + lockdep_assert_held(&prepare_lock); + + if (min_rate > core->max_rate || max_rate < core->min_rate) + return false; + + hlist_for_each_entry(user, &core->clks, clks_node) + if (min_rate > user->max_rate || max_rate < user->min_rate) + return false; + + return true; +} + void clk_hw_set_rate_range(struct clk_hw *hw, unsigned long min_rate, unsigned long max_rate) { @@ -2348,6 +2366,11 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max) clk->min_rate = min; clk->max_rate = max; + if (!clk_core_check_boundaries(clk->core, min, max)) { + ret = -EINVAL; + goto out; + } + rate = clk_core_get_rate_nolock(clk->core); if (rate < min || rate > max) { /* @@ -2376,6 +2399,7 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max) } } +out: if (clk->exclusive_count) clk_core_rate_protect(clk->core); -- 2.35.1
[PATCH v6 05/12] clk: Use clamp instead of open-coding our own
The code in clk_set_rate_range() will, if the current rate is outside of the new range, will force it to the minimum or maximum. Since it's running under the condition that the rate is either lower than the minimum, or higher than the maximum, this is equivalent to using clamp, while being less readable. Let's switch to using clamp instead. Signed-off-by: Maxime Ripard --- drivers/clk/clk.c | 6 +- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 6c4e10209568..c15ee5070f52 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2388,11 +2388,7 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max) * this corner case when determining the rate */ - if (rate < min) - rate = min; - else - rate = max; - + rate = clamp(clk->core->req_rate, min, max); ret = clk_core_set_rate_nolock(clk->core, rate); if (ret) { /* rollback the changes */ -- 2.35.1
[PATCH v6 04/12] clk: Always clamp the rounded rate
The current core while setting the min and max rate properly in the clk_request structure will not make sure that the requested rate is within these boundaries, leaving it to each and every driver to make sure it is. It's not clear if this was on purpose or not, but this introduces some inconsistencies within the API. For example, a user setting a range and then calling clk_round_rate() with a value outside of that range will get the same value back (ignoring any driver adjustements), effectively ignoring the range that was just set. Another one, arguably worse, is that it also makes clk_round_rate() and clk_set_rate() behave differently if there's a range and the rate being used for both is outside that range. As we have seen, the rate will be returned unchanged by clk_round_rate(), but clk_set_rate() will error out returning -EINVAL. Let's make sure the framework will always clamp the rate to the current range found on the clock, which will fix both these inconsistencies. Signed-off-by: Maxime Ripard --- drivers/clk/clk.c | 2 ++ drivers/clk/clk_test.c | 46 +- 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 112911138a7b..6c4e10209568 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1348,6 +1348,8 @@ static int clk_core_determine_round_nolock(struct clk_core *core, if (!core) return 0; + req->rate = clamp(req->rate, req->min_rate, req->max_rate); + /* * At this point, core protection will be disabled * - if the provider is not protected at all diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c index 0ca6cd391c8e..5a740f301f67 100644 --- a/drivers/clk/clk_test.c +++ b/drivers/clk/clk_test.c @@ -309,8 +309,7 @@ static void clk_range_test_multiple_disjoints_range(struct kunit *test) /* * Test that if our clock has some boundaries and we try to round a rate - * lower than the minimum, the returned rate won't be affected by the - * boundaries. + * lower than the minimum, the returned rate will be within range. */ static void clk_range_test_set_range_round_rate_lower(struct kunit *test) { @@ -327,18 +326,19 @@ static void clk_range_test_set_range_round_rate_lower(struct kunit *test) rate = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000); KUNIT_ASSERT_GT(test, rate, 0); - KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1 - 1000); + KUNIT_EXPECT_TRUE(test, rate >= DUMMY_CLOCK_RATE_1 && rate <= DUMMY_CLOCK_RATE_2); } /* * Test that if our clock has some boundaries and we try to set a rate - * lower than the minimum, we'll get an error. + * higher than the maximum, the new rate will be within range. */ static void clk_range_test_set_range_set_rate_lower(struct kunit *test) { struct clk_dummy_context *ctx = test->priv; struct clk_hw *hw = &ctx->hw; struct clk *clk = hw->clk; + unsigned long rate; KUNIT_ASSERT_EQ(test, clk_set_rate_range(clk, @@ -346,15 +346,20 @@ static void clk_range_test_set_range_set_rate_lower(struct kunit *test) DUMMY_CLOCK_RATE_2), 0); - KUNIT_ASSERT_LT(test, + KUNIT_ASSERT_EQ(test, clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000), 0); + + rate = clk_get_rate(clk); + KUNIT_ASSERT_GT(test, rate, 0); + KUNIT_EXPECT_TRUE(test, rate >= DUMMY_CLOCK_RATE_1 && rate <= DUMMY_CLOCK_RATE_2); } /* * Test that if our clock has some boundaries and we try to round and - * set a rate lower than the minimum, the values won't be consistent - * between clk_round_rate() and clk_set_rate(). + * set a rate lower than the minimum, the rate returned by + * clk_round_rate() will be consistent with the new rate set by + * clk_set_rate(). */ static void clk_range_test_set_range_set_round_rate_consistent_lower(struct kunit *test) { @@ -372,17 +377,16 @@ static void clk_range_test_set_range_set_round_rate_consistent_lower(struct kuni rounded = clk_round_rate(clk, DUMMY_CLOCK_RATE_1 - 1000); KUNIT_ASSERT_GT(test, rounded, 0); - KUNIT_EXPECT_LT(test, + KUNIT_ASSERT_EQ(test, clk_set_rate(clk, DUMMY_CLOCK_RATE_1 - 1000), 0); - KUNIT_EXPECT_NE(test, rounded, clk_get_rate(clk)); + KUNIT_EXPECT_EQ(test, rounded, clk_get_rate(clk)); } /* * Test that if our clock has some boundaries and we try to round a rate - * higher than the maximum, the returned rate won't be affected by the - * boundaries. + * higher than the maximum, the returned rate will be within range. */ static void clk_range_test_set_range_round_rate_higher(struct kunit *test) { @@ -399,18 +403,19 @@ static void clk_range_test_set_range_round_rate_higher(struct kunit *test) rate = clk_round_rate(clk, D
[PATCH v6 07/12] clk: Add clk_drop_range
In order to reset the range on a clock, we need to call clk_set_rate_range with a minimum of 0 and a maximum of ULONG_MAX. Since it's fairly inconvenient, let's introduce a clk_drop_range() function that will do just this. Suggested-by: Stephen Boyd Signed-off-by: Maxime Ripard --- drivers/clk/clk_test.c | 4 ++-- include/linux/clk.h| 11 +++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c index 60c206d1bcb0..490d364642b6 100644 --- a/drivers/clk/clk_test.c +++ b/drivers/clk/clk_test.c @@ -640,7 +640,7 @@ static void clk_range_test_multiple_set_range_rate_maximized(struct kunit *test) KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_1); KUNIT_ASSERT_EQ(test, - clk_set_rate_range(user2, 0, ULONG_MAX), + clk_drop_range(user2), 0); rate = clk_get_rate(clk); @@ -757,7 +757,7 @@ static void clk_range_test_multiple_set_range_rate_minimized(struct kunit *test) KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2); KUNIT_ASSERT_EQ(test, - clk_set_rate_range(user2, 0, ULONG_MAX), + clk_drop_range(user2), 0); rate = clk_get_rate(clk); diff --git a/include/linux/clk.h b/include/linux/clk.h index 266e8de3cb51..39faa54efe88 100644 --- a/include/linux/clk.h +++ b/include/linux/clk.h @@ -986,6 +986,17 @@ static inline void clk_bulk_disable_unprepare(int num_clks, clk_bulk_unprepare(num_clks, clks); } +/** + * clk_drop_range - Reset any range set on that clock + * @clk: clock source + * + * Returns success (0) or negative errno. + */ +static inline int clk_drop_range(struct clk *clk) +{ + return clk_set_rate_range(clk, 0, ULONG_MAX); +} + /** * clk_get_optional - lookup and obtain a reference to an optional clock * producer. -- 2.35.1
[PATCH v6 10/12] clk: bcm: rpi: Run some clocks at the minimum rate allowed
The core clock and M2MC clocks are shared between some devices (Unicam controllers and the HVS, and the HDMI controllers, respectively) that will have various, varying, requirements depending on their current work load. Since those loads can require a fairly high clock rate in extreme conditions (up to ~600MHz), we can end up running those clocks at their maximum frequency even though we no longer require such a high rate. Fortunately, those devices don't require an exact rate but a minimum rate, and all the drivers are using clk_set_min_rate. Thus, we can just rely on the fact that the clk_request minimum (which is the aggregated minimum of all the clock users) is what we want at all times. Signed-off-by: Maxime Ripard --- drivers/clk/bcm/clk-raspberrypi.c | 37 +++ 1 file changed, 37 insertions(+) diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c index c879f2e9a4a7..9d09621549b9 100644 --- a/drivers/clk/bcm/clk-raspberrypi.c +++ b/drivers/clk/bcm/clk-raspberrypi.c @@ -77,6 +77,7 @@ struct raspberrypi_clk_variant { boolexport; char*clkdev; unsigned long min_rate; + boolminimize; }; static struct raspberrypi_clk_variant @@ -87,6 +88,18 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = { }, [RPI_FIRMWARE_CORE_CLK_ID] = { .export = true, + + /* +* The clock is shared between the HVS and the CSI +* controllers, on the BCM2711 and will change depending +* on the pixels composited on the HVS and the capture +* resolution on Unicam. +* +* Since the rate can get quite large, and we need to +* coordinate between both driver instances, let's +* always use the minimum the drivers will let us. +*/ + .minimize = true, }, [RPI_FIRMWARE_M2MC_CLK_ID] = { .export = true, @@ -102,6 +115,16 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = { * in this situation. */ .min_rate = 12000, + + /* +* The clock is shared between the two HDMI controllers +* on the BCM2711 and will change depending on the +* resolution output on each. Since the rate can get +* quite large, and we need to coordinate between both +* driver instances, let's always use the minimum the +* drivers will let us. +*/ + .minimize = true, }, [RPI_FIRMWARE_V3D_CLK_ID] = { .export = true, @@ -206,12 +229,26 @@ static int raspberrypi_fw_set_rate(struct clk_hw *hw, unsigned long rate, static int raspberrypi_fw_dumb_determine_rate(struct clk_hw *hw, struct clk_rate_request *req) { + struct raspberrypi_clk_data *data = + container_of(hw, struct raspberrypi_clk_data, hw); + struct raspberrypi_clk_variant *variant = data->variant; + /* * The firmware will do the rounding but that isn't part of * the interface with the firmware, so we just do our best * here. */ + req->rate = clamp(req->rate, req->min_rate, req->max_rate); + + /* +* We want to aggressively reduce the clock rate here, so let's +* just ignore the requested rate and return the bare minimum +* rate we can get away with. +*/ + if (variant->minimize && req->min_rate > 0) + req->rate = req->min_rate; + return 0; } -- 2.35.1
[PATCH v6 11/12] drm/vc4: Add logging and comments
The HVS core clock isn't really obvious, so let's add a bunch more comments and some logging for easier debugging. Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_kms.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index 24de29bc1cda..6fe03fc17d73 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -389,8 +389,15 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state) 5, new_hvs_state->core_clock_rate); + drm_dbg(dev, "Raising the core clock at %lu Hz\n", core_rate); + + /* +* Do a temporary request on the core clock during the +* modeset. +*/ clk_set_min_rate(hvs->core_clk, core_rate); } + drm_atomic_helper_commit_modeset_disables(dev, state); vc4_ctm_commit(vc4, state); @@ -416,6 +423,10 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state) drm_dbg(dev, "Running the core clock at %lu Hz\n", new_hvs_state->core_clock_rate); + /* +* Request a clock rate based on the current HVS +* requirements. +*/ clk_set_min_rate(hvs->core_clk, new_hvs_state->core_clock_rate); } } -- 2.35.1
[PATCH v6 06/12] clk: Always set the rate on clk_set_range_rate
When we change a clock minimum or maximum using clk_set_rate_range(), clk_set_min_rate() or clk_set_max_rate(), the current code will only trigger a new rate change if the rate is outside of the new boundaries. However, a clock driver might want to always keep the clock rate to one of its boundary, for example the minimum to keep the power consumption as low as possible. Since they don't always get called though, clock providers don't have the opportunity to implement this behaviour. Let's trigger a clk_set_rate() on the previous requested rate every time clk_set_rate_range() is called. That way, providers that care about the new boundaries have a chance to adjust the rate, while providers that don't care about those new boundaries will return the same rate than before, which will be ignored by clk_set_rate() and won't result in a new rate change. Suggested-by: Stephen Boyd Signed-off-by: Maxime Ripard --- drivers/clk/clk.c | 45 drivers/clk/clk_test.c | 58 +++--- 2 files changed, 49 insertions(+), 54 deletions(-) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index c15ee5070f52..9bc8bf434b94 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2373,28 +2373,29 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max) goto out; } - rate = clk_core_get_rate_nolock(clk->core); - if (rate < min || rate > max) { - /* -* FIXME: -* We are in bit of trouble here, current rate is outside the -* the requested range. We are going try to request appropriate -* range boundary but there is a catch. It may fail for the -* usual reason (clock broken, clock protected, etc) but also -* because: -* - round_rate() was not favorable and fell on the wrong -* side of the boundary -* - the determine_rate() callback does not really check for -* this corner case when determining the rate -*/ - - rate = clamp(clk->core->req_rate, min, max); - ret = clk_core_set_rate_nolock(clk->core, rate); - if (ret) { - /* rollback the changes */ - clk->min_rate = old_min; - clk->max_rate = old_max; - } + /* +* Since the boundaries have been changed, let's give the +* opportunity to the provider to adjust the clock rate based on +* the new boundaries. +* +* We also need to handle the case where the clock is currently +* outside of the boundaries. Clamping the last requested rate +* to the current minimum and maximum will also handle this. +* +* FIXME: +* There is a catch. It may fail for the usual reason (clock +* broken, clock protected, etc) but also because: +* - round_rate() was not favorable and fell on the wrong +* side of the boundary +* - the determine_rate() callback does not really check for +* this corner case when determining the rate +*/ + rate = clamp(clk->core->req_rate, min, max); + ret = clk_core_set_rate_nolock(clk->core, rate); + if (ret) { + /* rollback the changes */ + clk->min_rate = old_min; + clk->max_rate = old_max; } out: diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c index 5a740f301f67..60c206d1bcb0 100644 --- a/drivers/clk/clk_test.c +++ b/drivers/clk/clk_test.c @@ -544,13 +544,12 @@ static struct kunit_suite clk_range_test_suite = { }; /* - * Test that if: - * - we have several subsequent calls to clk_set_rate_range(); - * - and we have a round_rate ops that always return the maximum - * frequency allowed; + * Test that if we have several subsequent calls to + * clk_set_rate_range(), the core will reevaluate whether a new rate is + * needed each and every time. * - * The clock will run at the minimum of all maximum boundaries - * requested, even if those boundaries aren't there anymore. + * With clk_dummy_maximize_rate_ops, this means that the the rate will + * trail along the maximum as it evolves. */ static void clk_range_test_set_range_rate_maximized(struct kunit *test) { @@ -591,18 +590,16 @@ static void clk_range_test_set_range_rate_maximized(struct kunit *test) rate = clk_get_rate(clk); KUNIT_ASSERT_GT(test, rate, 0); - KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2 - 1000); + KUNIT_EXPECT_EQ(test, rate, DUMMY_CLOCK_RATE_2); } /* - * Test that if: - * - we have several subsequent calls to clk_set_rate_range(), across - * multiple users; - * - and we have a round_rate ops that always return the maximum - * frequency allowed; + * Test that if we have several subsequent calls to + * c
[PATCH v6 12/12] drm/vc4: hdmi: Remove clock rate initialization
Now that the clock driver makes sure we never end up with a rate of 0, the HDMI driver doesn't need to care anymore. Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_hdmi.c | 13 - 1 file changed, 13 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 92b1530aa17b..21aff3ad96cf 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2576,19 +2576,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) vc4_hdmi->disable_4kp60 = true; } - /* -* If we boot without any cable connected to the HDMI connector, -* the firmware will skip the HSM initialization and leave it -* with a rate of 0, resulting in a bus lockup when we're -* accessing the registers even if it's enabled. -* -* Let's put a sensible default at runtime_resume so that we -* don't end up in this situation. -*/ - ret = clk_set_min_rate(vc4_hdmi->hsm_clock, HSM_MIN_CLOCK_FREQ); - if (ret) - goto err_put_ddc; - /* * We need to have the device powered up at this point to call * our reset hook and for the CEC init. -- 2.35.1
[PATCH v6 09/12] clk: bcm: rpi: Set a default minimum rate
The M2MC clock provides the state machine clock for both HDMI controllers. However, if no HDMI monitor is plugged in at boot, its clock rate will be left at 0 by the firmware and will make any register access end up in a CPU stall, even though the clock was enabled. We had some code in the HDMI controller to deal with this before, but it makes more sense to have it in the clock driver. Move it there. Signed-off-by: Maxime Ripard --- drivers/clk/bcm/clk-raspberrypi.c | 26 ++ 1 file changed, 26 insertions(+) diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c index f7185d421085..c879f2e9a4a7 100644 --- a/drivers/clk/bcm/clk-raspberrypi.c +++ b/drivers/clk/bcm/clk-raspberrypi.c @@ -76,6 +76,7 @@ struct raspberrypi_clk_data { struct raspberrypi_clk_variant { boolexport; char*clkdev; + unsigned long min_rate; }; static struct raspberrypi_clk_variant @@ -89,6 +90,18 @@ raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = { }, [RPI_FIRMWARE_M2MC_CLK_ID] = { .export = true, + + /* +* If we boot without any cable connected to any of the +* HDMI connector, the firmware will skip the HSM +* initialization and leave it with a rate of 0, +* resulting in a bus lockup when we're accessing the +* registers even if it's enabled. +* +* Let's put a sensible default so that we don't end up +* in this situation. +*/ + .min_rate = 12000, }, [RPI_FIRMWARE_V3D_CLK_ID] = { .export = true, @@ -267,6 +280,19 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi, } } + if (variant->min_rate) { + unsigned long rate; + + clk_hw_set_rate_range(&data->hw, variant->min_rate, max_rate); + + rate = raspberrypi_fw_get_rate(&data->hw, 0); + if (rate < variant->min_rate) { + ret = raspberrypi_fw_set_rate(&data->hw, variant->min_rate, 0); + if (ret) + return ERR_PTR(ret); + } + } + return &data->hw; } -- 2.35.1
[PATCH v6 08/12] clk: bcm: rpi: Add variant structure
We only export a bunch of firmware clocks, and some of them require special treatment. This has been do so far using some tests on the clock id in various places, but this is fairly hard to extend and doesn't scale very well. Since we'll need some more cases in the next patches, let's switch to a variant structure that defines the behaviour we need to have for a given clock. Signed-off-by: Maxime Ripard --- drivers/clk/bcm/clk-raspberrypi.c | 62 +++ 1 file changed, 46 insertions(+), 16 deletions(-) diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c index dd3b71eafabf..f7185d421085 100644 --- a/drivers/clk/bcm/clk-raspberrypi.c +++ b/drivers/clk/bcm/clk-raspberrypi.c @@ -56,6 +56,8 @@ static char *rpi_firmware_clk_names[] = { #define RPI_FIRMWARE_STATE_ENABLE_BIT BIT(0) #define RPI_FIRMWARE_STATE_WAIT_BITBIT(1) +struct raspberrypi_clk_variant; + struct raspberrypi_clk { struct device *dev; struct rpi_firmware *firmware; @@ -66,10 +68,36 @@ struct raspberrypi_clk_data { struct clk_hw hw; unsigned int id; + struct raspberrypi_clk_variant *variant; struct raspberrypi_clk *rpi; }; +struct raspberrypi_clk_variant { + boolexport; + char*clkdev; +}; + +static struct raspberrypi_clk_variant +raspberrypi_clk_variants[RPI_FIRMWARE_NUM_CLK_ID] = { + [RPI_FIRMWARE_ARM_CLK_ID] = { + .export = true, + .clkdev = "cpu0", + }, + [RPI_FIRMWARE_CORE_CLK_ID] = { + .export = true, + }, + [RPI_FIRMWARE_M2MC_CLK_ID] = { + .export = true, + }, + [RPI_FIRMWARE_V3D_CLK_ID] = { + .export = true, + }, + [RPI_FIRMWARE_PIXEL_BVB_CLK_ID] = { + .export = true, + }, +}; + /* * Structure of the message passed to Raspberry Pi's firmware in order to * change clock rates. The 'disable_turbo' option is only available to the ARM @@ -183,7 +211,8 @@ static const struct clk_ops raspberrypi_firmware_clk_ops = { static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi, unsigned int parent, - unsigned int id) + unsigned int id, + struct raspberrypi_clk_variant *variant) { struct raspberrypi_clk_data *data; struct clk_init_data init = {}; @@ -195,6 +224,7 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi, return ERR_PTR(-ENOMEM); data->rpi = rpi; data->id = id; + data->variant = variant; init.name = devm_kasprintf(rpi->dev, GFP_KERNEL, "fw-clk-%s", @@ -228,9 +258,9 @@ static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi, clk_hw_set_rate_range(&data->hw, min_rate, max_rate); - if (id == RPI_FIRMWARE_ARM_CLK_ID) { + if (variant->clkdev) { ret = devm_clk_hw_register_clkdev(rpi->dev, &data->hw, - NULL, "cpu0"); + NULL, variant->clkdev); if (ret) { dev_err(rpi->dev, "Failed to initialize clkdev\n"); return ERR_PTR(ret); @@ -264,27 +294,27 @@ static int raspberrypi_discover_clocks(struct raspberrypi_clk *rpi, return ret; while (clks->id) { - struct clk_hw *hw; + struct raspberrypi_clk_variant *variant; + + if (clks->id > RPI_FIRMWARE_NUM_CLK_ID) { + dev_err(rpi->dev, "Unknown clock id: %u", clks->id); + return -EINVAL; + } + + variant = &raspberrypi_clk_variants[clks->id]; + if (variant->export) { + struct clk_hw *hw; - switch (clks->id) { - case RPI_FIRMWARE_ARM_CLK_ID: - case RPI_FIRMWARE_CORE_CLK_ID: - case RPI_FIRMWARE_M2MC_CLK_ID: - case RPI_FIRMWARE_V3D_CLK_ID: - case RPI_FIRMWARE_PIXEL_BVB_CLK_ID: hw = raspberrypi_clk_register(rpi, clks->parent, - clks->id); + clks->id, variant); if (IS_ERR(hw)) return PTR_ERR(hw); data->hws[clks->id] = hw; data->num = clks->id + 1; - fallthrough; - - default: - clks++; - break; } + + clks++; } return 0; -- 2.35.1
Re: [Intel-gfx] [PATCH 0/7] drm/i915: Use the memcpy_from_wc function from drm
On 23.02.2022 10:02, Das, Nirmoy wrote: > > On 22/02/2022 15:51, Balasubramani Vivekanandan wrote: > > drm_memcpy_from_wc() performs fast copy from WC memory type using > > non-temporal instructions. Now there are two similar implementations of > > this function. One exists in drm_cache.c as drm_memcpy_from_wc() and > > another implementation in i915/i915_memcpy.c as i915_memcpy_from_wc(). > > drm_memcpy_from_wc() was the recent addition through the series > > https://patchwork.freedesktop.org/patch/436276/?series=90681&rev=6 > > > > The goal of this patch series is to change all users of > > i915_memcpy_from_wc() to drm_memcpy_from_wc() and a have common > > implementation in drm and eventually remove the copy from i915. > > > > Another benefit of using memcpy functions from drm is that > > drm_memcpy_from_wc() is available for non-x86 architectures. > > i915_memcpy_from_wc() is implemented only for x86 and prevents building > > i915 for ARM64. > > drm_memcpy_from_wc() does fast copy using non-temporal instructions for > > x86 and for other architectures makes use of memcpy() family of > > functions as fallback. > > > > Another major difference is unlike i915_memcpy_from_wc(), > > drm_memcpy_from_wc() will not fail if the passed address argument is not > > alignment to be used with non-temporal load instructions or if the > > platform lacks support for those instructions (non-temporal load > > instructions are provided through SSE4.1 instruction set extension). > > Instead drm_memcpy_from_wc() continues with fallback functions to > > complete the copy. > > This relieves the caller from checking the return value of > > i915_memcpy_from_wc() and explicitly using a fallback. > > > > Follow up series will be created to remove the memcpy_from_wc functions > > from i915 once the dependency is completely removed. > > Overall the series looks good to me but I think you can add another patch to > remove > > i915_memcpy_from_wc() as I don't see any other usages left after this series, > may be I > am missing something? I have changed all users of i915_memcpy_from_wc() to drm function. But this is another function i915_unaligned_memcpy_from_wc() in i915_memcpy.c which is blocking completely eliminating the i915_memcpy.c file from i915. This function accepts unaligned source address and does fast copy only for the aligned region of memory and remaining part is copied using memcpy function. Either I can move i915_unaligned_memcpy_from_wc() also to drm but I am concerned since it is more a platform specific handling, does it make sense to keep it in drm. Else I have retain to i915_unaligned_memcpy_from_wc() inside i915 and refactor the function to use drm_memcpy_from_wc() instead of the __memcpy_ntdqu(). But before I could do more changes, I wanted feedback on the current change. So I decided to go ahead with creating series for review. Regards, Bala > > Regards, > Nirmoy > > > > > Cc: Jani Nikula > > Cc: Lucas De Marchi > > Cc: David Airlie > > Cc: Daniel Vetter > > Cc: Chris Wilson > > Cc: Thomas Hellstr_m > > Cc: Joonas Lahtinen > > Cc: Rodrigo Vivi > > Cc: Tvrtko Ursulin > > > > Balasubramani Vivekanandan (7): > >drm: Relax alignment constraint for destination address > >drm: Add drm_memcpy_from_wc() variant which accepts destination > > address > >drm/i915: use the memcpy_from_wc call from the drm > >drm/i915/guc: use the memcpy_from_wc call from the drm > >drm/i915/selftests: use the memcpy_from_wc call from the drm > >drm/i915/gt: Avoid direct dereferencing of io memory > >drm/i915: Avoid dereferencing io mapped memory > > > > drivers/gpu/drm/drm_cache.c | 98 +-- > > drivers/gpu/drm/i915/gem/i915_gem_object.c| 8 +- > > drivers/gpu/drm/i915/gt/selftest_reset.c | 21 ++-- > > drivers/gpu/drm/i915/gt/uc/intel_guc_log.c| 11 ++- > > drivers/gpu/drm/i915/i915_gpu_error.c | 45 + > > .../drm/i915/selftests/intel_memory_region.c | 8 +- > > include/drm/drm_cache.h | 3 + > > 7 files changed, 148 insertions(+), 46 deletions(-) > >
Re: [PATCH] drm/simpledrm: Add "panel orientation" property on non-upright mounted LCD panels
Hi, On 2/22/22 20:14, Thomas Zimmermann wrote: > Hi > > Am 21.02.22 um 23:00 schrieb Hans de Goede: >> Some devices use e.g. a portrait panel in a standard laptop casing made >> for landscape panels. efifb calls drm_get_panel_orientation_quirk() and >> sets fb_info.fbcon_rotate_hint to make fbcon rotate the console so that >> it shows up-right instead of on its side. >> >> When switching to simpledrm to fbcon renders on its side. Call the > > Maybe '... fbcon renders sidewards.' that does not sound entirely right to me, so I've gone with: "When switching to simpledrm the fbcon renders on its side." as suggested by Javier (so s/to/the/ ). > >> drm_connector_set_panel_orientation_with_quirk() helper to add >> a "panel orientation" property on devices listed in the quirk table, >> to make the fbcon (and aware userspace apps) rotate the image to >> display properly. >> >> Cc: Javier Martinez Canillas >> Signed-off-by: Hans de Goede > > Acked-by: Thomas Zimmermann Thank you both for the review/ack. I'm currently doing a test-build of drm-misc-next with the patch with amended commit msg applied. Once that is done I'll push this out to drm-misc-next. Regards, Hans >> --- >> drivers/gpu/drm/tiny/simpledrm.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/gpu/drm/tiny/simpledrm.c >> b/drivers/gpu/drm/tiny/simpledrm.c >> index 04146da2d1d8..11576e0297e4 100644 >> --- a/drivers/gpu/drm/tiny/simpledrm.c >> +++ b/drivers/gpu/drm/tiny/simpledrm.c >> @@ -798,6 +798,9 @@ static int simpledrm_device_init_modeset(struct >> simpledrm_device *sdev) >> if (ret) >> return ret; >> drm_connector_helper_add(connector, &simpledrm_connector_helper_funcs); >> + drm_connector_set_panel_orientation_with_quirk(connector, >> + DRM_MODE_PANEL_ORIENTATION_UNKNOWN, >> + mode->hdisplay, mode->vdisplay); >> formats = simpledrm_device_formats(sdev, &nformats); >> >
[PATCH v2] drm/msm/dpu: Bind pingpong block to intf on active ctls in cmd encoder
As per the specification of DPU_CTL_ACTIVE_CFG the configuration of active blocks should be proactively specified, and the pingpong block is no different. The downstream display driver [1] confirms this by also calling bind_pingpong_blk on CTL_ACTIVE_CFG. Note that this else-if is always entered, as setup_intf_cfg - unlike this mainline dpu driver that combines both behind the same function pointer - is left NULL in favour of using setup_intf_cfg_v1 when CTL_ACTIVE_CFG is set. This solves continuous timeouts on at least the Qualcomm sm6125 SoC: [drm:dpu_encoder_frame_done_timeout:2091] [dpu error]enc31 frame done timeout [drm:_dpu_encoder_phys_cmd_handle_ppdone_timeout.isra.0] *ERROR* id:31 pp:0 kickoff timeout 0 cnt 1 koff_cnt 1 [drm:dpu_encoder_phys_cmd_prepare_for_kickoff] *ERROR* failed wait_for_idle: id:31 ret:-110 pp:0 In the same way this pingpong block should also be unbound followed by an interface flush when the encoder is disabled, according to the downstream display driver [2]. [1]: https://source.codeaurora.org/quic/la/platform/vendor/opensource/display-drivers/tree/msm/sde/sde_encoder_phys_cmd.c?h=LA.UM.9.16.r1-08500-MANNAR.0#n167 [2]: https://source.codeaurora.org/quic/la/platform/vendor/opensource/display-drivers/tree/msm/sde/sde_encoder.c?h=LA.UM.9.16.r1-08500-MANNAR.0#n2986 Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno --- Changes since v1: - Always unbind the pingpong block in dpu_encoder_phys_cmd_disable, instead of only if this encoder is the master. v1: https://lore.kernel.org/lkml/20211222105513.44860-1-marijn.suij...@somainline.org/ .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 19 +++ 1 file changed, 19 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c index 8e433af7aea4..1be01cbd960e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c @@ -71,6 +71,13 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg( intf_cfg.stream_sel = cmd_enc->stream_sel; intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc); ctl->ops.setup_intf_cfg(ctl, &intf_cfg); + + /* setup which pp blk will connect to this intf */ + if (test_bit(DPU_CTL_ACTIVE_CFG, &ctl->caps->features) && phys_enc->hw_intf->ops.bind_pingpong_blk) + phys_enc->hw_intf->ops.bind_pingpong_blk( + phys_enc->hw_intf, + true, + phys_enc->hw_pp->idx); } static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx) @@ -507,6 +514,7 @@ static void dpu_encoder_phys_cmd_disable(struct dpu_encoder_phys *phys_enc) { struct dpu_encoder_phys_cmd *cmd_enc = to_dpu_encoder_phys_cmd(phys_enc); + struct dpu_hw_ctl *ctl; if (!phys_enc->hw_pp) { DPU_ERROR("invalid encoder\n"); @@ -523,6 +531,17 @@ static void dpu_encoder_phys_cmd_disable(struct dpu_encoder_phys *phys_enc) if (phys_enc->hw_pp->ops.enable_tearcheck) phys_enc->hw_pp->ops.enable_tearcheck(phys_enc->hw_pp, false); + + if (phys_enc->hw_intf->ops.bind_pingpong_blk) { + phys_enc->hw_intf->ops.bind_pingpong_blk( + phys_enc->hw_intf, + false, + phys_enc->hw_pp->idx); + + ctl = phys_enc->hw_ctl; + ctl->ops.update_pending_flush_intf(ctl, phys_enc->intf_idx); + } + phys_enc->enable_state = DPU_ENC_DISABLED; } base-commit: 3c30cf91b5ecc7272b3d2942ae0505dd8320b81c -- 2.35.1
[PATCH] drm/panel: boe-tv101wum-nl6: Fix errors cases handling in prepare function
Make sure that pp3300 regulator and enable gpio are cleaned before leave in error cases. Fixes: 18c58153b8c62 ("drm/panel: boe-tv101wum-nl6: Support enabling a 3.3V rail") Signed-off-by: Benjamin Gaignard --- drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c index 5fcbde789ddb..382a17bb96d8 100644 --- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c +++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c @@ -1245,11 +1245,11 @@ static int boe_panel_prepare(struct drm_panel *panel) ret = regulator_enable(boe->pp3300); if (ret < 0) - return ret; + goto disablegpio; ret = regulator_enable(boe->pp1800); if (ret < 0) - return ret; + goto poweroff3v3; usleep_range(3000, 5000); @@ -1286,6 +1286,9 @@ static int boe_panel_prepare(struct drm_panel *panel) poweroff1v8: usleep_range(5000, 7000); regulator_disable(boe->pp1800); +poweroff3v3: + regulator_disable(boe->pp3300); +disablegpio: gpiod_set_value(boe->enable_gpio, 0); return ret; -- 2.32.0
Re: [PATCH v2] drm/msm/dpu: Bind pingpong block to intf on active ctls in cmd encoder
On Wed, 23 Feb 2022 at 14:40, Marijn Suijten wrote: > > As per the specification of DPU_CTL_ACTIVE_CFG the configuration of > active blocks should be proactively specified, and the pingpong block is > no different. > > The downstream display driver [1] confirms this by also calling > bind_pingpong_blk on CTL_ACTIVE_CFG. Note that this else-if is always > entered, as setup_intf_cfg - unlike this mainline dpu driver that > combines both behind the same function pointer - is left NULL in favour > of using setup_intf_cfg_v1 when CTL_ACTIVE_CFG is set. > > This solves continuous timeouts on at least the Qualcomm sm6125 SoC: > > [drm:dpu_encoder_frame_done_timeout:2091] [dpu error]enc31 frame done > timeout > [drm:_dpu_encoder_phys_cmd_handle_ppdone_timeout.isra.0] *ERROR* id:31 > pp:0 kickoff timeout 0 cnt 1 koff_cnt 1 > [drm:dpu_encoder_phys_cmd_prepare_for_kickoff] *ERROR* failed > wait_for_idle: id:31 ret:-110 pp:0 > > In the same way this pingpong block should also be unbound followed by > an interface flush when the encoder is disabled, according to the > downstream display driver [2]. > > [1]: > https://source.codeaurora.org/quic/la/platform/vendor/opensource/display-drivers/tree/msm/sde/sde_encoder_phys_cmd.c?h=LA.UM.9.16.r1-08500-MANNAR.0#n167 > [2]: > https://source.codeaurora.org/quic/la/platform/vendor/opensource/display-drivers/tree/msm/sde/sde_encoder.c?h=LA.UM.9.16.r1-08500-MANNAR.0#n2986 > > Signed-off-by: Marijn Suijten > Reviewed-by: AngeloGioacchino Del Regno > Reviewed-by: Dmitry Baryshkov As we are quite late in the development cycle, I'd delay this into the next release, if you don't mind. > --- > > Changes since v1: > - Always unbind the pingpong block in dpu_encoder_phys_cmd_disable, > instead of only if this encoder is the master. > > v1: > https://lore.kernel.org/lkml/20211222105513.44860-1-marijn.suij...@somainline.org/ > > .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 19 +++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > index 8e433af7aea4..1be01cbd960e 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > @@ -71,6 +71,13 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg( > intf_cfg.stream_sel = cmd_enc->stream_sel; > intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc); > ctl->ops.setup_intf_cfg(ctl, &intf_cfg); > + > + /* setup which pp blk will connect to this intf */ > + if (test_bit(DPU_CTL_ACTIVE_CFG, &ctl->caps->features) && > phys_enc->hw_intf->ops.bind_pingpong_blk) > + phys_enc->hw_intf->ops.bind_pingpong_blk( > + phys_enc->hw_intf, > + true, > + phys_enc->hw_pp->idx); > } > > static void dpu_encoder_phys_cmd_pp_tx_done_irq(void *arg, int irq_idx) > @@ -507,6 +514,7 @@ static void dpu_encoder_phys_cmd_disable(struct > dpu_encoder_phys *phys_enc) > { > struct dpu_encoder_phys_cmd *cmd_enc = > to_dpu_encoder_phys_cmd(phys_enc); > + struct dpu_hw_ctl *ctl; > > if (!phys_enc->hw_pp) { > DPU_ERROR("invalid encoder\n"); > @@ -523,6 +531,17 @@ static void dpu_encoder_phys_cmd_disable(struct > dpu_encoder_phys *phys_enc) > > if (phys_enc->hw_pp->ops.enable_tearcheck) > phys_enc->hw_pp->ops.enable_tearcheck(phys_enc->hw_pp, false); > + > + if (phys_enc->hw_intf->ops.bind_pingpong_blk) { > + phys_enc->hw_intf->ops.bind_pingpong_blk( > + phys_enc->hw_intf, > + false, > + phys_enc->hw_pp->idx); > + > + ctl = phys_enc->hw_ctl; > + ctl->ops.update_pending_flush_intf(ctl, phys_enc->intf_idx); > + } > + > phys_enc->enable_state = DPU_ENC_DISABLED; > } > > > base-commit: 3c30cf91b5ecc7272b3d2942ae0505dd8320b81c > -- > 2.35.1 > -- With best wishes Dmitry
Re: [Intel-gfx] [PATCH 0/3] Improve anti-pre-emption w/a for compute workloads
On 23/02/2022 02:22, John Harrison wrote: On 2/22/2022 01:53, Tvrtko Ursulin wrote: On 18/02/2022 21:33, john.c.harri...@intel.com wrote: From: John Harrison Compute workloads are inherently not pre-emptible on current hardware. Thus the pre-emption timeout was disabled as a workaround to prevent unwanted resets. Instead, the hang detection was left to the heartbeat and its (longer) timeout. This is undesirable with GuC submission as the heartbeat is a full GT reset rather than a per engine reset and so is much more destructive. Instead, just bump the pre-emption timeout Can we have a feature request to allow asking GuC for an engine reset? For what purpose? To allow "stopped heartbeat" to reset the engine, however.. GuC manages the scheduling of contexts across engines. With virtual engines, the KMD has no knowledge of which engine a context might be executing on. Even without virtual engines, the KMD still has no knowledge of which context is currently executing on any given engine at any given time. There is a reason why hang detection should be left to the entity that is doing the scheduling. Any other entity is second guessing at best. The reason for keeping the heartbeat around even when GuC submission is enabled is for the case where the KMD/GuC have got out of sync with either other somehow or GuC itself has just crashed. I.e. when no submission at all is working and we need to reset the GuC itself and start over. .. I wasn't really up to speed to know/remember heartbeats are nerfed already in GuC mode. I am not sure it was the best way since full reset penalizes everyone for one hanging engine. Better question would be why leave heartbeats around to start with with GuC? If you want to use it to health check GuC, as you say, maybe just send a CT message and expect replies? Then full reset would make sense. It also achieves the goal of not seconding guessing the submission backend you raise. Like it is now, and the need for this series demonstrates it, the whole thing has a pretty poor "impedance" match. Not even sure what intel_guc_find_hung_context is doing in intel_engine_hearbeat.c - why is that not in intel_gt_handle_error at least? Why is hearbeat code special and other callers of intel_gt_handle_error don't need it? Regards, Tvrtko
[PATCH][V2] drm/i915: make a handful of read-only arrays static const
Don't populate the read-only arrays on the stack but instead make them static const and signed 8 bit ints. Also makes the object code a little smaller. Reformat the statements to clear up checkpatch warning. Signed-off-by: Colin Ian King --- V2: Make arrays signed 8 bit integers as requested by Ville Syrjälä --- drivers/gpu/drm/i915/display/intel_vdsc.c | 16 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c index 3faea903b9ae..d49f66237ec3 100644 --- a/drivers/gpu/drm/i915/display/intel_vdsc.c +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c @@ -378,10 +378,18 @@ calculate_rc_params(struct rc_parameters *rc, { int bpc = vdsc_cfg->bits_per_component; int bpp = vdsc_cfg->bits_per_pixel >> 4; - int ofs_und6[] = { 0, -2, -2, -4, -6, -6, -8, -8, -8, -10, -10, -12, -12, -12, -12 }; - int ofs_und8[] = { 2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -10, -12, -12, -12 }; - int ofs_und12[] = { 2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -10, -12, -12, -12 }; - int ofs_und15[] = { 10, 8, 6, 4, 2, 0, -2, -4, -6, -8, -10, -10, -12, -12, -12 }; + static const s8 ofs_und6[] = { + 0, -2, -2, -4, -6, -6, -8, -8, -8, -10, -10, -12, -12, -12, -12 + }; + static const s8 ofs_und8[] = { + 2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -10, -12, -12, -12 + }; + static const s8 ofs_und12[] = { + 2, 0, 0, -2, -4, -6, -8, -8, -8, -10, -10, -10, -12, -12, -12 + }; + static const s8 ofs_und15[] = { + 10, 8, 6, 4, 2, 0, -2, -4, -6, -8, -10, -10, -12, -12, -12 + }; int qp_bpc_modifier = (bpc - 8) * 2; u32 res, buf_i, bpp_i; -- 2.34.1
Re: [Intel-gfx] [PATCH 1/3] drm/i915/guc: Limit scheduling properties to avoid overflow
On 23/02/2022 02:11, John Harrison wrote: On 2/22/2022 01:52, Tvrtko Ursulin wrote: On 18/02/2022 21:33, john.c.harri...@intel.com wrote: From: John Harrison GuC converts the pre-emption timeout and timeslice quantum values into clock ticks internally. That significantly reduces the point of 32bit overflow. On current platforms, worst case scenario is approximately Where does 32-bit come from, the GuC side? We already use 64-bits so that something to fix to start with. Yep... Yes, the GuC API is defined as 32bits only and then does a straight multiply by the clock speed with no range checking. We have requested 64bit support but there was push back on the grounds that it is not something the GuC timer hardware supports and such long timeouts are not real world usable anyway. As long as compute are happy with 100 seconds, then it "should be enough for everbody". :D ./gt/uc/intel_guc_fwif.h: u32 execution_quantum; ./gt/uc/intel_guc_submission.c: desc->execution_quantum = engine->props.timeslice_duration_ms * 1000; ./gt/intel_engine_types.h: unsigned long timeslice_duration_ms; timeslice_store/preempt_timeout_store: err = kstrtoull(buf, 0, &duration); So both kconfig and sysfs can already overflow GuC, not only because of tick conversion internally but because at backend level nothing was done for assigning 64-bit into 32-bit. Or I failed to find where it is handled. That's why I'm adding this range check to make sure we don't allow overflows. Yes and no, this fixes it, but the first bug was not only due GuC internal tick conversion. It was present ever since the u64 from i915 was shoved into u32 sent to GuC. So even if GuC used the value without additional multiplication, bug was be there. My point being when GuC backend was added timeout_ms values should have been limited/clamped to U32_MAX. The tick discovery is additional limit on top. 110 seconds. Rather than allowing the user to set higher values and then get confused by early timeouts, add limits when setting these values. Btw who is reviewing GuC patches these days - things have somehow gotten pretty quiet in activity and I don't think that's due absence of stuff to improve or fix? Asking since I think I noticed a few already which you posted and then crickets on the mailing list. Too much work to do and not enough engineers to do it all :(. Signed-off-by: John Harrison --- drivers/gpu/drm/i915/gt/intel_engine_cs.c | 15 +++ drivers/gpu/drm/i915/gt/sysfs_engines.c | 14 ++ drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 9 + 3 files changed, 38 insertions(+) diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index e53008b4dd05..2a1e9f36e6f5 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -389,6 +389,21 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id, if (GRAPHICS_VER(i915) == 12 && engine->class == RENDER_CLASS) engine->props.preempt_timeout_ms = 0; + /* Cap timeouts to prevent overflow inside GuC */ + if (intel_guc_submission_is_wanted(>->uc.guc)) { + if (engine->props.timeslice_duration_ms > GUC_POLICY_MAX_EXEC_QUANTUM_MS) { Hm "wanted".. There's been too much back and forth on the GuC load options over the years to keep track.. intel_engine_uses_guc work sounds like would work and read nicer. I'm not adding a new feature check here. I'm just using the existing one. If we want to rename it yet again then that would be a different patch set. $ grep intel_engine_uses_guc . -rl ./i915_perf.c ./i915_request.c ./selftests/intel_scheduler_helpers.c ./gem/i915_gem_context.c ./gt/intel_context.c ./gt/intel_engine.h ./gt/intel_engine_cs.c ./gt/intel_engine_heartbeat.c ./gt/intel_engine_pm.c ./gt/intel_reset.c ./gt/intel_lrc.c ./gt/selftest_context.c ./gt/selftest_engine_pm.c ./gt/selftest_hangcheck.c ./gt/selftest_mocs.c ./gt/selftest_workarounds.c Sounds better to me than intel_guc_submission_is_wanted. What does the reader know whether "is wanted" translates to "is actually used". Shrug on "is wanted". And limit to class instead of applying to all engines looks like a miss. As per follow up email, the class limit is not applied here. + drm_info(&engine->i915->drm, "Warning, clamping timeslice duration to %d to prevent possibly overflow\n", + GUC_POLICY_MAX_EXEC_QUANTUM_MS); + engine->props.timeslice_duration_ms = GUC_POLICY_MAX_EXEC_QUANTUM_MS; I am not sure logging such message during driver load is useful. Sounds more like a confused driver which starts with one value and then overrides itself. I'd just silently set the value appropriate for the active backend. Preemption timeout kconfig text already documents the fact timeouts can get overriden at runtime depending on platform+engine. So maybe just add same text to
Re: [PATCH v11 1/6] drm: Add arch arm64 for drm_clflush_virt_range
Hi Michael, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on drm-tip/drm-tip] [also build test WARNING on drm/drm-next] [cannot apply to drm-intel/for-linux-next v5.17-rc5 next-20220222] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Michael-Cheng/Use-drm_clflush-instead-of-clflush/20220223-140110 base: git://anongit.freedesktop.org/drm/drm-tip drm-tip config: s390-randconfig-r013-20220221 (https://download.01.org/0day-ci/archive/20220223/202202231817.dky1qgru-...@intel.com/config) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install s390 cross compiling tool for clang build # apt-get install binutils-s390x-linux-gnu # https://github.com/0day-ci/linux/commit/f4c92ba1f52db578a26ac9944e2cbe52c548e8e9 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Michael-Cheng/Use-drm_clflush-instead-of-clflush/20220223-140110 git checkout f4c92ba1f52db578a26ac9944e2cbe52c548e8e9 # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/gpu/drm/ If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All warnings (new ones prefixed by >>): In file included from drivers/gpu/drm/drm_cache.c:31: >> include/linux/cacheflush.h:12:46: warning: declaration of 'struct folio' >> will not be visible outside of this function [-Wvisibility] static inline void flush_dcache_folio(struct folio *folio) ^ In file included from drivers/gpu/drm/drm_cache.c:35: In file included from include/linux/iosys-map.h:9: In file included from include/linux/io.h:13: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __raw_readb(PCI_IOBASE + addr); ~~ ^ include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); ~~ ^ include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu' #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x)) ^ include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16' #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x)) ^ In file included from drivers/gpu/drm/drm_cache.c:35: In file included from include/linux/iosys-map.h:9: In file included from include/linux/io.h:13: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~ ^ include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32' #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x)) ^ In file included from drivers/gpu/drm/drm_cache.c:35: In file included from include/linux/iosys-map.h:9: In file included from include/linux/io.h:13: In file included from arch/s390/include/asm/io.h:75: include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writeb(value, PCI_IOBASE + addr); ~~ ^ include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
Re: [PATCH v10 3/4] drm/lsdc: add drm driver for loongson display controller
Hi Sui, Thank you for the patch! Yet something to improve: [auto build test ERROR on drm/drm-next] [also build test ERROR on robh/for-next v5.17-rc5 next-20220222] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Sui-Jingfeng/drm-lsdc-add-drm-driver-for-loongson-display-controller/20220220-225801 base: git://anongit.freedesktop.org/drm/drm drm-next config: riscv-randconfig-r012-20220221 (https://download.01.org/0day-ci/archive/20220223/202202231909.8x2nm3si-...@intel.com/config) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install riscv cross compiling tool for clang build # apt-get install binutils-riscv64-linux-gnu # https://github.com/0day-ci/linux/commit/3891cda03ed77e66fafaf7cfe075042e13f20173 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Sui-Jingfeng/drm-lsdc-add-drm-driver-for-loongson-display-controller/20220220-225801 git checkout 3891cda03ed77e66fafaf7cfe075042e13f20173 # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>): >> ld.lld: error: undefined symbol: ttm_bo_init >>> referenced by drm_gem_vram_helper.c >>> gpu/drm/drm_gem_vram_helper.o:(drm_gem_vram_create) in archive drivers/built-in.a -- >> ld.lld: error: undefined symbol: ttm_bo_put >>> referenced by drm_gem_vram_helper.c >>> gpu/drm/drm_gem_vram_helper.o:(drm_gem_vram_put) in archive drivers/built-in.a >>> referenced by drm_gem_vram_helper.c >>> gpu/drm/drm_gem_vram_helper.o:(drm_gem_vram_object_free) in archive drivers/built-in.a -- >> ld.lld: error: undefined symbol: ttm_tt_init >>> referenced by drm_gem_vram_helper.c >>> gpu/drm/drm_gem_vram_helper.o:(bo_driver_ttm_tt_create) in archive drivers/built-in.a -- >> ld.lld: error: undefined symbol: ttm_tt_fini >>> referenced by drm_gem_vram_helper.c >>> gpu/drm/drm_gem_vram_helper.o:(bo_driver_ttm_tt_destroy) in archive drivers/built-in.a -- >> ld.lld: error: undefined symbol: ttm_bo_move_memcpy >>> referenced by drm_gem_vram_helper.c >>> gpu/drm/drm_gem_vram_helper.o:(bo_driver_move) in archive drivers/built-in.a -- >> ld.lld: error: undefined symbol: ttm_bo_vunmap >>> referenced by drm_gem_vram_helper.c >>> gpu/drm/drm_gem_vram_helper.o:(drm_gem_vram_bo_driver_move_notify) in archive drivers/built-in.a -- >> ld.lld: error: undefined symbol: drm_gem_ttm_print_info >>> referenced by drm_gem_vram_helper.c >>> gpu/drm/drm_gem_vram_helper.o:(drm_gem_vram_object_funcs) in archive drivers/built-in.a -- >> ld.lld: error: undefined symbol: drm_gem_ttm_mmap >>> referenced by drm_gem_vram_helper.c >>> gpu/drm/drm_gem_vram_helper.o:(drm_gem_vram_object_funcs) in archive drivers/built-in.a -- >> ld.lld: error: undefined symbol: ttm_bo_eviction_valuable >>> referenced by drm_gem_vram_helper.c >>> gpu/drm/drm_gem_vram_helper.o:(bo_driver) in archive drivers/built-in.a -- >> ld.lld: error: undefined symbol: drm_gem_ttm_dumb_map_offset >>> referenced by lsdc_drv.c >>> gpu/drm/lsdc/lsdc_drv.o:(lsdc_vram_driver_stub) in archive drivers/built-in.a -- >> ld.lld: error: undefined symbol: ttm_bo_move_to_lru_tail >>> referenced by drm_gem_vram_helper.c >>> gpu/drm/drm_gem_vram_helper.o:(drm_gem_vram_pin) in archive drivers/built-in.a >>> referenced by drm_gem_vram_helper.c >>> gpu/drm/drm_gem_vram_helper.o:(drm_gem_vram_unpin) in archive drivers/built-in.a >>> referenced by drm_gem_vram_helper.c >>> gpu/drm/drm_gem_vram_helper.o:(drm_gem_vram_vmap) in archive drivers/built-in.a >>> referenced 1 more times .. --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
[PULL] drm-misc-next
Hi Dave, After few missing, here's the final pull req for -next in v5.18 drm-misc-next-2022-02-23: drm-misc-next for v5.18: UAPI Changes: Cross-subsystem Changes: - Split out panel-lvds and lvds dt bindings . - Put yes/no on/off disabled/enabled strings in linux/string_helpers.h and use it in drivers and tomoyo. - Clarify dma_fence_chain and dma_fence_array should never include eachother. - Flatten chains in syncobj's. - Don't double add in fbdev/defio when page is already enlisted. - Don't sort deferred-I/O pages by default in fbdev. Core Changes: - Fix missing pm_runtime_put_sync in bridge. - Set modifier support to only linear fb modifier if drivers don't advertise support. - As a result, we remove allow_fb_modifiers. - Add missing clear for EDID Deep Color Modes in drm_reset_display_info. - Assorted documentation updates. - Warn once in drm_clflush if there is no arch support. - Add missing select for dp helper in drm_panel_edp. - Assorted small fixes. - Improve fb-helper's clipping handling. - Don't dump shmem mmaps in a core dump. - Add accounting to ttm resource manager, and use it in amdgpu. - Allow querying the detected eDP panel through debugfs. - Add helpers for xrgb to 8 and 1 bits gray. - Improve drm's buddy allocator. - Add selftests for the buddy allocator. Driver Changes: - Add support for nomodeset to a lot of drm drivers. - Use drm_module_*_driver in a lot of drm drivers. - Assorted small fixes to bridge/lt9611, v3d, vc4, vmwgfx, mxsfb, nouveau, bridge/dw-hdmi, panfrost, lima, ingenic, sprd, bridge/anx7625, ti-sn65dsi86. - Add bridge/it6505. - Create DP and DVI-I connectors in ast. - Assorted nouveau backlight fixes. - Rework amdgpu reset handling. - Add dt bindings for ingenic,jz4780-dw-hdmi. - Support reading edid through aux channel in ingenic. - Add a drm driver for Solomon SSD130x OLED displays. - Add simple support for sharp LQ140M1JW46. - Add more panels to nt35560. The following changes since commit 53dbee4926d3706ca9e03f3928fa85b5ec3bc0cc: Merge tag 'drm-misc-next-2022-01-27' of git://anongit.freedesktop.org/drm/drm-misc into drm-next (2022-02-01 19:02:41 +1000) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-misc tags/drm-misc-next-2022-02-23 for you to fetch changes up to f915686bd97a9c234602426e6d132b74a112a8d6: drm/selftests: add drm buddy pathological testcase (2022-02-23 10:46:32 +0100) drm-misc-next for v5.18: UAPI Changes: Cross-subsystem Changes: - Split out panel-lvds and lvds dt bindings . - Put yes/no on/off disabled/enabled strings in linux/string_helpers.h and use it in drivers and tomoyo. - Clarify dma_fence_chain and dma_fence_array should never include eachother. - Flatten chains in syncobj's. - Don't double add in fbdev/defio when page is already enlisted. - Don't sort deferred-I/O pages by default in fbdev. Core Changes: - Fix missing pm_runtime_put_sync in bridge. - Set modifier support to only linear fb modifier if drivers don't advertise support. - As a result, we remove allow_fb_modifiers. - Add missing clear for EDID Deep Color Modes in drm_reset_display_info. - Assorted documentation updates. - Warn once in drm_clflush if there is no arch support. - Add missing select for dp helper in drm_panel_edp. - Assorted small fixes. - Improve fb-helper's clipping handling. - Don't dump shmem mmaps in a core dump. - Add accounting to ttm resource manager, and use it in amdgpu. - Allow querying the detected eDP panel through debugfs. - Add helpers for xrgb to 8 and 1 bits gray. - Improve drm's buddy allocator. - Add selftests for the buddy allocator. Driver Changes: - Add support for nomodeset to a lot of drm drivers. - Use drm_module_*_driver in a lot of drm drivers. - Assorted small fixes to bridge/lt9611, v3d, vc4, vmwgfx, mxsfb, nouveau, bridge/dw-hdmi, panfrost, lima, ingenic, sprd, bridge/anx7625, ti-sn65dsi86. - Add bridge/it6505. - Create DP and DVI-I connectors in ast. - Assorted nouveau backlight fixes. - Rework amdgpu reset handling. - Add dt bindings for ingenic,jz4780-dw-hdmi. - Support reading edid through aux channel in ingenic. - Add a drm driver for Solomon SSD130x OLED displays. - Add simple support for sharp LQ140M1JW46. - Add more panels to nt35560. Alex Bee (1): dt-bindings: gpu: mali-bifrost: describe clocks for the rk356x gpu Alexander Stein (1): drm: mxsfb: Use dev_err_probe() helper Allen Chen (1): drm/bridge: add it6505 driver Alyssa Rosenzweig (1): drm/panfrost: Handle IDVS_GROUP_SIZE feature Andrey Grodzovsky (13): drm/amdgpu: Introduce reset domain drm/amdgpu: Move scheduler init to after XGMI is ready drm/amdgpu: Serialize non TDR gpu recovery with TDRs drm/amd/virt: For SRIOV send GPU reset directly to TDR queue. drm/amdgpu: Drop hive->in_reset drm/amdgpu: Drop concurrent
Re: (subset) [PATCH] drm/edid: Always set RGB444
On Thu, 3 Feb 2022 12:54:16 +0100, Maxime Ripard wrote: > In order to fill the drm_display_info structure each time an EDID is > read, the code currently will call drm_add_display_info with the parsed > EDID. > > drm_add_display_info will then call drm_reset_display_info to reset all > the fields to 0, and then set them to the proper value depending on the > EDID. > > [...] Applied to drm/drm-misc (drm-misc-fixes). Thanks! Maxime
Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format
Hi, On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote: > Use the new property bus-format to set the enum bus_format and bpc. > Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support") > > Signed-off-by: Max Krummenacher > > --- > > drivers/gpu/drm/panel/panel-simple.c | 32 > 1 file changed, 32 insertions(+) > > Relates to the discussion: > https://lore.kernel.org/all/20220201110717.3585-1-cniederma...@dh-electronics.com/ > > Max > > diff --git a/drivers/gpu/drm/panel/panel-simple.c > b/drivers/gpu/drm/panel/panel-simple.c > index c5f133667a2d..5c07260de71c 100644 > --- a/drivers/gpu/drm/panel/panel-simple.c > +++ b/drivers/gpu/drm/panel/panel-simple.c > @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev, > struct panel_desc *desc; > unsigned int bus_flags; > struct videomode vm; > + const char *format = ""; > int ret; > > np = dev->of_node; > @@ -477,6 +478,37 @@ static int panel_dpi_probe(struct device *dev, > of_property_read_u32(np, "width-mm", &desc->size.width); > of_property_read_u32(np, "height-mm", &desc->size.height); > > + of_property_read_string(np, "bus-format", &format); > + if (!strcmp(format, "BGR888_1X24")) { > + desc->bpc = 8; > + desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24; > + } else if (!strcmp(format, "GBR888_1X24")) { > + desc->bpc = 8; > + desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24; > + } else if (!strcmp(format, "RBG888_1X24")) { > + desc->bpc = 8; > + desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24; > + } else if (!strcmp(format, "RGB444_1X12")) { > + desc->bpc = 6; > + desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12; > + } else if (!strcmp(format, "RGB565_1X16")) { > + desc->bpc = 6; > + desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16; > + } else if (!strcmp(format, "RGB666_1X18")) { > + desc->bpc = 6; > + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18; > + } else if (!strcmp(format, "RGB666_1X24_CPADHI")) { > + desc->bpc = 6; > + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI; > + } else if (!strcmp(format, "RGB888_1X24")) { > + desc->bpc = 8; > + desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24; > + } else { > + dev_err(dev, "%pOF: missing or unknown bus-format property\n", > + np); > + return -EINVAL; > + } > + It doesn't seem right, really. We can't the bus format / bpc be inferred from the compatible? I'd expect two panels that don't have the same bus format to not be claimed as compatible. Maxime signature.asc Description: PGP signature
Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format
On 2/23/22 14:41, Maxime Ripard wrote: Hi, On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote: Use the new property bus-format to set the enum bus_format and bpc. Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support") Signed-off-by: Max Krummenacher --- drivers/gpu/drm/panel/panel-simple.c | 32 1 file changed, 32 insertions(+) Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniederma...@dh-electronics.com/ Max diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index c5f133667a2d..5c07260de71c 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev, struct panel_desc *desc; unsigned int bus_flags; struct videomode vm; + const char *format = ""; int ret; np = dev->of_node; @@ -477,6 +478,37 @@ static int panel_dpi_probe(struct device *dev, of_property_read_u32(np, "width-mm", &desc->size.width); of_property_read_u32(np, "height-mm", &desc->size.height); + of_property_read_string(np, "bus-format", &format); + if (!strcmp(format, "BGR888_1X24")) { + desc->bpc = 8; + desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24; + } else if (!strcmp(format, "GBR888_1X24")) { + desc->bpc = 8; + desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24; + } else if (!strcmp(format, "RBG888_1X24")) { + desc->bpc = 8; + desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24; + } else if (!strcmp(format, "RGB444_1X12")) { + desc->bpc = 6; + desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12; + } else if (!strcmp(format, "RGB565_1X16")) { + desc->bpc = 6; + desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16; + } else if (!strcmp(format, "RGB666_1X18")) { + desc->bpc = 6; + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18; + } else if (!strcmp(format, "RGB666_1X24_CPADHI")) { + desc->bpc = 6; + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI; + } else if (!strcmp(format, "RGB888_1X24")) { + desc->bpc = 8; + desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24; + } else { + dev_err(dev, "%pOF: missing or unknown bus-format property\n", + np); + return -EINVAL; + } + It doesn't seem right, really. We can't the bus format / bpc be inferred from the compatible? I'd expect two panels that don't have the same bus format to not be claimed as compatible. Which compatible ? Note that this is for panel-dpi compatible, i.e. the panel which has timings specified in DT (and needs bus format specified there too). I agree this doesn't look right however, some more generic color channel width/shift/mapping might be better.
Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format
On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote: > On 2/23/22 14:41, Maxime Ripard wrote: > > Hi, > > > > On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote: > > > Use the new property bus-format to set the enum bus_format and bpc. > > > Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi > > > support") > > > > > > Signed-off-by: Max Krummenacher > > > > > > --- > > > > > > drivers/gpu/drm/panel/panel-simple.c | 32 > > > 1 file changed, 32 insertions(+) > > > > > > Relates to the discussion: > > > https://lore.kernel.org/all/20220201110717.3585-1-cniederma...@dh-electronics.com/ > > > > > > Max > > > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c > > > b/drivers/gpu/drm/panel/panel-simple.c > > > index c5f133667a2d..5c07260de71c 100644 > > > --- a/drivers/gpu/drm/panel/panel-simple.c > > > +++ b/drivers/gpu/drm/panel/panel-simple.c > > > @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev, > > > struct panel_desc *desc; > > > unsigned int bus_flags; > > > struct videomode vm; > > > + const char *format = ""; > > > int ret; > > > np = dev->of_node; > > > @@ -477,6 +478,37 @@ static int panel_dpi_probe(struct device *dev, > > > of_property_read_u32(np, "width-mm", &desc->size.width); > > > of_property_read_u32(np, "height-mm", &desc->size.height); > > > + of_property_read_string(np, "bus-format", &format); > > > + if (!strcmp(format, "BGR888_1X24")) { > > > + desc->bpc = 8; > > > + desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24; > > > + } else if (!strcmp(format, "GBR888_1X24")) { > > > + desc->bpc = 8; > > > + desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24; > > > + } else if (!strcmp(format, "RBG888_1X24")) { > > > + desc->bpc = 8; > > > + desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24; > > > + } else if (!strcmp(format, "RGB444_1X12")) { > > > + desc->bpc = 6; > > > + desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12; > > > + } else if (!strcmp(format, "RGB565_1X16")) { > > > + desc->bpc = 6; > > > + desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16; > > > + } else if (!strcmp(format, "RGB666_1X18")) { > > > + desc->bpc = 6; > > > + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18; > > > + } else if (!strcmp(format, "RGB666_1X24_CPADHI")) { > > > + desc->bpc = 6; > > > + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI; > > > + } else if (!strcmp(format, "RGB888_1X24")) { > > > + desc->bpc = 8; > > > + desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > > + } else { > > > + dev_err(dev, "%pOF: missing or unknown bus-format property\n", > > > + np); > > > + return -EINVAL; > > > + } > > > + > > > > It doesn't seem right, really. We can't the bus format / bpc be inferred > > from the compatible? I'd expect two panels that don't have the same bus > > format to not be claimed as compatible. > > Which compatible ? > > Note that this is for panel-dpi compatible, i.e. the panel which has timings > specified in DT (and needs bus format specified there too). panel-dpi is supposed to have two compatibles, the panel-specific one and panel-dpi. This would obviously be tied to the panel-specific one. Maxime signature.asc Description: PGP signature
[PATCH 0/3] drm/helpers: Make the suballocation manager drm generic.
Second version of the patch. I didn't fix the copyright (which ame up in the previous version), as I feel the original author should send a patch for that. I've made the suballocator into its own module, and did a cleanup pass on it. The suballocator is generic enough to be useful for any resource that can be subdivided and is guarded by a completion fence. Maarten Lankhorst (3): drm: Extract amdgpu_sa.c as a generic suballocation helper drm/amd: Convert amdgpu to use suballocation helper. drm/radeon: Use the drm suballocation manager implementation. drivers/gpu/drm/Kconfig| 6 + drivers/gpu/drm/Makefile | 3 + drivers/gpu/drm/amd/amdgpu/amdgpu.h| 29 +- drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 5 +- drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 21 +- drivers/gpu/drm/amd/amdgpu/amdgpu_sa.c | 320 +--- drivers/gpu/drm/drm_suballoc.c | 426 + drivers/gpu/drm/radeon/radeon.h| 55 +-- drivers/gpu/drm/radeon/radeon_ib.c | 10 +- drivers/gpu/drm/radeon/radeon_object.h | 23 +- drivers/gpu/drm/radeon/radeon_sa.c | 314 ++- drivers/gpu/drm/radeon/radeon_semaphore.c | 6 +- include/drm/drm_suballoc.h | 78 13 files changed, 603 insertions(+), 693 deletions(-) create mode 100644 drivers/gpu/drm/drm_suballoc.c create mode 100644 include/drm/drm_suballoc.h -- 2.34.1
[PATCH 1/3] drm: Extract amdgpu_sa.c as a generic suballocation helper
Suballocating a buffer object is something that is not driver generic, and is useful for other drivers as well. Signed-off-by: Maarten Lankhorst --- drivers/gpu/drm/Kconfig| 4 + drivers/gpu/drm/Makefile | 3 + drivers/gpu/drm/drm_suballoc.c | 426 + include/drm/drm_suballoc.h | 78 ++ 4 files changed, 511 insertions(+) create mode 100644 drivers/gpu/drm/drm_suballoc.c create mode 100644 include/drm/drm_suballoc.h diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index f1422bee3dcc..d60afd4b7476 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -237,6 +237,10 @@ config DRM_GEM_SHMEM_HELPER help Choose this if you need the GEM shmem helper functions +config DRM_SUBALLOC_HELPER + tristate + depends on DRM + config DRM_SCHED tristate depends on DRM diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index c2ef5f9fce54..3392cbce0d4f 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -42,6 +42,9 @@ obj-$(CONFIG_DRM_GEM_SHMEM_HELPER) += drm_shmem_helper.o obj-$(CONFIG_DRM_BUDDY) += drm_buddy.o +drm_suballoc_helper-y := drm_suballoc.o +obj-$(CONFIG_DRM_SUBALLOC_HELPER) += drm_suballoc_helper.o + drm_vram_helper-y := drm_gem_vram_helper.o obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o diff --git a/drivers/gpu/drm/drm_suballoc.c b/drivers/gpu/drm/drm_suballoc.c new file mode 100644 index ..4e8350c52a34 --- /dev/null +++ b/drivers/gpu/drm/drm_suballoc.c @@ -0,0 +1,426 @@ +/* + * Copyright 2011 Red Hat Inc. + * All Rights Reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the + * "Software"), to deal in the Software without restriction, including + * without limitation the rights to use, copy, modify, merge, publish, + * distribute, sub license, and/or sell copies of the Software, and to + * permit persons to whom the Software is furnished to do so, subject to + * the following conditions: + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM, + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE + * USE OR OTHER DEALINGS IN THE SOFTWARE. + * + * The above copyright notice and this permission notice (including the + * next paragraph) shall be included in all copies or substantial portions + * of the Software. + * + */ +/* + * Authors: + *Jerome Glisse + */ +/* Algorithm: + * + * We store the last allocated bo in "hole", we always try to allocate + * after the last allocated bo. Principle is that in a linear GPU ring + * progression was is after last is the oldest bo we allocated and thus + * the first one that should no longer be in use by the GPU. + * + * If it's not the case we skip over the bo after last to the closest + * done bo if such one exist. If none exist and we are not asked to + * block we report failure to allocate. + * + * If we are asked to block we wait on all the oldest fence of all + * rings. We just wait for any of those fence to complete. + */ + +#include +#include +#include +#include +#include +#include + +static void drm_suballoc_remove_locked(struct drm_suballoc *sa); +static void drm_suballoc_try_free(struct drm_suballoc_manager *sa_manager); + +/** + * drm_suballoc_manager_init - Initialise the drm_suballoc_manager + * + * @sa_manager: pointer to the sa_manager + * @size: number of bytes we want to suballocate + * @align: alignment for each suballocated chunk + * + * Prepares the suballocation manager for suballocations. + */ +void drm_suballoc_manager_init(struct drm_suballoc_manager *sa_manager, + u32 size, u32 align) +{ + u32 i; + + if (!align) + align = 1; + + /* alignment must be a power of 2 */ + BUG_ON(align & (align - 1)); + + init_waitqueue_head(&sa_manager->wq); + sa_manager->size = size; + sa_manager->align = align; + sa_manager->hole = &sa_manager->olist; + INIT_LIST_HEAD(&sa_manager->olist); + for (i = 0; i < DRM_SUBALLOC_MAX_QUEUES; ++i) + INIT_LIST_HEAD(&sa_manager->flist[i]); +} +EXPORT_SYMBOL(drm_suballoc_manager_init); + +/** + * drm_suballoc_manager_fini - Destroy the drm_suballoc_manager + * + * @sa_manager: pointer to the sa_manager + * + * Cleans up the suballocation manager after use. All fences added + * with drm_suballoc_free() must be signaled, or we cannot clean up + * the entire manager. + */ +void drm_suballoc_manager_fini(struct drm_suballoc_manager *sa_manager) +{ + struct d
[PATCH 2/3] drm/amd: Convert amdgpu to use suballocation helper.
Now that the suballocation helper is generic, we can use it in amdgpu. Signed-off-by: Maarten Lankhorst --- drivers/gpu/drm/Kconfig| 1 + drivers/gpu/drm/amd/amdgpu/amdgpu.h| 29 +- drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 5 +- drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 21 +- drivers/gpu/drm/amd/amdgpu/amdgpu_sa.c | 320 ++--- 5 files changed, 40 insertions(+), 336 deletions(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index d60afd4b7476..666cb4d251b9 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -278,6 +278,7 @@ config DRM_AMDGPU select DRM_DP_HELPER select DRM_KMS_HELPER select DRM_SCHED + select DRM_SUBALLOC_HELPER select DRM_TTM select DRM_TTM_HELPER select POWER_SUPPLY diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 8685784f24a9..6935d9ed9f8d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -61,6 +61,7 @@ #include #include #include +#include #include #include "dm_pp_interface.h" @@ -417,29 +418,11 @@ struct amdgpu_clock { * alignment). */ -#define AMDGPU_SA_NUM_FENCE_LISTS 32 - struct amdgpu_sa_manager { - wait_queue_head_t wq; - struct amdgpu_bo*bo; - struct list_head*hole; - struct list_headflist[AMDGPU_SA_NUM_FENCE_LISTS]; - struct list_headolist; - unsignedsize; - uint64_tgpu_addr; - void*cpu_ptr; - uint32_tdomain; - uint32_talign; -}; - -/* sub-allocation buffer */ -struct amdgpu_sa_bo { - struct list_headolist; - struct list_headflist; - struct amdgpu_sa_manager*manager; - unsignedsoffset; - unsignedeoffset; - struct dma_fence*fence; + struct drm_suballoc_manager base; + struct amdgpu_bo*bo; + uint64_tgpu_addr; + void*cpu_ptr; }; int amdgpu_fence_slab_init(void); @@ -470,7 +453,7 @@ struct amdgpu_flip_work { */ struct amdgpu_ib { - struct amdgpu_sa_bo *sa_bo; + struct drm_suballoc *sa_bo; uint32_tlength_dw; uint64_tgpu_addr; uint32_t*ptr; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c index bc1297dcdf97..883828a4988c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c @@ -69,7 +69,7 @@ int amdgpu_ib_get(struct amdgpu_device *adev, struct amdgpu_vm *vm, if (size) { r = amdgpu_sa_bo_new(&adev->ib_pools[pool_type], - &ib->sa_bo, size, 256); + &ib->sa_bo, size); if (r) { dev_err(adev->dev, "failed to get a new IB (%d)\n", r); return r; @@ -307,8 +307,7 @@ int amdgpu_ib_pool_init(struct amdgpu_device *adev) for (i = 0; i < AMDGPU_IB_POOL_MAX; i++) { r = amdgpu_sa_bo_manager_init(adev, &adev->ib_pools[i], - AMDGPU_IB_POOL_SIZE, - AMDGPU_GPU_PAGE_SIZE, + AMDGPU_IB_POOL_SIZE, 256, AMDGPU_GEM_DOMAIN_GTT); if (r) goto error; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h index 4c9cbdc66995..7db4fe1bc1d6 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h @@ -337,15 +337,20 @@ uint32_t amdgpu_bo_get_preferred_domain(struct amdgpu_device *adev, /* * sub allocation */ +static inline struct amdgpu_sa_manager * +to_amdgpu_sa_manager(struct drm_suballoc_manager *manager) +{ + return container_of(manager, struct amdgpu_sa_manager, base); +} -static inline uint64_t amdgpu_sa_bo_gpu_addr(struct amdgpu_sa_bo *sa_bo) +static inline uint64_t amdgpu_sa_bo_gpu_addr(struct drm_suballoc *sa_bo) { - return sa_bo->manager->gpu_addr + sa_bo->soffset; + return to_amdgpu_sa_manager(sa_bo->manager)->gpu_addr + sa_bo->soffset; } -static inline void * amdgpu_sa_bo_cpu_addr(struct amdgpu_sa_bo *sa_bo) +static inline void * amdgpu_sa_bo_cpu_addr(struct drm_suballoc *sa_bo) { - return sa_bo->manager->cpu_ptr + sa_bo->soffset; + return to_amdgpu_sa_manager(sa_bo->manager)->cpu_ptr + sa_bo->soffset; } int amdgpu_sa_bo_manager_init(struct amdgpu_device *adev, @@ -356,11
[PATCH 3/3] drm/radeon: Use the drm suballocation manager implementation.
Use the generic suballocation helper lifted from amdgpu. Note that the generic suballocator only allows a single alignment, so we may waste a few more bytes for radeon_semaphore, shouldn't be a big deal, could be re-added if needed. Signed-off-by: Maarten Lankhorst --- drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/radeon/radeon.h | 55 +--- drivers/gpu/drm/radeon/radeon_ib.c| 10 +- drivers/gpu/drm/radeon/radeon_object.h| 23 +- drivers/gpu/drm/radeon/radeon_sa.c| 314 ++ drivers/gpu/drm/radeon/radeon_semaphore.c | 6 +- 6 files changed, 52 insertions(+), 357 deletions(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 666cb4d251b9..16880a41f3d9 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -256,6 +256,7 @@ config DRM_RADEON select FW_LOADER select DRM_DP_HELPER select DRM_KMS_HELPER +select DRM_SUBALLOC_HELPER select DRM_TTM select DRM_TTM_HELPER select POWER_SUPPLY diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 08f83bf2c330..7db39ff11cd1 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -79,6 +79,7 @@ #include #include +#include #include "radeon_family.h" #include "radeon_mode.h" @@ -512,52 +513,12 @@ struct radeon_bo { }; #define gem_to_radeon_bo(gobj) container_of((gobj), struct radeon_bo, tbo.base) -/* sub-allocation manager, it has to be protected by another lock. - * By conception this is an helper for other part of the driver - * like the indirect buffer or semaphore, which both have their - * locking. - * - * Principe is simple, we keep a list of sub allocation in offset - * order (first entry has offset == 0, last entry has the highest - * offset). - * - * When allocating new object we first check if there is room at - * the end total_size - (last_object_offset + last_object_size) >= - * alloc_size. If so we allocate new object there. - * - * When there is not enough room at the end, we start waiting for - * each sub object until we reach object_offset+object_size >= - * alloc_size, this object then become the sub object we return. - * - * Alignment can't be bigger than page size. - * - * Hole are not considered for allocation to keep things simple. - * Assumption is that there won't be hole (all object on same - * alignment). - */ struct radeon_sa_manager { - wait_queue_head_t wq; - struct radeon_bo*bo; - struct list_head*hole; - struct list_headflist[RADEON_NUM_RINGS]; - struct list_headolist; - unsignedsize; - uint64_tgpu_addr; - void*cpu_ptr; - uint32_tdomain; - uint32_talign; -}; - -struct radeon_sa_bo; - -/* sub-allocation buffer */ -struct radeon_sa_bo { - struct list_headolist; - struct list_headflist; - struct radeon_sa_manager*manager; - unsignedsoffset; - unsignedeoffset; - struct radeon_fence *fence; + struct drm_suballoc_manager base; + struct radeon_bo*bo; + uint64_tgpu_addr; + void*cpu_ptr; + u32 domain; }; /* @@ -588,7 +549,7 @@ int radeon_mode_dumb_mmap(struct drm_file *filp, * Semaphores. */ struct radeon_semaphore { - struct radeon_sa_bo *sa_bo; + struct drm_suballoc *sa_bo; signed waiters; uint64_tgpu_addr; }; @@ -817,7 +778,7 @@ void radeon_irq_kms_disable_hpd(struct radeon_device *rdev, unsigned hpd_mask); */ struct radeon_ib { - struct radeon_sa_bo *sa_bo; + struct drm_suballoc *sa_bo; uint32_tlength_dw; uint64_tgpu_addr; uint32_t*ptr; diff --git a/drivers/gpu/drm/radeon/radeon_ib.c b/drivers/gpu/drm/radeon/radeon_ib.c index 62b116727b4f..bca2cbd27abf 100644 --- a/drivers/gpu/drm/radeon/radeon_ib.c +++ b/drivers/gpu/drm/radeon/radeon_ib.c @@ -61,7 +61,7 @@ int radeon_ib_get(struct radeon_device *rdev, int ring, { int r; - r = radeon_sa_bo_new(rdev, &rdev->ring_tmp_bo, &ib->sa_bo, size, 256); + r = radeon_sa_bo_new(&rdev->ring_tmp_bo, &ib->sa_bo, size); if (r) { dev_err(rdev->dev, "failed to get a new IB (%d)\n", r); return r; @@ -97,7 +97,7 @@ int radeon_ib_get(struct radeon_device *rdev, int ring, void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib) { radeon_sync_free(rdev, &ib->sync, ib->fence); - radeon_sa_bo_free(rdev, &ib->sa_bo, ib->fence); + radeon_sa_bo_free(&ib->sa_bo, ib
Re: [Intel-gfx] [PATCH 0/7] drm/i915: Use the memcpy_from_wc function from drm
On 23/02/2022 12:08, Balasubramani Vivekanandan wrote: On 23.02.2022 10:02, Das, Nirmoy wrote: On 22/02/2022 15:51, Balasubramani Vivekanandan wrote: drm_memcpy_from_wc() performs fast copy from WC memory type using non-temporal instructions. Now there are two similar implementations of this function. One exists in drm_cache.c as drm_memcpy_from_wc() and another implementation in i915/i915_memcpy.c as i915_memcpy_from_wc(). drm_memcpy_from_wc() was the recent addition through the series https://patchwork.freedesktop.org/patch/436276/?series=90681&rev=6 The goal of this patch series is to change all users of i915_memcpy_from_wc() to drm_memcpy_from_wc() and a have common implementation in drm and eventually remove the copy from i915. Another benefit of using memcpy functions from drm is that drm_memcpy_from_wc() is available for non-x86 architectures. i915_memcpy_from_wc() is implemented only for x86 and prevents building i915 for ARM64. drm_memcpy_from_wc() does fast copy using non-temporal instructions for x86 and for other architectures makes use of memcpy() family of functions as fallback. Another major difference is unlike i915_memcpy_from_wc(), drm_memcpy_from_wc() will not fail if the passed address argument is not alignment to be used with non-temporal load instructions or if the platform lacks support for those instructions (non-temporal load instructions are provided through SSE4.1 instruction set extension). Instead drm_memcpy_from_wc() continues with fallback functions to complete the copy. This relieves the caller from checking the return value of i915_memcpy_from_wc() and explicitly using a fallback. Follow up series will be created to remove the memcpy_from_wc functions from i915 once the dependency is completely removed. Overall the series looks good to me but I think you can add another patch to remove i915_memcpy_from_wc() as I don't see any other usages left after this series, may be I am missing something? I have changed all users of i915_memcpy_from_wc() to drm function. But this is another function i915_unaligned_memcpy_from_wc() in i915_memcpy.c which is blocking completely eliminating the i915_memcpy.c file from i915. This function accepts unaligned source address and does fast copy only for the aligned region of memory and remaining part is copied using memcpy function. Either I can move i915_unaligned_memcpy_from_wc() also to drm but I am concerned since it is more a platform specific handling, does it make sense to keep it in drm. Else I have retain to i915_unaligned_memcpy_from_wc() inside i915 and refactor the function to use drm_memcpy_from_wc() instead of the __memcpy_ntdqu(). I think for completeness it makes sense to remove i915_memcpy_from_wc() and its helper functions in this series. I don't think we can have i915_unaligned_memcpy_from_wc() if want i915 on ARM[0] so I think you can remove usages of i915_unaligned_memcpy_from_wc() as well. [0]IIUC CI_BUG_ON() check in i915_unaligned_memcpy_from_wc() will raise a build error on ARM Regards, Nirmoy But before I could do more changes, I wanted feedback on the current change. So I decided to go ahead with creating series for review. Regards, Bala Regards, Nirmoy Cc: Jani Nikula Cc: Lucas De Marchi Cc: David Airlie Cc: Daniel Vetter Cc: Chris Wilson Cc: Thomas Hellstr_m Cc: Joonas Lahtinen Cc: Rodrigo Vivi Cc: Tvrtko Ursulin Balasubramani Vivekanandan (7): drm: Relax alignment constraint for destination address drm: Add drm_memcpy_from_wc() variant which accepts destination address drm/i915: use the memcpy_from_wc call from the drm drm/i915/guc: use the memcpy_from_wc call from the drm drm/i915/selftests: use the memcpy_from_wc call from the drm drm/i915/gt: Avoid direct dereferencing of io memory drm/i915: Avoid dereferencing io mapped memory drivers/gpu/drm/drm_cache.c | 98 +-- drivers/gpu/drm/i915/gem/i915_gem_object.c| 8 +- drivers/gpu/drm/i915/gt/selftest_reset.c | 21 ++-- drivers/gpu/drm/i915/gt/uc/intel_guc_log.c| 11 ++- drivers/gpu/drm/i915/i915_gpu_error.c | 45 + .../drm/i915/selftests/intel_memory_region.c | 8 +- include/drm/drm_cache.h | 3 + 7 files changed, 148 insertions(+), 46 deletions(-)
Re: [Intel-gfx] [PATCH 0/7] drm/i915: Use the memcpy_from_wc function from drm
On 22/02/2022 15:51, Balasubramani Vivekanandan wrote: drm_memcpy_from_wc() performs fast copy from WC memory type using non-temporal instructions. Now there are two similar implementations of this function. One exists in drm_cache.c as drm_memcpy_from_wc() and another implementation in i915/i915_memcpy.c as i915_memcpy_from_wc(). drm_memcpy_from_wc() was the recent addition through the series https://patchwork.freedesktop.org/patch/436276/?series=90681&rev=6 The goal of this patch series is to change all users of i915_memcpy_from_wc() to drm_memcpy_from_wc() and a have common implementation in drm and eventually remove the copy from i915. Another benefit of using memcpy functions from drm is that drm_memcpy_from_wc() is available for non-x86 architectures. i915_memcpy_from_wc() is implemented only for x86 and prevents building i915 for ARM64. drm_memcpy_from_wc() does fast copy using non-temporal instructions for x86 and for other architectures makes use of memcpy() family of functions as fallback. Another major difference is unlike i915_memcpy_from_wc(), drm_memcpy_from_wc() will not fail if the passed address argument is not alignment to be used with non-temporal load instructions or if the platform lacks support for those instructions (non-temporal load instructions are provided through SSE4.1 instruction set extension). Instead drm_memcpy_from_wc() continues with fallback functions to complete the copy. This relieves the caller from checking the return value of i915_memcpy_from_wc() and explicitly using a fallback. Follow up series will be created to remove the memcpy_from_wc functions from i915 once the dependency is completely removed. Overall the series looks good to me but I think you can add another patch to remove i915_memcpy_from_wc() as I don't see any other usages left after this series, may be I am missing something? Regards, Nirmoy Cc: Jani Nikula Cc: Lucas De Marchi Cc: David Airlie Cc: Daniel Vetter Cc: Chris Wilson Cc: Thomas Hellstr_m Cc: Joonas Lahtinen Cc: Rodrigo Vivi Cc: Tvrtko Ursulin Balasubramani Vivekanandan (7): drm: Relax alignment constraint for destination address drm: Add drm_memcpy_from_wc() variant which accepts destination address drm/i915: use the memcpy_from_wc call from the drm drm/i915/guc: use the memcpy_from_wc call from the drm drm/i915/selftests: use the memcpy_from_wc call from the drm drm/i915/gt: Avoid direct dereferencing of io memory drm/i915: Avoid dereferencing io mapped memory drivers/gpu/drm/drm_cache.c | 98 +-- drivers/gpu/drm/i915/gem/i915_gem_object.c| 8 +- drivers/gpu/drm/i915/gt/selftest_reset.c | 21 ++-- drivers/gpu/drm/i915/gt/uc/intel_guc_log.c| 11 ++- drivers/gpu/drm/i915/i915_gpu_error.c | 45 + .../drm/i915/selftests/intel_memory_region.c | 8 +- include/drm/drm_cache.h | 3 + 7 files changed, 148 insertions(+), 46 deletions(-)
[PATCH] drm/vmwgfx: make vmw_pt_sys_placement static
Converts the variable vmw_pt_sys_placement to static to fix the following Sparse warning: warning: symbol 'vmw_pt_sys_placement' was not declared. Should it be static? Signed-off-by: Wambui Karuga --- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c index b84ecc6d6611..21057ff0d340 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c @@ -120,7 +120,7 @@ struct ttm_placement vmw_sys_placement = { .busy_placement = &sys_placement_flags }; -struct ttm_placement vmw_pt_sys_placement = { +static struct ttm_placement vmw_pt_sys_placement = { .num_placement = 1, .placement = &vmw_sys_placement_flags, .num_busy_placement = 1, -- 2.32.0
[PATCH] dt-bindings: display/ti: remove ti,hwmods property
Remove ti,hwmods from required properties, since the target-module approach is actually used. Signed-off-by: Angelo Dureghello --- Documentation/devicetree/bindings/display/ti/ti,dra7-dss.txt | 2 -- 1 file changed, 2 deletions(-) diff --git a/Documentation/devicetree/bindings/display/ti/ti,dra7-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,dra7-dss.txt index 91279f1060fe..948cb0f9f0e3 100644 --- a/Documentation/devicetree/bindings/display/ti/ti,dra7-dss.txt +++ b/Documentation/devicetree/bindings/display/ti/ti,dra7-dss.txt @@ -10,7 +10,6 @@ DSS Core Required properties: - compatible: "ti,dra7-dss" - reg: address and length of the register spaces for 'dss' -- ti,hwmods: "dss_core" - clocks: handle to fclk - clock-names: "fck" - syscon: phandle to control module core syscon node @@ -42,7 +41,6 @@ DISPC Required properties: - compatible: "ti,dra7-dispc" - reg: address and length of the register space -- ti,hwmods: "dss_dispc" - interrupts: the DISPC interrupt - clocks: handle to fclk - clock-names: "fck" -- 2.34.1
Re: [Intel-gfx] [PATCH 2/3] drm/i915/gt: Make the heartbeat play nice with long pre-emption timeouts
On 23/02/2022 02:45, John Harrison wrote: On 2/22/2022 03:19, Tvrtko Ursulin wrote: On 18/02/2022 21:33, john.c.harri...@intel.com wrote: From: John Harrison Compute workloads are inherantly not pre-emptible for long periods on current hardware. As a workaround for this, the pre-emption timeout for compute capable engines was disabled. This is undesirable with GuC submission as it prevents per engine reset of hung contexts. Hence the next patch will re-enable the timeout but bumped up by an order of magnititude. (Some typos above.) I'm spotting 'inherently' but not anything else. Magnititude! O;) However, the heartbeat might not respect that. Depending upon current activity, a pre-emption to the heartbeat pulse might not even be attempted until the last heartbeat period. Which means that only one Might not be attempted, but could be if something is running with lower priority. In which case I think special casing the last heartbeat does not feel right because it can end up resetting the engine before it was intended. Like if first heartbeat decides to preempt (the decision is backend specific, could be same prio + timeslicing), and preempt timeout has been set to heartbeat interval * 3, then 2nd heartbeat gets queued up, then 3rd, and so reset is triggered even before the first preempt timeout legitimately expires (or just as it is about to react). Instead, how about preempt timeout is always considered when calculating when to emit the next heartbeat? End result would be similar to your patch, in terms of avoiding the direct problem, although hang detection would be overall longer (but more correct I think). And it also means in the next patch you don't have to add coupling between preempt timeout and heartbeat to intel_engine_setup. Instead just some long preempt timeout would be needed. Granted, the decoupling argument is not super strong since then the heartbeat code has the coupling instead, but that still feels better to me. (Since we can say heartbeats only make sense on loaded engines, and so things like preempt timeout can legitimately be considered from there.) Incidentally, that would be similar to a patch which Chris had a year ago (https://patchwork.freedesktop.org/patch/419783/?series=86841&rev=1) to fix some CI issue. I'm not following your arguments. Chris' patch is about not having two i915 based resets triggered concurrently - i915 based engine reset and i915 based GT reset. The purpose of this patch is to allow the GuC based engine reset to have a chance to occur before the i915 based GT reset kicks in. It sounds like your argument above is about making the engine reset slower so that it doesn't happen before the appropriate heartbeat period for that potential reset scenario has expired. I don't see why that is at all necessary or useful. If an early heartbeat period triggers an engine reset then the heartbeat pulse will go through. The heartbeat will thus see a happy system and not do anything further. If the given period does not trigger an engine reset but still does not get the pulse through (because the pulse is of too low a priority) then we move on to the next period and bump the priority. If the pre-emption has actually already been triggered anyway (and we are just waiting a while for it to timeout) then that's fine. The priority bump will have no effect because the context is already attempting to run. The heartbeat code doesn't care which priority level actually triggers the reset. It just cares whether or not the pulse finally makes it through. And the GuC doesn't care which heartbeat period the i915 is in. All it knows is that it has a request to schedule and whether the current context is pre-empting or not. So if period #1 triggers the pre-emption but the timeout doesn't happen until period #3, who cares? The result is the same as if period #3 triggered the pre-emption and the timeout was shorter. The result being that the hung context is reset, the pulse makes it through and the heartbeat goes to sleep again. The only period that really matters is the final one. At that point the pulse request is at highest priority and so must trigger a pre-emption request. We then need at least one full pre-emption period (plus some wiggle room for random delays in reset time, context switching, processing messages, etc.) to allow the GuC based timeout and reset to occur. Hence ensuring that the final heartbeat period is at least twice the pre-emption timeout (because 1.25 times is just messy when working with ints!). That guarantees that GuC will get at least one complete opportunity to detect and recover the hang before i915 nukes the universe. Whereas, bumping all heartbeat periods to be greater than the pre-emption timeout is wasteful and unnecessary. That leads to a total heartbeat time of about a minute. Which is a very long time to wait for a hang to be detected and recovered. Especially
Re: [PATCH] drm/amd/display: move FPU-related code from dcn20 to dml folder
On 2022-02-21 06:31, Melissa Wen wrote: Move parts of dcn20 code that uses FPU to dml folder. It aims to isolate FPU operations as described by series: drm/amd/display: Introduce FPU directory inside DC https://patchwork.freedesktop.org/series/93042/ This patch moves the following functions from dcn20_resource to dml/dcn20_fpu and calls of public functions in dcn20_resource are wrapped by DC_FP_START/END(): - void dcn20_populate_dml_writeback_from_context - static bool is_dtbclk_required() - static enum dcn_zstate_support_state() - void dcn20_calculate_dlg_params() - static void swizzle_to_dml_params() - int dcn20_populate_dml_pipes_from_context() - void dcn20_calculate_wm() - void dcn20_cap_soc_clocks() - void dcn20_update_bounding_box() - void dcn20_patch_bounding_box() - bool dcn20_validate_bandwidth_fp() This movement also affects dcn30/31, as dcn20_calculate_dlg_params() is used by dcn30 and dcn31. For this reason, I included dcn20_fpu headers in dcn20_resource headers to make dcn20_calculate_dlg_params() visible to dcn30/31. Three new functions are created to isolate well-delimited FPU operations: - void dcn20_fpu_set_wb_arb_params(): set cli_watermark, pstate_watermark and time_per_pixel from wb_arb_params (struct mcif_arb_params), since those uses FPU operations on double types: WritebackUrgentWatermark, WritebackDRAMClockChangeWatermark, '16.0'. - void dcn20_fpu_set_wm_ranges(): set min_fill_clk_mhz and max_fill_clk_mhz involves FPU calcs on dram_speed_mts (double type); - void dcn20_fpu_adjust_dppclk(): adjust operation on RequiredDPPCLK that is a double. Signed-off-by: Melissa Wen --- drivers/gpu/drm/amd/display/dc/dcn20/Makefile | 25 - .../drm/amd/display/dc/dcn20/dcn20_resource.c | 1370 +--- .../drm/amd/display/dc/dcn20/dcn20_resource.h | 30 +- .../drm/amd/display/dc/dml/dcn20/dcn20_fpu.c | 1385 + .../drm/amd/display/dc/dml/dcn20/dcn20_fpu.h | 42 +- 5 files changed, 1451 insertions(+), 1401 deletions(-) diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile index 5fcaf78334ff..abaed2121feb 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile +++ b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile @@ -9,31 +9,6 @@ DCN20 = dcn20_resource.o dcn20_init.o dcn20_hwseq.o dcn20_dpp.o dcn20_dpp_cm.o d DCN20 += dcn20_dsc.o -ifdef CONFIG_X86 -CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o := -mhard-float -msse -endif - -ifdef CONFIG_PPC64 -CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o := -mhard-float -maltivec -endif - -ifdef CONFIG_CC_IS_GCC -ifeq ($(call cc-ifversion, -lt, 0701, y), y) -IS_OLD_GCC = 1 -endif -endif - -ifdef CONFIG_X86 -ifdef IS_OLD_GCC -# Stack alignment mismatch, proceed with caution. -# GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3 -# (8B stack alignment). -CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o += -mpreferred-stack-boundary=4 -else -CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o += -msse2 -endif -endif - AMD_DAL_DCN20 = $(addprefix $(AMDDALPATH)/dc/dcn20/,$(DCN20)) AMD_DISPLAY_FILES += $(AMD_DAL_DCN20) diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c index dfe2e1c25a26..63c50bee0144 100644 --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c @@ -63,7 +63,6 @@ #include "dcn20_dccg.h" #include "dcn20_vmid.h" #include "dc_link_ddc.h" -#include "dc_link_dp.h" #include "dce/dce_panel_cntl.h" #include "navi10_ip_offset.h" @@ -93,367 +92,6 @@ #define DC_LOGGER_INIT(logger) -struct _vcs_dpi_ip_params_st dcn2_0_ip = { - .odm_capable = 1, - .gpuvm_enable = 0, - .hostvm_enable = 0, - .gpuvm_max_page_table_levels = 4, - .hostvm_max_page_table_levels = 4, - .hostvm_cached_page_table_levels = 0, - .pte_group_size_bytes = 2048, - .num_dsc = 6, - .rob_buffer_size_kbytes = 168, - .det_buffer_size_kbytes = 164, - .dpte_buffer_size_in_pte_reqs_luma = 84, - .pde_proc_buffer_size_64k_reqs = 48, - .dpp_output_buffer_pixels = 2560, - .opp_output_buffer_lines = 1, - .pixel_chunk_size_kbytes = 8, - .pte_chunk_size_kbytes = 2, - .meta_chunk_size_kbytes = 2, - .writeback_chunk_size_kbytes = 2, - .line_buffer_size_bits = 789504, - .is_line_buffer_bpp_fixed = 0, - .line_buffer_fixed_bpp = 0, - .dcc_supported = true, - .max_line_buffer_lines = 12, - .writeback_luma_buffer_size_kbytes = 12, - .writeback_chroma_buffer_size_kbytes = 8, - .writeback_chroma_line_buffer_width_pixels = 4, - .writeback_max_hscl_ratio = 1, - .writeback_max_vscl_ratio = 1, - .writeback_min_hscl_ratio = 1, - .writeback_min_vscl_ratio = 1, - .writeback_max_hscl_taps = 12, - .writeback_max_vscl_taps = 12, -
Re: [PATCH v2 1/3] drm/mm: Ensure that the entry is not NULL before extracting rb_node
On 23/02/2022 04:35, Kasireddy, Vivek wrote: Hi Tvrtko, On 18/02/2022 03:47, Kasireddy, Vivek wrote: Hi Tvrtko, On 17/02/2022 07:50, Vivek Kasireddy wrote: While looking for next holes suitable for an allocation, although, it is highly unlikely, make sure that the DECLARE_NEXT_HOLE_ADDR macro is using a valid node before it extracts the rb_node from it. Was the need for this just a consequence of insufficient locking in the i915 patch? [Kasireddy, Vivek] Partly, yes; but I figured since we are anyway doing if (!entry || ..), it makes sense to dereference entry and extract the rb_node after this check. Unless I am blind I don't see that it makes a difference. "&entry->rb_hole_addr" is taking an address of, which works "fine" is [Kasireddy, Vivek] Ah, didn't realize it was the same thing as offsetof(). entry is NULL. And does not get past the !entry check for the actual de-reference via RB_EMPTY_NODE. With your patch you move that after the !entry check but still have it in the RB_EMPTY_NODE macro. Again, unless I am blind, I think just drop this patch. [Kasireddy, Vivek] Sure; do you want me to send another version with this patch dropped? Or, would you be able to just merge the other two from the latest version of this series? Please send without the first patch so we get clean set of CI results. You can use "--subject-prefix=CI" with git format-patchs and --suppress-cc=all with git send-email to avoid spamming people and let readers know the re-send is just for the purpose of getting CI results. Regards, Tvrtko
Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format
On 2/23/22 14:47, Maxime Ripard wrote: On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote: On 2/23/22 14:41, Maxime Ripard wrote: Hi, On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote: Use the new property bus-format to set the enum bus_format and bpc. Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support") Signed-off-by: Max Krummenacher --- drivers/gpu/drm/panel/panel-simple.c | 32 1 file changed, 32 insertions(+) Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniederma...@dh-electronics.com/ Max diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index c5f133667a2d..5c07260de71c 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev, struct panel_desc *desc; unsigned int bus_flags; struct videomode vm; + const char *format = ""; int ret; np = dev->of_node; @@ -477,6 +478,37 @@ static int panel_dpi_probe(struct device *dev, of_property_read_u32(np, "width-mm", &desc->size.width); of_property_read_u32(np, "height-mm", &desc->size.height); + of_property_read_string(np, "bus-format", &format); + if (!strcmp(format, "BGR888_1X24")) { + desc->bpc = 8; + desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24; + } else if (!strcmp(format, "GBR888_1X24")) { + desc->bpc = 8; + desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24; + } else if (!strcmp(format, "RBG888_1X24")) { + desc->bpc = 8; + desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24; + } else if (!strcmp(format, "RGB444_1X12")) { + desc->bpc = 6; + desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12; + } else if (!strcmp(format, "RGB565_1X16")) { + desc->bpc = 6; + desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16; + } else if (!strcmp(format, "RGB666_1X18")) { + desc->bpc = 6; + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18; + } else if (!strcmp(format, "RGB666_1X24_CPADHI")) { + desc->bpc = 6; + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI; + } else if (!strcmp(format, "RGB888_1X24")) { + desc->bpc = 8; + desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24; + } else { + dev_err(dev, "%pOF: missing or unknown bus-format property\n", + np); + return -EINVAL; + } + It doesn't seem right, really. We can't the bus format / bpc be inferred from the compatible? I'd expect two panels that don't have the same bus format to not be claimed as compatible. Which compatible ? Note that this is for panel-dpi compatible, i.e. the panel which has timings specified in DT (and needs bus format specified there too). panel-dpi is supposed to have two compatibles, the panel-specific one and panel-dpi. This would obviously be tied to the panel-specific one. This whole discussion is about the one which only has 'panel-dpi' compatible and describes the panel in DT completely. The specific compatible is not present in DT when this patch is needed.
Re: [PATCH] drm/sched: Add device pointer to drm_gpu_scheduler
On Wed, Feb 23, 2022 at 2:42 AM Christian König wrote: > > Well that's bad. This should not be pushed to amd-staging-drm-next at all. > > This patch is touching multiple drivers and therefore needs to go > upstream through drm-misc-next. > > Alex can you drop that one before you send out a pull request? I'm going > to cherry-pick it over to drm-misc-next. Yeah, no problem. Alex > > Thanks, > Christian. > > Am 23.02.22 um 08:15 schrieb Gu, JiaWei (Will): > > [AMD Official Use Only] > > > > Hi Christian, > > > > I noticed that and it has been fixed with the latest patch. > > And I pushed it to amd-staging-drm-next already. > > > > Best regards, > > Jiawei > > > > -Original Message- > > From: Koenig, Christian > > Sent: Wednesday, February 23, 2022 3:12 PM > > To: kernel test robot ; Gu, JiaWei (Will) > > ; dri-devel@lists.freedesktop.org; > > amd-...@lists.freedesktop.org; Grodzovsky, Andrey > > ; Liu, Monk ; Deng, Emily > > ; Chen, Horace > > Cc: kbuild-...@lists.01.org > > Subject: Re: [PATCH] drm/sched: Add device pointer to drm_gpu_scheduler > > > > Hi Jiawei, > > > > > > can you take a look at this? The kernel build robots screaming that this > > breaks the V3D build. Probably just a typo or missing include. > > > > I would rather like to push this sooner than later. > > > > Thanks, > > Christian. > > > > Am 21.02.22 um 16:51 schrieb kernel test robot: > >> Hi Jiawei, > >> > >> Thank you for the patch! Yet something to improve: > >> > >> [auto build test ERROR on drm/drm-next] [also build test ERROR on > >> drm-intel/for-linux-next drm-exynos/exynos-drm-next > >> tegra-drm/drm/tegra/for-next v5.17-rc5 next-20220217] [cannot apply to > >> drm-tip/drm-tip] [If your patch is applied to the wrong git tree, kindly > >> drop us a note. > >> And when submitting patch, we suggest to use '--base' as documented in > >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit- > >> scm.com%2Fdocs%2Fgit-format-patch&data=04%7C01%7CChristian.Koenig% > >> 40amd.com%7C33c94d7ecffe465c671d08d9f5522651%7C3dd8961fe4884e608e11a82 > >> d994e183d%7C0%7C0%7C637810555454343325%7CUnknown%7CTWFpbGZsb3d8eyJWIjo > >> iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000& > >> ;sdata=8Kj1h9%2BCR%2B8nDeUXW%2B%2FQOFbiavK5oHons0mRPyHhq%2F0%3D&re > >> served=0] > >> > >> url: > >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2F0day-ci%2Flinux%2Fcommits%2FJiawei-Gu%2Fdrm-sched-Add-device-pointer-to-drm_gpu_scheduler%2F20220221-175818&data=04%7C01%7CChristian.Koenig%40amd.com%7C33c94d7ecffe465c671d08d9f5522651%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637810555454343325%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=KMrQ%2FsAoUV768eWdTF1FdmXo44kDPjWKnwoi4rvVnqs%3D&reserved=0 > >> base: git://anongit.freedesktop.org/drm/drm drm-next > >> config: ia64-allmodconfig > >> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdow > >> nload.01.org%2F0day-ci%2Farchive%2F20220221%2F202202212330.nxcvFWEe-lk > >> p%40intel.com%2Fconfig&data=04%7C01%7CChristian.Koenig%40amd.com%7 > >> C33c94d7ecffe465c671d08d9f5522651%7C3dd8961fe4884e608e11a82d994e183d%7 > >> C0%7C0%7C637810555454343325%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD > >> AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=tLVb > >> OkxAyxSD%2BVUHUmS6BT5RfOzO4q3sotVZ2YHGV9o%3D&reserved=0) > >> compiler: ia64-linux-gcc (GCC) 11.2.0 > >> reproduce (this is a W=1 build): > >> wget > >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fraw.githubusercontent.com%2Fintel%2Flkp-tests%2Fmaster%2Fsbin%2Fmake.cross&data=04%7C01%7CChristian.Koenig%40amd.com%7C33c94d7ecffe465c671d08d9f5522651%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637810555454343325%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=8QLSr7JTjK87bBGwgOLxU6AU4bCeHoWX2zyx7SGYL7M%3D&reserved=0 > >> -O ~/bin/make.cross > >> chmod +x ~/bin/make.cross > >> # > >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2F0day-ci%2Flinux%2Fcommit%2F9fdafca855faca0a3b8f213f024985c4112fa0bb&data=04%7C01%7CChristian.Koenig%40amd.com%7C33c94d7ecffe465c671d08d9f5522651%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637810555454343325%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=W9HKTScDzhoA1DClCigH2QQUgcIzLStBS%2Bx9ieYPbK4%3D&reserved=0 > >> git remote add linux-review > >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2F0day-ci%2Flinux&data=04%7C01%7CChristian.Koenig%40amd.com%7C33c94d7ecffe465c671d08d9f5522651%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637810555454343325%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=FNJyugHVXenGmYqwgoK9kzKKjC3WGMia%2BNUduLNb0Pc%3D&reserved=0 > >> git fe
Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format
On Wed, Feb 23, 2022 at 03:09:08PM +0100, Marek Vasut wrote: > On 2/23/22 14:47, Maxime Ripard wrote: > > On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote: > > > On 2/23/22 14:41, Maxime Ripard wrote: > > > > Hi, > > > > > > > > On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote: > > > > > Use the new property bus-format to set the enum bus_format and bpc. > > > > > Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi > > > > > support") > > > > > > > > > > Signed-off-by: Max Krummenacher > > > > > > > > > > --- > > > > > > > > > >drivers/gpu/drm/panel/panel-simple.c | 32 > > > > > > > > > >1 file changed, 32 insertions(+) > > > > > > > > > > Relates to the discussion: > > > > > https://lore.kernel.org/all/20220201110717.3585-1-cniederma...@dh-electronics.com/ > > > > > > > > > > Max > > > > > > > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c > > > > > b/drivers/gpu/drm/panel/panel-simple.c > > > > > index c5f133667a2d..5c07260de71c 100644 > > > > > --- a/drivers/gpu/drm/panel/panel-simple.c > > > > > +++ b/drivers/gpu/drm/panel/panel-simple.c > > > > > @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev, > > > > > struct panel_desc *desc; > > > > > unsigned int bus_flags; > > > > > struct videomode vm; > > > > > + const char *format = ""; > > > > > int ret; > > > > > np = dev->of_node; > > > > > @@ -477,6 +478,37 @@ static int panel_dpi_probe(struct device *dev, > > > > > of_property_read_u32(np, "width-mm", &desc->size.width); > > > > > of_property_read_u32(np, "height-mm", &desc->size.height); > > > > > + of_property_read_string(np, "bus-format", &format); > > > > > + if (!strcmp(format, "BGR888_1X24")) { > > > > > + desc->bpc = 8; > > > > > + desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24; > > > > > + } else if (!strcmp(format, "GBR888_1X24")) { > > > > > + desc->bpc = 8; > > > > > + desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24; > > > > > + } else if (!strcmp(format, "RBG888_1X24")) { > > > > > + desc->bpc = 8; > > > > > + desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24; > > > > > + } else if (!strcmp(format, "RGB444_1X12")) { > > > > > + desc->bpc = 6; > > > > > + desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12; > > > > > + } else if (!strcmp(format, "RGB565_1X16")) { > > > > > + desc->bpc = 6; > > > > > + desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16; > > > > > + } else if (!strcmp(format, "RGB666_1X18")) { > > > > > + desc->bpc = 6; > > > > > + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18; > > > > > + } else if (!strcmp(format, "RGB666_1X24_CPADHI")) { > > > > > + desc->bpc = 6; > > > > > + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI; > > > > > + } else if (!strcmp(format, "RGB888_1X24")) { > > > > > + desc->bpc = 8; > > > > > + desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > > > > + } else { > > > > > + dev_err(dev, "%pOF: missing or unknown bus-format > > > > > property\n", > > > > > + np); > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > > > > It doesn't seem right, really. We can't the bus format / bpc be inferred > > > > from the compatible? I'd expect two panels that don't have the same bus > > > > format to not be claimed as compatible. > > > > > > Which compatible ? > > > > > > Note that this is for panel-dpi compatible, i.e. the panel which has > > > timings > > > specified in DT (and needs bus format specified there too). > > > > panel-dpi is supposed to have two compatibles, the panel-specific one > > and panel-dpi. This would obviously be tied to the panel-specific one. > > This whole discussion is about the one which only has 'panel-dpi' compatible > and describes the panel in DT completely. The specific compatible is not > present in DT when this patch is needed. From the panel-dpi DT binding: properties: compatible: description: Shall contain a panel specific compatible and "panel-dpi" in that order. items: - {} - const: panel-dpi The panel-specific compatible is mandatory, whether you like it or not. Maxime signature.asc Description: PGP signature
Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format
On 2/23/22 15:37, Maxime Ripard wrote: On Wed, Feb 23, 2022 at 03:09:08PM +0100, Marek Vasut wrote: On 2/23/22 14:47, Maxime Ripard wrote: On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote: On 2/23/22 14:41, Maxime Ripard wrote: Hi, On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote: Use the new property bus-format to set the enum bus_format and bpc. Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support") Signed-off-by: Max Krummenacher --- drivers/gpu/drm/panel/panel-simple.c | 32 1 file changed, 32 insertions(+) Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniederma...@dh-electronics.com/ Max diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index c5f133667a2d..5c07260de71c 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev, struct panel_desc *desc; unsigned int bus_flags; struct videomode vm; + const char *format = ""; int ret; np = dev->of_node; @@ -477,6 +478,37 @@ static int panel_dpi_probe(struct device *dev, of_property_read_u32(np, "width-mm", &desc->size.width); of_property_read_u32(np, "height-mm", &desc->size.height); + of_property_read_string(np, "bus-format", &format); + if (!strcmp(format, "BGR888_1X24")) { + desc->bpc = 8; + desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24; + } else if (!strcmp(format, "GBR888_1X24")) { + desc->bpc = 8; + desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24; + } else if (!strcmp(format, "RBG888_1X24")) { + desc->bpc = 8; + desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24; + } else if (!strcmp(format, "RGB444_1X12")) { + desc->bpc = 6; + desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12; + } else if (!strcmp(format, "RGB565_1X16")) { + desc->bpc = 6; + desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16; + } else if (!strcmp(format, "RGB666_1X18")) { + desc->bpc = 6; + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18; + } else if (!strcmp(format, "RGB666_1X24_CPADHI")) { + desc->bpc = 6; + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI; + } else if (!strcmp(format, "RGB888_1X24")) { + desc->bpc = 8; + desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24; + } else { + dev_err(dev, "%pOF: missing or unknown bus-format property\n", + np); + return -EINVAL; + } + It doesn't seem right, really. We can't the bus format / bpc be inferred from the compatible? I'd expect two panels that don't have the same bus format to not be claimed as compatible. Which compatible ? Note that this is for panel-dpi compatible, i.e. the panel which has timings specified in DT (and needs bus format specified there too). panel-dpi is supposed to have two compatibles, the panel-specific one and panel-dpi. This would obviously be tied to the panel-specific one. This whole discussion is about the one which only has 'panel-dpi' compatible and describes the panel in DT completely. The specific compatible is not present in DT when this patch is needed. From the panel-dpi DT binding: properties: compatible: description: Shall contain a panel specific compatible and "panel-dpi" in that order. items: - {} - const: panel-dpi The panel-specific compatible is mandatory, whether you like it or not. It doesn't seem to me that's the intended use per panel-simple.c , so maybe the bindings need to be fixed too ?
Re: [PATCH v10 3/4] drm/lsdc: add drm driver for loongson display controller
On Tue, Feb 22, 2022 at 10:46:35PM +0800, Sui Jingfeng wrote: > > On 2022/2/22 16:27, Maxime Ripard wrote: > > > + if (!of_device_is_available(output)) { > > > + of_node_put(output); > > > + drm_info(ddev, "connector%d is not available\n", index); > > > + return NULL; > > > + } > > > + > > > + disp_tims_np = of_get_child_by_name(output, "display-timings"); > > > + if (disp_tims_np) { > > > + lsdc_get_display_timings_from_dtb(output, &lconn->disp_tim); > > > + lconn->has_disp_tim = true; > > > + of_node_put(disp_tims_np); > > > + drm_info(ddev, "Found display timings provided by > > > connector%d\n", index); > > > + } > > > + > > > + connector_type = lsdc_get_connector_type(ddev, output, index); > > > + > > > + if (output) { > > > + of_node_put(output); > > > + output = NULL; > > > + } > > > + > > > +DT_SKIPED: > > > + > > > + /* Only create the i2c channel if display timing is not provided */ > > > + if (!lconn->has_disp_tim) { > > > + const struct lsdc_chip_desc * const desc = ldev->desc; > > > + > > > + if (desc->have_builtin_i2c) > > > + lconn->ddc = lsdc_create_i2c_chan(ddev, index); > > > + else > > > + lconn->ddc = lsdc_get_i2c_adapter(ddev, index); > > This looks weird: the connector bindings have a property to store the > > i2c controller connected to the DDC lines, so you should use that > > instead. > > > This is not weird, ast, mgag200, hibmc do the same thing. And none of them have DT support. Maxime signature.asc Description: PGP signature
Re: [PATCH] drm/simpledrm: Add "panel orientation" property on non-upright mounted LCD panels
On Wed, Feb 23, 2022 at 11:25 AM Hans de Goede wrote: > > Hi, > > On 2/22/22 20:14, Thomas Zimmermann wrote: > > Hi > > > > Am 21.02.22 um 23:00 schrieb Hans de Goede: > >> Some devices use e.g. a portrait panel in a standard laptop casing made > >> for landscape panels. efifb calls drm_get_panel_orientation_quirk() and > >> sets fb_info.fbcon_rotate_hint to make fbcon rotate the console so that > >> it shows up-right instead of on its side. > >> > >> When switching to simpledrm to fbcon renders on its side. Call the > > > > Maybe '... fbcon renders sidewards.' > > that does not sound entirely right to me, so I've gone with: > > "When switching to simpledrm the fbcon renders on its side." Not to completely bike shed but you could say "... the fbcon renders with the incorrect orientation" > as suggested by Javier (so s/to/the/ ). > > > > >> drm_connector_set_panel_orientation_with_quirk() helper to add > >> a "panel orientation" property on devices listed in the quirk table, > >> to make the fbcon (and aware userspace apps) rotate the image to > >> display properly. > >> > >> Cc: Javier Martinez Canillas > >> Signed-off-by: Hans de Goede > > > > Acked-by: Thomas Zimmermann > > Thank you both for the review/ack. I'm currently doing a > test-build of drm-misc-next with the patch with amended > commit msg applied. Once that is done I'll push this out > to drm-misc-next. > > Regards, > > Hans > > > > >> --- > >> drivers/gpu/drm/tiny/simpledrm.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/tiny/simpledrm.c > >> b/drivers/gpu/drm/tiny/simpledrm.c > >> index 04146da2d1d8..11576e0297e4 100644 > >> --- a/drivers/gpu/drm/tiny/simpledrm.c > >> +++ b/drivers/gpu/drm/tiny/simpledrm.c > >> @@ -798,6 +798,9 @@ static int simpledrm_device_init_modeset(struct > >> simpledrm_device *sdev) > >> if (ret) > >> return ret; > >> drm_connector_helper_add(connector, > >> &simpledrm_connector_helper_funcs); > >> +drm_connector_set_panel_orientation_with_quirk(connector, > >> + DRM_MODE_PANEL_ORIENTATION_UNKNOWN, > >> + mode->hdisplay, mode->vdisplay); > >> formats = simpledrm_device_formats(sdev, &nformats); > >> > > >
Re: [PATCH libdrm v2 20/25] tests: tegra: Add VIC 4.0 support
On Fri, Feb 18, 2022 at 11:29:34AM +0200, Mikko Perttunen wrote: > On 2/17/22 21:19, Thierry Reding wrote: > > From: Thierry Reding > > > > The Video Image Composer (VIC) 4.0 can be found on NVIDIA Tegra210 SoCs. > > It uses a different class (B0B6) that is slightly incompatible with the > > class found on earlier generations. > > > > Signed-off-by: Thierry Reding > > --- > > tests/tegra/meson.build | 2 + > > tests/tegra/vic.c | 7 + > > tests/tegra/vic40.c | 370 > > tests/tegra/vic40.h | 285 +++ > > 4 files changed, 664 insertions(+) > > create mode 100644 tests/tegra/vic40.c > > create mode 100644 tests/tegra/vic40.h > > > > diff --git a/tests/tegra/meson.build b/tests/tegra/meson.build > > index 1ee29d0afe1b..e9c2bc875a01 100644 > > --- a/tests/tegra/meson.build > > +++ b/tests/tegra/meson.build > > @@ -36,6 +36,8 @@ libdrm_test_tegra = static_library( > > 'vic.h', > > 'vic30.c', > > 'vic30.h', > > +'vic40.c', > > +'vic40.h', > > ), config_file ], > > include_directories : [inc_root, inc_drm, inc_tegra], > > link_with : libdrm, > > diff --git a/tests/tegra/vic.c b/tests/tegra/vic.c > > index f24961ac5c6d..e0a97c059eca 100644 > > --- a/tests/tegra/vic.c > > +++ b/tests/tegra/vic.c > > @@ -134,6 +134,10 @@ void vic_image_dump(struct vic_image *image, FILE *fp) > > int vic30_new(struct drm_tegra *drm, struct drm_tegra_channel *channel, > > struct vic **vicp); > > +/* from vic40.c */ > > +int vic40_new(struct drm_tegra *drm, struct drm_tegra_channel *channel, > > + struct vic **vicp); > > + > > int vic_new(struct drm_tegra *drm, struct drm_tegra_channel *channel, > > struct vic **vicp) > > { > > @@ -144,6 +148,9 @@ int vic_new(struct drm_tegra *drm, struct > > drm_tegra_channel *channel, > > switch (version) { > > case 0x40: > > return vic30_new(drm, channel, vicp); > > + > > +case 0x21: > > +return vic40_new(drm, channel, vicp); > > } > > return -ENOTSUP; > > diff --git a/tests/tegra/vic40.c b/tests/tegra/vic40.c > > new file mode 100644 > > index ..1a5d2af6b0b6 > > --- /dev/null > > +++ b/tests/tegra/vic40.c > > @@ -0,0 +1,370 @@ > > +/* > > + * Copyright © 2018 NVIDIA Corporation > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a > > + * copy of this software and associated documentation files (the > > "Software"), > > + * to deal in the Software without restriction, including without > > limitation > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > > + * and/or sell copies of the Software, and to permit persons to whom the > > + * Software is furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice shall be included > > in > > + * all copies or substantial portions of the Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS > > OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > > + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR > > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR > > + * OTHER DEALINGS IN THE SOFTWARE. > > + */ > > + > > +#include > > +#include > > + > > +#include "private.h" > > +#include "tegra.h" > > +#include "vic.h" > > +#include "vic40.h" > > + > > +struct vic40 { > > +struct vic base; > > + > > +struct { > > +struct drm_tegra_mapping *map; > > +struct drm_tegra_bo *bo; > > +} config; > > + > > +struct { > > +struct drm_tegra_mapping *map; > > +struct drm_tegra_bo *bo; > > +} filter; > > + > > +struct { > > +struct drm_tegra_mapping *map; > > +struct drm_tegra_bo *bo; > > +} hist; > > +}; > > Histogram buffer not necessary at least on VIC4.0 and later. (Same applies > to VIC4.1 and VIC4.2 patches). I'm pretty sure that I saw SMMU faults without this on all of Tegra210, Tegra186 and Tegra194. I'll go and test this once more. > Also not sure if it's worth duplicating all this for the very minor > differences between VIC4.0/4.1/4.2? In practice you would likely want to compress this a bit. However, as I mentioned before this is meant to serve as a reference implementation and therefore it's a bit more verbose than it would be in a more practical use-case. Thierry signature.asc Description: PGP signature
linux-next: manual merge of the drm-intel tree with the drm-intel-fixes tree
Hi all, Today's linux-next merge of the drm-intel tree got a conflict in: drivers/gpu/drm/i915/display/intel_bw.c between commit: ec663bca9128f ("drm/i915: Fix bw atomic check when switching between SAGV vs. no SAGV") from the drm-intel-fixes tree and commit: 6d8ebef53c2cc ("drm/i915: Extract intel_bw_check_data_rate()") from the drm-intel tree. I fixed it up (see below) and can carry the fix as necessary. This is now fixed as far as linux-next is concerned, but any non trivial conflicts should be mentioned to your upstream maintainer when your tree is submitted for merging. You may also want to consider cooperating with the maintainer of the conflicting tree to minimise any particularly complex conflicts. (took the drm-intel version)
Re: Report 2 in ext4 and journal based on v5.17-rc1
On Wed 23-02-22 09:35:34, Byungchul Park wrote: > On Mon, Feb 21, 2022 at 08:02:04PM +0100, Jan Kara wrote: > > On Thu 17-02-22 20:10:04, Byungchul Park wrote: > > > [9.008161] === > > > [9.008163] DEPT: Circular dependency has been detected. > > > [9.008164] 5.17.0-rc1-00015-gb94f67143867-dirty #2 Tainted: GW > > > [9.008166] --- > > > [9.008167] summary > > > [9.008167] --- > > > [9.008168] *** DEADLOCK *** > > > [9.008168] > > > [9.008168] context A > > > [9.008169] [S] > > > (unknown)(&(&journal->j_wait_transaction_locked)->dmap:0) > > > [9.008171] [W] wait(&(&journal->j_wait_commit)->dmap:0) > > > [9.008172] [E] > > > event(&(&journal->j_wait_transaction_locked)->dmap:0) > > > [9.008173] > > > [9.008173] context B > > > [9.008174] [S] down_write(mapping.invalidate_lock:0) > > > [9.008175] [W] > > > wait(&(&journal->j_wait_transaction_locked)->dmap:0) > > > [9.008176] [E] up_write(mapping.invalidate_lock:0) > > > [9.008177] > > > [9.008178] context C > > > [9.008179] [S] (unknown)(&(&journal->j_wait_commit)->dmap:0) > > > [9.008180] [W] down_write(mapping.invalidate_lock:0) > > > [9.008181] [E] event(&(&journal->j_wait_commit)->dmap:0) > > > [9.008181] > > > [9.008182] [S]: start of the event context > > > [9.008183] [W]: the wait blocked > > > [9.008183] [E]: the event not reachable > > > > So what situation is your tool complaining about here? Can you perhaps show > > it here in more common visualization like: > > Sure. > > > TASK1 TASK2 > > does foo, grabs Z > > does X, grabs lock Y > > blocks on Z > > blocks on Y > > > > or something like that? Because I was not able to decipher this from the > > report even after trying for some time... > > KJOURNALD2(kthread) TASK1(ksys_write) TASK2(ksys_write) > > wait A > --- stuck > wait B > --- stuck > wait C > --- stuck > > wake up B wake up C wake up A > > where: > A is a wait_queue, j_wait_commit > B is a wait_queue, j_wait_transaction_locked > C is a rwsem, mapping.invalidate_lock I see. But a situation like this is not necessarily a guarantee of a deadlock, is it? I mean there can be task D that will eventually call say 'wake up B' and unblock everything and this is how things were designed to work? Multiple sources of wakeups are quite common I'd say... What does Dept do to prevent false reports in cases like this? > The above is the simplest form. And it's worth noting that Dept focuses > on wait and event itself rather than grabing and releasing things like > lock. The following is the more descriptive form of it. > > KJOURNALD2(kthread) TASK1(ksys_write) TASK2(ksys_write) > > wait @j_wait_commit > ext4_truncate_failed_write() > down_write(mapping.invalidate_lock) > > ext4_truncate() > ... > wait @j_wait_transaction_locked > > ext_truncate_failed_write() > > down_write(mapping.invalidate_lock) > > ext4_should_retry_alloc() > ... > __jbd2_log_start_commit() > wake_up(j_wait_commit) > jbd2_journal_commit_transaction() >wake_up(j_wait_transaction_locked) > up_write(mapping.invalidate_lock) > > I hope this would help you understand the report. I see, thanks for explanation! So the above scenario is impossible because for anyone to block on @j_wait_transaction_locked the transaction must be committing, which is done only by kjournald2 kthread and so that thread cannot be waiting at @j_wait_commit. Essentially blocking on @j_wait_transaction_locked means @j_wait_commit wakeup was already done. I guess this shows there can be non-trivial dependencies between wait queues which are difficult to track in an automated way and without such tracking we are going to see false positives... Honza -- Jan Kara SUSE Labs, CR
Re: [PATCH libdrm v2 20/25] tests: tegra: Add VIC 4.0 support
On Wed, Feb 23, 2022 at 03:46:01PM +0100, Thierry Reding wrote: > On Fri, Feb 18, 2022 at 11:29:34AM +0200, Mikko Perttunen wrote: > > On 2/17/22 21:19, Thierry Reding wrote: > > > From: Thierry Reding > > > > > > The Video Image Composer (VIC) 4.0 can be found on NVIDIA Tegra210 SoCs. > > > It uses a different class (B0B6) that is slightly incompatible with the > > > class found on earlier generations. > > > > > > Signed-off-by: Thierry Reding > > > --- > > > tests/tegra/meson.build | 2 + > > > tests/tegra/vic.c | 7 + > > > tests/tegra/vic40.c | 370 > > > tests/tegra/vic40.h | 285 +++ > > > 4 files changed, 664 insertions(+) > > > create mode 100644 tests/tegra/vic40.c > > > create mode 100644 tests/tegra/vic40.h > > > > > > diff --git a/tests/tegra/meson.build b/tests/tegra/meson.build > > > index 1ee29d0afe1b..e9c2bc875a01 100644 > > > --- a/tests/tegra/meson.build > > > +++ b/tests/tegra/meson.build > > > @@ -36,6 +36,8 @@ libdrm_test_tegra = static_library( > > > 'vic.h', > > > 'vic30.c', > > > 'vic30.h', > > > +'vic40.c', > > > +'vic40.h', > > > ), config_file ], > > > include_directories : [inc_root, inc_drm, inc_tegra], > > > link_with : libdrm, > > > diff --git a/tests/tegra/vic.c b/tests/tegra/vic.c > > > index f24961ac5c6d..e0a97c059eca 100644 > > > --- a/tests/tegra/vic.c > > > +++ b/tests/tegra/vic.c > > > @@ -134,6 +134,10 @@ void vic_image_dump(struct vic_image *image, FILE > > > *fp) > > > int vic30_new(struct drm_tegra *drm, struct drm_tegra_channel *channel, > > > struct vic **vicp); > > > +/* from vic40.c */ > > > +int vic40_new(struct drm_tegra *drm, struct drm_tegra_channel *channel, > > > + struct vic **vicp); > > > + > > > int vic_new(struct drm_tegra *drm, struct drm_tegra_channel *channel, > > > struct vic **vicp) > > > { > > > @@ -144,6 +148,9 @@ int vic_new(struct drm_tegra *drm, struct > > > drm_tegra_channel *channel, > > > switch (version) { > > > case 0x40: > > > return vic30_new(drm, channel, vicp); > > > + > > > +case 0x21: > > > +return vic40_new(drm, channel, vicp); > > > } > > > return -ENOTSUP; > > > diff --git a/tests/tegra/vic40.c b/tests/tegra/vic40.c > > > new file mode 100644 > > > index ..1a5d2af6b0b6 > > > --- /dev/null > > > +++ b/tests/tegra/vic40.c > > > @@ -0,0 +1,370 @@ > > > +/* > > > + * Copyright © 2018 NVIDIA Corporation > > > + * > > > + * Permission is hereby granted, free of charge, to any person obtaining > > > a > > > + * copy of this software and associated documentation files (the > > > "Software"), > > > + * to deal in the Software without restriction, including without > > > limitation > > > + * the rights to use, copy, modify, merge, publish, distribute, > > > sublicense, > > > + * and/or sell copies of the Software, and to permit persons to whom the > > > + * Software is furnished to do so, subject to the following conditions: > > > + * > > > + * The above copyright notice and this permission notice shall be > > > included in > > > + * all copies or substantial portions of the Software. > > > + * > > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > > > EXPRESS OR > > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > > > MERCHANTABILITY, > > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT > > > SHALL > > > + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES > > > OR > > > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > > > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR > > > + * OTHER DEALINGS IN THE SOFTWARE. > > > + */ > > > + > > > +#include > > > +#include > > > + > > > +#include "private.h" > > > +#include "tegra.h" > > > +#include "vic.h" > > > +#include "vic40.h" > > > + > > > +struct vic40 { > > > +struct vic base; > > > + > > > +struct { > > > +struct drm_tegra_mapping *map; > > > +struct drm_tegra_bo *bo; > > > +} config; > > > + > > > +struct { > > > +struct drm_tegra_mapping *map; > > > +struct drm_tegra_bo *bo; > > > +} filter; > > > + > > > +struct { > > > +struct drm_tegra_mapping *map; > > > +struct drm_tegra_bo *bo; > > > +} hist; > > > +}; > > > > Histogram buffer not necessary at least on VIC4.0 and later. (Same applies > > to VIC4.1 and VIC4.2 patches). > > I'm pretty sure that I saw SMMU faults without this on all of Tegra210, > Tegra186 and Tegra194. I'll go and test this once more. Nevermind, I see now that this buffer isn't used at all on VIC 4.0, 4.1 and 4.2. I misread your comment as this buffer not needing to be supplied in the command streams. Removed all code related to that from the newer versions of VIC. Thierry signature.asc Description
Re: [PATCH libdrm v2 00/25] Update Tegra support
On Tue, Feb 22, 2022 at 10:41:05AM +0200, Mikko Perttunen wrote: > On 2/21/22 22:29, Dmitry Osipenko wrote: > > 18.02.2022 12:31, Mikko Perttunen пишет: > > > On 2/17/22 21:16, Thierry Reding wrote: > > > > ... > > > > > > Reviewed-by: Mikko Perttunen > > > > > > Left one cosmetic comment in the VIC4.0 patch, but overall looks OK. I > > > think it would be fine to have some basic tests in libdrm as well. > > > > There is a question about who is going to use this libdrm API. Are you > > going to use it in the VAAPI driver? > > > > Grate drivers can't use this API because: > > > > 1. More features are needed > > 2. There is no stable API > > 3. It's super painful to keep all drivers and libdrm in sync from a > > packaging perspective. > > > > It's much more practical nowadays to use DRM directly, without > > SoC-specific libdrm API, i.e. to bundle that SoC-specific API within the > > drivers. > > I'm not planning to use this in the VAAPI driver -- I don't personally have > any use case for the libdrm API. As I mentioned, the intention here was to provide a reference implementation along with a simple API that could be used to achieve results in an easy way, without having to do all the buffer tracking for relocations etc. manually. If other projects find this to be useful, that's great (I was thinking of using this to add a few tests to IGT). If they want to use their own constructs, that's absolutely fine, too. Thierry signature.asc Description: PGP signature
Re: [PATCH 0/3] drm/helpers: Make the suballocation manager drm generic.
Am 23.02.22 um 14:51 schrieb Maarten Lankhorst: Second version of the patch. I didn't fix the copyright (which ame up in the previous version), as I feel the original author should send a patch for that. I've made the suballocator into its own module, and did a cleanup pass on it. The suballocator is generic enough to be useful for any resource that can be subdivided and is guarded by a completion fence. Well the main issue is still that you removed the per allocation alignment. For amdgpu that is not much of a problem, but for radeon that could cause massive issues with UVD semaphore synchronization. Christian. Maarten Lankhorst (3): drm: Extract amdgpu_sa.c as a generic suballocation helper drm/amd: Convert amdgpu to use suballocation helper. drm/radeon: Use the drm suballocation manager implementation. drivers/gpu/drm/Kconfig| 6 + drivers/gpu/drm/Makefile | 3 + drivers/gpu/drm/amd/amdgpu/amdgpu.h| 29 +- drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 5 +- drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 21 +- drivers/gpu/drm/amd/amdgpu/amdgpu_sa.c | 320 +--- drivers/gpu/drm/drm_suballoc.c | 426 + drivers/gpu/drm/radeon/radeon.h| 55 +-- drivers/gpu/drm/radeon/radeon_ib.c | 10 +- drivers/gpu/drm/radeon/radeon_object.h | 23 +- drivers/gpu/drm/radeon/radeon_sa.c | 314 ++- drivers/gpu/drm/radeon/radeon_semaphore.c | 6 +- include/drm/drm_suballoc.h | 78 13 files changed, 603 insertions(+), 693 deletions(-) create mode 100644 drivers/gpu/drm/drm_suballoc.c create mode 100644 include/drm/drm_suballoc.h
Re: [PATCH v10 3/4] drm/lsdc: add drm driver for loongson display controller
On 2022/2/23 22:39, Maxime Ripard wrote: On Tue, Feb 22, 2022 at 10:46:35PM +0800, Sui Jingfeng wrote: On 2022/2/22 16:27, Maxime Ripard wrote: + if (!of_device_is_available(output)) { + of_node_put(output); + drm_info(ddev, "connector%d is not available\n", index); + return NULL; + } + + disp_tims_np = of_get_child_by_name(output, "display-timings"); + if (disp_tims_np) { + lsdc_get_display_timings_from_dtb(output, &lconn->disp_tim); + lconn->has_disp_tim = true; + of_node_put(disp_tims_np); + drm_info(ddev, "Found display timings provided by connector%d\n", index); + } + + connector_type = lsdc_get_connector_type(ddev, output, index); + + if (output) { + of_node_put(output); + output = NULL; + } + +DT_SKIPED: + + /* Only create the i2c channel if display timing is not provided */ + if (!lconn->has_disp_tim) { + const struct lsdc_chip_desc * const desc = ldev->desc; + + if (desc->have_builtin_i2c) + lconn->ddc = lsdc_create_i2c_chan(ddev, index); + else + lconn->ddc = lsdc_get_i2c_adapter(ddev, index); This looks weird: the connector bindings have a property to store the i2c controller connected to the DDC lines, so you should use that instead. This is not weird, ast, mgag200, hibmc do the same thing. And none of them have DT support. Maxime You are wrong, ast driver have dt support. See ast_detect_config_mode() in drm/ast/ast_main.c static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev) { struct device_node *np = dev->dev->of_node; struct ast_private *ast = to_ast_private(dev); struct pci_dev *pdev = to_pci_dev(dev->dev); uint32_t data, jregd0, jregd1; /* Defaults */ ast->config_mode = ast_use_defaults; *scu_rev = 0x; /* Check if we have device-tree properties */ if (np && !of_property_read_u32(np, "aspeed,scu-revision-id", scu_rev)) { /* We do, disable P2A access */ ast->config_mode = ast_use_dt; drm_info(dev, "Using device-tree for configuration\n"); return; } }
Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format
The goal here is to set the element bus_format in the struct panel_desc. This is an enum with the possible values defined in include/uapi/linux/media-bus-format.h. The enum values are not constructed in a way that you could calculate the value from color channel width/shift/mapping/whatever. You rather would have to check if the combination of color channel width/shift/mapping/whatever maps to an existing value and otherwise EINVAL out. I don't see the value in having yet another way of how this information can be specified and then having to write a more complicated parser which maps the dt data to bus_format. On Wed, Feb 23, 2022 at 2:45 PM Marek Vasut wrote: > > On 2/23/22 14:41, Maxime Ripard wrote: > > Hi, > > > > On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote: > >> Use the new property bus-format to set the enum bus_format and bpc. > >> Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support") > >> > >> Signed-off-by: Max Krummenacher > >> > >> --- > >> > >> drivers/gpu/drm/panel/panel-simple.c | 32 > >> 1 file changed, 32 insertions(+) > >> > >> Relates to the discussion: > >> https://lore.kernel.org/all/20220201110717.3585-1-cniederma...@dh-electronics.com/ > >> > >> Max > >> > >> diff --git a/drivers/gpu/drm/panel/panel-simple.c > >> b/drivers/gpu/drm/panel/panel-simple.c > >> index c5f133667a2d..5c07260de71c 100644 > >> --- a/drivers/gpu/drm/panel/panel-simple.c > >> +++ b/drivers/gpu/drm/panel/panel-simple.c > >> @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev, > >> struct panel_desc *desc; > >> unsigned int bus_flags; > >> struct videomode vm; > >> +const char *format = ""; > >> int ret; > >> > >> np = dev->of_node; > >> @@ -477,6 +478,37 @@ static int panel_dpi_probe(struct device *dev, > >> of_property_read_u32(np, "width-mm", &desc->size.width); > >> of_property_read_u32(np, "height-mm", &desc->size.height); > >> > >> +of_property_read_string(np, "bus-format", &format); > >> +if (!strcmp(format, "BGR888_1X24")) { > >> +desc->bpc = 8; > >> +desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24; > >> +} else if (!strcmp(format, "GBR888_1X24")) { > >> +desc->bpc = 8; > >> +desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24; > >> +} else if (!strcmp(format, "RBG888_1X24")) { > >> +desc->bpc = 8; > >> +desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24; > >> +} else if (!strcmp(format, "RGB444_1X12")) { > >> +desc->bpc = 6; > >> +desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12; > >> +} else if (!strcmp(format, "RGB565_1X16")) { > >> +desc->bpc = 6; > >> +desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16; > >> +} else if (!strcmp(format, "RGB666_1X18")) { > >> +desc->bpc = 6; > >> +desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18; > >> +} else if (!strcmp(format, "RGB666_1X24_CPADHI")) { > >> +desc->bpc = 6; > >> +desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI; > >> +} else if (!strcmp(format, "RGB888_1X24")) { > >> +desc->bpc = 8; > >> +desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24; > >> +} else { > >> +dev_err(dev, "%pOF: missing or unknown bus-format property\n", > >> +np); > >> +return -EINVAL; > >> +} > >> + > > > > It doesn't seem right, really. We can't the bus format / bpc be inferred > > from the compatible? I'd expect two panels that don't have the same bus > > format to not be claimed as compatible. > > Which compatible ? > > Note that this is for panel-dpi compatible, i.e. the panel which has > timings specified in DT (and needs bus format specified there too). > > I agree this doesn't look right however, some more generic color channel > width/shift/mapping might be better.
Re: [PATCH v10 2/4] Documentation/dt: Add descriptions for loongson display controller
On 2022/2/23 21:56, 隋景峰 wrote: Something like this: dt-bindings: display: Add Loongson display controller Hi, We are not a platform device driver, there is no of_device_id defined in my driver. In other word, my driver will not bind against devices whose compatible is "loongson,ls7a1000-dc". We just parse the device tree actively, find necessary information of interest. What's the meaning of dt-bindings by definition ? In this case, can I use the word "dt-bindings" in the commit title? I want to follow the conventions, but get some push back, Krzysztof say that he can not see any bindings, these are not bindings.
Re: [PATCH v10 2/4] Documentation/dt: Add descriptions for loongson display controller
On 23/02/2022 14:56, 隋景峰 wrote: > > > > > -Original Messages- > > From: "Rob Herring" > > Sent Time: 2022-02-23 07:02:34 (Wednesday) > > To: "Sui Jingfeng" <15330273...@189.cn> > > Cc: "Maxime Ripard" , "Thomas Zimmermann" > , "Roland Scheidegger" , "Zack > Rusin" , "Christian Gmeiner" , > "David Airlie" , "Daniel Vetter" , "Thomas > Bogendoerfer" , "Dan Carpenter" > , "Krzysztof Kozlowski" , "Andrey > Zhizhikin" , "Sam Ravnborg" > , "David S . Miller" , "Jiaxun Yang" > , "Lucas Stach" , "Maarten > Lankhorst" , "Ilia Mirkin" > , "Qing Zhang" , suijingfeng > , linux-m...@vger.kernel.org, > linux-ker...@vger.kernel.org, devicet...@vger.kernel.org, > dri-devel@lists.freedesktop.org > > Subject: Re: [PATCH v10 2/4] Documentation/dt: Add descriptions for > loongson display controller > > > > On Sun, Feb 20, 2022 at 10:55:52PM +0800, Sui Jingfeng wrote: > > > From: suijingfeng > > > > Follow the conventions of the subsystem for patch subjects. It should be > > evident with 'git log --oneline > Documentation/devicetree/bindings/display'. > > > > Something like this: > > > > dt-bindings: display: Add Loongson display controller > > > > Hi, > > We are not a platform device driver, there is no > of_device_id defined in my driver. In other word, > my driver will not bind against devices whose compatible > is "loongson,ls7a1000-dc". We just parse the device tree > actively, find necessary information of interest. > In this case, can I use the word "dt-bindings" in the commit title? This is a patch for specific subsystem, so as Rob said, it should follow subsystem conventions. The patch itself is a dt-bindings patch, so there is nothing here special which would encourage for any exception. > > I want to follow the conventions, but get some push back, > Krzysztof say that he can not see any bindings, these are not bindings. I said in comment to your patch with DTS, which you called bindings, that there are no bindings at all in it. Because in your patch with DTS you did not include bindings, but you called it bindings. Here, this is a patch with bindings, so your comment "these are not bindings" is not true. This link does not work... > This email and its attachments contain confidential information from Loongson > Technology , which is intended only for the person or entity whose address is > listed above. Any use of the information contained herein in any way > (including, but not limited to, total or partial disclosure, reproduction or > dissemination) by persons other than the intended recipient(s) is prohibited. > If you receive this email in error, please notify the sender by phone or > email immediately and delete it. Such automatic footers do not help. Could you work on a way to avoid them? Best regards, Krzysztof
Re: [PATCH v10 3/4] drm/lsdc: add drm driver for loongson display controller
On Wed, Feb 23, 2022 at 11:14:12PM +0800, Sui Jingfeng wrote: > > On 2022/2/23 22:39, Maxime Ripard wrote: > > On Tue, Feb 22, 2022 at 10:46:35PM +0800, Sui Jingfeng wrote: > > > On 2022/2/22 16:27, Maxime Ripard wrote: > > > > > + if (!of_device_is_available(output)) { > > > > > + of_node_put(output); > > > > > + drm_info(ddev, "connector%d is not available\n", index); > > > > > + return NULL; > > > > > + } > > > > > + > > > > > + disp_tims_np = of_get_child_by_name(output, "display-timings"); > > > > > + if (disp_tims_np) { > > > > > + lsdc_get_display_timings_from_dtb(output, > > > > > &lconn->disp_tim); > > > > > + lconn->has_disp_tim = true; > > > > > + of_node_put(disp_tims_np); > > > > > + drm_info(ddev, "Found display timings provided by > > > > > connector%d\n", index); > > > > > + } > > > > > + > > > > > + connector_type = lsdc_get_connector_type(ddev, output, index); > > > > > + > > > > > + if (output) { > > > > > + of_node_put(output); > > > > > + output = NULL; > > > > > + } > > > > > + > > > > > +DT_SKIPED: > > > > > + > > > > > + /* Only create the i2c channel if display timing is not > > > > > provided */ > > > > > + if (!lconn->has_disp_tim) { > > > > > + const struct lsdc_chip_desc * const desc = ldev->desc; > > > > > + > > > > > + if (desc->have_builtin_i2c) > > > > > + lconn->ddc = lsdc_create_i2c_chan(ddev, index); > > > > > + else > > > > > + lconn->ddc = lsdc_get_i2c_adapter(ddev, index); > > > > This looks weird: the connector bindings have a property to store the > > > > i2c controller connected to the DDC lines, so you should use that > > > > instead. > > > > > > > This is not weird, ast, mgag200, hibmc do the same thing. > > And none of them have DT support. > > > > Maxime > > You are wrong, ast driver have dt support. See ast_detect_config_mode() in > drm/ast/ast_main.c > > static void ast_detect_config_mode(struct drm_device *dev, u32 *scu_rev) > { > struct device_node *np = dev->dev->of_node; > struct ast_private *ast = to_ast_private(dev); > struct pci_dev *pdev = to_pci_dev(dev->dev); > uint32_t data, jregd0, jregd1; > > /* Defaults */ > ast->config_mode = ast_use_defaults; > *scu_rev = 0x; > > /* Check if we have device-tree properties */ > if (np && !of_property_read_u32(np, "aspeed,scu-revision-id", > scu_rev)) { > /* We do, disable P2A access */ > ast->config_mode = ast_use_dt; > drm_info(dev, "Using device-tree for configuration\n"); > return; > } > > > > } It doesn't seem to probe from the DT though. It uses 4 properties, and none of them are documented. It's still a widely different case than your driver that uses the connector binding, and therefore has access to the ddc bus there. Maxime signature.asc Description: PGP signature
Re: [PATCH] drm/bridge: ti-sn65dsi86: Properly undo autosuspend
Hi, On Tue, Feb 22, 2022 at 9:08 PM Laurent Pinchart wrote: > > On Tue, Feb 22, 2022 at 11:44:54PM +0100, Linus Walleij wrote: > > On Tue, Feb 22, 2022 at 11:19 PM Douglas Anderson > > wrote: > > > > > > The PM Runtime docs say: > > > Drivers in ->remove() callback should undo the runtime PM changes done > > > in ->probe(). Usually this means calling pm_runtime_disable(), > > > pm_runtime_dont_use_autosuspend() etc. > > > > > > We weren't doing that for autosuspend. Let's do it. > > > > > > Fixes: 9bede63127c6 ("drm/bridge: ti-sn65dsi86: Use pm_runtime > > > autosuspend") > > > Signed-off-by: Douglas Anderson > > > > Hm. I know a few places in drivers where I don't do this :/ > > It seems to be a very common problem indeed, I haven't seen any driver > yet that uses pm_runtime_dont_use_autosuspend(). We could play a game of > whack-a-mole, but we'll never win. Could this be solved in the runtime > PM framework instead ? pm_runtime_disable() could disable auto-suspend. > If there are legitimate use cases for disabling runtime PM temporarily > without disabling auto-suspend, then a new function designed > specifically for remove() that would take care of cleaning everything up > could be another option. Yeah, it would be good. It's probably not a yak I have time to shave right now, though. :( I _suspect_ that there are legitimate reasons we can't just magically do it in pm_runtime_disable(). If nothing else I believe there are legitimate code paths during normal operation that look like this: pm_runtime_disable(); do_something_that_needs_pm_runtime_disabled(); pm_runtime_enable(); Also: if it were really a simple problem to solve one would have thought that it would have been solved initially instead of adding documentation particularly mentioning pm_runtime_dont_use_autosuspend() How about a middle ground, though: we could add a devm function that does all the magic. Somewhat recently devm_pm_runtime_enable() was added. What if we add a variant for those that use autosuspend, like: devm_pm_runtime_enable_with_autosuspend(dev, initial_delay) That would: * pm_runtime_enable() * pm_runtime_set_autosuspend_delay() * pm_runtime_use_autosuspend() * Use devm_add_action_or_reset() to undo everything. Assuming that the pm_runtime folks are OK with that, we could transition things over to the new function once it rolls into mainline. So this doesn't magically fix all existing code but provides a path to make this more discoverable. -Doug
Re: [PATCH v10 2/4] Documentation/dt: Add descriptions for loongson display controller
On 23/02/2022 16:35, Sui Jingfeng wrote: > > On 2022/2/23 21:56, 隋景峰 wrote: >> Something like this: >> >> dt-bindings: display: Add Loongson display controller > > Hi, Thanks for resending in a proper format. I already replied to your original post, so let me paste it here as well. > > We are not a platform device driver, there is no > of_device_id defined in my driver. In other word, > my driver will not bind against devices whose compatible > is "loongson,ls7a1000-dc". We just parse the device tree > actively, find necessary information of interest. > > What's the meaning of dt-bindings by definition ? > In this case, can I use the word "dt-bindings" in the commit title? This is a patch for specific subsystem, so as Rob said, it should follow subsystem conventions. The patch itself is a dt-bindings patch, so there is nothing here special which would encourage for any exception. > > I want to follow the conventions, but get some push back, > Krzysztof say that he can not see any bindings, these are not bindings. I said in comment to your patch with DTS, which you called bindings, that there are no bindings at all in it. Because in your patch with DTS you did not include bindings, but you called it bindings. Here, this is a patch with bindings, so your comment "these are not bindings" is not true. Best regards, Krzysztof
Re: [PATCH v10 3/4] drm/lsdc: add drm driver for loongson display controller
On 2022/2/23 22:39, Maxime Ripard wrote: On Tue, Feb 22, 2022 at 10:46:35PM +0800, Sui Jingfeng wrote: On 2022/2/22 16:27, Maxime Ripard wrote: + if (!of_device_is_available(output)) { + of_node_put(output); + drm_info(ddev, "connector%d is not available\n", index); + return NULL; + } + + disp_tims_np = of_get_child_by_name(output, "display-timings"); + if (disp_tims_np) { + lsdc_get_display_timings_from_dtb(output, &lconn->disp_tim); + lconn->has_disp_tim = true; + of_node_put(disp_tims_np); + drm_info(ddev, "Found display timings provided by connector%d\n", index); + } + + connector_type = lsdc_get_connector_type(ddev, output, index); + + if (output) { + of_node_put(output); + output = NULL; + } + +DT_SKIPED: + + /* Only create the i2c channel if display timing is not provided */ + if (!lconn->has_disp_tim) { + const struct lsdc_chip_desc * const desc = ldev->desc; + + if (desc->have_builtin_i2c) + lconn->ddc = lsdc_create_i2c_chan(ddev, index); + else + lconn->ddc = lsdc_get_i2c_adapter(ddev, index); This looks weird: the connector bindings have a property to store the i2c controller connected to the DDC lines, so you should use that instead. This is not weird, ast, mgag200, hibmc do the same thing. And none of them have DT support. Maxime Ok, I have already correct this issue. see it at the next version.
Re: [PATCH] drm/msm/gpu: Fix crash on devices without devfreq support
On Tue, Feb 22, 2022 at 7:11 PM Dmitry Baryshkov wrote: > > On 19/02/2022 21:33, Rob Clark wrote: > > From: Rob Clark > > > > Avoid going down devfreq paths on devices where devfreq is not > > initialized. > > > > Reported-by: Linux Kernel Functional Testing > > Reported-by: Anders Roxell > > Signed-off-by: Rob Clark > > --- > > drivers/gpu/drm/msm/msm_gpu_devfreq.c | 31 +-- > > 1 file changed, 25 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c > > b/drivers/gpu/drm/msm/msm_gpu_devfreq.c > > index 9bf319be11f6..26a3669a97b3 100644 > > --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c > > +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c > > @@ -83,12 +83,17 @@ static struct devfreq_dev_profile msm_devfreq_profile = > > { > > static void msm_devfreq_boost_work(struct kthread_work *work); > > static void msm_devfreq_idle_work(struct kthread_work *work); > > > > +static bool has_devfreq(struct msm_gpu *gpu) > > +{ > > + return !!gpu->funcs->gpu_busy; > > I see that devfreq init will be skipped if gpu_busy is NULL. > Can we use gpu->devfreq instead of this condition? We could, but then we couldn't also use the same has_devfreq() helper in msm_devfreq_init(). I thought it was clearer to use the same helper everywhere. > I noticed that you have replaced some of gpu->devfreq checks with > has_devreq() calls. Is there any difference? It amounts to the same thing because if you don't have gpu_busy, then devfreq is never initialized. I just thought it clearer to use the same check in all places. BR, -R > > +} > > + > > void msm_devfreq_init(struct msm_gpu *gpu) > > { > > struct msm_gpu_devfreq *df = &gpu->devfreq; > > > > /* We need target support to do devfreq */ > > - if (!gpu->funcs->gpu_busy) > > + if (!has_devfreq(gpu)) > > return; > > > > dev_pm_qos_add_request(&gpu->pdev->dev, &df->idle_freq, > > @@ -149,6 +154,9 @@ void msm_devfreq_cleanup(struct msm_gpu *gpu) > > { > > struct msm_gpu_devfreq *df = &gpu->devfreq; > > > > + if (!has_devfreq(gpu)) > > + return; > > + > > devfreq_cooling_unregister(gpu->cooling); > > dev_pm_qos_remove_request(&df->boost_freq); > > dev_pm_qos_remove_request(&df->idle_freq); > > @@ -156,16 +164,24 @@ void msm_devfreq_cleanup(struct msm_gpu *gpu) > > > > void msm_devfreq_resume(struct msm_gpu *gpu) > > { > > - gpu->devfreq.busy_cycles = 0; > > - gpu->devfreq.time = ktime_get(); > > + struct msm_gpu_devfreq *df = &gpu->devfreq; > > > > - devfreq_resume_device(gpu->devfreq.devfreq); > > + if (!has_devfreq(gpu)) > > + return; > > + > > + df->busy_cycles = 0; > > + df->time = ktime_get(); > > + > > + devfreq_resume_device(df->devfreq); > > } > > > > void msm_devfreq_suspend(struct msm_gpu *gpu) > > { > > struct msm_gpu_devfreq *df = &gpu->devfreq; > > > > + if (!has_devfreq(gpu)) > > + return; > > + > > devfreq_suspend_device(df->devfreq); > > > > cancel_idle_work(df); > > @@ -185,6 +201,9 @@ void msm_devfreq_boost(struct msm_gpu *gpu, unsigned > > factor) > > struct msm_gpu_devfreq *df = &gpu->devfreq; > > uint64_t freq; > > > > + if (!has_devfreq(gpu)) > > + return; > > + > > freq = get_freq(gpu); > > freq *= factor; > > > > @@ -207,7 +226,7 @@ void msm_devfreq_active(struct msm_gpu *gpu) > > struct devfreq_dev_status status; > > unsigned int idle_time; > > > > - if (!df->devfreq) > > + if (!has_devfreq(gpu)) > > return; > > > > /* > > @@ -253,7 +272,7 @@ void msm_devfreq_idle(struct msm_gpu *gpu) > > { > > struct msm_gpu_devfreq *df = &gpu->devfreq; > > > > - if (!df->devfreq) > > + if (!has_devfreq(gpu)) > > return; > > > > msm_hrtimer_queue_work(&df->idle_work, ms_to_ktime(1), > > > -- > With best wishes > Dmitry
Re: [PATCH v6 23/23] dt-bindings: display: rockchip: dw-hdmi: fix ports description
On Thu, 17 Feb 2022 09:29:54 +0100, Sascha Hauer wrote: > Current port description doesn't cover all possible cases. It currently > expects one single port with two endpoints. > > When the HDMI connector is described in the device tree there can be two > ports, first one going to the VOP and the second one going to the connector. > > Also on SoCs which only have a single VOP there will be only one > endpoint instead of two. > > This patch addresses both issues. With this there can either be a single > port ("port") , or two of them ("port@0", "port@1") when the connector > is also in the device tree. Also the first or only port can either have > one endpoint ("endpoint") for single VOP SoCs or two ("endpoint@0", > "endpoint@1") for dual VOP SoCs. > > Signed-off-by: Sascha Hauer > --- > > Notes: > Changes since v5: > - new patch > > .../display/rockchip/rockchip,dw-hdmi.yaml| 24 +++ > 1 file changed, 9 insertions(+), 15 deletions(-) > Reviewed-by: Rob Herring
Re: [PATCH] drm/bridge: ti-sn65dsi86: Properly undo autosuspend
Hi Doug, On Wed, Feb 23, 2022 at 07:43:27AM -0800, Doug Anderson wrote: > On Tue, Feb 22, 2022 at 9:08 PM Laurent Pinchart wrote: > > On Tue, Feb 22, 2022 at 11:44:54PM +0100, Linus Walleij wrote: > > > On Tue, Feb 22, 2022 at 11:19 PM Douglas Anderson wrote: > > > > > > > > The PM Runtime docs say: > > > > Drivers in ->remove() callback should undo the runtime PM changes done > > > > in ->probe(). Usually this means calling pm_runtime_disable(), > > > > pm_runtime_dont_use_autosuspend() etc. > > > > > > > > We weren't doing that for autosuspend. Let's do it. > > > > > > > > Fixes: 9bede63127c6 ("drm/bridge: ti-sn65dsi86: Use pm_runtime > > > > autosuspend") > > > > Signed-off-by: Douglas Anderson > > > > > > Hm. I know a few places in drivers where I don't do this :/ > > > > It seems to be a very common problem indeed, I haven't seen any driver > > yet that uses pm_runtime_dont_use_autosuspend(). We could play a game of > > whack-a-mole, but we'll never win. Could this be solved in the runtime > > PM framework instead ? pm_runtime_disable() could disable auto-suspend. > > If there are legitimate use cases for disabling runtime PM temporarily > > without disabling auto-suspend, then a new function designed > > specifically for remove() that would take care of cleaning everything up > > could be another option. > > Yeah, it would be good. It's probably not a yak I have time to shave > right now, though. :( I don't insist on shaving that yak right now :-) This patch is fine. > I _suspect_ that there are legitimate reasons we can't just magically > do it in pm_runtime_disable(). If nothing else I believe there are > legitimate code paths during normal operation that look like this: > > pm_runtime_disable(); > do_something_that_needs_pm_runtime_disabled(); > pm_runtime_enable(); > > Also: if it were really a simple problem to solve one would have > thought that it would have been solved initially instead of adding > documentation particularly mentioning > pm_runtime_dont_use_autosuspend() I'm not sure, look at how long it took for us to get pm_runtime_resume_and_get(). The problem has been considered for years as a non-issue by the runtime PM developers. It feels like the API is developed without considering its users. > How about a middle ground, though: we could add a devm function that > does all the magic. Somewhat recently devm_pm_runtime_enable() was > added. What if we add a variant for those that use autosuspend, like: > > devm_pm_runtime_enable_with_autosuspend(dev, initial_delay) > > That would: > * pm_runtime_enable() > * pm_runtime_set_autosuspend_delay() > * pm_runtime_use_autosuspend() > * Use devm_add_action_or_reset() to undo everything. > > Assuming that the pm_runtime folks are OK with that, we could > transition things over to the new function once it rolls into > mainline. > > So this doesn't magically fix all existing code but provides a path to > make this more discoverable. Sounds like a good idea. I wonder if this could be handled by devm_pm_runtime_enable() actually. If a driver calls devm_pm_runtime_enable() and then enables auto-suspend, can't the runtime PM core reasonably expect that auto-suspend should be disabled at .remove() time ? The pm_runtime_disable_action() function could disable auto-suspend unconditionally (assuming pm_runtime_use_autosuspend() and pm_runtime_dont_use_autosuspend() don't need to be balanced, if they do, then I'll just go cry in a corner). -- Regards, Laurent Pinchart
Re: [PATCH v2] drm/panel: Select DRM_DP_HELPER for DRM_PANEL_EDP
Hi, On Tue, Feb 22, 2022 at 1:31 AM Geert Uytterhoeven wrote: > > On Tue, Feb 8, 2022 at 10:39 AM Geert Uytterhoeven > wrote: > > On Mon, Feb 7, 2022 at 12:31 PM Thomas Zimmermann > > wrote: > > > As reported in [1], DRM_PANEL_EDP depends on DRM_DP_HELPER. Select > > > the option to fix the build failure. The error message is shown > > > below. > > > > > > arm-linux-gnueabihf-ld: drivers/gpu/drm/panel/panel-edp.o: in function > > > `panel_edp_probe': panel-edp.c:(.text+0xb74): undefined reference to > > > `drm_panel_dp_aux_backlight' > > > make[1]: *** [/builds/linux/Makefile:1222: vmlinux] Error 1 > > > > > > The issue has been reported before, when DisplayPort helpers were > > > hidden behind the option CONFIG_DRM_KMS_HELPER. [2] > > > > > > v2: > > > * fix and expand commit description (Arnd) > > > > > > Signed-off-by: Thomas Zimmermann > > > > Thanks for your patch! > > > > This fixes the build errors I'm seeing, so > > Tested-by: Geert Uytterhoeven > > Is this planned to be queued? This is still failing in drm-next. > Thanks! Looks like this has been in drm-misc-next since Feb 4: --- commit eea89dff4c39a106f98d1cb5e4d626f8c63908b9 Author: Thomas Zimmermann AuthorDate: Thu Feb 3 10:39:22 2022 +0100 Commit: Thomas Zimmermann CommitDate: Fri Feb 4 09:38:47 2022 +0100 drm/panel: Select DRM_DP_HELPER for DRM_PANEL_EDP --- Maybe it needed to land elsewhere, though? -Doug
Re: [PATCH] drm/bridge_connector: enable HPD by default if supported
Hi Laurent, Nikita, Quoting Laurent Pinchart (2021-12-29 23:44:29) > Hi Nikita, > > Thank you for the patch. > > On Sat, Dec 25, 2021 at 09:31:51AM +0300, Nikita Yushchenko wrote: > > Hotplug events reported by bridge drivers over drm_bridge_hpd_notify() > > get ignored unless somebody calls drm_bridge_hpd_enable(). When the > > connector for the bridge is bridge_connector, such a call is done from > > drm_bridge_connector_enable_hpd(). > > > > However drm_bridge_connector_enable_hpd() is never called on init paths, > > documentation suggests that it is intended for suspend/resume paths. > > H... I'm in two minds about this. The problem description is > correct, but I wonder if HPD should be enabled unconditionally here, or > if this should be left to display drivers to control. > drivers/gpu/drm/imx/dcss/dcss-kms.c enables HPD manually at init time, > other drivers don't. > > It feels like this should be under control of the display controller > driver, but I can't think of a use case for not enabling HPD at init > time. Any second opinion from anyone ? This patch solves an issue I have where I have recently enabled HPD on the SN65DSI86, but without this, I do not get calls to my .hpd_enable or .hpd_disable hooks that I have added to the ti_sn_bridge_funcs. So it needs to be enabled somewhere, and this seems reasonable to me? It's not directly related to the display controller - as it's a factor of the bridge? On Falcon-V3U with HPD additions to SN65DSI86: Tested-by: Kieran Bingham > > In result, once encoders are switched to bridge_connector, > > bridge-detected HPD stops working. > > > > This patch adds a call to that API on init path. > > > > This fixes HDMI HPD with rcar-du + adv7513 case when adv7513 reports HPD > > events via interrupts. > > > > Fixes: c24110a8fd09 ("drm: rcar-du: Use drm_bridge_connector_init() helper") > > Signed-off-by: Nikita Yushchenko > > --- > > drivers/gpu/drm/drm_bridge_connector.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c > > b/drivers/gpu/drm/drm_bridge_connector.c > > index 791379816837..4f20137ef21d 100644 > > --- a/drivers/gpu/drm/drm_bridge_connector.c > > +++ b/drivers/gpu/drm/drm_bridge_connector.c > > @@ -369,8 +369,10 @@ struct drm_connector *drm_bridge_connector_init(struct > > drm_device *drm, > > connector_type, ddc); > > drm_connector_helper_add(connector, > > &drm_bridge_connector_helper_funcs); > > > > - if (bridge_connector->bridge_hpd) > > + if (bridge_connector->bridge_hpd) { > > connector->polled = DRM_CONNECTOR_POLL_HPD; > > + drm_bridge_connector_enable_hpd(connector); > > + } > > else if (bridge_connector->bridge_detect) > > connector->polled = DRM_CONNECTOR_POLL_CONNECT > > | DRM_CONNECTOR_POLL_DISCONNECT; > > -- > Regards, > > Laurent Pinchart
Re: [PATCH] drm/msm: Avoid dirtyfb stalls on video mode displays
On Wed, Feb 23, 2022 at 2:00 AM Dmitry Baryshkov wrote: > > On 19/02/2022 22:39, Rob Clark wrote: > > From: Rob Clark > > > > Someone on IRC once asked an innocent enough sounding question: Why > > with xf86-video-modesetting is es2gears limited at 120fps. > > > > So I broke out the perfetto tracing mesa MR and took a look. It turns > > out the problem was drm_atomic_helper_dirtyfb(), which would end up > > waiting for vblank.. es2gears would rapidly push two frames to Xorg, > > which would blit them to screen and in idle hook (I assume) call the > > DIRTYFB ioctl. Which in turn would do an atomic update to flush the > > dirty rects, which would stall until the next vblank. And then the > > whole process would repeat. > > > > But this is a bit silly, we only need dirtyfb for command mode DSI > > panels. So lets just skip it otherwise. > > > > Signed-off-by: Rob Clark > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 13 + > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 9 > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 1 + > > drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 9 > > drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 1 + > > drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h | 1 + > > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 8 +++ > > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 1 + > > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h | 1 + > > drivers/gpu/drm/msm/msm_fb.c | 64 ++- > > drivers/gpu/drm/msm/msm_kms.h | 2 + > > 11 files changed, 109 insertions(+), 1 deletion(-) > > > > I have checked your previous dirtyfb patch (and corresponding discussion). > > I'm not fond of the idea of acquiring locks, computing the value, then > releasing the locks and reacquiring the locks again. I understand the > original needs_dirtyfb approach was frowned upon. Do we have a chance of > introducing drm_atomic_helper_dirtyfb_unlocked()? Which would perform > all the steps of the orignal helper, but without locks acquirement (and > state allocation/freeing)? > The locking is really more just to avoid racing state access with state being free'd. The sort of race you could have is perhaps dirtyfb racing with attaching the fb to a cmd mode plane->crtc->encoder chain. I think this is relatively harmless since that act would flush the fb anyways. But it did give me an idea for a possibly simpler approach.. we might be able to just keep a refcnt of cmd mode panels the fb is indirectly attached to, and then msm_framebuffer_dirtyfb() simply has to check if that count is greater than zero. If we increment/decrement the count in fb->prepare()/cleanup() that should also solve the race. BR, -R
[GIT PULL] drm/tegra: Fixes for v5.17-rc6
Hi Dave, Daniel, The following changes since commit e783362eb54cd99b2cac8b3a9aeac942e6f6ac07: Linux 5.17-rc1 (2022-01-23 10:12:53 +0200) are available in the Git repository at: https://gitlab.freedesktop.org/drm/tegra.git tags/drm/tegra/for-5.17-rc6 for you to fetch changes up to 8913e1aea4b32a866343b14e565c62cec54f3f78: drm/tegra: dpaux: Populate AUX bus (2022-02-23 13:00:37 +0100) Thanks, Thierry drm/tegra: Fixes for v5.17-rc6 Contains a couple of fixes for Tegra186 suspend/resume, syncpoint waiting, a build warning and eDP on older Tegra devices. Dmitry Osipenko (1): gpu: host1x: Fix hang on Tegra186+ Jon Hunter (1): drm/tegra: Fix cast to restricted __le32 Mikko Perttunen (1): gpu: host1x: Always return syncpoint value when waiting Thierry Reding (1): drm/tegra: dpaux: Populate AUX bus drivers/gpu/drm/tegra/Kconfig | 1 + drivers/gpu/drm/tegra/dpaux.c | 7 +++ drivers/gpu/drm/tegra/falcon.c | 2 +- drivers/gpu/host1x/syncpt.c| 35 ++- 4 files changed, 19 insertions(+), 26 deletions(-)
Re: [PATCH] drm/bridge_connector: enable HPD by default if supported
Hello, On Wed, Feb 23, 2022 at 04:17:22PM +, Kieran Bingham wrote: > Quoting Laurent Pinchart (2021-12-29 23:44:29) > > On Sat, Dec 25, 2021 at 09:31:51AM +0300, Nikita Yushchenko wrote: > > > Hotplug events reported by bridge drivers over drm_bridge_hpd_notify() > > > get ignored unless somebody calls drm_bridge_hpd_enable(). When the > > > connector for the bridge is bridge_connector, such a call is done from > > > drm_bridge_connector_enable_hpd(). > > > > > > However drm_bridge_connector_enable_hpd() is never called on init paths, > > > documentation suggests that it is intended for suspend/resume paths. > > > > H... I'm in two minds about this. The problem description is > > correct, but I wonder if HPD should be enabled unconditionally here, or > > if this should be left to display drivers to control. > > drivers/gpu/drm/imx/dcss/dcss-kms.c enables HPD manually at init time, > > other drivers don't. > > > > It feels like this should be under control of the display controller > > driver, but I can't think of a use case for not enabling HPD at init > > time. Any second opinion from anyone ? > > This patch solves an issue I have where I have recently enabled HPD on > the SN65DSI86, but without this, I do not get calls to my .hpd_enable or > .hpd_disable hooks that I have added to the ti_sn_bridge_funcs. > > So it needs to be enabled somewhere, and this seems reasonable to me? > It's not directly related to the display controller - as it's a factor > of the bridge? > > On Falcon-V3U with HPD additions to SN65DSI86: > Tested-by: Kieran Bingham If you think this is right, then Reviewed-by: Laurent Pinchart > > > In result, once encoders are switched to bridge_connector, > > > bridge-detected HPD stops working. > > > > > > This patch adds a call to that API on init path. > > > > > > This fixes HDMI HPD with rcar-du + adv7513 case when adv7513 reports HPD > > > events via interrupts. > > > > > > Fixes: c24110a8fd09 ("drm: rcar-du: Use drm_bridge_connector_init() > > > helper") > > > Signed-off-by: Nikita Yushchenko > > > --- > > > drivers/gpu/drm/drm_bridge_connector.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c > > > b/drivers/gpu/drm/drm_bridge_connector.c > > > index 791379816837..4f20137ef21d 100644 > > > --- a/drivers/gpu/drm/drm_bridge_connector.c > > > +++ b/drivers/gpu/drm/drm_bridge_connector.c > > > @@ -369,8 +369,10 @@ struct drm_connector > > > *drm_bridge_connector_init(struct drm_device *drm, > > > connector_type, ddc); > > > drm_connector_helper_add(connector, > > > &drm_bridge_connector_helper_funcs); > > > > > > - if (bridge_connector->bridge_hpd) > > > + if (bridge_connector->bridge_hpd) { > > > connector->polled = DRM_CONNECTOR_POLL_HPD; > > > + drm_bridge_connector_enable_hpd(connector); > > > + } > > > else if (bridge_connector->bridge_detect) > > > connector->polled = DRM_CONNECTOR_POLL_CONNECT > > > | DRM_CONNECTOR_POLL_DISCONNECT; -- Regards, Laurent Pinchart
Re: [PATCH v3] simplefb: Enable boot time VESA graphic mode selection.
Hello Michal, On 2/18/22 17:04, Michal Suchanek wrote: > Since switch to simplefb/simpledrm VESA graphic modes are no longer > available with legacy BIOS. > Maybe you can mention that is the "vga=" kernel command line parameter since that may be more evident to people reading the commit message ? > The x86 realmode boot code enables the VESA graphic modes when option > FB_BOOT_VESA_SUPPORT is enabled. > > To enable use of VESA modes with simplefb in legacy BIOS boot mode drop I think you meant "VESA modes with the sysfb driver" ? or something like that since otherwise it seems that you meant to use it with the simplefb (drivers/video/fbdev/simplefb.c) fbdev driver, which doesn't support the "vga=" param as far as I understand (it just uses whatever was setup). The name sysfb_simplefb is really horrible, because it is too confusing and probably we should change it at some point... Patch itself looks good to me though. Reviewed-by: Javier Martinez Canillas Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH] drm/bridge: ti-sn65dsi86: Properly undo autosuspend
Hi, On Wed, Feb 23, 2022 at 7:55 AM Laurent Pinchart wrote: > > > How about a middle ground, though: we could add a devm function that > > does all the magic. Somewhat recently devm_pm_runtime_enable() was > > added. What if we add a variant for those that use autosuspend, like: > > > > devm_pm_runtime_enable_with_autosuspend(dev, initial_delay) > > > > That would: > > * pm_runtime_enable() > > * pm_runtime_set_autosuspend_delay() > > * pm_runtime_use_autosuspend() > > * Use devm_add_action_or_reset() to undo everything. > > > > Assuming that the pm_runtime folks are OK with that, we could > > transition things over to the new function once it rolls into > > mainline. > > > > So this doesn't magically fix all existing code but provides a path to > > make this more discoverable. > > Sounds like a good idea. I wonder if this could be handled by > devm_pm_runtime_enable() actually. If a driver calls > devm_pm_runtime_enable() and then enables auto-suspend, can't the > runtime PM core reasonably expect that auto-suspend should be disabled > at .remove() time ? The pm_runtime_disable_action() function could > disable auto-suspend unconditionally (assuming > pm_runtime_use_autosuspend() and pm_runtime_dont_use_autosuspend() don't > need to be balanced, if they do, then I'll just go cry in a corner). I like your idea. I think you're right that we can just leverage the existing function. This yak didn't look to hairy, so I posted a patch: https://lore.kernel.org/r/20220223083441.1.I925ce9fa12992a58caed6b297e0171d214866fe7@changeid I guess now we see what Rafael thinks. ;-) -Doug
Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format
On Wed, Feb 23, 2022 at 03:38:20PM +0100, Marek Vasut wrote: > On 2/23/22 15:37, Maxime Ripard wrote: > > On Wed, Feb 23, 2022 at 03:09:08PM +0100, Marek Vasut wrote: > > > On 2/23/22 14:47, Maxime Ripard wrote: > > > > On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote: > > > > > On 2/23/22 14:41, Maxime Ripard wrote: > > > > > > Hi, > > > > > > > > > > > > On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote: > > > > > > > Use the new property bus-format to set the enum bus_format and > > > > > > > bpc. > > > > > > > Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi > > > > > > > support") > > > > > > > > > > > > > > Signed-off-by: Max Krummenacher > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > drivers/gpu/drm/panel/panel-simple.c | 32 > > > > > > > > > > > > > > 1 file changed, 32 insertions(+) > > > > > > > > > > > > > > Relates to the discussion: > > > > > > > https://lore.kernel.org/all/20220201110717.3585-1-cniederma...@dh-electronics.com/ > > > > > > > > > > > > > > Max > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c > > > > > > > b/drivers/gpu/drm/panel/panel-simple.c > > > > > > > index c5f133667a2d..5c07260de71c 100644 > > > > > > > --- a/drivers/gpu/drm/panel/panel-simple.c > > > > > > > +++ b/drivers/gpu/drm/panel/panel-simple.c > > > > > > > @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev, > > > > > > > struct panel_desc *desc; > > > > > > > unsigned int bus_flags; > > > > > > > struct videomode vm; > > > > > > > + const char *format = ""; > > > > > > > int ret; > > > > > > > np = dev->of_node; > > > > > > > @@ -477,6 +478,37 @@ static int panel_dpi_probe(struct device > > > > > > > *dev, > > > > > > > of_property_read_u32(np, "width-mm", &desc->size.width); > > > > > > > of_property_read_u32(np, "height-mm", > > > > > > > &desc->size.height); > > > > > > > + of_property_read_string(np, "bus-format", &format); > > > > > > > + if (!strcmp(format, "BGR888_1X24")) { > > > > > > > + desc->bpc = 8; > > > > > > > + desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24; > > > > > > > + } else if (!strcmp(format, "GBR888_1X24")) { > > > > > > > + desc->bpc = 8; > > > > > > > + desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24; > > > > > > > + } else if (!strcmp(format, "RBG888_1X24")) { > > > > > > > + desc->bpc = 8; > > > > > > > + desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24; > > > > > > > + } else if (!strcmp(format, "RGB444_1X12")) { > > > > > > > + desc->bpc = 6; > > > > > > > + desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12; > > > > > > > + } else if (!strcmp(format, "RGB565_1X16")) { > > > > > > > + desc->bpc = 6; > > > > > > > + desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16; > > > > > > > + } else if (!strcmp(format, "RGB666_1X18")) { > > > > > > > + desc->bpc = 6; > > > > > > > + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18; > > > > > > > + } else if (!strcmp(format, "RGB666_1X24_CPADHI")) { > > > > > > > + desc->bpc = 6; > > > > > > > + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI; > > > > > > > + } else if (!strcmp(format, "RGB888_1X24")) { > > > > > > > + desc->bpc = 8; > > > > > > > + desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > > > > > > + } else { > > > > > > > + dev_err(dev, "%pOF: missing or unknown bus-format > > > > > > > property\n", > > > > > > > + np); > > > > > > > + return -EINVAL; > > > > > > > + } > > > > > > > + > > > > > > > > > > > > It doesn't seem right, really. We can't the bus format / bpc be > > > > > > inferred > > > > > > from the compatible? I'd expect two panels that don't have the same > > > > > > bus > > > > > > format to not be claimed as compatible. > > > > > > > > > > Which compatible ? > > > > > > > > > > Note that this is for panel-dpi compatible, i.e. the panel which has > > > > > timings > > > > > specified in DT (and needs bus format specified there too). > > > > > > > > panel-dpi is supposed to have two compatibles, the panel-specific one > > > > and panel-dpi. This would obviously be tied to the panel-specific one. > > > > > > This whole discussion is about the one which only has 'panel-dpi' > > > compatible > > > and describes the panel in DT completely. The specific compatible is not > > > present in DT when this patch is needed. > > > > From the panel-dpi DT binding: > > > > properties: > >compatible: > > description: > >Shall contain a panel specific compatible and "panel-dpi" > >in that order. > > items: > >- {} > >- const: panel-dpi > > > > The panel-specific compatible is mandatory, whether you like it or not. > > It doesn't seem to me that's the intended use per panel-simple.c , so maybe > the bindings need to be fi
Re: [PATCH v3] simplefb: Enable boot time VESA graphic mode selection.
On Wed, Feb 23, 2022 at 05:34:50PM +0100, Javier Martinez Canillas wrote: > Hello Michal, > > On 2/18/22 17:04, Michal Suchanek wrote: > > Since switch to simplefb/simpledrm VESA graphic modes are no longer > > available with legacy BIOS. > > Maybe you can mention that is the "vga=" kernel command line parameter > since that may be more evident to people reading the commit message ? Yes, I suppose that could be added. > > The x86 realmode boot code enables the VESA graphic modes when option > > FB_BOOT_VESA_SUPPORT is enabled. > > > > To enable use of VESA modes with simplefb in legacy BIOS boot mode drop > > I think you meant "VESA modes with the sysfb driver" ? or something like > that since otherwise it seems that you meant to use it with the simplefb > (drivers/video/fbdev/simplefb.c) fbdev driver, which doesn't support the > "vga=" param as far as I understand (it just uses whatever was setup). And the vga= is whatever was set up by the realmode code. And the config option for realmode code to do that is selected by vesafb and not simplefb so it does not wotk for simplefb/simpledrm/whatewer when efifib is not built into the kernel. > The name sysfb_simplefb is really horrible, because it is too confusing > and probably we should change it at some point... > > Patch itself looks good to me though. > > Reviewed-by: Javier Martinez Canillas Thanks Michal
Re: [PATCH v3] simplefb: Enable boot time VESA graphic mode selection.
On 2/23/22 17:45, Michal Suchánek wrote: [snip] >>> >>> To enable use of VESA modes with simplefb in legacy BIOS boot mode drop >> >> I think you meant "VESA modes with the sysfb driver" ? or something like >> that since otherwise it seems that you meant to use it with the simplefb >> (drivers/video/fbdev/simplefb.c) fbdev driver, which doesn't support the >> "vga=" param as far as I understand (it just uses whatever was setup). > > And the vga= is whatever was set up by the realmode code. And the config > option for realmode code to do that is selected by vesafb and not > simplefb so it does not wotk for simplefb/simpledrm/whatewer when efifib > is not built into the kernel. > Yes, that's what I tried to say. But your commit message says "To enable use of VESA modes with simplefb in legacy BIOS boot mode" and that isn't accurate AFAIU (unless you meant sysfb instead). Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format
On 2/23/22 17:39, Maxime Ripard wrote: On Wed, Feb 23, 2022 at 03:38:20PM +0100, Marek Vasut wrote: On 2/23/22 15:37, Maxime Ripard wrote: On Wed, Feb 23, 2022 at 03:09:08PM +0100, Marek Vasut wrote: On 2/23/22 14:47, Maxime Ripard wrote: On Wed, Feb 23, 2022 at 02:45:30PM +0100, Marek Vasut wrote: On 2/23/22 14:41, Maxime Ripard wrote: Hi, On Tue, Feb 22, 2022 at 09:47:23AM +0100, Max Krummenacher wrote: Use the new property bus-format to set the enum bus_format and bpc. Completes: commit 4a1d0dbc8332 ("drm/panel: simple: add panel-dpi support") Signed-off-by: Max Krummenacher --- drivers/gpu/drm/panel/panel-simple.c | 32 1 file changed, 32 insertions(+) Relates to the discussion: https://lore.kernel.org/all/20220201110717.3585-1-cniederma...@dh-electronics.com/ Max diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index c5f133667a2d..5c07260de71c 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -453,6 +453,7 @@ static int panel_dpi_probe(struct device *dev, struct panel_desc *desc; unsigned int bus_flags; struct videomode vm; + const char *format = ""; int ret; np = dev->of_node; @@ -477,6 +478,37 @@ static int panel_dpi_probe(struct device *dev, of_property_read_u32(np, "width-mm", &desc->size.width); of_property_read_u32(np, "height-mm", &desc->size.height); + of_property_read_string(np, "bus-format", &format); + if (!strcmp(format, "BGR888_1X24")) { + desc->bpc = 8; + desc->bus_format = MEDIA_BUS_FMT_BGR888_1X24; + } else if (!strcmp(format, "GBR888_1X24")) { + desc->bpc = 8; + desc->bus_format = MEDIA_BUS_FMT_GBR888_1X24; + } else if (!strcmp(format, "RBG888_1X24")) { + desc->bpc = 8; + desc->bus_format = MEDIA_BUS_FMT_RBG888_1X24; + } else if (!strcmp(format, "RGB444_1X12")) { + desc->bpc = 6; + desc->bus_format = MEDIA_BUS_FMT_RGB444_1X12; + } else if (!strcmp(format, "RGB565_1X16")) { + desc->bpc = 6; + desc->bus_format = MEDIA_BUS_FMT_RGB565_1X16; + } else if (!strcmp(format, "RGB666_1X18")) { + desc->bpc = 6; + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X18; + } else if (!strcmp(format, "RGB666_1X24_CPADHI")) { + desc->bpc = 6; + desc->bus_format = MEDIA_BUS_FMT_RGB666_1X24_CPADHI; + } else if (!strcmp(format, "RGB888_1X24")) { + desc->bpc = 8; + desc->bus_format = MEDIA_BUS_FMT_RGB888_1X24; + } else { + dev_err(dev, "%pOF: missing or unknown bus-format property\n", + np); + return -EINVAL; + } + It doesn't seem right, really. We can't the bus format / bpc be inferred from the compatible? I'd expect two panels that don't have the same bus format to not be claimed as compatible. Which compatible ? Note that this is for panel-dpi compatible, i.e. the panel which has timings specified in DT (and needs bus format specified there too). panel-dpi is supposed to have two compatibles, the panel-specific one and panel-dpi. This would obviously be tied to the panel-specific one. This whole discussion is about the one which only has 'panel-dpi' compatible and describes the panel in DT completely. The specific compatible is not present in DT when this patch is needed. From the panel-dpi DT binding: properties: compatible: description: Shall contain a panel specific compatible and "panel-dpi" in that order. items: - {} - const: panel-dpi The panel-specific compatible is mandatory, whether you like it or not. It doesn't seem to me that's the intended use per panel-simple.c , so maybe the bindings need to be fixed too ? It's not clear to me why this has anything to do with panel-simple, but the binding is correct, and that requirement is fairly standard. We have the same thing with panel-lvds for example. I think this patch is related to this patch, which was not mentioned in the commit message: [RFC][PATCH] Revert "drm/panel-simple: drop use of data-mapping property" (unless I am confused) With LVDS the situation is simpler, you have three formats and that's it (18bpp and 2 24bpp), with DPI it is more complex, since you need to deal with color channel width (888, 666 and even 565 and other oddities), then with mapping (RGB, BGR, etc), and then you can have panels with only 18bpp inputs wired to 24bpp DPI bus and vice versa which you also have to somehow describe.
Re: [PATCH] drm/bridge_connector: enable HPD by default if supported
Quoting Laurent Pinchart (2022-02-23 16:25:28) > Hello, > > On Wed, Feb 23, 2022 at 04:17:22PM +, Kieran Bingham wrote: > > Quoting Laurent Pinchart (2021-12-29 23:44:29) > > > On Sat, Dec 25, 2021 at 09:31:51AM +0300, Nikita Yushchenko wrote: > > > > Hotplug events reported by bridge drivers over drm_bridge_hpd_notify() > > > > get ignored unless somebody calls drm_bridge_hpd_enable(). When the > > > > connector for the bridge is bridge_connector, such a call is done from > > > > drm_bridge_connector_enable_hpd(). > > > > > > > > However drm_bridge_connector_enable_hpd() is never called on init paths, > > > > documentation suggests that it is intended for suspend/resume paths. > > > > > > H... I'm in two minds about this. The problem description is > > > correct, but I wonder if HPD should be enabled unconditionally here, or > > > if this should be left to display drivers to control. > > > drivers/gpu/drm/imx/dcss/dcss-kms.c enables HPD manually at init time, > > > other drivers don't. > > > > > > It feels like this should be under control of the display controller > > > driver, but I can't think of a use case for not enabling HPD at init > > > time. Any second opinion from anyone ? > > > > This patch solves an issue I have where I have recently enabled HPD on > > the SN65DSI86, but without this, I do not get calls to my .hpd_enable or > > .hpd_disable hooks that I have added to the ti_sn_bridge_funcs. > > > > So it needs to be enabled somewhere, and this seems reasonable to me? > > It's not directly related to the display controller - as it's a factor > > of the bridge? > > > > On Falcon-V3U with HPD additions to SN65DSI86: > > Tested-by: Kieran Bingham > > If you think this is right, then > > Reviewed-by: Laurent Pinchart I do, and at the very least it works for me, and fixes Nikita's issue so to me that's enough for: Reviewed-by: Kieran Bingham > > > > In result, once encoders are switched to bridge_connector, > > > > bridge-detected HPD stops working. > > > > > > > > This patch adds a call to that API on init path. > > > > > > > > This fixes HDMI HPD with rcar-du + adv7513 case when adv7513 reports HPD > > > > events via interrupts. > > > > > > > > Fixes: c24110a8fd09 ("drm: rcar-du: Use drm_bridge_connector_init() > > > > helper") > > > > Signed-off-by: Nikita Yushchenko > > > > --- > > > > drivers/gpu/drm/drm_bridge_connector.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c > > > > b/drivers/gpu/drm/drm_bridge_connector.c > > > > index 791379816837..4f20137ef21d 100644 > > > > --- a/drivers/gpu/drm/drm_bridge_connector.c > > > > +++ b/drivers/gpu/drm/drm_bridge_connector.c > > > > @@ -369,8 +369,10 @@ struct drm_connector > > > > *drm_bridge_connector_init(struct drm_device *drm, > > > > connector_type, ddc); > > > > drm_connector_helper_add(connector, > > > > &drm_bridge_connector_helper_funcs); > > > > > > > > - if (bridge_connector->bridge_hpd) > > > > + if (bridge_connector->bridge_hpd) { > > > > connector->polled = DRM_CONNECTOR_POLL_HPD; > > > > + drm_bridge_connector_enable_hpd(connector); > > > > + } > > > > else if (bridge_connector->bridge_detect) > > > > connector->polled = DRM_CONNECTOR_POLL_CONNECT > > > > | DRM_CONNECTOR_POLL_DISCONNECT; > > -- > Regards, > > Laurent Pinchart
Re: [PATCH v3] simplefb: Enable boot time VESA graphic mode selection.
On 2/23/22 17:54, Javier Martinez Canillas wrote: > On 2/23/22 17:45, Michal Suchánek wrote: > > [snip] > To enable use of VESA modes with simplefb in legacy BIOS boot mode drop >>> >>> I think you meant "VESA modes with the sysfb driver" ? or something like >>> that since otherwise it seems that you meant to use it with the simplefb >>> (drivers/video/fbdev/simplefb.c) fbdev driver, which doesn't support the >>> "vga=" param as far as I understand (it just uses whatever was setup). >> >> And the vga= is whatever was set up by the realmode code. And the config >> option for realmode code to do that is selected by vesafb and not >> simplefb so it does not wotk for simplefb/simpledrm/whatewer when efifib >> is not built into the kernel. >> > > Yes, that's what I tried to say. But your commit message says "To enable > use of VESA modes with simplefb in legacy BIOS boot mode" and that isn't > accurate AFAIU (unless you meant sysfb instead). In fact, probably the subject line should also be something like following: firmware: sysfb: Enable boot time VESA graphic mode selection Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v3] simplefb: Enable boot time VESA graphic mode selection.
On Wed, Feb 23, 2022 at 05:54:54PM +0100, Javier Martinez Canillas wrote: > On 2/23/22 17:45, Michal Suchánek wrote: > > [snip] > > >>> > >>> To enable use of VESA modes with simplefb in legacy BIOS boot mode drop > >> > >> I think you meant "VESA modes with the sysfb driver" ? or something like > >> that since otherwise it seems that you meant to use it with the simplefb > >> (drivers/video/fbdev/simplefb.c) fbdev driver, which doesn't support the > >> "vga=" param as far as I understand (it just uses whatever was setup). > > > > And the vga= is whatever was set up by the realmode code. And the config > > option for realmode code to do that is selected by vesafb and not > > simplefb so it does not wotk for simplefb/simpledrm/whatewer when efifib > > is not built into the kernel. > > > > Yes, that's what I tried to say. But your commit message says "To enable > use of VESA modes with simplefb in legacy BIOS boot mode" and that isn't > accurate AFAIU (unless you meant sysfb instead). config SYSFB_SIMPLEFB bool "Mark VGA/VBE/EFI FB as generic system framebuffer" depends on SYSFB + select BOOT_VESA_SUPPORT if X86 This to me means that it's simplefb specifically that requires it, not sysfb. More precisely SYSFB_SIMPLEFB which is the simplefb implementation on top of legacy BIOS. Thanks Michal
Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
On 2/18/2022 6:22 PM, Dmitry Baryshkov wrote: On Sat, 19 Feb 2022 at 03:55, Stephen Boyd wrote: Quoting Dmitry Baryshkov (2022-02-18 14:32:53) On 19/02/2022 00:31, Kuogee Hsieh wrote: On 2/11/2022 2:40 PM, Dmitry Baryshkov wrote: There is little point in having both connector and root bridge implementation in the same driver. Move connector's functionality to the bridge to let next bridge in chain to override it. Signed-off-by: Dmitry Baryshkov This patch break primary (edp) display -- right half of screen garbled -- screen shift vertically below are error messages seen -- [ 36.679216] panel-edp soc@0:edp_panel: No display modes [ 36.687272] panel-edp soc@0:edp_panel: No display modes [ 40.593709] panel-edp soc@0:edp_panel: No display modes [ 40.600285] panel-edp soc@0:edp_panel: No display modes So, before the patch the drm core was getting modes from the drm_connector (which means, modes from drm driver itself). With this patch the panel-edp tries to get modes. Could you please check, why panel_edp_get_modes() fails? Assuming that you use platform panel-edp binding (rather than 'edp-panel') could you please check you have either of the following: - ddc bus for EDID? I don't see anywhere where the ddc pointer is set for the dp bridge in msm_dp_bridge_init(). Is that required though? I'd think simple panel is still being used here so reading EDID isn't required. I meant the 'ddc-i2c-bus' property for the corresponding eDP panel. - either num_timing or num_modes in your panel desc. After reading the panel-edp's code I don't have another cause for panel_edp_get_modes(). It should either have a DDC bus specified using the mentioned device tree property, or it should have specified the timings. Kuogee, which platform were you using when testing this patch? Could you please share the dts fragment? I cherry-picked your patches on top of our internal release which is usually have some (or many) patches behind msm-next. where is "ddc-i2c-bus" located? msm_edp: edp@aea { compatible = "qcom,sc7280-edp"; reg = <0 0xaea 0 0x200>, <0 0xaea0200 0 0x200>, <0 0xaea0400 0 0xc00>, <0 0xaea1000 0 0x400>; interrupt-parent = <&mdss>; interrupts = <14>; clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_EDP_CLKREF_EN>, <&dispcc DISP_CC_MDSS_AHB_CLK>, <&dispcc DISP_CC_MDSS_EDP_AUX_CLK>, <&dispcc DISP_CC_MDSS_EDP_LINK_CLK>, <&dispcc DISP_CC_MDSS_EDP_LINK_INTF_CLK>, <&dispcc DISP_CC_MDSS_EDP_PIXEL_CLK>; clock-names = "core_xo", "core_ref", "core_iface", "core_aux", "ctrl_link", "ctrl_link_iface", "stream_pixel"; #clock-cells = <1>; assigned-clocks = <&dispcc DISP_CC_MDSS_EDP_LINK_CLK_SRC>, <&dispcc DISP_CC_MDSS_EDP_PIXEL_CLK_SRC>; assigned-clock-parents = <&edp_phy 0>, <&edp_phy 1>; phys = <&edp_phy>; phy-names = "dp"; operating-points-v2 = <&edp_opp_table>; power-domains = <&rpmhpd SC7280_CX>; #address-cells = <1>; #size-cells = <0>; status = "disabled"; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; edp_in: endpoint { remote-endpoint = <&dpu_intf5_out>; }; }; }; edp_opp_table: opp-table { compatible = "operating-points-v2"; opp-16000 { opp-hz = /bits/ 64 <16000>; required-opps = <&r
Re: [RFC PATCH 11/11] drm/bridge: ti-sn65dsi86: Support hotplug detection
Hi All, I'm working to respin the remainder of these patches, now that I have IRQ based HPD working on the SN65DSI86, and the (non-eDP) mode is used for Renesas R-Car boards. Quoting Doug Anderson (2021-06-24 00:51:12) > Hi, > > On Wed, Jun 23, 2021 at 4:26 PM Laurent Pinchart > wrote: > > > > > > @@ -1365,7 +1384,8 @@ static int ti_sn_bridge_probe(struct i2c_client > > > > *client, > > > > > > > > pdata->bridge.funcs = &ti_sn_bridge_funcs; > > > > pdata->bridge.of_node = client->dev.of_node; > > > > - pdata->bridge.ops = DRM_BRIDGE_OP_EDID; > > > > + pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT) > > > > > > Checking for "no_hpd" here is not the right test IIUC. You want to > > > check for eDP vs. DP (AKA whether a panel is downstream of you or a > > > connector). Specifically if downstream of you is a panel then (I > > > believe) HPD won't assert until you turn on the panel and you won't > > > turn on the panel (which happens in pre_enable, right?) until HPD > > > fires, so you've got a chicken-and-egg problem. If downstream of you > > > is a connector, though, then by definition HPD has to just work > > > without pre_enable running so then you're OK. > > > > Agreed. It's even more true now that your rework has landed, as in the > > eDP case EDID is handled by the panel driver. I'll rework this. > > > > Should I also condition setting HPD_DISABLE to the presence of a panel > > then ? I could drop of_property_read_bool() and set > > > > pdata->no_hpd = !!panel; > > > > > I guess then you'd need to figure out what to do if someone wants to > > > use "HPD" on eDP. Do you need to put a polling loop in pre_enable > > > then? Or you could just punt not support this case until someone needs > > > it. > > > > I think I'll stop short of saving the world this time, yes :-) We'll see > > what to do when this case arises. > > How about as a compromise you still call of_property_read_bool() but > add some type of warning in the logs if someone didn't set "no-hpd" > but they have a panel? Would a mix of the two work well? What about: pdata->no_hpd = of_property_read_bool(np, "no-hpd"); if (panel && !pdata->no_hpd) { DRM_ERROR("Panels will not function with HPD. Enforcing no-hpd\n"); pdata->no_hpd = true; } That leaves it still optional to DP connectors, but enforced on eDP? > > > > + | DRM_BRIDGE_OP_EDID; > > > > > > IMO somewhere in here if HPD is being used like this you should throw > > > in a call to pm_runtime_get_sync(). I guess in your solution the > > > regulators (for the bridge, not the panel) and enable pin are just > > > left on all the time, > > > > Correct, on my development board the SN65DSI86 is on all the time, I > > can't control that. > > > > > but plausibly someone might want to build a > > > system to use HPD and also have the enable pin and/or regulators > > > controlled by this driver, right? > > > > True. DRM doesn't make this very easy, as, as far as I can tell, there's > > no standard infrastructure for userspace to register an interest in HPD > > that could be notified to bridges. I think it should be fixable, but > > it's out of scope for this series :-) Should I still add a > > pm_runtime_get_sync() at probe time, or leave this to be addressed by > > someone who will need to implement power control ? > > IMO if you've detected you're running in DP mode you should add the > pm_runtime_get_sync() in probe to keep it powered all the time and > that seems the simplest. Technically I guess that's not really > required since you're polling and you could power off between polls, > but then you'd have to re-init a bunch of your state each time you > polled too. If you ever transitioned to using an IRQ for HPD then > you'd have to keep it always powered anyway. Hrm ... that's precisely what I've done. It's not IRQ based HPD... So would you like to see something like this during ti_sn_bridge_probe()? /* The device must remain powered up for HPD to be supported. */ if (!pdata->no_hpd) pm_runtime_get_sync(pdata->dev); -- Regards Kieran > > -Doug
Re: [PATCH v11 1/6] drm: Add arch arm64 for drm_clflush_virt_range
Hi Michael, Thank you for the patch! Yet something to improve: [auto build test ERROR on drm-tip/drm-tip] [also build test ERROR on drm/drm-next] [cannot apply to drm-intel/for-linux-next v5.17-rc5 next-20220222] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Michael-Cheng/Use-drm_clflush-instead-of-clflush/20220223-140110 base: git://anongit.freedesktop.org/drm/drm-tip drm-tip config: arm64-defconfig (https://download.01.org/0day-ci/archive/20220224/202202240139.lqifrvfs-...@intel.com/config) compiler: aarch64-linux-gcc (GCC) 11.2.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/f4c92ba1f52db578a26ac9944e2cbe52c548e8e9 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Michael-Cheng/Use-drm_clflush-instead-of-clflush/20220223-140110 git checkout f4c92ba1f52db578a26ac9944e2cbe52c548e8e9 # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm64 SHELL=/bin/bash If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>, old ones prefixed by <<): >> ERROR: modpost: "dcache_clean_inval_poc" [drivers/gpu/drm/drm.ko] undefined! --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
Re: [RFC PATCH v2 2/5] drm/msm/dp: support attaching bridges to the DP encoder
On 2/11/2022 2:40 PM, Dmitry Baryshkov wrote: Currently DP driver will allocate panel bridge for eDP panels. This supports only the following topology: - eDP encoder ⇒ eDP panel (wrapped using panel-bridge) Simplify this code to just check if there is any next bridge in the chain (be it a panel bridge or regular bridge). Rename panel_bridge field to next_bridge accordingly. This allows one to use e.g. one of the following display topologies: - eDP encoder ⇒ ptn3460 ⇒ fixed LVDS panel - eDP encoder ⇒ ptn3460 ⇒ LVDS connector with EDID lines for panel autodetect - eDP encoder ⇒ ptn3460 ⇒ THC63LVD1024 ⇒ DPI panel. - eDP encoder ⇒ LT8912 ⇒ DSI panel Signed-off-by: Dmitry Baryshkov --- Tested-by: Kuogee Hsieh drivers/gpu/drm/msm/dp/dp_display.c | 2 +- drivers/gpu/drm/msm/dp/dp_display.h | 2 +- drivers/gpu/drm/msm/dp/dp_drm.c | 4 ++-- drivers/gpu/drm/msm/dp/dp_parser.c | 31 +++-- drivers/gpu/drm/msm/dp/dp_parser.h | 2 +- 5 files changed, 21 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 44d42c76c2a3..45f9a912ecc5 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -266,7 +266,7 @@ static int dp_display_bind(struct device *dev, struct device *master, goto end; } - dp->dp_display.panel_bridge = dp->parser->panel_bridge; + dp->dp_display.next_bridge = dp->parser->next_bridge; dp->aux->drm_dev = drm; rc = dp_aux_register(dp->aux); diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h index e3adcd578a90..7af2b186d2d9 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.h +++ b/drivers/gpu/drm/msm/dp/dp_display.h @@ -16,7 +16,7 @@ struct msm_dp { struct drm_bridge *bridge; struct drm_connector *connector; struct drm_encoder *encoder; - struct drm_bridge *panel_bridge; + struct drm_bridge *next_bridge; bool is_connected; bool audio_enabled; bool power_on; diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index 26ef41a4c1b6..80f59cf99089 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -236,9 +236,9 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi return ERR_PTR(rc); } - if (dp_display->panel_bridge) { + if (dp_display->next_bridge) { rc = drm_bridge_attach(dp_display->encoder, - dp_display->panel_bridge, bridge, + dp_display->next_bridge, bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR); if (rc < 0) { DRM_ERROR("failed to attach panel bridge: %d\n", rc); diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c index a7acc23f742b..901d7967370f 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -265,23 +265,16 @@ static int dp_parser_clock(struct dp_parser *parser) return 0; } -static int dp_parser_find_panel(struct dp_parser *parser) +static int dp_parser_find_next_bridge(struct dp_parser *parser) { struct device *dev = &parser->pdev->dev; - struct drm_panel *panel; - int rc; + struct drm_bridge *bridge; - rc = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL); - if (rc) { - DRM_ERROR("failed to acquire DRM panel: %d\n", rc); - return rc; - } + bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); + if (IS_ERR(bridge)) + return PTR_ERR(bridge); - parser->panel_bridge = devm_drm_panel_bridge_add(dev, panel); - if (IS_ERR(parser->panel_bridge)) { - DRM_ERROR("failed to create panel bridge\n"); - return PTR_ERR(parser->panel_bridge); - } + parser->next_bridge = bridge; return 0; } @@ -307,10 +300,18 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) if (rc) return rc; + /* +* Currently we support external bridges only for eDP connectors. +* +* No external bridges are expected for the DisplayPort connector, +* it is physically present in a form of a DP or USB-C connector. +*/ if (connector_type == DRM_MODE_CONNECTOR_eDP) { - rc = dp_parser_find_panel(parser); - if (rc) + rc = dp_parser_find_next_bridge(parser); + if (rc) { + DRM_ERROR("DP: failed to find next bridge\n"); return rc; + } } /* Map the corresponding regulator information according to diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h inde
Re: [RFC PATCH 10/11] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
Hi Doug, Laurent, Quoting Laurent Pinchart (2021-06-23 14:59:00) > Hi Doug, > > On Wed, Mar 24, 2021 at 03:47:07PM -0700, Doug Anderson wrote: > > On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart wrote: > > > > > > Despite the SN65DSI86 being an eDP bridge, on some systems its output is > > > routed to a DisplayPort connector. Enable DisplayPort mode when the next > > > component in the display pipeline is not a panel, and disable eDP > > > features in that case. > > > > > > Signed-off-by: Laurent Pinchart > > > > > > --- > > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 32 --- > > > 1 file changed, 24 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > > index e2527d597ccb..f792227142a7 100644 > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > > @@ -55,6 +55,7 @@ > > > #define SN_LN_ASSIGN_REG 0x59 > > > #define LN_ASSIGN_WIDTH 2 > > > #define SN_ENH_FRAME_REG 0x5A > > > +#define ASSR_CONTROL BIT(0) > > > #define VSTREAM_ENABLEBIT(3) > > > #define LN_POLRS_OFFSET 4 > > > #define LN_POLRS_MASK 0xf0 > > > @@ -86,6 +87,8 @@ > > > #define SN_DATARATE_CONFIG_REG 0x94 > > > #define DP_DATARATE_MASK GENMASK(7, 5) > > > #define DP_DATARATE(x)((x) << 5) > > > +#define SN_TRAINING_SETTING_REG0x95 > > > +#define SCRAMBLE_DISABLE BIT(4) > > > #define SN_ML_TX_MODE_REG 0x96 > > > #define ML_TX_MAIN_LINK_OFF 0 > > > #define ML_TX_NORMAL_MODE BIT(0) > > > @@ -723,6 +726,11 @@ static int ti_sn_link_training(struct ti_sn_bridge > > > *pdata, int dp_rate_idx, > > > regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG, > > >DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx)); > > > > > > + /* For DisplayPort, use the standard DP scrambler seed. */ > > > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > > > + regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, > > > + ASSR_CONTROL, 0); > > > > I don't actually know anything about DP scrambler seeds. However: > > > > 1. From reading the docs, this field seems to be documented to be > > "read only" unless: > > > > 1a) The "TEST2" pin is pulled high when you power on the bridge. > > 1b) You set "ASSR_OVERRIDE" (page select to page 7, write to register > > 0x16, page select back to page 0). > > > > I don't know if TEST2 is being pulled high in your hardware, but at > > least I can see that 1b) isn't done. So I'm guessing that this line is > > a no-op? If I had to guess from all the hoops they're making you jump > > through there's some sort of errata around standard scrambling on this > > bridge chip. Are you sure it works OK? > > Good question :-) We managed to get the SN65DSI86 to work with an > external DP monitor yesterday, so it's possible (some modes don't > operate correctly yet, but I assume that to be an issue with the DSI > encoder). > > The TEST2 pin is strapped to ground on the board. > > According to the DisplayPort specification, eDP and DP use different > scrambler seeds to prevent interoperability between an eDP source and a > DP sink. I'll check what happens without this change. Without this change, the display still works... > > > 2. The docs I see claim that this field is 2 bits big. It seems like > > it would be nice to honor. Yeah, it's silly because 0x11 and 0x10 are > > "reserved" so it's really more like a 1-bit field, but still seems > > like it would be better to set both bits, or at least add a comment > > explaining why you're not matching the datasheet. > > Sure. > > > 3. Your patch doesn't seem to touch the bit of code in > > ti_sn_bridge_enable() that says this: > > > > /** > > * The SN65DSI86 only supports ASSR Display Authentication method and > > * this method is enabled by default. An eDP panel must support this > > * authentication method. We need to enable this method in the eDP panel > > * at DisplayPort address 0x0010A prior to link training. > > */ > > drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, > >DP_ALTERNATE_SCRAMBLER_RESET_ENABLE); > > > > Won't that be a problem? > > I'll have a look. I'm not sure I yet fully understand the requirements here, but could it be that only supporting ASSR is why the scrambling is disabled below? Commenting out that write does not affect the bring up of my DP monitor. > > > > + > > > /* enable DP PLL */ > > > regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1); > > > > > > @@ -734,6 +742,11 @@ static int ti_sn_link_training(s
Re: [PATCH v3] simplefb: Enable boot time VESA graphic mode selection.
On 2/23/22 18:12, Michal Suchánek wrote: > On Wed, Feb 23, 2022 at 05:54:54PM +0100, Javier Martinez Canillas wrote: [snip] >> >> Yes, that's what I tried to say. But your commit message says "To enable >> use of VESA modes with simplefb in legacy BIOS boot mode" and that isn't >> accurate AFAIU (unless you meant sysfb instead). > > config SYSFB_SIMPLEFB > bool "Mark VGA/VBE/EFI FB as generic system framebuffer" > depends on SYSFB > + select BOOT_VESA_SUPPORT if X86 > > This to me means that it's simplefb specifically that requires it, not sysfb. > More precisely SYSFB_SIMPLEFB which is the simplefb implementation on top of > legacy BIOS. > Ok, I see what you meant. The fact that simplefb is what's named to the part of the sysfb driver that register the "simple-framebuffer" platform device and also the name of the fbdev driver that matches the "simple-framebuffer" is too confusing. My point about the subject line remains thought, I would use something like: firmware: sysfb: Enable boot time VESA graphic mode selection for simplefb But I'll stop bike-shedding this. I don't mind if you keep the current line and feel free to keep my r-b tag. Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [RFC PATCH 10/11] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
Hi, On Wed, Feb 23, 2022 at 10:05 AM Kieran Bingham wrote: > > > > > + /* For DisplayPort, disable scrambling mode. */ > > > > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > > > > + regmap_update_bits(pdata->regmap, > > > > SN_TRAINING_SETTING_REG, > > > > + SCRAMBLE_DISABLE, SCRAMBLE_DISABLE); > > > > > > I'm assuming that this is the important part of your patch? Would be > > > sorta nice to include the "why" in your comment. Why do you want to > > > disable scrambling mode for DP but not for eDP? Maybe you care about > > > compatibility but not EMI if you're hooking up to random DP things? > > > > I'll investigate and include proper documentation in v2 (or drop the > > change altogether if it's not required). > > And indeed, this part is important. If I drop this hunk - then I get no > display output. > > I'm afraid I don't (yet) know the reasons 'why' to extend the comment, > beyond "Scrambling is not supported for DP". > > If anyone already does, please feel free to provide the text, and I'll > include it in the next revision, or I'll try to do some more digging > into this part. I don't know _tons_ about it, but I later learned that the "alternate" scrambler is used for eDP and the normal scrambler is used for DP. I don't have any background about why they are different other than what looks to be intentionally making the two things incompatible. ...so I guess that would make it pretty clear why you can't use the alternate scrambler for DP. I haven't personally done the research to know if you can be officially DP compliant with the scrambler disabled. I also don't know why the ti-sn65dsi86 makes it so difficult to switch to the standard scrambler or if it works at all... ;-) -Doug
Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
On 23/02/2022 20:21, Kuogee Hsieh wrote: On 2/18/2022 6:22 PM, Dmitry Baryshkov wrote: On Sat, 19 Feb 2022 at 03:55, Stephen Boyd wrote: Quoting Dmitry Baryshkov (2022-02-18 14:32:53) On 19/02/2022 00:31, Kuogee Hsieh wrote: On 2/11/2022 2:40 PM, Dmitry Baryshkov wrote: There is little point in having both connector and root bridge implementation in the same driver. Move connector's functionality to the bridge to let next bridge in chain to override it. Signed-off-by: Dmitry Baryshkov This patch break primary (edp) display -- right half of screen garbled -- screen shift vertically below are error messages seen -- [ 36.679216] panel-edp soc@0:edp_panel: No display modes [ 36.687272] panel-edp soc@0:edp_panel: No display modes [ 40.593709] panel-edp soc@0:edp_panel: No display modes [ 40.600285] panel-edp soc@0:edp_panel: No display modes So, before the patch the drm core was getting modes from the drm_connector (which means, modes from drm driver itself). With this patch the panel-edp tries to get modes. Could you please check, why panel_edp_get_modes() fails? Assuming that you use platform panel-edp binding (rather than 'edp-panel') could you please check you have either of the following: - ddc bus for EDID? I don't see anywhere where the ddc pointer is set for the dp bridge in msm_dp_bridge_init(). Is that required though? I'd think simple panel is still being used here so reading EDID isn't required. I meant the 'ddc-i2c-bus' property for the corresponding eDP panel. - either num_timing or num_modes in your panel desc. After reading the panel-edp's code I don't have another cause for panel_edp_get_modes(). It should either have a DDC bus specified using the mentioned device tree property, or it should have specified the timings. Kuogee, which platform were you using when testing this patch? Could you please share the dts fragment? I cherry-picked your patches on top of our internal release which is usually have some (or many) patches behind msm-next. where is "ddc-i2c-bus" located? In the panel device node. Can you please share it too? msm_edp: edp@aea { compatible = "qcom,sc7280-edp"; reg = <0 0xaea 0 0x200>, <0 0xaea0200 0 0x200>, <0 0xaea0400 0 0xc00>, <0 0xaea1000 0 0x400>; interrupt-parent = <&mdss>; interrupts = <14>; clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_EDP_CLKREF_EN>, <&dispcc DISP_CC_MDSS_AHB_CLK>, <&dispcc DISP_CC_MDSS_EDP_AUX_CLK>, <&dispcc DISP_CC_MDSS_EDP_LINK_CLK>, <&dispcc DISP_CC_MDSS_EDP_LINK_INTF_CLK>, <&dispcc DISP_CC_MDSS_EDP_PIXEL_CLK>; clock-names = "core_xo", "core_ref", "core_iface", "core_aux", "ctrl_link", "ctrl_link_iface", "stream_pixel"; #clock-cells = <1>; assigned-clocks = <&dispcc DISP_CC_MDSS_EDP_LINK_CLK_SRC>, <&dispcc DISP_CC_MDSS_EDP_PIXEL_CLK_SRC>; assigned-clock-parents = <&edp_phy 0>, <&edp_phy 1>; phys = <&edp_phy>; phy-names = "dp"; operating-points-v2 = <&edp_opp_table>; power-domains = <&rpmhpd SC7280_CX>; #address-cells = <1>; #size-cells = <0>; status = "disabled"; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; edp_in: endpoint { remote-endpoint = <&dpu_intf5_out>; }; }; }; edp_opp_table: opp-table { compatible = "operating-points-v2"; opp-16000 {
Re: [PATCH v3] simplefb: Enable boot time VESA graphic mode selection.
On Wed, Feb 23, 2022 at 07:13:07PM +0100, Javier Martinez Canillas wrote: > On 2/23/22 18:12, Michal Suchánek wrote: > > On Wed, Feb 23, 2022 at 05:54:54PM +0100, Javier Martinez Canillas wrote: > > [snip] > > >> > >> Yes, that's what I tried to say. But your commit message says "To enable > >> use of VESA modes with simplefb in legacy BIOS boot mode" and that isn't > >> accurate AFAIU (unless you meant sysfb instead). > > > > config SYSFB_SIMPLEFB > > bool "Mark VGA/VBE/EFI FB as generic system framebuffer" > > depends on SYSFB > > + select BOOT_VESA_SUPPORT if X86 > > > > This to me means that it's simplefb specifically that requires it, not > > sysfb. > > More precisely SYSFB_SIMPLEFB which is the simplefb implementation on top of > > legacy BIOS. > > > > Ok, I see what you meant. The fact that simplefb is what's named to the part > of the sysfb driver that register the "simple-framebuffer" platform device > and also the name of the fbdev driver that matches the "simple-framebuffer" > is too confusing. > > My point about the subject line remains thought, I would use something like: > > firmware: sysfb: Enable boot time VESA graphic mode selection for simplefb I see where the confusion comes from. The efifb (and probably vesafb) has implicit unstated dependency on sysfb. So the drivers that select BOOT_VESA_SUPPORT should instead depend on SYSFB, and then SYSFB can select BOOT_VESA_SUPPORT, and it will look much saner. Thanks Michal
Re: [RFC PATCH 11/11] drm/bridge: ti-sn65dsi86: Support hotplug detection
Hi, On Wed, Feb 23, 2022 at 9:43 AM Kieran Bingham wrote: > > Hi All, > > I'm working to respin the remainder of these patches, now that I have > IRQ based HPD working on the SN65DSI86, and the (non-eDP) mode is used > for Renesas R-Car boards. > > Quoting Doug Anderson (2021-06-24 00:51:12) > > Hi, > > > > On Wed, Jun 23, 2021 at 4:26 PM Laurent Pinchart > > wrote: > > > > > > > > @@ -1365,7 +1384,8 @@ static int ti_sn_bridge_probe(struct i2c_client > > > > > *client, > > > > > > > > > > pdata->bridge.funcs = &ti_sn_bridge_funcs; > > > > > pdata->bridge.of_node = client->dev.of_node; > > > > > - pdata->bridge.ops = DRM_BRIDGE_OP_EDID; > > > > > + pdata->bridge.ops = (pdata->no_hpd ? 0 : DRM_BRIDGE_OP_DETECT) > > > > > > > > Checking for "no_hpd" here is not the right test IIUC. You want to > > > > check for eDP vs. DP (AKA whether a panel is downstream of you or a > > > > connector). Specifically if downstream of you is a panel then (I > > > > believe) HPD won't assert until you turn on the panel and you won't > > > > turn on the panel (which happens in pre_enable, right?) until HPD > > > > fires, so you've got a chicken-and-egg problem. If downstream of you > > > > is a connector, though, then by definition HPD has to just work > > > > without pre_enable running so then you're OK. > > > > > > Agreed. It's even more true now that your rework has landed, as in the > > > eDP case EDID is handled by the panel driver. I'll rework this. > > > > > > Should I also condition setting HPD_DISABLE to the presence of a panel > > > then ? I could drop of_property_read_bool() and set > > > > > > pdata->no_hpd = !!panel; > > > > > > > I guess then you'd need to figure out what to do if someone wants to > > > > use "HPD" on eDP. Do you need to put a polling loop in pre_enable > > > > then? Or you could just punt not support this case until someone needs > > > > it. > > > > > > I think I'll stop short of saving the world this time, yes :-) We'll see > > > what to do when this case arises. > > > > How about as a compromise you still call of_property_read_bool() but > > add some type of warning in the logs if someone didn't set "no-hpd" > > but they have a panel? > > > Would a mix of the two work well? > > What about: > > pdata->no_hpd = of_property_read_bool(np, "no-hpd"); > if (panel && !pdata->no_hpd) { > DRM_ERROR("Panels will not function with HPD. Enforcing > no-hpd\n"); > pdata->no_hpd = true; > } > > That leaves it still optional to DP connectors, but enforced on eDP? Yeah, that's fine with me. Nits would be to use "warn" instead of "error" since this isn't fatal and use the non-SHOUTING versions of the prints since the SHOUTING versions are deprecated. > > > > > + | DRM_BRIDGE_OP_EDID; > > > > > > > > IMO somewhere in here if HPD is being used like this you should throw > > > > in a call to pm_runtime_get_sync(). I guess in your solution the > > > > regulators (for the bridge, not the panel) and enable pin are just > > > > left on all the time, > > > > > > Correct, on my development board the SN65DSI86 is on all the time, I > > > can't control that. > > > > > > > but plausibly someone might want to build a > > > > system to use HPD and also have the enable pin and/or regulators > > > > controlled by this driver, right? > > > > > > True. DRM doesn't make this very easy, as, as far as I can tell, there's > > > no standard infrastructure for userspace to register an interest in HPD > > > that could be notified to bridges. I think it should be fixable, but > > > it's out of scope for this series :-) Should I still add a > > > pm_runtime_get_sync() at probe time, or leave this to be addressed by > > > someone who will need to implement power control ? > > > > IMO if you've detected you're running in DP mode you should add the > > pm_runtime_get_sync() in probe to keep it powered all the time and > > that seems the simplest. Technically I guess that's not really > > required since you're polling and you could power off between polls, > > but then you'd have to re-init a bunch of your state each time you > > polled too. If you ever transitioned to using an IRQ for HPD then > > you'd have to keep it always powered anyway. > > > Hrm ... that's precisely what I've done. It's not IRQ based HPD... > > So would you like to see something like this during > ti_sn_bridge_probe()? > > /* The device must remain powered up for HPD to be supported. */ > if (!pdata->no_hpd) > pm_runtime_get_sync(pdata->dev); Yeah, seems reasonable. Probably you'd want to add a devm action to put it too? -Doug
Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
On 2/23/2022 10:22 AM, Dmitry Baryshkov wrote: On 23/02/2022 20:21, Kuogee Hsieh wrote: On 2/18/2022 6:22 PM, Dmitry Baryshkov wrote: On Sat, 19 Feb 2022 at 03:55, Stephen Boyd wrote: Quoting Dmitry Baryshkov (2022-02-18 14:32:53) On 19/02/2022 00:31, Kuogee Hsieh wrote: On 2/11/2022 2:40 PM, Dmitry Baryshkov wrote: There is little point in having both connector and root bridge implementation in the same driver. Move connector's functionality to the bridge to let next bridge in chain to override it. Signed-off-by: Dmitry Baryshkov This patch break primary (edp) display -- right half of screen garbled -- screen shift vertically below are error messages seen -- [ 36.679216] panel-edp soc@0:edp_panel: No display modes [ 36.687272] panel-edp soc@0:edp_panel: No display modes [ 40.593709] panel-edp soc@0:edp_panel: No display modes [ 40.600285] panel-edp soc@0:edp_panel: No display modes So, before the patch the drm core was getting modes from the drm_connector (which means, modes from drm driver itself). With this patch the panel-edp tries to get modes. Could you please check, why panel_edp_get_modes() fails? Assuming that you use platform panel-edp binding (rather than 'edp-panel') could you please check you have either of the following: - ddc bus for EDID? I don't see anywhere where the ddc pointer is set for the dp bridge in msm_dp_bridge_init(). Is that required though? I'd think simple panel is still being used here so reading EDID isn't required. I meant the 'ddc-i2c-bus' property for the corresponding eDP panel. - either num_timing or num_modes in your panel desc. After reading the panel-edp's code I don't have another cause for panel_edp_get_modes(). It should either have a DDC bus specified using the mentioned device tree property, or it should have specified the timings. Kuogee, which platform were you using when testing this patch? Could you please share the dts fragment? I cherry-picked your patches on top of our internal release which is usually have some (or many) patches behind msm-next. where is "ddc-i2c-bus" located? In the panel device node. Can you please share it too? &soc { edp_power_supply: edp_power { compatible = "regulator-fixed"; regulator-name = "edp_backlight_power"; regulator-always-on; regulator-boot-on; }; edp_backlight: edp_backlight { compatible = "pwm-backlight"; pwms = <&pm8350c_pwm 3 65535>; power-supply = <&edp_power_supply>; enable-gpio = <&pm8350c_gpios 7 GPIO_ACTIVE_HIGH>; pinctrl-names = "default"; pinctrl-0 = <&backlight_pwm_default>; }; edp_panel: edp_panel { compatible = "sharp_lq140m1jw46"; pinctrl-names = "default"; pinctrl-0 = <&edp_hot_plug_det>, <&edp_panel_power_default>; power-supply = <&edp_power_supply>; backlight = <&edp_backlight>; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; edp_panel_in: endpoint { remote-endpoint = <&edp_out>; }; }; }; }; }; msm_edp: edp@aea { compatible = "qcom,sc7280-edp"; reg = <0 0xaea 0 0x200>, <0 0xaea0200 0 0x200>, <0 0xaea0400 0 0xc00>, <0 0xaea1000 0 0x400>; interrupt-parent = <&mdss>; interrupts = <14>; clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_EDP_CLKREF_EN>, <&dispcc DISP_CC_MDSS_AHB_CLK>, <&dispcc DISP_CC_MDSS_EDP_AUX_CLK>, <&dispcc DISP_CC_MDSS_EDP_LINK_CLK>, <&dispcc DISP_CC_MDSS_EDP_LINK_INTF_CLK>, <&dispcc DISP_CC_MDSS_EDP_PIXEL_CLK>; clock-names = "core_xo", "core_ref", "core_iface", "core_aux", "ctrl_link", "ctrl_link_iface", "stream_pixel"; #clock-cells = <1>; assigned-clocks = <&dispcc DIS
Re: [PATCH v3] simplefb: Enable boot time VESA graphic mode selection.
On 2/23/22 19:23, Michal Suchánek wrote: [snip] >> My point about the subject line remains thought, I would use something like: >> >> firmware: sysfb: Enable boot time VESA graphic mode selection for simplefb > > I see where the confusion comes from. > Yeah. And just to clarify, the "simplefb" in the subject line I proposed was about the sysfb simplefb and not the fbdev simplefb :) > The efifb (and probably vesafb) has implicit unstated dependency on > sysfb. So the drivers that select BOOT_VESA_SUPPORT should instead > depend on SYSFB, and then SYSFB can select BOOT_VESA_SUPPORT, and it > will look much saner. > That indeed would be much nicer. And I agree with you that there's an implicit dependency that should be made explicit since SYSFB is what registers the "efi-framebuffer" or "vesa-framebuffer" if SYSFB_SIMPLEFB is not enabled. Should SYSFB should only select BOOT_VESA_SUPPORT if x86 ? I know that in practice shouldn't matter because BOOT_VESA_SUPPORT is under x86 but I guess is more correct if that's the case. And I think that FB_SIMPLE should depend on SYSFB_SIMPLEFB if !OF (since a "simple-framebuffer" platform device could be registered by OF if a Device Tree node with compatible "simple-framebuffer" exists). Best regards, -- Javier Martinez Canillas Linux Engineering Red Hat
Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
On Wed, 23 Feb 2022 at 21:27, Kuogee Hsieh wrote: > > > On 2/23/2022 10:22 AM, Dmitry Baryshkov wrote: > > On 23/02/2022 20:21, Kuogee Hsieh wrote: > >> > >> On 2/18/2022 6:22 PM, Dmitry Baryshkov wrote: > >>> On Sat, 19 Feb 2022 at 03:55, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2022-02-18 14:32:53) > > On 19/02/2022 00:31, Kuogee Hsieh wrote: > >> On 2/11/2022 2:40 PM, Dmitry Baryshkov wrote: > >>> There is little point in having both connector and root bridge > >>> implementation in the same driver. Move connector's > >>> functionality to the > >>> bridge to let next bridge in chain to override it. > >>> > >>> Signed-off-by: Dmitry Baryshkov > >> This patch break primary (edp) display > >> > >> -- right half of screen garbled > >> > >> -- screen shift vertically > >> > >> below are error messages seen -- > >> > >> [ 36.679216] panel-edp soc@0:edp_panel: No display modes > >> [ 36.687272] panel-edp soc@0:edp_panel: No display modes > >> [ 40.593709] panel-edp soc@0:edp_panel: No display modes > >> [ 40.600285] panel-edp soc@0:edp_panel: No display modes > > So, before the patch the drm core was getting modes from the > > drm_connector (which means, modes from drm driver itself). With this > > patch the panel-edp tries to get modes. > > > > Could you please check, why panel_edp_get_modes() fails? Assuming > > that > > you use platform panel-edp binding (rather than 'edp-panel') could > > you > > please check you have either of the following: > > - ddc bus for EDID? > I don't see anywhere where the ddc pointer is set for the dp bridge in > msm_dp_bridge_init(). Is that required though? I'd think simple > panel is > still being used here so reading EDID isn't required. > >>> I meant the 'ddc-i2c-bus' property for the corresponding eDP panel. > >>> > > - either num_timing or num_modes in your panel desc. > >>> After reading the panel-edp's code I don't have another cause for > >>> panel_edp_get_modes(). It should either have a DDC bus specified using > >>> the mentioned device tree property, or it should have specified the > >>> timings. > >>> > >>> Kuogee, which platform were you using when testing this patch? Could > >>> you please share the dts fragment? > >> > >> I cherry-picked your patches on top of our internal release which is > >> usually have some (or many) patches behind msm-next. > >> > >> where is "ddc-i2c-bus" located? > > > > In the panel device node. > > > > Can you please share it too? > > > &soc { > edp_power_supply: edp_power { > compatible = "regulator-fixed"; > regulator-name = "edp_backlight_power"; > > regulator-always-on; > regulator-boot-on; > }; > > edp_backlight: edp_backlight { > compatible = "pwm-backlight"; > > pwms = <&pm8350c_pwm 3 65535>; > power-supply = <&edp_power_supply>; > enable-gpio = <&pm8350c_gpios 7 GPIO_ACTIVE_HIGH>; > > pinctrl-names = "default"; > pinctrl-0 = <&backlight_pwm_default>; > }; > > edp_panel: edp_panel { > compatible = "sharp_lq140m1jw46"; I'd assume that the panel is supported by the patch https://patchwork.kernel.org/project/linux-arm-msm/patch/1644494255-6632-5-git-send-email-quic_sbill...@quicinc.com/ and the compatible value is just an old value. Provided that the panel description defines modes, I'd ask for some debug from the panel_edp_get_modes(). At least let's see why panel_edp_get_non_edid_modes() / panel_edp_get_display_modes() returns no modes. Regarding the ddc bus, if you have separate i2c bus connected to this panel, the ddc-i2c-bus = <&i2c_N>; property should go to this device node. > pinctrl-names = "default"; > pinctrl-0 = <&edp_hot_plug_det>, > <&edp_panel_power_default>; > > power-supply = <&edp_power_supply>; > backlight = <&edp_backlight>; > > ports { > #address-cells = <1>; > #size-cells = <0>; > port@0 { > reg = <0>; > edp_panel_in: endpoint { > remote-endpoint = <&edp_out>; > }; > }; > }; > }; > }; > > > > > >> > >> msm_edp: edp@aea { > >> compatible = "qcom,sc7280-edp"; > >> > >> reg = <0 0xaea 0 0x200>, > >><0 0xaea0200 0 0x200>, > >><0 0xaea0400 0 0xc00>, > >><0 0xaea10
Re: [Intel-gfx] [PATCH 1/3] drm/i915/guc: Limit scheduling properties to avoid overflow
On 2/23/2022 04:13, Tvrtko Ursulin wrote: On 23/02/2022 02:11, John Harrison wrote: On 2/22/2022 01:52, Tvrtko Ursulin wrote: On 18/02/2022 21:33, john.c.harri...@intel.com wrote: From: John Harrison GuC converts the pre-emption timeout and timeslice quantum values into clock ticks internally. That significantly reduces the point of 32bit overflow. On current platforms, worst case scenario is approximately Where does 32-bit come from, the GuC side? We already use 64-bits so that something to fix to start with. Yep... Yes, the GuC API is defined as 32bits only and then does a straight multiply by the clock speed with no range checking. We have requested 64bit support but there was push back on the grounds that it is not something the GuC timer hardware supports and such long timeouts are not real world usable anyway. As long as compute are happy with 100 seconds, then it "should be enough for everbody". :D Compute disable all forms of reset and rely on manual kill. So yes. But even if they aren't. That's all we can do at the moment. If there is a genuine customer requirement for more then we can push for full 64bit software implemented timers in the GuC but until that happens, we don't have much choice. ./gt/uc/intel_guc_fwif.h: u32 execution_quantum; ./gt/uc/intel_guc_submission.c: desc->execution_quantum = engine->props.timeslice_duration_ms * 1000; ./gt/intel_engine_types.h: unsigned long timeslice_duration_ms; timeslice_store/preempt_timeout_store: err = kstrtoull(buf, 0, &duration); So both kconfig and sysfs can already overflow GuC, not only because of tick conversion internally but because at backend level nothing was done for assigning 64-bit into 32-bit. Or I failed to find where it is handled. That's why I'm adding this range check to make sure we don't allow overflows. Yes and no, this fixes it, but the first bug was not only due GuC internal tick conversion. It was present ever since the u64 from i915 was shoved into u32 sent to GuC. So even if GuC used the value without additional multiplication, bug was be there. My point being when GuC backend was added timeout_ms values should have been limited/clamped to U32_MAX. The tick discovery is additional limit on top. I'm not disagreeing. I'm just saying that the truncation wasn't noticed until I actually tried using very long timeouts to debug a particular problem. Now that it is noticed, we need some method of range checking and this simple clamp solves all the truncation problems. 110 seconds. Rather than allowing the user to set higher values and then get confused by early timeouts, add limits when setting these values. Btw who is reviewing GuC patches these days - things have somehow gotten pretty quiet in activity and I don't think that's due absence of stuff to improve or fix? Asking since I think I noticed a few already which you posted and then crickets on the mailing list. Too much work to do and not enough engineers to do it all :(. Signed-off-by: John Harrison --- drivers/gpu/drm/i915/gt/intel_engine_cs.c | 15 +++ drivers/gpu/drm/i915/gt/sysfs_engines.c | 14 ++ drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 9 + 3 files changed, 38 insertions(+) diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index e53008b4dd05..2a1e9f36e6f5 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -389,6 +389,21 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id, if (GRAPHICS_VER(i915) == 12 && engine->class == RENDER_CLASS) engine->props.preempt_timeout_ms = 0; + /* Cap timeouts to prevent overflow inside GuC */ + if (intel_guc_submission_is_wanted(>->uc.guc)) { + if (engine->props.timeslice_duration_ms > GUC_POLICY_MAX_EXEC_QUANTUM_MS) { Hm "wanted".. There's been too much back and forth on the GuC load options over the years to keep track.. intel_engine_uses_guc work sounds like would work and read nicer. I'm not adding a new feature check here. I'm just using the existing one. If we want to rename it yet again then that would be a different patch set. $ grep intel_engine_uses_guc . -rl ./i915_perf.c ./i915_request.c ./selftests/intel_scheduler_helpers.c ./gem/i915_gem_context.c ./gt/intel_context.c ./gt/intel_engine.h ./gt/intel_engine_cs.c ./gt/intel_engine_heartbeat.c ./gt/intel_engine_pm.c ./gt/intel_reset.c ./gt/intel_lrc.c ./gt/selftest_context.c ./gt/selftest_engine_pm.c ./gt/selftest_hangcheck.c ./gt/selftest_mocs.c ./gt/selftest_workarounds.c Sounds better to me than intel_guc_submission_is_wanted. What does the reader know whether "is wanted" translates to "is actually used". Shrug on "is wanted". Yes, but isn't '_uses' the one that hits a BUG_ON if you call it too early in the boot up sequence? I never understood why that was necessary o
Re: [PATCH v2] drm/panel: Select DRM_DP_HELPER for DRM_PANEL_EDP
Hi Am 23.02.22 um 17:11 schrieb Doug Anderson: Hi, On Tue, Feb 22, 2022 at 1:31 AM Geert Uytterhoeven wrote: On Tue, Feb 8, 2022 at 10:39 AM Geert Uytterhoeven wrote: On Mon, Feb 7, 2022 at 12:31 PM Thomas Zimmermann wrote: As reported in [1], DRM_PANEL_EDP depends on DRM_DP_HELPER. Select the option to fix the build failure. The error message is shown below. arm-linux-gnueabihf-ld: drivers/gpu/drm/panel/panel-edp.o: in function `panel_edp_probe': panel-edp.c:(.text+0xb74): undefined reference to `drm_panel_dp_aux_backlight' make[1]: *** [/builds/linux/Makefile:1222: vmlinux] Error 1 The issue has been reported before, when DisplayPort helpers were hidden behind the option CONFIG_DRM_KMS_HELPER. [2] v2: * fix and expand commit description (Arnd) Signed-off-by: Thomas Zimmermann Thanks for your patch! This fixes the build errors I'm seeing, so Tested-by: Geert Uytterhoeven Is this planned to be queued? This is still failing in drm-next. Thanks! Looks like this has been in drm-misc-next since Feb 4: --- commit eea89dff4c39a106f98d1cb5e4d626f8c63908b9 Author: Thomas Zimmermann AuthorDate: Thu Feb 3 10:39:22 2022 +0100 Commit: Thomas Zimmermann CommitDate: Fri Feb 4 09:38:47 2022 +0100 drm/panel: Select DRM_DP_HELPER for DRM_PANEL_EDP --- Maybe it needed to land elsewhere, though? Sorry about the mess. We had some confusion with this cycle's drm-misc-next pull request, which got delayed significantly. There's been a PR today, which should be merged into drm-next any time now. The patch will be part of that. Best regards Thomas -Doug -- 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 OpenPGP_signature Description: OpenPGP digital signature
[PATCH] drm/msm: Avoid dirtyfb stalls on video mode displays (v2)
From: Rob Clark Someone on IRC once asked an innocent enough sounding question: Why with xf86-video-modesetting is es2gears limited at 120fps. So I broke out the perfetto tracing mesa MR and took a look. It turns out the problem was drm_atomic_helper_dirtyfb(), which would end up waiting for vblank.. es2gears would rapidly push two frames to Xorg, which would blit them to screen and in idle hook (I assume) call the DIRTYFB ioctl. Which in turn would do an atomic update to flush the dirty rects, which would stall until the next vblank. And then the whole process would repeat. But this is a bit silly, we only need dirtyfb for command mode DSI panels. So track in plane state whether dirtyfb is required, and track in the fb how many attached planes require dirtyfb so that we can skip it when not required. (Note, mdp4 does not actually have cmd mode support.) Signed-off-by: Rob Clark --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 20 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 5 +-- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 3 ++ drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 19 -- drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 8 + drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.h | 5 +++ drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 21 +-- drivers/gpu/drm/msm/msm_atomic.c | 15 drivers/gpu/drm/msm/msm_drv.h | 6 ++-- drivers/gpu/drm/msm/msm_fb.c | 41 ++ 10 files changed, 110 insertions(+), 33 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 662b7bc9c219..7763558ef566 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1046,6 +1046,20 @@ struct plane_state { u32 pipe_id; }; +static bool dpu_crtc_needs_dirtyfb(struct drm_crtc_state *cstate) +{ + struct drm_crtc *crtc = cstate->crtc; + struct drm_encoder *encoder; + + drm_for_each_encoder_mask (encoder, crtc->dev, cstate->encoder_mask) { + if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_CMD) { + return true; + } + } + + return false; +} + static int dpu_crtc_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state) { @@ -1066,6 +1080,7 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, const struct drm_plane_state *pipe_staged[SSPP_MAX]; int left_zpos_cnt = 0, right_zpos_cnt = 0; struct drm_rect crtc_rect = { 0 }; + bool needs_dirtyfb = dpu_crtc_needs_dirtyfb(crtc_state); pstates = kzalloc(sizeof(*pstates) * DPU_STAGE_MAX * 4, GFP_KERNEL); @@ -1097,6 +1112,7 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, /* get plane state for all drm planes associated with crtc state */ drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) { + struct dpu_plane_state *dpu_pstate = to_dpu_plane_state(pstate); struct drm_rect dst, clip = crtc_rect; if (IS_ERR_OR_NULL(pstate)) { @@ -1108,11 +1124,13 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, if (cnt >= DPU_STAGE_MAX * 4) continue; - pstates[cnt].dpu_pstate = to_dpu_plane_state(pstate); + pstates[cnt].dpu_pstate = dpu_pstate; pstates[cnt].drm_pstate = pstate; pstates[cnt].stage = pstate->normalized_zpos; pstates[cnt].pipe_id = dpu_plane_pipe(plane); + dpu_pstate->needs_dirtyfb = needs_dirtyfb; + if (pipe_staged[pstates[cnt].pipe_id]) { multirect_plane[multirect_count].r0 = pipe_staged[pstates[cnt].pipe_id]; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c index ca75089c9d61..6565682fe227 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c @@ -902,7 +902,7 @@ static int dpu_plane_prepare_fb(struct drm_plane *plane, if (pstate->aspace) { ret = msm_framebuffer_prepare(new_state->fb, - pstate->aspace); + pstate->aspace, pstate->needs_dirtyfb); if (ret) { DPU_ERROR("failed to prepare framebuffer\n"); return ret; @@ -933,7 +933,8 @@ static void dpu_plane_cleanup_fb(struct drm_plane *plane, DPU_DEBUG_PLANE(pdpu, "FB[%u]\n", old_state->fb->base.id); - msm_framebuffer_cleanup(old_state->fb, old_pstate->aspace); + msm_framebuffer_cleanup(old_state->fb, old_pstate->aspace, + old_pstate->needs_dirtyfb); } static bool dpu_plane_validate_src(struct drm_rect *src, diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h b/drivers/g
[PULL] drm-misc-fixes
Hi Dave and Daniel, here's drm-misc-fixes for this week. Best regards Thomas drm-misc-fixes-2022-02-23: * edid: Always set RGB444 * imx/dcss: Select GEM CMA helpers * radeon: Fix some variables's type * vc4: Fix codec cleanup; Fix PM reference counting The following changes since commit 439cf34c8e0a8a33d8c15a31be1b7423426bc765: drm/atomic: Don't pollute crtc_state->mode_blob with error pointers (2022-02-16 12:32:07 +0200) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-misc tags/drm-misc-fixes-2022-02-23 for you to fetch changes up to ecbd4912a693b862e25cba0a6990a8c95b00721e: drm/edid: Always set RGB444 (2022-02-23 14:12:01 +0100) * edid: Always set RGB444 * imx/dcss: Select GEM CMA helpers * radeon: Fix some variables's type * vc4: Fix codec cleanup; Fix PM reference counting Christian König (1): drm/radeon: fix variable type Maxime Ripard (3): drm/vc4: hdmi: Unregister codec device on unbind drm/vc4: crtc: Fix runtime_pm reference counting drm/edid: Always set RGB444 Rudi Heitbaum (1): drm/imx/dcss: i.MX8MQ DCSS select DRM_GEM_CMA_HELPER drivers/gpu/drm/drm_edid.c | 2 +- drivers/gpu/drm/imx/dcss/Kconfig| 1 + drivers/gpu/drm/radeon/radeon_uvd.c | 8 drivers/gpu/drm/vc4/vc4_crtc.c | 8 +--- drivers/gpu/drm/vc4/vc4_hdmi.c | 8 drivers/gpu/drm/vc4/vc4_hdmi.h | 1 + 6 files changed, 20 insertions(+), 8 deletions(-) -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer