Re: [PATCH 1/6] drm: Add connector atomic_begin/atomic_flush
Hi Algea, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on drm-tip/drm-tip] [also build test WARNING on linus/master drm-exynos/exynos-drm-next v5.8 next-20200812] [cannot apply to rockchip/for-next drm-intel/for-linux-next tegra-drm/drm/tegra/for-next drm/drm-next] [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/Algea-Cao/Support-change-dw-hdmi-output-color/20200812-164647 base: git://anongit.freedesktop.org/drm/drm-tip drm-tip config: m68k-randconfig-s032-20200812 (attached as .config) compiler: m68k-linux-gcc (GCC) 9.3.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.2-168-g9554805c-dirty # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=m68k 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 include/linux/err.h:5, from include/linux/dma-fence.h:16, from drivers/gpu/drm/drm_atomic_helper.c:28: include/linux/scatterlist.h: In function 'sg_set_buf': arch/m68k/include/asm/page_mm.h:169:49: warning: ordered comparison of pointer with null pointer [-Wextra] 169 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void *)PAGE_OFFSET && (void *)(kaddr) < high_memory) | ^~ include/linux/compiler.h:78:42: note: in definition of macro 'unlikely' 78 | # define unlikely(x) __builtin_expect(!!(x), 0) | ^ include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON' 143 | BUG_ON(!virt_addr_valid(buf)); | ^~ include/linux/scatterlist.h:143:10: note: in expansion of macro 'virt_addr_valid' 143 | BUG_ON(!virt_addr_valid(buf)); | ^~~ drivers/gpu/drm/drm_atomic_helper.c: In function 'drm_atomic_helper_commit_planes': >> drivers/gpu/drm/drm_atomic_helper.c:2475:52: warning: variable >> 'new_connector_state' set but not used [-Wunused-but-set-variable] 2475 | struct drm_connector_state *old_connector_state, *new_connector_state; | ^~~ vim +/new_connector_state +2475 drivers/gpu/drm/drm_atomic_helper.c 2428 2429 /** 2430 * drm_atomic_helper_commit_planes - commit plane state 2431 * @dev: DRM device 2432 * @old_state: atomic state object with old state structures 2433 * @flags: flags for committing plane state 2434 * 2435 * This function commits the new plane state using the plane and atomic helper 2436 * functions for planes and CRTCs. It assumes that the atomic state has already 2437 * been pushed into the relevant object state pointers, since this step can no 2438 * longer fail. 2439 * 2440 * It still requires the global state object @old_state to know which planes and 2441 * crtcs need to be updated though. 2442 * 2443 * Note that this function does all plane updates across all CRTCs in one step. 2444 * If the hardware can't support this approach look at 2445 * drm_atomic_helper_commit_planes_on_crtc() instead. 2446 * 2447 * Plane parameters can be updated by applications while the associated CRTC is 2448 * disabled. The DRM/KMS core will store the parameters in the plane state, 2449 * which will be available to the driver when the CRTC is turned on. As a result 2450 * most drivers don't need to be immediately notified of plane updates for a 2451 * disabled CRTC. 2452 * 2453 * Unless otherwise needed, drivers are advised to set the ACTIVE_ONLY flag in 2454 * @flags in order not to receive plane update notifications related to a 2455 * disabled CRTC. This avoids the need to manually ignore plane updates in 2456 * driver code when the driver and/or hardware can't or just don't need to deal 2457 * with updates on disabled CRTCs, for example when supporting runtime PM. 2458 * 2459 * Drivers may set the NO_DISABLE_AFTER_MODESET flag in @flags if the relevant 2460 * display controllers require to disable a CRTC's planes when the CRTC is 2461 * disabled. This function would skip the _plane_helper_funcs.atomic_disable 2462 * call for a plane if the CRTC of the old plane state needs a modesetting 2463 * operation. Of course, the drivers need to disable the planes in their CRTC 2464 * disable callbacks since no
Re: [PATCH 1/6] drm: Add connector atomic_begin/atomic_flush
Hi Algea,
Thank you for the patch.
On Wed, Aug 12, 2020 at 04:34:07PM +0800, Algea Cao wrote:
> In some situations, connector should get some work done
> when plane is updating. Such as when change output color
> format, hdmi should send AVMUTE to make screen black before
> crtc updating color format, or screen may flash. After color
> updating, hdmi should clear AVMUTE bring screen back to normal.
>
> The process is as follows:
> AVMUTE -> Update CRTC -> Update HDMI -> Clear AVMUTE
>
> So we introduce connector atomic_begin/atomic_flush.
Implementing this through .atomic_begin() and .atomic_flush() seems like
a pretty big hack to me. Furthermore, I think this should be implemented
as bridge operations, not at the connector level, as the HDMI encoder
may not be the component that implements the drm_connector.
> Signed-off-by: Algea Cao
>
> ---
>
> drivers/gpu/drm/drm_atomic_helper.c | 46
> include/drm/drm_modeset_helper_vtables.h | 19 ++
> 2 files changed, 65 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index f68c69a45752..f4abd700d2c4 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2471,6 +2471,8 @@ void drm_atomic_helper_commit_planes(struct drm_device
> *dev,
>struct drm_atomic_state *old_state,
>uint32_t flags)
> {
> + struct drm_connector *connector;
> + struct drm_connector_state *old_connector_state, *new_connector_state;
> struct drm_crtc *crtc;
> struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> struct drm_plane *plane;
> @@ -2479,6 +2481,28 @@ void drm_atomic_helper_commit_planes(struct drm_device
> *dev,
> bool active_only = flags & DRM_PLANE_COMMIT_ACTIVE_ONLY;
> bool no_disable = flags & DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET;
>
> + for_each_oldnew_connector_in_state(old_state, connector,
> +old_connector_state,
> +new_connector_state, i) {
> + const struct drm_connector_helper_funcs *funcs;
> +
> + if (!connector->state->crtc)
> + continue;
> +
> + if (!connector->state->crtc->state->active)
> + continue;
> +
> + funcs = connector->helper_private;
> +
> + if (!funcs || !funcs->atomic_begin)
> + continue;
> +
> + DRM_DEBUG_ATOMIC("flush beginning [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
> +
> + funcs->atomic_begin(connector, old_connector_state);
> + }
> +
> for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state,
> new_crtc_state, i) {
> const struct drm_crtc_helper_funcs *funcs;
>
> @@ -2550,6 +2574,28 @@ void drm_atomic_helper_commit_planes(struct drm_device
> *dev,
>
> funcs->atomic_flush(crtc, old_crtc_state);
> }
> +
> + for_each_oldnew_connector_in_state(old_state, connector,
> +old_connector_state,
> +new_connector_state, i) {
> + const struct drm_connector_helper_funcs *funcs;
> +
> + if (!connector->state->crtc)
> + continue;
> +
> + if (!connector->state->crtc->state->active)
> + continue;
> +
> + funcs = connector->helper_private;
> +
> + if (!funcs || !funcs->atomic_flush)
> + continue;
> +
> + DRM_DEBUG_ATOMIC("flushing [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
> +
> + funcs->atomic_flush(connector, old_connector_state);
> + }
> }
> EXPORT_SYMBOL(drm_atomic_helper_commit_planes);
>
> diff --git a/include/drm/drm_modeset_helper_vtables.h
> b/include/drm/drm_modeset_helper_vtables.h
> index 421a30f08463..10f3f2e2fe28 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1075,6 +1075,25 @@ struct drm_connector_helper_funcs {
> void (*atomic_commit)(struct drm_connector *connector,
> struct drm_connector_state *state);
>
> + /**
> + * @atomic_begin:
> + *
> + * flush atomic update
> + *
> + * This callback is used by the atomic modeset helpers but it is
> optional.
> + */
> + void (*atomic_begin)(struct drm_connector *connector,
> + struct drm_connector_state *state);
> +
> + /**
> + * @atomic_begin:
> + *
> + * begin atomic update
> + *
> + * This callback is used by the atomic modeset helpers but it is
> optional.
> + */
> + void (*atomic_flush)(struct drm_connector *connector,
> +
