drm/bridge: ps8640 and ti-sn65dsi86 updates
Subsystems (5)
cover drm/bridge: ps8640 and ti-sn65dsi86 updates
Add a new macro DRM_BRIDGE_STATE_OPS that can be used to assign atomic_reset and atomic_{duplicate,destroy}_state to the default implementations. They will be valid in most cases.
Add a drm variant of media-bus-format.h to hold a single function to get the bpc from the bus format.
Also add a small drm_atomic_helper_bridge_dsi_input_bus_fmt helper.
Update ti-sn65dsi86 to support atomic and NO_CONNECTOR. The NO_CONNECTOR support was from Rob Clark - I just rebased it. To support NO_CONNECTOR use the newly introduced function to lokk up bpc from the bus format.
Update parade-ps8640 to support atomic. To do this just migrate to the atomic variants of the operations and add the few mandatry missing callbacks.
A few of the patches are migrated from a patchset I posted several months ago and I decided to add them here for now, which explains why there is a v4 of a patch in a v1 submission.
For the output bus fmt stuff I did what I think is correct - but as I have paged out all my memory of this it may be all wrong.
The code builds - but needs testing.
I was temped to move bridge helpers to a new drm_bridge_helper.c but that will wait until next time.
Sam
Rob Clark (1):
drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support
Sam Ravnborg (8):
drm/bridge: add DRM_BRIDGE_STATE_OPS macro
drm: add drm specific media-bus-format header file
drm: add drm_atomic_helper_bridge_dsi_input_bus_fmt drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs drm/bridge: ti-sn65dsi86: Fetch bpc via drm_bridge_state drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs drm/bridge: ps8640: plug atomic_get_input_bus_fmts drm/bridge: Drop unused drm_bridge_chain functions
drivers/gpu/drm/bridge/parade-ps8640.c | 18 ++++-- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 57 +++++++++-------- drivers/gpu/drm/drm_atomic_helper.c | 41 ++++++++++++ drivers/gpu/drm/drm_bridge.c | 110 --------------------------------- include/drm/drm_atomic_helper.h | 7 +++ include/drm/drm_bridge.h | 40 ++++-------- include/drm/media-bus-format.h | 53 ++++++++++++++++ 7 files changed, 157 insertions(+), 169 deletions(-)
PATCH 1/9 drm/bridge: add DRM_BRIDGE_STATE_OPS macro
The DRM_BRIDGE_STATE_OPS can be used as shortcut for bridge drivers that do not subclass drm_bridge_state to assign the default operations for reset, duplicate and destroy of the state.
--- include/drm/drm_bridge.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
include/drm/drm_bridge.h +12 -0
@@ -935,4 +935,16 @@ static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, }#endif +/** +* DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs +* +* Bridge driver that do not subclass &drm_bridge_state can use the helpers +* for reset, duplicate, and destroy. This macro provides a shortcut for +* setting the helpers in the &drm_bridge_funcs structure. +*/ +#define DRM_BRIDGE_STATE_OPS \ + .atomic_reset = drm_atomic_helper_bridge_reset, \ + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, \ + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state +#endif
Signature · 1 line
2.32.0
Re: [PATCH v1 1/9] drm/bridge: add DRM_BRIDGE_STATE_OPS macro
Hi,
Quoted 28 lines · Sam Ravnborg
The
DRM_BRIDGE_STATE_OPScan be used as shortcut for bridge drivers that do not subclassdrm_bridge_stateto assign the default operations for reset, duplicate and destroy of the state.Signed-off-by Sam Ravnborg <[email protected]>Cc Douglas Anderson <[email protected]>Cc Andrzej Hajda <[email protected]>Cc Neil Armstrong <[email protected]>Cc Robert Foss <[email protected]>Cc Laurent Pinchart <[email protected]>Cc Jonas Karlman <[email protected]>Cc Jernej Skrabec <[email protected]>Cc Maxime Ripard <[email protected]>Cc Thomas Zimmermann <[email protected]>--- include/drm/drm_bridge.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)include/drm/drm_bridge.h +1
-0@@ -935,4 +935,16 @@ static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, }#endif +/**
include/drm/drm_bridge.h static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, · line 948
Quoted 10 lines
include/drm/drm_bridge.h +10
-0+* DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs +* +* Bridge driver that do not subclass &drm_bridge_state can use the helpers +* for reset, duplicate, and destroy. This macro provides a shortcut for +* setting the helpers in the &drm_bridge_funcs structure. +*/ +#define DRM_BRIDGE_STATE_OPS \ + .atomic_reset = drm_atomic_helper_bridge_reset, \ + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, \ + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state
Re: [PATCH v1 1/9] drm/bridge: add DRM_BRIDGE_STATE_OPS macro
In reply to Sam RavnborgHi Sam,
Thank you for the patch.
Quoted 23 lines · Sam Ravnborg
The
DRM_BRIDGE_STATE_OPScan be used as shortcut for bridge drivers that do not subclassdrm_bridge_stateto assign the default operations for reset, duplicate and destroy of the state.Signed-off-by Sam Ravnborg <[email protected]>Cc Douglas Anderson <[email protected]>Cc Andrzej Hajda <[email protected]>Cc Neil Armstrong <[email protected]>Cc Robert Foss <[email protected]>Cc Laurent Pinchart <[email protected]>Cc Jonas Karlman <[email protected]>Cc Jernej Skrabec <[email protected]>Cc Maxime Ripard <[email protected]>Cc Thomas Zimmermann <[email protected]>--- include/drm/drm_bridge.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)include/drm/drm_bridge.h +0
-0
include/drm/drm_bridge.h static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, · line 939
Quoted 5 lines
include/drm/drm_bridge.h +2
-0@@ -935,4 +935,16 @@ static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, }#endif +/** +* DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs
I'd name the macro DRM_BRIDGE_STATE_DEFAULT_OPS.
include/drm/drm_bridge.h static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, · line 948
Quoted 9 lines
include/drm/drm_bridge.h +9
-0+* +* Bridge driver that do not subclass &drm_bridge_state can use the helpers +* for reset, duplicate, and destroy. This macro provides a shortcut for +* setting the helpers in the &drm_bridge_funcs structure. +*/ +#define DRM_BRIDGE_STATE_OPS \ + .atomic_reset = drm_atomic_helper_bridge_reset, \ + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, \ + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state
I'm not a big fan of such macros for a small number of operations, as I find that it obfuscates the code a bit, but that could change once I get used to the new macro :-)
Quoted 2 lines
include/drm/drm_bridge.h +1
-0+#endif
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v1 1/9] drm/bridge: add DRM_BRIDGE_STATE_OPS macro
Hi Laurent,
Quoted 31 lines · Laurent Pinchart
Hi Sam,
Thank you for the patch.
Quoted 26 lines · Sam Ravnborg
The
DRM_BRIDGE_STATE_OPScan be used as shortcut for bridge drivers that do not subclassdrm_bridge_stateto assign the default operations for reset, duplicate and destroy of the state.Signed-off-by Sam Ravnborg <[email protected]>Cc Douglas Anderson <[email protected]>Cc Andrzej Hajda <[email protected]>Cc Neil Armstrong <[email protected]>Cc Robert Foss <[email protected]>Cc Laurent Pinchart <[email protected]>Cc Jonas Karlman <[email protected]>Cc Jernej Skrabec <[email protected]>Cc Maxime Ripard <[email protected]>Cc Thomas Zimmermann <[email protected]>--- include/drm/drm_bridge.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)include/drm/drm_bridge.h +0
-0@@ -935,4 +935,16 @@ static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, }#endif
Quoted 4 lines
include/drm/drm_bridge.h static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, · line 939
Quoted 2 lines
include/drm/drm_bridge.h +2
-0+/** +* DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs I'd name the macro
DRM_BRIDGE_STATE_DEFAULT_OPS.
OK, done.
Quoted 14 lines
include/drm/drm_bridge.h static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, · line 948
Quoted 10 lines
include/drm/drm_bridge.h +9
-0+* +* Bridge driver that do not subclass &drm_bridge_state can use the helpers +* for reset, duplicate, and destroy. This macro provides a shortcut for +* setting the helpers in the &drm_bridge_funcs structure. +*/ +#define DRM_BRIDGE_STATE_OPS \ + .atomic_reset = drm_atomic_helper_bridge_reset, \ + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, \ + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_stateI'm not a big fan of such macros for a small number of operations, as I find that it obfuscates the code a bit, but that could change once I get used to the new macro :-)
The use of the macro is a nice signal that all the relevant default operations are specified - no need to look up if all are included.
I have on my TODO to update all relevant bridge drivers to use it.
Quoted 10 lines
Reviewed-by Laurent Pinchart <[email protected]>Quoted 2 lines
include/drm/drm_bridge.h +1
-0+#endif Signature · 3 lines
Regards, Laurent Pinchart
PATCH 2/9 drm: add drm specific media-bus-format header file
In reply to Sam RavnborgFor now the header file includes a single method to retreive the bpc from the bus format.
The supported MEDIA_BUS_* codes are the ones used for the current panels in DRM. The list can be extended as there are a need for more formats.
--- include/drm/media-bus-format.h | 53 ++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 include/drm/media-bus-format.h
include/drm/media-bus-format.h new file +53 -0
@@ -0,0 +1,53 @@ +/* SPDX-License-Identifier: MIT */ +/* +* Copyright © 2022 Sam Ravnborg +*/ + +#ifndef __LINUX_DRM_MEDIA_BUS_FORMAT +#define __LINUX_DRM_MEDIA_BUS_FORMAT + +#include <linux/bug.h> +#include <linux/media-bus-format.h> +#include <linux/types.h> + +/** +* media_bus_format_to_bpc - The bits per color channel for the bus_format +* +* Based on the supplied bus_format return the maximum number of bits +* per color channel. +* +* RETURNS +* The number of bits per color channel, or -EINVAL if the bus_format +* is unknown. +*/ +static inline int media_bus_format_to_bpc (u32 bus_format) +{ +switch (bus_format) { +/* DPI */ +case MEDIA_BUS_FMT_RGB565_1X16 : +case MEDIA_BUS_FMT_RGB666_1X18 : +return 6 ; + +/* DPI */ +case MEDIA_BUS_FMT_RGB888_1X24 : +case MEDIA_BUS_FMT_RGB888_3X8 : +case MEDIA_BUS_FMT_RGB888_3X8_DELTA : +case MEDIA_BUS_FMT_Y8_1X8 : +return 8 ; + +/* LVDS */ +case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG : +return 6 ; + +/* LVDS */ +case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA : +case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG : +return 8 ; + +default : +WARN (1 ,"Unknown MEDIA_BUS format %d\n" , bus_format); +return -EINVAL ; + } +} + +#endif
Signature · 1 line
2.32.0
Doug Anderson
reviewerRe: [PATCH v1 2/9] drm: add drm specific media-bus-format header file
Hi,
Quoted 29 lines · Sam Ravnborg
For now the header file includes a single method to retreive the bpc from the bus format.
The supported MEDIA_BUS_* codes are the ones used for the current panels in DRM. The list can be extended as there are a need for more formats.Signed-off-by Sam Ravnborg <[email protected]>--- include/drm/media-bus-format.h | 53 ++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 include/drm/media-bus-format.hinclude/drm/media-bus-format.h new file +11
-0@@ -0,0 +1,53 @@ +/* SPDX-License-Identifier: MIT */ +/* +* Copyright © 2022 Sam Ravnborg +*/ + +#ifndef __LINUX_DRM_MEDIA_BUS_FORMAT +#define __LINUX_DRM_MEDIA_BUS_FORMAT + +#include <linux/bug.h> +#include <linux/media-bus-format.h> +#include <linux/types.h>
include/drm/media-bus-format.h line 21
Quoted 10 lines
include/drm/media-bus-format.h new file +10
-0+ +/** +* media_bus_format_to_bpc - The bits per color channel for the bus_format +* +* Based on the supplied bus_format return the maximum number of bits +* per color channel. +* +* RETURNS +* The number of bits per color channel, or -EINVAL if the bus_format +* is unknown.
kernel-doc doesn't like your syntax quite right. Try running:
./scripts/kernel-doc -rst -v include/drm/media-bus-format.h
It will tell you that you're missing a description of the parameter and the way you're describing the return value isn't in a way that it can parse...
Quoted 11 lines
include/drm/media-bus-format.h new file +11
-0+*/ +static inline int media_bus_format_to_bpc (u32 bus_format) +{ +switch (bus_format) { +/* DPI */ +case MEDIA_BUS_FMT_RGB565_1X16 : +case MEDIA_BUS_FMT_RGB666_1X18 : +return 6 ; + +/* DPI */ +case MEDIA_BUS_FMT_RGB888_1X24 :
include/drm/media-bus-format.h line 42
Quoted 10 lines
include/drm/media-bus-format.h new file +10
-0+case MEDIA_BUS_FMT_RGB888_3X8 : +case MEDIA_BUS_FMT_RGB888_3X8_DELTA : +case MEDIA_BUS_FMT_Y8_1X8 : +return 8 ; + +/* LVDS */ +case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG : +return 6 ; + +/* LVDS */
When applying your patches, I got a warning that both of your "/* LVDS */" comments have spaces before the tab.
I'm also not sure what the "sections" are supposed to mean. Are you saying that the bus formats are only for the given transport type? ...so we can't use MEDIA_BUS_FMT_RGB666_1X18 for eDP?
-Doug
Laurent Pinchart
reviewerRe: [PATCH v1 2/9] drm: add drm specific media-bus-format header file
In reply to Sam RavnborgHi Sam,
Thank you for the patch.
Quoted 1 line · Sam Ravnborg
For now the header file includes a single method to retreive the bpc
s/retreive/retrieve/
Quoted 44 lines
from the bus format.
The supported MEDIA_BUS_* codes are the ones used for the current panels in DRM. The list can be extended as there are a need for more formats.Signed-off-by Sam Ravnborg <[email protected]>--- include/drm/media-bus-format.h | 53 ++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 include/drm/media-bus-format.hinclude/drm/media-bus-format.h new file +28
-0@@ -0,0 +1,53 @@ +/* SPDX-License-Identifier: MIT */ +/* +* Copyright © 2022 Sam Ravnborg +*/ + +#ifndef __LINUX_DRM_MEDIA_BUS_FORMAT +#define __LINUX_DRM_MEDIA_BUS_FORMAT + +#include <linux/bug.h> +#include <linux/media-bus-format.h> +#include <linux/types.h> + +/** +* media_bus_format_to_bpc - The bits per color channel for the bus_format +* +* Based on the supplied bus_format return the maximum number of bits +* per color channel. +* +* RETURNS +* The number of bits per color channel, or -EINVAL if the bus_format +* is unknown. +*/ +static inline int media_bus_format_to_bpc (u32 bus_format) +{ +switch (bus_format) { +/* DPI */ +case MEDIA_BUS_FMT_RGB565_1X16 : +case MEDIA_BUS_FMT_RGB666_1X18 :
include/drm/media-bus-format.h line 38
Quoted 10 lines
include/drm/media-bus-format.h new file +10
-0+return 6 ; + +/* DPI */ +case MEDIA_BUS_FMT_RGB888_1X24 : +case MEDIA_BUS_FMT_RGB888_3X8 : +case MEDIA_BUS_FMT_RGB888_3X8_DELTA : +case MEDIA_BUS_FMT_Y8_1X8 : +return 8 ; + +/* LVDS */
Wrong indentation.
include/drm/media-bus-format.h line 51
Quoted 13 lines
include/drm/media-bus-format.h new file +13
-0+case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG : +return 6 ; + +/* LVDS */ +case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA : +case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG : +return 8 ; + +default : +WARN (1 ,"Unknown MEDIA_BUS format %d\n" , bus_format); +return -EINVAL ; + } +}
This seems a bit big for an inline function.
If we add more helper functions, it will result in lots of switch...case statements. Could we use the same approach as in drm_fourcc.h with a format info structure ?
Quoted 2 lines
include/drm/media-bus-format.h new file +2
-0+ +#endif
Signature · 3 lines
Regards, Laurent Pinchart
PATCH 3/9 drm: add drm_atomic_helper_bridge_dsi_input_bus_fmt
In reply to Sam RavnborgThere is a number of bridge drivers that supports a single media bus format for DSI. Add a helper to avoid duplicating the code.
--- drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 7 +++++ 2 files changed, 48 insertions(+)
drivers/gpu/drm/drm_atomic_helper.c +41 -0
@@ -3549,3 +3549,44 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge,return input_fmts; }EXPORT_SYMBOL (drm_atomic_helper_bridge_propagate_bus_fmt); + +/** +* drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format +* +* @bridge: bridge control structure +* @bridge_state: new bridge state +* @crtc_state: new CRTC state +* @conn_state: new connector state +* @output_fmt: tested output bus format +* @num_input_fmts: will contain the size of the returned array +* +* This helper is an implementation of the +* &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports +* a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24. +* +* RETURNS +* A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24, +* or NULL if the allocation failed +*/ +u32 * +drm_atomic_helper_bridge_dsi_input_bus_fmt (struct drm_bridge *bridge, +struct drm_bridge_state *bridge_state, +struct drm_crtc_state *crtc_state, +struct drm_connector_state *conn_state, +u32 output_fmt, +unsigned int *num_input_fmts) +{ +u32 *input_fmts; + + *num_input_fmts =0 ; + + input_fmts =kcalloc (MAX_INPUT_SEL_FORMATS ,sizeof (*input_fmts),GFP_KERNEL ); +if (!input_fmts) +return NULL ; + +/* This is the DSI-end bus format */ + input_fmts[0 ] =MEDIA_BUS_FMT_RGB888_1X24 ; + *num_input_fmts =1 ; + +return input_fmts; +}
include/drm/drm_atomic_helper.h +7 -0
@@ -231,4 +231,11 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge,u32 output_fmt, unsigned int *num_input_fmts); +u32 * +drm_atomic_helper_bridge_dsi_input_bus_fmt (struct drm_bridge *bridge, +struct drm_bridge_state *bridge_state, +struct drm_crtc_state *crtc_state, +struct drm_connector_state *conn_state, +u32 output_fmt, +unsigned int *num_input_fmts);#endif /* DRM_ATOMIC_HELPER_H_ */
Signature · 1 line
2.32.0
kernel test robot
build botRe: [PATCH v1 3/9] drm: add drm_atomic_helper_bridge_dsi_input_bus_fmt
I love your patch! Yet something to improve:
Hi Sam,
I love your patch! Yet something to improve:
[auto build test ERROR on drm/drm-next]
[also build test ERROR on drm-intel/for-linux-next drm-tip/drm-tip drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next v5.17-rc2 next-20220204] [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/Sam-Ravnborg/drm-bridge-ps8640-and-ti-sn65dsi86-updates/20220206-234638 base: git://anongit.freedesktop.org/drm/drm drm-next config: hexagon-buildonly-randconfig-r003-20220206 (https://download.01.org/0day-ci/archive/20220207/202202070113.dsmEk0pf-lkp(a)intel.com/config) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 6daaf5a44925592c764c59219b0024ee06317028)
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/d72252f11e9d51bd1da85eb99299f3d8fec36bc2
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sam-Ravnborg/drm-bridge-ps8640-and-ti-sn65dsi86-updates/20220206-234638 git checkout d72252f11e9d51bd1da85eb99299f3d8fec36bc2
# 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=hexagon SHELL=/bin/bash drivers/gpu/drm/
If you fix the issue, kindly add following tag as appropriate
All errors (new ones prefixed by >>):
>> drivers/gpu/drm/drm_atomic_helper.c:3583:23: error: use of undeclared identifier 'MAX_INPUT_SEL_FORMATS'
input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), GFP_KERNEL);
^1 error generated.
vim +/MAX_INPUT_SEL_FORMATS +3583 drivers/gpu/drm/drm_atomic_helper.c
3552
3553 /**
3554 * drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format
3555 *
3556 * @bridge: bridge control structure
3557 * @bridge_state: new bridge state
3558 * @crtc_state: new CRTC state
3559 * @conn_state: new connector state
3560 * @output_fmt: tested output bus format
3561 * @num_input_fmts: will contain the size of the returned array
3562 *
3563 * This helper is an implementation of the
3564 * &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports
3565 * a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24.
3566 *
3567 * RETURNS
3568 * A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24,
3569 * or NULL if the allocation failed
3570 */
3571 u32 *
3572 drm_atomic_helper_bridge_dsi_input_bus_fmt(struct drm_bridge *bridge,
3573 struct drm_bridge_state *bridge_state,
3574 struct drm_crtc_state *crtc_state,
3575 struct drm_connector_state *conn_state,
3576 u32 output_fmt,
3577 unsigned int *num_input_fmts)
3578 {
3579 u32 *input_fmts;
3580
3581 *num_input_fmts = 0;
3582
> 3583 input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), GFP_KERNEL);---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
Doug Anderson
reviewerRe: [PATCH v1 3/9] drm: add drm_atomic_helper_bridge_dsi_input_bus_fmt
In reply to Sam RavnborgHi,
Quoted 15 lines · Sam Ravnborg
There is a number of bridge drivers that supports a single media bus format for DSI. Add a helper to avoid duplicating the code.
Signed-off-by Sam Ravnborg <[email protected]>--- drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 7 +++++ 2 files changed, 48 insertions(+)drivers/gpu/drm/drm_atomic_helper.c +0
-0
drivers/gpu/drm/drm_atomic_helper.c drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, · line 3554
Quoted 6 lines
drivers/gpu/drm/drm_atomic_helper.c +3
-0@@ -3549,3 +3549,44 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge,return input_fmts; }EXPORT_SYMBOL (drm_atomic_helper_bridge_propagate_bus_fmt); + +/** +* drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format
Is the description right? It's called "input" format but it defines an output format?
drivers/gpu/drm/drm_atomic_helper.c drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, · line 3561
Quoted 6 lines
drivers/gpu/drm/drm_atomic_helper.c +6
-0+* @bridge: bridge control structure +* @bridge_state: new bridge state +* @crtc_state: new CRTC state +* @conn_state: new connector state +* @output_fmt: tested output bus format +* @num_input_fmts: will contain the size of the returned array
Maybe indicate that it's always 1 in the comments?
drivers/gpu/drm/drm_atomic_helper.c drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, · line 3567
Quoted 6 lines
drivers/gpu/drm/drm_atomic_helper.c +6
-0+* +* This helper is an implementation of the +* &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports +* a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24. +* +* RETURNS
kernel-doc can't parse this return syntax and warns:
warning: No description found for return value of
'drm_atomic_helper_bridge_dsi_input_bus_fmt'
Quoted 6 lines
drivers/gpu/drm/drm_atomic_helper.c +6
-0+* A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24, +* or NULL if the allocation failed +*/ +u32 * +drm_atomic_helper_bridge_dsi_input_bus_fmt(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state,
drivers/gpu/drm/drm_atomic_helper.c drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, · line 3583
Quoted 10 lines
drivers/gpu/drm/drm_atomic_helper.c +10
-0+struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state, + u32 output_fmt, + unsigned int *num_input_fmts) +{ + u32 *input_fmts; + + *num_input_fmts =0 ; + + input_fmts =kcalloc (MAX_INPUT_SEL_FORMATS ,sizeof (*input_fmts),GFP_KERNEL );
I probably wouldn't have bothered with kcalloc for something that's always just one value and you're setting it. Why not just
input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
...also MAX_INPUT_SEL_FORMATS isn't defined. I guess that's why you said it didn't compile?
Also: if it was common for others to want to provide fixed formats, I wonder about adding a helper function that did most of the work here? Dunno what it would be named since it's already a bit a of handful, but I'd expect to call it like:
static const u32 formats[] = { MEDIA_BUS_FMT_RGB888_1X24 }; return my_neat_helper(formats, ARRAY_SIZE(formats), num_output_formats)
Then my_neat_helper() could do kmemdup() on the array passed and fill in "num_output_formats" to be either the array size of 0 (if the kmemdup failed).
drivers/gpu/drm/drm_atomic_helper.c drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, · line 3588
Quoted 5 lines
drivers/gpu/drm/drm_atomic_helper.c +5
-0+if (!input_fmts) +return NULL ; + +/* This is the DSI-end bus format */ + input_fmts[0 ] =MEDIA_BUS_FMT_RGB888_1X24 ;
I'm not an expert, but I'm curious. Can't DSI run in other formats? ...or maybe I'm misunderstanding what this is for. I guess I'm not sure how it relates to:
enum mipi_dsi_pixel_format {
MIPI_DSI_FMT_RGB888,
MIPI_DSI_FMT_RGB666,
MIPI_DSI_FMT_RGB666_PACKED,
MIPI_DSI_FMT_RGB565,
};
-Doug
Laurent Pinchart
reviewerRe: [PATCH v1 3/9] drm: add drm_atomic_helper_bridge_dsi_input_bus_fmt
Hello,
Quoted 79 lines · Doug Anderson
Quoted 15 lines · Sam Ravnborg
There is a number of bridge drivers that supports a single media bus format for DSI. Add a helper to avoid duplicating the code.
Signed-off-by Sam Ravnborg <[email protected]>--- drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 7 +++++ 2 files changed, 48 insertions(+)drivers/gpu/drm/drm_atomic_helper.c +0
-0drivers/gpu/drm/drm_atomic_helper.c drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, · line 3554
Quoted 6 lines
drivers/gpu/drm/drm_atomic_helper.c +3
-0@@ -3549,3 +3549,44 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge,return input_fmts; }EXPORT_SYMBOL (drm_atomic_helper_bridge_propagate_bus_fmt); + +/** +* drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format Is the description right? It's called "input" format but it defines an output format?
drivers/gpu/drm/drm_atomic_helper.c drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, · line 3561
Quoted 6 lines
drivers/gpu/drm/drm_atomic_helper.c +6
-0+* @bridge: bridge control structure +* @bridge_state: new bridge state +* @crtc_state: new CRTC state +* @conn_state: new connector state +* @output_fmt: tested output bus format +* @num_input_fmts: will contain the size of the returned array Maybe indicate that it's always 1 in the comments?
drivers/gpu/drm/drm_atomic_helper.c drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, · line 3567
Quoted 6 lines
drivers/gpu/drm/drm_atomic_helper.c +6
-0+* +* This helper is an implementation of the +* &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports +* a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24. +* +* RETURNS kernel-doc can't parse this return syntax and warns:
warning: No description found for return value of
'drm_atomic_helper_bridge_dsi_input_bus_fmt'Quoted 6 lines
drivers/gpu/drm/drm_atomic_helper.c +6
-0+* A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24, +* or NULL if the allocation failed +*/ +u32 * +drm_atomic_helper_bridge_dsi_input_bus_fmt(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state,drivers/gpu/drm/drm_atomic_helper.c drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, · line 3583
Quoted 10 lines
drivers/gpu/drm/drm_atomic_helper.c +10
-0+struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state, + u32 output_fmt, + unsigned int *num_input_fmts) +{ + u32 *input_fmts; + + *num_input_fmts =0 ; + + input_fmts =kcalloc (MAX_INPUT_SEL_FORMATS ,sizeof (*input_fmts),GFP_KERNEL );I probably wouldn't have bothered with
kcallocfor something that's always just one value and you're setting it. Why not just
input_fmts=kmalloc(sizeof(*input_fmts), GFP_KERNEL);...also
MAX_INPUT_SEL_FORMATSisn't defined. I guess that's why you said it didn't compile?Also: if it was common for others to want to provide fixed formats, I wonder about adding a helper function that did most of the work here? Dunno what it would be named since it's already a bit a of handful, but I'd expect to call it like:
Quoted 6 lines
static const u32 formats[] = {
MEDIA_BUS_FMT_RGB888_1X24}; returnmy_neat_helper(formats, ARRAY_SIZE(formats), num_output_formats)Then
my_neat_helper()could dokmemdup()on the array passed and fill in "num_output_formats" to be either the array size of 0 (if the kmemdup failed).
I quite like that approach. We could even have a wrapper macro that adds the ARRAY_SIZE() argument automatically.
Quoted 16 lines
drivers/gpu/drm/drm_atomic_helper.c drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, · line 3588
Quoted 5 lines
drivers/gpu/drm/drm_atomic_helper.c +5
-0+if (!input_fmts) +return NULL ; + +/* This is the DSI-end bus format */ + input_fmts[0 ] =MEDIA_BUS_FMT_RGB888_1X24 ;I'm not an expert, but I'm curious. Can't DSI run in other formats? ...or maybe I'm misunderstanding what this is for. I guess I'm not sure how it relates to:
enum
mipi_dsi_pixel_format{
MIPI_DSI_FMT_RGB888,
MIPI_DSI_FMT_RGB666,
MIPI_DSI_FMT_RGB666_PACKED,
MIPI_DSI_FMT_RGB565,
};
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v1 3/9] drm: add drm_atomic_helper_bridge_dsi_input_bus_fmt
Quoted 85 lines · Laurent Pinchart
Hello,
Quoted 82 lines · Doug Anderson
Quoted 15 lines · Sam Ravnborg
There is a number of bridge drivers that supports a single media bus format for DSI. Add a helper to avoid duplicating the code.
Signed-off-by Sam Ravnborg <[email protected]>--- drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 7 +++++ 2 files changed, 48 insertions(+)drivers/gpu/drm/drm_atomic_helper.c +0
-0drivers/gpu/drm/drm_atomic_helper.c drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, · line 3554
Quoted 6 lines
drivers/gpu/drm/drm_atomic_helper.c +3
-0@@ -3549,3 +3549,44 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge,return input_fmts; }EXPORT_SYMBOL (drm_atomic_helper_bridge_propagate_bus_fmt); + +/** +* drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format Is the description right? It's called "input" format but it defines an output format?
drivers/gpu/drm/drm_atomic_helper.c drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, · line 3561
Quoted 6 lines
drivers/gpu/drm/drm_atomic_helper.c +6
-0+* @bridge: bridge control structure +* @bridge_state: new bridge state +* @crtc_state: new CRTC state +* @conn_state: new connector state +* @output_fmt: tested output bus format +* @num_input_fmts: will contain the size of the returned array Maybe indicate that it's always 1 in the comments?
drivers/gpu/drm/drm_atomic_helper.c drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, · line 3567
Quoted 6 lines
drivers/gpu/drm/drm_atomic_helper.c +6
-0+* +* This helper is an implementation of the +* &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports +* a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24. +* +* RETURNS kernel-doc can't parse this return syntax and warns:
warning: No description found for return value of
'drm_atomic_helper_bridge_dsi_input_bus_fmt'Quoted 6 lines
drivers/gpu/drm/drm_atomic_helper.c +6
-0+* A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24, +* or NULL if the allocation failed +*/ +u32 * +drm_atomic_helper_bridge_dsi_input_bus_fmt(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state,drivers/gpu/drm/drm_atomic_helper.c drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, · line 3583
Quoted 10 lines
drivers/gpu/drm/drm_atomic_helper.c +10
-0+struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state, + u32 output_fmt, + unsigned int *num_input_fmts) +{ + u32 *input_fmts; + + *num_input_fmts =0 ; + + input_fmts =kcalloc (MAX_INPUT_SEL_FORMATS ,sizeof (*input_fmts),GFP_KERNEL );I probably wouldn't have bothered with
kcallocfor something that's always just one value and you're setting it. Why not just
input_fmts=kmalloc(sizeof(*input_fmts), GFP_KERNEL);...also
MAX_INPUT_SEL_FORMATSisn't defined. I guess that's why you said it didn't compile?Also: if it was common for others to want to provide fixed formats, I wonder about adding a helper function that did most of the work here? Dunno what it would be named since it's already a bit a of handful, but I'd expect to call it like:
static const u32 formats[] = {
MEDIA_BUS_FMT_RGB888_1X24}; returnmy_neat_helper(formats, ARRAY_SIZE(formats), num_output_formats)
Quoted 6 lines
Quoted 3 lines
Then
my_neat_helper()could dokmemdup()on the array passed and fill in "num_output_formats" to be either the array size of 0 (if the kmemdup failed).I quite like that approach. We could even have a wrapper macro that adds the
ARRAY_SIZE()argument automatically.
Agree, will try to give that a spin.
And will then also process all the nice feedback from Douglas.
Sam
PATCH 4/9 drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs
In reply to Sam RavnborgMove away from the deprecated enable/diable operations in drm_bridge_funcs and enable atomic use.
--- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
drivers/gpu/drm/bridge/ti-sn65dsi86.c +13 -8
@@ -796,7 +796,8 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,return MODE_OK ; } -static void ti_sn_bridge_disable (struct drm_bridge *bridge) +static void ti_sn_bridge_atomic_disable (struct drm_bridge *bridge, +struct drm_bridge_state *old_bridge_state) {struct ti_sn65dsi86 *pdata =bridge_to_ti_sn65dsi86 (bridge);
@@ -1055,7 +1056,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,return ret; } -static void ti_sn_bridge_enable (struct drm_bridge *bridge) +static void ti_sn_bridge_atomic_enable (struct drm_bridge *bridge, +struct drm_bridge_state *old_bridge_state) {struct ti_sn65dsi86 *pdata =bridge_to_ti_sn65dsi86 (bridge);const char *last_err_str ="No supported DP rate" ;
@@ -1124,7 +1126,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge) VSTREAM_ENABLE); } -static void ti_sn_bridge_pre_enable (struct drm_bridge *bridge) +static void ti_sn_bridge_atomic_pre_enable (struct drm_bridge *bridge, +struct drm_bridge_state *old_bridge_state) {struct ti_sn65dsi86 *pdata =bridge_to_ti_sn65dsi86 (bridge);
@@ -1137,7 +1140,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)usleep_range (100 ,110 ); } -static void ti_sn_bridge_post_disable (struct drm_bridge *bridge) +static void ti_sn_bridge_atomic_post_disable (struct drm_bridge *bridge, +struct drm_bridge_state *old_bridge_state) {struct ti_sn65dsi86 *pdata =bridge_to_ti_sn65dsi86 (bridge);
@@ -1158,10 +1162,11 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = { .attach = ti_sn_bridge_attach, .detach = ti_sn_bridge_detach, .mode_valid = ti_sn_bridge_mode_valid, - .pre_enable = ti_sn_bridge_pre_enable, - .enable = ti_sn_bridge_enable, - .disable = ti_sn_bridge_disable, - .post_disable = ti_sn_bridge_post_disable, + .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable, + .atomic_enable = ti_sn_atomic_bridge_enable, + .atomic_disable = ti_sn_atomic_bridge_disable, + .atomic_post_disable = ti_sn_bridge_post_disable, + DRM_BRIDGE_STATE_OPS, }; static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
Signature · 1 line
2.32.0
kernel test robot
build botRe: [PATCH v1 4/9] drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs
I love your patch! Yet something to improve:
Hi Sam,
I love your patch! Yet something to improve:
[auto build test ERROR on drm/drm-next]
[also build test ERROR on drm-intel/for-linux-next drm-tip/drm-tip drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next v5.17-rc2 next-20220204] [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/Sam-Ravnborg/drm-bridge-ps8640-and-ti-sn65dsi86-updates/20220206-234638 base: git://anongit.freedesktop.org/drm/drm drm-next config: hexagon-buildonly-randconfig-r003-20220206 (https://download.01.org/0day-ci/archive/20220207/202202070105.4pH2BXwR-lkp(a)intel.com/config) compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 6daaf5a44925592c764c59219b0024ee06317028)
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/b4a6b21edb289882a1f838a231032110dd0f3d1e
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Sam-Ravnborg/drm-bridge-ps8640-and-ti-sn65dsi86-updates/20220206-234638 git checkout b4a6b21edb289882a1f838a231032110dd0f3d1e
# 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=hexagon SHELL=/bin/bash drivers/gpu/drm/bridge/
If you fix the issue, kindly add following tag as appropriate
All errors (new ones prefixed by >>):
>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:1166:19: error: use of undeclared identifier 'ti_sn_atomic_bridge_enable'
.atomic_enable = ti_sn_atomic_bridge_enable,
^
>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:1167:20: error: use of undeclared identifier 'ti_sn_atomic_bridge_disable'
.atomic_disable = ti_sn_atomic_bridge_disable,
^
>> drivers/gpu/drm/bridge/ti-sn65dsi86.c:1168:25: error: use of undeclared identifier 'ti_sn_bridge_post_disable'; did you mean 'ti_sn_bridge_atomic_disable'?
.atomic_post_disable = ti_sn_bridge_post_disable,
^~~~~~~~~~~~~~~~~~~~~~~~~
ti_sn_bridge_atomic_disable
drivers/gpu/drm/bridge/ti-sn65dsi86.c:799:13: note: 'ti_sn_bridge_atomic_disable' declared here
static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
^3 errors generated.
vim +/ti_sn_atomic_bridge_enable +1166 drivers/gpu/drm/bridge/ti-sn65dsi86.c
1160
1161 static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
1162 .attach = ti_sn_bridge_attach,
1163 .detach = ti_sn_bridge_detach,
1164 .mode_valid = ti_sn_bridge_mode_valid,
1165 .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable,
> 1166 .atomic_enable = ti_sn_atomic_bridge_enable, > 1167 .atomic_disable = ti_sn_atomic_bridge_disable, > 1168 .atomic_post_disable = ti_sn_bridge_post_disable,
1169 DRM_BRIDGE_STATE_OPS,
1170 };
1171
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
Doug Anderson
reviewerRe: [PATCH v1 4/9] drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs
In reply to Sam RavnborgHi,
Quoted 62 lines · Sam Ravnborg
Move away from the deprecated enable/diable operations in
drm_bridge_funcsand enable atomic use.Signed-off-by Sam Ravnborg <[email protected]>Cc Douglas Anderson <[email protected]>Cc Andrzej Hajda <[email protected]>Cc Neil Armstrong <[email protected]>Cc Robert Foss <[email protected]>Cc Laurent Pinchart <[email protected]>Cc Jonas Karlman <[email protected]>Cc Jernej Skrabec <[email protected]>--- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)drivers/gpu/drm/bridge/ti-sn65dsi86.c +8
-4@@ -796,7 +796,8 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,return MODE_OK ; } -static void ti_sn_bridge_disable (struct drm_bridge *bridge) +static void ti_sn_bridge_atomic_disable (struct drm_bridge *bridge, +struct drm_bridge_state *old_bridge_state) {struct ti_sn65dsi86 *pdata =bridge_to_ti_sn65dsi86 (bridge);@@ -1055,7 +1056,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,return ret; } -static void ti_sn_bridge_enable (struct drm_bridge *bridge) +static void ti_sn_bridge_atomic_enable (struct drm_bridge *bridge, +struct drm_bridge_state *old_bridge_state) {struct ti_sn65dsi86 *pdata =bridge_to_ti_sn65dsi86 (bridge);const char *last_err_str ="No supported DP rate" ;@@ -1124,7 +1126,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge) VSTREAM_ENABLE); } -static void ti_sn_bridge_pre_enable (struct drm_bridge *bridge) +static void ti_sn_bridge_atomic_pre_enable (struct drm_bridge *bridge, +struct drm_bridge_state *old_bridge_state) {struct ti_sn65dsi86 *pdata =bridge_to_ti_sn65dsi86 (bridge);@@ -1137,7 +1140,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)usleep_range (100 ,110 ); } -static void ti_sn_bridge_post_disable (struct drm_bridge *bridge) +static void ti_sn_bridge_atomic_post_disable (struct drm_bridge *bridge, +struct drm_bridge_state *old_bridge_state) {struct ti_sn65dsi86 *pdata =bridge_to_ti_sn65dsi86 (bridge);@@ -1158,10 +1162,11 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = { .attach = ti_sn_bridge_attach,
drivers/gpu/drm/bridge/ti-sn65dsi86.c static const struct drm_bridge_funcs ti_sn_bridge_funcs = { · line 1168
Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c +4
-4.detach = ti_sn_bridge_detach, .mode_valid = ti_sn_bridge_mode_valid, - .pre_enable = ti_sn_bridge_pre_enable, - .enable = ti_sn_bridge_enable, - .disable = ti_sn_bridge_disable, - .post_disable = ti_sn_bridge_post_disable, + .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable, + .atomic_enable = ti_sn_atomic_bridge_enable, + .atomic_disable = ti_sn_atomic_bridge_disable, + .atomic_post_disable = ti_sn_bridge_post_disable,
Compiler doesn't like the fact that you are inconsistent about whether it's "atomic_bridge" or "bridge_atomic". Probably should settle on "bridge_atomic"? ...and the "post_disable" needs "atomic" in the name...
drivers/gpu/drm/bridge/ti-sn65dsi86.c static const struct drm_bridge_funcs ti_sn_bridge_funcs = { · line 1169
Quoted 1 line
drivers/gpu/drm/bridge/ti-sn65dsi86.c +1
-0+ DRM_BRIDGE_STATE_OPS,
Wow, is it really that simple? I guess it seems to work OK...
Since I don't actually know tons about atomic and whether this is enough, consider my Reviewed-by tag to be pretty weak. That being said, this _seems_ right to me?
So once it compiles then I'm fine w/ my (weak) Reviewed-by and my Tested-by.
-Doug
Re: [PATCH v1 4/9] drm/bridge: ti-sn65dsi86: Use atomic variants of drm_bridge_funcs
In reply to Sam RavnborgHi Sam,
Thank you for the patch.
Quoted 1 line · Sam Ravnborg
Move away from the deprecated enable/diable operations in
s/diable/disable/
Quoted 61 lines
drm_bridge_funcsand enable atomic use.Signed-off-by Sam Ravnborg <[email protected]>Cc Douglas Anderson <[email protected]>Cc Andrzej Hajda <[email protected]>Cc Neil Armstrong <[email protected]>Cc Robert Foss <[email protected]>Cc Laurent Pinchart <[email protected]>Cc Jonas Karlman <[email protected]>Cc Jernej Skrabec <[email protected]>--- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)drivers/gpu/drm/bridge/ti-sn65dsi86.c +8
-4@@ -796,7 +796,8 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge,return MODE_OK ; } -static void ti_sn_bridge_disable (struct drm_bridge *bridge) +static void ti_sn_bridge_atomic_disable (struct drm_bridge *bridge, +struct drm_bridge_state *old_bridge_state) {struct ti_sn65dsi86 *pdata =bridge_to_ti_sn65dsi86 (bridge);@@ -1055,7 +1056,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,return ret; } -static void ti_sn_bridge_enable (struct drm_bridge *bridge) +static void ti_sn_bridge_atomic_enable (struct drm_bridge *bridge, +struct drm_bridge_state *old_bridge_state) {struct ti_sn65dsi86 *pdata =bridge_to_ti_sn65dsi86 (bridge);const char *last_err_str ="No supported DP rate" ;@@ -1124,7 +1126,8 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge) VSTREAM_ENABLE); } -static void ti_sn_bridge_pre_enable (struct drm_bridge *bridge) +static void ti_sn_bridge_atomic_pre_enable (struct drm_bridge *bridge, +struct drm_bridge_state *old_bridge_state) {struct ti_sn65dsi86 *pdata =bridge_to_ti_sn65dsi86 (bridge);@@ -1137,7 +1140,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)usleep_range (100 ,110 ); } -static void ti_sn_bridge_post_disable (struct drm_bridge *bridge) +static void ti_sn_bridge_atomic_post_disable (struct drm_bridge *bridge, +struct drm_bridge_state *old_bridge_state) {struct ti_sn65dsi86 *pdata =bridge_to_ti_sn65dsi86 (bridge);@@ -1158,10 +1162,11 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = { .attach = ti_sn_bridge_attach, .detach = ti_sn_bridge_detach,
drivers/gpu/drm/bridge/ti-sn65dsi86.c static const struct drm_bridge_funcs ti_sn_bridge_funcs = { · line 1169
Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c +5
-4.mode_valid = ti_sn_bridge_mode_valid, - .pre_enable = ti_sn_bridge_pre_enable, - .enable = ti_sn_bridge_enable, - .disable = ti_sn_bridge_disable, - .post_disable = ti_sn_bridge_post_disable, + .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable, + .atomic_enable = ti_sn_atomic_bridge_enable, + .atomic_disable = ti_sn_atomic_bridge_disable, + .atomic_post_disable = ti_sn_bridge_post_disable, + DRM_BRIDGE_STATE_OPS,
With the compilation fix,
Quoted 3 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c +0
-0}; static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
Signature · 3 lines
Regards, Laurent Pinchart
PATCH 5/9 drm/bridge: ti-sn65dsi86: Fetch bpc via drm_bridge_state
In reply to Sam RavnborgTo prepare for DRM_BRIDGE_ATTACH_NO_CONNECTOR support, fix so the bpc is found using the output format.
This avoids the use of the connector stored in the private data.
--- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
drivers/gpu/drm/bridge/ti-sn65dsi86.c +10 -6
@@ -33,6 +33,7 @@#include <drm/drm_panel.h> #include <drm/drm_print.h> #include <drm/drm_probe_helper.h> +#include <drm/media-bus-format.h> #define SN_DEVICE_REV_REG 0x08#define SN_DPPLL_SRC_REG 0x0A
@@ -823,9 +824,11 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)regmap_write (pdata->regmap,SN_DSIA_CLK_FREQ_REG , val); } -static unsigned int ti_sn_bridge_get_bpp (struct ti_sn65dsi86 *pdata) +static unsigned int ti_sn_bridge_get_bpp (struct drm_bridge_state *bridge_state) { -if (pdata->connector.display_info.bpc <=6 ) +int bpc =media_bus_format_to_bpc (bridge_state->output_bus_cfg.format); + +if (bpc <=6 )return 18 ;else return 24 ;
@@ -840,7 +843,8 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = { 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400 }; -static int ti_sn_bridge_calc_min_dp_rate_idx (struct ti_sn65dsi86 *pdata) +static int ti_sn_bridge_calc_min_dp_rate_idx (struct ti_sn65dsi86 *pdata, +struct drm_bridge_state *bridge_state) {unsigned int bit_rate_khz, dp_rate_mhz;unsigned int i;
@@ -848,7 +852,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata) &pdata->bridge.encoder->crtc->state->adjusted_mode;/* Calculate minimum bit rate based on our pixel clock. */ - bit_rate_khz = mode->clock *ti_sn_bridge_get_bpp (pdata); + bit_rate_khz = mode->clock *ti_sn_bridge_get_bpp (bridge_state);/* Calculate minimum DP data rate, taking 80% as per DP spec */ dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM,
@@ -1092,7 +1096,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, DP_ALTERNATE_SCRAMBLER_RESET_ENABLE);/* Set the DP output format (18 bpp or 24 bpp) */ - val = (ti_sn_bridge_get_bpp (pdata) ==18 ) ?BPP_18_RGB :0 ; + val = (ti_sn_bridge_get_bpp (old_bridge_state) ==18 ) ?BPP_18_RGB :0 ;regmap_update_bits (pdata->regmap,SN_DATA_FORMAT_REG ,BPP_18_RGB , val);/* DP lane config */
@@ -1103,7 +1107,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, valid_rates =ti_sn_bridge_read_valid_rates (pdata);/* Train until we run out of rates */ -for (dp_rate_idx =ti_sn_bridge_calc_min_dp_rate_idx (pdata); +for (dp_rate_idx =ti_sn_bridge_calc_min_dp_rate_idx (pdata, old_bridge_state); dp_rate_idx <ARRAY_SIZE (ti_sn_bridge_dp_rate_lut); dp_rate_idx++) {if (!(valid_rates &BIT (dp_rate_idx)))
Signature · 1 line
2.32.0
Doug Anderson
reviewerRe: [PATCH v1 5/9] drm/bridge: ti-sn65dsi86: Fetch bpc via drm_bridge_state
Hi,
Quoted 34 lines · Sam Ravnborg
To prepare for
DRM_BRIDGE_ATTACH_NO_CONNECTORsupport, fix so the bpc is found using the output format.This avoids the use of the connector stored in the private data.
Signed-off-by Sam Ravnborg <[email protected]>Cc Douglas Anderson <[email protected]>Cc Andrzej Hajda <[email protected]>Cc Neil Armstrong <[email protected]>Cc Robert Foss <[email protected]>Cc Laurent Pinchart <[email protected]>Cc Jonas Karlman <[email protected]>Cc Jernej Skrabec <[email protected]>--- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)drivers/gpu/drm/bridge/ti-sn65dsi86.c +1
-0@@ -33,6 +33,7 @@#include <drm/drm_panel.h> #include <drm/drm_print.h> #include <drm/drm_probe_helper.h> +#include <drm/media-bus-format.h> #define SN_DEVICE_REV_REG 0x08#define SN_DPPLL_SRC_REG 0x0A@@ -823,9 +824,11 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)regmap_write (pdata->regmap,SN_DSIA_CLK_FREQ_REG , val); }
drivers/gpu/drm/bridge/ti-sn65dsi86.c static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata) · line 834
Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c +4
-2-static unsigned int ti_sn_bridge_get_bpp (struct ti_sn65dsi86 *pdata) +static unsigned int ti_sn_bridge_get_bpp (struct drm_bridge_state *bridge_state) { -if (pdata->connector.display_info.bpc <=6 ) +int bpc =media_bus_format_to_bpc (bridge_state->output_bus_cfg.format); + +if (bpc <=6 )return 18 ;else return 24 ;
Unfortunately this doesn't work as hoped. :( bridge_state->output_bus_cfg.format is 0 in my testing...
...and then media_bus_format_to_bpc() returns an error, which is negative and <= 6. That's not super ideal...
I guess this gets down to what the output bus format is _supposed_ to be for eDP. Based on my understanding of eDP there isn't really a concept of a fixed "bus format" that the panel ought to work at. There is a concept of the number of bits per component (6, 8, 10, 12) that a panel can run at but then after that I believe the format is standard, or at least it's something that's dynamic / negotiated. Also note that the format on the bus is more related to the current mode we're running the panel in than some inherent property of the panel. For instance, a 10 bpc panel can likely have data provided in 8 bpc and 6 bpc. I've also seen 6 bpc panels that can accept 8 bpc data and can dither it. In any case, this type of stuff is really all dynamic for eDP. The old value we were looking at was really being interpreted as the "max" bpc that the panel could run in and we'd choose to run the panel in 8 bpc if the panel supported it and 6 bpc otherwise (this bridge chip only supports 6bpc or 8bpc).
So I guess the question is: how do we move forward here?
Do we need to somehow figure out how to get "output_bus_cfg.format" filled in? Any suggestions for how to do that? Do we just implement atomic_get_output_bus_fmts() and then pick one of MEDIA_BUS_FMT_RGB666_1X18 or MEDIA_BUS_FMT_RBG888_1X24 based on the bpc in the connector state? ...or do we just list both of them and something magically will pick the right one? Hrm, I tried that and it didn't magically work, but I didn't debug further...
One thing I realized is that, at least in theory, we might not know whether we want to run in 6 bpc or 8 bpc until we've done link training. It's at least somewhat plausible that there could be edge cases where we'd want to fall back to the low bpc if link training failed at the higher bpc. The driver doesn't support that right now, but ideally the design wouldn't preclude it. At the moment link training happens in enable. Maybe that's a problem to worry about another day, though?
-Doug
Re: [PATCH v1 5/9] drm/bridge: ti-sn65dsi86: Fetch bpc via drm_bridge_state
Quoted 70 lines · Doug Anderson
Hi,
Quoted 34 lines · Sam Ravnborg
To prepare for
DRM_BRIDGE_ATTACH_NO_CONNECTORsupport, fix so the bpc is found using the output format.This avoids the use of the connector stored in the private data.
Signed-off-by Sam Ravnborg <[email protected]>Cc Douglas Anderson <[email protected]>Cc Andrzej Hajda <[email protected]>Cc Neil Armstrong <[email protected]>Cc Robert Foss <[email protected]>Cc Laurent Pinchart <[email protected]>Cc Jonas Karlman <[email protected]>Cc Jernej Skrabec <[email protected]>--- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)drivers/gpu/drm/bridge/ti-sn65dsi86.c +1
-0@@ -33,6 +33,7 @@#include <drm/drm_panel.h> #include <drm/drm_print.h> #include <drm/drm_probe_helper.h> +#include <drm/media-bus-format.h> #define SN_DEVICE_REV_REG 0x08#define SN_DPPLL_SRC_REG 0x0A@@ -823,9 +824,11 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)regmap_write (pdata->regmap,SN_DSIA_CLK_FREQ_REG , val); }drivers/gpu/drm/bridge/ti-sn65dsi86.c static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata) · line 834
Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c +4
-2-static unsigned int ti_sn_bridge_get_bpp (struct ti_sn65dsi86 *pdata) +static unsigned int ti_sn_bridge_get_bpp (struct drm_bridge_state *bridge_state) { -if (pdata->connector.display_info.bpc <=6 ) +int bpc =media_bus_format_to_bpc (bridge_state->output_bus_cfg.format); + +if (bpc <=6 )return 18 ;else return 24 ;Unfortunately this doesn't work as hoped. :(
bridge_state->output_bus_cfg.formatis 0 in my testing......and then
media_bus_format_to_bpc()returns an error, which is negative and <= 6. That's not super ideal...I guess this gets down to what the output bus format is _supposed_ to be for eDP. Based on my understanding of eDP there isn't really a concept of a fixed "bus format" that the panel ought to work at. There is a concept of the number of bits per component (6, 8, 10, 12) that a panel can run at but then after that I believe the format is standard, or at least it's something that's dynamic / negotiated. Also note that the format on the bus is more related to the current mode we're running the panel in than some inherent property of the panel. For instance, a 10 bpc panel can likely have data provided in 8 bpc and 6 bpc. I've also seen 6 bpc panels that can accept 8 bpc data and can dither it. In any case, this type of stuff is really all dynamic for eDP. The old value we were looking at was really being interpreted as the "max" bpc that the panel could run in and we'd choose to run the panel in 8 bpc if the panel supported it and 6 bpc otherwise (this bridge chip only supports 6bpc or 8bpc).
Quoted 1 line
So I guess the question is: how do we move forward here?
I skipped a patch to find the connector - will try to give that a spin again.
Thanks for the testing and the feedback!
Sam
Quoted 19 lines
Do we need to somehow figure out how to get "output_bus_cfg.format" filled in? Any suggestions for how to do that? Do we just implement
atomic_get_output_bus_fmts()and then pick one ofMEDIA_BUS_FMT_RGB666_1X18orMEDIA_BUS_FMT_RBG888_1X24based on the bpc in the connector state? ...or do we just list both of them and something magically will pick the right one? Hrm, I tried that and it didn't magically work, but I didn't debug further...One thing I realized is that, at least in theory, we might not know whether we want to run in 6 bpc or 8 bpc until we've done link training. It's at least somewhat plausible that there could be edge cases where we'd want to fall back to the low bpc if link training failed at the higher bpc. The driver doesn't support that right now, but ideally the design wouldn't preclude it. At the moment link training happens in enable. Maybe that's a problem to worry about another day, though?
-Doug
PATCH 6/9 drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support
In reply to Sam RavnborgFrom: Rob Clark <[email protected]>
Slightly awkward to fish out the display_info when we aren't creating own connector. But I don't see an obvious better way.
v3:
- Rebased and dropped the ti_sn_bridge_get_bpp() patch as this was solved in a different way (Sam)
v2:
- Remove error return with NO_CONNECTOR flag (Rob)
--- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)
drivers/gpu/drm/bridge/ti-sn65dsi86.c +9 -11
@@ -746,11 +746,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,struct ti_sn65dsi86 *pdata =bridge_to_ti_sn65dsi86 (bridge);int ret; -if (flags &DRM_BRIDGE_ATTACH_NO_CONNECTOR ) { -DRM_ERROR ("Fix bridge driver to make connector optional!" ); -return -EINVAL ; - } - pdata->aux.drm_dev = bridge->dev; ret =drm_dp_aux_register (&pdata->aux);if (ret <0 ) {
@@ -758,12 +753,14 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,return ret; } - ret =ti_sn_bridge_connector_init (pdata); -if (ret <0 ) - goto err_conn_init; +if (!(flags &DRM_BRIDGE_ATTACH_NO_CONNECTOR )) { + ret =ti_sn_bridge_connector_init (pdata); +if (ret <0 ) + goto err_conn_init; -/* We never want the next bridge to *also* create a connector: */ - flags |=DRM_BRIDGE_ATTACH_NO_CONNECTOR ; +/* We never want the next bridge to *also* create a connector: */ + flags |=DRM_BRIDGE_ATTACH_NO_CONNECTOR ; + }/* Attach the next bridge */ ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
@@ -774,7 +771,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,return 0 ; err_dsi_host: -drm_connector_cleanup (&pdata->connector); +if (!(flags &DRM_BRIDGE_ATTACH_NO_CONNECTOR )) +drm_connector_cleanup (&pdata->connector); err_conn_init:drm_dp_aux_unregister (&pdata->aux);return ret;
Signature · 1 line
2.32.0
Re: [PATCH v1 6/9] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support
Hi,
Quoted 23 lines · Sam Ravnborg
From: Rob Clark <[email protected]>
Slightly awkward to fish out the
display_infowhen we aren't creating own connector. But I don't see an obvious better way.v3:
- Rebased and dropped theti_sn_bridge_get_bpp()patch as this was solved in a different way (Sam)v2:
- Remove error return withNO_CONNECTORflag (Rob)Signed-off-by Rob Clark <[email protected]>Signed-off-by Sam Ravnborg <[email protected]>Cc Rob Clark <[email protected]>Cc Douglas Anderson <[email protected]>Cc Andrzej Hajda <[email protected]>Cc Neil Armstrong <[email protected]>Cc Robert Foss <[email protected]>Cc Laurent Pinchart <[email protected]>Cc Jonas Karlman <[email protected]>Cc Jernej Skrabec <[email protected]>
Quoted 3 lines
--- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)
This is fine by me assuming we can fix the previous patches.
NOTE: to me, this isn't something to do _instead_ of my patch [1] but _in addition_ to it. ${SUBJECT} patch transitions us to a more modern approach of having the connector created elsewhere but doesn't remove the old fallback code. Might as well clean the fallback code up unless you think it's going to simply be deleted right away or something?
[1] https://lore.kernel.org/r/20220204161245.v2.1.I3ab26b7f197cc56c874246a43e57913e9c2c1028@changeid
Re: [PATCH v1 6/9] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support
Hello,
Quoted 28 lines · Doug Anderson
Quoted 23 lines · Sam Ravnborg
From: Rob Clark <[email protected]>
Slightly awkward to fish out the
display_infowhen we aren't creating own connector. But I don't see an obvious better way.v3:
- Rebased and dropped theti_sn_bridge_get_bpp()patch as this was solved in a different way (Sam)v2:
- Remove error return withNO_CONNECTORflag (Rob)Signed-off-by Rob Clark <[email protected]>Signed-off-by Sam Ravnborg <[email protected]>Cc Rob Clark <[email protected]>Cc Douglas Anderson <[email protected]>Cc Andrzej Hajda <[email protected]>Cc Neil Armstrong <[email protected]>Cc Robert Foss <[email protected]>Cc Laurent Pinchart <[email protected]>Cc Jonas Karlman <[email protected]>Cc Jernej Skrabec <[email protected]>Quoted 3 lines
--- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)
Quoted 3 lines
This is fine by me assuming we can fix the previous patches.
Reviewed-by Douglas Anderson <[email protected]>
Likewise,
Quoted 5 lines
NOTE: to me, this isn't something to do _instead_ of my patch [1] but
_inaddition_to it. ${SUBJECT} patch transitions us to a more modern approach of having the connector created elsewhere but doesn't remove the old fallback code. Might as well clean the fallback code up unless you think it's going to simply be deleted right away or something?
I don't really mind either way, but I of course would favour removal of connector support as soon as practical :-)
Quoted 1 line
[1] https://lore.kernel.org/r/20220204161245.v2.1.I3ab26b7f197cc56c874246a43e57913e9c2c1028@changeid
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v1 6/9] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support
In reply to Doug AndersonHi Douglas,
Quoted 32 lines · Doug Anderson
Hi,
Quoted 23 lines · Sam Ravnborg
From: Rob Clark <[email protected]>
Slightly awkward to fish out the
display_infowhen we aren't creating own connector. But I don't see an obvious better way.v3:
- Rebased and dropped theti_sn_bridge_get_bpp()patch as this was solved in a different way (Sam)v2:
- Remove error return withNO_CONNECTORflag (Rob)Signed-off-by Rob Clark <[email protected]>Signed-off-by Sam Ravnborg <[email protected]>Cc Rob Clark <[email protected]>Cc Douglas Anderson <[email protected]>Cc Andrzej Hajda <[email protected]>Cc Neil Armstrong <[email protected]>Cc Robert Foss <[email protected]>Cc Laurent Pinchart <[email protected]>Cc Jonas Karlman <[email protected]>Cc Jernej Skrabec <[email protected]>Quoted 3 lines
--- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)This is fine by me assuming we can fix the previous patches.
Quoted 7 lines
Reviewed-by Douglas Anderson <[email protected]>NOTE: to me, this isn't something to do _instead_ of my patch [1] but
_inaddition_to it. ${SUBJECT} patch transitions us to a more modern approach of having the connector created elsewhere but doesn't remove the old fallback code. Might as well clean the fallback code up unless you think it's going to simply be deleted right away or something?
Until we know all users have NO_CONNECTOR support we need the connector creation code.
Please just go ahead with your patches as you already got r-b from Laurent. For the debugfs patch I will look at it soonish unless someone beats me.
Sam
Re: [PATCH v1 6/9] drm/bridge: ti-sn65dsi86: Add NO_CONNECTOR support
In reply to Sam RavnborgHi Sam,
Quoting Sam Ravnborg (2022-02-06 15:44:02)
Quoted 63 lines
From: Rob Clark <[email protected]>
Slightly awkward to fish out the
display_infowhen we aren't creating own connector. But I don't see an obvious better way.v3:
- Rebased and dropped theti_sn_bridge_get_bpp()patch as this was solved in a different way (Sam)v2:
- Remove error return withNO_CONNECTORflag (Rob)Signed-off-by Rob Clark <[email protected]>Signed-off-by Sam Ravnborg <[email protected]>Cc Rob Clark <[email protected]>Cc Douglas Anderson <[email protected]>Cc Andrzej Hajda <[email protected]>Cc Neil Armstrong <[email protected]>Cc Robert Foss <[email protected]>Cc Laurent Pinchart <[email protected]>Cc Jonas Karlman <[email protected]>Cc Jernej Skrabec <[email protected]>--- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)drivers/gpu/drm/bridge/ti-sn65dsi86.c +7
-10@@ -746,11 +746,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,struct ti_sn65dsi86 *pdata =bridge_to_ti_sn65dsi86 (bridge);int ret; -if (flags &DRM_BRIDGE_ATTACH_NO_CONNECTOR ) { -DRM_ERROR ("Fix bridge driver to make connector optional!" ); -return -EINVAL ; - } - pdata->aux.drm_dev = bridge->dev; ret =drm_dp_aux_register (&pdata->aux);if (ret <0 ) {@@ -758,12 +753,14 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,return ret; } - ret =ti_sn_bridge_connector_init (pdata); -if (ret <0 ) - goto err_conn_init; +if (!(flags &DRM_BRIDGE_ATTACH_NO_CONNECTOR )) { + ret =ti_sn_bridge_connector_init (pdata); +if (ret <0 ) + goto err_conn_init; -/* We never want the next bridge to *also* create a connector: */ - flags |=DRM_BRIDGE_ATTACH_NO_CONNECTOR ; +/* We never want the next bridge to *also* create a connector: */ + flags |=DRM_BRIDGE_ATTACH_NO_CONNECTOR ; + }/* Attach the next bridge */ ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
drivers/gpu/drm/bridge/ti-sn65dsi86.c static int ti_sn_bridge_attach(struct drm_bridge *bridge, · line 774
Quoted 5 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c +1
-1@@ -774,7 +771,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,return 0 ; err_dsi_host: -drm_connector_cleanup (&pdata->connector); +if (!(flags &DRM_BRIDGE_ATTACH_NO_CONNECTOR ))
I think this is unreachable / always false due to the
flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR.
I've solved this locally by doing:
- /* Attach the next bridge */
+ /*
+ * Attach the next bridge We never want the next bridge to *also* create
+ * a connector:
+ */
ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
- &pdata->bridge, flags);
+ &pdata->bridge,
+ flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
if (ret < 0)
goto err_initted_aux;
+ if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
+ return 0;
+
pdata->connector = drm_bridge_connector_init(pdata->bridge.dev,
pdata->bridge.encoder);
if (IS_ERR(pdata->connector)) {
ret = PTR_ERR(pdata->connector);
goto err_initted_aux;
}
drm_connector_attach_encoder(pdata->connector, pdata->bridge.encoder);
return 0;
Which also fixes the support for
flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR
--
Regards
Kieran
Quoted 7 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c +1
-0+drm_connector_cleanup (&pdata->connector); err_conn_init:drm_dp_aux_unregister (&pdata->aux);return ret;Signature · 1 line
2.32.0
PATCH 7/9 drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs
In reply to Sam RavnborgThe atomic variants of enable/disable in drm_bridge_funcs are the preferred operations - introduce these.
The ps8640 driver used the non-atomic variants of the drm_bridge_chain_pre_enable/ drm_bridge_chain_post_disable - convert these to the atomic variants.
v4:
- Rebased
v3:
- Init state operations in drm_bridge_funcs (Laurent)
v2:
- Added a few more people to cc: (Jitao, Enric, Philip) to increase possibility to get test feedback
--- drivers/gpu/drm/bridge/parade-ps8640.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
drivers/gpu/drm/bridge/parade-ps8640.c +10 -6
@@ -13,6 +13,7 @@#include <linux/regmap.h> #include <linux/regulator/consumer.h> +#include <drm/drm_atomic_state_helper.h> #include <drm/drm_bridge.h> #include <drm/dp/drm_dp_aux_bus.h> #include <drm/dp/drm_dp_helper.h>
@@ -398,7 +399,8 @@ static const struct dev_pm_ops ps8640_pm_ops = { pm_runtime_force_resume) }; -static void ps8640_pre_enable (struct drm_bridge *bridge) +static void ps8640_atomic_pre_enable (struct drm_bridge *bridge, +struct drm_bridge_state *old_bridge_state) {struct ps8640 *ps_bridge =bridge_to_ps8640 (bridge);struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL ];
@@ -430,7 +432,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge) ps_bridge->pre_enabled = true; } -static void ps8640_post_disable (struct drm_bridge *bridge) +static void ps8640_atomic_post_disable (struct drm_bridge *bridge, +struct drm_bridge_state *old_bridge_state) {struct ps8640 *ps_bridge =bridge_to_ps8640 (bridge);
@@ -508,7 +511,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,* EDID, for this chip, we need to do a full poweron, otherwise it will * fail. */ -drm_bridge_chain_pre_enable (bridge); +drm_atomic_bridge_chain_pre_enable (bridge, connector->state->state); edid =drm_get_edid (connector, ps_bridge->page[PAGE0_DP_CNTL ]->adapter);
@@ -518,7 +521,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,* before, return the chip to its original power state. */ if (poweroff) -drm_bridge_chain_post_disable (bridge); +drm_atomic_bridge_chain_post_disable (bridge, connector->state->state);return edid; }
@@ -533,8 +536,9 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = { .attach = ps8640_bridge_attach, .detach = ps8640_bridge_detach, .get_edid = ps8640_bridge_get_edid, - .post_disable = ps8640_post_disable, - .pre_enable = ps8640_pre_enable, + .atomic_post_disable = ps8640_atomic_post_disable, + .atomic_pre_enable = ps8640_atomic_pre_enable, + DRM_BRIDGE_STATE_OPS, };static int ps8640_bridge_host_attach (struct device *dev,struct ps8640 *ps_bridge)
Signature · 1 line
2.32.0
PATCH 8/9 drm/bridge: ps8640: plug atomic_get_input_bus_fmts
In reply to Sam RavnborgUse the helper drm_atomic_helper_bridge_dsi_input_bus_fmt to announce the DSI media bus format.
--- drivers/gpu/drm/bridge/parade-ps8640.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/gpu/drm/bridge/parade-ps8640.c +2 -0
@@ -13,6 +13,7 @@#include <linux/regmap.h> #include <linux/regulator/consumer.h> +#include <drm/drm_atomic_helper.h> #include <drm/drm_atomic_state_helper.h> #include <drm/drm_bridge.h> #include <drm/dp/drm_dp_aux_bus.h>
@@ -538,6 +539,7 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = { .get_edid = ps8640_bridge_get_edid, .atomic_post_disable = ps8640_atomic_post_disable, .atomic_pre_enable = ps8640_atomic_pre_enable, + .atomic_get_input_bus_fmts = drm_atomic_helper_bridge_dsi_input_bus_fmt, DRM_BRIDGE_STATE_OPS, };
Signature · 1 line
2.32.0
PATCH 9/9 drm/bridge: Drop unused drm_bridge_chain functions
In reply to Sam RavnborgThe drm_bridge_chain_{pre_enable,enable,disable,post_disable} has no users left and we have atomic variants that should be used. Drop them so they do not gain new users.
Adjust a few comments to avoid references to the dropped functions.
--- drivers/gpu/drm/drm_bridge.c | 110 ----------------------------------- include/drm/drm_bridge.h | 28 --------- 2 files changed, 138 deletions(-)
drivers/gpu/drm/drm_bridge.c +0 -110
@@ -485,61 +485,6 @@ drm_bridge_chain_mode_valid(struct drm_bridge *bridge, }EXPORT_SYMBOL (drm_bridge_chain_mode_valid); -/** -* drm_bridge_chain_disable - disables all bridges in the encoder chain -* @bridge: bridge control structure -* -* Calls &drm_bridge_funcs.disable op for all the bridges in the encoder -* chain, starting from the last bridge to the first. These are called before -* calling the encoder's prepare op. -* -* Note: the bridge passed should be the one closest to the encoder -*/ -void drm_bridge_chain_disable (struct drm_bridge *bridge) -{ -struct drm_encoder *encoder; -struct drm_bridge *iter; - -if (!bridge) -return ; - - encoder = bridge->encoder; -list_for_each_entry_reverse (iter, &encoder->bridge_chain, chain_node) { -if (iter->funcs->disable) - iter->funcs->disable (iter); - -if (iter == bridge) -break ; - } -} -EXPORT_SYMBOL (drm_bridge_chain_disable); - -/** -* drm_bridge_chain_post_disable - cleans up after disabling all bridges in the -* encoder chain -* @bridge: bridge control structure -* -* Calls &drm_bridge_funcs.post_disable op for all the bridges in the -* encoder chain, starting from the first bridge to the last. These are called -* after completing the encoder's prepare op. -* -* Note: the bridge passed should be the one closest to the encoder -*/ -void drm_bridge_chain_post_disable (struct drm_bridge *bridge) -{ -struct drm_encoder *encoder; - -if (!bridge) -return ; - - encoder = bridge->encoder; -list_for_each_entry_from (bridge, &encoder->bridge_chain, chain_node) { -if (bridge->funcs->post_disable) - bridge->funcs->post_disable (bridge); - } -} -EXPORT_SYMBOL (drm_bridge_chain_post_disable); -/** * drm_bridge_chain_mode_set - set proposed mode for all bridges in the * encoder chain
@@ -569,61 +514,6 @@ void drm_bridge_chain_mode_set(struct drm_bridge *bridge, }EXPORT_SYMBOL (drm_bridge_chain_mode_set); -/** -* drm_bridge_chain_pre_enable - prepares for enabling all bridges in the -* encoder chain -* @bridge: bridge control structure -* -* Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder -* chain, starting from the last bridge to the first. These are called -* before calling the encoder's commit op. -* -* Note: the bridge passed should be the one closest to the encoder -*/ -void drm_bridge_chain_pre_enable (struct drm_bridge *bridge) -{ -struct drm_encoder *encoder; -struct drm_bridge *iter; - -if (!bridge) -return ; - - encoder = bridge->encoder; -list_for_each_entry_reverse (iter, &encoder->bridge_chain, chain_node) { -if (iter->funcs->pre_enable) - iter->funcs->pre_enable (iter); - -if (iter == bridge) -break ; - } -} -EXPORT_SYMBOL (drm_bridge_chain_pre_enable); - -/** -* drm_bridge_chain_enable - enables all bridges in the encoder chain -* @bridge: bridge control structure -* -* Calls &drm_bridge_funcs.enable op for all the bridges in the encoder -* chain, starting from the first bridge to the last. These are called -* after completing the encoder's commit op. -* -* Note that the bridge passed should be the one closest to the encoder -*/ -void drm_bridge_chain_enable (struct drm_bridge *bridge) -{ -struct drm_encoder *encoder; - -if (!bridge) -return ; - - encoder = bridge->encoder; -list_for_each_entry_from (bridge, &encoder->bridge_chain, chain_node) { -if (bridge->funcs->enable) - bridge->funcs->enable (bridge); - } -} -EXPORT_SYMBOL (drm_bridge_chain_enable); -/** * drm_atomic_bridge_chain_disable - disables all bridges in the encoder chain * @bridge: bridge control structure
include/drm/drm_bridge.h +0 -28
@@ -297,12 +297,6 @@ struct drm_bridge_funcs {* not enable the display link feeding the next bridge in the chain (if * there is one) when this callback is called. * -* Note that this function will only be invoked in the context of an -* atomic commit. It will not be invoked from -* &drm_bridge_chain_pre_enable. It would be prudent to also provide an -* implementation of @pre_enable if you are expecting driver calls into -* &drm_bridge_chain_pre_enable. -* * The @atomic_pre_enable callback is optional. */ void (*atomic_pre_enable)(struct drm_bridge *bridge,
@@ -323,11 +317,6 @@ struct drm_bridge_funcs {* callback must enable the display link feeding the next bridge in the * chain if there is one. * -* Note that this function will only be invoked in the context of an -* atomic commit. It will not be invoked from &drm_bridge_chain_enable. -* It would be prudent to also provide an implementation of @enable if -* you are expecting driver calls into &drm_bridge_chain_enable. -* * The @atomic_enable callback is optional. */ void (*atomic_enable)(struct drm_bridge *bridge,
@@ -345,12 +334,6 @@ struct drm_bridge_funcs {* The bridge can assume that the display pipe (i.e. clocks and timing * signals) feeding it is still running when this callback is called. * -* Note that this function will only be invoked in the context of an -* atomic commit. It will not be invoked from -* &drm_bridge_chain_disable. It would be prudent to also provide an -* implementation of @disable if you are expecting driver calls into -* &drm_bridge_chain_disable. -* * The @atomic_disable callback is optional. */ void (*atomic_disable)(struct drm_bridge *bridge,
@@ -370,13 +353,6 @@ struct drm_bridge_funcs {* signals) feeding it is no longer running when this callback is * called. * -* Note that this function will only be invoked in the context of an -* atomic commit. It will not be invoked from -* &drm_bridge_chain_post_disable. -* It would be prudent to also provide an implementation of -* @post_disable if you are expecting driver calls into -* &drm_bridge_chain_post_disable. -* * The @atomic_post_disable callback is optional. */ void (*atomic_post_disable)(struct drm_bridge *bridge,
@@ -868,13 +844,9 @@ enum drm_mode_status drm_bridge_chain_mode_valid(struct drm_bridge *bridge, const struct drm_display_info *info, const struct drm_display_mode *mode); -void drm_bridge_chain_disable (struct drm_bridge *bridge); -void drm_bridge_chain_post_disable (struct drm_bridge *bridge);void drm_bridge_chain_mode_set (struct drm_bridge *bridge,const struct drm_display_mode *mode,const struct drm_display_mode *adjusted_mode); -void drm_bridge_chain_pre_enable (struct drm_bridge *bridge); -void drm_bridge_chain_enable (struct drm_bridge *bridge); int drm_atomic_bridge_chain_check(struct drm_bridge *bridge, struct drm_crtc_state *crtc_state,
Signature · 1 line
2.32.0
Re: [PATCH v1 9/9] drm/bridge: Drop unused drm_bridge_chain functions
Hi Sam,
Thank you for the patch.
Quoted 8 lines · Sam Ravnborg
The drm_bridge_chain_{pre_enable,enable,disable,post_disable} has no users left and we have atomic variants that should be used. Drop them so they do not gain new users.
Adjust a few comments to avoid references to the dropped functions.
Signed-off-by Sam Ravnborg <[email protected]>Reviewed-by Maxime Ripard <[email protected]>
Quoted 211 lines
Cc Laurent Pinchart <[email protected]>Cc Maarten Lankhorst <[email protected]>Cc Maxime Ripard <[email protected]>Cc Thomas Zimmermann <[email protected]>Cc Andrzej Hajda <[email protected]>Cc Neil Armstrong <[email protected]>Cc Robert Foss <[email protected]>Cc Daniel Vetter <[email protected]>--- drivers/gpu/drm/drm_bridge.c | 110 ----------------------------------- include/drm/drm_bridge.h | 28 --------- 2 files changed, 138 deletions(-)drivers/gpu/drm/drm_bridge.c +0
-110@@ -485,61 +485,6 @@ drm_bridge_chain_mode_valid(struct drm_bridge *bridge, }EXPORT_SYMBOL (drm_bridge_chain_mode_valid); -/** -* drm_bridge_chain_disable - disables all bridges in the encoder chain -* @bridge: bridge control structure -* -* Calls &drm_bridge_funcs.disable op for all the bridges in the encoder -* chain, starting from the last bridge to the first. These are called before -* calling the encoder's prepare op. -* -* Note: the bridge passed should be the one closest to the encoder -*/ -void drm_bridge_chain_disable (struct drm_bridge *bridge) -{ -struct drm_encoder *encoder; -struct drm_bridge *iter; - -if (!bridge) -return ; - - encoder = bridge->encoder; -list_for_each_entry_reverse (iter, &encoder->bridge_chain, chain_node) { -if (iter->funcs->disable) - iter->funcs->disable (iter); - -if (iter == bridge) -break ; - } -} -EXPORT_SYMBOL (drm_bridge_chain_disable); - -/** -* drm_bridge_chain_post_disable - cleans up after disabling all bridges in the -* encoder chain -* @bridge: bridge control structure -* -* Calls &drm_bridge_funcs.post_disable op for all the bridges in the -* encoder chain, starting from the first bridge to the last. These are called -* after completing the encoder's prepare op. -* -* Note: the bridge passed should be the one closest to the encoder -*/ -void drm_bridge_chain_post_disable (struct drm_bridge *bridge) -{ -struct drm_encoder *encoder; - -if (!bridge) -return ; - - encoder = bridge->encoder; -list_for_each_entry_from (bridge, &encoder->bridge_chain, chain_node) { -if (bridge->funcs->post_disable) - bridge->funcs->post_disable (bridge); - } -} -EXPORT_SYMBOL (drm_bridge_chain_post_disable); -/** * drm_bridge_chain_mode_set - set proposed mode for all bridges in the * encoder chain @@ -569,61 +514,6 @@ void drm_bridge_chain_mode_set(struct drm_bridge *bridge, }EXPORT_SYMBOL (drm_bridge_chain_mode_set); -/** -* drm_bridge_chain_pre_enable - prepares for enabling all bridges in the -* encoder chain -* @bridge: bridge control structure -* -* Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder -* chain, starting from the last bridge to the first. These are called -* before calling the encoder's commit op. -* -* Note: the bridge passed should be the one closest to the encoder -*/ -void drm_bridge_chain_pre_enable (struct drm_bridge *bridge) -{ -struct drm_encoder *encoder; -struct drm_bridge *iter; - -if (!bridge) -return ; - - encoder = bridge->encoder; -list_for_each_entry_reverse (iter, &encoder->bridge_chain, chain_node) { -if (iter->funcs->pre_enable) - iter->funcs->pre_enable (iter); - -if (iter == bridge) -break ; - } -} -EXPORT_SYMBOL (drm_bridge_chain_pre_enable); - -/** -* drm_bridge_chain_enable - enables all bridges in the encoder chain -* @bridge: bridge control structure -* -* Calls &drm_bridge_funcs.enable op for all the bridges in the encoder -* chain, starting from the first bridge to the last. These are called -* after completing the encoder's commit op. -* -* Note that the bridge passed should be the one closest to the encoder -*/ -void drm_bridge_chain_enable (struct drm_bridge *bridge) -{ -struct drm_encoder *encoder; - -if (!bridge) -return ; - - encoder = bridge->encoder; -list_for_each_entry_from (bridge, &encoder->bridge_chain, chain_node) { -if (bridge->funcs->enable) - bridge->funcs->enable (bridge); - } -} -EXPORT_SYMBOL (drm_bridge_chain_enable); -/** * drm_atomic_bridge_chain_disable - disables all bridges in the encoder chain * @bridge: bridge control structure include/drm/drm_bridge.h +0
-28@@ -297,12 +297,6 @@ struct drm_bridge_funcs {* not enable the display link feeding the next bridge in the chain (if * there is one) when this callback is called. * -* Note that this function will only be invoked in the context of an -* atomic commit. It will not be invoked from -* &drm_bridge_chain_pre_enable. It would be prudent to also provide an -* implementation of @pre_enable if you are expecting driver calls into -* &drm_bridge_chain_pre_enable. -* * The @atomic_pre_enable callback is optional. */ void (*atomic_pre_enable)(struct drm_bridge *bridge,@@ -323,11 +317,6 @@ struct drm_bridge_funcs {* callback must enable the display link feeding the next bridge in the * chain if there is one. * -* Note that this function will only be invoked in the context of an -* atomic commit. It will not be invoked from &drm_bridge_chain_enable. -* It would be prudent to also provide an implementation of @enable if -* you are expecting driver calls into &drm_bridge_chain_enable. -* * The @atomic_enable callback is optional. */ void (*atomic_enable)(struct drm_bridge *bridge,@@ -345,12 +334,6 @@ struct drm_bridge_funcs {* The bridge can assume that the display pipe (i.e. clocks and timing * signals) feeding it is still running when this callback is called. * -* Note that this function will only be invoked in the context of an -* atomic commit. It will not be invoked from -* &drm_bridge_chain_disable. It would be prudent to also provide an -* implementation of @disable if you are expecting driver calls into -* &drm_bridge_chain_disable. -* * The @atomic_disable callback is optional. */ void (*atomic_disable)(struct drm_bridge *bridge,@@ -370,13 +353,6 @@ struct drm_bridge_funcs {* signals) feeding it is no longer running when this callback is * called. * -* Note that this function will only be invoked in the context of an -* atomic commit. It will not be invoked from -* &drm_bridge_chain_post_disable. -* It would be prudent to also provide an implementation of -* @post_disable if you are expecting driver calls into -* &drm_bridge_chain_post_disable. -* * The @atomic_post_disable callback is optional. */ void (*atomic_post_disable)(struct drm_bridge *bridge,@@ -868,13 +844,9 @@ enum drm_mode_status drm_bridge_chain_mode_valid(struct drm_bridge *bridge, const struct drm_display_info *info, const struct drm_display_mode *mode); -void drm_bridge_chain_disable (struct drm_bridge *bridge); -void drm_bridge_chain_post_disable (struct drm_bridge *bridge);void drm_bridge_chain_mode_set (struct drm_bridge *bridge,const struct drm_display_mode *mode,const struct drm_display_mode *adjusted_mode); -void drm_bridge_chain_pre_enable (struct drm_bridge *bridge); -void drm_bridge_chain_enable (struct drm_bridge *bridge); int drm_atomic_bridge_chain_check(struct drm_bridge *bridge, struct drm_crtc_state *crtc_state,
Signature · 3 lines
Regards, Laurent Pinchart
Quoted 2 lines
The code builds - but needs testing.
Hrmff, no it does not build. The fixes was by accident not included. Will wait a bit for feedback before posting a v2.
Sam
Hi Sam,
Quoting Sam Ravnborg (2022-02-06 19:09:11)
Quoted 7 lines
Quoted 2 lines
The code builds - but needs testing.
Hrmff, no it does not build. The fixes was by accident not included. Will wait a bit for feedback before posting a v2.
Sam
Do you have any plan to send a v2 on this series?
I have built up a series to extend the ti-sn65dsi86 which is now based on this. (which means I'll have an implied Tested-by: tag for these as well).
--
Kieran
Hi Kieran,
Quoted 9 lines · Kieran Bingham
Hi Sam,
Quoting Sam Ravnborg (2022-02-06 19:09:11)
Quoted 6 lines
Quoted 2 lines
The code builds - but needs testing.
Hrmff, no it does not build. The fixes was by accident not included. Will wait a bit for feedback before posting a v2.
Quoted 7 lines
Quoted 1 line
Sam
Do you have any plan to send a v2 on this series?
I have built up a series to extend the ti-sn65dsi86 which is now based on this. (which means I'll have an implied Tested-by: tag for these as well).
That is too good not to do something about it. I will give it a spin this weekend - I do not have time until then.
Sam